Use SameSite everywhere#308
Conversation
|
@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 and @anurse 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 !!!). The code to detect user-agents that suffers from this SameSite=None defect can be found in the following article: 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: |
@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