1351663 - Add sameSite attribute (no_restriction, lax, strict) to browser.cookies API

Status: VERIFIED FIXED

(WebExtensions :: Compatibility, enhancement, P5)

Description

[Rob Wu [:robwu]]

Once support for SameSite cookies lands (bug 795346), the browser.cookies WebExtension API should be updated to also provide information about the sameSite tag on cookies.

This is the documentation for the corresponding feature in Chrome:
https://developer.chrome.com/extensions/cookies#type-Cookie
https://developer.chrome.com/extensions/cookies#type-SameSiteStatus

Comment 1

[Caitlin Neiman [:caitmuenster]]

Hi Rob, this has been added to the agenda for the August 8 WebExtensions APIs triage meeting. Would you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1I-i1FQ38nwFHNl4hnzTctdsYHXOjJpeAgm1Bm3VoUUA/edit#heading=h.ixspnh6vr384

Comment 2

[Rob Wu [:robwu]]

(In reply to Caitlin Neiman (http://pronoun.is/she) from comment #1)
> Hi Rob, this has been added to the agenda for the August 8 WebExtensions
> APIs triage meeting. Would you be able to join us? 

I will attend if I have no other obligations at that time.
But there is nothing to implement yet, because the sameSite feature is not yet present in Firefox (see blocking bug 795346).
Once that bug is fixed, it is relatively easy to support the attribute in the extension API.

Comment 3

[Rob Wu [:robwu]]

Firefox has already supported SameSite cookies for some releases.

I'm going to implement the extension API now; otherwise cookie manager extensions will destroy the SameSite flag when they modify cookies.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1351663 - Skip "optimization" if SameSite flag changes

After writing a unit test I discovered that updating a cookie's samesite
flag did not work. This is caused by an "optimization" that avoids
modifying a cookie if any of the cookie attributes were not changed.
This check did not account for the SameSite flag, until now.

A unit test for this will be added in the next commit, at
toolkit/components/extensions/test/mochitest/test_ext_cookies_samesite.html

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

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1351663 - Ensure that OriginAttributes is initialized when nsCookieService::Add receives a SameSite parameter

InitializeOriginAttributes takes aArgc and only initializes the
parameter when aArgc is 1. nsCookieService::Add takes another optional
parameter, namely aSameSite. If a caller sets this SameSite flag, then
InitializeOriginAttributes would skip the initialization of the
OriginAttributes.

This was caught by a private browsing test in
toolkit/components/extensions/test/mochitest/test_ext_cookies.html
(after I added support for SameSite flag in the extension API)

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

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1351663 - Support SameSite flag in browser.cookies API

A part of cookies.json is copied from
https://chromium.googlesource.com/chromium/src/+/ea90e377568cce415d261ecd472ab941ccc42a8e/chrome/common/extensions/api/cookies.json

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

Comment 7

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

Comment on attachment 8984615 [details]
Bug 1351663 - Skip "optimization" if SameSite flag changes

https://reviewboard.mozilla.org/r/250500/#review256830

Comment 8

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

Comment on attachment 8984616 [details]
Bug 1351663 - Ensure that OriginAttributes is initialized when nsCookieService::Add receives a SameSite parameter

https://reviewboard.mozilla.org/r/250502/#review256832

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review259652

Hi Rob, sorry for let you wait for this first review round.

The change itself looks great to me, but I'm wondering if we could convert the new mochitest included in this patch into an xpcshell test.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_samesite.html:17
(Diff revision 1)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +add_task(async function test_samesite_cookies() {

is there anything that is preventing this mochitest to be turned into an xpcshell test?

e.g. webRequest has a xpcshell test for same site cookies handling in webRequest: https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/extensions/test/xpcshell/test_ext_same_site_cookies.js

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_samesite.html:30
(Diff revision 1)
> +        document.cookie = "test3=whatever; SameSite=strict";
> +        `,
> +    });
> +
> +    // Baseline. Every cookie must have the expected sameSite.
> +    let cookie = await browser.cookies.get({url, name: "test1"});

Nit, instead of "test1", "test2" and "test3" we could use a cookie name that also allow us to recognize immediately which behavior they are testing (e.g. we could name them "test_no_restriction", "test_lax" and "test_strict").

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review259652

> is there anything that is preventing this mochitest to be turned into an xpcshell test?
> 
> e.g. webRequest has a xpcshell test for same site cookies handling in webRequest: https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/components/extensions/test/xpcshell/test_ext_same_site_cookies.js

I initially created a mochitest because all other cookie API tests were mochitests.
Now I've converted it to a xpcshell test and it still works fine.

> Nit, instead of "test1", "test2" and "test3" we could use a cookie name that also allow us to recognize immediately which behavior they are testing (e.g. we could name them "test_no_restriction", "test_lax" and "test_strict").

While test1 and test2 have a constant `sameSite` value throughout the test, the `test3` cookie changes `sameSite` during the test. The expectation does already clearly show the expected `sameSite` value, so I'm inclined to keep the names as-is.

Comment 11

[Rob Wu [:robwu]]

Comment on attachment 8984615 [details]
Bug 1351663 - Skip "optimization" if SameSite flag changes

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

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8984616 [details]
Bug 1351663 - Ensure that OriginAttributes is initialized when nsCookieService::Add receives a SameSite parameter

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

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

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

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review259652

> While test1 and test2 have a constant `sameSite` value throughout the test, the `test3` cookie changes `sameSite` during the test. The expectation does already clearly show the expected `sameSite` value, so I'm inclined to keep the names as-is.

Sounds good, it is not a big deal.

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review260976

::: toolkit/components/extensions/schemas/cookies.json:28
(Diff revision 2)
>      "permissions": ["cookies"],
>      "types": [
>        {
> +        "id": "SameSiteStatus",
> +        "type": "string",
> +        "enum": ["no_restriction", "lax", "strict"],

How about renaming "no_restriction" to "unset"?

or to consider `null` or `undefined` as the value expected for a cookie that doesn't have any SameSite restriction?

Comment 16

[Rob Wu [:robwu]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review260976

> How about renaming "no_restriction" to "unset"?
> 
> or to consider `null` or `undefined` as the value expected for a cookie that doesn't have any SameSite restriction?

For API compatibility with Chrome, we have to use `no_restriction`.

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review260976

> For API compatibility with Chrome, we have to use `no_restriction`.

Well, in that case, I definitely agree with you.

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review261004

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a146704d807d
Skip "optimization" if SameSite flag changes r=valentin
https://hg.mozilla.org/integration/autoland/rev/75fd56bd459c
Ensure that OriginAttributes is initialized when nsCookieService::Add receives a SameSite parameter r=valentin
https://hg.mozilla.org/integration/autoland/rev/75cbadc45030
Support SameSite flag in browser.cookies API r=rpl

Comment 20

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/a146704d807d
https://hg.mozilla.org/mozilla-central/rev/75fd56bd459c
https://hg.mozilla.org/mozilla-central/rev/75cbadc45030

Comment 21

[CosminB]

Attachment: Bug1351663.png

This issue is verified as fixed on Firefox 63.0a1 (20180803104322) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached screenshot.

Comment 22

[Irene Smith]

Added the sameSite property to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/Cookie

sameSite
    A cookies.SameSiteStatus value that indicates the SameSite state of the cookie.

Added a page for the SameSiteStatus enumeration: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/SameSiteStatus

Also added the optional parameter to the set method: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/set

sameSite{{optional_inline}}
    A {{WebExtAPIRef("cookies.SameSiteStatus")}} value that indicates the SameSite state of the cookie. If omitted, it defaults to 0, 'no_restriction'.

Added the following to the release notes https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63:

cookies.Cookie now includes a property indicating the SameSite state of the cookie. The cookies.SameSiteStatus enumeration defines SameSite state values (bug 1351663).

I don't see anything about sameSite mentioned int he set method but Chrome defines it as an optional parameter so I added it. Let me know if it should be removed.

Comment 23

[Rob Wu [:robwu]]

- SameSiteStatus is not an integer but a string: https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/toolkit/components/extensions/schemas/cookies.json#24-29

- For the formatting of the enum documentation, could you try to be consistent with other enums, e.g. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/OnChangedCause ?

- The sidebar in MDN lists several types, but SameSiteStatus is not there. Could you add it?

- At https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/cookies/SameSiteStatus, could you link to the existing documentation of SameSite cookies?
  https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#SameSite_cookies

- Since the existing Web/HTTP/Cookies documentation is missing information, could you also extend the section? You could use this blog post as the basis for documentation (or just link to that blog post): https://blog.mozilla.org/security/2018/04/24/same-site-cookies-in-firefox-60/

- Please add "SameSite" to the compat table at the bottom of the documentation for cookies.set and cookies.Cookie. You can do that by sending a pull request to
  https://github.com/mdn/browser-compat-data
  This is the file that you need to edit:
  https://github.com/mdn/browser-compat-data/blob/master/webextensions/api/cookies.json

Comment 24

[Irene Smith]

Working on this now.

Comment 25

[Irene Smith]

(In reply to Rob Wu [:robwu] from comment #23)
> - SameSiteStatus is not an integer but a string:
> https://searchfox.org/mozilla-central/rev/
> a23c3959b62cd3f616435e02810988ef5bac9031/toolkit/components/extensions/
> schemas/cookies.json#24-29
> 
> - For the formatting of the enum documentation, could you try to be
> consistent with other enums, e.g.
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> cookies/OnChangedCause ?
> 
> - The sidebar in MDN lists several types, but SameSiteStatus is not there.
> Could you add it?

sameSiteStatus is now in the sidebar.

> 
> - At
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> cookies/SameSiteStatus, could you link to the existing documentation of
> SameSite cookies?
>   https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#SameSite_cookies

done

> 
> - Since the existing Web/HTTP/Cookies documentation is missing information,
> could you also extend the section? You could use this blog post as the basis
> for documentation (or just link to that blog post):
> https://blog.mozilla.org/security/2018/04/24/same-site-cookies-in-firefox-60/
> 

I have no problem with updating the page. I have created a github issue for the update:
  https://github.com/mdn/sprints/issues/504

> - Please add "SameSite" to the compat table at the bottom of the
> documentation for cookies.set and cookies.Cookie. You can do that by sending
> a pull request to
>   https://github.com/mdn/browser-compat-data
>   This is the file that you need to edit:
>  
> https://github.com/mdn/browser-compat-data/blob/master/webextensions/api/
> cookies.json

pull request created