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