Bug 59843

Created attachment 91764 [details] Patch

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 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.

This is probably a dumb question, but doesn't cross-origin XHR in isolated worlds already work, as evidenced by Chrome and Safari extensions?

> 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.

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.

> 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

(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.

> 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.

(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?

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.

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?

(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.

Created attachment 92517 [details] Patch

(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.

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.

(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).

> 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.

(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.

> 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.

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.)

ok.

Created attachment 94857 [details] Patch

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 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.

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 on attachment 94857 [details] Patch I'm marking this R+ for now, but please give other folks a chance to comment before landing.

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.

Created attachment 94997 [details] Patch for landing

(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.

(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.

(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.

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 on attachment 94997 [details] Patch for landing Clearing flags on attachment: 94997 Committed r87423: <http://trac.webkit.org/changeset/87423>

All reviewed patches have been landed. Closing bug.

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.

Eric/Adam: the commit queue is still processing this patch even though it already landed it an hour ago.

(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.

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.

(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.

Great! Thanks Adam.