github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

♻️ Address review findings on trusted alternate origins

Browse Source 
Verify the OIDC state token once per callback and reuse the parsed
claims, removing the duplicate verification while preserving the
invalid-state error redirect.

Test that the trusted-origin middleware is registered in the root
router chain and ordered before auth and errors, and cover the OIDC
logout post-logout redirect with and without a trusted origin.

Warn at startup about malformed PENPOT_TRUSTED_ORIGINS entries, unify
the request stamp keyword spelling, and clean up the test helpers.

Closes #11625

AI-assisted-by: deepseek-v4.1-flash
This commit is contained in:
Andrey Antukh 2026-09-23 18:17:42 +00:00
parent 60e8bf134a
commit a146fe2a73
11 changed files with 197 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
backend/src/app/auth/oidc.clj
View File

@ -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

11
backend/src/app/config.clj
View File

@ -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

29
backend/src/app/http.clj
View File

@ -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)

13
backend/src/app/http/middleware.clj
View File

@ -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

30
backend/test/backend_tests/auth_oidc_test.clj
View File

@ -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"))))))

15
backend/test/backend_tests/config_test.clj
View File

@ -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)))))

45
backend/test/backend_tests/helpers.clj
View File

@ -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]

14
backend/test/backend_tests/http_middleware_test.clj
View File

@ -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)])))))

35
backend/test/backend_tests/rpc_auth_test.clj
View File

@ -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)))))))))

60
backend/test/backend_tests/rpc_team_test.clj
View File

@ -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)))))))

3
docs/technical-guide/configuration.md
View File

@ -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