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

  1. (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
  2. (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.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.
  4. 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:

  1. Clean install of Drupal 8.
  2. Add an unlimited-value file field on the article node type.
  3. Create a new article node, and upload several files into the new file field using the multiple file widget.
  4. 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.
  5. 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

CommentFileSizeAuthor
#64 2502781-64.patch1.66 KBstefan.r
#64 interdiff-40-64.txt736 bytesstefan.r
#57 2502781_57.patch614 bytesjohnshortess
#50 multfiles.png184.54 KBYesCT
#50 output.txt39.33 KBYesCT
#50 output.txt39.33 KBYesCT
#46 without-patch.txt37.23 KBRavindraSingh
#46 with-patch.txt38.29 KBRavindraSingh
#46 Screen Shot 2015-07-27 at 7.28.38 PM.png223.71 KBRavindraSingh
#40 interdiff.2502781.37.40.txt1.29 KBjohnshortess
#40 2502781_40.patch1.76 KBjohnshortess
#37 2502781_37.patch1.57 KBtedstein
#37 interdiff.2502781_31_37.txt768 bytestedstein
#32 2502781_31.patch1.45 KBtedstein
#32 interdiff.2502781_29_31.txt737 bytestedstein
#30 interdiff.2502781_29_30.txt801 bytestedstein
#30 2502781_30.patch1.52 KBtedstein
#29 interdiff.18_29.txt681 bytestedstein
#29 2502781_29.patch1.49 KBtedstein
#23 interdiff.2502871.18.23.txt464 bytesjohnshortess
#23 2502781_23.patch1.07 KBjohnshortess
#18 2502781_18.patch1.06 KBchx
#15 2502781-15.patch2.73 KBmandclu
#12 2502781-12.patch2.65 KBpwolanin
#8 2502781-table-preprocess-7.patch1023 bytespwolanin
#4 interdiff.txt2.02 KBjoelpittet
#3 remove_safemarkup_set-2502781-3.patch3.52 KBjoelpittet
#1 remove_safemarkup_set-2502781-1.patch3.78 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
3.78 KB

I'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.

Status: Needs review » Needs work

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

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.52 KB

This may still fail but it should be a bit closer with less exceptions, I hope.

joelpittet’s picture

FileSize
2.02 KB

Interdiff between 1 and 3

Status: Needs review » Needs work

The last submitted patch, 3: remove_safemarkup_set-2502781-3.patch, failed testing.

pwolanin’s picture

Let me take a look

joelpittet’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1023 bytes

I think we need to take a step back and remove the drupal_render() calls in template_preprocess_table()

pwolanin’s picture

xjm’s picture

Status: Needs review » Postponed
xjm’s picture

Status: Postponed » Needs work

That one is in. :)

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

Trying to refactor to use render arrays, but still seeing test breakage and some wonkiness in the UI, though manual testing works to upload files.

Status: Needs review » Needs work

The last submitted patch, 12: 2502781-12.patch, failed testing.

mandclu’s picture

Working to resolve this issue as part of #drupalnorth2015

mandclu’s picture

FileSize
2.73 KB

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

star-szr’s picture

Status: Needs work » Needs review

Setting to needs review so it gets tested, thanks @mandclu!

Status: Needs review » Needs work

The last submitted patch, 15: 2502781-15.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

To fix this issue IMO this little is enough.

Status: Needs review » Needs work

The last submitted patch, 18: 2502781_18.patch, failed testing.

lauriii’s picture

+++ b/core/modules/file/file.field.inc
@@ -121,7 +117,9 @@ function template_preprocess_file_widget_multiple(&$variables) {
+      'data' => $operations_elements,

Good 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

dawehner’s picture

Good 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

So what using render($operations_elements) and describe why we need it at that point?

johnshortess’s picture

@tedstein and I are sprinting at DrupalGovCon, and attempting @dawehner's suggestion from #21.

johnshortess’s picture

Here'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.

johnshortess’s picture

Status: Needs work » Needs review

Changing status to send to testbot.

tedstein’s picture

The 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?

Status: Needs review » Needs work

The last submitted patch, 23: 2502781_23.patch, failed testing.

joelpittet’s picture

Show()/hide() functions still exists. May need to use that or maybe de-reference. Need to dig a bit deeper. Give show a try though

tedstein’s picture

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

tedstein’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
681 bytes

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

tedstein’s picture

Reworked 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?

lauriii’s picture

I think changing #print manually is little bit dirty. I personally think calling drupal_render is maybe a little less dirty thing to do.

tedstein’s picture

FileSize
737 bytes
1.45 KB

Ignore 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?

johnshortess’s picture

Just 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?

tedstein’s picture

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

  // Do not print elements twice.
  if (!empty($elements ['#printed'])) {
    return '';
  }

(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?

The last submitted patch, 30: 2502781_30.patch, failed testing.

johnshortess’s picture

I've manually tested this, and confirmed that the output is identical (and not double-escaped) with and without the patch.

tedstein’s picture

FileSize
768 bytes
1.57 KB

Created 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

cilefen’s picture

  1. +++ b/core/modules/file/file.field.inc
    @@ -99,10 +98,6 @@ function template_preprocess_file_widget_multiple(&$variables) {
         // Render the previously hidden elements, using render() instead of
         // drupal_render(), to undo the earlier hide().
    

    This comment has lost its meaning in the place where it is. It actually explains what is being done on line 124 in a way.

  2. +++ b/core/modules/file/file.field.inc
    @@ -121,7 +116,16 @@ function template_preprocess_file_widget_multiple(&$variables) {
    +    // If we have rendered the element once (multiple files) then we need to set
    +    // #printed to false.
    +    // @todo Fix what marks $operations_elements #printed too early.
    +    // https://www.drupal.org/node/2538710
    

    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.

cilefen’s picture

Also, I think these button elements are interface-translated which is an admin permission so by fiat we do not need special XSS tests.

johnshortess’s picture

Here'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.

chx’s picture

Instead of show, copy them?

YesCT’s picture

@johnshortess please update the issue summary and include the step for testing this manually.

johnshortess’s picture

Issue summary: View changes
johnshortess’s picture

Issue summary: View changes
RavindraSingh’s picture

Assigned: Unassigned » RavindraSingh

I am reviewing this.

RavindraSingh’s picture

I 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 -
HTML markup

Let someone also verify this so keeping needs review only.

RavindraSingh’s picture

Assigned: RavindraSingh » Unassigned
YesCT’s picture

Issue summary: View changes

I'm going to look at this whole thing and see if there is anything left to do.

YesCT’s picture

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

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
39.33 KB
39.33 KB
184.54 KB

multiple file field UI

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

chx’s picture

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

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Setting back to NW according to #51

chx’s picture

Status: Needs work » Reviewed & tested by the community

Don't; I edited it ; you are too fast :) I presume #40 was marked RTBC so unhiding that patch.

RavindraSingh’s picture

@chx, thats correct, we had reviewed #40 patch.

Thanks

xjm’s picture

cilefen’s picture

Status: Reviewed & tested by the community » Needs work

From @alexpott:

// Delay rendering of the buttons, so that they can be rendered later in the
    // "operations" column.
    $operations_elements = array();
    foreach (Element::children($widget) as $sub_key) {
      if (isset($widget[$sub_key]['#type']) && $widget[$sub_key]['#type'] == 'submit') {
        hide($widget[$sub_key]);
        $operations_elements[] = &$widget[$sub_key];
      }
    }

let's turn this around to be

// Delay rendering of the buttons, so that they can be rendered later in the
    // "operations" column.
    $operations_elements = array();
    foreach (Element::children($widget) as $sub_key) {
      if (isset($widget[$sub_key]['#type']) && $widget[$sub_key]['#type'] == 'submit') {
        $operations_elements[] = $widget[$sub_key];
        hide($widget[$sub_key]);
      }
    }

Hopefully swapping that around will mean we won't need to do the show.

johnshortess’s picture

Status: Needs work » Needs review
FileSize
614 bytes

Here'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.

Status: Needs review » Needs work

The last submitted patch, 57: 2502781_57.patch, failed testing.

alexpott’s picture

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

+++ b/core/modules/file/file.field.inc
@@ -121,7 +113,16 @@ function template_preprocess_file_widget_multiple(&$variables) {
+    // We need to use show() here rather than render() - there are multiple
+    // files so this will be processed again.
+    // @todo Fix what marks $operations_elements #printed too early.
+    // See https://www.drupal.org/node/2538710

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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

This looks correct to me.

(But we should definitely try to get rid of hide(), show() and render().)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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

stefan.r’s picture

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

stefan.r’s picture

I don't think I had uploaded any patch there in #62, a drupal.org bug?

stefan.r’s picture

Status: Needs work » Needs review
FileSize
736 bytes
1.66 KB
pwolanin’s picture

I'm confused - the last patch doesn't seem to change the hide()?

alexpott’s picture

@pwolanin see #59

cilefen’s picture

Status: Needs review » Reviewed & tested by the community

It looks to me that all concerns have been addressed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed f802277 on 8.0.x
    Issue #2502781 by tedstein, johnshortess, joelpittet, pwolanin, stefan.r...

Status: Fixed » Closed (fixed)

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