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.

Files: 
CommentFileSizeAuthor
#33 2076847-33-local-image-filter.patch2.31 KBmr.baileys
PASSED: [[SimpleTest]]: [MySQL] 59,257 pass(es).
[ View ]
#31 image-filter-2076847-31.patch2.8 KBzero2one
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]
#29 HtmlImageSecureTest.txt10.05 KBzero2one
#27 image-filter-2076847-27.patch2.37 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,851 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#20 image-filter-2076847-20.patch1.66 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 image-filter-2076847.17.patch1.7 KBsocketwench
FAILED: [[SimpleTest]]: [MySQL] 58,719 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#13 image-filter-2076847.13.interdiff.txt1 KBlarowlan
#13 image-filter-2076847.13.patch1.7 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 image-filter-2076847.9.patch2.48 KBWim Leers
FAILED: [[SimpleTest]]: [MySQL] 58,469 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 interdiff.txt1.78 KBWim Leers
#6 image-filter-2076847.interdiff.txt971 byteslarowlan
#6 image-filter-2076847.4.patch1.65 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
#3 image-filter-2076847.fail_.patch809 byteslarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 image-filter-2076847.pass_.patch1.57 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 58,221 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Issue tags:+Needs tests

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

Should be a pretty easy fix :)

Assigned:Unassigned» larowlan

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

Assigned:larowlan» Unassigned
Status:Active» Needs review
StatusFileSize
new1.57 KB
FAILED: [[SimpleTest]]: [MySQL] 58,221 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new809 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,525 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Subdirectory issues

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
new971 bytes

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

Green this time :)

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

Issue tags:-Needs tests
StatusFileSize
new1.78 KB
new2.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,469 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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

<?php
$url_parts
= parse_url($src);
$path = $url_parts['path'];
?>

should be fine.

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

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1 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.

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

No longer applies. :(

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,719 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reroll!

Status:Needs review» Needs work

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

What about #15?

Status:Needs work» Needs review
StatusFileSize
new1.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

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?

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 58,851 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new10.05 KB

I don't get it.

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

See attachment.

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

StatusFileSize
new2.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Fixed issue status.

StatusFileSize
new2.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,257 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

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

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.