FIX: Better error when SSO fails due to blank secret (#7946)

FIX: Better error when SSO fails due to blank secret (#7946)

  • FIX: Better error when SSO fails due to blank secret

  • Update spec/requests/session_controller_spec.rb

Co-Authored-By: Robin Ward robin.ward@gmail.com

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index 1ca3f8a..4acc0b2 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -49,7 +49,12 @@ class SessionController < ApplicationController
     payload ||= request.query_string
 
     if SiteSetting.enable_sso_provider
-      sso = SingleSignOnProvider.parse(payload)
+      begin
+        sso = SingleSignOnProvider.parse(payload)
+      rescue SingleSignOnProvider::BlankSecret
+        render plain: I18n.t("sso.missing_secret"), status: 400
+        return
+      end
 
       if sso.return_sso_url.blank?
         render plain: "return_sso_url is blank, it must be provided", status: 400
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index fe69780..6dc6bd9 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -2120,6 +2120,7 @@ en:
     timeout_expired: "Account login timed out, please try logging in again."
     no_email: "No email address was provided. Please contact the site's administrator."
     email_error: "An account could not be registered with the email address <b>%{email}</b>. Please contact the site's administrator."
+    missing_secret: "SSO authentication failed due to missing secret. Contact the site administrators to fix this problem."
 
   original_poster: "Original Poster"
   most_posts: "Most Posts"
diff --git a/lib/single_sign_on_provider.rb b/lib/single_sign_on_provider.rb
index c551187..774c5ee 100644
--- a/lib/single_sign_on_provider.rb
+++ b/lib/single_sign_on_provider.rb
@@ -3,9 +3,15 @@
 require_dependency 'single_sign_on'
 
 class SingleSignOnProvider < SingleSignOn
+  class BlankSecret < RuntimeError; end
 
   def self.parse(payload, sso_secret = nil)
     set_return_sso_url(payload)
+    if sso_secret.blank? && self.sso_secret.blank?
+      host = URI.parse(@return_sso_url).host
+      Rails.logger.warn("SSO failed; website #{host} is not in the `sso_provider_secrets` site settings")
+      raise BlankSecret
+    end
 
     super
   end
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index f3491ef..8e5ec95 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -816,6 +816,16 @@ RSpec.describe SessionController do
         expect(response.status).to eq(500)
       end
 
+      it "fails with a nice error message if secret is blank" do
+        SiteSetting.sso_provider_secrets = ""
+        sso = SingleSignOnProvider.new
+        sso.nonce = "mynonce"
+        sso.return_sso_url = "http://website.without.secret.com/sso"
+        get "/session/sso_provider", params: Rack::Utils.parse_query(sso.payload("aasdasdasd"))
+        expect(response.status).to eq(400)
+        expect(response.body).to eq(I18n.t("sso.missing_secret"))
+      end
+
       it "successfully redirects user to return_sso_url when the user is logged in" do
         sign_in(@user)

GitHub sha: 525920a9

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there:

This seems to be a bug.

sso_secret is nil for sure :joy:

Yes the sso_secret parameter is nil, but the if statement condition has && self.sso_secret.blank? which calls a method in the same file that looks at the sso_provider_secrets site setting and returns the secret associated with the host of @return_sso_url. If the method returns nil AND the sso_secret parameter is nil, then the BlankSecret error is raised.