Problem/Motivation

The |passthrough filter was introduced in an effort to identify and "tag" when to use ! with the {% trans %} tag.

Proposed resolution

  • Remove the |passthrough filter and use |raw.

Remaining tasks

TBD

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#6 interdiff-3-6.txt1.9 KBtadityar
#6 passthrough-raw-2282101-6.patch6.31 KBtadityar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,488 pass(es), 13 fail(s), and 0 exception(s). View
#3 passthrough-raw-2282101-3.patch6.34 KBtadityar
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,487 pass(es), 11 fail(s), and 0 exception(s). View

Comments

tadityar’s picture

So do I have to create a new filter named 'raw' or do I just remove passthrough filter and change occurences of |passthrough in twig files with |raw?

tadityar’s picture

Status: Active » Needs review
FileSize
6.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,487 pass(es), 11 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 3: passthrough-raw-2282101-3.patch, failed testing.

joelpittet’s picture

Thanks for tackling this @tadityar! Here some notes that may help on your patch in #3

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -92,7 +92,6 @@ public function getFilters() {
    -      new \Twig_SimpleFilter('passthrough', 'twig_raw_filter', array('is_safe' => array('html'))),
    

    Nice!

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
    @@ -149,9 +149,6 @@ protected function compileString(\Twig_NodeInterface $body) {
    -              case 'passthrough':
    -                $argPrefix = '!';
    -                break;
    

    This case may need to stay but changed to raw as this trans function maps to the t function.

  3. +++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
    @@ -119,11 +119,6 @@ public function testTwigTransTags() {
    -      'PAS-THRU: &"<>',
    -      '{{ token|passthrough }} was successfully translated and prefixed with "!".'
    -    );
    
    @@ -188,7 +183,6 @@ protected function checkForDebugMarkup($visible) {
    -      '{{ token|passthrough }}' => '<!-- TRANSLATION: "Pass-through: !string" -->',
    

    This test should stay, though also be |raw.

tadityar’s picture

Status: Needs work » Needs review
FileSize
6.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,488 pass(es), 13 fail(s), and 0 exception(s). View
1.9 KB

Changed the patch based on #5!

Status: Needs review » Needs work

The last submitted patch, 6: passthrough-raw-2282101-6.patch, failed testing.

joelpittet’s picture

Hmm, I wonder what's up here... it would be interesting to see the compiled php from this and what it's producing. I think the |raw escape filter is evaluated before the translation filter, just a guess.

Thanks again for working on this. Any guesses?

tadityar’s picture

@joelpittet if so then how do we change the order?

No guesses here.

joelpittet’s picture

There is a priority value on the Twig extension, it would likely need to go before... but this is just a guess and probably not a good way to go about this... Needs to be played with or stepped through to find out whats going on for sure.

davidhernandez’s picture

How can I manually test this?

effulgentsia’s picture

As of #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped, we no longer have any core non-test use cases for the |passthrough filter. Do we want to use this issue to remove that filter entirely? Or do we want to leave that filter in to support a hypothetical contrib/custom use-case where the filter would be needed? If we want to keep it, +1 to renaming it to "raw", but what would such a use case be and is there a better way of supporting it?

effulgentsia’s picture

Issue tags: +SafeMarkup
markcarver’s picture

Hmm, that's a good question. I'm not entirely sure we need either now TBH, unless we intend to keep parity with the ! prefix used in String::format().

The only reason I introduced the |passthrough "filter" in #1927584: Add support for the Twig {% trans %} tag extension was because the native internal |raw filter isn't actually defined as a "filter" and isn't detectable in the code (or at least it wasn't at the time). I opened this issue to eventually address this one off "hack" until something better came along, which I think #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped addresses quite well.

I think we should just go ahead and kill the |passthrough filter entirely and stop worrying about trying to support ! prefixed variables or trying to detect the native |raw filter in {% trans %} tags.

Fabianx’s picture

The |raw filter will never be safe, unless you rename it in a first 'node visitor pass' to passthrough within those elements.

Twig has internal optimizations to remove |raw, which I don't think the trans code is capable of:

e.g.

{{ foo }} gets translated to twig_print_var(drupal_escape_filter('foo'))

while

{{ foo|raw }} gets translated to twig_print_var('foo')

This is how it works :).

On the other hand:

{% set x = foo|raw %}

IIRC should mark x as safe always and not be optimized out, so probably this is fine to remove and leave to that special case if you really need raw.

markcarver’s picture

Title: Remove |passthrough and use |raw filter in Twig » Remove |passthrough filter in Twig

Yes, trans has difficulty in detecting the internal |raw "filter", likely due to internal processes. It's not treated the same as other filters IIRC.

I agree that a simple work around would be to set a new variable outside of the trans tag if it's really needed. That being said, I do not think core has any real case for this, especially after #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped made it in.

We should just remove |passthrough filter entirely and update the change record accordingly.

effulgentsia’s picture

Title: Remove |passthrough filter in Twig » Remove |passthrough and |raw filters in Twig
Issue tags: +Security improvements
Related issues: +#2500747: Remove 'html' option from theme('time')

What if we went further and removed |raw as well? Core's last usage of it is in #2500747: Remove 'html' option from theme('time') with a patch there to fix it. Removing the filter entirely would nudge contrib to better practices (i.e., let Drupal's autoescaping/SafeMarkup implementations do their job, don't try to circumvent them). And if there's a legitimate use case for it (which I'm skeptical about), there can be a contrib module that adds it.

joelpittet’s picture

@effulgentsia I'd really rather not remove |raw. There will be way to many support questions on why Twig's auto-escape is on but |raw doesn't work.

I agree we shouldn't encourage it's use but I'd really not want to address the consequences of removing this feature of upstream Twig. I'll be sure to include that in the document I'm writing up for tomorrow for the D8 Theme Guide on security.

Gábor Hojtsy’s picture

effulgentsia’s picture

Title: Remove |passthrough and |raw filters in Twig » Remove |passthrough filter in Twig

#18 is reasonable. Restoring issue title and scope to #16.

stefan.r’s picture

#2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness repurposes it, i.e. admin XSS filtering if unsafe and printing as-is if safe.

effulgentsia’s picture

Is there a use-case for a Twig author to ever make the decision of whether an unsafe variable should be filtered vs. escaped? Seems to me that if a variable should be filtered, then a preprocess function or similar should do it, not Twig. Therefore, unless I'm wrong about that, I'd rather we remove the filter rather than requiring core to support poor practices.

joelpittet’s picture

re #22 I'd prefer getting raw values (un-pre-escaped) so I have the flexibility to deal with them how they will be marked up in twig. Twig can deal with the escaping and we have control if we ever need it.

I know some don't share this opinion but having this control in twig even if it's not best practice is important and I'll likely find a decent usecase when in practice not unlike PHP filter is not best practice and security vector but in a pinch can be harmless and can save the day or even just allow for a quick prototype to prove a bit of code will work(then refactor), yet still a "possibly" dangerous if used with unsafe values(and other reasons). Feels like a bit of hand-holding that is one step too far in the name of security.

davidhernandez’s picture

What he said ^. And we like to avoid preprocess as much as possible.

joelpittet’s picture

Just found some new minor new information I should share regarding next release of twig. (we are on 1.20)

{% raw %}{% endraw %} is depreciated in 1.21 and replaced with verbatim

+Tags
+----
+
+* As of Twig 1.x, the ``raw`` tag is deprecated. You should use ``verbatim``
+  instead.

This is not the |raw filter, which still exists, but just an FYI.

The verbatim tag existed since 1.12

added verbatim as an alias for the raw tag to avoid confusion with the raw filter
joelpittet’s picture

Status: Needs work » Closed (duplicate)