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

Use SameSite everywhere #308

Merged
merged 5 commits into from
Sep 20, 2019
Merged

Use SameSite everywhere #308

merged 5 commits into from
Sep 20, 2019

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Sep 9, 2019

@blowdart we took a community PR #299 that provided the initial SameSite implementation. This PR expands on that to more components, sets None for OIDC, lights up the System.Web implementation, etc..

The updated SystemWebCookieManager won't fix OIDC until System.Web gets patched.

I know there is some duplication here, this is old, decoupled code that we want to limit changes to.

Note that since this API never existed before we were able to make it nullable rather than adding Unspecified (-1) to the enum.

Also fixes #234

@Tratcher Tratcher self-assigned this Sep 9, 2019
@Tratcher Tratcher requested a review from HaoK September 9, 2019 21:04
@Tratcher Tratcher added this to the 4.1.0 milestone Sep 15, 2019
@Tratcher Tratcher marked this pull request as ready for review September 17, 2019 21:44
@Tratcher
Copy link
Member Author

@anurse let's get this one reviewed and merged so we can unblock partner testing via nightlies. I've run compat testing already. See the sample SameSiteCookieManager that we'll include in docs.

@Tratcher Tratcher merged commit 0fc4611 into dev Sep 20, 2019
@Tratcher Tratcher deleted the tratcher/samesite branch September 20, 2019 21:30
@josundt
Copy link

josundt commented Feb 10, 2020

@Tratcher and @anurse
Cookies that do not include any SameSite property has up till now been treated by browsers as if no SameSite policy was precent (= SameSite=None).

With the Chromium 80 update, the browser will start treating cookies without the SameSite property as SameSite=Lax. (Ref: https://www.chromium.org/updates/same-site).

This behavioral browser change breaks many applications, especially applications using OpenID Connect for authentication/authorization (since the OpenID Connect spec relies on IFRAMEs for certain features - Single-Sign-Out etc).

Such applications now need to explicitly include the SameSite=None property to work with Chromium 80.

But then another problem arises: Cookies with the SameSite=None property is misinterpreted in certain versions of Safari on Mac and iOS (SameSite=None is interpreted as SameSite=Strict !!!).
For this reason, user-agent sniffing is unfortunately required to make OpenID connect dependent applications work with Chromium 80 and certain Safari versions at the same time.

The code to detect user-agents that suffers from this SameSite=None defect can be found in the following article:

https://www.thinktecture.com/identity/samesite/prepare-your-identityserver/

I think Microsoft.Security.Owin.Cookies should include this user-agent detection in the CookieAuthenticationHandler class.

When CookieAuthenticationOptions.CookieSameSite = SameSiteMode.None, the cookie should omit the SameSite property when a browser with the SameSite=None defect is detected.

Until this is in place, I can't use the CookieAuhenticationMiddleware or CookieAuhenticationHandler classes, I needed to make my own implementations based on existing source code.

Let me know if a PRs are accepted as I have already implemented it for our applications.

I have now created an issue for this:
#333

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.

Please respect HttpOnly / Secure options in SignOut cookie
3 participants