Implement a new IDN display policy

components/url_formatter/url_formatter.cc

Index: components/url_formatter/url_formatter.cc
diff --git a/components/url_formatter/url_formatter.cc b/components/url_formatter/url_formatter.cc
index a8031bd2df26138a3c64ce1b602f0411563126cc..5786b62c1d2aebd7d9427f9abb35eeb626b3d39b 100644
--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -5,24 +5,22 @@
 #include "components/url_formatter/url_formatter.h"
 
 #include <algorithm>
-#include <map>
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/memory/singleton.h"
-#include "base/stl_util.h"
-#include "base/strings/string_tokenizer.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/numerics/safe_conversions.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_offset_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "third_party/icu/source/common/unicode/uidna.h"
 #include "third_party/icu/source/common/unicode/uniset.h"
-#include "third_party/icu/source/common/unicode/uscript.h"
 #include "third_party/icu/source/i18n/unicode/regex.h"
-#include "third_party/icu/source/i18n/unicode/ulocdata.h"
+#include "third_party/icu/source/i18n/unicode/uspoof.h"
 #include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
 
@@ -32,11 +30,9 @@ namespace {
 
 base::string16 IDNToUnicodeWithAdjustments(
     const std::string& host,
-    const std::string& languages,
     base::OffsetAdjuster::Adjustments* adjustments);
 bool IDNToUnicodeOneComponent(const base::char16* comp,
                               size_t comp_len,
-                              const std::string& languages,
                               base::string16* out);
 
 class AppendComponentTransform {
@@ -55,17 +51,14 @@ class AppendComponentTransform {
 
 class HostComponentTransform : public AppendComponentTransform {
  public:
-  explicit HostComponentTransform(const std::string& languages)
-      : languages_(languages) {}
+  explicit HostComponentTransform() {}
 
  private:
   base::string16 Execute(
       const std::string& component_text,
       base::OffsetAdjuster::Adjustments* adjustments) const override {
-    return IDNToUnicodeWithAdjustments(component_text, languages_, adjustments);
+    return IDNToUnicodeWithAdjustments(component_text, adjustments);
   }
-
-  const std::string& languages_;
 };
 
 class NonHostComponentTransform : public AppendComponentTransform {
@@ -157,7 +150,6 @@ void AdjustAllComponentsButScheme(int delta, url::Parsed* parsed) {
 // Helper for FormatUrlWithOffsets().
 base::string16 FormatViewSourceUrl(
     const GURL& url,
-    const std::string& languages,
     FormatUrlTypes format_types,
     net::UnescapeRule::Type unescape_rules,
     url::Parsed* new_parsed,
@@ -173,7 +165,7 @@ base::string16 FormatViewSourceUrl(
   base::string16 result(
       base::ASCIIToUTF16(kViewSource) +
       FormatUrlWithAdjustments(GURL(url_str.substr(kViewSourceLength)),
-                               languages, format_types, unescape_rules,
+                               std::string(), format_types, unescape_rules,
                                new_parsed, prefix_end, adjustments));
   // Revise |adjustments| by shifting to the offsets to prefix that the above
   // call to FormatUrl didn't get to see.
@@ -197,16 +189,10 @@ base::string16 FormatViewSourceUrl(
   return result;
 }
 
-// TODO(brettw) bug 734373: check the scripts for each host component and
-// don't un-IDN-ize if there is more than one. Alternatively, only IDN for
-// scripts that the user has installed. For now, just put the entire
-// path through IDN. Maybe this feature can be implemented in ICU itself?
-//
-// We may want to skip this step in the case of file URLs to allow unicode
-// UNC hostnames regardless of encodings.
+// TODO(brettw) We may want to skip this step in the case of file URLs to
+// allow unicode UNC hostnames regardless of encodings.
 base::string16 IDNToUnicodeWithAdjustments(
     const std::string& host,
-    const std::string& languages,
     base::OffsetAdjuster::Adjustments* adjustments) {
   if (adjustments)
     adjustments->clear();
@@ -232,7 +218,7 @@ base::string16 IDNToUnicodeWithAdjustments(
       // Add the substring that we just found.
       converted_idn =
           IDNToUnicodeOneComponent(input16.data() + component_start,
-                                   component_length, languages, &out16);
+                                   component_length, &out16);
     }
     size_t new_component_length = out16.length() - new_component_start;
 
@@ -248,245 +234,187 @@ base::string16 IDNToUnicodeWithAdjustments(
   return out16;
 }
 
-// Does some simple normalization of scripts so we can allow certain scripts
-// to exist together.
-// TODO(brettw) bug 880223: we should allow some other languages to be
-// oombined such as Chinese and Latin. We will probably need a more
-// complicated system of language pairs to have more fine-grained control.
-UScriptCode NormalizeScript(UScriptCode code) {
-  switch (code) {
-    case USCRIPT_KATAKANA:
-    case USCRIPT_HIRAGANA:
-    case USCRIPT_KATAKANA_OR_HIRAGANA:
-    case USCRIPT_HANGUL:  // This one is arguable.
-      return USCRIPT_HAN;
-    default:
-      return code;
-  }
-}
-
-bool IsIDNComponentInSingleScript(const base::char16* str, int str_len) {
-  UScriptCode first_script = USCRIPT_INVALID_CODE;
-  bool is_first = true;
-
-  int i = 0;
-  while (i < str_len) {
-    unsigned code_point;
-    U16_NEXT(str, i, str_len, code_point);
-
-    UErrorCode err = U_ZERO_ERROR;
-    UScriptCode cur_script = uscript_getScript(code_point, &err);
-    if (err != U_ZERO_ERROR)
-      return false;  // Report mixed on error.
-    cur_script = NormalizeScript(cur_script);
-
-    // TODO(brettw) We may have to check for USCRIPT_INHERENT as well.
-    if (is_first && cur_script != USCRIPT_COMMON) {
-      first_script = cur_script;
-      is_first = false;
-    } else {
-      if (cur_script != USCRIPT_COMMON && cur_script != first_script)
-        return false;
-    }
-  }
-  return true;
-}
+// A helper class for IDN Spoof checker 1st pass. When created lazily
+// the first time it's necessary, it'll initialize and set up |USpoofChecker|
+// with the level of script mixing, allowed characters, and types of checks.

[Ryan Sleevi, 2015/09/08 21:45:58]

Comment nit: I found this comment a bit hard to read. I've tried rewriting it,
both to make sure I correctly understand and to offer feedback.

// A helper class for the first pass of IDN spoof checking.

I'm not clear what "level of script mixing, allowed characters, and types of
checks" is meant to convey. As I read it, it suggests parameters / inputs, but
perhaps you mean it as "The policies that the Chromium security and
internationalization teams have decided on", in which case, it might be best set

// A helper class for the first pass of IDN spoof checking, used to
// ensure that no IDN input meets Chromium's standard of spoofability.
// For a more thorough explanation of how spoof checking works in
// Chromium, see [link to Markdown file explaining?]

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+class IDNSpoofChecker {
+ public:
+  IDNSpoofChecker();
+  // Return true if |label| is deemed safe to display in Unicode. The check
+  // is done with |checker_|. In case |checker_| is not properly initialized,
+  // all the IDN labels are regarded as unsafe and false is returned.
+  // Besides, it internally calls IDNSpoofCheckerExtra::Check if additional
+  // check is necessary.

[Ryan Sleevi, 2015/09/08 21:45:58]

Comment nit: Suggested rewording

// Returns true if |label| is safe to display as Unicode. In the event
// of library failure, all IDN inputs will be treated as unsafe.


In this reword, I try to remove the documentation about the implementation ("is
done with |checker_|", "Besides, it internally calls"), since that should be
both obvious from the code and fragile to document. Instead, it just spells out
the API contract.

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+  bool Check(base::StringPiece16 label);
 
-// Check if the script of a language can be 'safely' mixed with
-// Latin letters in the ASCII range.
-bool IsCompatibleWithASCIILetters(const std::string& lang) {
-  // For now, just list Chinese, Japanese and Korean (positive list).
-  // An alternative is negative-listing (languages using Greek and
-  // Cyrillic letters), but it can be more dangerous.
-  return !lang.substr(0, 2).compare("zh") || !lang.substr(0, 2).compare("ja") ||
-         !lang.substr(0, 2).compare("ko");
-}
+ private:
+  USpoofChecker* checker_;
+  DISALLOW_COPY_AND_ASSIGN(IDNSpoofChecker);
+};
 
-typedef std::map<std::string, icu::UnicodeSet*> LangToExemplarSetMap;
+base::LazyInstance<IDNSpoofChecker>::Leaky g_idn_spoof_checker =
+    LAZY_INSTANCE_INITIALIZER;
 
-class LangToExemplarSet {
+// A helper class for IDN Spoof checker 2nd pass. It is lazily created
+// the first time it's necessary and initializes a Unicode set and
+// a regex necessary for extra checks done on top of checks done with
+// |USpoofChecker| in IDNSpoofChecker.

[Ryan Sleevi, 2015/09/08 21:45:59]

// A helper class for the second pass of IDN spoof checking.
// For a more thorough explanation of how spoof checking works in Chromium,
// see [link to Markdown file explaining first and second passes]

It's unclear to me why this is a separate helper object, as opposed to just
being one spoof checker that does both passes, but I haven't read the rest of
the code yet, I'm just basing it on "read this file and try to understand from
header comments what will happen"

[jungshik at Google, 2016/03/11 20:05:37]

I removed this extra class and fold that into the first class.

+class IDNSpoofCheckerExtra {
  public:
-  static LangToExemplarSet* GetInstance() {
-    return Singleton<LangToExemplarSet>::get();
-  }
+  IDNSpoofCheckerExtra();
+  // Return true if extra checks on |label| find it to be safe to display
+  // in Unicode. Called by IDNSpoofChecker::Check.

[Ryan Sleevi, 2015/09/08 21:45:58]

Comment nit: I would remove the second sentence - that's discussing
implementation details of some 'other' implementation (even if it is in the same
file)

// Returns true if |label| is safe to display as Unicode according to
// extended spoofability criteria. Because this extended criteria may
// be expensive, it should only be used after first ensuring that |label|
// is safe using IDNSpoofChecker::Check.


Of course, the problem with the above suggested reword is you see how tightly
coupled IDNSpoofChecker and IDNSpoofCheckerExtra are. If *Extra is an
implementation detail of IDNSpoofChecker, then SpoofChecker shouldn't be talking
about it. If it's the caller's responsibility to call both, then we should
document that. But if IDNSpoofChecker is calling IDNSpoofCheckerExtra and that's
the API contract, then the comments should be clearer to that point, such as

// Implementation detail of IDNSpoofChecker.
// As some checks for spoofability are expensive to compute,
IDNSpoofCheckerExtra
// implements the secondary checks after a given |label| has passed
// the initial spoof checking.
// This class should never be called directly - spoofability should
// be checked with IDNSpoofChecker - and just exists to encapsulate the
// more expensive logic.

[jungshik at Google, 2016/03/11 20:05:37]

Initially, I expected ExtraCheck to have more expensive logic (such as checking
against known good domains, etc) than what id has now, but I didn't do that (at
least in this CL) so that folding ExtraCheck into IDNSpoofChecker is simpler.
There's a bit more initialization to do for extra check, but it's not expensive.
When we have more expensive logic, we can separate that out. 

+  bool Check(base::StringPiece16 label);
 
  private:
-  LangToExemplarSetMap map;
-  LangToExemplarSet() {}
-  ~LangToExemplarSet() {
-    STLDeleteContainerPairSecondPointers(map.begin(), map.end());
-  }
-
-  friend class Singleton<LangToExemplarSet>;
-  friend struct DefaultSingletonTraits<LangToExemplarSet>;
-  friend bool GetExemplarSetForLang(const std::string&, icu::UnicodeSet**);
-  friend void SetExemplarSetForLang(const std::string&, icu::UnicodeSet*);
-
-  DISALLOW_COPY_AND_ASSIGN(LangToExemplarSet);
+  icu::UnicodeSet non_ascii_latin_;
+  icu::RegexPattern* dangerous_pattern_;
+  DISALLOW_COPY_AND_ASSIGN(IDNSpoofCheckerExtra);
 };
 
-bool GetExemplarSetForLang(const std::string& lang,
-                           icu::UnicodeSet** lang_set) {
-  const LangToExemplarSetMap& map = LangToExemplarSet::GetInstance()->map;
-  LangToExemplarSetMap::const_iterator pos = map.find(lang);
-  if (pos != map.end()) {
-    *lang_set = pos->second;
-    return true;
+base::LazyInstance<IDNSpoofCheckerExtra>::Leaky g_idn_spoof_checker_extra =
+    LAZY_INSTANCE_INITIALIZER;
+
+IDNSpoofChecker::IDNSpoofChecker() {
+  UErrorCode status = U_ZERO_ERROR;
+  checker_ = uspoof_open(&status);
+  if (U_FAILURE(status)) {
+    checker_ = nullptr;
+    LOG(ERROR) << "IDN spoof checker failed to open with error, "
+               << u_errorName(status)
+               << " ; all IDN will be shown in punycode.";
+    return;
   }
-  return false;
-}
 
-void SetExemplarSetForLang(const std::string& lang, icu::UnicodeSet* lang_set) {
-  LangToExemplarSetMap& map = LangToExemplarSet::GetInstance()->map;
-  map.insert(std::make_pair(lang, lang_set));
+  // Use 'hightly restrictive' restritiction level to limit the script mixing
+  // to Latin + Han + {Hiragana + Katakana, Bopomofo, Hangul}.
+  // See http://www.unicode.org/reports/tr39/#Restriction_Level_Detection
+  // The default is highly restrictive so that it's not set explicitly.

[Ryan Sleevi, 2015/09/08 21:45:58]

I have trouble making sense of this sentence. It seems to suggest that the API
call is unnecessary (because it's the default), but then you're setting it, so
I'm not sure how to make sense of it.

[jungshik at Google, 2016/03/11 20:05:37]

Sorry for the confusion. (yeah, it's confusing.). 
Anyway, I switched to moderately restrictive and the comment was rewritten
accordingly. 

+  // TODO(jshin): Firefox uses 'moderately restrictive' by default. Review
+  // using that, instead.

[Ryan Sleevi, 2015/09/08 21:45:58]

Is there a bug open for this review? Just some sort of tracking bug for Chromium
:)

[jungshik at Google, 2016/03/11 20:05:37]

By switching to moderately restrictive, no more need for TODO.

+  uspoof_setRestrictionLevel(checker_, USPOOF_HIGHLY_RESTRICTIVE);
+
+  // The recommended set and inclusion set come from
+  // http://unicode.org/reports/tr39/ and
+  // http://www.unicode.org/Public/security/latest/xidmodifications.txt
+  // The list can undergo some changes as a new version of Unicode is
+  // released and we update our copy of ICU.

[Ryan Sleevi, 2015/09/08 21:45:58]

Comment nit: Avoid pronouns in comments (
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M
)

// The recommended set and inclusion set come from ...
// This list may change over time, and will be updated whenever the
// included version of ICU is updated.

(This avoids ambiguity for things like Chromium linking against system ICU, or
non-Chromium distributions, etc - the "who is 'we'" question)

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+  const icu::UnicodeSet* recommended_set =
+      uspoof_getRecommendedUnicodeSet(&status);
+  icu::UnicodeSet allowed_set;
+  allowed_set.addAll(*recommended_set);
+  const icu::UnicodeSet* inclusion_set = uspoof_getInclusionUnicodeSet(&status);
+  allowed_set.addAll(*inclusion_set);

[Ryan Sleevi, 2015/09/08 21:45:58]

So you explain where the lists come from, but don't quite explain the policy
here (as in, why is it OK to add these to the allowed sets).

I suspect this is something that might be best in an .md or explainer.

[jungshik at Google, 2016/03/11 20:05:37]

Made the comment more self-explanatory. A separate document will have more
details.

+
+  // From UAX 31 Table 6:
+  // http://www.unicode.org/reports/tr31/#Aspirational_Use_Scripts
+  const icu::UnicodeSet aspirational_scripts(
+      UNICODE_STRING_SIMPLE(
+          "[[:sc=Cans:][:sc=Plrd:][:sc=Mong:][:sc=Tfng:][:sc=Yiii:]]"),
+      status);
+  allowed_set.addAll(aspirational_scripts);
+
+  // Add 'Black Heart Suit' and 'Circled White Star'.
+  // TODO(jshin): How about other heart-like characters and Emoji (e.g.
+  // U+1F600) ?

[Ryan Sleevi, 2015/09/08 21:45:58]

Document and explain :P

[jungshik at Google, 2016/03/11 20:05:37]

Yeah... they're rather arbitrary (they come from ...) . I just nixed them for
now. I need to give more though on what to do with Emoji and other symbols.
(it'd be great if we can just pass them all (most of them) and have an entirely
different layer of defense against spoofing such as safe-browsing that can do a
lot more off-line processing with a lot more data).   

+  allowed_set.add(0x2665u);
+  allowed_set.add(0x272au);
+
+  // Remove the following three characters listed in Mozilla's blacklist (
+  // http://kb.mozillazine.org/Network.IDN.blacklist_chars ) but
+  // not yet excluded from |allowed_set| up to this point:
+  // Combining Long Solidus Overlay, Hebrew Punctuation Gershayim, and
+  // Hyphenation Point

[Ryan Sleevi, 2015/09/08 21:45:58]

comment nit: "." at the end of Hyphenation Point (ending the sentence)

comment nit: This reads a bit weird, the "not yet excluded from |allowed_set|".
Up until this point, there's no exclusions from allowed_set, so it's kinda
grammatically weird.

A suggested reword might be something like

// The following three characters are included in (recommended? inclusion) set,
// but are blacklisted as part of Mozilla's IDN blacklist (link here).
// Explicitly remove them, which does nothing if they're removed from
// (recommended, inclusion?) set in a future ICU update.

[jungshik at Google, 2016/03/11 20:05:37]

I reviewed those 3 characters again and they can be safely allowed except for
U+0338 which is a bit problematic when rendered with a broken font/text renering
engine because it can be mistaken for a forward slash. (with the correct
rendering, it should not). I'm dropping only that character. 

+  allowed_set.remove(0x338u);   // Combining Long Solidus Overlay
+  allowed_set.remove(0x5f4u);   // Hebrew Punctuation Gershayim
+  allowed_set.remove(0x2027u);  // Hyphenation Point
+
+  // TODO(jshin): Decide what to do with '+' and U+0020. For now, leave
+  // them out as Mozilla does.

[Ryan Sleevi, 2015/09/08 21:45:58]

Bug #? :)

[jungshik at Google, 2016/03/11 20:05:37]

Not necessary. They'd better be kept out. (per both uts46 and idna2008)  Removed
the comment. 

+  uspoof_setAllowedUnicodeSet(checker_, &allowed_set, &status);
+
+  int32_t checks = uspoof_getChecks(checker_, &status);
+  // Do not allow mixed numbering systems (e.g. ASCII digits and
+  // Devanagari digits) or invisible characters or multiple occurrences
+  // there is a script mixing.
+  checks |= USPOOF_MIXED_NUMBERS | USPOOF_AUX_INFO;
+
+  // USPOOF_INVISBLE should be on by this point without being
+  // explicitly turned on.
+  DCHECK(checks & USPOOF_INVISIBLE);
+
+  // Disable whole-script-confusable check because even 'pax' (Latin)
+  // and b<u-umlaut>cher cannot pass the test because Cyrillic/Greek have
+  // confusable characters for all letters in them.

[Ryan Sleevi, 2015/09/08 21:45:59]

comment nit: Grammatically, this is weird because of the double "because"

// Disable whole-script-confusable checks.
// While this check would be desirable, simple strings such as
// 'pax' (Latin) and 'b<u-umlaut>cher' (Latin?) are whole script confusable
// with other scripts (such as Cyrillic/Greek).

[Did the above capture the concerns?]

I'm also not sure the "check against a list of well known good domain names" -
is this talking about whitelisted TLDs? whitelisted strings? Something else?

[jungshik at Google, 2016/03/11 20:05:37]

Rewrote the comment with more details on a possible alternative.

+  // TODO(jshin): Disabling this check has a downside. One way to alleviate
+  // is to check against a list of well known good domain names.
+  checks ^= USPOOF_WHOLE_SCRIPT_CONFUSABLE;
+
+  uspoof_setChecks(checker_, checks, &status);
+  DCHECK(U_SUCCESS(status));
 }
 
-static base::LazyInstance<base::Lock>::Leaky g_lang_set_lock =
-    LAZY_INSTANCE_INITIALIZER;
-
-// Returns true if all the characters in component_characters are used by
-// the language |lang|.
-bool IsComponentCoveredByLang(const icu::UnicodeSet& component_characters,
-                              const std::string& lang) {
-  CR_DEFINE_STATIC_LOCAL(const icu::UnicodeSet, kASCIILetters, ('a', 'z'));
-  icu::UnicodeSet* lang_set = nullptr;
-  // We're called from both the UI thread and the history thread.
-  {
-    base::AutoLock lock(g_lang_set_lock.Get());
-    if (!GetExemplarSetForLang(lang, &lang_set)) {
-      UErrorCode status = U_ZERO_ERROR;
-      ULocaleData* uld = ulocdata_open(lang.c_str(), &status);
-      // TODO(jungshik) Turn this check on when the ICU data file is
-      // rebuilt with the minimal subset of locale data for languages
-      // to which Chrome is not localized but which we offer in the list
-      // of languages selectable for Accept-Languages. With the rebuilt ICU
-      // data, ulocdata_open never should fall back to the default locale.
-      // (issue 2078)
-      // DCHECK(U_SUCCESS(status) && status != U_USING_DEFAULT_WARNING);
-      if (U_SUCCESS(status) && status != U_USING_DEFAULT_WARNING) {
-        lang_set = reinterpret_cast<icu::UnicodeSet*>(ulocdata_getExemplarSet(
-            uld, nullptr, 0, ULOCDATA_ES_STANDARD, &status));
-        // On success, if |lang| is compatible with ASCII Latin letters, add
-        // them.
-        if (lang_set && IsCompatibleWithASCIILetters(lang))
-          lang_set->addAll(kASCIILetters);
-      }
+inline bool IDNSpoofChecker::Check(base::StringPiece16 label) {
+  UErrorCode status = U_ZERO_ERROR;
+  int32_t results = uspoof_check(checker_, label.data(),
+                                 base::checked_cast<int32_t>(label.size()),
+                                 NULL, &status);
+  // If uspoof_check fails, consider all IDN unsafe to be conservative.
+  if (U_FAILURE(status) || results & USPOOF_ALL_CHECKS)
+    return false;
 
-      if (!lang_set)
-        lang_set = new icu::UnicodeSet(1, 0);
+  // If there's no script mixing, the input is regarded as safe
+  // without any extra check.
+  if (results == USPOOF_ASCII || results == USPOOF_SINGLE_SCRIPT_RESTRICTIVE)
+    return true;
 
-      lang_set->freeze();
-      SetExemplarSetForLang(lang, lang_set);
-      ulocdata_close(uld);
-    }
-  }
-  return !lang_set->isEmpty() && lang_set->containsAll(component_characters);
+  return g_idn_spoof_checker_extra.Get().Check(label);
 }
 
-// Returns true if the given Unicode host component is safe to display to the
-// user.
-bool IsIDNComponentSafe(const base::char16* str,
-                        int str_len,
-                        const std::string& languages) {
-  // Most common cases (non-IDN) do not reach here so that we don't
-  // need a fast return path.
-  // TODO(jungshik) : Check if there's any character inappropriate
-  // (although allowed) for domain names.
-  // See http://www.unicode.org/reports/tr39/#IDN_Security_Profiles and
-  // http://www.unicode.org/reports/tr39/data/xidmodifications.txt
-  // For now, we borrow the list from Mozilla and tweaked it slightly.
-  // (e.g. Characters like U+00A0, U+3000, U+3002 are omitted because
-  //  they're gonna be canonicalized to U+0020 and full stop before
-  //  reaching here.)
-  // The original list is available at
-  // http://kb.mozillazine.org/Network.IDN.blacklist_chars and
-  // at
-  // http://mxr.mozilla.org/seamonkey/source/modules/libpref/src/init/all.js#703
-
+IDNSpoofCheckerExtra::IDNSpoofCheckerExtra() {
   UErrorCode status = U_ZERO_ERROR;
-#ifdef U_WCHAR_IS_UTF16
-  icu::UnicodeSet dangerous_characters(
-      icu::UnicodeString(
-          L"[[\\ \u00ad\u00bc\u00bd\u01c3\u0337\u0338"
-          L"\u05c3\u05f4\u06d4\u0702\u115f\u1160][\u2000-\u200b]"
-          L"[\u2024\u2027\u2028\u2029\u2039\u203a\u2044\u205f]"
-          L"[\u2154-\u2156][\u2159-\u215b][\u215f\u2215\u23ae"
-          L"\u29f6\u29f8\u2afb\u2afd][\u2ff0-\u2ffb][\u3014"
-          L"\u3015\u3033\u3164\u321d\u321e\u33ae\u33af\u33c6\u33df\ufe14"
-          L"\ufe15\ufe3f\ufe5d\ufe5e\ufeff\uff0e\uff06\uff61\uffa0\ufff9]"
-          L"[\ufffa-\ufffd]\U0001f50f\U0001f510\U0001f512\U0001f513]"),
-      status);
-  DCHECK(U_SUCCESS(status));
-  icu::RegexMatcher dangerous_patterns(
-      icu::UnicodeString(
-          // Lone katakana no, so, or n
-          L"[^\\p{Katakana}][\u30ce\u30f3\u30bd][^\\p{Katakana}]"
-          // Repeating Japanese accent characters
-          L"|[\u3099\u309a\u309b\u309c][\u3099\u309a\u309b\u309c]"),
+  non_ascii_latin_ = icu::UnicodeSet(
+      UNICODE_STRING_SIMPLE("[[:sc=Latn:] - [a-zA-Z]]"), status);
+
+  dangerous_pattern_ = icu::RegexPattern::compile(
+      UNICODE_STRING_SIMPLE(
+          // Lone (out-of-context) katakana no, so, zo, or n
+          // They can be mistaken for a slash. Only allow them
+          // when enclosed by Katakana, Hiragana and Han.
+          "[^\\p{Katakana}\\p{Hiragana}\\p{Han}]"
+          "[\\u30ce\\u30f3\\u30bd\\u30be]"
+          "[^\\p{Katakana}\\p{Hiragana}\\p{Han}]"

[Ryan Sleevi, 2015/09/08 21:45:58]

As written, doesn't this mean that a lone katakana no, so, zo, or n (without any
context) will be allowed, since it'll fail the regex before/after?

Or that

"\\u0061\\u30ce" would be treated as safe (I'm probably butchering things here)

[jungshik at Google, 2016/03/11 20:05:37]

Thank you for catching it. Rewrote the regex. 

+          // Repeating Japanese accent characters. USPOOF_INVISIBLE
+          // only checks for a repeated occurence of the same combining
+          // mark, but we block a sequence of similary looking

[Ryan Sleevi, 2015/09/08 21:45:58]

same comments about "we"

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+          // Japanese combining marks as well.

[Ryan Sleevi, 2015/09/08 21:45:58]

So, overall comment wise, this is somewhat hard to read because the comments are
written for the positive case, but the actual regex is the negative (dangerous
patterns).

It may help to ensure the comments match the negativity.

// Disallow the katakana no, so, zo, or n, as they may be mistaken
// for slashes, unless they're entirely enclosed by Katakana, Hiragana,
// or Han scripts.

// Disallow repeating Japanese accent characters. Because
// USPOOF_INVISIBLE will only check for repeated occurrences of the
// same combining mark, it's necessary to block any sequence of
// similar looking Japanese combining marks as well.

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+          "|[\\u3099-\\u309c][\\u3099-\\u309c]"),
       0, status);
-#else
-  icu::UnicodeSet dangerous_characters(
-      icu::UnicodeString(
-          "[[\\u0020\\u00ad\\u00bc\\u00bd\\u01c3\\u0337\\u0338"
-          "\\u05c3\\u05f4\\u06d4\\u0702\\u115f\\u1160][\\u2000-\\u200b]"
-          "[\\u2024\\u2027\\u2028\\u2029\\u2039\\u203a\\u2044\\u205f]"
-          "[\\u2154-\\u2156][\\u2159-\\u215b][\\u215f\\u2215\\u23ae"
-          "\\u29f6\\u29f8\\u2afb\\u2afd][\\u2ff0-\\u2ffb][\\u3014"
-          "\\u3015\\u3033\\u3164\\u321d\\u321e\\u33ae\\u33af\\u33c6\\u33df\\ufe"
-          "14"
-          "\\ufe15\\ufe3f\\ufe5d\\ufe5e\\ufeff\\uff0e\\uff06\\uff61\\uffa0\\uff"
-          "f9]"
-          "[\\ufffa-\\ufffd]\\U0001f50f\\U0001f510\\U0001f512\\U0001f513]",
-          -1, US_INV),
-      status);
-  DCHECK(U_SUCCESS(status));
-  icu::RegexMatcher dangerous_patterns(
-      icu::UnicodeString(
-          // Lone katakana no, so, or n
-          "[^\\p{Katakana}][\\u30ce\\u30f3\\u30bd][^\\p{Katakana}]"
-          // Repeating Japanese accent characters
-          "|[\\u3099\\u309a\\u309b\\u309c][\\u3099\\u309a\\u309b\\u309c]"),
-      0, status);
-#endif
   DCHECK(U_SUCCESS(status));
-  icu::UnicodeSet component_characters;
-  icu::UnicodeString component_string(str, str_len);
-  component_characters.addAll(component_string);
-  if (dangerous_characters.containsSome(component_characters))
-    return false;
+}
 
-  DCHECK(U_SUCCESS(status));
-  dangerous_patterns.reset(component_string);
-  if (dangerous_patterns.find())
+inline bool IDNSpoofCheckerExtra::Check(base::StringPiece16 label) {
+  // This is called only if script mixing is detected.

[Ryan Sleevi, 2015/09/08 21:45:59]

Same comment about documenting 'how it's used' and such. This comment seems
superfluous, but if you're trying to document a precondition, do it as such


// |label| is a string that contains multiple, mixed scripts.

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+  // Limit Latin letters that can be mixed with other scripts to
+  // ASCII-Latin instead  of any Latin.

[Ryan Sleevi, 2015/09/08 21:45:58]

comment nit: s/  / /

[jungshik at Google, 2016/03/11 20:05:37]

Done.

+  icu::UnicodeString label_string(FALSE, label.data(),
+                                  base::checked_cast<int32_t>(label.size()));
+  if (non_ascii_latin_.containsSome(label_string))
     return false;
 
-  // If the language list is empty, the result is completely determined
-  // by whether a component is a single script or not. This will block
-  // even "safe" script mixing cases like <Chinese, Latin-ASCII> that are
-  // allowed with |languages| (while it blocks Chinese + Latin letters with
-  // an accent as should be the case), but we want to err on the safe side
-  // when |languages| is empty.
-  if (languages.empty())
-    return IsIDNComponentInSingleScript(str, str_len);
-
-  // |common_characters| is made up of  ASCII numbers, hyphen, plus and
-  // underscore that are used across scripts and allowed in domain names.
-  // (sync'd with characters allowed in url_canon_host with square
-  // brackets excluded.) See kHostCharLookup[] array in url_canon_host.cc.
-  icu::UnicodeSet common_characters(UNICODE_STRING_SIMPLE("[[0-9]\\-_+\\ ]"),
-                                    status);
+  UErrorCode status = U_ZERO_ERROR;
+  scoped_ptr<icu::RegexMatcher> dangerous_pattern_matcher(
+      dangerous_pattern_->matcher(label_string, status));
   DCHECK(U_SUCCESS(status));
-  // Subtract common characters because they're always allowed so that
-  // we just have to check if a language-specific set contains
-  // the remainder.
-  component_characters.removeAll(common_characters);
-
-  base::StringTokenizer t(languages, ",");
-  while (t.GetNext()) {
-    if (IsComponentCoveredByLang(component_characters, t.token()))
-      return true;
-  }
-  return false;
+  return !dangerous_pattern_matcher->find();
+
+  // TODO(jshin): Check spoofing attempt against a list of 'good' domains

[Ryan Sleevi, 2015/09/08 21:45:58]

BUG #? Is this domains (labels?) or TLDs?

[jungshik at Google, 2016/03/11 20:05:37]

What I had in mind is now documented in another part of the code as a possible
alternative with more details. 

+}
+
+// Returns true if the given Unicode host component is safe to display to the
+// user.
+bool IsIDNComponentSafe(base::StringPiece16 label) {
+  return g_idn_spoof_checker.Get().Check(label);
 }
 
 // A wrapper to use LazyInstance<>::Leaky with ICU's UIDNA, a C pointer to
@@ -522,16 +450,14 @@ struct UIDNAWrapper {
   UIDNA* value;
 };
 
-static base::LazyInstance<UIDNAWrapper>::Leaky g_uidna =
-    LAZY_INSTANCE_INITIALIZER;
+base::LazyInstance<UIDNAWrapper>::Leaky g_uidna = LAZY_INSTANCE_INITIALIZER;
 
-// Converts one component of a host (between dots) to IDN if safe. The result
-// will be APPENDED to the given output string and will be the same as the input
-// if it is not IDN or the IDN is unsafe to display.  Returns whether any
-// conversion was performed.
+// Converts one component (label) of a host (between dots) to Unicode if safe.
+// The result will be APPENDED to the given output string and will be the
+// same as the input if it is not Punycode or the IDN is unsafe to display.
+// Returns whether any conversion was performed.
 bool IDNToUnicodeOneComponent(const base::char16* comp,
                               size_t comp_len,
-                              const std::string& languages,
                               base::string16* out) {
   DCHECK(out);
   if (comp_len == 0)
@@ -544,7 +470,7 @@ bool IDNToUnicodeOneComponent(const base::char16* comp,
     UIDNA* uidna = g_uidna.Get().value;
     DCHECK(uidna != NULL);
     size_t original_length = out->length();
-    int output_length = 64;
+    int32_t output_length = 64;
     UIDNAInfo info = UIDNA_INFO_INITIALIZER;
     UErrorCode status;
     do {
@@ -562,8 +488,9 @@ bool IDNToUnicodeOneComponent(const base::char16* comp,
       // Converted successfully. Ensure that the converted component
       // can be safely displayed to the user.
       out->resize(original_length + output_length);
-      if (IsIDNComponentSafe(out->data() + original_length, output_length,
-                             languages))
+      if (IsIDNComponentSafe(
+          base::StringPiece16(out->data() + original_length,
+                              base::checked_cast<size_t>(output_length))))
         return true;
     }
 
@@ -598,7 +525,7 @@ base::string16 FormatUrl(const GURL& url,
   if (offset_for_adjustment)
     offsets.push_back(*offset_for_adjustment);
   base::string16 result =
-      FormatUrlWithOffsets(url, languages, format_types, unescape_rules,
+      FormatUrlWithOffsets(url, std::string(), format_types, unescape_rules,
                            new_parsed, prefix_end, &offsets);
   if (offset_for_adjustment)
     *offset_for_adjustment = offsets[0];
@@ -615,7 +542,7 @@ base::string16 FormatUrlWithOffsets(
     std::vector<size_t>* offsets_for_adjustment) {
   base::OffsetAdjuster::Adjustments adjustments;
   const base::string16& format_url_return_value =
-      FormatUrlWithAdjustments(url, languages, format_types, unescape_rules,
+      FormatUrlWithAdjustments(url, std::string(), format_types, unescape_rules,
                                new_parsed, prefix_end, &adjustments);
   base::OffsetAdjuster::AdjustOffsets(adjustments, offsets_for_adjustment);
   if (offsets_for_adjustment) {
@@ -650,7 +577,7 @@ base::string16 FormatUrlWithAdjustments(
   if (url.SchemeIs(kViewSource) &&
       !base::StartsWith(url.possibly_invalid_spec(), kViewSourceTwice,
                         base::CompareCase::INSENSITIVE_ASCII)) {
-    return FormatViewSourceUrl(url, languages, format_types, unescape_rules,
+    return FormatViewSourceUrl(url, format_types, unescape_rules,
                                new_parsed, prefix_end, adjustments);
   }
 
@@ -720,7 +647,7 @@ base::string16 FormatUrlWithAdjustments(
     *prefix_end = static_cast<size_t>(url_string.length());
 
   // Host.
-  AppendFormattedComponent(spec, parsed.host, HostComponentTransform(languages),
+  AppendFormattedComponent(spec, parsed.host, HostComponentTransform(),
                            &url_string, &new_parsed->host, adjustments);
 
   // Port.
@@ -796,12 +723,12 @@ void AppendFormattedHost(const GURL& url,
                          base::string16* output) {
   AppendFormattedComponent(
       url.possibly_invalid_spec(), url.parsed_for_possibly_invalid_spec().host,
-      HostComponentTransform(languages), output, NULL, NULL);
+      HostComponentTransform(), output, NULL, NULL);
 }
 
 base::string16 IDNToUnicode(const std::string& host,
                             const std::string& languages) {
-  return IDNToUnicodeWithAdjustments(host, languages, NULL);
+  return IDNToUnicodeWithAdjustments(host, NULL);
 }
 
 }  // url_formatter