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

Issue 2641003002: Show scheme in black and content in gray for data: protocol urls (Closed)

Created:
10 months, 1 week ago by elawrence
Modified:
8 months, 3 weeks ago
CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Show scheme in black and content in gray for data: protocol urls Data URIs can be used for spoofing the user because attacker controlled text appears near the front of the string, and thus a fake origin can be placed the URI such that the user believes it represents the true origin of the page. To mitigate this, we will render the data scheme in black and the rest of the data URI string in the deemphasized text color. BUG=533705 Review-Url: https://codereview.chromium.org/2641003002 Cr-Commit-Position: refs/heads/master@{#454699} Committed: https://chromium.googlesource.com/chromium/src/+/486bb18560662755558c628e39ed8b3a3de272a2

Patch Set 1 #

Patch Set 2 : Fix off-by-one #

Total comments: 3

Patch Set 3 : Address review feedback #

Total comments: 2

Patch Set 4 : Hoist deemphasis logic #

Total comments: 4

Patch Set 5 : Hoist yet more #

Patch Set 6 : Fixup DEPS #

Patch Set 7 : Rebase #

Patch Set 8 : Update iOS #

Total comments: 66

Patch Set 9 : Address reviewer feedback #

Patch Set 10 : Fix omnibox_edit_unittest #

Patch Set 11 : Only call UpdateSchemeStyle when a scheme is present. #

Total comments: 10

Patch Set 12 : Rebase #

Patch Set 13 : Address Nits #

Total comments: 2

Patch Set 14 : Move misplaced #include to mm files #

Unified diffs Side-by-side diffs Delta from patch set Stats (+384 lines, -167 lines) Patch
M chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +12 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +57 lines, -62 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +19 lines, -35 lines 0 comments Download
M chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +120 lines, -3 lines 0 comments Download
M components/omnibox/browser/BUILD.gn View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/omnibox/browser/DEPS View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_edit_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +23 lines, -0 lines 0 comments Download
M components/omnibox/browser/omnibox_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +61 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/omnibox/omnibox_view_ios.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +13 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +67 lines, -64 lines 0 comments Download
M ui/gfx/range/BUILD.gn View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 83 (50 generated)
Peter Kasting
Was this intended to be ready for review and just not sent?
10 months ago (2017-01-23 09:29:15 UTC) #1
elawrence
On 2017/01/23 09:29:15, Peter Kasting wrote: > Was this intended to be ready for review ...
10 months ago (2017-01-23 14:06:57 UTC) #2
elawrence
Please take a look? Design decided we should use the standard level of deemphasis. Mobile ...
9 months, 3 weeks ago (2017-02-03 22:37:55 UTC) #4
Peter Kasting
Commented only on views, but the Mac comments are similar. Since we seem to be ...
9 months, 2 weeks ago (2017-02-06 22:42:28 UTC) #5
elawrence
> Since we seem to be duplicating code on views and Mac, consider hoisting much ...
9 months ago (2017-02-21 22:29:09 UTC) #11
Peter Kasting
On 2017/02/06 22:42:28, Peter Kasting (backlogged) wrote: > Since we seem to be duplicating code ...
9 months ago (2017-02-23 01:30:43 UTC) #12
elawrence
> Did you look into this suggestion? I'm really hoping to avoid the duplication > ...
9 months ago (2017-02-23 18:41:42 UTC) #17
Peter Kasting
On 2017/02/23 18:41:42, elawrence wrote: > I suppose I could refactor EmphasizeURLComponents into a function ...
9 months ago (2017-02-24 01:53:40 UTC) #19
elawrence
> https://codereview.chromium.org/2641003002/diff/100001/components/omnibox/browser/omnibox_view.cc#newcode195 > components/omnibox/browser/omnibox_view.cc:195: // complicated dependencies. > Urgh. > Can we not have extensions/common/ ...
9 months ago (2017-02-24 19:35:58 UTC) #20
Peter Kasting
(If possible, please reply to review comments inline with the commenting tool rather than via ...
9 months ago (2017-02-24 20:34:57 UTC) #21
elawrence
pkasting@chromium.org: I've hoisted all of the scheme analysis logic to the OmniboxView and left the ...
8 months, 4 weeks ago (2017-02-28 22:46:53 UTC) #43
Eugene But
rohitrao@, who owns omnibox would be a better reviewer. I can comment on this CL ...
8 months, 4 weeks ago (2017-02-28 23:04:38 UTC) #45
Peter Kasting
Cool, I think this is almost ready to go. Most of the remaining comments are ...
8 months, 4 weeks ago (2017-03-01 02:39:11 UTC) #48
Eugene But
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.h File ios/chrome/browser/ui/omnibox/omnibox_view_ios.h (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.h#newcode144 ios/chrome/browser/ui/omnibox/omnibox_view_ios.h:144: // Update colors in |attributing_display_string_|. On 2017/03/01 02:39:11, Peter ...
8 months, 4 weeks ago (2017-03-01 06:08:08 UTC) #49
Robert Sesek
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h#newcode242 chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:242: NSMutableAttributedString* attributing_display_string_; Who owns this? Either document as "Weak" ...
8 months, 3 weeks ago (2017-03-01 18:29:45 UTC) #50
elawrence
Thanks for having a look! https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h#newcode187 chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.h:187: // Update colors in ...
8 months, 3 weeks ago (2017-03-01 21:49:02 UTC) #51
Eugene But
https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/370001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode660 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:660: (security_level == security_state::HTTP_SHOW_WARNING)) On 2017/03/01 21:49:02, elawrence wrote: > ...
8 months, 3 weeks ago (2017-03-01 22:31:19 UTC) #53
Peter Kasting
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc#newcode275 chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 21:49:01, elawrence wrote: ...
8 months, 3 weeks ago (2017-03-01 23:18:47 UTC) #55
elawrence
https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc File chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc (right): https://codereview.chromium.org/2641003002/diff/370001/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc#newcode275 chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc:275: // as a hostname. On 2017/03/01 23:18:47, Peter Kasting ...
8 months, 3 weeks ago (2017-03-01 23:50:53 UTC) #56
elawrence
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme? ...
8 months, 3 weeks ago (2017-03-02 19:08:01 UTC) #57
elawrence
rdevlin.cronin@chromium.org: Please approve addition of "+extensions/common/constants.h" to the DEPS so that I can use kExtensionScheme?
8 months, 3 weeks ago (2017-03-02 19:08:14 UTC) #59
Devlin
On 2017/03/02 19:08:14, elawrence wrote: > mailto:rdevlin.cronin@chromium.org: Please approve addition of > "+extensions/common/constants.h" to the ...
8 months, 3 weeks ago (2017-03-02 19:32:05 UTC) #62
elawrence
> I'm fine with this, but it's probably more a question for omnibox owners, aka ...
8 months, 3 weeks ago (2017-03-02 19:41:57 UTC) #63
Devlin
On 2017/03/02 19:41:57, elawrence wrote: > > I'm fine with this, but it's probably more ...
8 months, 3 weeks ago (2017-03-02 19:59:53 UTC) #64
Peter Kasting
LGTM on this. I'm hoping to find a quick and easy way of avoiding exposing ...
8 months, 3 weeks ago (2017-03-03 02:31:13 UTC) #67
Peter Kasting
Update: I have a working way of doing this "correctly"; it requires a couple precursor ...
8 months, 3 weeks ago (2017-03-03 09:17:14 UTC) #68
Peter Kasting
On 2017/03/03 09:17:14, Peter Kasting wrote: > Update: I have a working way of doing ...
8 months, 3 weeks ago (2017-03-03 09:18:41 UTC) #69
elawrence
Thanks for your feedback so far! rsesek@chromium.org: Please review the tiny change to ui/gfx/range/BUILD.gn. rohitrao@chromium.org: ...
8 months, 3 weeks ago (2017-03-03 16:07:42 UTC) #71
Robert Sesek
cocoa/ and ui/gfx/range/ LGTM
8 months, 3 weeks ago (2017-03-03 16:29:17 UTC) #72
rohitrao (ping after 24h)
lgtm https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode645 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); If the range is ...
8 months, 3 weeks ago (2017-03-03 21:53:51 UTC) #77
elawrence
Thanks for the reviews, all! https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm File ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm (right): https://codereview.chromium.org/2641003002/diff/510001/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm#newcode645 ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm:645: : NSMakeRange(0, [attributing_display_string_ length]); ...
8 months, 3 weeks ago (2017-03-03 22:01:59 UTC) #78
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2641003002/530001
8 months, 3 weeks ago (2017-03-03 22:02:40 UTC) #80
commit-bot: I haz the power
8 months, 3 weeks ago (2017-03-03 23:04:29 UTC) #83
Message was sent while issue was closed.
Committed patchset #14 (id:530001) as
https://chromium.googlesource.com/chromium/src/+/486bb18560662755558c628e39ed...

Powered by Google App Engine
This is Rietveld efc10ee0f