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 <ricea@chromium.org>
Reviewed-by: Lingqi Chi <lingqi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1384197}

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 =

chrome/browser/predictors/perform_network_context_prefetch_unittest.cc

@@ -12,14 +12,19 @@
 #include <string_view>
 #include <utility>
 
+#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"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #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<GURL>& resources) {
     const net::SchemefulSite site(page_url);
     const auto nak = net::NetworkAnonymizationKey::CreateSameSite(site);
-    std::vector<PrefetchRequest> 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<TestingProfile> profile_;
   base::test::TestFuture<const HttpRequest&> 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<base::test::MockLog> 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<base::test::MockLog> 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