Problem/Motivation

file_save_upload calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.
  4. Usability review needed regarding output of single <li> rather than the use of the if/else pattern to not render list items when only one item is present.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. under structure, content types, article, manage fields, image, set maximum file size to 100, set minimum resolution to 20000 x 20000
  3. at node/add/article upload a image file that violates both criteria 2M and 1440 × 960
  4. see error with two items in the list
  5. change one of the image field requirements to be more reasonable, make maximum file size 2000000
  6. reload the node/add/article
  7. upload an image (similar example image is fine and should just violate the min resolution now)
  8. see one error in the list
  9. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  10. (N/A) If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

before with only one message

after with only one message

before with more than one message

after with more than one message

---------- with java script -------
no change

API changes

N/A

Follow-ups

Files: 
CommentFileSizeAuthor
#34 PATCH16-node-add-article-1434308366762(1).png136.26 KBcrowdcg
#34 HEAD-node-add-article-1434307087482(1).png136.34 KBcrowdcg
#27 interdiff-2501827-16.txt3.21 KBjcloys
#27 remove_assertion_message_2501827_16.patch3.67 KBjcloys
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,778 pass(es). View
#23 extension-error.png161.55 KBYesCT
#18 duplicate.png123.5 KBYesCT
#16 double-error.png122.07 KBYesCT
#16 txt-error.png73.25 KBYesCT
#15 interdiff.txt2.47 KBkgoel
#15 2501827-15.patch3.77 KBkgoel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,850 pass(es). View
#13 Screen Shot 2015-06-12 at 10.11.52 PM.png43.96 KBkgoel
#13 file_test.zip834 byteskgoel
#13 2501827-13.patch1.29 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,840 pass(es), 2 fail(s), and 0 exception(s). View
#8 Screen Shot 2015-06-07 at 9.25.32 AM.png51.72 KBcwells
#8 remove-2501827-8.patch861 bytescwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,410 pass(es). View
#4 remove_safemarkup_set-2501827-4.patch866 bytescwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,697 pass(es), 11 fail(s), and 0 exception(s). View
#1 remove_safemarkup_set-2501827-1.patch867 bytescwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/file.module. View

Comments

cwells’s picture

Title: Remove SafeMarkup::set in file_save_upload » Remove SafeMarkup::set() in file_save_upload
Assigned: Unassigned » cwells
Status: Active » Needs review
FileSize
867 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/file/file.module. View
cwells’s picture

Status: Needs review » Needs work

The last submitted patch, 1: remove_safemarkup_set-2501827-1.patch, failed testing.

cwells’s picture

FileSize
866 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,697 pass(es), 11 fail(s), and 0 exception(s). View

woops!

cwells’s picture

Status: Needs work » Needs review

Kickbot.

cwells’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 4: remove_safemarkup_set-2501827-4.patch, failed testing.

cwells’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
861 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,410 pass(es). View
51.72 KB

Adjust spacing to be more intelligent anyway, which may fix some of these tests. With this latest patch we also created a dummy module to invoke several file errors.

cwells’s picture

Issue summary: View changes
joelpittet’s picture

Assigned: cwells » Unassigned
Status: Needs review » Reviewed & tested by the community

Very nice and thank you for the manual testing!

Having the extra space in there with the UL item list doesn't matter in HTML and this is much cleaner.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

Thanks @cwells and @joelpittet. +1 for the manual testing.

In this issue I think we should refactor the code flow a bit. In general, rather than using SafeMarkup::format() to stick together two variables that have already been individually sanitized and added to the safe list with the if/else deciding what those variables are, it would be better to have two separate code paths that each assemble the markup in the appropriate place. This both makes it easier to understand what the code is doing (and that it will neither introduce double-escaping nor XSS) and reduces the overhead.

Ideally, we'd do away with the drupal_render() call entirely. But (as @alexpott pointed out), in this case it's being passed to drupal_set_message() which doesn't support render arrays. I'd like to see an issue to discuss whether we should optionally make drupal_set_message() support render arrays by at the minimum using the renderer itself when needed (there are pros and cons to that), or using hashing or suchlike to provide the array keys (pros and cons to that as well) and supporting actual render arrays in a bubbleable-and-best-practice-and-etc. fashion. All that is totally out of scope here, though. So an @todo and a followup for that would be lovely. "Decide how and whether to support render arrays for drupal_set_message()" would probably be the title.

However, even within the function itself, we can simplify it. Regarding this change:

+++ b/core/modules/file/file.module
@@ -856,11 +856,12 @@ function file_save_upload($form_field_name, $validators = array(), $destination
-        $message = SafeMarkup::set($message . drupal_render($item_list));
+        $error_text = drupal_render($item_list);

In HEAD, that is taking one string, and appending it to a rendered render array. It'd be better to just put the string in the render array first (since we already have one), and then do the drupal_render() on the whole thing.

I also question why we have this if/else in the first place. Looking at the whole of file_save_upload(), it's there solely to make it an item list if there's more than one item, but a single string if it's not. That seems wrong to me -- a list of only one item is still a list, and it should be up to the themeable output to decide what to do with a list that has only one item in it, not a particular CRUD operation. So I'd consider removing that else condition with the array_pop() entirely (we'd compare screenshots for a single item) and then with the suggestion above we just have one render array with one drupal_render() call only, and no extra SafeMarkup anything.

YesCT’s picture

kgoel’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,840 pass(es), 2 fail(s), and 0 exception(s). View
834 bytes
43.96 KB

I took the approach as suggested in #11. $message is an array with error and item_list arrays and adding render on drupal_set_message, got rid of if and else statement so now it displays the item list regardless of number of items. I have added test module which I created to test the error message on the node edit/add page.

Added screenshot here

Status: Needs review » Needs work

The last submitted patch, 13: 2501827-13.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,850 pass(es). View
2.47 KB
YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
73.25 KB
122.07 KB

I dont see what in the patch is making this happen...

but just uploading a txt file, does not cause the list.

doing a different test,
changing the article content type field setting for image to have a max allowed size,
and uploading a png greater than that,
then I *do* get the list,
but... I get it twice.

I dont know why.

YesCT’s picture

Actually, the double error message is in head also. So I do not think this issue needs to fix that. ... maybe it is already covered by a child of the follow-up to the inline error issue? #2504847: [meta] Roadmap for stabilizing Inline Form Errors module (IFE)

[edit: Maybe it is #2482783: File upload errors not set or shown correctly ?]

YesCT’s picture

Issue summary: View changes
FileSize
123.5 KB

here is the screenshot of head.

kgoel’s picture

I dont see what in the patch is making this happen...

but just uploading a txt file, does not cause the list.

If you try to upload a txt file for an image field in article content type, you get the error which is coming from file.js line 124

var error = Drupal.t("The selected file %filename cannot be uploaded. Only files with the following extensions are allowed: %extensions.",

YesCT’s picture

Status: Needs work » Needs review

soo... there is no safeMarkup::set() in the js.

So I think the patch might be ok.

YesCT’s picture

+++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
@@ -298,7 +298,8 @@ public function testDefaultImages() {
-    $this->assertText(t('The specified file text-0.txt could not be uploaded. Only files with the following extensions are allowed: png gif jpg jpeg.'), 'Non-image file cannot be used as default image.');
+    $this->assertText('The specified file text-0.txt could not be uploaded.', 'Non-image file cannot be used as default image.');
+    $this->assertText('Only files with the following extensions are allowed: png gif jpg jpeg.', 'Non-image file cannot be used as default image.');

I wonder if this change is not necessary. Since that error comes from the js and is not reformatted in this issue.

[edit]
oops. I see it was an actual fail in #13.. I will look into why, if the error is coming from the js which is not changed.

YesCT’s picture

To get the list without the module, set a small (100) maximum file size, and a large (20000 x 20000) minimal resolution, and select an image that is .. medium say: 2M and 1440 × 960 or so.

------------

Another problem I noticed that already exists in head, which most likely would only happen to people testing this,
is that if the node/add/article page is reloaded (so no error is present)
and the maximum file size is set, and the minimal resolution,
and try and upload an image,
see the list of the two errors,
in another window, change the article content type image field to have a giant maximum file size,
back in the node add window, try to upload the image again...
and instead of only seeing the minimal resolution error, both errors are still there.
but, if reload the node/add/article page
then only the one appropriate error is shown.

again, this occurs in head, so not the problem of this issue.

YesCT’s picture

FileSize
161.55 KB

answering my own question from #21

simpletest does not have javascript. :) so the error is displayed as a list.

YesCT’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

updated issue summary.

I think all the things @xjm asked for have been done in the patch,
and the concerns I found are not really to be solved in this issue.

rtbc.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/file.module
    @@ -850,18 +850,18 @@ function file_save_upload($form_field_name, $validators = array(), $destination
    +      $message = array(
    +        'error' => array(
    +          '#markup' => t('The specified file %name could not be uploaded.', array('%name' => $file->getFilename())),
    +        ),
    +        'item_list' => array(
    

    Nice, this is very clear now.

  2. +++ b/core/modules/file/file.module
    @@ -850,18 +850,18 @@ function file_save_upload($form_field_name, $validators = array(), $destination
    +      // @todo Add support render_arrays for drupal_set_message. See
    +      //  https://www.drupal.org/node/2505497
    

    Minor: I don't think we need the underscore for "render arrays". Also drupal_set_message() should have parentheses at the end, and I guess we need a period at the end of the sentence.

  3. +++ b/core/modules/image/src/Tests/ImageFieldDefaultImagesTest.php
    @@ -298,7 +298,8 @@ public function testDefaultImages() {
    +    $this->assertText('The specified file text-0.txt could not be uploaded.', 'Non-image file cannot be used as default image.');
    +    $this->assertText('Only files with the following extensions are allowed: png gif jpg jpeg.', 'Non-image file cannot be used as default image.');
    
    +++ b/core/modules/image/src/Tests/ImageFieldValidateTest.php
    @@ -45,7 +45,8 @@ function testResolution() {
    +    $this->assertRaw(t('The specified file %name could not be uploaded.', array('%name' => $image_that_is_too_small->filename)), 'Node save failed when minimum image resolution was not met.');
    +    $this->assertRaw(t('The image is too small; the minimum dimensions are %dimensions pixels.', array('%dimensions' => '50x50')), 'Node save failed when minimum image resolution was not met.');
    

    So it makes sense for these to be two separate assertion messages now. However, having two separate assertions with the same assertion message adds difficulty for debugging.

    I'd actually just remove the assertion message from all four of these assertions now. assertText() has an okay default for it.

jcloys’s picture

Looking at this now.

jcloys’s picture

Status: Needs work » Needs review
FileSize
3.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,778 pass(es). View
3.21 KB

Made changes per 2 and 3 above.

xjm’s picture

Looking good!

Before RTBCing, can we get input on whether removing the special case for one entry in the item list is out of scope or not desirable?

If we think so, then we should still use a render array, but the else condition would use it as well.

cwells’s picture

Just my $0.02 - I think getting rid of the special case for one entry IS desireable, because it makes everything a lot more consistent. I think our default markup would do something like:

<li class="first last..."></li>

...which means we could li.first.last { list-type-type: none; } or some such if people really wanted the single-entry to look a little different. Personally, I like the look of

crowdcg’s picture

Status: Needs review » Needs work
Issue tags: +Needs usability review

Based on reading through the issue and installing/testing the patch locally, as well as talking with @davidhernandez IRL, I think it is desirable to keep this for consistency. However, we should get usability feedback whether or not it is ok to leave the one bullet point.

crowdcg’s picture

Status: Needs work » Needs review

Accidentally changed status, setting it back.

YesCT’s picture

If this had to have a new patch made again, someone could fix the space on the indent of the second line of the @todo to be indented two spaces.
https://www.drupal.org/node/1354#todo

but that is very not blocking.

crowdcg’s picture

Updated issue summary to reflect that this issue needs usability review, as well as provided additional screenshots of more than one error. Lastly, this issue does not require automated test coverage for the sanitization of the string, since it was only previously calling SafeMarkup::set(), which is consistent with other child issues of the meta #2280965: [meta] Remove every SafeMarkup::set() call.

crowdcg’s picture

Bojhan’s picture

Issue tags: -Needs usability review

I am not a big fan of single bullet point errors (bullets are visually, much more sensible when there are several). However it is by large a non-issue from a usability perspective. So I think its OK, to move forward with this direction.

In this case we lead with "upload is failing" not a specific error. If we were to lead with a specific error, it would be weird to not have this in a bullet.

Regarding #29, we should always avoid changes that are visually different from the semantic intent - unless it has big UX benefits.

pwolanin’s picture

Issue summary: View changes
Status: Needs review » Needs work

Need to add another assertText for 'Image exceeding max resolution was properly resized.', since that text was taken out since it's on another line.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

oops - mis-read the change. I thought it was like the concatenated string above in the patch. That was just removing the assertion message to actually show the text being searched for.

xjm’s picture

Title: Remove SafeMarkup::set() in file_save_upload » Remove SafeMarkup::set() in file_save_upload() and allow render/template code to control a single-item list

Retitling to include the scope of the item list simplification.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice work! More and more, render arrays seem like a sound solution. @alexpott also looked this over and was +1.

I like also that we removed some unneeded theme-babysitting, if you will.

As a required part of a critical issue, this issue can be completed any time during the beta phase per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Congratulations @jcloys on your first Drupal 8 commit credit!

I made a small docs fix on commit:

diff --git a/core/modules/file/file.module b/core/modules/file/file.module
index 912e9b9..9f0074f 100644
--- a/core/modules/file/file.module
+++ b/core/modules/file/file.module
@@ -859,7 +859,7 @@ function file_save_upload($form_field_name, $validators = array(), $destination
           '#items' => $errors,
         ),
       );
-      // @todo Add support render arrays for drupal_set_message(). See
+      // @todo Add support for render arrays in drupal_set_message()? See
       //  https://www.drupal.org/node/2505497.
       drupal_set_message(\Drupal::service('renderer')->render($message), 'error');
       $files[$i] = FALSE;

  • xjm committed 0f88b17 on 8.0.x
    Issue #2501827 by cwells, kgoel, jcloys, YesCT, crowdcg, Bojhan: Remove...

Status: Fixed » Closed (fixed)

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