Percent-encode illegal characters in Android page info popup URL

chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java

Index: chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
index 6b6d07a3fa6620310bc2b4716e1851141934c4e4..340a2ba599238de46ec48bb01895aef4d2532cc4 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java
@@ -9,6 +9,7 @@ import android.content.Context;
 import android.content.DialogInterface;
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
+import android.net.Uri;
 import android.text.Layout;
 import android.text.Spannable;
 import android.text.SpannableStringBuilder;
@@ -164,6 +165,13 @@ public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedList
 
     private static final int MAX_TABLET_DIALOG_WIDTH_DP = 400;
 
+    // This is the "reserved" character set from RFC 3986, plus the '%' character.
+    private static final String URI_RESERVED_CHARACTERS = "!*'();:@&=+$,/?#[]%";
+
+    private static final char FIRST_UNICODE_WHITESPACE = '\u2000';
+    private static final char FINAL_UNICODE_WHITESPACE = '\u200F';
+    private static final char UNICODE_NBSP = '\u00A0';
+
     private final Context mContext;
     private final Profile mProfile;
     private final WebContents mWebContents;
@@ -289,7 +297,8 @@ public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedList
         }
         mSecurityLevel = ToolbarModel.getSecurityLevelForWebContents(mWebContents);
 
-        SpannableStringBuilder urlBuilder = new SpannableStringBuilder(mFullUrl);
+        String displayUrl = encodeSuspiciousFragment(mFullUrl);
+        SpannableStringBuilder urlBuilder = new SpannableStringBuilder(displayUrl);
         OmniboxUrlEmphasizer.emphasizeUrl(urlBuilder, mContext.getResources(), mProfile,
                 mSecurityLevel, mIsInternalPage, true);
         mUrlTitle.setText(urlBuilder);
@@ -300,6 +309,35 @@ public class WebsiteSettingsPopup implements OnClickListener, OnItemSelectedList
     }
 
     /**
+     * Percent-encodes a URL fragment if it contains suspicious unicode whitespace characters.

[Matt Giuca, 2015/03/24 06:28:37]

nit: Unicode is a proper noun (always capitalize when using in a sentence).

[tsergeant, 2015/03/25 01:55:24]

Done.

+     * Otherwise, returns the original URL. All parts of the URL other than the fragment will
+     * already be encoded.
+     */
+    public static String encodeSuspiciousFragment(String urlStr) {

[Matt Giuca, 2015/03/24 06:28:37]

What do you mean by "fragment" here? Do you mean simply "a piece of a URL", or
the specific meaning of "fragment" which is the part after the '#'?

If it's the former, you should pick a different word for it. If it's the latter,
I don't see that reflected in the code (there is nothing here which goes after
the fragment only), in which case this should just be renamed to
encodeSuspiciousUrl.

[tsergeant, 2015/03/25 01:55:24]

The intention was that even though we're working on the entire URL, only the
fragment will ever be modified. Fixed, anyway.

[Matt Giuca, 2015/03/25 03:23:24]

You should remove the last sentence of the documentation (which says it only
affects the fragment).

+        if (isSuspiciousUrl(urlStr)) {

[Matt Giuca, 2015/03/24 06:28:37]

I'm not too keen on this behaviour, as you will be encoding non-suspicious
characters conditionally on the presence of other suspicious characters.

Using your test example:
"Düsseldorf" becomes "Düsseldorf", but
"Düsseldorf, Germany" becomes "D%C3%BCsseldorf,%20Germany".

If you don't have a reason to encode the 'ü' in the former example, you should
not encode the 'ü' in the latter either.

(At the very least, you should include both the "Düsseldorf" and "Düsseldorf,
Germany" examples in the tests, so it's clear what the behaviour is.)

If your new plan is essentially to only consider spaces to be a problem, then I
think you should just encode the space characters. That makes it hard to use
Uri.encode (since it requires a whitelist, not a blacklist), but since you are
prepared to manually iterate over the string, you could just call Uri.encode on
just the characters you identify as spaces, and then put them back together.

[tsergeant, 2015/03/25 01:55:24]

Makes sense, Done.

+            // Explicitly pass the set of reserved characters to prevent them from being escaped.
+            return Uri.encode(urlStr, URI_RESERVED_CHARACTERS);
+        }
+        return urlStr;
+    }
+
+    /**
+     * Returns true if the fragment string contains characters that could be used to inject a
+     * phishing attack into the popup. We filter based on any unicode whitespace character.
+     */
+    public static boolean isSuspiciousUrl(String urlStr) {
+        for (int i = 0; i < urlStr.length(); i++) {
+            char fragmentChar = urlStr.charAt(i);
+            if ((fragmentChar >= FIRST_UNICODE_WHITESPACE
+                        && fragmentChar <= FINAL_UNICODE_WHITESPACE)
+                    || fragmentChar == ' '
+                    || fragmentChar == UNICODE_NBSP)
+                return true;
+        }
+        return false;
+    }
+
+    /**
      * Sets the visibility of the lower area of the dialog (containing the permissions and 'Site
      * Settings' button).
      *