🐛 Populate ::id in wrap-authz so delete-fn actually deletes#20
Open
awais786 wants to merge 1 commit into
Open
Conversation
session/delete-fn at backend/src/app/http/session.clj:193-198 reads `(get request ::id)` (i.e. `:app.http.session/id`) and only calls `delete-session` when that key is present. But nothing in the codebase ever set it — `session/wrap-authz` stored the resolved session at `::session` and the profile-id at `::profile-id`, never at `::id`. So every call site of delete-fn (logout endpoints, the new SSO re-key mismatch flush in #18) cleared the browser cookie but left the http_session_v2 row in place until the GC task expired it on schedule — default 7 days. Security impact: a captured/leaked cookie remained replayable for up to 7 days after the user clicked logout. JWT signature still validates (no rotation per session), `:exp` is not set on the token claims, and the row exists, so the server happily authenticates the replay. Mitigated in practice by HttpOnly/Secure/SameSite=Lax on the cookie plus the GC ceiling, but the contract delete-fn advertises — server- side invalidation at logout time — was not upheld. Fix: wrap-authz now also assocs `::id (:id session)` alongside `::session` and `::profile-id`, in both the cookie and bearer branches. delete-fn then has the key it expects and the `(some->> (get request ::id) (delete-session manager))` actually fires. Regression guard: session-delete-fn-removes-server-side-row asserts that wrap-authz populates ::id and that running delete-fn afterwards removes the row from the session manager. Surfaced by Copilot review on #18 (discussion r3252847526 and r3252847543), but pre-existing — every call site of delete-fn already had this gap. PR #18 is the first time we'd be calling delete-fn from a new path, which is what made it visible. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
3 tasks
There was a problem hiding this comment.
Pull request overview
This PR primarily tightens authentication/session handling for SSO/forward-auth deployments by ensuring server-side sessions are actually deleted on logout, and by adding/adjusting related SSO-oriented behaviors across backend, frontend, tests, and Docker runtime config.
Changes:
- Populate
::app.http.session/idinsession/wrap-authzsosession/delete-fncan delete the underlying session row (not just clear the cookie). - Add
X-Auth-Request-*forward-auth middleware (optional via flags) with optional auto-registration and SMB team auto-join behavior. - Disable local email-change flows (RPC + token redemption) and adjust UI/tests accordingly; add runtime-injected frontend config for an SSO signout URL and tweak logout redirect behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/app/main/ui/settings/profile.cljs | Removes “change email” UI; renders email as read-only with rationale comment. |
| frontend/src/app/main/ui/dashboard/sidebar.cljs | Hides “Create team” in dashboard when :x-auth-request-headers flag is enabled. |
| frontend/src/app/main/ui.cljs | Adjusts onboarding modal logic to skip team onboarding under forward-auth/shared-team scenarios. |
| frontend/src/app/main/data/auth.cljs | Changes logout flow to compute a “portal” redirect URI (intended to escape ForwardAuth). |
| frontend/src/app/config.cljs | Adds mpass-signout-url config value sourced from runtime config.js. |
| docker/images/files/nginx-entrypoint.sh | Injects penpotMpassSignoutUrl into frontend config.js when MPASS_SIGNOUT_URL is set. |
| docker/images/files/config.js | Adds placeholder penpotMpassSignoutUrl config var. |
| backend/test/backend_tests/rpc_profile_test.clj | Updates tests to assert email-change is fully disabled and profile row is not mutated. |
| backend/test/backend_tests/http_middleware_test.clj | Adds regression test for delete-fn session row deletion; adds X-Auth-Request middleware tests. |
| backend/test/backend_tests/helpers.clj | Adds :default-email-domain to test config defaults. |
| backend/test/backend_tests/config_session_cookie_test.clj | New tests covering cookie max-age/renewal-max-age config defaults and overrides. |
| backend/src/app/rpc/commands/verify_token.clj | Disables :change-email token redemption with a restriction error. |
| backend/src/app/rpc/commands/profile.clj | Disables :request-email-change RPC with a restriction error. |
| backend/src/app/rpc.clj | Adds auth-request/authz middleware to RPC routes (behind flags). |
| backend/src/app/http/session.clj | Makes renewal window configurable; populates ::id in wrap-authz; uses configurable renewal in cookie. |
| backend/src/app/http/auth_request.clj | New forward-auth middleware using X-Auth-Request-* headers with optional auto-register + SMB team join. |
| backend/src/app/config.clj | Adds cookie duration defaults + schema entries; adds :default-email-domain / :smb-default-workspace-name. |
| backend/scripts/_env | Enables new forward-auth flags in the dev script env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
241
to
254
| request | ||
| (cond-> request | ||
| (some? session) | ||
| (-> (assoc ::profile-id (:profile-id session)) | ||
| ;; ::id is the key delete-fn reads to remove the | ||
| ;; server-side session row. Setting it here means | ||
| ;; any downstream call to session/delete-fn (logout, | ||
| ;; SSO re-key flush) actually deletes the row; | ||
| ;; without it, delete-fn falls through and only | ||
| ;; clears the browser cookie — leaving the row | ||
| ;; replayable until GC (auth-token-cookie-max-age, | ||
| ;; default 7d). | ||
| (assoc ::id (:id session)) | ||
| (assoc ::session session))) |
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
session/delete-fnat backend/src/app/http/session.clj:193-198 reads(get request ::id)— i.e.:app.http.session/id— and only invokesdelete-sessionwhen that key is present. But nothing in the codebase ever set it:session/wrap-authzstored the resolved session at `::session` and the profile-id at `::profile-id`, never at `::id`. So every call site of `delete-fn` (the standard logout endpoint, the new SSO re-key mismatch flush in #18) cleared the browser cookie but left the underlying `http_session_v2` row alive until the GC task expired it.Default GC ceiling:
auth-token-cookie-max-age= 7 days (session.clj:34).Security impact
The replay path:
wrap-authdecodes the JWT, signature is still valid (no per-session key rotation), the token has no:expclaim (seeassign-token), sotokens/verifydoes not reject on expiry.wrap-authzthen reads `:sid` from claims, callsread-session, the DB row still exists, and the replay is authenticated.Severity: medium. Not a remote-exploitable hole — attacker needs the cookie value first — but logout does not fulfil its contract under the threat models where logout matters most.
Fix
One-line addition in
wrap-authz, in both the cookie and bearer branches:```clojure
(-> (assoc ::profile-id (:profile-id session))
(assoc ::id (:id session)) ;; <-- new
(assoc ::session session))
```
`delete-fn` then has the key it expects and the
(some->> (get request ::id) (delete-session manager))actually fires.Regression guard
`session-delete-fn-removes-server-side-row` asserts:
```clojure
(t/is (= (:id session) (:app.http.session/id request))
"wrap-authz must populate ::id from the resolved session")
…
(let [delete! (session/delete-fn {::session/manager manager})
_ (delete! request {::yres/status 200})]
(t/is (nil? (#'session/read-session manager (:id session)))
"after delete-fn runs, the server-side session row must be gone"))
```
Relation to #18
Surfaced by Copilot review on #18 (discussions r3252847526 and r3252847543) but pre-existing — every call site of `delete-fn` already had this gap. PR #18 is the first time `delete-fn` is invoked from a new code path, which is what made the latent issue visible.
PR #18 stays focused on the re-key boundary. This PR addresses the upstream gap that makes `delete-fn` actually do what it advertises. The two are independent and can merge in either order — PR #18's mismatch-flush works today (cookie cleared, replays caught by JWT-row mismatch after this lands), and this PR makes the server-side delete real for everyone.
Test plan
clj -X:test :nses '[backend-tests.http-middleware-test]'passes — new testsession-delete-fn-removes-server-side-rowexercises both invariantssession-authztest continues to pass (it doesn't read `::id` but the request now has it)🤖 Generated with Claude Code