OOR-CORS: Modifies CORS::GetErrorString to take a unified struct

CORS::GetErrorString takes multiple arguments, and required parameters
depend on error kinds. This will make developers who want to call it
be confused, when we add more parameters, we need to modify all call
sites.

This patch introduces CORS::ErrorParameter to unify parameters for
GetErrorString. This solves problems mentioned above, and also allows
us to have better coding error checks.

Once the OOR migration is done, we will be able to remove error specific
ErrorParameter builders, or can even remove ErrorParameter again because
we can remove the most intermediate call sites from Blink.

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

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

@@ -53,6 +53,7 @@ base::Optional<mojom::CORSError> CheckRedirectLocation(const GURL& redirect_url,
 
 // Performs the required CORS checks on the response to a preflight request.
 // Returns |kPreflightSuccess| if preflight response was successful.
+// TODO(toyoshim): Rename to CheckPreflightStatus.
 COMPONENT_EXPORT(NETWORK_CPP)
 base::Optional<mojom::CORSError> CheckPreflight(const int status_code);
 

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

@@ -50,6 +50,7 @@
 #include "platform/SharedBuffer.h"
 #include "platform/exported/WrappedResourceRequest.h"
 #include "platform/loader/cors/CORS.h"
+#include "platform/loader/cors/CORSErrorString.h"
 #include "platform/loader/fetch/FetchParameters.h"
 #include "platform/loader/fetch/Resource.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
@@ -282,10 +283,8 @@ void DocumentThreadableLoader::StartBlinkCORS(const ResourceRequest& request) {
     Clear();
     ResourceError error = ResourceError::CancelledDueToAccessCheckError(
         request.Url(), ResourceRequestBlockedReason::kOther,
-        CORS::GetErrorString(network::mojom::CORSError::kDisallowedByMode,
-                             request.Url(), KURL(),
-                             0 /* response_status_code */, HTTPHeaderMap(),
-                             *GetSecurityOrigin(), request_context_));
+        CORS::GetErrorString(
+            CORS::ErrorParameter::CreateForDisallowedByMode(request.Url())));
     GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(
         kJSMessageSource, kErrorMessageLevel, error.LocalizedDescription()));
     client->DidFail(error);
@@ -690,10 +689,8 @@ bool DocumentThreadableLoader::RedirectReceivedBlinkCORS(
     DispatchDidFailAccessControlCheck(
         ResourceError::CancelledDueToAccessCheckError(
             original_url, ResourceRequestBlockedReason::kOther,
-            CORS::GetErrorString(*redirect_error, original_url, new_url,
-                                 redirect_response.HttpStatusCode(),
-                                 redirect_response.HttpHeaderFields(),
-                                 *GetSecurityOrigin(), request_context_)));
+            CORS::GetErrorString(CORS::ErrorParameter::CreateForRedirectCheck(
+                *redirect_error, original_url, new_url))));
     return false;
   }
 
@@ -708,10 +705,11 @@ bool DocumentThreadableLoader::RedirectReceivedBlinkCORS(
       DispatchDidFailAccessControlCheck(
           ResourceError::CancelledDueToAccessCheckError(
               original_url, ResourceRequestBlockedReason::kOther,
-              CORS::GetErrorString(*access_error, original_url, new_url,
-                                   redirect_response.HttpStatusCode(),
-                                   redirect_response.HttpHeaderFields(),
-                                   *GetSecurityOrigin(), request_context_)));
+              CORS::GetErrorString(CORS::ErrorParameter::CreateForAccessCheck(
+                  *access_error, original_url,
+                  redirect_response.HttpStatusCode(),
+                  redirect_response.HttpHeaderFields(), *GetSecurityOrigin(),
+                  request_context_, new_url))));
       return false;
     }
   }
@@ -835,9 +833,11 @@ void DocumentThreadableLoader::HandlePreflightResponse(
     builder.Append(
         "Response to preflight request doesn't pass access "
         "control check: ");
-    builder.Append(CORS::GetErrorString(
-        *cors_error, response.Url(), KURL(), response.HttpStatusCode(),
-        response.HttpHeaderFields(), *GetSecurityOrigin(), request_context_));
+    builder.Append(
+        CORS::GetErrorString(CORS::ErrorParameter::CreateForAccessCheck(
+            *cors_error, response.Url(), response.HttpStatusCode(),
+            response.HttpHeaderFields(), *GetSecurityOrigin(),
+            request_context_)));
     HandlePreflightFailure(response.Url(), builder.ToString());
     return;
   }
@@ -846,11 +846,9 @@ void DocumentThreadableLoader::HandlePreflightResponse(
       CORS::CheckPreflight(response.HttpStatusCode());
   if (preflight_error) {
     HandlePreflightFailure(
-        response.Url(),
-        CORS::GetErrorString(*preflight_error, response.Url(), KURL(),
-                             response.HttpStatusCode(),
-                             response.HttpHeaderFields(), *GetSecurityOrigin(),
-                             request_context_));
+        response.Url(), CORS::GetErrorString(
+                            CORS::ErrorParameter::CreateForPreflightStatusCheck(
+                                response.HttpStatusCode())));
     return;
   }
 
@@ -860,10 +858,9 @@ void DocumentThreadableLoader::HandlePreflightResponse(
     if (external_preflight_status) {
       HandlePreflightFailure(
           response.Url(),
-          CORS::GetErrorString(*external_preflight_status, response.Url(),
-                               KURL(), response.HttpStatusCode(),
-                               response.HttpHeaderFields(),
-                               *GetSecurityOrigin(), request_context_));
+          CORS::GetErrorString(
+              CORS::ErrorParameter::CreateForExternalPreflightCheck(
+                  *external_preflight_status, response.HttpHeaderFields())));
       return;
     }
   }
@@ -953,11 +950,9 @@ void DocumentThreadableLoader::HandleResponseBlinkCORS(
       DispatchDidFailAccessControlCheck(
           ResourceError::CancelledDueToAccessCheckError(
               response.Url(), ResourceRequestBlockedReason::kOther,
-              CORS::GetErrorString(network::mojom::CORSError::kInvalidResponse,
-                                   response.Url(), KURL(),
-                                   response.HttpStatusCode(),
-                                   response.HttpHeaderFields(),
-                                   *GetSecurityOrigin(), request_context_)));
+              CORS::GetErrorString(
+                  CORS::ErrorParameter::CreateForInvalidResponse(
+                      response.Url(), *GetSecurityOrigin()))));
       return;
     }
 
@@ -987,10 +982,10 @@ void DocumentThreadableLoader::HandleResponseBlinkCORS(
       DispatchDidFailAccessControlCheck(
           ResourceError::CancelledDueToAccessCheckError(
               response.Url(), ResourceRequestBlockedReason::kOther,
-              CORS::GetErrorString(*access_error, response.Url(), KURL(),
-                                   response.HttpStatusCode(),
-                                   response.HttpHeaderFields(),
-                                   *GetSecurityOrigin(), request_context_)));
+              CORS::GetErrorString(CORS::ErrorParameter::CreateForAccessCheck(
+                  *access_error, response.Url(), response.HttpStatusCode(),
+                  response.HttpHeaderFields(), *GetSecurityOrigin(),
+                  request_context_))));
       return;
     }
   }
@@ -1146,10 +1141,11 @@ void DocumentThreadableLoader::DispatchDidFail(const ResourceError& error) {
     GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(
         kJSMessageSource, kErrorMessageLevel,
         "Failed to load " + error.FailingURL() + ": " +
-            CORS::GetErrorString(error.CORSErrorStatus()->cors_error,
-                                 KURL(error.FailingURL()), KURL(),
-                                 response_code, HTTPHeaderMap(),
-                                 *GetSecurityOrigin(), request_context_)
+            CORS::GetErrorString(CORS::ErrorParameter::Create(
+                                     error.CORSErrorStatus()->cors_error,
+                                     KURL(error.FailingURL()), KURL(),
+                                     response_code, HTTPHeaderMap(),
+                                     *GetSecurityOrigin(), request_context_))
                 .Utf8()
                 .data()));
   }

third_party/WebKit/Source/platform/loader/BUILD.gn

@@ -20,6 +20,8 @@ blink_platform_sources("loader") {
     "SubresourceIntegrity.h",
     "cors/CORS.cpp",
     "cors/CORS.h",
+    "cors/CORSErrorString.cpp",
+    "cors/CORSErrorString.h",
     "fetch/AccessControlStatus.h",
     "fetch/BufferingDataPipeWriter.cpp",
     "fetch/BufferingDataPipeWriter.h",

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

@@ -4,6 +4,8 @@
 
 #include "platform/loader/cors/CORS.h"
 
+#include <string>
+
 #include "platform/network/HTTPHeaderMap.h"
 #include "platform/network/http_names.h"
 #include "platform/runtime_enabled_features.h"
@@ -18,12 +20,6 @@ namespace blink {
 
 namespace {
 
-bool IsInterestingStatusCode(int status_code) {
-  // Predicate that gates what status codes should be included in console error
-  // messages for responses containing no access control headers.
-  return status_code >= 400;
-}
-
 base::Optional<std::string> GetHeaderValue(const HTTPHeaderMap& header_map,
                                            const AtomicString& header_name) {
   if (header_map.Contains(header_name)) {
@@ -38,150 +34,6 @@ base::Optional<std::string> GetHeaderValue(const HTTPHeaderMap& header_map,
 
 namespace CORS {
 
-String GetErrorString(const network::mojom::CORSError error,
-                      const KURL& request_url,
-                      const KURL& redirect_url,
-                      const int response_status_code,
-                      const HTTPHeaderMap& response_header,
-                      const SecurityOrigin& origin,
-                      const WebURLRequest::RequestContext context) {
-  static const char kNoCorsInformation[] =
-      " Have the server send the header with a valid value, or, if an opaque "
-      "response serves your needs, set the request's mode to 'no-cors' to "
-      "fetch the resource with CORS disabled.";
-
-  String redirect_denied =
-      redirect_url.IsValid()
-          ? String::Format(
-                "Redirect from '%s' to '%s' has been blocked by CORS policy: ",
-                request_url.GetString().Utf8().data(),
-                redirect_url.GetString().Utf8().data())
-          : String();
-
-  switch (error) {
-    case network::mojom::CORSError::kDisallowedByMode:
-      return String::Format(
-          "Failed to load '%s': Cross origin requests are not allowed by "
-          "request mode.",
-          request_url.GetString().Utf8().data());
-    case network::mojom::CORSError::kInvalidResponse:
-      return String::Format(
-          "%sInvalid response. Origin '%s' is therefore not allowed access.",
-          redirect_denied.Utf8().data(), origin.ToString().Utf8().data());
-    case network::mojom::CORSError::kWildcardOriginNotAllowed:
-      return String::Format(
-          "%sThe value of the 'Access-Control-Allow-Origin' header in the "
-          "response must not be the wildcard '*' when the request's "
-          "credentials mode is 'include'. Origin '%s' is therefore not allowed "
-          "access.%s",
-          redirect_denied.Utf8().data(), origin.ToString().Utf8().data(),
-          context == WebURLRequest::kRequestContextXMLHttpRequest
-              ? " The credentials mode of requests initiated by the "
-                "XMLHttpRequest is controlled by the withCredentials attribute."
-              : "");
-    case network::mojom::CORSError::kMissingAllowOriginHeader:
-      return String::Format(
-          "%sNo 'Access-Control-Allow-Origin' header is present on the "
-          "requested resource. Origin '%s' is therefore not allowed access."
-          "%s%s",
-          redirect_denied.Utf8().data(), origin.ToString().Utf8().data(),
-          IsInterestingStatusCode(response_status_code)
-              ? String::Format(" The response had HTTP status code %d.",
-                               response_status_code)
-                    .Utf8()
-                    .data()
-              : "",
-          context == WebURLRequest::kRequestContextFetch
-              ? " If an opaque response serves your needs, set the request's "
-                "mode to 'no-cors' to fetch the resource with CORS disabled."
-              : "");
-    case network::mojom::CORSError::kMultipleAllowOriginValues:
-      return String::Format(
-          "%sThe 'Access-Control-Allow-Origin' header contains multiple values "
-          "'%s', but only one is allowed. Origin '%s' is therefore not allowed "
-          "access.%s",
-          redirect_denied.Utf8().data(),
-          response_header.Get(HTTPNames::Access_Control_Allow_Origin)
-              .Utf8()
-              .data(),
-          origin.ToString().Utf8().data(),
-          context == WebURLRequest::kRequestContextFetch ? kNoCorsInformation
-                                                         : "");
-    case network::mojom::CORSError::kInvalidAllowOriginValue:
-      return String::Format(
-          "%sThe 'Access-Control-Allow-Origin' header contains the invalid "
-          "value '%s'. Origin '%s' is therefore not allowed access.%s",
-          redirect_denied.Utf8().data(),
-          response_header.Get(HTTPNames::Access_Control_Allow_Origin)
-              .Utf8()
-              .data(),
-          origin.ToString().Utf8().data(),
-          context == WebURLRequest::kRequestContextFetch ? kNoCorsInformation
-                                                         : "");
-    case network::mojom::CORSError::kAllowOriginMismatch:
-      return String::Format(
-          "%sThe 'Access-Control-Allow-Origin' header has a value '%s' that is "
-          "not equal to the supplied origin. Origin '%s' is therefore not "
-          "allowed access.%s",
-          redirect_denied.Utf8().data(),
-          response_header.Get(HTTPNames::Access_Control_Allow_Origin)
-              .Utf8()
-              .data(),
-          origin.ToString().Utf8().data(),
-          context == WebURLRequest::kRequestContextFetch ? kNoCorsInformation
-                                                         : "");
-    case network::mojom::CORSError::kDisallowCredentialsNotSetToTrue:
-      return String::Format(
-          "%sThe value of the 'Access-Control-Allow-Credentials' header in "
-          "the response is '%s' which must be 'true' when the request's "
-          "credentials mode is 'include'. Origin '%s' is therefore not allowed "
-          "access.%s",
-          redirect_denied.Utf8().data(),
-          response_header.Get(HTTPNames::Access_Control_Allow_Credentials)
-              .Utf8()
-              .data(),
-          origin.ToString().Utf8().data(),
-          (context == WebURLRequest::kRequestContextXMLHttpRequest
-               ? " The credentials mode of requests initiated by the "
-                 "XMLHttpRequest is controlled by the withCredentials "
-                 "attribute."
-               : ""));
-    case network::mojom::CORSError::kPreflightInvalidStatus:
-      return String::Format(
-          "Response for preflight has invalid HTTP status code %d.",
-          response_status_code);
-    case network::mojom::CORSError::kPreflightMissingAllowExternal:
-      return WebString(
-          "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 "
-          "'https://wicg.github.io/cors-rfc1918/').");
-    case network::mojom::CORSError::kPreflightInvalidAllowExternal:
-      return String::Format(
-          "The 'Access-Control-Allow-External' header in the preflight "
-          "response for this external request had a value of '%s',  not 'true' "
-          "(This is an experimental header which is defined in "
-          "'https://wicg.github.io/cors-rfc1918/').",
-          response_header.Get(HTTPNames::Access_Control_Allow_External)
-              .Utf8()
-              .data());
-    case network::mojom::CORSError::kRedirectDisallowedScheme:
-      return String::Format(
-          "%sRedirect location '%s' has a disallowed scheme for cross-origin "
-          "requests.",
-          redirect_denied.Utf8().data(),
-          redirect_url.GetString().Utf8().data());
-    case network::mojom::CORSError::kRedirectContainsCredentials:
-      return String::Format(
-          "%sRedirect location '%s' contains a username and password, which is "
-          "disallowed for cross-origin requests.",
-          redirect_denied.Utf8().data(),
-          redirect_url.GetString().Utf8().data());
-  }
-  NOTREACHED();
-  return WebString();
-}
-
 WTF::Optional<network::mojom::CORSError> CheckAccess(
     const KURL& response_url,
     const int response_status_code,

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

@@ -5,12 +5,9 @@
 #ifndef CORS_h
 #define CORS_h
 
-#include <string>
-
 #include "platform/PlatformExport.h"
 #include "platform/wtf/Optional.h"
 #include "platform/wtf/text/WTFString.h"
-#include "public/platform/WebURLRequest.h"
 #include "services/network/public/mojom/cors.mojom-shared.h"
 #include "services/network/public/mojom/fetch_api.mojom-shared.h"
 
@@ -23,18 +20,6 @@ class SecurityOrigin;
 // CORS related utility functions.
 namespace CORS {
 
-// Stringify CORSError mainly for inspector messages. Generated string should
-// not be exposed to JavaScript for security reasons.
-// For errors during the redirect check, valid KURL should be set to
-// |redirect_url|. Otherwise, it should be KURL(), the invalid instance.
-PLATFORM_EXPORT String GetErrorString(const network::mojom::CORSError,
-                                      const KURL& request_url,
-                                      const KURL& redirect_url,
-                                      const int response_status_code,
-                                      const HTTPHeaderMap&,
-                                      const SecurityOrigin&,
-                                      const WebURLRequest::RequestContext);
-
 // Thin wrapper functions below are for calling ::network::cors functions from
 // Blink core. Once Out-of-renderer CORS is enabled, following functions will
 // be removed.

third_party/WebKit/Source/platform/loader/cors/CORSErrorString.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 CORSErrorString_h
+#define CORSErrorString_h
+
+#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 {
+
+// CORS error strings related utility functions.
+namespace CORS {
+
+// A struct to pass error dependent arguments for |GetErrorString|.
+struct PLATFORM_EXPORT ErrorParameter {
+  // Creates an ErrorParameter for generic cases. Use this function if |error|
+  // can contain any.
+  static ErrorParameter Create(const network::mojom::CORSError,
+                               const KURL& first_url,
+                               const KURL& second_url,
+                               const int status_code,
+                               const HTTPHeaderMap&,
+                               const SecurityOrigin&,
+                               const WebURLRequest::RequestContext);
+
+  // Creates an ErrorParameter for kDisallowedByMode.
+  static ErrorParameter CreateForDisallowedByMode(const KURL& request_url);
+
+  // Creates an ErrorParameter for kInvalidResponse.
+  static ErrorParameter CreateForInvalidResponse(const KURL& request_url,
+                                                 const SecurityOrigin&);
+
+  // Creates an ErrorParameter for an error that CORS::CheckAccess() returns.
+  // |error| for redirect check needs to specify a valid |redirect_url|. The
+  // |redirect_url| can be omitted not to include redirect related information.
+  static ErrorParameter CreateForAccessCheck(
+      const network::mojom::CORSError,
+      const KURL& request_url,
+      int response_status_code,
+      const HTTPHeaderMap& response_header_map,
+      const SecurityOrigin&,
+      const WebURLRequest::RequestContext,
+      const KURL& redirect_url = KURL());
+
+  // Creates an ErrorParameter for kPreflightInvalidStatus that
+  // CORS::CheckPreflight() returns.
+  static ErrorParameter CreateForPreflightStatusCheck(int response_status_code);
+
+  // Creates an ErrorParameter for an error that CORS::CheckExternalPreflight()
+  // returns.
+  static ErrorParameter CreateForExternalPreflightCheck(
+      const network::mojom::CORSError,
+      const HTTPHeaderMap& response_header_map);
+
+  // Creates an ErrorParameter for CORS::CheckRedirectLocation() returns.
+  static ErrorParameter CreateForRedirectCheck(network::mojom::CORSError,
+                                               const KURL& request_url,
+                                               const KURL& redirect_url);
+
+  // Should not be used directly by external callers. Use Create functions
+  // above.
+  ErrorParameter(const network::mojom::CORSError,
+                 const KURL& first_url,
+                 const KURL& second_url,
+                 const int status_code,
+                 const HTTPHeaderMap&,
+                 const SecurityOrigin&,
+                 const WebURLRequest::RequestContext,
+                 bool unknown);
+
+  // Members that this struct carries.
+  const network::mojom::CORSError error;
+  const KURL& first_url;
+  const KURL& second_url;
+  const int status_code;
+  const HTTPHeaderMap& header_map;
+  const SecurityOrigin& origin;
+  const WebURLRequest::RequestContext context;
+
+  // Set to true when an ErrorParameter was created in a wrong way. Used in
+  // GetErrorString() to be robust for coding errors.
+  const bool unknown;
+};
+
+// Stringify CORSError mainly for inspector messages. Generated string should
+// not be exposed to JavaScript for security reasons.
+PLATFORM_EXPORT String GetErrorString(const ErrorParameter&);
+
+}  // namespace CORS
+
+}  // namespace blink
+
+#endif  // CORSErrorString_h

third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp

@@ -33,6 +33,7 @@
 #include "platform/exported/WrappedResourceRequest.h"
 #include "platform/exported/WrappedResourceResponse.h"
 #include "platform/loader/cors/CORS.h"
+#include "platform/loader/cors/CORSErrorString.h"
 #include "platform/loader/fetch/FetchContext.h"
 #include "platform/loader/fetch/Resource.h"
 #include "platform/loader/fetch/ResourceError.h"
@@ -301,11 +302,11 @@ bool ResourceLoader::WillFollowRedirect(
 
         if (!unused_preload) {
           Context().AddErrorConsoleMessage(
-              CORS::GetErrorString(
+              CORS::GetErrorString(CORS::ErrorParameter::Create(
                   *cors_error, redirect_response.Url(), new_url,
                   redirect_response.HttpStatusCode(),
                   redirect_response.HttpHeaderFields(), *source_origin.get(),
-                  resource_->LastResourceRequest().GetRequestContext()),
+                  resource_->LastResourceRequest().GetRequestContext())),
               FetchContext::kJSSource);
         }
 
@@ -475,11 +476,11 @@ CORSStatus ResourceLoader::DetermineCORSStatus(const ResourceResponse& response,
   error_msg.Append("' from origin '");
   error_msg.Append(source_origin->ToString());
   error_msg.Append("' has been blocked by CORS policy: ");
-  error_msg.Append(CORS::GetErrorString(
+  error_msg.Append(CORS::GetErrorString(CORS::ErrorParameter::Create(
       *cors_error, initial_request.Url(), KURL(),
       response_for_access_control.HttpStatusCode(),
       response_for_access_control.HttpHeaderFields(), *source_origin,
-      initial_request.GetRequestContext()));
+      initial_request.GetRequestContext())));
 
   return CORSStatus::kFailed;
 }