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

Backport to 1.0: allow explicit SameSite=None cookies (#1282) #1346

Closed
wants to merge 1 commit into from

Conversation

theikkila
Copy link

fixes #1035
(cherry picked from commit a328794)

@theikkila theikkila changed the title Backport: allow explicit SameSite=None cookies (#1282) Backport to 1.0: allow explicit SameSite=None cookies (#1282) Feb 7, 2020
@robjtede robjtede requested a review from a team February 7, 2020 16:24
@robjtede
Copy link
Member

robjtede commented Feb 7, 2020

Thanks for the backport. Not sure what the best approach is for the changelogs. @actix/contributors

@JohnTitor
Copy link
Member

Uhm, I wouldn't backport this as discussed on former PR.

@JohnTitor
Copy link
Member

As doc says, None here means "No SameSite attribute" and we should expect so. One avoidance is to add "The None SameSite attribute" and leave None variant. But naming is a problem and may cause confusion (yeah, current naming also causes it though :/).

@robjtede
Copy link
Member

This is cherry picked from my fix. The issue before was that None and Some(SameSite::None) are treated the same which is wrong.

@JohnTitor
Copy link
Member

Yeah, I'd just say when you want to fix it on 2.0 or 1.0, you should come up with another way that doesn't break current behavior.

@robjtede
Copy link
Member

Hmm, I stil consider this a bug and not a breaking change per se.
To do what you're asking there'd have to be either a feature flag or an extra field on Cookie to enable the behavior.

@JohnTitor
Copy link
Member

Uhmm, okay I'll re-visit here once we make a new alpha release for actix-http.

@JohnTitor
Copy link
Member

Okay, I was thinking about this and would like to accept. But we're going ahead to 3.0 so we should backport to 2.0.

@robjtede
Copy link
Member

In light of recent issues, I don't think we should backport this without also backporting the cookie crate migration... @actix/core-contributors ?

@robjtede robjtede closed this Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants