Remove !placeholder and unsafe string return from SafeMarkup::format()

Let's see how this fails

This is the final step of #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand and thus is critical.

#2571673: Convert Views t() usage where it is used as an attribute value will actually make this possible

Would be great to use assert() here

Removed a couple of test failures.

On this

Rerolled this on top of #2571673: Convert Views t() usage where it is used as an attribute value and fixed more failures and stuff. Let's see how it works.

1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -28,24 +26,18 @@
   -
            case '%':
   -        default:
              // Escaped and placeholder.
   -          if (!SafeMarkup::isSafe($value)) {
   
   @@ -56,14 +48,27 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE)
   -            $safe = FALSE;
   -          }
   +        default:
   +          // By default we escape any unknown placeholder.
   +          $args[$key] = static::placeholderEscape($value);
   +          break;
          }
   

   We should not change the behaviour of the default in this issue.

2. +++ b/core/tests/Drupal/Tests/Core/StringTranslation/TranslationManagerTest.php
   @@ -25,6 +25,9 @@ class TranslationManagerTest extends UnitTestCase {
   +  /**
   +   * {@inheritdoc}
   +   */
   
   @@ -35,19 +38,18 @@ protected function setUp() {
   -      array(1, 'Singular', '@count plural', array(), array(), 'Singular', TRUE),
   -      array(2, 'Singular', '@count plural', array(), array(), '2 plural', TRUE),
   +      [1, 'Singular', '@count plural', array(), array(), 'Singular'],
   +      [2, 'Singular', '@count plural', array(), array(), '2 plural'],
   ...
   +      [2, 'Singular', '@count @arg', array('@arg' => '<script>'), array(), '2 &lt;script&gt;'],
   +      [2, 'Singular', '@count %arg', array('%arg' => '<script>'), array(), '2 <em class="placeholder">&lt;script&gt;</em>'],
   

   out of scope changes

1: I think we should, the docs say that's the recommended default behavior.
2: Those lines are removing the last parameter.

1: I think we should, the docs say that's the recommended default behavior.

Well, this is the thing. This will lead to another level of discussion, too bad. I'm fine with changing the behaviour now.

2: Those lines are removing the last parameter.

A good point.

Fair enough

Rerolled

1. +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
   @@ -100,14 +87,28 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE)
   +        case '%':
   +        default:
   +          // Escaped and placeholder.
   +          $args[$key] = '<em class="placeholder">' . static::placeholderEscape($value) . '</em>';
   +          break;
          }
        }
   

   Opened #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat() for removing the default.

2. +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
   @@ -183,20 +183,7 @@ public static function checkPlain($text) {
   +    return new FormattableString($string, $args);
   

   :)

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -1307,7 +1307,10 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
          return TRUE;
   

   Much better docs than last time I reviewed, and the comparison makes sense as well now.

4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
   @@ -110,20 +110,7 @@ public function getStringTranslation($langcode, $string, $context) {
   +    return new TranslatableString($string, $args, $options, $this);
   

   :)

5. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -307,6 +307,52 @@ public function testBuildFormWithObject() {
   +  public function providerTestBuildFormWithTriggeringElement() {
   +    $plain_text = 'Other submit value';
   +    $markup = 'Other submit <input> value';
   +    return [
   +      'plain-text' => [$plain_text, $plain_text],
   +      'markup' => [$markup, $markup],
   +      'escaped-markup' => [Html::escape($markup), $markup],
   +      'markup-escaped' => [$markup, $markup],
   +      'escaped-escaped' => [Html::escape($markup), $markup],
   +    ];
   +  }
   

   markup-escaped is the same as markup-markup

   And escaped-escaped is the same as escaped-markup.

RTBC for me except for that last point.

Working on it

This depends on #2571673: Convert Views t() usage where it is used as an attribute value. I will reroll it as soon as that goes in.

Found a leftover in the documentation.

Here's a reroll on top of the latest version of #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format(). The review file includes only the changes performed here.

Setting to needs review for the bot, still postponed on #2571695: Remove !placeholder and unsafe string return from SafeMarkup::format().

Not sure what happened but only the last two patches should be taken into account.

And here it is (ignore the interdiff :)

Good bye and thanks for all the fish!

Dear testbot, we don't have to like you!

Nearly there!

1. TranslationManager::formatPlural() is still doing isSafe logic - also can remove SafeMarkup use too

2. +++ b/core/lib/Drupal/Core/StringTranslation/PluralTranslatableString.php
   @@ -107,21 +107,9 @@ public function __construct($count, $singular, $plural, array $args = [], array
   -      if (0 === strpos($arg_key, '!') && !SafeMarkup::isSafe($args[$arg_key])) {
   

   Can remove the use

3. UnitTestCase::getStringTranslationStub() always needs to return a TranslatableString for the translate mock

I don't think we have a CR specifically for removing !placeholder yet, so we'll need that too.

It turned out to be harder than it was.

while working on #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure I noticed that the third argument in protected static function placeholderFormat($string, array $args, &$safe = TRUE) is not used anywhere in core (and not tested directly). glad to see this issue will be removing the arg. I will keep working on 2570431, and assuming that arg will not be there makes writing the docs for the @param easier *grin*

Thank you @plach and @borisson_ for the quick help to fix it

Someone in a future will review something.

cross-post since sitting next to daniel and he fixed it fast.

+++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
@@ -100,14 +80,28 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE)
+   * @return string
+   *   The properly escaped replacement value.
+   */
+  protected static function placeholderEscape($value) {
+    return SafeMarkup::isSafe($value) ? $value : Html::escape($value);
+  }

this says it returns a string, but it sometimes returns value which could have been a SafeStringInterface object.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -183,20 +183,56 @@ public static function checkPlain($text) {
+    $sort_array = static::castSafeStrings($array);
+    $result = uasort($sort_array, $compare_function);
+
+    $sorted_array = [];
+    foreach (array_keys($sort_array) as $key) {
+      $sorted_array[$key] = $array[$key];
     }
-    $safe_string = new FormattableString($string, $args);
+    $array = $sorted_array;
+    return $result;

this could use some inline comments.

Some additional docs.

Let's not make things harder.

1. +++ b/core/modules/user/tests/src/Unit/PermissionHandlerTest.php
   @@ -407,3 +410,28 @@ public function titleDescriptionRestrictAccess() {
   +
   +class TestTranslationManager implements TranslationInterface {
   

   missing docs for this class.

2. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -42,7 +42,8 @@ protected function tearDown() {
   -   *   @link http://twig.sensiolabs.org/doc/filters/escape.html Twig escape documentation @endlink
   +   *
   +   * @link http://twig.sensiolabs.org/doc/filters/escape.html Twig escape documentation @endlink
   

   unrelated change. (maybe an accident, change in indent looks wrong.)

3. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -95,6 +96,7 @@ public function providerSet() {
       * Tests SafeMarkup::setMultiple().
   +   *
       * @dataProvider providerSet
   

   unrelated change.

4. +++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
   @@ -180,13 +182,35 @@ function testCheckPlain($text, $expected, $message, $ignorewarnings = FALSE) {
   -    $tests[] = array("Foo\xC0barbaz", 'Foo�barbaz', 'SafeMarkup::checkPlain() escapes invalid sequence "Foo\xC0barbaz"', TRUE);
   -    $tests[] = array("\xc2\"", '�"', 'SafeMarkup::checkPlain() escapes invalid sequence "\xc2\""', TRUE);
   -    $tests[] = array("Fooÿñ", "Fooÿñ", 'SafeMarkup::checkPlain() does not escape valid sequence "Fooÿñ"');
   +    $tests[] = array(
   

   some more. might be worth looking for more in the file.

(I did not read every line very carefully.)

Removed a couple of unrelated changes.

thanks. those changes look good, adds the docs too.

there is a needs change record tag on the issue, so we either need a new change record, or add this issue to one to reuse (or both), before we can rtbc it.

We have a change record now.

Last changes look good!

Thanks for adding the change record, adding words we talked about and issues. That along with the other recent improvments look good. rtbc.

Re-rolled the patch from comment 61, removing the now obsoleted patches from ContactPersonalTest and core/modules/migrate/src/Plugin/migrate/id_map/Sql.php, as those were handled separately in #2575599: Remove !placeholder in ContactPersonalTest and Drupal\migrate\Plugin\migrate\id_map\Sql.

Thanks! This will come back green

Committed 3904439 and pushed to 8.0.x. Thanks!

I also credited everyone who worked on the original #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests as that was a 500k patch that tried to do everything.

Not sure where to file the follow up, seems this caused many messages in drush to show up like this:

$ drush en comment
<em class="placeholder">comment</em> is already enabled. 

That's because drush is still using !placeholders and because unknown placeholders fall through to %. #2575703: Remove default fall-through from PlaceholderTrait::placeholderFormat() would remove that and you'd get an error about an invalid placeholder instead, somehow.

Filed https://github.com/drush-ops/drush/issues/1637

This is a pretty big API break then deep in beta and I don't see signoff in the IS. Contrib will be affected by this as well (which explains why my migrate CI is unreadable this morning).

@neclimdul this was discussed in great depth both in the issue queue and on multiple hangouts, official sign-off/decision is mostly documented at #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand.

The tl;dr version is that we realised that leaving !placeholder in was going to result in a combination of double-escaping bugs and XSS via mis-use of SafeString to bypass the escaping, and the only way towards a consistent sanitization API was to do the hard BC break and remove the placeholder altogether.

However it took about 3 weeks to make that change across core, and the actual removal just landed in this issue.

I'm hoping to roll a new beta asap once #2570431: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure lands so that there's a marker for contrib modules to upgrade to prior to the release candidate. This will feature prominently in the release notes.

I'm not question the commit, just this is the issue that broke things. None of those things you mentioned are covered in the issue which is why I called attention to the IS. I should have tagged the summary for update.

Also, it might also be worth publishing the CR to alleviate some confusion from the disruption. I'm not sure "placeholdering" is a word though...

Is there a task issue somewhere I cannot find to update the API documentation for t(). There are many places in the docs where it refers to the return of t() as a string value. In talking with various Drupal people, the knowledge of this change hasn't propagated much and is really going to surprise people. I was arguing with somebody who was pointing to the API and various things until I pulled the actual code to show them.

Even this issue was not very easy to find. Was trying to search for stuff like "drupal 8 t() no longer returns a string".

This removed the Twig filter 'passthrough', but the change record doesn't mention that.

cool, passthrough removed.

How to pass variable with html to trans now?

{% set html_string = "<a href=''>a</a>" %}

{% trans %}
  Some text {{ html_string|raw }}
{% endtrans %}

not works now =)