FIX: Return 422 instead of 500 for invalid SSO signature (#6738)

app/controllers/admin/users_controller.rb

@@ -444,7 +444,11 @@ class Admin::UsersController < Admin::AdminController
   def sync_sso
     return render body: nil, status: 404 unless SiteSetting.enable_sso
 
-    sso = DiscourseSingleSignOn.parse("sso=#{params[:sso]}&sig=#{params[:sig]}")
+    begin
+      sso = DiscourseSingleSignOn.parse("sso=#{params[:sso]}&sig=#{params[:sig]}")
+    rescue DiscourseSingleSignOn::ParseError => e
+      return render json: failed_json.merge(message: I18n.t("sso.login_error")), status: 422
+    end
 
     begin
       user = sso.lookup_or_create_user

app/controllers/session_controller.rb

@@ -111,7 +111,17 @@ class SessionController < ApplicationController
     params.require(:sso)
     params.require(:sig)
 
-    sso = DiscourseSingleSignOn.parse(request.query_string)
+    begin
+      sso = DiscourseSingleSignOn.parse(request.query_string)
+    rescue DiscourseSingleSignOn::ParseError => e
+      if SiteSetting.verbose_sso_logging
+        Rails.logger.warn("Verbose SSO log: Signature parse error\n\n#{e.message}\n\n#{sso.diagnostics}")
+      end
+
+      # Do NOT pass the error text to the client, it would give them the correct signature
+      return render_sso_error(text: I18n.t("sso.login_error"), status: 422)
+    end
+
     if !sso.nonce_valid?
       if SiteSetting.verbose_sso_logging
         Rails.logger.warn("Verbose SSO log: Nonce has already expired\n\n#{sso.diagnostics}")

lib/single_sign_on.rb

@@ -1,5 +1,7 @@
 class SingleSignOn
 
+  class ParseError < RuntimeError; end
+
   ACCESSORS = %i{
     add_groups
     admin moderator
@@ -61,9 +63,9 @@ class SingleSignOn
     if sso.sign(parsed["sso"]) != parsed["sig"]
       diags = "\n\nsso: #{parsed["sso"]}\n\nsig: #{parsed["sig"]}\n\nexpected sig: #{sso.sign(parsed["sso"])}"
       if parsed["sso"] =~ /[^a-zA-Z0-9=\r\n\/+]/m
-        raise RuntimeError, "The SSO field should be Base64 encoded, using only A-Z, a-z, 0-9, +, /, and = characters. Your input contains characters we don't understand as Base64, see http://en.wikipedia.org/wiki/Base64 #{diags}"
+        raise ParseError, "The SSO field should be Base64 encoded, using only A-Z, a-z, 0-9, +, /, and = characters. Your input contains characters we don't understand as Base64, see http://en.wikipedia.org/wiki/Base64 #{diags}"
       else
-        raise RuntimeError, "Bad signature for payload #{diags}"
+        raise ParseError, "Bad signature for payload #{diags}"
       end
     end
 

spec/requests/admin/users_controller_spec.rb

@@ -846,6 +846,19 @@ RSpec.describe Admin::UsersController do
       expect(response.status).to eq(403)
       expect(JSON.parse(response.body)["message"]).to include("Primary email can't be blank")
     end
+
+    it 'should return the right message if the signature is invalid' do
+      sso.name = "Dr. Claw"
+      sso.username = "dr_claw"
+      sso.email = "dr@claw.com"
+      sso.external_id = "2"
+
+      correct_payload = Rack::Utils.parse_query(sso.payload)
+      post "/admin/users/sync_sso.json", params: correct_payload.merge(sig: "someincorrectsignature")
+      expect(response.status).to eq(422)
+      expect(JSON.parse(response.body)["message"]).to include(I18n.t('sso.login_error'))
+      expect(JSON.parse(response.body)["message"]).not_to include(correct_payload["sig"])
+    end
   end
 
   describe '#disable_second_factor' do

spec/requests/session_controller_spec.rb

@@ -557,6 +557,26 @@ RSpec.describe SessionController do
       expect(response.status).to eq(419)
     end
 
+    it 'returns the correct error code for invalid signature' do
+      sso = get_sso('/hello/world')
+      sso.external_id = '997'
+      sso.sso_url = "http://somewhere.over.com/sso_login"
+
+      correct_params = Rack::Utils.parse_query(sso.payload)
+      get "/session/sso_login", params: correct_params.merge("sig": "thisisnotthesigyouarelookingfor"), headers: headers
+      expect(response.status).to eq(422)
+      expect(response.body).not_to include(correct_params["sig"]) # Check we didn't send the real sig back to the client
+      logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+      expect(logged_on_user).to eq(nil)
+
+      correct_params = Rack::Utils.parse_query(sso.payload)
+      get "/session/sso_login", params: correct_params.merge("sig": "thisisasignaturewith@special!characters"), headers: headers
+      expect(response.status).to eq(422)
+      expect(response.body).not_to include(correct_params["sig"]) # Check we didn't send the real sig back to the client
+      logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+      expect(logged_on_user).to eq(nil)
+    end
+
     describe 'local attribute override from SSO payload' do
       before do
         SiteSetting.email_editable = false