Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Coming from #1932652: Add image uploading to WYSIWYGs through editor.module.
I'm running Acquia DevDesktop which installs Apache on a port, so my site URL is e.g. http://8x.localhost:8082/.
Something in FilterHtmlImageSecure must be messing up the path parsing because at this point:
// Verify that $src starts with $base_path.
// This also ensures that external images cannot be referenced.
$src = $image->getAttribute('src');
If I var_dump($src) I get ":8082/sites/default/files/inline-images/BQWQqhZCEAECbbZ.png"
This obviously doesn't match the base path, so the image gets flagged as insecure.
Comment | File | Size | Author |
---|---|---|---|
#33 | 2076847-33-local-image-filter.patch | 2.31 KB | mr.baileys |
#31 | image-filter-2076847-31.patch | 2.8 KB | zero2one |
#29 | HtmlImageSecureTest.txt | 10.05 KB | zero2one |
#27 | image-filter-2076847-27.patch | 2.37 KB | zero2one |
#20 | image-filter-2076847-20.patch | 1.66 KB | zero2one |
Comments
Comment #1
Wim LeersI've wanted to report this for a long time, but kept forgetting.
Should be a pretty easy fix :)
Comment #2
larowlanWe have php unit for image class so adding the test should be easy too
Will tackle shortly
Comment #3
larowlanWow filter module is in need of some love, such a mis-mash of OO/procedural
Comment #5
larowlanSubdirectory issues
Comment #6
larowlanIn a sub-dir the :80 goes before the base_path() :)
Comment #7
larowlanGreen this time :)
Comment #8
hass CreditAttribution: hass commentedI would have expected drupal_parse_url or php parse url for such a task...
Comment #9
Wim LeersI'm afraid the test you added fails on instances running on anything else than port 80 :)
With
http://unus:8082
as the$http_base_url
, the code you added would generate something like this:http::8082/:8082/unus:8082/core/misc/druplicon.png
. :) Doesn't seem like that could ever work, could it? :DI've tried to come up with an alternative. It works locally (on my port 8082 install), so if this comes back green, it should work in all cases since AFAIK testbot runs on port 80 by default. Nevertheless, somebody with a port 80 install should also run this test locally, just to make sure.
Comment #11
tstoecklerYes, I agree with #8 and the tests in #9 seem to prove that parse_url() is a much more reliable way of handling this than regex-ing the hell out of this.
I think something like
should be fine.
Comment #12
webchickThat sounds like "needs work."
Thanks for the quick turnaround on this though! I can't wait to see images again. :D
Comment #13
larowlanFrom the php manual for parse_url():
So parse_url() is out.
This should remove the hard-coded 80 and use the port the test is running on.
Interdiff is against the patch at #6
Comment #15
hass CreditAttribution: hass commenteddrupal_parse_url() works well with internal urls and the new function https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util..., too.
Comment #16
webchickNo longer applies. :(
Comment #17
socketwench CreditAttribution: socketwench commentedReroll!
Comment #19
hass CreditAttribution: hass commentedWhat about #15?
Comment #20
zero2one CreditAttribution: zero2one commentedI fixed the test (fatal PHP error + errors in the test path)
Comment #21
zero2one CreditAttribution: zero2one commentedBTW: this issue is related to https://drupal.org/node/2099205
Comment #23
zero2one CreditAttribution: zero2one commentedDamn :(
I need some help here:
I'm guessing that the test bot does not have a request port number?
Should I add a verbose message so we can see what the test is inserting for the Image URL(s) so we can debug what kind of URL is generated trough the testbot?
Comment #24
hass CreditAttribution: hass commentedSee #15. don't attach another patch with regexes, please.
Comment #25
mr.baileys@hass: drupal_parse_url() (deprecated) and Url::parse() both explicitly warn that…
The URLs in this case are not generated by the system, and might come from/link to external resources, so it seems that drupal_parse_url() and Url::parse() are ruled out for this particular issue.
Comment #26
hass CreditAttribution: hass commentedI do not think so or we need to fix url::parse() to work reliable if it does not. I'm have no idea when this function may fail. We see here that we need parse_url() from PHP as the regexes prove to fail. Aside of this this are local/internal urls to images.
Comment #27
zero2one CreditAttribution: zero2one commentedWe wanted to be to good :-)
If the domain contains a port we want the strip that anyway as long as the host (domain) equals the domain the site is hosted on.
There is a test afterwards to see if the image exists within the local filesystem.
So my new fix: remove the host + any optional port the image src has (preg_replace).
I added to the test 4 test URI's with different port number, they should all be removed.
BTW: I moved the code to create the replace pattern outside the foreach loop, that needs to be created only once.
Comment #29
zero2one CreditAttribution: zero2one commentedI don't get it.
The test runs fine on my machine (both UI and CLI)
See attachment.
Comment #30
mr.baileys$base_url
can contain a subdirectory (http://example.com/drupal), in which case adding the port to$http_base_url
will not work (http://example.com/drupal:8080…). The testbot on Drupal.org installs the site being tested in a subdirectory.Try running the tests locally with Drupal installed in a subdirectory, they should fail on your end too.
Comment #31
zero2one CreditAttribution: zero2one commentedOff course (missed that one).
Hereby the new patch with fixed test URL's.
Tested with and without subdirectory, tested with and without domain with a port number.
Comment #32
zero2one CreditAttribution: zero2one commentedFixed issue status.
Comment #33
mr.baileysLooking at the history of that code, it seems that the bug was introduced in #1999376: Use Symfony Request for filter module
$_SERVER['HTTP_HOST'] contains the hostname + port (if non-standard), while HttpRequest::getHost() never contains the port number.
Simpler patch attached that uses HttpRequest::getHttpHost() instead of HttpRequest::getHost(), which does include non-standard ports.
I left the tests as is, they might require some further clean-up.
Comment #34
Wim LeersLooks great! Great work, zero2one & mr.baileys! Go team Belgium! :D
Comment #35
webchickYayyyyy! I have images again!! :D
Committed and pushed to 8.x. Thanks SO much folks!!