Persist the 'state' parameter until a successful callback.

We discovered the following issue while using [omniauth-facebook][1]. We
are seeing an unexpectedly high number of CSRF violations (roughly 6%
of sign-ins) when users return from facebook.com.

Every time the user visits `/auth/facebook` and invokes the
`request_phase` method, a new `state` value is generated and put in the
session. If a user opens the site in two tabs and navigates to
`/auth/facebook` in each, then they have two copies of
https://www.facebook.com/dialog/oauth open, each with a different
`state` param. But, only one of these state params is in the session
under the `omniauth.state` key.

One of these pages will thus have a `state` param that does not match
the session. It is also possible to trigger this with concurrent or
duplicate requests, or by using the back/forward buttons, causing a race
condition in updating the session.

The page with the `state` value that "lost" the race will authorize
Facebook's permissions, but then fail when redirected to
`/auth/facebook/callback` due to the token mismatch.

I wondered why this gem always assigns a new `state` value on visiting
`/auth/:provider`, compared to how [rack-protection][2] and [Rails][3]
implement CSRF protection using a guarded assignment. My assumption in
this patch is that this is to avoid sending a value to one provider that
could be redeemed by another. If I send a state value to alice.com, then
someone working for alice.com could take that value and the account of
the user that authenticated there, then email the user a phishing
callback link for bob.com, including a `state` value that will work.

To avoid this, I have introduced a namespace into the session: each
`state` value is keyed by the class name of the particular provider, so
you can have a different state value per provider, but have them be
reusable. The value is still removed from the session on completion to
avoid replay attacks.

Keeping the `state` constant until the auth process completes means it's
safe to open multiple tabs and perform concurrent requests, or anything
else that might leave the session in an inconsistent or unpredictable
state.

[1]: https://github.com/mkdynamic/omniauth-facebook
[2]: https://github.com/sinatra/rack-protection/blob/v1.5.3/lib/rack/protection/authenticity_token.rb#L24
[3]: https://github.com/rails/rails/blob/v4.2.3/actionpack/lib/action_controller/metal/request_forgery_protection.rb#L316

lib/omniauth/strategies/oauth2.rb

@@ -53,14 +53,13 @@ def request_phase
       end
 
       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"] ||= {}
+        site = self.class.name
+        state = state_store.fetch(site) do
+          state_store[site] = SecureRandom.hex(24)
         end
-        session["omniauth.state"] = params[:state]
-        params
+
+        options.authorize_params[:state] = state
+        options.authorize_params.merge(options_for("authorize"))
       end
 
       def token_params
@@ -69,11 +68,18 @@ def token_params
 
       def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity
         error = request.params["error_reason"] || request.params["error"]
+        site = self.class.name
+        expected_state = state_store[site]
+        actual_state = request.params["state"].to_s
+
         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"))
+          description = request.params["error_description"] || request.params["error_reason"]
+          error_uri = request.params["error_uri"]
+          fail!(error, CallbackError.new(request.params["error"], description, error_uri))
+        elsif !options.provider_ignores_state && (actual_state.empty? || actual_state != expected_state)
           fail!(:csrf_detected, CallbackError.new(:csrf_detected, "CSRF detected"))
         else
+          state_store.delete(site)
           self.access_token = build_access_token
           self.access_token = access_token.refresh! if access_token.expired?
           super
@@ -109,6 +115,22 @@ def options_for(option)
         hash
       end
 
+    private
+
+      def state_store
+        if OmniAuth.config.test_mode
+          @env ||= {}
+          @env["rack.session"] ||= {}
+        end
+
+        state_key = "omniauth.oauth2.state"
+        state_store = session[state_key]
+        unless state_store.is_a?(Hash)
+          state_store = session[state_key] = {}
+        end
+        state_store
+      end
+
       # An error that is indicated in the OAuth 2.0 callback.
       # This could be a `redirect_uri_mismatch` or other
       class CallbackError < StandardError

spec/omniauth/strategies/oauth2_spec.rb

@@ -53,11 +53,49 @@ def app
       expect(instance.authorize_params["scope"]).to eq("bar")
       expect(instance.authorize_params["foo"]).to eq("baz")
     end
+  end
 
-    it "includes random state in the authorize params" do
-      instance = subject.new("abc", "def")
-      expect(instance.authorize_params.keys).to eq(["state"])
-      expect(instance.session["omniauth.state"]).not_to be_empty
+  describe "state handling" do
+    SocialNetwork = Class.new(OmniAuth::Strategies::OAuth2)
+
+    let(:client_options) { {:site => "https://graph.example.com"} }
+    let(:instance) { SocialNetwork.new(-> env {}) }
+
+    before do
+      allow(SecureRandom).to receive(:hex).with(24).and_return("hex-1", "hex-2")
+    end
+
+    it "includes a state scoped to the client" do
+      expect(instance.authorize_params["state"]).to eq("hex-1")
+      expect(instance.session["omniauth.oauth2.state"]).to eq("SocialNetwork" => "hex-1")
+    end
+
+    context "once a state value has been generated" do
+      before do
+        instance.authorize_params
+      end
+
+      it "does not replace an existing session value" do
+        expect(instance.authorize_params["state"]).to eq("hex-1")
+        expect(instance.session["omniauth.oauth2.state"]).to eq("SocialNetwork" => "hex-1")
+      end
+    end
+
+    context "on a successful callback" do
+      let(:request) { double("Request", :params => {"code" => "auth-code", "state" => "hex-1"}) }
+      let(:access_token) { double("AccessToken", :expired? => false, :expires? => false, :token => "access-token") }
+
+      before do
+        allow(instance).to receive(:request).and_return(request)
+        allow(instance).to receive(:build_access_token).and_return(access_token)
+
+        instance.authorize_params
+        instance.callback_phase
+      end
+
+      it "removes the value from the session" do
+        expect(instance.session["omniauth.oauth2.state"]).to eq({})
+      end
     end
   end