Issue #2399037 by effulgentsia: String::format() marks a resulting string as...

Issue #2399037 by effulgentsia: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument

core/lib/Drupal/Component/Utility/String.php

@@ -96,6 +96,8 @@ public static function decodeEntities($text) {
    * @see t()
    */
   public static function format($string, array $args = array()) {
+    $safe = TRUE;
+
     // Transform arguments before inserting them.
     foreach ($args as $key => $value) {
       switch ($key[0]) {
@@ -112,9 +114,18 @@ public static function format($string, array $args = array()) {
 
         case '!':
           // Pass-through.
+          if (!SafeMarkup::isSafe($value)) {
+            $safe = FALSE;
+          }
       }
     }
-    return SafeMarkup::set(strtr($string, $args));
+
+    $output = strtr($string, $args);
+    if ($safe) {
+      SafeMarkup::set($output);
+    }
+
+    return $output;
   }
 
   /**

core/modules/file/file.field.inc

@@ -189,16 +189,16 @@ function template_preprocess_file_upload_help(&$variables) {
     $max = $upload_validators['file_validate_image_resolution'][0];
     $min = $upload_validators['file_validate_image_resolution'][1];
     if ($min && $max && $min == $max) {
-      $descriptions[] = t('Images must be exactly !size pixels.', array('!size' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be exactly <strong>@size</strong> pixels.', array('@size' => $max));
     }
     elseif ($min && $max) {
-      $descriptions[] = t('Images must be larger than !min pixels. Images larger than !max pixels will be resized.', array('!min' => '<strong>' . $min . '</strong>', '!max' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images must be larger than <strong>@min</strong> pixels. Images larger than <strong>@max</strong> pixels will be resized.', array('@min' => $min, '@max' => $max));
     }
     elseif ($min) {
-      $descriptions[] = t('Images must be larger than !min pixels.', array('!min' => '<strong>' . $min . '</strong>'));
+      $descriptions[] = t('Images must be larger than <strong>@min</strong> pixels.', array('@min' => $min));
     }
     elseif ($max) {
-      $descriptions[] = t('Images larger than !max pixels will be resized.', array('!max' => '<strong>' . $max . '</strong>'));
+      $descriptions[] = t('Images larger than <strong>@max</strong> pixels will be resized.', array('@max' => $max));
     }
   }
 

core/modules/views/src/Tests/Plugin/DisplayTest.php

@@ -243,7 +243,7 @@ public function testInvalidDisplayPlugins() {
 
     $this->drupalGet('test_display_invalid');
     $this->assertResponse(200);
-    $this->assertText('The "invalid" plugin does not exist.');
+    $this->assertText('The "invalid" plugin does not exist.');
 
     // Rebuild the router, and ensure that the path is not accessible anymore.
     views_invalidate_cache();
@@ -273,7 +273,7 @@ public function testInvalidDisplayPlugins() {
     // plugin warning message.
     $this->drupalGet('<front>');
     $this->assertResponse(200);
-    $this->assertText('The "invalid" plugin does not exist.');
+    $this->assertText('The &quot;invalid&quot; plugin does not exist.');
     $this->assertNoBlockAppears($block);
   }
 

core/modules/views_ui/src/ViewEditForm.php

@@ -137,12 +137,12 @@ public function form(array $form, FormStateInterface $form_state) {
       $lock_message_substitutions = array(
         '!user' => drupal_render($username),
         '!age' => $this->dateFormatter->formatInterval(REQUEST_TIME - $view->lock->updated),
-        '!break' => $view->url('break-lock-form'),
+        '@url' => $view->url('break-lock-form'),
       );
       $form['locked'] = array(
         '#type' => 'container',
         '#attributes' => array('class' => array('view-locked', 'messages', 'messages--warning')),
-        '#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),
         '#weight' => -10,
       );
     }

core/tests/Drupal/Tests/Component/Utility/StringTest.php

@@ -7,6 +7,7 @@
 
 namespace Drupal\Tests\Component\Utility;
 
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Tests\UnitTestCase;
 use Drupal\Component\Utility\String;
 
@@ -71,10 +72,13 @@ function providerCheckPlain() {
    *   The expected result from calling the function.
    * @param string $message
    *   The message to display as output to the test.
+   * @param bool $expected_is_safe
+   *   Whether the result is expected to be safe for HTML display.
    */
-  function testFormat($string, $args, $expected, $message) {
+  function testFormat($string, $args, $expected, $message, $expected_is_safe) {
     $result = String::format($string, $args);
     $this->assertEquals($expected, $result, $message);
+    $this->assertEquals($expected_is_safe, SafeMarkup::isSafe($result), 'String::format correctly sets the result as safe or not safe.');
   }
 
   /**
@@ -83,10 +87,11 @@ function testFormat($string, $args, $expected, $message) {
    * @see testFormat()
    */
   function providerFormat() {
-    $tests[] = array('Simple text', array(), 'Simple text', 'String::format leaves simple text alone.');
-    $tests[] = array('Escaped text: @value', array('@value' => '<script>'), 'Escaped text: &lt;script&gt;', 'String::format replaces and escapes string.');
-    $tests[] = array('Placeholder text: %value', array('%value' => '<script>'), 'Placeholder text: <em class="placeholder">&lt;script&gt;</em>', 'String::format replaces, escapes and themes string.');
-    $tests[] = array('Verbatim text: !value', array('!value' => '<script>'), 'Verbatim text: <script>', 'String::format replaces verbatim string as-is.');
+    $tests[] = array('Simple text', array(), 'Simple text', 'String::format leaves simple text alone.', TRUE);
+    $tests[] = array('Escaped text: @value', array('@value' => '<script>'), 'Escaped text: &lt;script&gt;', 'String::format replaces and escapes string.', TRUE);
+    $tests[] = array('Placeholder text: %value', array('%value' => '<script>'), 'Placeholder text: <em class="placeholder">&lt;script&gt;</em>', 'String::format replaces, escapes and themes string.', TRUE);
+    $tests[] = array('Verbatim text: !value', array('!value' => '<script>'), 'Verbatim text: <script>', 'String::format replaces verbatim string as-is.', FALSE);
+    $tests[] = array('Verbatim text: !value', array('!value' => SafeMarkup::set('<span>Safe HTML</span>')), 'Verbatim text: <span>Safe HTML</span>', 'String::format replaces verbatim string as-is.', TRUE);
 
     return $tests;
   }