9 years, 7 months ago
(2011-05-26 22:14:39 UTC)
#1
Matt Perry
http://codereview.chromium.org/7071025/diff/1001/chrome/renderer/extensions/user_script_slave.cc File chrome/renderer/extensions/user_script_slave.cc (right): http://codereview.chromium.org/7071025/diff/1001/chrome/renderer/extensions/user_script_slave.cc#newcode78 chrome/renderer/extensions/user_script_slave.cc:78: WebSecurityPolicy::addOriginAccessWhitelistEntry( note that we never clear the origin access ...
9 years, 7 months ago
(2011-05-26 22:39:48 UTC)
#2
On 2011/05/26 22:39:48, Matt Perry wrote: > chrome/renderer/extensions/user_script_slave.cc:78: > WebSecurityPolicy::addOriginAccessWhitelistEntry( > note that we never ...
9 years, 7 months ago
(2011-05-27 02:03:00 UTC)
#3
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
http://codereview.chromium.org/7071025/diff/4001/chrome/renderer/extensions/user_script_slave.cc File chrome/renderer/extensions/user_script_slave.cc (right): http://codereview.chromium.org/7071025/diff/4001/chrome/renderer/extensions/user_script_slave.cc#newcode79 chrome/renderer/extensions/user_script_slave.cc:79: void UserScriptSlave::InitializeIsolatedWorld( This seems like a lot of effort ...
9 years, 7 months ago
(2011-05-27 19:10:54 UTC)
#4
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
On Fri, May 27, 2011 at 12:10 PM, <mpcomplete@chromium.org> wrote: > I'm still of the ...
9 years, 7 months ago
(2011-05-27 20:44:34 UTC)
#5
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
> The only thing that this doesn't handle is an extension getting > reloaded with ...
9 years, 7 months ago
(2011-05-27 20:55:28 UTC)
#6
On Fri, May 27, 2011 at 1:55 PM, <mpcomplete@chromium.org> wrote: > EXTENSION_UNLOADED isn't fired in ...
9 years, 7 months ago
(2011-05-27 22:00:36 UTC)
#7
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
Cool, LGTM. Now that content scripts can XHR to content_script hosts, we should change it ...
9 years, 7 months ago
(2011-05-27 22:14:35 UTC)
#8
http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/user_script_slave.cc File chrome/renderer/extensions/user_script_slave.cc (right): http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/user_script_slave.cc#newcode85 chrome/renderer/extensions/user_script_slave.cc:85: extension->GetEffectiveHostPermissions().patterns(); Sorry for butting in, but it seems unexpected ...
9 years, 7 months ago
(2011-05-29 04:44:09 UTC)
#9
9 years, 6 months ago
(2011-05-31 18:13:32 UTC)
#10
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
On Tue, May 31, 2011 at 12:13 PM, <mpcomplete@chromium.org> wrote: > > http://codereview.chromium.org/7071025/diff/7007/chrome/renderer/extensions/user_script_slave.cc > File ...
9 years, 6 months ago
(2011-05-31 23:50:16 UTC)
#11
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
On 2011/05/31 23:50:16, Aaron Boodman wrote: > On Tue, May 31, 2011 at 12:13 PM, ...
9 years, 6 months ago
(2011-06-01 00:27:48 UTC)
#12
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
On Tue, May 31, 2011 at 6:27 PM, <mpcomplete@chromium.org> wrote: > OK, that's a good ...
9 years, 6 months ago
(2011-06-01 14:23:30 UTC)
#13
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
Issue 7071025: Use WebFrame::setIsolatedWorldSecurityOrigin to allow cross-origin XHRs in content scripts.
(Closed)
Created 9 years, 7 months ago by Mihai Parparita -not on Chrome
Modified 9 years, 6 months ago
Reviewers: Matt Perry, Aaron Boodman
Base URL: svn://svn.chromium.org/chrome/trunk/src
Comments: 7