After a successful login if I go back in browser I get CSRF detected error

[ristovskiv]

I've build an OAuth2 provider using the doorkeeper engine with devise. On my client app I'm using a custom omniauth-oauth2 -v 1.3.1 strategy. Everything works well, but after I sign in if I hit the back button on the browser I get

I dug into the omniauth-oath2 code and it appears that there are two methods that I think are closely related to this problem, one that creates the session["omniauth.state"] from the url params:

def authorize_params
        options.authorize_params[:state] = SecureRandom.hex(24)
        params = options.authorize_params.merge(options_for("authorize"))
        if OmniAuth.config.test_mode
          @env ||= {}
          @env["rack.session"] ||= {}
        end
        session["omniauth.state"] = params[:state]
        params
end

and one that checks this and deletes this from the session on the callback process:

def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity
        error = request.params["error_reason"] || request.params["error"]
        if error
          fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"]))
        elsif !options.provider_ignores_state && (request.params["state"].to_s.empty? || request.params["state"] != session.delete("omniauth.state"))
          fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
        else
          self.access_token = build_access_token
          self.access_token = access_token.refresh! if access_token.expired?
          super
        end
      rescue ::OAuth2::Error, CallbackError => e
        fail!(:invalid_credentials, e)
      rescue ::Timeout::Error, ::Errno::ETIMEDOUT => e
        fail!(:timeout, e)
      rescue ::SocketError => e
        fail!(:failed_to_connect, e)
  end

It appears that somehow the first method is not called on going back from the browser. Does someone had the same problem and is there a solution to this problem without setting the :provider_ignores_state to true. Do I need to set something on the provider, I really don't get it, plus I can't seem to find this problem anywhere on the web and I highly doubt it I'm the first one that recognizes it, but I may be the first one that can't find a reasonable solution without affecting security.

[gregnavis]

I'm also running into this problem in my project. provider_ignores_state is definitely at most a short-term countermeasure. Another problem it causes is that if your logs are littered with false positive CSRF errors then you'll start ignoring them and will be more likely to miss a real threat.

There's also an old issue #58 that I think is related to this one. Let me explain my thinking.

Background

Let's illustrate the general flow with an example of an app offering Facebook sign in:

1. The app redirects to Facebook and passes state in the URL. state is also saved in the session for later comparison.
2. Facebook authenticates the user and redirects back to the app. It forwards state it received in the URL.
3. The app compares state received from Facebook with the one in the session.

Notice that request_phase (and authorize_params) and callback_phase are called in different request-response cycles. The former is called in step 1. The latter in step 3.

Root Cause

The fact that state is removed from the session triggers the CSRF error in the following way:

1. The app sets state to (say) 1234abcd and passes it to Facebook.
2. After a successful authorization, Facebook forwards 1234abcd back to the app.
3. The app's callback compares the state in the session with the one form Facebook. They match and no error is triggered.
4. The user clicks Back in the browser and is redirected back to the Facebook page responsible for the redirect in step 2.
5. The user lands on the callback (step 3) again. Facebook passes state in the URL but it's been deleted from the session. A CSRF error is triggered.

Solution

The issue could be solved by not removing the state from the session. It should also keep a list of all known states in order to support multiple sign ins (e.g. using multiple providers). The downside is we'll need a way to clean up old states. This could be handled in request_phase just before redirecting to the provider.

A related idea is that the library user should be able to decide how to store states. For example, in my Rails app I'd be able to do something like (ignore the pros and cons of this implementation; that's not the point):

class CookieStore
  def add(state)
    cookies.signed[state] = { value: 'accept', expires: 15.minutes.from_now }
  end

  def has?(state)
    cookies[state]
  end
end

Summary

The issue could be solved (and the gems capabilities expanded) with the following:

• Introduce the concept of a state store with methods to add a new state, ask for an existing state and clear stale states.
• Implement a store that reproduces the current behavior and using it in authorize_params and callback_phase.
• Document how to implement custom stores.
• (Optionally) Extend the default store to support multiple states + per state expiration times.

If you think this is a good plan then I'm happy to submit a PR this week.

[imme5150]

It seems like we're having this issue as well. I'm wondering if we could just not try to log them in w/o a state if they are already logged in?

When they hit back, the site knows that they are already logged in, so it seems like there is nothing left to do. If they don't have a state set we can assume it's a back button and just keep them logged in. We could throw an error if the email was different or something or maybe log a warning.

Is there a newer version of the gem that solves this issue?