Problem/Motivation
#1825952: Turn on twig autoescape by default introduced autoescape and with it a mechanism to mark strings as safe. This works great as long as render arrays are used or only t() functions, but it begins to be a problem once user input is involved that needs to be concatenated or modified in some way:
Consider the following code:
$build['#markup'] = SafeMarkup::set('<span class="x">' . $my_maybe_secure_value . '</span>');
The problem here is that $my_maybe_secure_value might not be known if its secure at the moment. A way to mitigate that is to use:
$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::check($my_maybe_secure_value) . '</span>');
but that NEEDS to be done or a XSS will happen. We also discourage direct usage of SafeMarkup::set() as it is marked as internal only function, so we need something better.
This can get complicated very fast:
$summary_escaped = '';
$separator = '';
foreach ($summary as $summary_item) {
$summary_escaped .= $separator . SafeMarkup::escape($summary_item);
$separator = '<br />';
}
$field_row['settings_summary'] = array(
'#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . $summary_escaped . '</div>'),
);
and escaping gets to be something that is difficult, discouraged and an uneasy task.
We could use for this particular example (if SafeMarkup::escape supported arrays - it does not):
$summary_escaped = SafeMarkup::escape($summary);
$field_row['settings_summary'] = array(
'#markup' => SafeMarkup::set('<div class="field-plugin-summary">' . implode("<br />", $summary_escaped) . '</div>'),
);
which is way shorter, but still uses SafeMarkup::set directly, which module authors should never think about.
This would not a problem if the whole section used a template as we then would be getting the autoescape for free and the BIG and BEST solution is to move all those little markup things into a larger encapsulating template.
However we need something in between for until more code can be converted.
Proposed resolution
This issue proposes to introduce a new #type called twig_inline that allows safe creation of markup without calling SafeMarkup::set() directly. This also decreases the chance of single tags getting marked as safe as render arrays are usually just passed up un-rendered to the top page level.
(at least once the SafeMarkup::set() within drupal_render uses the $recursive flag)
Before:
$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::escape($possible_unsafe_var) . '</span>');
After:
$build['string'] = array(
'#type' => 'inline_template',
'#template' => '<span class="x">{{ var }}</span>',
'#context' => array(
'var' => $possible_unsafe_var,
),
);
This also has the advantage that we store less #markup within render arrays, which we also should discourage. While before the render array was only changeable with either re-creating the value or using str_replace, now the template can theoretically still be changed in larger encompassing templates or preprocess functions, which is a slight theming win, too. For forms a simple form_alter() can also be enough.
The more complicated example looks then like:
$field_row['settings_summary'] = array(
'#type' => 'inline_template',
'#template' => '<div class="field-plugin-summary">{{ summary|safe_join("<br />") }}</div>',
'#context' => array('summary' => $summary),
);
safe_join is needed here which does automatic escaping before implode - comparable to the SafeMarkup::check() originally done. We had been debating to directly have |join always escape and be a safe filter, but that discussion is still ongoing.
As already discussed in the Motivation section the best would be to move as much of those to larger encompassing structures and templates, but this is probably the best we can do in the short term.
Remaining tasks
* Reviews and discussions.
User interface changes
* None.
API changes
* Addition of #type => 'twig_inline'.
Original report by Fabianx
I wondered if we should allow simple twig inline templates:
Before:
$build['#markup'] = SafeMarkup::set('<span class="x">' . SafeMarkup::escape($possible_unsafe_var) . '</span>');
After:
$build['string'] = array(
'#type' => 'inline_template',
'#template' => '<div>{{ var }}</div>',
'#context' => array(
'var' => $possible_unsafe_var,
),
);
That gives us autoescape for free and is compiled and cached to disk like normal templates, so pretty performant and not more difficult to use than t().
Comment | File | Size | Author |
---|---|---|---|
#62 | interdiff.txt | 6.38 KB | dawehner |
#62 | 2289999-62.patch | 13.71 KB | dawehner |
#45 | 2289999-45.patch | 13.7 KB | dawehner |
#37 | interdiff.txt | 2.96 KB | dawehner |
#37 | 2289999.patch | 13.7 KB | dawehner |
Comments
Comment #1
dawehnerA couple of questions:
Comment #2
Fabianx CreditAttribution: Fabianx commentedComment #3
Fabianx CreditAttribution: Fabianx commented#1:
> Would that compile the string to disk and just load it using some hash?
Yes!
> In case it cannot be compiled is there any other way that twig does not have to parse the string on each corresponding request?
It is compiled to disk, so this is not a concern.
> OT: Did we considered #2300737: Register twig service as part of core.services.yml before ?
I don't think we had considered it. I guess it can just be moved over?
Comment #4
dawehnerThis turned out to be easier than thought, tbh.
Comment #6
dawehnerComment #7
YesCT CreditAttribution: YesCT commentedComment #8
YesCT CreditAttribution: YesCT commentedoops.
Comment #9
chx CreditAttribution: chx commentedI like Yoda conditions and the De Morgan's laws as much as the next guy but I rather prefer not having them in core. That said, this is ready, my changes are cosmetic. Also this blocks an uncounted number of issues so bumping a bit.
Comment #10
chx CreditAttribution: chx commentedHowever, this doesn't create the render element in the IS so with a heavy heart resetting to CNW.
Comment #11
dawehnerThere we go.
Comment #12
chx CreditAttribution: chx commentedLovely!
Comment #14
tstoecklerShouldn't the test be doing a drupal_render()?
That said also having a test that tests renderInlineTemplate() seems like a good idea, as well.
Comment #15
star-szrThis is awesome! Can we use Twig coding standards in the test please? Spaces inside the curly braces,
{{ llama }}
instead of{{llama}}
:)Test failure is unrelated I think, had it on another patch last night.
Comment #16
dawehnerThanks @tstoeckler, cottser
It is impressive how much you can do wrong.
Comment #17
tstoecklerWell, none of us caught that from looking at it, so.... ....yay test coverage! :-)
Comment #18
Fabianx CreditAttribution: Fabianx commentedThis is wonderful!!! Thanks so much dawehner!
RTBC + 1
Do we need a change notice for this? Update an existing one?
Comment #19
catchWon't this break if a site uses read-only PhpStorage and builds Twig templates as part of a build step? That's been proposed as one of the options for dealing with preventing writing PHP in production, not sure how you'd get past this (except for executing all the code that renders a template inline).
Comment #20
star-szrSmall coding standards fix. Somewhat crossposting with #19, I don't know the answer to that.
Comment #21
Fabianx CreditAttribution: Fabianx commented#19:
I think there are several possibilities:
a) Detect all twig inline templates, similar to how we can find t() strings - good DX, difficult but manageable build script; good performance.
b) Instead of cache to disk, cache to memcache or similar for inline templates if storage is read-only. Won't be as fast due to missing APC support, but still faster than compiling - good DX, less performance
c) Register inline templates centrally and use an index into that table - worse DX, but easy to find.
I am for a) as '#template' => 'abc', should not be too difficult to find within source files.
Comment #22
dawehnerWe do have one already: https://www.drupal.org/node/2306083
Once we require this, it seems to be not far away from just writing a full template, tbh.
Does that mean we don't have to care about that for core now? This would be a problem of people which don't want to use PhpStorage, which is part of core already.
Comment #23
chx CreditAttribution: chx commented> This would be a problem of people which don't want to use PhpStorage,
No, these people would use phpstorage but probably not the mtime variant -- for them the writing happens on staging and then the generated files get deployed to production so that production doesn't need writing PHP files.
But catch missed a crucial step here: we do not have anything to ensure all templates get generated on staging. That's left as an exercise to such developers already . They will likely need to visit a lot of pages or somehow get Twig to compile the templates. Now this mythical script will need to search for #template as well. My hand is up writing this script provided it doesn't need to happen this month.
Setting to CNW for adding a comment on renderInlineTemplate to not use it. You can always achieve what you want with a render array plus #template . Also to comment that #template must be a constant much like
t()
as Fabianx notes. Once #1617948: [policy for now] New standard for documenting form/render elements and properties happens we can put that comment on the element too.Comment #24
dawehnerIs this documentation good enough?
Comment #25
chx CreditAttribution: chx commentedThat will do for now then when the new standard comes I will move it in place with an example.
Comment #26
catchYes PhpStorage but read-only is what I should have written.
This needs a 'why'.
I can live with the 'you have to do some extra work to compile inline templates' but can we please open an explicit follow-up. Compiling a bunch of stuff that you know lives in a finite number of places is a much easier task than this.
Comment #27
Fabianx CreditAttribution: Fabianx commentedProposal for extending that comment:
Comment #28
Fabianx CreditAttribution: Fabianx commentedComment #29
dawehnerI really liked the suggestion from Fabianx, so here is a patch with it.
Comment #30
chx CreditAttribution: chx commentedFiled a child issue as catch asked; now both his comment and his issue is in place, let's try it one more time!
Comment #31
andypostauto-escape is on now, suppose test should check that too
Comment #32
Dries CreditAttribution: Dries commentedIt's not clear what the real reason for this change is. Is it performance? Is it DX? (I personally find the 'Before' easier to understand/read/write than the 'After'.)
I'd love to see a couple of real use cases in the patch; some conversions in the patch would make it easier to evaluate.
Comment #33
chx CreditAttribution: chx commentedComment #34
RainbowArrayI'm really not a fan of this.
We've been doing a lot of work to convert theme functions to use Twig templates instead. That has some great benefits for themers, because they can find the appropriate Twig template with Twig debug and copy it into their theme if they want to override the markup.
With this, markup will be back in functions once again, and not necessarily in a place where it's possible to override the markup. That means themers digging through PHP in order to find out if there's any way to override markup. It's extraordinarily annoying right now when you find markup that's hard-coded in some contrib module in a way that's nearly impossible to override without hacking the module. This seems like it would make it even more likely for that to happen.
The particular example is with a render array, so I may be completely off base here, as that should be something a themer can modify, although probably only with digging into PHP. And there may well be other instances of markup making its way directly into render arrays rather than going through a template. So I might be completely off-base. If I am, please ignore me. Just sharing my initial impression.
Comment #35
chx CreditAttribution: chx commentedAre you ready to mandate a separate Twig template then for every instance of #prefix and #suffix in core or similar?
Comment #36
RainbowArrayLike I said, I might not be aware of everything that's going on. If there's already other markup not in file-based Twig template, then my concerns are moot.
Comment #37
dawehnerConverted two places: One which is pretty straightforward: views UI, and one which can replace a foreach loop
Comment #38
Fabianx CreditAttribution: Fabianx commented#32: I will update the issue summary.
#34: You have a point here, but the problem is that so much HTML is still hardcoded in core, but it is difficult to just move that to larger encapsulation templates with ease and we need something that is still discouraged, but not as difficult or unsafe to use as SafeMarkup::set(). I'll explain in the issue summary.
#37:
This should be using safe_join however, to do the same as the loop above.
Comment #39
marcvangendAs a site-building themer with development skills (doing a bit of everything), I am in favor of this patch.
In reply to mdrummond's concerns: Developers who don't care about templates, overrides and doing things The Drupal Way™, can already use
'#markup' => '<div>foo</div>'
. I agree we don't want to see that kind of code in core or contrib, but discarding this patch will not stop anyone from doing it wrong. Meanwhile the patch will reduce the chance that markup-in-code introduces security issues, so that's a win.Dries: To me, the main advantage of this patch is the DX when writing custom code. In D7, #markup has proven to be a really useful tool in my custom modules. I use it for rapid prototyping (just whack the HTML mockup in there, so the client can see it in the demo, and refactor later), for quick fixes (sometimes every minute counts), or simply when you're certain that your output does not need to be overridden. This patch would allow me to combine that with the advantages of twig.
Comment #40
Fabianx CreditAttribution: Fabianx commentedI updated the issue summary to include more motivation and discussion of the various points.
Comment #41
star-szrThe example in #37 (Field UI) makes it very clear to me the benefit of this. Nice one @dawehner :)
I don't think
{%
prints anything, I think for all these since they are short you want something like:Also we shouldn't add additional whitespace inside the strong tags for these, it wasn't there before.
Comment #42
star-szr…well we usually use single quotes, so
{{ 'Query'|t }}
:)Comment #43
LewisNyman CreditAttribution: LewisNyman commentedIsn't this what template suggestions or twig blocks are for?
The first objective of #2289511: [meta] Results of Drupalcon Austin's Consensus Banana is to move mark up into templates and out of PHP. Maybe it needs to be possible to do this to cover custom module edge but we shouldn't encourage this in core or contrib because this causes massive pain, requiring themers to wade through php and learn a load of Drupalisms to change markup.
Comment #44
star-szrFrom the issue summary:
This is a stepping stone and way better than #markup IMO.
Comment #45
dawehnerIt does work without the print statement:
https://www.drupal.org/node/2289999#comment-8994155 also explained why this should be useful in general during development.
Comment #46
Fabianx CreditAttribution: Fabianx commented#45: Could you still address using safe_join instead of join? Thanks! (explained in #38)
Comment #47
chx CreditAttribution: chx commentedComment #48
LewisNyman CreditAttribution: LewisNyman commentedAgreed. Thumbs up as a stepping stone.
Comment #49
dawehnerGreat that you agreed.
@Fabianx
It is up to you to RTBC again.
Comment #50
star-szr#45, thanks for that. Didn't know that :) I double checked #2040089: Support for Drupal 8 twig %trans% template translatables and it supports that syntax so that works.
Comment #51
RainbowArrayThumbs up to RTBC.
Comment #52
Fabianx CreditAttribution: Fabianx commentedRTBC
Comment #53
tim.plunkettCan we get some clarification that this is an optional change, and not everyone will be forced into this? What horrible DX...
Comment #54
alexpottAlso I think we need a change record for this.
Comment #55
alexpottComment #56
dawehneras written in IRC I disagree with making #safe_markup ... we should better discourage people from writing unsecure code, always,
but rather encourage to write proper templates.
Based on this for me it would be fine with having no #safe_markup functionality or something similar at all.
Comment #57
chx CreditAttribution: chx commentedEdit: nevermind, I forgot my place. It is not mine to argue anything any more.
Comment #58
chx CreditAttribution: chx commentedComment #59
joelpittetI really like a bunch of things in this patch. Nice work @Fabianx!
Don't want to derail this too much but was wondering your thoughts on something a bit more inline with the way twig does inline templates?
Do these inline templates need to go through the drupal_render pipeline?
FYI I'm referencing/plugging my quick loader swap hack from the Twig lab in Austin:
https://github.com/DrupalTwig/axe/blob/master/src/Controller/AxeControll...
And would love to avoid any new #type/#theme/render array if possible:)
Comment #60
joelpittet#59 never mind #59 I just realized I can do what I was asking...
And do share the DX concerns but mostly like I said before, I don't like #type. But this looks like an improvement to me:
@FabianX could I propose renderInlineTemplate be named renderInline()? Because render() expects a template and I'd like to use that method directly(selfish i know) for some fun. And 'twig_inline' be renamed to 'inline_template'?
No worries if you disagree, just thought I'd ask.
Comment #61
chx CreditAttribution: chx commentedWhat this patch offers (not because I want to disagree but because it seems this is not clear to others): since the template+variable is not fused into a string, the "clean 1/3" from #2289511: [meta] Results of Drupalcon Austin's Consensus Banana can write a theme which alters #template to not contain div etc.
Of course, everything in a template is better. But, see #38 on the feasibility.
Comment #62
dawehnerDone
Comment #63
Fabianx CreditAttribution: Fabianx commentedI am happy with the rename, we still need a change record.
#60: You can do whatever you please, but please bear in mind the translation tools might need other changes to also work with ->renderInline() - that is why we discourage direct usage at the moment.
There is two possibilities for the simpler cases to be _clean_ for themers if one does not write a clean template from the beginning.
Anything else will not allow a themer to change the output properly, but we still want to discourage usage of SafeMarkup::set as much as possible.
So if someone says this is 'horrible DX' or "don't like this", they need to fix their module to work properly with templates, because what they had before was horrible TX (themer experience)!
If you do it right from the start, you will never need to use this and writing object oriented templates and using suggestions should not be more difficult than writing object oriented classes.
Summary
This approach is secure by default, more themer friendly than before, achieves the 1/3 of clean-markup-themers goal and you will never need to use this if you do it right.
It is a stepping stone.
Comment #64
dawehnerHere is one: [#2311123]
Comment #65
Fabianx CreditAttribution: Fabianx commentedComment #66
joelpittetRTBC++ thank you @Fabianx and thanks @dawehner for the change record. I tweaked the title and linked it up. Pity change records don't magically convert to urls like issues do. Here's the link.
https://www.drupal.org/node/2311123
Comment #67
dawehnerIt was a pleasure to write the change record ;)
Comment #68
chx CreditAttribution: chx commentedSimilarly to rewrite it ;)
Comment #69
Fabianx CreditAttribution: Fabianx commentedThanks, chx!
Comment #70
tstoecklerNot sure how relevant this is but AFAIK the following snippet in #63 is incorrect:
I'm pretty sure that the
SafeMarkup
call is in fact not needed for#prefix
and#suffix
. See also line 3279 of common.inc indrupal_render()
Comment #71
Fabianx CreditAttribution: Fabianx commented#70, you are right - I missed that, but that was probably an oversight as we either:
a) debate whether #prefix, #suffix should be considered safe similar to #markup vs. #safe_markup
b) Add a SafeMarkup::check() on it during drupal_render()
We will probably address as part of "Check and document all SafeMarkup" calls in core, though, so for now this leaves another option - if #markup is checked currently in drupal_render() - if its not that's a security issue again again in the issue summary and another reason why "inline_template" is just safer to use as it guarantees the escaping in an unobtrusive way.
Comment #72
Fabianx CreditAttribution: Fabianx commentedAs per #70, code would currently need to be:
as we currently do not escape #markup. (@see #2273925: Ensure #markup is XSS escaped in Renderer::doRender()) and #prefix, #suffix.
I did just in my example, expect it to be safe, so I fell into the security trap as well, which as already said is another good point for 'inline_template' and #markup being escaped by default. If even core devs that wrote the autoescape part, can't get this right (aka me in this case) ...
Comment #73
andypostThe prefix/suffix primary used in forms to add wrapping DIV with ID to allow core's ajax work.
IMO this example is more confusing then the issue.
Can someone explain how this would help to fix all double-escape issues that postponed on this one?
Would be great to provide example based on #2309929: HTML double-escaping in field forms
Comment #74
effulgentsia CreditAttribution: effulgentsia commentedSince this is RTBC again, assigning the commit evaluation to Dries due to #32, but that doesn't need to stop additional feedback and responses by others in the meantime.
Comment #75
catchI think Dries' concerns were adequately addressed in the remaining comments, and this is blocking all the double-escaping issues at the moment, so I'd suggest we unassign on Monday if there's no feedback from Dries by then.
Comment #76
Dries CreditAttribution: Dries commentedThank you for the sample conversions and the excellent issue summary and change record. I agree that removing usages of SafeMarkup::set() and required calls to SafeMarkup::check() is a DX win, and helps people avoid security bugs from forgetting to SafeMarkup::check().
I don't think that inline Twig strings inside of PHP code is good DX, but it seems like everyone here agrees that the solution to that is to move that markup to template files in followups as we're able to, and that works for me.
I haven't reviewed the changes to TwigEnvironment in detail, so I'm unassigning myself so that catch or alexpott can commit this once they've done that.
Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat if I don't use Twig? Changing templates is one thing but changing the code is something totally different. Though I understand why this is needed I don't think this is a good solution:
How about something in this direction:
+ parser for each template engine that, in case of Twig, would change
'<span class="x">[mytoken] or [mytoken|t]</span>'
into'<span class="x">{{ mytoken }} or {{mytoken|t }}</span>'
and in case og phpengine to'<span class="x">mytoken or t(mytoken)</span>'
.Comment #78
dawehner@ivanjaros
This is not the point. The twig services will be always available in the DIC. You don't and actually can't directly replace this snippets inside your theme.
Beside from that, building an abstraction over template engines is well described in http://xkcd.com/927/ . You could also easily build a parser for twig and convert it
to your wanted language instead of inventing a new one.
Comment #79
alexpottCommitted 76608ff and pushed to 8.x. Thanks!
Fixed on commit.
Comment #81
Gábor HojtsyI see there was minor discussion of how this is compatible with translations. Looks like both the inline_template and the renderInline() would need specific support in potx. We are not looking at PHP files for Twig translatable text of course. So we need reliable ways to identify these template fragments and then parse them as Twig. See #2315329: Support for Drupal 8 inline twig translatable, help appreciated there in figuring out what are the possible forms. The change notice explains two forms: https://www.drupal.org/node/2311123, can we rely on such specific things as an array defined all at once with type key before the template key? Ie. if you define it the other way around it will not be translatable? Consider we need to statically parse this PHP code to then statically parse the Twig code. See? Discussion should happen in #2315329: Support for Drupal 8 inline twig translatable, thanks again.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedSpecifically, the change notice says:
But, the PHPDoc for renderInline() says:
So, what's an example of an "unavoidable" situation that the change notice is referring to that would justify violating the PHPDoc of the method? If there is one, we should probably add that information to the PHPDoc and change record. If there isn't, then that's one less bit of work for #2315329: Support for Drupal 8 inline twig translatable.
Comment #83
dawehnerMeh, I just didn't wanted to write the same string twice. The situation feels similar to #attached and _drupal_add_css.
Comment #84
larowlanNote #2316941: Use the builder pattern to make it easier to create render arrays