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.

Files: 
CommentFileSizeAuthor
#13 filter_html_image_secure_process-2096855-13-fail.patch1.7 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,648 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#13 filter_html_image_secure_process-2096855-13-pass.patch2.38 KBzero2one
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]
#11 filter_html_image_secure_process-2096855-11-pass.patch2.5 KBzero2one
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es).
[ View ]
#9 filter_html_image_secure_process-2096855-9-fail.patch1.73 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,858 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 filter_html_image_secure_process-2096855-9-pass.patch2.42 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,849 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 filter_html_image_secure_process-2096855-5-fail.patch1.83 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,563 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#6 filter_html_image_secure_process-2096855-5-pass.patch2.52 KBzero2one
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]
#3 filter_html_image_secure_process-2096855-7894861.patch2.59 KBzero2one
FAILED: [[SimpleTest]]: [MySQL] 58,547 pass(es), 6 fail(s), and 2 exception(s).
[ View ]
#1 filter_html_image_secure_process_support_spaces-2096855.patch818 byteszero2one
PASSED: [[SimpleTest]]: [MySQL] 58,703 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new818 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,703 pass(es).
[ View ]

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.

Issue tags:+Needs tests

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

StatusFileSize
new2.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,547 pass(es), 6 fail(s), and 2 exception(s).
[ View ]

Added the scenario's to the tests.

Please review the test code.

  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!

Issue tags:-Needs tests

Tag can go, we have tests now

Issue tags:+Needs tests
StatusFileSize
new2.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]
new1.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,563 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

Issue tags:-Needs tests

crosspost

StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,849 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.73 KB
FAILED: [[SimpleTest]]: [MySQL] 58,858 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,751 pass(es).
[ View ]
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new2.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,781 pass(es).
[ View ]
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,716 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Changed issue status

StatusFileSize
new2.38 KB
PASSED: [[SimpleTest]]: [MySQL] 58,335 pass(es).
[ View ]
new1.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,648 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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! :)

Title:The "Restrict images to this site" filter does not support images with spaces in the filenameFilterHtmlImageSecure does not support images with spaces in the filename

.

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.

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

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

Status:Patch (to be ported)» Closed (fixed)

Yep.

Is this what d.o use?