Fix/proxy auth stale session on user switch#21
Closed
awais786 wants to merge 19 commits into
Closed
Conversation
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) <noreply@anthropic.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/d72618d7-825c-4877-82b4-8129c361442f Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi <awais.qureshi@arbisoft.com>
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) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi <awais.qureshi@arbisoft.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/8bb70191-b81f-401c-9ff4-f33c74c775fc Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/b389792f-d18f-4b49-9241-455c18e728c0 Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
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) <noreply@anthropic.com>
Three concrete fixes on top of f1f1a6f: 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates proxy-auth handling so browser sessions are reconciled with the current X-Auth-Request-Email identity instead of blindly trusting an existing Penpot session cookie.
Changes:
- Reworks X-Auth-Request middleware to re-key mismatched sessions, clear stale auth state, and preserve sessions on transient lookup failures.
- Prevents session-cookie renewal on error responses or when a handler already set the auth cookie.
- Adds middleware tests covering stale-session rekeying, cookie renewal interactions, and error cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
backend/src/app/http/auth_request.clj |
Adds proxy identity reconciliation, stale-session clearing, transient lookup handling, and rekey cookie behavior. |
backend/src/app/http/session.clj |
Guards session renewal on error responses and existing auth-cookie response writes. |
backend/test/backend_tests/http_middleware_test.clj |
Adds tests for stale proxy-auth session switching and renewal behavior. |
Comments suppressed due to low confidence (1)
backend/src/app/http/auth_request.clj:240
- This mismatch-denial path has the same server-side invalidation gap: calling
delete-fnwith the current request only expires the browser cookie because the request does not contain theapp.http.session/idkey thatdelete-fnuses to delete the session row. Use the id from::session/session(or attach the expected key before deletion) so the stale session is actually flushed.
(let [delete-session! (session/delete-fn cfg)]
(delete-session! request response))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+207
to
+216
| (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))) |
There was a problem hiding this comment.
Implemented in 3130d1c.
I now project the decoded session’s :id into ::session/id before calling session/delete-fn in both flush paths (unresolvable-email and blocked/inactive mismatch), so delete-fn can remove the server-side row. I also added tests to assert the session id is passed through those branches. This is backend-only, so there are no UI changes/screenshots.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Awais Qureshi <awais.qureshi@arbisoft.com>
Agent-Logs-Url: https://github.com/Pressingly/penpot/sessions/bedf497a-3ef2-4e74-899c-886ba1a7c532 Co-authored-by: awais786 <445320+awais786@users.noreply.github.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
wrap-authz(X-Auth-Request middleware) used to bail the momentwrap-sessionhad set::session/profile-id. Once Penpot's ownauth-tokencookie was issued, the upstreamX-Auth-Request-Emailwas no longer consulted._oauth2_proxycookie and Cognito SSO session but not Penpot'sauth-tokencookie onpaint.<domain>— refreshing the Penpot tab kept serving the previous user.auth-tokencookie on the response, replacing the stale browser cookie.auth-tokencookie viasession/delete-fn. The request continues unauthenticated (downstream handlers respond with 401/redirect-to-login per their own rules) rather than as the previous user.session.cljensures the stale session cookie is NOT renewed on the denial response.::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)
paint.<domain>) in a tab.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.