Bug 26196 – Fix the problem that worker's importScripts fails if the script URL is redirected from different origin.

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

[Jian Li]

Per the spec, the scripts loaded by worker's importScripts can come or redirect from any domain. Currently DocumentThreadableLoader::willSendRequest always checks if the redirection URL is from the same domain. To fix this problem, we need to pass additional enum parameter to the loader to tell it to perform the redirection check or not.

[Jian Li]

Created attachment 30957 [details]
Proposed Patch

[David Levin]

Comment on attachment 30957 [details]
Proposed Patch

Just a few things to fix up on landing.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 40d853a..6b451d0 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +

I think the current style is to add the bug title here.
> +        https://bugs.webkit.org/show_bug.cgi?id=26196
> +        Add a test case in the importScripts layout test to cover the scenario that the import script URL can come from different redirect origin.

Interestingly, the changelog text is typically wrapped.

Perhaps 
  "can come from different origin through a redirect."
would be slightly clearer.

> +
> +        * http/tests/workers/resources/worker-importScripts.js:
> +        * http/tests/workers/worker-importScripts-expected.txt:
> +
>  2009-06-02  Alexey Proskuryakov  <[e-mail]>
>  
>          Land correct results for a test I just added (forgot to regenerate them after adding new
> diff --git a/LayoutTests/http/tests/workers/resources/worker-importScripts.js b/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> index ddf479c..364c502 100644
> --- a/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> +++ b/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> @@ -7,6 +7,7 @@ postMessage("PASS: importScripts(), exists, is a function, and doesn't throw whe
>  var source1 = "worker-importScripts-source1.js";
>  var source2 = "worker-importScripts-source2.js";
>  var differentOrigin = "http://localhost:8000/workers/resources/worker-importScripts-differentOrigin.js";
> +var differentRedirectOrigin = "/resources/redirect.php?url=http://localhost:8000/workers/resources/worker-importScripts-differentOrigin.js";
>  var syntaxErrorSource = "worker-importScripts-syntaxError.js";
>  var fakeSource = "nonexistant";
>  var loadedSource1 = false;
> @@ -29,6 +30,16 @@ if (differentOriginLoaded)
>  
>  resetLoadFlags();
>  
> +try {
> +    importScripts(differentRedirectOrigin)
> +} catch(e) {
> +    postMessage("FAIL: Threw " + e + " when attempting cross origin load on redirection");
Add "."

> +}
> +if (differentOriginLoaded)
> +    postMessage("PASS: executed script from different redirect origin");
I think this could be re-worded slight (similar to the ChangeLog and add a ".").

Add an else to print a FAIL message so that one can tell that it failed just by running it in a browser.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index b119fa6..bf8ebc3 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,34 @@

Same fix up in this changelog as suggested above.

>  
> -DocumentThreadableLoader::DocumentThreadableLoader(Document* document, ThreadableLoaderClient* client, const ResourceRequest& request, LoadCallbacks callbacksSetting, ContentSniff contentSniff, StoredCredentials storedCredentials)
> +DocumentThreadableLoader::DocumentThreadableLoader(Document* document, ThreadableLoaderClient* client, const ResourceRequest& request, LoadCallbacks callbacksSetting, ContentSniff contentSniff, StoredCredentials storedCredentials, RedirectOriginCheck redirectOriginCheck)
>      : m_client(client)
>      , m_document(document)
>      , m_allowStoredCredentials(storedCredentials == AllowStoredCredentials)
>      , m_sameOriginRequest(document->securityOrigin()->canRequest(request.url()))
> +    , m_checkRedirectOrigin(redirectOriginCheck == RequireSameRedirectOrigin)
>  {

It would be nice to assert that redirectOriginCheck has one of the two allowed values (and feel free to add it for storedCredentials which I should have done).

[Jian Li]

Commited as http://trac.webkit.org/changeset/44538.

All feedbacks are addressed before landing except adding ".". Per discussion with levin, we do not want to add it to be consistent with other part of the test script.

[Alexey Proskuryakov]

I cannot really parse "RequireSameRedirectOrigin" - is the order of words as intended here? Would "AllowCrossOriginRedirect" be clearer perhaps?

[Adam Barth]

(In reply to comment #4)
> I cannot really parse "RequireSameRedirectOrigin" - is the order of words as
> intended here? Would "AllowCrossOriginRedirect" be clearer perhaps?

Yeah, I agree.  Maybe call the opposite DenyCrossOriginRedirect?

[Alexey Proskuryakov]

Sounds fine to me.