Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123Identify 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.If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.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:
- Clean install of Drupal 8.
- under structure, content types, article, manage fields, image, set maximum file size to 100, set minimum resolution to 20000 x 20000
- at node/add/article upload a image file that violates both criteria 2M and 1440 × 960
- see error with two items in the list
- change one of the image field requirements to be more reasonable, make maximum file size 2000000
- reload the node/add/article
- upload an image (similar example image is fine and should just violate the min resolution now)
- see one error in the list
- Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
- (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
Comment | File | Size | Author |
---|---|---|---|
#34 | PATCH16-node-add-article-1434308366762(1).png | 136.26 KB | seantwalsh |
#34 | HEAD-node-add-article-1434307087482(1).png | 136.34 KB | seantwalsh |
#27 | interdiff-2501827-16.txt | 3.21 KB | jcloys |
#27 | remove_assertion_message_2501827_16.patch | 3.67 KB | jcloys |
#23 | extension-error.png | 161.55 KB | YesCT |
Comments
Comment #1
chrisfromredfinComment #2
chrisfromredfinComment #4
chrisfromredfinwoops!
Comment #5
chrisfromredfinKickbot.
Comment #6
chrisfromredfinComment #8
chrisfromredfinAdjust 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.
Comment #9
chrisfromredfinComment #10
joelpittetVery 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.
Comment #11
xjmThanks @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 todrupal_set_message()
which doesn't support render arrays. I'd like to see an issue to discuss whether we should optionally makedrupal_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:
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 onedrupal_render()
call only, and no extraSafeMarkup
anything.Comment #12
YesCT CreditAttribution: YesCT commentedfollow-up: #2505497: Support render arrays for drupal_set_message()
Comment #13
kgoel CreditAttribution: kgoel at Forum One commentedI 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
Comment #15
kgoel CreditAttribution: kgoel at Forum One commentedComment #16
YesCT CreditAttribution: YesCT commentedI 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.
Comment #17
YesCT CreditAttribution: YesCT commentedActually, 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 ?]
Comment #18
YesCT CreditAttribution: YesCT commentedhere is the screenshot of head.
Comment #19
kgoel CreditAttribution: kgoel at Forum One commentedIf 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.",
Comment #20
YesCT CreditAttribution: YesCT commentedsoo... there is no safeMarkup::set() in the js.
So I think the patch might be ok.
Comment #21
YesCT CreditAttribution: YesCT commentedI 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.
Comment #22
YesCT CreditAttribution: YesCT commentedTo 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.
Comment #23
YesCT CreditAttribution: YesCT commentedanswering my own question from #21
simpletest does not have javascript. :) so the error is displayed as a list.
Comment #24
YesCT CreditAttribution: YesCT commentedupdated 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.
Comment #25
xjmNice, this is very clear now.
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.
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.Comment #26
jcloys CreditAttribution: jcloys commentedLooking at this now.
Comment #27
jcloys CreditAttribution: jcloys commentedMade changes per 2 and 3 above.
Comment #28
xjmLooking 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.
Comment #29
chrisfromredfinJust 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 ofComment #30
seantwalshBased 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.
Comment #31
seantwalshAccidentally changed status, setting it back.
Comment #32
YesCT CreditAttribution: YesCT commentedIf 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.
Comment #33
seantwalshUpdated 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.
Comment #34
seantwalshComment #35
Bojhan CreditAttribution: Bojhan as a volunteer commentedI 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.
Comment #36
pwolanin CreditAttribution: pwolanin at Acquia commentedNeed to add another assertText for 'Image exceeding max resolution was properly resized.', since that text was taken out since it's on another line.
Comment #37
pwolanin CreditAttribution: pwolanin at Acquia commentedoops - 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.
Comment #38
xjmRetitling to include the scope of the item list simplification.
Comment #39
xjmNice 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: