Add more docs that translate() also marks strings as safe

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.

#2472731-58: Twig problems with 1) translation filters 'passthrough' and 'placeholder' and 2) HTML 'escaping'

I think it should be expected in Twig, we are just mapping to t().

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

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

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

@pwolanin I agree.

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.

tried to document it. please check the wording.

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 <em>emphasized</em> 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 <a href="https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21StringTranslation%21TranslationInterface.php/function/TranslationInterface%3A%3Atranslate/8">TranslationInterface::translate()</a>

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.

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

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

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.

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.

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

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

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?

@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

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?

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

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?

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

Remove from multilingual sprint then.

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?

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

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.

It seems like we need to still resolve this?

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.

The patch in #15 still applies cleanly.

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.

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.

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.

Thanks for the quick feedback!

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

Great! I think this is fine now. Thanks!

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

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.

... in #30.

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

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

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?

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

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.

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

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

Yay, looks good with me :)

Committed and pushed to 8.0.x. Thanks!

Superb, thanks!