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

feat: add none as a valid sameSite option #111

Merged
merged 1 commit into from Oct 11, 2019
Merged

feat: add none as a valid sameSite option #111

merged 1 commit into from Oct 11, 2019

Conversation

@dougwilson dougwilson added this to the 0.8 milestone May 17, 2019
@dougwilson
Copy link
Contributor

Hi @panva thank you so much for putting this together! I am working to kick off the 0.8 branch to include the 0.8 milestone items in (including both your PRs) to get them out there very soon!

@panva
Copy link
Contributor Author

panva commented Sep 25, 2019

@dougwilson - can I somehow help with the 0.8 release? as this module is required by koa for handling cookies the absence of none as a possible value is a bummer for people getting frameworks ready for the chrome change.

Could we just merge this into the 0.7.x release without thinking about 0.8? Below 1.0.0 under semver this is a new feature which should not warrant 0.8 breaking change. That would mean koa does not have to have a new release with a new semver range included too.

@dougwilson
Copy link
Contributor

dougwilson commented Sep 25, 2019

The reason this is 0.8 is because it is a new feature, and since there are no rules in 0.x, I try to follow the path vs feature ones. Semver FAQ recommends also just bumping the minor part in the 0.x series for simplicity. But really, that has nothing to do with this not landing yet and the stuff slated for 0.8 above is not all required to land this.

Instead, from what I recall, there is an issue where another user objected to the way this pr was done and there was to be a discussion before landing this, which is why it was on hold. Did that discussion ever resolve?

@dougwilson
Copy link
Contributor

It is unfortunate as you happened to bump this at just the wrong time :) my priority today will be to resolve pillarjs/hbs#176 . Ideally I can get that resolved and out there before I leave on a trip. Once I get back from said trip, I will come back to this PR and check on what the state of the discussion is on the linked issue.

If you can help get a consensus on there that would help a lot. Otherwise we are looking for I will circle back on this in two weeks' time, I promise.

@panva
Copy link
Contributor Author

panva commented Sep 25, 2019

The issue was about allowing any value which I think there's a consensus on that its not needed. For now allowing a spec-defined value just like the other two is all that's needed.

If we could just land this in isolation and release soon it would be amazing. Other issues can probably wait for 0.9 then (milestone name change).

@dougwilson
Copy link
Contributor

Yes, I will do so in 2 weeks.

@panva
Copy link
Contributor Author

panva commented Oct 9, 2019

🙏

@dougwilson
Copy link
Contributor

Hi all, I will be working to get this published today, after two other modules that just need security fixes released, but they should be straightforward.

@dougwilson dougwilson mentioned this pull request Oct 9, 2019
5 tasks
@dougwilson dougwilson changed the base branch from master to 0.8 October 10, 2019 04:13
@dougwilson dougwilson self-assigned this Oct 10, 2019
@dougwilson dougwilson merged commit fac05de into pillarjs:0.8 Oct 11, 2019
@panva
Copy link
Contributor Author

panva commented Oct 12, 2019

Thank you @dougwilson

@panva panva deleted the feat-cookies-none branch October 12, 2019 07:14
clauderic added a commit to clauderic/cookies that referenced this pull request Nov 4, 2019
This PR follow's up on pillarjs#111 and updates the documentation to reflect the fact that `'none'` was added as an option that can be passed to `sameSite`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants