Bug 1607615 - Allow CORS preflights with a default of 5 seconds for expiry if Access-Control-Max-Age hasn't been sent; r=mayhemer
authorEhsan Akhgari <ehsan@mozilla.com>
Tue, 14 Jan 2020 17:13:11 +0000
changeset 510211 cd769fb3a217024bea3100681e0907e07d1fdd4a
parent 510210 40bdfc030ef8cc0db850fb033cc83d082d96bc2b
child 510212 3ca301cd061fd96824213ad6eeccf633a10975eb
push id37014
push usercbrindusan@mozilla.com
push dateTue, 14 Jan 2020 21:43:07 +0000
treeherdermozilla-central@12d8255184b1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1607615
milestone74.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1607615 - Allow CORS preflights with a default of 5 seconds for expiry if Access-Control-Max-Age hasn't been sent; r=mayhemer The default expiry value is chosen based on what Chromium uses: https://source.chromium.org/chromium/chromium/src/+/master:services/network/public/cpp/cors/preflight_result.cc;l=27;drc=529117e5ed802c91a5cf192a72b4097d27fcb928?originalUrl=https:%2F%2Fcs.chromium.org%2F Differential Revision: https://phabricator.services.mozilla.com/D59032
dom/security/test/cors/test_CrossSiteXHR_cache.html
netwerk/protocol/http/nsCORSListenerProxy.cpp
testing/web-platform/meta/cors/preflight-cache.htm.ini
testing/web-platform/tests/cors/preflight-cache.htm
--- a/dom/security/test/cors/test_CrossSiteXHR_cache.html
+++ b/dom/security/test/cors/test_CrossSiteXHR_cache.html
@@ -58,26 +58,41 @@ function* runTest() {
              headers: { "x-my-header": "myValue",
                         "y-my-header": "second" },
            },
            { pass: 1,
              method: "GET",
              headers: { "y-my-header": "hello" },
              allowHeaders: "y-my-header",
            },
-           { pass: 0,
+           { pass: 1,
              method: "GET",
              headers: { "y-my-header": "hello" },
            },
            { pass: 1,
              method: "GET",
              headers: { "y-my-header": "hello" },
              allowHeaders: "y-my-header,x-my-header",
              cacheTime: 3600,
            },
+           { pass: 0,
+             method: "GET",
+             headers: { "x-my-header": "myValue",
+                        "y-my-header": "second" },
+           },
+           { newTest: "*******" },
+           { pass: 1,
+             method: "GET",
+             headers: { "y-my-header": "hello" },
+             allowHeaders: "y-my-header,x-my-header",
+           },
+           { pass: 1,
+             method: "GET",
+             headers: { "y-my-header": "hello" },
+           },
            { pass: 1,
              method: "GET",
              headers: { "x-my-header": "myValue",
                         "y-my-header": "second" },
            },
            { newTest: "*******" },
            { pass: 0,
              method: "GET",
@@ -219,17 +234,17 @@ function* runTest() {
            },
            { pass: 0,
              method: "PATCH",
            },
            { pass: 1,
              method: "PATCH",
              allowMethods: "PATCH",
            },
-           { pass: 0,
+           { pass: 1,
              method: "PATCH",
            },
            { pass: 1,
              method: "PATCH",
              allowMethods: "PATCH",
              cacheTime: 3600,
            },
            { pass: 1,
@@ -237,16 +252,25 @@ function* runTest() {
            },
            { pass: 0,
              method: "DELETE",
            },
            { pass: 0,
              method: "PUT",
            },
            { newTest: "*******" },
+           { pass: 1,
+             method: "PATCH",
+             allowMethods: "PATCH",
+             cacheTime: 3600,
+           },
+           { pass: 1,
+             method: "PATCH",
+           },
+           { newTest: "*******" },
            { pass: 0,
              method: "DELETE",
            },
            { pass: 1,
              method: "DELETE",
              allowMethods: "DELETE",
              cacheTime: 2
            },
--- a/netwerk/protocol/http/nsCORSListenerProxy.cpp
+++ b/netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ -45,16 +45,18 @@
 #include "nsQueryObject.h"
 #include "mozilla/StaticPrefs_network.h"
 #include <algorithm>
 
 using namespace mozilla;
 using namespace mozilla::net;
 
 #define PREFLIGHT_CACHE_SIZE 100
+// 5 seconds is chosen to be compatible with Chromium.
+#define PREFLIGHT_DEFAULT_EXPIRY_SECONDS 5
 
 static void LogBlockedRequest(nsIRequest* aRequest, const char* aProperty,
                               const char16_t* aParam, uint32_t aBlockingReason,
                               nsIHttpChannel* aCreatingChannel) {
   nsresult rv = NS_OK;
 
   nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
 
@@ -1107,37 +1109,37 @@ NS_IMPL_ISUPPORTS(nsCORSPreflightListene
                   nsIChannelEventSink)
 
 void nsCORSPreflightListener::AddResultToCache(nsIRequest* aRequest) {
   nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequest);
   NS_ASSERTION(http, "Request was not http");
 
   // The "Access-Control-Max-Age" header should return an age in seconds.
   nsAutoCString headerVal;
+  uint32_t age = 0;
   Unused << http->GetResponseHeader(
       NS_LITERAL_CSTRING("Access-Control-Max-Age"), headerVal);
   if (headerVal.IsEmpty()) {
-    return;
-  }
-
-  // Sanitize the string. We only allow 'delta-seconds' as specified by
-  // http://dev.w3.org/2006/waf/access-control (digits 0-9 with no leading or
-  // trailing non-whitespace characters).
-  uint32_t age = 0;
-  nsACString::const_char_iterator iter, end;
-  headerVal.BeginReading(iter);
-  headerVal.EndReading(end);
-  while (iter != end) {
-    if (*iter < '0' || *iter > '9') {
-      return;
+    age = PREFLIGHT_DEFAULT_EXPIRY_SECONDS;
+  } else {
+    // Sanitize the string. We only allow 'delta-seconds' as specified by
+    // http://dev.w3.org/2006/waf/access-control (digits 0-9 with no leading or
+    // trailing non-whitespace characters).
+    nsACString::const_char_iterator iter, end;
+    headerVal.BeginReading(iter);
+    headerVal.EndReading(end);
+    while (iter != end) {
+      if (*iter < '0' || *iter > '9') {
+        return;
+      }
+      age = age * 10 + (*iter - '0');
+      // Cap at 24 hours. This also avoids overflow
+      age = std::min(age, 86400U);
+      ++iter;
     }
-    age = age * 10 + (*iter - '0');
-    // Cap at 24 hours. This also avoids overflow
-    age = std::min(age, 86400U);
-    ++iter;
   }
 
   if (!age || !EnsurePreflightCache()) {
     return;
   }
 
   // String seems fine, go ahead and cache.
   // Note that we have already checked that these headers follow the correct
deleted file mode 100644
--- a/testing/web-platform/meta/cors/preflight-cache.htm.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[preflight-cache.htm]
-  [preflight for x-print should be cached]
-    expected: FAIL
-
--- a/testing/web-platform/tests/cors/preflight-cache.htm
+++ b/testing/web-platform/tests/cors/preflight-cache.htm
@@ -59,16 +59,25 @@ test(function() {
     did_preflight(false, client, {token: id})
 },
 'preflight for x-print should be cached')
 
 test(function() {
     var time = new Date().getTime()
     var client = new XMLHttpRequest()
 
+    var id = did_preflight(true, client, {extra:'max_age='})
+    did_preflight(false, client, {extra:'max_age=', token: id})
+},
+'age = blank, should be cached')
+
+test(function() {
+    var time = new Date().getTime()
+    var client = new XMLHttpRequest()
+
     var id = did_preflight(true, client, {extra:'max_age=0'})
     did_preflight(true, client, {extra:'max_age=0', token: id})
 },
 'age = 0, should not be cached')
 
 test(function() {
     var time = new Date().getTime()
     var client = new XMLHttpRequest()