From c94e3970a3cae8a17d2e9a7fe0d46e6ef4ddb97b Mon Sep 17 00:00:00 2001 From: awais786 Date: Sat, 16 May 2026 18:32:02 +0500 Subject: [PATCH] :bug: Populate ::id in wrap-authz so delete-fn actually deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Pressingly/penpot#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) --- backend/src/app/http/session.clj | 10 ++++++ .../backend_tests/http_middleware_test.clj | 34 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index bcbb2517a02..227a3000353 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -242,6 +242,15 @@ (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))) response @@ -264,6 +273,7 @@ request (cond-> request (some? session) (-> (assoc ::profile-id (:profile-id session)) + (assoc ::id (:id session)) (assoc ::session session)))] (handler request)) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index f7d4f70d1c7..3915d5b5e8f 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -140,6 +140,40 @@ (t/is (= (:id session) (:sid claims))) (t/is (= (:id profile) (:uid claims))))) +(t/deftest session-delete-fn-removes-server-side-row + ;; Regression guard for the gap where session/delete-fn read ::id from + ;; the request but session/wrap-authz never set it: every logout + ;; cleared the browser cookie but left the http_session_v2 row alive + ;; until GC, so a captured cookie remained replayable for up to the + ;; auth-token-cookie-max-age window (7d default). This test pins that + ;; wrap-authz now assocs ::id and delete-fn therefore actually + ;; deletes the row. + (let [cfg th/*system* + manager (session/inmemory-manager) + profile (th/create-profile* 92) + handler (-> (fn [req] req) + (#'session/wrap-authz {::session/manager manager}) + (#'mw/wrap-auth {:bearer (partial session/decode-token cfg) + :cookie (partial session/decode-token cfg)})) + session (->> (session/create-session manager {:profile-id (:id profile) + :user-agent "user agent"}) + (#'session/assign-token cfg)) + request (handler (->DummyRequest {} {"auth-token" (:token session)}))] + + ;; Sanity: wrap-authz populated ::id so delete-fn has something to read. + (t/is (= (:id session) (:app.http.session/id request)) + "wrap-authz must populate ::id from the resolved session") + + ;; Sanity: the row exists before delete. + (t/is (some? (#'session/read-session manager (:id session)))) + + ;; delete-fn reads ::id from the request; with the fix in place this + ;; actually removes the in-memory 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")))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; X-Auth-Request middleware tests ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;