Changeset 47388 in webkit

Timestamp:

Aug 17, 2009 1:43:57 PM (15 years ago)

Author:

eric@webkit.org

Message:

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-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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt (added)
• LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/loader/DocumentThreadableLoader.cpp (modified) (3 diffs)
• WebCore/loader/DocumentThreadableLoader.h (modified) (2 diffs)
• WebCore/loader/ThreadableLoader.h (modified) (2 diffs)
• WebCore/page/EventSource.cpp (modified) (1 diff)
• WebCore/workers/Worker.cpp (modified) (1 diff)
• WebCore/workers/WorkerContext.cpp (modified) (1 diff)
• WebCore/workers/WorkerScriptLoader.cpp (modified) (3 diffs)
• WebCore/workers/WorkerScriptLoader.h (modified) (1 diff)
• WebCore/xml/XMLHttpRequest.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/WebCore/loader/DocumentThreadableLoader.cpp

@@ -170 +170 @@
     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();
@@ -323 +322 @@
     }
 
-    // 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;
@@ -339 +339 @@
 }
 
+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

trunk/WebCore/loader/DocumentThreadableLoader.h

@@ -41 +41 @@
 namespace WebCore {
     class Document;
+    class KURL;
     struct ResourceRequest;
     class ThreadableLoaderClient;
@@ -86 +87 @@
 
         void loadRequest(const ResourceRequest&, bool skipCanLoadCheck);
+        bool isAllowedRedirect(const KURL&);
 
         RefPtr<SubresourceLoader> m_loader;

trunk/WebCore/loader/ThreadableLoader.h

@@ -55 +55 @@
     };
     
-    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;
@@ -67 +62 @@
         bool forcePreflight;  // If AccessControl is used, whether to force a preflight.
         CrossOriginRequestPolicy crossOriginRequestPolicy;
-        CrossOriginRedirectPolicy crossOriginRedirectPolicy;
     };
 

trunk/WebCore/page/EventSource.cpp

@@ -95 +95 @@
     options.sniffContent = false;
     options.allowCredentials = true;
-    options.crossOriginRedirectPolicy = DenyCrossOriginRedirect;
 
     m_loader = ThreadableLoader::create(scriptExecutionContext(), this, request, options);

trunk/WebCore/workers/Worker.cpp

@@ -59 +59 @@
 
     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.
 }

trunk/WebCore/workers/WorkerContext.cpp

@@ -257 +257 @@
     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.

trunk/WebCore/workers/WorkerScriptLoader.cpp

@@ -51 +51 @@
 }
 
-void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRedirectPolicy crossOriginRedirectPolicy)
+void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRequestPolicy crossOriginRequestPolicy)
 {
     m_url = url;
@@ -63 +63 @@
     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);
@@ -81 +80 @@
     ThreadableLoaderOptions options;
     options.allowCredentials = true;
-    options.crossOriginRequestPolicy = AllowCrossOriginRequests;
-    options.crossOriginRedirectPolicy = crossOriginRedirectPolicy;
+    options.crossOriginRequestPolicy = crossOriginRequestPolicy;
 
     m_threadableLoader = ThreadableLoader::create(scriptExecutionContext, this, *request, options);

trunk/WebCore/workers/WorkerScriptLoader.h

@@ -47 +47 @@
         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();

trunk/WebCore/xml/XMLHttpRequest.cpp

@@ -499 +499 @@
     options.allowCredentials = m_sameOriginRequest || m_includeCredentials;
     options.crossOriginRequestPolicy = UseAccessControl;
-    options.crossOriginRedirectPolicy = DenyCrossOriginRedirect;
 
     m_exceptionCode = 0;