Twig placeholder filter should not map to raw filter

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

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

So looks like that is not how it works. Halp!

Tweeted about this as a last resort to get some help :/ https://twitter.com/gaborhojtsy/status/608167337469091841

Some 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:

function twig_no_op($string) {
  return $string;
}

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 a

function noOp($string) {
  return $string;
}

method.

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

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

#6: chx is right, just need to define the function in twig.engine.

Sorry for the confusion, @all.

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

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

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

Escalating to critical; we have a triage call tomorrow where we can discuss this.

Lets define:

function twig_no_op($string) {
  return $string;
}

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.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -111,13 +111,13 @@ public function getFilters() {
-      new \Twig_SimpleFilter('passthrough', 'twig_raw_filter', array('is_safe' => array('html'))),
...
+      new \Twig_SimpleFilter('passthrough', 'twig_no_op'),
+      new \Twig_SimpleFilter('placeholder', 'twig_no_op'),

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

@FabianX: right, we twig_raw_filter is already there and doing that. Entirely new patch using that then. Needs tests.

Now instead of "Placeholder: %placeholder" it generates "Placeholder: @placeholder"

Yeah, needs some TwigNodeVisitor love to go one node further down because now there is the twigDrupalEscapeFilter in the chain.

In #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 in update-project-status.html.twig and is:

<li>
  {% trans %}
    Enabled: {{ includes|placeholder }}
  {% endtrans %}
</li>

a significantly better themer and translator experience than:

<li>
  {% trans %}
    Enabled: <em class="placeholder">{{ includes }}</em>
  {% endtrans %}
</li>

?

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.

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

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

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

Let's first put up test coverage so that we know what should actually happen.

Let's first put up test coverage so that we know what should actually happen.

There we go. here is a solution I think fabian had in mind.

We we need to check for the proper HTML.

+++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
@@ -176,6 +178,20 @@ public function testTwigTransDebug() {
+    $this->assertEqual(2, substr_count($this->getRawContent(), '<em class="placeholder">' . SafeMarkup::checkPlain($script)) . '</em>');

The /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.

Good point

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

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

Overall 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!

- Make 'placeholder' use ($this, escapePlaceholder) and leave if as 'safe'.
- Remove 'escapePlaceholder'
- Remove node visitor changes

Ah that is much nicer!

- public function testTwigTransTags() {
+ public function ptestTwigTransTags() {

leftover debug?

leftover debug?

Thank you for pointing that out. I guess it should that automate sort of locally ...

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -132,7 +132,7 @@ public function getFilters() {
+      new \Twig_SimpleFilter('placeholder', [$this, 'escapePlaceholder'], array('is_safe' => array('html'), 'needs_environment' => TRUE)),

Now there is mixed use of array() and [] on a single line. Could we change all the arrays to use [] on that line?

Now there is mixed use of array() and [] on a single line. Could we change all the arrays to use [] on that line?

Sure

Just a very minor changes for the patch. Otherwise this looks very clean, fixes the concerns on the issue and adds test coverage for that.

Sure

Ha, I actually had a patch but just forgot to upload it :)

The interdiff in #48 looks totally fine.

RTBC + 1, nice patch!

#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!

Thanks a lot :)