Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1679)

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
Reviewers:
Ted C, Matt Giuca
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.

Description

Percent-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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -1 line) Patch
M chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java View 1 2 3 4 4 chunks +27 lines, -1 line 0 comments Download
A chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (6 generated)
tsergeant
tedchoc@, please take a look.
5 years, 9 months ago (2015-03-19 02:52:00 UTC) #2
Matt Giuca
Drive-by. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode168 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:168: // The set of characters which have syntactic ...
5 years, 9 months ago (2015-03-19 03:12:57 UTC) #4
tsergeant
Updated to only perform the encode if the URL fragment contains whitespace. https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java ...
5 years, 9 months ago (2015-03-24 05:33:47 UTC) #6
Matt Giuca
https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/1/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode168 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:168: // The set of characters which have syntactic meaning ...
5 years, 9 months ago (2015-03-24 06:28:37 UTC) #7
tsergeant
https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode312 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:312: * Percent-encodes a URL fragment if it contains suspicious ...
5 years, 9 months ago (2015-03-25 01:55:24 UTC) #8
Matt Giuca
lgtm with nits. https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java File chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/1011383005/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode316 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:316: public static String encodeSuspiciousFragment(String urlStr) { ...
5 years, 9 months ago (2015-03-25 03:23:24 UTC) #9
Ted C
lgtm w/ nits. The test case would be good to see if it can be ...
5 years, 9 months ago (2015-03-25 19:55:38 UTC) #10
tsergeant
> The test case would be good to see if it can be switched out, ...
5 years, 9 months ago (2015-03-25 23:44:47 UTC) #11
Matt Giuca
https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java#newcode35 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 ...
5 years, 9 months ago (2015-03-26 00:02:09 UTC) #12
Ted C
On 2015/03/25 23:44:47, tsergeant wrote: > > The test case would be good to see ...
5 years, 9 months ago (2015-03-26 00:07:36 UTC) #13
tsergeant
https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java (right): https://codereview.chromium.org/1011383005/diff/100001/chrome/android/javatests/src/org/chromium/chrome/browser/WebsiteSettingsPopupTest.java#newcode35 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 ...
5 years, 9 months ago (2015-03-26 00:29:12 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011383005/30003
5 years, 9 months ago (2015-03-26 00:52:39 UTC) #18
commit-bot: I haz the power
Committed patchset #7 (id:30003)
5 years, 9 months ago (2015-03-26 01:57:04 UTC) #19
commit-bot: I haz the power
5 years, 9 months ago (2015-03-26 01:57:48 UTC) #20
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/b1130cfc5dca2d5f01edafd7d50f795f391ab904
Cr-Commit-Position: refs/heads/master@{#322296}

Powered by Google App Engine
This is Rietveld 408576698