Skip to content
This repository was archived by the owner on Dec 13, 2018. It is now read-only.

OAuth authentication broken due to SameSite cookie policy #1231

Closed
analogrelay opened this issue May 24, 2017 · 8 comments
Closed

OAuth authentication broken due to SameSite cookie policy #1231

analogrelay opened this issue May 24, 2017 · 8 comments
Assignees

Comments

@analogrelay
Copy link

The SameSite=strict cookie default added in #908 breaks OAuth handlers setting Cookies on signin because the user action was taken on a different origin.

The problem only when the user actually has to log in or otherwise interact with their OAuth provider. In that case, user action is initiated in the domain of the OAuth provider, so when the browser redirects back to the relying party, which then sets a SameSite=strict cookie, the browser refuses to send it on the next request. The flow is something like this (for app at http://app and provider at http://provider:

  1. Browser sends GET http://app
  2. 302 redirect to http://provider
  3. Browser sends GET http://provider
  4. 200 response with login page
  5. User completes login
  6. Browser sends POST http://provider
  7. 302 redirect to http://app/signin-oauth?code=...&state=...
  8. Browser sends GET http://app/signin-oauth?code=...&state=...
  9. 302 redirect to http://app with Set-Cookie header with samesite=strict
  10. Browser sends GET http://app however since the last document rendered to the user had an origin of provider, it refuses to send the cookie set by step 9.

This leads to a cycle in most ASP.NET OAuth configurations because it keeps trying to authenticate via a series of 302s (because the provider a) already has a login cookie and b) is the origin site, so it keeps getting it's login cookie).

This should repro trivially by running any of the social login samples in an incognito window (forcing you to login).

Assuming I'm not missing something simple, all OAuth-based authentication providers are broken by this new default. We need to make some kind of fix/change in preview 2.

/cc @blowdart @Tratcher @muratg

@analogrelay
Copy link
Author

To clarify, I believe only Chrome is broken right now because Chrome is the only browser properly adhering to SameSite, but as other browsers come online they will be broken.

@muratg
Copy link
Contributor

muratg commented May 24, 2017

cc @JunTaoLuo

@analogrelay
Copy link
Author

FYI: The way this manifests in the SocialSample sample is that the user is not logged in when the page first loads after a log in. If you refresh the page, you are logged in. However, if automatic challenging is on, you'll get into a redirect loop, since the cookie wasn't sent, so the handler thinks it needs to log in again.

@JunTaoLuo
Copy link
Contributor

We obviously need to at least set the samesite attribute to lax to keep things working. The spec for same site does not mention redirects so I'm not sure why the origin of the last document rendered is used to determine whether to send the cookie.

@analogrelay
Copy link
Author

The samesite spec refers specifically to the URL in the browser address bar, which is not updated during redirects: https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-2.1

@analogrelay
Copy link
Author

analogrelay commented May 24, 2017

But yeah, using lax seems to be the best option. This is literally the kind of thing samesite is trying to prevent, sending the cookies when a user gets to your site via a link or redirect.

@blowdart
Copy link
Member

Yup, we had emails about this early in the week, the oauth cookies need to be lax. As does the forms auth ones probably, and maybe even session.

@analogrelay
Copy link
Author

It makes me wonder how valuable strict is if the main authentication cookie (which seems like the thing you'd want to protect) can't use it. I wonder if a CORS-style list of allowed origins would have been better than a simple "strict" behavior. Lax is an improvement enough I suppose, since you couldn't do a cross-site form post.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants