summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2016-10-03 21:11:06 +0100
committerNathaniel Catchpole2016-10-03 21:11:06 +0100
commit652ab4a02e1a1aab9dbe2de213fd9fd29e6356eb (patch)
tree22b2f357aec13c8562dea6215073e8fc898c705d
parent29a02cbe2306035ad16c16804ba0f0272630f511 (diff)
Issue #2807705 by alexpott, dawehner, aburke626: FormattableMarkup::placeholderFormat() can result in unsafe replacements
-rw-r--r--core/lib/Drupal/Component/Render/FormattableMarkup.php7
-rw-r--r--core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php64
-rw-r--r--core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php52
3 files changed, 74 insertions, 49 deletions
diff --git a/core/lib/Drupal/Component/Render/FormattableMarkup.php b/core/lib/Drupal/Component/Render/FormattableMarkup.php
index d9fbf2f..6797b97 100644
--- a/core/lib/Drupal/Component/Render/FormattableMarkup.php
+++ b/core/lib/Drupal/Component/Render/FormattableMarkup.php
@@ -227,11 +227,18 @@ class FormattableMarkup implements MarkupInterface, \Countable {
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;
}
}
diff --git a/core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php b/core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php
index e39617d..5d4cad9 100644
--- a/core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php
+++ b/core/tests/Drupal/Tests/Component/Render/FormattableMarkupTest.php
@@ -14,6 +14,20 @@ use Drupal\Tests\UnitTestCase;
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 @@ class FormattableMarkupTest extends UnitTestCase {
$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'],
+ ];
+ }
+
}
diff --git a/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
index cbf86d2..8c1fdce 100644
--- a/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
+++ b/core/tests/Drupal/Tests/Component/Utility/SafeMarkupTest.php
@@ -23,20 +23,6 @@ use Drupal\Tests\UnitTestCase;
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}
*/
protected function tearDown() {
@@ -137,7 +123,7 @@ class SafeMarkupTest extends UnitTestCase {
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 @@ class SafeMarkupTest extends UnitTestCase {
$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);
- }
}