Problem/Motivation

According to a few sources, the t() variable placeholders are supposed to work in the following way, using a {% trans %} block:

{% set string = '&"<>' %}
{% trans %}
  Escaped: {{ string }}          <-- default, same as @  (i.e. escape HTML)
{% endtrans %}
{% trans %}
  Pass-through: {{ string|passthrough }}   <-- same as !  (i.e. insert as is)
{% endtrans %}
{% trans %}
  Placeholder: {{ string|placeholder }}    <-- same as %  (i.e. <em class="placeholder">[escaped HTML]</em>')
{% endtrans %}

Sources:

However, running some tests, I am not able to get this to work. Please see transtest module at github and post #23 for examples. @lauriii has identified spacing could be a problem, but this does not solve all issues:

It is clearly confusing that {% trans %} reads the white spaces in the beginning and ending of the text so that

{% trans %}
  Translatable text
{% endtrans %}

!=

{% trans %}Translatable text{% endtrans %}

Translation Issues

The test module roughly produces:

Testing translation of 'Set up translations' to 'Configurar traducciones'
Data shown in 'expected' is for page [path/to/drupal]/es/trans-test. For author, 1) translates as expected, but not 4), 6) & 7). (The rest, 2, 3) and 4) are not documented to work and do not translate.)

  1. {{ string|t }} ⇒ Configurar traducciones
    expected: Configurar traducciones
  2. {{ string|passthrough }} ⇒ Set up translations
    expected: [not documented, no translation expected]
  3. {{ string|placeholder }} ⇒ Set up translations
    expected: [not documented, no translation expected]
  4. {% trans %} {{ string }} {% endtrans %} ⇒ Set up translations
    expected: Configurar traducciones
  5. {% trans %} {{ string|t }} {% endtrans %} ⇒ Set up translations
    expected: [t in &#x7B;% trans %&#x7D; undocumented, unclear whether translation expected]
  6. {% trans %} {{ string|passthrough }} {% endtrans %} ⇒ Set up translations
    expected: Configurar traducciones
  7. {% trans %} {{ string|placeholder }} {% endtrans %} ⇒ Set up translations
    expected: Configurar traducciones

Proposed resolution

Unknown yet.

Remaining tasks

  • Other users download and install test module, and verify that problem replicates.
  • Identify problem causes
  • Fix problem causes

User interface changes

Probably none

API changes

Probably none

Comments

Berdir’s picture

That's what node.html.twig is doing:

{% trans %}Submitted by {{ author_name }} on {{ date }}{% endtrans %}

Pretty sure that is working...

star-szr’s picture

Version: 8.0.0-beta9 » 8.0.x-dev
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs tests

Thanks for the report @Mark_L6n. And hi :)

I don't understand this part of your test code, what's with the encoding?

&#x7B;&#x7B; string|t &#x7D;&#x7D; ⇒ {{ string|t }}

Ideally if there is a bug we should be able to write a test to show the failure. Also, have you checked on the latest 8.0.x?

Mark_L6n’s picture

Hi @Cottser! That part is confusing to read ;-) The HTML entitiies just make the text in the 'output' section above (what you see in the browser) easy to read:
{{ string|t }} ⇒ Since they're HTML entities, they don't invoke Twig's variable substitution, as the ⇒ {{ string|t }} portion does.
No, not tried on current dev, sorry, just beta 9.
@Berdir, that looks like test #4 above. The variable is getting substituted, but not translated. According to the docs, {% trans %} {{ string }} {% endtrans %} should be getting translated in the default setting, which is using @ instead of ! or %.

star-szr’s picture

Ah, I see what you mean now :)

I do think something changed somewhat recently with |passthrough (#2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped), but it sounds like you're having a more fundamental issue.

Where are you putting this test code, @Mark_L6n?

Mark_L6n’s picture

It's in the Twig file of a basic custom module, that has in the Controller file render array:

  '#type' => 'page',
  '#theme' => '<module name>',
star-szr’s picture

Hmmm, could you maybe post that code or the bare minimum version of that code to GitHub or something? The fact that there's both #type and #theme is a bit surprising, but I'm not familiar with #type page :)

Mark_L6n’s picture

@Cottser, I created a simple module to demonstrate this and put it here.
This had the same results if both #type and #theme are used, or if #theme is used by itself.
Also, tested it on a current dev version, with the same result.

star-szr’s picture

That's great, thanks @Mark_L6n I'll take a look when I can.

Mark_L6n’s picture

Component: translation.module » language system

Issue wasn't showing up on issue list for Multilingual Initiative. Realized component wrong; don't know where problem is yet, so trying 'language system'.

Gábor Hojtsy’s picture

Status: Postponed (maintainer needs more info) » Active
Issue tags: +language-ui, +sprint, +Security

Tagging with D8MI tags and security. Also active given the reproduction code in https://github.com/Mark-L6n/transtest

klausi’s picture

Priority: Major » Critical
Issue tags: +Needs issue summary update

Potential security issues are critical. Please shorten and summarize the issue summary. Please try XSS attack vectors and where they might apply.

Mark_L6n’s picture

Issue summary: View changes

Simplified and hopefully improved explanation in issue summary. Referenced transtest module at github also altered to display results more clearly.

star-szr’s picture

Hi @Mark_L6n, I took a look at this on the plane over to DrupalCon. The most fundamental thing is I think that trans tags do not seem to work inside loops since they are trying to get the context (variable) from the global $context, not the variables inside the loop.

Someone (possibly @joelpittet) mentioned that this may be by design.

Mark_L6n’s picture

Thanks for looking @Cottser. Tried a few more things, but can't get them to work:

  1. The newer version of the code mentioned in post 12 doesn't have a loop anymore, but the same problems are there.
  2. Tried putting the variable declaration inside the trans block, but got a 'The website has encountered an error' page:
    {% trans %}
        {% set string = 'Set up translations' %}
        &#x7B;% trans %&#x7D; &#x7B;&#x7B; string|passthrough &#x7D;&#x7D; &#x7B;% endtrans %&#x7D; ⇒ {{ string|passthrough }}
    {% endtrans %}
    

    It turns out that {% set string = 'Set up translations' %} fails by itself inside the trans block.

  3. Tried using a hard-coded string, but also got a 'The website has encountered an error' page:
    {% trans %}
        &#x7B;% trans %&#x7D; &#x7B;&#x7B; 'Set up translations'|passthrough &#x7D;&#x7D; &#x7B;% endtrans %&#x7D; ⇒ {{ 'Set up translations'|passthrough }}
    {% endtrans %}
    
Mark_L6n’s picture

Issue summary: View changes
Mark_L6n’s picture

Mark_L6n’s picture

@Cottser and any Twig people, a side question: if we can put the t filter anywhere, why not passthrough and placeholder? Why are passthrough and placeholder only supposed to work in a trans block? It would be consistent and easier to use if all 3 filters could be employed the same way.

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Placeholders seems to working fine. Problem is that you probably had some extra spaces there so it doesn't apply the translation for you string. However the security issue we found here is still something that exists and needs more thinking (whether its security hole at all because it works like that by design). Anyway to move this forward we need to update the issue summary.

lauriii’s picture

Assigned: lauriii » Unassigned
lauriii’s picture

Title: t() variable placeholders not appearing to work in Twig {% trans %} » {% trans %} block should ignore white spaces in the beginning and ending
Category: Bug report » Task
Priority: Critical » Normal
Issue summary: View changes
Issue tags: -language-ui, -sprint, -Security, -Needs issue summary update
lauriii’s picture

Issue summary: View changes
Mark_L6n’s picture

I tried a few things with spaces removed, results on right:
{% trans %}string{% endtrans %} cadena
{% trans %}string|passthrough{% endtrans %} string|passthrough
{% trans %}{{string}}{% endtrans %} [error, page fails to load]
{% trans %}{{string|passthrough}}{% endtrans %} [error, page fails to load]
{% trans %}'string'{% endtrans %} [page fails to load]
{% trans %}'string'|passthrough{% endtrans %} [page fails to load]
{% trans %}{{ string|passthrough }}{% endtrans %} [error, page fails to load]
{% trans %} {{ string|passthrough }} {% endtrans %} Set up translations
In those:

  • The variable is only substituted in the last example. (which has spaces)
  • The variable is never translated. The only translation is for a string, without quotation marks.
  • The passthrough filter never works

Is there another way these should be formatted with spaces? Otherwise, it appears that there are problems with variable substitution and translation within the trans blocks.

lauriii’s picture

You should be translating the string outside of the {% trans %}. {% trans %} will fail if you are only printing a variable because there is nothing to be translated.

Mark_L6n’s picture

I'm confused then, as according to the docs, the passthrough and placeholder filters (which are the way to call t() with ! and %) are to be placed inside of a trans block.
Quote from original issue summary:

According to a few sources, the t() variable placeholders are supposed to work in the following way, using a {% trans %} block:
{% set string = '&"<>' %}
{% trans %}
  Escaped: {{ string }}          <-- default, same as @  (i.e. escape HTML)
{% endtrans %}
{% trans %}
  Pass-through: {{ string|passthrough }}   <-- same as !  (i.e. insert as is)
{% endtrans %}
{% trans %}
  Placeholder: {{ string|placeholder }}    <-- same as %  (i.e. <em class="placeholder">[escaped HTML]</em>')
{% endtrans %}

Sources:

So, currently, is there any way to use the passthrough and placeholder filters?
How is this for a proposed solution:
1) Do as you said in previous post: "You should be translating the string outside of the {% trans %}"
2) Make the passthrough and placeholder filters be called in just the same way as the t filter.

lauriii’s picture

AFAIK they just set a t() placeholders without meaning that they would be actually translated. So if you want the translation placeholders to be translated, you have to do it somewhere else like you have to do then you use t() or |t.

Gábor Hojtsy’s picture

Priority: Normal » Critical
Issue tags: +Security

@lauriii: what makes this a non-critical and non-security? Your change in #21 provided no comment on these changes whatsoever.

Mark_L6n’s picture

@lauriii, I suggest restoring the original title of this thread and the issue summary that was significantly redone in post 12, and then starting a separate issue for "{% trans %} block should ignore white spaces in the beginning and ending", and referring to it from here.
The original title was: "t() variable placeholders not appearing to work in Twig {% trans %}" . Maybe this could be changed to something like "Twig translation filters passthrough and placeholder not working". Some reasons:

  • passthrough and placeholder filters don't work as listed in the above 4 docs (see post #25), and may not be working at all
  • People searching for the same problem will be able to find this issue
  • The first ~15 comments refer to the original title/summary, and will make more sense with it
  • Some of the HTML 'escaping' (converting to HTML entities) problems have nothing to do with the {% trans %} block

In fact, an argument could be made for splitting up the original issue into 2:

  1. Twig translation filters passthrough and placeholder not working
  2. Twig issues with HTML 'escaping' (converting to HTML entities)
lauriii’s picture

Mark_L6n’s picture

Title: {% trans %} block should ignore white spaces in the beginning and ending » Twig problems with 1) translation filters 'passthrough' and 'placeholder' and 2) HTML 'escaping'
Issue summary: View changes

OK, modified title to be more descriptive of issues as they currently stand, and put back previous issue summary.

Mark_L6n’s picture

Added to issue summary a quote from @lauriii's previous summary about spaces in trans blocks.

star-szr’s picture

Just want to mention, there are tests in core to prove that placeholder and passthrough work. \Drupal\system\Tests\Theme\TwigTransTest.

lauriii’s picture

Tested/discussed this shortly with @joelpittet and neither of us cannot find problem on the way the filters work. Inside a {% trans %} both {{ string|passthrough }} and {{ string|placeholder }} escapes the variable as expected. I also checked the test coverage and as far as I can see it should point this bug if it would exist.

Anyway I think we should discuss if #2488304: Add more docs that translate() also marks strings as safe should be considered as a critical, release blocking issue.

alexpott’s picture

If we have a problem it seems that we should be able to write a failing test very simply for this. I'm tempted to demote this from critical since we still do not have a scope of what is actually critical.

@Mark_L6n is there any chance you could write a test to show the errors you are getting - as @Cottser has pointed out we do have test coverage of functionality you have described as not working.

Fabianx’s picture

Category: Task » Support request
Priority: Critical » Major

Demoting to major, there is no security issue. (I tested all possible cases.)

It sounds like the Twig extension is not loaded or something.

I found a bug that using "'" within translations creates a compile error due to the debug output, but that is not critical.

There is one thing to follow-up on:

- ! in the trans extension still behaves like it did originally in SafeMarkup::format(). That should be changed in the other issue.

Fabianx’s picture

Oh and just to prove, this is the generated PHP output:

        // line 3
        echo t("Escaped: @string          <-- default, same as @  (i.e. escape HTML)", array("@string" =>         // line 4
(isset($context["string"]) ? $context["string"] : null), )) . '
<!-- TRANSLATION: "Escaped: @string          <-- default, same as @  (i.e. escape HTML)" -->
';
        // line 6
        echo t("Pass-through: !string   <-- same as !  (i.e. insert as is)", array("!string" =>         // line 7
(isset($context["string"]) ? $context["string"] : null), )) . '
<!-- TRANSLATION: "Pass-through: !string   <-- same as !  (i.e. insert as is)" -->
';
        // line 9
        echo t("Placeholder: %string    <-- same as %  (i.e. <em class=\"placeholder\">[escaped HTML]</em>)", array("%string" =>         // line 10
(isset($context["string"]) ? $context["string"] : null), )) . '
<!-- TRANSLATION: "Placeholder: %string    <-- same as %  (i.e. <em class=\"placeholder\">[escaped HTML]</em>)" -->
';

for this template:

test.html.twig:

Hi {{ xss }}
{% set string = xss %}
{% trans %}
  Escaped: {{ string }}          <-- default, same as @  (i.e. escape HTML)
{% endtrans %}
{% trans %}
  Pass-through: {{ string|passthrough }}   <-- same as !  (i.e. insert as is)
{% endtrans %}
{% trans %}
  Placeholder: {{ string|placeholder }}    <-- same as %  (i.e. <em class="placeholder">[escaped HTML]</em>)
{% endtrans %}

created via this drush script:

test-twig.php:

<?php

require_once "core/themes/engines/twig/twig.engine";

print twig_render_template('test.html.twig', [ 'xss' => '<script>alert("xss");</script>']);

Put both in the same folder and run:

drush scr $(pwd)/test-twig.php
Fabianx’s picture

The reason transtest fails is that it uses:

string|t, which marks the string as safe automatically, hence afterwards it does not get escaped anymore.

That is expected behavior however and you should never pipe a string to |t directly, same as you should not use:

t($some_unsafe_user_string);

Any other problems with translations should be the same when using t() directly, so please add t() tests to trans-test as well.

The other escaping issue I found is a 'arbitrary code execution' issue for dynamic twig templates, but as we don't support those in core officially and you can disable the debug output, its only a major follow-up. Opening an issue now.

Fabianx’s picture

What is a little strange is that both

|passthrough and |placeholder use twig_raw_filter function, which is expected for passthrough, but not for placeholder. We should probably remove both as they are essentially no-op now.

These are all from a pre-autoescape enabled world though ...

Fabianx’s picture

To the author: The best test for XSS is:

<script>alert("xss");</script>

this code.

An alert is very visible and the output is empty then :).

Mark_L6n’s picture

@Cottser, thanks for the link to the test—that way I can see how you (or whoever wrote the test) is intending to use it, as opposed to reading about it in the 4 locations cited above.
In the page, I see:

{{ token|passthrough }} 
{{ token|placeholder }} 

Don't see them within a trans block, though.
When I try to replicate that in the test module, variable substitution takes place but not translation (whether in a trans block or not):

{% set token = 'Set up translations' %}
{{ token|passthrough }} 
{{ token|placeholder }} 

Could one of you fork the test module I posted at github, try to get passthrough and placeholder filters working, and show me how it's done?
[edit: whoops, see cross-posted with @fabianx's last few posts]

Fabianx’s picture

#40:

- placeholder and passthrough do nothing outside a trans block
- If translation inside a trans block is not working, then t() is broken as you can see by the above compiled code.

Are you at LA, if no I am in #drupal-contribute online, if yes, I am in the coder lounge, west-in.

Fabianx’s picture

Pere Orga’s picture

Category: Support request » Bug report
            {% set author = "<script>alert('author')</script>" %}
            {% set date = "<script>alert('date')</script>" %}
            <li>
              {% trans %}
                Submitted by {{ author }} on {{ date }}
              {% endtrans %}
            </li>

I get the first alert('author') when running this.

To me, and based on https://www.drupal.org/node/2047135, this is a XSS vulnerability.

Pere Orga’s picture

Category: Bug report » Support request

I have to retract my last comment, there's no vulnerability.

Sorry for the noise.

Pere Orga’s picture

Except for |placeholder. This is giving me unfiltered output (without the need of a %trans% block):

{% set placeholder = "<script>alert('placeholder');</script>"  %}
{{ placeholder|placeholder }}

I've been trying to exploit this in a theme that uses |placeholder on the node title in node.html.twig, but didn't work. And the reason is that AFAICS all variables are already sanitized by then. When I use {{ label|passthrough }} in node.html.twig I get the title with all html escaped, including tags such as <i> or <strong>.

I don't think these issues are related to the original issue though.

lauriii’s picture

Status: Active » Closed (works as designed)
Issue tags: -Needs tests, -Needs security review

Closing this issue since there is no clear path how to proceed. This issue has became more like an issue where we try to find all the bugs on certain area. Finding bugs is great (yay, more work!) but opening one issue including all of them makes it hard for anyone to try to move it forward.

We have filed two follow ups for this issue:
#2489024: Arbitrary code execution via 'trans' extension for dynamic twig templates (when debug output is on)
#2488304: Add more docs that translate() also marks strings as safe

If there is any remaining bugs that I missed, please feel free to file a new follow-up issue instead of re-opening this.

Pere Orga’s picture

We have filed two follow ups for this issue:
#2489024: Arbitrary code execution via 'trans' extension for dynamic twig templates
#2488304: Remove SafeMarkup::set() from translation::translate()

If there is any remaining bugs that I missed, please feel free to file a new follow-up issue instead of re-opening this.

I can't see how these issues include the ones I pointed out in #45, but I agree that I shouldn't start thinking out loud here; I will familiarize myself with D8 before creating these new issues to make sure I don't miss something.

lauriii’s picture

I'm sorry if I missed a bug there. Thank you for coming back and pointing that out. If you think there is a bug, please feel free to open another issue for that. This issue is not a bug report, but instead a support request and for that reason additional bug reports shouldn't be filed here. We should file new issue for each bug we find and fix them there so we can follow the progress of each bug.

Mark_L6n’s picture

A closing summary from my perspective for anyone who finds this in the future:

  • For the part of the problem "1) translation filters 'passthrough' and 'placeholder' ": I wasn't able to get the passthrough and placeholder filters to translate text (using the transtest module linked to in post 7; some additional examples of things tried are in posts 23 & 40). I'm stopping work on this due to time, but if someone wanted to start a followup issue, one thing to look at is how this is being tested (see posts 32, 40) and perhaps develop another test (see post 34).
  • For "2) HTML 'escaping' ", see the followup issues in posts 42 and 46.
Gábor Hojtsy’s picture

@lauriii: I did not yet have the time to devote enough hours to this issue to understand the underlying problems but your comments which in my understanding basically say "we found a bunch of other issues but did not actually work on the actual issue reported here, so we close it works as designed" does not seem to be a logical statement to me?! I mean if you found a bunch of other problems for which you opened other issues, then how is not even trying to reproduce the bug reported here means it is working as designed?

Fabianx’s picture

Status: Closed (works as designed) » Active

Yes, lets create another follow-up to remove placeholder and passthrough as functions, so that it works like SafeMarkup::format() again or at least do not map them to twig_raw_filter() anymore ...

Gábor Hojtsy’s picture

Category: Support request » Bug report
Priority: Major » Critical
Issue tags: +Security

Is it true that this is NOT a security issue due to:

{% set string = '&"<>' %}
{% trans %}
  Escaped: {{ string }}          <-- default, same as @  (i.e. escape HTML)
{% endtrans %}
{% trans %}
  Pass-through: {{ string|passthrough }}   <-- same as !  (i.e. insert as is)
{% endtrans %}
{% trans %}
  Placeholder: {{ string|placeholder }}    <-- same as %  (i.e. <em class="placeholder">[escaped HTML]</em>')
{% endtrans %}

The first use of string in the translatable string marks it as a safe string? How does it do that? Sorry but I still need some more background to see why is this not a security issue. In #19 Laurii said it is not a security issue because it works like this by design. What? How? In #35, Fabianx says its not a security issue because he tested all cases. How does either of these explain how is this not a security issue?! IMHO we should really be more serious when we dismissing something like this. Again, can someone explain how was this ruled out, why is the shown test code not a security issue? Is it supposed to expose raw HTML as it does? Why? How is it not a security issue? Happy to see this demoted as soon as it is explained.

Gábor Hojtsy’s picture

Priority: Critical » Major
Issue summary: View changes
Issue tags: -Security

All right, I had a good chat with @laurii in IRC. Here is the gist. This was the security claim of the issue:

HTML-escaping Issues

A seemingly related result has security implications, as characters might not be getting escaped correctly. Also roughly from the transtest module:

Testing HTML 'escaping' (converting to HTML entities) with string &"<> ⇒ &amp;&quot;&lt;&gt;
Need to look at source code to evaluate, which is a little difficult: look after ⇒ in correct section.
Not sure what is expected in all cases, so 'expected' is not listed. However, encounter strange behavior: 1, 2 & 4 are identical.
In author's test, 1) and 2) are converted to entities, but identical 4) is not, nor are any of the other examples.
So appears that something is happening to the variable containing &"<> after it is used with t filter. Furthermore, if 3) (that uses the t filter) is deleted, then 3 of the 4 trans block examples (7,8&10) escape the HTML.

  1. {{ string }} ⇒ &"<>
  2. {{ string }} ⇒ &"<>
  3. {{ string|t }} ⇒ &"<>
  4. {{ string }} ⇒ &"<>
  5. {{ string|passthrough }} ⇒ &"<>
  6. {{ string|placeholder }} ⇒ &"<>
  7. {% trans %} {{ string }} {% endtrans %} ⇒ &"<>
  8. {% trans %} {{ string|t }} {% endtrans %} ⇒ &"<>
  9. {% trans %} {{ string|passthrough }} {% endtrans %} ⇒ &"<>
  10. {% trans %} {{ string|placeholder }} {% endtrans %} ⇒ &"<>

The reason 4 works different from 1 and 2 is that 3 uses t() which stores the string as a safe markup source string. That is what #2488304: Add more docs that translate() also marks strings as safe was opened for to discuss. If that issue is to be resolved as it currently suggests, then 4 would work the same as 1 and 2 because 3 would not change how the dynamic string is considered. The reason this is "by design" is that the idea is that t() directly is only used for static code strings and not dynamic variables like that.

This DOES NOT contradict the tested example in core which is this (in twig_theme_test.trans.html.twig and at the top of the issue summary):

{# Test trans tag with different filters applied to tokens. #}
{% set string = '&"<>' %}
<div>
  {% trans %}
    Escaped: {{ string }}
  {% endtrans %}
</div>
<div>
  {% trans %}
    Pass-through: {{ string|passthrough }}
  {% endtrans %}
</div>
<div>
  {% trans %}
    Placeholder: {{ string|placeholder }}
  {% endtrans %}
</div>

Here string itself is never translated directly, so the passthrough and placeholder let it through / escape it properly as expected. (Tested in TwigTransTest.php).

So in short the "security problem" is that then this does not escape the output either:

$string = '&"<>' ;
print t($string);
print t('Not escaped now because t() was used on it directly before: %string', array('%string' => $string));

This problem has nothing to do with twig per say, its how SafeMarkup works.

Gábor Hojtsy’s picture

A better all-around example for the "security problem":

$string = '&"<>' ;
print t('Escaped now because t() was not yet used on it directly: %string', array('%string' => $string));
print t($string); // <== this registers $string as safe so the following will not escape it anymore
print t('Not escaped now because t() was used on it directly before: %string', array('%string' => $string));

Posting also on #2488304: Add more docs that translate() also marks strings as safe.

star-szr’s picture

Thank you for all that @Gábor Hojtsy it wasn't clear to me either. Really appreciate the digging you did.

As for the "security problem" (scare quotes?) you're still not supposed to t() variables, right? Variables always need to go through placeholders of one type or another?

Fabianx’s picture

Yes, the major bug here is that you can call:

{{ var | placeholder }}

outside of a {% trans %} statement and that we map

'placeholder' => 'twig_raw_filter',

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.

Gábor Hojtsy’s picture

@Fabianx: can you open an issue for that? Then we can close this one for good.

@Cottser: I think we cannot assume that all strings to be translated are typed in verbatim in the template, can we? So there is a trust issue with a variable you get, that the template will need to trust it comes from a trusted source. Or if all strings to be translated are typed in directly then no problem there :)

star-szr’s picture

@Gábor Hojtsy I might be missing something obvious but here goes… in other words is #54 only an issue because we are talking about it coming from a Twig template? It seems like it's insecure code in PHP land so should be insecure in Twig (or PHPTemplate, or etc) too, feeding variables directly to t() like that.

Attempted Twig version of #54, the second trans block string does not get escaped because it's in SafeMarkup:

{# This 'string' variable could also be passed into the template, just building here as an example. #}
{% set string = '&"<>' %}

{% trans %}
Escaped now because t() was not yet used on it directly: {{ string|placeholder }}
{% endtrans %}

{{ string|t }}

{% trans %}
Not escaped now because t() was used on it directly before: {{ string|placeholder }}
{% endtrans %}
Gábor Hojtsy’s picture

@Cottser: right, we are on the same page, the "problem" is due to how t() works, and that is how t() is documented to work too. See https://www.drupal.org/node/2488304#comment-9961087 I don't think we need to keep this open for that. Not sure where |t / {% trans %} is documented but it should ensure that it documents the same expectation, that would be for https://www.drupal.org/node/2488304#comment-9961087 to resolve IMHO.

I believe the only thing left this issue is open for is the lack of followup for #56.

star-szr’s picture

Thanks, good stuff. I'll go ahead and create a follow-up for #56.

star-szr’s picture

Category: Bug report » Support request
Status: Active » Fixed

Done, #2495179: Twig placeholder filter should not map to raw filter. Not really sure what to do with the category/status, hope this works for everyone :)

Status: Fixed » Closed (fixed)

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