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

Issue 1180393003: Added characters that look like padlocks to URL unescaping blacklist. (Closed)

Created:
4 years, 5 months ago by Matt Giuca
Modified:
4 years, 4 months ago
Reviewers:
Peter Kasting, jam, mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org, 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

Added characters that look like padlocks to URL unescaping blacklist. This blacklists the following Unicode characters: - U+1F50F LOCK WITH INK PEN - U+1F510 CLOSED LOCK WITH KEY - U+1F512 LOCK - U+1F513 OPEN LOCK This prevents LOCK characters from appearing in a URL in the Chrome UI, potentially looking like an SSL padlock icon (e.g., "google.com/🔒" is now displayed as "google.com/%F0%9F%94%92"). This presented a spoofing risk due to a few complications: 1. In RTL mode, the end of the URL (path/query) is aligned right up against the right edge of the Omnibox, where the SSL padlock is usually displayed. 2. On Mac, ChromeOS, and Android, LOCK characters are displayed in colour, making them more convincing. Note: These characters will still be unescaped when using the SPOOFING_AND_CONTROL_CHARS unescape rule (used for decoding data URLs, previously known as CONTROL_CHARS). BUG=495934, 421332 TBR=jam@chromium.org Committed: https://crrev.com/7c2cbc445a81424c7df48ebe61ec4d0dcadd5dff Cr-Commit-Position: refs/heads/master@{#335870}

Patch Set 1 #

Total comments: 13

Patch Set 2 : Comment change. #

Patch Set 3 : Combine if statements. #

Total comments: 9

Patch Set 4 : Code and comment updates. #

Patch Set 5 : Rename to NON_DISPLAY_CHARS. #

Patch Set 6 : Rename NON_DISPLAY_CHARS to SPOOFING_AND_CONTROL_CHARS. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -46 lines) Patch
M chrome/browser/safe_browsing/safe_browsing_util.cc View 1 2 3 4 5 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/instant_types.cc View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M components/policy/core/common/cloud/device_management_service_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/web_contents/web_drag_source_mac.mm View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/web_request/form_data_parser.cc View 1 2 3 4 5 2 chunks +5 lines, -4 lines 0 comments Download
M net/base/data_url.cc View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M net/base/escape.h View 1 2 3 4 5 1 chunk +7 lines, -6 lines 0 comments Download
M net/base/escape.cc View 1 2 3 4 5 4 chunks +54 lines, -7 lines 0 comments Download
M net/base/escape_unittest.cc View 1 2 3 4 5 6 chunks +52 lines, -17 lines 0 comments Download
M storage/common/fileapi/file_system_util.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 31 (9 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180393003/1
4 years, 5 months ago (2015-06-16 02:07:44 UTC) #2
Matt Giuca
mmenke: OWNERS. pkasting: Because this change affects the Omnibox. Note: This is a follow-up to ...
4 years, 5 months ago (2015-06-16 02:52:24 UTC) #4
Matt Giuca
PS: You can see before/after screenshots at http://crbug.com/495934#c18
4 years, 5 months ago (2015-06-16 02:53:24 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 5 months ago (2015-06-16 03:10:55 UTC) #7
Peter Kasting
UNESCAPE_CONTROL_CHARS seems like a bit of a misnomer for unescaping these, but I see why ...
4 years, 5 months ago (2015-06-16 06:36:15 UTC) #8
Matt Giuca
> I wonder if we should name this UNESCAPE_DANGEROUS_CHARS or something. I'm happy to do ...
4 years, 5 months ago (2015-06-16 07:55:34 UTC) #9
Peter Kasting
Renaming the enum is up to you. DANGEROUS_CHARS is a bit vague and threatening about ...
4 years, 5 months ago (2015-06-16 08:05:08 UTC) #10
mmenke
Sorry, forgot about this one. Split a lot of ways this week. https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc File net/base/escape.cc ...
4 years, 5 months ago (2015-06-17 19:50:17 UTC) #11
Matt Giuca
https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc File net/base/escape.cc (right): https://codereview.chromium.org/1180393003/diff/1/net/base/escape.cc#newcode181 net/base/escape.cc:181: if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 3, &second_byte)) Done. (Not a ...
4 years, 4 months ago (2015-06-22 07:27:03 UTC) #12
Peter Kasting
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc File net/base/escape.cc (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newcode196 net/base/escape.cc:196: fourth_byte != 0x93)) { Nit: Simpler: return UnescapeUnsignedCharAtIndex(escaped_text, index ...
4 years, 4 months ago (2015-06-22 07:35:22 UTC) #13
mmenke
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc File net/base/escape.cc (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newcode257 net/base/escape.cc:257: // be used to imitate parts of the browser ...
4 years, 4 months ago (2015-06-22 17:05:08 UTC) #14
Matt Giuca
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc File net/base/escape.cc (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.cc#newcode196 net/base/escape.cc:196: fourth_byte != 0x93)) { On 2015/06/22 07:35:22, Peter Kasting ...
4 years, 4 months ago (2015-06-23 04:14:11 UTC) #16
Peter Kasting
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h File net/base/escape.h (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newcode98 net/base/escape.h:98: // characters (such as LOCK). On 2015/06/23 04:14:10, Matt ...
4 years, 4 months ago (2015-06-23 04:35:53 UTC) #17
Matt Giuca
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h File net/base/escape.h (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newcode98 net/base/escape.h:98: // characters (such as LOCK). On 2015/06/23 04:35:53, Peter ...
4 years, 4 months ago (2015-06-23 06:21:25 UTC) #18
Peter Kasting
https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h File net/base/escape.h (right): https://codereview.chromium.org/1180393003/diff/40001/net/base/escape.h#newcode98 net/base/escape.h:98: // characters (such as LOCK). On 2015/06/23 06:21:24, Matt ...
4 years, 4 months ago (2015-06-23 06:55:15 UTC) #19
mmenke
LGTM
4 years, 4 months ago (2015-06-23 15:22:55 UTC) #20
Matt Giuca
jam@chromium.org: TBR for mechanical changes (renaming constant).
4 years, 4 months ago (2015-06-24 00:57:23 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180393003/120001
4 years, 4 months ago (2015-06-24 00:59:57 UTC) #25
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/71328)
4 years, 4 months ago (2015-06-24 02:36:15 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1180393003/120001
4 years, 4 months ago (2015-06-24 03:19:10 UTC) #29
commit-bot: I haz the power
Committed patchset #6 (id:120001)
4 years, 4 months ago (2015-06-24 03:59:57 UTC) #30
commit-bot: I haz the power
4 years, 4 months ago (2015-06-24 04:00:43 UTC) #31
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/7c2cbc445a81424c7df48ebe61ec4d0dcadd5dff
Cr-Commit-Position: refs/heads/master@{#335870}

Powered by Google App Engine
This is Rietveld 408576698