OOR-CORS: implement CORS preflicht in the network service

This patch adds preflight cache and request implementation
in cors::PreflightController.

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

services/network/cors/preflight_controller.cc

@@ -6,9 +6,13 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
 #include "base/strings/string_util.h"
 #include "net/http/http_request_headers.h"
 #include "services/network/public/cpp/cors/cors.h"
+#include "services/network/public/cpp/cors/cors_error_status.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
 namespace network {
@@ -17,6 +21,15 @@ namespace cors {
 
 namespace {
 
+base::Optional<std::string> GetHeaderString(
+    const scoped_refptr<net::HttpResponseHeaders>& headers,
+    const std::string& header_name) {
+  std::string header_value;
+  if (!headers->GetNormalizedHeader(header_name, &header_value))
+    return base::nullopt;
+  return header_value;
+}
+
 // Algorithm step 3 of the CORS-preflight fetch,
 // https://fetch.spec.whatwg.org/#cors-preflight-fetch-0, that requires
 //  - CORS-safelisted request-headers excluded
@@ -46,10 +59,7 @@ std::string CreateAccessControlRequestHeadersHeader(
   return base::JoinString(filtered_headers, ",");
 }
 
-}  // namespace
-
-// static
-std::unique_ptr<ResourceRequest> PreflightController::CreatePreflightRequest(
+std::unique_ptr<ResourceRequest> CreatePreflightRequest(
     const ResourceRequest& request) {
   DCHECK(!request.url.has_username());
   DCHECK(!request.url.has_password());
@@ -96,6 +106,164 @@ std::unique_ptr<ResourceRequest> PreflightController::CreatePreflightRequest(
   return preflight_request;
 }
 
+}  // namespace
+
+class PreflightController::PreflightLoader final {
+ public:
+  PreflightLoader(PreflightController* controller,
+                  CompletionCallback completion_callback,
+                  const ResourceRequest& request,
+                  const net::NetworkTrafficAnnotationTag& annotation_tag)
+      : controller_(controller),
+        completion_callback_(std::move(completion_callback)),
+        origin_(*request.request_initiator),
+        url_(request.url),
+        credentials_mode_(request.fetch_credentials_mode) {
+    loader_ = SimpleURLLoader::Create(CreatePreflightRequest(request),
+                                      annotation_tag);
+  }
+
+  void Request(mojom::URLLoaderFactory* loader_factory) {
+    DCHECK(loader_);
+
+    loader_->SetOnRedirectCallback(base::BindRepeating(
+        &PreflightLoader::HandleRedirect, base::Unretained(this)));
+    loader_->SetOnResponseStartedCallback(base::BindRepeating(
+        &PreflightLoader::HandleResponseHeader, base::Unretained(this)));
+    loader_->DownloadToString(
+        loader_factory,
+        base::BindOnce(&PreflightLoader::HandleResponseBody,
+                       base::Unretained(this)),
+        0);
+  }
+
+ private:
+  void HandleRedirect(const net::RedirectInfo& redirect_info,
+                      const network::ResourceResponseHead& response_head) {
+    // Preflight should not allow any redirect.
+    FinalizeLoader();
+
+    // TODO(toyoshim): Define kDisallowedPreflightRedirect in a separate patch.
+    std::move(completion_callback_)
+        .Run(CORSErrorStatus(mojom::CORSError::kPreflightInvalidStatus));
+
+    RemoveFromController();
+    // |this| is deleted here.
+  }
+
+  void HandleResponseHeader(const GURL& final_url,
+                            const ResourceResponseHead& head) {
+    FinalizeLoader();
+
+    base::Optional<mojom::CORSError> error =
+        CheckPreflight(head.headers->response_code());
+
+    if (!error) {
+      // TODO(toyoshim): Reflect --allow-file-access-from-files flag.
+      error = CheckAccess(
+          final_url, head.headers->response_code(),
+          GetHeaderString(head.headers,
+                          cors::header_names::kAccessControlAllowOrigin),
+          GetHeaderString(head.headers,
+                          cors::header_names::kAccessControlAllowCredentials),
+          credentials_mode_, origin_, false /* allow_file_origin */);
+    }
+
+    // TODO(toyoshim): Add remaining checks before or after the following check.
+    // See DocumentThreadableLoader::HandlePreflightResponse() and
+    // CORS::EnsurePreflightResultAndCacheOnSuccess() in Blink.
+
+    if (!error) {
+      // TODO(toyoshim): Define header names in public/cpp/cors/cors.h.
+      result_ = PreflightResult::Create(
+          credentials_mode_,
+          GetHeaderString(head.headers, "Access-Control-Allow-Methods"),
+          GetHeaderString(head.headers, "Access-Control-Allow-Headers"),
+          GetHeaderString(head.headers, "Access-Control-Max-Age"), &error);
+    }
+
+    if (!error) {
+      DCHECK(result_);
+      controller_->AppendToCache(origin_, url_, std::move(result_));
+      std::move(completion_callback_).Run(base::nullopt);
+    } else {
+      std::move(completion_callback_).Run(CORSErrorStatus(*error));
+    }
+    RemoveFromController();
+    // |this| is deleted here.
+  }
+
+  void HandleResponseBody(std::unique_ptr<std::string> response_body) {
+    NOTREACHED();
+  }
+
+  void FinalizeLoader() {
+    DCHECK(loader_);
+    loader_.reset();
+  }
+
+  // Removes |this| instance from |controller_|. Once the method returns, |this|
+  // is already removed.
+  void RemoveFromController() { controller_->RemoveLoader(this); }
+
+  // PreflightController owns all PreflightLoader instances, and should outlive.
+  PreflightController* const controller_;
+
+  std::unique_ptr<SimpleURLLoader> loader_;
+  PreflightController::CompletionCallback completion_callback_;
+
+  const url::Origin origin_;
+  const GURL url_;
+  const mojom::FetchCredentialsMode credentials_mode_;
+
+  std::unique_ptr<PreflightResult> result_;
+
+  DISALLOW_COPY_AND_ASSIGN(PreflightLoader);
+};
+
+// static
+std::unique_ptr<ResourceRequest>
+PreflightController::CreatePreflightRequestForTesting(
+    const ResourceRequest& request) {
+  return CreatePreflightRequest(request);
+}
+
+PreflightController::PreflightController() = default;
+
+PreflightController::~PreflightController() = default;
+
+void PreflightController::PerformPreflightCheck(
+    CompletionCallback callback,
+    const ResourceRequest& request,
+    const net::NetworkTrafficAnnotationTag& annotation_tag,
+    mojom::URLLoaderFactory* loader_factory) {
+  DCHECK(request.request_initiator);
+
+  if (cache_.CheckIfRequestCanSkipPreflight(
+          request.request_initiator->Serialize(), request.url,
+          request.fetch_credentials_mode, request.method, request.headers)) {
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+
+  auto emplaced_pair = loaders_.emplace(std::make_unique<PreflightLoader>(
+      this, std::move(callback), request, annotation_tag));
+  (*emplaced_pair.first)->Request(loader_factory);
+}
+
+void PreflightController::RemoveLoader(PreflightLoader* loader) {
+  auto it = loaders_.find(loader);
+  DCHECK(it != loaders_.end());
+  loaders_.erase(it);
+}
+
+void PreflightController::AppendToCache(
+    const url::Origin& origin,
+    const GURL& url,
+    std::unique_ptr<PreflightResult> result) {
+  cache_.AppendEntry(origin.Serialize(), url, std::move(result));
+}
+
 }  // namespace cors
 
 }  // namespace network

services/network/cors/preflight_controller.h

@@ -6,9 +6,21 @@
 #define SERVICES_NETWORK_CORS_PREFLIGHT_CONTROLLER_H_
 
 #include <memory>
+#include <string>
 
+#include "base/callback.h"
 #include "base/component_export.h"
+#include "base/containers/unique_ptr_adapters.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/cpp/cors/cors_error_status.h"
+#include "services/network/public/cpp/cors/preflight_cache.h"
+#include "services/network/public/cpp/cors/preflight_result.h"
 #include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/mojom/fetch_api.mojom.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
+#include "url/gurl.h"
 
 namespace network {
 
@@ -18,18 +30,40 @@ namespace cors {
 // its result, and owning a CORS-preflight cache.
 // TODO(toyoshim): Features explained above not fully implemented yet.
 // See also https://crbug.com/803766 to check a design doc.
-class COMPONENT_EXPORT(NETWORK_SERVICE) PreflightController {
+class COMPONENT_EXPORT(NETWORK_SERVICE) PreflightController final {
  public:
+  using CompletionCallback =
+      base::OnceCallback<void(base::Optional<CORSErrorStatus>)>;
   // Creates a CORS-preflight ResourceRequest for a specified |request| for a
   // URL that is originally requested.
-  // Note: This function is exposed for testing only purpose, and production
-  // code outside this class should not call this function directly.
-  static std::unique_ptr<ResourceRequest> CreatePreflightRequest(
+  static std::unique_ptr<ResourceRequest> CreatePreflightRequestForTesting(
       const ResourceRequest& request);
 
-  // TODO(toyoshim): Implements an asynchronous interface to consult about
-  // CORS-preflight check, that manages a preflight cache, and may make a
-  // preflight request internally.
+  PreflightController();
+  ~PreflightController();
+
+  // Determines if a CORS-preflight request is needed, and checks the cache, or
+  // makes a preflight request if it is needed. A result will be notified
+  // synchronously or asynchronously.
+  void PerformPreflightCheck(
+      CompletionCallback callback,
+      const ResourceRequest& resource_request,
+      const net::NetworkTrafficAnnotationTag& traffic_annotation,
+      mojom::URLLoaderFactory* loader_factory);
+
+ private:
+  class PreflightLoader;
+
+  void RemoveLoader(PreflightLoader* loader);
+  void AppendToCache(const url::Origin& origin,
+                     const GURL& url,
+                     std::unique_ptr<PreflightResult> result);
+
+  PreflightCache cache_;
+  std::set<std::unique_ptr<PreflightLoader>, base::UniquePtrComparator>
+      loaders_;
+
+  DISALLOW_COPY_AND_ASSIGN(PreflightController);
 };
 
 }  // namespace cors

services/network/cors/preflight_controller_unittest.cc

@@ -4,8 +4,20 @@
 
 #include "services/network/cors/preflight_controller.h"
 
+#include <memory>
+
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
 #include "net/http/http_request_headers.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "net/test/embedded_test_server/request_handler_util.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/network_service.h"
 #include "services/network/public/cpp/cors/cors.h"
+#include "services/network/public/mojom/network_service.mojom.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/origin.h"
 
@@ -25,7 +37,7 @@ TEST(PreflightControllerCreatePreflightRequestTest, LexicographicalOrder) {
   request.headers.SetHeader("Strawberry", "Red");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   std::string header;
   EXPECT_TRUE(
@@ -46,7 +58,7 @@ TEST(PreflightControllerCreatePreflightRequestTest, ExcludeSimpleHeaders) {
   request.headers.SetHeader("Save-Data", "on");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   // Do not emit empty-valued headers; an empty list of non-"CORS safelisted"
   // request headers should cause "Access-Control-Request-Headers:" to be
@@ -63,7 +75,7 @@ TEST(PreflightControllerCreatePreflightRequestTest,
   request.headers.SetHeader("Content-Type", "text/plain");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   // Empty list also; see comment in test above.
   std::string header;
@@ -77,7 +89,7 @@ TEST(PreflightControllerCreatePreflightRequestTest, IncludeNonSimpleHeader) {
   request.headers.SetHeader("X-Custom-Header", "foobar");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   std::string header;
   EXPECT_TRUE(preflight->headers.GetHeader(
@@ -92,7 +104,7 @@ TEST(PreflightControllerCreatePreflightRequestTest,
   request.headers.SetHeader("Content-Type", "application/octet-stream");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   std::string header;
   EXPECT_TRUE(preflight->headers.GetHeader(
@@ -106,13 +118,127 @@ TEST(PreflightControllerCreatePreflightRequestTest, ExcludeForbiddenHeaders) {
   request.headers.SetHeader("referer", "https://www.google.com/");
 
   std::unique_ptr<ResourceRequest> preflight =
-      PreflightController::CreatePreflightRequest(request);
+      PreflightController::CreatePreflightRequestForTesting(request);
 
   std::string header;
   EXPECT_FALSE(preflight->headers.GetHeader(
       cors::header_names::kAccessControlRequestHeaders, &header));
 }
 
+class PreflightControllerTest : public testing::Test {
+ public:
+  PreflightControllerTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::IO) {
+    mojom::NetworkServicePtr network_service_ptr;
+    mojom::NetworkServiceRequest network_service_request =
+        mojo::MakeRequest(&network_service_ptr);
+    network_service_ = NetworkService::Create(
+        std::move(network_service_request), nullptr /* net_log */);
+
+    network_service_ptr->CreateNetworkContext(
+        mojo::MakeRequest(&network_context_ptr_),
+        mojom::NetworkContextParams::New());
+
+    network_context_ptr_->CreateURLLoaderFactory(
+        mojo::MakeRequest(&url_loader_factory_ptr_), 0 /* process_id */);
+  }
+
+ protected:
+  void HandleRequestCompletion(base::Optional<CORSErrorStatus> status) {
+    status_ = status;
+    run_loop_->Quit();
+  }
+
+  GURL GetURL(const std::string& path) { return test_server_.GetURL(path); }
+
+  void PerformPreflightCheck(const ResourceRequest& request) {
+    DCHECK(preflight_controller_);
+    run_loop_ = std::make_unique<base::RunLoop>();
+    preflight_controller_->PerformPreflightCheck(
+        base::BindOnce(&PreflightControllerTest::HandleRequestCompletion,
+                       base::Unretained(this)),
+        request, TRAFFIC_ANNOTATION_FOR_TESTS, url_loader_factory_ptr_.get());
+    run_loop_->Run();
+  }
+
+  base::Optional<CORSErrorStatus> status() { return status_; }
+  base::Optional<CORSErrorStatus> success() { return base::nullopt; }
+  size_t access_count() { return access_count_; }
+
+ private:
+  void SetUp() override {
+    preflight_controller_ = std::make_unique<PreflightController>();
+
+    test_server_.RegisterRequestHandler(base::BindRepeating(
+        &PreflightControllerTest::ServePreflight, base::Unretained(this)));
+
+    EXPECT_TRUE(test_server_.Start());
+  }
+
+  std::unique_ptr<net::test_server::HttpResponse> ServePreflight(
+      const net::test_server::HttpRequest& request) {
+    access_count_++;
+    std::unique_ptr<net::test_server::BasicHttpResponse> response;
+    if (request.method != net::test_server::METHOD_OPTIONS)
+      return response;
+
+    response = std::make_unique<net::test_server::BasicHttpResponse>();
+    if (net::test_server::ShouldHandle(request, "/404")) {
+      response->set_code(net::HTTP_NOT_FOUND);
+    } else if (net::test_server::ShouldHandle(request, "/allow")) {
+      response->set_code(net::HTTP_OK);
+      url::Origin origin = url::Origin::Create(test_server_.base_url());
+      response->AddCustomHeader(cors::header_names::kAccessControlAllowOrigin,
+                                origin.Serialize());
+      // TODO(toyoshim): Define header names in public/cpp/cors/cors.h
+      response->AddCustomHeader("Access-Control-Allow-Methods", "GET, OPTIONS");
+      response->AddCustomHeader("Access-Control-Max-Age", "1000");
+      response->AddCustomHeader(net::HttpRequestHeaders::kCacheControl,
+                                "no-store");
+    }
+    return response;
+  }
+
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+
+  std::unique_ptr<mojom::NetworkService> network_service_;
+  mojom::NetworkContextPtr network_context_ptr_;
+  mojom::URLLoaderFactoryPtr url_loader_factory_ptr_;
+
+  net::test_server::EmbeddedTestServer test_server_;
+  size_t access_count_ = 0;
+
+  std::unique_ptr<PreflightController> preflight_controller_;
+  base::Optional<CORSErrorStatus> status_;
+};
+
+TEST_F(PreflightControllerTest, CheckInvalidRequest) {
+  ResourceRequest request;
+  request.url = GetURL("/404");
+  request.request_initiator = url::Origin::Create(request.url);
+
+  PerformPreflightCheck(request);
+  ASSERT_TRUE(status());
+  EXPECT_EQ(mojom::CORSError::kPreflightInvalidStatus, status()->cors_error);
+  EXPECT_EQ(1u, access_count());
+}
+
+TEST_F(PreflightControllerTest, CheckValidRequest) {
+  ResourceRequest request;
+  request.url = GetURL("/allow");
+  request.request_initiator = url::Origin::Create(request.url);
+
+  PerformPreflightCheck(request);
+  ASSERT_FALSE(status());
+  EXPECT_EQ(1u, access_count());
+
+  PerformPreflightCheck(request);
+  ASSERT_FALSE(status());
+  EXPECT_EQ(1u, access_count());  // Should be from the preflight cache.
+}
+
 }  // namespace
 
 }  // namespace cors

services/network/public/mojom/cors.mojom

@@ -23,7 +23,9 @@ enum CORSError {
   kDisallowCredentialsNotSetToTrue,
 
   // Preflight
+  // Failed to check HTTP response ok status in CORS-preflight response.
   kPreflightInvalidStatus,
+
   // "Access-Control-Allow-External:"
   // ( https://wicg.github.io/cors-rfc1918/#headers ) specific error
   // conditions: