Referrer Policy: Add new policies to URLRequest.

net/url_request/url_request.cc

Index: net/url_request/url_request.cc
diff --git a/net/url_request/url_request.cc b/net/url_request/url_request.cc
index a2ce07eaf13057c5608b9467e377ed77a9e98240..07531cd247e0f51b484eb1a37643275fd8228b07 100644
--- a/net/url_request/url_request.cc
+++ b/net/url_request/url_request.cc
@@ -516,6 +516,24 @@ void URLRequest::SetReferrer(const std::string& 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;
@@ -650,12 +668,7 @@ void URLRequest::StartJob(URLRequestJob* job) {
 
   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_))) {