Implement the "first-party-only" cookie flag.

Implement the "First-Party-Only" cookie attribute.

First-party-only cookies allow servers to mitigate the risk of cross-site
request forgery and related information leakage attacks by asserting that a
particular cookie should only be sent in a "first-party" context.

This patch adds support for the 'First-Party-Only' attribute to the
CookieMonster and CookieStore, but does not yet wire up requests such that
the flag has any effect. https://codereview.chromium.org/940373002 will do so
by correctly setting the first-party URL on the CookieOptions object used to
load cookies for a request.

Spec: https://tools.ietf.org/html/draft-west-first-party-cookies

Intent to Implement: https://groups.google.com/a/chromium.org/d/msg/blink-dev/vT98riFhhT0/3Q-lADqsh0UJ

BUG=459154
TBR=dpolukhin@chromium.org

Committed: https://crrev.com/ae819bb3096b63a11b8c1ff47dd3b69f85ea241b
Cr-Commit-Position: refs/heads/master@{#317544}

[Mike West, 2015-02-17 10:48:58]

Patchset #1 (id:1) has been deleted

[Mike West, 2015-02-17 10:53:05]

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

[Mike West, 2015-02-17 10:54:38]

Hi Erik, would you mind taking a look at this patch? It's a low priority
experiment that I don't think we're in any rush to get out there, but there's a
bit of interest from folks at Mozilla to do something in this area, and I'm
hoping we can bring some implementation experience to the table when designing
the feature.

Thanks!

-mike

[Mike West, 2015-02-17 12:34:40]

Patchset #2 (id:40001) has been deleted

[Mike West, 2015-02-17 12:34:49]

Patchset #2 (id:60001) has been deleted

[Mike West, 2015-02-17 13:54:34]

Patchset #2 (id:80001) has been deleted

[Mike West, 2015-02-17 15:18:27]

Patchset #2 is the patch, plus the required clang-formatting. It's a bit noisy
(but the bots are happy).

I split a full clang-format of the various files into
https://codereview.chromium.org/929303003, and uploaded patchset #3 rebased onto
that, which I hope will make things easier to read. We can talk about the best
way to land those changes once you're actually happy with the patch in the first
place. :)

[Mike West, 2015-02-19 06:58:29]

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

[Mike West, 2015-02-19 06:58:30]

Ping!

+mmenke@, just in case you have some free time you'd like to fill with a review.
:)

[erikwright (departed), 2015-02-19 17:46:39]

https://codereview.chromium.org/876973003/diff/140001/net/cookies/canonical_c...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/876973003/diff/140001/net/cookies/canonical_c...
net/cookies/canonical_cookie_unittest.cc:259: expiration_time, last_access_time,
secure, httponly, firstparty,
Should a test be added here that checks the behaviour when
other_cookie.firstparty != cookie.firstparty?

https://codereview.chromium.org/876973003/diff/140001/net/cookies/parsed_cook...
File net/cookies/parsed_cookie_unittest.cc (right):

https://codereview.chromium.org/876973003/diff/140001/net/cookies/parsed_cook...
net/cookies/parsed_cookie_unittest.cc:151: ParsedCookie pc("  A  = BC 
;secure;;;   first-party");
This test previously covered trailing whitespace. Can you preserve that please?

[erikwright (departed), 2015-02-19 17:51:27]

Sorry, took Friday off, Monday holiday, sick Tue+Wed.

LGTM. I think landing the clang-format first and the actual changes separate is
a good plan.

[Mike West, 2015-02-19 17:56:48]

On 2015/02/19 at 17:51:27, erikwright wrote:
> Sorry, took Friday off, Monday holiday, sick Tue+Wed.
> 
> LGTM. I think landing the clang-format first and the actual changes separate
is a good plan.

Thanks for taking a look, and sorry to hear that you were sick!

I'll fix up the tests as you've suggested, and wait until after the branch point
to land this, just in case. :)

[mmenke, 2015-02-19 18:53:54]

On 2015/02/19 17:56:48, Mike West wrote:
> On 2015/02/19 at 17:51:27, erikwright wrote:
> > Sorry, took Friday off, Monday holiday, sick Tue+Wed.
> > 
> > LGTM. I think landing the clang-format first and the actual changes separate
> is a good plan.
> 
> Thanks for taking a look, and sorry to hear that you were sick!
> 
> I'll fix up the tests as you've suggested, and wait until after the branch
point
> to land this, just in case. :)

I'll happily defer to Erik on this one.  Looks like he may not own
net/url_request/.  That comment LGTM.  :)

[Mike West, 2015-02-19 19:04:06]

On Thu, Feb 19, 2015 at 7:53 PM, <mmenke@chromium.org> wrote:

>
> I'll happily defer to Erik on this one.  Looks like he may not own
> net/url_request/.  That comment LGTM.  :)
>

Do you have a preference regarding how you'd like me to implement that LGTM?

I'm planning on adding a command-line flag in //content to control the
feature, and I guess I'll need to add a policy enum to URLRequest based on
that flag to control whether or not we treat first-party cookies as
first-party or not?

Is there a particular way you'd like to see that done (or a particular way
I totally shouldn't do it)? Or are there //net flags that I'm missing
somewhere? :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2015-02-19 21:04:36]

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

https://codereview.chromium.org/876973003/diff/140001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:635: // first-party-url to the
first-party-for-cookies value. crbug.com/459154
So we already have URLRequest::FirstPartyURLPolicy and
URLRequest::ReferrerPolicy.  If we're going to be adding yet another, maybe
bundle them up in a struct and pass them all together?  I'm really not a big fan
of growing the size of URLRequest - it has a huge number of public methods, and
I'd really like to see us cut down on them.

[Mike West, 2015-02-20 10:28:58]

New patchsets have been uploaded after l-g-t-m from
erikwright@chromium.org,mmenke@chromium.org

[Mike West, 2015-02-20 10:36:00]

mkwst@chromium.org changed reviewers:
+ bauerb@chromium.org, dpolukhin@chromium.org, isherman@chromium.org

[Mike West, 2015-02-20 10:36:01]

bauerb@: Would you mind taking a look at the small changes to chrome/android and
chrome/browser/android to bring them into line with the //net changes?

dpolukhin@: Please review the (tiny) unittest change in chrome/browser/chromeos.

isherman@: Could you stamp the histogram change, please?

https://codereview.chromium.org/876973003/diff/140001/net/cookies/canonical_c...
File net/cookies/canonical_cookie_unittest.cc (right):

https://codereview.chromium.org/876973003/diff/140001/net/cookies/canonical_c...
net/cookies/canonical_cookie_unittest.cc:259: expiration_time, last_access_time,
secure, httponly, firstparty,
On 2015/02/19 17:46:38, erikwright wrote:
> Should a test be added here that checks the behaviour when
> other_cookie.firstparty != cookie.firstparty?

This is already done around line #231. I've made first-party behave like
httponly insofar as it doesn't effect "equivalence".

[Bernhard Bauer, 2015-02-20 11:10:17]

Android LGTM, but if I may make a small suggestion:

Could we name the flag "first-party only" (at least in code, if we don't want to
change the publicly-exposed part)? I'm a bit afraid that people would confuse a
cookie that has the "first-party" flag with a "first-party cookie", which really
means a (generic) cookie that happens to be sent to a first-party entity.

https://codereview.chromium.org/876973003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/876973003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:401: false,  //
TODO(mkwst): FirstParty
Can you elaborate on this TODO?

[Mike West, 2015-02-20 11:33:32]

On 2015/02/20 at 11:10:17, bauerb wrote:
> Android LGTM, but if I may make a small suggestion:
> 
> Could we name the flag "first-party only" (at least in code, if we don't want
to change the publicly-exposed part)? I'm a bit afraid that people would confuse
a cookie that has the "first-party" flag with a "first-party cookie", which
really means a (generic) cookie that happens to be sent to a first-party entity.

That makes a good deal of sense. `first-party-only` is probably a better name;
I'll revamp the patch and the spec. :)

[Mike West, 2015-02-20 12:06:51]

New patchsets have been uploaded after l-g-t-m from bauerb@chromium.org

[Bernhard Bauer, 2015-02-20 12:10:44]

Nice, thanks!

[Mike West, 2015-02-20 12:32:48]

On 2015/02/20 at 12:10:44, bauerb wrote:
> Nice, thanks!

https://github.com/mikewest/internetdrafts/commit/58d3818315e6ff61bf975985c59...

I've updated the CL description as well.

[Ilya Sherman, 2015-02-20 21:36:33]

histograms.xml LGTM, though I wonder whether we really need so many migration
time histograms...

[Mike West, 2015-02-23 05:06:59]

TBRing dpolukhin@chromium.org for the trivial change to
chrome/browser/chromeos/login/profile_auth_data_unittest.cc (simply adding one
boolean entry to the CanonicalCookie constructor calls.

https://codereview.chromium.org/876973003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/cookies/cookies_api.cc (right):

https://codereview.chromium.org/876973003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/cookies/cookies_api.cc:401: false,  //
TODO(mkwst): FirstParty
On 2015/02/20 11:10:17, Bernhard Bauer wrote:
> Can you elaborate on this TODO?

If we end up shipping this, we'll need to extend the `chrome.cookies` API to
expose this flag. I'll broaden the comment for clarity. :)

[Mike West, 2015-02-23 05:07:02]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-23 05:07:28]

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

[commit-bot: I haz the power, 2015-02-23 05:10:43]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-02-23 05:11:39]

Patchset 6 (id:??) landed as
https://crrev.com/ae819bb3096b63a11b8c1ff47dd3b69f85ea241b
Cr-Commit-Position: refs/heads/master@{#317544}