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

SameSite.None for cookies is ignored #12125

Closed
kevingosse opened this issue Jul 12, 2019 · 6 comments
Closed

SameSite.None for cookies is ignored #12125

kevingosse opened this issue Jul 12, 2019 · 6 comments
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer breaking-change This issue / pr will introduce a breaking change, when resolved / merged.

Comments

@kevingosse
Copy link

When setting SameSite to None in a cookie, the value isn't added to the actual cookie:

https://github.com/aspnet/AspNetCore/blob/master/src/Http/Headers/src/SetCookieHeaderValue.cs#L132-L136

This was fine until now because None was the default value used by all web browsers when the property is not set.
Unfortunately, Chrome is changing that, and starting with version 80 the default value will be Lax: https://www.chromestatus.com/feature/5088147346030592

It means that whenever we set SameSite to None in AspNetCore, no value is sent to the browser, and it's going to be interpreted as "Lax" by Chrome.

As a fix, we can either remove the if (SameSite != SameSiteMode.None) check in SetCookieHeaderValue, or add a "SameSiteSet" field to detect when the user wants to let the browser decide of the default value.

@blowdart
Copy link
Contributor

@Tratcher

@Tratcher
Copy link
Member

That's because "None" isn't defined in the current spec as a valid header value, only Lax and Strict. None specified by the absence of the SameSite key.
https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1

Apparently None is defined in a new draft standard where they change the default to Lax.
https://tools.ietf.org/html/draft-west-cookie-incrementalism-00

This is a very problematic change as Lax is incompatible with OAuth & OIDC. We'd have to patch every supported version and add support to Katana.

It's also noted in https://www.chromestatus.com/feature/5088147346030592 that None is incompatible with Safari, you end up with Strict behavior instead. That's an absolute block from making this change, there would be no combination of settings that worked in both Chrome and Safari.

@Tratcher Tratcher added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer breaking-change This issue / pr will introduce a breaking change, when resolved / merged. and removed area-servers labels Jul 27, 2019
@analogrelay analogrelay added this to the 3.1.0 milestone Aug 1, 2019
@analogrelay
Copy link
Contributor

This will likely need to be evaluated as a backport to 2.1/2.2/3.0. We will need to wait until we can validate major browsers support SameSite=none. We'll have to quirk this when we service older releases.

@Tratcher
Copy link
Member

Tratcher commented Sep 4, 2019

"• P0 - Chrome change (SameSite cookie - https://www.chromestatus.com/feature/5088147346030592) in Chrome 78 beta (Sep 19), and Chrome 80 stable (Jan 2020).
• P1 - Safari change (ITP 2.2 included in iOS 12.3, already live)."

@Tratcher
Copy link
Member

Tratcher commented Oct 1, 2019

Fixed in 3.1.0-preview1. Leaving this open to track all of the patches.

Tratcher added a commit that referenced this issue Oct 2, 2019
Tratcher added a commit that referenced this issue Oct 2, 2019
Tratcher added a commit that referenced this issue Oct 2, 2019
analogrelay pushed a commit that referenced this issue Oct 3, 2019
* Re-implement SameSite for 2019 #12125

* Rename compat flag

* Use Microsoft.AspNetCore.SuppressSameSiteNone compat key

* Backport CookiePolicy quirk and sample

* Patch config
analogrelay pushed a commit that referenced this issue Oct 3, 2019
* Re-implement SameSite for 2019 #12125

* Rename compat flag

* References

* Use Microsoft.AspNetCore.SuppressSameSiteNone compat key

* Patchconfig

* Port CookiePolicy fix
Tratcher added a commit that referenced this issue Oct 8, 2019
@analogrelay
Copy link
Contributor

The patches are all in, right? I think this can be closed now.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

No branches or pull requests

5 participants