OOR-CORS: store more information to CORSErrorStatus on preflight errors

Current implementation does not store rejected method or header
information to CORSErrorStatus on CORS-preflight checks, and
results in missing hint parameter on CreateErrorString() in Blink.
This patch provides required information to generate right error
messages, and fixes some crashed layout tests.

Bug: 803766, 836741
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: If4de6b916a77d204812a7b010e39080bf059bc9e
Tbr: kinuko@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/1030670
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554642}

services/network/cors/preflight_controller.cc

@@ -150,34 +150,19 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
       detected_error);
 }
 
-base::Optional<mojom::CORSError> CheckPreflightResult(
+base::Optional<CORSErrorStatus> CheckPreflightResult(
     PreflightResult* result,
-    const ResourceRequest& original_request,
-    scoped_refptr<net::HttpResponseHeaders>* error_header) {
-  DCHECK(error_header);
-
+    const ResourceRequest& original_request) {
   base::Optional<mojom::CORSError> error =
       result->EnsureAllowedCrossOriginMethod(original_request.method);
   if (error)
-    return error;
+    return CORSErrorStatus(*error, original_request.method);
 
   std::string detected_error_header;
   error = result->EnsureAllowedCrossOriginHeaders(original_request.headers,
                                                   &detected_error_header);
-  if (error) {
-    // Gather information to report the error's details.
-    DCHECK(!detected_error_header.empty());
-    std::string header_value;
-    bool found = original_request.headers.GetHeader(detected_error_header,
-                                                    &header_value);
-    DCHECK(found);
-    // Status line below is dummy to construct a response header instance.
-    *error_header =
-        base::MakeRefCounted<net::HttpResponseHeaders>(base::StringPrintf(
-            "HTTP/1.0 200 OK\n%s: %s", detected_error_header.c_str(),
-            header_value.c_str()));
-    return error;
-  }
+  if (error)
+    return CORSErrorStatus(*error, detected_error_header);
 
   return base::nullopt;
 }
@@ -291,27 +276,25 @@ class PreflightController::PreflightLoader final {
     std::unique_ptr<PreflightResult> result = CreatePreflightResult(
         final_url, head, original_request_, &detected_error);
 
-    scoped_refptr<net::HttpResponseHeaders> error_header;
+    base::Optional<CORSErrorStatus> detected_error_status;
     if (result) {
       // Preflight succeeded. Check |original_request_| with |result|.
       DCHECK(!detected_error);
-      detected_error =
-          CheckPreflightResult(result.get(), original_request_, &error_header);
+      detected_error_status =
+          CheckPreflightResult(result.get(), original_request_);
+    } else {
+      DCHECK(detected_error);
+      detected_error_status = CORSErrorStatus(*detected_error);
     }
 
     // TODO(toyoshim): Check the spec if we cache |result| regardless of
     // following checks.
-    if (!detected_error) {
+    if (!detected_error_status) {
       controller_->AppendToCache(*original_request_.request_initiator,
                                  original_request_.url, std::move(result));
     }
 
-    if (detected_error) {
-      std::move(completion_callback_)
-          .Run(CORSErrorStatus(*detected_error, error_header));
-    } else {
-      std::move(completion_callback_).Run(base::nullopt);
-    }
+    std::move(completion_callback_).Run(detected_error_status);
 
     RemoveFromController();
     // |this| is deleted here.

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

@@ -19,18 +19,15 @@ CORSErrorStatus::CORSErrorStatus(const CORSErrorStatus& status) = default;
 CORSErrorStatus::CORSErrorStatus(network::mojom::CORSError error)
     : cors_error(error) {}
 
-CORSErrorStatus::CORSErrorStatus(
-    network::mojom::CORSError error,
-    scoped_refptr<net::HttpResponseHeaders> headers)
-    : CORSErrorStatus(error) {
-  related_response_headers = headers;
-}
+CORSErrorStatus::CORSErrorStatus(network::mojom::CORSError error,
+                                 const std::string& failed_parameter)
+    : cors_error(error), failed_parameter(failed_parameter) {}
 
 CORSErrorStatus::~CORSErrorStatus() = default;
 
 bool CORSErrorStatus::operator==(const CORSErrorStatus& rhs) const {
   return cors_error == rhs.cors_error &&
-         related_response_headers == rhs.related_response_headers;
+         failed_parameter == rhs.failed_parameter;
 }
 
 }  // namespace network

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

@@ -5,6 +5,8 @@
 #ifndef SERVICES_NETWORK_PUBLIC_CPP_CORS_CORS_ERROR_STATUS_H_
 #define SERVICES_NETWORK_PUBLIC_CPP_CORS_CORS_ERROR_STATUS_H_
 
+#include <string>
+
 #include "base/component_export.h"
 #include "base/memory/scoped_refptr.h"
 #include "net/http/http_response_headers.h"
@@ -22,9 +24,8 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) CORSErrorStatus {
   CORSErrorStatus(const CORSErrorStatus& status);
 
   explicit CORSErrorStatus(network::mojom::CORSError error);
-  CORSErrorStatus(
-      network::mojom::CORSError error,
-      scoped_refptr<net::HttpResponseHeaders> related_response_headers);
+  CORSErrorStatus(network::mojom::CORSError error,
+                  const std::string& failed_parameter);
 
   ~CORSErrorStatus();
 
@@ -33,9 +34,8 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) CORSErrorStatus {
 
   network::mojom::CORSError cors_error;
 
-  // Partial HTTP response headers including status line that will be useful to
-  // generate a human readable error message.
-  scoped_refptr<net::HttpResponseHeaders> related_response_headers;
+  // Contains request method name, or header name that didn't pass a CORS check.
+  std::string failed_parameter;
 };
 
 }  // namespace network

services/network/public/cpp/network_param_ipc_traits.h

@@ -277,7 +277,7 @@ IPC_ENUM_TRAITS_MAX_VALUE(network::mojom::RequestContextFrameType,
 
 IPC_STRUCT_TRAITS_BEGIN(network::CORSErrorStatus)
   IPC_STRUCT_TRAITS_MEMBER(cors_error)
-  IPC_STRUCT_TRAITS_MEMBER(related_response_headers)
+  IPC_STRUCT_TRAITS_MEMBER(failed_parameter)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(network::URLLoaderCompletionStatus)

third_party/WebKit/LayoutTests/TestExpectations

@@ -1228,18 +1228,6 @@ crbug.com/826878 [ Mac ] virtual/color_space/fast/canvas/color-space/toDataURL-c
 # Total  2098
 # Counts skip: 2 crash: 14 text: 67 image: 1 pass: 2005 timeout: 9
 
-# Release build only wpt test crashes on CORS-preflight. Should be fixed soon.
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any.worker.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight-star.any.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight-star.any.worker.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight.any.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight.any.worker.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Crash ]
-crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Crash ]
-
 # Skipped tests in dictionary order.
 crbug.com/757165 [ Win ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/navigation-redirect.https.html [ Skip ]
 crbug.com/691944 virtual/outofblink-cors/external/wpt/service-workers/service-worker/update-after-oneday.https.html [ Skip ]
@@ -1285,20 +1273,17 @@ crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/simple-cross-
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url.html [ Timeout ]
 
 # Failing tests in dictionary order.
-crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Failure ]
-crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Failure ]
+crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Failure ]
+crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect.any.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect.any.worker.html [ Failure ]
 crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/img-mime-types-coverage.tentative.sub.html [ Failure ]
-crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/img-png-mislabeled-as-html-nosniff.tentative.sub.html [ Failure ]
-crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/preload-image-png-mislabeled-as-html-nosniff.tentative.sub.html [ Failure ]
-crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/script-html-correctly-labeled.tentative.sub.html [ Failure ]
 crbug.com/745327 virtual/outofblink-cors/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html [ Failure Pass ]
 crbug.com/834183 virtual/outofblink-cors/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html [ Failure Pass ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-image-cache.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-image.https.html [ Failure ]
-crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Failure ]
-crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Failure ]
+crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Failure ]
+crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-event-referrer-policy.https.html [ Failure ]
 crbug.com/595993 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-header-visibility.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-request-html-imports.https.html [ Failure ]

third_party/blink/renderer/core/loader/document_threadable_loader.cc

@@ -1116,20 +1116,15 @@ void DocumentThreadableLoader::DispatchDidFail(const ResourceError& error) {
   if (error.CORSErrorStatus()) {
     DCHECK(out_of_blink_cors_);
     // TODO(toyoshim): Should consider to pass correct arguments instead of
-    // WebURL() and WebHTTPHeaderMap() to GetErrorString().
-    // We still need plumbing required information.
-    const int response_code =
-        error.CORSErrorStatus()->related_response_headers
-            ? error.CORSErrorStatus()->related_response_headers->response_code()
-            : 0;
+    // KURL(), 0, and HTTPHeaderMap() to GetErrorString().
+    // We still need plumbing some more information.
     GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(
         kJSMessageSource, kErrorMessageLevel,
         "Failed to load " + error.FailingURL() + ": " +
-            CORS::GetErrorString(CORS::ErrorParameter::Create(
-                                     error.CORSErrorStatus()->cors_error,
-                                     KURL(error.FailingURL()), KURL(),
-                                     response_code, HTTPHeaderMap(),
-                                     *GetSecurityOrigin(), request_context_))
+            CORS::GetErrorString(
+                CORS::ErrorParameter::Create(
+                    *error.CORSErrorStatus(), KURL(error.FailingURL()), KURL(),
+                    0, HTTPHeaderMap(), *GetSecurityOrigin(), request_context_))
                 .Utf8()
                 .data()));
   }

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

@@ -37,15 +37,25 @@ ErrorParameter CreateWrongParameter(network::mojom::CORSError error) {
 
 // static
 ErrorParameter ErrorParameter::Create(
-    const network::mojom::CORSError error,
+    const network::CORSErrorStatus& error_status,
     const KURL& first_url,
     const KURL& second_url,
     const int status_code,
     const HTTPHeaderMap& header_map,
     const SecurityOrigin& origin,
     const WebURLRequest::RequestContext context) {
-  return ErrorParameter(error, first_url, second_url, status_code, header_map,
-                        origin, context, String(), false);
+  String hint;
+  switch (error_status.cors_error) {
+    case network::mojom::CORSError::kMethodDisallowedByPreflightResponse:
+    case network::mojom::CORSError::kHeaderDisallowedByPreflightResponse:
+      DCHECK(!error_status.failed_parameter.empty());
+      hint = String(error_status.failed_parameter.c_str());
+      break;
+    default:
+      break;
+  }
+  return ErrorParameter(error_status.cors_error, first_url, second_url,
+                        status_code, header_map, origin, context, hint, false);
 }
 
 // static

third_party/blink/renderer/platform/loader/cors/cors_error_string.h

@@ -6,6 +6,7 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_LOADER_CORS_CORS_ERROR_STRING_H_
 
 #include "base/macros.h"
+#include "services/network/public/cpp/cors/cors_error_status.h"
 #include "services/network/public/mojom/cors.mojom-shared.h"
 #include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
@@ -23,7 +24,7 @@ namespace CORS {
 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,
+  static ErrorParameter Create(const network::CORSErrorStatus&,
                                const KURL& first_url,
                                const KURL& second_url,
                                const int status_code,

third_party/blink/renderer/platform/loader/fetch/resource_loader.cc

@@ -308,7 +308,8 @@ bool ResourceLoader::WillFollowRedirect(
         if (!unused_preload) {
           Context().AddErrorConsoleMessage(
               CORS::GetErrorString(CORS::ErrorParameter::Create(
-                  *cors_error, redirect_response.Url(), new_url,
+                  network::CORSErrorStatus(*cors_error),
+                  redirect_response.Url(), new_url,
                   redirect_response.HttpStatusCode(),
                   redirect_response.HttpHeaderFields(), *source_origin.get(),
                   resource_->LastResourceRequest().GetRequestContext())),
@@ -487,7 +488,7 @@ CORSStatus ResourceLoader::DetermineCORSStatus(const ResourceResponse& response,
   error_msg.Append(source_origin->ToString());
   error_msg.Append("' has been blocked by CORS policy: ");
   error_msg.Append(CORS::GetErrorString(CORS::ErrorParameter::Create(
-      *cors_error, initial_request.Url(), KURL(),
+      network::CORSErrorStatus(*cors_error), initial_request.Url(), KURL(),
       response_for_access_control.HttpStatusCode(),
       response_for_access_control.HttpHeaderFields(), *source_origin,
       initial_request.GetRequestContext())));