From 127214c61353a6528f947c951e60778a3281013c Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 12 Mar 2024 16:16:04 +0000 Subject: [PATCH] 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 | 12 ++++++--- config/locales/server.en.yml | 2 ++ lib/discourse_connect_base.rb | 34 +++++++++++++++++------- spec/requests/session_controller_spec.rb | 18 +++++++++++++ 4 files changed, 53 insertions(+), 13 deletions(-) diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 05afd6c2cf9..ac87a172dec 100644 --- a/app/controllers/session_controller.rb +++ b/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? diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index dac1b83cdbd..57faea0eb46 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/lib/discourse_connect_base.rb b/lib/discourse_connect_base.rb index 4cd51f6a327..6797d5dc35b 100644 --- a/lib/discourse_connect_base.rb +++ b/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] diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index f0393f9e748..d8f0e9d4f24 100644 --- a/spec/requests/session_controller_spec.rb +++ b/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)