Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call
Problem/Motivation
template_preprocess_views_view_table
calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
- Remove the call by refactoring the code. (COMPLETED)
If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.- To test the conversion to a render array: Edit the content admin page view, and edit one of the fields, and customize the field HTML element (under Style settings)
Remaining tasks
Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123COMPLETEDIdentify 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.COMPLETEDIf the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.NOT APPLICABLE
Manual testing steps (for double escaping)
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Visit the content admin page at admin/content
- Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#71 | 2502089-71.patch | 9.59 KB | stefan.r |
#71 | interdiff-66-71.txt | 3.19 KB | stefan.r |
#66 | interdiff.txt | 764 bytes | MauPalantir |
#66 | remove-2502089-66.patch | 9.36 KB | MauPalantir |
#59 | interdiff.txt | 3.9 KB | MauPalantir |
Comments
Comment #1
joelpittetMay have overstepped on one but let's see.
Comment #3
joelpittetLet's see without the overstep.
Comment #5
joelpittetMade a few typos in the last one.
Comment #6
star-szrComment #7
akalata CreditAttribution: akalata as a volunteer commentedmanually tested this patch; the HTML output is identical.
Comment #12
akalata CreditAttribution: akalata as a volunteer commentedManually tested this patch; the HTML output is identical.
Patch replaces SafeMarkup::set with an inline template in one instance, and with SafeMarkup::format in 3 others
Prep for RTBC once testbot starts working again:
- $label is passed through
SafeMarkup::checkPlain
- content provided by #theme tablesort_indicator is presumed safe
- $element_type comes from
Markup::elementType
, where user-provided elements are passed throughSafeMarkup::checkPlain
- $field_output passed through
HandlerBase::sanitizeValue
as part of $handler->getField (FieldPluginBase::render)Optional separator is passed through
Xss::filterAdmin
- Columns are passed through
Table::sanitizeColumns
at the beginning of this functionComment #13
akalata CreditAttribution: akalata as a volunteer commentedComment #14
akalata CreditAttribution: akalata as a volunteer commentedComment #15
xjmAw, I was hoping this would be another slaying of the preprocess altogether. :) Is there a reason that's not doable here?
Comment #16
akalata CreditAttribution: akalata as a volunteer commentedThere's a lot more going on in template_preprocess_views_view_table() than the SafeMarkup::set, so removing the preprocess completely would be out of scope at the very least (and my intuition says is probably impossible).
Comment #17
xjmSo I was confusing this issue with #2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple() which is the one that got unblocked by the table rendering fix.
I discussed this at length with @akalata and @davidhernandez. They pointed out that because this is a table, converting this logic to template logic would be much more difficult than #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info(), and recommended going forward with this patch (since it's an improvement over HEAD). I asked if there was an existing issue to remove or at least reduce this preprocess function (since I would feel better about committing this with an @todo), and @akalata referenced #1742890: [meta] What will preprocess functions look like with Twig? and #2348381: [META-0 theme functions left] Convert/refactor core theme functions and #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates., but did not find any specific issues about this. @davidhernandez and @akalata also discussed the pros and cons a bit of converting this preprocess to template logic, but agreed that it would be an extensive amount of work.
@joelpittet was talking yesterday in IRC about a long-term hope to improve table theming, so I'd like some more feedback from him on this point as to whether there's a way we can leverage templates more here, or for Views tables in general, or for tables in general, and whether there are followup issues we should reference. Assigning to him at NR for that.
Comment #18
xjmSo, hmm. I just noticed this now that I'm reading the patch more closely.
This is not safe. At least in the context of this line, for all we know,
@element_type
could bescript
.At a minimum, we need a comment to document what @akalata said in her review, but could we actually convert this in particular (and possibly other things in this patch as well) to render arrays? We're already using them elsewhere in this preprocess.
Comment #19
xjmThis is actually a great specific example where "refactor the code flow to make it clear that variables are safe" could also apply.
Comment #20
akalata CreditAttribution: akalata as a volunteer commentedI do see how the combination of
@element_type
with@field_output
could result in an unsafe string, but I think this might be another case where we bump up against the need to accept what we're getting (similar to #2273925: Ensure #markup is XSS escaped in Renderer::doRender() or #2501403: Document SafeMarkup::set in Xss::filter). See also pwolanin/joelpittet's conversation in the latter (#11 and 12).There are a few other SafeMarkup issues where we're struggling with the same idea:
I found a few issues around the discussion of theme_table in general, which would affect this preprocess function:
Comment #21
xjmA difference here versus the issues listed above is that this SafeMarkup call is not internal to the sanitization APIs. This is a theme preprocess function.
It should be straightforward to convert it to a render array. One line of the patch already uses one.
Comment #22
akalata CreditAttribution: akalata as a volunteer commentedI had to call drupal_render manually here, since I don't know enough about how the render API delays rendering until later in the process.
I've updated the issue summary to demonstrate how to test customizable $element_type, which is being supplied to Views UI through views.settings.field_rewrite_elements and probably do not need to be escaped. Combined with the santization checking of the actual field value in FieldPluginBase::render, I think we can consider this result of the
#type => html_tag
to be safe.Comment #23
YesCT CreditAttribution: YesCT commentedwhat would we call this general approach in words?
In the summary it says
"Remove the call by refactoring the code."
So this is ... using SafeMarkup::format with two placeholders and concatenating two strings?
is this one using a render array?
this one is using a render array.
these two are using format() to concatenate two strings.
Comment #24
YesCT CreditAttribution: YesCT commentedI'm not sure if I'm testing the correct thing, cause this causes double escaped html in both head and the patch.
If this patch is supposed to fix this, maybe that makes this a bug and not a task?
....
Or if this is not what this issue is about, I think I need even more specific instructions on what to test. ... and we need a separate issue to fix that double escaping.
Here is what I did:
If this issue is to fix that double escaping that is in head, we should add a test.
Comment #25
akalata CreditAttribution: akalata as a volunteer commentedAddressing #23
This is removing
SafeMarkup::set
across 4 uses in a single function; the specific approach for each is not necessarily identical across each instance.Re #24, I think that the double-escaping in HEAD could make this a bug fix rather than only a task, but I don't think the double-escaping fix needs to be a new issue. Your testing approach is spot-on. I will work on that fix this afternoon.
Comment #26
akalata CreditAttribution: akalata as a volunteer commentedTrying to replicate the double-escaping I noticed a detail that I'd missed -- this patch addresses the SafeMarkup::set around the field element, whereas YesCT's testing revealed double escaping in the label element.
This double escaping is likely coming from a line not already included in this patch,
$label = SafeMarkup::checkPlain(!empty($fields[$field]) ? $fields[$field]->label() : '');
. So now I'm thinking this bug should be a new issue.Comment #27
akalata CreditAttribution: akalata as a volunteer commentedComment #28
akalata CreditAttribution: akalata as a volunteer commentedNew issue created for the double-escaping: #2506485: Test that Header label + customized label wrapper of a Views table display is not double escaped
Comment #29
YesCT CreditAttribution: YesCT commentedok I'll retest.
@akalata opened #2506485: Test that Header label + customized label wrapper of a Views table display is not double escaped for the other bug
Comment #30
YesCT CreditAttribution: YesCT commentedOK. tested this time with the field html.
head, with no change to view, markup:
with patch, with no change to view, markup:
head, with h2, markup:
with patch, with h2, markup:
So, no change. As expected.
As far as I understand, this addresses @xjm #21 asking to use a render array.
Comment #31
YesCT CreditAttribution: YesCT commentedoh, and here is the setting to change to test.
Comment #32
xjmComment #33
xjmComment #34
xjmHm, so if we need the extra
drupal_render()
that won't help. The only time that should probably happen for now is as a workaround for #2505497: Support render arrays for drupal_set_message(). I was thinking more along the lines of "rewrite lots of the whole function and put it in a render array" or such.I really feel like this whole function needs a refactor -- it is doing so much sanitization on its own and concatenating tags together in ways that don't seem correct, especially in a preprocess function. And it's very heavy for a preprocess. The
SafeMarkup::set()
are just the most obvious part of it.I pinged dawehner for feedback on what else we can do here.
Comment #35
dawehnerPuh, so
\template_preprocess_views_view_table
() is basically just a different version of\template_preprocess_views_view_fields
Currently views_view_table() is such complex because we try to have a usable template itself, while ensuring the configurability,
so we could move more into the template but then this would mean more complexity there.
OT: My vision for those bits.
Instead of using configuration and then runtime code to change things in my perfect world, we would use the configuration in order to provide twig templates for themers
to start with. So by default the template is quite complex, but we could make it possible to have a template with the wrapper element types / css classes already built in ... but I fear this is just a vision, which would be way too difficult to implement.
Comment #36
xjmHuh. Thanks @dawehner. So template_preprocess_views_view_fields() is indeed doing many of the same kinda-ishy things with HTML assembly, but without needing all the overuse of SafeMarkup. So this means one or more of three things:
template_preprocess_views_view_fields()
to nix all the SafeMarkup overuse.If point 1 and/or 3 are true, then we could fix this function that way (not just the
SafeMarkup::set()
but all theSafeMarkup
use), file a followup for @dawehner's ideal vision in #35, and add an @todo to the patch referencing that followup.Comment #37
dawehnerI'm aware of #2363423: views-view-fields.html.twig gets escaped which makes the _fields() preprocess function way better to read, and fixes some double escaping issues.
Comment #38
xjm#2363423: views-view-fields.html.twig gets escaped is lovely. Let's postpone this on that.
Comment #39
xjm@Cottser clarified in #2363423: views-view-fields.html.twig gets escaped that theme functions must return strings, which explains why @akalata didn't see a way to use a render array without rendering immediately. I looked closely and the existing render array in the earlier patch is actually being passed to the link generator which (turns out) accepts a render array for the first argument (the fact that it's called
$text
nonwithstanding).So once #2363423: views-view-fields.html.twig gets escaped is done, we can refactor this using that as a model.
Comment #40
xjmComment #41
stefan.r CreditAttribution: stefan.r commentedComment #42
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedComment #43
joelpittet@MauPalantir noticed there was a bunch of class concatenation done in
template_preprocess_views_view_table()
, I've opened a follow-up to clean that up. #2554957: Use Attribute::addClass() instead of CSS class concatenation in template_preprocess_views_view_table()Comment #44
joelpittetStill assigned to @Mau Palantir for review, was working with her on IRC to flesh out some things and though this may not work, I'm hoping it is in the right direction. I await her review!
This takes the markup build-up out of the preprocess and into the template which removes the need to escape/checkplain/set as safe, etc. Also avoids unnatural render arrays.
Comment #45
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedThe TBODY fields content fail to render when patched with #44.
Comment #46
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedHmm, at second glance it seems to work, I think I forgot to clear cache between patches:D (I havent used drupal in a while, so takes time to get back to old routines)
btw it seems like the original implementation had broken the operations menu stuff too...
Comment #48
joelpittetOh that's the ticket! Classy! I knew duplicating templates would bite me in the ass... d'oh!
Currently in head the separator doesn't work at all, but this patch still has an issue where the other cells are not rendered after the separator. Likely because I've not flattened them and it expects to use the flattened result of the previous field.
Comment #49
joelpittetThis makes the links on the header and the sort indicator render correctly.
Comment #51
joelpittetFixes my poor typos.
Comment #53
joelpittetLast failure is due to the stuff I mentioned in #48.
This kinda fixes it but I'm not happy with the approach.
Comment #55
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedThe separator doesnt show up with patch in #53.
It seems like $column_reference['content'] remains empty even if I add multiple fields to the same column at the UI, thus separator doesnt get added to field_output. (line 585)
Comment #56
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedThe label double-escaping mentioned above in the thread is solved.
Comment #57
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedI fixed the missing separator. I did not like the inconsistent naming of content vs field output, so I put field output + separator in 'content' as it was in the original implementation but as proper render arrays.
Comment #58
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedComment #59
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedUmm... where is the patch? D.o thinks its there but not belonging to a comment. weird...
Comment #60
stefan.r CreditAttribution: stefan.r commentedJust out of interest, what still needs to happen here, just manual testing and review? Do we still have the test failure or other todos in the patch?
Comment #61
stefan.r CreditAttribution: stefan.r commentedComment #66
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commentedFix empty table messages (BTW on manual testing the empty message seems to be duplicated in HEAD…).
Comment #67
joelpittetTestbot engage
Comment #68
MauPalantir CreditAttribution: MauPalantir at Cheppers for Acquia commented@joelpittet, otherwise it only needs manual testing.
About the duplication I experienced: it seems that there is a line in views-view.twig that prints {{ empty }} separately, but all the views styles seem to implement printing empty message as the first row.
So either one should be cleared.
The twig output causing this issue was introduced in
https://www.drupal.org/node/2510794
Comment #69
joelpittetCool, maybe @stefan.r can give it a manual test to see if there is anything. Thanks for the solution. I'll also likely do a manual test as well a bit later.
Comment #70
stefan.r CreditAttribution: stefan.r commentedI assume this comes from the style settings, we are sure there is no way there will be a malicious tag in here?
In my manual testing this code didn't trigger, how do we test this with content already in it?
Comment #71
stefan.r CreditAttribution: stefan.r commentedIn manual testing the HTML output looked the same, we just missed the title attribute on the header links.
Comment #72
dawehnerDid you configured the table style settings to include this separator? Note you need to render multiple fields in the same column for that.
Comment #73
stefan.r CreditAttribution: stefan.r commented@dawehner thanks. Just tested that with the patch applied and it worked great.
Comment #74
Wim Leers@stefan.r: So does this need further manual testing?
Comment #75
dawehnerNice!
It always hurts me a bit if we don't share "code" between templates but instead just duplicate the block of template, but well, this is the world of template, in which things are different :)
Comment #76
stefan.r CreditAttribution: stefan.r commented@WimLeers I don't think it does, maybe @joelpittet could have a final look at this but could also be after commit as @dawehner seems to think this patch is ready now
Comment #77
joelpittetre #70.1 It it should and will get escaped in twig unless view decided upstream to mark it as safe, but with our other meta this is unlikely. But also I think these options are chosen not written by the user and you need views UI admin to get at it, so lots of ways it won't cause problems.
Comment #78
joelpittet@dawehner re #75 which "code" are you referring to sharing? I share the same sentiments towards classy duplicating all templates because it needs to add a couple CSS classes... but that's the way things went, waiting for history to decide on that one:P
Comment #79
joelpittetThanks for catching that bit @stefan.r, I missed it!
Could possibly create a column.link_attributes variable. But I think this works well too.
Comment #80
stefan.r CreditAttribution: stefan.r commented#77 the reason I asked is when I hardcoded $variables['header'][$field]['wrapper_element'] = 'script'; it did end up the in HTML...
Comment #81
joelpittetTotally right to be concerned about that. Though my other points should cover the concern. The plugin would have to provide these values to the user so I think we are safe. Also our Xss::filter would need the
<>
to strip it out anyway.Comment #82
alexpottIt's great that we're using Twig to solve this one. Nice work everyone. Committed 65d1748 and pushed to 8.0.x. Thanks!