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

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
Committed: https://crrev.com/202534e3fa636aa1c9ce73c30dbbba854992488f
Cr-Commit-Position: refs/heads/master@{#369754}

[Mike West, 2015-10-20 09:30:18]

mkwst@chromium.org changed reviewers:
+ estark@chromium.org, mmenke@chromium.org, nasko@chromium.org

[Mike West, 2015-10-20 09:30:18]

Hi Matt, Emily, and Nasko!

I've consolidated the various FPO patches I've asked y'all to look at into this
one. It teaches 'URLRequest' about the request's initiator origin, and uses that
to populate a single "include FPO cookies?" flag on CookieOptions for the
request. I hope this is simpler to understand, more performant, and correct. :)

WDYT?

[nasko, 2015-10-20 22:36:29]

nasko@chromium.org changed reviewers:
+ clamy@chromium.org

[nasko, 2015-10-20 22:36:30]

The content/ part looks fine to me. Adding clamy@ to ensure I'm not missing some
bits that will break PlzNavigate.

https://codereview.chromium.org/1411813003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:180: // URLs).
Is this TODO going to be addressed in this CL?

https://codereview.chromium.org/1411813003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:663: 
nit: No need for empty line.

[estark, 2015-10-20 23:41:58]

mostly just questions from me

https://codereview.chromium.org/1411813003/diff/20001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/1411813003/diff/20001/content/common/resource...
content/common/resource_messages.h:164: // fo cookie checks like
'First-Party-Only'.
typo: for

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3227: // requests). For redirects, it is
updated by WebURLLoaderImpl.
Is this comment true? In WebURLLoaderImpl I only see a TODO about this:
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3241: // If the first-party isn't set, we
need to set the first-party state as
This comment isn't parsing for me. Is it possible that "first-party state" is
supposed to say "requestor origin" or something like that? Or is that what
first-party state means? I guess what I'm saying is that I'm confused. :)

[clamy, 2015-10-21 11:32:32]

PlzNavigate part lgtm.

[mmenke, 2015-10-21 15:36:38]

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
new_request->set_initiator(request_data.request_initiator);
What about AppCache?

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
new_request->set_initiator(request_data.request_initiator);
What about ServiceWorker script requests, which issue their own nested requests?
 Probably have to modify it to copy these fields, at least.

https://codereview.chromium.org/1411813003/diff/40001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/content/child/resource_...
content/child/resource_dispatcher.cc:758: request->request_initiator =
request_info.request_initiator;
How does this work for ServiceWorker?

https://codereview.chromium.org/1411813003/diff/40001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1411813003/diff/40001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:13: #include "url/origin.h"
Don't need the last two of these any more.

https://codereview.chromium.org/1411813003/diff/40001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:34: bool include_first_party_only() const { return
include_first_party_only_; }
Can we rename this to "include_first_party_only_cookies", and add documentation?

This was confusing me because it sounds like "include first party cookies,
only".

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:476: bool URLRequest::is_safe_method() const {
This isn't an accessor, nor is it a "very short inlined function", so I think
this should be named IsSafeMethod.

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:476: bool URLRequest::is_safe_method() const {
Maybe IsMethodSafe, to make it clearer this is an operation on the request's
method, not the request itself.

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:290: const url::Origin initiator() const { return
initiator_; }
const url::Origin&

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:290: const url::Origin initiator() const { return
initiator_; }
I'm concerned that I have no idea what the difference between initiator and
first_party_for_cookies is, and the code here has no pointers to definitions for
either.  Can we add some pointers to RFCs?

[Mike West, 2015-10-22 13:17:03]

Thanks for taking a look, Matt. Mind taking another? In particular, I'd love
help with your appcache and SW questions. I don't know those well enough to be
sure that I'm touching the part that needs touching. :/

https://codereview.chromium.org/1411813003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:180: // URLs).
On 2015/10/20 at 22:36:30, nasko (slow to review) wrote:
> Is this TODO going to be addressed in this CL?

No. It's just something I noticed in passing.

https://codereview.chromium.org/1411813003/diff/20001/content/common/resource...
File content/common/resource_messages.h (right):

https://codereview.chromium.org/1411813003/diff/20001/content/common/resource...
content/common/resource_messages.h:164: // fo cookie checks like
'First-Party-Only'.
On 2015/10/20 at 23:41:58, estark wrote:
> typo: for

Fixed.

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3227: // requests). For redirects, it is
updated by WebURLLoaderImpl.
On 2015/10/20 at 23:41:58, estark wrote:
> Is this comment true? In WebURLLoaderImpl I only see a TODO about this:
https://code.google.com/p/chromium/codesearch#chromium/src/components/html_vi...

It's certainly true that we update the first party for a URLRequest during
redirects in URLRequestJob::ComputeRedirectInfo (controlled via the request's
`URLRequest::FirstPartyURLPolicy` enum). I guess things moved around since this
comment was written? I can update it to be less specific. :)

https://codereview.chromium.org/1411813003/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:3241: // If the first-party isn't set, we
need to set the first-party state as
On 2015/10/20 at 23:41:58, estark wrote:
> This comment isn't parsing for me. Is it possible that "first-party state" is
supposed to say "requestor origin" or something like that? Or is that what
first-party state means? I guess what I'm saying is that I'm confused. :)

Yup. I can't type, obviously. Fixing the comment.

https://codereview.chromium.org/1411813003/diff/20001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/20001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:663: 
On 2015/10/20 at 22:36:30, nasko (slow to review) wrote:
> nit: No need for empty line.

Done.

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
new_request->set_initiator(request_data.request_initiator);
On 2015/10/21 at 15:36:38, mmenke wrote:
> What about ServiceWorker script requests, which issue their own nested
requests?  Probably have to modify it to copy these fields, at least.

Yup. I missed that. They should copy over the request details in the same way
they copy the first party URL. I'll have to poke around a bit to make sure that
fetches from those contexts also grab the correct initiator, but I think this
might be magically taken care of by the fact that SWs somehow pretend to have a
document context.

> What about AppCache?

What about it? :)

Does AppCache trigger requests on its own? I guess it does, right. Hrm. We can
use the origin of the manifest, I suppose. That's what it looks like we're doing
now for cookies.

I'm not sure that's sane, but it's status quo, and it looks like it matches
Firefox's behavior, which is a minor miracle. :)

https://codereview.chromium.org/1411813003/diff/40001/net/cookies/cookie_opti...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1411813003/diff/40001/net/cookies/cookie_opti...
net/cookies/cookie_options.h:34: bool include_first_party_only() const { return
include_first_party_only_; }
On 2015/10/21 at 15:36:38, mmenke wrote:
> Can we rename this to "include_first_party_only_cookies", and add
documentation?
> 
> This was confusing me because it sounds like "include first party cookies,
only".

Sure. Done.

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:476: bool URLRequest::is_safe_method() const {
On 2015/10/21 at 15:36:38, mmenke wrote:
> Maybe IsMethodSafe, to make it clearer this is an operation on the request's
method, not the request itself.

Done.

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:290: const url::Origin initiator() const { return
initiator_; }
On 2015/10/21 at 15:36:38, mmenke wrote:
> I'm concerned that I have no idea what the difference between initiator and
first_party_for_cookies is, and the code here has no pointers to definitions for
either.  Can we add some pointers to RFCs?

Done and done.

[mmenke, 2015-10-22 19:41:04]

mmenke@chromium.org changed reviewers:
+ michaeln@chromium.org

[mmenke, 2015-10-22 19:41:05]

[+michaeln] for ServiceWorker/AppCache considerations.  See
https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
for a description of the new field.  The concern is making sure it's set
correctly for ServiceWorker and AppCache.

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1411813003/diff/40001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1242:
new_request->set_initiator(request_data.request_initiator);
On 2015/10/22 13:17:02, Mike West (slow) wrote:
> On 2015/10/21 at 15:36:38, mmenke wrote:
> > What about ServiceWorker script requests, which issue their own nested
> requests?  Probably have to modify it to copy these fields, at least.
> 
> Yup. I missed that. They should copy over the request details in the same way
> they copy the first party URL. I'll have to poke around a bit to make sure
that
> fetches from those contexts also grab the correct initiator, but I think this
> might be magically taken care of by the fact that SWs somehow pretend to have
a
> document context.

I can't comment on that...  Should we have some sort of tests to make sure we
aren't leaking cookies for SW's?

ServiceWorkerReadFromCacheJob does create nested requests for SW scripts, which
it should presumably also copy the new field to.

I'm far from an expert on SW or cookies.

> > What about AppCache?
> 
> What about it? :)
> 
> Does AppCache trigger requests on its own? I guess it does, right. Hrm. We can
> use the origin of the manifest, I suppose. That's what it looks like we're
doing
> now for cookies.
> 
> I'm not sure that's sane, but it's status quo, and it looks like it matches
> Firefox's behavior, which is a minor miracle. :)

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request.h:295: //    sites
nit:  Reformat

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request.h:306: // 4.3 of
https://tools.ietf.org/html/draft-west-first-party-cookies.
Thanks for the detailed description!

[mmenke, 2015-10-22 19:57:22]

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:667:
options.set_include_first_party_only_cookies();
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.

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:677: }
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.

[michaeln, 2015-10-23 23:29:50]

+mek@, seems like something that will matter for foreign fetch

https://codereview.chromium.org/1411813003/diff/60001/content/browser/appcach...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/1411813003/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_update_job.cc:167:
request_->set_initiator(url::Origin(job_->manifest_url_));
appcache lgtm

https://codereview.chromium.org/1411813003/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1411813003/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:207:
net_request_->set_initiator(request()->initiator());
ditto serviceworker

[michaeln, 2015-10-23 23:30:12]

Description was changed from

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
==========

to

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
==========

[michaeln, 2015-10-23 23:30:12]

michaeln@chromium.org changed reviewers:
+ mek@chromium.org

[michaeln, 2015-10-23 23:31:04]

> +mek@, seems like something that will matter for foreign fetch
sry for the spam, mek@ really added now

[mmenke, 2015-11-12 19:38:23]

On 2015/10/23 23:31:04, michaeln wrote:
> > +mek@, seems like something that will matter for foreign fetch
> sry for the spam, mek@ really added now

Is this one still waiting on me?  Sorry, I got it confused with your other CL.

[Mike West, 2015-11-13 15:02:08]

On 2015/11/12 at 19:38:23, mmenke wrote:
> On 2015/10/23 23:31:04, michaeln wrote:
> > > +mek@, seems like something that will matter for foreign fetch
> > sry for the spam, mek@ really added now
> 
> Is this one still waiting on me?  Sorry, I got it confused with your other CL.

Sorry, it's waiting on me to write some tests for the Service Worker and
Appcache bits. I'd be happy for you to finish up a review of the net/url_request
bits, though! :)

[Mike West, 2015-12-07 13:26:00]

On 2015/11/13 at 15:02:08, Mike West wrote:
> On 2015/11/12 at 19:38:23, mmenke wrote:
> > On 2015/10/23 23:31:04, michaeln wrote:
> > > > +mek@, seems like something that will matter for foreign fetch
> > > sry for the spam, mek@ really added now
> > 
> > Is this one still waiting on me?  Sorry, I got it confused with your other
CL.
> 
> Sorry, it's waiting on me to write some tests for the Service Worker and
Appcache bits. I'd be happy for you to finish up a review of the net/url_request
bits, though! :)

Mark, would you mind taking a look at the net/url_request bits? I still haven't
put together the SW/Appcache tests, but they'll simply be checking that the
relevant property is set on the outgoing request. The core functionality is all
tested/implemented in the bits I'd like you to sign off on. :)

[mmenke, 2015-12-07 16:27:56]

On 2015/12/07 13:26:00, Mike West wrote:
> On 2015/11/13 at 15:02:08, Mike West wrote:
> > On 2015/11/12 at 19:38:23, mmenke wrote:
> > > On 2015/10/23 23:31:04, michaeln wrote:
> > > > > +mek@, seems like something that will matter for foreign fetch
> > > > sry for the spam, mek@ really added now
> > > 
> > > Is this one still waiting on me?  Sorry, I got it confused with your other
> CL.
> > 
> > Sorry, it's waiting on me to write some tests for the Service Worker and
> Appcache bits. I'd be happy for you to finish up a review of the
net/url_request
> bits, though! :)
> 
> Mark, would you mind taking a look at the net/url_request bits? I still
haven't
> put together the SW/Appcache tests, but they'll simply be checking that the
> relevant property is set on the outgoing request. The core functionality is
all
> tested/implemented in the bits I'd like you to sign off on. :)

"Mark?"  Are you talking to me?

[Mike West, 2015-12-07 17:27:09]

On 2015/12/07 at 16:27:56, mmenke wrote:
> "Mark?"  Are you talking to me?

... Did you know that I honestly thought your name was "Mark" when I typed that?
Sorry, I'm an idiot. :(

[mmenke, 2015-12-07 17:30:43]

On 2015/12/07 17:27:09, Mike West wrote:
> On 2015/12/07 at 16:27:56, mmenke wrote:
> > "Mark?"  Are you talking to me?
> 
> ... Did you know that I honestly thought your name was "Mark" when I typed
that?
> Sorry, I'm an idiot. :(

No worries.  I'll put this in my queue, though at the end of it, since I'm not
blocking anything currently.  May be a day or two.

[Mike West, 2016-01-12 13:57:40]

On 2015/12/07 at 17:30:43, mmenke wrote:
> On 2015/12/07 17:27:09, Mike West wrote:
> > On 2015/12/07 at 16:27:56, mmenke wrote:
> > > "Mark?"  Are you talking to me?
> > 
> > ... Did you know that I honestly thought your name was "Mark" when I typed
that?
> > Sorry, I'm an idiot. :(
> 
> No worries.  I'll put this in my queue, though at the end of it, since I'm not
blocking anything currently.  May be a day or two.

Hi! It's been a day or two... Would you mind taking a look at the //net code? I
plan to test the appcache bits via web platform tests, which I'll be landing on
GitHub later this week. :)

[mmenke, 2016-01-12 14:30:13]

On 2016/01/12 13:57:40, Mike West (OOO) wrote:
> On 2015/12/07 at 17:30:43, mmenke wrote:
> > On 2015/12/07 17:27:09, Mike West wrote:
> > > On 2015/12/07 at 16:27:56, mmenke wrote:
> > > > "Mark?"  Are you talking to me?
> > > 
> > > ... Did you know that I honestly thought your name was "Mark" when I typed
> that?
> > > Sorry, I'm an idiot. :(
> > 
> > No worries.  I'll put this in my queue, though at the end of it, since I'm
not
> blocking anything currently.  May be a day or two.
> 
> Hi! It's been a day or two... Would you mind taking a look at the //net code?
I
> plan to test the appcache bits via web platform tests, which I'll be landing
on
> GitHub later this week. :)

Oh...you thought I meant Earth day.  It hasn't even been one day yet on Mercury.

I'll try and get to this today (Really!)

[mmenke, 2016-01-12 16:20:59]

Looks good!

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
net/cookies/cookie_options.h:13: #include "url/origin.h"
No longer needed.

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
net/cookies/cookie_options.h:62: url::Origin first_party_;
No longer used.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:308: // 4.3 of
https://tools.ietf.org/html/draft-west-first-party-cookies.
Thanks for this great description!

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:320: bool IsMethodSafe() const;
This class already supports a huge API, I don't want to add methods to it that
we don't have to.  Maybe just make "bool IsMethodSafe(const std::string&
method)" and put it in an anonymous namespace in url_request_http_job.cc?"

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:688:
origin.IsSameOriginWith(request_->initiator()))) {
So for "unsafe" requests without an initiator set, we'll never send first party
only cookies?  I'm not concerned about this effect on requests from the
renderer, but this also affects all internal requests issues by Chrome.

Are we not sending first party cookies for those, anyways, because
first_party_for_cookies isn't set by (most/all?) callers, and by default, we
don't update it on redirects?

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:2711: // method is "safe"
+period.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:2714:
network_delegate.set_experimental_cookie_features_enabled(true);
Optional nit:  Can we just make a single network delegate, set it as the
delegate for the context, and then use it for all the tests here?  Think that
makes it easier to see experimental features are enabled for all test cases
here.

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

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-01-13 08:10:22]

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request.h:295: //    sites
On 2015/10/22 at 19:41:05, mmenke wrote:
> nit:  Reformat

Yikes. Thanks!

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:667:
options.set_include_first_party_only_cookies();
On 2015/10/22 at 19:57:21, mmenke wrote:
> 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.

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.

https://codereview.chromium.org/1411813003/diff/60001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:677: }
On 2015/10/22 at 19:57:21, mmenke wrote:
> 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.

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.

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
File net/cookies/cookie_options.h (right):

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
net/cookies/cookie_options.h:13: #include "url/origin.h"
On 2016/01/12 at 16:20:58, mmenke wrote:
> No longer needed.

Done.

https://codereview.chromium.org/1411813003/diff/140001/net/cookies/cookie_opt...
net/cookies/cookie_options.h:62: url::Origin first_party_;
On 2016/01/12 at 16:20:58, mmenke wrote:
> No longer used.

Thanks!

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:48: }
Dropping this in favor of including the header; the member below needs it, and
now it's not transitively available from `cookie_options.h`.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:320: bool IsMethodSafe() const;
On 2016/01/12 at 16:20:58, mmenke wrote:
> This class already supports a huge API, I don't want to add methods to it that
we don't have to.  Maybe just make "bool IsMethodSafe(const std::string&
method)" and put it in an anonymous namespace in url_request_http_job.cc?"

Done.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request.h:320: bool IsMethodSafe() const;
On 2016/01/12 at 16:20:58, mmenke wrote:
> This class already supports a huge API, I don't want to add methods to it that
we don't have to.  Maybe just make "bool IsMethodSafe(const std::string&
method)" and put it in an anonymous namespace in url_request_http_job.cc?"

Done.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:688:
origin.IsSameOriginWith(request_->initiator()))) {
On 2016/01/12 at 16:20:58, mmenke wrote:
> So for "unsafe" requests without an initiator set, we'll never send first
party only cookies?  I'm not concerned about this effect on requests from the
renderer, but this also affects all internal requests issues by Chrome.
> 
> Are we not sending first party cookies for those, anyways, because
first_party_for_cookies isn't set by (most/all?) callers, and by default, we
don't update it on redirects?

Well, we can decide how this ought to work. Since we're not actually using
first-party-only cookies for any service that internal requests issue at the
moment, I think I'd prefer to err on the side of not sending the cookies if the
initiator isn't set, as that means that things fail closed if I've screwed up
some piece of the renderer side. If we decide that we need first-party-only
cookies for some service, then that service can/should ensure that requests it
makes have a reasonable initiator.

WDYT?

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:2711: // method is "safe"
On 2016/01/12 at 16:20:59, mmenke wrote:
> +period.

Done.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:2714:
network_delegate.set_experimental_cookie_features_enabled(true);
On 2016/01/12 at 16:20:59, mmenke wrote:
> Optional nit:  Can we just make a single network delegate, set it as the
delegate for the context, and then use it for all the tests here?  Think that
makes it easier to see experimental features are enabled for all test cases
here.

Sure. Done.

[Mike West, 2016-01-13 08:10:22]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1411813003/#ps160001
(title: "mmenke@ feedback.")

[Mike West, 2016-01-13 08:10:30]

The CQ bit was unchecked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-01-13 08:10:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411813003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411813003/160001

[Mike West, 2016-01-13 08:12:24]

On 2016/01/13 at 08:10:33, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1411813003/160001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1411813003/160001

Sorry, I hit "send + cq" rather than "send". :/ Cancelled.

[mmenke, 2016-01-13 16:30:16]

URLFetcher supports setting first_party_for_cookies, but not initiator.  With
this behavior, that makes no sense.  You should either update the API and fix
the (3?) callers, or remove the API and fix callers.  I guess the former is the
way to go, though I'd love to see that API trimmed down a bit.

Also, there's a new caller to URLRequest::set_first_party_for_cookies: 
content/browser/loader/async_revalidation_manager.cc.  That needs to be updated,
too.

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1411813003/diff/140001/net/url_request/url_re...
net/url_request/url_request_http_job.cc:688:
origin.IsSameOriginWith(request_->initiator()))) {
On 2016/01/13 08:10:22, Mike West (OOO) wrote:
> On 2016/01/12 at 16:20:58, mmenke wrote:
> > So for "unsafe" requests without an initiator set, we'll never send first
> party only cookies?  I'm not concerned about this effect on requests from the
> renderer, but this also affects all internal requests issues by Chrome.
> > 
> > Are we not sending first party cookies for those, anyways, because
> first_party_for_cookies isn't set by (most/all?) callers, and by default, we
> don't update it on redirects?
> 
> Well, we can decide how this ought to work. Since we're not actually using
> first-party-only cookies for any service that internal requests issue at the
> moment, I think I'd prefer to err on the side of not sending the cookies if
the
> initiator isn't set, as that means that things fail closed if I've screwed up
> some piece of the renderer side. If we decide that we need first-party-only
> cookies for some service, then that service can/should ensure that requests it
> makes have a reasonable initiator.
> 
> WDYT?

This sounds reasonable.

[Mike West, 2016-01-14 12:33:10]

On 2016/01/13 at 16:30:16, mmenke wrote:
> URLFetcher supports setting first_party_for_cookies, but not initiator.  With
this behavior, that makes no sense.  You should either update the API and fix
the (3?) callers, or remove the API and fix callers.  I guess the former is the
way to go, though I'd love to see that API trimmed down a bit.

I've changed the API from `URLFetcher::SetFirstPartyForCookies` to
`URLFetcher::SetInitiatorURL`, which populates both the first-party and the
initiator for the request. Given the way that method is used at its single
callsite, that seems like a reasonable thing to do (modulo the fact that the
upstream callsites don't actually grabbing the first-party URL in the correct
way, but just the top-level address. I'll fix that in a followup).

Does that seem sane?

> Also, there's a new caller to URLRequest::set_first_party_for_cookies: 
content/browser/loader/async_revalidation_manager.cc.  That needs to be updated,
too.

Done, thanks.

[mmenke, 2016-01-14 16:47:42]

LGTM.  For the record, I reviewed everything but content/browser/frame_host/*,
content/child/, and content/renderer (Looked at all of them, but not
sufficiently familiar to sign off on them).

https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
net/url_request/url_fetcher_core.cc:559: : initiator_);
Hrm...wasn't even aware of this code.  :(

[Mike West, 2016-01-14 19:51:47]

On 2016/01/14 at 16:47:42, mmenke wrote:
> LGTM.  For the record, I reviewed everything but content/browser/frame_host/*,
content/child/, and content/renderer (Looked at all of them, but not
sufficiently familiar to sign off on them).

Thank you.

clamy@ signed off on the `frame_host` changes for PlzNavigate, and that hasn't
changed since her review.

nasko@, would you mind looking at content/child and content/renderer?

>
https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
> File net/url_request/url_fetcher_core.cc (right):
> 
>
https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
> net/url_request/url_fetcher_core.cc:559: : initiator_);
> Hrm...wasn't even aware of this code.  :(

Yeah. The layering of URLFetcher seemed strange to me as I was skimming through
it for this change. I assume it makes sense to someone. :)

[mmenke, 2016-01-14 19:54:48]

On 2016/01/14 19:51:47, Mike West (OOO) wrote:
> On 2016/01/14 at 16:47:42, mmenke wrote:
> > LGTM.  For the record, I reviewed everything but
content/browser/frame_host/*,
> content/child/, and content/renderer (Looked at all of them, but not
> sufficiently familiar to sign off on them).
> 
> Thank you.
> 
> clamy@ signed off on the `frame_host` changes for PlzNavigate, and that hasn't
> changed since her review.
> 
> nasko@, would you mind looking at content/child and content/renderer?
> 
> >
>
https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
> > File net/url_request/url_fetcher_core.cc (right):
> > 
> >
>
https://codereview.chromium.org/1411813003/diff/180001/net/url_request/url_fe...
> > net/url_request/url_fetcher_core.cc:559: : initiator_);
> > Hrm...wasn't even aware of this code.  :(
> 
> Yeah. The layering of URLFetcher seemed strange to me as I was skimming
through
> it for this change. I assume it makes sense to someone. :)

It serves two purposes:  Lets people drive a URLRequest from another thread, and
gives people a simpler interface to URLRequest.  It does have some big problems
(Unbounded memory usage, if you're not using it to save stuff directly to a
file, for instance).

[Mike West, 2016-01-15 06:24:05]

mkwst@chromium.org changed reviewers:
+ jochen@chromium.org

[Mike West, 2016-01-15 06:24:06]

jochen@: Perhaps you can take a look at the (~4 files, ~dozen lines) in
//content/child and //content/renderer this morning? Hooray for time zones! :)

[Mike West, 2016-01-15 06:24:38]

On 2016/01/14 at 19:54:48, mmenke wrote:
> It serves two purposes:  Lets people drive a URLRequest from another thread,
and gives people a simpler interface to URLRequest.  It does have some big
problems (Unbounded memory usage, if you're not using it to save stuff directly
to a file, for instance).

I think that makes sense. Thanks for the clarification!

[jochen (gone - plz use gerrit), 2016-01-15 14:48:25]

lgtm

[Mike West, 2016-01-15 14:51:22]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-01-15 14:51:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/1411813003/#ps180001
(title: "Feedback.")

[commit-bot: I haz the power, 2016-01-15 14:51:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1411813003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1411813003/180001

[commit-bot: I haz the power, 2016-01-15 16:07:22]

Description was changed from

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
==========

to

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
==========

[commit-bot: I haz the power, 2016-01-15 16:07:23]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-01-15 16:08:18]

Description was changed from

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114
==========

to

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

This patch adds an 'initiator' field to 'URLRequest' in order to keep track
of the origin of the context which initiated a request. This allows us to
correctly perform the initiator requests specified in [1], which prevent
first-party-only cookies from being sent along with a request if that request
is "unsafe", and is initiated from a third-party origin (e.g. a form
submission to 'bank.com' from 'evil.com').

[1]: https://tools.ietf.org/html/draft-west-first-party-cookies-04#section-4.3

BUG=544114

Committed: https://crrev.com/202534e3fa636aa1c9ce73c30dbbba854992488f
Cr-Commit-Position: refs/heads/master@{#369754}
==========

[commit-bot: I haz the power, 2016-01-15 16:08:20]

Patchset 10 (id:??) landed as
https://crrev.com/202534e3fa636aa1c9ce73c30dbbba854992488f
Cr-Commit-Position: refs/heads/master@{#369754}