Follow-up to #2538950: Replace SafeMarkup::format() in template_preprocess_html with placeholders in the template
Problem/Motivation
We want to avoid encouraging the use of the |raw
filter in templates and we introduced a few back in late just before RC.
Proposed resolution
Use Safestring/Markup or just remove the |raw
filter from the templates.
Why this should be an RC target
Using |raw in templates encourages it's use and could prove to expose security holes. We actively removed these from core and heavily documented the use of SafeMarkup::set()
for this very reason.
Comment | File | Size | Author |
---|---|---|---|
#41 | 2603074-41.patch | 2.07 KB | alexpott |
#41 | 27-41-interdiff.txt | 651 bytes | alexpott |
#27 | interdiff.txt | 717 bytes | lauriii |
#27 | remove_raw_from_use_in-2603074-27.patch | 2.84 KB | lauriii |
#13 | remove_raw_from_use_in-2603074-13.patch | 2.07 KB | joelpittet |
Comments
Comment #2
joelpittetTagging for RC triage because this is a security issue and a bit of a regression as we'd already removed all the |raw's.
Comment #3
joelpittetThis should be enough, let's see if testbot and y'all agree.
Comment #4
star-szrComment #5
nadavoid CreditAttribution: nadavoid at Jackson River commentedLooks good to me. And the
Markup::create
usage in the preprocess function demonstrates a great alternative to using :raw. The only remaining |raw usage in core now is in a test, which is testing how |raw behaves. So this seems good and complete.Comment #6
lauriiiI think it might be a good idea to write a comment in the
template_preprocess_html
for theMarkup::create()
since that will be a place that many people will see. Does that make any sense?Comment #7
joelpittetMakes sense but what should the comment say?
Comment #8
lauriiiProbably something similar we did write for the
SafeMarkup::set
calls. Maybe just explain why its necessary to use Markup there and why we can assume it's safe in this particular use caseComment #9
Wim LeersThis looks great.
Comment #10
joelpittet@Wim Leers any chance you can tell us what is in 'placeholder_token' that needs to be not-autoescaped so we can write docs for this?
Comment #11
Wim LeersIt's already documented?
A token is just… a token. It's there to prevent collisions. E.g. imagine somebody writing an article about how
html.html.twig
works. And they include<css-placeholder>
in that example. Without a token, we'd end up replacing that as well. A (random) token prevents it.See #2538950: Replace SafeMarkup::format() in template_preprocess_html with placeholders in the template for background.
Comment #12
joelpittet@Wim Leers oh I meant why it needs to be not escaped. And the documentation is really around why are we marking as
Markup
. Hmmm, well one part of the inline comment just needs to say 'The output of this variable is non-user generated content.' Which is for the "safeness" factor.The only other bit is why can't that variable be escaped... Are there characters that get escaped that should be HTML escaped? I wonder if we even need to mark it as Markup/|raw?
Comment #13
joelpittetI'm curious, taking it off RTBC to see if we can just let it be escaped... because this would be ideal and best practice in general.
Comment #14
Wim LeersOh, this uses
randomBytesBase64()
! Which means it's URL-safe, which implies it's HTML-safe also.+1
Assigning to alexpott for commit, since he's the SafeMarkup expert.
Comment #15
lauriiiLol nice! RTBC++
Comment #16
alexpottHmmm... So https://www.drupal.org/node/2565021 will need an update. I'm not super convinced about this btw - this was an intentional use of
|raw
and I think a justified one regardless of the content. If we want to stop usage of raw then we need to break http://twig.sensiolabs.org/doc/filters/raw.html and throw an exception. Given the content of html.html.twig I really don't think this is where people are going to learn to theme from.Comment #17
joelpittetWe don't want to stop using
|raw
we just want to seriously discourage it. There could be valid use cases but we should always look up the stack and Mark it as such as close to the source of the input as possible so we know source and it's not unsanitized user input.That's how we've been doing things so far anyway.
Comment #18
alexpottRe #17 well I'm arguing this is a valid use case - these strings never make it to the browser.
Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section
<h3>Why this should be an RC target</h3>
to the summary.Comment #19
alexpottIf we change this issue to add a comment to the template about why raw is safe then it'd be rc eligible.
Comment #20
joelpittetWhat is the validity behind this use-case? I'm missing something re #18 @alexpott
Comment #21
joelpittetAdded the RC target section to the IS.
Comment #22
alexpottThe validity is that this never ever ends up in output but is replaced. It is not twig content.
Comment #23
joelpittetThat is interesting but I still don't understand why it needs
|raw
. Tests pass without it.Comment #24
joelpittetAnd like I added to the IS it encourages
|raw
's use which we are/have been discouraging like the plague since we introduced autoescaping.Comment #25
alexpottTests only pass without it because there cannot be a character that twig auto escapes in it. If there was placeholdering would fail because Twig has altered the placeholders. The point is that Twig should not touch these placeholders at all... ever... under no circumstance, therefore,
|raw
since whilst the current implementation of twig's escape filter is justhtmlspecialchars()
- that does not have to be the case.Comment #26
joelpittetIf that is the case I would suggest recommending wrapping in
Markup()
so that at the very least the value that is intended to be RAW is marked as such at thesource
of it rather than the destination. So that we don't encourage usage of |raw in the template level.Comment #27
lauriiiComment #28
xjmComment #29
Wim LeersSeems ready. But too late for either 8.0 or 8.1.
Comment #30
star-szrHmmm couldn't this potentially break things in preprocess and/or html.html.twig templates if we change this from being a string to a Markup object?
Comment #31
lauriiiGiven how unlikely it would be to modify this anywhere I'd say we can go with this.
Should we still add tests that |raw filter works with Markup objects?
Comment #32
Alluuu CreditAttribution: Alluuu commentedWhat's the alternative to using raw filter if I need to get the raw value in the template for field's I know only an admin can have input?
Comment #33
joelpittet@Alluuu I wish I had a succinct answer for that. My recommendation is ensure the marking of what kind of data it is as close to the source as you can get it. That way you can sanitize it if needed and you are more confident knowing the value has come from a source you expect it too and what kind of data it is. The template can be sent values from different sources, maybe not likely but it depends which modules or themes are using the templates. Wrapping the value in a Markup() object early as you can get it will help remove that distance from the source and the output use.
Hand waving answer;) Hopefully that helps a bit more around the thought process though... at least mine.
Comment #34
alexpottI'm still not a fan of this change - we changing from one thing that is discouraged to another thing that is discouraged. Why don't we just add some comments in the templates to explain the use of
|raw
and explain why is it wrong in nearly all other circumstances.Comment #35
alexpottFor #34
Comment #36
star-szrTo me one reason would be that people (myself included) often don't read comments/help text/etc. :(
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm inclined to like #34 as justification to not change it since it's a placeholder, after all.
Comment #38
alexpott@vilepickle - did you mean to rtbc this change? Your comment sounds like you support my comment in #34 but the action of rtbc-ing sounds like you think we should commit this patch. Setting back to needs review for clarification.
Comment #39
tstoeckler$placeholder = Markup::create('<' . $placeholder_name . '-placeholder token="' . $variables['placeholder_token'] . '">');
Placeholders can be render arrays right? Can't we do:
If not, we could still do that, and then
$renderer->render()
it explicitly.I might be missing something...
Comment #40
alexpott@tstoeckler seems a shame to invoke the render system to do that.
In my opinion this is a valid use of raw. HOWEVER, I think we might be missing something. I think we can just remove the
|raw
because theplaceholder_token
variable is created using$variables['placeholder_token'] = Crypt::randomBytesBase64(55);
and I'm not sure that can ever create a character that needs escaping...Comment #41
alexpottSo this should work :)
Comment #42
alexpottI think the reason we originally added the
|raw
is that the template was using code like{{ head }}
to print the entire placeholder - however now we build most of the placeholder in the twig template and only add the token it might be unnecessary.Comment #43
alexpottNone of the characters listed on https://stackoverflow.com/questions/9752050/what-characters-uses-base64e... should be escaped... we do stuff to make it filename safe...
But
-
and_
also will not be escaped so #41 looks like the way to go :)Comment #44
alexpottlol so it seems I've got back to #13. Which means I re-read my comment in #16
Which, thinking about again I still stand by this. The addition of the call to Markup::create() in #27 confused me.
Comment #45
Wim LeersRegardless of which syntax/implementation we end up using: what we're doing here is printing an attribute value.
Because it's an attribute value, the value that Twig ends up printing must conform to the requirements of a HTML attribute value.
Hence I think Twig escaping this is fine. Because if Twig's escaping would result in a different value, then it would already result in broken HTML anyway.
In other words: it's not Twig's escaping or lack thereof that must guarantee things work. It's the placeholder token itself that must guarantee this, which means it must be restricted to characters allowed in HTML attribute values.
Therefore I think #13 and #41 (which are identical!) are both correct.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott: Sorry, didn't realize the RTBC would signify the patch review, guess I should've thought that out. But yeah I agreed with your assessment.
Comment #47
joelpittetThanks for the thought put into this everybody.
Comment #48
catchCommitted/pushed to 8.2.x, thanks!