Problem/Motivation

SafeMarkup::set() is being used in translation::translate() which in some cases might unintentionally mark a string as safe. Consider the following:

$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));

While t() is not supposed to be used on user input (and it is documented on it), we should expand the documentation towards Twig integration (only documented in docs pages, so no patch) and translate() itself.

Proposed resolution

Expand documentation to make the behavior more evident.

Remaining tasks

Commit.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#56 2488304.55.patch2.28 KBMariano
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,194 pass(es). View
#44 2280965.43.patch1.61 KBgenjohnson
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,919 pass(es). View
#44 interdiff-2488304-41-43.txt1.75 KBgenjohnson
#42 2488304.41.patch1.53 KBgenjohnson
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,707 pass(es). View
#15 2488304.15.patch1.52 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,147 pass(es). View
#1 translate-escape.patch714 byteslauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,750 pass(es), 25 fail(s), and 0 exception(s). View

Comments

lauriii’s picture

Status: Active » Needs review
FileSize
714 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,750 pass(es), 25 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: translate-escape.patch, failed testing.

Gábor Hojtsy’s picture

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

Title: Remove SafeMarkup::set() from translation::translate() » SafeMarkup::set() in translation::translate() may "leak" user input into the safe markup list
Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

As per t()'s docs:

 * It is especially important never to call t($user_text) where
 * $user_text is some text that a user entered - doing that can lead to
 * cross-site scripting and other security problems.

So developers should expect this in PHP. Should they expect it in TWIG.

Cottser’s picture

Gábor Hojtsy’s picture

@Cottser: is there specific docs in Twig for this that should be checked?

lauriii’s picture

Should we escape everything or would checkAdminXss be enough for this

Cottser’s picture

https://www.drupal.org/node/2357633 might be a good spot to explain it.

pwolanin’s picture

I'm confused by this issue - it seems it currently works as intended and need documentation here not a change in the code.

Cottser’s picture

@pwolanin I agree.

YesCT’s picture

I will work on a patch to add docs to the set call (and the parent that method is inheritdoc'ing from), and update the handbook page @Cottser referenced.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,147 pass(es). View

tried to document it. please check the wording.

YesCT’s picture

currently the doc page @Cottser linked to, https://www.drupal.org/node/2357633 , says:

t
The t filter will run the variable through the Drupal t() function, which will return a translated string. This filter should be used for any interface strings manually placed in the template that will appear for users.

Example: <a href="{{ url('<front>') }}" title="{{ 'Home'|t }}" rel="home" class="site-logo"></a>

passthrough, and placeholder
In order to safely escape all of the Twig variables detected in an {% trans %} tag, the variables are sent with an @ prefix by default. To pass-through a variable (!) or use as a placeholder (%), the passthrough or placeholder filters are used.

passthrough gets no sanitization or formatting and should only be used for text that has already been prepared for HTML display. placeholder gets escaped to HTML and formatted using drupal_placeholder(), which makes it display as emphasized text.

Example: {% trans %}Submitted on {{ date|passthrough }}{% endtrans %}

Here is my first try and writing down things that should not be done (similar to the suggestion in the php code/patch):

Never use a variable that might be user input without sanitizing it.
For example, never: {% trans %}{{ user_input|passthrough }}{% endtrans %}
And, never: {{ user_input|t }}

See TranslationInterface::translate()

But I wonder if the example there already:
Example: {% trans %}Submitted on {{ date|passthrough }}{% endtrans %}
(which is recommending a way to do something) could be changed to be a safer, more applicable example also.

davidhernandez’s picture

Playing around with this, and want to write this down to make sure it is clear.

Seems ok:

{% trans %} Text {{ var1 }}{% endtrans %}
{% trans %} {{ var1|t }}{% endtrans %}
{% trans %} {{ var1|placeholder }}{% endtrans %} // and adds emphasis
{% trans %} {{ var1|raw }}{% endtrans %}
{{ var1 }}

Unsafe:

{% trans %} {{ var1|passthrough }}{% endtrans %}
{{ var1|t }}
{{ var1|placeholder }}
{{ var1|passthrough }}
{{ var1|raw }}

Bad:
{% trans %}{{ var1 }}{% endtrans %} // Without any additional text it breaks site. Bug?

One of the issues we were having is results seem to vary based on what template I did this in. I'm not sure why. Even a pure string of text that is unsafe and set directly in the template was being escaped. I had to do the above in the page template to get it working right.

So, it looks like the handbook page is correct, and the only thing you can expect to be unsafe when specifically using the trans tag is passthrough. ...BUT, using those filters on their own, is also bad. I see no reason why someone would try to use placeholder or passthrough on their own, but stranger things have happened, and people will use t and raw. We might want to detail these specifics in the handbook page.

Regarding the actual patch, this one sentence reads a little funny. "where $user_text is some text that a user entered" I might try to rewrite that. Otherwise, it is understandable.

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
@@ -17,6 +17,10 @@
+   * It is important never to call translate($user_text) where $user_text is
+   * some text that a user entered - doing that can lead to cross-site scripting
davidhernandez’s picture

Issue tags: +D8 Accelerate

Adding the D8 accelerate tag because we're working on this during a sprint.

crowdcg’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the patch in #15 and it adds necessary docs discussed in the issue. I've also updated the handbook page per #17 (see https://www.drupal.org/node/2357633/revisions/view/8201009/8562083), and I've opened the follow-up issue regarding {% trans %}{{ var1 }}{% endtrans %} // Without any additional text it breaks site., see #2505517: Wrapping var in {{% trans %}} can break your site.

Regarding the suggested wording change in #17 it seems unneeded since the wording highlighted is already used elsewhere in core and is clear despite being overly explicit.

Mark_L6n’s picture

Regarding a comment in #17:

BUT, using those filters on their own, is also bad. I see no reason why someone would try to use placeholder or passthrough on their own, but stranger things have happened, and people will use t and raw.

An example case of the benefit of passthrough: this appears to be roughly like a combination of t() and raw, translating a string and then not escaping it (i.e. not turning some characters into HTML entities). I've had problems with things being turned into HTML entities in the past, and would like to have the option of preventing this should a problem arise.

davidhernandez’s picture

What I meant was using the filters without using the trans tag, which is what causes the problem.

Gábor Hojtsy’s picture

@davidhernandez: well #2495179: Twig placeholder filter should not map to raw filter made placeholder safe to use outside of trans, and passthrough is unsafe either way. So should be no such issues with the filters outside of trans anymore. Just trans making a possibly unsafe thing safe (same as t(), but documented on t() and not on trans).

effulgentsia’s picture

never: {% trans %}{{ user_input|passthrough }}{% endtrans %}

See also #2282101: Remove |passthrough filter in Twig.

And, never: {{ user_input|t }}

Could be a followup, but is it possible to prevent this? i.e., can Twig filters know if they are passed a Twig literal vs. a Twig variable, and throw an exception for the latter?

Gábor Hojtsy’s picture

@effulgentsia: I am not sure how the common Twig patterns are shaping up but I think it would still be possible to pass in a variable value from PHP as a twig variable that only gets t-ed on demand in twig, no? That kind of use would not pass your test I guess. Not sure if that kind of use is possible/encouraged in Twig or I am just making it up :D

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 2488304.15.patch, failed testing.

Status: Needs work » Needs review

davidhernandez queued 15: 2488304.15.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 15: 2488304.15.patch, failed testing.

Status: Needs work » Needs review

Gábor Hojtsy queued 15: 2488304.15.patch for re-testing.

Gábor Hojtsy’s picture

Title: SafeMarkup::set() in translation::translate() may "leak" user input into the safe markup list » Add more docs that translate() also marks strings as safe
Component: theme system » documentation
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs work

Usually developers will either use StringTranslationTrait or \Drupal::t() instead of using this method directly, and I don't think they will necessarily drill down in to see this documentation. I think maybe this is important enough to add the note in those places as well?

Gábor Hojtsy’s picture

@xjm: I believe the doc added to translate() here was copied verbatim from t(), that has:

It is especially important never to call t($user_text) where $user_text is some text that a user entered - doing that can lead to cross-site scripting and other security problems.

It is true this patch does not add that warning to StringTranslationTrait. I don't think there is a Drupal::t() anymore. It is also true that the TranslationInterface by itself does not mandate any integration with safemarkup. So adding the doc there seems like was not 100% correct either. Someone implementing that interface may just as well implement it differently under the hood.

So looks like:

1. Remove the TranslationInterface hunk from the patch.
2. Add the security note to StringTranslationTrait instead (either directly on the trait or on the t() and formatPlural()) methods.

jhodgdon’s picture

Did a quick check of the existing documentation of this...

a) t() function says:
You should never use t() to translate variables, such as calling t($text) unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array). It is especially important never to call t($user_text) where $user_text is some text that a user entered - doing that can lead to cross-site scripting and other security problems.

b) I think all (?) of the t() methods on classes come from StringTranslationTrait::t(). That says:
See the t() documentation for details.

So it seems to me that this is covered, even without the patch, for calls to t() and Foo::t().

The issue summary is about what is happening in Twig though. I am not sure where someone would go for this documentation?

Gábor Hojtsy’s picture

Status: Needs work » Closed (won't fix)

@jhodgdon: right, we wanted to spread that security note wider that just one place, so other methods that expose the same functionality document it as well. If your position as a docs maintainer is that a security note does not necessitate copying the same text around because it is implicitly referenced, then we better won't fix this. I think this issue was so simple that its hard to justify wasting time arguing to so much about it.

Gábor Hojtsy’s picture

Issue tags: -sprint

Remove from multilingual sprint then.

jhodgdon’s picture

Status: Closed (won't fix) » Reviewed & tested by the community
Issue tags: +sprint

My comment in #32 was in reference to xjm's suggestion in #31 about the need to add this note to StringTranslationTrait. My feeling is that StringTranslationTrait has it covered pretty well.

The patch, however, adds it to the base class/method that actually does the translation, which doesn't reference the t() docs. So I think we should go ahead with that?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@jhodgdon: not sure I get what you mean there? In #31 I observed that the interface is not the right place to document how something works internally (in terms of its side effects especially) because implementing the interface should not require implementing side effects. So the point would be to put the docs on StringTranslationTrait instead. In #32 you said that is not a good idea because it already implicitly has that by reference to t(). So reading those two it looks like you did not agree with the direction suggested and I don't agree with the current patch.

jhodgdon’s picture

Ah, I see. We can add more docs to StringTranslationTrait. I don't completely think it's necessary but on the other hand this is important.

jhodgdon’s picture

It seems like we need to still resolve this?

genjohnson’s picture

I'm sprinting at DrupalCorn Camp and will work on this. If the patch in #15 doesn't apply I'll re-roll that first, and then from #36 it sounds like the documentation should not be provided on the interface and added to StringTranslationTrait instead, so I'll work on a patch for that.

genjohnson’s picture

The patch in #15 still applies cleanly.

jhodgdon’s picture

Great! Yes, that seems to be the right thing to do. I would not bother to "reroll" the patch since it's only a few lines. Just add the three lines of comment to TranslationManager.php (changing the @see reference to StringTranslationTrait), and also add docs to StringTranslationTrait... not sure if it should go in the Trait doc block at the top or in the methods? Probably in the t() method.

genjohnson’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,707 pass(es). View

Ok, here's a patch that adds a comment to TranslationManager.php and docs to StringTranslationTrait in the t() method.

I'm especially not sure if I did the @see reference to StringTranslationTrait correctly.

jhodgdon’s picture

Status: Needs review » Needs work

Looks pretty good!

A couple of small details to address:

  1. +++ b/core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
    @@ -36,6 +36,10 @@
    +   * It is important never to call t($user_text) where $user_text is
    +   * some text that a user entered - doing that can lead to cross-site scripting
    +   * and other security problems.
    +   *
        * See the t() documentation for details.
        */
    

    I think maybe we should put this new note after the "See the t() ..." sentence, because that sentence is really saying "All the docs for this function are on the t() function instead." With the current placement, it looks like it's saying "For more notes on not calling t() on user text".

    Also the wrapping on this paragraph needs adjusting.

    And I think since we are putting this note here, we need to say:

    ... never to call $this->t($user_text) ...
    because it is a method now not a function.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -142,6 +142,9 @@ public function getStringTranslation($langcode, $string, $context) {
    +      // @see \Drupal\Core\StringTranslation\StringTranslationTrait
    

    This @see needs to point to StringTranslationTrait::t(), which is where the note has been added.

genjohnson’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
1.61 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,919 pass(es). View

Thanks for the quick feedback!

Here's a patch (and interdiff) which addresses all of issues raised in #43.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Great! I think this is fine now. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -142,6 +142,9 @@ public function getStringTranslation($langcode, $string, $context) {
+      // @see \Drupal\Core\StringTranslation\StringTranslationTrait::t()

Why the @see to the trait. This is the method that should have the documentation about the safe in TranslationInterface::translate and the trait should point to that.

The mismash of documentation between the trait, t() and the interface is super confusing. Everything calls the method on the service - therefore I think TranslationInterface::translate is the correct place for all the documentation.

A safe-ness needs to be mandated by the interface as if t() does not return strings marked as safe everything will be very very broken.

jhodgdon’s picture

I do not think there is any way to fix this that everyone will be happy with. When we didn't have it this way, xjm objected.

chx’s picture

... in #30.

jhodgdon’s picture

Could @xjm and @alexpott please confer and figure out what you want on this issue?

xjm’s picture

Hm the current patch does not look like what I thought I was suggesting either. I was trying to suggest that at a high level on t() and on the trait and on the interface and on \Drupal::t() we should briefly reiterate the message that the first argument should only ever be for strings defined in code, because it is too important to rely on developers carefully reading the docs in another place. I do agree that other than that all the main documentation should be in one place, which is what I think @alexpott was getting at in #46. In HEAD that appears to be on procedural t() so we should consolidate it there.

So basically, the more detailed information should go on t(), and possibly a short single sentence on the other entry points, plus the inline comment in the translation manager. Whoever updates the patch can use their best judgment as to balancing being clear about it with being redundant. ;)

manningpete’s picture

Assigned: Unassigned » manningpete
manningpete’s picture

To clarify:

I am going to attach the more verbose description against using t(user test) on the main Drupal t() function which is I think here?

lines 336 of bootstrap.inc (or does the verbose description go somewhere else?)

Then the shorter one liner, which I shortened to

Never call $this->t($user_text) where $user_text is text that a user entered; doing so can lead to cross-site scripting and other security problems.

And which (per #50), I plan to add to:

"the trait" /core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php

"the interface" /core/lib/Drupal/Core/StringTranslation/TranslationManager.php

" \Drupal::t()" I'm not sure where this is?

manningpete’s picture

I'm going to roll this patch now according to what I described in #52: longer text in bootstrap, shorter to trait and manager.

jhodgdon’s picture

Strategy sounds good to me, but I thought some of the previous patches were OK too, so take that as you may. ;)

Regarding \Drupal::t(), that doesn't actually exist, as far as I can tell. The global thing is the bare t() function.

Mariano’s picture

Assigned: manningpete » Mariano

Taking a look at this as part of the Acquia Build Week Hackathon

Mariano’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 104,194 pass(es). View

As per #52, added the improved short message to:

- core/lib/Drupal/Core/StringTranslation/StringTranslationTrait.php
- core/lib/Drupal/Core/StringTranslation/TranslationInterface.php

and the inline message to:

- core/lib/Drupal/Core/StringTranslation/TranslationManager.php

\Drupal::t() doesn't exist.

Global t() already has a detailed explanation referenced in #7

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay, looks good with me :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 549337a on 8.0.x
    Issue #2488304 by genjohnson, lauriii, YesCT, Mariano, Gábor Hojtsy,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

Status: Fixed » Closed (fixed)

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