OOR-CORS: Skip CORS preflight cache if the original request wants

CORS preflight cache wasn't skipped regardless of the load_flags,
but with this change, it is skipped if the original request has
one of flags that want to skip caches.

Also, we do not store results to the cache if net::LOAD_DISABLE_CACHE
is specified.

Bug: 941297
Change-Id: Ibf356949dd6267eb65faf650d76b0c5e58ce5d5d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1520427Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#641113}

content/browser/loader/cors_preflight_cache_browsertest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/macros.h"
+#include "base/strings/string16.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/lock.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/thread_annotations.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/content_browser_test_utils.h"
+#include "content/shell/browser/shell.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 "services/network/public/cpp/cors/cors.h"
+#include "services/network/public/cpp/features.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+const char kTestPath[] = "/loader/cors_preflight.html";
+const base::string16 kTestDone = base::string16(base::ASCIIToUTF16("DONE"));
+
+// Tests end to end behaviors on CORS preflight and its cache.
+class CorsPreflightCacheBrowserTest : public ContentBrowserTest {
+ protected:
+  CorsPreflightCacheBrowserTest() {
+    scoped_feature_list_.InitWithFeatures({network::features::kOutOfBlinkCors,
+                                           network::features::kNetworkService},
+                                          {});
+  }
+  ~CorsPreflightCacheBrowserTest() override = default;
+
+  void SetUpOnMainThread() override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+
+    cross_origin_test_server_.RegisterRequestHandler(base::BindRepeating(
+        &CorsPreflightCacheBrowserTest::HandleRequest, base::Unretained(this)));
+    ASSERT_TRUE(cross_origin_test_server_.Start());
+  }
+
+ protected:
+  uint16_t cross_origin_port() { return cross_origin_test_server_.port(); }
+  size_t options_count() {
+    base::AutoLock lock(lock_);
+    return options_count_;
+  }
+  size_t get_count() {
+    base::AutoLock lock(lock_);
+    return get_count_;
+  }
+
+ private:
+  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request) {
+    std::unique_ptr<net::test_server::BasicHttpResponse> response =
+        std::make_unique<net::test_server::BasicHttpResponse>();
+    response->set_code(net::HTTP_OK);
+    response->AddCustomHeader(
+        network::cors::header_names::kAccessControlAllowOrigin, "*");
+    if (request.method == net::test_server::METHOD_OPTIONS) {
+      response->AddCustomHeader(
+          network::cors::header_names::kAccessControlAllowMethods,
+          "GET, OPTIONS");
+      response->AddCustomHeader(
+          network::cors::header_names::kAccessControlAllowHeaders, "foo");
+      response->AddCustomHeader(
+          network::cors::header_names::kAccessControlMaxAge, "60");
+      base::AutoLock lock(lock_);
+      options_count_++;
+    } else if (request.method == net::test_server::METHOD_GET) {
+      base::AutoLock lock(lock_);
+      get_count_++;
+    }
+    return response;
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+  net::EmbeddedTestServer cross_origin_test_server_;
+  base::Lock lock_;
+
+  size_t options_count_ GUARDED_BY(lock_) = 0;
+  size_t get_count_ GUARDED_BY(lock_) = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(CorsPreflightCacheBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(CorsPreflightCacheBrowserTest, Default) {
+  std::unique_ptr<TitleWatcher> watcher1 =
+      std::make_unique<TitleWatcher>(shell()->web_contents(), kTestDone);
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(base::StringPrintf(
+                   "%s?;%d;default", kTestPath, cross_origin_port()))));
+  EXPECT_EQ(kTestDone, watcher1->WaitAndGetTitle());
+  EXPECT_EQ(1u, options_count());
+  EXPECT_EQ(1u, get_count());
+
+  std::unique_ptr<TitleWatcher> watcher2 =
+      std::make_unique<TitleWatcher>(shell()->web_contents(), kTestDone);
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(base::StringPrintf(
+                   "%s?;%d;default", kTestPath, cross_origin_port()))));
+  EXPECT_EQ(kTestDone, watcher2->WaitAndGetTitle());
+  EXPECT_EQ(1u, options_count());
+  EXPECT_EQ(2u, get_count());
+
+  std::unique_ptr<TitleWatcher> watcher3 =
+      std::make_unique<TitleWatcher>(shell()->web_contents(), kTestDone);
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(base::StringPrintf(
+                   "%s?;%d;reload", kTestPath, cross_origin_port()))));
+  EXPECT_EQ(kTestDone, watcher3->WaitAndGetTitle());
+  EXPECT_EQ(2u, options_count());
+  EXPECT_EQ(3u, get_count());
+}
+
+}  // namespace
+
+}  // namespace content

content/test/BUILD.gn

@@ -821,6 +821,7 @@ test("content_browsertests") {
     "../browser/keyboard_lock_browsertest.h",
     "../browser/keyboard_lock_browsertest_mac.mm",
     "../browser/loader/cors_file_origin_browsertest.cc",
+    "../browser/loader/cors_preflight_cache_browsertest.cc",
     "../browser/loader/cross_site_document_blocking_browsertest.cc",
     "../browser/loader/loader_browsertest.cc",
     "../browser/loader/prefetch_browsertest.cc",

content/test/data/loader/cors_preflight.html

+<html>
+<head>
+<script>
+const cors_url = location.protocol + "//" + location.hostname + ":" +
+    location.search.split(';')[1];
+const cache_mode = location.search.split(';')[2];
+fetch(cors_url, {cache: cache_mode, headers: [["foo", "bar"]]}).then(() => {
+  document.title = "DONE";
+});
+</script>
+</head>
+</html>

services/network/cors/preflight_controller.cc

@@ -27,6 +27,11 @@ namespace cors {
 
 namespace {
 
+int RetrieveCacheFlags(int load_flags) {
+  return load_flags & (net::LOAD_VALIDATE_CACHE | net::LOAD_BYPASS_CACHE |
+                       net::LOAD_DISABLE_CACHE);
+}
+
 base::Optional<std::string> GetHeaderString(
     const scoped_refptr<net::HttpResponseHeaders>& headers,
     const std::string& header_name) {
@@ -81,6 +86,7 @@ std::unique_ptr<ResourceRequest> CreatePreflightRequest(
 
   preflight_request->fetch_credentials_mode =
       mojom::FetchCredentialsMode::kOmit;
+  preflight_request->load_flags = RetrieveCacheFlags(request.load_flags);
   preflight_request->load_flags |= net::LOAD_DO_NOT_SAVE_COOKIES;
   preflight_request->load_flags |= net::LOAD_DO_NOT_SEND_COOKIES;
   preflight_request->load_flags |= net::LOAD_DO_NOT_SEND_AUTH_DATA;
@@ -308,9 +314,8 @@ class PreflightController::PreflightLoader final {
           CheckPreflightResult(result.get(), original_request_);
     }
 
-    // TODO(toyoshim): Check the spec if we cache |result| regardless of
-    // following checks.
-    if (!detected_error_status) {
+    if (!(original_request_.load_flags & net::LOAD_DISABLE_CACHE) &&
+        !detected_error_status) {
       controller_->AppendToCache(*original_request_.request_initiator,
                                  original_request_.url, std::move(result));
     }
@@ -406,7 +411,7 @@ void PreflightController::PerformPreflightCheck(
     base::OnceCallback<void()> preflight_finalizer) {
   DCHECK(request.request_initiator);
 
-  if (!request.is_external_request &&
+  if (!RetrieveCacheFlags(request.load_flags) && !request.is_external_request &&
       cache_.CheckIfRequestCanSkipPreflight(
           request.request_initiator->Serialize(), request.url,
           request.fetch_credentials_mode, request.method, request.headers,

services/network/cors/preflight_controller_unittest.cc

@@ -350,6 +350,25 @@ TEST_F(PreflightControllerTest, CheckValidRequest) {
   EXPECT_EQ(net::OK, net_error());
   ASSERT_FALSE(status());
   EXPECT_EQ(1u, access_count());  // Should be from the preflight cache.
+
+  // Verify if cache related flags work to skip the preflight cache.
+  request.load_flags = net::LOAD_VALIDATE_CACHE;
+  PerformPreflightCheck(request);
+  EXPECT_EQ(net::OK, net_error());
+  ASSERT_FALSE(status());
+  EXPECT_EQ(2u, access_count());
+
+  request.load_flags = net::LOAD_BYPASS_CACHE;
+  PerformPreflightCheck(request);
+  EXPECT_EQ(net::OK, net_error());
+  ASSERT_FALSE(status());
+  EXPECT_EQ(3u, access_count());
+
+  request.load_flags = net::LOAD_DISABLE_CACHE;
+  PerformPreflightCheck(request);
+  EXPECT_EQ(net::OK, net_error());
+  ASSERT_FALSE(status());
+  EXPECT_EQ(4u, access_count());
 }
 
 TEST_F(PreflightControllerTest, CheckTaintedRequest) {