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.

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

Issue tags: +rc target triage

Tagging for RC triage because this is a security issue and a bit of a regression as we'd already removed all the |raw's.

joelpittet’s picture

Status: Active » Needs review
FileSize
2.75 KB

This should be enough, let's see if testbot and y'all agree.

Cottser’s picture

Issue tags: +Twig
nadavoid’s picture

Status: Needs review » Reviewed & tested by the community

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

lauriii’s picture

I think it might be a good idea to write a comment in the template_preprocess_html for the Markup::create() since that will be a place that many people will see. Does that make any sense?

joelpittet’s picture

Makes sense but what should the comment say?

lauriii’s picture

Probably 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 case

Wim Leers’s picture

This looks great.

joelpittet’s picture

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

Wim Leers’s picture

It's already documented?

 * - placeholder_token: The token for generating head, css, js and js-bottom
 *   placeholders.

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.

joelpittet’s picture

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

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
697 bytes
2.07 KB

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

Wim Leers’s picture

Assigned: Unassigned » alexpott
Status: Needs review » Reviewed & tested by the community

Oh, 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.

lauriii’s picture

Lol nice! RTBC++

alexpott’s picture

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

joelpittet’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Re #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.

alexpott’s picture

If we change this issue to add a comment to the template about why raw is safe then it'd be rc eligible.

joelpittet’s picture

Status: Needs work » Needs review

What is the validity behind this use-case? I'm missing something re #18 @alexpott

joelpittet’s picture

Issue summary: View changes

Added the RC target section to the IS.

alexpott’s picture

The validity is that this never ever ends up in output but is replaced. It is not twig content.

joelpittet’s picture

That is interesting but I still don't understand why it needs |raw. Tests pass without it.

joelpittet’s picture

And like I added to the IS it encourages |raw's use which we are/have been discouraging like the plague since we introduced autoescaping.

alexpott’s picture

Tests 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 just htmlspecialchars() - that does not have to be the case.

joelpittet’s picture

If 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 the source of it rather than the destination. So that we don't encourage usage of |raw in the template level.

lauriii’s picture

xjm’s picture

Issue tags: -rc target triage
Wim Leers’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs review » Reviewed & tested by the community

Seems ready. But too late for either 8.0 or 8.1.

Cottser’s picture

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

lauriii’s picture

Assigned: alexpott » Unassigned

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

Alluuu’s picture

What'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?

joelpittet’s picture

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

alexpott’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

For #34

Cottser’s picture

To me one reason would be that people (myself included) often don't read comments/help text/etc. :(

vilepickle’s picture

Status: Needs review » Reviewed & tested by the community

I'm inclined to like #34 as justification to not change it since it's a placeholder, after all.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

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

tstoeckler’s picture

$placeholder = Markup::create('<' . $placeholder_name . '-placeholder token="' . $variables['placeholder_token'] . '">');

Placeholders can be render arrays right? Can't we do:

$placeholder = [
  '#type' => 'html_tag',
  '#tag' => $placeholder_name . '-placeholder',
  '#attributes' => ['token' => $variables['placeholder_token']],
];

If not, we could still do that, and then $renderer->render() it explicitly.

I might be missing something...

alexpott’s picture

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

alexpott’s picture

So this should work :)

alexpott’s picture

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

alexpott’s picture

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

  public static function randomBytesBase64($count = 32) {
    return str_replace(['+', '/', '='], ['-', '_', ''], base64_encode(static::randomBytes($count)));
  }

But - and _ also will not be escaped so #41 looks like the way to go :)

alexpott’s picture

lol so it seems I've got back to #13. Which means I re-read my comment in #16

Tests 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 just htmlspecialchars() - that does not have to be the case.

Which, thinking about again I still stand by this. The addition of the call to Markup::create() in #27 confused me.

Wim Leers’s picture

+++ b/core/modules/system/templates/html.html.twig
@@ -28,10 +28,10 @@
-    <head-placeholder token="{{ placeholder_token|raw }}">
+    <head-placeholder token="{{ placeholder_token }}">

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

vilepickle’s picture

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the thought put into this everybody.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

  • catch committed bd634aa on 8.2.x
    Issue #2603074 by joelpittet, alexpott, lauriii: Remove |raw from use in...

Status: Fixed » Closed (fixed)

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