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.
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
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
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
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:
- First-Party (FP) Authentication provider sets cookie, without SameSite flag.
- Authentication provider loaded as third-party (3p) in frame: cookies included.
- This feature lands. SameSite=Lax by default. Existing cookies migrated to SameSite=None.
- 3p Authentication provider is able to read existing cookies.
- FP Authentication provider sets cookie without SameSite flag. Defaults to SameSite=Lax
- 3p Authentication provider is NOT able to read the updated cookies.
- This feature is disabled via the prefs. SameSite=None by default. No migration happens.
- 3p Authenication provider is STILL NOT able to read the updated cookies.
- FP Authentication provider reads the updated cookie and sets cookie without SameSite flag. Defaults to SameSite=None.
- 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.