[webkit-reviews] review granted: [Bug 26146] Change to use ThreadableLoader to load the worker script in order to check URL origin for redirection. : [Attachment 31321] Proposed Patch

Mon Jun 15 18:01:09 PDT 2009

David Levin <levin at chromium.org> has granted Jian Li <jianli at chromium.org>'s
request for review:
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

Attachment 31321: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=31321&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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/wor
ker-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.