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

VERIFIED FIXED in Firefox 63

Status

P5
normal
VERIFIED FIXED
2 years ago
4 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

52 Branch
mozilla63
dev-doc-complete

Firefox Tracking Flags

(firefox63 verified)

Details

(Whiteboard: [design-decision-approved] triaged)

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
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

Updated

2 years ago
Whiteboard: [design-decision-needed] triaged

Updated

2 years ago
Priority: -- → P5
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
(Assignee)

Comment 2

2 years ago
(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.
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged

Updated

11 months ago
See Also: → bug 1286858
(Assignee)

Comment 3

8 months ago
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.
Assignee: nobody → rob
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

8 months ago
mozreview-review
Comment on attachment 8984615 [details]
Bug 1351663 - Skip "optimization" if SameSite flag changes

https://reviewboard.mozilla.org/r/250500/#review256830
Attachment #8984615 - Flags: review?(valentin.gosu) → review+

Comment 8

8 months ago
mozreview-review
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
Attachment #8984616 - Flags: review?(valentin.gosu) → review+

Updated

7 months ago
Product: Toolkit → WebExtensions

Comment 9

7 months ago
mozreview-review
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").
Attachment #8984617 - Flags: review?(lgreco)
(Assignee)

Comment 10

7 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

7 months ago
mozreview-review-reply
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

7 months ago
mozreview-review
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?
Attachment #8984617 - Flags: review?(lgreco)
(Assignee)

Comment 16

7 months ago
mozreview-review-reply
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

7 months ago
mozreview-review-reply
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

7 months ago
mozreview-review
Comment on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review261004
Attachment #8984617 - Flags: review+

Updated

7 months ago
Keywords: dev-doc-needed

Comment 19

7 months ago
Pushed by rob@robwu.nl:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a146704d807d
https://hg.mozilla.org/mozilla-central/rev/75fd56bd459c
https://hg.mozilla.org/mozilla-central/rev/75cbadc45030
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 21

6 months ago
Created attachment 8997434 [details]
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.

Updated

6 months ago
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified

Comment 22

4 months ago
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.
Flags: needinfo?(rob)
(Assignee)

Comment 23

4 months ago
- 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
Flags: needinfo?(rob) → needinfo?(ismith)

Comment 24

4 months ago
Working on this now.
Flags: needinfo?(ismith)

Comment 25

4 months ago
(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

Updated

4 months ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.