From e1a2d0b932ebca87efbf4cf3dad3b68197336870 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Wed, 26 Aug 2026 17:24:59 +0200 Subject: [PATCH] :bug: Fix nitrate sso failure message (#11214) --- backend/src/app/auth/oidc.clj | 180 ++++++++++++------ backend/test/backend_tests/auth_oidc_test.clj | 136 +++++++++++++ frontend/src/app/main/errors.cljs | 6 + .../test/frontend_tests/main_errors_test.cljs | 47 ++++- 4 files changed, 306 insertions(+), 63 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 164edafd76..9fb3e75bd1 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -42,31 +42,52 @@ ;; OIDC PROVIDER (GENERIC) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +(defn- raise-invalid-sso-config + "Raise a controlled validation error for OIDC provider configuration failures." + [& {:keys [hint cause] :as params}] + (throw (ex-info (or hint "invalid-sso-config") + (-> params + (dissoc :cause) + (assoc :type :validation + :code :invalid-sso-config)) + cause))) + (defn- discover-oidc-config [cfg {:keys [base-uri skip-ssrf-check?] :as provider}] - (let [uri (u/join base-uri ".well-known/openid-configuration") - rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})] + (let [uri (u/join base-uri ".well-known/openid-configuration")] + (try + (let [rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})] + (if (= 200 (:status rsp)) + (let [data (-> rsp :body json/decode) + token-uri (get data :token_endpoint) + auth-uri (get data :authorization_endpoint) + user-uri (get data :userinfo_endpoint) + jwks-uri (get data :jwks_uri) + logout-uri (get data :end_session_endpoint)] - (if (= 200 (:status rsp)) - (let [data (-> rsp :body json/decode) - token-uri (get data :token_endpoint) - auth-uri (get data :authorization_endpoint) - user-uri (get data :userinfo_endpoint) - jwks-uri (get data :jwks_uri) - logout-uri (get data :end_session_endpoint)] + (-> provider + (assoc :token-uri token-uri) + (assoc :auth-uri auth-uri) + (assoc :user-uri user-uri) + (assoc :jwks-uri jwks-uri) + (assoc :logout-uri logout-uri))) - (-> provider - (assoc :token-uri token-uri) - (assoc :auth-uri auth-uri) - (assoc :user-uri user-uri) - (assoc :jwks-uri jwks-uri) - (assoc :logout-uri logout-uri))) - - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "unable to discover OIDC configuration" - :discover-uri uri - :response-status-code (:status rsp))))) + (raise-invalid-sso-config + :hint "unable to discover OIDC configuration" + :discover-uri uri + :response-status-code (:status rsp)))) + (catch Throwable cause + ;; Controlled raises above are ExceptionInfo and would otherwise be + ;; re-wrapped by this catch, dropping fields like :response-status-code. + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + ;; Wrap SSRF blocks, DNS failures, TLS errors, etc. — from the caller's + ;; perspective these are all "bad/unreachable issuer URL". + (raise-invalid-sso-config + :hint "unable to discover OIDC configuration" + :discover-uri uri + :cause cause)))))) (def ^:private default-oidc-scopes #{"openid" "profile" "email"}) @@ -107,16 +128,29 @@ (defn- fetch-oidc-jwks [cfg jwks-uri {:keys [skip-ssrf-check?]}] - (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})] - (if (= 200 status) - (-> body json/decode :keys process-oidc-jwks) - (ex/raise :type ::internal - :code :unable-to-fetch-sso-jwks - :hint "unable to retrieve JWKs (unexpected response status code)" - :response-status-code status)))) + (try + (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})] + (if (= 200 status) + (-> body json/decode :keys process-oidc-jwks) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs (unexpected response status code)" + :jwks-uri jwks-uri + :response-status-code status))) + (catch Throwable cause + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs" + :jwks-uri jwks-uri + :cause cause))))) (defn- populate-jwks - "Fetch and Add (if possible) JWK's to the OIDC provider" + "Fetch and add JWKs to the OIDC provider. + + When `:strict-jwks?` is set (organization SSO), failures raise a controlled + validation error. Otherwise JWKS is best-effort: log and continue without keys + so global OIDC/GitLab providers can still initialize if JWKS is temporarily down." [cfg provider] (try (if-let [jwks (when-let [jwks-uri (:jwks-uri provider)] @@ -124,20 +158,28 @@ (assoc provider :jwks jwks) provider) (catch Throwable cause - (l/warn :hint "unable to fetch JWKs for the OIDC provider" - :provider (str (:id provider)) - :cause cause) - provider))) + (if (:strict-jwks? provider) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs" + :provider (:id provider) + :cause cause)) + (do + (l/warn :hint "unable to fetch JWKs for the OIDC provider" + :provider (str (:id provider)) + :cause cause) + provider))))) (defn- prepare-oidc-provider [cfg params] (when-not (and (string? (:base-uri params)) (string? (:client-id params)) (string? (:client-secret params))) - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "missing params for provider initialization" - :provider (:id params))) + (raise-invalid-sso-config + :hint "missing params for provider initialization" + :provider (:id params))) (try (if (and (string? (:token-uri params)) @@ -150,11 +192,13 @@ (with-meta provider {::discovered true}))) (catch Throwable cause - (ex/raise :type ::internal - :type :invalid-sso-config - :hint "unexpected exception on configuring provider" - :provider (:id params) - :cause cause)))) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unexpected exception on configuring provider" + :provider (:id params) + :cause cause))))) (defmethod ig/assert-key ::providers/generic [_ params] @@ -322,10 +366,9 @@ [cfg params] (when-not (and (string? (:client-id params)) (string? (:client-secret params))) - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "missing params for provider initialization" - :provider (:id params))) + (raise-invalid-sso-config + :hint "missing params for provider initialization" + :provider (:id params))) (try (let [provider (populate-jwks cfg params)] @@ -336,11 +379,13 @@ :client-secret (d/obfuscate-string (:client-secret provider))) provider) (catch Throwable cause - (ex/raise :type ::internal - :type :invalid-sso-config - :hint "unexpected exception on configuring provider" - :provider (:id params) - :cause cause)))) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unexpected exception on configuring provider" + :provider (:id params) + :cause cause))))) (defmethod ig/init-key ::providers/gitlab [_ cfg] @@ -867,7 +912,10 @@ :base-uri (some-> (non-blank-uri issuer) (str/rtrim "/") (str "/")) - :scopes default-oidc-scopes})) + :scopes default-oidc-scopes + ;; Organization SSO is configured by customers; discovery + ;; and JWKS failures must surface as controlled errors. + :strict-jwks? true})) (defn build-organization-sso-auth-redirect-uri "Build the OIDC authorization redirect URI for an organization SSO config. @@ -877,16 +925,24 @@ issuer (organization-sso-discovery-uri sso) dest-url (or dest-url (str (cf/get :public-uri)))] (when-not issuer - (ex/raise :type :validation - :code :invalid-sso-config - :hint "missing issuer")) - (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso)) - state-token (tokens/generate cfg {:iss "oidc" - :dest-url dest-url - :organization-id organization-id - :issuer issuer - :exp (ct/in-future "4h")})] - (build-auth-redirect-uri oidc-provider state-token)))) + (raise-invalid-sso-config + :hint "missing issuer" + :organization-id organization-id)) + (try + (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso)) + state-token (tokens/generate cfg {:iss "oidc" + :dest-url dest-url + :organization-id organization-id + :issuer issuer + :exp (ct/in-future "4h")})] + (build-auth-redirect-uri oidc-provider state-token)) + (catch Throwable cause + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw (ex-info (ex-message cause) + (assoc (ex-data cause) :organization-id organization-id) + (ex-cause cause))) + (throw cause)))))) (def ^:private probe-auth-code "penpot-sso-config-probe") diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index b99de502c4..29469e36d7 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -15,6 +15,7 @@ [app.setup :as-alias setup] [app.tokens :as tokens] [clojure.test :as t] + [cuerdas.core :as str] [mockery.core :refer [with-mocks]] [yetti.response :as-alias yres])) @@ -587,3 +588,138 @@ :issuer "https://idp.example.com"}) (t/is (not (true? (:skip-ssrf-check? @captured-params))) "SSRF protection must be disabled for organization SSO"))))) + +(defn- ssl-handshake-failure + [] + (javax.net.ssl.SSLHandshakeException. "Remote host terminated the handshake")) + +(t/deftest prepare-organization-sso-provider-raises-on-discovery-network-failure + (t/testing "SSL/network failures during OIDC discovery become controlled validation errors" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] (throw (ssl-handshake-failure)))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://wrong-idp.example.com"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))))))) + +(t/deftest prepare-organization-sso-provider-raises-on-discovery-non-200 + (t/testing "non-200 OIDC discovery responses become controlled validation errors" + (with-mocks [http-mock {:target 'app.http.client/req + :return {:status 404 :body "not found"}}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com"}) + (catch Throwable t t)) + data (ex-data e)] + (t/is (ex/error? e)) + (t/is (= :validation (:type data))) + (t/is (= :invalid-sso-config (:code data))) + (t/is (= 404 (:response-status-code data))) + (t/is (= "unable to discover OIDC configuration" (ex-message e))) + (t/is (str/includes? (str (:discover-uri data)) "openid-configuration")))))) + +(t/deftest prepare-organization-sso-provider-raises-on-ssrf-blocked-issuer + (t/testing "SSRF/DNS failures for the issuer URL become invalid-sso-config, not ssrf-blocked-target" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] + (ex/raise :type :validation + :code :ssrf-blocked-target + :hint "uri host could not be resolved"))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://unresolvable.invalid"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e))))))))) + +(t/deftest prepare-organization-sso-provider-raises-on-jwks-network-failure + (t/testing "SSL/network failures while fetching JWKs become controlled validation errors" + (let [discovery-body (str "{\"authorization_endpoint\":\"https://idp.example.com/auth\"," + "\"token_endpoint\":\"https://idp.example.com/token\"," + "\"userinfo_endpoint\":\"https://idp.example.com/userinfo\"," + "\"jwks_uri\":\"https://idp.example.com/jwks\"}")] + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [_cfg request & _] + (if (str/includes? (str (:uri request)) "openid-configuration") + {:status 200 :body discovery-body} + (throw (ssl-handshake-failure))))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e))))))))) + +(t/deftest populate-jwks-strict-wraps-non-invalid-sso-config-errors + (t/testing "strict JWKS path wraps unrelated structured errors instead of rethrowing them" + (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks + :side-effect (fn [& _] + (ex/raise :type :validation + :code :ssrf-blocked-target + :hint "uri host could not be resolved"))}] + (let [e (try + (#'oidc/populate-jwks + {} + {:id "oidc" + :jwks-uri "https://idp.example.com/jwks" + :strict-jwks? true}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e))))))))) + +(t/deftest populate-jwks-strict-rethrows-invalid-sso-config + (t/testing "strict JWKS path rethrows an already-controlled invalid-sso-config" + (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks + :side-effect (fn [& _] + (ex/raise :type :validation + :code :invalid-sso-config + :hint "unable to retrieve JWKs" + :jwks-uri "https://idp.example.com/jwks"))}] + (let [e (try + (#'oidc/populate-jwks + {} + {:id "oidc" + :jwks-uri "https://idp.example.com/jwks" + :strict-jwks? true}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= "unable to retrieve JWKs" (ex-message e))) + (t/is (= "https://idp.example.com/jwks" (:jwks-uri (ex-data e)))))))) + +(t/deftest build-organization-sso-auth-redirect-uri-raises-on-unreachable-provider + (t/testing "check-nitrate-sso path surfaces a controlled error when the issuer is unreachable" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] (throw (ssl-handshake-failure)))}] + (let [e (try + (oidc/build-organization-sso-auth-redirect-uri + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://wrong-idp.example.com"} + :dest-url "https://localhost:3449/#/dashboard" + :organization-id #uuid "00000000-0000-0000-0000-000000000001") + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))))))) diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index f8b5ad9dc4..48aba11881 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -389,6 +389,12 @@ :level :error :timeout 3000}))) + (= code :invalid-sso-config) + ;; SSO error page needs :organization-id to retry + (if (:organization-id error) + (st/async-emit! (rt/assign-exception (assoc error :type :sso-error))) + (st/async-emit! (rt/assign-exception error))) + :else (st/async-emit! (rt/assign-exception error)))) diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index 207b295a11..d35af2f2ca 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -12,7 +12,8 @@ - exception->error-data – pure transformer - on-error re-entrancy guard – prevents recursive invocations - flash schedules async emit – ntf/show is not emitted synchronously - - organization SSO recovery – expired SSO sessions go back to the provider" + - organization SSO recovery – expired SSO sessions go back to the provider + - invalid-sso-config handler – requires :organization-id to promote to :sso-error" (:require [app.main.errors :as errors] [app.main.repo :as rp] @@ -351,3 +352,47 @@ (t/is (nil? @assigned*)) (done')))) done)))) + +;; --------------------------------------------------------------------------- +;; :validation / :invalid-sso-config +;; +;; The SSO error page needs an organization-id to retry meaningfully. Promote +;; to :sso-error only when that id is present; otherwise keep :validation so +;; we do not surface a broken SSO dialog for a future code path that omits it. +;; --------------------------------------------------------------------------- + +(defn- capture-async-exception + "Invoke `ptk/handle-error` while capturing the error map passed to + `rt/assign-exception` via `st/async-emit!`. + + `st/async-emit!` is variadic (`[& params]`); the mock must be too, + otherwise CLJS looks up `IFn$_invoke$arity$variadic` and throws." + [error] + (let [captured (atom nil)] + (with-redefs [st/async-emit! (fn [& events] + (reset! captured (first events))) + rt/assign-exception (fn [err] err)] + (ptk/handle-error error) + @captured))) + +(t/deftest invalid-sso-config-with-organization-id-promotes-to-sso-error + (t/testing "invalid-sso-config with :organization-id is shown as :sso-error" + (let [org-id #uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + assigned (capture-async-exception + {:type :validation + :code :invalid-sso-config + :organization-id org-id + :hint "missing issuer"})] + (t/is (= :sso-error (:type assigned))) + (t/is (= org-id (:organization-id assigned))) + (t/is (= :invalid-sso-config (:code assigned)))))) + +(t/deftest invalid-sso-config-without-organization-id-keeps-validation + (t/testing "invalid-sso-config without :organization-id must not become :sso-error" + (let [assigned (capture-async-exception + {:type :validation + :code :invalid-sso-config + :hint "missing issuer"})] + (t/is (= :validation (:type assigned))) + (t/is (nil? (:organization-id assigned))) + (t/is (= :invalid-sso-config (:code assigned))))))