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

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

Created:
1 year, 3 months ago by elawrence
Modified:
1 year, 1 month 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?
1 year, 2 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 ...
1 year, 2 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 ...
1 year, 2 months 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 ...
1 year, 2 months ago (2017-02-06 22:42:28 UTC) #5
elawrence
> Since we seem to be duplicating code on views and Mac, consider hoisting much ...
1 year, 1 month 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 ...
1 year, 1 month ago (2017-02-23 01:30:43 UTC) #12
elawrence
> Did you look into this suggestion? I'm really hoping to avoid the duplication > ...
1 year, 1 month 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 ...
1 year, 1 month 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/ ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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" ...
1 year, 1 month 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 ...
1 year, 1 month 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: > ...
1 year, 1 month 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: ...
1 year, 1 month 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 ...
1 year, 1 month 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? ...
1 year, 1 month 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?
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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 ...
1 year, 1 month 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: ...
1 year, 1 month ago (2017-03-03 16:07:42 UTC) #71
Robert Sesek
cocoa/ and ui/gfx/range/ LGTM
1 year, 1 month 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 ...
1 year, 1 month 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]); ...
1 year, 1 month 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
1 year, 1 month ago (2017-03-03 22:02:40 UTC) #80
commit-bot: I haz the power
1 year, 1 month 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 408576698