Reject insane requests in CORSURLLoaderFactory

With this CL, CORSURLLoaderFactory rejects ill-configuared requests.

 - CORS needs a proper origin (including an opaque unique origin)
   attached to a request. Hence CORSURLLoaderFactory rejects a request
   which has a CORS-enabled mode and null request_initiator. Also,
   a request with null request_initiator won't set the CORS flag with
   this CL.
 - The relationship between fetch credentials mode and load_flags is
   a bit unclear. If a request's credentials mode is "omit" but
   one of LOAD_DO_NOT_SAVE_COOKIES, LOAD_DO_NOT_SEND_COOKIES and
   LOAD_DO_NOT_SEND_AUTH_DATA is not set on load_flags, that is likely
   a mis-configuration, so fail the request.

Bug: 736308, 862184
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I51fb491b865de330b22b028a0eddbc30043e6b69
Reviewed-on: https://chromium-review.googlesource.com/1136342
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576430}

services/network/cors/cors_url_loader.cc

@@ -21,9 +21,10 @@ bool CalculateCORSFlag(const ResourceRequest& request) {
       request.fetch_request_mode == mojom::FetchRequestMode::kNoCORS) {
     return false;
   }
+  // CORS needs a proper origin (including a unique opaque origin). If the
+  // request doesn't have one, CORS should not work.
+  DCHECK(request.request_initiator);
   url::Origin url_origin = url::Origin::Create(request.url);
-  if (!request.request_initiator.has_value())
-    return true;
   url::Origin security_origin(request.request_initiator.value());
   return !security_origin.IsSameOriginWith(url_origin);
 }
@@ -284,7 +285,7 @@ void CORSURLLoader::OnReceiveRedirect(
   // |request|’s current url’s origin and |request|’s origin is not same origin
   // with |request|’s current url’s origin, then set |request|’s tainted origin
   // flag.
-  if (!request_.request_initiator ||
+  if (request_.request_initiator &&
       (!url::Origin::Create(redirect_info.new_url)
             .IsSameOriginWith(url::Origin::Create(request_.url)) &&
        !request_.request_initiator->IsSameOriginWith(
@@ -353,17 +354,16 @@ void CORSURLLoader::StartRequest() {
   // |httpRequest|’s header list.
   if (fetch_cors_flag_ ||
       (request_.method != "GET" && request_.method != "HEAD")) {
-    url::Origin empty_origin;
-    const url::Origin& origin = tainted_ || !request_.request_initiator
-                                    ? empty_origin
-                                    : *request_.request_initiator;
-    request_.headers.SetHeader(net::HttpRequestHeaders::kOrigin,
-                               origin.Serialize());
+    if (request_.request_initiator) {
+      request_.headers.SetHeader(
+          net::HttpRequestHeaders::kOrigin,
+          (tainted_ ? url::Origin() : *request_.request_initiator).Serialize());
+    }
   }
 
   if (request_.fetch_request_mode == mojom::FetchRequestMode::kSameOrigin) {
-    if (!request_.request_initiator ||
-        !request_.request_initiator->IsSameOriginWith(
+    DCHECK(request_.request_initiator);
+    if (!request_.request_initiator->IsSameOriginWith(
             url::Origin::Create(request_.url))) {
       HandleComplete(URLLoaderCompletionStatus(
           CORSErrorStatus(mojom::CORSError::kDisallowedByMode)));

services/network/cors/cors_url_loader_factory.cc

@@ -4,9 +4,11 @@
 
 #include "services/network/cors/cors_url_loader_factory.h"
 
+#include "base/logging.h"
 #include "net/base/load_flags.h"
 #include "services/network/cors/cors_url_loader.h"
 #include "services/network/network_context.h"
+#include "services/network/public/cpp/cors/cors.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/resource_request.h"
 #include "services/network/public/mojom/fetch_api.mojom.h"
@@ -66,6 +68,11 @@ void CORSURLLoaderFactory::CreateLoaderAndStart(
     const ResourceRequest& resource_request,
     mojom::URLLoaderClientPtr client,
     const net::MutableNetworkTrafficAnnotationTag& traffic_annotation) {
+  if (!IsSane(resource_request)) {
+    client->OnComplete(URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
+    return;
+  }
+
   if (base::FeatureList::IsEnabled(features::kOutOfBlinkCORS) &&
       !disable_web_security_) {
     auto loader = std::make_unique<CORSURLLoader>(
@@ -96,6 +103,33 @@ void CORSURLLoaderFactory::DeleteIfNeeded() {
     context_->DestroyURLLoaderFactory(this);
 }
 
+bool CORSURLLoaderFactory::IsSane(const ResourceRequest& request) {
+  // CORS needs a proper origin (including a unique opaque origin). If the
+  // request doesn't have one, CORS cannot work.
+  if (!request.request_initiator &&
+      request.fetch_request_mode != mojom::FetchRequestMode::kNavigate &&
+      request.fetch_request_mode != mojom::FetchRequestMode::kNoCORS) {
+    LOG(WARNING) << "|fetch_request_mode| is " << request.fetch_request_mode
+                 << ", but |request_initiator| is not set.";
+    return false;
+  }
+
+  const auto load_flags_pattern = net::LOAD_DO_NOT_SAVE_COOKIES |
+                                  net::LOAD_DO_NOT_SEND_COOKIES |
+                                  net::LOAD_DO_NOT_SEND_AUTH_DATA;
+  // The credentials mode and load_flags should match.
+  if (request.fetch_credentials_mode == mojom::FetchCredentialsMode::kOmit &&
+      (request.load_flags & load_flags_pattern) != load_flags_pattern) {
+    LOG(WARNING) << "|fetch_credentials_mode| and |load_flags| contradict each "
+                    "other.";
+    return false;
+  }
+
+  // TODO(yhirano): If the request mode is "no-cors", the redirect mode should
+  // be "follow".
+  return true;
+}
+
 }  // namespace cors
 
 }  // namespace network

services/network/cors/cors_url_loader_factory.h

@@ -62,6 +62,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoaderFactory final
 
   void DeleteIfNeeded();
 
+  static bool IsSane(const ResourceRequest& request);
+
   mojo::BindingSet<mojom::URLLoaderFactory> bindings_;
 
   // Used when constructed by NetworkContext.

services/network/cors/cors_url_loader_unittest.cc

@@ -251,6 +251,135 @@ class CORSURLLoaderTest : public testing::Test {
   DISALLOW_COPY_AND_ASSIGN(CORSURLLoaderTest);
 };
 
+TEST_F(CORSURLLoaderTest, SameOriginWithoutInitiator) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kSameOrigin;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kInclude;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  RunUntilComplete();
+
+  EXPECT_FALSE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, NoCORSWithoutInitiator) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNoCORS;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kInclude;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  NotifyLoaderClientOnReceiveResponse();
+  NotifyLoaderClientOnComplete(net::OK);
+  RunUntilComplete();
+
+  EXPECT_TRUE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_TRUE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::OK, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, CORSWithoutInitiator) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kCORS;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kInclude;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  RunUntilComplete();
+
+  EXPECT_FALSE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, NavigateWithoutInitiator) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNavigate;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kInclude;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  NotifyLoaderClientOnReceiveResponse();
+  NotifyLoaderClientOnComplete(net::OK);
+  RunUntilComplete();
+
+  EXPECT_TRUE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_TRUE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::OK, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, CredentialsModeAndLoadFlagsContradictEachOther1) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNavigate;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.load_flags =
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  RunUntilComplete();
+
+  EXPECT_FALSE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, CredentialsModeAndLoadFlagsContradictEachOther2) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNavigate;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.load_flags =
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_AUTH_DATA;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  RunUntilComplete();
+
+  EXPECT_FALSE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
+TEST_F(CORSURLLoaderTest, CredentialsModeAndLoadFlagsContradictEachOther3) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kNavigate;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.load_flags =
+      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SEND_AUTH_DATA;
+  request.url = GURL("http://example.com/");
+  request.request_initiator = base::nullopt;
+
+  CreateLoaderAndStart(request);
+  RunUntilComplete();
+
+  EXPECT_FALSE(IsNetworkLoaderStarted());
+  EXPECT_FALSE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
 TEST_F(CORSURLLoaderTest, SameOriginRequest) {
   const GURL url("http://example.com/foo.png");
   CreateLoaderAndStart(url.GetOrigin(), url,
@@ -292,7 +421,7 @@ TEST_F(CORSURLLoaderTest, CrossOriginRequestWithNoCORSModeAndPatchMethod) {
   const GURL url("http://other.com/foo.png");
   ResourceRequest request;
   request.fetch_request_mode = mojom::FetchRequestMode::kNoCORS;
-  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kInclude;
   request.method = "PATCH";
   request.url = url;
   request.request_initiator = url::Origin::Create(origin);

services/network/public/cpp/resource_request.h

@@ -126,6 +126,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) ResourceRequest {
 
   // https://fetch.spec.whatwg.org/#concept-request-mode
   // Used mainly by CORS handling (out-of-blink CORS), CORB, Service Worker.
+  // CORS handling needs a proper origin (including a unique opaque origin).
+  // Hence a request with kSameOrigin, kCORS, or kCORSWithForcedPreflight should
+  // have a non-null request_initiator.
   mojom::FetchRequestMode fetch_request_mode = mojom::FetchRequestMode::kNoCORS;
 
   // https://fetch.spec.whatwg.org/#concept-request-credentials-mode