Closed Bug 1551798 Opened 5 years ago Closed 5 years ago

Prototype SameSite=Lax by default

Categories

(Core :: Networking: Cookies, enhancement, P3)

Product:
Core
Component:
Networking: Cookies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-triaged])

Attachments

(6 files, 3 obsolete files)

Bug 1551798 - Rename nsICookie2.SAMESITE_UNSET to nsICookie2.SAMESITE_NONE, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1551798 - SameSite=lax by default and SameSite=none only if secure, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1551798 - SameSite=lax by default and SameSite=none only if secure - tests, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1551798 - Store sameSite value as received from the wire in the database - migration 10, r=ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1551798 - Test for cookie migration 10, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review
Bug 1551798 - nsICookie.sameSite should return rawSameSite if lax-by-default pref is set to false, r?ehsan
47 bytes, text/x-phabricator-request
Details | Review

We should consider this proposal:
https://tools.ietf.org/html/draft-west-cookie-incrementalism-00

In this bug I want to prototype it behind pref.

Priority: -- → P3
Whiteboard: [necko-triaged]

Can you please send an intent to implement thread? Thanks!

Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)

Thanks for bringing this to my attention via the intent-to-implement. Earlier, SameSite=none cookies was the topic of bug 1550032 (extension API support for SameSite=none).

Why was SAMESITE_NONE renamed to SAMESITE_UNSET, instead of adding SAMESITE_UNSET (and keeping SAMESITE_NONE)? RFC 6265bis adds the none value for SameSite to allow websites to explicitly force the behavior of SameSite=none.

You may also be interested in some of the external references in the comment at https://bugzilla.mozilla.org/show_bug.cgi?id=1550032#c1

Blocks: 1550032
Flags: needinfo?(amarchesini)

Why was SAMESITE_NONE renamed to SAMESITE_UNSET, instead of adding SAMESITE_UNSET (and keeping SAMESITE_NONE)? [RFC 6265bis]

My patch does the opposite. I renamed 'UNSET' to 'NONE' to follow the spec naming. (Note that my patches are not landed yet)
Existing code without my patches still uses 'UNSET': https://searchfox.org/mozilla-central/rev/952521e6164ddffa3f34bc8cfa5a81afc5b859c4/netwerk/cookie/nsICookie2.idl#17-19

'UNSET' is not a valid SameSite type. By spec/rfc if SameSite is not passed or it has a 'invalid/unknown' value, we should consider it as 'None':

'[...] If the value is "None", the cookie will be sent with same-site and cross-site requests. If the "SameSite" attribute's value is something other
than these three known keywords, the attribute's value will be treated as "None".'

Flags: needinfo?(amarchesini)
Type: defect → enhancement

In comment 5 I mixed the names, but the question was not about the name of the enum, but about why there is no new enum.

In the currrent implementation, sameSite=Lax may be stored in the database even if the site has not specified a SameSite flag. This means that even if the experiment is stopped, there will be cookies in the database with the sameSite=Lax flag, of which it is not clear whether it was explicit (via SameSite=Lax) or implicit (via this experiment). It is not possible to meaningfully migrate the cookies to SameSite=none if we wanted to.
And we cannot retroactively migrate existing cookies with SameSite=none to SameSite=Lax either, because we don't know if the site had explicitly set SameSite=none or whether this was an implicit default of the browser.

By having a separate state, SAMESITE_UNSET, we can freely try to respect the preference at cookie retrieval time depending on the preference, and also have the ability to migrate sameSite flag of cookies if desired.

Thanks Rob. I think you are right in that we need to store the value of the SameSite attribute that was specified by the client.

Andrea and I chatted about how to do that on IRC today. He suggested storing the value obtained from the client (either HTTP headers or document.cookie in JS) in a separate field of the database and keeping the existing logic which I think is a reasonable decision.

Depends on D32482

(In reply to :Ehsan Akhgari from comment #9)

Thanks Rob. I think you are right in that we need to store the value of the SameSite attribute that was specified by the client.

Andrea and I chatted about how to do that on IRC today. He suggested storing the value obtained from the client (either HTTP headers or document.cookie in JS) in a separate field of the database and keeping the existing logic which I think is a reasonable decision.

HTTP headers and document.cookie are not the only way to create cookies. nsCookieService::Add (via Services.cookies.add) can also create cookies, one of which is part of the API for extensions. Callers should ideally not be concerned about the logic of the sameSite calculation. Ideally, when sameSite is left unspecified, the defaults (depending on the pref) should be used. When the caller explicitly chooses the "None" value, sameSite should be forced to "None".

Storing in a separate database column could be useful if you want to migrate all cookies in the future. But it does not seem to be used when the pref is flipped (actually, in the WIP patch it's not read at all). What is the desired behavior when the pref is flipped in either direction?

Is it feasible to only have one member "sameSite" whose "sameSite" lazily becomes Lax or None depending on the preference? (possibly by introducing a new enum value different from SAMESITE_NONE, e.g. SAMESITE_UNSET.)

If you want to continue the direction of "rawSameSite", could you also allow callers of Services.cookies.add to modify this value? (to support the use case of bug 1550032)

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fe740c90e2b
Rename nsICookie2.SAMESITE_UNSET to nsICookie2.SAMESITE_NONE, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/d88b0002d736
SameSite=lax by default and SameSite=none only if secure, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/b87996b8a411
SameSite=lax by default and SameSite=none only if secure - tests, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/bbc3f88b8c03
Store sameSite value as received from the wire in the database - migration 10, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c89daff34d97
Test for cookie migration 10, r=Ehsan
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51e4490f3e16
Rename nsICookie2.SAMESITE_UNSET to nsICookie2.SAMESITE_NONE, r=Ehsan

https://hg.mozilla.org/mozilla-central/rev/51e4490f3e16

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox69: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

(In reply to Rob Wu [:robwu] from comment #12)

(In reply to :Ehsan Akhgari from comment #9)

Thanks Rob. I think you are right in that we need to store the value of the SameSite attribute that was specified by the client.

Andrea and I chatted about how to do that on IRC today. He suggested storing the value obtained from the client (either HTTP headers or document.cookie in JS) in a separate field of the database and keeping the existing logic which I think is a reasonable decision.

HTTP headers and document.cookie are not the only way to create cookies. nsCookieService::Add (via Services.cookies.add) can also create cookies, one of which is part of the API for extensions. Callers should ideally not be concerned about the logic of the sameSite calculation. Ideally, when sameSite is left unspecified, the defaults (depending on the pref) should be used. When the caller explicitly chooses the "None" value, sameSite should be forced to "None".

Right, I think I agree with everything above.

One thing to note here is that https://tools.ietf.org/html/draft-west-cookie-incrementalism-00 explains how to parse this attribute out of the Set-Cookie HTTP header (or things that look like it...) You are completely right that this feature, as Andrea has implemented it, isn't accessible from nsICookieManager.add() because, well, that's a low-level API and the Set-Cookie parsing code actually ends up calling that itself.

It is entirely reasonable to request adding a facility to that API to imitate the same behaviour at that level too. :-) (I actually don't think we'd want to reintroduce SAMESITE_UNSET for that, but rather mark the add method as [optional_argc] and make the sameSite argument optional to pass, and in case the caller omits it and the prefs are set and whatnot implement the same behaviour as draft-west-cookie-incrementalism-00.) Do you mind filing that bug please?

Storing in a separate database column could be useful if you want to migrate all cookies in the future. But it does not seem to be used when the pref is flipped (actually, in the WIP patch it's not read at all). What is the desired behavior when the pref is flipped in either direction?

The idea behind the rawSameSite column is to preserve the "on the wire" value, that is, the value found in the Set-Cookie header (or the equivalent string that the cookie manager parses cookies from). That column is only ever useful if we turned on this feature in an experiment and migrated users' data, and later on decided that this was a bad idea and we should go back; in that case it will allow us to restore the sameSite values for cookies that existed in the database before this feature was introduced.

Is it feasible to only have one member "sameSite" whose "sameSite" lazily becomes Lax or None depending on the preference? (possibly by introducing a new enum value different from SAMESITE_NONE, e.g. SAMESITE_UNSET.)

I don't believe that is a desirable change. Here is why. The sameSite-ness of cookies determines a very important property of them: which requests they will get attached to! Once a developer places a cookie that is SameSite=Lax, it is unexpected for that cookie to appear in the third-party context from that point onward. And note that the presence of this feature in the browser is feature detectable, so in general we should assume that developers will be able to tell if the browser treats their cookies as Lax because they didn't include the sameSite attribute.

Now let's imagine that for some reason the configuration of the browser is changed to disable this feature after it's been enabled, then using your approach suddenly such cookies that used to be SameSite=Lax will become SameSite=None and will start to appear in the third-party context. Cookies that are unexpected to web applications can break them in surprising ways.

Conceptually, it is helpful to think of the cookie database as a persistent store, that is, once you record a cookie there it will stay that way and won't change from under your nose based on things like browser configuration...

If you want to continue the direction of "rawSameSite", could you also allow callers of Services.cookies.add to modify this value? (to support the use case of bug 1550032)

I hope the above is sufficient to address that concern. If not, please do let us know!

Flags: needinfo?(rob)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ba747afb3d0
SameSite=lax by default and SameSite=none only if secure, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/eae7f88660c8
SameSite=lax by default and SameSite=none only if secure - tests, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/0f6e797b434f
Store sameSite value as received from the wire in the database - migration 10, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/f2ac8efb023d
Test for cookie migration 10, r=Ehsan
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11ddc433b632
Add comments about the next removing patches, r=Ehsan

Backed out for failures on test_rawSameSite.js

backout: https://hg.mozilla.org/integration/autoland/rev/733c887985c0c4e93e410e5f258d46b5e56d54d9

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=f2ac8efb023db179f685b54e3c9d8106842751c3&selectedJob=249123184

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249123184&repo=autoland&lineNumber=1624

[task 2019-05-30T06:22:18.579Z] 06:22:18 INFO - TEST-START | netwerk/cookie/test/unit/test_rawSameSite.js
[task 2019-05-30T06:22:19.085Z] 06:22:19 WARNING - TEST-UNEXPECTED-FAIL | netwerk/cookie/test/unit/test_rawSameSite.js | xpcshell return code: 0
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - TEST-INFO took 506ms
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - >>>>>>>
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | xpcw: cd /sdcard/tests/xpc/netwerk/cookie/test/unit
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _PREFS_FILE = "/sdcard/tests/xpc/user.js"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _HEAD_FILES = []; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_rawSameSite.js"]; -e const _TEST_NAME = "netwerk/cookie/test/unit/test_rawSameSite.js"; -e _execute_test(); quit(0);
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Unnamed thread 74f1f0421020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Unnamed thread 74f1f0421020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.085Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Unnamed thread 74f1f0421020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Unnamed thread 74f1f0421020] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: No Android crash handler set: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 1456
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2561
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/extensions/permissions/nsPermissionManager.cpp, line 2903
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 662
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - TEST-PASS | netwerk/cookie/test/unit/test_rawSameSite.js | run_test/< - [run_test/< : 89] 10 == 10
[task 2019-05-30T06:22:19.086Z] 06:22:19 WARNING - TEST-UNEXPECTED-FAIL | netwerk/cookie/test/unit/test_rawSameSite.js | run_test/< - [run_test/< : 94] false == true
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - test_rawSameSite.js:run_test/<:94
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - test_rawSameSite.js:run_test:85
[task 2019-05-30T06:22:19.086Z] 06:22:19 INFO - /sdcard/tests/xpc/head.js:_execute_test:523
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - -e:null:1
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - exiting test
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: OOPDeinit() without successful OOPInit(): file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 3055
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | [6325, Main Thread] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 194
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | nsStringStats
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mAllocCount: 10629
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mReallocCount: 0
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mFreeCount: 10629
[task 2019-05-30T06:22:19.087Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mShareCount: 9752
[task 2019-05-30T06:22:19.088Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mAdoptCount: 195
[task 2019-05-30T06:22:19.088Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => mAdoptFreeCount: 195
[task 2019-05-30T06:22:19.088Z] 06:22:19 INFO - netwerk/cookie/test/unit/test_rawSameSite.js | => Process ID: 6325, Thread ID: 128582692219712
[task 2019-05-30T06:22:19.088Z] 06:22:19 INFO - <<<<<<<

Flags: needinfo?(amarchesini)
See Also: → 1555652

Thanks for your reply Ehsan!

(In reply to :Ehsan Akhgari from comment #19)

(In reply to Rob Wu [:robwu] from comment #12)

(In reply to :Ehsan Akhgari from comment #9)

Thanks Rob. I think you are right in that we need to store the value of the SameSite attribute that was specified by the client.

Andrea and I chatted about how to do that on IRC today. He suggested storing the value obtained from the client (either HTTP headers or document.cookie in JS) in a separate field of the database and keeping the existing logic which I think is a reasonable decision.

HTTP headers and document.cookie are not the only way to create cookies. nsCookieService::Add (via Services.cookies.add) can also create cookies, one of which is part of the API for extensions. Callers should ideally not be concerned about the logic of the sameSite calculation. Ideally, when sameSite is left unspecified, the defaults (depending on the pref) should be used. When the caller explicitly chooses the "None" value, sameSite should be forced to "None".

Right, I think I agree with everything above.

One thing to note here is that https://tools.ietf.org/html/draft-west-cookie-incrementalism-00 explains how to parse this attribute out of the Set-Cookie HTTP header (or things that look like it...) You are completely right that this feature, as Andrea has implemented it, isn't accessible from nsICookieManager.add() because, well, that's a low-level API and the Set-Cookie parsing code actually ends up calling that itself.

It is entirely reasonable to request adding a facility to that API to imitate the same behaviour at that level too. :-) (I actually don't think we'd want to reintroduce SAMESITE_UNSET for that, but rather mark the add method as [optional_argc] and make the sameSite argument optional to pass, and in case the caller omits it and the prefs are set and whatnot implement the same behaviour as draft-west-cookie-incrementalism-00.) Do you mind filing that bug please?

While writing the bug (bug 1555652), the following thought stuck with me: rawSameSite supposedly reflects the "wire" value of SameSite. Since rawSameSite can only have the same values as the sameSite enum, the implementation can still not distinguish between an explicitly set SameSite=None/Lax vs an implicit (default) SameSite value.
In the database (rawSameSite column), there must be a way to distinguish explicit values versus unset values. Whether this is an enum value (SAMESITE_UNSET = -1) or NULL does not really matter.

In your proposed API, [optional_argc] could be used to differentiate between the two, but this would force all callers who care about preserving the wire value to have separate branches depending on whether SameSite is set, with calls only differing by the number of arguments. This looks like an unwieldy API, and being able to pass a value for "unset" would make it more easier to pass the right sameSite value (without accidentally changing the value when forwarding cookies). Since the database already needs a way to distinguish between the two, we could as well introduce a SAMESITE_UNSET value (maybe only used with rawSameSite and not sameSite).

If there is a SAMESITE_UNSET value, then there is no need for two separate parameters in nsCookieService::Add: The implementation can derive sameSite from the passed-in rawSameSite, and maintain the invariant (sameSiteRaw == sameSite || rawSameSite == SAMESITE_UNSET).

Storing in a separate database column could be useful if you want to migrate all cookies in the future. But it does not seem to be used when the pref is flipped (actually, in the WIP patch it's not read at all). What is the desired behavior when the pref is flipped in either direction?

The idea behind the rawSameSite column is to preserve the "on the wire" value, that is, the value found in the Set-Cookie header (or the equivalent string that the cookie manager parses cookies from). That column is only ever useful if we turned on this feature in an experiment and migrated users' data, and later on decided that this was a bad idea and we should go back; in that case it will allow us to restore the sameSite values for cookies that existed in the database before this feature was introduced.

In the current migration (https://phabricator.services.mozilla.com/D32482), all existing cookies in the database are assigned rawSameSite = default = SAMESITE_NONE (0), even if their sameSite had a different value. There is also no validation that (sameSiteRaw == sameSite || rawSameSite == SAMESITE_UNSET). Since there is currently no SAMESITE_UNSET, the validation would be weakened to (sameSiteRaw == sameSite || rawSameSite = SAMESITE_NONE).

Is it feasible to only have one member "sameSite" whose "sameSite" lazily becomes Lax or None depending on the preference? (possibly by introducing a new enum value different from SAMESITE_NONE, e.g. SAMESITE_UNSET.)

I don't believe that is a desirable change. Here is why. The sameSite-ness of cookies determines a very important property of them: which requests they will get attached to! Once a developer places a cookie that is SameSite=Lax, it is unexpected for that cookie to appear in the third-party context from that point onward. And note that the presence of this feature in the browser is feature detectable, so in general we should assume that developers will be able to tell if the browser treats their cookies as Lax because they didn't include the sameSite attribute.

Now let's imagine that for some reason the configuration of the browser is changed to disable this feature after it's been enabled, then using your approach suddenly such cookies that used to be SameSite=Lax will become SameSite=None and will start to appear in the third-party context. Cookies that are unexpected to web applications can break them in surprising ways.

Conceptually, it is helpful to think of the cookie database as a persistent store, that is, once you record a cookie there it will stay that way and won't change from under your nose based on things like browser configuration...

The idea of trying to maintain consistent behavior regardless of preferences sounds good.
The current approach results in little chance of breakage when cookies are not modified. For long-lasting write-once cookies, this approach works well.
But as soon as cookies are modified, sites that previously worked may start to behave unexpectedly. Here is a concrete scenario:

  1. First-Party (FP) Authentication provider sets cookie, without SameSite flag.
  2. Authentication provider loaded as third-party (3p) in frame: cookies included.
  4. This feature lands. SameSite=Lax by default. Existing cookies migrated to SameSite=None.
  5. 3p Authentication provider is able to read existing cookies.
  6. FP Authentication provider sets cookie without SameSite flag. Defaults to SameSite=Lax
  7. 3p Authentication provider is NOT able to read the updated cookies.
  9. This feature is disabled via the prefs. SameSite=None by default. No migration happens.
  10. 3p Authenication provider is STILL NOT able to read the updated cookies.
  11. FP Authentication provider reads the updated cookie and sets cookie without SameSite flag. Defaults to SameSite=None.
  12. 3p Authentication provider is able to read the last updated cookies.

The difference in behavior at step 5 and 7 is surprising.
If SameSite=Lax is not compatible with the web, then it may be better to know sooner than later (i.e. let step 4 behave as step 6), e.g. by storing "unspecified" as a separate state and determine the exact value at runtime.

I was initially suggesting to determine SameSite at runtime, to allow users to "unbreak" websites (that are incompatible with SameSite=Lax) by flipping the preference.

Chromium also takes the approach that I described, using "unspecified" cookies:

  • They use one field for sameSite, and support the "unspecified" enum value (ref)
  • When a specified value for sameSite is needed, the value is determined based on the preferences (ref).
  • Existing cookies are migrated to have the "unspecified" status (ref).
  • New cookies default to "unspecified" (ref).
  • Support "unspecified" in extension API (ref) (relevant to bug 1550032).

I do see that both approaches have their pros and cons, and although I favor the "unspecified" approach, I do not strongly object to the current proposed approach if you're consciously choosing to accept the disadvantages.

If you want to continue the direction of "rawSameSite", could you also allow callers of Services.cookies.add to modify this value? (to support the use case of bug 1550032)

I hope the above is sufficient to address that concern. If not, please do let us know!

Partially. Your proposed change to Services.cookies.add enables my use case of bug 1550032, but I still see some things that can be improved.

I don't want to force my opinion here, so I will just state that the incompleteness of rawSameSite (i.e. not knowing "unset" as described above) looks like a blocker to relanding the patches in this bug, and that my other suggestions (if accepted at all!) could be done in a separate bug if it is easier.

Flags: needinfo?(rob)

(In reply to Rob Wu [:robwu] from comment #23)

Thanks for your reply Ehsan!

(In reply to :Ehsan Akhgari from comment #19)

(In reply to Rob Wu [:robwu] from comment #12)

(In reply to :Ehsan Akhgari from comment #9)

Thanks Rob. I think you are right in that we need to store the value of the SameSite attribute that was specified by the client.

Andrea and I chatted about how to do that on IRC today. He suggested storing the value obtained from the client (either HTTP headers or document.cookie in JS) in a separate field of the database and keeping the existing logic which I think is a reasonable decision.

HTTP headers and document.cookie are not the only way to create cookies. nsCookieService::Add (via Services.cookies.add) can also create cookies, one of which is part of the API for extensions. Callers should ideally not be concerned about the logic of the sameSite calculation. Ideally, when sameSite is left unspecified, the defaults (depending on the pref) should be used. When the caller explicitly chooses the "None" value, sameSite should be forced to "None".

Right, I think I agree with everything above.

One thing to note here is that https://tools.ietf.org/html/draft-west-cookie-incrementalism-00 explains how to parse this attribute out of the Set-Cookie HTTP header (or things that look like it...) You are completely right that this feature, as Andrea has implemented it, isn't accessible from nsICookieManager.add() because, well, that's a low-level API and the Set-Cookie parsing code actually ends up calling that itself.

It is entirely reasonable to request adding a facility to that API to imitate the same behaviour at that level too. :-) (I actually don't think we'd want to reintroduce SAMESITE_UNSET for that, but rather mark the add method as [optional_argc] and make the sameSite argument optional to pass, and in case the caller omits it and the prefs are set and whatnot implement the same behaviour as draft-west-cookie-incrementalism-00.) Do you mind filing that bug please?

While writing the bug (bug 1555652), the following thought stuck with me: rawSameSite supposedly reflects the "wire" value of SameSite. Since rawSameSite can only have the same values as the sameSite enum, the implementation can still not distinguish between an explicitly set SameSite=None/Lax vs an implicit (default) SameSite value.
In the database (rawSameSite column), there must be a way to distinguish explicit values versus unset values. Whether this is an enum value (SAMESITE_UNSET = -1) or NULL does not really matter.

In your proposed API, [optional_argc] could be used to differentiate between the two, but this would force all callers who care about preserving the wire value to have separate branches depending on whether SameSite is set, with calls only differing by the number of arguments. This looks like an unwieldy API, and being able to pass a value for "unset" would make it more easier to pass the right sameSite value (without accidentally changing the value when forwarding cookies). Since the database already needs a way to distinguish between the two, we could as well introduce a SAMESITE_UNSET value (maybe only used with rawSameSite and not sameSite).

If there is a SAMESITE_UNSET value, then there is no need for two separate parameters in nsCookieService::Add: The implementation can derive sameSite from the passed-in rawSameSite, and maintain the invariant (sameSiteRaw == sameSite || rawSameSite == SAMESITE_UNSET).

I do hear you on the weirdness of the API I proposed with [optional_argc]. To be honest I just couldn't think of a better idea but I'm open to ideas.

But we need to preserve one important invariant here. An "unset" value for the sameSite attribute is meaningless as far as handling of cookies is concerned. The sameSite attribute for a cookie can only take three values: None, Lax and Strict. It is only upon the delivery of the cookie when sameSite could be left unspecified. But that doesn't make Unspecified a meaningful value for the sameSite attribute. Do you see the distinction?

Because of this, the problem with having a SAMESITE_UNSET value similar to other SAMESITE_* values is that it makes it look like the sameSite attribute can take four values not three.

Perhaps if we can't really think of anything better, we need a whole new enum for the input values to nsICookieManager.add(), like INPUT_SAMESITE_UNSPECIFIED, INPUT_SAMESITE_NONE and so on and let the sameSite attribute itself still take the three meaningful values?

Storing in a separate database column could be useful if you want to migrate all cookies in the future. But it does not seem to be used when the pref is flipped (actually, in the WIP patch it's not read at all). What is the desired behavior when the pref is flipped in either direction?

The idea behind the rawSameSite column is to preserve the "on the wire" value, that is, the value found in the Set-Cookie header (or the equivalent string that the cookie manager parses cookies from). That column is only ever useful if we turned on this feature in an experiment and migrated users' data, and later on decided that this was a bad idea and we should go back; in that case it will allow us to restore the sameSite values for cookies that existed in the database before this feature was introduced.

In the current migration (https://phabricator.services.mozilla.com/D32482), all existing cookies in the database are assigned rawSameSite = default = SAMESITE_NONE (0), even if their sameSite had a different value. There is also no validation that (sameSiteRaw == sameSite || rawSameSite == SAMESITE_UNSET). Since there is currently no SAMESITE_UNSET, the validation would be weakened to (sameSiteRaw == sameSite || rawSameSite = SAMESITE_NONE).

Yes. That is because the idea behind this proposal is that all cookies that were delivered before this proposal was activated in the browser which didn't specify a sameSite attribute would be treated as SameSite=None. (Note that this isn't super clear in the text of the draft, but we double-checked this point with Mike West.)

Is it feasible to only have one member "sameSite" whose "sameSite" lazily becomes Lax or None depending on the preference? (possibly by introducing a new enum value different from SAMESITE_NONE, e.g. SAMESITE_UNSET.)

I don't believe that is a desirable change. Here is why. The sameSite-ness of cookies determines a very important property of them: which requests they will get attached to! Once a developer places a cookie that is SameSite=Lax, it is unexpected for that cookie to appear in the third-party context from that point onward. And note that the presence of this feature in the browser is feature detectable, so in general we should assume that developers will be able to tell if the browser treats their cookies as Lax because they didn't include the sameSite attribute.

Now let's imagine that for some reason the configuration of the browser is changed to disable this feature after it's been enabled, then using your approach suddenly such cookies that used to be SameSite=Lax will become SameSite=None and will start to appear in the third-party context. Cookies that are unexpected to web applications can break them in surprising ways.

Conceptually, it is helpful to think of the cookie database as a persistent store, that is, once you record a cookie there it will stay that way and won't change from under your nose based on things like browser configuration...

The idea of trying to maintain consistent behavior regardless of preferences sounds good.
The current approach results in little chance of breakage when cookies are not modified. For long-lasting write-once cookies, this approach works well.
But as soon as cookies are modified, sites that previously worked may start to behave unexpectedly. Here is a concrete scenario:

  1. First-Party (FP) Authentication provider sets cookie, without SameSite flag.
  2. Authentication provider loaded as third-party (3p) in frame: cookies included.
  4. This feature lands. SameSite=Lax by default. Existing cookies migrated to SameSite=None.
  5. 3p Authentication provider is able to read existing cookies.
  6. FP Authentication provider sets cookie without SameSite flag. Defaults to SameSite=Lax
  7. 3p Authentication provider is NOT able to read the updated cookies.
  9. This feature is disabled via the prefs. SameSite=None by default. No migration happens.

I will note that your step 9 is purely imaginary and will never happen, FWIW. :-) The whole reason why we added the rawSameSite attribute was such that if we ever do step 9, we do so with a migration to restore the cookie to SameSite=None at this point.

  1. 3p Authenication provider is STILL NOT able to read the updated cookies.
  2. FP Authentication provider reads the updated cookie and sets cookie without SameSite flag. Defaults to SameSite=None.
  3. 3p Authentication provider is able to read the last updated cookies.

The difference in behavior at step 5 and 7 is surprising.

The surprise here being the lack of familiarity with https://tools.ietf.org/html/draft-west-cookie-incrementalism-00? :-) Because if I'm understanding your scenario correctly, what happens in step 7 is exactly the desired outcome of this proposal.

If SameSite=Lax is not compatible with the web, then it may be better to know sooner than later (i.e. let step 4 behave as step 6), e.g. by storing "unspecified" as a separate state and determine the exact value at runtime.

FWIW, SameSite=Lax has known incompatibilities with the web as it only covers safe HTTP methods (https://tools.ietf.org/html/rfc7231#section-4.2.1). It's not really a binary thing we're looking to find out here (compatible or incompatible with the web) but rather how incompatible...

I was initially suggesting to determine SameSite at runtime, to allow users to "unbreak" websites (that are incompatible with SameSite=Lax) by flipping the preference.

Chromium also takes the approach that I described, using "unspecified" cookies:

  • They use one field for sameSite, and support the "unspecified" enum value (ref)
  • When a specified value for sameSite is needed, the value is determined based on the preferences (ref).
  • Existing cookies are migrated to have the "unspecified" status (ref).
  • New cookies default to "unspecified" (ref).
  • Support "unspecified" in extension API (ref) (relevant to bug 1550032).

I do see that both approaches have their pros and cons, and although I favor the "unspecified" approach, I do not strongly object to the current proposed approach if you're consciously choosing to accept the disadvantages.

Indeed it seems like Chromium has taken the approach you're suggesting.

I don't think I have anything more to add on top of what I have said before. I think it is time for Andrea to say something here. FWIW I once spoke to him on IRC about your suggestion and wasn't able to convince him to change his approach.

I still think that writing something consistent to the cookie database makes it easier to reason about the state of cookies at any given time, but I think the point of being able to flip this pref and test whether a problem in a site would fix itself is a good one. Our current implementation doesn't provide such capability, and instead you'd basically need to retrace all of your steps to reproduce the problem in a new profile which doesn't have the pref set in the first place...

(In reply to :Ehsan Akhgari from comment #24)

Perhaps if we can't really think of anything better, we need a whole new enum for the input values to nsICookieManager.add(), like INPUT_SAMESITE_UNSPECIFIED, INPUT_SAMESITE_NONE and so on and let the sameSite attribute itself still take the three meaningful values?

Sounds good to me. sameSite could then have the value as described here, and rawSameSite would then store the original input.
This would fix bug 1555652.

In the current migration (https://phabricator.services.mozilla.com/D32482), all existing cookies in the database are assigned rawSameSite = default = SAMESITE_NONE (0), even if their sameSite had a different value.

Yes. That is because the idea behind this proposal is that all cookies that were delivered before this proposal was activated in the browser which didn't specify a sameSite attribute would be treated as SameSite=None. (Note that this isn't super clear in the text of the draft, but we double-checked this point with Mike West.)

(keeping the above quote, since it is relevant below)

  1. This feature lands. SameSite=Lax by default. Existing cookies migrated to SameSite=None.
  2. 3p Authentication provider is able to read existing cookies.
  3. FP Authentication provider sets cookie without SameSite flag. Defaults to SameSite=Lax
  4. 3p Authentication provider is NOT able to read the updated cookies.

The difference in behavior at step 5 and 7 is surprising.

The surprise here being the lack of familiarity with https://tools.ietf.org/html/draft-west-cookie-incrementalism-00? :-) Because if I'm understanding your scenario correctly, what happens in step 7 is exactly the desired outcome of this proposal.

The surprise is not that 7 happens, but that step 5 and 7 differs. I would expect 5 to behave like 7, i.e. previous absence of "SameSite" to be treated as "SameSite=Lax", and not "SameSite=None". Although the specification does not define migration of existing cookies, from the described change to step 13 of the algorithm for storing cookies I guessed that existing cookies would also be migrated to SameSite=Lax.

According to the previous quote (referencing Mike West), existing cookies have to be treated as SameSite=None. This is counter to my guess, and also counter to the current implementation of Chromium:

  • They use one field for sameSite, and support the "unspecified" enum value (ref)
  • When a specified value for sameSite is needed, the value is determined based on the preferences (ref).
  • Existing cookies are migrated to have the "unspecified" status (ref).
  • New cookies default to "unspecified" (ref).
  • Support "unspecified" in extension API (ref) (relevant to bug 1550032).

I do see that both approaches have their pros and cons, and although I favor the "unspecified" approach, I do not strongly object to the current proposed approach if you're consciously choosing to accept the disadvantages.

Indeed it seems like Chromium has taken the approach you're suggesting.

I wonder whether Chromium's implementation matches the intention of the specification editor (i.e. mkwst).
SameSite=Lax by default is a nice feature, and it would be quite nice if the major implementations behaved consistently.

(In reply to Rob Wu [:robwu] from comment #25)

In the current migration (https://phabricator.services.mozilla.com/D32482), all existing cookies in the database are assigned rawSameSite = default = SAMESITE_NONE (0), even if their sameSite had a different value.

Yes. That is because the idea behind this proposal is that all cookies that were delivered before this proposal was activated in the browser which didn't specify a sameSite attribute would be treated as SameSite=None. (Note that this isn't super clear in the text of the draft, but we double-checked this point with Mike West.)

(keeping the above quote, since it is relevant below)

  1. This feature lands. SameSite=Lax by default. Existing cookies migrated to SameSite=None.
  2. 3p Authentication provider is able to read existing cookies.
  3. FP Authentication provider sets cookie without SameSite flag. Defaults to SameSite=Lax
  4. 3p Authentication provider is NOT able to read the updated cookies.

The difference in behavior at step 5 and 7 is surprising.

The surprise here being the lack of familiarity with https://tools.ietf.org/html/draft-west-cookie-incrementalism-00? :-) Because if I'm understanding your scenario correctly, what happens in step 7 is exactly the desired outcome of this proposal.

The surprise is not that 7 happens, but that step 5 and 7 differs. I would expect 5 to behave like 7, i.e. previous absence of "SameSite" to be treated as "SameSite=Lax", and not "SameSite=None". Although the specification does not define migration of existing cookies, from the described change to step 13 of the algorithm for storing cookies I guessed that existing cookies would also be migrated to SameSite=Lax.

According to the previous quote (referencing Mike West), existing cookies have to be treated as SameSite=None. This is counter to my guess, and also counter to the current implementation of Chromium:

Yeah, this is actually puzzling to me. What I said in comment 24 was based on my reading of step 13 in that I read it such that it would only affect the storage of cookies received after the browser is in "lax by default" mode. We were not sure if this reading is accurate which is why we double checked with Mike.

But when you posted the Chromium code links I spent some time to read what they do and if I understand their code correctly, this is indeed not what they do... So I'm not puzzled.

  • They use one field for sameSite, and support the "unspecified" enum value (ref)
  • When a specified value for sameSite is needed, the value is determined based on the preferences (ref).
  • Existing cookies are migrated to have the "unspecified" status (ref).
  • New cookies default to "unspecified" (ref).
  • Support "unspecified" in extension API (ref) (relevant to bug 1550032).

I do see that both approaches have their pros and cons, and although I favor the "unspecified" approach, I do not strongly object to the current proposed approach if you're consciously choosing to accept the disadvantages.

Indeed it seems like Chromium has taken the approach you're suggesting.

I wonder whether Chromium's implementation matches the intention of the specification editor (i.e. mkwst).
SameSite=Lax by default is a nice feature, and it would be quite nice if the major implementations behaved consistently.

It very much looks like their implementation doesn't match Mike's intention. And yes, I certainly agree that ultimately implementations (and ideally the draft spec) would all agree on what it is that should be happening. :-) We should probably clarify this before relanding.

Thanks for the Interesting conversation.

There is a few major points here.

The first one is pref flip: as you know, I introduce a new 'rawSameSite' column that contains the sameSite value as received from the wire. This value is meant to be used only in case we want to revert/disable the feature. Currently the values in this column are not read. I have something better to propose: if the feature is enable, we read 'sameSite' column, if disabled, we read the 'rawSameSite' column. I can update the patches to have this pref check in the query string if we all agree on it.
Why this? 'rawSameSite' contains the value as received from the wire. Reading that value, we are exactly in the same configuration as we are now, without these patches. Let me go through the different options:

  1. pre-existing cookie with sameSite != unset/none (rawSameSite == sameSite)
    reading this cookie, we expose sameSite as the cookie wants.

  2. pre-existing cookie with sameSite == none (rawSameSite == sameSite)
    the cookie will be exposed as 'none' (previous called 'unset'). In case mkwst suggests to treat them as 'lax', we can add an extra migration to update 'sameSite' column to be 'lax' if rawSameSite is 'none'. This will make the cookie similar to the next option:

  3. a new cookie received without sameSite - treated as lax (rawSameSite = none; sameSite = lax)
    If the pref is enabled, we expose the cookie as 'lax'. Otherwise it will be exposed as 'none'.

  4. cookie received with sameSite == lax/strict/none (rawSameSite == sameSite == wire value)
    the cookie is exposed as received.

Note that I don't need to use 'unset' value at all. The current set of patches (with the pref check to be added) seems to be flexible enough.

About nsICookieService::add(), I think it's not a serious issue. The spec doesn't consider 'unset' as a possible value: we just have 'none', 'lax' and 'strict'. This XPCOM method is not directly exposed to content, and the caller should pass the correct value for same-site. The value is directly stored into the database. I suspect the main concern is about web-extension. We should decide what is the default value for sameSite unset for WebExtensions. To me, we should follow the same logic as network cookies.

Flags: needinfo?(amarchesini)
Attachment #9068436 - Attachment is obsolete: true

For webExtensions, 'rawSameSite' is always set to what has been received by the
cookie creation dictionary. 'sameSite', is equal to 'rawSameSite' if set in the
dictionary, otherwise it is set to the value of
nsICookieManager.defaultSameSite attribute.

nsICookieManager.defaultSameSite attribute returns None or Lax based on the
pref network.cookie.sameSite.laxByDefault.

Depends on D33305

robwu, I have a question for you: sameSite, in WebExtension code, is an optional attribute with default value "no_restriction":
https://searchfox.org/mozilla-central/rev/89235894292ac4ab4bd7dac44bc60c2e051905f4/toolkit/components/extensions/schemas/cookies.json#146

This means that it's not possible to detect "none" vs "unset". Are you planning to change the default value?
I would suggest to land the current code without the latest 2 patches (nsICookieManager.add() and nsICookie.rawSameSite exposed) and deal with this issue in a follow up. Is it OK for you?

I'm indeed planning to change the default in the WebExtensions API. The changes that I intend to make are listed at:
https://bugzilla.mozilla.org/show_bug.cgi?id=1550032#c3
https://bugzilla.mozilla.org/show_bug.cgi?id=1550032#c5

I would suggest to land the current code without the latest 2 patches ... and deal with this issue in a follow up.

I added two comments to https://phabricator.services.mozilla.com/D32482 , about updating logic to make sure that the rawSameSite and sameSite values have consistent and reliable values.
There is still a pending question (to mkwst) about whether to migrate existing cookies to SameSite=Lax or SameSite=None. If you don't mind potentially having to bump the database schema again, then you can land the patches after addressing https://phabricator.services.mozilla.com/D32482#988338

From https://phabricator.services.mozilla.com/D33306#986252
Talking to Andrea on IRC we decided to change the approach and add a new method for consumers who want to use the lax-by-default behaviour.

The use case is a bit wider than "lax-by-default". It is about preserving the rawSameSite value (or whatever can be used to remember that sameSite was unset) when editing/restoring cookies.

Attachment #9069025 - Attachment is obsolete: true
Attachment #9069016 - Attachment is obsolete: true
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e2cb9325361
SameSite=lax by default and SameSite=none only if secure, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/bd894a6a1b39
SameSite=lax by default and SameSite=none only if secure - tests, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/6a1d1b2c43ad
Store sameSite value as received from the wire in the database - migration 10, r=Ehsan,robwu
https://hg.mozilla.org/integration/autoland/rev/46779c833e09
Test for cookie migration 10, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/457ea1f6e8e3
nsICookie.sameSite should return rawSameSite if lax-by-default pref is set to false, r=Ehsan

https://blog.chromium.org/2019/10/developers-get-ready-for-new.html

Chrome plans to implement the new model with Chrome 80 in February 2020.

Blocks: 1604212
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: