UX: Improve error handling for DiscourseConnect (#26140)

Previously, if the sso= payload was invalid Base64, but signed correctly, there would be no useful log or error. This commit improves things by:

- moving the base64 check before the signature checking so that it's properly surfaced
- split the ParseError exception into PayloadParseError and SignatureError
- add user-facing errors for both of those
- add/improve spec for both

app/controllers/session_controller.rb

@@ -168,13 +168,19 @@ class SessionController < ApplicationController
 
     begin
       sso = DiscourseConnect.parse(request.query_string, secure_session: secure_session)
-    rescue DiscourseConnect::ParseError => e
+    rescue DiscourseConnect::PayloadParseError => e
       connect_verbose_warn do
-        "Verbose SSO log: Signature parse error\n\n#{e.message}\n\n#{sso&.diagnostics}"
+        "Verbose SSO log: Payload is not base64\n\n#{e.message}\n\n#{sso&.diagnostics}"
+      end
+
+      return render_sso_error(text: I18n.t("discourse_connect.payload_parse_error"), status: 422)
+    rescue DiscourseConnect::SignatureError => e
+      connect_verbose_warn do
+        "Verbose SSO log: Signature verification failed\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("discourse_connect.login_error"), status: 422)
+      return render_sso_error(text: I18n.t("discourse_connect.signature_error"), status: 422)
     end
 
     if !sso.nonce_valid?

config/locales/server.en.yml

@@ -2669,6 +2669,8 @@ en:
     missing_secret: "Authentication failed due to missing secret. Contact the site administrators to fix this problem."
     invite_redeem_failed: "Invite redemption failed. Please contact the site's administrator."
     invalid_parameter_value: "Authentication failed due to invalid value for `%{param}` parameter. Contact the site administrators to fix this problem."
+    payload_parse_error: "Authentication failed (payload is not valid Base64). Please contact the site's administrator."
+    signature_error: "Authentication failed (signature incorrect). Please contact the site's administrator."
 
   original_poster: "Original Poster"
   most_recent_poster: "Most Recent Poster"

lib/discourse_connect_base.rb

@@ -4,6 +4,12 @@ class DiscourseConnectBase
   class ParseError < RuntimeError
   end
 
+  class PayloadParseError < ParseError
+  end
+
+  class SignatureError < ParseError
+  end
+
   ACCESSORS = %i[
     add_groups
     admin
@@ -80,19 +86,27 @@ class DiscourseConnectBase
     sso.sso_secret = sso_secret if sso_secret
 
     parsed = Rack::Utils.parse_query(payload)
+
+    raise PayloadParseError.new(<<~MSG) if parsed["sso"] =~ %r{[^a-zA-Z0-9=\r\n/+]}m
+        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.
+        
+        sso: #{parsed["sso"]}
+      MSG
+
     decoded = Base64.decode64(parsed["sso"])
     decoded_hash = Rack::Utils.parse_query(decoded)
 
-    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"] =~ %r{[^a-zA-Z0-9=\r\n/+]}m
-        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 ParseError, "Bad signature for payload #{diags}"
-      end
-    end
+    raise SignatureError, <<~MSG if sso.sign(parsed["sso"]) != parsed["sig"]
+        Bad signature for payload
+
+        sso: #{parsed["sso"]}
+        
+        sig: #{parsed["sig"]}
+        
+        expected sig: #{sso.sign(parsed["sso"])}
+        MSG
 
     ACCESSORS.each do |k|
       val = decoded_hash[k.to_s]

spec/requests/session_controller_spec.rb

@@ -1268,6 +1268,23 @@ RSpec.describe SessionController do
       end
     end
 
+    it "returns the correct error code for invalid payload" do
+      sso = get_sso("/hello/world")
+      sso.external_id = "997"
+      sso.sso_url = "http://somewhere.over.com/sso_login"
+
+      params = Rack::Utils.parse_query(sso.payload)
+      params["sso"] = "#{params["sso"]}%3C"
+      params["sig"] = sso.sign(params["sso"])
+
+      get "/session/sso_login", params: params, headers: headers
+      expect(response.status).to eq(422)
+      expect(response.body).to include(I18n.t("discourse_connect.payload_parse_error"))
+
+      logged_on_user = Discourse.current_user_provider.new(request.env).current_user
+      expect(logged_on_user).to eq(nil)
+    end
+
     it "returns the correct error code for invalid signature" do
       sso = get_sso("/hello/world")
       sso.external_id = "997"
@@ -1278,6 +1295,7 @@ RSpec.describe SessionController do
           params: correct_params.merge(sig: "thisisnotthesigyouarelookingfor"),
           headers: headers
       expect(response.status).to eq(422)
+      expect(response.body).to include(I18n.t("discourse_connect.signature_error"))
       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)