String::format() marks a resulting string as safe even when passed an unsafe passthrough argument

Thank you for pointing out towards this issue in the other issue.

So code, which calls String::format() with arbitrary HTML, has to take care that the raw HTML string is already safe, makes sense.

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -136,12 +136,12 @@ public function form(array $form, FormStateInterface $form_state) {
         '!age' => $this->dateFormatter->formatInterval(REQUEST_TIME - $view->lock->updated),
...
-        '#children' => $this->t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="!break">break this lock</a>.', $lock_message_substitutions),
+        '#children' => $this->t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="@url">break this lock</a>.', $lock_message_substitutions),

Do we need age to be HTML? Just being curious here.

Great to see the test coverage being expanded.

Committed 44313c6 and pushed to 8.0.x. Thanks!

Thanks for adding the eta evaluation to the issue summary.

This is a good security improvement but breaks a number of things (see #2409477: Various ! placeholders throughout core are now showing escaped HTML (especially <a href="!..."> examples)) so I think it really needs a change record, or maybe an update to an existing change record.

Also, the current function documentation really only makes clear that you are responsible for sanitizing user-supplied text before using it with a !-placeholder, not that you're responsible for "sanitizing" (or at least telling Drupal about) something that is already 100% guaranteed to be safe - the latter is a new requirement introduced by this issue. See the issue I linked to above for more details.

Passthrough should be passthrough and assumed safe as it's explicitly set. I think that change needs to be reverted. The other ! to @ changes should be fine.

Or do we have to loop through all incoming variables and mark them safe or escape them? Sounds expensive

I actually wonder if we should roll this back until it's a bit more baked, and at least covers 80%+ of the brokenness introduced. We've been fighting with #2297711: Fix HTML escaping due to Twig autoescape for over 6 months; we don't really need to open another front on that war while we're in beta.

One of the fixes that is super overkill and still doesn't totally fix #2409881: Exception log messages are double-escaped when viewing a single entry:

diff --git a/core/modules/dblog/src/Controller/DbLogController.php b/core/modules/dblog/src/Controller/DbLogController.php
index e8b1ba5..175bc31 100644
--- a/core/modules/dblog/src/Controller/DbLogController.php
+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -9,6 +9,7 @@

 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Unicode;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\Xss;
 use Drupal\Core\Controller\ControllerBase;
@@ -342,11 +343,12 @@ public function formatMessage($row) {
     if (isset($row->message) && isset($row->variables)) {
       // Messages without variables or user specified text.
       if ($row->variables === 'N;') {
-        $message = $row->message;
+        $message = SafeMarkup::checkAdminXss($row->message);
       }
       // Message to translate with injected variables.
       else {
-        $message = $this->t($row->message, unserialize($row->variables));
+        $variables = array_map('Drupal\Component\Utility\SafeMarkup::checkAdminXss', unserialize($row->variables));
+        $message = $this->t($row->message, $variables);
       }
     }
     else {

There is a bit of an issue with explicit intent on this one too. There is an intent to show RAW values, regardless of their safety. And when the internal logic subverts this request it undermines the intent written with the !.

We are babysitting quite a bit, but there has got to be some limit where we need to give up a bit of control/security for DX. I personally think this cross that line. It will create more DX WTFs than it will help with security, IMO.

I'm sure I could be convinced otherwise and this borders that line pretty tight, but that's just how I feel.

A bit more DX confusion happening here #2309215: HTML double-escaping in revision messages because of this.

In general I am +1 to making the APIs not easy to fail.

I however would have implemented this differently, by just running ! values through SafeMarkup::check() in the net effect this is the same, because:

- the offending string is check-plained

However it has the advantage / disadvantage that only the offending part is check plained, while with the current patch the whole string is check plained.

So if the string is gonna be check plained anyway in most cases, we can just use the check function in the first place would be my argument.

The disadvantage is that !url will probably work in 99% of the cases, but still internally check plained in most cases.

Though given this is the most breaking thing, I cannot see why we don't just can make the Link Generator or whatever is used for !url return SafeMarkup if that is so common.

So this patch very easily highlights insecure strings.

On the other hand, people start putting the output of t() to #markup and hence trying to circumvent the system, which is not a good sign, because this means contrib authors will try the same.

On the other hand of the other hand, Using String::format('!foo') is something that we also do not want to encourage obviously.

Another possibility is to just run the SafeMarkup::checkAdminXSS on the non-safe ! values like we do for #prefix and #suffix.

@effulgentsia: It would be great if you could chime in on this again.

To summarize again there are three ways forward that I can currently see:

a) Leave things as is, possibly return SafeMarkup for the most used cases for !url and !link and fix the remainder
b) Use SafeMarkup::check() and just check_plain unescaped ! strings
c) Use SafeMarkup::checkAdminXss() and just check unescaped ! strings

Even in b), c) link functions should probably return SafeMarkup if those need to be safe in many cases in core.

Thanks @all for making Drupal more secure.

#29: That makes a _lot_ of sense!

Thank you so much for the insightful approach!

RTBC for the CR, looks great!

The RTBC is for the CR.

Published the change notice.

We revisited this today in a call with @joelpittet, @effulgentsia, and @Cottser.

In general, I'm in agreement with #24: I think we're breaking the user intent of the ! token here. I'd prefer to allow ! to be truly raw over requiring developers to add SafeMarkup::set() calls, which should be for internal use.

We discussed the following possible approach to ease up this change a bit:

1. Review all uses of ! in core, and convert them to @ instead where appropriate.
2. Evaluate the remaining uses; check if they're being double-escaped.
3. Possibly add another placeholder type to SafeMarkup::format() to distinguish between "don't escape this now, but let Twig do it" and "No really, I mean it, I want this variable to be printed unsanitized exactly as-is in the resultant string.
4. Do not ever add the variable itself to the safe list -- only the resultant string.

I realize I'm posting on a closed issue. :) I'm not quite to the point of asking for a revert here without being more familiar with specifics, but tagging to make sure at least we come back to this.

Thanks for sticking with it #39.

I really like the new ! approach TBH.

@effulgentsia Sorry for being such a hard sell but I'm slowly getting used to the idea.

Right now the @ and ! are so similar now it's hard to distinguish when I'd ever need to use !.

Can you think of a use-case to even have ! now that it's powers are neutered?

Here's a little example script:

use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Render\Renderer;

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">@var', ['@var' => '<a>icon</a>'])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">!var', ['!var' => '<b>icon</b>'])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">@var', ['@var' => SafeMarkup::set('<c>icon</c>')])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">!var', ['!var' => SafeMarkup::set('<d>icon</d>')])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

Output:

<a href="#">&lt;a&gt;icon&lt;/a&gt;
&lt;a href=&quot;#&quot;&gt;&lt;b&gt;icon&lt;/b&gt;
<a href="#"><c>icon</c>
<a href="#"><d>icon</d>

I don't see where B would have any use-case any further. And C and D produce the same results.

@effulgentsia Deprecating ! may be a good call because right now it's kind of unintuitive how variables can affect the surrounding string they are contained within. Or maybe at the very least for DX this is a job for that fancy Assert thing #2444003: Optimize Drupal\Core\Template\Attribute?

A render strategy for format would be awesome for placeholder because CLI could send context (in my dreams) that would prevent placeholder<em> from mucking up the terminal output for watchdog messages and other messages!

This came up again in #2506035: Do not add placeholders from SafeMarkup::format() to the safe list.

The behavior of ! placeholders is completely counterintuitive now. Instead of printing a string with exactly what you passed, it ultimately escapes every other piece of the markup in the string as well. That is a bug. We should either revert this or remove ! entirely.

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

#2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand is now critical, so this will be revisited before RC via that.