search

vapor / http

🚀 Non-blocking, event-driven HTTP built on Swift NIO.
MIT License
240 stars 65 forks source link

SameSite Attribute support none, default = lax #376

Closed pankajsoni19 closed 4 years ago

pankajsoni19 commented 4 years ago

Adds the none attribute to SameSite cookies (#376). This resolves vapor/vapor#375.

⚠️ This contains an additional case in a public enum which may cause a breaking change if you're switching on it

0xTim commented 4 years ago

Just a note that this is going to need to be changed slightly. The same site config is a public enum so we can't just add a new case as that would need a major release

pankajsoni19 commented 4 years ago

I am using code from my forked repo. So getting this merged is not a priority for me. Though this is actually a big change from how cookies are handled in chrome, safari, firefox, that this should be merged in one way or other soon.

tanner0101 commented 4 years ago

We can't merge this as is for Vapor 3, we'd need to find a way that doesn't break public API. I've added this as a todo for 4 since we should be able to slip this in before official release there.

0xTim commented 4 years ago

@tanner0101 given the changes to how we handle enums as public APIs in Vapor - can we merge this PR?

tanner0101 commented 4 years ago

@0xTim yeah adding a case to this one should be fine. I'm more concerned about the change in default now actually. Will adding lax by default cause any issues?

0xTim commented 4 years ago

@tanner0101 Looking at this link we should be fine - if no same site attribute is set it defaults to lax anyway

0xTim commented 4 years ago

@tanner0101 Was that the right link? Also will the release bot handle Vapor 3 releases correctly?

tanner0101 commented 4 years ago

Oops, no meant to paste this one: https://github.com/vapor/vapor/blob/master/.github/contributing.md#releases

And yup release bot should work for this. If not, I'll make the tag manually after.

0xTim commented 4 years ago

@tanner0101 looks like release bot isn't enabled on this repo

tanner0101 commented 4 years ago

release-bot is added at the organization level, so it runs on all vapor/* repos. Looks like it gave a 200 response for that webhook so must be a bug. I'll look into it.

Tagged this manually at https://github.com/vapor/http/releases/tag/3.4.0. Thanks!