Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for SameSite=None #89

Merged
merged 1 commit into from May 16, 2019

Conversation

Projects
None yet
3 participants
@rowan-m
Copy link
Contributor

commented May 7, 2019

Add support for the None value for the SameSite attribute as per https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-03

@dougwilson dougwilson self-assigned this May 8, 2019

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Nice! Now that None is finally a value of the attribute, we should probably make sameSite: false set the none header, instead of not setting the header. Would this make sense with your understanding of the new emun value?

@LouisStAmour

This comment has been minimized.

Copy link

commented May 10, 2019

On the one hand, SameSite defaults to None if unspecified, but on the other, the tests currently expect false to not set a value making changing false a backwards incompatible change. With every other flag, coincidentally or not, it looks to me like setting the option to false prevents the flag from being added. I’m not sure I see much benefit to changing that behaviour?

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Well, the spec is a draft and changes were always expected until it was finalized. It's more of a question here, for those most familiar with the spec and what one would expect. Like if a user passed in { sameSite: false } does what is produced now meet user expectations?

Remember that the sameSite was added to this module with there was no None option, so how it works now did not take into consider a None possibly.

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 13, 2019

So taking a look through the updated specification, it looks like if the attribute is not set, then it is specified to act the same as if it was SameSite=None. From the storage model specification:

  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".
@dougwilson

This comment has been minimized.

Copy link
Member

commented May 13, 2019

And the specification's changelog links to the following GitHub issue, which also says a missing SameSite attribute will have the identical behavior as SameSite=None: httpwg/http-extensions#788

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 13, 2019

The above is research into answering what { sameSite: false } should result in (#89 (comment)).

@rowan-m

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

So, yes - I agree that a false value should map to None. I can certainly update the PR to do that. However, it does feel like that moves it from being an addition to potentially introducing backwards compatibility issues. While clients should either ignore or interpret the None value, I'm wondering about people that might have more brittle test suites. Happy to go for it, but would appreciate your view on how that might affect the ecosystem.

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Hi @rowan-m . I was just updating your PR :D . The idea I had when I called out false when the original spec was added was that { sameSite: false } meant "no, this is not a same site cookie". The old drafts had no such enumeration as None and so left out the cookie for that case specifically, which is indeed why I added tests there.

As far as impact, there are a few things that work in our favor here:

  1. The version of this module is in the 0.x series of semver
  2. This is still implementing a draft, which is subject to change, so not changing is a hard thing to do
  3. There is already precedent for drastic changes, like when SameSite used to be called first party only and in version 0.3.0 is was simply removed and replaced with SameSite.

I can also update the false behavior in this module if you think it's agreeable to do so from just how the spec should work. I saw that Chrome announced it is going to go outside the existing draft on how it will treat a missing SameSite attribute?

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 14, 2019

So I've also been thinking about @LouisStAmour 's point where false just doesn't set any other flag to anything. I think a bunch of it is coincidence, but like { expires: false } one could say has a meaning and not setting expires is a different meaning from "false".

So I think, in the end, especially if this just eases landing, I'm leaning towards just keeping this PR as-is. Any yays / nays ?

@LouisStAmour

This comment has been minimized.

Copy link

commented May 14, 2019

So as stated earlier, my preference is to leave the PR as-is. The only reason I would consider changing it is if the spec more strongly encouraged implementors to set a SameSite value. But adding a value of None isn’t technically a deprecation or warning of the previous default behaviour, so I see no reason to change at this time. And if the default changes in future, keeping the existing behaviour is still valid for this library, it just produces a cookie that will be interpreted differently. If this were reading the property, I would consider returning a default value of “None” but there’s a difference between assembly/parsing and interpretation. If we keep the library as strictly assembly/parsing, then we shouldn’t make assumptions about interpretation that might change in future.

@rowan-m

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Makes sense to me. Enables developers to use the new value now, doesn't change any existing behaviour.

@dougwilson dougwilson merged commit b223a34 into jshttp:master May 16, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

dougwilson added a commit that referenced this pull request May 16, 2019

@dougwilson

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Included in the 0.4.0 release.

@dougwilson
Copy link
Member

left a comment

Landed 🎉

@rowan-m rowan-m deleted the rowan-m:samesite-none branch May 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.