XHR for Suborigin should only preflight if non-simple headers.

The status quo was sending a preflight for every XHR request from a suborigin, regardless of the type of headers. This is overly aggressive and should only be done for non-simple headers. This modifies XHR to do that and adds a variety of tests to verify. BUG=336894 [email protected] Review URL: https://codereview.chromium.org/1536903002 Cr-Commit-Position: refs/heads/master@{#366272}
crosswalk-project · Dec 19, 2015 · 76d068c · 76d068c
1 parent c67d53c
commit 76d068c
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 14 deletions.
diff --git a/third_party/WebKit/LayoutTests/http/tests/security/resources/cors-script.php b/third_party/WebKit/LayoutTests/http/tests/security/resources/cors-script.php
@@ -10,6 +10,10 @@
 if (strtolower($_GET["credentials"]) == "true") {
     header("Access-Control-Allow-Credentials: true");
 }
+$custom_header_arg = strtolower($_GET["custom"]);
+if (!(empty($custom_header_arg))) {
+    header("Access-Control-Allow-Headers: " . $custom_header_arg);
+}
 
 if ($_SERVER["HTTP_SUBORIGIN"] == "foobar") {
     header("Access-Control-Allow-Suborigin: foobar");

diff --git a/...http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-failure-output-expected.txt b/...http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-failure-output-expected.txt
@@ -1,4 +1,6 @@
 CONSOLE MESSAGE: line 9: If a Suborigin makes a request, a response without an Access-Control-Allow-Suborigin header should fail and should output a reasonable error message.
 CONSOLE ERROR: XMLHttpRequest cannot load http://127.0.0.1:8000/security/resources/cors-script.php?cors=false. Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://foobar_127.0.0.1:8000' is therefore not allowed access.
 ALERT: PASS: XHR correctly failed
+CONSOLE ERROR: XMLHttpRequest cannot load http://127.0.0.1:8000/security/resources/cors-script.php?cors=false. No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://foobar_127.0.0.1:8000' is therefore not allowed access.
+ALERT: PASS: XHR correctly failed
 
diff --git a/...outTests/http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-failure-output.php b/...outTests/http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-failure-output.php
@@ -13,21 +13,43 @@
 
 function success() {
     alert("PASS: XHR correctly failed");
-    if (window.testRunner)
-        testRunner.notifyDone();
+    next();
 }
 
 function failure() {
     alert("FAIL: XHR incorrectly succeeded");
-    if (window.testRunner)
+    next();
+}
+
+// First one should fail with preflight failure. Second one should
+// fail with access control header failure.
+var tests = [
+    function() {
+        var xhr = new XMLHttpRequest();
+        xhr.onerror = success;
+        xhr.onload = failure;
+        xhr.open("GET", "http://127.0.0.1:8000/security/resources/cors-script.php?cors=false");
+        xhr.setRequestHeader("x-custom-header", "foobar");
+        xhr.send();
+    },
+    function() {
+        var xhr = new XMLHttpRequest();
+        xhr.onerror = success;
+        xhr.onload = failure;
+        xhr.open("GET", "http://127.0.0.1:8000/security/resources/cors-script.php?cors=false");
+        xhr.send();
+    }
+];
+
+function next() {
+    if (tests.length !== 0) {
+        tests.shift()();
+    } else if (window.testRunner) {
         testRunner.notifyDone();
+    }
 }
 
-var xhr = new XMLHttpRequest();
-xhr.onerror = success;
-xhr.onload = failure;
-xhr.open("GET", "http://127.0.0.1:8000/security/resources/cors-script.php?cors=false");
-xhr.send();
+next();
 </script>
 </body>
 </html>
diff --git a/...t/LayoutTests/http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-preflight.php b/...t/LayoutTests/http/tests/security/suborigins/crossorigin/suborigin-cors-xhr-preflight.php
@@ -42,35 +42,63 @@
     }
 
     xhr.open("GET", this.src);
+    // Set a custom header to force a preflight. Even though the
+    // scheme/host/port of the source and destination origins are the same, the
+    // Suborigin should cause the request to be treated as cross-origin.
+    xhr.setRequestHeader("x-custom-header", "foobar");
     xhr.send();
 };
 
 var xorigin_preflight_script = "http://127.0.0.1:8000/security/resources/cors-script.php";
 
 // XHR preflight tests
 new SuboriginXHRTest(
-    true,
-    "Basic anonymous XHR preflight",
+    false,
+    "Complex anonymous XHR preflight, no AC for custom header",
     xorigin_preflight_script + "?cors=http://foobar_127.0.0.1:8000",
     "anonymous").execute();
 
 new SuboriginXHRTest(
     true,
-    "Basic anonymous XHR preflight with '*' ACAO",
+    "Complex anonymous XHR preflight, has AC for custom header",
+    xorigin_preflight_script + "?cors=http://foobar_127.0.0.1:8000&custom=x-custom-header",
+    "anonymous").execute();
+
+new SuboriginXHRTest(
+    false,
+    "Complex anonymous XHR preflight with '*' ACAO, no AC for custom header",
     xorigin_preflight_script + "?cors=*",
     "anonymous").execute();
 
 new SuboriginXHRTest(
     true,
-    "Basic XHR with credentials preflight",
+    "Complex anonymous XHR preflight with '*' ACAO, has AC for custom header",
+    xorigin_preflight_script + "?cors=*&custom=x-custom-header",
+    "anonymous").execute();
+
+new SuboriginXHRTest(
+    false,
+    "Complex XHR with credentials preflight, no AC for custom header",
     xorigin_preflight_script + "?cors=http://foobar_127.0.0.1:8000&credentials=true",
     "use-credentials").execute();
 
+new SuboriginXHRTest(
+    true,
+    "Complex XHR with credentials preflight, has AC for custom header",
+    xorigin_preflight_script + "?cors=http://foobar_127.0.0.1:8000&credentials=true&custom=x-custom-header",
+    "use-credentials").execute();
+
 new SuboriginXHRTest(
     false,
-    "Basic XHR with credentials preflight with '*' ACAO",
+    "Complex XHR with credentials preflight with '*' ACAO, no AC for custom header",
     xorigin_preflight_script + "?cors=*&credentials=true",
     "use-credentials").execute();
+
+new SuboriginXHRTest(
+    false,
+    "Complex XHR with credentials preflight with '*' ACAO, has AC for custom header",
+    xorigin_preflight_script + "?cors=*&credentials=true&custom=x-custom-header",
+    "use-credentials").execute();
 </script>
 </body>
 </html>
diff --git a/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp b/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
@@ -894,7 +894,7 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody, Excepti
         request.addHTTPHeaderFields(m_requestHeaders);
 
     ThreadableLoaderOptions options;
-    options.preflightPolicy = (uploadEvents || securityOrigin()->hasSuborigin()) ? ForcePreflight : ConsiderPreflight;
+    options.preflightPolicy = uploadEvents ? ForcePreflight : ConsiderPreflight;
     options.crossOriginRequestPolicy = UseAccessControl;
     options.initiator = FetchInitiatorTypeNames::xmlhttprequest;
     options.contentSecurityPolicyEnforcement = ContentSecurityPolicy::shouldBypassMainWorld(&executionContext) ? DoNotEnforceContentSecurityPolicy : EnforceConnectSrcDirective;

diff --git a/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h b/third_party/WebKit/Source/platform/weborigin/SecurityOrigin.h
@@ -98,6 +98,11 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
     // familiar with Suborigins, you probably want canAccess() for now.
     // Suborigins is a spec in progress, and where it should be enforced is
     // still in flux. See https://crbug.com/336894 for more details.
+    //
+    // TODO(jww): Once the Suborigin spec has become more settled, and we are
+    // confident in the correctness of our implementation, canAccess should be
+    // made to check the suborigin and this should be turned into
+    // canAccessBypassSuborigin check, which should be the exceptional case.
     bool canAccessCheckSuborigins(const SecurityOrigin*) const;
 
     // Returns true if this SecurityOrigin can read content retrieved from
@@ -111,6 +116,11 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
     // Suborigins, you probably want canRequest() for now. Suborigins is a spec
     // in progress, and where it should be enforced is still in flux. See
     // https://crbug.com/336894 for more details.
+    //
+    // TODO(jww): Once the Suborigin spec has become more settled, and we are
+    // confident in the correctness of our implementation, canRequest should be
+    // made to check the suborigin and this should be turned into
+    // canRequestBypassSuborigin check, which should be the exceptional case.
     bool canRequestNoSuborigin(const KURL&) const;
 
     // Returns true if drawing an image from this URL taints a canvas from