NetworkService: Make requests with bad referrers fail.

This moves logic for this case from ChromeNetworkDelegate to
NetworkContext's NetworkDelegate, and adds a Mojo parameter to cause
requests with bad referrers to fail (enabled by default).

Also removes the action Net.URLRequest_StartJob_InvalidReferrer rather
than moving it, as it was unowned.

Bug: 852871
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I9d6d55157ba28e2b12ca7136b78330100a8673aa
Reviewed-on: https://chromium-review.googlesource.com/1103119Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568807}

chrome/browser/net/chrome_network_delegate.cc

@@ -94,24 +94,6 @@ void ForceGoogleSafeSearchCallbackWrapper(net::CompletionOnceCallback callback,
   std::move(callback).Run(rv);
 }
 
-void ReportInvalidReferrerSendOnUI() {
-  base::RecordAction(
-      base::UserMetricsAction("Net.URLRequest_StartJob_InvalidReferrer"));
-}
-
-void ReportInvalidReferrerSend(const GURL& target_url,
-                               const GURL& referrer_url) {
-  LOG(ERROR) << "Cancelling request to " << target_url
-             << " with invalid referrer " << referrer_url;
-  // Record information to help debug http://crbug.com/422871
-  if (!target_url.SchemeIsHTTPOrHTTPS())
-    return;
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::BindOnce(&ReportInvalidReferrerSendOnUI));
-  base::debug::DumpWithoutCrashing();
-  NOTREACHED();
-}
-
 // Record network errors that HTTP requests complete with, including OK and
 // ABORTED.
 void RecordNetworkErrorHistograms(const net::URLRequest* request,
@@ -496,7 +478,9 @@ bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
     const net::URLRequest& request,
     const GURL& target_url,
     const GURL& referrer_url) const {
-  ReportInvalidReferrerSend(target_url, referrer_url);
+  // These errors should be handled by the NetworkDelegate wrapper created by
+  // the owning NetworkContext.
+  NOTREACHED();
   return true;
 }
 

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -56,6 +56,7 @@
 #include "net/test/embedded_test_server/http_response.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "net/url_request/url_request.h"
 #include "services/network/public/cpp/features.h"
 #include "services/network/public/cpp/resource_response.h"
 #include "services/network/public/cpp/resource_response_info.h"
@@ -945,6 +946,36 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
   EXPECT_EQ("None", referrer);
 }
 
+// Make sure that sending referrers that violate the referrer policy results in
+// errors.
+IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
+                       PolicyViolatingReferrers) {
+  std::unique_ptr<network::ResourceRequest> request =
+      std::make_unique<network::ResourceRequest>();
+  request->url = embedded_test_server()->GetURL("/echoheader?Referer");
+  request->referrer = GURL("http://referrer/");
+  request->referrer_policy = net::URLRequest::NO_REFERRER;
+  content::SimpleURLLoaderTestHelper simple_loader_helper;
+  std::unique_ptr<network::SimpleURLLoader> simple_loader =
+      network::SimpleURLLoader::Create(std::move(request),
+                                       TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      loader_factory(), simple_loader_helper.GetCallback());
+  simple_loader_helper.WaitForCallback();
+  if (GetParam().network_context_type == NetworkContextType::kSafeBrowsing &&
+      base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+    // Safebrowsing ignores referrers, when the network service is enabled, so
+    // the requests succeed.
+    EXPECT_EQ(net::OK, simple_loader->NetError());
+    ASSERT_TRUE(simple_loader_helper.response_body());
+    EXPECT_EQ("None", *simple_loader_helper.response_body());
+  } else {
+    // In all other cases, the invalid referrer causes the request to fail.
+    EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT, simple_loader->NetError());
+  }
+}
+
 class NetworkContextConfigurationFixedPortBrowserTest
     : public NetworkContextConfigurationBrowserTest {
  public:

ios/chrome/browser/net/ios_chrome_network_delegate.cc

@@ -13,7 +13,6 @@
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/histogram_functions.h"
-#include "base/metrics/user_metrics.h"
 #include "base/path_service.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
@@ -29,11 +28,6 @@ namespace {
 
 const char kDNTHeader[] = "DNT";
 
-void ReportInvalidReferrerSendOnUI() {
-  base::RecordAction(
-      base::UserMetricsAction("Net.URLRequest_StartJob_InvalidReferrer"));
-}
-
 void ReportInvalidReferrerSend(const GURL& target_url,
                                const GURL& referrer_url) {
   LOG(ERROR) << "Cancelling request to " << target_url
@@ -41,8 +35,6 @@ void ReportInvalidReferrerSend(const GURL& target_url,
   // Record information to help debug http://crbug.com/422871
   if (!target_url.SchemeIsHTTPOrHTTPS())
     return;
-  web::WebThread::PostTask(web::WebThread::UI, FROM_HERE,
-                           base::Bind(&ReportInvalidReferrerSendOnUI));
   base::debug::DumpWithoutCrashing();
   NOTREACHED();
 }

net/base/layered_network_delegate.cc

@@ -231,18 +231,19 @@ bool LayeredNetworkDelegate::
         const URLRequest& request,
         const GURL& target_url,
         const GURL& referrer_url) const {
-  OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
+  return OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
+             request, target_url, referrer_url) ||
+         nested_network_delegate_
+             ->CancelURLRequestWithPolicyViolatingReferrerHeader(
                  request, target_url, referrer_url);
-  return nested_network_delegate_
-      ->CancelURLRequestWithPolicyViolatingReferrerHeader(request, target_url,
-                                                          referrer_url);
 }
 
-void LayeredNetworkDelegate::
+bool LayeredNetworkDelegate::
     OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
         const URLRequest& request,
         const GURL& target_url,
         const GURL& referrer_url) const {
+  return false;
 }
 
 bool LayeredNetworkDelegate::OnCanQueueReportingReport(

net/base/layered_network_delegate.h

@@ -163,7 +163,9 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
 
   virtual void OnAreExperimentalCookieFeaturesEnabledInternal() const;
 
-  virtual void OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
+  // If this returns false, it short circuits the corresponding call in any
+  // nested NetworkDelegates.
+  virtual bool OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
       const URLRequest& request,
       const GURL& target_url,
       const GURL& referrer_url) const;

net/base/layered_network_delegate_unittest.cc

@@ -322,7 +322,7 @@ class TestLayeredNetworkDelegate : public LayeredNetworkDelegate {
     EXPECT_EQ(1, (*counters_)["on_can_enable_privacy_mode_count"]);
   }
 
-  void OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
+  bool OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
       const URLRequest& request,
       const GURL& target_url,
       const GURL& referrer_url) const override {
@@ -332,6 +332,7 @@ class TestLayeredNetworkDelegate : public LayeredNetworkDelegate {
     EXPECT_EQ(1, (*counters_)
                      ["on_cancel_url_request_with_policy_"
                       "violating_referrer_header_count"]);
+    return false;
   }
 
   void OnCanQueueReportingReportInternal(

services/network/network_context.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/command_line.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop_current.h"
@@ -222,9 +223,12 @@ class NetworkContext::ContextNetworkDelegate
  public:
   ContextNetworkDelegate(
       std::unique_ptr<net::NetworkDelegate> nested_network_delegate,
-      bool enable_referrers)
+      bool enable_referrers,
+      bool validate_referrer_policy_on_initial_request)
       : LayeredNetworkDelegate(std::move(nested_network_delegate)),
-        enable_referrers_(enable_referrers) {}
+        enable_referrers_(enable_referrers),
+        validate_referrer_policy_on_initial_request_(
+            validate_referrer_policy_on_initial_request) {}
 
   ~ContextNetworkDelegate() override {}
 
@@ -234,12 +238,31 @@ class NetworkContext::ContextNetworkDelegate
       request->SetReferrer(std::string());
   }
 
+  bool OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
+      const net::URLRequest& request,
+      const GURL& target_url,
+      const GURL& referrer_url) const override {
+    // TODO(mmenke): Once the network service has shipped on all platforms,
+    // consider moving this logic into URLLoader, and removing this method from
+    // NetworkDelegate. Can just have a DCHECK in URLRequest instead.
+    if (!validate_referrer_policy_on_initial_request_)
+      return false;
+
+    LOG(ERROR) << "Cancelling request to " << target_url
+               << " with invalid referrer " << referrer_url;
+    // Record information to help debug issues like http://crbug.com/422871.
+    if (target_url.SchemeIsHTTPOrHTTPS())
+      base::debug::DumpWithoutCrashing();
+    return true;
+  }
+
   void set_enable_referrers(bool enable_referrers) {
     enable_referrers_ = enable_referrers;
   }
 
  private:
   bool enable_referrers_;
+  bool validate_referrer_policy_on_initial_request_;
 
   DISALLOW_COPY_AND_ASSIGN(ContextNetworkDelegate);
 };
@@ -854,7 +877,9 @@ URLRequestContextOwner NetworkContext::ApplyContextParamsToBuilder(
         std::unique_ptr<ContextNetworkDelegate> context_network_delegate =
             std::make_unique<ContextNetworkDelegate>(
                 std::move(nested_network_delegate),
-                network_context_params->enable_referrers);
+                network_context_params->enable_referrers,
+                network_context_params
+                    ->validate_referrer_policy_on_initial_request);
         if (out_context_network_delegate)
           *out_context_network_delegate = context_network_delegate.get();
         return context_network_delegate;

services/network/network_context_unittest.cc

@@ -35,6 +35,7 @@
 #include "components/network_session_configurator/browser/network_session_configurator.h"
 #include "components/network_session_configurator/common/network_switches.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
+#include "mojo/public/cpp/system/data_pipe_utils.h"
 #include "net/base/cache_type.h"
 #include "net/base/hash_value.h"
 #include "net/base/ip_endpoint.h"
@@ -815,6 +816,75 @@ TEST_F(NetworkContextTest, CertReporting) {
   }
 }
 
+// Test that valid referrers are allowed, while invalid ones result in errors.
+TEST_F(NetworkContextTest, Referrers) {
+  const GURL kReferrer = GURL("http://referrer/");
+  net::test_server::EmbeddedTestServer test_server;
+  test_server.AddDefaultHandlers(
+      base::FilePath(FILE_PATH_LITERAL("services/test/data")));
+  ASSERT_TRUE(test_server.Start());
+
+  for (bool validate_referrer_policy_on_initial_request : {false, true}) {
+    for (net::URLRequest::ReferrerPolicy referrer_policy :
+         {net::URLRequest::NEVER_CLEAR_REFERRER,
+          net::URLRequest::NO_REFERRER}) {
+      mojom::NetworkContextParamsPtr context_params = CreateContextParams();
+      context_params->validate_referrer_policy_on_initial_request =
+          validate_referrer_policy_on_initial_request;
+      std::unique_ptr<NetworkContext> network_context =
+          CreateContextWithParams(std::move(context_params));
+
+      mojom::URLLoaderFactoryPtr loader_factory;
+      mojom::URLLoaderFactoryParamsPtr params =
+          mojom::URLLoaderFactoryParams::New();
+      params->process_id = 0;
+      network_context->CreateURLLoaderFactory(
+          mojo::MakeRequest(&loader_factory), std::move(params));
+
+      ResourceRequest request;
+      request.url = test_server.GetURL("/echoheader?Referer");
+      request.referrer = kReferrer;
+      request.referrer_policy = referrer_policy;
+
+      mojom::URLLoaderPtr loader;
+      TestURLLoaderClient client;
+      loader_factory->CreateLoaderAndStart(
+          mojo::MakeRequest(&loader), 0 /* routing_id */, 0 /* request_id */,
+          0 /* options */, request, client.CreateInterfacePtr(),
+          net::MutableNetworkTrafficAnnotationTag(
+              TRAFFIC_ANNOTATION_FOR_TESTS));
+
+      client.RunUntilComplete();
+      EXPECT_TRUE(client.has_received_completion());
+
+      // If validating referrers, and the referrer policy is not to send
+      // referrers, the request should fail.
+      if (validate_referrer_policy_on_initial_request &&
+          referrer_policy == net::URLRequest::NO_REFERRER) {
+        EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT,
+                  client.completion_status().error_code);
+        EXPECT_FALSE(client.response_body().is_valid());
+        continue;
+      }
+
+      // Otherwise, the request should succeed.
+      EXPECT_EQ(net::OK, client.completion_status().error_code);
+      std::string response_body;
+      ASSERT_TRUE(client.response_body().is_valid());
+      EXPECT_TRUE(mojo::BlockingCopyToString(client.response_body_release(),
+                                             &response_body));
+      if (referrer_policy == net::URLRequest::NO_REFERRER) {
+        // If not validating referrers, and the referrer policy is not to send
+        // referrers, the referrer should be cleared.
+        EXPECT_EQ("None", response_body);
+      } else {
+        // Otherwise, the referrer should be send.
+        EXPECT_EQ(kReferrer.spec(), response_body);
+      }
+    }
+  }
+}
+
 // Validates that clearing the HTTP cache when no cache exists does complete.
 TEST_F(NetworkContextTest, ClearHttpCacheWithNoCache) {
   mojom::NetworkContextParamsPtr context_params = CreateContextParams();

services/network/public/mojom/network_context.mojom

@@ -50,6 +50,10 @@ struct NetworkContextParams {
   // If false, the referrer of requests is never populated.
   bool enable_referrers = true;
 
+  // If true, requests initiated with referrers that don't match their referrer
+  // policy will fail.
+  bool validate_referrer_policy_on_initial_request = true;
+
   // Handles PAC script execution. If not populated, will attempt to use
   // platform implementation to execute PAC scripts, if available (Only
   // available on Windows and Mac).

tools/metrics/actions/actions.xml

@@ -12988,6 +12988,7 @@ should be able to be added at any place in this file.
 </action>
 
 <action name="Net.URLRequest_StartJob_InvalidReferrer">
+  <obsolete>Deprecated 6/2018.</obsolete>
   <owner>Please list the metric's owners. Add more owner tags as needed.</owner>
   <description>Please enter the description of this user action.</description>
 </action>