summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2016-01-01 01:30:51 (GMT)
committerNathaniel Catchpole2016-01-01 01:30:51 (GMT)
commit4b703191b3e7ec66c3f8a1777b04c63cc3c19b22 (patch)
tree9af9a428916df8ee92b9911e29bd65de7c3f83a3
parent7a2970f7ac07554a137775dea4a9192f716d717f (diff)
Issue #2371861 by DuaelFr, YesCT, Gábor Hojtsy, tucho: Strings including tokens in href or src attributes cannot be translated due to safeness check incompatibilities
-rw-r--r--core/modules/locale/locale.module25
-rw-r--r--core/modules/locale/src/Tests/LocaleStringIsSafeTest.php106
-rw-r--r--core/modules/locale/tests/modules/locale_test/locale_test.module57
-rw-r--r--core/modules/locale/tests/modules/locale_test/templates/locale-test-tokenized.html.twig1
4 files changed, 189 insertions, 0 deletions
diff --git a/core/modules/locale/locale.module b/core/modules/locale/locale.module
index ca8df47..74b76ec 100644
--- a/core/modules/locale/locale.module
+++ b/core/modules/locale/locale.module
@@ -1035,6 +1035,31 @@ function locale_translation_use_remote_source() {
* layout issues (div) or a possible attack vector (img).
*/
function locale_string_is_safe($string) {
+ // Some strings have tokens in them. For tokens in the first part of href or
+ // src HTML attributes, \Drupal\Component\Utility\Xss::filter() removes part
+ // of the token, the part before the first colon.
+ // \Drupal\Component\Utility\Xss::filter() assumes it could be an attempt to
+ // inject javascript. When \Drupal\Component\Utility\Xss::filter() removes
+ // part of tokens, it causes the string to not be translatable when it should
+ // be translatable.
+ // @see \Drupal\locale\Tests\LocaleStringIsSafeTest::testLocaleStringIsSafe()
+ //
+ // We can recognize tokens since they are wrapped with brackets and are only
+ // composed of alphanumeric characters, colon, underscore, and dashes. We can
+ // be sure these strings are safe to strip out before the string is checked in
+ // \Drupal\Component\Utility\Xss::filter() because no dangerous javascript
+ // will match that pattern.
+ //
+ // Strings with tokens should not be assumed to be dangerous because even if
+ // we evaluate them to be safe here, later replacing the token inside the
+ // string will automatically mark it as unsafe as it is not the same string
+ // anymore.
+ //
+ // @todo Do not strip out the token. Fix
+ // \Drupal\Component\Utility\Xss::filter() to not incorrectly alter the
+ // string. https://www.drupal.org/node/2372127
+ $string = preg_replace('/\[[a-z0-9_-]+(:[a-z0-9_-]+)+\]/i', '', $string);
+
return Html::decodeEntities($string) == Html::decodeEntities(Xss::filter($string, array('a', 'abbr', 'acronym', 'address', 'b', 'bdo', 'big', 'blockquote', 'br', 'caption', 'cite', 'code', 'col', 'colgroup', 'dd', 'del', 'dfn', 'dl', 'dt', 'em', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'ins', 'kbd', 'li', 'ol', 'p', 'pre', 'q', 'samp', 'small', 'span', 'strong', 'sub', 'sup', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'tr', 'tt', 'ul', 'var')));
}
diff --git a/core/modules/locale/src/Tests/LocaleStringIsSafeTest.php b/core/modules/locale/src/Tests/LocaleStringIsSafeTest.php
new file mode 100644
index 0000000..603187f
--- /dev/null
+++ b/core/modules/locale/src/Tests/LocaleStringIsSafeTest.php
@@ -0,0 +1,106 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\locale\Tests\LocaleStringIsSafeTest.
+ */
+
+namespace Drupal\locale\Tests;
+
+use Drupal\simpletest\KernelTestBase;
+
+/**
+ * Tests locale translation safe string handling.
+ *
+ * @group locale
+ */
+class LocaleStringIsSafeTest extends KernelTestBase {
+
+ /**
+ * Modules to enable.
+ *
+ * @var array
+ */
+ public static $modules = ['locale', 'locale_test'];
+
+ /**
+ * Tests for locale_string_is_safe().
+ */
+ public function testLocaleStringIsSafe() {
+ // Check a translatable string without HTML.
+ $string = 'Hello world!';
+ $result = locale_string_is_safe($string);
+ $this->assertTrue($result);
+
+ // Check a translatable string which includes trustable HTML.
+ $string = 'Hello <strong>world</strong>!';
+ $result = locale_string_is_safe($string);
+ $this->assertTrue($result);
+
+ // Check an untranslatable string which includes untrustable HTML (according
+ // to the locale_string_is_safe() function definition).
+ $string = 'Hello <img src="world.png" alt="world" />!';
+ $result = locale_string_is_safe($string);
+ $this->assertFalse($result);
+
+ // Check a translatable string which includes a token in an href attribute.
+ $string = 'Hi <a href="[current-user:url]">user</a>';
+ $result = locale_string_is_safe($string);
+ $this->assertTrue($result);
+ }
+
+ /**
+ * Tests if a translated and tokenized string is properly escaped by Twig.
+ *
+ * In each assert* call we add a new line at the expected result to match the
+ * newline at the end of the template file.
+ */
+ public function testLocalizedTokenizedString() {
+ $tests_to_do = [
+ 1 => [
+ 'original' => 'Go to the <a href="[locale_test:security_test1]">frontpage</a>',
+ 'replaced' => 'Go to the &lt;a href=&quot;javascript:alert(&amp;#039;Mooooh!&amp;#039;);&quot;&gt;frontpage&lt;/a&gt;',
+ ],
+ 2 => [
+ 'original' => 'Hello <strong>[locale_test:security_test2]</strong>!',
+ 'replaced' => 'Hello &lt;strong&gt;&amp;lt;script&amp;gt;alert(&amp;#039;Mooooh!&amp;#039;);&amp;lt;/script&amp;gt;&lt;/strong&gt;!',
+ ],
+ ];
+
+ foreach ($tests_to_do as $i => $test) {
+ $original_string = $test['original'];
+ $rendered_original_string = \Drupal::theme()->render('locale_test_tokenized', ['content' => $original_string]);
+ // Twig assumes that strings are unsafe so it escapes them, and so the
+ // original and the rendered version should be different.
+ $this->assertNotEqual(
+ $rendered_original_string,
+ $original_string . "\n",
+ 'Security test ' . $i . ' before translation'
+ );
+
+ // Pass the original string to the t() function to get it marked as safe.
+ $safe_string = t($original_string);
+ $rendered_safe_string = \Drupal::theme()->render('locale_test_tokenized', ['content' => $safe_string]);
+ // t() function always marks the string as safe so it won't be escaped,
+ // and should be the same as the original.
+ $this->assertEqual(
+ $rendered_safe_string,
+ $original_string . "\n",
+ 'Security test ' . $i . ' after translation before token replacement'
+ );
+
+ // Replace tokens in the safe string to inject it with dangerous content.
+ // @see locale_test_tokens().
+ $unsafe_string = \Drupal::token()->replace($safe_string);
+ $rendered_unsafe_string = \Drupal::theme()->render('locale_test_tokenized', ['content' => $unsafe_string]);
+ // Token replacement changes the string so it is not marked as safe
+ // anymore. Check it is escaped the way we expect.
+ $this->assertEqual(
+ $rendered_unsafe_string,
+ $test['replaced'] . "\n",
+ 'Security test ' . $i . ' after translation after token replacement'
+ );
+ }
+ }
+
+}
diff --git a/core/modules/locale/tests/modules/locale_test/locale_test.module b/core/modules/locale/tests/modules/locale_test/locale_test.module
index 83909d3..b59dfee 100644
--- a/core/modules/locale/tests/modules/locale_test/locale_test.module
+++ b/core/modules/locale/tests/modules/locale_test/locale_test.module
@@ -146,3 +146,60 @@ function locale_test_language_fallback_candidates_locale_lookup_alter(array &$ca
\Drupal::state()->set('locale.test_language_fallback_candidates_locale_lookup_alter_candidates', $candidates);
\Drupal::state()->set('locale.test_language_fallback_candidates_locale_lookup_alter_context', $context);
}
+
+/**
+ * Implements hook_theme().
+ */
+function locale_test_theme() {
+ $return = [];
+
+ $return['locale_test_tokenized'] = [
+ 'variable' => ['content' => ''],
+ ];
+
+ return $return;
+}
+
+/**
+ * Implements hook_token_info().
+ */
+function locale_test_token_info() {
+ $info = [];
+
+ $info['types']['locale_test'] = [
+ 'name' => t('Locale test'),
+ 'description' => t('Locale test'),
+ ];
+
+ $info['tokens']['locale_test']['security_test1'] = [
+ 'type' => 'text',
+ 'name' => t('Security test 1'),
+ ];
+ $info['tokens']['locale_test']['security_test2'] = [
+ 'type' => 'text',
+ 'name' => t('Security test 2'),
+ ];
+
+ return $info;
+}
+
+/**
+ * Implements hook_tokens().
+ */
+function locale_test_tokens($type, $tokens, array $data = [], array $options = []) {
+ $return = [];
+ if ($type == 'locale_test') {
+ foreach ($tokens as $name => $original) {
+ switch ($name) {
+ case 'security_test1':
+ $return[$original] = "javascript:alert('Mooooh!');";
+ break;
+ case 'security_test2':
+ $return[$original] = "<script>alert('Mooooh!');</script>";
+ break;
+ }
+ }
+ }
+
+ return $return;
+}
diff --git a/core/modules/locale/tests/modules/locale_test/templates/locale-test-tokenized.html.twig b/core/modules/locale/tests/modules/locale_test/templates/locale-test-tokenized.html.twig
new file mode 100644
index 0000000..cddd070
--- /dev/null
+++ b/core/modules/locale/tests/modules/locale_test/templates/locale-test-tokenized.html.twig
@@ -0,0 +1 @@
+{{ content }}