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
template_preprocess_file_widget_multiple()
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.- tests not needed. See #39 "button elements are interface-translated which is an admin permission so we do not need special XSS tests"
Remaining tasks
- (done) (Refactor using show() ) Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
- (done) 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.
If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.- Manual testing/ markup before & after screenshots.
Manual testing steps (for XSS and double escaping)
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Add an unlimited-value file field on the article node type.
- Create a new article node, and upload several files into the new file field using the multiple file widget.
- After uploading the files but before saving the node, compare the output of the node add form, in HEAD and with the patch applied. Confirm that the markup is identical and there is no double-escaping.
If there is any user or calling code input in the string, submit
alert('XSS');and ensure that it is sanitized.
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#64 | 2502781-64.patch | 1.66 KB | stefan.r |
#64 | interdiff-40-64.txt | 736 bytes | stefan.r |
#57 | 2502781_57.patch | 614 bytes | johnshortess |
#40 | interdiff.2502781.37.40.txt | 1.29 KB | johnshortess |
#37 | 2502781_37.patch | 1.57 KB | tedstein |
Comments
Comment #1
joelpittetI've refactored this preprocess to use #type table properly(without setting #rows like #type table) and don't have to do any of the early drupal_render/hide/render stuff.
Comment #3
joelpittetThis may still fail but it should be a bit closer with less exceptions, I hope.
Comment #4
joelpittetInterdiff between 1 and 3
Comment #6
pwolanin CreditAttribution: pwolanin at Acquia commentedLet me take a look
Comment #7
joelpittetI probably got a bit overzealous on some of the hide/unset changes but the direction should work (did for most of the
#type => table
conversions).#rows
key on#type => table
side-steps it's#pre_render
callback which deals with element children as if they were rows.Good luck! Ping me if you have any questions.
Comment #8
pwolanin CreditAttribution: pwolanin at Acquia commentedI think we need to take a step back and remove the drupal_render() calls in template_preprocess_table()
Comment #9
pwolanin CreditAttribution: pwolanin at Acquia commentedpostponed on making the table change in #2505469: Remove drupal_render() calls from template_preprocess_table()
Comment #10
xjmComment #11
xjmThat one is in. :)
Comment #12
pwolanin CreditAttribution: pwolanin at Acquia commentedTrying to refactor to use render arrays, but still seeing test breakage and some wonkiness in the UI, though manual testing works to upload files.
Comment #14
mandclu CreditAttribution: mandclu commentedWorking to resolve this issue as part of #drupalnorth2015
Comment #15
mandclu CreditAttribution: mandclu commentedUpdated the previous patch to fix the duplicate listing of uploaded files on the edit page. This reduced testing errors by one but increased the exceptions by one.
Comment #16
star-szrSetting to needs review so it gets tested, thanks @mandclu!
Comment #18
chx CreditAttribution: chx commentedTo fix this issue IMO this little is enough.
Comment #20
lauriiiGood idea! The reason why this is not working is that in the render array we get from the widget, #printed is set to TRUE which prevents it from being rendered
Comment #21
dawehnerSo what using render($operations_elements) and describe why we need it at that point?
Comment #22
johnshortess@tedstein and I are sprinting at DrupalGovCon, and attempting @dawehner's suggestion from #21.
Comment #23
johnshortessHere's a new patch incorporating @dawehner's suggestion from #21. I haven't commented the change yet, until I see if the testbot likes it.
Comment #24
johnshortessChanging status to send to testbot.
Comment #25
tedstein CreditAttribution: tedstein at Inner File Software commentedThe patch uploaded above runs render() but I don't believe it will return anything.
My guess (and this might be what lauriii and dawehner are saying):
#printed is set to true when the first file is rendered, which is why render() fails when there are multiple files attached. There is no #markup element, so render() returns nothing.
My suggestion:
Test to see if #printed is true and #markup is not set and, if so, change #printed to false. Thoughts?
Comment #27
joelpittetShow()/hide() functions still exists. May need to use that or maybe de-reference. Need to dig a bit deeper. Give show a try though
Comment #28
tedstein CreditAttribution: tedstein at Inner File Software commentedThanks joelpittet. show() doesn't work because it doesn't recursively search the array setting #printed to false (there are multiple elements). I think I might have something working with array_walk_recursive(). I will upload the patch tomorrow morning at the sprint.
Comment #29
tedstein CreditAttribution: tedstein at Inner File Software commentedI worked off of the strategy comment 18 started. I wanted to use show() to fix the #printed, but it was a couple levels deep in the array. I used array_walk_recursive() instead.
I think this patch does it, but it doesn't feel super "Drupally."
Ideas for a better fix:
1. Find where #printed gets set to TRUE and address it there.
2. Update show() to walk recursively (I could just move my code there), but that seems a bit much.
3. Iterate on the array more selectively and use show() without modifying show().
In any event, I think this works. Sending to testbot.
Comment #30
tedstein CreditAttribution: tedstein at Inner File Software commentedReworked the patch above to more specifically target the elements that need to be displayed and use show(). I think this one should be good to go?
Comment #31
lauriiiI think changing #print manually is little bit dirty. I personally think calling drupal_render is maybe a little less dirty thing to do.
Comment #32
tedstein CreditAttribution: tedstein at Inner File Software commentedIgnore the one above.
This one reworks patch 29 to more specifically target the elements that need to be displayed and use show(). I think *this one* should be good to go?
Comment #33
johnshortessJust talked to @tedstein and @pwolanin at the drupalgovcon sprint. The consensus here is that the patch in #32 (assuming testbot pass) is a good solution for the immediate issue of removing SafeMarkup::set, but a follow-up issue should be filed to get rid of the premature #printed in the first place. Thoughts?
Comment #34
tedstein CreditAttribution: tedstein at Inner File Software commented> I think changing #print manually is little bit dirty. I personally think calling drupal_render is maybe a little less dirty thing to do.
drupal_render() is deprecated. The render service has this check:
(https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!Renderer.p...)
Thus, we need show. I think that we should prevent the initial render, but that seems outside the scope of this issue. I think we should consider creating a new issue to stop the initial render, but I also think that should be a separate issue and the patch above (#32) solves this issue. Thoughts?
Comment #36
johnshortessI've manually tested this, and confirmed that the output is identical (and not double-escaped) with and without the patch.
Comment #37
tedstein CreditAttribution: tedstein at Inner File Software commentedCreated a new issue to figure out why #printed is already marked as true. Added a link to the new issue in the code.
#2538710: Refactor template_preprocess_file_widget_multiple() to use the twig template and not do so much early rendering
Comment #38
cilefen CreditAttribution: cilefen commentedThis comment has lost its meaning in the place where it is. It actually explains what is being done on line 124 in a way.
Based on the comment I mentioned in #1, it seems that it was understood that these elements had been hidden. I don't know if that means the same thing as '#printed'. That's just an observation.
Comment #39
cilefen CreditAttribution: cilefen commentedAlso, I think these button elements are interface-translated which is an admin permission so by fiat we do not need special XSS tests.
Comment #40
johnshortessHere's @tedstein's patch with some updated comments that attempt to address @cilefen's notes in #38. No code has changed in this patch, just comments.
Comment #41
chx CreditAttribution: chx commentedInstead of show, copy them?
Comment #42
YesCT CreditAttribution: YesCT commented@johnshortess please update the issue summary and include the step for testing this manually.
Comment #43
johnshortessComment #44
johnshortessComment #45
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedI am reviewing this.
Comment #46
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedI am not able to verify 4th step mentioned in Issue description.
"After uploading the files but before saving the node, compare the output of the node add form, in HEAD and with the patch applied. Confirm that the markup is identical and there is no double-escaping."
I have followed the manual steps to test with or without patch on new installation of Drupal 8.0.x.
Attaching the node_add_form markup in diff files.
https://www.drupal.org/files/issues/without-patch.txt
https://www.drupal.org/files/issues/with-patch.txt
Screenshot -
Let someone also verify this so keeping needs review only.
Comment #47
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #48
YesCT CreditAttribution: YesCT commentedI'm going to look at this whole thing and see if there is anything left to do.
Comment #49
YesCT CreditAttribution: YesCT commentedlooks like those files of html markup were not make from a scratch install with exactly the same steps, so that makes it more difficult to compare.
I'm going to try and redo that now.
Comment #50
YesCT CreditAttribution: YesCT commentedthe markup is exactly the same (except for hashes). (I copied the output to a git repo, added the output from with and without the patch to different branches and looked at the git diff --color-words)
looks like the comments were updated as requested, and tests are not needed, so rtbc.
Comment #51
chx CreditAttribution: chx commentedWhich patch is RTBC? I do not see any unhidden patches in the issue summary. The last patch was using
show
edit:which was called "dirty" before and I recommended copying them because they are not the same -- they are copies of the same.this function is already using hide() so adding a show() won't make it worse.Comment #52
lauriiiSetting back to NW according to #51
Comment #53
chx CreditAttribution: chx commentedDon't; I edited it ; you are too fast :) I presume #40 was marked RTBC so unhiding that patch.
Comment #54
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented@chx, thats correct, we had reviewed #40 patch.
Thanks
Comment #55
xjmComment #56
cilefen CreditAttribution: cilefen commentedFrom @alexpott:
let's turn this around to be
Hopefully swapping that around will mean we won't need to do the show.
Comment #57
johnshortessHere's a new patch, with @alexpott's suggestion mentioned by @cilefen in #56. It seems to work locally. Let's see if the testbot likes it.
Comment #59
alexpottHmmm so my idea in #56 is not going to work. Doing the full render must add information that is required to render the submit button properly. I think given that, and the requirement of the form to render the submit button outside of the widget the patch in #40 is good to go.
We know precisely where we hide this - it is earlier is this preprocess function. This comment should just say that - no need for an @todo.
Comment #60
Wim LeersThis looks correct to me.
(But we should definitely try to get rid of
hide()
,show()
andrender()
.)Comment #61
alexpottI think the comment in #40 needs fixing and #2538710: Refactor template_preprocess_file_widget_multiple() to use the twig template and not do so much early rendering should be repurpose to move as much of preprocess into the template as possible.
Comment #62
stefan.r CreditAttribution: stefan.r commented#printed is set to TRUE when we do hide($widget[$sub_key]), and show() just undoes that, right? So #2538710: Refactor template_preprocess_file_widget_multiple() to use the twig template and not do so much early rendering would be a won't fix?
Comment #63
stefan.r CreditAttribution: stefan.r commentedI don't think I had uploaded any patch there in #62, a drupal.org bug?
Comment #64
stefan.r CreditAttribution: stefan.r commentedComment #65
pwolanin CreditAttribution: pwolanin at Acquia commentedI'm confused - the last patch doesn't seem to change the hide()?
Comment #66
alexpott@pwolanin see #59
Comment #67
cilefen CreditAttribution: cilefen commentedIt looks to me that all concerns have been addressed.
Comment #68
alexpottThis is making HEAD less bad by doing a late render. The show() is unfortunate but HEAD only gets away with it because it is using render() which ignores whether something is printed.
The followup (#2538710: Refactor template_preprocess_file_widget_multiple() to use the twig template and not do so much early rendering) needs to be re-scoped to look at the entire template_preprocess_file_widget_multiple() and try to move as much as possible into the twig template.
Committed f802277 and pushed to 8.0.x. Thanks!