[Extensions] Handle RTL and directional characters in extension names

This CL introduces base::i18n::EnsureTerminatedDirectionalFormatting to ensure that the supplied text contains no unterminated directional formatting characters. Additionally, this CL introduces base::i18n::SanitizeUserSuppliedString which adjusts the supplied text according to the locale direction and relies on the previous method to handle unterminated directional formatting characters. The SanitizeUserSuppliedString is applied to sanitize RTL and directional characters in extension names. Bug: 685747 Change-Id: I48db3a37b8a54d5c4ca5556d56cde4c9da31abe3 Reviewed-on: https://chromium-review.googlesource.com/876522 Commit-Queue: catmullings <catmullings@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Jungshik Shin <jshin@chromium.org> Reviewed-by: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#536375}
Denger-Network · Feb 13, 2018 · 867fbaa · 867fbaa
1 parent 057f7f7
commit 867fbaa
Show file tree

Hide file tree

Showing 5 changed files with 156 additions and 3 deletions.
diff --git a/base/i18n/rtl.cc b/base/i18n/rtl.cc
@@ -380,6 +380,25 @@ bool UnadjustStringForLocaleDirection(string16* text) {
 
 #endif  // !OS_WIN
 
+void EnsureTerminatedDirectionalFormatting(string16* text) {
+  int count = 0;
+  for (auto c : *text) {
+    if (c == kLeftToRightEmbeddingMark || c == kRightToLeftEmbeddingMark ||
+        c == kLeftToRightOverride || c == kRightToLeftOverride) {
+      ++count;
+    } else if (c == kPopDirectionalFormatting && count > 0) {
+      --count;
+    }
+  }
+  for (int j = 0; j < count; j++)
+    text->push_back(kPopDirectionalFormatting);
+}
+
+void SanitizeUserSuppliedString(string16* text) {
+  EnsureTerminatedDirectionalFormatting(text);
+  AdjustStringForLocaleDirection(text);
+}
+
 bool StringContainsStrongRTLChars(const string16& text) {
   const UChar* string = text.c_str();
   size_t length = text.length();

diff --git a/base/i18n/rtl.h b/base/i18n/rtl.h
@@ -116,6 +116,15 @@ BASE_I18N_EXPORT bool AdjustStringForLocaleDirection(string16* text);
 // Undoes the actions of the above function (AdjustStringForLocaleDirection).
 BASE_I18N_EXPORT bool UnadjustStringForLocaleDirection(string16* text);
 
+// Ensures |text| contains no unterminated directional formatting characters, by
+// appending the appropriate pop-directional-formatting characters to the end of
+// |text|.
+BASE_I18N_EXPORT void EnsureTerminatedDirectionalFormatting(string16* text);
+
+// Sanitizes the |text| by terminating any directional override/embedding
+// characters and then adjusting the string for locale direction.
+BASE_I18N_EXPORT void SanitizeUserSuppliedString(string16* text);
+
 // Returns true if the string contains at least one character with strong right
 // to left directionality; that is, a character with either R or AL Unicode
 // BiDi character type.

diff --git a/base/i18n/rtl_unittest.cc b/base/i18n/rtl_unittest.cc
@@ -468,6 +468,87 @@ TEST_F(RTLTest, UnadjustStringForLocaleDirection) {
   EXPECT_EQ(was_rtl, IsRTL());
 }
 
+TEST_F(RTLTest, EnsureTerminatedDirectionalFormatting) {
+  struct {
+    const wchar_t* unformated_text;
+    const wchar_t* formatted_text;
+  } cases[] = {
+      // Tests string without any dir-formatting characters.
+      {L"google.com", L"google.com"},
+      // Tests string with properly terminated dir-formatting character.
+      {L"\x202egoogle.com\x202c", L"\x202egoogle.com\x202c"},
+      // Tests string with over-terminated dir-formatting characters.
+      {L"\x202egoogle\x202c.com\x202c", L"\x202egoogle\x202c.com\x202c"},
+      // Tests string beginning with a dir-formatting character.
+      {L"\x202emoc.elgoog", L"\x202emoc.elgoog\x202c"},
+      // Tests string that over-terminates then re-opens.
+      {L"\x202egoogle\x202c\x202c.\x202eom",
+       L"\x202egoogle\x202c\x202c.\x202eom\x202c"},
+      // Tests string containing a dir-formatting character in the middle.
+      {L"google\x202e.com", L"google\x202e.com\x202c"},
+      // Tests string with multiple dir-formatting characters.
+      {L"\x202egoogle\x202e.com/\x202eguest",
+       L"\x202egoogle\x202e.com/\x202eguest\x202c\x202c\x202c"},
+      // Test the other dir-formatting characters (U+202A, U+202B, and U+202D).
+      {L"\x202agoogle.com", L"\x202agoogle.com\x202c"},
+      {L"\x202bgoogle.com", L"\x202bgoogle.com\x202c"},
+      {L"\x202dgoogle.com", L"\x202dgoogle.com\x202c"},
+  };
+
+  const bool was_rtl = IsRTL();
+
+  test::ScopedRestoreICUDefaultLocale restore_locale;
+  for (size_t i = 0; i < 2; ++i) {
+    // Toggle the application default text direction (to try each direction).
+    SetRTL(!IsRTL());
+    for (size_t i = 0; i < arraysize(cases); ++i) {
+      string16 unsanitized_text = WideToUTF16(cases[i].unformated_text);
+      string16 sanitized_text = WideToUTF16(cases[i].formatted_text);
+      EnsureTerminatedDirectionalFormatting(&unsanitized_text);
+      EXPECT_EQ(sanitized_text, unsanitized_text);
+    }
+  }
+  EXPECT_EQ(was_rtl, IsRTL());
+}
+
+TEST_F(RTLTest, SanitizeUserSuppliedString) {
+  struct {
+    const wchar_t* unformatted_text;
+    const wchar_t* formatted_text;
+  } cases[] = {
+      // Tests RTL string with properly terminated dir-formatting character.
+      {L"\x202eكبير Google التطبيق\x202c", L"\x202eكبير Google التطبيق\x202c"},
+      // Tests RTL string with over-terminated dir-formatting characters.
+      {L"\x202eكبير Google\x202cالتطبيق\x202c",
+       L"\x202eكبير Google\x202cالتطبيق\x202c"},
+      // Tests RTL string that over-terminates then re-opens.
+      {L"\x202eكبير Google\x202c\x202cالتطبيق\x202e",
+       L"\x202eكبير Google\x202c\x202cالتطبيق\x202e\x202c"},
+      // Tests RTL string with multiple dir-formatting characters.
+      {L"\x202eك\x202eبير Google الت\x202eطبيق",
+       L"\x202eك\x202eبير Google الت\x202eطبيق\x202c\x202c\x202c"},
+      // Test the other dir-formatting characters (U+202A, U+202B, and U+202D).
+      {L"\x202aكبير Google التطبيق", L"\x202aكبير Google التطبيق\x202c"},
+      {L"\x202bكبير Google التطبيق", L"\x202bكبير Google التطبيق\x202c"},
+      {L"\x202dكبير Google التطبيق", L"\x202dكبير Google التطبيق\x202c"},
+
+  };
+
+  for (size_t i = 0; i < arraysize(cases); ++i) {
+    // On Windows for an LTR locale, no changes to the string are made.
+    string16 prefix, suffix = WideToUTF16(L"");
+#if !defined(OS_WIN)
+    prefix = WideToUTF16(L"\x200e\x202b");
+    suffix = WideToUTF16(L"\x202c\x200e");
+#endif  // !OS_WIN
+    string16 unsanitized_text = WideToUTF16(cases[i].unformatted_text);
+    string16 sanitized_text =
+        prefix + WideToUTF16(cases[i].formatted_text) + suffix;
+    SanitizeUserSuppliedString(&unsanitized_text);
+    EXPECT_EQ(sanitized_text, unsanitized_text);
+  }
+}
+
 class SetICULocaleTest : public PlatformTest {};
 
 TEST_F(SetICULocaleTest, OverlongLocaleId) {

diff --git a/chrome/common/extensions/extension_unittest.cc b/chrome/common/extensions/extension_unittest.cc
@@ -9,11 +9,15 @@
 #include "base/macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/command.h"
 #include "chrome/common/extensions/extension_test_util.h"
 #include "chrome/common/extensions/manifest_handlers/content_scripts_handler.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/grit/generated_resources.h"
 #include "components/crx_file/id_util.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
@@ -27,6 +31,7 @@
 #include "skia/ext/image_operations.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
 
@@ -150,6 +155,43 @@ TEST(ExtensionTest, EmptyName) {
   EXPECT_EQ("", extension->name());
 }
 
+TEST(ExtensionTest, RTLNameInLTRLocale) {
+  // Test the case when a directional override is the first character.
+  auto run_rtl_test = [](const wchar_t* name, const wchar_t* expected) {
+    SCOPED_TRACE(
+        base::StringPrintf("Name: %ls, Expected: %ls", name, expected));
+    DictionaryBuilder manifest;
+    manifest.Set("name", base::WideToUTF8(name))
+        .Set("manifest_version", 2)
+        .Set("description", "some description")
+        .Set("version",
+             "0.1");  // <NOTE> Moved this here to avoid the MergeManifest call.
+    scoped_refptr<const Extension> extension =
+        ExtensionBuilder().SetManifest(manifest.Build()).Build();
+    ASSERT_TRUE(extension);
+    const int kResourceId = IDS_EXTENSION_PERMISSIONS_PROMPT_TITLE;
+    const base::string16 expected_utf16 = base::WideToUTF16(expected);
+    EXPECT_EQ(l10n_util::GetStringFUTF16(kResourceId, expected_utf16),
+              l10n_util::GetStringFUTF16(kResourceId,
+                                         base::UTF8ToUTF16(extension->name())));
+    EXPECT_EQ(base::WideToUTF8(expected), extension->name());
+  };
+
+  run_rtl_test(L"\x202emoc.elgoog", L"\x202emoc.elgoog\x202c");
+  run_rtl_test(L"\x202egoogle\x202e.com/\x202eguest",
+               L"\x202egoogle\x202e.com/\x202eguest\x202c\x202c\x202c");
+  run_rtl_test(L"google\x202e.com", L"google\x202e.com\x202c");
+
+  run_rtl_test(L"كبير Google التطبيق",
+#if !defined(OS_WIN)
+               L"\x200e\x202bكبير Google التطبيق\x202c\x200e");
+#else
+               // On Windows for an LTR locale, no changes to the string are
+               // made.
+               L"كبير Google التطبيق");
+#endif  // !OS_WIN
+}
+
 TEST(ExtensionTest, GetResourceURLAndPath) {
   scoped_refptr<Extension> extension = LoadManifestStrict("empty_manifest",
       "empty.json");

diff --git a/extensions/common/extension.cc b/extensions/common/extension.cc
@@ -555,10 +555,12 @@ bool Extension::LoadName(base::string16* error) {
     *error = base::ASCIIToUTF16(errors::kInvalidName);
     return false;
   }
+
   non_localized_name_ = base::UTF16ToUTF8(localized_name);
-  base::i18n::AdjustStringForLocaleDirection(&localized_name);
-  display_name_ =
-      base::UTF16ToUTF8(base::CollapseWhitespace(localized_name, true));
+  base::string16 sanitized_name =
+      base::CollapseWhitespace(localized_name, true);
+  base::i18n::SanitizeUserSuppliedString(&sanitized_name);
+  display_name_ = base::UTF16ToUTF8(sanitized_name);
   return true;
 }