Apply Referrer-Policy header when following redirects

net/url_request/url_request_job.cc

Index: net/url_request/url_request_job.cc
diff --git a/net/url_request/url_request_job.cc b/net/url_request/url_request_job.cc
index b9c8646f729e75e45b45d65b635d1f51e78da254..45f4d71a4fe628b5bb6ac40463c7d62737450651 100644
--- a/net/url_request/url_request_job.cc
+++ b/net/url_request/url_request_job.cc
@@ -14,6 +14,7 @@
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
@@ -62,6 +63,107 @@ std::string ComputeMethodForRedirect(const std::string& method,
   return method;
 }
 
+// Given a referrer policy |token| (from the list of policies defined in
+// https://w3c.github.io/webappsec-referrer-policy/#referrer-policies),
+// this function sets |new_referrer_policy| to the new referrer policy that
+// should be used, and sets |new_referrer_source| to the value that
+// should be used as input to ComputeReferrerForRedirect().
+// ComputeReferrerForRedirect() is responsible for applying the
+// request's URLRequest::ReferrerPolicy to compute the final referrer.
+//
+// If |token| does not match any policy, then this function does not
+// modify |new_referrer_policy| or |new_referrer_source|.
+//
+// Note that
+// https://w3c.github.io/webappsec-referrer-policy/#determine-policy-for-token
+// defines legacy keywords "never", "default", and "always", which this
+// function respects even though authors are discouraged from using them.
+void GetReferrerPolicyFromToken(const std::string& token,
+                                const std::string& referrer_source,
+                                URLRequest::ReferrerPolicy* new_referrer_policy,
+                                std::string* new_referrer_source) {

[mmenke, 2016/06/27 18:38:05]

"source" is a really confusing name.  It's actually the original referrer,
right?

Also, the original referrer is updated based on referrer policy in
ComputeReferrerForRedirect, which is called after this method, so why are we
updating referrer here, too?  Just so we can clear the referrer "no-referer",
since we don't have a URLRequest::NO_REFERRER?  I think if that's the case, it
would be much simpler to add that policy, and respect it in
ComputeReferrerForRedirect.

When there are multiple tokens, repeatedly computing, and then ignoring,
new_referrer_source is also weird.  Only calculating it once we have the policy
at the end also more closely adhere to how the spec is written, though the
result is the same.

[estark, 2016/06/27 22:18:34]

Heh, I called it "referrer source" because I found "original referrer" to be a
really confusing name. :) Well, that and because the spec calls it
referrerSource (step 3 of
https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer). I
find "original referrer" confusing because the request's referrer might have
several different values as it follows several redirects, and I was getting
confused as to which value was the "original" one.

I don't feel too strongly though and am happy to change it, just want to resolve
the below points first.
We'd need to add two policies, I think -- NO_REFERRER and ORIGIN.

Adding those policies feels a little weird to me. URLRequest::ReferrerPolicy is
just about behavior on redirects, and to an external caller, there'd be no
difference between NO_REFERRER and NEVER_CLEAR_REFERRER, or between ORIGIN and
NEVER_CLEAR_REFERRER. In other words we'd be adding values to the public enum
just to facilitate an internal implementation detail of URLRequestJob.

I'll defer to your judgement if you still think that's the right way to do it,
just wanted to point out that IMO it confuses the public API. I can also play
with restructuring the code a bit, maybe there's a way to make it less confusing
without adding new URLRequest::ReferrerPolicy values.
Acknowledged. Will address this once above question is resolved.

[mmenke, 2016/06/28 14:29:23]

"referrer_source" sounds like the source of the referrer to me.  I'm fine with
anything clearly indicating it is the old referrer (old_referrer? 
previous_referrer?)
I think having a 1-to-1 mapping between URLRequest::ReferrerPolicy and the
specced out "referrer policies" in that doc makes sense, though that's not
really my main concern here.  What bugs me about this is that
"ComputeReferrerForRedirect" no longer computes the referrer for the redirect. 
It's a combination of ComputeReferrerForRedirect and the referrer changes made
here.  And repeatedly updating new_referrer_source when there are multiple
headers also seems really weird.

Also note that if we switch to having ComputeReferrerForRedirect actually
compute the referrer, there will be a difference.  It would clear the referrer
on NO_REFERRER, and do nothing on NEVER_CLEAR_REFERRER.  And on ORIGIN, it would
make the referrer the origin.  Since it would do this on every redirect, there
would be a bit of wasted work, with multiple redirect, of course.

One ugliness with that approach is that we wouldn't adhere to those policies, if
set, for the initial request, and blink wouldn't be setting those policies. 
(Well beyond the scope of this CL, but if we switched to having the renderer
just ass along teh policy and the original referrer, and net actually enforce
those policies, that might reduce the amount of code to manage referrers. 
Haven't thought about the practicality of that, not familiar with all the stuff,
renderer side, to deal with it)

Another option I could live with would be to make one method compute both the
policy and the referrer, and to stick with the reduced set of policies.  I
suspect that would result in much harder to read code, but if we can get
something reasonable with that approach...

+  // For a referrer policy of 'no-referrer', clear the referrer
+  // source. With an empty referrer source, the request's referrer
+  // policy doesn't really matter because the final referrer will always
+  // be empty.
+  if (base::CompareCaseInsensitiveASCII(token, "never") == 0 ||
+      base::CompareCaseInsensitiveASCII(token, "no-referrer") == 0) {
+    *new_referrer_policy = URLRequest::NEVER_CLEAR_REFERRER;
+    *new_referrer_source = std::string();
+    return;
+  }
+
+  // For a referrer policy of 'no-referrer-when-downgrade', setting the
+  // referrer policy is sufficient; ComputeReferrerForRedirect() will
+  // apply this policy to strip the referrer if necessary.
+  if (base::CompareCaseInsensitiveASCII(token, "default") == 0 ||
+      base::CompareCaseInsensitiveASCII(token, "no-referrer-when-downgrade") ==
+          0) {
+    *new_referrer_policy =
+        URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE;
+    *new_referrer_source = referrer_source;
+    return;
+  }
+
+  // For a referrer policy of 'origin', strip the referrer source to the
+  // origin. After doing so, there is no need to modify the referrer
+  // further, so set the policy to leave the referrer alone.
+  if (base::CompareCaseInsensitiveASCII(token, "origin") == 0) {
+    *new_referrer_policy = URLRequest::NEVER_CLEAR_REFERRER;
+    *new_referrer_source = GURL(referrer_source).GetOrigin().spec();
+    return;
+  }
+
+  // For a referrer policy of 'origin-when-cross-origin', setting the
+  // URLRequest's ReferrerPolicy is sufficient;
+  // ComputeReferrerForRedirect() will apply this policy to strip the
+  // referrer if necessary.
+  if (base::CompareCaseInsensitiveASCII(token, "origin-when-cross-origin") ==
+      0) {
+    *new_referrer_policy = URLRequest::ORIGIN_ONLY_ON_TRANSITION_CROSS_ORIGIN;
+    *new_referrer_source = referrer_source;
+    return;
+  }
+
+  // For a referrer policy of 'unsafe-url', leave the referrer source
+  // untouched and set the policy so that ComputeReferrerForRedirect()
+  // will not modify it.
+  if (base::CompareCaseInsensitiveASCII(token, "always") == 0 ||
+      base::CompareCaseInsensitiveASCII(token, "unsafe-url") == 0) {
+    *new_referrer_policy = URLRequest::NEVER_CLEAR_REFERRER;
+    *new_referrer_source = referrer_source;
+    return;
+  }
+
+  return;
+}
+
+// A redirect response can contain a Referrer-Policy header
+// (https://w3c.github.io/webappsec-referrer-policy/). This function
+// checks for a Referrer-Policy header, and parses it if
+// present. Returns the referrer policy that should be used for the
+// request. Sets |new_referrer_source| to the URL that should be used as
+// what the spec calls the "referrerSource" -- that is, the referrer
+// that will be used as input to ComputeReferrerForRedirect().
+URLRequest::ReferrerPolicy ProcessReferrerPolicyHeaderOnRedirect(
+    URLRequest* request,
+    std::string* new_referrer_source) {
+  URLRequest::ReferrerPolicy new_policy = request->referrer_policy();
+  *new_referrer_source = request->referrer();
+
+  std::string referrer_policy_header;
+  request->GetResponseHeaderByName("Referrer-Policy", &referrer_policy_header);
+  std::vector<std::string> policy_tokens =
+      base::SplitString(referrer_policy_header, ",", base::TRIM_WHITESPACE,
+                        base::SPLIT_WANT_NONEMPTY);
+
+  for (const auto& token : policy_tokens) {
+    GetReferrerPolicyFromToken(token, request->referrer(), &new_policy,
+                               new_referrer_source);
+  }
+  return new_policy;
+}
+
 }  // namespace
 
 URLRequestJob::URLRequestJob(URLRequest* request,
@@ -344,6 +446,9 @@ GURL URLRequestJob::ComputeReferrerForRedirect(
 
     case URLRequest::NEVER_CLEAR_REFERRER:
       return original_referrer;
+    case URLRequest::MAX_REFERRER_POLICY:
+      NOTREACHED();
+      return original_referrer;
   }
 
   NOTREACHED();
@@ -398,6 +503,7 @@ void URLRequestJob::NotifyHeadersComplete() {
 
   GURL new_location;
   int http_status_code;
+
   if (IsRedirectResponse(&new_location, &http_status_code)) {
     // Redirect response bodies are not read. Notify the transaction
     // so it does not treat being stopped as an error.
@@ -944,11 +1050,23 @@ RedirectInfo URLRequestJob::ComputeRedirectInfo(const GURL& location,
         request_->first_party_for_cookies();
   }
 
+  // |referrer_source| is the starting-point referrer, which may be
+  // modified by ComputeReferrerForRedirect() to produce a new referrer
+  // that obeys the request's referrer policy.
+  std::string referrer_source;
+  if (request_->context()->enable_referrer_policy_header()) {

[mmenke, 2016/06/27 17:18:04]

Question...I may have asked this before, but it seems really weird to me...

Suppose we have:

Site A -> Site B1 -> Site B2 -> Site C.

Assume site B1/B2 are secure, and on the same site and site C is not (I don't
care about site A).

Site A sets NEVER_CLEAR_REFERRER on the redirect.  Site B is an old site, and
expects us to clear referrers on secure to insecure, but instead, inherits site
A's referrer policy, so site B assumes B2's URL is private, but since Site A set
NEVER_CLEAR_REFERRER, it inherits Site A's referrer policy.  Is this a problem? 
It seems really weird to me.

Note that this is the case for both subframes and main frames (Which don't even
get CORS protection).  I'm not sure if this is an issue for main frames
currently - I'm not sure how referrer_policy is set by Blink.

[mmenke, 2016/06/27 17:20:04]

That is, Site C is not secure, and not on the same site as B1/B2.

[estark, 2016/06/27 17:27:52]

That does seem weird, but I'm pretty sure that's been the case for several
years, for as long as referrer policy has been implemented, right? And that
weirdness isn't really affected by this CL... if anything, what this CL
implements slightly improves the situation. I know your point is that B might be
an old site, but now at least B would have the option to set a Referrer-Policy
header to change the policy back to the default despite A having set unsafe-url.
Before this CL, it doesn't really matter if B is an old site or not, because B
would have no means of changing the referrer policy in the middle of the request
even if it wanted to.

+    redirect_info.new_referrer_policy =
+        ProcessReferrerPolicyHeaderOnRedirect(request_, &referrer_source);
+  } else {
+    referrer_source = request_->referrer();
+    redirect_info.new_referrer_policy = request_->referrer_policy();
+  }
+
   // Alter the referrer if redirecting cross-origin (especially HTTP->HTTPS).
   redirect_info.new_referrer =
-      ComputeReferrerForRedirect(request_->referrer_policy(),
-                                 request_->referrer(),
-                                 redirect_info.new_url).spec();
+      ComputeReferrerForRedirect(redirect_info.new_referrer_policy,
+                                 referrer_source, redirect_info.new_url)
+          .spec();
 
   std::string include_referer;
   request_->GetResponseHeaderByName("include-referer-token-binding-id",