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

Change SameSite default to None #8043

Merged
merged 2 commits into from Mar 2, 2019
Merged

Change SameSite default to None #8043

merged 2 commits into from Mar 2, 2019

Conversation

Tratcher
Copy link
Member

#2675 #4661 Note this changes the basic infrastructure defaults but does not change any given component's behavior as each component already specified their SameSite config.

SameSite defaults:

  • Session: Lax
  • CookieTempDataProvider: Lax
  • Antiforgery: Strict
  • CookieAuth: Lax
  • Twitter state cookie: Lax
  • Remote auth corrilation cookie (OAuth): None
  • OIDC nonce cookie: None

[WIP] Running tests to make sure I didn't miss any.

@Tratcher Tratcher added this to the 3.0.0-preview4 milestone Feb 28, 2019
@Tratcher Tratcher self-assigned this Feb 28, 2019
@blowdart
Copy link
Contributor

If this doesn't affect any of our components anyway, then why do the change from "most secure" at all? We'd still be broken.

@Tratcher
Copy link
Member Author

The point here is that SameSite has been breaking arbitrary components that aren't aware of it so we're changing to an opt-in model. Our components have already opted in as far as they're able.

@blowdart
Copy link
Contributor

Opt-in to security is generally not the route we take though

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine code wise, I defer to @blowdart if this is something that's safe or good to do

@Tratcher
Copy link
Member Author

If it was a stable security feature I'd agree, but it's not, Apple keeps breaking it.

@blowdart
Copy link
Contributor

So now I ask the impossible, if it's affecting other components can I get an idea of how widespread this is?

@Tratcher
Copy link
Member Author

We have three datapoints:

  1. The CookiePolicy SameSite default broke all remote auth and had to be disabled in the templates. People still regularly break this if they don't have that line.
  2. Anyone calling Response.Cookies.Append also defaulted to Lax. There haven't been as many direct reports of this issue.
  3. Anyone using Lax on IOS is broken and they've had to disable it everywhere. We've gotten a lot of hits on this one and it's not just auth cookies, it affects session, app cookies (See 2), etc..

This change fixes the first two. It doesn't fix the 3rd.

@blowdart
Copy link
Contributor

OK fair, I submit :)

Copy link
Contributor

@blowdart blowdart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks meh to me, but what can I do? :D

@Tratcher Tratcher marked this pull request as ready for review March 1, 2019 17:40
@Tratcher Tratcher requested a review from jkotalik as a code owner March 1, 2019 17:40
@analogrelay
Copy link
Contributor

@Tratcher please rebase on master ASAP. This is failing due to a flaky test that has been skipped in master

@analogrelay
Copy link
Contributor

/azp run AspNetCore-helix-test

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Tratcher Tratcher merged commit 93b195e into master Mar 2, 2019
@Tratcher Tratcher deleted the tratcher/samesite branch March 2, 2019 00:22
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants