OOR-CORS: remove unnecessary HTTP status existence check

cors::CheckAccessInternal() ensures if the HTTP response status is
set to a non-zero value. This code was originally introduced in
Blink side, but it's out-of-spec implementation.

CORS preflight response expects an ok status for CORS checks and
our implementation implicitly rejects the status 0. Actual response
does not have such CORS specific status code check, and fetch() or
XHR can receive an error status as long as the response has a right
access control headers. But since 0 is not used in HTTP, we don't
need such check in CORS code.

So, let's remove this stale check and relevant test,
WebAssociatedURLLoaderTest.
CrossOriginWithAccessControlFailureBadStatusCode.
This was added by https://codereview.chromium.org/110273006/,
but the original change's intention does not help developers today.

Also note that usual server applications such as Apache
converts this kind of invalid status from CGI to 500 to
assume such situation as an internal server error.

This patch also integrate the ok status check into the
cors::PreflightAccessCheck so to minimize exposed functions.

Bug: 1039174
Change-Id: Ib8f8d84e200a01884daab9d726ac52b38e36a591
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1983696
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730111}

content/browser/web_package/prefetched_signed_exchange_cache.cc

@@ -162,7 +162,7 @@ class InnerResponseURLLoader : public network::mojom::URLLoader {
     if (network::cors::ShouldCheckCors(request.url, request.request_initiator,
                                        request.mode)) {
       const auto error_status = network::cors::CheckAccess(
-          request.url, response_->headers->response_code(),
+          request.url,
           GetHeaderString(
               *response_,
               network::cors::header_names::kAccessControlAllowOrigin),

services/network/cors/cors_url_loader.cc

@@ -252,14 +252,13 @@ void CorsURLLoader::OnReceiveResponse(mojom::URLResponseHeadPtr response_head) {
   DCHECK(forwarding_client_);
   DCHECK(!deferred_redirect_url_);
 
-  int response_status_code =
-      response_head->headers ? response_head->headers->response_code() : 0;
-
+  // See 10.7.4 of https://fetch.spec.whatwg.org/#http-network-or-cache-fetch
   const bool is_304_for_revalidation =
-      request_.is_revalidating && response_status_code == 304;
+      request_.is_revalidating && response_head->headers &&
+      response_head->headers->response_code() == 304;
   if (fetch_cors_flag_ && !is_304_for_revalidation) {
     const auto error_status = CheckAccess(
-        request_.url, response_status_code,
+        request_.url,
         GetHeaderString(*response_head,
                         header_names::kAccessControlAllowOrigin),
         GetHeaderString(*response_head,
@@ -296,7 +295,7 @@ void CorsURLLoader::OnReceiveRedirect(const net::RedirectInfo& redirect_info,
   // failure, then return a network error.
   if (fetch_cors_flag_ && IsCorsEnabledRequestMode(request_.mode)) {
     const auto error_status = CheckAccess(
-        request_.url, response_head->headers->response_code(),
+        request_.url,
         GetHeaderString(*response_head,
                         header_names::kAccessControlAllowOrigin),
         GetHeaderString(*response_head,

services/network/cors/preflight_controller.cc

@@ -134,9 +134,8 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
     base::Optional<CorsErrorStatus>* detected_error_status) {
   DCHECK(detected_error_status);
 
-  const int response_code = head.headers ? head.headers->response_code() : 0;
   *detected_error_status = CheckPreflightAccess(
-      final_url, response_code,
+      final_url, head.headers ? head.headers->response_code() : 0,
       GetHeaderString(head.headers, header_names::kAccessControlAllowOrigin),
       GetHeaderString(head.headers,
                       header_names::kAccessControlAllowCredentials),
@@ -145,13 +144,6 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
   if (*detected_error_status)
     return nullptr;
 
-  base::Optional<mojom::CorsError> error;
-  error = CheckPreflight(response_code);
-  if (error) {
-    *detected_error_status = CorsErrorStatus(*error);
-    return nullptr;
-  }
-
   if (original_request.is_external_request) {
     *detected_error_status = CheckExternalPreflight(GetHeaderString(
         head.headers, header_names::kAccessControlAllowExternal));
@@ -159,6 +151,7 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
       return nullptr;
   }
 
+  base::Optional<mojom::CorsError> error;
   auto result = PreflightResult::Create(
       original_request.credentials_mode,
       GetHeaderString(head.headers, header_names::kAccessControlAllowMethods),

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

@@ -123,16 +123,10 @@ bool IsNoCorsSafelistedHeaderNameLowerCase(const std::string& lower_name) {
 
 base::Optional<CorsErrorStatus> CheckAccessInternal(
     const GURL& response_url,
-    const int response_status_code,
     const base::Optional<std::string>& allow_origin_header,
     const base::Optional<std::string>& allow_credentials_header,
     mojom::CredentialsMode credentials_mode,
     const url::Origin& origin) {
-  // TODO(toyoshim): This response status code check should not be needed. We
-  // have another status code check after a CheckAccess() call if it is needed.
-  if (!response_status_code)
-    return CorsErrorStatus(mojom::CorsError::kInvalidResponse);
-
   if (allow_origin_header == kAsterisk) {
     // A wildcard Access-Control-Allow-Origin can not be used if credentials are
     // to be sent, even with Access-Control-Allow-Credentials set to true.
@@ -240,14 +234,13 @@ const char kAccessControlRequestMethod[] = "Access-Control-Request-Method";
 // See https://fetch.spec.whatwg.org/#cors-check.
 base::Optional<CorsErrorStatus> CheckAccess(
     const GURL& response_url,
-    const int response_status_code,
     const base::Optional<std::string>& allow_origin_header,
     const base::Optional<std::string>& allow_credentials_header,
     mojom::CredentialsMode credentials_mode,
     const url::Origin& origin) {
-  base::Optional<CorsErrorStatus> error_status = CheckAccessInternal(
-      response_url, response_status_code, allow_origin_header,
-      allow_credentials_header, credentials_mode, origin);
+  const auto error_status =
+      CheckAccessInternal(response_url, allow_origin_header,
+                          allow_credentials_header, credentials_mode, origin);
   ReportAccessCheckResultMetric(error_status ? AccessCheckResult::kNotPermitted
                                              : AccessCheckResult::kPermitted);
   if (error_status) {
@@ -285,47 +278,58 @@ base::Optional<CorsErrorStatus> CheckPreflightAccess(
     const base::Optional<std::string>& allow_credentials_header,
     mojom::CredentialsMode actual_credentials_mode,
     const url::Origin& origin) {
-  const auto error_status = CheckAccessInternal(
-      response_url, response_status_code, allow_origin_header,
-      allow_credentials_header, actual_credentials_mode, origin);
+  // Step 7 of https://fetch.spec.whatwg.org/#cors-preflight-fetch
+  auto error_status = CheckAccessInternal(response_url, allow_origin_header,
+                                          allow_credentials_header,
+                                          actual_credentials_mode, origin);
+  const bool has_ok_status = IsOkStatus(response_status_code);
+
   ReportAccessCheckResultMetric(
-      error_status ? AccessCheckResult::kNotPermittedInPreflight
-                   : AccessCheckResult::kPermittedInPreflight);
-  if (!error_status)
-    return base::nullopt;
+      (error_status || !has_ok_status)
+          ? AccessCheckResult::kNotPermittedInPreflight
+          : AccessCheckResult::kPermittedInPreflight);
 
-  // TODO(toyoshim): Remove following two lines when the status code check is
-  // removed from CheckAccess().
-  if (error_status->cors_error == mojom::CorsError::kInvalidResponse)
-    return error_status;
-
-  mojom::CorsError error = error_status->cors_error;
-  switch (error_status->cors_error) {
-    case mojom::CorsError::kWildcardOriginNotAllowed:
-      error = mojom::CorsError::kPreflightWildcardOriginNotAllowed;
-      break;
-    case mojom::CorsError::kMissingAllowOriginHeader:
-      error = mojom::CorsError::kPreflightMissingAllowOriginHeader;
-      break;
-    case mojom::CorsError::kMultipleAllowOriginValues:
-      error = mojom::CorsError::kPreflightMultipleAllowOriginValues;
-      break;
-    case mojom::CorsError::kInvalidAllowOriginValue:
-      error = mojom::CorsError::kPreflightInvalidAllowOriginValue;
-      break;
-    case mojom::CorsError::kAllowOriginMismatch:
-      error = mojom::CorsError::kPreflightAllowOriginMismatch;
-      break;
-    case mojom::CorsError::kInvalidAllowCredentials:
-      error = mojom::CorsError::kPreflightInvalidAllowCredentials;
-      break;
-    default:
-      NOTREACHED();
-      break;
+  // Prefer using a preflight specific error code.
+  if (error_status) {
+    switch (error_status->cors_error) {
+      case mojom::CorsError::kWildcardOriginNotAllowed:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightWildcardOriginNotAllowed;
+        break;
+      case mojom::CorsError::kMissingAllowOriginHeader:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightMissingAllowOriginHeader;
+        break;
+      case mojom::CorsError::kMultipleAllowOriginValues:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightMultipleAllowOriginValues;
+        break;
+      case mojom::CorsError::kInvalidAllowOriginValue:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightInvalidAllowOriginValue;
+        break;
+      case mojom::CorsError::kAllowOriginMismatch:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightAllowOriginMismatch;
+        break;
+      case mojom::CorsError::kInvalidAllowCredentials:
+        error_status->cors_error =
+            mojom::CorsError::kPreflightInvalidAllowCredentials;
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+  } else if (!has_ok_status) {
+    error_status = base::make_optional<CorsErrorStatus>(
+        mojom::CorsError::kPreflightInvalidStatus);
+  } else {
+    return base::nullopt;
   }
-  UMA_HISTOGRAM_ENUMERATION("Net.Cors.PreflightCheckError", error);
 
-  return CorsErrorStatus(error, error_status->failed_parameter);
+  UMA_HISTOGRAM_ENUMERATION("Net.Cors.PreflightCheckError",
+                            error_status->cors_error);
+  return error_status;
 }
 
 base::Optional<CorsErrorStatus> CheckRedirectLocation(
@@ -359,16 +363,6 @@ base::Optional<CorsErrorStatus> CheckRedirectLocation(
   return base::nullopt;
 }
 
-base::Optional<mojom::CorsError> CheckPreflight(const int status_code) {
-  // CORS preflight with 3XX is considered network error in
-  // Fetch API Spec: https://fetch.spec.whatwg.org/#cors-preflight-fetch
-  // CORS Spec: http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0
-  // https://crbug.com/452394
-  if (IsOkStatus(status_code))
-    return base::nullopt;
-  return mojom::CorsError::kPreflightInvalidStatus;
-}
-
 // https://wicg.github.io/cors-rfc1918/#http-headerdef-access-control-allow-external
 base::Optional<CorsErrorStatus> CheckExternalPreflight(
     const base::Optional<std::string>& allow_external) {

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

@@ -53,7 +53,6 @@ extern const char kAccessControlRequestMethod[];
 COMPONENT_EXPORT(NETWORK_CPP)
 base::Optional<CorsErrorStatus> CheckAccess(
     const GURL& response_url,
-    const int response_status_code,
     const base::Optional<std::string>& allow_origin_header,
     const base::Optional<std::string>& allow_credentials_header,
     mojom::CredentialsMode credentials_mode,
@@ -93,12 +92,6 @@ base::Optional<CorsErrorStatus> CheckRedirectLocation(
     bool cors_flag,
     bool tainted);
 
-// 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);
-
 // Checks errors for the currently experimental "Access-Control-Allow-External:"
 // header. Shares error conditions with standard preflight checking.
 // See https://crbug.com/590714.

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

@@ -570,8 +570,7 @@ bool ThreadableLoader::RedirectReceived(
 
     if (cors_flag_) {
       if (const auto error_status = cors::CheckAccess(
-              original_url, redirect_response.HttpStatusCode(),
-              redirect_response.HttpHeaderFields(),
+              original_url, redirect_response.HttpHeaderFields(),
               new_request.GetCredentialsMode(), *GetSecurityOrigin())) {
         DispatchDidFail(ResourceError(original_url, *error_status));
         return false;
@@ -751,14 +750,6 @@ void ThreadableLoader::HandlePreflightResponse(
     return;
   }
 
-  base::Optional<network::mojom::CorsError> preflight_error =
-      cors::CheckPreflight(response.HttpStatusCode());
-  if (preflight_error) {
-    HandlePreflightFailure(response.CurrentRequestUrl(),
-                           network::CorsErrorStatus(*preflight_error));
-    return;
-  }
-
   base::Optional<network::CorsErrorStatus> error_status;
   if (actual_request_.IsExternalRequest()) {
     error_status = cors::CheckExternalPreflight(response.HttpHeaderFields());
@@ -860,8 +851,8 @@ void ThreadableLoader::ResponseReceived(Resource* resource,
 
   if (cors_flag_) {
     base::Optional<network::CorsErrorStatus> access_error = cors::CheckAccess(
-        response.CurrentRequestUrl(), response.HttpStatusCode(),
-        response.HttpHeaderFields(), credentials_mode_, *GetSecurityOrigin());
+        response.CurrentRequestUrl(), response.HttpHeaderFields(),
+        credentials_mode_, *GetSecurityOrigin());
     if (access_error) {
       ReportResponseReceived(resource->InspectorId(), response);
       DispatchDidFail(

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

@@ -387,36 +387,6 @@ TEST_F(WebAssociatedURLLoaderTest, CrossOriginWithAccessControlFailure) {
   EXPECT_FALSE(did_receive_response_);
 }
 
-// Test an unsuccessful cross-origin load using CORS.
-TEST_F(WebAssociatedURLLoaderTest,
-       CrossOriginWithAccessControlFailureBadStatusCode) {
-  // This is cross-origin since the frame was loaded from www.test.com.
-  KURL url =
-      ToKURL("http://www.other.com/CrossOriginWithAccessControlFailure.html");
-  WebURLRequest request(url);
-  request.SetMode(network::mojom::RequestMode::kCors);
-  request.SetCredentialsMode(network::mojom::CredentialsMode::kOmit);
-
-  expected_response_ = WebURLResponse();
-  expected_response_.SetMimeType("text/html");
-  expected_response_.SetHttpStatusCode(0);
-  expected_response_.AddHttpHeaderField("access-control-allow-origin", "*");
-  RegisterMockedURLLoadWithCustomResponse(url, expected_response_,
-                                          frame_file_path_);
-
-  WebAssociatedURLLoaderOptions options;
-  expected_loader_ = CreateAssociatedURLLoader(options);
-  EXPECT_TRUE(expected_loader_);
-  expected_loader_->LoadAsynchronously(request, this);
-
-  // Failure should not be reported synchronously.
-  EXPECT_FALSE(did_fail_);
-  // The loader needs to receive the response, before doing the CORS check.
-  ServeRequests();
-  EXPECT_TRUE(did_fail_);
-  EXPECT_FALSE(did_receive_response_);
-}
-
 // Test a same-origin URL redirect and load.
 TEST_F(WebAssociatedURLLoaderTest, RedirectSuccess) {
   KURL url = ToKURL("http://www.test.com/RedirectSuccess.html");

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

@@ -160,12 +160,11 @@ namespace cors {
 
 base::Optional<network::CorsErrorStatus> CheckAccess(
     const KURL& response_url,
-    const int response_status_code,
     const HTTPHeaderMap& response_header,
     network::mojom::CredentialsMode credentials_mode,
     const SecurityOrigin& origin) {
   return network::cors::CheckAccess(
-      response_url, response_status_code,
+      response_url,
       GetHeaderValue(response_header, http_names::kAccessControlAllowOrigin),
       GetHeaderValue(response_header,
                      http_names::kAccessControlAllowCredentials),
@@ -201,11 +200,6 @@ base::Optional<network::CorsErrorStatus> CheckRedirectLocation(
       url, request_mode, origin_to_pass, cors_flag == CorsFlag::Set, false);
 }
 
-base::Optional<network::mojom::CorsError> CheckPreflight(
-    const int preflight_response_status_code) {
-  return network::cors::CheckPreflight(preflight_response_status_code);
-}
-
 base::Optional<network::CorsErrorStatus> CheckExternalPreflight(
     const HTTPHeaderMap& response_header) {
   return network::cors::CheckExternalPreflight(

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

@@ -35,7 +35,6 @@ namespace cors {
 // be removed.
 PLATFORM_EXPORT base::Optional<network::CorsErrorStatus> CheckAccess(
     const KURL&,
-    const int response_status_code,
     const HTTPHeaderMap&,
     network::mojom::CredentialsMode,
     const SecurityOrigin&);
@@ -53,9 +52,6 @@ PLATFORM_EXPORT base::Optional<network::CorsErrorStatus> CheckRedirectLocation(
     const SecurityOrigin*,
     CorsFlag);
 
-PLATFORM_EXPORT base::Optional<network::mojom::CorsError> CheckPreflight(
-    const int preflight_response_status_code);
-
 PLATFORM_EXPORT base::Optional<network::CorsErrorStatus> CheckExternalPreflight(
     const HTTPHeaderMap&);
 

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

@@ -756,9 +756,9 @@ bool ResourceLoader::WillFollowRedirect(
               new_url, request_mode, origin.get(),
               GetCorsFlag() ? CorsFlag::Set : CorsFlag::Unset);
       if (!cors_error && GetCorsFlag()) {
-        cors_error = cors::CheckAccess(
-            new_url, redirect_response.HttpStatusCode(),
-            redirect_response.HttpHeaderFields(), credentials_mode, *origin);
+        cors_error =
+            cors::CheckAccess(new_url, redirect_response.HttpHeaderFields(),
+                              credentials_mode, *origin);
       }
       if (cors_error) {
         HandleError(
@@ -1021,9 +1021,8 @@ void ResourceLoader::DidReceiveResponseInternal(
       !(resource_->IsCacheValidator() && response.HttpStatusCode() == 304)) {
     if (GetCorsFlag()) {
       base::Optional<network::CorsErrorStatus> cors_error = cors::CheckAccess(
-          response.CurrentRequestUrl(), response.HttpStatusCode(),
-          response.HttpHeaderFields(), initial_request.GetCredentialsMode(),
-          *resource_->GetOrigin());
+          response.CurrentRequestUrl(), response.HttpHeaderFields(),
+          initial_request.GetCredentialsMode(), *resource_->GetOrigin());
       if (cors_error) {
         HandleError(ResourceError(response.CurrentRequestUrl(), *cors_error));
         return;