Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(756)

Issue 2182633007: Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost. (Closed)

Created:
4 years, 4 months ago by ananta
Modified:
4 years, 4 months ago
Reviewers:
Charlie Reis, jam
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, ncarter (slow), nasko
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid using ContentBrowserClient::IsIllegalOrigin in ResourceDispatcherHost. The approach taken here is to register the schemes, origins and the process ids which are allowed to access these origins in the ResourceDispatcherHost class. The ResourceDispatcherHost class now provides the following functionality. 1. Register a URL scheme for access check. Please see the AddSchemeForAccessCheck method. 2. Register and unregister a URL origin for access checks. Please see the RegisterOriginForAccessChecks and UnregisterOriginForAccessChecks methods. 3. Register and unregister process ids which are allowed to access or commit the URL. Please see the AddProcessforOrigin and RemoveProcessForOrigin methods. 4. Functionality to check if a URL origin is allowed. This is provided via a new method in the ResourceDispatcherHostImpl class called IsIlegalOrigin(). The other changes in this patchset are to register the extension URL scheme for access check and to push all extension URLs to ResourceDispatcherHost along with information whether they have publicly accessible resources. BUG=598073

Patch Set 1 #

Patch Set 2 : git cl format #

Patch Set 3 : The access information for origins is now keyed off the url host. #

Patch Set 4 : Update comments #

Patch Set 5 : Pass the proper extension urls when adding owners/guests #

Patch Set 6 : Fix compile failures #

Patch Set 7 : Fix compile failures #

Total comments: 25

Patch Set 8 : Address review comments #

Patch Set 9 : Remove includes #

Patch Set 10 : Rebased to tip #

Patch Set 11 : Merge ResourceDispatcherHostImpl correctly to tip #

Patch Set 12 : Fix compile errors #

Total comments: 18

Patch Set 13 : Address jam and creis comments and bring back methods to register and unregister an origin for acce… #

Patch Set 14 : Address other comments and update the RDH::IsIllegalOrigin function to deny origins not found in th… #

Patch Set 15 : Register all extensions via the RDH::RegisterOriginForAccessChecks method along with information wh… #

Patch Set 16 : Revert changes to extension_system_impl.h #

Patch Set 17 : Define an enum which controls origin access checks in RDH and pass it in the RegisterSchemeForAcces… #

Patch Set 18 : Revert changes to base_paths #

Patch Set 19 : Fix the ResourceDispatcherHost enum #

Patch Set 20 : Fix compile errors #

Patch Set 21 : Remove the IsIllegalOrigin function from ContentBrowserClient #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+546 lines, -116 lines) Patch
M chrome/browser/browser_process_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -12 lines 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +60 lines, -52 lines 0 comments Download
M chrome/browser/extensions/extension_system_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +46 lines, -3 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +82 lines, -1 line 2 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 6 chunks +210 lines, -4 lines 0 comments Download
M content/browser/security_exploit_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +14 lines, -13 lines 0 comments Download
M content/public/browser/content_browser_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -8 lines 0 comments Download
M content/public/browser/content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -6 lines 0 comments Download
M content/public/browser/resource_dispatcher_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +50 lines, -0 lines 4 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +16 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +58 lines, -11 lines 0 comments Download
M extensions/shell/test/shell_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 49 (37 generated)
ananta
4 years, 4 months ago (2016-07-29 23:38:40 UTC) #3
jam
https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc#newcode1158 chrome/browser/chrome_content_browser_client.cc:1158: bool ChromeContentBrowserClient::IsIllegalOrigin( remove this method here and in the ...
4 years, 4 months ago (2016-08-01 20:06:02 UTC) #10
ananta
https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc#newcode1158 chrome/browser/chrome_content_browser_client.cc:1158: bool ChromeContentBrowserClient::IsIllegalOrigin( On 2016/08/01 20:06:01, jam wrote: > remove ...
4 years, 4 months ago (2016-08-02 00:40:50 UTC) #11
jam
lgtm with comments https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2182633007/diff/120001/chrome/browser/chrome_content_browser_client.cc#newcode1158 chrome/browser/chrome_content_browser_client.cc:1158: bool ChromeContentBrowserClient::IsIllegalOrigin( On 2016/08/02 00:40:49, ananta ...
4 years, 4 months ago (2016-08-02 17:01:46 UTC) #24
Charlie Reis
I'd like to review as well, if that's ok. I'm not sure I understand yet ...
4 years, 4 months ago (2016-08-02 20:41:42 UTC) #26
ananta
https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc (left): https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#oldcode292 chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:292: return true; On 2016/08/02 20:41:42, Charlie Reis wrote: > ...
4 years, 4 months ago (2016-08-02 22:28:28 UTC) #27
ananta
On 2016/08/02 22:28:28, ananta wrote: > https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc > File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc > (left): > > https://codereview.chromium.org/2182633007/diff/220001/chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc#oldcode292 ...
4 years, 4 months ago (2016-08-02 22:29:24 UTC) #28
jam
On 2016/08/02 22:29:24, ananta wrote: > I brought back the functionality to register and unregister ...
4 years, 4 months ago (2016-08-02 23:43:58 UTC) #32
ananta
We now have a green try run. Changes made since the last iteration include registering ...
4 years, 4 months ago (2016-08-03 23:35:23 UTC) #46
Charlie Reis
Sorry for the delay-- I'm back from the offsite. Thanks for working through that. I ...
4 years, 4 months ago (2016-08-09 02:07:48 UTC) #47
ananta
On 2016/08/09 02:07:48, Charlie Reis wrote: > Sorry for the delay-- I'm back from the ...
4 years, 4 months ago (2016-08-10 01:06:58 UTC) #48
Charlie Reis
4 years, 4 months ago (2016-08-10 20:46:38 UTC) #49
On 2016/08/10 01:06:58, ananta wrote:
> We are abandoning this patch and trying out another approach which adds an
> interceptor to resource dispatcher host. That patch
> is here https://codereview.chromium.org/2222723002/

Great-- I like the way that one is shaping up.

Powered by Google App Engine
This is Rietveld 408576698