Clean up the CORS preflight part of DocumentThreadableLoader::MakeCrossOriginAccessRequest()

The bunch of if-clauses are hard to read. Reorganize them for better
readability.

Unify the multiple checks on IsExternalRequest.

Early return for IsExternalRequest().

Don't check CORS-safelisted-ness when either of the external request or
forced preflight condition is already met.

Clarify where the ServiceWorkerMode configuration is required and its
reason for each place.

Bug: 695808, 604084
Change-Id: I2a056afeebea4d6472e60c52cc72fd58c6bef703
Reviewed-on: https://chromium-review.googlesource.com/542676Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Commit-Queue: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481846}

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

@@ -275,34 +275,56 @@ void DocumentThreadableLoader::Start(const ResourceRequest& request) {
     new_request.SetFetchRequestMode(options_.fetch_request_mode);
   }
 
-  // We assume that ServiceWorker is skipped for sync requests and unsupported
-  // protocol requests by content/ code.
-  if (async_ &&
-      request.GetServiceWorkerMode() ==
-          WebURLRequest::ServiceWorkerMode::kAll &&
-      SchemeRegistry::ShouldTreatURLSchemeAsAllowingServiceWorkers(
-          request.Url().Protocol()) &&
-      loading_context_->GetResourceFetcher()->IsControlledByServiceWorker()) {
-    if (new_request.GetFetchRequestMode() ==
-            WebURLRequest::kFetchRequestModeCORS ||
-        new_request.GetFetchRequestMode() ==
-            WebURLRequest::kFetchRequestModeCORSWithForcedPreflight) {
-      fallback_request_for_service_worker_ = ResourceRequest(request);
-      // m_fallbackRequestForServiceWorker is used when a regular controlling
-      // service worker doesn't handle a cross origin request. When this happens
-      // we still want to give foreign fetch a chance to handle the request, so
-      // only skip the controlling service worker for the fallback request. This
-      // is currently safe because of http://crbug.com/604084 the
-      // wasFallbackRequiredByServiceWorker flag is never set when foreign fetch
-      // handled a request.
-      fallback_request_for_service_worker_.SetServiceWorkerMode(
-          WebURLRequest::ServiceWorkerMode::kForeign);
-    }
-    LoadRequest(new_request, resource_loader_options_);
+  // Process the CORS protocol inside the DocumentThreadableLoader for the
+  // following cases:
+  //
+  // - When the request is sync or the protocol is unsupported since we can
+  //   assume that any SW is skipped for such requests by content/ code.
+  // - When the ServiceWorkerMode is not kAll, the local SW will be skipped.
+  //   The request can still be intercepted by a foreign SW, but we cannot know
+  //   whether such a foreign fetch interception happens or not at this point.
+  // - If we're not yet controlled by a local SW, then we're sure that this
+  //   request won't be intercepted by a local SW. In case we end up with
+  //   sending a CORS preflight request, the actual request to be sent later
+  //   may be intercepted. This is taken care of in LoadPreflightRequest() by
+  //   setting the ServiceWorkerMode to kNone.
+  //
+  // From the above analysis, you can see that the request can never be
+  // intercepted by a local SW inside this if-block. It's because:
+  // - the ServiceWorkerMode needs to be kAll, and
+  // - we're controlled by a SW at this point
+  // to allow a local SW to intercept the request. Even when the request gets
+  // issued asnychronously after performing the CORS preflight, it doesn'g get
+  // intercepted since LoadPreflightRequest() sets the flag to kNone in
+  // advance.
+  if (!async_ ||
+      request.GetServiceWorkerMode() !=
+          WebURLRequest::ServiceWorkerMode::kAll ||
+      !SchemeRegistry::ShouldTreatURLSchemeAsAllowingServiceWorkers(
+          request.Url().Protocol()) ||
+      !loading_context_->GetResourceFetcher()->IsControlledByServiceWorker()) {
+    DispatchInitialRequest(new_request);
     return;
   }
 
-  DispatchInitialRequest(new_request);
+  if (new_request.GetFetchRequestMode() ==
+          WebURLRequest::kFetchRequestModeCORS ||
+      new_request.GetFetchRequestMode() ==
+          WebURLRequest::kFetchRequestModeCORSWithForcedPreflight) {
+    // Save the request to fallback_request_for_service_worker to use when the
+    // local SW doesn't handle (call respondWith()) a CORS enabled request.
+    fallback_request_for_service_worker_ = ResourceRequest(request);
+
+    // We still want to give a foreign SW if any a chance to handle the
+    // request. So, only skip the controlling local SW for the fallback
+    // request. This is currently safe because of http://crbug.com/604084. The
+    // WasFallbackRequiredByServiceWorker() flag is never set when a foreign SW
+    // handled a request.
+    fallback_request_for_service_worker_.SetServiceWorkerMode(
+        WebURLRequest::ServiceWorkerMode::kForeign);
+  }
+
+  LoadRequest(new_request, resource_loader_options_);
 }
 
 void DocumentThreadableLoader::DispatchInitialRequest(
@@ -321,13 +343,39 @@ void DocumentThreadableLoader::DispatchInitialRequest(
 }
 
 void DocumentThreadableLoader::PrepareCrossOriginRequest(
-    ResourceRequest& request) {
+    ResourceRequest& request) const {
   if (GetSecurityOrigin())
     request.SetHTTPOrigin(GetSecurityOrigin());
   if (override_referrer_)
     request.SetHTTPReferrer(referrer_after_redirect_);
 }
 
+void DocumentThreadableLoader::LoadPreflightRequest(
+    const ResourceRequest& actual_request,
+    const ResourceLoaderOptions& actual_options) {
+  ResourceRequest preflight_request =
+      CreateAccessControlPreflightRequest(actual_request);
+
+  // TODO(tyoshino): Call prepareCrossOriginRequest(preflightRequest) to
+  // also set the referrer header.
+  if (GetSecurityOrigin())
+    preflight_request.SetHTTPOrigin(GetSecurityOrigin());
+
+  actual_request_ = actual_request;
+  actual_options_ = actual_options;
+
+  // Explicitly set the ServiceWorkerMode to None here. Although the page is
+  // not controlled by a SW at this point, a new SW may be controlling the
+  // page when this actual request gets sent later. We should not send the
+  // actual request to the SW. See https://crbug.com/604583.
+  actual_request_.SetServiceWorkerMode(WebURLRequest::ServiceWorkerMode::kNone);
+
+  // Create a ResourceLoaderOptions for preflight.
+  ResourceLoaderOptions preflight_options = actual_options;
+
+  LoadRequest(preflight_request, preflight_options);
+}
+
 void DocumentThreadableLoader::MakeCrossOriginAccessRequest(
     const ResourceRequest& request) {
   DCHECK(options_.fetch_request_mode == WebURLRequest::kFetchRequestModeCORS ||
@@ -368,74 +416,71 @@ void DocumentThreadableLoader::MakeCrossOriginAccessRequest(
 
   cross_origin_request.RemoveUserAndPassFromURL();
 
-  bool skip_preflight = false;
-  if (options_.preflight_policy == kPreventPreflight) {
-    skip_preflight = true;
-  } else {
+  // Enforce the CORS preflight for checking the Access-Control-Allow-External
+  // header. The CORS preflight cache doesn't help for this purpose.
+  if (request.IsExternalRequest()) {
+    LoadPreflightRequest(cross_origin_request, cross_origin_options);
+    return;
+  }
+
+  if (options_.fetch_request_mode !=
+      WebURLRequest::kFetchRequestModeCORSWithForcedPreflight) {
+    if (options_.preflight_policy == kPreventPreflight) {
+      PrepareCrossOriginRequest(cross_origin_request);
+      LoadRequest(cross_origin_request, cross_origin_options);
+      return;
+    }
+
     DCHECK_EQ(options_.preflight_policy, kConsiderPreflight);
 
     // We use ContainsOnlyCORSSafelistedOrForbiddenHeaders() here since
-    // |request| may have been modified in the process of loading (not from the
-    // user's input). For example, referrer. We need to accept them. For
+    // |request| may have been modified in the process of loading (not from
+    // the user's input). For example, referrer. We need to accept them. For
     // security, we must reject forbidden headers/methods at the point we
     // accept user's input. Not here.
     if (FetchUtils::IsCORSSafelistedMethod(request.HttpMethod()) &&
         FetchUtils::ContainsOnlyCORSSafelistedOrForbiddenHeaders(
-            request.HttpHeaderFields()))
-      skip_preflight = true;
+            request.HttpHeaderFields())) {
+      PrepareCrossOriginRequest(cross_origin_request);
+      LoadRequest(cross_origin_request, cross_origin_options);
+      return;
+    }
   }
 
-  if (!request.IsExternalRequest() &&
-      options_.fetch_request_mode !=
-          WebURLRequest::kFetchRequestModeCORSWithForcedPreflight &&
-      skip_preflight) {
-    PrepareCrossOriginRequest(cross_origin_request);
-    LoadRequest(cross_origin_request, cross_origin_options);
-    return;
-  }
+  // Now, we need to check that the request passes the CORS preflight either by
+  // issuing a CORS preflight or based on an entry in the CORS preflight cache.
 
-  // Explicitly set the ServiceWorkerMode to None here. Although the page is
-  // not controlled by a SW at this point, a new SW may be controlling the
-  // page when this request gets sent later. We should not send the actual
-  // request to the SW. https://crbug.com/604583
-  // Similarly we don't want any requests that could involve a CORS preflight
-  // to get intercepted by a foreign fetch service worker, even if we have the
-  // result of the preflight cached already. https://crbug.com/674370
-  cross_origin_request.SetServiceWorkerMode(
-      WebURLRequest::ServiceWorkerMode::kNone);
+  bool should_ignore_preflight_cache = false;
+  if (!IsMainThread()) {
+    // TODO(horo): Currently we don't support the CORS preflight cache on worker
+    // thread when off-main-thread-fetch is enabled. See
+    // https://crbug.com/443374.
+    should_ignore_preflight_cache = true;
+  } else {
+    // Prevent use of the CORS preflight cache when instructed by the DevTools
+    // not to use caches.
+    probe::shouldForceCORSPreflight(GetDocument(),
+                                    &should_ignore_preflight_cache);
+  }
 
-  bool should_force_preflight = request.IsExternalRequest();
-  if (!should_force_preflight)
-    probe::shouldForceCORSPreflight(GetDocument(), &should_force_preflight);
-  // TODO(horo): Currently we don't support the CORS preflight cache on worker
-  // thread when off-main-thread-fetch is enabled. https://crbug.com/443374
-  bool can_skip_preflight =
-      IsMainThread() &&
-      CrossOriginPreflightResultCache::Shared().CanSkipPreflight(
+  if (should_ignore_preflight_cache ||
+      !CrossOriginPreflightResultCache::Shared().CanSkipPreflight(
           GetSecurityOrigin()->ToString(), cross_origin_request.Url(),
           cross_origin_request.GetFetchCredentialsMode(),
           cross_origin_request.HttpMethod(),
-          cross_origin_request.HttpHeaderFields());
-  if (can_skip_preflight && !should_force_preflight) {
-    PrepareCrossOriginRequest(cross_origin_request);
-    LoadRequest(cross_origin_request, cross_origin_options);
+          cross_origin_request.HttpHeaderFields())) {
+    LoadPreflightRequest(cross_origin_request, cross_origin_options);
     return;
   }
 
-  ResourceRequest preflight_request =
-      CreateAccessControlPreflightRequest(cross_origin_request);
-  // TODO(tyoshino): Call prepareCrossOriginRequest(preflightRequest) to
-  // also set the referrer header.
-  if (GetSecurityOrigin())
-    preflight_request.SetHTTPOrigin(GetSecurityOrigin());
-
-  // Create a ResourceLoaderOptions for preflight.
-  ResourceLoaderOptions preflight_options = cross_origin_options;
-
-  actual_request_ = cross_origin_request;
-  actual_options_ = cross_origin_options;
+  // We don't want any requests that could involve a CORS preflight to get
+  // intercepted by a foreign SW, even if we have the result of the preflight
+  // cached already. See https://crbug.com/674370.
+  cross_origin_request.SetServiceWorkerMode(
+      WebURLRequest::ServiceWorkerMode::kNone);
 
-  LoadRequest(preflight_request, preflight_options);
+  PrepareCrossOriginRequest(cross_origin_request);
+  LoadRequest(cross_origin_request, cross_origin_options);
 }
 
 DocumentThreadableLoader::~DocumentThreadableLoader() {

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

@@ -136,9 +136,12 @@ class CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader,
   void MakeCrossOriginAccessRequest(const ResourceRequest&);
   // Loads m_fallbackRequestForServiceWorker.
   void LoadFallbackRequestForServiceWorker();
-  // Loads m_actualRequest.
+  // Issues a CORS preflight.
+  void LoadPreflightRequest(const ResourceRequest&,
+                            const ResourceLoaderOptions&);
+  // Loads actual_request_.
   void LoadActualRequest();
-  // Clears m_actualRequest and reports access control check failure to
+  // Clears actual_request_ and reports access control check failure to
   // m_client.
   void HandlePreflightFailure(const String& url,
                               const String& error_description);
@@ -152,7 +155,8 @@ class CORE_EXPORT DocumentThreadableLoader final : public ThreadableLoader,
   void LoadRequestAsync(const ResourceRequest&, ResourceLoaderOptions);
   void LoadRequestSync(const ResourceRequest&, ResourceLoaderOptions);
 
-  void PrepareCrossOriginRequest(ResourceRequest&);
+  void PrepareCrossOriginRequest(ResourceRequest&) const;
+
   // This method modifies the ResourceRequest by calling
   // SetAllowStoredCredentials() on it based on same-origin-ness and the
   // credentials mode.