[OOR-CORS] Implement redirect logic in CORSURLLoader

This CL adds redirect logic to CORSURLLoader. There are some TODOs and
I'll address them later.

Bug: 736308
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I8db4eccb6c5bb453882f6a469d4185487848773a
Reviewed-on: https://chromium-review.googlesource.com/1088715Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566751}

services/network/cors/cors_url_loader.h

@@ -29,7 +29,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoader
       public mojom::URLLoaderClient {
  public:
   // Assumes network_loader_factory outlives this loader.
-  // TODO(yhirano): Remove |preflight_finalizer| when the network service is
+  // TODO(yhirano): Remove |request_finalizer| when the network service is
   // fully enabled.
   CORSURLLoader(
       int32_t routing_id,
@@ -39,7 +39,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoader
       mojom::URLLoaderClientPtr client,
       const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
       mojom::URLLoaderFactory* network_loader_factory,
-      const base::RepeatingCallback<void(int)>& preflight_finalizer);
+      const base::RepeatingCallback<void(int)>& request_finalizer);
 
   ~CORSURLLoader() override;
 
@@ -68,12 +68,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoader
   void OnComplete(const URLLoaderCompletionStatus& status) override;
 
  private:
-  void StartNetworkRequest(
-      int32_t routing_id,
-      int32_t request_id,
-      uint32_t options,
-      const net::MutableNetworkTrafficAnnotationTag& traffic_annotation,
-      base::Optional<CORSErrorStatus> status);
+  void StartRequest();
+  void StartNetworkRequest(base::Optional<CORSErrorStatus> status);
 
   // Called when there is a connection error on the upstream pipe used for the
   // actual request.
@@ -82,6 +78,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoader
   // Handles OnComplete() callback.
   void HandleComplete(const URLLoaderCompletionStatus& status);
 
+  // We need to save these for redirect.
+  const int32_t routing_id_;
+  const int32_t request_id_;
+  const uint32_t options_;
+
   // This raw URLLoaderFactory pointer is shared with the CORSURLLoaderFactory
   // that created and owns this object.
   mojom::URLLoaderFactory* network_loader_factory_;
@@ -105,6 +106,21 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CORSURLLoader
   // Corresponds to the Fetch spec, https://fetch.spec.whatwg.org/.
   bool fetch_cors_flag_;
 
+  net::RedirectInfo redirect_info_;
+
+  // https://fetch.spec.whatwg.org/#concept-request-tainted-origin
+  bool tainted_ = false;
+
+  // https://fetch.spec.whatwg.org/#concept-request-redirect-count
+  int redirect_count_ = 0;
+
+  // Used to finalize preflight / redirect requests.
+  // TODO(yhirano): Remove this once the network service is fully enabled.
+  base::RepeatingCallback<void(int)> request_finalizer_;
+
+  // We need to save this for redirect.
+  net::MutableNetworkTrafficAnnotationTag traffic_annotation_;
+
   // Used to run asynchronous class instance bound callbacks safely.
   base::WeakPtrFactory<CORSURLLoader> weak_factory_;
 

services/network/cors/preflight_controller.cc

@@ -61,7 +61,8 @@ std::string CreateAccessControlRequestHeadersHeader(
 }
 
 std::unique_ptr<ResourceRequest> CreatePreflightRequest(
-    const ResourceRequest& request) {
+    const ResourceRequest& request,
+    bool tainted) {
   DCHECK(!request.url.has_username());
   DCHECK(!request.url.has_password());
 
@@ -98,8 +99,9 @@ std::unique_ptr<ResourceRequest> CreatePreflightRequest(
 
   DCHECK(request.request_initiator);
   preflight_request->request_initiator = request.request_initiator;
-  preflight_request->headers.SetHeader(net::HttpRequestHeaders::kOrigin,
-                                       request.request_initiator->Serialize());
+  preflight_request->headers.SetHeader(
+      net::HttpRequestHeaders::kOrigin,
+      (tainted ? url::Origin() : *request.request_initiator).Serialize());
 
   // TODO(toyoshim): Remove the following line once the network service is
   // enabled by default.
@@ -116,6 +118,7 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
     const GURL& final_url,
     const ResourceResponseHead& head,
     const ResourceRequest& original_request,
+    bool tainted,
     base::Optional<mojom::CORSError>* detected_error) {
   DCHECK(detected_error);
 
@@ -127,7 +130,8 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
       GetHeaderString(head.headers,
                       cors::header_names::kAccessControlAllowCredentials),
       original_request.fetch_credentials_mode,
-      *original_request.request_initiator, false /* allow_file_origin */);
+      tainted ? url::Origin() : *original_request.request_initiator,
+      false /* allow_file_origin */);
   if (*detected_error)
     return nullptr;
 
@@ -225,13 +229,15 @@ class PreflightController::PreflightLoader final {
   PreflightLoader(PreflightController* controller,
                   CompletionCallback completion_callback,
                   const ResourceRequest& request,
+                  bool tainted,
                   const net::NetworkTrafficAnnotationTag& annotation_tag,
                   base::OnceCallback<void()> preflight_finalizer)
       : controller_(controller),
         completion_callback_(std::move(completion_callback)),
         original_request_(request),
+        tainted_(tainted),
         preflight_finalizer_(std::move(preflight_finalizer)) {
-    loader_ = SimpleURLLoader::Create(CreatePreflightRequest(request),
+    loader_ = SimpleURLLoader::Create(CreatePreflightRequest(request, tainted),
                                       annotation_tag);
   }
 
@@ -275,7 +281,7 @@ class PreflightController::PreflightLoader final {
 
     base::Optional<mojom::CORSError> detected_error;
     std::unique_ptr<PreflightResult> result = CreatePreflightResult(
-        final_url, head, original_request_, &detected_error);
+        final_url, head, original_request_, tainted_, &detected_error);
 
     base::Optional<CORSErrorStatus> detected_error_status;
     if (result) {
@@ -331,6 +337,8 @@ class PreflightController::PreflightLoader final {
   PreflightController::CompletionCallback completion_callback_;
   const ResourceRequest original_request_;
 
+  const bool tainted_;
+
   // This is needed because we sometimes need to cancel the preflight loader
   // synchronously.
   // TODO(yhirano): Remove this when the network service is fully enabled.
@@ -342,8 +350,9 @@ class PreflightController::PreflightLoader final {
 // static
 std::unique_ptr<ResourceRequest>
 PreflightController::CreatePreflightRequestForTesting(
-    const ResourceRequest& request) {
-  return CreatePreflightRequest(request);
+    const ResourceRequest& request,
+    bool tainted) {
+  return CreatePreflightRequest(request, tainted);
 }
 
 // static
@@ -360,6 +369,7 @@ void PreflightController::PerformPreflightCheck(
     CompletionCallback callback,
     int32_t request_id,
     const ResourceRequest& request,
+    bool tainted,
     const net::NetworkTrafficAnnotationTag& annotation_tag,
     mojom::URLLoaderFactory* loader_factory,
     base::OnceCallback<void()> preflight_finalizer) {
@@ -374,7 +384,7 @@ void PreflightController::PerformPreflightCheck(
   }
 
   auto emplaced_pair = loaders_.emplace(std::make_unique<PreflightLoader>(
-      this, std::move(callback), request, annotation_tag,
+      this, std::move(callback), request, tainted, annotation_tag,
       std::move(preflight_finalizer)));
   (*emplaced_pair.first)->Request(loader_factory, request_id);
 }

services/network/cors/preflight_controller.h

@@ -37,7 +37,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PreflightController final {
   // Creates a CORS-preflight ResourceRequest for a specified |request| for a
   // URL that is originally requested.
   static std::unique_ptr<ResourceRequest> CreatePreflightRequestForTesting(
-      const ResourceRequest& request);
+      const ResourceRequest& request,
+      bool tainted = false);
 
   // Obtains the shared default controller instance.
   // TODO(toyoshim): Find a right owner rather than a single design.
@@ -61,6 +62,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) PreflightController final {
       CompletionCallback callback,
       int32_t request_id,
       const ResourceRequest& resource_request,
+      bool tainted,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
       mojom::URLLoaderFactory* loader_factory,
       base::OnceCallback<void()> preflight_finalizer);

services/network/cors/preflight_controller_unittest.cc

@@ -130,6 +130,19 @@ TEST(PreflightControllerCreatePreflightRequestTest, ExcludeForbiddenHeaders) {
       cors::header_names::kAccessControlRequestHeaders, &header));
 }
 
+TEST(PreflightControllerCreatePreflightRequestTest, Tainted) {
+  ResourceRequest request;
+  request.request_initiator = url::Origin::Create(GURL("https://example.com"));
+
+  std::unique_ptr<ResourceRequest> preflight =
+      PreflightController::CreatePreflightRequestForTesting(request, true);
+
+  std::string header;
+  EXPECT_TRUE(
+      preflight->headers.GetHeader(net::HttpRequestHeaders::kOrigin, &header));
+  EXPECT_EQ(header, "null");
+}
+
 class PreflightControllerTest : public testing::Test {
  public:
   PreflightControllerTest()
@@ -161,13 +174,14 @@ class PreflightControllerTest : public testing::Test {
 
   GURL GetURL(const std::string& path) { return test_server_.GetURL(path); }
 
-  void PerformPreflightCheck(const ResourceRequest& request) {
+  void PerformPreflightCheck(const ResourceRequest& request,
+                             bool tainted = false) {
     DCHECK(preflight_controller_);
     run_loop_ = std::make_unique<base::RunLoop>();
     preflight_controller_->PerformPreflightCheck(
         base::BindOnce(&PreflightControllerTest::HandleRequestCompletion,
                        base::Unretained(this)),
-        0 /* request_id */, request, TRAFFIC_ANNOTATION_FOR_TESTS,
+        0 /* request_id */, request, tainted, TRAFFIC_ANNOTATION_FOR_TESTS,
         url_loader_factory_ptr_.get(),
         base::BindOnce(&PreflightControllerTest::CancelPreflight,
                        base::Unretained(this)));
@@ -198,11 +212,15 @@ class PreflightControllerTest : public testing::Test {
 
     response = std::make_unique<net::test_server::BasicHttpResponse>();
     if (net::test_server::ShouldHandle(request, "/404") ||
-        net::test_server::ShouldHandle(request, "/allow")) {
+        net::test_server::ShouldHandle(request, "/allow") ||
+        net::test_server::ShouldHandle(request, "/tainted")) {
       response->set_code(net::test_server::ShouldHandle(request, "/404")
                              ? net::HTTP_NOT_FOUND
                              : net::HTTP_OK);
-      url::Origin origin = url::Origin::Create(test_server_.base_url());
+      const url::Origin origin =
+          net::test_server::ShouldHandle(request, "/tainted")
+              ? url::Origin()
+              : url::Origin::Create(test_server_.base_url());
       response->AddCustomHeader(cors::header_names::kAccessControlAllowOrigin,
                                 origin.Serialize());
       response->AddCustomHeader(header_names::kAccessControlAllowMethods,
@@ -257,6 +275,16 @@ TEST_F(PreflightControllerTest, CheckValidRequest) {
   EXPECT_EQ(1u, access_count());  // Should be from the preflight cache.
 }
 
+TEST_F(PreflightControllerTest, CheckTaintedRequest) {
+  ResourceRequest request;
+  request.url = GetURL("/tainted");
+  request.request_initiator = url::Origin::Create(request.url);
+
+  PerformPreflightCheck(request, true /* tainted */);
+  ASSERT_FALSE(status());
+  EXPECT_EQ(1u, access_count());
+}
+
 // TODO(yhirano): Remove this test case when the network service is fully
 // enabled.
 TEST_F(PreflightControllerTest, CancelPreflightIsCalled) {

third_party/WebKit/LayoutTests/TestExpectations

@@ -1786,10 +1786,6 @@ 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 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/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 ]
@@ -1849,7 +1845,6 @@ crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/origin-exact-
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/origin-exact-matching/46.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/origin-exact-matching/47.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/origin-whitelisting-ip-addresses.html [ Failure ]
-crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/redirect-cors-origin-null.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/workers/xmlhttprequest-allowed-with-disabled-web-security.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/xmlhttprequest-allowed-with-disabled-web-security.html [ Failure ]
 # ====== Out of Blink CORS related tests END ======

third_party/WebKit/LayoutTests/virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any-expected.txt

-This is a testharness.js-based test.
-PASS Set cookies
-PASS Testing credentials after cross-origin redirection with CORS and no preflight
-FAIL Testing credentials after cross-origin redirection with CORS and preflight promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch"
-PASS Clean cookies
-Harness: the test ran to completion.
-

third_party/WebKit/LayoutTests/virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any.worker-expected.txt

-This is a testharness.js-based test.
-PASS Set cookies
-PASS Testing credentials after cross-origin redirection with CORS and no preflight
-FAIL Testing credentials after cross-origin redirection with CORS and preflight promise_test: Unhandled rejection with value: object "TypeError: Failed to fetch"
-PASS Clean cookies
-Harness: the test ran to completion.
-

third_party/WebKit/LayoutTests/virtual/outofblink-cors/http/tests/xmlhttprequest/open-in-body-onload-sync-to-invalid-redirect-response-handling-expected.txt

+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+CONSOLE ERROR: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+XMLHttpRequest doesn't crash even when open() is invoked synchronously to handling of a redirect response to a cross-origin request.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

third_party/WebKit/LayoutTests/virtual/outofblink-cors/http/tests/xmlhttprequest/redirect-cross-origin-sync-double-expected.txt

+CONSOLE WARNING: line 25: Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
+CONSOLE ERROR: line 26: Failed to load http://localhost:8000/xmlhttprequest/resources/redirect.php?url=/xmlhttprequest/resources/reply.xml: No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'http://127.0.0.1:8000' is therefore not allowed access.
+Test that a cross-origin chain of redirects to a server that responds is indistinguishable from one that does not. Should say PASS:
+
+PASS
+

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

@@ -604,6 +604,7 @@ bool DocumentThreadableLoader::RedirectReceived(
   const KURL& original_url = redirect_response.Url();
 
   if (!actual_request_.IsNull()) {
+    DCHECK(!out_of_blink_cors_);
     ReportResponseReceived(resource->Identifier(), redirect_response);
 
     HandlePreflightFailure(
@@ -643,6 +644,15 @@ bool DocumentThreadableLoader::RedirectReceived(
     return false;
   }
 
+  if (out_of_blink_cors_) {
+    client_->DidReceiveRedirectTo(new_url);
+    if (client_->IsDocumentThreadableLoaderClient()) {
+      return static_cast<DocumentThreadableLoaderClient*>(client_)
+          ->WillFollowRedirect(new_url, redirect_response);
+    }
+    return true;
+  }
+
   // Allow same origin requests to continue after allowing clients to audit the
   // redirect.
   if (IsAllowedRedirect(new_request.GetFetchRequestMode(), new_url)) {