Pass mismatched state error through to the app

* Allows app to treat this case as a failed sign-in * Generic `invalid_request` error in flash[:google_sign_in_error] rather returning 422 Unprocessable Entity directly
basecamp · Mar 6, 2019 · f26af12 · f26af12
1 parent d404fe0
commit f26af12
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 14 deletions.
diff --git a/app/controllers/google_sign_in/callbacks_controller.rb b/app/controllers/google_sign_in/callbacks_controller.rb
@@ -2,33 +2,29 @@
 
 class GoogleSignIn::CallbacksController < GoogleSignIn::BaseController
   def show
-    if valid_request?
-      redirect_to proceed_to_url, flash: google_sign_in_response
-    else
-      head :unprocessable_entity
-    end
+    redirect_to proceed_to_url, flash: google_sign_in_response
   rescue GoogleSignIn::RedirectProtector::Violation => error
     logger.error error.message
     head :bad_request
   end
 
   private
-    def valid_request?
-      flash[:state].present? && params[:state] == flash[:state]
-    end
-
     def proceed_to_url
       flash[:proceed_to].tap { |url| GoogleSignIn::RedirectProtector.ensure_same_origin(url, request.url) }
     end
 
     def google_sign_in_response
-      if params[:code].present?
+      if valid_request? && params[:code].present?
         { google_sign_in_token: id_token }
       else
         { google_sign_in_error: error_message }
       end
     end
 
+    def valid_request?
+      flash[:state].present? && params[:state] == flash[:state]
+    end
+
     def id_token
       client.auth_code.get_token(params[:code])['id_token']
     end

diff --git a/lib/google_sign_in.rb b/lib/google_sign_in.rb
@@ -5,7 +5,7 @@ module GoogleSignIn
   mattr_accessor :client_id
   mattr_accessor :client_secret
 
-  # https://tools.ietf.org/html/rfc6749#section-4.1.2.1
+  # Authorization Code Grant errors: https://tools.ietf.org/html/rfc6749#section-4.1.2.1
   OAUTH2_ERRORS = %w[
     invalid_request
     unauthorized_client

diff --git a/test/controllers/callbacks_controller_test.rb b/test/controllers/callbacks_controller_test.rb
@@ -46,8 +46,13 @@ class GoogleSignIn::CallbacksControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "protecting against CSRF without flash state" do
+    post google_sign_in.authorization_url, params: { proceed_to: 'http://www.example.com/login' }
+    assert_response :redirect
+
     get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: 'invalid')
-    assert_response :unprocessable_entity
+    assert_redirected_to 'http://www.example.com/login'
+    assert_nil flash[:google_sign_in_token]
+    assert_equal 'invalid_request', flash[:google_sign_in_error]
   end
 
   test "protecting against CSRF with invalid state" do
@@ -56,7 +61,9 @@ class GoogleSignIn::CallbacksControllerTest < ActionDispatch::IntegrationTest
     assert_not_nil flash[:state]
 
     get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy', state: 'invalid')
-    assert_response :unprocessable_entity
+    assert_redirected_to 'http://www.example.com/login'
+    assert_nil flash[:google_sign_in_token]
+    assert_equal 'invalid_request', flash[:google_sign_in_error]
   end
 
   test "protecting against CSRF with missing state" do
@@ -65,7 +72,9 @@ class GoogleSignIn::CallbacksControllerTest < ActionDispatch::IntegrationTest
     assert_not_nil flash[:state]
 
     get google_sign_in.callback_url(code: '4/SgCpHSVW5-Cy')
-    assert_response :unprocessable_entity
+    assert_redirected_to 'http://www.example.com/login'
+    assert_nil flash[:google_sign_in_token]
+    assert_equal 'invalid_request', flash[:google_sign_in_error]
   end
 
   test "protecting against open redirects" do