WICG / cookie-store

Asynchronous access to cookies from JavaScript
https://wicg.github.io/cookie-store/
Apache License 2.0
144 stars 35 forks source link

Add an "unspecified" option to CookieSameSite enum #102

Closed chlily1 closed 4 years ago

chlily1 commented 5 years ago

Given the recently updated RFC 6265bis, which adds a "SameSite=None" value in addition to Strict and Lax, I think there should be a distinction drawn between cookies that explicitly request SameSite=None, and cookies that just don't specify a SameSite attribute. The default behavior for the latter can change whereas the former will always mean "unrestricted".

I think we should add an "unspecified" option to the CookieSameSite enum to cover the latter case, distinct from the "unrestricted" option. In the Set a Cookie algorithm, we would then append SameSite/None to attributes for "unrestricted", and do nothing for "unspecified".

This would not change the default value away from "strict".

I have a Chromium CL up to add an "unspecified" value.

inexorabletash commented 5 years ago

It seems weird that "The default behavior for the latter can change" - wouldn't we want the behavior for a new API to always be predictable?

i.e. can't we take advantage of this being a new API to force developers to explicitly select the desired SameSite behavior? I guess this means the new API cannot be used to emulate the behavior of the old API, but is that a requirement?

(Note that I may be missing subtleties here, I haven't been thinking about cookies for a while. Please correct my mistakes!)

chlily1 commented 5 years ago

By "The default behavior for the latter can change", I meant that user agents can and do choose to treat explicitly-specified SameSite=None cookies differently from cookies that don't specify a SameSite attribute.

More accurately capturing this distinction would be the goal of adding "unspecified". If the user agent is treating the cookie as unspecified, it would be inaccurate to map it to any of the existing options, "unrestricted", "strict", or "lax".

You're right that one question is whether the API should allow the developer to set a cookie without SameSite and leave it up to the default behavior of the user agent, or force an explicit choice of SameSite while setting cookies. I think the latter is a reasonable option; if that's what we're going for, one option could be to expose "unspecified" for gets but not for sets.

(Either way, we probably want to append SameSite=None if there is an explicit choice of "unrestricted".)

inexorabletash commented 5 years ago

Thanks for the clarification!

chlily1 commented 5 years ago

Anyone have any opinions on this? As @pwnall suggested to me elsewhere, there are a few options:

  1. choose to say "unspecified" maps to "lax" (or "no restriction"; neither would be completely accurate)
  2. expose "unspecified" for both reads/writes (which is what my Chromium patch above would do)
  3. expose "unspecified" only on the read path and not for writes
domenic commented 5 years ago

Personally I like 3 and 1 more than 2.

oyiptong commented 5 years ago

I like 3 best, followed by 1 and 2.

To understand this better, IIUC:

Are my assumptions correct?

chlily1 commented 5 years ago

For Chromium:

  • the user would be able to configure the user agent to choose the default value for "unspecified"

Yes, this is an experimental setting that will be available as of M76.

  • the default behavior would be to map "unspecified" to "no restriction" by default

Yes, "unspecified" will map to "no restriction" by default; the user can choose to make "unspecified" behave like "lax".

oyiptong commented 5 years ago

Thanks for the clarification. I think I like 3, 2 and 1 in that order of preference.

I like 3 more strongly as it reflects the state of the developer's non-specification of the preference, the distinction you aim to achieve, actionable by the developer.

2 is then a close follower, though allowing unspecified on the write path, IIUC, is a cosmetic addition upon just not specifying SameSite.

1 is the least desirable to me, because, there's the loss of the developer preference.

chlily1 commented 5 years ago

Thanks for your input!

chlily1 commented 5 years ago

Updated Chromium CL.

inexorabletash commented 5 years ago

FYI, there are some just published docs that provide more context around these changes:

https://web.dev/samesite-cookies-explained/ https://tools.ietf.org/html/draft-west-cookie-incrementalism-00

I'll put a PR up for option 3

inexorabletash commented 5 years ago

But also... I want to revisit the decision in https://github.com/WICG/cookie-store/issues/36 about having SameSite=None be represented in the enum as "unrestricted" rather than "none".

When that was decided, the string "None" was not a value for the SameSite attribute in the cookie header; it was the implicit default. Now that can (and should) be explicitly specified as "None", diverging between the header and the API spelling for the same thing will be confusing for developers. I think the API should align with the RFC, and we should make the enum value "none".

inexorabletash commented 5 years ago

Thinking through the spec change for "unspecified":

Cookies normally get into the cookie store via the Set-Cookie header which processes the SameSite Attribute, and eventually invokes "receives a cookie" This same logic is followed for document.cookie (which invokes the same cookie header parsing) and this API (which invokes "receives a cookie").

The "receives a cookie" algorithm includes:

  1. If the cookie-attribute-list contains an attribute with an attribute-name of "SameSite", set the cookie's same-site-flag to attribute-value (i.e. either "Strict", "Lax", or "None"). Otherwise, set the cookie's same-site-flag to "None".

https://tools.ietf.org/html/draft-west-cookie-incrementalism-00 proposes changing this to:

  1. If the cookie-attribute-list contains an attribute with an attribute-name of "SameSite", set the cookie's same-site-flag to attribute-value. Otherwise, set the cookie's same-site-flag to "Lax".

Note that in either case, when that algorithm is finished the cookie has a specified same-site-flag of either "Strict", "Lax", or "None". So the cookie store will never hold a cookie with an unspecified same-site-flag.

Therefore, the query methods for this API would never see a cookie with an unspecified same-site-flag, and therefore would never need to handle that in create a CookieListItem from cookie.

So... at a spec level, I'm not sure how to easily define this. It would require this API to expose a cookie storage mechanism distinct from the RFC.

It looks like Chrome's implementation works this way - the "unspecified" state is persisted in the underlying cookie store, and mapped to "None" or "Lax" at a higher level and lazily.

chlily1 commented 5 years ago

Thanks for the analysis.

It looks like Chrome's implementation works this way - the "unspecified" state is persisted in the underlying cookie store, and mapped to "None" or "Lax" at a higher level and lazily.

This is unfortunately necessary because the samesite-by-default feature may be turned on and off by the user, so it needs to be switchable.

Therefore, the query methods for this API would never see a cookie with an unspecified same-site-flag, and therefore would never need to handle that in create a CookieListItem from cookie.

That's a good point. We could just forget about "unspecified" and just use what the RFC defines as the same-site-flag (in the Chromium implementation, the EffectiveSameSite -- this would probably require some plumbing to surface that information when constructing the CookieListItem).

ayuishii commented 4 years ago

Closing since it looks like there is a general agreement here for removing "unspecified" since query methods will never see a cookie with an unspecified same-site-flag, and setting it to how RFC defines as the same-site-flag.

I've created an issue here for Chromium side implementation.

Let me know if there are any outstanding points that still need discussion, and we can re-open. Thanks everyone!