🐛 Re-key session when X-Auth-Request identity changes

[awais786]

Summary

• wrap-authz (X-Auth-Request middleware) used to bail the moment wrap-session had set ::session/profile-id. Once Penpot's own auth-token cookie was issued, the upstream X-Auth-Request-Email was no longer consulted.
• After a portal "Log out of all apps" + login as a different user — which clears the shared _oauth2_proxy cookie and Cognito SSO session but not Penpot's auth-token cookie on paint.<domain> — refreshing the Penpot tab kept serving the previous user.
• Fix: when the proxy header is present, always resolve the header email's profile and reconcile against the session's profile-id.
  • Match → pass through unchanged (steady state).
  • Mismatch with a resolvable upstream identity → re-key: drop the stale session keys, inject the new profile-id, and issue a fresh auth-token cookie on the response, replacing the stale browser cookie.
  • Mismatch where the upstream email does NOT resolve to a local profile (unknown user + auto-register off) → clear the in-flight local session markers AND expire the browser's auth-token cookie via session/delete-fn. The request continues unauthenticated (downstream handlers respond with 401/redirect-to-login per their own rules) rather than as the previous user.
  • Mismatch with a blocked or inactive incoming profile → 403; the renewal guard in session.clj ensures the stale session cookie is NOT renewed on the denial response.
• API-key (::actoken/profile-id) auth is unaffected — programmatic identity is issued out-of-band by the user and not subject to browser SSO state.

Repro (before this PR)

1. Log in to the FOSS portal as user A.
2. Open Penpot (paint.<domain>) in a tab.
3. On the portal, click Log out of all apps.
4. Log in as user B via password.
5. Refresh the Penpot tab → still shows user A.

Same architectural class as Pressingly/plane#29 and Pressingly/outline#19 — native session cookie that outlives upstream auth state. SurfSense doesn't exhibit it because FastAPI re-derives identity from headers on every request.

Session-renewal guard (session.clj)

The outer session/authz middleware renews the incoming cookie when the session is due for renewal. Without an additional guard it would overwrite wrap-authz's freshly-issued re-key cookie (or extend the stale session on a 4xx denial). The guard now requires all of:

• session present
• renew-session? is true
• response status is success-shaped — (or (nil? status) (< status 400)) against qualified ::yres/status
• response does NOT already carry an auth-token cookie

If any one is false, renewal is skipped. The combination prevents both the re-key-overwrite and the denial-extends-stale paths.

Test plan

• clj -X:test :nses '[backend-tests.http-middleware-test]' passes — new cases:
  • x-auth-request-rekeys-when-session-identity-differs — alice's session-pid + bob's email header → captured request sees bob's pid, response carries a new auth-token cookie
  • x-auth-request-no-rekey-when-session-matches-header — steady-state guard (no new cookie when session already matches)
  • x-auth-request-drops-local-session-when-header-email-unresolvable — alice's session-pid + unknown header email (auto-register off) → captured request has no ::session/profile-id, response expires the auth-token cookie via session/delete-fn. Replaces the earlier preserves-session-… test; the new invariant is "drop & expire," not "preserve."
  • x-auth-request-rekey-not-overwritten-by-session-renewal — integration through session/authz + wrap-auth, with a renewal-threshold-due seeded cookie; asserts the re-keyed token survives outer renewal
  • session-authz-does-not-renew-on-error-response — the status guard exercised against a 403 response
• Existing tests still pass (auto-register, blocked, inactive, access-token-passthrough)
• Manual: rebuild Penpot backend, run the QA's exact repro — refreshing the Penpot tab as user B now serves user B (new auth-token cookie is set on the same response)

Trade-off

Adds one DB lookup per authenticated SSO request (resolving the header email's profile). The previous fast-path for the steady-state case is restored implicitly via the match branch (handler runs without re-keying); only the lookup itself is added. Worth it for correctness; revisit with a short-lived email→pid cache if it shows up in profiling.

Out of scope

This addresses Penpot only. Plane has the analogous fix in Pressingly/plane#29, Outline in Pressingly/outline#19, Twenty PR forthcoming.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR updates the X-Auth-Request authentication middleware so proxy-provided identity can re-key a stale Penpot browser session when the upstream SSO user changes.

Changes:

• Reworked wrap-authz to resolve the proxy email even when a session profile already exists.
• Added re-key behavior when the proxy profile differs from the current session profile.
• Updated and expanded middleware tests for re-key, no-rekey, and unresolved-header cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
backend/src/app/http/auth_request.clj	Adds proxy-vs-session identity comparison and re-keying behavior.
backend/test/backend_tests/http_middleware_test.clj	Updates existing X-Auth-Request tests and adds mismatch/match session scenarios.

Comments suppressed due to low confidence (2)

backend/test/backend_tests/http_middleware_test.clj:198

• This profile is inactive by default, so wrap-authz returns 403 before reaching the session-match branch and captured remains nil. Mark the fixture profile active so this steady-state test actually verifies the no-rekey path.

  (let [profile  (th/create-profile* 1)

backend/src/app/http/auth_request.clj:161

• This only replaces ::session/profile-id; the request still carries the old ::session/session from session/authz. Downstream code that reads the session object (for example password change invalidates other sessions via session/get-session) will operate on Alice's stale session while ::session/profile-id says Bob, which can invalidate the wrong user's sessions or skip Bob's session invalidation.

                    response        (-> request
                                        (assoc ::session/profile-id (:id profile))
                                        handler)]

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[awais786]

Pressingly/plane#29 @copilot use this PR also for reference.

[Copilot]

Pressingly/plane#29 @copilot use this PR also for reference.

Used Pressingly/plane#29 as reference and aligned the Penpot fix accordingly in e850a4f: re-key now remains authoritative even when outer session renewal runs, plus middleware tests now cover the renewal-overwrite regression path.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

backend/src/app/http/auth_request.clj:162

• Re-keying only overwrites ::session/profile-id, while the request still carries auth artifacts from the old cookie such as ::session/session and ::http/auth-data. Downstream code that reads those values (for example session/get-session during password changes, or logout's auth-data claims) can act on or log the previous user's session even though the request is treated as the header user.

                    response        (-> request
                                        (assoc ::session/profile-id (:id profile))
                                        handler)]

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

backend/src/app/http/auth_request.clj:169

• The re-keyed handler call still runs inside session/wrap-authz's clock binding for the old session profile. If the old profile has a virtual clock offset, downstream work and the replacement session timestamps are produced under the wrong user's clock after this identity switch.

                    response        (-> request
                                        (assoc ::session/profile-id (:id profile))
                                        handler)]

[awais786]

@copilot apply changes based on this feedback

[Copilot]

Applied in f06d71b: re-key now drops stale request session state (::session/session*) before injecting the new profile so downstream sees consistent identity/session data.

[awais786]

@copilot apply changes based on this feedback

[Copilot]

Applied the linked feedback in f06d71b: fixed the status-key renewal guard, removed stale session object leakage during re-key, and made the unresolvable-header path clear local session state and expire the auth-token cookie, with tests updated for these cases.

[Copilot]

Pressingly/plane#29 @copilot use this PR also for reference.

Aligned with the Plane reference in e850a4f and kept the same approach through follow-up hardening in f06d71b.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

backend/src/app/http/auth_request.clj:145

• The inactive-profile denial has the same stale-cookie gap as the blocked-profile branch: if this request arrived with another user's valid local session, the response is a 403 but the old browser cookie is left intact. If a later request reaches the backend without the proxy header, the no-header path will trust that stale session again, so this mismatch path should expire the local session cookie too.

            (not (:is-active profile))
            (do
              (l/wrn :hint "x-auth-request: profile is not active, denying access"
                     :email email
                     :profile-id (str (:id profile)))

[awais786]

@copilot apply changes based on this feedback

[Copilot]

Applied in 179bd40: profile-lookup exceptions are now handled as a distinct error state, so we no longer treat operational failures as "profile not found" or clear local auth state/cookies on that path. Added coverage in x-auth-request-preserves-local-session-when-profile-lookup-errors.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (2)

backend/src/app/http/auth_request.clj:200

• The denied mismatched-session path also only sends an expired cookie; it does not invalidate the old session in the manager because the request has ::session/session rather than the :app.http.session/id key read by session/delete-fn. The stale auth-token can therefore still authenticate if it is replayed, despite the "identity mismatch SHALL flush" invariant.

                  (let [delete-session! (session/delete-fn cfg)]
                    (delete-session! request response))

backend/test/backend_tests/http_middleware_test.clj:446

• This blocked/inactive mismatch test only checks the deletion cookie, not that Alice's existing server-side session is actually invalidated. Because the production path relies on session/delete-fn, adding a manager-level assertion would catch stale auth-token replay after the 403 response.

    (t/is (= 403 (::yres/status response)))
    (t/is (= 0 (get-in response [::yres/cookies "auth-token" :max-age])))))

[awais786]

Confirmed and tracked as a separate PR — #20. delete-fn reads ::id but wrap-authz never set it, so every call site of delete-fn (logout endpoints, this PR's mismatch-flush) was clearing the cookie without removing the server-side row. Replayable for up to 7 days until GC.

This is pre-existing — affects Penpot's standard logout flow too, not just this PR. Fix is one line in wrap-authz but touches shared session middleware, so it's split out: see #20 for the upstream fix + a regression-guard test.

This PR's commit c95d6d346 adjusts the mismatch-flush path to pass the original (pre-clear) request to delete-fn so ::id survives once #20 lands. PRs are independent and can merge in either order.

[awais786]

Tracked in #20. The test as written exercises the contract delete-fn claims to uphold (server-side state cleared on logout), but with the pre-existing ::id-never-set bug it would have been verifying a no-op. #20 adds session-delete-fn-removes-server-side-row against the session-manager directly, which is the right level for the invariant. Once #20 merges, this test's browser-cookie assertion remains meaningful but the server-side invariant has its own dedicated guard upstream.

[awais786]

Security follow-up: session/delete-fn doesn't actually delete server-side state — tracked in #20

Copilot's review surfaced a real, pre-existing security issue while reviewing this PR's delete-session! call sites (r3252847526 and r3252847543). Verified directly in the code:

session/delete-fn at session.clj:193-198 reads (get request ::id) — but session/wrap-authz (and nothing else in the codebase) ever sets that key. Grep confirms one read, zero writes. So every call site of delete-fn — including Penpot's standard logout endpoint, not just this PR's new SSO mismatch-flush — clears the browser cookie but leaves the underlying http_session_v2 row alive until the GC task expires it on schedule. Default ceiling: auth-token-cookie-max-age = 7 days.

Replay path: captured cookie → wrap-auth decodes JWT → signature valid (no per-session key rotation) → no :exp claim (assign-token) → wrap-authz resolves session via :sid → DB row exists → request authenticated. Logout's user-visible behaviour worked (cookie cleared); the contract delete-fn advertised — server-side invalidation at logout time — did not.

Severity: medium. Not remotely exploitable (attacker needs the cookie first), but logout doesn't fulfill its contract under the threat models where logout matters most: shared computer, post-incident cleanup, compromised endpoint. Mitigated in practice by HttpOnly / Secure / SameSite=Lax + the 7d GC ceiling.

Scope decision for this PR: the fix is a one-line addition in wrap-authz but it changes shared session middleware that all logout flows depend on — separate review surface, separate test plan. Bundling it here would mix concerns. Split out to #20 (branch fix/session-wraps-authz-sets-id) so #18's review surface stays scoped to the re-key boundary.

Independent merge order: #18's mismatch-flush works today (cookie cleared, replays caught by JWT-vs-row mismatch after #20 lands). #20 makes server-side delete real for every call site, not just this one. Either can ship first.

Update on this PR: commit c95d6d346 adjusts the mismatch-flush path to pass the original (pre-clear) request to delete-fn so ::id survives once #20 is in — this PR is correctly shaped today, will be functionally complete after #20 merges.