Currently there is a check in DocumentThreadableLoader::makeSimpleCrossOriginAccessRequest() which prevents XHR to schemes which are not in in the HTTP family as defined by KURL or in Chromium's case KURLGoogle. After talking with ap@ about this his suggestion was to make the "chrome-extension" scheme part of the http family in this context. This change could be made in KURLGoogle.cpp and would only affect builds that use GOOGLEURL. After looking into this it scares me though. There are a lot of things that rely on protocolInHTTPFamily() and it is not entirely clear to me what all the affects of this change would be when dealing with chrome-extension: URIs. http://code.google.com/p/chromium/source/search?q=protocolInHTTPFamily Doing this feels like a hack and I am worried that there will be unintended consequences. I suspect that the correct way to do this is to add a registry of schemas which can have simple XHR sent to them but I have no idea how to implement this. Assuming that we could do this we could simply add to the check in makeSimpleCrossOriginAccessRequest() something like if (!request.url().protocolInHTTPFamily() && !urlHasXHRSafeScheme(request.url())) Can either of you give me any insight as to where something like this would live and where it would be initialized?
> his suggestion was to make the "chrome-extension" scheme part of the http family in this context s/suggestion/idea/ :)
I pointed Cris towards SchemeRegistry.h
Created attachment 124016 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment on attachment 124016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124016&action=review LGTM, but you'll need fishd to review. > Source/WebCore/platform/SchemeRegistry.cpp:158 > + DEFINE_STATIC_LOCAL(URLSchemesMap, CORSEnabledSchemes, ()); > + return CORSEnabledSchemes; I would probably initialize this with http and https.
Comment on attachment 124016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124016&action=review I vaguely remember this being discussed multiple times in the past. There isn't already a solution that works for Safari extension, is there? > Source/WebCore/platform/SchemeRegistry.cpp:156 > +static URLSchemesMap& CORSEnabledSchemes() > +{ Please add ASSERT(isMainThread()) here, as this function and its callers are not thread safe.
Created attachment 124022 [details] Patch
Created attachment 124024 [details] Patch
> I vaguely remember this being discussed multiple times in the past. There isn't already a solution that works for Safari extension, is there? Yeah I went looking for this. The check is very straightforward in KURL (only http and https).
Comment on attachment 124024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124024&action=review > Source/WebKit/chromium/public/WebSecurityPolicy.h:65 > + WEBKIT_EXPORT static void registerCORSEnabledScheme(const WebString&); registerURLSchemeAsCORSEnabled for consistent naming?
Created attachment 124047 [details] Patch
Comment on attachment 124047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124047&action=review > Source/WebCore/platform/SchemeRegistry.cpp:290 > +void SchemeRegistry::registerCORSEnabledScheme(const String& scheme) registerURLSchemeAs{Foo} to follow the conventions here too, right?
(In reply to comment #0) > Currently there is a check in DocumentThreadableLoader::makeSimpleCrossOriginAccessRequest() which prevents XHR to schemes which are not in in the HTTP family as defined by KURL or in Chromium's case KURLGoogle. > > After talking with ap@ about this his suggestion was to make the "chrome-extension" scheme part of the http family in this context. This change could be made in KURLGoogle.cpp and would only affect builds that use GOOGLEURL. After looking into this it scares me though. There are a lot of things that rely on protocolInHTTPFamily() and it is not entirely clear to me what all the affects of this change would be when dealing with chrome-extension: URIs. > > http://code.google.com/p/chromium/source/search?q=protocolInHTTPFamily > > Doing this feels like a hack and I am worried that there will be unintended consequences. I suspect that the correct way to do this is to add a registry of schemas which can have simple XHR sent to them but I have no idea how to implement this. Assuming that we could do this we could simply add to the check in makeSimpleCrossOriginAccessRequest() something like > > if (!request.url().protocolInHTTPFamily() && !urlHasXHRSafeScheme(request.url())) > > Can either of you give me any insight as to where something like this would live and where it would be initialized? I am a bit unclear how this is different than https://bugs.webkit.org/show_bug.cgi?id=59843. Is the fix for that no longer working?
My understanding of that is that it allows scripts in the extension context to XHR. Perhaps I am missing something? This change would allow XHR into the extension context (or any other registered scheme) which as far as I can tell is not currently possible. This would of course also fail later if the request is from the web and the requested resource is in an extension (origins do not match) unless CORS headers are set. Is there a way to do this that I am not seeing?
Created attachment 124147 [details] Patch
(In reply to comment #14) > My understanding of that is that it allows scripts in the extension context to XHR. Perhaps I am missing something? > > This change would allow XHR into the extension context (or any other registered scheme) which as far as I can tell is not currently possible. This would of course also fail later if the request is from the web and the requested resource is in an extension (origins do not match) unless CORS headers are set. > > Is there a way to do this that I am not seeing? I am still not entirely sure I understand what you are trying to do, but I thought that WebView's _addOriginAccessWhitelistEntryWithSourceOrigin was supposed to allow access, including XHRs, from the specified source origins to any destination that with the specified protocol. If that is not working, we should probably fix that instead of adding something new to SchemeRegistry.
To clarify, the use case I am addressing is this. A user has an extension installed which contains a static resource. This resource is accessible through the URI chrome-extension://blah/resource.xml We would like for a site (http://a.com) to be able to request resource.xml via XHR. As far as I can tell this will always fail because of the check in makeSimpleCrossOriginAccessRequest() which limits these requests to HTTP schemes. I'll take a look at what you mentioned and see if it addresses this use case.
Ok took a look at addOriginAccessWhitelistEntryWithSourceOrigin.. This only allows you to register specifically whitelisted cross origin paths (ie. you must know both source and destination origin). The presence of a crossOriginRequestPolicy equal to UseAccessControl means that even when no whitelist entry exists this should be decided by CORS headers. In the case of HTTP/HTTPS this works correctly however in cases where the destination is not in the http family this does not work. There is no way to simply add these to the whitelist as 1. They will be blocked by the check I mention in comment 1 as they are not in the HTTP family 2. We would like all origins to be able to request these resources, whether or not the request succeeds is decided by whether CORS headers are supplied in the response.
(In reply to comment #18) > Ok took a look at addOriginAccessWhitelistEntryWithSourceOrigin.. This only allows you to register specifically whitelisted cross origin paths (ie. you must know both source and destination origin). You don't actually need to know the full destination origin. If you pass in just the desired scheme and a empty string for the destination host, it will consider that as access to all hosts with that scheme. > > The presence of a crossOriginRequestPolicy equal to UseAccessControl means that even when no whitelist entry exists this should be decided by CORS headers. In the case of HTTP/HTTPS this works correctly however in cases where the destination is not in the http family this does not work. > > There is no way to simply add these to the whitelist as > > 1. They will be blocked by the check I mention in comment 1 as they are not in the HTTP family Then maybe we should modify that check to respect SecurityPolicy::isAccessWhiteListed. > 2. We would like all origins to be able to request these resources, whether or not the request succeeds is decided by whether CORS headers are supplied in the response. You want all origins to be able to request these resources, or just chrome-extension origins? Isn't the list of chrome-extension origins a finite one you know when you install the extensions? Couldn't you just add them then?
> You want all origins to be able to request these resources, or just chrome-extension origins? Isn't the list of chrome-extension origins a finite one you know when you install the extensions? Couldn't you just add them then? We want all origins to be able to request them (whether it is allowed or not is decided by CORS headers). The only thing that isAccessWhiteListed() decides in the case of XHR is whether it is treated as a same origin request. It is perfectly valid for example for http://a.com to send and XHR request to http://b.com (although if b.com does not specifically set a CORS header to allow such requests it will fail). The most telling example would be the case of a extension which injects a content script into all origins. This script would run in the context of every site and would therefore currently have no way of XHRing resources from the extension that it is part of.
(In reply to comment #20) > > You want all origins to be able to request these resources, or just chrome-extension origins? Isn't the list of chrome-extension origins a finite one you know when you install the extensions? Couldn't you just add them then? > > We want all origins to be able to request them (whether it is allowed or not is decided by CORS headers). The only thing that isAccessWhiteListed() decides in the case of XHR is whether it is treated as a same origin request. It is perfectly valid for example for http://a.com to send and XHR request to http://b.com (although if b.com does not specifically set a CORS header to allow such requests it will fail). > > The most telling example would be the case of a extension which injects a content script into all origins. This script would run in the context of every site and would therefore currently have no way of XHRing resources from the extension that it is part of. That case should definitely be covered by the fix for https://bugs.webkit.org/show_bug.cgi?id=59843.
Jessie, this mechanism you're referring to are for granting certain origins more privileges than they had before. That's not really related to this patch. Currently only requests for HTTP and HTTPS can use CORS. This patch lets other schemes respond with CORS headers (e.g., Access-Control-Allow-Origin). For example, a Chrome Extension might want to allow web origins to access some of its resources (e.g., images). With this patch, it can do that by sending an appropriate Access-Control-Allow-Origin header.
(Feel free to ping me on IRC if that doesn't make sense to you.)
> We would like for a site (http://a.com) to be able to request resource.xml via XHR. From later comments, it appears that it's not "a site", but any site. Is my understanding correct? If pages in the Open Web start using chrome-extension URLs, wouldn't that be similar in spirit to features like <http://msdn.microsoft.com/en-us/library/ie/ms533086(v=vs.85).aspx>?
> 1. They will be blocked by the check I mention in comment 1 as they are not in the HTTP family I think that requests allowed by addOriginAccessWhitelistEntryWithSourceOrigin are not subject to this check. They are handled as same origin requests (it would not make much sense to send a true CORS request from a safari-extension origin to a Web site). It seems that it should be possible to use addOriginAccessWhitelistEntryWithSourceOrigin for what you want by abusing the mechanism. For each loaded page, you'd allow its origin access chrome-extension. That would be inefficient, and would result in abandoned memory, but it should try to have clarity on what the proposed API allows in addition to what we already have to avoid future confusion.
> From later comments, it appears that it's not "a site", but any site. Is my understanding correct? > > If pages in the Open Web start using chrome-extension URLs, wouldn't that be similar in spirit to features like <http://msdn.microsoft.com/en-us/library/ie/ms533086(v=vs.85).aspx>? Not really. The main use case here is an extension's content script that wants to use textures stored in the extension package in WebGL. Because WebGL requires the loaded images to use CORS, the extension needs to be able to set CORS headers. > I think that requests allowed by addOriginAccessWhitelistEntryWithSourceOrigin are not subject to this check. They are handled as same origin requests Correct. > It seems that it should be possible to use addOriginAccessWhitelistEntryWithSourceOrigin for what you want by abusing the mechanism. For each loaded page, you'd allow its origin access chrome-extension. That would be inefficient, and would result in abandoned memory, but it should try to have clarity on what the proposed API allows in addition to what we already have to avoid future confusion. Unfortunately, that's not the case. We don't want to grant access to the whole origin, we just want the extension to be able to use CORS to control access to its resources, just like a web site can use CORS to control access to its resources.
Comment on attachment 124147 [details] Patch This patch looks fine.
Comment on attachment 124147 [details] Patch We haven't finished the discussion. That should have been obvious. So, the use case is actually much more limited. There should be a way to limit the change in a way without exposing the evil to the Open Web. Also, if the issue here is not with getting the data (which is already possible from content scripts), but with fooling WebGL into accepting it, shouldn't you look into that?
Posting abarth's explanation from #webkit of the reasoning behind this patch Currently, the default is that web pages can include whatever resources (e.g., images) from chrome-extensions. this was a mistake in the original design of the chrome extension system which we are now correcting as part of our "manifest_version 2" project. unfortunately, there are a handful of extensions that use this facility for purposes that seem fine so we want to continue to allow them to do that. therefore, we're letting extensions control access to their resources using CORS which will let them scope access in a more fine-grained manner leading to less use of the chrome-extension URI scheme outside of extensions but these are all internal chrome-internal matters. The relevant part for WebKit is that we want to be able to use CORS with some non-HTTP/HTTPS schemes how we choose to use that functionality in Chrome is up to the Chromium project
We had much more discussion on IRC. I think that this change is not so great, as it adds a too big handle to SecurityOrigin, that's too easy to abuse. Also, we have way too many bells and whistles on SecurityOrigin already, to the point that we needed a long discussion here about how existing ones related to this one. That's terrible. I'm fine with getting this patch landed in current form if it's really important.
Comment on attachment 124147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124147&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Without this, of course.
> > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > Without this, of course. The change is hard to test in run-webkit-tests because it only uses HTTPFamily URLs. Cris is adding a test downstream along with the code that calls this API: http://codereview.chromium.org/9152022/
Understood. What I'm saying is that it won't land with an OOPS.
Committed r106057: <http://trac.webkit.org/changeset/106057>