diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index 18ef3380e4..faca707f9c 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -6,7 +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`. +- 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`. MCP OAuth (`app.http.mcp-oauth`) sits outside the root middleware chain with its own middleware, so it wraps its own routes with `wrap-trusted-origin` instead; `authorize` stages the stamped `:origin` in its Redis `:request` record and `decision` re-applies it for the approval redirect, `issuer`/`resource` metadata and the code-exchange `iss`, keeping the whole flow on one base. - 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/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 01dc0f5645..99deb76b08 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -453,22 +453,9 @@ [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 [] - (str (build-public-uri "api/auth/oidc/callback"))) + (cf/get-public-uri "api/auth/oidc/callback")) (defn build-auth-redirect-uri [provider token] diff --git a/backend/src/app/http/mcp_oauth.clj b/backend/src/app/http/mcp_oauth.clj index 5184cc7e65..7dbd5e5889 100644 --- a/backend/src/app/http/mcp_oauth.clj +++ b/backend/src/app/http/mcp_oauth.clj @@ -145,7 +145,12 @@ :challenge (:code_challenge params) :state (:state params) :resource (resource) - :scope "mcp"} + :scope "mcp" + ;; The trusted alternate origin stamped on + ;; this request (nil on the canonical base). + ;; Decision re-applies it so every step of + ;; the flow agrees on one base. + :origin (:app.http/trusted-origin request)} 600)] (redirect (append-query (str (cf/get-public-uri)) {:screen "mcp-authorize" :request handle}))))) @@ -215,9 +220,12 @@ (assoc :token-id (str (:id token)))) 120)] {:code code}))] - (response {:redirect-uri (append-query (:redirect-uri value) - (merge result {:state (:state value) - :iss (issuer)}))})))) + (binding [cf/config (cf/with-public-uri (:origin value) cf/config)] + ;; Re-applied from the staged request so :iss matches the + ;; authorize-time base (nil keeps the canonical one). + (response {:redirect-uri (append-query (:redirect-uri value) + (merge result {:state (:state value) + :iss (issuer)}))}))))) (defn- client-authentication "Extract one OAuth client-authentication method, rejecting mixed credentials. @@ -336,7 +344,10 @@ ;; middleware. Only the approval UI endpoints require a Penpot login session. (defmethod ig/init-key ::routes [_ cfg] - ["" + ;; Trusted-origin runs outermost so a listed request Origin is honored + ;; without inheriting the RPC transforms above (form parsing, protocol + ;; JSON names and session handling stay OAuth-specific below). + ["" {:middleware [[mw/trusted-origin]]} ["/.well-known/oauth-authorization-server/api/mcp/oauth" (endpoint cfg (fn [_ _] (response (authorization-metadata))) :get true)] ["/api/mcp/oauth" diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index 059135e803..e7470eba87 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -234,11 +234,6 @@ 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"} @@ -953,6 +948,44 @@ (t/is (= "https://alt.example.com" @seen)) (t/is (.contains loc "https://alt.example.com?screen=auth-verify-token&"))))))) +(t/deftest callback-token-exchange-uses-stored-trusted-origin + ;; Unlike callback-applies-stored-trusted-origin, this drives the real + ;; token exchange (mocking app.http.client/req instead of get-info) so + ;; the redirect_uri sent to the provider is asserted, not only the + ;; auth-request one and the post-exchange redirect. + (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) + bodies (atom []) + provider {:type "oidc" :id "oidc" + :user-info-source "userinfo" + :token-uri "https://idp.example.com/token" + :user-uri "https://idp.example.com/userinfo"} + 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 provider) + app.auth.oidc/get-profile (constantly test-profile) + app.auth.oidc/update-profile-with-info (fn [_ profile _] profile) + app.loggers.audit/submit (constantly nil)] + (with-mocks [http-mock {:target 'app.http.client/req + :return (fn [_ request & _] + (swap! bodies conj request) + (if (= (:uri request) (:token-uri provider)) + {:status 200 + :body "{\"access_token\":\"at\",\"token_type\":\"Bearer\"}"} + {:status 200 + :body "{\"email\":\"u@e.com\",\"name\":\"U\"}"}))}] + (let [result (#'oidc/callback-handler cfg request) + token-body (->> @bodies + (filter #(= (:uri %) (:token-uri provider))) + (first) + (:body) + (u/query-string->map))] + (t/is (= "https://alt.example.com/api/auth/oidc/callback" + (:redirect_uri token-body))) + (t/is (.contains (redirect-location result) + "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"}) diff --git a/backend/test/backend_tests/mcp_oauth_http_test.clj b/backend/test/backend_tests/mcp_oauth_http_test.clj index 3a19d8bd93..da7c40489c 100644 --- a/backend/test/backend_tests/mcp_oauth_http_test.clj +++ b/backend/test/backend_tests/mcp_oauth_http_test.clj @@ -251,3 +251,75 @@ :redirect_uri "https://attacker.example/callback"}))] (t/is (= 400 (::yres/status reply))) (t/is (nil? (get-in reply [::yres/headers "location"]))))) + +(t/deftest trusted-origin-flow + (binding [cf/config (assoc cf/config + :public-uri "http://localhost:3449" + :trusted-origins #{"https://alt.example.com"})] + (let [alt "https://alt.example.com"] + ;; Discovery honors a trusted Origin and ignores anything else. + (doseq [{:keys [origin expected]} [{:origin alt :expected alt} + {:origin "https://evil.example.com" + :expected "http://localhost:3449"} + {:origin nil + :expected "http://localhost:3449"}]] + (let [headers (cond-> {} (some? origin) (assoc "origin" origin)) + metadata (body (request "/.well-known/oauth-authorization-server/api/mcp/oauth" + :headers headers)) + resource (body (request "/api/mcp/oauth/resource" :headers headers))] + (t/is (= (str expected "/api/mcp/oauth") (:issuer metadata)) origin) + (t/is (= (str expected "/mcp/stream") (:resource resource)) origin))) + ;; An unlisted Origin keeps the canonical approval redirect. + (let [client (clients/register! th/*system* {:redirect_uris ["https://example.com/callback"] + :token_endpoint_auth_method "none"}) + params {:client_id (:client_id client) + :redirect_uri (first (:redirect_uris client)) + :response_type "code" :scope "mcp" + :resource (oauth/resource) + :state "client-state" + :code_challenge (state/digest (apply str (repeat 43 "a"))) + :code_challenge_method "S256"} + reply (request "/api/mcp/oauth/authorize" + :headers {"origin" "https://evil.example.com"} + :query (form params))] + (t/is (= 303 (::yres/status reply))) + (t/is (str/starts-with? (get-in reply [::yres/headers "location"]) + "http://localhost:3449/?screen=mcp-authorize"))) + ;; Full flow on the alternate base: authorize, approve and exchange. + (let [profile (th/create-profile* 1 {:is-active true}) + cookies (login-cookie profile) + client (clients/register! th/*system* {:redirect_uris ["https://example.com/callback"] + :token_endpoint_auth_method "none"}) + reply (request "/api/mcp/oauth/authorize" + :headers {"origin" alt} + :query (form {:client_id (:client_id client) + :redirect_uri (first (:redirect_uris client)) + :response_type "code" :scope "mcp" + :resource (str alt "/mcp/stream") + :state "client-state" + :code_challenge (state/digest (apply str (repeat 43 "a"))) + :code_challenge_method "S256"})) + location (get-in reply [::yres/headers "location"]) + handle (get (uri/query-string->map (.getQuery (java.net.URI. location))) + :request) + details (body (request "/api/mcp/oauth/request" + :query (form {:request handle}) :cookies cookies)) + submit #(request "/api/mcp/oauth/decision" :method :post :cookies cookies + :headers {"content-type" "application/x-www-form-urlencoded" + "origin" alt} + :body-bytes (form %)) + approved (submit {:request handle :decision "approve" :csrf (:csrf details)}) + params (uri/query-string->map + (.getQuery (java.net.URI. (:redirect-uri (body approved))))) + exchange {:grant_type "authorization_code" :code (:code params) + :client_id (:client_id client) :redirect_uri "https://example.com/callback" + :resource (str alt "/mcp/stream") + :code_verifier (apply str (repeat 43 "a"))} + token (request "/api/mcp/oauth/token" :method :post + :headers {"content-type" "application/x-www-form-urlencoded"} + :body-bytes (form exchange))] + (t/is (= 303 (::yres/status reply))) + (t/is (str/starts-with? location (str alt "/?screen=mcp-authorize"))) + (t/is (= 200 (::yres/status approved))) + (t/is (= (str alt "/api/mcp/oauth") (:iss params))) + (t/is (= 200 (::yres/status token))))))) diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 1f42be7075..8b0b968406 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -431,7 +431,9 @@ instead. When a request carries an `Origin` header that matches one of the liste 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`. +keep using `PENPOT_PUBLIC_URI`. Removing an origin from the list does not +invalidate OIDC states already signed with it; they keep using it until they +expire (up to four hours). ```bash # Backend @@ -449,6 +451,9 @@ PENPOT_TRUSTED_ORIGINS: https://alt.penpot.mycompany.com, https://staging.penpot - 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`. +- MCP OAuth endpoints (discovery metadata, the approval redirect and the + code-exchange `iss`) follow the stamped origin the same way; without a trusted + `Origin` they keep the canonical `PENPOT_PUBLIC_URI`. ## Email configuration