Merge pull request #18 from AlexanderPavlenko/csrf

Use "state" param to mitigate CSRF

Author: mbleigh

lib/omniauth/strategies/oauth2.rb

@@ -3,6 +3,7 @@
 require 'oauth2'
 require 'omniauth'
 require 'timeout'
+require 'securerandom'
 
 module OmniAuth
   module Strategies
@@ -47,7 +48,16 @@ def request_phase
       end
 
       def authorize_params
-        options.authorize_params.merge(options.authorize_options.inject({}){|h,k| h[k.to_sym] = options[k] if options[k]; h})
+        if options.authorize_params[:state].to_s.empty?
+          options.authorize_params[:state] = SecureRandom.hex(24)
+        end
+        params = options.authorize_params.merge(options.authorize_options.inject({}){|h,k| h[k.to_sym] = options[k] if options[k]; h})
+        if OmniAuth.config.test_mode
+          @env ||= {}
+          @env['rack.session'] ||= {}
+        end
+        session['omniauth.state'] = params[:state]
+        params
       end
 
       def token_params
@@ -58,6 +68,9 @@ def callback_phase
         if request.params['error'] || request.params['error_reason']
           raise CallbackError.new(request.params['error'], request.params['error_description'] || request.params['error_reason'], request.params['error_uri'])
         end
+        if request.params['state'].to_s.empty? || request.params['state'] != session.delete('omniauth.state')
+          raise CallbackError.new(nil, :csrf_detected)
+        end
 
         self.access_token = build_access_token
         self.access_token = access_token.refresh! if access_token.expired?

spec/omniauth/strategies/oauth2_spec.rb

@@ -4,6 +4,14 @@
   def app; lambda{|env| [200, {}, ["Hello."]]} end
   let(:fresh_strategy){ Class.new(OmniAuth::Strategies::OAuth2) }
 
+  before do
+    OmniAuth.config.test_mode = true
+  end
+
+  after do
+    OmniAuth.config.test_mode = false
+  end
+
   describe '#client' do
     subject{ fresh_strategy }
 
@@ -22,13 +30,20 @@ def app; lambda{|env| [200, {}, ["Hello."]]} end
     subject { fresh_strategy }
 
     it 'should include any authorize params passed in the :authorize_params option' do
-      instance = subject.new('abc', 'def', :authorize_params => {:foo => 'bar', :baz => 'zip'})
-      instance.authorize_params.should == {'foo' => 'bar', 'baz' => 'zip'}
+      instance = subject.new('abc', 'def', :authorize_params => {:foo => 'bar', :baz => 'zip', :state => '123'})
+      instance.authorize_params.should == {'foo' => 'bar', 'baz' => 'zip', 'state' => '123'}
     end
 
     it 'should include top-level options that are marked as :authorize_options' do
-      instance = subject.new('abc', 'def', :authorize_options => [:scope, :foo], :scope => 'bar', :foo => 'baz')
-      instance.authorize_params.should == {'scope' => 'bar', 'foo' => 'baz'}
+      instance = subject.new('abc', 'def', :authorize_options => [:scope, :foo], :scope => 'bar', :foo => 'baz', :authorize_params => {:state => '123'})
+      instance.authorize_params.should == {'scope' => 'bar', 'foo' => 'baz', 'state' => '123'}
+    end
+
+    it 'should include random state in the authorize params' do
+      instance = subject.new('abc', 'def')
+      instance.authorize_params.keys.should == ['state']
+      instance.session['omniauth.state'].should_not be_empty
+      instance.session['omniauth.state'].should == instance.authorize_params['state']
     end
   end