Handle Origin header for Vary checks in network service memory cache

This CL allows network service memory cache to use `Origin` header for
Vary checks. `Origin` is usually unspecified or specified by renderers
like blink::ResourceRequestHead::NeedsHTTPOrigin().

The underlying layer modifies `Origin` when cross origin redirects
happen. To handle that case, this CL deletes stored responses when
redirects happen.

Bug: 1339708
Change-Id: I92d1a8414db797f91eecb9445fd0af985176342c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3760336
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1025055}

services/network/network_service_memory_cache.cc

@@ -75,6 +75,19 @@ std::string GenerateCacheKeyForResourceRequest(
       /*use_single_keyed_cache=*/false, /*single_key_checksum=*/"");
 }
 
+std::string GenerateCacheKeyForURLRequest(
+    const net::URLRequest& url_request,
+    mojom::RequestDestination request_destination) {
+  bool is_subframe_document_resource =
+      request_destination == mojom::RequestDestination::kIframe;
+  return net::HttpCache::GenerateCacheKey(
+      url_request.url(), url_request.load_flags(),
+      url_request.isolation_info().network_isolation_key(),
+      /*upload_data_identifier=*/0, is_subframe_document_resource,
+      /*use_single_keyed_cache=*/false,
+      /*single_key_checksum=*/"");
+}
+
 bool CheckCrossOriginReadBlocking(const ResourceRequest& resource_request,
                                   const mojom::URLResponseHead& response,
                                   const base::RefCountedBytes& content) {
@@ -105,6 +118,32 @@ bool CheckCrossOriginReadBlocking(const ResourceRequest& resource_request,
   return decision == corb::ResponseAnalyzer::Decision::kAllow;
 }
 
+// Checks whether Vary header in the cached response only has headers that the
+// in-memory cache can handle.
+bool VaryHasSupportedHeadersOnly(
+    const net::HttpResponseHeaders& cached_response_headers) {
+  size_t iter = 0;
+  std::string value;
+  while (cached_response_headers.EnumerateHeader(
+      &iter, net::HttpResponseHeaders::kVary, &value)) {
+    // Accept-Encoding will be set if missing.
+    if (value == net::HttpRequestHeaders::kAcceptEncoding)
+      continue;
+    // Origin header might be missing or already be specified by the client
+    // side. The underlying layer (URLLoader and //net) didn't set/update Origin
+    // header unless cross-origin redirects happened. The in-memory cache
+    // doesn't store response when redirects happened.
+    if (value == net::HttpRequestHeaders::kOrigin)
+      continue;
+
+    // TODO(https://crbug.com/1339708): Support more headers. We need to extract
+    // some header calculations from net::URLRequestHttpJob.
+    return false;
+  }
+
+  return true;
+}
+
 bool MatchVaryHeader(const ResourceRequest& resource_request,
                      const net::HttpVaryData& vary_data,
                      const net::HttpResponseHeaders& cached_response_headers,
@@ -114,15 +153,7 @@ bool MatchVaryHeader(const ResourceRequest& resource_request,
     return true;
   }
 
-  std::string vary_value;
-  // There should be Vary header when `vary_data` is valid.
-  CHECK(cached_response_headers.GetNormalizedHeader(
-      net::HttpResponseHeaders::kVary, &vary_value));
-
-  // Currently the in-memory cache can only handle Accept-Encoding header.
-  // TODO(https://crbug.com/1339708): Support more headers. We need to extract
-  // some header calculations from net::URLRequestHttpJob.
-  if (vary_value != net::HttpRequestHeaders::kAcceptEncoding)
+  if (!VaryHasSupportedHeadersOnly(cached_response_headers))
     return false;
 
   net::HttpRequestInfo request_info;
@@ -234,14 +265,8 @@ NetworkServiceMemoryCache::MaybeCreateWriter(
   if (validation_type != net::VALIDATION_NONE)
     return nullptr;
 
-  bool is_subframe_document_resource =
-      request_destination == mojom::RequestDestination::kIframe;
-  std::string cache_key = net::HttpCache::GenerateCacheKey(
-      url_request->url(), url_request->load_flags(),
-      url_request->isolation_info().network_isolation_key(),
-      /*upload_data_identifier=*/0, is_subframe_document_resource,
-      /*use_single_keyed_cache=*/false,
-      /*single_key_checksum=*/"");
+  std::string cache_key =
+      GenerateCacheKeyForURLRequest(*url_request, request_destination);
 
   return std::make_unique<NetworkServiceMemoryCacheWriter>(
       weak_ptr_factory_.GetWeakPtr(), GetNextTraceId(), max_per_entry_bytes_,
@@ -412,6 +437,21 @@ void NetworkServiceMemoryCache::OnLoaderCompleted(
   url_loaders_.erase(it);
 }
 
+void NetworkServiceMemoryCache::OnRedirect(
+    const net::URLRequest* url_request,
+    mojom::RequestDestination request_destination) {
+  DCHECK(url_request);
+
+  if (url_request->method() != net::HttpRequestHeaders::kGetMethod)
+    return;
+
+  std::string cache_key =
+      GenerateCacheKeyForURLRequest(*url_request, request_destination);
+  auto it = entries_.Peek(cache_key);
+  if (it != entries_.end())
+    EraseEntry(it);
+}
+
 void NetworkServiceMemoryCache::SetCurrentTimeForTesting(
     base::Time current_time) {
   current_time_for_testing_ = current_time;

services/network/network_service_memory_cache.h

@@ -99,6 +99,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkServiceMemoryCache {
   // Called when a custom URLLoader is completed.
   void OnLoaderCompleted(NetworkServiceMemoryCacheURLLoader* loader);
 
+  // Called when a redirect happens for a request.
+  void OnRedirect(const net::URLRequest* url_request,
+                  mojom::RequestDestination request_destination);
+
   void SetCurrentTimeForTesting(base::Time current_time);
 
   mojom::URLResponseHeadPtr GetResponseHeadForTesting(

services/network/network_service_memory_cache_unittest.cc

@@ -160,16 +160,21 @@ class NetworkServiceMemoryCacheTest : public testing::Test {
   ~NetworkServiceMemoryCacheTest() override = default;
 
   void SetUp() override {
-    // The following setup similar to CorsURLLoaderFactoryTest.
+    should_redirect_in_cacheable_handler_ = false;
 
     test_server_.AddDefaultHandlers();
     test_server_.RegisterRequestHandler(
         base::BindRepeating(&CacheableResponseHandler));
     test_server_.RegisterRequestHandler(
         base::BindRepeating(&CacheableWithoutContentLengthHandler));
     test_server_.RegisterRequestHandler(base::BindRepeating(&CorbCheckHandler));
+    test_server_.RegisterRequestHandler(base::BindRepeating(
+        &NetworkServiceMemoryCacheTest::CacheableOrRedirectHandler,
+        base::Unretained(this)));
     ASSERT_TRUE(test_server_.Start());
 
+    // The following setup similar to CorsURLLoaderFactoryTest.
+
     network_service_ = NetworkService::CreateForTesting();
 
     auto context_params = mojom::NetworkContextParams::New();
@@ -212,6 +217,10 @@ class NetworkServiceMemoryCacheTest : public testing::Test {
     return *network_context_->GetMemoryCache();
   }
 
+  void MakeCacheableHandlerSendRedirect() {
+    should_redirect_in_cacheable_handler_ = true;
+  }
+
   ResourceRequest CreateRequest(const std::string& relative_path) {
     ResourceRequest request;
     GURL url = test_server().GetURL(relative_path);
@@ -275,6 +284,25 @@ class NetworkServiceMemoryCacheTest : public testing::Test {
   }
 
  private:
+  std::unique_ptr<net::test_server::HttpResponse> CacheableOrRedirectHandler(
+      const net::test_server::HttpRequest& request) {
+    if (request.GetURL().path_piece() != "/cacheable_or_redirect")
+      return nullptr;
+
+    auto response = std::make_unique<net::test_server::BasicHttpResponse>();
+
+    if (should_redirect_in_cacheable_handler_) {
+      response->set_code(net::HttpStatusCode::HTTP_FOUND);
+      response->AddCustomHeader("location", "/cacheable");
+    } else {
+      constexpr size_t kBodySize = 64;
+      response->AddCustomHeader("cache-control", "max-age=60");
+      response->set_content(std::string(kBodySize, 'a'));
+    }
+
+    return response;
+  }
+
   base::test::ScopedFeatureList scoped_feature_list_;
   base::test::TaskEnvironment task_environment_;
   std::unique_ptr<net::URLRequestContext> url_request_context_;
@@ -284,6 +312,8 @@ class NetworkServiceMemoryCacheTest : public testing::Test {
 
   net::test_server::EmbeddedTestServer test_server_;
 
+  bool should_redirect_in_cacheable_handler_ = false;
+
   std::unique_ptr<mojom::URLLoaderFactory> cors_url_loader_factory_;
   mojo::Remote<mojom::URLLoaderFactory> cors_url_loader_factory_remote_;
 
@@ -569,6 +599,19 @@ TEST_F(NetworkServiceMemoryCacheTest, CanServe_VaryHeaderAcceptEncoding) {
   ASSERT_TRUE(CanServeFromMemoryCache(request));
 }
 
+TEST_F(NetworkServiceMemoryCacheTest, CanServe_MultipleVaryHeader) {
+  ResourceRequest request =
+      CreateRequest("/echoheadercache?Accept-Encoding,Origin");
+  request.headers.SetHeader("accept-encoding", "gzip");
+  request.headers.SetHeader("origin", "https://a.test");
+
+  StoreResponseToMemoryCache(request);
+  ASSERT_TRUE(CanServeFromMemoryCache(request));
+
+  request.headers.SetHeader("origin", "https://b.test");
+  ASSERT_FALSE(CanServeFromMemoryCache(request));
+}
+
 // TODO(https://crbug.com/1339708): Change the test name and the expectation
 // once we implement appropriate Vary checks.
 TEST_F(NetworkServiceMemoryCacheTest, CanServe_UnsupportedVaryHeaderCookie) {
@@ -579,13 +622,11 @@ TEST_F(NetworkServiceMemoryCacheTest, CanServe_UnsupportedVaryHeaderCookie) {
   ASSERT_FALSE(CanServeFromMemoryCache(request));
 }
 
-// TODO(https://crbug.com/1339708): Change the test name and the expectation
-// once we implement appropriate Vary checks.
 TEST_F(NetworkServiceMemoryCacheTest, CanServe_UnsupportedMultipleVaryHeader) {
   ResourceRequest request =
-      CreateRequest("/echoheadercache?Accept-Encoding,Origin");
+      CreateRequest("/echoheadercache?Accept-Encoding,X-Foo");
   request.headers.SetHeader("accept-encoding", "gzip");
-  request.headers.SetHeader("origin", "https://a.test");
+  request.headers.SetHeader("x-foo", "bar");
 
   StoreResponseToMemoryCache(request);
   ASSERT_FALSE(CanServeFromMemoryCache(request));
@@ -643,6 +684,28 @@ TEST_F(NetworkServiceMemoryCacheTest, EvictLeastRecentlyUsed) {
   ASSERT_EQ(memory_cache().total_bytes(), kBodySize * 2);
 }
 
+// Tests that a stored response is deleted when a subsequent request that
+// bypasses the cache results in a redirect.
+TEST_F(NetworkServiceMemoryCacheTest, CachedAfterRedirect) {
+  ResourceRequest request = CreateRequest("/cacheable_or_redirect");
+
+  StoreResponseToMemoryCache(request);
+  ASSERT_TRUE(CanServeFromMemoryCache(request));
+
+  MakeCacheableHandlerSendRedirect();
+  request.load_flags |= net::LOAD_BYPASS_CACHE;
+
+  LoaderPair pair = CreateLoaderAndStart(request);
+  pair.client->RunUntilRedirectReceived();
+  pair.loader_remote->FollowRedirect(
+      /*removed_headers=*/{}, /*modified_headers=*/{},
+      /*modified_cors_exempt_headers=*/{}, /*new_url=*/absl::nullopt);
+  pair.client->RunUntilComplete();
+
+  request.load_flags &= ~net::LOAD_BYPASS_CACHE;
+  ASSERT_FALSE(CanServeFromMemoryCache(request));
+}
+
 TEST_F(NetworkServiceMemoryCacheTest, Clear) {
   constexpr int kBodySize = 64;
 

services/network/url_loader.cc

@@ -1386,6 +1386,9 @@ void URLLoader::OnReceivedRedirect(net::URLRequest* url_request,
   DispatchOnRawResponse();
   ReportFlaggedResponseCookies();
 
+  if (memory_cache_)
+    memory_cache_->OnRedirect(url_request_.get(), request_destination_);
+
   const CrossOriginEmbedderPolicy kEmpty;
   // Enforce the Cross-Origin-Resource-Policy (CORP) header.
   const CrossOriginEmbedderPolicy& cross_origin_embedder_policy =