Bug 26146 – Change to use ThreadableLoader to load the worker script in order to check URL origin for redirection.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jian Li]

Per the discussion on Worker URL origin check for redirection scenario (http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-May/019965.html), we feel that it is a good practice to check every URL on the redirect chain in order to make sure it is coming from the same origin as the original document.

Currently the URL origin is only checked before we use DocLoader to load the script. To support the redirection URL origin check, we need to switch to using ThreadableLoader, i.e. SubresourceLoader, to load the script since it enforces such check for every redirection URL.

[Jian Li]

Created attachment 30922 [details]
Proposed Patch

[Alexey Proskuryakov]

Comment on attachment 30922 [details]
Proposed Patch

I didn't full review this yet, but wanted to share some comments anyway.

> +        https://bugs.webkit.org/show_bug.cgi?id=26146
> +        Test for the above bug.

Please add a title of the bug (or, better yet, what is fixed, because the bug's title mentions both the problem and a suggested approach, which may be confusing to some watchers).

> +        * http/tests/workers/resources/redirect.php: Copied from LayoutTests/http/tests/xmlhttprequest/resources/redirect.php.

We have a copy in http/tests/resources (I think that we should remove the xmlhttprequest and other copies one day for consistency).

I don't understand why importScripts() uses WorkerScriptLoader - isn't it ok for these scripts to come from any domain, or via redirects?

[Jian Li]

(In reply to comment #2)
> (From update of attachment 30922 [details] [review])
> I didn't full review this yet, but wanted to share some comments anyway.
> 
> > +        https://bugs.webkit.org/show_bug.cgi?id=26146
> > +        Test for the above bug.
> 
> Please add a title of the bug (or, better yet, what is fixed, because the bug's
> title mentions both the problem and a suggested approach, which may be
> confusing to some watchers).
I will add more details in next patch.
> 
> > +        * http/tests/workers/resources/redirect.php: Copied from LayoutTests/http/tests/xmlhttprequest/resources/redirect.php.
> 
> We have a copy in http/tests/resources (I think that we should remove the
> xmlhttprequest and other copies one day for consistency).
I will change to use that copy.
> 
> I don't understand why importScripts() uses WorkerScriptLoader - isn't it ok
> for these scripts to come from any domain, or via redirects?
> 
importScripts() uses WorkerImportScriptsClient, that has been renamed to WorkerScriptLoader so that both normal script loading and importScript could use the same functionaility.

Yes, currently there is a bug in redirect handling for importScripts. It should allow script coming and redirecting from any origin. I will fix it in other patch and also add a layout test for it next time.

[Alexey Proskuryakov]

Comment on attachment 30922 [details]
Proposed Patch

Marking r- per the above discussion to get this out of review queue.

[Jian Li]

Created attachment 30961 [details]
Proposed Patch

The importScripts redirect check bug is fixed in the patch for bug 26196. I've updated this patch to be built on top of patch for bug 26196. In addition, I have updated LayoutTests/ChangeLog and changed to use redirect.php in http/tests/resources.

[Adam Barth]

Comment on attachment 30961 [details]
Proposed Patch

> +void WorkerScriptLoader::load(ScriptExecutionContext* scriptExecutionContext, const String& url, RedirectOriginCheck redirectOriginCheck, WorkerScriptLoaderClient* client)
> +{
> +    ResourceRequest request(url);
> +    request.setHTTPMethod("GET");
> +    request.setHTTPOrigin(scriptExecutionContext->securityOrigin()->toString());

Why don't you need to set the Referer here too?

> +        ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent;

This flag is super mysterious to me, but I suspect this usage is "correct" insofar as it is consistent with other locations.

> +    if (response.httpStatusCode() / 100 != 2 && response.httpStatusCode() != 0) {

This is super ugly.  I know Chromium does this, but do we do this elsewhere in Webcore?  If so, please file a bug to clean up this nonsense. 

> +    protected:
> +        virtual ~WorkerScriptLoaderClient() { }

Why is this protected?

> +        WorkerScriptLoader()
> +            : m_client(0)
> +            , m_failed(false)
> +            , m_identifier(0)
> +        {
> +        }

Please move this function to the .cpp file.  It is an implementation detail.

> +        // If loaderClient is provided, load the script asynchronously. Otherwise, load it synchronously.
> +        void load(ScriptExecutionContext* scriptExecutionContext, const String& url, RedirectOriginCheck redirectOriginCheck, WorkerScriptLoaderClient* client);

Why not have two entry points, one for sync and one for async?  Conditioning this on the nullity of the client is asking for future bugs.

The above comments are all pretty minor, but I'd like to see an updated patch before giving an r+.

Thanks!

[Jian Li]

Created attachment 31122 [details]
Proposed Patch

[Jian Li]

(In reply to comment #6)
> (From update of attachment 30961 [details] [review])
> > +void WorkerScriptLoader::load(ScriptExecutionContext* scriptExecutionContext, const String& url, RedirectOriginCheck redirectOriginCheck, WorkerScriptLoaderClient* client)
> > +{
> > +    ResourceRequest request(url);
> > +    request.setHTTPMethod("GET");
> > +    request.setHTTPOrigin(scriptExecutionContext->securityOrigin()->toString());
> 
> Why don't you need to set the Referer here too?

We don't really neeed to set the Referer here since underneath it, SubresourceLoader will set the correct Referer. 
> 
> > +        ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent;
> 
> This flag is super mysterious to me, but I suspect this usage is "correct"
> insofar as it is consistent with other locations.
> 
Yes, it is used in other places.
> > +    if (response.httpStatusCode() / 100 != 2 && response.httpStatusCode() != 0) {
> 
> This is super ugly.  I know Chromium does this, but do we do this elsewhere in
> Webcore?  If so, please file a bug to clean up this nonsense. 
> 
Yes, this is kind of ugly. However, it is widely used throughout WebCore as a way to check HTTP status code.
> > +    protected:
> > +        virtual ~WorkerScriptLoaderClient() { }
> 
> Why is this protected?

Yes, this has to be protected because the derived class will get to this.
> 
> > +        WorkerScriptLoader()
> > +            : m_client(0)
> > +            , m_failed(false)
> > +            , m_identifier(0)
> > +        {
> > +        }
> 
> Please move this function to the .cpp file.  It is an implementation detail.

Done.
> 
> > +        // If loaderClient is provided, load the script asynchronously. Otherwise, load it synchronously.
> > +        void load(ScriptExecutionContext* scriptExecutionContext, const String& url, RedirectOriginCheck redirectOriginCheck, WorkerScriptLoaderClient* client);
> 
> Why not have two entry points, one for sync and one for async?  Conditioning
> this on the nullity of the client is asking for future bugs.
> 
Done.
> The above comments are all pretty minor, but I'd like to see an updated patch
> before giving an r+.
> 
> Thanks!
> 

[Adam Barth]

Comment on attachment 31122 [details]
Proposed Patch

LGTM, with one question:

> +    request.setHTTPOrigin(scriptExecutionContext->securityOrigin()->toString());

I still don't understand this line of code.  Where in http://www.whatwg.org/specs/web-workers/current-work/ does it say to send the Origin header?  Usually, we send the Origin header for POST requests and requests generated from cross-origin XMLHttpRequests.

[Alexey Proskuryakov]

> > +        ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent;
> 
> This flag is super mysterious to me, but I suspect this usage is "correct"
> insofar as it is consistent with other locations.

It is not consistent, and I think it's incorrect. The only place we use similar logic is in WorkerThreadableLoader::loadResourceSynchronously(), while in this patch, it's added for async loading.

The original copy/paste source for this was XMLHttpRequest - we had to enable content sniffing for local files in order for extension-based MIME type guessing to work on Windows, working around a CFNetwork bug. This is no longer needed, and the code was removed in <http://trac.webkit.org/changeset/43511>. I think that the whole ContentSniff enum can be removed now, together with associated function arguments (in a separate patch, of course).

[Jian Li]

Thanks for sharing the background info about this enum. I will change to always use DoNotSniffContent. Also, I will change the enum value RequireSameRedirectOrigin to DenyCrossOriginRedirect as suggested by Adam in another thread.

I am still waiting for Oliver to confirm the removal of Origin header. Once all these are settled, I will send in a new patch and then land it.

(In reply to comment #10)
> > > +        ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent;
> > 
> > This flag is super mysterious to me, but I suspect this usage is "correct"
> > insofar as it is consistent with other locations.
> 
> It is not consistent, and I think it's incorrect. The only place we use similar
> logic is in WorkerThreadableLoader::loadResourceSynchronously(), while in this
> patch, it's added for async loading.
> 
> The original copy/paste source for this was XMLHttpRequest - we had to enable
> content sniffing for local files in order for extension-based MIME type
> guessing to work on Windows, working around a CFNetwork bug. This is no longer
> needed, and the code was removed in <http://trac.webkit.org/changeset/43511>. I
> think that the whole ContentSniff enum can be removed now, together with
> associated function arguments (in a separate patch, of course).
> 

[Adam Barth]

Comment on attachment 31122 [details]
Proposed Patch

Changing back to ? because we're waiting on feedback from Olliej

[Oliver Hunt]

Comment on attachment 31122 [details]
Proposed Patch

I think Dave Levin should review this patch, but also i'm unsure what it is trying to do -- the bug conflates changing the loader and adding origin checks, and i'm unsure whether this effects new Worker or importScripts

The discussion i had with firefox people some time ago resulted in the conclusion that importScripts should not have an origin check (eg. it would behave as <script>)

[David Levin]

Comment on attachment 31122 [details]
Proposed Patch

Tthis is looking pretty good.  As far the code itself, it all looks good as far as the algorithm. I have one concern about the script encoding below when loading the worker script itself.

Several style/comment/etc. comments below.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> [phone]  Jian Li  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 26146: Change to use ThreadableLoader to load the worker script
> +        in order to check URL origin for redirection.
> +        https://bugs.webkit.org/show_bug.cgi?id=26146
> +
> +        Test: http/tests/workers/worker-redirect.html
> +
> +        * WebCore.xcodeproj/project.pbxproj:
> +        * workers/Worker.cpp:
> +        (WebCore::Worker::Worker):
> +        (WebCore::Worker::notifyFinished):
> +        * workers/Worker.h:
> +        * workers/WorkerContext.cpp:
> +        (WebCore::WorkerContext::importScripts):
> +        * workers/WorkerImportScriptsClient.cpp: Removed.
> +        * workers/WorkerImportScriptsClient.h: Removed.
> +        * workers/WorkerScriptLoader.cpp: Renamed from workers/WorkerImportScriptsClient.cpp.
> +        (WebCore::WorkerScriptLoader::loadSynchronously):
> +        (WebCore::WorkerScriptLoader::loadAsynchronously):
> +        (WebCore::WorkerScriptLoader::didFinishLoading):
> +        (WebCore::WorkerScriptLoader::didFail):
> +        (WebCore::WorkerScriptLoader::didFailRedirectCheck):
> +        (WebCore::WorkerScriptLoader::didReceiveAuthenticationCancellation):
> +        (WebCore::WorkerScriptLoader::notifyFinished):
> +        * workers/WorkerScriptLoader.h: Renamed from workers/WorkerImportScriptsClient.h.
> +

It would be nice to have some more function level of file level comments.  For example, why was 
WorkerScriptLoader.cpp renamed from workers/WorkerImportScriptsClient.cpp could be one comment.

It would have been nice to separate the two changes as I see them:
1. Making WorkerImportScriptsClient more generic so other could use it.
2. Making WorkerContext use it for loading scripts.



> diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj

There are several other makefiles that need to be fixed up.



> diff --git a/WebCore/workers/Worker.h b/WebCore/workers/Worker.h
> +        OwnPtr<WorkerScriptLoader> m_workerScriptLoader;

What about m_scriptLoader as the variable name?


> diff --git a/WebCore/workers/WorkerContext.cpp b/WebCore/workers/WorkerContext.cpp

> +        WorkerScriptLoader workerScriptLoader;
Consider using scriptLoader instead of orkerScriptLoader (just because shorter variable names are easier to skim and not get confused).



> diff --git a/WebCore/workers/WorkerScriptLoader.cpp b/WebCore/workers/WorkerScriptLoader.cpp
> new file mode 100644
> index 0000000..a0f3c48
> --- /dev/null
> +++ b/WebCore/workers/WorkerScriptLoader.cpp


> +     // We only support synchronous script loading for importScripts in WorkerContext.
I wonder if this comment is necessary.  Especially because it could easily get out of date and I'm not sure that it adds value.

How about just removing it?  The assert and the code right below it makes it clear that the sync load path is only for use by workers.


> +void WorkerScriptLoader::didReceiveResponse(const ResourceResponse& response)
> +{
> +    if (response.httpStatusCode() / 100 != 2 && response.httpStatusCode() != 0) {

Avoid comparisons to zero.

s/response.httpStatusCode() != 0/response.httpStatusCode()/


+   m_responseEncoding = response.textEncodingName();
About this for the script for the Worker itself. dimich did a lot of investigation about the encodings for worker scripts, so
we should get a comment from him if this is the right thing or not.


> +
> +}
Add the namespace comment.


> diff --git a/WebCore/workers/WorkerScriptLoader.h b/WebCore/workers/WorkerScriptLoader.h


> +    class WorkerScriptLoaderClient {
> +    public:
> +        virtual void notifyFinished() { }
> +        
> +    protected:
> +        virtual ~WorkerScriptLoaderClient() { }
> +    };
> +
It would be good to have WorkerScriptLoaderClient in a separate header file.

Why does the destructor need to be defined (as virtual)?



> +    class WorkerScriptLoader : public ThreadableLoaderClient {

There are a lot of parameter names that don't add any information.  Please remove them.

First example "ScriptExecutionContext* scriptExecutionContext"


>
> +}
Add namespace comment.

> +#endif // ENABLE(WORKERS)
> +#endif
Add comment about what this ends.

[Jian Li]

Created attachment 31321 [details]
Proposed Patch

Fixed all per levin's feedbacks. I've talked with dimich about the text encoding and what we do now is fine.

I am going to use a different patch to change the enum values.

[David Levin]

Comment on attachment 31321 [details]
Proposed Patch

Just a few things here that you can fix on landing.


> diff --git a/LayoutTests/http/tests/workers/worker-redirect.html b/LayoutTests/http/tests/workers/worker-redirect.html
worker-redirect-cross-origin.html?

> @@ -0,0 +1,28 @@
> +<body>
> +<p>Test worker script URL origin check for redirections.</p>

Test that loading the worker's script does not allow a cross origin redirect (bug <a href="https://bugs.webkit.org/show_bug.cgi?id=26146">bug 26146<a/>)


> +var worker = new Worker('/resources/redirect.php?url=http://localhost:8000/workers/resources/worker-redirect-target.js');
> +worker.onerror = function(evt) {
> +    log("SUCCESS: threw error when attempting to load script redirected from different origin");
SUCCESS: threw error when attempting to redirected cross origin while loading the worker script.

> +worker.onmessage = function(evt) {
> +    log("FAIL: executed script redirected from different origin");
FAIL: executed script when redirect cross origin.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        * workers/WorkerImportScriptsClient.h: Removed.
> +        * workers/WorkerScriptLoader.cpp: Renamed from workers/WorkerImportScriptsClient.cpp.
> +          This to make it more generic so other could use it.
so others
(or you could just say "Worker script loading").


> +        (WebCore::WorkerScriptLoader::notifyFinished):
> +        * workers/WorkerScriptLoader.h: Renamed from workers/WorkerImportScriptsClient.h.
> +          This to make it more generic so other could use it.
others


> diff --git a/WebCore/workers/Worker.h b/WebCore/workers/Worker.h
> +#include "KURL.h"
> +#include "WorkerScriptLoader.h"
Only needs WorkerScriptLoaderClient.h and add "class WorkerScriptLoader;" below.



> diff --git a/WebCore/workers/WorkerScriptLoader.h b/WebCore/workers/WorkerScriptLoader.h
> +#include "ThreadableLoaderClient.h"
> +#include "WorkerScriptLoaderClient.h"
I don't think you need to include this header.  Only add "class WorkerScriptLoaderClient;" below.

[Jian Li]

Committed as hhttp://trac.webkit.org/changeset/44726