From e9f67b8e8951bfbf64a83492151d6c42f9be51af Mon Sep 17 00:00:00 2001
From: Yutaka Hirano <yhirano@chromium.org>
Date: Tue, 16 Mar 2021 08:20:14 +0000
Subject: [PATCH] CORS preflight should use the default max-age on parse
 failure

- When failing to parse "access-control-max-age", use the default
  value (5 seconds) in stead of 0.
- Negative value is not a parse error.
- Make the parser robust against integer overflow.

Note the parsing logic is still underspecified. See
https://github.com/whatwg/fetch/issues/814.

Bug: 651743
Change-Id: I4c2dcb3812cf757235c57298dfe17a7e4fe2e489
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2755991
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863191}
---
 services/network/cors/preflight_result.cc     | 46 ++++++++++---------
 .../network/cors/preflight_result_unittest.cc |  3 --
 .../wpt/cors/preflight-cache-expected.txt     |  9 ----
 3 files changed, 24 insertions(+), 34 deletions(-)
 delete mode 100644 third_party/blink/web_tests/external/wpt/cors/preflight-cache-expected.txt

diff --git a/services/network/cors/preflight_result.cc b/services/network/cors/preflight_result.cc
index 198621dbd5270e..5c03958553929d 100644
--- a/services/network/cors/preflight_result.cc
+++ b/services/network/cors/preflight_result.cc
@@ -41,21 +41,32 @@ base::TimeTicks Now() {
   return base::TimeTicks::Now();
 }
 
-bool ParseAccessControlMaxAge(const base::Optional<std::string>& max_age,
-                              base::TimeDelta* expiry_delta) {
-  DCHECK(expiry_delta);
+base::TimeDelta ParseAccessControlMaxAge(
+    const base::Optional<std::string>& max_age) {
+  if (!max_age) {
+    return kDefaultTimeout;
+  }
 
-  if (!max_age)
-    return false;
+  int64_t seconds;
+  if (!base::StringToInt64(*max_age, &seconds)) {
+    return kDefaultTimeout;
+  }
 
-  uint64_t delta;
-  if (!base::StringToUint64(*max_age, &delta))
-    return false;
+  // Negative value doesn't make sense - use 0 instead, to represent that the
+  // entry cannot be cached.
+  if (seconds < 0) {
+    return base::TimeDelta();
+  }
+  // To avoid integer overflow, we compare seconds instead of comparing
+  // TimeDeltas.
+  static_assert(
+      kMaxTimeout == base::TimeDelta::FromSeconds(kMaxTimeout.InSeconds()),
+      "`kMaxTimeout` must be a multiple of one second.");
+  if (seconds >= kMaxTimeout.InSeconds()) {
+    return kMaxTimeout;
+  }
 
-  *expiry_delta = base::TimeDelta::FromSeconds(delta);
-  if (*expiry_delta > kMaxTimeout)
-    *expiry_delta = kMaxTimeout;
-  return true;
+  return base::TimeDelta::FromSeconds(seconds);
 }
 
 // Parses |string| as a Access-Control-Allow-* header value, storing the result
@@ -192,16 +203,7 @@ base::Optional<mojom::CorsError> PreflightResult::Parse(
   if (!ParseAccessControlAllowList(allow_headers_header, &headers_, true))
     return mojom::CorsError::kInvalidAllowHeadersPreflightResponse;
 
-  base::TimeDelta expiry_delta;
-  if (max_age_header) {
-    // Set expiry_delta to 0 on invalid Access-Control-Max-Age headers so to
-    // invalidate the entry immediately. CORS-preflight response should be still
-    // usable for the request that initiates the CORS-preflight.
-    if (!ParseAccessControlMaxAge(max_age_header, &expiry_delta))
-      expiry_delta = base::TimeDelta();
-  } else {
-    expiry_delta = kDefaultTimeout;
-  }
+  const base::TimeDelta expiry_delta = ParseAccessControlMaxAge(max_age_header);
   absolute_expiry_time_ = Now() + expiry_delta;
 
   return base::nullopt;
diff --git a/services/network/cors/preflight_result_unittest.cc b/services/network/cors/preflight_result_unittest.cc
index 8b8a3d2820edb6..8eaef937590df1 100644
--- a/services/network/cors/preflight_result_unittest.cc
+++ b/services/network/cors/preflight_result_unittest.cc
@@ -177,9 +177,6 @@ TEST_F(PreflightResultTest, MaxAge) {
   EXPECT_EQ(base::TimeTicks() + base::TimeDelta::FromSeconds(573),
             result1->absolute_expiry_time());
 
-  // Negative values are invalid. The preflight result itself can be usable, but
-  // should not cache such results. PreflightResult expresses it as a result
-  // with 'Access-Control-Max-Age: 0'.
   std::unique_ptr<PreflightResult> result2 =
       PreflightResult::Create(mojom::CredentialsMode::kOmit, base::nullopt,
                               base::nullopt, std::string("-765"), nullptr);
diff --git a/third_party/blink/web_tests/external/wpt/cors/preflight-cache-expected.txt b/third_party/blink/web_tests/external/wpt/cors/preflight-cache-expected.txt
deleted file mode 100644
index d82bf39c8e51d2..00000000000000
--- a/third_party/blink/web_tests/external/wpt/cors/preflight-cache-expected.txt
+++ /dev/null
@@ -1,9 +0,0 @@
-This is a testharness.js-based test.
-PASS Test preflight
-PASS preflight for x-print should be cached
-FAIL age = blank, should be cached assert_equals: did preflight expected "0" but got "1"
-PASS age = 0, should not be cached
-PASS age = -1, should not be cached
-PASS preflight first request, second from cache, wait, third should preflight again
-Harness: the test ran to completion.
-