1286858 - Modify nsCookie*, interfaces and storage to include the SameSite attribute

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P2)

Description

[Mark Goodwin [:mgoodwin]]

See https://tools.ietf.org/html/draft-west-first-party-cookies-07 - section 4.2

Comment 1

[m.kurz]

Mark, what's the roadmap for the SameSite cookie attribute? Are you working on this?

Comment 2

[Mark Goodwin [:mgoodwin]]

(In reply to m.kurz from comment #1)
> Mark, what's the roadmap for the SameSite cookie attribute? Are you working
> on this?

I'm hoping for firefox 51 (or shortly after).

Yes, I am actively working on this.

Comment 3

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1286858 - Data storage and interface changes for SameSite cookies

Review commit: https://reviewboard.mozilla.org/r/66028/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66028/

Comment 4

[Mark Goodwin [:mgoodwin]]

Valentin; at this stage this is more feedback? than r? - but AFICT mozreview has no support for such a thing.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Looks good overall. I haven't looked at the SQL bits too closely.

I would add a check to nsCookie::Create() that aSameSite has a sane value.
Also, some tests that actually use this feature would be great, and the comments in nsICookie2.idl should specify what the attribute is used for. Thanks!

Comment 6

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66028/diff/1-2/

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

https://reviewboard.mozilla.org/r/66028/#review70856

Looks good to me. Thanks!

Comment 8

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e134b91e926

Comment 9

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3053ee77f57

Comment 10

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3441cc99c727

Comment 11

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66028/diff/2-3/

Comment 12

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66028/diff/3-4/

Comment 13

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66028/diff/4-5/

Comment 14

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66028/diff/5-6/

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

https://reviewboard.mozilla.org/r/66028/#review121508

Looks good to me.
r=me with the following question answered.

::: netwerk/cookie/nsCookieService.cpp:2819
(Diff revision 6)
>          "creationTime, "
>          "isSecure, "
> -        "isHttpOnly "
> +        "isHttpOnly, "
> +        "baseDomain, "
> +        "originAttributes, "
> +        "sameSite "

baseDomain and originAttributes weren't listed here before. Was that a bug?

Comment 16

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8773284 [details]
Bug 1286858 - Data storage and interface changes for SameSite cookies

https://reviewboard.mozilla.org/r/66028/#review121596

::: netwerk/cookie/nsCookieService.cpp:2819
(Diff revision 6)
>          "creationTime, "
>          "isSecure, "
> -        "isHttpOnly "
> +        "isHttpOnly, "
> +        "baseDomain, "
> +        "originAttributes, "
> +        "sameSite "

Well, not really.

They're not actually used in GetCookieFromRow but we need to have them in the data for the row if we want to use IDX_SAME_SITE for the index in that function. Since the use of these greatly helps readability, I added them to that statement.

Comment 17

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b9fef87bb5a

Comment 18

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 19

[Jason Duell]

Not happening for 57.

Mark, what's the status with this work?  Anything we can do to move it along?

Comment 20

[Mark Goodwin [:mgoodwin]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #19)
> Not happening for 57.
> 
> Mark, what's the status with this work?  Anything we can do to move it along?

The patch on this bug needs rebasing and landing. With regards to the rest of the work; I'm unable to work on this in the near-term due to other priorities in CryptoEng - would be really happy to see someone else pick it up if someone in necko has time?

Comment 21

[Amy Chung [:Amy]]

I could take this bug.

Comment 22

[Amy Chung [:Amy]]

Attachment: Bug 1286858 - Data storage and interface changes for SameSite cookies

Comment 23

[Amy Chung [:Amy]]

[Try Server]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b304474708389e207dfdf6c66e1592a2a8a01f

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23221e95f9ca
Data storage and interface changes for SameSite cookies. r=valentin

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/23221e95f9ca