From 77491ea662ce9a7a4dde6271e858e4a075b6d240 Mon Sep 17 00:00:00 2001 From: awais786 Date: Fri, 15 May 2026 18:03:35 +0500 Subject: [PATCH 01/19] :bug: Re-key session when X-Auth-Request identity changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The X-Auth-Request middleware short-circuited as soon as wrap-session had resolved a profile-id from the auth-token cookie. That meant once Penpot issued its own session cookie, the upstream X-Auth-Request-Email was no longer consulted at all. After a portal "log out of all apps" + login as a different user (which clears the shared _oauth2_proxy cookie and Cognito session but NOT Penpot's auth-token cookie on its own subdomain), wrap-session kept resolving the previous user from the stale cookie, and wrap-authz stepped aside — so refreshing the Penpot tab kept serving the previous user. Now: when X-Auth-Request-Email is present, always resolve the proxy- asserted profile and compare it against the existing session-pid. - Match → pass through unchanged (steady state). - Mismatch (or no existing session) → re-key: inject the new profile-id into the request and issue a fresh auth-token cookie on the response. Access-token (API key) auth is unaffected — programmatic identity is not a browser SSO session and the header has no bearing on it. Cost: one extra DB lookup per authenticated SSO request to resolve the header email's profile. The previous fast-path is restored implicitly for the match case (downstream handler runs without re-keying), so only the lookup itself is added. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/app/http/auth_request.clj | 125 +++++++++++------- .../backend_tests/http_middleware_test.clj | 48 ++++++- 2 files changed, 121 insertions(+), 52 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index b0057519077..0c32042d010 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -79,60 +79,89 @@ (defn- wrap-authz [handler cfg] (fn [request] - ;; Skip when a prior middleware (session or access-token) already - ;; resolved a profile — we only act as a fallback. - (if (or (some? (::session/profile-id request)) - (some? (::actoken/profile-id request))) - (handler request) - (let [email-claim (yreq/get-header request "x-auth-request-email")] - (if (str/blank? email-claim) - (handler request) - (let [local-part (first (str/split email-claim #"@")) - email (resolve-email email-claim) - fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) - local-part) - profile (try + (let [atoken-pid (::actoken/profile-id request) + session-pid (::session/profile-id request) + email-claim (yreq/get-header request "x-auth-request-email")] + (cond + ;; Access-token (API key) — programmatic identity issued out-of-band + ;; by the user. Not a browser SSO session, so the header is not + ;; meaningful here. Pass through unconditionally. + (some? atoken-pid) + (handler request) + + ;; No proxy header — trust whatever wrap-session decided (session + ;; cookie, or anonymous). Without a header we have no upstream + ;; identity to compare against. + (str/blank? email-claim) + (handler request) + + :else + (let [local-part (first (str/split email-claim #"@")) + email (resolve-email email-claim) + fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) + local-part) + profile (try (get-or-register-profile cfg email fullname) (catch Throwable cause (l/err :hint "x-auth-request: error resolving profile" :email email :cause cause) nil))] - (cond - (nil? profile) - (do - (l/wrn :hint "x-auth-request: no profile found for email, passing through unauthenticated" - :email email) - (handler request)) - - (:is-blocked profile) - (do - (l/wrn :hint "x-auth-request: profile is blocked, denying access" - :email email - :profile-id (str (:id profile))) - {::yres/status 403}) - - (not (:is-active profile)) - (do - (l/wrn :hint "x-auth-request: profile is not active, denying access" - :email email - :profile-id (str (:id profile))) - {::yres/status 403}) - - :else - (do - (l/dbg :hint "x-auth-request: authenticating via forwarded header" - :email email - :profile-id (str (:id profile))) - (let [create-session! (session/create-fn cfg profile) - ;; Inject profile-id into the request so this very - ;; request is also treated as authenticated downstream. - response (-> request - (assoc ::session/profile-id (:id profile)) - handler)] - ;; Attach a session cookie so the browser is authenticated - ;; for all subsequent requests without needing to log in. - (create-session! request response)))))))))) + (cond + (nil? profile) + ;; Header email doesn't resolve to a profile (and auto-register + ;; is off). Pass through with whatever session wrap-session set + ;; — we don't have a profile to switch *to*. + (do + (l/wrn :hint "x-auth-request: no profile found for email, passing through unauthenticated" + :email email) + (handler request)) + + (:is-blocked profile) + (do + (l/wrn :hint "x-auth-request: profile is blocked, denying access" + :email email + :profile-id (str (:id profile))) + {::yres/status 403}) + + (not (:is-active profile)) + (do + (l/wrn :hint "x-auth-request: profile is not active, denying access" + :email email + :profile-id (str (:id profile))) + {::yres/status 403}) + + ;; Existing browser session matches the proxy-asserted identity. + ;; Steady-state case — no work to do. + (and session-pid (= session-pid (:id profile))) + (handler request) + + ;; Either no existing session, or the session points at a + ;; *different* profile than oauth2-proxy is asserting. Re-key. + ;; + ;; Re-keying is what fixes the stale-session bug after the + ;; portal "log out of all apps" + new-user login pattern: + ;; oauth2-proxy + Cognito are cleared, but Penpot's own + ;; auth-token cookie on its subdomain survives. Without this + ;; branch, wrap-session would resolve the old session-pid and + ;; this middleware (under the previous always-skip-when-session + ;; rule) would never override it. + :else + (do + (when session-pid + (l/inf :hint "x-auth-request: proxy identity differs from existing session — re-keying" + :session-profile-id (str session-pid) + :header-profile-id (str (:id profile)))) + (l/dbg :hint "x-auth-request: authenticating via forwarded header" + :email email + :profile-id (str (:id profile))) + (let [create-session! (session/create-fn cfg profile) + response (-> request + (assoc ::session/profile-id (:id profile)) + handler)] + ;; Issue a fresh auth-token cookie; replaces the stale one + ;; the browser still has (if any). + (create-session! request response))))))))) (def authz {:name ::authz diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 78174841de3..c4034b9a794 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -156,18 +156,58 @@ (handler (->DummyRequest {} {})) (t/is (nil? (::session/profile-id @captured))))) -(t/deftest x-auth-request-skips-when-session-present +(t/deftest x-auth-request-preserves-session-when-header-email-unresolvable + ;; When wrap-session has already set a profile-id, and the proxy header + ;; email cannot be resolved to a profile (unknown email + auto-register + ;; off), the existing session passes through unchanged. The middleware + ;; only re-keys when it has a *real* alternative identity to switch to. (let [profile-id (random-uuid) - called? (volatile! false) handler (#'app.http.auth-request/wrap-authz - (fn [req] (vreset! called? true) req) + (fn [req] req) (make-xauth-cfg)) request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) (assoc ::session/profile-id profile-id)) result (handler request)] - ;; profile-id must pass through unchanged — middleware must not overwrite it (t/is (= profile-id (::session/profile-id result))))) +(t/deftest x-auth-request-rekeys-when-session-identity-differs + ;; Repro of the QA-reported bug: alice's auth-token cookie persists on + ;; Penpot's subdomain after the portal "log out of all apps"; bob then + ;; logs in upstream. wrap-session resolves alice's profile-id from the + ;; old cookie, but oauth2-proxy is forwarding bob's email. The middleware + ;; must re-key to bob. + (let [alice (th/create-profile* 1) + bob (th/create-profile* 2) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice))) + response (handler request)] + ;; Downstream handler sees bob's profile-id, not alice's. + (t/is (= (:id bob) (::session/profile-id @captured))) + ;; A fresh auth-token cookie is issued for bob's session. + (t/is (contains? (::yres/cookies response) "auth-token")))) + +(t/deftest x-auth-request-no-rekey-when-session-matches-header + ;; Steady-state: the browser session matches the proxy identity. No + ;; re-key, no new cookie — the session passes through cleanly. This + ;; guards against issuing a fresh cookie on every authenticated request. + (let [profile (th/create-profile* 1) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email profile)} {}) + (assoc ::session/profile-id (:id profile))) + response (handler request)] + (t/is (= (:id profile) (::session/profile-id @captured))) + ;; No new auth-token cookie when the session already matches. + (t/is (not (contains? (::yres/cookies response) "auth-token"))))) + (t/deftest x-auth-request-skips-when-access-token-present (let [profile-id (random-uuid) handler (#'app.http.auth-request/wrap-authz From 616dd800622e37bcf094746aa336aa74e1ba3598 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:51:01 +0000 Subject: [PATCH 02/19] :bug: Fix auth-request rekey renewal overwrite Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/auth_request.clj | 5 +-- backend/src/app/http/session.clj | 5 ++- .../backend_tests/http_middleware_test.clj | 35 +++++++++++++++++-- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index 0c32042d010..084b7a29d83 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -113,8 +113,9 @@ ;; is off). Pass through with whatever session wrap-session set ;; — we don't have a profile to switch *to*. (do - (l/wrn :hint "x-auth-request: no profile found for email, passing through unauthenticated" - :email email) + (l/wrn :hint "x-auth-request: no profile found for email, preserving current auth context" + :email email + :session-profile-id (some-> session-pid str)) (handler request)) (:is-blocked profile) diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index bcbb2517a02..3b31d2b6e5d 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -248,7 +248,10 @@ (binding [ct/*clock* (clock/get-clock (:profile-id session))] (handler request))] - (if (and session (renew-session? session)) + (if (and session + (renew-session? session) + (not (contains? (::yres/cookies response) + (cf/get :auth-token-cookie-name)))) (let [session (->> session (update-session manager) (assign-token cfg))] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index c4034b9a794..811759eaa60 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -176,8 +176,8 @@ ;; logs in upstream. wrap-session resolves alice's profile-id from the ;; old cookie, but oauth2-proxy is forwarding bob's email. The middleware ;; must re-key to bob. - (let [alice (th/create-profile* 1) - bob (th/create-profile* 2) + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) captured (volatile! nil) cfg (make-xauth-cfg) handler (#'app.http.auth-request/wrap-authz @@ -195,7 +195,7 @@ ;; Steady-state: the browser session matches the proxy identity. No ;; re-key, no new cookie — the session passes through cleanly. This ;; guards against issuing a fresh cookie on every authenticated request. - (let [profile (th/create-profile* 1) + (let [profile (th/create-profile* 1 {:is-active true}) captured (volatile! nil) cfg (make-xauth-cfg) handler (#'app.http.auth-request/wrap-authz @@ -208,6 +208,35 @@ ;; No new auth-token cookie when the session already matches. (t/is (not (contains? (::yres/cookies response) "auth-token"))))) +(t/deftest x-auth-request-rekey-not-overwritten-by-session-renewal + ;; Integration guard: session/authz wraps x-auth-request and can renew the + ;; incoming cookie after inner middleware returns. Ensure bob's re-keyed + ;; cookie wins even when renewal is forced. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + cfg (make-xauth-cfg) + middleware (-> (fn [req] {::yres/status 200 + :seen-profile-id (::session/profile-id req)}) + (#'app.http.auth-request/wrap-authz cfg) + (#'session/wrap-authz cfg) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + seeded-token (get-in ((session/create-fn cfg alice) + (->DummyRequest {} {}) + {::yres/status 200}) + [::yres/cookies "auth-token" :value]) + response (with-redefs [#'session/renew-session? (constantly true)] + (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} + {"auth-token" seeded-token}))) + rekeyed-token (get-in response [::yres/cookies "auth-token" :value]) + followup ((-> identity + (#'session/wrap-authz cfg) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + (->DummyRequest {} {"auth-token" rekeyed-token}))] + (t/is (= (:id bob) (:seen-profile-id response))) + (t/is (= (:id bob) (::session/profile-id followup))))) + (t/deftest x-auth-request-skips-when-access-token-present (let [profile-id (random-uuid) handler (#'app.http.auth-request/wrap-authz From f2eab9ea351cae9039d30ffcba329685882e60d6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:51:59 +0000 Subject: [PATCH 03/19] :bug: Add renewal-path integration guard for rekey Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/test/backend_tests/http_middleware_test.clj | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 811759eaa60..2438ab09cc6 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -225,7 +225,10 @@ (->DummyRequest {} {}) {::yres/status 200}) [::yres/cookies "auth-token" :value]) - response (with-redefs [#'session/renew-session? (constantly true)] + response (binding [cf/config (assoc cf/config + :auth-token-cookie-renewal-max-age + (ct/duration {:millis 0}))] + (Thread/sleep 5) (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} {"auth-token" seeded-token}))) rekeyed-token (get-in response [::yres/cookies "auth-token" :value]) @@ -234,6 +237,8 @@ (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) :cookie (partial session/decode-token cfg)})) (->DummyRequest {} {"auth-token" rekeyed-token}))] + (t/is (some? rekeyed-token)) + (t/is (not= seeded-token rekeyed-token)) (t/is (= (:id bob) (:seen-profile-id response))) (t/is (= (:id bob) (::session/profile-id followup))))) From 8b28a68326626ec20fe7f96c041230eae120db27 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:52:57 +0000 Subject: [PATCH 04/19] :bug: Stabilize rekey renewal integration test Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- .../test/backend_tests/http_middleware_test.clj | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 2438ab09cc6..bb4ce074334 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -215,20 +215,23 @@ (let [alice (th/create-profile* 1 {:is-active true}) bob (th/create-profile* 2 {:is-active true}) cfg (make-xauth-cfg) + t0 (ct/inst "2025-01-01T00:00:00Z") + t1 (ct/plus t0 (ct/duration {:seconds 2})) middleware (-> (fn [req] {::yres/status 200 :seen-profile-id (::session/profile-id req)}) (#'app.http.auth-request/wrap-authz cfg) (#'session/wrap-authz cfg) (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) :cookie (partial session/decode-token cfg)})) - seeded-token (get-in ((session/create-fn cfg alice) - (->DummyRequest {} {}) - {::yres/status 200}) - [::yres/cookies "auth-token" :value]) + seeded-token (binding [ct/*clock* (ct/fixed-clock t0)] + (get-in ((session/create-fn cfg alice) + (->DummyRequest {} {}) + {::yres/status 200}) + [::yres/cookies "auth-token" :value])) response (binding [cf/config (assoc cf/config :auth-token-cookie-renewal-max-age - (ct/duration {:millis 0}))] - (Thread/sleep 5) + (ct/duration {:seconds 1})) + ct/*clock* (ct/fixed-clock t1)] (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} {"auth-token" seeded-token}))) rekeyed-token (get-in response [::yres/cookies "auth-token" :value]) From 98585355405d798f72a0e931fe5e851cb010a2f7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:53:40 +0000 Subject: [PATCH 05/19] :lipstick: Clarify renewal-threshold test setup Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/test/backend_tests/http_middleware_test.clj | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index bb4ce074334..dc6166b6579 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -229,6 +229,8 @@ {::yres/status 200}) [::yres/cookies "auth-token" :value])) response (binding [cf/config (assoc cf/config + ;; Renewal is triggered once elapsed age exceeds + ;; this threshold. :auth-token-cookie-renewal-max-age (ct/duration {:seconds 1})) ct/*clock* (ct/fixed-clock t1)] From eedfa49f234f21752acd2f5327e6f9138f65c41c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:54:12 +0000 Subject: [PATCH 06/19] :lipstick: Name renewal threshold in test Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/test/backend_tests/http_middleware_test.clj | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index dc6166b6579..0913d903bc5 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -217,6 +217,9 @@ cfg (make-xauth-cfg) t0 (ct/inst "2025-01-01T00:00:00Z") t1 (ct/plus t0 (ct/duration {:seconds 2})) + ;; Keep this lower than (t1 - t0) so the stale incoming session is + ;; always considered due for renewal in this test. + renewal-threshold (ct/duration {:seconds 1}) middleware (-> (fn [req] {::yres/status 200 :seen-profile-id (::session/profile-id req)}) (#'app.http.auth-request/wrap-authz cfg) @@ -232,7 +235,7 @@ ;; Renewal is triggered once elapsed age exceeds ;; this threshold. :auth-token-cookie-renewal-max-age - (ct/duration {:seconds 1})) + renewal-threshold) ct/*clock* (ct/fixed-clock t1)] (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} {"auth-token" seeded-token}))) From 696a278c6b73f3a9764c0d229855ee5537954cc4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 18:54:57 +0000 Subject: [PATCH 07/19] :lipstick: Refine renewal integration assertions Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/test/backend_tests/http_middleware_test.clj | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 0913d903bc5..428f20d9b70 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -240,15 +240,12 @@ (middleware (->DummyRequest {"x-auth-request-email" (:email bob)} {"auth-token" seeded-token}))) rekeyed-token (get-in response [::yres/cookies "auth-token" :value]) - followup ((-> identity - (#'session/wrap-authz cfg) - (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) - :cookie (partial session/decode-token cfg)})) - (->DummyRequest {} {"auth-token" rekeyed-token}))] + followup (middleware (->DummyRequest {} {"auth-token" rekeyed-token}))] + (t/is (neg? (compare renewal-threshold (ct/diff t0 t1)))) (t/is (some? rekeyed-token)) (t/is (not= seeded-token rekeyed-token)) (t/is (= (:id bob) (:seen-profile-id response))) - (t/is (= (:id bob) (::session/profile-id followup))))) + (t/is (= (:id bob) (:seen-profile-id followup))))) (t/deftest x-auth-request-skips-when-access-token-present (let [profile-id (random-uuid) From 83c31494800b86efd03d7ee65e0d49e4216f124a Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Sat, 16 May 2026 00:08:00 +0500 Subject: [PATCH 08/19] :bug: Apply Copilot review fix for proxy mismatch handling Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi --- backend/src/app/http/session.clj | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index 3b31d2b6e5d..02fc2262353 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -250,6 +250,7 @@ (if (and session (renew-session? session) + (< (get response :status 200) 400) (not (contains? (::yres/cookies response) (cf/get :auth-token-cookie-name)))) (let [session (->> session From 8901d6c6fc05657f35f71bf6e6cf840ed6dc5e89 Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 00:56:44 +0500 Subject: [PATCH 09/19] :bug: Drop local session when proxy email is unresolvable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the last open Copilot review comment on PR #18: the `(nil? profile)` branch in wrap-authz used to preserve alice's session-pid when the proxy header asserted an unknown email. That served alice's data to whoever is now upstream — the exact stale- session leak this PR was created to close. Behaviour change: when proxy header is present and resolves to no local profile (unknown user + auto-register off), the middleware now dissocs ::session/profile-id from the request before passing through. Downstream handlers respond per their own anonymous-request rules (401, redirect to login, etc.) rather than serving the previous user. Brings the nil-profile path in line with proxy-auth-middleware Rule 2 in the openspec contract: "Identity mismatch SHALL flush the existing session immediately", regardless of whether we can re-key to a known alternative identity. Test: x-auth-request-preserves-session-when-header-email-unresolvable → x-auth-request-drops-local-session-when-header-email-unresolvable Asserts the downstream handler does NOT see alice's session-pid when the header email is unresolvable. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/app/http/auth_request.clj | 15 ++++++++--- .../backend_tests/http_middleware_test.clj | 26 ++++++++++++------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index 084b7a29d83..0a39b9f14f3 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -110,13 +110,20 @@ (cond (nil? profile) ;; Header email doesn't resolve to a profile (and auto-register - ;; is off). Pass through with whatever session wrap-session set - ;; — we don't have a profile to switch *to*. + ;; is off). The upstream identity is something the local DB + ;; doesn't know — we cannot safely keep serving whatever session + ;; cookie alice happens to have in this browser, because the + ;; upstream says alice is no longer the active identity. Drop + ;; the local session-pid so the request continues unauthenticated + ;; (downstream handlers will respond with 401/redirect-to-login + ;; per their own rules) rather than as the previous user. (do - (l/wrn :hint "x-auth-request: no profile found for email, preserving current auth context" + (l/wrn :hint "x-auth-request: no profile found for email, dropping local session" :email email :session-profile-id (some-> session-pid str)) - (handler request)) + (-> request + (dissoc ::session/profile-id) + handler)) (:is-blocked profile) (do diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 428f20d9b70..664c181d93a 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -156,19 +156,27 @@ (handler (->DummyRequest {} {})) (t/is (nil? (::session/profile-id @captured))))) -(t/deftest x-auth-request-preserves-session-when-header-email-unresolvable - ;; When wrap-session has already set a profile-id, and the proxy header - ;; email cannot be resolved to a profile (unknown email + auto-register - ;; off), the existing session passes through unchanged. The middleware - ;; only re-keys when it has a *real* alternative identity to switch to. +(t/deftest x-auth-request-drops-local-session-when-header-email-unresolvable + ;; When wrap-session has resolved alice's profile-id from the local + ;; auth-token cookie, and the proxy header asserts an email that does + ;; NOT resolve to a Penpot profile (unknown upstream user + auto-register + ;; off), the middleware MUST drop alice's session-pid before calling + ;; the downstream handler. The upstream identity has changed; continuing + ;; to serve alice would leak her data to whoever is now upstream. + ;; + ;; Per openspec proxy-auth-middleware Rule 2: "Identity mismatch SHALL + ;; flush the existing session immediately", even when we cannot re-key + ;; to a known new identity. (let [profile-id (random-uuid) + captured (volatile! nil) handler (#'app.http.auth-request/wrap-authz - (fn [req] req) + (fn [req] (vreset! captured req) req) (make-xauth-cfg)) request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) - (assoc ::session/profile-id profile-id)) - result (handler request)] - (t/is (= profile-id (::session/profile-id result))))) + (assoc ::session/profile-id profile-id))] + (handler request) + ;; Downstream handler must NOT see alice's profile-id. + (t/is (nil? (::session/profile-id @captured))))) (t/deftest x-auth-request-rekeys-when-session-identity-differs ;; Repro of the QA-reported bug: alice's auth-token cookie persists on From b4d403157841a5a0cda3f0ad8de05f00640bade3 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Sat, 16 May 2026 01:05:48 +0500 Subject: [PATCH 10/19] :bug: Apply Copilot review fix for proxy mismatch handling Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi --- backend/src/app/http/auth_request.clj | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index 0a39b9f14f3..139c776d358 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -113,17 +113,26 @@ ;; is off). The upstream identity is something the local DB ;; doesn't know — we cannot safely keep serving whatever session ;; cookie alice happens to have in this browser, because the - ;; upstream says alice is no longer the active identity. Drop - ;; the local session-pid so the request continues unauthenticated - ;; (downstream handlers will respond with 401/redirect-to-login - ;; per their own rules) rather than as the previous user. + ;; upstream says alice is no longer the active identity. Clear + ;; the in-flight local session markers and expire the browser's + ;; auth-token cookie so subsequent requests cannot resurrect the + ;; stale local session. The request then continues + ;; unauthenticated (downstream handlers will respond with + ;; 401/redirect-to-login per their own rules). (do - (l/wrn :hint "x-auth-request: no profile found for email, dropping local session" + (l/wrn :hint "x-auth-request: no profile found for email, clearing local session" :email email :session-profile-id (some-> session-pid str)) - (-> request - (dissoc ::session/profile-id) - handler)) + (let [request (dissoc request + ::session/profile-id + ::session/session-id + ::session/session) + response (handler request)] + (update response :headers + (fn [headers] + (assoc (or headers {}) + "set-cookie" + "auth-token=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Path=/; HttpOnly; Secure; SameSite=Lax"))))) (:is-blocked profile) (do From cd66afc39132b26c25bf7af34072dcd7de729e1c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 20:09:01 +0000 Subject: [PATCH 11/19] :bug: Harden proxy mismatch session clearing paths Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/auth_request.clj | 21 ++++---- backend/src/app/http/session.clj | 8 +-- .../backend_tests/http_middleware_test.clj | 51 +++++++++++++++---- 3 files changed, 56 insertions(+), 24 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index 139c776d358..ae0a8da8d66 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -123,16 +123,14 @@ (l/wrn :hint "x-auth-request: no profile found for email, clearing local session" :email email :session-profile-id (some-> session-pid str)) - (let [request (dissoc request - ::session/profile-id - ::session/session-id - ::session/session) - response (handler request)] - (update response :headers - (fn [headers] - (assoc (or headers {}) - "set-cookie" - "auth-token=; Max-Age=0; Expires=Thu, 01 Jan 1970 00:00:00 GMT; Path=/; HttpOnly; Secure; SameSite=Lax"))))) + (let [delete-session! (session/delete-fn cfg) + request (dissoc request + ::session/profile-id + ::session/id + ::session/session-id + ::session/session) + response (handler request)] + (delete-session! request response))) (:is-blocked profile) (do @@ -174,6 +172,9 @@ :profile-id (str (:id profile))) (let [create-session! (session/create-fn cfg profile) response (-> request + (dissoc ::session/id + ::session/session-id + ::session/session) (assoc ::session/profile-id (:id profile)) handler)] ;; Issue a fresh auth-token cookie; replaces the stale one diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index 02fc2262353..5d8921a850c 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -249,10 +249,10 @@ (handler request))] (if (and session - (renew-session? session) - (< (get response :status 200) 400) - (not (contains? (::yres/cookies response) - (cf/get :auth-token-cookie-name)))) + (renew-session? session) + (< (or (::yres/status response) 200) 400) + (not (contains? (::yres/cookies response) + (cf/get :auth-token-cookie-name)))) (let [session (->> session (update-session manager) (assign-token cfg))] diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 664c181d93a..62184ee7dbd 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -140,6 +140,27 @@ (t/is (= (:id session) (:sid claims))) (t/is (= (:id profile) (:uid claims))))) +(t/deftest session-authz-does-not-renew-on-error-response + (let [cfg th/*system* + manager (session/inmemory-manager) + profile (th/create-profile* 91) + t0 (ct/inst "2025-01-01T00:00:00Z") + t1 (ct/plus t0 (ct/duration {:seconds 2})) + threshold (ct/duration {:seconds 1}) + handler (-> (fn [_] {::yres/status 403}) + (#'session/wrap-authz {::session/manager manager}) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + token (binding [ct/*clock* (ct/fixed-clock t0)] + (->> (session/create-session manager {:profile-id (:id profile) + :user-agent "user agent"}) + (#'session/assign-token cfg))) + response (binding [cf/config (assoc cf/config :auth-token-cookie-renewal-max-age threshold) + ct/*clock* (ct/fixed-clock t1)] + (handler (->DummyRequest {} {"auth-token" (:token token)})))] + (t/is (= 403 (::yres/status response))) + (t/is (not (contains? (::yres/cookies response) "auth-token"))))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; X-Auth-Request middleware tests ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -167,16 +188,22 @@ ;; Per openspec proxy-auth-middleware Rule 2: "Identity mismatch SHALL ;; flush the existing session immediately", even when we cannot re-key ;; to a known new identity. - (let [profile-id (random-uuid) - captured (volatile! nil) - handler (#'app.http.auth-request/wrap-authz - (fn [req] (vreset! captured req) req) - (make-xauth-cfg)) - request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) - (assoc ::session/profile-id profile-id))] - (handler request) + (let [profile-id (random-uuid) + stale-session {:id (random-uuid) :profile-id profile-id} + captured (volatile! nil) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) req) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) + (assoc ::session/profile-id profile-id) + (assoc ::session/session stale-session)) + response (handler request)] ;; Downstream handler must NOT see alice's profile-id. - (t/is (nil? (::session/profile-id @captured))))) + (t/is (nil? (::session/profile-id @captured))) + ;; Downstream handler must NOT see alice's stale local session. + (t/is (nil? (::session/session @captured))) + ;; Browser cookie is explicitly expired. + (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) (t/deftest x-auth-request-rekeys-when-session-identity-differs ;; Repro of the QA-reported bug: alice's auth-token cookie persists on @@ -192,10 +219,14 @@ (fn [req] (vreset! captured req) {::yres/status 200}) cfg) request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) - (assoc ::session/profile-id (:id alice))) + (assoc ::session/profile-id (:id alice)) + (assoc ::session/session {:id (random-uuid) + :profile-id (:id alice)})) response (handler request)] ;; Downstream handler sees bob's profile-id, not alice's. (t/is (= (:id bob) (::session/profile-id @captured))) + ;; Stale local session object is removed before downstream handling. + (t/is (nil? (::session/session @captured))) ;; A fresh auth-token cookie is issued for bob's session. (t/is (contains? (::yres/cookies response) "auth-token")))) From 9274d25892739b770aa1338b4c7395bde4610aa2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 20:09:38 +0000 Subject: [PATCH 12/19] :lipstick: Remove unused session key dissoc Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/auth_request.clj | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index ae0a8da8d66..013d3363394 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -126,7 +126,6 @@ (let [delete-session! (session/delete-fn cfg) request (dissoc request ::session/profile-id - ::session/id ::session/session-id ::session/session) response (handler request)] @@ -172,8 +171,7 @@ :profile-id (str (:id profile))) (let [create-session! (session/create-fn cfg profile) response (-> request - (dissoc ::session/id - ::session/session-id + (dissoc ::session/session-id ::session/session) (assoc ::session/profile-id (:id profile)) handler)] From a3d95c006f3055f9cab235f613f745ea117687ec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 15 May 2026 20:10:29 +0000 Subject: [PATCH 13/19] :lipstick: Tighten renewal status guard semantics Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/session.clj | 21 ++++++++++--------- .../backend_tests/http_middleware_test.clj | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index 5d8921a850c..1fc1614f780 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -248,16 +248,17 @@ (binding [ct/*clock* (clock/get-clock (:profile-id session))] (handler request))] - (if (and session - (renew-session? session) - (< (or (::yres/status response) 200) 400) - (not (contains? (::yres/cookies response) - (cf/get :auth-token-cookie-name)))) - (let [session (->> session - (update-session manager) - (assign-token cfg))] - (assign-session-cookie response session)) - response)) + (let [status (::yres/status response)] + (if (and session + (renew-session? session) + (or (nil? status) (< status 400)) + (not (contains? (::yres/cookies response) + (cf/get :auth-token-cookie-name)))) + (let [session (->> session + (update-session manager) + (assign-token cfg))] + (assign-session-cookie response session)) + response))) (= type :bearer) (let [session (case (:ver metadata) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 62184ee7dbd..198332aa9fc 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -147,7 +147,7 @@ t0 (ct/inst "2025-01-01T00:00:00Z") t1 (ct/plus t0 (ct/duration {:seconds 2})) threshold (ct/duration {:seconds 1}) - handler (-> (fn [_] {::yres/status 403}) + handler (-> (fn [_req] {::yres/status 403}) (#'session/wrap-authz {::session/manager manager}) (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) :cookie (partial session/decode-token cfg)})) From fd2f32e928228fbdd3486d40a9296c375b67d30c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 12:51:57 +0000 Subject: [PATCH 14/19] :bug: Preserve local session on profile lookup errors Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/b389792f-d18f-4b49-9241-455c18e728c0 Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/auth_request.clj | 32 ++++++++++++------- .../backend_tests/http_middleware_test.clj | 20 ++++++++++++ 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index 013d3363394..dfbf9fa29f7 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -96,18 +96,28 @@ (handler request) :else - (let [local-part (first (str/split email-claim #"@")) - email (resolve-email email-claim) - fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) - local-part) - profile (try - (get-or-register-profile cfg email fullname) - (catch Throwable cause - (l/err :hint "x-auth-request: error resolving profile" - :email email - :cause cause) - nil))] + (let [local-part (first (str/split email-claim #"@")) + email (resolve-email email-claim) + fullname (or (not-empty (yreq/get-header request "x-auth-request-user")) + local-part) + profile-state (try + {:status :ok + :profile (get-or-register-profile cfg email fullname)} + (catch Throwable cause + (l/err :hint "x-auth-request: error resolving profile" + :email email + :cause cause) + {:status :error + :cause cause})) + profile (:profile profile-state)] (cond + (= :error (:status profile-state)) + (do + (l/wrn :hint "x-auth-request: preserving local auth state because profile resolution failed" + :email email + :session-profile-id (some-> session-pid str)) + (handler request)) + (nil? profile) ;; Header email doesn't resolve to a profile (and auto-register ;; is off). The upstream identity is something the local DB diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 198332aa9fc..0e044159ad4 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -205,6 +205,26 @@ ;; Browser cookie is explicitly expired. (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) +(t/deftest x-auth-request-preserves-local-session-when-profile-lookup-errors + (let [profile-id (random-uuid) + stale-session {:id (random-uuid) :profile-id profile-id} + captured (volatile! nil) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) req) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) + (assoc ::session/profile-id profile-id) + (assoc ::session/session stale-session)) + response (with-redefs [app.http.auth-request/get-or-register-profile + (fn [& _] + (throw (ex-info "db down" {})))] + (handler request))] + ;; Operational errors should not be treated as "unknown user" and + ;; destructively clear local session state. + (t/is (= profile-id (::session/profile-id @captured))) + (t/is (= stale-session (::session/session @captured))) + (t/is (not (contains? (::yres/cookies response) "auth-token"))))) + (t/deftest x-auth-request-rekeys-when-session-identity-differs ;; Repro of the QA-reported bug: alice's auth-token cookie persists on ;; Penpot's subdomain after the portal "log out of all apps"; bob then From f1f1a6f9e6459d41c9b326c2efc7b02665ec8b5d Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 17:59:06 +0500 Subject: [PATCH 15/19] :bug: Address review findings on re-key boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five related findings from Copilot review of the proxy-identity re-key path, all stemming from the same root: ::http/auth-data and the local session cookie were not being kept consistent with ::session/profile-id across the re-key / flush boundary, and edge cases (transient lookup failure, logout response, blocked-with-mismatch) had gaps. 1. Profile-lookup exceptions are now distinguished from "no profile found" via the ::lookup-error sentinel. A Postgres blip used to be collapsed to nil and hit the "unknown user" branch, flushing the in-flight session of every authenticated user during the outage. Now: 503 on transient failure, session state untouched. 2. The unresolvable-email branch now dissocs ::http/auth-data alongside the session keys via a new clear-stale-session helper. Without this, downstream readers (errors.clj, access_token.clj) that consult ::http/auth-data.claims.uid directly would still see the old profile-id after the rest of the middleware thought it had flushed. 3. Blocked / inactive incoming identity now flushes the existing mismatched session before returning 403. Per the openspec contract (proxy-auth-middleware Rule 2: "Identity mismatch SHALL flush"), alice's local session should not survive an attempted switch to a refused user — otherwise her auth-token cookie outlives the intended denial and her next request still serves her. 4. The re-key branch also routes through clear-stale-session, so the downstream handler sees a clean request with the new profile-id and no leftover claims from the previous identity. 5. The unconditional create-session! after the re-keyed handler is now guarded by response-has-auth-cookie?. If the handler already wrote the auth-token cookie (an explicit logout that cleared it, or an auth endpoint that re-issued it), we step aside rather than clobber the handler's intent — same shape as the renewal guard in session.clj. Tests: - x-auth-request-503-on-transient-lookup-error - x-auth-request-rekey-clears-stale-auth-data - x-auth-request-unresolvable-email-clears-stale-auth-data - x-auth-request-blocked-incoming-clears-existing-mismatched-session - x-auth-request-rekey-does-not-overwrite-handler-cookie The previously-passing tests (steady-state pass-through, unresolvable email drops session, basic re-key, blocked/inactive 403 without existing session, renewal-overwrite integration guard) still pass unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/app/http/auth_request.clj | 102 ++++++++++++---- .../backend_tests/http_middleware_test.clj | 115 ++++++++++++++++++ 2 files changed, 192 insertions(+), 25 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index dfbf9fa29f7..b313356e188 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -20,6 +20,7 @@ [app.common.logging :as l] [app.config :as cf] [app.db :as db] + [app.http :as-alias http] [app.http.access-token :as-alias actoken] [app.http.session :as session] [app.rpc.commands.auth :as auth] @@ -76,6 +77,37 @@ ;; MIDDLEWARE ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; The profile-lookup try/catch (below) returns a map `{:status …}` so +;; the caller can distinguish a transient DB/IO failure from a clean +;; "no profile for this email". Collapsing both to `nil` would let a +;; Postgres blip silently flush an otherwise-valid session. On +;; transient failure we preserve the local session and pass through +;; unchanged — graceful degradation rather than fail-closed 503. + +(defn- clear-stale-session + "Returns a request with the local-session keys AND ::http/auth-data + removed. Both must be cleared together because downstream handlers + read identity from BOTH `::session/profile-id` (the post-wrap-authz + shorthand) and `::http/auth-data.claims.uid` (the raw decoded cookie + claims, used by errors.clj and access_token.clj). Leaving auth-data + in place after re-keying or after a mismatch flush leaks the stale + identity to those readers." + [request] + (dissoc request + ::session/profile-id + ::session/session-id + ::session/session + ::http/auth-data)) + +(defn- response-has-auth-cookie? + "True when the handler already wrote the auth-token cookie on the + response. Used to avoid clobbering an explicit logout / re-issue + written by a downstream handler with a fresh re-key cookie. Same + guard shape as session.clj's renewal path." + [response] + (contains? (::yres/cookies response) + (cf/get :auth-token-cookie-name))) + (defn- wrap-authz [handler cfg] (fn [request] @@ -111,6 +143,11 @@ :cause cause})) profile (:profile profile-state)] (cond + ;; Transient failure resolving the proxy identity. We have NO + ;; basis to flush the existing session (a Postgres restart or + ;; network blip would otherwise log everyone out), so preserve + ;; whatever wrap-session decided. The next successful request + ;; reconciles normally. (= :error (:status profile-state)) (do (l/wrn :hint "x-auth-request: preserving local auth state because profile resolution failed" @@ -124,36 +161,44 @@ ;; doesn't know — we cannot safely keep serving whatever session ;; cookie alice happens to have in this browser, because the ;; upstream says alice is no longer the active identity. Clear - ;; the in-flight local session markers and expire the browser's - ;; auth-token cookie so subsequent requests cannot resurrect the - ;; stale local session. The request then continues - ;; unauthenticated (downstream handlers will respond with - ;; 401/redirect-to-login per their own rules). + ;; the in-flight local session markers AND auth-data, then + ;; expire the browser's auth-token cookie so subsequent + ;; requests cannot resurrect the stale local session. The + ;; request then continues unauthenticated (downstream handlers + ;; will respond with 401/redirect-to-login per their own rules). (do (l/wrn :hint "x-auth-request: no profile found for email, clearing local session" :email email :session-profile-id (some-> session-pid str)) (let [delete-session! (session/delete-fn cfg) - request (dissoc request - ::session/profile-id - ::session/session-id - ::session/session) + request (clear-stale-session request) response (handler request)] (delete-session! request response))) - (:is-blocked profile) + ;; Blocked / inactive incoming identity. Return 403 — and if + ;; there was a session for a DIFFERENT user, flush it too. The + ;; openspec Rule 2 contract (identity mismatch SHALL flush) + ;; applies regardless of whether the new identity is usable; + ;; otherwise alice's session survives an attempted switch to + ;; the blocked user bob and the next request still serves alice. + (or (:is-blocked profile) + (not (:is-active profile))) (do - (l/wrn :hint "x-auth-request: profile is blocked, denying access" + (l/wrn :hint (if (:is-blocked profile) + "x-auth-request: profile is blocked, denying access" + "x-auth-request: profile is not active, denying access") :email email - :profile-id (str (:id profile))) - {::yres/status 403}) - - (not (:is-active profile)) - (do - (l/wrn :hint "x-auth-request: profile is not active, denying access" - :email email - :profile-id (str (:id profile))) - {::yres/status 403}) + :profile-id (str (:id profile)) + :session-profile-id (some-> session-pid str)) + (let [response {::yres/status 403}] + (if (and session-pid (not= session-pid (:id profile))) + ;; Identity mismatch with a refused incoming user — also + ;; clear the existing local session cookie so alice + ;; doesn't keep her session via the still-valid + ;; auth-token cookie on her browser. + (let [delete-session! (session/delete-fn cfg)] + (delete-session! request response)) + response))) ;; Existing browser session matches the proxy-asserted identity. ;; Steady-state case — no work to do. @@ -181,13 +226,20 @@ :profile-id (str (:id profile))) (let [create-session! (session/create-fn cfg profile) response (-> request - (dissoc ::session/session-id - ::session/session) + clear-stale-session (assoc ::session/profile-id (:id profile)) handler)] - ;; Issue a fresh auth-token cookie; replaces the stale one - ;; the browser still has (if any). - (create-session! request response))))))))) + ;; Issue a fresh auth-token cookie unless the downstream + ;; handler already wrote the auth-token cookie on the + ;; response (e.g. an explicit logout that called + ;; clear-session-cookie, or an auth endpoint that re-issued + ;; the cookie itself). Otherwise the re-key would clobber + ;; the handler's intent — most visibly, it would prevent a + ;; user from ever completing a logout while the SSO + ;; cookie is still present. + (if (response-has-auth-cookie? response) + response + (create-session! request response)))))))))) (def authz {:name ::authz diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 0e044159ad4..3fbf27d524f 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -354,6 +354,121 @@ (handler (->DummyRequest {"x-auth-request-email" "nobody@example.com"} {})) (t/is (nil? (::session/profile-id @captured))))) +(t/deftest x-auth-request-preserves-session-on-transient-lookup-error + ;; A Postgres blip / network timeout while resolving the header email's + ;; profile MUST NOT be conflated with "email doesn't resolve". The + ;; former is transient and the local session should survive; the + ;; latter is an identity mismatch and the session should flush. + ;; Without this guard, a single DB error would silently log every + ;; SSO user out. + ;; + ;; Graceful degradation: on transient failure pass through with + ;; whatever wrap-session decided. Downstream gets alice's existing + ;; session-pid (or anonymous if none), and the next successful + ;; request reconciles normally. + (with-mocks [_ {:target 'app.http.auth-request/get-or-register-profile + :return (fn [& _] (throw (ex-info "db boom" {})))}] + (let [captured (volatile! nil) + profile-id (random-uuid) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) + (assoc ::session/profile-id profile-id)) + response (handler request)] + ;; Downstream handler ran with alice's session intact. + (t/is (= profile-id (::session/profile-id @captured))) + (t/is (= 200 (::yres/status response))) + ;; No cookie clear / no fresh cookie issued — session untouched. + (t/is (not (contains? (::yres/cookies response) "auth-token")))))) + +(t/deftest x-auth-request-rekey-clears-stale-auth-data + ;; The re-keyed request must have ::http/auth-data removed alongside + ;; the session keys. Without this, downstream code that reads + ;; ::http/auth-data.claims.uid (errors.clj, access_token.clj) would + ;; still see alice's profile-id after we re-keyed to bob, leaking the + ;; stale identity across the boundary the rest of the middleware + ;; thinks it has crossed. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + captured (volatile! nil) + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice)) + (assoc ::http/auth-data {:type :cookie + :claims {:uid (:id alice) + :sid (random-uuid)}}))] + (handler request) + (t/is (= (:id bob) (::session/profile-id @captured))) + (t/is (nil? (::http/auth-data @captured))))) + +(t/deftest x-auth-request-unresolvable-email-clears-stale-auth-data + ;; Same invariant on the unresolvable-email branch: ::http/auth-data + ;; must be removed so downstream readers don't see alice's stale + ;; claims when we've explicitly decided to flush her session. + (let [profile-id (random-uuid) + captured (volatile! nil) + handler (#'app.http.auth-request/wrap-authz + (fn [req] (vreset! captured req) {::yres/status 200}) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" "ghost@example.com"} {}) + (assoc ::session/profile-id profile-id) + (assoc ::http/auth-data {:type :cookie + :claims {:uid profile-id}}))] + (handler request) + (t/is (nil? (::http/auth-data @captured))) + (t/is (nil? (::session/profile-id @captured))))) + +(t/deftest x-auth-request-blocked-incoming-clears-existing-mismatched-session + ;; openspec proxy-auth-middleware Rule 2: "Identity mismatch SHALL + ;; flush". When alice is logged in locally and oauth2-proxy forwards + ;; bob's email but bob's profile is blocked, we still 403 — and we + ;; still flush alice's session, because the upstream identity has + ;; changed regardless of whether the new identity is usable. + ;; Otherwise alice's auth-token cookie stays valid and her next + ;; request still serves her, defeating the flush guarantee. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + _ (th/db-update! :profile {:is-blocked true} {:id (:id bob)}) + handler (#'app.http.auth-request/wrap-authz + (fn [_] {::yres/status 200}) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice)) + (assoc ::session/session {:id (random-uuid) + :profile-id (:id alice)})) + response (handler request)] + (t/is (= 403 (::yres/status response))) + (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) + +(t/deftest x-auth-request-rekey-does-not-overwrite-handler-cookie + ;; If the downstream handler already wrote the auth-token cookie on + ;; the response (e.g. an explicit logout that called + ;; session/clear-session-cookie), the re-key path MUST NOT clobber + ;; that cookie with a fresh one. Otherwise a logout from a + ;; re-key-eligible session would be silently undone. + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + cookie-name (cf/get :auth-token-cookie-name) + logout-cookie {:path "/" :value "" :max-age 0} + handler-resp {::yres/status 200 + ::yres/cookies {cookie-name logout-cookie}} + cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [_] handler-resp) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice))) + response (handler request)] + ;; The handler's logout cookie is preserved — the middleware + ;; recognised the response already carried an auth-token cookie and + ;; stepped aside. + (t/is (= logout-cookie (get-in response [::yres/cookies cookie-name]))))) + (t/deftest x-auth-request-auto-register-creates-active-profile (binding [cf/flags (conj cf/flags :x-auth-request-auto-register)] (let [email "newuser@example.com" From 7f4e36c52c32b61a7af0108636817ddfacb0e7ab Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 18:24:18 +0500 Subject: [PATCH 16/19] :bug: Address follow-up review on the re-key fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three concrete fixes on top of f1f1a6f9e: 1. clear-stale-session docstring no longer overstates downstream readers. access_token.clj reads ::http/auth-data but only branches on :type :token; the cookie-auth path this middleware reconciles skips it via the access-token short-circuit, so only errors.clj is a real reader. 2. Removed the dead `::session/session-id` dissoc. wrap-session stores the resolved session at ::session, never at ::session/session-id — nothing in backend/src/app/http/ ever sets that key. Cleanup, no behavior change. 3. Narrowed the lookup-error transient catch from `Throwable` to a known-transient set: SQLException, SQLTransientException, SQLNonTransientConnectionException, IOException, PSQLException. Walks the cause chain so wrapped ExecutionException(SQLException) still classifies. Non-transient exceptions (validation failures, programming bugs, NPEs) now propagate to the standard 500 handler instead of being silently swallowed as "preserve session" — which would otherwise mask real bugs behind quiet pass-throughs. Tests: - Updated x-auth-request-preserves-session-on-transient-lookup-error to throw java.sql.SQLException (the narrowed transient class) - New x-auth-request-rethrows-non-transient-lookup-error pins the rethrow behavior for ex-info / validation-class exceptions Separately, lines 176/204 (delete-fn does not delete server-side state because nothing in the codebase sets the ::id key wrap-session expects) are noted as a pre-existing Penpot issue, not introduced by this PR. Surfacing as a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/app/http/auth_request.clj | 66 ++++++++++++++----- .../backend_tests/http_middleware_test.clj | 19 +++++- 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index b313356e188..f3a4094a41e 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -77,25 +77,51 @@ ;; MIDDLEWARE ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; The profile-lookup try/catch (below) returns a map `{:status …}` so -;; the caller can distinguish a transient DB/IO failure from a clean -;; "no profile for this email". Collapsing both to `nil` would let a -;; Postgres blip silently flush an otherwise-valid session. On -;; transient failure we preserve the local session and pass through -;; unchanged — graceful degradation rather than fail-closed 503. +;; The profile-lookup try/catch (below) narrows what counts as +;; "transient" to known DB/IO failure classes — Postgres exceptions, +;; JDBC SQL exceptions, JDBC connection exceptions, java.io.IOException. +;; A broader `catch Throwable` would also swallow validation errors, +;; bad-input parses, and programming bugs, masking them as transient +;; failures that silently extend a stale session. On transient +;; failure we preserve whatever wrap-session decided and pass through +;; unchanged — graceful degradation, but only for the failure modes +;; that genuinely warrant it. Any other exception escapes the catch +;; and surfaces through the standard error handler as a 500. + +(def ^:private transient-exception-classes + #{java.sql.SQLException + java.sql.SQLTransientException + java.sql.SQLNonTransientConnectionException + java.io.IOException + org.postgresql.util.PSQLException}) + +(defn- transient-exception? + "True iff cause (or any cause-chain ancestor) is an instance of a + class we treat as transient. Walks the cause chain so a wrapped + ExecutionException with a SQLException root still classifies." + [cause] + (loop [ex cause] + (cond + (nil? ex) false + (some #(instance? % ex) transient-exception-classes) true + :else (recur (.getCause ^Throwable ex))))) (defn- clear-stale-session "Returns a request with the local-session keys AND ::http/auth-data removed. Both must be cleared together because downstream handlers read identity from BOTH `::session/profile-id` (the post-wrap-authz - shorthand) and `::http/auth-data.claims.uid` (the raw decoded cookie - claims, used by errors.clj and access_token.clj). Leaving auth-data - in place after re-keying or after a mismatch flush leaks the stale - identity to those readers." + shorthand) and `::http/auth-data.claims.uid` (used by errors.clj for + log enrichment on cookie-auth requests). Leaving auth-data in place + after re-keying or after a mismatch flush leaks the stale identity + to that reader. + + (access_token.clj reads ::http/auth-data too, but only branches on + `:type :token` — the API-key path is short-circuited before we get + here, so it's not relevant to the cookie-auth flow this middleware + reconciles.)" [request] (dissoc request ::session/profile-id - ::session/session-id ::session/session ::http/auth-data)) @@ -136,11 +162,19 @@ {:status :ok :profile (get-or-register-profile cfg email fullname)} (catch Throwable cause - (l/err :hint "x-auth-request: error resolving profile" - :email email - :cause cause) - {:status :error - :cause cause})) + (if (transient-exception? cause) + (do + (l/err :hint "x-auth-request: transient error resolving profile, preserving session" + :email email + :cause cause) + {:status :error + :cause cause}) + ;; Non-transient — programming bug, validation + ;; failure, bad input. Rethrow so the standard + ;; error pipeline returns 500 instead of + ;; silently masking it as a "preserve session" + ;; pass-through. + (throw cause)))) profile (:profile profile-state)] (cond ;; Transient failure resolving the proxy identity. We have NO diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 3fbf27d524f..3e9f4d286c2 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -367,7 +367,7 @@ ;; session-pid (or anonymous if none), and the next successful ;; request reconciles normally. (with-mocks [_ {:target 'app.http.auth-request/get-or-register-profile - :return (fn [& _] (throw (ex-info "db boom" {})))}] + :return (fn [& _] (throw (java.sql.SQLException. "db boom")))}] (let [captured (volatile! nil) profile-id (random-uuid) cfg (make-xauth-cfg) @@ -383,6 +383,23 @@ ;; No cookie clear / no fresh cookie issued — session untouched. (t/is (not (contains? (::yres/cookies response) "auth-token")))))) +(t/deftest x-auth-request-rethrows-non-transient-lookup-error + ;; A genuine bug (validation failure, NPE, ExceptionInfo from input + ;; validation, etc.) MUST NOT be silently absorbed as "transient". A + ;; broad catch Throwable would mask programming errors as a quiet + ;; "preserve session" pass-through and the bug would never surface. + ;; Narrow the transient-exception classification to DB/IO failures + ;; only; rethrow everything else so the standard 500 path runs. + (with-mocks [_ {:target 'app.http.auth-request/get-or-register-profile + :return (fn [& _] (throw (ex-info "bad input" {:type :validation})))}] + (let [cfg (make-xauth-cfg) + handler (#'app.http.auth-request/wrap-authz + (fn [_] {::yres/status 200}) + cfg) + request (-> (->DummyRequest {"x-auth-request-email" "user@example.com"} {}) + (assoc ::session/profile-id (random-uuid)))] + (t/is (thrown? clojure.lang.ExceptionInfo (handler request)))))) + (t/deftest x-auth-request-rekey-clears-stale-auth-data ;; The re-keyed request must have ::http/auth-data removed alongside ;; the session keys. Without this, downstream code that reads From c95d6d34678263ddf0000fd030bc73a897416843 Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 18:30:28 +0500 Subject: [PATCH 17/19] :bug: Pass original request to delete-fn so ::id survives the clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clear-stale-session sanitises the request for the downstream handler, but session/delete-fn reads ::id from the request to remove the server-side session row. Routing the cleared request to BOTH the handler and delete-fn meant delete-fn always saw a request with no ::id and silently fell through to "cookie clear only" — server-side state was left intact. Pass the original (pre-clear) request to delete-fn; the handler still gets the sanitised version. No functional change today because the upstream session/wrap-authz doesn't yet set ::id either (separate follow-up issue), but this is the correct shape for when that fix lands. Co-Authored-By: Claude Opus 4.7 (1M context) --- backend/src/app/http/auth_request.clj | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index f3a4094a41e..ae0b34fc56d 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -205,8 +205,14 @@ :email email :session-profile-id (some-> session-pid str)) (let [delete-session! (session/delete-fn cfg) - request (clear-stale-session request) - response (handler request)] + cleared (clear-stale-session request) + response (handler cleared)] + ;; delete-fn reads ::id from the request — pass the + ;; original (pre-clear) so the server-side row is + ;; actually removed, not just the browser cookie. + ;; Until the companion wrap-authz fix sets ::id, the + ;; server-side delete is a no-op; this is still the + ;; correct shape for when that lands. (delete-session! request response))) ;; Blocked / inactive incoming identity. Return 403 — and if From 7eeec35bd50b4bdeb2fc13ef02658237e72912d8 Mon Sep 17 00:00:00 2001 From: Awais Qureshi Date: Sun, 17 May 2026 00:53:23 +0500 Subject: [PATCH 18/19] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi --- backend/test/backend_tests/http_middleware_test.clj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 3e9f4d286c2..84d6a6b956d 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -217,7 +217,7 @@ (assoc ::session/session stale-session)) response (with-redefs [app.http.auth-request/get-or-register-profile (fn [& _] - (throw (ex-info "db down" {})))] + (throw (java.sql.SQLException. "db down")))] (handler request))] ;; Operational errors should not be treated as "unknown user" and ;; destructively clear local session state. From 3130d1c4e6688ce2c729eb3a803ac67b3ad31abd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 19:55:52 +0000 Subject: [PATCH 19/19] :bug: Pass resolved session id to delete-fn during auth-request flush Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/bedf497a-3ef2-4e74-899c-886ba1a7c532 Co-authored-by: awais786 <445320+awais786@users.noreply.github.com> --- backend/src/app/http/auth_request.clj | 25 ++++++----- .../backend_tests/http_middleware_test.clj | 43 +++++++++++++++++++ 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/backend/src/app/http/auth_request.clj b/backend/src/app/http/auth_request.clj index ae0b34fc56d..a77be7d84f8 100644 --- a/backend/src/app/http/auth_request.clj +++ b/backend/src/app/http/auth_request.clj @@ -125,6 +125,14 @@ ::session/session ::http/auth-data)) +(defn- with-session-id + "Attach ::session/id from the decoded local session map when present so + session/delete-fn can remove the backing server-side row." + [request] + (if-let [sid (some-> request ::session/session :id)] + (assoc request ::session/id sid) + request)) + (defn- response-has-auth-cookie? "True when the handler already wrote the auth-token cookie on the response. Used to avoid clobbering an explicit logout / re-issue @@ -205,15 +213,12 @@ :email email :session-profile-id (some-> session-pid str)) (let [delete-session! (session/delete-fn cfg) - cleared (clear-stale-session request) - response (handler cleared)] - ;; delete-fn reads ::id from the request — pass the - ;; original (pre-clear) so the server-side row is - ;; actually removed, not just the browser cookie. - ;; Until the companion wrap-authz fix sets ::id, the - ;; server-side delete is a no-op; this is still the - ;; correct shape for when that lands. - (delete-session! request response))) + cleared (clear-stale-session request) + response (handler cleared)] + ;; delete-fn reads ::session/id from the request. Preserve + ;; the original request shape for deletion and project id + ;; from the decoded ::session/session map when available. + (delete-session! (with-session-id request) response))) ;; Blocked / inactive incoming identity. Return 403 — and if ;; there was a session for a DIFFERENT user, flush it too. The @@ -237,7 +242,7 @@ ;; doesn't keep her session via the still-valid ;; auth-token cookie on her browser. (let [delete-session! (session/delete-fn cfg)] - (delete-session! request response)) + (delete-session! (with-session-id request) response)) response))) ;; Existing browser session matches the proxy-asserted identity. diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index 84d6a6b956d..0aa40bc692c 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -440,6 +440,26 @@ (t/is (nil? (::http/auth-data @captured))) (t/is (nil? (::session/profile-id @captured))))) +(t/deftest x-auth-request-unresolvable-email-passes-session-id-to-delete-fn + (let [profile-id (random-uuid) + session-id (random-uuid) + deleted-req (volatile! nil) + handler (#'app.http.auth-request/wrap-authz + (fn [_req] {::yres/status 200}) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" "ghost@example.com"} {}) + (assoc ::session/profile-id profile-id) + (assoc ::session/session {:id session-id + :profile-id profile-id})) + response (with-mocks [_ {:target 'app.http.session/delete-fn + :return (fn [_cfg] + (fn [req resp] + (vreset! deleted-req req) + (session/clear-session-cookie resp)))}] + (handler request))] + (t/is (= session-id (::session/id @deleted-req))) + (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) + (t/deftest x-auth-request-blocked-incoming-clears-existing-mismatched-session ;; openspec proxy-auth-middleware Rule 2: "Identity mismatch SHALL ;; flush". When alice is logged in locally and oauth2-proxy forwards @@ -462,6 +482,29 @@ (t/is (= 403 (::yres/status response))) (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) +(t/deftest x-auth-request-blocked-incoming-passes-session-id-to-delete-fn + (let [alice (th/create-profile* 1 {:is-active true}) + bob (th/create-profile* 2 {:is-active true}) + session-id (random-uuid) + deleted-req (volatile! nil) + _ (th/db-update! :profile {:is-blocked true} {:id (:id bob)}) + handler (#'app.http.auth-request/wrap-authz + (fn [_] {::yres/status 200}) + (make-xauth-cfg)) + request (-> (->DummyRequest {"x-auth-request-email" (:email bob)} {}) + (assoc ::session/profile-id (:id alice)) + (assoc ::session/session {:id session-id + :profile-id (:id alice)})) + response (with-mocks [_ {:target 'app.http.session/delete-fn + :return (fn [_cfg] + (fn [req resp] + (vreset! deleted-req req) + (session/clear-session-cookie resp)))}] + (handler request))] + (t/is (= 403 (::yres/status response))) + (t/is (= session-id (::session/id @deleted-req))) + (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age]))))) + (t/deftest x-auth-request-rekey-does-not-overwrite-handler-cookie ;; If the downstream handler already wrote the auth-token cookie on ;; the response (e.g. an explicit logout that called