Bug 77041 – Allow the registration of schemes which can receive CORS requests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Cris Neckar]

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?

[Alexey Proskuryakov]

> his suggestion was to make the "chrome-extension" scheme part of the http family in this context

s/suggestion/idea/ :)

[Adam Barth]

I pointed Cris towards SchemeRegistry.h

[Cris Neckar]

Created attachment 124016 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

[Cris Neckar]

Created attachment 124022 [details]
Patch

[Cris Neckar]

Created attachment 124024 [details]
Patch

[Cris Neckar]

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

[Darin Fisher (:fishd, Google)]

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?

[Cris Neckar]

Created attachment 124047 [details]
Patch

[Darin Fisher (:fishd, Google)]

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?

[Jessie Berlin]

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

[Cris Neckar]

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?

[Cris Neckar]

Created attachment 124147 [details]
Patch

[Jessie Berlin]

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

[Cris Neckar]

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.

[Cris Neckar]

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.

[Jessie Berlin]

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

[Cris Neckar]

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

[Jessie Berlin]

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

[Adam Barth]

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.

[Adam Barth]

(Feel free to ping me on IRC if that doesn't make sense to you.)

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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

[Adam Barth]

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

[Adam Barth]

Comment on attachment 124147 [details]
Patch

This patch looks fine.

[Alexey Proskuryakov]

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?

[Cris Neckar]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

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

[Alexey Proskuryakov]

Understood. What I'm saying is that it won't land with an OOPS.

[Cris Neckar]

Committed r106057: <http://trac.webkit.org/changeset/106057>