Closed Bug 1351663 Opened 7 years ago Closed 6 years ago

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

Categories

(WebExtensions :: Compatibility, enhancement, P5)

Product:
WebExtensions
Component:
Compatibility
Version:
52 Branch
Type:
enhancement

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: robwu, Assigned: robwu)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved] triaged)

Attachments

(4 files)

Bug 1351663 - Skip "optimization" if SameSite flag changes
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 1351663 - Ensure that OriginAttributes is initialized when nsCookieService::Add receives a SameSite parameter
59 bytes, text/x-review-board-request
valentin
: review+
Details
Bug 1351663 - Support SameSite flag in browser.cookies API
59 bytes, text/x-review-board-request
rpl
: review+
Details
Bug1351663.png
103.73 KB, image/png
Details 
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
Whiteboard: [design-decision-needed] triaged
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
(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
See Also: → 1286858
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 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 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+
Product: Toolkit → WebExtensions
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)
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 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 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)
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 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 on attachment 8984617 [details]
Bug 1351663 - Support SameSite flag in browser.cookies API

https://reviewboard.mozilla.org/r/250514/#review261004
Attachment #8984617 - Flags: review+
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
Attached image 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.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
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)
- 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)
Working on this now.
Flags: needinfo?(ismith)
(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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: