From 11973357f725699f61477e62208379996e4b583b Mon Sep 17 00:00:00 2001 From: Adam Rice Date: Mon, 18 Nov 2024 08:23:33 +0000 Subject: [PATCH] Subresource prefetch: Fix the referrer value PerformNetworkContextPrefetch() used an incorrect referrer_policy by default, leading to mismatches. It also failed to trim the referrer on cross-origin fetches. Use a referrer_policy of REDUCE_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN and only include the origin on cross-origin fetches. Also prevent prefetches over insecure HTTP. Add tests for the new functionality. BUG=342445996 Change-Id: I8900cb550f9d80df152b3b14f0bcc7707e9159f1 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6021766 Commit-Queue: Adam Rice Reviewed-by: Lingqi Chi Cr-Commit-Position: refs/heads/main@{#1384197} --- .../perform_network_context_prefetch.cc | 36 ++++- ...rform_network_context_prefetch_unittest.cc | 153 ++++++++++++++++-- 2 files changed, 172 insertions(+), 17 deletions(-) diff --git a/chrome/browser/predictors/perform_network_context_prefetch.cc b/chrome/browser/predictors/perform_network_context_prefetch.cc index 0a96bda650f578..65faa78d907944 100644 --- a/chrome/browser/predictors/perform_network_context_prefetch.cc +++ b/chrome/browser/predictors/perform_network_context_prefetch.cc @@ -60,6 +60,17 @@ const std::string SerializeHeaderString(const T& value) { .value_or(std::string()); } +// Returns the correct referrer header for a request to `url` from `page` having +// origin `page_origin`. `page_origin` is redundant but is included to save +// recalculating it. Currently assumes the policy is +// REDUCE_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN. +GURL CalculateReferrer(const GURL& page, + const url::Origin& page_origin, + const GURL& url) { + DCHECK(page.SchemeIsCryptographic() && url.SchemeIsCryptographic()); + return page_origin.IsSameOriginWith(url) ? page : page_origin.GetURL(); +} + // Heuristic to identify a favicon request. bool LooksLikeFavicon(const GURL& url) { const std::string& as_string = url.possibly_invalid_spec(); @@ -101,9 +112,10 @@ void PrefetchResource( // TODO(crbug.com/342445996): We need the predictor to predict the referrer // policy so that we can create the referrer fields correctly. - request.referrer = page; - request.referrer_policy = - net::ReferrerPolicy::ORIGIN_ONLY_ON_TRANSITION_CROSS_ORIGIN; + static constexpr net::ReferrerPolicy kExpectedReferrerPolicy = + net::ReferrerPolicy::REDUCE_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN; + request.referrer = CalculateReferrer(page, page_origin, url); + request.referrer_policy = kExpectedReferrerPolicy; auto& headers = request.headers; headers.SetHeader("Purpose", "prefetch"); @@ -238,6 +250,15 @@ void PerformNetworkContextPrefetch(Profile* profile, // incognito or not. return; } + // Only support secure connections. + // TODO(crbug.com/342445996): Maybe relax this restriction if this code is + // used by features that support insecure prefetches. See also + // CalculateReferrer(). + if (!page.SchemeIsCryptographic()) { + DLOG(ERROR) << "PerformNetworkContextPrefetch() called for non-SSL page: " + << page << " (ignored)"; + return; + } const auto page_origin = url::Origin::Create(page); content::StoragePartition* storage_partition = profile->GetStoragePartitionForUrl(page); @@ -270,6 +291,15 @@ void PerformNetworkContextPrefetch(Profile* profile, // TODO(crbug.com/342445996): Support more resource types. continue; } + // Only support secure subresources. + // TODO(crbug.com/342445996): Relax this restriction in future if needed. + if (!url.SchemeIsCryptographic()) { + DLOG(ERROR) + << "PerformNetworkContextPrefetch() called for non-SSL subresource: " + << url << " (ignored)"; + continue; + } + // TODO(crbug.com/342445996): Usually requests will have the same origin, so // maybe cache (origin, accept_langage) to avoid wasteful recalculation? const std::string accept_language = diff --git a/chrome/browser/predictors/perform_network_context_prefetch_unittest.cc b/chrome/browser/predictors/perform_network_context_prefetch_unittest.cc index 603d85afeecd9f..5f7c2eb47d56fd 100644 --- a/chrome/browser/predictors/perform_network_context_prefetch_unittest.cc +++ b/chrome/browser/predictors/perform_network_context_prefetch_unittest.cc @@ -12,6 +12,9 @@ #include #include +#include "base/containers/to_vector.h" +#include "base/logging.h" +#include "base/test/mock_log.h" #include "base/test/scoped_command_line.h" #include "base/test/test_future.h" #include "chrome/browser/predictors/predictors_features.h" @@ -19,7 +22,9 @@ #include "chrome/test/base/testing_profile.h" #include "components/variations/scoped_variations_ids_provider.h" #include "content/public/test/browser_task_environment.h" +#include "content/public/test/test_host_resolver.h" #include "net/base/schemeful_site.h" +#include "net/dns/mock_host_resolver.h" #include "net/test/embedded_test_server/embedded_test_server.h" #include "net/test/embedded_test_server/http_request.h" #include "services/network/public/cpp/features.h" @@ -34,9 +39,12 @@ namespace predictors { namespace { +using net::test_server::EmbeddedTestServer; +using net::test_server::EmbeddedTestServerHandle; using net::test_server::HttpRequest; using net::test_server::HttpResponse; using network::mojom::RequestDestination; +using ::testing::_; using ::testing::A; using ::testing::AnyOf; using ::testing::ExplainMatchResult; @@ -45,9 +53,11 @@ using ::testing::IsSupersetOf; using ::testing::Pair; using ::testing::StartsWith; using ::testing::StrCaseEq; +using ::testing::StrictMock; -constexpr std::string_view kPagePath = "/"; +constexpr std::string_view kPagePath = "/page"; constexpr std::string_view kResourcePath = "/nocontent"; +constexpr std::string_view kHostname = "a.test"; class PerformNetworkContextPrefetchRecorderTest : public ::testing::Test { public: @@ -64,9 +74,12 @@ class PerformNetworkContextPrefetchRecorderTest : public ::testing::Test { } void SetUp() override { + host_resolver_.host_resolver()->AddRule("*", "127.0.0.1"); test_server_.AddDefaultHandlers(); - test_server_.RegisterRequestMonitor( - request_future_.GetSequenceBoundRepeatingCallback()); + test_server_.RegisterRequestMonitor(GetFutureCallback()); + // This SSL config is only needed for tests that test cross-origin behavior, + // but it does no harm for the other tests. + test_server_.SetSSLConfig(EmbeddedTestServer::CERT_TEST_NAMES); test_server_handle_ = test_server_.StartAndReturnHandle(); ASSERT_TRUE(test_server_handle_); // Treat 127.0.0.1 as "public" to avoid being blocked by local network @@ -76,20 +89,57 @@ class PerformNetworkContextPrefetchRecorderTest : public ::testing::Test { base::StringPrintf("127.0.0.1:%d=public", test_server_.port())); } - GURL GetURL() const { return test_server_.GetURL(kResourcePath); } + GURL PageURL(std::string_view hostname = kHostname) const { + return test_server_.GetURL(hostname, kPagePath); + } + + GURL ResourceURL(std::string_view hostname = kHostname) const { + return test_server_.GetURL(hostname, kResourcePath); + } + + void DoPrefetch(RequestDestination destination, + std::string_view page_path = kPagePath, + std::string_view resource_path = kResourcePath) { + DoPrefetch(destination, test_server_.GetURL(kHostname, page_path), + test_server_.GetURL(kHostname, resource_path)); + } - void DoPrefetch(RequestDestination destination) { - const GURL page_url = test_server_.GetURL(kPagePath); + void DoPrefetch(RequestDestination destination, + const GURL& page_url, + const GURL& resource_url) { + DoPrefetches(destination, page_url, {resource_url}); + } + + void DoPrefetches(RequestDestination destination, + const GURL& page_url, + const std::vector& resources) { const net::SchemefulSite site(page_url); const auto nak = net::NetworkAnonymizationKey::CreateSameSite(site); - std::vector requests = { - PrefetchRequest(GetURL(), nak, destination)}; + auto requests = base::ToVector(resources, [&](const GURL& resource_url) { + return PrefetchRequest(resource_url, nak, destination); + }); PerformNetworkContextPrefetch(profile_.get(), page_url, std::move(requests)); } HttpRequest GetRequest() { return request_future_.Get(); } + void ExpectNoRequest() { + EXPECT_FALSE(request_future_.IsReady()); + // Unfortunately the only way to wait for nothing to happen is to use a + // delay. + static constexpr auto kWaitFor = base::Milliseconds(100); + base::RunLoop run_loop; + base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask( + FROM_HERE, run_loop.QuitClosure(), kWaitFor); + run_loop.Run(); + EXPECT_FALSE(request_future_.IsReady()); + } + + EmbeddedTestServer::MonitorRequestCallback GetFutureCallback() { + return request_future_.GetSequenceBoundRepeatingCallback(); + } + private: base::test::ScopedCommandLine command_line_; base::test::ScopedFeatureList features_; @@ -100,8 +150,9 @@ class PerformNetworkContextPrefetchRecorderTest : public ::testing::Test { variations::VariationsIdsProvider::Mode::kUseSignedInState}; std::unique_ptr profile_; base::test::TestFuture request_future_; - net::test_server::EmbeddedTestServer test_server_; - net::test_server::EmbeddedTestServerHandle test_server_handle_; + content::TestHostResolver host_resolver_; + EmbeddedTestServer test_server_{EmbeddedTestServer::TYPE_HTTPS}; + EmbeddedTestServerHandle test_server_handle_; }; // The combination of matchers required to match a header value is long and @@ -114,7 +165,7 @@ auto HasHeader(std::string_view name, ValueMatcher value_matcher) { TEST_F(PerformNetworkContextPrefetchRecorderTest, Script) { DoPrefetch(RequestDestination::kScript); - auto request = GetRequest(); + const auto request = GetRequest(); EXPECT_EQ(request.relative_url, "/nocontent"); EXPECT_EQ(request.method, net::test_server::METHOD_GET); EXPECT_EQ(request.method_string, "GET"); @@ -124,8 +175,7 @@ TEST_F(PerformNetworkContextPrefetchRecorderTest, Script) { EXPECT_THAT(request.headers, HasHeader("Accept", "*/*")); EXPECT_THAT(request.headers, HasHeader("Accept-Language", "en")); EXPECT_THAT(request.headers, HasHeader("Purpose", "prefetch")); - EXPECT_THAT(request.headers, - HasHeader("Referer", StartsWith("http://127.0.0.1:"))); + EXPECT_THAT(request.headers, HasHeader("Referer", PageURL().spec())); EXPECT_THAT(request.headers, HasHeader("sec-ch-ua", HasSubstr("v="))); EXPECT_THAT(request.headers, HasHeader("sec-ch-ua-mobile", AnyOf("?0", "?1"))); @@ -143,13 +193,88 @@ TEST_F(PerformNetworkContextPrefetchRecorderTest, Script) { TEST_F(PerformNetworkContextPrefetchRecorderTest, Style) { DoPrefetch(RequestDestination::kStyle); - auto request = GetRequest(); + const auto request = GetRequest(); // Only test things that differ from the previous test. EXPECT_THAT(request.headers, HasHeader("Accept", "text/css,*/*;q=0.1")); EXPECT_THAT(request.headers, HasHeader("Sec-Fetch-Dest", "style")); } +class InsecureTestServer final { + public: + explicit InsecureTestServer(const EmbeddedTestServer::MonitorRequestCallback& + monitor_request_callback) { + server_.AddDefaultHandlers(); + server_.RegisterRequestMonitor(monitor_request_callback); + handle_ = server_.StartAndReturnHandle(); + EXPECT_TRUE(handle_); + } + + GURL GetURL(std::string_view path) const { return server_.GetURL(path); } + + private: + EmbeddedTestServer server_{EmbeddedTestServer::TYPE_HTTP}; + EmbeddedTestServerHandle handle_; +}; + +constexpr auto kERROR = ::logging::LOGGING_ERROR; + +TEST_F(PerformNetworkContextPrefetchRecorderTest, NonSSLPage) { + InsecureTestServer insecure(GetFutureCallback()); + { + StrictMock log; + EXPECT_CALL(log, Log(kERROR, _, _, _, HasSubstr("SSL"))); + log.StartCapturingLogs(); + + DoPrefetch(RequestDestination::kStyle, insecure.GetURL("/"), + insecure.GetURL("/style.css")); + } + ExpectNoRequest(); +} + +TEST_F(PerformNetworkContextPrefetchRecorderTest, NonSSLResource) { + InsecureTestServer insecure(GetFutureCallback()); + { + StrictMock log; + EXPECT_CALL(log, Log(kERROR, _, _, _, HasSubstr("SSL"))); + log.StartCapturingLogs(); + + // Do two fetches, so that the second one succeeds. + DoPrefetches(RequestDestination::kStyle, PageURL(), + {insecure.GetURL("/style.css"), ResourceURL()}); + } + + const auto request = GetRequest(); + + // The first request should have been skipped and only the second one issued. + EXPECT_TRUE(request.base_url.SchemeIs("https")); + EXPECT_EQ(request.relative_url, kResourcePath); +} + +TEST_F(PerformNetworkContextPrefetchRecorderTest, ReferrerSameOrigin) { + DoPrefetch(RequestDestination::kStyle); + const auto request = GetRequest(); + + EXPECT_THAT(request.headers, HasHeader("Referer", PageURL().spec())); +} + +TEST_F(PerformNetworkContextPrefetchRecorderTest, ReferrerCrossOrigin) { + // These are both included in CERT_TEST_NAMES + constexpr char kPageHostname[] = "a.test"; + constexpr char kResourceHostname[] = "b.test"; + + DoPrefetch(RequestDestination::kStyle, PageURL(kPageHostname), + ResourceURL(kResourceHostname)); + + const auto request = GetRequest(); + + const auto page_origin = url::Origin::Create(PageURL(kPageHostname)); + const auto expected_referrer = page_origin.Serialize() + "/"; + + EXPECT_THAT(request.headers, HasHeader("Referer", expected_referrer)); + EXPECT_THAT(request.headers, HasHeader("Sec-Fetch-Site", "cross-site")); +} + } // namespace } // namespace predictors