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

Files: 
CommentFileSizeAuthor
#48 interdiff.txt1.67 KBlauriii
#48 twig_placeholder_filter-2495179-48.patch5.7 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,041 pass(es). View
#45 2495179-45.patch5.72 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,008 pass(es). View
#45 interdiff.txt961 bytesdawehner
#41 interdiff.txt4.1 KBdawehner
#41 2495179-41.patch6.39 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,689 pass(es). View
#38 interdiff.txt2.35 KBGábor Hojtsy
#38 2495179-38.patch8.15 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,647 pass(es). View
#37 interdiff.txt716 bytesdawehner
#37 2495179-37.patch8.53 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,482 pass(es). View
#35 interdiff.txt717 bytesdawehner
#35 2495179-35.patch8.53 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,440 pass(es). View
#33 interdiff.txt3.69 KBdawehner
#33 2495179-33.patch8.53 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,433 pass(es), 1 fail(s), and 0 exception(s). View
#30 2495179-30.patch4.85 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,440 pass(es), 2 fail(s), and 0 exception(s). View
#21 2495179-21.patch1.2 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,272 pass(es), 4 fail(s), and 0 exception(s). View
#6 2495179.patch1.59 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,847 pass(es), 6 fail(s), and 0 exception(s). View

Comments

Cottser’s picture

Issue summary: View changes

I 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 :)

Remove placeholder and passthrough as functions, so that it works like SafeMarkup::format() again…

Gábor Hojtsy’s picture

Issue tags: +language-base
Gábor Hojtsy’s picture

Issue tags: +security
Gábor Hojtsy’s picture

Issue tags: +hardening

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.

Gábor Hojtsy’s picture

Issue tags: +sprint
Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
1.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,847 pass(es), 6 fail(s), and 0 exception(s). View

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?

Status: Needs review » Needs work

The last submitted patch, 6: 2495179.patch, failed testing.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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

chx’s picture

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.

chx’s picture

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.

Gábor Hojtsy’s picture

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

Cottser’s picture

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

Fabianx’s picture

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

Sorry for the confusion, @all.

Gábor Hojtsy’s picture

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

Fabianx’s picture

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

Gábor Hojtsy’s picture

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

webchick’s picture

Priority: Major » Critical

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

Fabianx’s picture

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.

Fabianx’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
1.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,272 pass(es), 4 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 21: 2495179-21.patch, failed testing.

Gábor Hojtsy’s picture

lauriii’s picture

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

Fabianx’s picture

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

effulgentsia’s picture

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.

Fabianx’s picture

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.

Gábor Hojtsy’s picture

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

effulgentsia’s picture

Issue tags: +Triaged D8 critical

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,440 pass(es), 2 fail(s), and 0 exception(s). View

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

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 30: 2495179-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
8.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,433 pass(es), 1 fail(s), and 0 exception(s). View
3.69 KB

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

Status: Needs review » Needs work

The last submitted patch, 33: 2495179-33.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,440 pass(es). View
717 bytes

We we need to check for the proper HTML.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ 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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,482 pass(es). View
716 bytes

Good point

Gábor Hojtsy’s picture

FileSize
8.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,647 pass(es). View
2.35 KB

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.

dawehner’s picture

Assigned: Unassigned » Fabianx

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.

Fabianx’s picture

Assigned: Fabianx » Unassigned
Status: Needs review » Needs work

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!

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,689 pass(es). View
4.1 KB

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

Ah that is much nicer!

Status: Needs review » Needs work

The last submitted patch, 41: 2495179-41.patch, failed testing.

dawehner queued 41: 2495179-41.patch for re-testing.

chx’s picture

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

leftover debug?

dawehner’s picture

Status: Needs work » Needs review
FileSize
961 bytes
5.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,008 pass(es). View

leftover debug?

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

lauriii’s picture

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

dawehner’s picture

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

Sure

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
5.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,041 pass(es). View
1.67 KB

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.

dawehner’s picture

Sure

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

The interdiff in #48 looks totally fine.

Fabianx’s picture

RTBC + 1, nice patch!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 83c3d9e on 8.0.x
    Issue #2495179 by dawehner, Gábor Hojtsy, lauriii, Fabianx, chx,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks a lot :)

Status: Fixed » Closed (fixed)

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