Skip to content

Change SameSite default to None#8043

Merged
Tratcher merged 2 commits intomasterfrom
tratcher/samesite
Mar 2, 2019
Merged

Change SameSite default to None#8043
Tratcher merged 2 commits intomasterfrom
tratcher/samesite

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
@Tratcher Tratcher requested review from HaoK and blowdart February 28, 2019 20:48
@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 middlewares 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 middlewares

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants