[OOR-CORS] Have PreflightResult functions return CORSErrorStatus

...instead of mojom::CORSError. Currently they return a mojom::CORSError
and callers need to compose a CORSErrorStatus from it. That requires
callers to know which specific errors are returned from the function,
which is not good.

Bug: 736308
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I5b0dfaf5b906e0203c9a72ebaef759a8fafa4ad3
Reviewed-on: https://chromium-review.googlesource.com/1147681Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577443}

services/network/cors/preflight_controller.cc

@@ -169,18 +169,12 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
 base::Optional<CORSErrorStatus> CheckPreflightResult(
     PreflightResult* result,
     const ResourceRequest& original_request) {
-  base::Optional<mojom::CORSError> error =
+  base::Optional<CORSErrorStatus> status =
       result->EnsureAllowedCrossOriginMethod(original_request.method);
-  if (error)
-    return CORSErrorStatus(*error, original_request.method);
-
-  std::string detected_error_header;
-  error = result->EnsureAllowedCrossOriginHeaders(original_request.headers,
-                                                  &detected_error_header);
-  if (error)
-    return CORSErrorStatus(*error, detected_error_header);
+  if (status)
+    return status;
 
-  return base::nullopt;
+  return result->EnsureAllowedCrossOriginHeaders(original_request.headers);
 }
 
 // TODO(toyoshim): Remove this class once the Network Service is enabled.

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

@@ -112,8 +112,7 @@ PreflightResult::PreflightResult(
 
 PreflightResult::~PreflightResult() = default;
 
-base::Optional<mojom::CORSError>
-PreflightResult::EnsureAllowedCrossOriginMethod(
+base::Optional<CORSErrorStatus> 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
@@ -127,13 +126,13 @@ PreflightResult::EnsureAllowedCrossOriginMethod(
   if (!credentials_ && methods_.find("*") != methods_.end())
     return base::nullopt;
 
-  return mojom::CORSError::kMethodDisallowedByPreflightResponse;
+  return CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                         method);
 }
 
-base::Optional<mojom::CORSError>
+base::Optional<CORSErrorStatus>
 PreflightResult::EnsureAllowedCrossOriginHeaders(
-    const net::HttpRequestHeaders& headers,
-    std::string* detected_header) const {
+    const net::HttpRequestHeaders& headers) const {
   if (!credentials_ && headers_.find("*") != headers_.end())
     return base::nullopt;
 
@@ -149,9 +148,8 @@ PreflightResult::EnsureAllowedCrossOriginHeaders(
       // fine.
       if (IsForbiddenHeader(key))
         continue;
-      if (detected_header)
-        *detected_header = header.key;
-      return mojom::CORSError::kHeaderDisallowedByPreflightResponse;
+      return CORSErrorStatus(
+          mojom::CORSError::kHeaderDisallowedByPreflightResponse, header.key);
     }
   }
   return base::nullopt;
@@ -172,7 +170,7 @@ bool PreflightResult::EnsureAllowedRequest(
   if (EnsureAllowedCrossOriginMethod(method))
     return false;
 
-  if (EnsureAllowedCrossOriginHeaders(headers, nullptr))
+  if (EnsureAllowedCrossOriginHeaders(headers))
     return false;
 
   return true;

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

@@ -11,6 +11,7 @@
 #include "base/component_export.h"
 #include "base/containers/flat_set.h"
 #include "base/optional.h"
+#include "services/network/public/cpp/cors/cors_error_status.h"
 #include "services/network/public/mojom/cors.mojom-shared.h"
 #include "services/network/public/mojom/fetch_api.mojom-shared.h"
 
@@ -45,19 +46,17 @@ class COMPONENT_EXPORT(NETWORK_CPP) PreflightResult final {
   ~PreflightResult();
 
   // Checks if the given |method| is allowed by the CORS-preflight response.
-  base::Optional<mojom::CORSError> EnsureAllowedCrossOriginMethod(
+  base::Optional<CORSErrorStatus> 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.
+  // response.
   // 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;
+  base::Optional<CORSErrorStatus> EnsureAllowedCrossOriginHeaders(
+      const net::HttpRequestHeaders& headers) const;
 
   // Checks if the given combination of |credentials_mode|, |method|, and
   // |headers| is allowed by the CORS-preflight response.

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

@@ -26,7 +26,7 @@ struct TestCase {
   const std::string request_headers;
   const mojom::FetchCredentialsMode request_credentials_mode;
 
-  const base::Optional<mojom::CORSError> expected_result;
+  const base::Optional<CORSErrorStatus> expected_result;
 };
 
 const TestCase method_cases[] = {
@@ -67,29 +67,36 @@ const TestCase method_cases[] = {
     // Not found in the preflight response and the safe lit.
     {"", "", mojom::FetchCredentialsMode::kOmit, "OPTIONS", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "OPTIONS")},
     {"", "", mojom::FetchCredentialsMode::kOmit, "PUT", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "PUT")},
     {"", "", mojom::FetchCredentialsMode::kOmit, "DELETE", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "DELETE")},
     {"GET", "", mojom::FetchCredentialsMode::kOmit, "PUT", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "PUT")},
     {"GET, POST, DELETE", "", mojom::FetchCredentialsMode::kOmit, "PUT", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "PUT")},
 
     // Request method is normalized to upper-case, but allowed methods is not.
     // Comparison is in case-sensitive, that means allowed methods should be in
     // upper case.
     {"put", "", mojom::FetchCredentialsMode::kOmit, "PUT", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "PUT")},
     {"put", "", mojom::FetchCredentialsMode::kOmit, "put", "",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kMethodDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kMethodDisallowedByPreflightResponse,
+                     "put")},
     {"PUT", "", mojom::FetchCredentialsMode::kOmit, "put", "",
      mojom::FetchCredentialsMode::kOmit, base::nullopt},
     // ... But, GET is always allowed by the safe list.
@@ -117,7 +124,8 @@ const TestCase header_cases[] = {
      mojom::FetchCredentialsMode::kOmit, base::nullopt},
     {"GET", "*", mojom::FetchCredentialsMode::kInclude, "GET", "xyzzy:t",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kHeaderDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
+                     "xyzzy")},
 
     // Forbidden headers can pass.
     {"GET", "", mojom::FetchCredentialsMode::kOmit, "GET",
@@ -126,13 +134,16 @@ const TestCase header_cases[] = {
     // Not found in the preflight response and the safe list.
     {"GET", "", mojom::FetchCredentialsMode::kOmit, "GET", "X-MY-HEADER:t",
      mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kHeaderDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
+                     "X-MY-HEADER")},
     {"GET", "X-SOME-OTHER-HEADER", mojom::FetchCredentialsMode::kOmit, "GET",
      "X-MY-HEADER:t", mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kHeaderDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
+                     "X-MY-HEADER")},
     {"GET", "X-MY-HEADER", mojom::FetchCredentialsMode::kOmit, "GET",
      "X-MY-HEADER:t\r\nY-MY-HEADER:t", mojom::FetchCredentialsMode::kOmit,
-     mojom::CORSError::kHeaderDisallowedByPreflightResponse},
+     CORSErrorStatus(mojom::CORSError::kHeaderDisallowedByPreflightResponse,
+                     "Y-MY-HEADER")},
 };
 
 TEST_F(PreflightResultTest, MaxAge) {
@@ -177,7 +188,7 @@ TEST_F(PreflightResultTest, EnsureHeaders) {
     net::HttpRequestHeaders headers;
     headers.AddHeadersFromString(test.request_headers);
     EXPECT_EQ(test.expected_result,
-              result->EnsureAllowedCrossOriginHeaders(headers, nullptr));
+              result->EnsureAllowedCrossOriginHeaders(headers));
   }
 }
 

third_party/blink/renderer/platform/loader/cors/cors.cc

@@ -146,6 +146,7 @@ bool EnsurePreflightResultAndCacheOnSuccess(
   DCHECK(error_description);
 
   base::Optional<network::mojom::CORSError> error;
+  base::Optional<network::CORSErrorStatus> status;
 
   std::unique_ptr<network::cors::PreflightResult> result =
       network::cors::PreflightResult::Create(
@@ -164,23 +165,22 @@ bool EnsurePreflightResultAndCacheOnSuccess(
     return false;
   }
 
-  error = result->EnsureAllowedCrossOriginMethod(
+  status = result->EnsureAllowedCrossOriginMethod(
       std::string(request_method.Ascii().data()));
-  if (error) {
+  if (status) {
     *error_description = CORS::GetErrorString(
-        CORS::ErrorParameter::CreateForPreflightResponseCheck(*error,
-                                                              request_method));
+        CORS::ErrorParameter::CreateForPreflightResponseCheck(
+            status->cors_error, request_method));
     return false;
   }
 
-  std::string detected_error_header;
-  error = result->EnsureAllowedCrossOriginHeaders(
-      *CreateNetHttpRequestHeaders(request_header_map), &detected_error_header);
-  if (error) {
+  status = result->EnsureAllowedCrossOriginHeaders(
+      *CreateNetHttpRequestHeaders(request_header_map));
+  if (status) {
     *error_description = CORS::GetErrorString(
         CORS::ErrorParameter::CreateForPreflightResponseCheck(
-            *error, String(detected_error_header.data(),
-                           detected_error_header.length())));
+            status->cors_error, String(status->failed_parameter.data(),
+                                       status->failed_parameter.length())));
     return false;
   }