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

Issue 1658913002: Make extensions use a correct same-origin check. (Closed)

Created:
3 years, 10 months ago by palmer
Modified:
3 years, 10 months ago
Reviewers:
Devlin, benwells, meacer, brettw
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make extensions use a correct same-origin check. GURL::GetOrigin does not do the right thing for all types of URLs. BUG=573317 Committed: https://crrev.com/5c437bcc7a51edbef45242c5173cf7871fde2866 Cr-Commit-Position: refs/heads/master@{#373381}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Create and use a convenience function; De Morgan's Lawyer makes a compelling argument #

Patch Set 3 : Don't break the tests — we shouldn't have been adding null origins anyway. #

Total comments: 4

Patch Set 4 : Respond to comments. #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+31 lines, -18 lines) Patch
M extensions/browser/api/web_request/web_request_permissions.cc View 1 2 chunks +4 lines, -3 lines 0 comments Download
M extensions/browser/guest_view/extension_options/extension_options_guest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/guest_view/extension_view/extension_view_guest.cc View 1 3 chunks +4 lines, -3 lines 0 comments Download
M extensions/common/url_pattern_set.cc View 1 2 3 2 chunks +6 lines, -3 lines 3 comments Download
M extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc View 1 2 chunks +3 lines, -2 lines 0 comments Download
M extensions/renderer/file_system_natives.cc View 1 2 3 3 chunks +4 lines, -3 lines 0 comments Download
M extensions/renderer/programmatic_script_injector.cc View 2 chunks +2 lines, -1 line 0 comments Download
M url/origin.h View 1 1 chunk +3 lines, -2 lines 0 comments Download
M url/origin.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 32 (10 generated)
palmer
meacer: Any thoughts on whether I'm on the right track?
3 years, 10 months ago (2016-02-02 00:54:20 UTC) #3
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658913002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658913002/1
3 years, 10 months ago (2016-02-02 00:55:28 UTC) #4
meacer
I'm sure there is a good reason for this, but I'm wondering why there isn't ...
3 years, 10 months ago (2016-02-02 01:35:08 UTC) #5
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/173535)
3 years, 10 months ago (2016-02-02 01:41:55 UTC) #7
palmer
https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_request/web_request_permissions.cc File extensions/browser/api/web_request/web_request_permissions.cc (right): https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_request/web_request_permissions.cc#newcode135 extensions/browser/api/web_request/web_request_permissions.cc:135: url::Origin(extension->url()))))) { On 2016/02/02 01:35:08, Mustafa Emre Acer wrote: ...
3 years, 10 months ago (2016-02-02 23:12:38 UTC) #8
palmer
> I'm sure there is a good reason for this, but I'm wondering why there ...
3 years, 10 months ago (2016-02-02 23:13:49 UTC) #9
meacer
On 2016/02/02 23:13:49, palmer wrote: > > I'm sure there is a good reason for ...
3 years, 10 months ago (2016-02-02 23:27:00 UTC) #10
meacer
https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_request/web_request_permissions.cc File extensions/browser/api/web_request/web_request_permissions.cc (right): https://codereview.chromium.org/1658913002/diff/1/extensions/browser/api/web_request/web_request_permissions.cc#newcode135 extensions/browser/api/web_request/web_request_permissions.cc:135: url::Origin(extension->url()))))) { On 2016/02/02 23:12:38, palmer wrote: > On ...
3 years, 10 months ago (2016-02-02 23:27:22 UTC) #11
palmer
brettw: I added a simple convenience function to url::Origin which makes the call sites much ...
3 years, 10 months ago (2016-02-03 00:17:12 UTC) #13
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658913002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658913002/40001
3 years, 10 months ago (2016-02-03 00:23:27 UTC) #15
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
3 years, 10 months ago (2016-02-03 02:24:30 UTC) #17
brettw
lgtm https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_pattern_set.cc File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_pattern_set.cc#newcode158 extensions/common/url_pattern_set.cc:158: const url::Origin real_origin = url::Origin(origin); This would be ...
3 years, 10 months ago (2016-02-03 19:12:38 UTC) #18
meacer
Lgtm (not an owner for any of these files)
3 years, 10 months ago (2016-02-03 20:34:40 UTC) #19
palmer
https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_pattern_set.cc File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/1658913002/diff/40001/extensions/common/url_pattern_set.cc#newcode158 extensions/common/url_pattern_set.cc:158: const url::Origin real_origin = url::Origin(origin); On 2016/02/03 19:12:38, brettw ...
3 years, 10 months ago (2016-02-03 20:47:58 UTC) #20
palmer
benwells and/or rdevlin.cronin: Could you please take an OWNERS look at extensions/? Thanks!
3 years, 10 months ago (2016-02-03 20:50:13 UTC) #22
Devlin
extensions lgtm https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc#newcode159 extensions/common/url_pattern_set.cc:159: DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin()))); Can we not use the static ...
3 years, 10 months ago (2016-02-03 21:01:25 UTC) #23
palmer
https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc#newcode159 extensions/common/url_pattern_set.cc:159: DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin()))); On 2016/02/03 21:01:25, Devlin wrote: > Can we ...
3 years, 10 months ago (2016-02-03 21:03:08 UTC) #24
Devlin
https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc File extensions/common/url_pattern_set.cc (right): https://codereview.chromium.org/1658913002/diff/60001/extensions/common/url_pattern_set.cc#newcode159 extensions/common/url_pattern_set.cc:159: DCHECK(real_origin.IsSameOriginWith(url::Origin(origin.GetOrigin()))); On 2016/02/03 21:03:08, palmer wrote: > On 2016/02/03 ...
3 years, 10 months ago (2016-02-03 21:10:11 UTC) #25
palmer
> > No, I actually want to maintain the check on GURL::GetOrigin. > > I'm ...
3 years, 10 months ago (2016-02-03 21:14:35 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1658913002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1658913002/60001
3 years, 10 months ago (2016-02-03 21:48:26 UTC) #29
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years, 10 months ago (2016-02-03 23:21:45 UTC) #30
commit-bot: I haz the power
3 years, 10 months ago (2016-02-03 23:22:42 UTC) #32
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/5c437bcc7a51edbef45242c5173cf7871fde2866
Cr-Commit-Position: refs/heads/master@{#373381}

Powered by Google App Engine
This is Rietveld 408576698