Issue #2807705 by alexpott, dawehner, aburke626:...

Issue #2807705 by alexpott, dawehner, aburke626: FormattableMarkup::placeholderFormat() can result in unsafe replacements

core/lib/Drupal/Component/Render/FormattableMarkup.php

@@ -227,11 +227,18 @@ protected static function placeholderFormat($string, array $args) {
         default:
           // We do not trigger an error for placeholder that start with an
           // alphabetic character.
+          // @todo https://www.drupal.org/node/2807743 Change to an exception
+          //   and always throw regardless of the first character.
           if (!ctype_alpha($key[0])) {
             // We trigger an error as we may want to introduce new placeholders
             // in the future without breaking backward compatibility.
             trigger_error('Invalid placeholder (' . $key . ') in string: ' . $string, E_USER_ERROR);
           }
+          elseif (strpos($string, $key) !== FALSE) {
+            trigger_error('Invalid placeholder (' . $key . ') in string: ' . $string, E_USER_DEPRECATED);
+          }
+          // No replacement possible therefore we can discard the argument.
+          unset($args[$key]);
           break;
       }
     }

core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php

@@ -13,6 +13,20 @@
  */
 class FormattableMarkupTest extends UnitTestCase {
 
+  /**
+   * The error message of the last error in the error handler.
+   *
+   * @var string
+   */
+  protected $lastErrorMessage;
+
+  /**
+   * The error number of the last error in the error handler.
+   *
+   * @var int
+   */
+  protected $lastErrorNumber;
+
   /**
    * @covers ::__toString
    * @covers ::jsonSerialize
@@ -35,4 +49,54 @@ public function testCount() {
     $this->assertEquals(strlen($string), $formattable_string->count());
   }
 
+  /**
+   * Custom error handler that saves the last error.
+   *
+   * We need this custom error handler because we cannot rely on the error to
+   * exception conversion as __toString is never allowed to leak any kind of
+   * exception.
+   *
+   * @param int $error_number
+   *   The error number.
+   * @param string $error_message
+   *   The error message.
+   */
+  public function errorHandler($error_number, $error_message) {
+    $this->lastErrorNumber = $error_number;
+    $this->lastErrorMessage = $error_message;
+  }
+
+  /**
+   * @covers ::__toString
+   * @dataProvider providerTestUnexpectedPlaceholder
+   */
+  public function testUnexpectedPlaceholder($string, $arguments, $error_number, $error_message) {
+    // We set a custom error handler because of https://github.com/sebastianbergmann/phpunit/issues/487
+    set_error_handler([$this, 'errorHandler']);
+    // We want this to trigger an error.
+    $markup = new FormattableMarkup($string, $arguments);
+    // Cast it to a string which will generate the errors.
+    $output = (string) $markup;
+    restore_error_handler();
+    // The string should not change.
+    $this->assertEquals($string, $output);
+    $this->assertEquals($error_number, $this->lastErrorNumber);
+    $this->assertEquals($error_message, $this->lastErrorMessage);
+  }
+
+  /**
+   * Data provider for FormattableMarkupTest::testUnexpectedPlaceholder().
+   *
+   * @return array
+   */
+  public function providerTestUnexpectedPlaceholder() {
+    return [
+      ['Non alpha starting character: ~placeholder', ['~placeholder' => 'replaced'], E_USER_ERROR, 'Invalid placeholder (~placeholder) in string: Non alpha starting character: ~placeholder'],
+      ['Alpha starting character: placeholder', ['placeholder' => 'replaced'], E_USER_DEPRECATED, 'Invalid placeholder (placeholder) in string: Alpha starting character: placeholder'],
+      // Ensure that where the placeholder is located in the the string is
+      // irrelevant.
+      ['placeholder', ['placeholder' => 'replaced'], E_USER_DEPRECATED, 'Invalid placeholder (placeholder) in string: placeholder'],
+    ];
+  }
+
 }

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

@@ -22,20 +22,6 @@
  */
 class SafeMarkupTest extends UnitTestCase {
 
-  /**
-   * The error message of the last error in the error handler.
-   *
-   * @var string
-   */
-  protected $lastErrorMessage;
-
-  /**
-   * The error number of the last error in the error handler.
-   *
-   * @var int
-   */
-  protected $lastErrorNumber;
-
   /**
    * {@inheritdoc}
    */
@@ -137,7 +123,7 @@ public function testFormat($string, array $args, $expected, $message, $expected_
     UrlHelper::setAllowedProtocols(['http', 'https', 'mailto']);
 
     $result = SafeMarkup::format($string, $args);
-    $this->assertEquals($expected, $result, $message);
+    $this->assertEquals($expected, (string) $result, $message);
     $this->assertEquals($expected_is_safe, $result instanceof MarkupInterface, 'SafeMarkup::format correctly sets the result as safe or not safe.');
 
     foreach ($args as $arg) {
@@ -171,42 +157,10 @@ function providerFormat() {
     $tests['non-url-with-colon'] = ['Hey giraffe <a href=":url">MUUUH</a>', [':url' => "llamas: they are not URLs"], 'Hey giraffe <a href=" they are not URLs">MUUUH</a>', '', TRUE];
     $tests['non-url-with-html'] = ['Hey giraffe <a href=":url">MUUUH</a>', [':url' => "<span>not a url</span>"], 'Hey giraffe <a href="&lt;span&gt;not a url&lt;/span&gt;">MUUUH</a>', '', TRUE];
 
+    // Tests non-standard placeholders that will not replace.
+    $tests['non-standard-placeholder'] = ['Hey hey', ['risky' => "<script>alert('foo');</script>"], 'Hey hey', '', TRUE];
     return $tests;
   }
-  /**
-   * Custom error handler that saves the last error.
-   *
-   * We need this custom error handler because we cannot rely on the error to
-   * exception conversion as __toString is never allowed to leak any kind of
-   * exception.
-   *
-   * @param int $error_number
-   *   The error number.
-   * @param string $error_message
-   *   The error message.
-   */
-  public function errorHandler($error_number, $error_message) {
-    $this->lastErrorNumber = $error_number;
-    $this->lastErrorMessage = $error_message;
-  }
-
-  /**
-   * String formatting with SafeMarkup::format() and an unsupported placeholder.
-   *
-   * When you call SafeMarkup::format() with an unsupported placeholder, an
-   * InvalidArgumentException should be thrown.
-   */
-  public function testUnexpectedFormat() {
-
-    // We set a custom error handler because of https://github.com/sebastianbergmann/phpunit/issues/487
-    set_error_handler([$this, 'errorHandler']);
-    // We want this to trigger an error.
-    $error = SafeMarkup::format('Broken placeholder: ~placeholder', ['~placeholder' => 'broken'])->__toString();
-    restore_error_handler();
-
-    $this->assertEquals(E_USER_ERROR, $this->lastErrorNumber);
-    $this->assertEquals('Invalid placeholder (~placeholder) in string: Broken placeholder: ~placeholder', $this->lastErrorMessage);
-  }
 
 }