hook_tokens() $sanitize option incompatible with Html sanitisation requirements

"suitable for HTML" means always sanitized?

Posting a patch that breaks everything, but doesn't switch to escaping yet

* Patch needed a re-roll.
* updated unit tests which were still explicitly testing for unsanitized token values.

@mr.baileys, @stefan.r, @pwolanin, and I discussed this issue extensively during the DrupalCon Barcelona sprints.

+++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
@@ -188,9 +188,8 @@ public function viewElements(FieldItemListInterface $items) {
+        // @todo fix this
+        $link_title = \Drupal::token()->replace($item->title, array($entity->getEntityTypeId() => $entity), array('clear' => TRUE));

I believe this line is an excellent example of why this change is difficult. $link_title contains text that is unsanitized (the original title string minus the token tags), and text that is sanitized (the token replacement values). Because this is one single string, we must either leave the unsanitized parts unsanitized, or sanitize the entire string, causing the token replacement values to be sanitized twice.

My suggestion is to explore whether we can remove sanitizing from the token API and move the responsibility for doing this to the calling code. We have many more safety nets for this in D8 than we ever had in D7, so it's a potentially manageable approach.

Alright, taking a stab at that approach.

Another reason why this is a security improvement, is that the more context we pass on to our filtering/escaping code (the longer the strings are, ish), the better it can filter/escape security hazards.

Some extra test coverage for this probably wouldn't hurt..

1. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -28,16 +30,33 @@
   +   * The renderer. We depend on the XSS filtering in
   +   * \Drupal\Core\Render|Renderer::renderPlain(), so we cannot type hint on
   

   Missing line change

2. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -45,7 +64,7 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
   +    return new static($configuration, $plugin_id, $plugin_definition, $container->get('token'), $container->get('renderer'));
   

   This is getting big, maybe change it for multi line syntax

3. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -55,7 +74,15 @@ public function execute($entity = NULL) {
   +    $build = [
   +      '#markup' => $this->token->replace($this->configuration['message'], $this->configuration),
   +    ];
   +    $message = $this->renderer->renderPlain($build);
   

   Can you explain why this escaping method was chosen?

4. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -271,6 +271,8 @@ public function getUploadLocation($data = array()) {
   +    // @todo Is any valid URI always safe output? If so, handle invalid URIs
   +    //   here, and certainly do not return them.
        return $settings['uri_scheme'] . '://' . $destination;
   

   UrlHelper::stripDangerousProtocols()

+++ b/core/modules/action/src/Plugin/Action/MessageAction.php
@@ -47,11 +47,6 @@
-   * @param \Drupal\Core\Render\Renderer $renderer
-   * The renderer. We depend on the XSS filtering in
-   * \Drupal\Core\Render|Renderer::renderPlain(), so we cannot type hint on
-   * \Drupal\Core\Render\RendererInterface.

Why did you remove this documentation?

We should remove $sanitize and return things 'suitable for an Html context'.

+1

it's changing the default return value generally from something escaped to not escaped

Huh? You mean the other way, right?

Doing that should be release blocking

Maybe it should be regardless, but I think it's less clear cut if what we end up returning is always safe for HTML usage.

2. Doing that should be release blocking since it's changing the default return value generally from something escaped to not escaped.

Huh? You mean the other way, right?

It will be in the format which can be accepted by the output strategies.

Looking at the test fails

Fixing one token output and the test. Since we want the process node body or summary, the test needs to have that as the expected value.

Need to check other tokens in the patch.

Some notes:

* hook_tokens() should document what the result should be
* MessageAction doesn't need use Drupal\Core\Render\RendererInterface; or use Drupal\Component\Utility\Xss; anymore
* TestExample has also an unused used statement
* Token::replace() should have documentation what the result is (the HTML representation and other particular bit if it constructs HTML should escape it properly)

We'll want to do #27 for comments and taxonomy terms as well. Checked with @amateescu and ->processed will use the filter that is defined for the body field which seems like the right thing to do here.

ok, working on those now.

We should remove $sanitize and return things 'suitable for an Html context'

I'm confused what makes the following (and most of the other changes in the patch) suitable for HTML context:

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -185,7 +179,9 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
+   *   Text with tokens replaced. Note that the token replacements may contain
+   *   user input and the resulting string should be considered unsafe and needs
+   *   to be escaped or filtered before output.
+++ b/core/lib/Drupal/Core/Utility/token.api.php
@@ -97,7 +96,7 @@ function hook_tokens($type, $tokens, array $data, array $options, \Drupal\Core\R
-          $replacements[$original] = $sanitize ? Html::escape($node->getTitle()) : $node->getTitle();
+          $replacements[$original] = $node->getTitle();
+++ b/core/modules/comment/comment.tokens.inc
@@ -146,23 +144,23 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
-          $replacements[$original] = $sanitize ? Html::escape($mail) : $mail;
+          $replacements[$original] = $mail;

Or is the idea that something else (e.g., Token::replace()) will then convert all these values to the same thing that SafeMarkup::format() does for "@" placeholders?

@effulgentsia - the idea is to get a consistent return value and then escape/filter where needed.

Re #34, sure, but seems to me that "where needed" is before Token::replace() is done. Otherwise, what Token::replace() returns isn't suitable for HTML, and a caller would have no way to make it suitable for HTML. E.g., how would the caller be able to make "[node:title] [node:body]" suitable for HTML if Token::replace() doesn't?

But I kind of like the idea of making Token::replace() itself Html::escape() all values that aren't SafeString already prior to doing the str_replace(). Would there be anything wrong with that?

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -179,7 +179,9 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
+   *   Text with tokens replaced. Note that the token replacements may contain
+   *   user input and the resulting string should be considered unsafe and needs
+   *   to be escaped or filtered before output.

Shouldn't replace take care of that? The result should be proper HTML.

But I kind of like the idea of making Token::replace() itself Html::escape() all values that aren't SafeString already prior to doing the str_replace(). Would there be anything wrong with that?

We actually need it, as otherwise we easily end up with escaping, don't we?

The assignment confuses me

@effulgentsia - that sounds like the complexity we wanted to avoid. We want to rely on autoescape or otherwise avoid early escaping.

if I got a node body (which is HTML) I don't want it escaped.

So let's get back to an example of actual usecases of token:

$string = 'Foo <h2>{{ title}} </h2>';
Token::replace($string);

so how do we ensure that the output will be safe at some point? Escaping outside would not be right.

This is what alex and discussed.

Overall looks good, found a few things.

1. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -271,6 +271,8 @@ public function getUploadLocation($data = array()) {
   +    // @todo Is any valid URI always safe output? If so, handle invalid URIs
   +    //   here, and certainly do not return them.
   

   We should not add a TODO without a link to an issue (so we know this will actually be done)

2. +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
   @@ -55,25 +55,25 @@ function testNodeTokenReplacement() {
   -      'body' => array(array('value' => $this->randomMachineName(32), 'summary' => $this->randomMachineName(16), 'format' => 'plain_text')),
   +      'body' => array(array('value' => 'Regular NODE body for the test.', 'summary' => 'Fancy NODE summary.', 'format' => 'plain_text')),
   
   @@ -104,49 +104,29 @@ function testNodeTokenReplacement() {
   -      'body' => array(array('value' => $this->randomMachineName(32), 'format' => 'plain_text')),
   +      'body' => array(array('value' => 'A string that looks random like TR5c2I', 'format' => 'plain_text')),
   

   Why stop using randomMachineName? Is this change really needed?

To illustrate #35, I tried the following:

First installed HEAD and:

1. Set Drupal's emailer to test_mail_collector by going to /admin/config/development/configuration/single/export, exporting the config for "Simple configuration" => "system.mail", copying it to my clipboard, going to the Import tab, reselecting "Simple configuration" => "system.mail", pasting what I had copied, changing "php_mail" to "test_mail_collector", and clicking the "Import" button.
2. Enabled Action module and created a "Send email" Advanced action and configured it with the following value for Message:
   

   Title: [node:title]
   
   Body: [node:body]
   

3. Created and enabled an action_test.module with the following code:
   

   use Drupal\node\NodeInterface;
   use Drupal\system\Entity\Action;
   function action_test_node_presave(NodeInterface $node) {
     $action = Action::load('send_email');
     $action->execute([$node]);
   }
   

4. Created and saved a node with a title of Use <em> tags instead of <i> tags and a body of <p><strong>Strong</strong> and <em>emphasized</em>.</p> (I generated the latter by just typing "Strong and emphasized." into the CKEditor and then bolding "Strong" and italicizing "emphasized".)
5. Looked to see what got added to my database in the {key_value} table for collection=state and name=system.test_mail_collector. The entry contains a bunch of properties, but the relevant one for this issue is:
   

   s:4:"body";s:76:"Title: Use <em> tags instead of <i> tags
   
   Body:
   *Strong* and /emphasized/.
   
   "
   

6. The above looks correct to me: within a plain-text email, the node title is what I entered and the node body got reformatted from HTML markup to the "*" and "/" replacements per MailFormatHelper::htmlToText().

Next I applied #41 and:

1. Edited and resaved the node without making any changes.
2. Looked to see what now got added to the system.test_mail_collector state entry and found:
   

   s:4:"body";s:75:"Title: Use / tags instead of / tags
   
   Body: /
   *Strong* and /emphasized/.
   
   /
   "
   

3. The above doesn't seem desirable at all. What happened is that because the node title didn't get escaped in the token replacement, it got treated as HTML within the preparation of the email body and MailFormatHelper::htmlToText() tried to perform its conversions on that, but those tags are unclosed, so it found some random place further in the string to insert the closing "/"s.

I still think that #36 would be the approach that would actually do what the proposed resolution in the issue summary says:

always return values suitable for HTML

"Suitable for HTML" means [node:title] needs to get escaped and [node:body] does not. Which if we make the ->processed property of formatted text fields return a SafeString and if we make Token::replace() run the same logic that SafeMarkup::format() runs on @ placeholders, would be what we'd get.

@effulgentsia - it's not at all clear to me why [node:title] should be escaped and [node:body] not?

Re #44, because Node::baseFieldDefinitions() defines 'title' as being of type 'string' and field.storage.node.body.yml defines 'body' as being of type 'text_with_summary'.

And Drupal\Core\Field\Plugin\Field\FieldType\StringItem has these annotations:

 *   label = @Translation("Text (plain)"),
 *   description = @Translation("A field containing a plain string value."),

And Drupal\text\Plugin\Field\FieldType\TextWithSummaryItem has these annotations:

 *   label = @Translation("Text (formatted, long, with summary)"),
 *   description = @Translation("This field stores long text with a format and an optional summary."),

Which to me means that string fields (such as node title) need to be interpreted as plain-text while text_with_summary fields (such as node body) need to be interpreted based on whatever their format is, which means their ->processed property needs to be treated as HTML.

#82751: Allow some HTML tags in node titles is an issue for allowing node titles to have HTML, but IMO, any patch that does that would need to either change the 'title' field from type string to one of the 'text*' types, or would need to redefine what type 'string' means.

#45 and #46 seem in line with what me and @Xano had in mind here. We want this to return HTML - without any special sanitization applied to it (either escaping or filtering) -- except if we're dealing with plain text, in which case we convert to HTML by escaping.

#42.2 is not technically needed but we don't gain anything either by using random names everywhere, other than potentially chasing down random fails.

So for all the fields we need to figure out their type, HTML escape anyway if they're to be interpreted as plain text and not do any sanitization if they're to be interpreted as HTML.

Addressing some previous feedback in the mean time,

Just to make my suggestion in #36 concrete, this includes the change to Token::replace() that I'm recommending. This also reverts all the test changes, since with this approach, those will all need to be re-evaluated.

With this approach, Twig's {{ foo }}, t()'s @foo, and Token::replace()'s [foo] all behave the same way.

So for all the fields we need to figure out their type, HTML escape anyway if they're to be interpreted as plain text and not do any sanitization if they're to be interpreted as HTML.

I think this patch mostly covers it. Text fields' ->processed is already a SafeString, so that covers node body and term descriptions. Most everything else should be escaped, which is what happens if nothing says otherwise. Possibly the only things not yet covered here are configuration values that we want to treat as HTML, such as site slogan and vocabulary description. Those we probably want the corresponding hook_tokens() implementation to Xss:filterAdmin() and then wrap in a SafeString. Or put them in a #markup element and drupal_render() that element, which amounts to the same thing.

I do not see Dave Reid in this issue. That's worrisome.

@chx Informed him on twitter, maybe that works better than components on drupal.org, which IMHO don't help at all for that regard of information

Expanded the test coverage, to document what exactly should happen for various cases. The tricky thing is certainly, we here something like t() but with user input.

Still missing to address comment about @todo from #42.

@googletorp I think #49 accidently threw away my changes in #48 and rolled the new patch against an older version - will reapply the interdiff

Hmm actually this is a completely different approach to what was discussed previously. Not touching this for now.

With this approach, Twig's {{ foo }}, t()'s @foo, and Token::replace()'s [foo] all behave the same way.

I don't really see this... these all do a safety check and otherwise escape now but isn't that's where the similarity ends as these all have widely differing use cases?

It would be good to see a list of pros and cons of both approaches - this last one where we santize the placeholders separately feels like an expansion of scope but does seem worth exploring.

It feels like we should have a better way to Xss filter and mark a string safe than this:

+    $build = [
+      '#markup' => $this->token->replace($this->configuration['message'], $this->configuration),
+    ];

The patch is injecting the renderer and adding another dependency but we are actually not rendering at all?

While I understand @effulgentsia's concerns, I do want to add that any string that contains HTML can be perfectly acceptable plain text, as long as it's used outside an HTML context. If we escape node titles in node_tokens(), for instance, we also run a high risk of accidentally escaping it again later on in the process.

The patch is injecting the renderer and adding another dependency but we are actually not rendering at all?

I have a response to that: #2573233: Add a proxy variant of the renderer

It feels like we should have a better way to Xss filter and mark a string safe than this:

Well, its the way how I think we should do it. It promotes the primary way of how we deal with things: render API.

Here are a couple test fixes, working on the last one. NR just for the bot

last test fix

+++ b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
@@ -217,11 +217,6 @@ function testAccountLanguageSettingsUI() {
     $this->drupalGet('admin/config/people/accounts');
     $this->assertFieldChecked('edit-language-content-translation');
-
-    // Make sure account settings can be saved.
-    $this->drupalPostForm('admin/config/people/accounts', array('anonymous' => 'Save me please!'), 'Save configuration');
-    $this->assertFieldByName('anonymous', 'Save me please!', 'Anonymous name has been changed.');
-    $this->assertText('The configuration options have been saved.');
   }

Looks like you have a bunch of accidental reverts in the patch?

I'm deeply, deeply concerned about this change and its affect on contrib. For history, the Token module used to literally provide different tokens like [node:title-raw] before we could use $options['sanitize'], but it becomes much harder for end users to manage all those tokens, and know which one to use when. While I agree that it would be nice if we could provide context like mail vs plain text vs html.

My concern is that modules like Pathauto will exclusively have to rely on un-HTMLing and un-escaping every single token, and we've found that the process is not reliable 100% of the time. The issue summary here does not give contrib its due process for how to handle this change besides basically "sorry, you no longer can do this even though you could in Drupal 7." Using MailFormatHelper::htmlToText() is just not an option in a lot of those cases.

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -210,8 +212,12 @@ public function replace($text, array $data = array(), array $options = array(),
+    $tokens = [];
+    $values = [];
+    foreach ($replacements as $token => $value) {
+      $tokens[] = $token;
+      $values[] = SafeMarkup::isSafe($value) ? (string) $value : Html::escape($value);
+    }

Why couldn't this code conditionally run here if $options['sanitize'] is TRUE? I like that we're simplifying all the code in the individual hook_tokens() implementations, but I do not like that this is forced on every use of Token::replace().

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -210,8 +212,12 @@ public function replace($text, array $data = array(), array $options = array(),
+      $values[] = SafeMarkup::isSafe($value) ? (string) $value : Html::escape($value);

Let's just change this to

$options += ['sanitize' => TRUE];
$values[] = !$options['sanitize'] || SafeMarkup::isSafe($value) ? (string) $value : Html::escape($value);

I still propose #70 as the way for us to not break things completely for contrib, seeing as we are close to RC1, and still make this change for most people. I'm just still *extremely* concerned that this is a very short-sighted change without much concern for contrib, which makes *extensive* use of this API, much more than core does.

If #2509218: Ensure that SafeString objects can be used in non-HTML contexts were available, I would prefer if we could select an output strategy directly in the arguments to Token::replace() as an alternative to having to manually unescape everything, which is also the DX regression caused by this change.

If #2509218: Ensure that SafeString objects can be used in non-HTML contexts were available, I would prefer if we could select an output strategy directly in the arguments to Token::replace() as an alternative to having to manually unescape everything, which is also the DX regression caused by this change.

This is a good point, but that particular developer experience improvement is not critical for itself, but we can improve that in a major task, still, given that it will be an optional argument.
Do you think this is okay?

@Dave Reid: I looked at what pathauto currently does, both the 7.x-1.x branch in https://www.drupal.org/project/pathauto and the current 8.x port in https://github.com/md-systems/pathauto. In addition to passing 'sanitize' => FALSE, they both do this to the return value:

// In 8.x github sandbox
$output = strip_tags(Html::decodeEntities($string));

// In 7.x-1.x
$output = strip_tags(decode_entities($string));

That there's that line of code in both the 7.x and 8.x versions is already an indication that 'sanitize' => FALSE is not working as pathauto wants it to work with respect to getting back something that's not HTML. You could not pass 'sanitize' => FALSE and still end up with the same thing if you run the above on the output.

Furthermore, by decoding and then stripping, you're also losing "HTML" that was entered into plain text and escaped. See more about that in #43, but basically I tried a node title of Use <em> tags instead of <i> tags, and again, regardless of the 'sanitize' parameter, by pathauto decoding and then stripping, it ends up with a URL alias of use-tags-instead-tags, which doesn't contain the most important meaning of the node title.

Instead, in #2509218: Ensure that SafeString objects can be used in non-HTML contexts, we have a PlainTextSimpleOutput strategy that strips tags, then decodes, so if pathauto uses Token::replace() with no 'sanitize' parameter, and then calls:

$output = PlainTextSimpleOutput::renderFromHtml($string);

It would end up with a URL alias of use-em-tags-instead-i-tags (due to the removal of punctuation characters that PathautoManager::cleanString() also does), which I think would be better.

I don't think that in pathauto's case, having to call PlainTextSimpleOutput:: renderFromHtml($string) instead of strip_tags(Html::decodeEntities($string)) is a DX regression. But, adding a $render_strategy argument to Token::replace() to make the DX better also seems like a proposal worth considering.

adding a $render_strategy argument to Token::replace() is not on the short path to victory here. Let's not add more complexity.

I think we have a consensus plan and should get this fixed as planned.

+++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
@@ -53,26 +53,26 @@ function testCommentTokenReplacement() {
-    $tests['[comment:author]'] = Xss::filter($comment->getAuthorName());
-    $tests['[comment:title]'] = Xss::filter($comment->getSubject());
+    $tests['[comment:author]'] = Html::escape($comment->getAuthorName());
+    $tests['[comment:title]'] = Html::escape($comment->getSubject());

For a clean patch review and git history, this should be committed separately. I opened #2573657: comment_tokens() filters instead of escapes some plain-text strings for that.

Some small test additions.

@effulgentsia - I don't think it makes sense to commit separately. We can can specify that on any backport issue?

I don't think it makes sense to commit separately

I think it's important to commit separately the parts that actually change the output of what callers that currently call Token::replace() without a 'sanitize' => FALSE would get for any core token. Then the rest of this patch is just a removal of 'sanitize' => FALSE and refactor of the Html::escape() responsibility from the hook_tokens() implementation to Token::replace() itself. #2573657: comment_tokens() filters instead of escapes some plain-text strings is the only such output change in this patch, so I'd like committed separately.

There's also the following that I think we should revert in this patch:

1. +++ b/core/modules/system/system.tokens.inc
   @@ -111,14 +109,14 @@ function system_tokens($type, $tokens, array $data, array $options, BubbleableMe
   -          $replacements[$original] = $sanitize ? Xss::filterAdmin($slogan) : $slogan;
   +          $replacements[$original] = $slogan;
   

2. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -165,11 +164,11 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
   -          $replacements[$original] = $sanitize ? Xss::filter($vocabulary->getDescription()) : $vocabulary->getDescription();
   +          $replacements[$original] = $vocabulary->getDescription();
   

I don't think this patch should change site slogan and vocabulary description from HTML to plain-text, which is what the above lines do. These are stored in configuration rather than content entity fields, so we can't use #46 to judge that, but these are configuration values that have traditionally been interpreted as HTML. And for example, SystemBrandingBlock::build() still does.

So I think for these, we need to go back to Xss::filter(Admin?)(), but then turn them into a SafeString as well so Token::replace() then knows to not escape them.

reroll of 78

@effulgentsia I have added those 2 cases. Haven't reviewed the rest yet, so do we have any other ones still?

Fixes tests.

+++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
@@ -262,4 +263,50 @@ public function testResetInfo() {
+    // @todo what do we do with this particular case?
+    $data['simple-placeholder-with-attribute-xss'] = [
+      '<a href="[token:meh]">Giraffe</a>',
+      ['[token:meh]' => "javascript:alert('123')"],
+      '<a href="' . Html::escape("javascript:alert('123')") . '">Giraffe</a>',
+    ];
+    // @todo what do we do with this particular case?
+    $data['simple-placeholder-with-attribute-xss'] = [
+      '<a href="[token:meh]">Giraffe</a>',
+      ['[token:meh]' => '" onclick=alert(muh)"'],
+      '<a href="&quot; onclick=alert(muh)&quot;">Giraffe</a>',
+    ];
+    $data['xss-from-the-string'] = [
+      '<script>alert("123")</script>',
+      [],
+      '<script>alert("123")</script>',
+    ];

Here's a patch with my suggestion for how to resolve these @todos.

And here's some docs that explain #90.

+++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -185,7 +181,46 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
+   *     @code
+   *     $html_output = $token_service->replace($text, $data);
+   *     return [
+   *       '#markup' => Xss::filter($html_output),
+   *     ];
+   *     @endcode
+   *     Note that it is the output of this function that must be filtered. It
+   *     is not enough to only filter the $text argument, because XSS filtering
+   *     is designed to operate on a complete HTML fragment, not a string with
+   *     placeholders.

There's an interesting consequence of this that might need discussion. Say that an administrative user puts some fancy tag (e.g., <table>) into the site slogan. Then a non-administrative user creates a tokenized string such as [site:name] [site:slogan]. Then calling code following the above recommendation will filter out the site slogan's <table> tag from the resulting string. I don't know if that's good or bad, and if we decide it's bad, how it can be solved.

Hm, thinking about it more, I think #92 is a real problem.

Consider a token string with [node:body] in it on a site that also uses Media module or similar:

• If the token string itself was added by a non-admin user and #91's recommendation is followed, then even <img> tags would get stripped by Xss::filter() running on the output. It's certainly not desirable to strip images from a rendered node body just because the person who entered the token string isn't allowed to insert his or her own <img> tags into it.
• With a module like media_youtube, the rendered node body could even include <iframe> tags. And Xss::filterAdmin() strips those, so having to run Token::replace()'d output through a '#markup' means losing videos, etc. from a rendered node body.

So, I think this means that Token::replace() should return a SafeString object. But this requires trusting the $text argument itself, which usually comes from user input. We can Xss::filterAdmin() that input, but that would mean that a token string of <a href="[comment:homepage]">Foo</a> would always get converted to <a href="homepage]">Foo</a>, because [comment is not an allowed URL protocol. Which is definitely preferable to the token name not being stripped, because otherwise <a href="[comment:title]">Foo</a> could be used as a way of bypassing URL filtering (since there's no reason to expect comment_tokens() to treat 'title' as a URL). So, as long as:

1. All token names have a colon in them and without that colon don't get recognized as a token.
2. We're willing to not support a use case of allowing the token string itself (the $text parameter to Token::replace()) to use a token within any attribute that Xss::attributes() strips protocols for.
3. We're willing to not support tokenized tags, such as <[fences:tag-name]>Foo</[fences:tag-name]>, or anything else that Xss::filterAdmin() would filter out prior to those being replaced.

Then I think Token::replace() can just Xss::filterAdmin() $text and then return a SafeString, which would allow the [node:body] examples above to work properly, but still be safe in all cases.

And if callers want to sanitize $text more restrictively than Xss::filterAdmin(), then they can always call Xss::filter() themselves on it prior to passing it into Token::replace().

Thoughts?

Potentially, we can support #93.2 and #93.3 with a $option for whether to Xss::filterAdmin() $text or $output? By default the former, but for modules that want to do either 93.2 or 93.3, they could ask for the latter?

Then I think Token::replace() can just Xss::filterAdmin() $text and then return a SafeString, which would allow the [node:body] examples above to work properly, but still be safe in all cases.

Ugh. No. That approach could still leave us with the following:

// Unchanged by Xss::filterAdmin() or Xss::filter().
$text = '<a title="[comment:body]">Foo</a>';

// Unchanged by $comment->comment_body->processed.
$comment->comment_body->value = '" onclick="alert(1)';

// Result is '<a title="" onclick="alert(1)">Foo</a>', so must not be marked safe.
$output = $token_service->replace($text, ['comment' => $comment]);

Which means, if we need a solution to #92 / the [node:body] case of #93, then we need some other way of escaping or filtering $text, such as:

1. Either deciding to not support a use case of $text containing HTML at all, and therefore calling Html::escape() on it prior to doing token replacements.
2. Or filtering $text from having any tokens in attribute values. This could be accomplished via the filterAttributes() function proposed in #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default.

Ok, so after the last 4 comments, here's my current recommendation:

As part of this issue, change #91 to implement #95.2, which means:

• Either commit #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default or extract its filterAttributes() function into this patch.
• Make Token::replace() do a Xss::filterAdmin() on $text and a filterAttributes() on $text to remove all tokens from attributes.
• Make Token::replace() return a SafeString object.
• Fix TokenTest::testReplaceUnsafe() accordingly.

Then, have a non-critical follow-up to add a $options['filter_mode'] (or better name) which could be set to Token::POST_FILTER, which would thus instruct Token::replace() to not filter $text, and instead Xss::filterAdmin() the output. This would then address the rare use cases like #93.2 and #93.3.

Maybe have another non-critical follow-up to allow $options['filter_mode'] to be set to Token::NO_FILTER, which would instruct Token::replace() to neither filter $text nor the output. And consequently, just return a bare string, not a SafeString. This would allow for exotic callers that both want tokens in attributes and simultaneously want IFRAMEs within node bodies retained. But the caller would then need to know how to handle the output string. For example, the caller could turn the string into a SafeString, but would need to document what steps it has taken to ensure that it's truly safe.

I'm tempted to set to "needs work" for the bullet points above, but leaving at "needs review" since so far this is only my recommendation, so could use more consensus before implementing.

i don't know that we need to be supporting tokens in attributes - we don't in SafeMarkup::format() either.

This all sounds pretty complicated by the way. This is a hard problem (similar to translation except also with untrusted user input outside of the tokens) but isn't there an easier possible solution?

So, I think this means that Token::replace() should return a SafeString object. But this requires trusting the $text argument itself, which usually comes from user input.

I don't think we can trust $text... which makes this all pretty hard.

Maybe we can whitelist the specific iframe/whatever tag we're looking to insert somehow (and skip sanitizing that)

This is the problem with trying to make this change all at once. With 100% certainty I've used tokens used in the same blob of text used both as part of an attribute, an attribute itself, or HTML. You cannot simply know ahead of time how the user is using tokens, which is the entire reason the token API exists.

@Dave Reid I think what we mostly want is a predictable result from Token::replace() - in talks at Drupalcon BCN it was decided we'd use HTML everywhere for transporting strings, and then convert to plain text or attribute-safe values as needed (or other contexts). We assume input and output are markup in t() as well.

The problem is this "translation" is even harder than t() because the input is untrusted. The sanitize parameter seems problematic. If you have a better solution it'd be good to hear.

This is the problem with trying to make this change all at once. With 100% certainty I've used tokens used in the same blob of text used both as part of an attribute, an attribute itself, or HTML. You cannot simply know ahead of time how the user is using tokens, which is the entire reason the token API exists.

We don't necessarily have to cater to all use cases for D8 in the same string though. If truly it's all of those, maybe they could see about using other options rather than token API?

STOP

STOP

Take a deep breath, write a new issue summary (or an issue summary) because the current one and the whole issue is completely off the rails and confusing and needs to be re-focused.

We need to do a requirements analysis: what is the purpose of this API? What are the inputs? What are the output contexts? Is one API function doing too much? Should we split? How much existing functionality can we reuse?

Go!

Then I think Token::replace() can just Xss::filterAdmin() $text and then return a SafeString, which would allow the [node:body] examples above to work properly, but still be safe in all cases.

And if callers want to sanitize $text more restrictively than Xss::filterAdmin(), then they can always call Xss::filter() themselves on it prior to passing it into Token::replace().

@effulgentsia I think the previous approach was to have this applied on the full text for all cases after having used the unsanitized HTML values everywhere - could you explain what the advantages of the current approach are? Maybe if something needs to be /not/ XSS filtered we could somehow white-list it?

In any case @chx made very good point in #101, let's start by addressing that?

Looks like we need to work more on this, also we still have a @todo with no issue related to it. Todo's should always have a followup issue or we should fix it in this patch.

We had a big discussion with dawehner and catch and the simple decision is this.

#75 shows the main customer of the token API, pathauto already uses strip_tags. Let's do this. If you want to produce HTML use Twig. If you want plain text, you can use tokens.

So the plan is to use strip_tags() and/or Xss::filter() on all token output? Are we going to escape any of them?

Sorry, but tokens need to support HTML by default. There are *too many* use cases in contrib that would just simply break moving to D8. I would rather it return HTML by default (and not really care about using tokens in attributes, should be a trusted user that is allowed to do something like that), and we can add DX later for modules like Pathauto that need plain text versions only.

I'm not sure if we are talking about the token values being forced to plain text or the whole resulting output.

For the token values, I'm not sure. But the whole output isn't an option, to give two examples:

* Using HTML for the user mails (e.g. new account, password recovery). While very ugly to edit, it was always possible to add HTML in there and send HTML mails (using contrib modules)
* In simplenews, I'm using token replacements and I'm doing that *after* a node has been built and rendered. I might send the same mail to 100k recipients, with the only difference between the token values (e.g. Hi [firstname] [lastname]). I do not want to rebuild the node for every recipient. I've discussed with @dawehner that it might be a possibility to use an inline twig template with the rendered output as template, not sure yet.

+++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
@@ -268,9 +268,11 @@ public function getUploadLocation($data = array()) {
+    // @todo Is any valid URI always safe output? If so, handle invalid URIs
+    //   here, and certainly do not return them.

We still need to either fix this, or make a follow up issue which solves this, and link to it in the comment

Looking into in right now

Worked on an idea I had, let's Token::replace() return a TokenString object. This object escapes
the string by default. For some usecases, for which we need HTML, it though has an additional method ::getXssFilteredRawString()
which returns as xss filtered result of the token replacement result.

Way less test failures for now.

This has at least a failure in some link test due to some weird UTF8 weirdness.

This was a nice drupalcon!

Remaining issue: #2568045: Make it impossible to double escape with #plain_text or #2575615: Introduce HtmlEscapedText and remove SafeMarkup::setMultiple() and SafeMarkup::getAll() and remove the static safeStrings list which would solve things

$string = Html::escape($string) should be $string = Html::escape($string);

Let's fix it :).

1. +++ b/core/lib/Drupal/Component/Utility/Xss.php
   @@ -30,7 +30,7 @@ class Xss {
   -  protected static $htmlTags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd');
   +  protected static $htmlTags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'p');
   

   I think this change makes sense overall, but can you explain why it's necessary to do this here?

   (And why it's only necessary to whitelist <p> and not also <br>?)

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   The token result is the entered HTML text with tokens replaced. The
   

   "entered" is wrong here, because it implies "entered by the user", i.e. "user input". But it's really just $text with tokens replaced.

3. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   its intended to be used as plain text, the usage of
   

   s/its/it is/.

   Fixed.

4. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   PlainTextOutput::renderFromHtml() is suggested. If the result is just
   

   I think: s/suggested/recommended/ ?

5. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   printed as part of a template relying on twig autoescaping is possible,
   

   s/twig/Twig/

   Fixed.

6. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   it would be xss admin filtered.
   

   "xss admin filtered" is not very understandable.

   Fixed.

7. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -210,8 +211,13 @@ public function replace($text, array $data = array(), array $options = array(),
   +    // Normalize the token values to be formatted for plain text.
   +    $tokens = [];
   +    $values = [];
   +    foreach ($replacements as $token => $value) {
   +      $tokens[] = $token;
   +      $values[] = $value instanceof SafeStringInterface ? $value : Html::escape($value);
   +    }
   

   The comment seems to not really match the logic: if it already is a string that is known to be safe, then nothing happens. So it's totally possible that markup that is known to be safe remains markup, and is not "formatted for plain text".

8. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -65,7 +64,9 @@
   + *   strings from the original text. The returned values must be either plain
   + *   text strings, or an object implementing SafeStringInterface if they are
   + *   HTML-formatted.
   

   This comment makes a lot more sense; I think we want something similar in the previous point?

9. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -28,16 +28,33 @@
   +   * We depend on the XSS filtering in
   +   * \Drupal\Core\Render|Renderer::renderPlain(), so we cannot type hint on
   +   * \Drupal\Core\Render\RendererInterface.
   +   *
   +   * @var \Drupal\Core\Render\Renderer
   

   Interesting! I wonder if we shouldn't update RendererInterface's docs instead?

10. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
    @@ -28,16 +28,33 @@
       /**
        * Constructs a MessageAction object.
    +   *
    +   * @param \Drupal\Core\Render\Renderer $renderer
    +   *   The renderer. We depend on the XSS filtering in
    +   *   \Drupal\Core\Render|Renderer::renderPlain(), so we cannot type hint on
    +   *   \Drupal\Core\Render\RendererInterface.
        */
    -  public function __construct(array $configuration, $plugin_id, $plugin_definition, Token $token) {
    +  public function __construct(array $configuration, $plugin_id, $plugin_definition, Token $token, Renderer $renderer) {
    

    Incomplete docblock.

    Fixed.

11. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
    @@ -55,7 +78,15 @@ public function execute($entity = NULL) {
    +    // Depend on \Drupal\Core\Render\Renderer::renderPlain()'s sanitization of
    +    // the message, so it won't be fully escaped during the main rendering
    +    // process.
    

    I'm pretty sure this means it's relying on the automatic filtering, which means this indeed won't be escaped.

    But "fully escaped" and "the main rendering process" are rather vague terms.

12. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
    @@ -55,7 +78,15 @@ public function execute($entity = NULL) {
    +    $build = [
    +      '#markup' => $this->token->replace($this->configuration['message'], $this->configuration),
    +    ];
    +    $message = $this->renderer->renderPlain($build);
    

    This is just a way to let the Renderer do filtering for you, this could just as well do:

    $message = $this->token->replace($this->configuration['message'], $this->configuration);
    $message = SafeString::create(Xss::filterAdmin($message));
    

    Would (something like) that perhaps be actually better? Right now, it feels like the Render system is only used by accident.

13. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
    --- a/core/modules/comment/comment.tokens.inc
    +++ b/core/modules/comment/comment.tokens.inc
    

    Two unused use statements here now.

    Fixed.

14. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
    @@ -50,29 +53,29 @@ function testCommentTokenReplacement() {
         $tests['[comment:author]'] = Html::escape($comment->getAuthorName());
    ...
         $tests['[comment:title]'] = Html::escape($comment->getSubject());
    ...
         $tests['[comment:entity]'] = Html::escape($node->getTitle());
    ...
         $tests['[comment:entity:title]'] = Html::escape($node->getTitle());
    

    Aren't these Html::escape() calls wrong too? I suspect the only reason these still exist is because they didn't happen to cause any test failures.

    EDIT: nope, I was wrong, the auto-escaping behavior in the token service is what does this.

15. --- a/core/modules/content_translation/content_translation.module
    +++ b/core/modules/content_translation/content_translation.module
    @@ -471,14 +471,7 @@ function content_translation_language_configuration_element_process(array $eleme
         );
     
         $submit_name = isset($form['actions']['save_continue']) ? 'save_continue' : 'submit';
    -    // Only add the submit handler on the submit button if the #submit property
    -    // is already available, otherwise this breaks the form submit function.
    -    if (isset($form['actions'][$submit_name]['#submit'])) {
    -      $form['actions'][$submit_name]['#submit'][] = 'content_translation_language_configuration_element_submit';
    -    }
    -    else {
    -      $form['#submit'][] = 'content_translation_language_configuration_element_submit';
    -    }
    +    $form['actions'][$submit_name]['#submit'][] = 'content_translation_language_configuration_element_submit';
       }
       return $element;
     }
    diff --git a/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    index f1ddd4a..c928bac 100644
    --- a/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    +++ b/core/modules/content_translation/src/Tests/ContentTranslationSettingsTest.php
    @@ -217,11 +217,6 @@ function testAccountLanguageSettingsUI() {
         $this->drupalPostForm('admin/config/people/accounts', $edit, t('Save configuration'));
         $this->drupalGet('admin/config/people/accounts');
         $this->assertFieldChecked('edit-language-content-translation');
    -
    -    // Make sure account settings can be saved.
    -    $this->drupalPostForm('admin/config/people/accounts', array('anonymous' => 'Save me please!'), 'Save configuration');
    -    $this->assertFieldByName('anonymous', 'Save me please!', 'Anonymous name has been changed.');
    -    $this->assertText('The configuration options have been saved.');
       }
    

    These changes seem unrelated?

16. --- a/core/modules/file/file.module
    +++ b/core/modules/file/file.module
    

    One unused use statement because of the token hook changes here.

    Fixed.

17. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
    @@ -268,9 +268,11 @@ public function getUploadLocation($data = array()) {
    -    // Replace tokens.
    +    // Replace tokens. We do not apply any sanitization to the destination here.
    

    Shouldn't we document why?

18. +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
    --- a/core/modules/node/node.tokens.inc
    +++ b/core/modules/node/node.tokens.inc
    

    One unused use statement because of the token hook changes here.

    Fixed.

19. +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
    @@ -99,37 +99,26 @@ function testNodeTokenReplacement() {
    +    debug($tests);
    

    Debug statement that can be removed :)

    Fixed.

20. +++ b/core/modules/system/src/Tests/System/TokenReplaceUnitTest.php
    @@ -8,6 +8,7 @@
    +use Drupal\Component\Utility\PlainTextOutput;
    

    Unused.

    Fixed.

21. +++ b/core/modules/system/system.module
    --- a/core/modules/system/system.tokens.inc
    +++ b/core/modules/system/system.tokens.inc
    

    Two unused use statements because of the token hook changes here.

    Fixed.

22. +++ b/core/modules/system/system.tokens.inc
    @@ -111,14 +109,15 @@ function system_tokens($type, $tokens, array $data, array $options, BubbleableMe
    +          $build = ['#markup' => $config->get('slogan')];
    +          $slogan = \Drupal::service('renderer')->renderPlain($build);
    

    Similar remark as earlier.

23. +++ b/core/modules/system/system.tokens.inc
    --- a/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
    +++ b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
    

    Two unused use statement because of the test changes here.

    Fixed.

24. +++ b/core/modules/taxonomy/src/Tests/TokenReplaceTest.php
    --- a/core/modules/taxonomy/taxonomy.tokens.inc
    +++ b/core/modules/taxonomy/taxonomy.tokens.inc
    

    Two unused use statements because of the token hook changes here.

    Fixed.

25. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
    @@ -165,11 +164,12 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
    +          $build = ['#markup' => $vocabulary->getDescription()];
    +          $replacements[$original] = \Drupal::service('renderer')->renderPlain($build);
    

    And again.

26. +++ b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
    @@ -121,7 +121,8 @@ public function getAttributes() {
    +    $tags = Xss::getHtmlTagList();
    

    Dead code.

    Fixed.

27. +++ b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
    --- a/core/modules/user/src/Tests/UserTokenReplaceTest.php
    +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
    

    One unused use statement because of the token hook changes here.

    Fixed.

28. +++ b/core/modules/user/user.module
    --- a/core/modules/user/user.tokens.inc
    +++ b/core/modules/user/user.tokens.inc
    

    One unused use statement because of the token hook changes here.

    Fixed.

29. +++ b/core/modules/user/user.tokens.inc
    @@ -102,6 +100,7 @@ function user_tokens($type, $tokens, array $data, array $options, BubbleableMeta
    +          debug($account->getDisplayName());
    

    Leftover debug statement.

    Fixed.

30. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -162,7 +162,8 @@ public function render($empty = FALSE) {
    +        // We cast as we need the integer/string value provided by the tokenize.
    

    "the tokenize"?

31. +++ b/core/modules/views/src/Tests/Handler/AreaTest.php
    @@ -109,6 +109,8 @@ public function testRenderArea() {
    +    debug($output);
    +    debug(Xss::filterAdmin($header_string));
    

    Debug leftovers.

    Fixed.

32. +++ b/core/modules/views/src/Tests/TokenReplaceTest.php
    --- a/core/modules/views/views.tokens.inc
    +++ b/core/modules/views/views.tokens.inc
    

    One unused use statement because of the token hook changes here.

    Fixed.

33. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
    @@ -7,11 +7,16 @@
    +use Drupal\Component\Utility\SafeStringInterface;
    +use Drupal\Component\Utility\Xss;
    

    Unused.

    Fixed.

34. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
    @@ -7,11 +7,16 @@
    +use Drupal\Core\Utility\TokenString;
    

    Unused and non-existent.

    Fixed.

1. +++ b/core/lib/Drupal/Component/Utility/Xss.php
   @@ -30,7 +30,7 @@ class Xss {
   +  protected static $htmlTags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd', 'p');
   

   Where specifically do we need the p? I don't see any Xss::filter() in the patch anymore?

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -141,7 +143,9 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   A HTML string containing replaceable tokens. The caller is responsible
   +   *   for calling \Drupal\Component\Utility\Html::escape() in case the expected
   +   *   input was plain text.
   

   This could be clearer.. "expected input was"?

3. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   it is intended to be used as plain text, the usage of
   

   s/it/the result/

4. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -28,16 +28,43 @@
   +   *   The renderer. We depend on the XSS filtering in
   +   *   \Drupal\Core\Render|Renderer::renderPlain(), so we cannot type hint on
   +   *   \Drupal\Core\Render\RendererInterface.
   

   Not in scope here, but maybe we codify on the interface that the result must be XSS filtered on the interface so we can just type hint the interface instead?

5. +++ b/core/modules/comment/comment.tokens.inc
   @@ -146,23 +142,23 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   +          $replacements[$original] = UrlHelper::stripDangerousProtocols($comment->getHomepage());
   

   Is this needed? In HTML contexts we expect the caller to XSS filter, right?

6. +++ b/core/modules/comment/comment.tokens.inc
   @@ -146,23 +142,23 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   +          $replacements[$original] = $comment->comment_body->processed;
   

   I guess the processed values return a SafeStringInterface object, so they wouldn't be auto-escaped?

7. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -260,7 +260,7 @@ public static function validateMaxFilesize($element, FormStateInterface $form_st
   -   *   A file directory URI with tokens replaced.
   +   *   An unsanitized file directory URI with tokens replaced.
   
   @@ -268,9 +268,11 @@ public function getUploadLocation($data = array()) {
   -    // Replace tokens.
   +    // Replace tokens. We do not apply any sanitization to the destination here.
   ...
   +    // @todo Is any valid URI always safe output? If so, handle invalid URIs
   +    //   here, and certainly do not return them.
   

   Don't know if these changes are still needed?

8. +++ b/core/modules/user/user.module
   @@ -916,10 +917,9 @@ function user_mail($key, &$message, $params) {
   +  // @todo fix this
   

   I don't think there's anything to fix here anymore?

9. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
   @@ -162,7 +162,8 @@ public function render($empty = FALSE) {
   +        // We cast as we need the integer/string value provided by the tokenize.
   

   s/tokenize/tokenization? Also why do we need this to be a string?

Thank you @Wim Leers and @stefan.r

I think: s/suggested/recommended/ ?

Feels similar for me, fixed!

Interesting! I wonder if we shouldn't update RendererInterface's docs instead?

Interesting! I wonder if we shouldn't update RendererInterface's docs instead?

Well, I think its pointless to typehint against a specific implementation. There are way more areas in core you would have to worry about if the renderer doesn't work as you expect it to be.

Would (something like) that perhaps be actually better? Right now, it feels like the Render system is only used by accident.

Indeed this is much better IMHO

These changes seem unrelated?

Yeah I think so.

Shouldn't we document why?

Now that we use the PlainTextOutput this is no longer true.

Similar remark as earlier.

At some point we might want a XssFilteredMarkup object.

Is this needed? In HTML contexts we expect the caller to XSS filter, right?

Well, but just in case the output calls xss::filter, which might not be the case when $text is not user input.

I guess the processed values return a SafeStringInterface object, so they wouldn't be auto-escaped?

I guess the processed values return a SafeStringInterface object, so they wouldn't be auto-escaped?

I guess the processed values return a SafeStringInterface object, so they wouldn't be auto-escaped?

Yes, exactly, see \Drupal\text\TextProcessed::getValue

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -210,8 +211,13 @@ public function replace($text, array $data = array(), array $options = array(),
   +    // Escape the tokens, unless they are explicitly markup.
   +    $tokens = [];
   +    $values = [];
   +    foreach ($replacements as $token => $value) {
   +      $tokens[] = $token;
   +      $values[] = $value instanceof SafeStringInterface ? $value : Html::escape($value);
   +    }
   

   I would say this should happen *before* the 'callback' is executed above. Pathauto needs to call PlainTextOutput::renderFromHtml on each individual token, but would need to do so after this sanitization happens.

2. +++ b/core/modules/file/file.module
   @@ -984,7 +982,7 @@ function file_tokens($type, $tokens, array $data, array $options, BubbleableMeta
   -          $replacements[$original] = $sanitize ? Html::escape(file_create_url($file->getFileUri())) : file_create_url($file->getFileUri());
   +          $replacements[$original] = file_create_url($file->getFileUri());
   

   This seems suspect for me. I don't think the HTML link should be escaped by default. Should this be a safe string?

3. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -165,11 +162,12 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
   -          $replacements[$original] = $sanitize ? Xss::filter($vocabulary->getDescription()) : $vocabulary->getDescription();
   +          $build = ['#markup' => $vocabulary->getDescription()];
   +          $replacements[$original] = \Drupal::service('renderer')->renderPlain($build);
   

   Why is this not returning an Xss filtered string with SafeString?

4. +++ b/core/modules/user/user.tokens.inc
   @@ -110,14 +107,14 @@ function user_tokens($type, $tokens, array $data, array $options, BubbleableMeta
   -          $replacements[$original] = $sanitize ? Html::escape($account->getEmail()) : $account->getEmail();
   +          $replacements[$original] = $account->getEmail();
   

   I could see this one not wanting to be escaped by default. Should this be using SafeString? Also why would we need to sanitize an email address?

I would say this should happen *before* the 'callback' is executed above.

Should it really? If callbacks do their own sanitization they can mark the string as safe to skip autoescaping in Token::replace()...

Pathauto needs to call PlainTextOutput::renderFromHtml on each individual token, but would need to do so after this sanitization happens.

Could Pathauto not do this on the text with the tokens replaced rather than on the individual tokens? I don't know that we should want to have any use case that combines plain text and HTML? When outputting the string we're either in an HTML context or in a plain text context right?

Why is this not returning an Xss filtered string with SafeString?

That's what #markup does, I think SafeString is @internal and not supposed to be used in modules like taxonomy?

The idea was to use #markup + renderPlain() everywhere to create XSS filtered safe strings, just to set the right example but I agree it's a bit silly, we might want a helper method that does this as well.

I could see this one not wanting to be escaped by default. Should this be using SafeString? Also why would we need to sanitize an email address?

I think the Html::escape() is just used here to convert plain text (the email address - which might contain characters like &'"<>) into HTML?

I would say this should happen *before* the 'callback' is executed above.

I agree with #153's questioning of this. But the issue summary says that the escaping of raw strings should happen in Token::generate(), which I agree with. So that would put it before the callback.

Should it really? If callbacks do their own sanitization they can mark the string as safe to skip autoescaping in Token::replace()...

Callbacks should not be doing sanitization. That is for hook_tokens_alter().

Could Pathauto not do this on the text with the tokens replaced rather than on the individual tokens? I don't know that we should want to have any use case that combines plain text and HTML? When outputting the string we're either in an HTML context or in a plain text context right?

As an API, the callback should be executed last before token replacement. I don't think we should be changing this behavior in Drupal 8.

That's what #markup does, I think SafeString is @internal and not supposed to be used in modules like taxonomy?

The idea was to use #markup + renderPlain() everywhere to create XSS filtered safe strings, just to set the right example but I agree it's a bit silly, we might want a helper method that does this as well.

We are literally using the same patten of using SafeString elsewhere with other token values here. I'm asking for consistency with what is being used with [site:slogan] which is $replacements[$original] = SafeString::create(Xss::filterAdmin($slogan));. the taxonomy description should be doing the same.

I think the Html::escape() is just used here to convert plain text (the email address - which might contain characters like &'"<>) into HTML?

Conceded, looks like '&' is the only valid character allowed in email addresses, so that seems ok.

Also, taxonomy term description can contain HTML, so it should not be rendered as plain.

Heh, $renderder->renderPlain is confusingly named... it does render HTML :)

Callbacks should not be doing sanitization. That is for hook_tokens_alter().

Ah so we should do things before the callbacks, fair.

As an API, the callback should be executed last before token replacement. I don't think we should be changing this behavior in Drupal 8.

Sure, let's not change that now.

We are literally using the same patten of using SafeString elsewhere with other token values here. I'm asking for consistency with what is being used with [site:slogan] which is $replacements[$original] = SafeString::create(Xss::filterAdmin($slogan));. the taxonomy description should be doing the same.

What about introducing a XssFilteredMarkup much like the EscapedString? Otherwise I think we should standardize on using renderPlain() which renders a render array without taking care of #attachments

This seems suspect for me. I don't think the HTML link should be escaped by default. Should this be a safe string?

Are you sure about that? drupal_attributes() already
escaped all the values.

@dawehner: Sorry, file_create_url() returns a string URL, not a link. I don't think it needs HTML escaping because it has been generated without user input.

So per the issue summary Token::replace() will work similarly to t() in that both the input and output are expected to be HTML, i.e. when working with plain text, the input needs to be escaped to be converted to HTML and the output (the text with tokens replaced) will need to be converted from HTML using PlainTextOutput::renderFromHtml().

So even if autoescaping happens before the callback (fair enough), I don't think we should ever need to run PlainTextOutput::renderFromHtml() on individual tokens, given that PlainTextOutput is intended for converting full HTML strings, such as the output of Token::replace(), to plain text contexts, such as email? So for this to be consistent with what happens in t() and elsewhere in core, the text format conversion should rather not happen in the callback (even if it's currently documented that the callback *should* be used for that), but somewhere else (see EmailAction::execute()).

@dawehner I do think depending on the renderer and running $renderer->renderPlain(['#markup' => 'html string') is a bit bothersome, so something more convenient such as XssFilteredMarkup does make sense.

@dawehner: Sorry, file_create_url() returns a string URL, not a link. I don't think it needs HTML escaping because it has been generated without user input.

To make this patch more easily reviewable, I wonder if we could put assert()'s or comments above the $replacements[$type] statements clarifying the type all the values (i.e. SafeString/URL/link/plain text string)

To make this patch more easily reviewable, I wonder if we could put assert()'s or comments above the $replacements[$type] statements clarifying the type all the values (i.e. SafeString/URL/link/plain text string)

Added that to some pieces but I think

#2577827: Add a XssFilteredMarkup

Catch also suggested to open up a follow up to review all core tokens, just to be sure.

Looks great!

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -141,7 +143,9 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   A HTML string containing replaceable tokens. The caller is responsible
   

   What about "An HTML string..."?

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   PlainTextOutput::renderFromHtml() is recommended. If the result is just
   +   *   printed as part of a template relying on Twig autoescaping is possible,
   

   Should we mention SafeStringInterface here?

3. +++ b/core/lib/Drupal/Core/Utility/token.api.php
   @@ -97,7 +96,7 @@ function hook_tokens($type, $tokens, array $data, array $options, \Drupal\Core\R
   -          $replacements[$original] = $sanitize ? Html::escape($node->getTitle()) : $node->getTitle();
   +          $replacements[$original] = $node->getTitle();
   

   Lovely :)

4. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -55,8 +68,12 @@ public function execute($entity = NULL) {
   -    drupal_set_message($message);
   +    $message = $this->token->replace($this->configuration['message'], $this->configuration);
   +    $build = [
   +      '#markup' => $message,
   +    ];
   +
   +    drupal_set_message($this->renderer->renderPlain($build));
   

   Just curious: wouldn't this be equivalent to the following code?

   $message = Xss::filterAdmin($this->token->replace($this->configuration['message'], $this->configuration));
   drupal_set_message(SafeString::create($message));
   

   Is replacing this kind of code the goal of XssFilteredMarkup? If so +1 :)
   Also, in that case can we avoid to inject the renderer since this is just temporary code?

5. +++ b/core/modules/comment/comment.tokens.inc
   @@ -146,23 +142,25 @@ function comment_tokens($type, $tokens, array $data, array $options, BubbleableM
   +          // processed returns a \Drupal\Component\Utility\SafeStringInterface
   +          // via check_markup().
   
   +++ b/core/modules/node/node.tokens.inc
   @@ -159,12 +155,14 @@ function node_tokens($type, $tokens, array $data, array $options, BubbleableMeta
   +            // processed returns a \Drupal\Component\Utility\SafeStringInterface
   +            // via check_markup().
   
   +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -109,11 +106,13 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
   +          // processed returns a \Drupal\Component\Utility\SafeStringInterface
   +          // via check_markup().
   

   Can we enclose these occurrences of "processed" in double quotes? The comment is hard to read otherwise.

6. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -32,13 +32,16 @@ function testCommentTokenReplacement() {
   +    $node = $this->drupalCreateNode(array('type' => 'article', 'title' => '<script>alert("123")</script>'));
   
   +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
   @@ -188,9 +188,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +        $link_title = \Drupal::token()->replace($item->title, array($entity->getEntityTypeId() => $entity), array('clear' => TRUE));
   
   +++ b/core/modules/link/src/Plugin/Field/FieldFormatter/LinkSeparateFormatter.php
   @@ -54,9 +54,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +        $link_title = \Drupal::token()->replace($item->title, array($entity->getEntityTypeId() => $entity), array('clear' => TRUE));
   
   +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
   @@ -55,11 +55,11 @@ function testNodeTokenReplacement() {
   +      'body' => array(array('value' => 'Regular NODE body for the test.', 'summary' => 'Fancy NODE summary.', 'format' => 'plain_text')),
   
   @@ -104,32 +104,20 @@ function testNodeTokenReplacement() {
   +      $this->assertEqual($output, $expected, format_string('Node token %token replaced.', array('%token' => $input)));
   ...
   +      'body' => array(array('value' => 'A string that looks random like TR5c2I', 'format' => 'plain_text')),
   
   @@ -138,15 +126,7 @@ function testNodeTokenReplacement() {
   +      $this->assertEqual($output, $expected, format_string('Node token %token replaced for node without a summary.', array('%token' => $input)));
   
   +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
   @@ -153,7 +139,7 @@ function testUserTokenReplacement() {
   +      $output = $token_service->replace($input, array('user' => $account), array('langcode' => $language_interface->getId(), 'callback' => 'user_mail_tokens', 'clear' => TRUE));
   
   @@ -162,7 +148,7 @@ function testUserTokenReplacement() {
   +      $output = $token_service->replace($input, array('user' => $account), array('callback' => 'user_mail_tokens', 'clear' => TRUE));
   
   @@ -170,9 +156,17 @@ function testUserTokenReplacement() {
   +        $output = $token_service->replace($input, array('user' => $account), array('langcode' => 'de', 'callback' => 'user_mail_tokens', 'clear' => TRUE));
   
   +++ b/core/modules/user/user.module
   @@ -916,10 +917,8 @@ function user_mail($key, &$message, $params) {
   +  $token_options = array('langcode' => $langcode, 'callback' => 'user_mail_tokens', 'clear' => TRUE);
   

   Can we use [] instead of array() since we are changing these lines?

7. +++ b/core/modules/comment/src/Tests/CommentTokenReplaceTest.php
   @@ -114,35 +117,16 @@ function testCommentTokenReplacement() {
   +      $this->assertEqual($output, $expected, format_string('Comment token %token replaced.', array('%token' => $input)));
   ...
   +    $this->assertEqual($output, Html::escape($author_name), format_string('Comment author token %token replaced.', array('%token' => $input)));
   
   +++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
   @@ -138,15 +126,7 @@ function testNodeTokenReplacement() {
   -      $this->assertEqual($output, $expected, format_string('Unsanitized node token %token replaced for node without a summary.', array('%token' => $input)));
   +      $this->assertEqual($output, $expected, format_string('Node token %token replaced for node without a summary.', array('%token' => $input)));
   
   +++ b/core/modules/system/src/Tests/System/TokenReplaceUnitTest.php
   @@ -129,29 +129,9 @@ public function testSystemSiteTokenReplacement() {
   +      $this->assertEqual($output, $expected, format_string('System site information token %token replaced.', array('%token' => $input)));
   
   +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
   @@ -106,7 +107,7 @@ function testUserTokenReplacement() {
   +      $this->assertEqual($output, $expected, format_string('User token %token replaced.', array('%token' => $input)));
   
   +++ b/core/modules/views/src/Tests/TokenReplaceTest.php
   @@ -72,7 +72,7 @@ function testTokenReplacement() {
   +      $this->assertIdentical((string) $output, $expected_output, format_string('Token %token replaced correctly.', array('%token' => $token)));
   

   Can we use FormattableString and [] instead of array() since we are changing these lines?

8. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -268,9 +269,12 @@ public function getUploadLocation($data = array()) {
   +    // @todo Is any valid URI always safe output? If so, handle invalid URIs
   +    //   here, and certainly do not return them.
        return $settings['uri_scheme'] . '://' . $destination;
   

   "If not", I'd say :)

   Also, are we planning a follow-up or will this be addressed in this issue?

9. +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
   @@ -170,9 +156,17 @@ function testUserTokenReplacement() {
   +    $this->assertEqual($output, $expected, SafeMarkup::format('User token %token does not escape safe markup.', ['%token' => 'display-name']));
   

   FormattableString?

10. +++ b/core/modules/views/src/Plugin/views/area/Entity.php
    @@ -162,7 +162,9 @@ public function render($empty = FALSE) {
    +        // We cast as we need the integer/string value provided by the
    +        // tokenizeValue() call.
    

    [übernitpick] Can we use ::tokenizeValue() to make the comment more readable? :)

I think the comments in 8 can just be removed

Should we mention SafeStringInterface here?

Good question, not sure actually. I mean we better refer to some potential summary documentation for our sanitization system.

"If not", I'd say :)

Also, are we planning a follow-up or will this be addressed in this issue?

It is certainly out of scope of this issue. Well, you know, its one of those todos, we could just remove and nobody will notice, in other words, someone put some thoughts in adding it.

[übernitpick] Can we use ::tokenizeValue() to make the comment more readable? :)

You try hard :) Not sure whether you can trump the empty line at some point.

If we happen to reroll this:

+++ b/core/modules/node/src/Tests/NodeTokenReplaceTest.php
@@ -104,7 +105,7 @@ function testNodeTokenReplacement() {
+      $this->assertEqual($output, $expected, format_string('Node token %token replaced.', ['%token' => $input]));

+++ b/core/modules/views/src/Tests/TokenReplaceTest.php
@@ -72,7 +72,7 @@ function testTokenReplacement() {
+      $this->assertIdentical($output, $expected_output, format_string('Token %token replaced correctly.', array('%token' => $token)));

A couple of lingering format_string :)

And I meant this

My main concern is the use of the Renderer for XSS filtering. I raised this in #148, @stefan.r raised it also in #163, @plach raised it in #172.4. I think those concerns will be addressed in #2577827: Add a XssFilteredMarkup, but if that's the case, I think it's important to add @todos to the relevant places.

1. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -185,7 +180,13 @@ public function __construct(ModuleHandlerInterface $module_handler, CacheBackend
   +   *   the result is intended to be used as plain text, the usage of
   

   s/the usage of/using/

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -204,6 +205,11 @@ public function replace($text, array $data = array(), array $options = array(),
   +    // Escape the tokens, unless they are explicitly markup.
   +    foreach ($replacements as $token => $value) {
   +      $replacements[$token] = $value instanceof SafeStringInterface ? $value : Html::escape($value);
   

   Observation: #2576533: Rename SafeStringInterface to MarkupInterface and move related classes will make this a lot clearer.

3. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -33,19 +33,32 @@ class MessageAction extends ConfigurableActionBase implements ContainerFactoryPl
       * Constructs a MessageAction object.
   +   *
   +   * @param \Drupal\Core\Utility\Token $token
   +   *   The token replacement service.
   +   * @param \Drupal\Core\Render\RendererInterface $renderer
   +   *   The renderer.
       */
   

   This is missing the docs for the first 3 params, which I added in #148.

4. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -55,8 +68,12 @@ public function execute($entity = NULL) {
   +    $message = $this->token->replace($this->configuration['message'], $this->configuration);
   +    $build = [
   +      '#markup' => $message,
   +    ];
   +
   +    drupal_set_message($this->renderer->renderPlain($build));
   

   Why does this use the renderer? I think a @todo Fix in https://www.drupal.org/node/2577827 may be necessary.

5. +++ b/core/modules/file/src/Plugin/Field/FieldType/FileItem.php
   @@ -268,9 +269,13 @@ public function getUploadLocation($data = array()) {
   +    // @todo Is any valid URI always safe output? If not, handle invalid URIs
   +    //   here, and certainly do not return them, see
   +    //   https://www.drupal.org/node/2578193.
   

   I think we should fix this @todo here?

6. +++ b/core/modules/system/system.tokens.inc
   @@ -111,14 +107,17 @@ function system_tokens($type, $tokens, array $data, array $options, BubbleableMe
   -          $replacements[$original] = $sanitize ? Xss::filterAdmin($slogan) : $slogan;
   +          $build = [
   +            '#markup' => $slogan,
   +          ];
   +          $replacements[$original] = \Drupal::service('renderer')->renderPlain($build);
   

   Why does this use the renderer? I think a @todo Fix in https://www.drupal.org/node/2577827 may be necessary.

7. +++ b/core/modules/taxonomy/taxonomy.tokens.inc
   @@ -165,11 +164,12 @@ function taxonomy_tokens($type, $tokens, array $data, array $options, Bubbleable
   +          $build = ['#markup' => $vocabulary->getDescription()];
   +          $replacements[$original] = \Drupal::service('renderer')->renderPlain($build);
   

   Why does this use the renderer? I think a @todo Fix in https://www.drupal.org/node/2577827 may be necessary.

Why does this use the renderer? I think a @todo Fix in https://www.drupal.org/node/2577827 may be necessary.

Good point, we could also avoid to inject the renderer, since that's temporary.

#178 My thoughts exactly!

Thank you for your review wim!

This is missing the docs for the first 3 params, which I added in #148.

Sure, let's copy the things from the parent class.

I think we should fix this @todo here?

I added a todo for that. Well, the situation did not got worse by this patch IMHO. Its the intended usecase of producing HTML, which means that things gets escaped, I don't see why anything needs special treatment for now.

Good point, we could also avoid to inject the renderer, since that's temporary.

Let's just go with the proper way for now, given that the other issue hasn't been settled yet, like whether we want to deal with that or not.

This time with the interdiff of my filesystem

Thanks!

+++ b/core/modules/action/src/Plugin/Action/MessageAction.php
@@ -42,6 +42,14 @@ class MessageAction extends ConfigurableActionBase implements ContainerFactoryPl
+   * @param \Drupal\Core\Utility\Token
+   *   The token service.
    * @param \Drupal\Core\Utility\Token $token
    *   The token replacement service.

Now we have this one twice. Can be fixed on commit.

webchick, stefan.r and myself should also be credited since we all were present on a recent call that decided the approach to take.

1. diff --git a/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
   index f525d83..ac73cee 100644
   --- a/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
   +++ b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
   @@ -8,7 +8,6 @@
    namespace Drupal\tour\Plugin\tour\tip;
    
    use Drupal\Component\Utility\Html;
   -use Drupal\Component\Utility\Xss;
    use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
    use Drupal\Core\Utility\Token;
    use Drupal\tour\TipPluginBase;
   

   Unused use

2. +++ b/core/lib/Drupal/Core/Utility/Token.php
   @@ -204,6 +205,11 @@ public function replace($text, array $data = array(), array $options = array(),
   +      $replacements[$token] = $value instanceof SafeStringInterface ? $value : Html::escape($value);
   

   This needs to be SafeMarkup::isSafe() since the static safe list still exists (although is not used by core)

3. +++ b/core/modules/user/src/Tests/UserTokenReplaceTest.php
   @@ -48,6 +48,7 @@ function testUserTokenReplacement() {
   +    \Drupal::state()->set('user_hooks_test_user_format_name_alter_safe', TRUE);
   

   Nice, I hoped to see this!

Re-testing #180, it failed because the #174 patch actually got committed in the mean time, which meant the patch could not possibly apply. Since it was then reverted, a re-test now should work.

Okay I will do the fixes in #187 on commit

Here is a new patch.

Committed 6b4e81a and pushed to 8.0.x. Thanks!

I think we need a change record since this modifies the expectation of hook_tokens() implementors. Also need to review tokens.api.php for changes as well.

+++ b/core/lib/Drupal/Core/Utility/token.api.php
@@ -65,7 +64,9 @@
  *   An associative array of replacement values, keyed by the raw [type:token]
- *   strings from the original text.
+ *   strings from the original text. The returned values must be either plain
+ *   text strings, or an object implementing SafeStringInterface if they are
+ *   HTML-formatted.

It's OK for the returned values to include HTML (not plain text strings), since they will be auto-escaped? Maybe it's my definition of 'plain text strings' doesn't seem to match everyone else's that works on Drupal 8? If its the latter, I find that very confusing.

Working on a CR. I'm really confused how this could have happened :)

I guess my definition is based on what is *inside* the string, not on how it will be output. For example, I considered results of check_plain() a plain string (based on the pre-D8 function name, and that it no longer contains any HTML tags), and results of XSS filter function containing HTML. You're describing the opposite. Maybe I'm the only one confused by that.

@Dave Reid yes, others (including Dave Rothstein himself) had the same confusion about escaped HTML vs plain text.

Currently all of Token::replace(), t() and format_string() have HTML markup as both input and output.

Maybe the planned rename from SafeString to Markup will make this all clearer... Markup objects can be converted to plain text contexts using PlainTextOutput if needed.

I'm just confused about the terminology semantics, not needing to be lectured about the change.

The patch in #190 didn't actaully contain the interdiff. Committing it. Committed 10626d8 and pushed to 8.0.x. Thanks!

Leaving at "needs work" for the CR.

The CR is available at https://www.drupal.org/node/2578365. I just performed a couple of adjustments, mainly I removed the renderer example since we are planning to introduce XssFilteredMarkup and replaced it with FormattableString.

Edit: that was even more confusing, I mentioned the XssFilteredMarkup issue instead.

Change record exists, any improvements can be made there directly.

Back to 1. :)

I'm very happy to see this committed. Here's a small follow-up: #2578569: Move token sanitization from Token::replace() to Token::generate()

Bot, stop!

Posting this here for now, will probably open a follow-up issue.

I've been working on updating token.module and adjusting functionality/tests for this. Which is probably something we should have done before committing this, to make sure that this works for more than the few use cases that core has.

I think there is at least one example there why supporting some sort of sanitize => FALSE is useful and important.

The basic use case is when you have user-provided, unsafe input and want it to be continue unsafe and un-escaped, because you then rely on autoescape.

One example in token.module is the block label, it has this code:

function token_block_view_alter(&$build, BlockPluginInterface $block) {
  $config = $block->getConfiguration();
  $label = $config['label'];
  if ($label != '<none>') {
    // The label is automatically escaped, avoid escaping it twice.
    $build['#configuration']['label'] = \Drupal::token()->replace($label, array(), array('sanitize' => FALSE));
  }
}

The problem is that now the block label tokens are escaped twice. There's a test that is creating a node with a ' in it, and right now, that is getting escaped twice (which is exactly what this code is testing), since we force-escape all token return values and then escape the whole string again.

I don't see a proper way to fix this right now. What technically works is using PlainTextOutput::renderFromHtml() but clearly it is not correct to use that in non-plaintext output.

We don't have to pass it to hook implementations, but I really think we need a flag to prevent auto-escaping. We even document:
The caller is responsible for choosing the right escaping / sanitization but don't actually allow to caller to do that, at least not for token values. But if the token input is untrusted and will be escaped later, we must treat token replacements as untrusted too or we are guaranteed to have double-escaping problems?

What technically works is using PlainTextOutput::renderFromHtml() but clearly it is not correct to use that in non-plaintext output.

I don't see why it's clearly not correct. "The label is automatically escaped" means that your intended semantics for $build['#configuration']['label'] is that you want that value to be treated as plain-text. So I think PlainTextOutput::renderFromHtml() would be the correct thing here. Unless you want to support markup in the block's label (and have it interpreted by the browser as markup), which would then require you to figure out how to properly sanitize it, but that doesn't appear to be your use case, so I think PlainTextOutput::renderFromHtml() is still correct here until you decide you want to change your use case.

@catch I think we'd have to escape the input too

I'm not sure, but I see a difference between plain text and user input. The first is something that is used in a plain text context, like drush or a mail subject. The second is not-yet-escaped text that *will* be used in an HTML context.

I either need to run that on the complete return value, which might have an unexpected effect (not 100% sure) on the provided block label. What if the block label is, to use an existing example mentioned above: I like the <strong>&lt;blink&gt;</strong> tag [some-token]. Is first calling PlainTextOutput::renderFromHtml() and then auto-escaping it again not going to change the output? (It's not going to work as expected anyway, but still). Alternatively, I could set that as the callback for each replacement, but that seems like a complicated API for an IMHO not so uncommon use case.

For those cases wouldn't taking the return value and marking it as Safestring work?

No, I can definitely not do that, exactly because the original string is user-provided and *not* safe. Token::replace() does not escape that, just the token replacements. And that's exactly the problem, that I get a string back that was *partially* escaped. That's completely at odds with the whole SafeString concept. We either need fully escaped or safe strings that we can mark as such or unescaped user-input that will be auto-escaped, not a mix.

Re #214, that gets back to the last part of #211. If you want block labels to support HTML, then maybe what you want is:

$build['#configuration']['label'] = ['#markup' => \Drupal::token()->replace($label)];

?

Or if we don't want to use a '#markup' array for that kind of use case, there's #2577827: Add a XssFilteredMarkup.