2009-08-17 Aaron Boodman <aa@chromium.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Aug 2009 20:43:57 +0000 (20:43 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 17 Aug 2009 20:43:57 +0000 (20:43 +0000)
        Reviewed by Alexey Proskuryakov.

        https://bugs.webkit.org/show_bug.cgi?id=28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy and
        ThreadableLoaderOptions::crossOriginRedirectPolicy since they are always set the same way.

        Also, tightened up behavior of XMLHttpRequest with cross-origin redirects and access control. We have not implemented redirects
        for access control, so we should never redirect across origins. But in two edge cases, we were:

        * Asynchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
          authorization headers) to a resource on origin A.
        * Synchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
          authorization headers) to another resource on origin B (this time sending access control authorization headers).

        * http/tests/xmlhttprequest/access-control-and-redirects-expected.txt: Added.
        * http/tests/xmlhttprequest/access-control-and-redirects.html: Added.

2009-08-17  Aaron Boodman  <aa@chromium.org>

        Reviewed by Alexey Proskuryakov.

        https://bugs.webkit.org/show_bug.cgi?id=28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy and
        ThreadableLoaderOptions::crossOriginRedirectPolicy since they are always set the same way.

        Also, tightened up behavior of XMLHttpRequest with cross-origin redirects and access control. We have not implemented
        redirects access control, so we should never redirect across origins. But in two edge cases, we were:

        * Asynchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
          authorization headers) to a resource on origin A.
        * Synchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
          authorization headers) to another resource on origin B (this time sending access control authorization headers).

        Test: http/tests/xmlhttprequest/access-control-and-redirects.html

        * loader/DocumentThreadableLoader.cpp:
        (WebCore::DocumentThreadableLoader::willSendRequest): Refactor redirect checking code into shared location.
        (WebCore::DocumentThreadableLoader::loadRequest): Ditto.
        (WebCore::DocumentThreadableLoader::isAllowedRedirect): Ditto.
        * loader/DocumentThreadableLoader.h:
        * loader/ThreadableLoader.h:
        (WebCore::ThreadableLoaderOptions::ThreadableLoaderOptions): Remove ThreadableLoaderOptions::crossOriginRedirectPolicy.
        * page/EventSource.cpp:
        (WebCore::EventSource::connect): Ditto.
        * workers/Worker.cpp: Ditto.
        (WebCore::Worker::Worker): Ditto.
        * workers/WorkerContext.cpp: Ditto.
        (WebCore::WorkerContext::importScripts): Ditto.
        * workers/WorkerScriptLoader.cpp:
        (WebCore::WorkerScriptLoader::loadSynchronously): Ditto.
        (WebCore::WorkerScriptLoader::loadAsynchronously): Ditto.
        * workers/WorkerScriptLoader.h:
        * xml/XMLHttpRequest.cpp:
        (WebCore::XMLHttpRequest::createRequest): Ditto.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@47388 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/loader/DocumentThreadableLoader.cpp
WebCore/loader/DocumentThreadableLoader.h
WebCore/loader/ThreadableLoader.h
WebCore/page/EventSource.cpp
WebCore/workers/Worker.cpp
WebCore/workers/WorkerContext.cpp
WebCore/workers/WorkerScriptLoader.cpp
WebCore/workers/WorkerScriptLoader.h
WebCore/xml/XMLHttpRequest.cpp

index f34eaba..352b739 100644 (file)
@@ -1,3 +1,21 @@
+2009-08-17  Aaron Boodman  <aa@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        https://bugs.webkit.org/show_bug.cgi?id=28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy and
+        ThreadableLoaderOptions::crossOriginRedirectPolicy since they are always set the same way.
+
+        Also, tightened up behavior of XMLHttpRequest with cross-origin redirects and access control. We have not implemented redirects
+        for access control, so we should never redirect across origins. But in two edge cases, we were:
+
+        * Asynchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
+          authorization headers) to a resource on origin A.
+        * Synchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
+          authorization headers) to another resource on origin B (this time sending access control authorization headers).
+
+        * http/tests/xmlhttprequest/access-control-and-redirects-expected.txt: Added.
+        * http/tests/xmlhttprequest/access-control-and-redirects.html: Added.
+
 2009-08-16  Darin Adler  <darin@apple.com>
 
         Reviewed by Dan Bernstein.
diff --git a/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt b/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt
new file mode 100644 (file)
index 0000000..cd6e3ad
--- /dev/null
@@ -0,0 +1,23 @@
+Tests that redirects between origins are never allowed, even when access control is involved.
+
+Per the spec, these test cases should be allowed, but cross-origin redirects are currently unsupported in WebCore.
+
+Testing /resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi (sync)
+Expecting success: false
+PASS: Error: NETWORK_ERR: XMLHttpRequest Exception 101
+Testing /resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi(async)
+Expecting success: false
+PASS: 0
+Testing http://localhost:8000/resources/redirect.php?url=http://127.0.0.1:8000/xmlhttprequest/resources/access-control-basic-allow.cgi (sync)
+Expecting success: false
+PASS: Error: NETWORK_ERR: XMLHttpRequest Exception 101
+Testing http://localhost:8000/resources/redirect.php?url=http://127.0.0.1:8000/xmlhttprequest/resources/access-control-basic-allow.cgi(async)
+Expecting success: false
+PASS: 0
+Testing http://localhost:8000/resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi (sync)
+Expecting success: false
+PASS: Error: NETWORK_ERR: XMLHttpRequest Exception 101
+Testing http://localhost:8000/resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi(async)
+Expecting success: false
+PASS: 0
+
diff --git a/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html b/LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html
new file mode 100644 (file)
index 0000000..e7d5fa4
--- /dev/null
@@ -0,0 +1,63 @@
+<p>Tests that redirects between origins are never allowed, even when access control is involved.</p>
+<p>Per the spec, these test cases should be allowed, but cross-origin redirects are currently unsupported in WebCore.</p>
+
+<pre id="console"></pre>
+<script>
+if (window.layoutTestController) {
+    layoutTestController.dumpAsText();
+    layoutTestController.waitUntilDone();
+}
+
+function log(message)
+{
+    document.getElementById('console').appendChild(document.createTextNode(message + '\n'));
+}
+
+function runTest(url, expectSyncSuccess, expectAsyncSuccess)
+{
+    log("Testing " + url + " (sync)");
+    log("Expecting success: " + expectSyncSuccess);
+
+    var req = new XMLHttpRequest();
+    req.open("GET", url, false);
+
+    try {
+        req.send(null);
+        log((expectSyncSuccess ? "PASS" : "FAIL") + ": " + req.responseText);
+    } catch (ex) {
+        log((expectSyncSuccess ? "FAIL" : "PASS") + ": " + ex);
+    }
+    
+    log("Testing " + url + "(async)");
+    log("Expecting success: " + expectAsyncSuccess);
+
+    req = new XMLHttpRequest();
+    req.open("GET", url, true);
+    req.onload = function() {
+        log((expectAsyncSuccess ? "PASS" : "FAIL") + ": " + req.responseText);
+        nextTest();
+    }
+    req.onerror = function() {
+        log((expectAsyncSuccess ? "FAIL" : "PASS") + ": " + req.status);
+        nextTest();
+    }
+    req.send(null);
+}
+
+var tests = [
+    ["/resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi", false, false],
+    ["http://localhost:8000/resources/redirect.php?url=http://127.0.0.1:8000/xmlhttprequest/resources/access-control-basic-allow.cgi", false, false],
+    ["http://localhost:8000/resources/redirect.php?url=http://localhost:8000/xmlhttprequest/resources/access-control-basic-allow.cgi", false, false]
+]
+
+var currentTest = 0;
+
+function nextTest() {
+    if (currentTest < tests.length)
+        runTest.apply(null, tests[currentTest++]);
+    else if (window.layoutTestController)
+        layoutTestController.notifyDone();
+}
+
+nextTest();
+</script>
index b036ac3..4562573 100644 (file)
@@ -1,3 +1,40 @@
+2009-08-17  Aaron Boodman  <aa@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        https://bugs.webkit.org/show_bug.cgi?id=28313: Combine ThreadableLoaderOptions::crossOriginRequestPolicy and
+        ThreadableLoaderOptions::crossOriginRedirectPolicy since they are always set the same way.
+
+        Also, tightened up behavior of XMLHttpRequest with cross-origin redirects and access control. We have not implemented
+        redirects access control, so we should never redirect across origins. But in two edge cases, we were:
+
+        * Asynchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
+          authorization headers) to a resource on origin A.
+        * Synchronous XHR: Script on origin A requests resource from origin B. Server redirects (without sending access control
+          authorization headers) to another resource on origin B (this time sending access control authorization headers).
+
+        Test: http/tests/xmlhttprequest/access-control-and-redirects.html
+
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::willSendRequest): Refactor redirect checking code into shared location.
+        (WebCore::DocumentThreadableLoader::loadRequest): Ditto.
+        (WebCore::DocumentThreadableLoader::isAllowedRedirect): Ditto.
+        * loader/DocumentThreadableLoader.h:
+        * loader/ThreadableLoader.h:
+        (WebCore::ThreadableLoaderOptions::ThreadableLoaderOptions): Remove ThreadableLoaderOptions::crossOriginRedirectPolicy.
+        * page/EventSource.cpp:
+        (WebCore::EventSource::connect): Ditto.
+        * workers/Worker.cpp: Ditto.
+        (WebCore::Worker::Worker): Ditto.
+        * workers/WorkerContext.cpp: Ditto.
+        (WebCore::WorkerContext::importScripts): Ditto.
+        * workers/WorkerScriptLoader.cpp:
+        (WebCore::WorkerScriptLoader::loadSynchronously): Ditto.
+        (WebCore::WorkerScriptLoader::loadAsynchronously): Ditto.
+        * workers/WorkerScriptLoader.h: 
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::createRequest): Ditto.
+
 2009-08-17  Adam Langley  <agl@google.com>
 
         Reviewed by Dan Bernstein (relanding of r47157).
index 74186d6..0d8dc18 100644 (file)
@@ -169,8 +169,7 @@ void DocumentThreadableLoader::willSendRequest(SubresourceLoader* loader, Resour
     ASSERT(m_client);
     ASSERT_UNUSED(loader, loader == m_loader);
 
-    // FIXME: This needs to be fixed to follow the redirect correctly even for cross-domain requests.
-    if (m_options.crossOriginRedirectPolicy == DenyCrossOriginRedirect && !m_document->securityOrigin()->canRequest(request.url())) {
+    if (!isAllowedRedirect(request.url())) {
         RefPtr<DocumentThreadableLoader> protect(this);
         m_client->didFailRedirectCheck();
         request = ResourceRequest();
@@ -322,9 +321,10 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, bool
         return;
     }
 
-    // FIXME: This check along with the one in willSendRequest is specific to xhr and
-    // should be made more generic.
-    if (m_sameOriginRequest && !m_document->securityOrigin()->canRequest(response.url())) {
+    // FIXME: FrameLoader::loadSynchronously() does not tell us whether a redirect happened or not, so we guess by comparing the
+    // request and response URLs. This isn't a perfect test though, since a server can serve a redirect to the same URL that was
+    // requested.
+    if (request.url() != response.url() && !isAllowedRedirect(response.url())) {
         m_client->didFailRedirectCheck();
         return;
     }
@@ -338,4 +338,15 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, bool
     didFinishLoading(identifier);
 }
 
+bool DocumentThreadableLoader::isAllowedRedirect(const KURL& url)
+{
+    if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests)
+        return true;
+
+    // FIXME: We need to implement access control for each redirect. This will require some refactoring though, because the code
+    // that processes redirects doesn't know about access control and expects a synchronous answer from its client about whether
+    // a redirect should proceed.
+    return m_sameOriginRequest && m_document->securityOrigin()->canRequest(url);
+}
+
 } // namespace WebCore
index a8cbf46..64b1a22 100644 (file)
@@ -40,6 +40,7 @@
 
 namespace WebCore {
     class Document;
+    class KURL;
     struct ResourceRequest;
     class ThreadableLoaderClient;
 
@@ -85,6 +86,7 @@ namespace WebCore {
         void preflightFailure();
 
         void loadRequest(const ResourceRequest&, bool skipCanLoadCheck);
+        bool isAllowedRedirect(const KURL&);
 
         RefPtr<SubresourceLoader> m_loader;
         ThreadableLoaderClient* m_client;
index 56babb9..a52bfad 100644 (file)
@@ -54,19 +54,13 @@ namespace WebCore {
         AllowCrossOriginRequests
     };
     
-    enum CrossOriginRedirectPolicy {
-        DenyCrossOriginRedirect,
-        AllowCrossOriginRedirect
-    };
-    
     struct ThreadableLoaderOptions {
-        ThreadableLoaderOptions() : sendLoadCallbacks(false), sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(DenyCrossOriginRequests), crossOriginRedirectPolicy(AllowCrossOriginRedirect) { }
+        ThreadableLoaderOptions() : sendLoadCallbacks(false), sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(DenyCrossOriginRequests) { }
         bool sendLoadCallbacks;
         bool sniffContent;
         bool allowCredentials;  // Whether HTTP credentials and cookies are sent with the request.
         bool forcePreflight;  // If AccessControl is used, whether to force a preflight.
         CrossOriginRequestPolicy crossOriginRequestPolicy;
-        CrossOriginRedirectPolicy crossOriginRedirectPolicy;
     };
 
     // Useful for doing loader operations from any thread (not threadsafe, 
index cd27287..47243d9 100644 (file)
@@ -94,7 +94,6 @@ void EventSource::connect()
     options.sendLoadCallbacks = true;
     options.sniffContent = false;
     options.allowCredentials = true;
-    options.crossOriginRedirectPolicy = DenyCrossOriginRedirect;
 
     m_loader = ThreadableLoader::create(scriptExecutionContext(), this, request, options);
 
index a906134..d4d0e9b 100644 (file)
@@ -58,7 +58,7 @@ Worker::Worker(const String& url, ScriptExecutionContext* context, ExceptionCode
         return;
 
     m_scriptLoader = new WorkerScriptLoader();
-    m_scriptLoader->loadAsynchronously(scriptExecutionContext(), scriptURL, DenyCrossOriginRedirect, this);
+    m_scriptLoader->loadAsynchronously(scriptExecutionContext(), scriptURL, DenyCrossOriginRequests, this);
     setPendingActivity(this);  // The worker context does not exist while loading, so we must ensure that the worker object is not collected, as well as its event listeners.
 }
 
index 3d6adbf..3cb1733 100644 (file)
@@ -256,7 +256,7 @@ void WorkerContext::importScripts(const Vector<String>& urls, const String& call
 
     for (Vector<KURL>::const_iterator it = completedURLs.begin(); it != end; ++it) {
         WorkerScriptLoader scriptLoader;
-        scriptLoader.loadSynchronously(scriptExecutionContext(), *it, AllowCrossOriginRedirect);
+        scriptLoader.loadSynchronously(scriptExecutionContext(), *it, AllowCrossOriginRequests);
 
         // If the fetching attempt failed, throw a NETWORK_ERR exception and abort all these steps.
         if (scriptLoader.failed()) {
index 06416f4..52baf2d 100644 (file)
@@ -50,7 +50,7 @@ WorkerScriptLoader::WorkerScriptLoader()
 {
 }
 
-void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRedirectPolicy crossOriginRedirectPolicy)
+void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRequestPolicy crossOriginRequestPolicy)
 {
     m_url = url;
 
@@ -62,13 +62,12 @@ void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecuti
 
     ThreadableLoaderOptions options;
     options.allowCredentials = true;
-    options.crossOriginRequestPolicy = AllowCrossOriginRequests;
-    options.crossOriginRedirectPolicy = crossOriginRedirectPolicy;
+    options.crossOriginRequestPolicy = crossOriginRequestPolicy;
 
     WorkerThreadableLoader::loadResourceSynchronously(static_cast<WorkerContext*>(scriptExecutionContext), *request, *this, options);
 }
     
-void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRedirectPolicy crossOriginRedirectPolicy, WorkerScriptLoaderClient* client)
+void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRequestPolicy crossOriginRequestPolicy, WorkerScriptLoaderClient* client)
 {
     ASSERT(client);
     m_client = client;
@@ -80,8 +79,7 @@ void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecut
 
     ThreadableLoaderOptions options;
     options.allowCredentials = true;
-    options.crossOriginRequestPolicy = AllowCrossOriginRequests;
-    options.crossOriginRedirectPolicy = crossOriginRedirectPolicy;
+    options.crossOriginRequestPolicy = crossOriginRequestPolicy;
 
     m_threadableLoader = ThreadableLoader::create(scriptExecutionContext, this, *request, options);
 }
index 2924ec8..47623f6 100644 (file)
@@ -46,8 +46,8 @@ namespace WebCore {
     public:
         WorkerScriptLoader();
 
-        void loadSynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRedirectPolicy);
-        void loadAsynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRedirectPolicy, WorkerScriptLoaderClient*);
+        void loadSynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRequestPolicy);
+        void loadAsynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRequestPolicy, WorkerScriptLoaderClient*);
 
         void notifyError();
 
index 17b1bb1..0b2fabb 100644 (file)
@@ -498,7 +498,6 @@ void XMLHttpRequest::createRequest(ExceptionCode& ec)
     options.forcePreflight = forcePreflight;
     options.allowCredentials = m_sameOriginRequest || m_includeCredentials;
     options.crossOriginRequestPolicy = UseAccessControl;
-    options.crossOriginRedirectPolicy = DenyCrossOriginRedirect;
 
     m_exceptionCode = 0;
     m_error = false;