Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 1077483002: Truncate long URL fragments in Android page info popup (Closed)

Created:
4 years ago by tsergeant
Modified:
4 years ago
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Truncate long URL fragments in Android page info popup This replaces percent-encoding whitespace as a method to combat the use of crafted URL fragments to inject messages into the page info popup. By truncating the URL so that at most two lines of the fragment are shown, we prevent lengthy messages from being injected while minimising the effect on most URLs. BUG=466351 Committed: https://crrev.com/5bf8a2dccf4777c5f065d918d1d9a2bbaa013f9f Cr-Commit-Position: refs/heads/master@{#325358}

Patch Set 1 #

Patch Set 2 : Remove test #

Patch Set 3 : Formatting #

Total comments: 2

Patch Set 4 : Handle URLs with no origin truncation #

Total comments: 2

Patch Set 5 : Actually handle URLs with no origin truncation #

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

Messages

Total messages: 13 (3 generated)
tsergeant
dtrainor@, can you please review? sashab@, can you take a quick look if you get ...
4 years ago (2015-04-15 01:10:58 UTC) #2
juhonurm
https://codereview.chromium.org/1077483002/diff/40001/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/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode135 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135: int originEndIndex = OmniboxUrlEmphasizer.getOriginEndIndex(urlText, mProfile); OmniboxUrlEmphasizer.getOriginEndIndex only works with ...
4 years ago (2015-04-15 05:00:41 UTC) #4
tsergeant
Thanks, good pickup. https://codereview.chromium.org/1077483002/diff/40001/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/1077483002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode135 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:135: int originEndIndex = OmniboxUrlEmphasizer.getOriginEndIndex(urlText, mProfile); On ...
4 years ago (2015-04-15 05:47:20 UTC) #5
juhonurm
https://codereview.chromium.org/1077483002/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/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode150 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:150: fragmentStartIndex = urlText.length(); Now the original issue is ignored ...
4 years ago (2015-04-15 06:07:58 UTC) #6
tsergeant
/facepalm Thanks, actually fixed this time. https://codereview.chromium.org/1077483002/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/1077483002/diff/60001/chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java#newcode150 chrome/android/java/src/org/chromium/chrome/browser/WebsiteSettingsPopup.java:150: fragmentStartIndex = urlText.length(); ...
4 years ago (2015-04-15 07:13:54 UTC) #7
juhonurm
lgtm
4 years ago (2015-04-15 13:26:08 UTC) #8
David Trainor- moved to gerrit
lgtm
4 years ago (2015-04-15 16:30:04 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1077483002/80001
4 years ago (2015-04-16 00:12:37 UTC) #11
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years ago (2015-04-16 00:36:30 UTC) #12
commit-bot: I haz the power
4 years ago (2015-04-16 00:37:21 UTC) #13
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/5bf8a2dccf4777c5f065d918d1d9a2bbaa013f9f
Cr-Commit-Position: refs/heads/master@{#325358}

Powered by Google App Engine
This is Rietveld 408576698