diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 2941afbca5..1a309c1d5e 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -1068,24 +1068,27 @@ :organization-id organization-id :organization-name organization-name})))))) -(defn- trusted-origin-from-state - "Best-effort extraction of the trusted alternate origin carried by the - OIDC state token. Returns nil when the token is missing or invalid." - [cfg state-token] +(defn- parse-state + "Verifies the OIDC state token once. Returns `[state failure]` where + exactly one of the two is non-nil: the decoded claims on success, or the + verification error when the token is missing, invalid or expired." + [cfg params] (try - (:origin (tokens/verify cfg {:token state-token :iss "oidc"})) - (catch Throwable _ nil))) + [(tokens/verify cfg {:token (:state params) :iss "oidc"}) nil] + (catch Throwable cause + [nil cause]))) (defn- handle-oidc-callback - [cfg {:keys [params] :as request}] + [cfg {:keys [params] :as request} state failure] (if-let [error (get params :error)] (do (submit-organization-sso-oauth-failed-event cfg request (:state params) error) (redirect-with-error "unable-to-auth" error)) (try - (let [code (get params :code) - state (get params :state) - state (tokens/verify cfg {:token state :iss "oidc"})] + (when (some? failure) + (throw failure)) + + (let [code (get params :code)] ;; Organization SSO flow: state carries :dest-url — exchange the authorization ;; code with the OIDC provider to verify authentication actually occurred. @@ -1163,8 +1166,9 @@ state for the whole callback (token exchange redirect_uri and every redirect target)." [cfg {:keys [params] :as request}] - (binding [cf/config (cf/with-public-uri (trusted-origin-from-state cfg (:state params)) cf/config)] - (handle-oidc-callback cfg request))) + (let [[state failure] (parse-state cfg params)] + (binding [cf/config (cf/with-public-uri (:origin state) cf/config)] + (handle-oidc-callback cfg request state failure)))) (def ^:private schema:routes-params [:map diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 364247bb06..86d1f53ef2 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -437,6 +437,17 @@ [s] (some-> s str/trim str/lower not-empty)) +(def ^:private origin-re + #"(?i)^[a-z][a-z0-9+.\-]*://[^/?#\s]+$") + +(defn valid-origin? + "Returns true when `s` is a bare origin: `scheme://host[:port]` with no + path, query, fragment or trailing slash. Used to warn about malformed + :trusted-origins entries; matching stays fail-closed regardless." + [s] + (let [s (normalize-origin s)] + (boolean (and (some? s) (re-matches origin-re s))))) + (defn trusted-origin? "Returns true when `origin` matches one of the configured :trusted-origins. Matching is exact on the normalized origin (trimmed diff --git a/backend/src/app/http.clj b/backend/src/app/http.clj index 80c93d3964..a7be30b087 100644 --- a/backend/src/app/http.clj +++ b/backend/src/app/http.clj @@ -264,20 +264,27 @@ [_ params] (assert (sm/check schema:router-params params))) +(defn- root-middleware + "Root middleware chain applied to every route. Exposed as a function so + tests can assert the trusted-origin middleware is registered and ordered + so it wraps the rest of the chain." + [cfg] + [[mw/server-timing] + [mw/trusted-origin] + [sec/sec-fetch-metadata] + [mw/params] + [mw/format-response] + [mw/auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg) + :token (partial actoken/decode-token cfg)}] + [mw/parse-request] + [mw/errors errors/handle] + [mw/restrict-methods]]) + (defmethod ig/init-key ::router [_ cfg] (rr/router - [["" {:middleware [[mw/server-timing] - [mw/trusted-origin] - [sec/sec-fetch-metadata] - [mw/params] - [mw/format-response] - [mw/auth {:bearer (partial session/decode-token cfg) - :cookie (partial session/decode-token cfg) - :token (partial actoken/decode-token cfg)}] - [mw/parse-request] - [mw/errors errors/handle] - [mw/restrict-methods]]} + [["" {:middleware (root-middleware cfg)} (::mtx/routes cfg) (::assets/routes cfg) diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 79fdb9aea9..ef80e2ce25 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -46,9 +46,20 @@ (handler (assoc request ::http/trusted-origin origin))) (handler request))))) +(defn- warn-invalid-trusted-origins! + "Log a warning for :trusted-origins entries that are not bare origins. + They are inert (fail-closed); the warning only surfaces the typo." + [] + (doseq [origin (cf/get :trusted-origins) + :when (not (cf/valid-origin? origin))] + (l/wrn :hint "ignoring malformed trusted origin (expected scheme://host[:port])" + :origin origin))) + (def trusted-origin {:name ::trusted-origin - :compile (constantly wrap-trusted-origin)}) + :compile (fn [& _] + (warn-invalid-trusted-origins!) + wrap-trusted-origin)}) (def params {:name ::params diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index 0da100e398..8fcbbe5c4f 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -813,3 +813,33 @@ loc (redirect-location result)] (t/is (.contains loc "https://alt.example.com?screen=auth-login&")) (t/is (.contains loc "error=unable-to-auth")))))) + +(t/deftest callback-honors-state-origin-even-when-not-in-config + ;; The signed state origin was trusted when the flow started; it is used + ;; as-is for the lifetime of the token, even if the operator removed it. + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + state (make-state-token cfg {:origin "https://alt.example.com"}) + request (default-request cfg :state state)] + (binding [cf/config {:public-uri "http://localhost:3449" + :trusted-origins #{}}] + (with-redefs [app.auth.oidc/resolve-provider (constantly {:type "oidc" :id "oidc"}) + app.auth.oidc/get-info (constantly {:email "u@e.com" :fullname "U" + :backend "oidc" :email-verified false + :props {}}) + app.auth.oidc/get-profile (constantly test-profile) + app.auth.oidc/update-profile-with-info (fn [_ profile _] profile) + app.loggers.audit/submit (constantly nil)] + (let [result (#'oidc/callback-handler cfg request) + loc (redirect-location result)] + (t/is (.contains loc "https://alt.example.com?screen=auth-verify-token&"))))))) + +(t/deftest callback-without-state-redirects-to-canonical-error + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + request {:params {}}] + (binding [cf/config {:public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"}}] + (let [result (#'oidc/callback-handler cfg request) + loc (redirect-location result)] + (t/is (= 302 (::yres/status result))) + (t/is (.contains loc "http://localhost:3449?screen=auth-login&")) + (t/is (.contains loc "error=unable-to-auth")))))) diff --git a/backend/test/backend_tests/config_test.clj b/backend/test/backend_tests/config_test.clj index 10f387ad96..0bb19d6054 100644 --- a/backend/test/backend_tests/config_test.clj +++ b/backend/test/backend_tests/config_test.clj @@ -69,3 +69,18 @@ (t/is (= "https://alt.example.com" (:public-uri (cf/with-public-uri "https://alt.example.com" config)))) (t/is (= config (cf/with-public-uri nil config))))) + +(t/deftest valid-origin?-checks-bare-origin-shape + (t/testing "accepts scheme://host[:port], including IPv6" + (doseq [origin ["http://localhost:3449" + "https://alt.example.com" + "https://alt.example.com:8443" + "https://[::1]:3449" + "HTTPS://Alt.Example.com"]] + (t/is (cf/valid-origin? origin) origin))) + + (t/testing "rejects nil, blank, missing scheme, paths, query and trailing slash" + (doseq [origin [nil "" " " "alt.example.com" "localhost:3449" + "https://alt.example.com/" "https://alt.example.com/penpot" + "https://alt.example.com?x=1" "https://alt.example.com#frag"]] + (t/is (not (cf/valid-origin? origin)) (str origin))))) diff --git a/backend/test/backend_tests/helpers.clj b/backend/test/backend_tests/helpers.clj index 598d53a2af..17792986f7 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -20,7 +20,6 @@ [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] - [app.http :as-alias http] [app.http.middleware :as mw] [app.main :as main] [app.media] @@ -476,16 +475,22 @@ ([data headers] (build-rpc-params data (make-dummy-request {:headers (or headers {})})))) +(defn- resolve-method! + "Resolve a registered method fn from a method registry, raising the + standard `rpc-method-not-found` error when missing." + [registry type label] + (let [[mdata method-fn] (get-in *system* [registry type])] + (when-not method-fn + (ex/raise :type :assertion + :code :rpc-method-not-found + :hint (str label " '" (name type) "' not found"))) + [mdata method-fn])) + (defn command! ([data] (command! data {})) ([{:keys [::type] :as data} {:keys [headers]}] - (let [[mdata method-fn] (get-in *system* [:app.rpc/methods type])] - (when-not method-fn - (ex/raise :type :assertion - :code :rpc-method-not-found - :hint (str/ffmt "rpc method '%' not found" (name type)))) - + (let [[_ method-fn] (resolve-method! :app.rpc/methods type "rpc method")] (let [params (prepare-rpc-params data headers)] (try-on! (method-fn params)))))) @@ -493,11 +498,7 @@ ([data] (management-command! data {})) ([{:keys [::type] :as data} {:keys [headers]}] - (let [[_ method-fn] (get-in *system* [:app.rpc/management-methods type])] - (when-not method-fn - (ex/raise :type :assertion - :code :rpc-method-not-found - :hint (str/ffmt "management rpc method '%' not found" (name type)))) + (let [[_ method-fn] (resolve-method! :app.rpc/management-methods type "management rpc method")] (let [params (prepare-rpc-params data headers)] (try-on! (method-fn params)))))) @@ -506,19 +507,17 @@ the request `Origin` header can override the config :public-uri. Accepts the same data map as `command!` plus an options map with - `:headers` for the dummy request." + `:headers` for the dummy request and `:request-attrs` merged into it + (for request-scoped values such as `:app.http/auth-data`)." ([data] (command-through-middleware! data {})) - ([{:keys [::type] :as data} {:keys [headers]}] - (let [[_ method-fn] (get-in *system* [:app.rpc/methods type])] - (when-not method-fn - (ex/raise :type :assertion - :code :rpc-method-not-found - :hint (str/ffmt "rpc method '%' not found" (name type)))) - (let [handler (fn [request] - (try-on! (method-fn (build-rpc-params data request))))] - ((mw/wrap-trusted-origin handler) - (make-dummy-request {:headers (or headers {})})))))) + ([{:keys [::type] :as data} {:keys [headers request-attrs]}] + (let [[_ method-fn] (resolve-method! :app.rpc/methods type "rpc method") + request (merge (make-dummy-request {:headers (or headers {})}) + request-attrs) + handler (fn [request] + (try-on! (method-fn (build-rpc-params data request))))] + ((mw/wrap-trusted-origin handler) request)))) (defn run-task! ([name] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index a2526d03a3..3b1887e7e2 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -675,7 +675,7 @@ handler (mw/wrap-trusted-origin (fn [request] (reset! seen {:public-uri (cf/get :public-uri) - :stamp (::http/trusted-origin request)}) + :stamp (:app.http/trusted-origin request)}) {}))] (with-redefs [cf/config (assoc cf/config :public-uri "http://localhost:3449" @@ -707,3 +707,15 @@ (t/testing "the binding does not leak past the request" (handler (th/make-dummy-request {:headers {"origin" "https://alt.example.com"}})) (t/is (= "http://localhost:3449" (cf/get :public-uri))))))) + +(t/deftest root-middleware-registers-trusted-origin-before-auth + ;; The feature only works if `mw/trusted-origin` is part of the root chain + ;; and ordered so it wraps auth and errors; guard against silent removal or + ;; reordering. + (let [names (mapv (comp :name first) (#'app.http/root-middleware {})) + pos (fn [n] (first (keep-indexed (fn [i x] (when (= n x) i)) names)))] + (t/is (some? (pos ::mw/trusted-origin))) + (t/is (some? (pos ::mw/auth))) + (t/is (some? (pos ::mw/errors))) + (t/is (apply < (remove nil? [(pos ::mw/trusted-origin) (pos ::mw/auth)]))) + (t/is (apply < (remove nil? [(pos ::mw/trusted-origin) (pos ::mw/errors)]))))) diff --git a/backend/test/backend_tests/rpc_auth_test.clj b/backend/test/backend_tests/rpc_auth_test.clj index 802c70ac08..625e8a02b5 100644 --- a/backend/test/backend_tests/rpc_auth_test.clj +++ b/backend/test/backend_tests/rpc_auth_test.clj @@ -8,12 +8,17 @@ (:require [app.auth.ldap :as ldap] [app.auth.login-lockout :as lol] + [app.auth.oidc :as oidc] [app.common.flags :as flags] [app.common.generic-pool :as gpool] + [app.common.uri :as u] [app.common.uuid :as uuid] + [app.config :as cf] + [app.http :as-alias http] [app.http.session :as session] [app.loggers.audit :as-alias audit] [app.redis :as rds] + [app.rpc :as-alias rpc] [app.rpc.commands.ldap :as ldap-cmd] [backend-tests.helpers :as th] [clojure.test :as t] @@ -252,4 +257,34 @@ (t/is (not= (:id typed) (::audit/profile-id (meta result))) "session must not be bound to the typed-email profile"))))) +(t/deftest logout-uses-trusted-origin-for-post-logout-redirect + (let [profile-id (uuid/random) + provider {:logout-uri "https://idp.example.com/logout" + :client-id "test-client"} + data {::th/type :logout + ::rpc/profile-id profile-id + :profile-id profile-id} + attrs {::http/auth-data {:claims {:sso-provider-id "oidc" + :sso-session-id "sid"}}} + config (assoc cf/config + :public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"})] + + (with-redefs [oidc/get-provider (fn [_ _] provider)] + (t/testing "trusted origin becomes the post-logout redirect" + (with-redefs [cf/config config] + (let [out (th/command-through-middleware! + data {:headers {"origin" "https://alt.example.com"} + :request-attrs attrs}) + q (u/query-string->map (:query (:redirect-uri (:result out))))] + (t/is (= "https://alt.example.com" (get q :post_logout_redirect_uri)))))) + + (t/testing "without a trusted origin the canonical base is used" + (with-redefs [cf/config config] + (let [out (th/command-through-middleware! + data {:headers {"origin" "https://evil.example.com"} + :request-attrs attrs}) + q (u/query-string->map (:query (:redirect-uri (:result out))))] + (t/is (= "http://localhost:3449" (get q :post_logout_redirect_uri))))))))) + diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index a156faa374..07fecfeef9 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -1752,40 +1752,26 @@ (t/is (th/success? out)) (t/is (nil? (some #(= (:id project) (:id %)) (:result out)))))))) -(t/deftest create-team-invitations-uses-trusted-origin-as-link-base - (with-mocks [mock {:target 'app.email/send! :return nil}] - (let [owner (th/create-profile* 1 {:is-active true}) - team (th/create-team* 1 {:profile-id (:id owner)}) - data {::th/type :create-team-invitations - ::rpc/profile-id (:id owner) - :team-id (:id team) - :role :editor - :emails ["invitee@example.com"]}] - - (with-redefs [cf/config (assoc cf/config - :public-uri "http://localhost:3449" - :trusted-origins #{"https://alt.example.com"})] - (let [out (th/command-through-middleware! - data {:headers {"origin" "https://alt.example.com"}}) - email (-> @mock :call-args-list first first)] - (t/is (th/success? out)) - (t/is (= "https://alt.example.com" (:public-uri email)))))))) - -(t/deftest create-team-invitations-ignores-untrusted-origin - (with-mocks [mock {:target 'app.email/send! :return nil}] - (let [owner (th/create-profile* 1 {:is-active true}) - team (th/create-team* 1 {:profile-id (:id owner)}) - data {::th/type :create-team-invitations - ::rpc/profile-id (:id owner) - :team-id (:id team) - :role :editor - :emails ["invitee@example.com"]}] - - (with-redefs [cf/config (assoc cf/config - :public-uri "http://localhost:3449" - :trusted-origins #{"https://alt.example.com"})] - (let [out (th/command-through-middleware! - data {:headers {"origin" "https://evil.example.com"}}) - email (-> @mock :call-args-list first first)] - (t/is (th/success? out)) - (t/is (= "http://localhost:3449" (:public-uri email)))))))) +(t/deftest create-team-invitations-link-base-follows-trusted-origins + (let [owner (th/create-profile* 1 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)}) + cases [{:origin "https://alt.example.com" + :email "trusted@example.com" + :expected "https://alt.example.com"} + {:origin "https://evil.example.com" + :email "untrusted@example.com" + :expected "http://localhost:3449"}]] + (doseq [{:keys [origin email expected]} cases] + (with-mocks [mock {:target 'app.email/send! :return nil}] + (with-redefs [cf/config (assoc cf/config + :public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"})] + (let [data {::th/type :create-team-invitations + ::rpc/profile-id (:id owner) + :team-id (:id team) + :role :editor + :emails [email]} + out (th/command-through-middleware! data {:headers {"origin" origin}}) + sent (-> @mock :call-args-list first first)] + (t/is (th/success? out) origin) + (t/is (= expected (:public-uri sent)) origin))))))) diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 53710d47ea..310f94894d 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -401,7 +401,8 @@ PENPOT_TRUSTED_ORIGINS: https://alt.penpot.mycompany.com, https://staging.penpot ``` - The value is a comma or whitespace separated list of bare origins - (`scheme://host[:port]`). Paths are not supported. + (`scheme://host[:port]`). Paths are not supported; malformed entries are ignored + and a warning is logged at startup. - The setting is optional and backend-only. With no value, behavior is unchanged (fail-closed). - Each alternate origin must register its own OIDC callback URL