Teach URLRequest about initiator checks for First-Party-Only cookies.

net/url_request/url_request_http_job.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_http_job.h"
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
@@ skipping 641 matching lines @@
                    weak_factory_.GetWeakPtr()));
   } else {
     DoStartTransaction();
   }
 }
 
 void URLRequestHttpJob::DoLoadCookies() {
   CookieOptions options;
   options.set_include_httponly();
 
-  // TODO(mkwst): Drop this `if` once we decide whether or not to ship
-  // first-party cookies: https://crbug.com/459154
-  if (network_delegate() &&
-      network_delegate()->AreExperimentalCookieFeaturesEnabled())
-    options.set_first_party(url::Origin(request_->first_party_for_cookies()));
-  else
-    options.set_include_first_party_only();
+  url::Origin origin(request_->url());
+  if (origin.IsSameOriginWith(
+          url::Origin(request_->first_party_for_cookies())) &&
+      (request_->IsMethodSafe() ||
+       origin.IsSameOriginWith(request_->initiator()))) {
+    options.set_include_first_party_only_cookies();

[mmenke, 2015/10/22 19:57:21]

What about requests that don't set either of these fields on the request?  A
fair number of internal Chrome requests want cookies, I believe (Safebrowsing
certainly does).  This would effectively not allow first party only cookies for
those requests.

[Mike West, 2016/01/13 08:10:21]

If requests aren't being made in a web context, it's not clear what value
first-party-only would have. I think it's reasonable to lock out
first-party-only cookies in those contexts, honestly.

For SafeBrowsing in particular, there's actually a separate cookie jar entirely,
and the cookies are never exposed to the web (at least, not in the content
area). I don't think they'd see much value in FPO.

If they decide there's value in adding the flag, we can evaluate from what
context those requests should be made. Until they do so, I'd prefer to treat
those requests as blocking FPO cookies.

+  }
+
+  // TODO(mkwst): If first-party-only cookies aren't enabled, pretend the
+  // request is first-party regardless, in order to include all cookies. Drop
+  // this check once we decide whether or not we're shipping this feature:
+  // https://crbug.com/459154
+  if (!network_delegate() ||
+      !network_delegate()->AreExperimentalCookieFeaturesEnabled()) {
+    options.set_include_first_party_only_cookies();
+  }

[mmenke, 2015/10/22 19:57:21]

Think this is clearer as:

if (!network_delegate() || !...) {
  options.set_include_first_party_only_cookies();
} else if (origin.IsSameOriginWith...) {
  options.set_include_first_party_only_cookies();
}

Having this block just overwrite the work done by the previous block when
triggered seems a bit confusing to me.

Or could just swap the order of the if's, and start the second with a check that
include_first_party_only_cookies is not set, if you think that's clearer.

[Mike West, 2016/01/13 08:10:21]

My goal was to have something that I could just delete when we ship. I guess
putting it into an if/else block isn't any worse than the current state. Done in
the latest patchset.

 
   request_->context()->cookie_store()->GetCookiesWithOptionsAsync(
       request_->url(), options, base::Bind(&URLRequestHttpJob::OnCookiesLoaded,
                                            weak_factory_.GetWeakPtr()));
 }
 
 void URLRequestHttpJob::CheckCookiePolicyAndLoad(
     const CookieList& cookie_list) {
   if (CanGetCookies(cookie_list))
     DoLoadCookies();
@@ skipping 899 matching lines @@
   return override_response_headers_.get() ?
              override_response_headers_.get() :
              transaction_->GetResponseInfo()->headers.get();
 }
 
 void URLRequestHttpJob::NotifyURLRequestDestroyed() {
   awaiting_callback_ = false;
 }
 
 }  // namespace net