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
whatwg/fetch#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

@@ -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;

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);