If you add (upload) an image trough the CK editor and:

  • That image contains a space in its filename
  • And you have selected the Basic HTML format
  • And that Basic HTML format has the "Restrict images to this site" filter enabled

Then the rendered node will have a blocked image icon and the text that the image is blocked due to security reasons.

This caused by the _filter_html_image_secure_process() function.

It searches trough the DOM in the HTML for image tags, extract the src of them and uses that path to see if it is an internal URL and if the image exists.

The HTML has the rawurl encoded version of the filename. This means that the space is in the src rendered as "%20".
Using that path to get the imagesize breaks because the file is not found.

CommentFileSizeAuthor
#13 filter_html_image_secure_process-2096855-13-fail.patch1.7 KBzero2one
#13 filter_html_image_secure_process-2096855-13-pass.patch2.38 KBzero2one
#12 filter_html_image_secure_process-2096855-12-fail.patch1.81 KBzero2one
#12 filter_html_image_secure_process-2096855-12-pass.patch2.5 KBzero2one
#11 filter_html_image_secure_process-2096855-11-fail.patch1.81 KBzero2one
#11 filter_html_image_secure_process-2096855-11-pass.patch2.5 KBzero2one
#9 filter_html_image_secure_process-2096855-9-fail.patch1.73 KBzero2one
#9 filter_html_image_secure_process-2096855-9-pass.patch2.42 KBzero2one
#6 filter_html_image_secure_process-2096855-5-fail.patch1.83 KBzero2one
#6 filter_html_image_secure_process-2096855-5-pass.patch2.52 KBzero2one
#3 filter_html_image_secure_process-2096855-7894861.patch2.59 KBzero2one
#1 filter_html_image_secure_process_support_spaces-2096855.patch818 byteszero2one
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zero2one’s picture

zero2one CreditAttribution: zero2one commented
Status: Active » Needs review
FileSize
filter_html_image_secure_process_support_spaces-2096855.patch818 bytes

I created a patch for this:

It first passes the rawurl encoded src of the image trough the rawurldecode function.
And uses the result to get the image size.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: +Needs tests

Thanks, can you also add this scenario to the tests for that filter?

zero2one’s picture

zero2one CreditAttribution: zero2one commented
FileSize
filter_html_image_secure_process-2096855-7894861.patch2.59 KB

Added the scenario's to the tests.

Please review the test code.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
  1. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php
@@ -91,6 +91,13 @@ function testImageSource() {
+    ¶

    There's some trailing whitespace here 

  2. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php
@@ -91,6 +91,13 @@ function testImageSource() {
+    // @see https://drupal.org/node/2096855#comment-7894015

    We don't normally reference fixes in comments, can you remove this @see?

Can we get two versions of the patch, one that is 'tests only' i.e. doesn't include the fix.
And a second one that is tests + fix.
If you upload them to the same comment, with the tests only one first, we should see 'fail' then 'pass' for the patches.
This give the committer confidence that a) we've found the problem b) we have a test so it never breaks again c) we have a fix

Thanks!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: -Needs tests

Tag can go, we have tests now

zero2one’s picture

zero2one CreditAttribution: zero2one commented
Issue tags: +Needs tests
FileSize
filter_html_image_secure_process-2096855-5-pass.patch2.52 KB
filter_html_image_secure_process-2096855-5-fail.patch1.83 KB

This are the patches:

  • One that has the test, but that fails because of the fix not in place.
  • One that has the test, and passes because of the fix

I also removed the whitespace.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Just nitpicks now

  1. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php
@@ -92,6 +92,12 @@ function testImageSource() {
+    // Put a test image in the files directory with special filename

    (nitpick) needs a . on end of line as per coding standards (see node/1354) 

  2. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php
@@ -92,6 +92,12 @@ function testImageSource() {
+    $special_base_uri = str_replace($test_images[0]->filename, NULL, $test_images[0]->uri);

    I think there might be a function to do this, basepath perhaps? 

  3. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHtmlImageSecureTest.php
@@ -92,6 +92,12 @@ function testImageSource() {
+    $special_image    = rawurlencode($special_filename);

    no need for indent/alignment I think

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: -Needs tests

crosspost

zero2one’s picture

zero2one CreditAttribution: zero2one commented
FileSize
filter_html_image_secure_process-2096855-9-pass.patch2.42 KB
filter_html_image_secure_process-2096855-9-fail.patch1.73 KB

I refactored the code to match the standards.

I removed the code to get the base path, the file is in stored in the root of the public folder (public://) anyway.

Status: Needs review » Needs work

The last submitted patch, filter_html_image_secure_process-2096855-9-pass.patch, failed testing.

zero2one’s picture

zero2one CreditAttribution: zero2one commented
FileSize
filter_html_image_secure_process-2096855-11-pass.patch2.5 KB
filter_html_image_secure_process-2096855-11-fail.patch1.81 KB

public:// worked locally but not on the d.o test bot.

Reverted the code to a string_replace. It's less environment dependent.

zero2one’s picture

zero2one CreditAttribution: zero2one commented
Status: Needs work » Needs review
FileSize
filter_html_image_secure_process-2096855-12-pass.patch2.5 KB
filter_html_image_secure_process-2096855-12-fail.patch1.81 KB

Changed issue status

zero2one’s picture

zero2one CreditAttribution: zero2one commented
FileSize
filter_html_image_secure_process-2096855-13-pass.patch2.38 KB
filter_html_image_secure_process-2096855-13-fail.patch1.7 KB

Removed the change of the file mode of the settings.php file from the patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

I asked for a reroll in person of #13 because it contained an inappropriate permission change.

Now the patch is perfect. Nice and simple, additional test coverage to prevent regressions, a fail and pass patch to prove it works.

Thanks! :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: The "Restrict images to this site" filter does not support images with spaces in the filename » FilterHtmlImageSecure does not support images with spaces in the filename

.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Awesome work!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

hass’s picture

hass CreditAttribution: hass commented
Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to 7.x
Related issues: +#2125301: %20 in image file names triggers the "local images only" filter

#2125301: %20 in image file names triggers the "local images only" filter

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

#18 this filter does not exist in Drupal 7. I think you'll want to open an issue for https://drupal.org/project/filter_html_image_secure.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Patch (to be ported) » Closed (fixed)

Yep.

hass’s picture

hass CreditAttribution: hass commented

Is this what d.o use?

tvn’s picture

tvn CreditAttribution: tvn commented

Yes, D.o is using https://drupal.org/project/filter_html_image_secure

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Issue tags: -Needs backport to 7.x

This is the issue that is affecting d.o #2125301: %20 in image file names triggers the "local images only" filter for future reference.