Bug 26196 - Fix the problem that worker's importScripts fails if the script URL is redirected from different origin.
Summary: Fix the problem that worker's importScripts fails if the script URL is redire...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 26146
  Show dependency treegraph
 
Reported: 2009-06-04 13:14 PDT by Jian Li
Modified: 2009-06-15 02:38 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Patch (23.45 KB, patch)
2009-06-04 13:19 PDT, Jian Li
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2009-06-04 13:14:52 PDT
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.
Comment 1 Jian Li 2009-06-04 13:19:47 PDT
Created attachment 30957 [details]
Proposed Patch
Comment 2 David Levin 2009-06-08 18:03:19 PDT
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 @@
> +2009-06-04  Jian Li  <jianli@chromium.org>
> +
> +        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  <ap@webkit.org>
>  
>          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).
Comment 3 Jian Li 2009-06-09 11:27:08 PDT
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.
Comment 4 Alexey Proskuryakov 2009-06-13 13:50:03 PDT
I cannot really parse "RequireSameRedirectOrigin" - is the order of words as intended here? Would "AllowCrossOriginRedirect" be clearer perhaps?
Comment 5 Adam Barth 2009-06-13 14:16:31 PDT
(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?
Comment 6 Alexey Proskuryakov 2009-06-15 02:38:33 PDT
Sounds fine to me.