From f43cc96f15c32f04b9b7bf05b8641991b9d4906d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 23 Sep 2026 17:40:27 +0000 Subject: [PATCH] :sparkles: Honor trusted alternate origins for backend links and OIDC Add an optional PENPOT_TRUSTED_ORIGINS setting listing alternate origins as bare scheme://host[:port] entries. Matching is exact on the trimmed, lowercased Origin; unlisted origins and requests without Origin keep the canonical public-uri, and server-to-server or queued work is unchanged. Malformed entries produce a startup warning and never match. Stamp the trusted origin on each request with a global http middleware that binds config public-uri for the request extent. Expose the chain as root-middleware so tests can assert trusted-origin runs before auth and errors. Email links, asset URLs, audit and webhook link fields, and redirects then use the alternate base while serving that request. Carry the origin through OIDC in the signed state, since the IdP callback is a navigation without Origin: auth and organization SSO store it, and the callback re-applies it with with-public-uri for the token exchange redirect_uri and every redirect target, including the logout post-logout redirect. Verify the state token once per callback and reuse the claims. Build redirect URIs with the uniform ensure-path-slash plus join pattern so subpaths survive, using append-query-param for hash routes. Closes #11625 AI-assisted-by: deepseek-v4.1-flash AI-assisted-by: qwen3.7-plus AI-assisted-by: gpt-5.6-luna --- .../auth-permissions-product-domains.md | 1 + .serena/memories/backend/subtleties.md | 1 + backend/src/app/auth/oidc.clj | 56 ++++- backend/src/app/config.clj | 45 ++++ backend/src/app/http.clj | 28 ++- backend/src/app/http/middleware.clj | 28 +++ backend/src/app/rpc/commands/nitrate.clj | 5 +- backend/test/backend_tests/auth_oidc_test.clj | 192 +++++++++++++++++- backend/test/backend_tests/config_test.clj | 46 +++++ backend/test/backend_tests/helpers.clj | 66 ++++-- .../backend_tests/http_middleware_test.clj | 49 +++++ backend/test/backend_tests/rpc_auth_test.clj | 35 ++++ .../test/backend_tests/rpc_nitrate_test.clj | 32 ++- backend/test/backend_tests/rpc_team_test.clj | 23 +++ docker/images/docker-compose.yaml | 5 + docs/technical-guide/configuration.md | 32 +++ 16 files changed, 602 insertions(+), 42 deletions(-) diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index 4cd1658906..18ef3380e4 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -6,6 +6,7 @@ - Public auth endpoints must explicitly set `::rpc/auth false`; RPC auth defaults to enabled. Session cookie creation/deletion is usually attached as an RPC response transform. - Basic Penpot registration is token staged: prepare/register creates or verifies temporary tokens, then profile creation/session setup is reused by other auth backends. The frontend `/auth/verify-token` flow is a hub for registration confirmation, email change, and invitation tokens. - OIDC-compatible providers share a generic flow: redirect to provider, validate callback/request token, fetch identity data, then login an existing profile or register a new one. Known providers may have hardcoded endpoints; generic OIDC can use discovery/configured endpoints. +- Generic OIDC authorization stores a trusted alternate `Origin` (request `:app.http/trusted-origin`, see `mem:backend/subtleties`) as the `:origin` claim in the signed state; the callback re-applies it to the config public URI so the token-exchange `redirect_uri` and redirect targets stay on the alternate domain. Organization SSO mirrors this through `build-organization-sso-auth-redirect-uri :origin`. - LDAP login validates credentials against the external directory, fetches identity data, then logs in or registers a matching Penpot profile. LDAP registration is not a separate Penpot signup flow. - LDAP session identity MUST come from the directory-returned email (`info.email`): the profile matching the typed email can differ (aliases, UPNs, multi-valued `mail` attributes) and is used only for lockout checks, never to bind the session. - Account lockout (flag `:account-lockout`, `app.auth.login-lockout`) is Redis-backed and keyed per profile id. Password and LDAP flows check/increment on the profile derived from the typed email and clear on the profile that actually logs in. diff --git a/.serena/memories/backend/subtleties.md b/.serena/memories/backend/subtleties.md index 5976fac9ad..576d744d12 100644 --- a/.serena/memories/backend/subtleties.md +++ b/.serena/memories/backend/subtleties.md @@ -52,6 +52,7 @@ - `app.config/config` and `flags` are dynamic `defonce` vars populated from `PENPOT_*` env vars through the shared schema string transformer. Tests and tooling can bind them. - Duration config values are parsed as ISO-8601 with the `PT` prefix, so a day is written in hours (`:jobs-retention` and `:jobs-user-ttl` default to a week, and an operator writes `168h`): `7d` does not parse, and the value fails validation at boot instead of being silently ignored. - `parse-flags` automatically adds `:disable-secure-session-cookies` when `public-uri` is plain HTTP and not localhost. This changes cookie defaults without an explicit env flag. +- A request `Origin` listed in `:trusted-origins` (`PENPOT_TRUSTED_ORIGINS`, bare origins, comma/whitespace separated) is stamped on the request as `:app.http/trusted-origin` by the http `wrap-trusted-origin` middleware, which binds `app.config/config` with `:public-uri` replaced by that origin for the dynamic extent of the request. Every request-scoped link builder (emails, asset URLs, audit/webhook link fields, redirects) then follows it; unlisted or absent origins keep the canonical `:public-uri` (fail-closed). Background workers run outside the binding and keep the canonical base. - The backend sets Clojure `*assert*` globally from the `:backend-asserts` feature flag. Assertion-dependent checks can therefore differ by runtime flags. - Request body parsing is mostly POST-oriented and supports Transit JSON plus plain JSON. Plain JSON request keys are kebab-decoded before being merged into `:params`. A body the parser cannot read is a client error, classified at the parse site and never by exception class (`clojure.data.json` signals its syntax errors with a bare `java.lang.Exception`): 400 `:validation`/`:malformed-json`, one `warn` line (`malformed request body`) carrying path, peer address and content-type instead of a stack trace, and `penpot_http_malformed_requests_total`. `RequestTooBigException` keeps its 413. - Response formatting negotiates with `Accept` or `_fmt=json`. Transit is the default for collection/boolean bodies; JSON encoding has special pointer-map handling. diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 55aeff66fc..01dc0f5645 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -453,10 +453,22 @@ [fitem & items] (str/split path separator)] (into [(keyword (:type provider) (str/kebab fitem))] (map keyword) items))) +(defn- build-public-uri + "Join a relative path onto the configured public URI, preserving any + subpath. Uses `u/ensure-path-slash` + `u/join` to match the pattern + used throughout the backend and frontend. + `path` must not start with `/` — a leading slash would be treated as + absolute by `u/join`, silently dropping the configured subpath." + [path] + {:pre [(not (str/starts-with? path "/"))]} + (-> (cf/get :public-uri) + (u/uri) + (u/ensure-path-slash) + (u/join path))) + (defn- build-redirect-uri [] - (let [public (u/uri (cf/get :public-uri))] - (str (assoc public :path "/api/auth/oidc/callback")))) + (str (build-public-uri "api/auth/oidc/callback"))) (defn build-auth-redirect-uri [provider token] @@ -916,7 +928,7 @@ (defn build-organization-sso-auth-redirect-uri "Build the OIDC authorization redirect URI for an organization SSO config. Raises if the config is incomplete or OIDC discovery fails." - [cfg sso & {:keys [dest-url organization-id provider]}] + [cfg sso & {:keys [dest-url organization-id provider origin]}] (let [organization-id (or organization-id (:organization-id sso)) issuer (organization-sso-discovery-uri sso) dest-url (or dest-url (str (cf/get :public-uri)))] @@ -930,6 +942,7 @@ :dest-url dest-url :organization-id organization-id :issuer issuer + :origin origin :exp (ct/in-future "4h")})] (build-auth-redirect-uri oidc-provider state-token)) (catch Throwable cause @@ -1013,6 +1026,11 @@ :provider (:id provider) :invitation-token (:invitation-token params) :external-session-id esid + ;; Trusted alternate origin stamped by the http + ;; trusted-origin middleware; carried through the signed + ;; state so the callback (a navigation without Origin) + ;; can rebuild links and the token-exchange redirect_uri. + :origin (:app.http/trusted-origin request) :props props :exp (ct/in-future "4h")} state (tokens/generate cfg (d/without-nils params)) @@ -1064,16 +1082,27 @@ :organization-id organization-id :organization-name organization-name})))))) -(defn- callback-handler - [cfg {:keys [params] :as request}] +(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 + [(tokens/verify cfg {:token (:state params) :iss "oidc"}) nil] + (catch Throwable cause + [nil cause]))) + +(defn- handle-oidc-callback + [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. @@ -1148,6 +1177,17 @@ (l/err :hint "error on process oidc callback" :cause cause)) (redirect-with-error "unable-to-auth" (ex-message cause))))))) +(defn- callback-handler + "Callback entrypoint. The IdP redirect is a browser navigation that + normally carries no Origin, so the http trusted-origin middleware + cannot help here; re-apply the trusted origin stored in the signed + state for the whole callback (token exchange redirect_uri and every + redirect target)." + [cfg {:keys [params] :as 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 ::session/manager diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 99f47ea40c..263a023a00 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -163,6 +163,14 @@ ;; separated list of origins (e.g. "https://plugins.example.com"). [:allowed-origins {:optional true} [::sm/set :string]] + ;; Explicit allowlist of alternate origins the backend may use as the + ;; public base for links generated while serving a request whose + ;; `Origin` header matches. Configured via PENPOT_TRUSTED_ORIGINS as a + ;; comma/whitespace separated list of bare origins + ;; (e.g. "https://alt.example.com"). Fail-closed: origins not listed + ;; keep using :public-uri. + [:trusted-origins {:optional true} [::sm/set :string]] + [:exporter-shared-key {:optional true} :string] [:admin-console-shared-key {:optional true} :string] [:nexus-shared-key {:optional true} :string] @@ -504,6 +512,43 @@ (or (c/get config :jobs-user-ttl) (ct/duration {:days 7}))) +(defn normalize-origin + "Normalize an origin string for comparison: trim and lowercase it. + Returns nil for nil/blank input." + [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 + and lowercased); no paths, wildcards or subdomain matching." + [origin] + (let [origin (normalize-origin origin)] + (boolean + (and (some? origin) + (some #(= origin (normalize-origin %)) + (c/get config :trusted-origins)))))) + +(defn with-public-uri + "Returns a copy of `config` with :public-uri replaced by `base`. When + `base` is nil the config is returned unchanged, so callers can use it + for requests (or tokens) without a trusted origin." + [base config] + (if (some? base) + (assoc config :public-uri base) + config)) + (defn get "A configuration getter. Helps code be more testable." ([key] diff --git a/backend/src/app/http.clj b/backend/src/app/http.clj index 83af60c3d2..de0535eee0 100644 --- a/backend/src/app/http.clj +++ b/backend/src/app/http.clj @@ -253,19 +253,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 (::mtx/metrics cfg)] + [mw/errors errors/handle] + [mw/restrict-methods]]) + (defmethod ig/init-key ::router [_ cfg] (rr/router - [["" {:middleware [[mw/server-timing] - [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 (::mtx/metrics cfg)] - [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 ad80bfe9c4..b44fa04115 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -35,6 +35,34 @@ {:name ::server-timing :compile (constantly ymw/wrap-server-timing)}) +(defn wrap-trusted-origin + "When the request carries an `origin` header listed in + :trusted-origins, stamp it on the request and bind the config + :public-uri to it for the dynamic extent of the request. Otherwise the + request is left untouched and the canonical :public-uri is used." + [handler] + (fn [request] + (let [origin (cf/normalize-origin (yreq/get-header request "origin"))] + (if (cf/trusted-origin? origin) + (binding [cf/config (cf/with-public-uri origin cf/config)] + (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 (fn [& _] + (warn-invalid-trusted-origins!) + wrap-trusted-origin)}) + (def params {:name ::params :compile (constantly ymw/wrap-params)}) diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index af52bede6f..d470268864 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -739,7 +739,10 @@ (let [redirect-uri (oidc/build-organization-sso-auth-redirect-uri cfg sso :dest-url url - :organization-id organization-id) + :organization-id organization-id + ;; Trusted alternate origin stamped by the + ;; http trusted-origin middleware. + :origin (:app.http/trusted-origin request)) organization-id (or organization-id (:organization-id sso))] (oidc/submit-organization-sso-auth-started-event cfg request profile-id organization-id) diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index ea249d3144..059135e803 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -10,6 +10,7 @@ [app.common.data :as d] [app.common.exceptions :as ex] [app.common.time :as ct] + [app.common.uri :as u] [app.common.uuid :as uuid] [app.config :as cf] [app.http.session :as session] @@ -157,6 +158,13 @@ (t/is (.contains loc "error=auth-error")) (t/is (not (.contains loc "hint="))))))) +(t/deftest redirect-with-error-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-with-error "auth-error") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-login&")) + (t/is (.contains loc "error=auth-error"))))) + (t/deftest redirect-to-verify-token-builds-verify-url (binding [cf/config {:public-uri "http://localhost:3449"}] (let [result (#'oidc/redirect-to-verify-token "test-token-value") @@ -165,11 +173,72 @@ (t/is (.contains loc "http://localhost:3449?screen=auth-verify-token&")) (t/is (.contains loc "token=test-token-value"))))) +(t/deftest redirect-to-verify-token-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-to-verify-token "test-token-value") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-verify-token&"))))) + (t/deftest build-redirect-uri-constructs-redirect (binding [cf/config {:public-uri "http://localhost:3449"}] (t/is (= "http://localhost:3449/api/auth/oidc/callback" (#'oidc/build-redirect-uri))))) +(t/deftest build-redirect-uri-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (t/is (= "http://localhost:3449/penpot/api/auth/oidc/callback" + (#'oidc/build-redirect-uri))))) + +(t/deftest build-redirect-uri-with-subpath-and-trailing-slash + (binding [cf/config {:public-uri "http://localhost:3449/penpot/"}] + (t/is (= "http://localhost:3449/penpot/api/auth/oidc/callback" + (#'oidc/build-redirect-uri))))) + +(t/deftest redirect-with-error-subpath-screen-query + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-with-error "auth-error" "hint message") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-login&")) + (t/is (.contains loc "error=auth-error")) + (t/is (.contains loc "hint=hint")) + ;; Screen routing lives in the query string, not the fragment + (t/is (not (.contains loc "#")))))) + +(t/deftest redirect-to-register-subpath + (let [test-key (byte-array (map byte (range 32))) + cfg {::setup/props {:tokens-key test-key}}] + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [info {:email "u@e.com" :fullname "U" :backend "oidc" + :email-verified false :props {}} + provider {:type "oidc" :id "oidc"} + result (#'oidc/redirect-to-register cfg info provider) + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-register-validate&")) + (t/is (.contains loc "token=")))))) + +(t/deftest redirect-to-register-no-subpath + (let [test-key (byte-array (map byte (range 32))) + cfg {::setup/props {:tokens-key test-key}}] + (binding [cf/config {:public-uri "http://localhost:3449"}] + (let [info {:email "u@e.com" :fullname "U" :backend "oidc" + :email-verified false :props {}} + provider {:type "oidc" :id "oidc"} + result (#'oidc/redirect-to-register cfg info provider) + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449?screen=auth-register-validate&")) + (t/is (.contains loc "token=")))))) + +(t/deftest redirect-to-verify-token-subpath-no-trailing-slash + (binding [cf/config {:public-uri "http://localhost:3449/my-app"}] + (let [result (#'oidc/redirect-to-verify-token "test-token-value") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/my-app?screen=auth-verify-token&"))))) + +(t/deftest build-public-uri-no-subpath + (binding [cf/config {:public-uri "http://localhost:3449"}] + (t/is (= "http://localhost:3449/api/auth/oidc/callback" + (str (#'oidc/build-public-uri "api/auth/oidc/callback")))))) + (t/deftest fetch-user-info-returns-decoded-body-on-success (let [cfg {} provider {:user-uri "https://provider.example.com/userinfo"} @@ -482,7 +551,7 @@ (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) state (make-state-token cfg {}) request (default-request cfg :state state)] - (binding [cf/config {:public-uri "http://localhost:3449"} + (binding [cf/config {:public-uri "http://localhost:3449/penpot"} cf/flags #{:registration}] (with-redefs [app.auth.oidc/resolve-provider (constantly {:type "oidc" :id "oidc"}) app.auth.oidc/get-info (constantly {:email "u@e.com" :fullname "U" @@ -491,7 +560,7 @@ app.auth.oidc/get-profile (constantly (assoc test-profile :is-active false))] (let [result (#'oidc/callback-handler cfg request) loc (redirect-location result)] - (t/is (.contains loc "http://localhost:3449?screen=auth-register-validate&")) + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-register-validate&")) (t/is (.contains loc "token="))))))) (t/deftest callback-success-flow @@ -806,3 +875,122 @@ (t/is (ex/error? e)) (t/is (= :validation (:type (ex-data e)))) (t/is (= :invalid-sso-config (:code (ex-data e)))))))) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; trusted alternate origins +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(def ^:private trusted-origin-provider + {:id "oidc" + :type "oidc" + :auth-uri "https://idp.example.com/auth" + :client-id "test-client" + :scopes ["openid"]}) + +(t/deftest auth-handler-carries-trusted-origin-in-state + (with-redefs [app.auth.oidc/resolve-provider (constantly trusted-origin-provider) + app.loggers.audit/extract-utm-params (constantly {}) + app.loggers.audit/get-external-session-id (constantly nil)] + (t/testing "trusted origin is stored in the state and used for redirect_uri" + (let [request (assoc {:params {}} :app.http/trusted-origin "https://alt.example.com") + result (binding [cf/config {:public-uri "https://alt.example.com" + :trusted-origins #{"https://alt.example.com"}}] + (#'oidc/auth-handler base-cfg request)) + uri (get-in result [::yres/body :redirect-uri]) + query (u/query-string->map (:query (u/uri uri))) + claims (tokens/verify base-cfg {:token (:state query) :iss "oidc"})] + (t/is (= "https://alt.example.com" (:origin claims))) + (t/is (= "https://alt.example.com/api/auth/oidc/callback" (:redirect_uri query))))) + + (t/testing "without a trusted origin no :origin claim is stored" + (let [result (binding [cf/config {:public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"}}] + (#'oidc/auth-handler base-cfg {:params {}})) + uri (get-in result [::yres/body :redirect-uri]) + query (u/query-string->map (:query (u/uri uri))) + claims (tokens/verify base-cfg {:token (:state query) :iss "oidc"})] + (t/is (not (contains? claims :origin))) + (t/is (= "http://localhost:3449/api/auth/oidc/callback" (:redirect_uri query))))))) + +(t/deftest build-organization-sso-auth-redirect-uri-stores-trusted-origin + (let [sso {:organization-id test-organization-id + :issuer "https://idp.example.com" + :client-id "test-client" + :client-secret "test-secret"} + provider trusted-origin-provider + uri (binding [cf/config {:public-uri "https://alt.example.com" + :trusted-origins #{"https://alt.example.com"}}] + (oidc/build-organization-sso-auth-redirect-uri + base-cfg sso + :dest-url "https://alt.example.com/#/dashboard" + :organization-id test-organization-id + :provider provider + :origin "https://alt.example.com")) + query (u/query-string->map (:query (u/uri uri))) + claims (tokens/verify base-cfg {:token (:state query) :iss "oidc"})] + (t/is (= "https://alt.example.com" (:origin claims))) + (t/is (= "https://alt.example.com/#/dashboard" (:dest-url claims))) + (t/is (= "https://alt.example.com/api/auth/oidc/callback" (:redirect_uri query))))) + +(t/deftest callback-applies-stored-trusted-origin + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + seen (atom nil) + 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 #{"https://alt.example.com"}}] + (with-redefs [app.auth.oidc/resolve-provider (constantly {:type "oidc" :id "oidc"}) + app.auth.oidc/get-info (fn [_ _ _ _] + (reset! seen (cf/get :public-uri)) + {: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 (= "https://alt.example.com" @seen)) + (t/is (.contains loc "https://alt.example.com?screen=auth-verify-token&"))))))) + +(t/deftest callback-error-uses-stored-trusted-origin + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + state (make-state-token cfg {:origin "https://alt.example.com"}) + request (assoc (default-request cfg :state state) + :params {:state state :error "access_denied"})] + (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 (.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 74a159a703..3b53fdee15 100644 --- a/backend/test/backend_tests/config_test.clj +++ b/backend/test/backend_tests/config_test.clj @@ -86,3 +86,49 @@ (t/testing "and falls back to a week" (with-redefs [cf/config (dissoc cf/config :jobs-user-ttl)] (t/is (= (ct/duration {:days 7}) (cf/get-jobs-user-ttl)))))) + +(t/deftest trusted-origins-decodes-from-env-string + (t/testing "comma and whitespace separated origins become a set" + (let [config (cf/decode-config {:tenant "default" + :host "localhost" + :public-uri "http://localhost:3449" + :trusted-origins "https://a.example.com, https://b.example.com https://c.example.com"})] + (t/is (= #{"https://a.example.com" "https://b.example.com" "https://c.example.com"} + (:trusted-origins config)))))) + +(t/deftest trusted-origin?-checks-normalized-allowlist + (with-redefs [cf/config (assoc cf/config :trusted-origins #{"https://alt.example.com"})] + (t/testing "exact and normalized matches" + (t/is (cf/trusted-origin? "https://alt.example.com")) + (t/is (cf/trusted-origin? " HTTPS://Alt.Example.com "))) + (t/testing "nil, blank, unlisted, subdomain and path variants are rejected" + (t/is (not (cf/trusted-origin? nil))) + (t/is (not (cf/trusted-origin? ""))) + (t/is (not (cf/trusted-origin? "https://other.example.com"))) + (t/is (not (cf/trusted-origin? "https://sub.alt.example.com"))) + (t/is (not (cf/trusted-origin? "https://alt.example.com/penpot")))))) + +(t/deftest trusted-origin?-without-configuration + (with-redefs [cf/config (assoc cf/config :trusted-origins nil)] + (t/is (not (cf/trusted-origin? "https://alt.example.com"))))) + +(t/deftest with-public-uri-overrides-base + (let [config {:public-uri "http://localhost:3449" :host "localhost"}] + (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 378450f056..3c4df31051 100644 --- a/backend/test/backend_tests/helpers.clj +++ b/backend/test/backend_tests/helpers.clj @@ -21,6 +21,7 @@ [app.common.uuid :as uuid] [app.config :as cf] [app.db :as db] + [app.http.middleware :as mw] [app.jobs :as jobs] [app.main :as main] [app.media] @@ -507,25 +508,24 @@ remote-addr server-name server-port scheme protocol path query ssl-client-cert))) -(defn- prepare-rpc-params - [data] +(defn- build-rpc-params + "Builds the params map for a command from `data` and a ring request. + A plain-map `:app.http/request` in `data`'s metadata is honored, so + tests can inject headers, remote address and auth context (e.g. + :app.http/auth-key-id). Non-map requests (reify IRequest stubs) fall + back to `request`." + [data request] (let [params (reduce-kv (fn [params k v] (if (qualified-keyword? k) (assoc params k v) params)) {} (dissoc data ::type)) - ;; Honor a caller-supplied request map from the data metadata so - ;; tests can inject headers, remote address and auth context (e.g. - ;; :app.http/auth-key-id). Non-map requests (reify IRequest stubs) - ;; fall back to the dummy request, which carries the body params - ;; the validation wrapper reads. supplied (-> data meta :app.http/request) request (if (map? supplied) (assoc supplied :params (merge (:params supplied) (d/without-qualified data))) - (assoc (make-dummy-request) - :params (d/without-qualified data)))] + (assoc request :params (d/without-qualified data)))] (-> params (assoc :app.rpc/request-at (ct/now)) @@ -538,16 +538,29 @@ "multipart/")) (with-meta {:app.http/request request})))) -(defn command! - [{:keys [::type] :as data}] - (let [[mdata method-fn] (get-in *system* [:app.rpc/methods type])] +(defn- prepare-rpc-params + ([data] (prepare-rpc-params data {})) + ([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/ffmt "rpc method '%' not found" (name type)))) + :hint (str label " '" (name type) "' not found"))) + [mdata method-fn])) - (let [params (prepare-rpc-params data)] - (try-on! (method-fn params))))) +(defn command! + ([data] + (command! data {})) + ([{:keys [::type] :as data} {:keys [headers]}] + (let [[_ method-fn] (resolve-method! :app.rpc/methods type "rpc method")] + (let [params (prepare-rpc-params data headers)] + (try-on! (method-fn params)))))) (defn command-raw! "Like `command!`, but returns the raw result without unwrapping it, so a test @@ -572,11 +585,7 @@ tests exercise the allowed path without repeating the key; a test that checks a rejection declares the key explicitly, including `nil`." [{:keys [::type] :as data}] - (let [[mdata 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 [[mdata method-fn] (resolve-method! :app.rpc/management-methods type "management rpc method")] (let [request (-> data meta :app.http/request) key-id (cond (contains? data ::rpc/auth-key-id) @@ -598,6 +607,23 @@ params (prepare-rpc-params data)] (try-on! (method-fn params))))) +(defn command-through-middleware! + "Runs an RPC method wrapped in the HTTP trusted-origin middleware, so + 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 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 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)))) + (def multipart-headers {"content-type" "multipart/form-data; boundary=test"}) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 57766ab3b7..1f9456da1b 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -761,3 +761,52 @@ decoded (json/decode encoded :key-fn json/read-kebab-key)] (t/is (str/includes? encoded "invalid-storage-metadata")) (t/is (string? (:explain decoded)))))) +(t/deftest trusted-origin-middleware-binds-public-uri + (let [seen (atom nil) + handler (mw/wrap-trusted-origin + (fn [request] + (reset! seen {:public-uri (cf/get :public-uri) + :stamp (:app.http/trusted-origin request)}) + {}))] + (with-redefs [cf/config (assoc cf/config + :public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"})] + (t/testing "trusted origin overrides the public base and stamps the request" + (reset! seen nil) + (handler (th/make-dummy-request {:headers {"origin" "https://alt.example.com"}})) + (t/is (= "https://alt.example.com" (:public-uri @seen))) + (t/is (= "https://alt.example.com" (:stamp @seen)))) + + (t/testing "origin is normalized before matching and use" + (reset! seen nil) + (handler (th/make-dummy-request {:headers {"origin" " HTTPS://Alt.Example.com "}})) + (t/is (= "https://alt.example.com" (:public-uri @seen))) + (t/is (= "https://alt.example.com" (:stamp @seen)))) + + (t/testing "untrusted origin is ignored" + (reset! seen nil) + (handler (th/make-dummy-request {:headers {"origin" "https://evil.example.com"}})) + (t/is (= "http://localhost:3449" (:public-uri @seen))) + (t/is (nil? (:stamp @seen)))) + + (t/testing "missing origin is ignored" + (reset! seen nil) + (handler (th/make-dummy-request {})) + (t/is (= "http://localhost:3449" (:public-uri @seen))) + (t/is (nil? (:stamp @seen)))) + + (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 e2a4a54f8c..992d21a3bd 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.auth :as auth] [app.rpc.commands.ldap :as ldap-cmd] [app.storage :as sto] @@ -256,6 +261,36 @@ (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))))))))) + ;; --- Profile picture import diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index abbfc679fc..e404d66418 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -175,13 +175,43 @@ (t/is (= {:authorized false :redirect-uri redirect-uri} (:result out))) - (t/is (= #{:dest-url :organization-id} (set (keys @redirect-options)))) + (t/is (= #{:dest-url :organization-id :origin} (set (keys @redirect-options)))) (t/is (= "https://penpot.example.com/#/workspace" (str (:dest-url @redirect-options)))) (t/is (nil? (:organization-id @redirect-options))) + (t/is (nil? (:origin @redirect-options))) (t/is (= {:profile-id (:id team-owner) :organization-id organization-id} @started-event))))))) +(t/deftest check-nitrate-sso-passes-trusted-origin + (let [team-owner (th/create-profile* 1 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id team-owner)}) + organization-id (uuid/random) + redirect-options (atom nil) + params {::th/type :check-nitrate-sso + ::rpc/profile-id (:id team-owner) + :team-id (:id team) + :url "https://alt.example.com/#/workspace"}] + (binding [cf/flags (conj cf/flags :admin-console)] + (with-redefs [cf/config (assoc cf/config + :public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"}) + nitrate/call + (active-sso-call-mock + (:id team) + organization-id + (:id team-owner)) + oidc/build-organization-sso-auth-redirect-uri + (fn [_cfg _sso & options] + (reset! redirect-options (apply hash-map options)) + "https://idp.example.com/authorize") + oidc/submit-organization-sso-auth-started-event + (constantly nil)] + (let [out (th/command-through-middleware! + params {:headers {"origin" "https://alt.example.com"}})] + (t/is (th/success? out)) + (t/is (= "https://alt.example.com" (:origin @redirect-options)))))))) + (t/deftest check-nitrate-sso-reports-redirect-failure (let [profile (th/create-profile* 1 {:is-active true}) organization-id (uuid/random) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 51a1e746e1..b0bfa0387b 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -2446,3 +2446,26 @@ :role :owner}) props (:app.loggers.audit/props (meta out))] (t/is (= (:id owner) (:prev-owner-id props)))))) +(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-template :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 second)] + (t/is (th/success? out) origin) + (t/is (= expected (:public-uri sent)) origin))))))) diff --git a/docker/images/docker-compose.yaml b/docker/images/docker-compose.yaml index cb0fe7d676..aec1162270 100644 --- a/docker/images/docker-compose.yaml +++ b/docker/images/docker-compose.yaml @@ -31,6 +31,11 @@ x-flags: &penpot-flags ## front must forward the subpath unchanged, without rewriting it. x-uri: &penpot-public-uri PENPOT_PUBLIC_URI: http://localhost:9001 + ## Optional: alternate origins the backend may use as the base for generated + ## links (emails, assets, redirects) when a request Origin matches. Bare + ## origins only, comma/whitespace separated; each must register its OIDC + ## callback URL (https:///api/auth/oidc/callback) at the IdP. + # PENPOT_TRUSTED_ORIGINS: https://alt.example.com, https://staging.example.com x-body-size: &penpot-http-body-size # Max body size diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 2f6498b02f..1f42be7075 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -418,6 +418,38 @@ callback URLs and cookie settings. Active users may need to sign in again. may limit the usage of Penpot; as an example, the clipboard does not work with HTTP.

+### Trusted alternate origins + +When Penpot is served on an alternate domain (a temporary deployment, a canary, or a +migration between domains) while `PENPOT_PUBLIC_URI` still points at the canonical +domain, the backend keeps building absolute links from `PENPOT_PUBLIC_URI`. Users +browsing the alternate domain then get links (for example in invitation emails) that +send them to the canonical deployment. + +`PENPOT_TRUSTED_ORIGINS` lets operators list the alternate origins the backend may use +instead. When a request carries an `Origin` header that matches one of the listed +origins, the backend uses that origin as the base for the links it generates while +serving the request (transactional emails, asset URLs, audit and webhook link fields, +and redirects). Origins that are not listed, and requests without an `Origin` header, +keep using `PENPOT_PUBLIC_URI`. + +```bash +# Backend +PENPOT_TRUSTED_ORIGINS: https://alt.penpot.mycompany.com, https://staging.penpot.mycompany.com +``` + +- The value is a comma or whitespace separated list of bare origins + (`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 + (`https:///api/auth/oidc/callback`) at the identity provider. If you use SSO + logout, its post-logout redirect URL must be registered as well. +- Notifications sent later by background workers keep the canonical `PENPOT_PUBLIC_URI`, + because they run outside the request that triggered them. The same applies to + server-to-server calls (for example the management API) that carry no `Origin`. + ## Email configuration By default, smtp flag is disabled, the email will be