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

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')));
 }
 

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'
+      );
+    }
+  }
+
+}

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;
+}

core/modules/locale/tests/modules/locale_test/templates/locale-test-tokenized.html.twig

@@ -0,0 +1 @@
+{{ content }}