Non-web-accessible extension URLs should not load in non-extension processes

Non-web-accessible extension URLs should not load in non-extension processes

This is a slightly modified version of my previous CL: https://codereview.chromium.org/12218064/.
The only difference is that we allow any resource request to succeed, if the extension has any web_acessible_resources. The reason for that we have been lax and allowed subresource loads, even if they are not explicitly added to the manifest (see crbug.com/179127 for details). This should be tightened up with a v3 manifest requirement to explicitly list all subresources.

BUG=173688
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192121

[nasko, 2013-03-26 17:50:22]

Hey guys,
This is another attempt to restrict extension resource loads. It is almost
identical to my previous CL, with one change.
Thanks,
Nasko

[Matt Perry, 2013-03-27 23:53:00]

https://codereview.chromium.org/12457042/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12457042/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_protocols.cc:334: if
(extensions::WebAccessibleResourcesInfo::HasWebAccessibleResources(
This will break extensions written before web_accessible_resources. I think
web_accessible_resources was introduced in manifest v2. We should also return
true here if the extension is on manifest v1.

[jochen (gone - plz use gerrit), 2013-04-02 15:14:25]

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:21: #include
"webkit/glue/glue_serialize.h"
not needed?

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:23: namespace content {
no namespace content in chrome/

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:33:
ChromeSecurityExploitBrowserTest() {}
nit. virtual destructor

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:55: };
nit. disallow copy/assign

[nasko, 2013-04-02 18:25:16]

Fixed all the issues pointed out in review.

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
File chrome/browser/chrome_security_exploit_browsertest.cc (right):

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:21: #include
"webkit/glue/glue_serialize.h"
On 2013/04/02 15:14:25, jochen wrote:
> not needed?

Done.

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:23: namespace content {
On 2013/04/02 15:14:25, jochen wrote:
> no namespace content in chrome/

Done.

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:33:
ChromeSecurityExploitBrowserTest() {}
On 2013/04/02 15:14:25, jochen wrote:
> nit. virtual destructor

Done.

https://codereview.chromium.org/12457042/diff/1/chrome/browser/chrome_securit...
chrome/browser/chrome_security_exploit_browsertest.cc:55: };
On 2013/04/02 15:14:25, jochen wrote:
> nit. disallow copy/assign

Done.

https://codereview.chromium.org/12457042/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12457042/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_protocols.cc:334: if
(extensions::WebAccessibleResourcesInfo::HasWebAccessibleResources(
On 2013/03/27 23:53:00, Matt Perry wrote:
> This will break extensions written before web_accessible_resources. I think
> web_accessible_resources was introduced in manifest v2. We should also return
> true here if the extension is on manifest v1.

Done.

[Matt Perry, 2013-04-02 18:31:32]

lgtm

[Charlie Reis, 2013-04-02 22:17:38]

Great. LGTM with nits.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:301: // process to request each
other's resources. We can't do more precise check,
nit: do a more

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:302: // since the renderer can
lie which extension has made the request.
nit: can lie about

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:331: // Extensions with
web_accessible_resources, allow loading by regular
nit: comma should be a colon.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:343: // checking if the
extension contains DevTools page.
nit: contains a DevTools page.
(or is it "the DevTools page"?)

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:344: if
(extensions::ManifestURL::GetDevToolsPage(extension).is_empty()) {
nit: no braces.

https://codereview.chromium.org/12457042/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/12457042/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/resource_request_policy.cc:31: //
extension_protocols.cc AllowExtensionResourceLoad.
nit: extension_protocols.cc's

[jochen (gone - plz use gerrit), 2013-04-03 07:19:39]

lgtm

[nasko, 2013-04-03 15:51:50]

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_protocols.cc (right):

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:301: // process to request each
other's resources. We can't do more precise check,
On 2013/04/02 22:17:38, creis wrote:
> nit: do a more

Done.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:302: // since the renderer can
lie which extension has made the request.
On 2013/04/02 22:17:38, creis wrote:
> nit: can lie about

Done.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:331: // Extensions with
web_accessible_resources, allow loading by regular
On 2013/04/02 22:17:38, creis wrote:
> nit: comma should be a colon.

Done.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:343: // checking if the
extension contains DevTools page.
On 2013/04/02 22:17:38, creis wrote:
> nit: contains a DevTools page.
> (or is it "the DevTools page"?)

Done.

https://codereview.chromium.org/12457042/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/extension_protocols.cc:344: if
(extensions::ManifestURL::GetDevToolsPage(extension).is_empty()) {
On 2013/04/02 22:17:38, creis wrote:
> nit: no braces.

Done.

https://codereview.chromium.org/12457042/diff/7001/chrome/renderer/extensions...
File chrome/renderer/extensions/resource_request_policy.cc (right):

https://codereview.chromium.org/12457042/diff/7001/chrome/renderer/extensions...
chrome/renderer/extensions/resource_request_policy.cc:31: //
extension_protocols.cc AllowExtensionResourceLoad.
On 2013/04/02 22:17:38, creis wrote:
> nit: extension_protocols.cc's

Done.

[commit-bot: I haz the power, 2013-04-03 15:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/12457042/18001

[commit-bot: I haz the power, 2013-04-03 19:50:50]

Change committed as 192121