Use WebFrame::setIsolatedWorldSecurityOrigin to allow cross-origin XHRs in content scripts.

Use WebFrame::setIsolatedWorldSecurityOrigin to allow cross-origin XHRs in content scripts.

http://trac.webkit.org/changeset/87423 adds the ability to associated a security origin with
an isolated world. Use that to make content script isolated worlds use the extension's
origin, and whitelist that origin in the page's renderer process for the host permissions
in the manifest.

BUG=18857
TEST=no
R=mpcomplete@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87114

[Matt Perry, 2011-05-26 22:39:48]

http://codereview.chromium.org/7071025/diff/1001/chrome/renderer/extensions/u...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7071025/diff/1001/chrome/renderer/extensions/u...
chrome/renderer/extensions/user_script_slave.cc:78:
WebSecurityPolicy::addOriginAccessWhitelistEntry(
note that we never clear the origin access whitelist. if an extension is
reloaded, existing renderer processes will have the old whitelists. this is a
minor point, so I think it's fine to add a TODO for now.

[Mihai Parparita -not on Chrome, 2011-05-27 02:03:00]

On 2011/05/26 22:39:48, Matt Perry wrote:
> chrome/renderer/extensions/user_script_slave.cc:78:
> WebSecurityPolicy::addOriginAccessWhitelistEntry(
> note that we never clear the origin access whitelist. if an extension is
> reloaded, existing renderer processes will have the old whitelists. this is a
> minor point, so I think it's fine to add a TODO for now.

As discussed in person, this is a bit more serious, since on every page load* we
were actually adding one whitelist entry per extension with the frame's URL, and
one or more with each host permission origin. This is (partly) because
WebSecurityPolicy::addOriginAccessWhitelistEntry just appends to a vector, so we
can keep adding the same entries over and over again.

I changed things around to keep track of what origin we whitelisted for a frame,
so we can unregister it when it gets navigated. I also keep track of what host
permission have already been whitelisted, and don't whitelist them again.

* actually, up to 3 times every page load, since UserScriptSlave::InjectScripts
gets called for each run location

[Matt Perry, 2011-05-27 19:10:54]

http://codereview.chromium.org/7071025/diff/4001/chrome/renderer/extensions/u...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7071025/diff/4001/chrome/renderer/extensions/u...
chrome/renderer/extensions/user_script_slave.cc:79: void
UserScriptSlave::InitializeIsolatedWorld(
This seems like a lot of effort to work around deficiencies in the WebKit API.
None of this would be necessary with the following changes to WebKit:
- change the origin access whitelist to hold a set instead of a vector, so that
we don't have to keep the extension_origins_ map.
- add WebSecurityPolicy::removeOriginAccessWhitelistEntriesForOrigin(url).

Are these feasible?

I'm still of the opinion that we should just whitelist the union of
host_permissions and content_script.urls. This would make the logic even
simpler, because we would only need to initialize the isolated world when we
create it, rather than with each page navigation.

[Mihai Parparita -not on Chrome, 2011-05-27 20:44:34]

On Fri, May 27, 2011 at 12:10 PM,  <mpcomplete@chromium.org> wrote:
> I'm still of the opinion that we should just whitelist the union of
> host_permissions and content_script.urls. This would make the logic even
> simpler, because we would only need to initialize the isolated world
> when we create it, rather than with each page navigation.

OK, that does seem simpler, switched to that.

The only thing that this doesn't handle is an extension getting
reloaded with additional host permissions. Should I have
UserScriptSlave listen for EXTENSION_UNLOADED and clear that
extension's entry in the isolated world ID map?

Mihai

[Matt Perry, 2011-05-27 20:55:28]

> The only thing that this doesn't handle is an extension getting
> reloaded with additional host permissions. Should I have
> UserScriptSlave listen for EXTENSION_UNLOADED and clear that
> extension's entry in the isolated world ID map?

EXTENSION_UNLOADED isn't fired in the renderer process. The renderer equivalent
is ExtensionDispatcher::OnUnloaded. That seems like a good place to hook into.

http://codereview.chromium.org/7071025/diff/7002/chrome/renderer/extensions/u...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7071025/diff/7002/chrome/renderer/extensions/u...
chrome/renderer/extensions/user_script_slave.cc:58:
frame->setIsolatedWorldSecurityOrigin(
Is this necessary? If it's already in the map, this should have been set
already.

http://codereview.chromium.org/7071025/diff/7002/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/manifest.json
(right):

http://codereview.chromium.org/7071025/diff/7002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/manifest.json:8:
"matches": [ "<all_urls>" ]
I think you need to change this to localhost only.

[Mihai Parparita -not on Chrome, 2011-05-27 22:00:36]

On Fri, May 27, 2011 at 1:55 PM,  <mpcomplete@chromium.org> wrote:
> EXTENSION_UNLOADED isn't fired in the renderer process. The renderer
> equivalent
> is ExtensionDispatcher::OnUnloaded. That seems like a good place to hook
> into.

Thanks, that works.

>
http://codereview.chromium.org/7071025/diff/7002/chrome/renderer/extensions/u...
> chrome/renderer/extensions/user_script_slave.cc:58:
> frame->setIsolatedWorldSecurityOrigin(
> Is this necessary? If it's already in the map, this should have been set
> already.

Yeah, the isolated world -> origin map is per WebFrame, so we can't
just set it once. Added a comment.

>
chrome/test/data/extensions/api_test/cross_origin_xhr/content_script/manifest.json:8:
> "matches": [ "<all_urls>" ]
> I think you need to change this to localhost only.

Thanks, fixed. I realized that GetEffectiveHostPermissions didn't
actually return the user script match patterns since we didn't have
that data in the copy of Extension in the renderer process, so I had
to add that key to kRendererExtensionKeys.

Mihai

[Matt Perry, 2011-05-27 22:14:35]

Cool, LGTM.

Now that content scripts can XHR to content_script hosts, we should change it so
that extension processes can as well. I'll file a bug.

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/e...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/e...
chrome/renderer/extensions/extension_dispatcher.cc:156:
UserScriptSlave::RemoveIsolatedWorld(id);
Could you add a comment here explaining that we do this so that it resets the
origin whitelist for this extension, in case it's reloaded?

[Aaron Boodman, 2011-05-29 04:44:09]

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
chrome/renderer/extensions/user_script_slave.cc:85:
extension->GetEffectiveHostPermissions().patterns();
Sorry for butting in, but it seems unexpected to me that content_scripts.urls
has the side-effect of allowing cross-origin XHR. Why is that easier to
implement? Couldn't this line just check host_permissions() instead?

[Matt Perry, 2011-05-31 18:13:32]

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
chrome/renderer/extensions/user_script_slave.cc:85:
extension->GetEffectiveHostPermissions().patterns();
On 2011/05/29 04:44:09, Aaron Boodman wrote:
> Sorry for butting in, but it seems unexpected to me that content_scripts.urls
> has the side-effect of allowing cross-origin XHR. Why is that easier to
> implement? Couldn't this line just check host_permissions() instead?

I advocated doing this for 2 main reasons:
1. From a security perspective, being able to inject script is the same as being
able to issue cross-origin XHR to a host.
2. We have to allow XHR to the origin of the content script's frame. Since the
extension's isolated world has a shared security context, this means it really
has cross-origin access to (host_permissions + all frame origins that this
extension has a content script running on). Given that oddity, it seemed more
reasonable to just allow access to all content_script.urls at all times.

Erik, Mihai, and I were trying to remember why you didn't want cross-origin XHR
to content script URLs. Could you explain the reasoning? It seems to me that
requesting access to a host should be universal, regardless of where you request
it.

[Aaron Boodman, 2011-05-31 23:50:16]

On Tue, May 31, 2011 at 12:13 PM,  <mpcomplete@chromium.org> wrote:
>
>
http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
> File chrome/renderer/extensions/user_script_slave.cc (right):
>
>
http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
> chrome/renderer/extensions/user_script_slave.cc:85:
> extension->GetEffectiveHostPermissions().patterns();
> On 2011/05/29 04:44:09, Aaron Boodman wrote:
>>
>> Sorry for butting in, but it seems unexpected to me that
>
> content_scripts.urls
>>
>> has the side-effect of allowing cross-origin XHR. Why is that easier
>
> to
>>
>> implement? Couldn't this line just check host_permissions() instead?
>
> I advocated doing this for 2 main reasons:
> 1. From a security perspective, being able to inject script is the same
> as being able to issue cross-origin XHR to a host.

Agree.

> 2. We have to allow XHR to the origin of the content script's frame.
> Since the extension's isolated world has a shared security context, this
> means it really has cross-origin access to (host_permissions + all frame
> origins that this extension has a content script running on).

Agree.

> Erik, Mihai, and I were trying to remember why you didn't want
> cross-origin XHR to content script URLs. Could you explain the
> reasoning? It seems to me that requesting access to a host should be
> universal, regardless of where you request it.

I just don't see declaring content scripts in the manifest as
"requesting access to a host". The fact that it implies access to the
host is kind of an implementation detail.

Granting access implicitly seems like it could lead to surprising
behavior. For example, removing a content script could cause some
previously-working cross-origin XHRs to stop working.

- a

[Matt Perry, 2011-06-01 00:27:48]

On 2011/05/31 23:50:16, Aaron Boodman wrote:
> On Tue, May 31, 2011 at 12:13 PM,  <mailto:mpcomplete@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
> > File chrome/renderer/extensions/user_script_slave.cc (right):
> >
> >
>
http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/u...
> > chrome/renderer/extensions/user_script_slave.cc:85:
> > extension->GetEffectiveHostPermissions().patterns();
> > On 2011/05/29 04:44:09, Aaron Boodman wrote:
> >>
> >> Sorry for butting in, but it seems unexpected to me that
> >
> > content_scripts.urls
> >>
> >> has the side-effect of allowing cross-origin XHR. Why is that easier
> >
> > to
> >>
> >> implement? Couldn't this line just check host_permissions() instead?
> >
> > I advocated doing this for 2 main reasons:
> > 1. From a security perspective, being able to inject script is the same
> > as being able to issue cross-origin XHR to a host.
> 
> Agree.
> 
> > 2. We have to allow XHR to the origin of the content script's frame.
> > Since the extension's isolated world has a shared security context, this
> > means it really has cross-origin access to (host_permissions + all frame
> > origins that this extension has a content script running on).
> 
> Agree.
> 
> > Erik, Mihai, and I were trying to remember why you didn't want
> > cross-origin XHR to content script URLs. Could you explain the
> > reasoning? It seems to me that requesting access to a host should be
> > universal, regardless of where you request it.
> 
> I just don't see declaring content scripts in the manifest as
> "requesting access to a host". The fact that it implies access to the
> host is kind of an implementation detail.
> 
> Granting access implicitly seems like it could lead to surprising
> behavior. For example, removing a content script could cause some
> previously-working cross-origin XHRs to stop working.

OK, that's a good point. We still can't avoid the issue of content scripts
sometimes getting access to other origins in content_scripts.urls (if other
scripts happen to be running in those origins). But maybe that's unlikely in
practice and not as big a deal.

[Aaron Boodman, 2011-06-01 14:23:30]

On Tue, May 31, 2011 at 6:27 PM,  <mpcomplete@chromium.org> wrote:
> OK, that's a good point. We still can't avoid the issue of content scripts
> sometimes getting access to other origins in content_scripts.urls (if other
> scripts happen to be running in those origins). But maybe that's unlikely in
> practice and not as big a deal.

I think that the behavior before this change was that content scripts
could only XHR to the origin of the frame they were running in
(because the isolated world had the same origin has the frame). So it
seems you could lock the behavior down to XHR being allowed to (the
extension's origin, the frame's origin, any origins in the extension's
host permissions). That would remove all the ambiguity.

- a