OOR-CORS: Port WebCORSPreflightResultCacheItem to network service

This patch reimplements WebCORSPreflightResultCacheItem equivalent
in network service, and share the implementation between the network
service and Blink. The next patch will replace whole
WebCORSPreflightResultCache, and this is the preparation to make
the next patch small.

Bug: 803766
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ifa06967830465236dbe198908062e1952095e55b
Reviewed-on: https://chromium-review.googlesource.com/915543
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537769}

services/network/public/cpp/BUILD.gn

@@ -16,6 +16,8 @@ component("cpp") {
     "cors/cors_url_loader.h",
     "cors/cors_url_loader_factory.cc",
     "cors/cors_url_loader_factory.h",
+    "cors/preflight_result.cc",
+    "cors/preflight_result.h",
     "features.cc",
     "features.h",
     "mutable_network_traffic_annotation_tag_mojom_traits.h",
@@ -102,6 +104,7 @@ source_set("tests") {
 
   sources = [
     "cors/cors_unittest.cc",
+    "cors/preflight_result_unittest.cc",
     "mutable_network_traffic_annotation_tag_mojom_traits_unittest.cc",
     "mutable_partial_network_traffic_annotation_tag_mojom_traits_unittest.cc",
     "network_mojom_traits_unittest.cc",

services/network/public/cpp/cors/preflight_result.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/network/public/cpp/cors/preflight_result.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/time/default_tick_clock.h"
+#include "base/time/tick_clock.h"
+#include "base/time/time.h"
+#include "net/http/http_request_headers.h"
+#include "services/network/public/cpp/cors/cors.h"
+
+namespace network {
+
+namespace cors {
+
+namespace {
+
+// Timeout values below are at the discretion of the user agent.
+
+// Default cache expiry time for an entry that does not have
+// Access-Control-Max-Age header in its CORS-preflight response.
+constexpr base::TimeDelta kDefaultTimeout = base::TimeDelta::FromSeconds(5);
+
+// Maximum cache expiry time. Even if a CORS-preflight response contains
+// Access-Control-Max-Age header that specifies a longer expiry time, this
+// maximum time is applied.
+//
+// Note: Should be short enough to minimize the risk of using a poisoned cache
+// after switching to a secure network.
+// TODO(toyoshim): Consider to invalidate all entries when network configuration
+// is changed. See also discussion at https://crbug.com/131368.
+constexpr base::TimeDelta kMaxTimeout = base::TimeDelta::FromSeconds(600);
+
+// Holds TickClock instance to overwrite TimeTicks::Now() for testing.
+base::TickClock* tick_clock_for_testing = nullptr;
+
+base::TimeTicks Now() {
+  if (tick_clock_for_testing)
+    return tick_clock_for_testing->NowTicks();
+  return base::TimeTicks::Now();
+}
+
+bool ParseAccessControlMaxAge(const base::Optional<std::string>& max_age,
+                              base::TimeDelta* expiry_delta) {
+  DCHECK(expiry_delta);
+
+  if (!max_age)
+    return false;
+
+  uint64_t delta;
+  if (!base::StringToUint64(*max_age, &delta))
+    return false;
+
+  *expiry_delta = base::TimeDelta::FromSeconds(delta);
+  if (*expiry_delta > kMaxTimeout)
+    *expiry_delta = kMaxTimeout;
+  return true;
+}
+
+// At this moment, this function always succeeds.
+bool ParseAccessControlAllowList(const base::Optional<std::string>& string,
+                                 base::flat_set<std::string>* set,
+                                 bool insert_in_lower_case) {
+  DCHECK(set);
+
+  if (!string)
+    return true;
+
+  for (const auto& value : base::SplitString(
+           *string, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
+    // TODO(toyoshim): Strict ABNF header field checks want to be applied, e.g.
+    // strict VCHAR check of RFC-7230.
+    set->insert(insert_in_lower_case ? base::ToLowerASCII(value) : value);
+  }
+  return true;
+}
+
+}  // namespace
+
+// static
+void PreflightResult::SetTickClockForTesting(base::TickClock* tick_clock) {
+  tick_clock_for_testing = tick_clock;
+}
+
+// static
+std::unique_ptr<PreflightResult> PreflightResult::Create(
+    const mojom::FetchCredentialsMode credentials_mode,
+    const base::Optional<std::string>& allow_methods_header,
+    const base::Optional<std::string>& allow_headers_header,
+    const base::Optional<std::string>& max_age_header,
+    base::Optional<mojom::CORSError>* detected_error) {
+  std::unique_ptr<PreflightResult> result =
+      base::WrapUnique(new PreflightResult(credentials_mode));
+  base::Optional<mojom::CORSError> error =
+      result->Parse(allow_methods_header, allow_headers_header, max_age_header);
+  if (error) {
+    if (detected_error)
+      *detected_error = error;
+    return nullptr;
+  }
+  return result;
+}
+
+PreflightResult::PreflightResult(
+    const mojom::FetchCredentialsMode credentials_mode)
+    : credentials_(credentials_mode == mojom::FetchCredentialsMode::kInclude) {}
+
+PreflightResult::~PreflightResult() = default;
+
+base::Optional<mojom::CORSError>
+PreflightResult::EnsureAllowedCrossOriginMethod(
+    const std::string& method) const {
+  // Request method is normalized to upper case, and comparison is performed in
+  // case-sensitive way, that means access control header should provide an
+  // upper case method list.
+  const std::string normalized_method = base::ToUpperASCII(method);
+  if (methods_.find(normalized_method) != methods_.end() ||
+      IsCORSSafelistedMethod(normalized_method)) {
+    return base::nullopt;
+  }
+
+  if (!credentials_ && methods_.find("*") != methods_.end())
+    return base::nullopt;
+
+  return mojom::CORSError::kMethodDisallowedByPreflightResponse;
+}
+
+base::Optional<mojom::CORSError>
+PreflightResult::EnsureAllowedCrossOriginHeaders(
+    const net::HttpRequestHeaders& headers,
+    std::string* detected_header) const {
+  if (!credentials_ && headers_.find("*") != headers_.end())
+    return base::nullopt;
+
+  for (const auto& header : headers.GetHeaderVector()) {
+    // Header list check is performed in case-insensitive way. Here, we have a
+    // parsed header list set in lower case, and search each header in lower
+    // case.
+    const std::string key = base::ToLowerASCII(header.key);
+    if (headers_.find(key) == headers_.end() &&
+        !IsCORSSafelistedHeader(key, header.value)) {
+      // Forbidden headers are forbidden to be used by JavaScript, and checked
+      // beforehand. But user-agents may add these headers internally, and it's
+      // fine.
+      if (IsForbiddenHeader(key))
+        continue;
+      if (detected_header)
+        *detected_header = header.key;
+      return mojom::CORSError::kHeaderDisallowedByPreflightResponse;
+    }
+  }
+  return base::nullopt;
+}
+
+bool PreflightResult::EnsureAllowedRequest(
+    mojom::FetchCredentialsMode credentials_mode,
+    const std::string& method,
+    const net::HttpRequestHeaders& headers) const {
+  if (absolute_expiry_time_ <= Now())
+    return false;
+
+  if (!credentials_ &&
+      credentials_mode == mojom::FetchCredentialsMode::kInclude) {
+    return false;
+  }
+
+  if (EnsureAllowedCrossOriginMethod(method))
+    return false;
+
+  if (EnsureAllowedCrossOriginHeaders(headers, nullptr))
+    return false;
+
+  return true;
+}
+
+base::Optional<mojom::CORSError> PreflightResult::Parse(
+    const base::Optional<std::string>& allow_methods_header,
+    const base::Optional<std::string>& allow_headers_header,
+    const base::Optional<std::string>& max_age_header) {
+  DCHECK(methods_.empty());
+  DCHECK(headers_.empty());
+
+  // Keeps parsed method case for case-sensitive search.
+  if (!ParseAccessControlAllowList(allow_methods_header, &methods_, false))
+    return mojom::CORSError::kInvalidAllowMethodsPreflightResponse;
+
+  // Holds parsed headers in lower case for case-insensitive search.
+  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;
+  }
+  absolute_expiry_time_ = Now() + expiry_delta;
+
+  return base::nullopt;
+}
+
+}  // namespace cors
+
+}  // namespace network

services/network/public/cpp/cors/preflight_result.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SERVICES_NETWORK_PUBLIC_CPP_CORS_PREFLIGHT_RESULT_H_
+#define SERVICES_NETWORK_PUBLIC_CPP_CORS_PREFLIGHT_RESULT_H_
+
+#include <memory>
+#include <string>
+
+#include "base/component_export.h"
+#include "base/containers/flat_set.h"
+#include "base/optional.h"
+#include "services/network/public/mojom/cors.mojom-shared.h"
+#include "services/network/public/mojom/fetch_api.mojom-shared.h"
+
+namespace base {
+class TickClock;
+}  // namespace base
+
+namespace net {
+class HttpRequestHeaders;
+}  // namespace net
+
+namespace network {
+
+namespace cors {
+
+// Holds CORS-preflight request results, and provides access check methods.
+// Each instance can be cached by CORS-preflight cache.
+// See https://fetch.spec.whatwg.org/#concept-cache.
+class COMPONENT_EXPORT(NETWORK_CPP) PreflightResult final {
+ public:
+  static void SetTickClockForTesting(base::TickClock* tick_clock);
+
+  // Creates a PreflightResult instance from a CORS-preflight result. Returns
+  // nullptr and |detected_error| is populated with the failed reason if the
+  // passed parameters contain an invalid entry, and the pointer is valid.
+  static std::unique_ptr<PreflightResult> Create(
+      const mojom::FetchCredentialsMode credentials_mode,
+      const base::Optional<std::string>& allow_methods_header,
+      const base::Optional<std::string>& allow_headers_header,
+      const base::Optional<std::string>& max_age_header,
+      base::Optional<mojom::CORSError>* detected_error);
+  ~PreflightResult();
+
+  // Checks if the given |method| is allowed by the CORS-preflight response.
+  base::Optional<mojom::CORSError> EnsureAllowedCrossOriginMethod(
+      const std::string& method) const;
+
+  // Checks if the given all |headers| are allowed by the CORS-preflight
+  // response. |detected_header| indicates the disallowed header name if the
+  // pointer is valid.
+  // This does not reject when the headers contain forbidden headers
+  // (https://fetch.spec.whatwg.org/#forbidden-header-name) because they may be
+  // added by the user agent. They must be checked separately and rejected for
+  // JavaScript-initiated requests.
+  base::Optional<mojom::CORSError> EnsureAllowedCrossOriginHeaders(
+      const net::HttpRequestHeaders& headers,
+      std::string* detected_header) const;
+
+  // Checks if the given combination of |credentials_mode|, |method|, and
+  // |headers| is allowed by the CORS-preflight response.
+  // This also does not reject the forbidden headers as
+  // EnsureAllowCrossOriginHeaders does not.
+  bool EnsureAllowedRequest(mojom::FetchCredentialsMode credentials_mode,
+                            const std::string& method,
+                            const net::HttpRequestHeaders& headers) const;
+
+  // Refers the cache expiry time.
+  base::TimeTicks absolute_expiry_time() const { return absolute_expiry_time_; }
+
+ protected:
+  explicit PreflightResult(const mojom::FetchCredentialsMode credentials_mode);
+
+  base::Optional<mojom::CORSError> Parse(
+      const base::Optional<std::string>& allow_methods_header,
+      const base::Optional<std::string>& allow_headers_header,
+      const base::Optional<std::string>& max_age_header);
+
+ private:
+  // Holds an absolute time when the result should be expired in the
+  // CORS-preflight cache.
+  base::TimeTicks absolute_expiry_time_;
+
+  // Corresponds to the fields of the CORS-preflight cache with the same name in
+  // the fetch spec.
+  // |headers_| holds strings in lower case for case-insensitive search.
+  bool credentials_;
+  base::flat_set<std::string> methods_;
+  base::flat_set<std::string> headers_;
+
+  DISALLOW_COPY_AND_ASSIGN(PreflightResult);
+};
+
+}  // namespace cors
+
+}  // namespace network
+
+#endif  // SERVICES_NETWORK_PUBLIC_CPP_CORS_PREFLIGHT_RESULT_H_

services/network/public/mojom/cors.mojom

@@ -30,6 +30,20 @@ enum CORSError {
   kPreflightMissingAllowExternal,
   kPreflightInvalidAllowExternal,
 
+  // Failed to parse Access-Control-Allow-Methods response header field in
+  // CORS-preflight response.
+  kInvalidAllowMethodsPreflightResponse,
+
+  // Failed to parse Access-Control-Allow-Headers response header field in
+  // CORS-preflight response.
+  kInvalidAllowHeadersPreflightResponse,
+
+  // Not allowed by Access-Control-Allow-Methods in CORS-preflight response.
+  kMethodDisallowedByPreflightResponse,
+
+  // Not allowed by Access-Control-Allow-Headers in CORS-preflight response.
+  kHeaderDisallowedByPreflightResponse,
+
   // Redirect
   kRedirectDisallowedScheme,
   kRedirectContainsCredentials,

third_party/WebKit/LayoutTests/external/wpt/cors/preflight-cache-expected.txt

-This is a testharness.js-based test.
-PASS Test preflight
-PASS preflight for x-print should be cached
-PASS age = 0, should not be cached
-FAIL age = -1, should not be cached assert_equals: did preflight expected "1" but got "0"
-PASS preflight first request, second from cache, wait, third should preflight again
-Harness: the test ran to completion.
-

third_party/WebKit/LayoutTests/external/wpt/fetch/api/cors/cors-preflight-star.any.js

@@ -32,13 +32,18 @@ function preflightTest(succeeds, withCredentials, allowMethod, allowHeader, useM
   }, "CORS that " + (succeeds ? "succeeds" : "fails") + " with credentials: " + withCredentials + "; method: " + useMethod + " (allowed: " + allowMethod + "); header: " + useHeader + " (allowed: " + allowHeader + ")")
 }
 
+// "GET" does not pass the case-sensitive method check, but in the safe list.
 preflightTest(true, false, "get", "x-test", "GET", ["X-Test", "1"])
+// Headers check is case-insensitive, and "*" works as any for method.
 preflightTest(true, false, "*", "x-test", "SUPER", ["X-Test", "1"])
+// "*" works as any only without credentials.
 preflightTest(true, false, "*", "*", "OK", ["X-Test", "1"])
 preflightTest(false, true, "*", "*", "OK", ["X-Test", "1"])
 preflightTest(false, true, "*", "", "PUT", [])
 preflightTest(true, true, "PUT", "*", "PUT", [])
-preflightTest(false, true, "put", "*", "PUT", [])
 preflightTest(false, true, "get", "*", "GET", ["X-Test", "1"])
 preflightTest(false, true, "*", "*", "GET", ["X-Test", "1"])
+// Exact character match works even for "*" with credentials.
 preflightTest(true, true, "*", "*", "*", ["*", "1"])
+// "PUT" does not pass the case-sensitive method check, and not in the safe list.
+preflightTest(false, true, "put", "*", "PUT", [])

third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp

@@ -866,24 +866,14 @@ void DocumentThreadableLoader::HandlePreflightResponse(
   }
 
   WebString access_control_error_description;
-  std::unique_ptr<WebCORSPreflightResultCacheItem> preflight_result =
-      WebCORSPreflightResultCacheItem::Create(
-          actual_request_.GetFetchCredentialsMode(),
-          response.HttpHeaderFields(), access_control_error_description);
-
-  if (!preflight_result ||
-      !preflight_result->AllowsCrossOriginMethod(
-          actual_request_.HttpMethod(), access_control_error_description) ||
-      !preflight_result->AllowsCrossOriginHeaders(
+  if (!WebCORSPreflightResultCache::Shared().EnsureResultAndMayAppendEntry(
+          response.HttpHeaderFields(), GetSecurityOrigin()->ToString(),
+          actual_request_.Url(), actual_request_.HttpMethod(),
           actual_request_.HttpHeaderFields(),
-          access_control_error_description)) {
+          actual_request_.GetFetchCredentialsMode(),
+          &access_control_error_description)) {
     HandlePreflightFailure(response.Url(), access_control_error_description);
-    return;
   }
-
-  WebCORSPreflightResultCache::Shared().AppendEntry(
-      GetSecurityOrigin()->ToString(), actual_request_.Url(),
-      std::move(preflight_result));
 }
 
 void DocumentThreadableLoader::ReportResponseReceived(

third_party/WebKit/Source/platform/exported/WebCORSPreflightResultCacheTest.cpp

@@ -4,6 +4,7 @@
 
 #include "public/platform/WebCORSPreflightResultCache.h"
 
+#include "base/strings/stringprintf.h"
 #include "base/test/simple_test_tick_clock.h"
 #include "platform/network/HTTPHeaderMap.h"
 #include "platform/testing/URLTestHelpers.h"
@@ -42,39 +43,11 @@ class TestWebCORSPreflightResultCache : public WebCORSPreflightResultCache {
 
 class WebCORSPreflightResultCacheTest : public ::testing::Test {
  protected:
-  std::unique_ptr<WebCORSPreflightResultCacheItem> CreateCacheItem(
-      const AtomicString allow_methods,
-      const AtomicString allow_headers,
-      network::mojom::FetchCredentialsMode credentials_mode,
-      const int max_age = -1) {
-    HTTPHeaderMap response_header;
-
-    if (!allow_methods.IsEmpty())
-      response_header.Set("Access-Control-Allow-Methods", allow_methods);
-
-    if (!allow_headers.IsEmpty())
-      response_header.Set("Access-Control-Allow-Headers", allow_headers);
-
-    if (max_age > -1) {
-      response_header.Set("Access-Control-Max-Age",
-                          AtomicString::Number(max_age));
-    }
-
-    WebString error_description;
-    std::unique_ptr<WebCORSPreflightResultCacheItem> item =
-        WebCORSPreflightResultCacheItem::Create(
-            credentials_mode, response_header, error_description, clock());
-
-    EXPECT_TRUE(item);
-
-    return item;
-  }
-
   base::SimpleTestTickClock* clock() { return &clock_; }
 
   // This is by no means a robust parser and works only for the headers strings
   // used in this tests.
-  HTTPHeaderMap ParseHeaderString(std::string headers) {
+  HTTPHeaderMap ParseHeaderString(const std::string& headers) {
     HTTPHeaderMap header_map;
     std::stringstream stream;
     stream.str(headers);
@@ -100,18 +73,21 @@ TEST_F(WebCORSPreflightResultCacheTest, CacheTimeout) {
   WebURL other_url = URLTestHelpers::ToKURL("http://www.test.com/B");
 
   test::TestWebCORSPreflightResultCache cache;
+  network::cors::PreflightResult::SetTickClockForTesting(clock());
 
   // Cache should be empty:
   EXPECT_EQ(0, cache.CacheSize());
 
   cache.AppendEntry(
       origin, url,
-      CreateCacheItem("POST", "",
-                      network::mojom::FetchCredentialsMode::kInclude, 5));
+      network::cors::PreflightResult::Create(
+          network::mojom::FetchCredentialsMode::kInclude, std::string("POST"),
+          base::nullopt, std::string("5"), nullptr));
   cache.AppendEntry(
       origin, other_url,
-      CreateCacheItem("POST", "",
-                      network::mojom::FetchCredentialsMode::kInclude, 5));
+      network::cors::PreflightResult::Create(
+          network::mojom::FetchCredentialsMode::kInclude, std::string("POST"),
+          base::nullopt, std::string("5"), nullptr));
 
   // Cache size should be 3 (counting origins and urls separately):
   EXPECT_EQ(3, cache.CacheSize());
@@ -141,6 +117,8 @@ TEST_F(WebCORSPreflightResultCacheTest, CacheTimeout) {
   // Cache size should be 0, with the expired entry removed by call to
   // CanSkipPreflight():
   EXPECT_EQ(0, cache.CacheSize());
+
+  network::cors::PreflightResult::SetTickClockForTesting(nullptr);
 }
 
 TEST_F(WebCORSPreflightResultCacheTest, CacheSize) {
@@ -156,35 +134,38 @@ TEST_F(WebCORSPreflightResultCacheTest, CacheSize) {
 
   cache.AppendEntry(
       origin, url,
-      CreateCacheItem("POST", "",
-                      network::mojom::FetchCredentialsMode::kInclude));
+      network::cors::PreflightResult::Create(
+          network::mojom::FetchCredentialsMode::kInclude, std::string("POST"),
+          base::nullopt, base::nullopt, nullptr));
 
   // Cache size should be 2 (counting origins and urls separately):
   EXPECT_EQ(2, cache.CacheSize());
 
   cache.AppendEntry(
       origin, other_url,
-      CreateCacheItem("POST", "",
-                      network::mojom::FetchCredentialsMode::kInclude));
+      network::cors::PreflightResult::Create(
+          network::mojom::FetchCredentialsMode::kInclude, std::string("POST"),
+          base::nullopt, base::nullopt, nullptr));
 
   // Cache size should now be 3 (1 origin, 2 urls):
   EXPECT_EQ(3, cache.CacheSize());
 
   cache.AppendEntry(
       other_origin, url,
-      CreateCacheItem("POST", "",
-                      network::mojom::FetchCredentialsMode::kInclude));
+      network::cors::PreflightResult::Create(
+          network::mojom::FetchCredentialsMode::kInclude, std::string("POST"),
+          base::nullopt, base::nullopt, nullptr));
   // Cache size should now be 4 (4 origin, 3 urls):
   EXPECT_EQ(5, cache.CacheSize());
 }
 
 TEST_F(WebCORSPreflightResultCacheTest, CanSkipPreflight) {
   const struct {
-    const AtomicString allow_methods;
-    const AtomicString allow_headers;
+    const std::string allow_methods;
+    const std::string allow_headers;
     const network::mojom::FetchCredentialsMode cache_credentials_mode;
 
-    const AtomicString request_method;
+    const std::string request_method;
     const std::string request_headers;
     const network::mojom::FetchCredentialsMode request_credentials_mode;
 
@@ -248,7 +229,7 @@ TEST_F(WebCORSPreflightResultCacheTest, CanSkipPreflight) {
 
       // Credential mode mismatch:
       {"GET", "", network::mojom::FetchCredentialsMode::kOmit, "GET", "",
-       network::mojom::FetchCredentialsMode::kInclude, false},
+       network::mojom::FetchCredentialsMode::kOmit, true},
       {"GET", "", network::mojom::FetchCredentialsMode::kOmit, "GET", "",
        network::mojom::FetchCredentialsMode::kInclude, false},
   };
@@ -264,9 +245,10 @@ TEST_F(WebCORSPreflightResultCacheTest, CanSkipPreflight) {
     WebString origin("null");
     WebURL url = URLTestHelpers::ToKURL("http://www.test.com/");
 
-    std::unique_ptr<WebCORSPreflightResultCacheItem> item = CreateCacheItem(
-        test.allow_methods, test.allow_headers, test.cache_credentials_mode);
-
+    std::unique_ptr<network::cors::PreflightResult> item =
+        network::cors::PreflightResult::Create(
+            test.cache_credentials_mode, test.allow_methods, test.allow_headers,
+            base::nullopt, nullptr);
     EXPECT_TRUE(item);
 
     test::TestWebCORSPreflightResultCache cache;
@@ -274,7 +256,8 @@ TEST_F(WebCORSPreflightResultCacheTest, CanSkipPreflight) {
     cache.AppendEntry(origin, url, std::move(item));
 
     EXPECT_EQ(cache.CanSkipPreflight(origin, url, test.request_credentials_mode,
-                                     test.request_method,
+                                     String(test.request_method.data(),
+                                            test.request_method.length()),
                                      ParseHeaderString(test.request_headers)),
               test.can_skip_preflight);
   }

third_party/WebKit/Source/platform/loader/cors/CORSErrorString.cpp

@@ -6,6 +6,7 @@
 
 #include "platform/network/HTTPHeaderMap.h"
 #include "platform/network/http_names.h"
+#include "platform/weborigin/SecurityOrigin.h"
 #include "platform/wtf/StdLibExtras.h"
 
 namespace blink {
@@ -26,10 +27,10 @@ bool IsInterestingStatusCode(int status_code) {
 }
 
 ErrorParameter CreateWrongParameter(network::mojom::CORSError error) {
-  return ErrorParameter(error, GetInvalidURL(), GetInvalidURL(),
-                        0 /* status_code */, HTTPHeaderMap(),
-                        *SecurityOrigin::CreateUnique(),
-                        WebURLRequest::kRequestContextUnspecified, true);
+  return ErrorParameter(
+      error, GetInvalidURL(), GetInvalidURL(), 0 /* status_code */,
+      HTTPHeaderMap(), *SecurityOrigin::CreateUnique(),
+      WebURLRequest::kRequestContextUnspecified, String(), true);
 }
 
 }  // namespace
@@ -44,7 +45,7 @@ ErrorParameter ErrorParameter::Create(
     const SecurityOrigin& origin,
     const WebURLRequest::RequestContext context) {
   return ErrorParameter(error, first_url, second_url, status_code, header_map,
-                        origin, context, false);
+                        origin, context, String(), false);
 }
 
 // static
@@ -53,17 +54,18 @@ ErrorParameter ErrorParameter::CreateForDisallowedByMode(
   return ErrorParameter(network::mojom::CORSError::kDisallowedByMode,
                         request_url, GetInvalidURL(), 0 /* status_code */,
                         HTTPHeaderMap(), *SecurityOrigin::CreateUnique(),
-                        WebURLRequest::kRequestContextUnspecified, false);
+                        WebURLRequest::kRequestContextUnspecified, String(),
+                        false);
 }
 
 // static
 ErrorParameter ErrorParameter::CreateForInvalidResponse(
     const KURL& request_url,
     const SecurityOrigin& origin) {
-  return ErrorParameter(network::mojom::CORSError::kInvalidResponse,
-                        request_url, GetInvalidURL(), 0 /* status_code */,
-                        HTTPHeaderMap(), origin,
-                        WebURLRequest::kRequestContextUnspecified, false);
+  return ErrorParameter(
+      network::mojom::CORSError::kInvalidResponse, request_url, GetInvalidURL(),
+      0 /* status_code */, HTTPHeaderMap(), origin,
+      WebURLRequest::kRequestContextUnspecified, String(), false);
 }
 
 // static
@@ -85,7 +87,7 @@ ErrorParameter ErrorParameter::CreateForAccessCheck(
     case network::mojom::CORSError::kDisallowCredentialsNotSetToTrue:
       return ErrorParameter(error, request_url, redirect_url,
                             response_status_code, response_header_map, origin,
-                            context, false);
+                            context, String(), false);
     default:
       NOTREACHED();
   }
@@ -98,7 +100,8 @@ ErrorParameter ErrorParameter::CreateForPreflightStatusCheck(
   return ErrorParameter(network::mojom::CORSError::kPreflightInvalidStatus,
                         GetInvalidURL(), GetInvalidURL(), response_status_code,
                         HTTPHeaderMap(), *SecurityOrigin::CreateUnique(),
-                        WebURLRequest::kRequestContextUnspecified, false);
+                        WebURLRequest::kRequestContextUnspecified, String(),
+                        false);
 }
 
 // static
@@ -108,16 +111,36 @@ ErrorParameter ErrorParameter::CreateForExternalPreflightCheck(
   switch (error) {
     case network::mojom::CORSError::kPreflightMissingAllowExternal:
     case network::mojom::CORSError::kPreflightInvalidAllowExternal:
-      return ErrorParameter(error, GetInvalidURL(), GetInvalidURL(),
-                            0 /* status_code */, response_header_map,
-                            *SecurityOrigin::CreateUnique(),
-                            WebURLRequest::kRequestContextUnspecified, false);
+      return ErrorParameter(
+          error, GetInvalidURL(), GetInvalidURL(), 0 /* status_code */,
+          response_header_map, *SecurityOrigin::CreateUnique(),
+          WebURLRequest::kRequestContextUnspecified, String(), false);
+    default:
+      NOTREACHED();
+  }
+  return CreateWrongParameter(error);
+}
+
+// static
+ErrorParameter ErrorParameter::CreateForPreflightResponseCheck(
+    const network::mojom::CORSError error,
+    const String& hint) {
+  switch (error) {
+    case network::mojom::CORSError::kInvalidAllowMethodsPreflightResponse:
+    case network::mojom::CORSError::kInvalidAllowHeadersPreflightResponse:
+    case network::mojom::CORSError::kMethodDisallowedByPreflightResponse:
+    case network::mojom::CORSError::kHeaderDisallowedByPreflightResponse:
+      return ErrorParameter(
+          error, GetInvalidURL(), GetInvalidURL(), 0 /* status_code */,
+          HTTPHeaderMap(), *SecurityOrigin::CreateUnique(),
+          WebURLRequest::kRequestContextUnspecified, hint, false);
     default:
       NOTREACHED();
   }
   return CreateWrongParameter(error);
 }
 
+// static
 ErrorParameter ErrorParameter::CreateForRedirectCheck(
     network::mojom::CORSError error,
     const KURL& request_url,
@@ -125,10 +148,10 @@ ErrorParameter ErrorParameter::CreateForRedirectCheck(
   switch (error) {
     case network::mojom::CORSError::kRedirectDisallowedScheme:
     case network::mojom::CORSError::kRedirectContainsCredentials:
-      return ErrorParameter(error, request_url, redirect_url,
-                            0 /* status_code */, HTTPHeaderMap(),
-                            *SecurityOrigin::CreateUnique(),
-                            WebURLRequest::kRequestContextUnspecified, false);
+      return ErrorParameter(
+          error, request_url, redirect_url, 0 /* status_code */,
+          HTTPHeaderMap(), *SecurityOrigin::CreateUnique(),
+          WebURLRequest::kRequestContextUnspecified, String(), false);
     default:
       NOTREACHED();
   }
@@ -142,6 +165,7 @@ ErrorParameter::ErrorParameter(const network::mojom::CORSError error,
                                const HTTPHeaderMap& header_map,
                                const SecurityOrigin& origin,
                                const WebURLRequest::RequestContext context,
+                               const String& hint,
                                bool unknown)
     : error(error),
       first_url(first_url),
@@ -150,6 +174,7 @@ ErrorParameter::ErrorParameter(const network::mojom::CORSError error,
       header_map(header_map),
       origin(origin),
       context(context),
+      hint(hint),
       unknown(unknown) {}
 
 String GetErrorString(const ErrorParameter& param) {
@@ -265,7 +290,7 @@ String GetErrorString(const ErrorParameter& param) {
           "Response for preflight has invalid HTTP status code %d.",
           param.status_code);
     case network::mojom::CORSError::kPreflightMissingAllowExternal:
-      return WebString(
+      return String(
           "No 'Access-Control-Allow-External' header was present in the "
           "preflight response for this external request (This is an "
           "experimental header which is defined in "
@@ -279,6 +304,24 @@ String GetErrorString(const ErrorParameter& param) {
           param.header_map.Get(HTTPNames::Access_Control_Allow_External)
               .Utf8()
               .data());
+    case network::mojom::CORSError::kInvalidAllowMethodsPreflightResponse:
+      return String(
+          "Cannot parse Access-Control-Allow-Methods response header field in "
+          "preflight response.");
+    case network::mojom::CORSError::kInvalidAllowHeadersPreflightResponse:
+      return String(
+          "Cannot parse Access-Control-Allow-Headers response header field in "
+          "preflight response.");
+    case network::mojom::CORSError::kMethodDisallowedByPreflightResponse:
+      return String::Format(
+          "Method %s is not allowed by Access-Control-Allow-Methods in "
+          "preflight response.",
+          param.hint.Utf8().data());
+    case network::mojom::CORSError::kHeaderDisallowedByPreflightResponse:
+      return String::Format(
+          "Request header field %s is not allowed by "
+          "Access-Control-Allow-Headers in preflight response.",
+          param.hint.Utf8().data());
     case network::mojom::CORSError::kRedirectDisallowedScheme:
       return String::Format(
           "%sRedirect location '%s' has a disallowed scheme for cross-origin "
@@ -293,7 +336,7 @@ String GetErrorString(const ErrorParameter& param) {
           param.second_url.GetString().Utf8().data());
   }
   NOTREACHED();
-  return WebString();
+  return String();
 }
 
 }  // namespace CORS

third_party/WebKit/Source/platform/loader/cors/CORSErrorString.h

@@ -7,14 +7,15 @@
 
 #include "base/macros.h"
 #include "platform/PlatformExport.h"
-#include "platform/network/HTTPHeaderMap.h"
 #include "platform/weborigin/KURL.h"
-#include "platform/weborigin/SecurityOrigin.h"
 #include "public/platform/WebURLRequest.h"
 #include "services/network/public/mojom/cors.mojom-shared.h"
 
 namespace blink {
 
+class HTTPHeaderMap;
+class SecurityOrigin;
+
 // CORS error strings related utility functions.
 namespace CORS {
 
@@ -59,6 +60,15 @@ struct PLATFORM_EXPORT ErrorParameter {
       const network::mojom::CORSError,
       const HTTPHeaderMap& response_header_map);
 
+  // Creates an ErrorParameter for an error that is related to CORS-preflight
+  // response checks.
+  // |hint| should contain a banned request method for
+  // kMethodDisallowedByPreflightResponse, a banned request header name for
+  // kHeaderDisallowedByPreflightResponse, or can be omitted for others.
+  static ErrorParameter CreateForPreflightResponseCheck(
+      const network::mojom::CORSError,
+      const String& hint);
+
   // Creates an ErrorParameter for CORS::CheckRedirectLocation() returns.
   static ErrorParameter CreateForRedirectCheck(network::mojom::CORSError,
                                                const KURL& request_url,
@@ -73,6 +83,7 @@ struct PLATFORM_EXPORT ErrorParameter {
                  const HTTPHeaderMap&,
                  const SecurityOrigin&,
                  const WebURLRequest::RequestContext,
+                 const String& hint,
                  bool unknown);
 
   // Members that this struct carries.
@@ -83,6 +94,7 @@ struct PLATFORM_EXPORT ErrorParameter {
   const HTTPHeaderMap& header_map;
   const SecurityOrigin& origin;
   const WebURLRequest::RequestContext context;
+  const String& hint;
 
   // Set to true when an ErrorParameter was created in a wrong way. Used in
   // GetErrorString() to be robust for coding errors.

third_party/WebKit/public/platform/DEPS

@@ -24,12 +24,15 @@ include_rules = [
     "+net/http",
     "+public/platform",
     "-public/web",
+    "+services/network/public/cpp/cors/cors_error_status.h",
+    "+services/network/public/cpp/cors/preflight_result.h",
+
     # Enforce to use mojom-shared.h in WebKit/public so that it can compile
     # inside and outside Blink.
-    "+services/network/public/cpp/cors/cors_error_status.h",
     "+services/network/public/mojom/cors.mojom-shared.h",
     "+services/network/public/mojom/fetch_api.mojom-shared.h",
     "+services/network/public/mojom/request_context_frame_type.mojom-shared.h",
+
     "+services/service_manager/public/mojom",
     "+third_party/skia",
     "+ui/gfx",

third_party/WebKit/public/platform/WebCORSPreflightResultCache.h

@@ -38,55 +38,10 @@
 #include "public/platform/WebURL.h"
 #include "public/platform/WebURLRequest.h"
 #include "public/platform/WebURLResponse.h"
-
-namespace base {
-class TickClock;
-}
+#include "services/network/public/cpp/cors/preflight_result.h"
 
 namespace blink {
 
-// Represents an entry of the CORS-preflight cache.
-// See https://fetch.spec.whatwg.org/#concept-cache.
-class BLINK_PLATFORM_EXPORT WebCORSPreflightResultCacheItem {
- public:
-  WebCORSPreflightResultCacheItem(const WebCORSPreflightResultCacheItem&) =
-      delete;
-  WebCORSPreflightResultCacheItem& operator=(
-      const WebCORSPreflightResultCacheItem&) = delete;
-
-  static std::unique_ptr<WebCORSPreflightResultCacheItem> Create(
-      const network::mojom::FetchCredentialsMode,
-      const WebHTTPHeaderMap&,
-      WebString& error_description,
-      base::TickClock* = nullptr);
-
-  bool AllowsCrossOriginMethod(const WebString& method,
-                               WebString& error_description) const;
-  bool AllowsCrossOriginHeaders(const WebHTTPHeaderMap&,
-                                WebString& error_description) const;
-  bool AllowsRequest(network::mojom::FetchCredentialsMode,
-                     const WebString& method,
-                     const WebHTTPHeaderMap& request_headers) const;
-
- private:
-  WebCORSPreflightResultCacheItem(network::mojom::FetchCredentialsMode,
-                                  base::TickClock*);
-
-  bool Parse(const WebHTTPHeaderMap& response_header,
-             WebString& error_description);
-
-  // FIXME: A better solution to holding onto the absolute expiration time might
-  // be to start a timer for the expiration delta that removes this from the
-  // cache when it fires.
-  base::TimeTicks absolute_expiry_time_;
-
-  // Corresponds to the fields of the CORS-preflight cache with the same name.
-  bool credentials_;
-  base::flat_set<std::string> methods_;
-  WebHTTPHeaderSet headers_;
-  base::TickClock* clock_;
-};
-
 class BLINK_PLATFORM_EXPORT WebCORSPreflightResultCache {
  public:
   WebCORSPreflightResultCache(const WebCORSPreflightResultCache&) = delete;
@@ -96,9 +51,26 @@ class BLINK_PLATFORM_EXPORT WebCORSPreflightResultCache {
   // Returns a WebCORSPreflightResultCache which is shared in the same thread.
   static WebCORSPreflightResultCache& Shared();
 
+  // TODO(toyoshim): Move to platform/loader/cors, as
+  // CORS::EnsurePreflightResultAndCacheOnSuccess when
+  // WebCORSPreflightResultCache is ported to network service.
+  bool EnsureResultAndMayAppendEntry(
+      const WebHTTPHeaderMap& response_header_map,
+      const WebString& origin,
+      const WebURL& request_url,
+      const WebString& request_method,
+      const WebHTTPHeaderMap& request_header_map,
+      network::mojom::FetchCredentialsMode request_credentials_mode,
+      WebString* error_description);
+
+  // TODO(toyoshim): Remove the following method that is used only for testing
+  // outside this class implementation.
   void AppendEntry(const WebString& origin,
                    const WebURL&,
-                   std::unique_ptr<WebCORSPreflightResultCacheItem>);
+                   std::unique_ptr<network::cors::PreflightResult>);
+
+  // TODO(toyoshim): Move to platform/loader/cors, as CORS::CanSkipPreflight
+  // when WebCORSPreflightResultCache is ported to network service.
   bool CanSkipPreflight(const WebString& origin,
                         const WebURL&,
                         network::mojom::FetchCredentialsMode,
@@ -113,10 +85,10 @@ class BLINK_PLATFORM_EXPORT WebCORSPreflightResultCache {
 
   typedef std::map<
       std::string,
-      std::map<std::string, std::unique_ptr<WebCORSPreflightResultCacheItem>>>
-      WebCORSPreflightResultHashMap;
+      std::map<std::string, std::unique_ptr<network::cors::PreflightResult>>>
+      PreflightResultHashMap;
 
-  WebCORSPreflightResultHashMap preflight_hash_map_;
+  PreflightResultHashMap preflight_hash_map_;
 };
 
 }  // namespace blink