From 6548e01f18c24ec8703bb85557d7509dbeace013 Mon Sep 17 00:00:00 2001
From: James Edwards-Jones <jedwardsjones@gitlab.com>
Date: Sat, 19 Jan 2019 20:41:39 +0000
Subject: [PATCH] Avoid CSRF check on SAML failure endpoint

SAML and OAuth failures should cause a message to be presented, as well
as logging that an attempt was made. These were incorrectly prevented by
the CSRF check on POST endpoints such as SAML.

In addition we were using a NullSession forgery protection, which made
testing more difficult and could have allowed account linking to take
place if a CSRF was ever needed but not present.
---
 .../omniauth_callbacks_controller.rb          |  2 +-
 .../jej-avoid-csrf-check-on-saml-failure.yml  |  5 ++++
 .../omniauth_callbacks_controller_spec.rb     | 23 +++++++++++++++++++
 3 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 changelogs/unreleased/jej-avoid-csrf-check-on-saml-failure.yml

diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index f8e482937d5..97120273d6b 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -4,7 +4,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   include AuthenticatesWithTwoFactor
   include Devise::Controllers::Rememberable
 
-  protect_from_forgery except: [:kerberos, :saml, :cas3], prepend: true
+  protect_from_forgery except: [:kerberos, :saml, :cas3, :failure], with: :exception, prepend: true
 
   def handle_omniauth
     omniauth_flow(Gitlab::Auth::OAuth)
diff --git a/changelogs/unreleased/jej-avoid-csrf-check-on-saml-failure.yml b/changelogs/unreleased/jej-avoid-csrf-check-on-saml-failure.yml
new file mode 100644
index 00000000000..18cced2906a
--- /dev/null
+++ b/changelogs/unreleased/jej-avoid-csrf-check-on-saml-failure.yml
@@ -0,0 +1,5 @@
+---
+title: Display SAML failure messages instead of expecting CSRF token
+merge_request: 24509
+author:
+type: fixed
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb
index 59463462e5a..232a5e2793b 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
@@ -45,6 +45,29 @@ describe OmniauthCallbacksController, type: :controller do
       end
     end
 
+    context 'when sign in fails' do
+      include RoutesHelpers
+
+      let(:extern_uid) { 'my-uid' }
+      let(:provider) { :saml }
+
+      def stub_route_as(path)
+        allow(@routes).to receive(:generate_extras) { [path, []] }
+      end
+
+      it 'it calls through to the failure handler' do
+        request.env['omniauth.error'] = OneLogin::RubySaml::ValidationError.new("Fingerprint mismatch")
+        request.env['omniauth.error.strategy'] = OmniAuth::Strategies::SAML.new(nil)
+        stub_route_as('/users/auth/saml/callback')
+
+        ForgeryProtection.with_forgery_protection do
+          post :failure
+        end
+
+        expect(flash[:alert]).to match(/Fingerprint mismatch/)
+      end
+    end
+
     context 'when a redirect fragment is provided' do
       let(:provider) { :jwt }
       let(:extern_uid) { 'my-uid' }