Extension resources should only load in contexts the extension has permission to access.

Extension resources should only load in contexts the extension has permission to access.

See http://codereview.chromium.org/10792008/ for background.

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

[Mike West, 2012-07-30 14:03:38]

Hello Aaron and Matt!

Since you two were involved in the original pass at this functionality back in
early 2011 (see the revert here: http://codereview.chromium.org/6478019), I
figured you'd be thrilled to review another round.

This CL is mostly brought to you through the magic of copy and paste; I have no
idea what I'm doing this deep in the extension system, so feel more than free to
rip this CL up and point me in a different direction. I'm happy to make whatever
changes are necessary. :)

Thanks!

-mike

[Aaron Boodman, 2012-07-30 14:28:26]

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_resource_request_policy.cc:60:
extension->IsResourceWebAccessible(resource_url.path()) ||
The parenthetical comment about manifest v2+ is handled internally by
IsResourceWebAccessible, so you could simplify this:

The resource is not web accessible (and this check isn't disabled...)

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_resource_request_policy.cc:69:
extension->GetEffectiveHostPermissions().MatchesURL(page_url);
I don't think you should check page_url here.

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_resource_request_policy.cc:69:
extension->GetEffectiveHostPermissions().MatchesURL(page_url);
Can you test whether this seems to work with declared content scripts? That was
why it looks like we reverted the original CL.

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_resource_request_policy.cc:78: bool
is_own_resource = frame_url.GetOrigin() == extension->url() ||
This looks wrong, but you didn't add it. Imagine the following:

root: a.com
frame: b.com
img: a.com

This code will decide that the request for a.com is_own_resource (because of of
the page_url check), but that doesn't seem right.

https://chromiumcodereview.appspot.com/10828067/diff/1/chrome/renderer/extens...
chrome/renderer/extensions/extension_resource_request_policy.cc:91:
page_url.SchemeIs(chrome::kDataScheme);
Again, don't think we should be checking page_url. Elsewhere in this file too.

[Mike West, 2012-07-31 09:25:06]

Running the `about:blank` workaround through the trybots while I fiddle with
content_script tests.

http://codereview.chromium.org/10828067/diff/1/chrome/renderer/extensions/ext...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10828067/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:60:
extension->IsResourceWebAccessible(resource_url.path()) ||
On 2012/07/30 14:28:26, Aaron Boodman wrote:
> The parenthetical comment about manifest v2+ is handled internally by
> IsResourceWebAccessible, so you could simplify this:
> 
> The resource is not web accessible (and this check isn't disabled...)

Done.

http://codereview.chromium.org/10828067/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:69:
extension->GetEffectiveHostPermissions().MatchesURL(page_url);
On 2012/07/30 14:28:26, Aaron Boodman wrote:
> Can you test whether this seems to work with declared content scripts? That
was
> why it looks like we reverted the original CL.

I think this is covered by the existing content_script API tests. It's probably
a good idea to have an explicit test here as well, though.

http://codereview.chromium.org/10828067/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:78: bool
is_own_resource = frame_url.GetOrigin() == extension->url() ||
On 2012/07/30 14:28:26, Aaron Boodman wrote:
> This looks wrong, but you didn't add it. Imagine the following:
> 
> root: http://a.com
> frame: http://b.com
> img: http://a.com
> 
> This code will decide that the request for http://a.com is_own_resource
(because of of
> the page_url check), but that doesn't seem right.

As discussed, I've changed this logic to the following:

If `frame_url` is `about:blank`, and `frame` has a `parent()`, then we use the
parent instead of the frame for all the checks. This covers the case in which an
`iframe` (which starts it's life as `about:blank`) loads an extension resource.
In that case, we want to check that it's containing frame could load the
extension resource.

http://codereview.chromium.org/10828067/diff/1/chrome/renderer/extensions/ext...
chrome/renderer/extensions/extension_resource_request_policy.cc:91:
page_url.SchemeIs(chrome::kDataScheme);
On 2012/07/30 14:28:26, Aaron Boodman wrote:
> Again, don't think we should be checking page_url. Elsewhere in this file too.

Done.

[Aaron Boodman, 2012-07-31 09:50:10]

cool

http://codereview.chromium.org/10828067/diff/4001/chrome/renderer/extensions/...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10828067/diff/4001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:52: // Loading a
resource into an empty iframe should check against the iframe's
How about:

// In the case of loading a frame, frame* points to the frame 
// being loaded, not the containing frame. This means that
// frame->document().url() ends up not being useful to us.
//
// WebKit doesn't currently pass us enough information to
// know when we're a frame, so we hack it by checking for
// 'about:blank', which should only happen in this 
// situation.
//
// TODO(aa): Fix WebKit to pass the context of the load.
// crbug.com/xxxx

http://codereview.chromium.org/10828067/diff/4001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:72: bool
is_access_permitted =
is_access_permitted is too general to be meaningful in this function (the whole
function is about access being permitted).

How about extension_has_access_to_frame.

http://codereview.chromium.org/10828067/diff/4001/chrome/renderer/extensions/...
chrome/renderer/extensions/extension_resource_request_policy.cc:97: if
(!is_empty_origin && !is_own_resource && !is_dev_tools) {
I think the commentary makes this harder to read. With some variable renaming, I
think it can stand with far fewer comments.

bool is_resource_web_accessible = ...
bool extension_has_access_to_frame = ...
bool frame_has_empty_origin = ...
bool is_own_resource = ...
bool frame_is_devtools = ...
bool frame_is_data_url = ...
bool frame_is_extension = ...

if (!frame_has_empty_origin && !is_own_resource && !frame_is_devtools) {
  if (!is_resource_web_accessible) {
    // ... error message
    return false;
  }

  if (!extension_has_access_to_frame && !frame_is_extension &&
!frame_is_data_url) {
    // ... error
    return false;
  }
}

[Matt Perry, 2012-07-31 12:11:35]

I'll defer to Aaron here.

[Mike West, 2012-07-31 14:04:02]

On 2012/07/31 09:50:10, Aaron Boodman wrote:
> cool

I've made these changes, and filed crbug.com/139788 to cover the context issue.

The good news is that the existing content_script tests cover declared injection
without permissions (see `api_tests/content_scripts/all_frames` for instance),
and they pass on the trybots.

The bad news is that this change apparently breaks a single nacl test, and those
don't seem possible to run locally. There's a nice buildbot script, however.
Fun.

So I'll debug via trybot for a bit before uploading another patch for review.

[Aaron Boodman, 2012-07-31 14:31:01]

If you can't get it today, I'll take over.

[Mike West, 2012-07-31 14:35:17]

On 2012/07/31 14:31:01, Aaron Boodman wrote:
> If you can't get it today, I'll take over.

Which "it"?

I won't get to the WebKit bit today. Or tomorrow, probably. I'm hoping you'll
grab it.

The NaCl bit is mine. I'll get it working, no worries.

[Aaron Boodman, 2012-07-31 14:56:27]

On 2012/07/31 14:35:17, Mike West (chromium) wrote:
> On 2012/07/31 14:31:01, Aaron Boodman wrote:
> > If you can't get it today, I'll take over.
> 
> Which "it"?

The NaCL it. I'm definitely doing the WebKit it.

[Mike West, 2012-08-01 08:09:28]

On 2012/07/31 14:56:27, Aaron Boodman wrote:
> On 2012/07/31 14:35:17, Mike West (chromium) wrote:
> > On 2012/07/31 14:31:01, Aaron Boodman wrote:
> > > If you can't get it today, I'll take over.
> > 
> > Which "it"?
> 
> The NaCL it. I'm definitely doing the WebKit it.

Let's talk about the NaCl it today. This change breaks NaCl's ability to handle
MIME types as a plugin given the way it's implemented. I'm hoping you have some
ideas for a workaround. :)

[Mike West, 2012-08-01 10:54:45]

On 2012/08/01 08:09:28, Mike West (chromium) wrote:
> On 2012/07/31 14:56:27, Aaron Boodman wrote:
> > On 2012/07/31 14:35:17, Mike West (chromium) wrote:
> > > On 2012/07/31 14:31:01, Aaron Boodman wrote:
> > > > If you can't get it today, I'll take over.
> > > 
> > > Which "it"?
> > 
> > The NaCL it. I'm definitely doing the WebKit it.
> 
> Let's talk about the NaCl it today. This change breaks NaCl's ability to
handle
> MIME types as a plugin given the way it's implemented. I'm hoping you have
some
> ideas for a workaround. :)

The workaround we talked about ("Does it look like a nexe?") is in this patch.
I've also limited it only to cases where the extension actually declares a NaCl
module, which seems reasonable.

Throwing it to the trybots now. WDYT?

[Aaron Boodman, 2012-08-01 21:56:54]

LGTM

woo

http://codereview.chromium.org/10828067/diff/12003/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10828067/diff/12003/chrome/common/extensions/e...
chrome/common/extensions/extension.h:358: bool IsResourceNaClModule(const
std::string& resource) const;
IsResourceNaclManifest()? To distinguish from actual nexes?

Same comment below.

http://codereview.chromium.org/10828067/diff/12003/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_resource_request_policy.cc (right):

http://codereview.chromium.org/10828067/diff/12003/chrome/renderer/extensions...
chrome/renderer/extensions/extension_resource_request_policy.cc:53: // In the
case of loading a frame, frame* points to the frame
Can you rewrap this comment to take advantage of 80 cols?

[Mike West, 2012-08-02 09:09:56]

On 2012/08/01 21:56:54, Aaron Boodman wrote:
> LGTM
> 
> woo

Woo, indeed. :)

Try failures are different on linux/mac. Looks like flake, and the important
tests passed, so I'll throw it into the queue. Assuming this makes it into M23,
we should probably remember to revert it on the beta branch. Given that the
feature was reverted once last year, I'd like it to bake a little in dev before
too many people see it.

[commit-bot: I haz the power, 2012-08-02 09:10:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10828067/16003

[commit-bot: I haz the power, 2012-08-02 09:10:30]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p1 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 3.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-02 09:23:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10828067/7005

[commit-bot: I haz the power, 2012-08-02 09:23:11]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p1 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 3.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[Mike West, 2012-08-02 09:25:09]

On 2012/08/02 09:23:11, I haz the power (commit-bot) wrote:
> Failed to apply patch for

Grrr. I just rebased this to ToT and nothing changed.

[Mike West, 2012-08-02 11:04:01]

On 2012/08/02 09:25:09, Mike West (chromium) wrote:
> On 2012/08/02 09:23:11, I haz the power (commit-bot) wrote:
> > Failed to apply patch for
> 
> Grrr. I just rebased this to ToT and nothing changed.

Ok, CQ gets one more chance, then I'm just landing this. :)

[commit-bot: I haz the power, 2012-08-02 11:04:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10828067/7005

[commit-bot: I haz the power, 2012-08-02 11:04:17]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p1 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 3.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[commit-bot: I haz the power, 2012-08-02 11:09:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/10828067/7006

[commit-bot: I haz the power, 2012-08-02 11:10:00]

Failed to apply patch for
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
While running patch -p1 --forward --force;
patching file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

[Mike West, 2012-08-02 11:11:03]

On 2012/08/02 11:10:00, I haz the power (commit-bot) wrote:
> Failed to apply patch for
>
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json:
> While running patch -p1 --forward --force;
> patching file
>
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json
> Hunk #1 FAILED at 1.
> 1 out of 1 hunk FAILED -- saving rejects to file
>
chrome/test/data/extensions/api_test/extension_resource_request_policy/web_accessible/manifest.json.rej

I hate it. Landing.