"!"-prefixed tokens should Xss::filterAdmin() but not affect safeness

Let's see what this breaks

This will still need a test case for Twig translation of a safe string in pass-through. I didn't get it to skip admin XSS filtering with string|raw|passthrough in twig_theme_test.trans.html.twig, what am I doing wrong?

We had a random fail in MigrateExecutableTest as the random error message included an & which got filtered to &. I changed random->string() to random->name() there as we already have test coverage for admin XSS filtering the !placeholders.

@alexpott failed to mention in the summary that this was my idea. :P Updating the summary to include more information from the duplicate I was creating.

Those Twig tests will fail now that they're reverted I think?

For Twig I was looking for {{ string|passthrough }} to behave the same as !placeholder (post-patch) -- ie. admin filter if unsafe (which was being tested for in the reverted tests) and pass through as-is if safe, that's not what we want?

Not sure I follow... The test changes were passing in in unsafe text, which got XSS filtered as intended, and as mentioned in #7 it just needed another test case for safe text as well?

The test case tested for filtering, not escaping, so the test string may have been confusing?

$string = '&"<>';
Xss::filterAdmin($string) == '&amp;"&lt;&gt;';
SafeMarkup::escape($string) == '&amp;&quot;&lt;&gt;';

Discussed this with @alexpott on IRC and actually #19/#20 was the confusion -- so re-adding the test from #7 and wrapping the bad string in <script> tags to make it more obvious we're filtering (not escaping).

I still think the twig test case should have a test for a safe string that gets passed through as-is though. Is there any way we can do this from the twig template itself? I had already tried string = 'bad'|raw but per #11 we can't do that.

Re #11 / #21, Twig literals are always marked as safe, so:

{% set foo = '<script>alert("xss")</script>' %}
{% trans %}Foo {{ foo }} {{ foo|passthrough }} {% endtrans %}

should pass the script tags through to both.

If you mark a non-literal as safe, you can also do:

{% set foo = some_variable|raw %}

and proceed to pass {{ foo }} inside {% trans %} without it getting escaped or filtered. But, that's a really really bad idea, since how as a Twig author do you know that some_variable doesn't have user input in it?

Ah I tried that and it considered everything unsafe... so the passthrough filter needs to be fixed, and #21 needs to be fixed as well.

Any way I can mark a twig literal as unsafe in the template?

Just went in with a debugger to check #22 -- that may not be how it works? With the current patch the following piece of twig template compiles to the piece of PHP code underneath:

{% set string = '<script>&"<></script>' %}
{% set string2 = string|raw %}
<div>
  {% trans %}
    Escaped: {{ string }}
  {% endtrans %}
</div>
<div>
  {% trans %}
    Admin filtered: {{ string|passthrough }}
  {% endtrans %}
</div>

compiles to:

       // line 44
        $context["string"] = '<script>&\"<></script>';

        $context["string2"] = (isset($context["string"]) ? $context["string"] : null);

        echo t("Admin filtered: !string", array("!string" => (isset($context["string"]) ? $context["string"] : null)));

        echo "</div>
<div>
  ";
        echo t("Admin filtered & raw: !string2", array("!string2" => (isset($context["string2"]) ? $context["string2"] : null)));

So I think we're good with the last patch, it just needs a test passing in a safe string.

Interdiff in #27 looks good!

Upon discussing with @alexpott the test mentionef in #7 shouldn't be necessary as we already have test coverage through SafeMarkupTest and no-one would need to define a safe string (safe as in SafeMarkup::isSafe) in the template just to put it into a !placeholder, as they might as well put the string in the template directly.

This adds test coverage just to illustrate in the twig trans test how the passthrough filter works exactly, and to prevent any regressions later on.

I'm curious whether / where we might have documentation for | passthrough, I mean there we should document the behaviour of the code. ...

It's documented in https://www.drupal.org/node/2357633 and https://www.drupal.org/node/2047135 but those took a while to find... they're a bit hidden away

Could be good to update docs elsewhere as well, ie:

https://www.drupal.org/node/1823416
https://www.drupal.org/theme-guide/8/twig

Overall I think this looks like a nicer approach than escaping unsafe passthrough values (though maybe a bit more subtle when things go wrong with unsafe values only "some" tags are missing).

Which issues are blocked on this?

1. +++ b/core/modules/system/tests/modules/twig_extension_test/templates/twig_extension_test.filter.html.twig
   @@ -1,3 +1,16 @@
   +<!--
   + The placeholder and passthrough filters should only be used in trans tags but
   + their effects need to be tested outside trans tags.
   +-->
   

   Nit: Twig comment {##} with two space indent.

2. +++ b/core/modules/system/tests/modules/twig_extension_test/templates/twig_extension_test.filter.html.twig
   @@ -1,3 +1,16 @@
   +</div>
   \ No newline at end of file
   

   This file needs an EOF new line.

3. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
   @@ -49,7 +49,13 @@
   +    {# Note this should never be necessary: we can just print the safe string in the template directly. #}
   

   nit: wrap 80 chars, and likely could use a rewrite to make it more clear, it reads funny "Note this"

   "necessary: we".

4. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
   @@ -59,11 +65,11 @@
   -    This {{ token.name }} has a length of: {{ count }}. It contains: {{ token.numbers|placeholder }} and {{ token.bad_text }}. Lets pass the bad text through: {{ token.bad_text|passthrough }}.
   +    This {{ token.name }} has a length of: {{ count }}. It contains: {{ token.numbers|placeholder }} and {{ token.bad_text }}. Let's filter the bad text: {{ token.bad_text|passthrough }}.
   

   A bit leary about filtering through something called 'passthrough'. probably my biggest concern, but I have the same problem with escaping |passthrough as well. So my concern may be moot.

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -237,7 +237,7 @@ public function import() {
   -        $this->t('Migration failed with source plugin exception: !e', array('!e' => $e->getMessage())), 'error');
   +        $this->t('Migration failed with source plugin exception: @e', array('@e' => $e->getMessage())), 'error');
   
   +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Utility\Html;
   
   @@ -72,7 +73,7 @@ public function testImportWithFailingRewind() {
   -      ->with("Migration failed with source plugin exception: $exception_message");
   +      ->with('Migration failed with source plugin exception: ' . Html::escape($exception_message));
   

   Is this change necessary?

   It looks like the scope of #2559467: Replace !placeholder with @placeholder in migrate and migrate_drupal modules

Some uses of ! are for non-HTML contexts where no escaping or filtering is wanted. For example, suppose you want to send an email with a subject that says <script> tags are dangerous which you do via t('!tags are dangerous', ['!tags' => '<script>']). AFAICT, this issue would make that impossible. I don't think that needs to block this issue though, and one possible solution to that is #2509218: Ensure that SafeString objects can be used in non-HTML contexts.

@effulgentsia with this patch we could still do:

$subject = t('!tags are dangerous', ['!tags' => SafeString::create('<script>')]);

or instead of SafeString any other object implementing SafeStringInterface.

@joelpittet re point 4 I agree it is oddly named... it "passes through" most of HTML tags ;)

When I hear passthrough, I personally think of "pass through everything and anything". If we do think of a better name, maybe we could still deprecate the passthrough name?

point 5 is necessary as !placeholder now XSS filters.. so we'd need to update the unit test anyway to use Xss::filterAdmin() if we sticked with "!". But actually escaping seems more correct here.

I think we have to leave |passthrough but at least that has meaning with a "filter":) Whereas |passthrough and escape unsafe... a bit more confusing:P

indeed!

reroll

I think we have to leave |passthrough

I disagree. I think |passthrough should be removed from core. I can't think of what a legitimate use of it would be. I think it adds confusion and has 0 value. But, removing it doesn't need to be part of this issue's scope. We have #2282101: Remove |passthrough filter in Twig for it.

@effulgentsia ah well in that case you're likely right since '!' doesn't provide raw values anymore it is kinda of useless because it's name is strange and a bit unlikely to be used as it was only added for feature parity with format_string() when it was put in.

Only reason I mentioned we have to leave it is to avoid unnecessary changes before RC, but if you or another committer agree to commit it, cool with me, less crufty stuff.

Did some minor nitpick clean-up. Unnecessary use case because both in Utility and indent on Twig comment.

Reviewed the code one more (few more) time(s) and I think this is good to go. @xjm which change records need updating?

Drafted a change record https://www.drupal.org/node/2560761

We should probably update https://www.drupal.org/node/2445441 also (or instead). Thanks @joelpittet!

Thanks @xjm i've updated that one see the diff.
https://www.drupal.org/node/2445441/revisions/view/8837825/8837949

Made two small updates, xjm/joelpittet can you check this looks sane?

https://www.drupal.org/node/2445441/revisions/view/8837949/8838119
https://www.drupal.org/node/2560761/revisions/view/8834621/8838125

@stefan.r I preferred my way of showing the difference by showing same input and how the results differ but if there is presendent to show it the way you changed it then fair enough.
I just clarified an it https://www.drupal.org/node/2560761/revisions/view/8838125/8838207

The other changes on the older CR seem fine to me too.

Actually you're right, that may be clearer and getting rid of the print statements was not something I deliberately did, was just out of habit.

I have reverted that, it does a print + comment now.

Sorry for bumping this back.. Found few more nits that could be fixed:

1. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -198,25 +198,24 @@ public static function checkPlain($text) {
   +   *     XSS/HTML filter for admin-only use will be applied. Only
       *     use this when the resulting string is being generated for one of:
   

   Line change too early

2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
   @@ -127,12 +128,11 @@ public function getFilters() {
   +      new \Twig_SimpleFilter('passthrough', [$this, 'passthrough'], array('is_safe' => array('html'))),
   

   Mixed use of [] and array() could be changed to one or the other

3. +++ b/core/modules/system/tests/modules/twig_extension_test/twig_extension_test.module
   @@ -11,7 +11,7 @@
   +      'variables' => array('message' => NULL, 'safe_join_items' => NULL, 'placeholder_text' => NULL, 'passthrough_text' => NULL, 'passthrough_safe_text' => NULL),
   

   This could be converted into multi line array

4. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
   @@ -59,11 +69,11 @@
   +{% set token = {'name': 'complex token', 'numbers': '12345', 'bad_text': '<script>&"<></script>' } %}
   

   This could be also on multiple lines

@lauriii those are all nitpicks, they shouldn't block RTBC and can be fixed on commit.

Thanks for reviewing the issue but I don't think those should hold it up.

Thanks @lauriii & @joelpittet, let's have these fixed on commit then.

Multi line arrays++, the big single line ones always give grief with patches. In fact the exact one in point 3 clashed with another commit a few days ago :)

@lauriii feel free to post a nit-fix or I will if it's not committed by the time I get to the office.

Nits picked, thanks again for noticing them @lauriii just hope not block on nits.

Thanks for fixing the nits :) Usually I try to not block the commit if there is very few nits but 4 nits might be already something that is better to handle in a new patch :)

Haha you have a nit breaking point:) aka "nit count breakpoint"

Use only for fields where it is impractical to use the whole filter system, but where some (mainly inline) mark-up is desired (so \Drupal\Component\Utility\SafeMarkup::checkPlain() is not acceptable).

This is for text fields, not for things that go in attributes. ! is often (in fact mostly when it's not being mis-used) used for URLs.

I think we need to know what happens if you put something in !placeholder that would get caught by Xss:filterBadProtocol() - which is run on things that are attributes by Xss::filterAdmin() through 2-3 levels of function call, but not literal strings that can end up as attributes.

That needs tests, so marking CNW.

Also I'd really like to see this reviewed by either chx or Heine prior to commit.

Also if we do a bad job here and leave an attack vector like protocols or something else, then this could make things worse rather than better - because instead of filtering/escaping before using !, people will assume we covered it in this issue.

Adding the "official" signoff tag (that may not actually be used yet, but time to start maybe). ;)

Also, I don't think this patch is actually doing what my original idea was, which was to XSS filter the whole string instead of marking it unsafe (not just the placeholder). That would be more equivalent to what #markup does. That is a bigger API break and carries the risk of conflating filtering and escaping further, but we could also have more confidence about it.

Hey that may be a better idea @xjm. XSS::filtering the whole thing may fix the problem @catch said. Because if we don't do that then that vector will still exist (as it did before) for

print t('<a href="!url">Text</a>', ['!url' => 'javascript:alert(\'hi\')']);

Will this do the trick or have I misunderstood the task?

Maybe a bit too far on all the unsafe things.

Well we're breaking BC anyway and this is what #markup does as well, so filtering the whole string before marking it safe may not be a bad idea and should also resolve part of the security concern earlier.

If there's a combination of @placeholders and !placeholders and we want to prevent escaping and then filtering the same piece of text, just wondering if we should leave the processing of @placeholders until after filtering the whole string? Or would that cause problems?

If that would actually be fine, I also wonder if just for consistency we shouldn't do the same regardless of whether we pass in !placeholders or not - i.e. just admin filter the whole string before replacing @placeholders, even if there are no !placeholders at all.

Marked #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand postponed on this.

@alexpott if I understand correctly what we'd gain with your idea is we make code that had been using !placeholders for anything other than URLs obsolete as it might have been insecure and buggy anyway, and it'll now be more clearly documented what !placeholders are for. If people want raw output we'll offer something else for that, which will still have similar security risks but won't touch the safe list?

And !placeholders could only be used anymore for SafeMarkup::format('<a href="!url">link</a>', ['!url' => $url]); (similarly with t()), which is 90% of use cases, but what would this look like for the other 10% where we raw output for something that is not a URL, could you give some code examples to illustrate?

@alexpott doesn't the current patch make headway or what is wrong with it specifically? What triggering the new plan with regards to this issue?

This removes escaping from !, which it never used to have and now is getting back to less confusion with @. For me it's a marked improvement especially when I was looking at the CR and how things made more sense to me now.

I'm for having @ and % escape always as it did in D7, but I'd put that on a follow-up. I'd like to put the done sticker on this and take it out of the oven personally. But obviously if this won't work as a way forward then let's flesh out the problem a bit more.

So D7 contrib also did things like these, these would need to be refactored mostly?

'!username' => theme('username', array('account' => $variables['author'])),
'!errors' => theme('item_list', array('items' => $items)))) . '</div>',
'!ctools_plugin_example_help' => theme('advanced_help_topic', array('module' => 'ctools_plugin_example', 'topic' => 'Chaos-Tools--CTools--Plugin-Examples', 'type' => 'title')),
'!geocoder_api' => '<a href="http://geocoder.ca/?premium_api=1">Geocoder API</a>'
'!url' => l($t('PDF settings page'), 'admin/config/user-interface/print/pdf'))),

Yes those would need refactoring.

The advantage though of refactoring them is that in most cases the string being created is often unsuitable for translation.

@alexpott pointed out https://api.drupal.org/api/drupal/core%21includes%21file.inc/function/fi... as a place that would break if we restrict ! to URLs.

The <br /> tag assumes that the .htaccess lines will be the last part of the string (because they're actually supposed to be a block level element below the string). However without knowing that the placeholder is a huge block level element, you might well translate the sentence from:

Security warning: Couldn't write .htaccess file. Please create a .htaccess file in your %directory directory which contains the following lines: <code>!htaccess

to:

Security warning: please create a .htaccess file in your %directory containing !htaccess because we were unable to write to the file.

Then the formatting when it's actually shown to the user would be completely wrong.

@alexpott thank you for the lay of the land in #69, that was helpful.

So can we confidently use UrlHelper::filterBadProtocol() all unsafe arguments assuming some may not be href attributes? i.e. for the other 10%.

I think that would be ideal because it would be much safer than it is now with escaping. Just want to ask the question.

This would mean for this patch, we'd not touch the entire string, just the arguments. Which I'm all for because it's back before my last 2 patches but with extra security. win-win.

Hopefully I interpreted intentions correctly. Here's the cheaper and more safe patch.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -228,16 +227,14 @@ public static function format($string, array $args) {
+            // Escape and filter bad protocols.
+            $value = Html::escape($value);
+            $args[$key] = UrlHelper::filterBadProtocol($value);

should we do a regex assert() here making sure the !placeholder is inside a href attribute?

@xjm mentioned changing "!" to be a URL placeholder is a completely non-transparent BC break - doing the assert would at least make the BC break explicit and a bit more transparent

Just to add to this, XSS filtering everything coming out of t()/SafeMarkup::format() (perhaps conditionally on whether we have !placeholders or not) feels like just as big of a BC break, and a more complex and confusing one too. So I would actually lean toward something like the current patch with that explicit assert failure if developers leave any !placeholders that are not inside an href attribute. Assuming test coverage, for contrib they'd just need to make their tests green to fix this.

Considering how bad the implementation in HEAD is I don't think we can prevent ugly BC breaks anyway - and both options (both the filtering and the repurposing '!' to be a href placeholder) seem a lot better than leaving this broken :)

Just to be clear, I meant to say #75 is on the right path if we decide to go for the URL !placeholders. But some of the assert() checks will fail (and the patch will be red) until we've removed all usage of "!" from non-URLs in core. So this would be blocked on:

#2564321: file_save_htaccess() generates error logs which are escaped incorrectly
#2509218: Ensure that SafeString objects can be used in non-HTML contexts
#2564353: Remove !placeholder usage from SafeMarkup::format()

Re #80

e effect of https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Utility!UrlH... is to:

1. strip bad protocols
2. Html::escape()

So at least some other strings that got passed as !placeholder before would still be OK - like the results of most t() calls.

Given that it doesn't seem that destructive to do this.

I think we should move the assert() to a follow-up, we haven't used that to communicate an API change before, I'd more expect trigger_error() and E_USER_DEPRECATED in a situation like that - but given a lot of strings will still work and the worst that can happen is double escaping, not sure we need to be as aggressive as that.

If we do the assert() in a follow-up, the three issue in #80 don't need to be hard blockers.

Also even if they were, I think we still need to agree properly that the URL !placeholder is what we're doing.

@catch Oh I should have caught that, will remove the double escaping introduced.

The !url placeholder seems to be the biggest use-case by far and seems like the logical approach, and removes a big chunk of another patch we are working on #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations

Regarding #82: a trigger_error() with E_USER_DEPRECATED would be fine as well, it would amount to the same thing as an assert(), just that it would also end up in the error logs of production setups that have deprecated notices in their reporting level.

There'd also be a tiny performance cost as we'd evaluate the test condition on every !placeholder call even on production setups. But the trigger_error could be removed after x amount of months whenever people have gotten the message.

One worry though, if we're still lenient about where people can use !placeholders and we essentially say "just leave them there, it might just work, the worst that can happen is double escaping": double escaping is actally a pretty bad thing to have on a live site, and this won't help the confusion about the re-purposing of the !prefix.

I've been trying to confirm the % of !placeholder usage in core that are URL as noted in #69, #72. I can't find any statement in #72 that is incorrect, but I've been trying with a few different regexes to specifically look for !placeholder in t() that is not preceded by href=. Here's an example:
[\s>.\(]t\('[^']*(?<!href=")![a-zA-Z]

A search with the above regex in core produces over 180 matches. It may be getting late for my eyes, but most of them do look like of instances of !placeholder not in href

Handy regex @justAChris:) The first one it found was this one which we should discuss:

t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href="!server_url">!server_url</a>.', array('!server_url' => $server_url)),

Where the placeholder is used inside and outside the href. Would the solution be adding two placeholders?

Edit: NM that's silly we can leave them both as !, if it's unsafe it will be escaped and protocol filtered, if it's safe it won't have tags in it.

This patch fixes the test failures in #75 and removes the double escaping mentioned in #82

In case anyone wants to crunch some contrib data, attaching a grep for !placeholders from drush making https://github.com/Berdir/d8modulestatus/blob/master/project.make (D8) as well as from a D7 distribution with 130 modules

Trying to update the issue summary and title to match the new proposal.

I'd prefer to have separate issues for the separate proposals, as they are radically different and need separate review. The recent change of direction makes it more difficult to get the security team review for the earlier proposal.

Also, I think the current suggestion is not a good idea. Using !placeholder for URLs was arguably not correct in the first place; it's only sufficient for sanitization when used with a method that already does the full URL filtering, like check_url() or whatever the D7 equivalent was. Changing a generic API for "don't escape whatever I put in this translation placeholder" for "do a specific thing that only will work in some usecases but not most of them" is worse than a clean, complete, explicit BC break in terms of the disruption.

From the patch:

+++ b/core/modules/system/src/Tests/Common/XssUnitTest.php
@@ -41,7 +41,10 @@ function testT() {
+    $this->assertEqual($text, '<a href="alert(&#039;red card&#039;)">Protocol stripped out<a>', 't XSS admin filters unsafe attribute values.');

This is an example of where we should test using an an exploit that does not contain escapeable characters. #2506021: When testing for XSS with assembled HTML, use a test exploit that does not contain quotes

Using !placeholder for URLs was arguably not correct in the first place; it's only sufficient for sanitization when used with a method that already does the full URL filtering, like check_url() or whatever the D7 equivalent was.

That's the case when the URL comes from user input, but many, many cases where ! is used it's for string literals or generated URLs (both in 7.x and 8.x) where the check_url() is superfluous.

Having discussed this with @catch, let's rename this back to its old name and go back to discussing the XSS filtering here, as @xjm has raised some concerns about having the !placeholders be for URLs and this issue became about something else as of #66. I think we are also still waiting for that security review.

Opened #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols for looking into the URL placeholders

Retitling back to the patch here, since #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand is the overall issue about the problem.

The committers agreed this morning that are ruling out this option, so closing as wontfix.

Thanks everyone for your hard work exploring this issue and fleshing out its strengths and weaknesses!

Also (as an aside) It now looks a shame that @ and % only escape if the text is not marked safe.

For what it's worth, I have a patch in #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure that tries to change that. It's not as disruptive as you might think.

I think it may be necessary to remove that behavior from SafeMarkup::format() for both security and bugfix reasons.