Problem/Motivation
From a Twig template, you can call:
{{ var|placeholder }}
outside of a {% trans %} statement and we map the placeholder Twig filter to the vendor 'twig_raw_filter' PHP function.
Proposed resolution
Since t() and SafeMarkup::format() have changed, we really should just use a filter that is not marked as safe instead, like twig_no_op.
OR
Remove placeholder and passthrough as functions, so that it works like SafeMarkup::format() again.
Remaining tasks
TBD
User interface changes
n/a
API changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#48 | interdiff.txt | 1.67 KB | lauriii |
#48 | twig_placeholder_filter-2495179-48.patch | 5.7 KB | lauriii |
#45 | 2495179-45.patch | 5.72 KB | dawehner |
#45 | interdiff.txt | 961 bytes | dawehner |
#41 | interdiff.txt | 4.1 KB | dawehner |
Comments
Comment #1
star-szrI don't personally understand the following from #2472731-51: Twig problems with 1) translation filters 'passthrough' and 'placeholder' and 2) HTML 'escaping' but adding as a proposed resolution anyway :)
Comment #2
Gábor HojtsyComment #3
Gábor HojtsyComment #4
Gábor HojtsyWhere is this currently done and how would we go about this? I am not familiar with Twig at all to be able to do this unfortunately but concerned about the lack of security hardening here.
Comment #5
Gábor HojtsyComment #6
Gábor HojtsyLooking at TwigNodeTrans::compileString(), it does not really matter what the filters are mapped to does it? It is just their names that matter. Is this true?
Comment #8
Gábor HojtsySo looks like that is not how it works. Halp!
Comment #9
Gábor HojtsyTweeted about this as a last resort to get some help :/ https://twitter.com/gaborhojtsy/status/608167337469091841
Comment #10
chx CreditAttribution: chx commentedSome observations:
Twig_SimpleFilter::__construct function arguments are $name, $callable, array $options = array(). As Twig does not require PHP 5.4 it does not type hint with callable but the argument name is telling.
Indeed twig_no_filter is a callable, a function defined in core/vendor/twig/twig/lib/Twig/Extension/Escaper.php line 108.
OTOH git grep twig_no_op comes back empty. There's no way that would work: the Twig compiler in Twig_Node_Expression_Call compiles a function call via compiler raw so it's passed right through into the compiled template as it is and will fatal immediately when PHP calls it.
You need to define a twig_no_op function if that's what you want:
I do not comprehend the issue fully yet so I am emphasizing the if that's what you want part.
Or, you can copy the pattern of
new \Twig_SimpleFilter('render', array($this, 'renderVar')),
in the getFilters function you are patching and have amethod.
Comment #11
chx CreditAttribution: chx commentedLet me emphasize this: I am under the effect of multiple painkillers and so I have not considered anything security wise. I have pointed out the Twig bits as that's what the helping cry seemed to be for but I have no idea whether any of the suggestions in #10 are secure. Security requires thinking, explaining Twig integration doesn't and thinking is not something I am capable of ATM.
Comment #12
Gábor Hojtsy@chx: I went by Cottser's suggestion that we should use twig_no_op. OTOH a search for twig_no_filter comes up empty for me. There seems to be a twig_raw_filter() in Escaper.php. It says "Marks a variable as being safe" which is what we are trying to avoid. Otherwise you attempt to use the placeholder filter and you assume it does something for your security (like within t) but it does not. Maybe we need a no-op then that returns an empty string? Not sure how easy it is to debug that then.
Comment #13
star-szr@Gábor Hojtsy yeah I just copied and pasted @Fabianx's comment from the other issue; I didn't know whether
twig_no_op()
existed or not. @chx is right on about the Twig integration parts! Not quite awake yet enough to be any more helpful than that.Comment #14
Fabianx CreditAttribution: Fabianx as a volunteer commented#6: chx is right, just need to define the function in twig.engine.
Sorry for the confusion, @all.
Comment #15
Gábor Hojtsy@Cottser, @Fabianx: right, but what we do now is we channel these through raw output. We would need to use a callback that does "whatever twig would do if you did not have a filter at all". Is there a callback for the behavior that would happen if you don't use a filter? :)
Comment #16
Fabianx CreditAttribution: Fabianx as a volunteer commented#15: That is not needed as the filter is essentially never called.
The code always converts any twig variables printed inside the thing into a t() function directly.
So just defining twig_no_op in twig.engine should make it pass.
Comment #17
Gábor Hojtsy@Fabianx: in #15 I asked what goes inside that callback if there is not one already? I think it should do the same as if no filter was called (ie. escape), and not return an empty string, because the later would be quite a bit harder to debug IMHO. Of course those actually using Twig may have a different opinion on the expected behavior which I hoped to grab with my tweet / waiting on this issue, but neither proved that people have a strong opinion either way.
Comment #18
webchickEscalating to critical; we have a triage call tomorrow where we can discuss this.
Comment #19
Fabianx CreditAttribution: Fabianx as a volunteer commentedLets define:
in twig.engine then.
The important thing is that placeholder and passthrough use this filter and that in the definition this filter is _not_ defined as safe, which was the problem why it was potentially security problematic before.
Comment #20
Fabianx CreditAttribution: Fabianx as a volunteer commentedThis is the important part.
Theoretically could even continue to use 'twig_raw_filter' just not with 'is_safe' set.
That is what makes it problematic.
Comment #21
Gábor Hojtsy@FabianX: right, we twig_raw_filter is already there and doing that. Entirely new patch using that then. Needs tests.
Comment #23
Gábor HojtsyComment #24
lauriiiNow instead of "Placeholder: %placeholder" it generates "Placeholder: @placeholder"
Comment #25
Fabianx CreditAttribution: Fabianx as a volunteer commentedYeah, needs some TwigNodeVisitor love to go one node further down because now there is the twigDrupalEscapeFilter in the chain.
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn #2282101-16: Remove |passthrough filter in Twig, we seem to have agreement on removing the
|passthrough
filter from core entirely (not the|raw
one, but that's irrelevant for this issue). Should we also remove|placeholder
? Its only core usage is inupdate-project-status.html.twig
and is:a significantly better themer and translator experience than:
?
I don't want to add scope-creep to this issue if the filters can be made secure without much work. Just wondering if removing would be the more desired and faster path forward.
Comment #27
Fabianx CreditAttribution: Fabianx as a volunteer commentedI think it is good to keep placeholder for '%' styling, it just needs some work on the TwigNodeVisitor to look one Node level deeper for the filter.
Comment #28
Gábor Hojtsy@effulgentsia: We don't do t('Some module name') either in Drupal for the sanity of translators and for the customizability of placeholder markup for themers. Trans just transforms into a t(), so if we use internal markup like that there, why not be consistent with t() and abandon the % modifier? If we don't abandon that in t() then we should not abandon it here either for consistency IMHO.
Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer #18, we discussed on the committers' triage call, and decided that it is indeed critical.
To be honest, I wasn't convinced at first. After all, this isn't a vulnerability on its own, it requires a Twig template to print out
{{ foo|placeholder }}
outside of a{% trans %}
statement, and not every possible pathway to Twig printing XSS is a critical bug. For example, per #2282101-18: Remove |passthrough filter in Twig, we're still considering to leave{{ foo|raw }}
as a supported capability.But, the difference is that using
|raw
is known to be a risky filter, whereas there's no reasonable expectation for|placeholder
to be, especially to be inconsistently so depending on what twig commands it's inside of.Comment #30
dawehnerLet's first put up test coverage so that we know what should actually happen.
Comment #31
dawehnerLet's first put up test coverage so that we know what should actually happen.
Comment #33
dawehnerThere we go. here is a solution I think fabian had in mind.
Comment #35
dawehnerWe we need to check for the proper HTML.
Comment #36
Gábor HojtsyThe /em should be part of the substr_count(). While the test returns the proper result this way too, that seems to be because 2 . '</em>' equals 2.
Comment #37
dawehnerGood point
Comment #38
Gábor HojtsyMinor fixes:
1. Wrapping of comments.
2. Unused use around the area we touch.
3. Accidentally disabled test methods.
I also looked at whether hardcoding the placeholder markup like this is fine, but SafeMarkup does the same thing, so yeah.
Comment #39
dawehnerThank you gabor for quickly fixing those bits.
Regarding part 3) In Drupal 7 we used to have
theme_placeholder()
but that caused a lot of problems, because you can easily have a t() call really early in the bootstrap,which then initialised the theme system, before it was ready to do so, so
theme_placeholder()
got removed.Let's assign to fabian to ensure that the approach taken here is sane.
Comment #40
Fabianx CreditAttribution: Fabianx as a volunteer commentedOverall the idea is nice.
However we can do better and don't need the node visitor changes.
- Make 'placeholder' use ($this, escapePlaceholder) and leave if as 'safe'.
- Remove 'escapePlaceholder'
- Remove node visitor changes
=> during twig trans, there is no additional filter - except for the output one, which is handled, so its detected as %placeholder. => GOOD
=> outside of twig trans, placeholder is executed, which adds the nice [em] markup AND then goes to auto-escape it => GOOD
=> SOLVES the issue => VERY GOOD
The reason, why this was uhm, interesting is that we marked something as safe and then do not escape it, which is usually auto-escaped ...
By using the new function, we restore sanity in all cases. We also ensure markup is always the same.
Great work, dawehner!
Comment #41
dawehnerAh that is much nicer!
Comment #44
chx CreditAttribution: chx commented- public function testTwigTransTags() {
+ public function ptestTwigTransTags() {
leftover debug?
Comment #45
dawehnerThank you for pointing that out. I guess it should that automate sort of locally ...
Comment #46
lauriiiNow there is mixed use of array() and [] on a single line. Could we change all the arrays to use [] on that line?
Comment #47
dawehnerSure
Comment #48
lauriiiJust a very minor changes for the patch. Otherwise this looks very clean, fixes the concerns on the issue and adds test coverage for that.
Comment #49
dawehnerHa, I actually had a patch but just forgot to upload it :)
The interdiff in #48 looks totally fine.
Comment #50
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1, nice patch!
Comment #51
alexpott#48 introduces inconsistency within the
twig_theme_test_theme()
method.TwigExtension::getFilters()
already has an inconsistent line. So #45 doesn't introduce any new inconsistencies whereas #48 does. So I'm committing #45 since array syntax is not a reason to hold up a critical.Committed 83c3d9e and pushed to 8.0.x. Thanks!
Comment #53
Gábor HojtsyThanks a lot :)