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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Issue tags: +Needs tests

I've wanted to report this for a long time, but kept forgetting.

Should be a pretty easy fix :)

larowlan’s picture

Assigned: Unassigned » larowlan

We have php unit for image class so adding the test should be easy too
Will tackle shortly

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
1.57 KB
809 bytes

Wow filter module is in need of some love, such a mis-mash of OO/procedural

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847.pass_.patch, failed testing.

larowlan’s picture

Subdirectory issues

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
971 bytes

In a sub-dir the :80 goes before the base_path() :)

larowlan’s picture

Green this time :)

hass’s picture

I would have expected drupal_parse_url or php parse url for such a task...

Wim Leers’s picture

Issue tags: -Needs tests
FileSize
1.78 KB
2.48 KB

I'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? :D

I'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.

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847.9.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review

Yes, 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

$url_parts = parse_url($src);
$path = $url_parts['path'];

should be fine.

webchick’s picture

Status: Needs review » Needs work

That sounds like "needs work."

Thanks for the quick turnaround on this though! I can't wait to see images again. :D

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
1 KB

From the php manual for parse_url():

Note:
This function doesn't work with relative URLs.

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

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847.13.patch, failed testing.

hass’s picture

drupal_parse_url() works well with internal urls and the new function https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util..., too.

webchick’s picture

No longer applies. :(

socketwench’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Reroll!

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847.17.patch, failed testing.

hass’s picture

What about #15?

zero2one’s picture

Status: Needs work » Needs review
FileSize
1.66 KB

I fixed the test (fatal PHP error + errors in the test path)

zero2one’s picture

BTW: this issue is related to https://drupal.org/node/2099205

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847-20.patch, failed testing.

zero2one’s picture

Damn :(

I need some help here:

  • The test runs fine locally (trough interface)
  • Manual tests prove that the fix works

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?

hass’s picture

See #15. don't attach another patch with regexes, please.

mr.baileys’s picture

@hass: drupal_parse_url() (deprecated) and Url::parse() both explicitly warn that…

This function should only be used for URLs that have been generated by the system. It should not be used for URLs that come from external sources, or URLs that link to external resources.

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.

hass’s picture

I 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.

zero2one’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

We 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.

Status: Needs review » Needs work

The last submitted patch, image-filter-2076847-27.patch, failed testing.

zero2one’s picture

FileSize
10.05 KB

I don't get it.

The test runs fine on my machine (both UI and CLI)

See attachment.

mr.baileys’s picture

$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.

zero2one’s picture

Off 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.

zero2one’s picture

Status: Needs work » Needs review

Fixed issue status.

mr.baileys’s picture

Looking at the history of that code, it seems that the bug was introduced in #1999376: Use Symfony Request for filter module

-    $image->setAttribute('src', preg_replace('|^https?://' . $_SERVER['HTTP_HOST'] . '|', '', $src));
+    $request = Drupal::request();
+    $image->setAttribute('src', preg_replace('|^https?://' . $request->getHost() . '|', '', $src));

$_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.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! Great work, zero2one & mr.baileys! Go team Belgium! :D

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yayyyyy! I have images again!! :D

Committed and pushed to 8.x. Thanks SO much folks!!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.