Referrer Policy: Add new policies to URLRequest.

net/url_request/url_request.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_request.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
@@ skipping 498 matching lines @@
 void URLRequest::SetReferrer(const std::string& referrer) {
   DCHECK(!is_pending_);
   GURL referrer_url(referrer);
   if (referrer_url.is_valid()) {
     referrer_ = referrer_url.GetAsReferrer().spec();
   } else {
     referrer_ = referrer;
   }
 }
 
+bool URLRequest::IsReferrerInvalid() const {

[mmenke, 2014/11/19 16:29:03]

The new code here is not currently being exercised in tests.

[mmenke, 2014/11/19 16:29:03]

Suggest moving this out of URLRequest and into an anonymous namespace, making it
take 3 arguments.  (referrer, url, and the policy), to make it clearer what it's
doing, and avoid adding more stuff to an 850 line header file.

[mmenke, 2014/11/19 16:29:03]

As-is, this method is very hard to read.  My suggestion in URLRequestJob would
work around that, and result in more robust code.

I've also included some suggestions here, but think the other approach is much
cleaner.

[Mike West, 2014/11/20 10:45:30]

Killed all of this in the new patchset. Thanks!

+  GURL referrer(referrer_);
+  bool referrer_policy_requires_secure_referrer_transitions =
+      referrer_policy_ ==
+          CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE ||
+      referrer_policy_ ==
+          REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN;

[mmenke, 2014/11/19 16:29:03]

Rather than store a bool, why not just return false early.

[mmenke, 2014/11/19 16:29:03]

For security reasons, I suggest a whitelist rather than a blacklist, in case a
new scheme is added and someone forgets to add something here (For example, if
only the Job logic is update).  Or alternatively, a switch statement with 4
cases - then adding a new option without updating the list would result in a
warning that would lead to a compile failure.

+  bool secure_referrer_in_insecure_request =
+      referrer.SchemeIsSecure() && !url().SchemeIsSecure();

[mmenke, 2014/11/19 16:29:03]

Once you have the above code doing an early return, you can get rid of this
bool, too, and just return true instantly if it's true.

+  bool referrer_is_origin_only = referrer.GetOrigin() == referrer;
+  bool cross_origin_referrer = referrer.GetOrigin() != url().GetOrigin();

[mmenke, 2014/11/19 16:29:03]

Suggest just inlining these bools in the if statement - which, with the above
suggestions should be simpler.

+  return (referrer_policy_requires_secure_referrer_transitions &&
+          secure_referrer_in_insecure_request) ||
+         (referrer_policy_ ==
+              REDUCE_REFERRER_GRANULARITY_ON_TRANSITION_CROSS_ORIGIN &&
+          cross_origin_referrer && !referrer_is_origin_only);
+}
+
 void URLRequest::set_referrer_policy(ReferrerPolicy referrer_policy) {
   DCHECK(!is_pending_);
   referrer_policy_ = referrer_policy;
 }
 
 void URLRequest::set_delegate(Delegate* delegate) {
   delegate_ = delegate;
 }
 
 void URLRequest::Start() {
@@ skipping 114 matching lines @@
   job_->SetPriority(priority_);
 
   if (upload_data_stream_.get())
     job_->SetUpload(upload_data_stream_.get());
 
   is_pending_ = true;
   is_redirecting_ = false;
 
   response_info_.was_cached = false;
 
-  // If the referrer is secure, but the requested URL is not, the referrer
-  // policy should be something non-default. If you hit this, please file a
-  // bug.
-  if (referrer_policy_ ==
-          CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE &&
-      GURL(referrer_).SchemeIsSecure() && !url().SchemeIsSecure()) {
+  if (IsReferrerInvalid()) {
     if (!network_delegate_ ||
         !network_delegate_->CancelURLRequestWithPolicyViolatingReferrerHeader(
             *this, url(), GURL(referrer_))) {
       referrer_.clear();
     } else {
       // We need to clear the referrer anyway to avoid an infinite recursion
       // when starting the error job.
       referrer_.clear();
       std::string source("delegate");
       net_log_.AddEvent(NetLog::TYPE_CANCELLED,
@@ skipping 560 matching lines @@
       new base::debug::StackTrace(NULL, 0);
   *stack_trace_copy = stack_trace;
   stack_trace_.reset(stack_trace_copy);
 }
 
 const base::debug::StackTrace* URLRequest::stack_trace() const {
   return stack_trace_.get();
 }
 
 }  // namespace net