Bug 59843 - Support cross-origin XMLHttpRequest in isolated worlds
Summary: Support cross-origin XMLHttpRequest in isolated worlds
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mihai Parparita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-29 16:26 PDT by Mihai Parparita
Modified: 2011-06-01 10:58 PDT (History)
13 users (show)

See Also:


Attachments
Patch (54.63 KB, patch)
2011-04-29 16:27 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (40.72 KB, patch)
2011-05-05 17:58 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (36.88 KB, patch)
2011-05-25 14:23 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch for landing (40.36 KB, patch)
2011-05-26 10:16 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2011-04-29 16:26:52 PDT
Support cross-origin XMLHttpRequest in isolated worlds
Comment 1 Mihai Parparita 2011-04-29 16:27:41 PDT
Created attachment 91764 [details]
Patch
Comment 2 Mihai Parparita 2011-04-29 16:33:32 PDT
This is a very rough sketch of the simpler approach, where the XMLHttpRequest constructor has a SecurityOrigin passed to it, and it (and the DocumentThreadableLoader that it uses) use that instead of the document's/script execution context's.

The V8 bindings get the set of whitelisted origins from the V8IsolatedContext. Does it seem right to hang the whitelist off the V8IsolatedContext?

The copying of document's SecurityOrigin that WhitelistedSecurityOrigin seems like a bad idea. Ideally I'd like to turn WhitelistedSecurityOrigin into a facade that delegates to the document's, except for the whitelisted queries.

There's also no JSC support, I haven't looked into what it would take on that side.
Comment 3 Adam Barth 2011-04-29 16:42:13 PDT
Comment on attachment 91764 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91764&action=review

The whitespace changes are also distracting.  Maybe make them separately?

> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:59
> +    RefPtr<SecurityOrigin> securityOrigin = 0;

RefPtr automagically inits to 0.

> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:62
> +      securityOrigin = WhitelistedSecurityOrigin::create(
> +          context->securityOrigin(), isolatedContext->originAccessWhitelist());

Bad indent here.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:71
> -    , m_sameOriginRequest(document->securityOrigin()->canRequest(request.url()))
> +    , m_sameOriginRequest((options.securityOrigin ? options.securityOrigin.get() : document->securityOrigin())->canRequest(request.url()))

This should be a helper function.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:99
> +        SecurityOrigin* securityOrigin = options.securityOrigin ? options.securityOrigin.get() : document->securityOrigin();

Another instance of the helper function.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:406
> +SecurityOrigin* DocumentThreadableLoader::securityOrigin() const
> +{
> +    return m_options.securityOrigin ? m_options.securityOrigin.get() : m_document->securityOrigin();
>  }

Ah, you wrote it but didn't use it everywhere.  :)

> Source/WebCore/loader/ThreadableLoader.h:60
> +        ThreadableLoaderOptions() : sendLoadCallbacks(false), sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(DenyCrossOriginRequests), securityOrigin(0) { }

RefPtr automagically inits to zero.

> Source/WebCore/page/SecurityOrigin.h:224
> +class WhitelistedSecurityOrigin : public SecurityOrigin {

Can do we this without making a subclass?  That doesn't seem like the right approach.  You should hand the XMLHttpRequest constructor the SecurityOrigin for the content script, not the one from the main world augmented with extra data.
Comment 4 Alexey Proskuryakov 2011-04-29 22:30:52 PDT
This is probably a dumb question, but doesn't cross-origin XHR in isolated worlds already work, as evidenced by Chrome and Safari extensions?
Comment 5 Adam Barth 2011-04-29 22:39:40 PDT
> This is probably a dumb question, but doesn't cross-origin XHR in isolated worlds already work, as evidenced by Chrome and Safari extensions?

In the Chrome extension system (and possibly the Safari system as well), extensions can have privileges to make XMLHttpRequests to some origins without the restrictions from CORS.  Mihai would like to extend that privilege to extension content scripts as well as background pages in the Chrome system.
Comment 6 Alexey Proskuryakov 2011-04-29 22:53:21 PDT
CC'ing some people who may have an opinion about this. Making XHR requests from content scripts seems suspicious to me, but I admit knowing very little about extensions.
Comment 7 Adam Barth 2011-04-29 23:04:42 PDT
> Making XHR requests from content scripts seems suspicious to me, but I admit knowing very little about extensions.

It's a top feature request from Chrome extension authors:
http://code.google.com/p/chromium/issues/detail?id=18857
Comment 8 Mihai Parparita 2011-04-29 23:34:56 PDT
(In reply to comment #7)
> Mihai would like to extend that privilege to extension content scripts as well as background pages in the Chrome system.

Background pages in Chrome can already make cross-origin requests (this is enabled by SecurityOrigin::addOriginAccessWhitelistEntry) to hosts that are listed in the permissions section of the manifest. I'm not clear if global HTML pages (the Safari extension system equivalent) can do this too, at least from my reading of http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/AddingaGlobalHTMLPage/AddingaGlobalHTMLPage.html and the permissions section. Pointers by someone familiar with the Safari extension system would be appreciated.

Given that content scripts can already communicate with background pages (and vice-versa), it's already possible to do cross-origin requests from them by having the background page do the request and pass the response back to the script, so this is not adding any new capabilities.

There are two reasons for wanting to allow content scripts to have cross-origin access (within their isolated world) directly:
1. For a large class of extensions, this removes the need for a background page. In the Chromium multi-process model, this means that that whole process can be gotten rid of (I'm not sure if in a WebKit2 model Safari would also have global HTML pages in their own process)
2. Extension authors find the content script/background page interaction confusing and cumbersome. Additionally, the Greasemonkey compatibility layer that Chromium provides is currently not as compatible as it could be, since GM_xmlhttprequest (which can make cross-origin requests with Greasemonkey) does not behave the same way.
Comment 9 Alexey Proskuryakov 2011-04-30 00:51:28 PDT
> For a large class of extensions, this removes the need for a background page.

Safari documentation recommends keeping injected scripts lightweight (which is only natural, because they are loaded into every frame). As much code as possible should be in global page.
Comment 10 Jessie Berlin 2011-04-30 09:02:56 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Mihai would like to extend that privilege to extension content scripts as well as background pages in the Chrome system.
> 
> Background pages in Chrome can already make cross-origin requests (this is enabled by SecurityOrigin::addOriginAccessWhitelistEntry) to hosts that are listed in the permissions section of the manifest. I'm not clear if global HTML pages (the Safari extension system equivalent) can do this too, at least from my reading of http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/AddingaGlobalHTMLPage/AddingaGlobalHTMLPage.html and the permissions section. Pointers by someone familiar with the Safari extension system would be appreciated.

In Safari Extensions, Global pages *can* make cross-origin requests to hosts that are in the whitelist specified in the Extension builder.

In addition, full pages (Extension content loaded into a tab, not content scripts) can also make cross-origin requests to the hosts that are in the whitelist specified in the Extension builder.

> 
> Given that content scripts can already communicate with background pages (and vice-versa), it's already possible to do cross-origin requests from them by having the background page do the request and pass the response back to the script, so this is not adding any new capabilities.

Which makes me a little suspicious of adding another way to do the same thing.

> 
> There are two reasons for wanting to allow content scripts to have cross-origin access (within their isolated world) directly:
> 1. For a large class of extensions, this removes the need for a background page. In the Chromium multi-process model, this means that that whole process can be gotten rid of (I'm not sure if in a WebKit2 model Safari would also have global HTML pages in their own process)
> 2. Extension authors find the content script/background page interaction confusing and cumbersome. Additionally, the Greasemonkey compatibility layer that Chromium provides is currently not as compatible as it could be, since GM_xmlhttprequest (which can make cross-origin requests with Greasemonkey) does not behave the same way.

I am not so much convinced by (1) because keeping the injected scripts lightweight means shorter loading times. 

I am also not really convinced by the beginning argument of (2) because async communication is the model that we have intentionally exposed to developers.

As to the second part of (2) - the Greasemonkey compatibility layer - I am a bit ambivalent. I don't think that all browser Extension APIs need to be able to do the exact same things. However, It does seem to be a big request by Chrome Extension Developers.

Tim, what do you think?
Comment 11 Timothy Hatcher 2011-04-30 10:41:07 PDT
Yes, global page in Safari are the more-or-less same as Chrome's background page.

Keeping content scripts light-weight is nice, but not always practical. And when content scripts are  whitelisted suitably they don't really impact things much. And if you let developers eliminate a global page that only exists for cross-origin XHR, then you eliminate the overhead of a whole WebView (and maybe a process) along with the required IPC in Chrome and WebKit2.

The current model somewhat encurages developers to cache responses for XHRs in the global page (assuming multiple tabs might be requesting the same info.) (At least that is what I try to do.) But I suspect most developers just wire things up to do direct XHRs and don't do anything smart. And even create wrappers like GM_xmlhttprequest to help them.

Given that, I think we should just allow cross-origin XHRs an simplify things for developers and make it more efficient than what they already do with a global page.
Comment 12 Sam Weinig 2011-04-30 17:26:19 PDT
It seems slightly confusing to me that we would allow violation of the same-origin policy for only XHR. Is there a reason to limit this?  Perhaps all origin checks should by tied to a world?
Comment 13 Aaron Boodman 2011-04-30 18:10:09 PDT
(In reply to comment #12)
> It seems slightly confusing to me that we would allow violation of the same-origin policy for only XHR. Is there a reason to limit this?  Perhaps all origin checks should by tied to a world?

XHR is all we needed when we first implemented this. There was no reason that I remember other than that.

In Chrome extensions, when you request permission to an origin, access to script that origin (via programmatic script injection) comes with it. So your proposal wouldn't be a capability increase for us.
Comment 14 Mihai Parparita 2011-05-05 17:58:08 PDT
Created attachment 92517 [details]
Patch
Comment 15 Mihai Parparita 2011-05-05 18:05:27 PDT
(new patch fixes other comments)

(In reply to comment #3)
> > Source/WebCore/page/SecurityOrigin.h:224
> > +class WhitelistedSecurityOrigin : public SecurityOrigin {
> 
> Can do we this without making a subclass?  That doesn't seem like the right approach.  You should hand the XMLHttpRequest constructor the SecurityOrigin for the content script, not the one from the main world augmented with extra data.

Not quite sure I understand. I'd like the content script to have access to the origin that the document that it's injected is in (hence the copy from the main world), plus additional entries. Right now SecurityOrigin only stores data about one origin.

Or are you suggesting that I create a new (unique and/or keyed off the world ID) for the content script/isolated world, and then use SecurityOrigin::addOriginAccessWhitelistEntry to give it access to additional origins?

I might actually prefer that, since then I wouldn't have to create the V8IsolatedContext in  V8Proxy::addIsolatedWorldOriginAccessWhitelistEntry (just to have something to hang the entries off of), which in turn requires the extension ID to be passed around everywhere.
Comment 16 Adam Barth 2011-05-05 18:17:24 PDT
Can an extension have a content script injected into a page which it isn't allowed to issue XMLHttpRequests for?  Those two abilities seem equally powerful.
Comment 17 Mihai Parparita 2011-05-05 20:23:43 PDT
(In reply to comment #16)
> Can an extension have a content script injected into a page which it isn't allowed to issue XMLHttpRequests for?  Those two abilities seem equally powerful.

Yes, I believe the list of host/origin permissions at the extension manifest level can be different from the list of match patterns for the content scripts.

However, I don't think we would actually want to disallow the content script from make requests to the parent page's origin, since that's possible right now (that's all they can do), and so we'd risk breaking existing extensions (we can try to figure out how many Chrome extensions that would be affected if you think it's a worthwhile compatibility break).
Comment 18 Adam Barth 2011-05-05 20:27:48 PDT
> Yes, I believe the list of host/origin permissions at the extension manifest level can be different from the list of match patterns for the content scripts.

Can we just make them the same?  There isn't much sense in them being different.
Comment 19 Mihai Parparita 2011-05-05 20:30:54 PDT
(In reply to comment #18)
> Can we just make them the same?  There isn't much sense in them being different.

Possible scenario: an extension that shows the Barnes&Noble price for a book when I'm on Amazon's page. Then the permission would be for bn.com, but the content script match pattern would be for amazon.com.
Comment 20 Adam Barth 2011-05-05 20:34:13 PDT
> Possible scenario: an extension that shows the Barnes&Noble price for a book when I'm on Amazon's page. Then the permission would be for bn.com, but the content script match pattern would be for amazon.com.

Right but, in the end, the extension has access to both bn.com and amazon.com.  We could just reflect that information in it's privileges.
Comment 21 Timothy Hatcher 2011-05-05 20:35:35 PDT
Safari is the same way. Domain access can differ from content script patterns. I makes sense to allow conten scripts to be a subset of the domain list (XHR access only to one domain with content scripts on another domain), but it does not make sense to have content script patterns be a super set (since those patterns outside the domain set wont work and get filtered out, at least in Safari.)
Comment 22 Adam Barth 2011-05-05 22:03:04 PDT
ok.
Comment 23 Mihai Parparita 2011-05-25 14:23:36 PDT
Created attachment 94857 [details]
Patch
Comment 24 Mihai Parparita 2011-05-25 14:26:43 PDT
I've changed the approach slightly (as discussed in comment 15) to accomplish this via a ScriptController::setIsolatedWorldSecurityOrigin function, which allows XHRs in an isolated world to be associated with an arbitrary origin. Then extensions can set the associated security origin for the isolated world to "chrome-extension://<extension ID>" and then use addOriginAccessWhitelistEntry (like they already do to allow cross-origin XHR from extension pages).

The patch does not currently have JSC support or an implementation of LayoutTestController.setIsolatedWorldSecurityOrigin for any port but Chromium. I will add those if this approach seems reasonable.
Comment 25 Adam Barth 2011-05-25 16:13:54 PDT
Comment on attachment 94857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94857&action=review

This looks like a nice approach.  It would be slightly prettier if DocumentThreadableLoader could be fully detached from Document, but we can't quite do that yet because of the loading system being entangled with Frame.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:376
> +    return m_options.securityOrigin ? m_options.securityOrigin.get() : m_document->securityOrigin();

It's interesting, you can almost remove the m_document pointer from DocumentThreadableLoader.  There's still a tiny dependency on Frame / FrameLoader though.

> Source/WebKit/chromium/public/WebFrame.h:256
> +    virtual void setIsolatedWorldSecurityOrigin(int worldId, const WebSecurityOrigin&) = 0;

It's strange that this is scoped to a WebFrame.  It seems more global somehow.
Comment 26 Adam Barth 2011-05-25 16:19:54 PDT
My read of the foregoing discussion is that it's unclear whether Safari will want to surface this feature in its extension system, but that there's general agreement that this isn't an unreasonable feature for embedders to want.

The approach in Mihai's current patch doesn't impose much of a cost.  The main complexity is to let the constructor of XMLHttpRequest and DocumentThreadableLoader override the SecurityOrigin of the associated Document.

The main question seems to be whether we should ask Mihai to provide a JSC implementation of this feature as well.  Given the uncertainty about whether Safari will want to surface this feature in its extension system, I'm inclined to let him off the hook for now.  If there's a desire for a JSC implementation as well, I can certainly help strong-arm him into providing it as well.
Comment 27 Adam Barth 2011-05-25 16:20:39 PDT
Comment on attachment 94857 [details]
Patch

I'm marking this R+ for now, but please give other folks a chance to comment before landing.
Comment 28 Marshall Greenblatt 2011-05-26 07:32:14 PDT
I have a use case (bug #61007) that's related to but not identical to the isolated worlds requirements. I'm using a custom (non-HTTP) protocol and would like to allow cross-origin XMLHttpRequest. Due to CrossOriginAccessControl non-HTTP protocols are not allowed to execute cross-origin XHR even when using the "Access-Control-Allow-Origin" header. So I think my only option currently is to grantUniversalAccess() which I would rather not do. For my use case it would be very nice if I could set the the SecurityOrigin (with whitelisting capabilities) on the WebFrame as a whole. Is this a feature that could be supported based on this patch? Thanks.
Comment 29 Mihai Parparita 2011-05-26 10:16:37 PDT
Created attachment 94997 [details]
Patch for landing
Comment 30 Mihai Parparita 2011-05-26 10:24:34 PDT
(In reply to comment #25)
> It's interesting, you can almost remove the m_document pointer from DocumentThreadableLoader.  There's still a tiny dependency on Frame / FrameLoader though.

Is it worth changing the DocumentThreadableLoader constructor to take a Frame instead of a Document, to make the dependency slightly smaller?

 > > Source/WebKit/chromium/public/WebFrame.h:256
> > +    virtual void setIsolatedWorldSecurityOrigin(int worldId, const WebSecurityOrigin&) = 0;
> 
> It's strange that this is scoped to a WebFrame.  It seems more global somehow.

It needs to use the ScriptController instance, which is owned by the Frame.

(In reply to comment #26)
> If there's a desire for a JSC implementation as well, I can certainly help strong-arm him into providing it as well.

I've filed bug 61540 about the JSC implementation and added the new test to Skipped files for now. I'll start looking to an implementation of that this week.
Comment 31 Mihai Parparita 2011-05-26 10:25:39 PDT
(In reply to comment #28)
> I have a use case (bug #61007) that's related to but not identical to the isolated worlds requirements. I'm using a custom (non-HTTP) protocol and would like to allow cross-origin XMLHttpRequest. Due to CrossOriginAccessControl non-HTTP protocols are not allowed to execute cross-origin XHR even when using the "Access-Control-Allow-Origin" header. So I think my only option currently is to grantUniversalAccess() which I would rather not do. For my use case it would be very nice if I could set the the SecurityOrigin (with whitelisting capabilities) on the WebFrame as a whole. Is this a feature that could be supported based on this patch? Thanks.

Document exposes a setSecurityOrigin method that might be appropriate for your needs. I don't think adding not-quire-related functionality makes sense for this patch.
Comment 32 Adam Barth 2011-05-26 10:41:02 PDT
(In reply to comment #30)
> (In reply to comment #25)
> > It's interesting, you can almost remove the m_document pointer from DocumentThreadableLoader.  There's still a tiny dependency on Frame / FrameLoader though.
> 
> Is it worth changing the DocumentThreadableLoader constructor to take a Frame instead of a Document, to make the dependency slightly smaller?

Probably not.  Eventually we'll remove the Frame dependency from the loader.  We can handle this issue at that time.
Comment 33 WebKit Commit Bot 2011-05-26 12:50:54 PDT
The commit-queue encountered the following flaky tests while processing attachment 94997 [details]:

inspector/debugger/debugger-scripts.html bug 59921 (authors: pfeldman@chromium.org and podivilov@chromium.org)
The commit-queue is continuing to process your patch.
Comment 34 WebKit Commit Bot 2011-05-26 12:53:13 PDT
Comment on attachment 94997 [details]
Patch for landing

Clearing flags on attachment: 94997

Committed r87423: <http://trac.webkit.org/changeset/87423>
Comment 35 WebKit Commit Bot 2011-05-26 12:53:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 WebKit Commit Bot 2011-05-26 14:36:50 PDT
The commit-queue encountered the following flaky tests while processing attachment 94997 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.
Comment 37 Mihai Parparita 2011-05-26 14:39:55 PDT
Eric/Adam: the commit queue is still processing this patch even though it already landed it an hour ago.
Comment 38 Eric Seidel (no email) 2011-05-26 14:44:01 PDT
(In reply to comment #37)
> Eric/Adam: the commit queue is still processing this patch even though it already landed it an hour ago.

It's trouble with our lock table, I think.  Sometimes two machines will get assigned the same patch even though they should not.  If you feel well versed in AppEngine datastore and transactions, perhaps you can tell me where I went wrong. :)  I'm happy to give you a tour of the code.  Or it's possible that my re-write in https://bugs.webkit.org/show_bug.cgi?id=60993 will fix this class of bug.
Comment 39 Timothy Hatcher 2011-06-01 06:17:27 PDT
How does this affect thing like localStorage which use the security origin to determin what data store to access?

You might break content scripts if localStorage starts using the extension's security origin instead of the frame's security origin.
Comment 40 Adam Barth 2011-06-01 10:52:01 PDT
(In reply to comment #39)
> How does this affect thing like localStorage which use the security origin to determin what data store to access?

It doesn't.

> You might break content scripts if localStorage starts using the extension's security origin instead of the frame's security origin.

Indeed.
Comment 41 Timothy Hatcher 2011-06-01 10:58:05 PDT
Great! Thanks Adam.