From e632190e0ae4b53e222c928cdd8321bd5508954b Mon Sep 17 00:00:00 2001 From: Kenichi Ishibashi Date: Sun, 17 Jul 2022 00:29:54 +0000 Subject: [PATCH] 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 Commit-Queue: Kenichi Ishibashi Cr-Commit-Position: refs/heads/main@{#1025055} --- .../network/network_service_memory_cache.cc | 74 ++++++++++++++----- .../network/network_service_memory_cache.h | 4 + .../network_service_memory_cache_unittest.cc | 73 ++++++++++++++++-- services/network/url_loader.cc | 3 + 4 files changed, 132 insertions(+), 22 deletions(-) diff --git a/services/network/network_service_memory_cache.cc b/services/network/network_service_memory_cache.cc index 6c2c88e644a4d5..c850af4942db44 100644 --- a/services/network/network_service_memory_cache.cc +++ b/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( 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; diff --git a/services/network/network_service_memory_cache.h b/services/network/network_service_memory_cache.h index eaa1f968dd4e46..c3cc4921c64572 100644 --- a/services/network/network_service_memory_cache.h +++ b/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( diff --git a/services/network/network_service_memory_cache_unittest.cc b/services/network/network_service_memory_cache_unittest.cc index 0f44815b6d6387..287c114c40fb45 100644 --- a/services/network/network_service_memory_cache_unittest.cc +++ b/services/network/network_service_memory_cache_unittest.cc @@ -160,7 +160,7 @@ 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( @@ -168,8 +168,13 @@ class NetworkServiceMemoryCacheTest : public testing::Test { 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 CacheableOrRedirectHandler( + const net::test_server::HttpRequest& request) { + if (request.GetURL().path_piece() != "/cacheable_or_redirect") + return nullptr; + + auto response = std::make_unique(); + + 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 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 cors_url_loader_factory_; mojo::Remote 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; diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc index a42e8eeb681361..6805b5f3976f60 100644 --- a/services/network/url_loader.cc +++ b/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 =