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

Can't verify CSRF token authenticity when returning from auth #54

Closed
waissbluth opened this issue Jul 17, 2020 · 36 comments
Closed

Can't verify CSRF token authenticity when returning from auth #54

waissbluth opened this issue Jul 17, 2020 · 36 comments

Comments

@waissbluth
Copy link

waissbluth commented Jul 17, 2020

I am getting 422 Unprocessable Entity errors caused by ActionController::InvalidAuthenticityToken after successful auth redirecting back to the app. I have seen some posts that include the provider_ignores_state: true setting. However, this does not make a difference either (and would potentially cause security issues?).

I am using this as a strategy for Devise, and not directly as Rack middleware, and this is the only auth provider that causes trouble (the other being LinkedIn and Facebook, which work fine).

Is there any reason why this gem would not work as a provider for devise? It seems to be working OK until the redirect step when the CSRF protection causes the problem.

@waissbluth
Copy link
Author

waissbluth commented Jul 17, 2020

@nhosoya i noticed skip_forgery_protection for the callback in omniauth-apple-sample, which changes the error to the following in my case:
ERROR -- omniauth: (apple) Authentication failure! csrf_detected: OmniAuth::Strategies::OAuth2::CallbackError, csrf_detected | CSRF detected

@btalbot
Copy link
Contributor

btalbot commented Jul 17, 2020

Does the client use the full server-side flow by first making a call to /auth/apple (or maybe /users/auth/apple for devise) and then following redirects or do you use a hybrid client-server flow where the client gets the code and then submits once done to the apple callback?

If using the hybrid and skipping the request phase, then you'll need to set provider_ignores_state: true.

If the state value is stored by the client in a cookie, you'll need to ensure that the cookie's Same-Site is set to None (default is often Lax now). It turns out that the callback-phase call made by the apple .js libs seems to set the http Origin and Referer to appleid.apple.com values which prevent cookies from being sent with Same-Site of anything other than None.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

@waissbluth
Copy link
Author

Hi @btalbot, it is a full server-side flow.

I tried changing the cookie to Same-Site: None but it did not help (it was indeed defaulting Lax). I also tried setting provider_ignores_state: true as well just in case, but again it did not make a difference, still getting the same CSRF problem.

@hikaru-w
Copy link

hikaru-w commented Jul 20, 2020

Hello.
I was having trouble with the exact same situation. When I got an error in the processing of the callback, it reproduced.
In my case, after the callback, I tried verifying it with only redirect_to root_path and it worked well, so I will continue to implement it.
I hope this will help you.

@waissbluth
Copy link
Author

Hello @hikaru-w, what do you mean? Are you saying that when you redirect_to root_path it works OK but otherwise it doesn't? What did you need to do to reproduce the error in your environment?

@btalbot I can confirm that changing to Same-Site: None does allow me to disable CSRF checks with skip_forgery_protection and it does work in that case (not even that was working without the Same-Site setting. It is still not ideal though; do you have any ideas as to why this is?

@btalbot
Copy link
Contributor

btalbot commented Jul 21, 2020

No, I have no idea why disabling csrf protections would depend on cookie same-site policy as the cookie same site is only enforced by the browser. Disabling csrf protections would make cookie state irrelevant.

For whatever reason, Sign In With Apple requires the callback to be made using a POST (via response_mode=form_post) and since that is done by javascript loaded from appleid.apple.com, the Origin and Referer headers are both from apple.com domain. This prevents any but Same-Site=None cookies from being sent by a conforming browser.

About the change to the callback suddenly making the provider work, this indicates to me that there is some other error in the callback which is causing the some confusing error message.

Please post the simplest possible callback method that you can get to cause the issue.

It sounds like others are reporting that a simple redirect_to root_path works and if so, the provider is working but there might be a bug in unrelated errors in a callback and incorrectly be reported as a csrf violation.

@waissbluth
Copy link
Author

Hi @btalbot and thanks for the thorough reply. I am able to reproduce the 422 even this with minimal callback controller. This, along with the fact that other providers (e.g. Facebook and LinkedIn) work fine and have essentially identical callback flows to Apple (find or create a user, sign them in) leads me to believe the issue lies somewhere else and is specific to either the Apple flow or somehow caused by this gem. Any other insight is appreciated.

class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController
  def apple
    redirect_to root_path, notice: 'Came back from Apple, did not do anything.'
  end

  def failure
    redirect_to root_path, notice: 'External provider failure.'
  end
end

@waissbluth
Copy link
Author

waissbluth commented Jul 22, 2020

I realized that callbacks are defined as GET|POST:

% rake routes | grep auth
    user_facebook_omniauth_authorize   GET|POST /users/auth/facebook(.:format)            users/omniauth_callbacks#passthru
    user_facebook_omniauth_callback    GET|POST /users/auth/facebook/callback(.:format)   users/omniauth_callbacks#facebook
    user_linkedin_omniauth_authorize   GET|POST /users/auth/linkedin(.:format)            users/omniauth_callbacks#passthru
    user_linkedin_omniauth_callback    GET|POST /users/auth/linkedin/callback(.:format)   users/omniauth_callbacks#linkedin
    user_apple_omniauth_authorize      GET|POST /users/auth/apple(.:format)               users/omniauth_callbacks#passthru
    user_apple_omniauth_callback       GET|POST /users/auth/apple/callback(.:format)      users/omniauth_callbacks#apple

However, while LinkedIn and Facebook send a GET to the callback, Apple performs a POST, which explains why it would behave differently. This essentially means that the app is not enforcing CSRF protection for other providers (or any GET requests, which are supposed not to cause changes (?)). I am not sure whether CSRF protection is a) valuable (forging a request could lead to false user creation?) or b) possible (since one cannot ask 3rd party providers to include an X-CSRF-Token in the header).

@waissbluth
Copy link
Author

Finally found a good reference for what is going on. Closing this issue since the problem is more general to omniauth. However, given that Apple POSTs by default it might be worth mentioning in the README or examples.

@btalbot
Copy link
Contributor

btalbot commented Jul 22, 2020

Yeah, apple doesn't make it easy for some reason. If you have additional CSRF protections enabled, that may be an issue as that middleware will see a POST with no anti-csrf token and not XHR headers. Probably need to disable whatever CSRF middleware you have for the apple callback route in particular.

@yjukaku
Copy link

yjukaku commented Jul 23, 2020

@waissbluth Thanks for sharing your research on this issue. We ran into the same problem, but we already have the omniauth-rails_csrf_protection gem installed and are using button_to/POST in our initial request to Apple. Any ideas why we'd still be getting the CSRF token failure when returning from Apple? It seems like that gem + POST requests should make this problem go away.

@yjukaku
Copy link

yjukaku commented Jul 23, 2020

Ah, looks like that gem doesn't solve the issue of Apple POSTing to the /callback URL, but a different one (it prevents GET requests for the OAuth request phase).

@btalbot
Copy link
Contributor

btalbot commented Jul 23, 2020

The callback POST request is made by apple client using Origin and Referer using appleid.apple.com domains. This means that any cookies being used (for sessions, state, etc) will only be passed to the callback method if the cookie Same-Site=None. The default Same-Site is typically Lax now with modern browsers. If you don't get the session you expect in the callback phase, that is likely the issue.

@yjukaku
Copy link

yjukaku commented Jul 23, 2020

Thanks @btalbot for your answer. So just to clarify, the issue is that we are setting our CSRF token in our session cookie, and that cookie won't be sent to our server when Apple makes a POST request to our site? And the reason is that our session cookie is likely set to use a Same-Site value of Lax, which prevents browsers from sending cookies for cross site requests?

@walterdavis
Copy link

walterdavis commented Jul 23, 2020 via email

@oboxodo
Copy link

oboxodo commented Aug 5, 2021

@btalbot Hi there. FTR, I work with @yjukaku who already posted some comments in this thread last year.

I've been taking a 2nd look into this problem because Rails 6.1 has a new default config for cookies_same_site_protection = :lax and that broke Apple Sign In for us. I think this means many people will be googling for this problem as more teams start adopting that new default.

As mentioned before, Chrome has been using lax as a default for a while already, but that was not breaking Apple Sign In because their default version of lax is "special" and still sends cookies on POST requests as explained in "What is the Lax + POST mitigation?" but only if it was set in the last 2 minutes:

This is a specific exception made to account for existing cookie usage on some Single Sign-On implementations where a CSRF token is expected on a cross-site POST request. This is purely a temporary solution and will be removed in the future. It does not add any new behavior, but instead is just not applying the new SameSite=Lax default in certain scenarios.

Specifically, a cookie that is at most 2 minutes old will be sent on a top-level cross-site POST request. However, if you rely on this behavior, you should update these cookies with the SameSite=None; Secure attributes to ensure they continue to function in the future.

This seems to indicate that the only way to get the Rails' session cookie when Apple makes their POST request is to configure the CookieStore with same_site: :none and force_ssl = true which in turn makes the cookies secure too. But even that is not perfect because many browsers are known to be incompatible with SameSite=none. 😞

But wouldn't that also make the session cookie unusable in http://localhost? This would force all developers to setup an SSL certificate to develop locally and be able to use HTTPS if they want to use a session at all, even regardless of omniauth.

There should be some other way...

Doing a bit more digging, I noticed the CSRF detected error I'm getting comes from this line in omniauth-oauth2. The problem there is that session.delete("omniauth.state")) is not able to read the omniauth.state from the session because the session cookie wasn't part of the POST request.

I can think of 2 possible workarounds and I would like your feedback:

  1. Given this seems to affect omniauth-apple but not other identity providers (because they do GETs), implement a workaround in this gem that would store the omniauth.state in a separate, specific cookie with SameSite=none before taking the user to Apple's servers. Then in the callback phase that cookie would be available and the normal flow would continue. The downside is that the regular session would still not be valid so any kind of detection of the user being already logged in would not work. Not sure how to solve that.
  2. Hijack the callback POST request and "transform" it into a GET to the same callback URL, passing through the params gotten from Apple. Yup... this pretty much defeats Apple's attempt to improve privacy using a POST, but given all other Identity Providers are using GET... it shouldn't be a big deal.

Thoughts?

Given this could potentially require a change in omniauth-oauth2, I think it'd be good if we could get an opinion from @BobbyMcWho.

@btalbot
Copy link
Contributor

btalbot commented Aug 5, 2021

Hi, @oboxodo

It seems like the cleanest work-around would be to include a before_request_phase hook which inspects the request and if using apple, then ensure current session cookie Same-Site is set appropriately for the browser (either 'None' or unset). Then, in the callback phase, reset the Same-Site to your desired value, like 'Lax', as needed. Might be also smart to set the cookie TTL to be shorter than normal as well if you're concerned about Same-Site abuse over a longer time. This pretty much gives the same effect as the chrome work-around you point out above.

@BobbyMcWho
Copy link

It's been omniauth's goal to not add any Rails specific code to the core libraries, so if there's a solution that omniauth-apple or individual app developers can take when specifically using Rails, I'd tend to lean that direction.

@oboxodo
Copy link

oboxodo commented Aug 5, 2021

@btalbot 🤔 that sounds like it might work. but...

if using apple, then ensure current session cookie Same-Site is set appropriately for the browser (either 'None' or unset)

Do you know from the top of your head how I would do that? It seems to me in the context of the before_request_phase block I might have access to omniauth's session which in turn is env["rack.session"]... but I'm not sure how to change the SameSite option in there.

@BobbyMcWho thanks for chiming in. I actually think this is not specific to Rails. The fact that Rails 6.1 starts defaulting all cookies to use SameSite=lax might make this problem more visible, though. I think the problem is that if a rack app stores the session in a cookie with SameSite=lax, which I think is going to be the most desired option, then ANY Identity Provider doing a POST for the callback phase will have trouble. Regardless of Rails. This is why I think this might be an issue for omniauth to tackle. But of course I could be wrong.

@BobbyMcWho
Copy link

That makes sense, if it's something that folks believe needs fixed on the omniauth-oauth2 side, I'd be happy to review a PR in the short term or implement something in the long term, I'd likely need to brush up on the oauth spec again to see if there's something called out explicitly

@btalbot
Copy link
Contributor

btalbot commented Aug 5, 2021

Do you know from the top of your head how I would do that? It seems to me in the context of the before_request_phase block I might have access to omniauth's session which in turn is env["rack.session"]... but I'm not sure how to change the SameSite option in there.

The hook gets the request.env so from that you can check or update the session options as needed:

env['rack.session.options']['same_site'] = 'None'
env['rack.session.options']['expire_after'] = 120

@oboxodo
Copy link

oboxodo commented Aug 6, 2021

Ok so... this seems to work:

# This is a default value in Rails 6.1 which can be loaded implicitly as part of
# `config.load_defaults 6.1` in application.rb or explicitly from new_framework_defaults_6_1.rb.
config.action_dispatch.cookies_same_site_protection = :lax
# config/initializers/omniauth.rb

previous_before_request_phase = OmniAuth.config.before_request_phase
OmniAuth.config.before_request_phase = -> (env) do
  # This is just in case there was something else configured before
  previous_before_request_phase.call(env) if previous_before_request_phase

  if ENV["OMNIAUTH_CHANGES_SAME_SITE_TO_NONE"] == "true"
    # Make sure the session cookie's SameSite option is set to `None` so that when
    # Apple does the callback phase with a POST request, we'll get the session cookie.
    #
    # Ideally, I'd check here if this request is going to Apple and not some other
    # Identity Provider where doing this is not necessary. But I'm not sure how to
    # check for that in here.
    env['rack.session.options']['same_site'] = :none
    env['rack.session.options']['secure'] = true
  end
end

What are the downsides?

  • If the user makes ANY request to the website after initiating the request phase but before the callback request comes from the Identity Provider, then the session cookie will be set to lax. Then when the callback phase is initiated, the cookie won't come included in the POST request. It's an edge case, but could happen. And given Apple's rules about only sending the user's name and email on the very first attempt, this could be problematic.
  • As explained in this article, not all browsers understand SameSite=None appropriately. In order for this "hack" to be more fail-proof, we should determine if setting it to :none or nil (to let the browser use its default behavior) according to the exceptions outlined in that article's pseudocode. Luckily, rails_same_site_cookie gem seems to have that code almost ready to use. But still feels hacky.

I decided to take a look at what are other Rails-based websites doing with their session cookies and these are my findings:

  • Basecamp, Hey: They're not setting SameSite yet, letting the browser decide. Supposedly, if they continue with this decision long enough, it'd be the same as setting SameSite=Lax. But, for now, chrome still includes these cookies on POST requests.
  • Cookpad, GitHub: they set SameSite=Lax explicitly.
  • Shopify: I couldn't figure it out.

I'm still unsure what we'll do. Most probably we'll avoid setting SameSite explicitly for now... but at some point explicit Lax seems to be the best choice.

In any case, I do consider omniauth should potentially try to handle this stuff off-the-shelf. This is certainly NOT Rails-specific. I think the best option could be for omniauth to make itself independent from the session and use a specific cookie of its own to store its intermediate state. This cookie would need to have SameSite=None, but also have into account the browsers that don't handle that well.

Thank you for your pointers @btalbot!
Thanks for hearing @BobbyMcWho!

@BobbyMcWho
Copy link

One thing to note is that if you're setting previous_before_request_phase in the initializer, it'll be static for every user as whatever it was at app startup

@oboxodo
Copy link

oboxodo commented Aug 6, 2021

@BobbyMcWho do you mean that code will get executed only once at app initialization? I think the proc itself will get executed each time someone clicks on a "sign up with X " link/button in the app to initiate a request phase, right?

oboxodo added a commit to oboxodo/rails that referenced this issue Aug 6, 2021
I want to embrace Rails 6.1's new `config.action_dispatch.cookies_same_site_protection = :lax` default value for _most_ of our cookies, but I'd like our session cookie (using the `CookieStore`) to still avoid setting any `SameSite` option for now. The main reason having to do with Apple Sign In and Omniauth not playing nice with `Lax` (because Apple uses POST instead of GET) but you can read more about it [here](nhosoya/omniauth-apple#54 (comment)) if you're interested interested.

The way I was trying to do this was with the following settings:

```ruby
# config/application.rb
# This is not needed explicitly in Rails 6.1+ because it's the default but it's here to help with the explanation.
config.action_dispatch.cookies_same_site_protection = :lax
# Specifically for the session cookie, avoid setting any explicit `SameSite` value letting the browser use its default.
config.session_store :cookie_store, key: "_myapp_session", same_site: nil
```

**But that didn't work** and `lax` was applied even for the session cookie. With this patch, I could use `same_site: :nil` and it'd work.

Regardless of why I need it, I do think it's a useful option and the current code doesn't seem to allow avoiding to set ANY `SameSite` value on a cookie by cookie bases, once a default has been set. It's either all or nothing.

I think with this little change I would be able to still configure a default to be used whenever I don't set `:same_site` explicitly, but I can use the "special" `:nil` value if I want to make sure that cookie doesn't set any explicit value regardless of the configured default. I'm not a fan of the `:nil` special value but couldn't think of anything better than that.

I guess another possibility would be to allow the `cookies_same_site_protection` proc to read the cookie name (we'd need to pass `options` on top of the `request`, I guess) so that the dev can decide to use a different default value depending on the cookie name. but this would be a bigger change, potentially not backward-compatible.

I assume you'd need an accompanying automated test in order to merge this but I wanted to get some opinions about this before dedicating more time on it.
@theblang
Copy link

theblang commented Dec 2, 2021

@oboxodo Thanks so much for your very helpful notes! Also, thanks so much @dlin-me for doing the legwork to create this utility that's based on the Chromium incompatible browsers list.

For anyone who finds this, we decided to make the session cookie SameSite=None; Secure and just hide Apple OAuth for those older incompatible browsers.

@ianrandmckenzie
Copy link

ianrandmckenzie commented Dec 7, 2021

# config/initializers/omniauth.rb

previous_before_request_phase = OmniAuth.config.before_request_phase
OmniAuth.config.before_request_phase = -> (env) do
  # This is just in case there was something else configured before
  previous_before_request_phase.call(env) if previous_before_request_phase

  if ENV["OMNIAUTH_CHANGES_SAME_SITE_TO_NONE"] == "true"
    # Make sure the session cookie's SameSite option is set to `None` so that when
    # Apple does the callback phase with a POST request, we'll get the session cookie.
    #
    # Ideally, I'd check here if this request is going to Apple and not some other
    # Identity Provider where doing this is not necessary. But I'm not sure how to
    # check for that in here.
    env['rack.session.options']['same_site'] = :none
    env['rack.session.options']['secure'] = true
  end
end

Hey, thanks for this. I made a small adjustment to verify that the Apple callback path is being used under these conditions. My app seems to get a lot of 'volunteer' penetration testers so if someone could exploit my Discord callback being open (my other strategy), I'm sure they would. Heh

# config/initializers/omniauth.rb

previous_before_request_phase = OmniAuth.config.before_request_phase
OmniAuth.config.before_request_phase = -> (env) do
  # This is just in case there was something else configured before
  previous_before_request_phase.call(env) if previous_before_request_phase

  if ENV["OMNIAUTH_CHANGES_SAME_SITE_TO_NONE"] == "true" && env['REQUEST_PATH'] == '/users/auth/apple'
    # Make sure the session cookie's SameSite option is set to `None` so that when
    # Apple does the callback phase with a POST request, we'll get the session cookie.
    #
    # Ideally, I'd check here if this request is going to Apple and not some other
    # Identity Provider where doing this is not necessary. But I'm not sure how to
    # check for that in here.
    env['rack.session.options']['same_site'] = 'None'
    env['rack.session.options']['secure'] = true
  end
end

@jmonteiro
Copy link

jmonteiro commented Dec 2, 2022

An alternative approach, at least possible on Rails 7, is adding the following to the config/application.rb file, placing this inside your Application class:

config.action_dispatch.cookies_same_site_protection = lambda { |request|
  if request.path.starts_with?("/auth/apple")
    :none
  else
    :lax
  end
}

@dcrec1
Copy link

dcrec1 commented Jan 31, 2023

I fixed this with the following codes:

In the Omniauth callbacks controller:

private
  
def verified_request?
  action_name == 'apple' || super
end

and in application.rb:

config.action_dispatch.cookies_same_site_protection = lambda { |request|
  request.path == '/users/auth/apple' ? :none : :lax
}

@antulik
Copy link

antulik commented Jul 22, 2023

As mentioned above if there is another request it will set session cookie back to :lax, this is a fix to keep it for 2 minutes in Rails 7

# config/application.rb
config.action_dispatch.cookies_same_site_protection = lambda { |request|
  cookies = request.env["action_dispatch.cookies"]
  if request.path.starts_with?("/auth/apple", "/users/auth/apple")
    cookies[:apple_signin] = { same_site: :none, expires: 2.minutes.from_now, secure: true,
      value: "true", http_only: true }
  end

  cookies.key?(:apple_signin) ? :none : :lax
}

@HalfBloody
Copy link

Could it be that this leads to not being able to set a cookie in auth/apple/callback?

I just implemented the workaround of @antulik (and @oboxodo) and I don't get an error but when after creating the user and setting the user cookie with cookies.encrypted[:session_id]. Then I redirect to the root page and there is no cookie. The other auth method works fine. That leads me to believe that it must the Apple cookie issue.

Is there a way to set the cookie in the callbacks controller, like setting cookie policy back to :lax? Is there any workaround for this? Like downgrading the gem?

@boyfunky
Copy link

I am still facing issues with this as well. I get the error

OAuth2::AccessToken.from_hash: `hash` contained more than one 'token' key (["access_token", "id_token"]); using "access_token".
(apple) Authentication failure! invalid_credentials: OmniAuth::Strategies::OAuth2::CallbackError, id_token_claims_invalid | nonce invalid

Anyone knows how to resolve this issue?

@slim1979
Copy link

I am still facing issues with this as well. I get the error

OAuth2::AccessToken.from_hash: `hash` contained more than one 'token' key (["access_token", "id_token"]); using "access_token".
(apple) Authentication failure! invalid_credentials: OmniAuth::Strategies::OAuth2::CallbackError, id_token_claims_invalid | nonce invalid

Anyone knows how to resolve this issue?

this worked for me

@nomasprime
Copy link

nomasprime commented Jan 17, 2024

Tried all solutions here and none have worked so far.

Seems like gem's currently unmaintained and broken.

@jtomchak
Copy link

Tried all solutions here and none have worked so far.

Seems like gem's currently unmaintained and broken.

This worked on Rails 7.1 for me #54 (comment)

@BobbyMcWho
Copy link

PRs are welcome

@jtomchak
Copy link

Works as designed on this end. Thanks 🙌

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

No branches or pull requests