Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1011383005:
Percent-encode illegal characters in Android page info popup URL (Closed)
|
Created:
5 years, 9 months ago by tsergeant Modified:
5 years, 9 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, sashab, benwells, Matt Giuca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPercent-encode illegal characters in Android page info popup URL
This prevents whitespace and non-ASCII characters from being displayed in
the url, stopping attacks where a carefully crafted URL can be used
to display a message in the popup.
BUG=466351
Committed: https://crrev.com/b1130cfc5dca2d5f01edafd7d50f795f391ab904
Cr-Commit-Position: refs/heads/master@{#322296}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Only encode when the URL contains unusual characters #Patch Set 3 : Reformat #
Total comments: 7
Patch Set 4 : Only encode whitespace characters #
Total comments: 10
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : Unicode test #Patch Set 7 : Change unicode representation #
Messages
Total messages: 20 (6 generated)
tsergeant@chromium.org changed reviewers: + tedchoc@chromium.org
tedchoc@, please take a look.
mgiuca@chromium.org changed reviewers: + mgiuca@chromium.org
Drive-by. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:168: // The set of characters which have syntactic meaning in a URL. // This is the "reserved" character set from RFC 3986, plus the '%' character. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:287: // allowed in a URL (spaces, ASCII control characters, non-ASCII Unicode). You are also allowing the following: "<>\^`{|} (Those characters are in neither the reserved nor unreserved sets, and are therefore illegal in URLs.) Assuming my understanding of Uri.encode is correct --- that it escapes all characters other than URI_RESERVED_CHARACTERS. So you should mention that in the comment. Also, explain the purpose of not simply calling encode with no |allow| argument: // Do not use the default allowed character set of Uri.encode, as that will escape reserved syntactic characters (eg, '&'). We must only escape illegal characters. (Feel free to rephrase.) https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:288: mFullUrl = Uri.encode(mWebContents.getVisibleUrl(), URI_RESERVED_CHARACTERS); Would it make sense to break this line out into a separate testable function? You could write a couple of tests that try URLs with characters from all the different sets (reserved, unreserved, space, control, non-ASCII and the above special illegal characters), to not only show that it behaves correctly, but also demonstrate the purpose of this encoding.
Patchset #3 (id:40001) has been deleted
Updated to only perform the encode if the URL fragment contains whitespace. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:168: // The set of characters which have syntactic meaning in a URL. On 2015/03/19 03:12:57, Matt Giuca (OOO til Mar 23) wrote: > // This is the "reserved" character set from RFC 3986, plus the '%' character. Done. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:288: mFullUrl = Uri.encode(mWebContents.getVisibleUrl(), URI_RESERVED_CHARACTERS); On 2015/03/19 03:12:57, Matt Giuca wrote: > Would it make sense to break this line out into a separate testable function? > > You could write a couple of tests that try URLs with characters from all the > different sets (reserved, unreserved, space, control, non-ASCII and the above > special illegal characters), to not only show that it behaves correctly, but > also demonstrate the purpose of this encoding. Done.
https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:168: // The set of characters which have syntactic meaning in a URL. nit: I meant to append my extra comment, not replace (your comment was also helpful). Feel free to put back, or just leave as-is. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: * Percent-encodes a URL fragment if it contains suspicious unicode whitespace characters. nit: Unicode is a proper noun (always capitalize when using in a sentence). https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316: public static String encodeSuspiciousFragment(String urlStr) { 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. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:317: if (isSuspiciousUrl(urlStr)) { 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.
https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: * Percent-encodes a URL fragment if it contains suspicious unicode whitespace characters. On 2015/03/24 06:28:37, Matt Giuca wrote: > nit: Unicode is a proper noun (always capitalize when using in a sentence). Done. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316: public static String encodeSuspiciousFragment(String urlStr) { On 2015/03/24 06:28:37, Matt Giuca wrote: > 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. The intention was that even though we're working on the entire URL, only the fragment will ever be modified. Fixed, anyway. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:317: if (isSuspiciousUrl(urlStr)) { On 2015/03/24 06:28:37, Matt Giuca wrote: > 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. Makes sense, Done.
lgtm with nits. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316: public static String encodeSuspiciousFragment(String urlStr) { You should remove the last sentence of the documentation (which says it only affects the fragment). https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:315: char fragmentChar = urlStr.charAt(i); nit: Rename fragmentChar to something else (maybe just 'c'), since this is not just the fragment.
lgtm w/ nits. The test case would be good to see if it can be switched out, but won't block on it. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: public static String encodeSuspiciousUrl(String urlStr) { This name to me implies that the URL is suspicious. Maybe just prepareUrlForDisplay or something. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:319: || fragmentChar == UNICODE_NBSP) braces are required in java unless the statement and conditional can all fit on a single line. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:13: public class WebsiteSettingsPopupTest extends InstrumentationTestCase { can this extend directly from TestCase instead of InstrumentationTestCase. Would be good if this can stay as a simple junit test for now. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:32: WebsiteSettingsPopup.encodeSuspiciousUrl("http://example.com/?q=a#Düsseldorf, Germany"), >100 chars
> The test case would be good to see if it can be switched out, but won't block on > it. Can you clarify what you mean by that? Something other than rewriting it as junit? https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: public static String encodeSuspiciousUrl(String urlStr) { On 2015/03/25 19:55:38, Ted C wrote: > This name to me implies that the URL is suspicious. > > Maybe just prepareUrlForDisplay or something. Done. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:315: char fragmentChar = urlStr.charAt(i); On 2015/03/25 03:23:24, Matt Giuca wrote: > nit: Rename fragmentChar to something else (maybe just 'c'), since this is not > just the fragment. Done. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:319: || fragmentChar == UNICODE_NBSP) On 2015/03/25 19:55:38, Ted C wrote: > braces are required in java unless the statement and conditional can all fit on > a single line. Done. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:13: public class WebsiteSettingsPopupTest extends InstrumentationTestCase { On 2015/03/25 19:55:38, Ted C wrote: > can this extend directly from TestCase instead of InstrumentationTestCase. > > Would be good if this can stay as a simple junit test for now. Done. https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:32: WebsiteSettingsPopup.encodeSuspiciousUrl("http://example.com/?q=a#Düsseldorf, Germany"), On 2015/03/25 19:55:38, Ted C wrote: > >100 chars Done.
https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:35: "http://example.com/?q=a#Düsseldorf,%20Germany"); Please add a test with an astral character (e.g., '𝒜') so we can be sure your iteration over the string correctly preserves them.
On 2015/03/25 23:44:47, tsergeant wrote: > > The test case would be good to see if it can be switched out, but won't block > on > > it. Nope...that is it. > > Can you clarify what you mean by that? Something other than rewriting it as > junit? > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java > (right): > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: > public static String encodeSuspiciousUrl(String urlStr) { > On 2015/03/25 19:55:38, Ted C wrote: > > This name to me implies that the URL is suspicious. > > > > Maybe just prepareUrlForDisplay or something. > > Done. > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:315: > char fragmentChar = urlStr.charAt(i); > On 2015/03/25 03:23:24, Matt Giuca wrote: > > nit: Rename fragmentChar to something else (maybe just 'c'), since this is not > > just the fragment. > > Done. > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:319: > || fragmentChar == UNICODE_NBSP) > On 2015/03/25 19:55:38, Ted C wrote: > > braces are required in java unless the statement and conditional can all fit > on > > a single line. > > Done. > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java > (right): > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:13: > public class WebsiteSettingsPopupTest extends InstrumentationTestCase { > On 2015/03/25 19:55:38, Ted C wrote: > > can this extend directly from TestCase instead of InstrumentationTestCase. > > > > Would be good if this can stay as a simple junit test for now. > > Done. > > https://codereview.chromium.org/1011383005/diff/80001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:32: > WebsiteSettingsPopup.encodeSuspiciousUrl("http://example.com/?q=a#Düsseldorf, > Germany"), > On 2015/03/25 19:55:38, Ted C wrote: > > >100 chars > > Done.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java:35: "http://example.com/?q=a#Düsseldorf,%20Germany"); On 2015/03/26 00:02:08, Matt Giuca wrote: > Please add a test with an astral character (e.g., '𝒜') so we can be sure your > iteration over the string correctly preserves them. Done.
The CQ bit was checked by tsergeant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/1011383005/#ps30003 (title: "Change unicode representation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011383005/30003
Message was sent while issue was closed.
Committed patchset #7 (id:30003)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b1130cfc5dca2d5f01edafd7d50f795f391ab904 Cr-Commit-Position: refs/heads/master@{#322296} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
