psa-systems/bunyip
Watch
9
0
Fork
You've already forked bunyip
 0
Code Issues Pull requests Projects Releases 18 Packages Wiki Activity Actions

fix(auth): logout clears cookies unconditionally, even when access_token is stale #319

Merged
YousifShkara merged 1 commit from fix/BUNYIP-323-logout-clears-cookies-unconditionally into main 2026-07-02 09:06:03 +02:00
Conversation 0 Commits 1 Files changed 2 +198 -11
YousifShkara commented 2026-07-02 08:41:53 +02:00
Owner
Copy link

David tried to sign out and re-register with a corrected (lowercase) email but the logout did not clear his session; he stayed signed in. Root cause: bunyip-api's POST /v1/auth/logout required a valid AuthenticatedUser extractor. Once the access_token expired (up to 15 minutes after last refresh) the extractor 401'd BEFORE the handler ran, so no clearing Set-Cookie headers were emitted. bunyip-web's own /logout handler forwarded whatever bunyip-api returned via .unwrap_or_default(); a 401 body yielded an empty Set-Cookie vec, and only the bunyip_2fa challenge cookie was cleared. The browser kept access_token + refresh_token + bunyip_op_session intact, and the next request re-authenticated via the refresh cycle - the user was still signed in from every visible surface.

Fix at both layers so the failure mode cannot recur through either path:

  1. bunyip-api POST /v1/auth/logout now takes OptionalUser instead of AuthenticatedUser. With a valid user the pre-existing DB revoke + OIDC op-session fan-out still run (best-effort; a DB error inside the revoke logs a warn but does not block cookie clearing). Without a valid user those DB writes are skipped and the residual state ages out on its own schedule. Cookies are cleared via AuthCookies::clear on every response - the whole point of the endpoint from the browser's perspective is to emit clearing Set-Cookie headers, and that must happen whether or not the caller can prove who they are.

  2. bunyip-web adds a defensive bunyip_auth_cookie_clears(cfg) helper that produces host-only + domain-scoped clears for the three auth cookie names (access_token, refresh_token, bunyip_op_session), mirroring AuthCookies::clear's two-axis pattern in crates/bunyip-domain/src/middleware/auth.rs. The /logout handler always merges these into the response, so an unreachable / errored bunyip-api still ends with the browser losing its cookies. The existing unwrap_or_default() on the API call now degrades to the API's clears when present and the defensive clears when absent - either way the browser purges cookies.

Three unit tests in logout_clear_tests pin the shape:

  • dev config (no app_domain, http api origin) yields exactly the three host-only clears with no Secure and no Domain=.
  • prod config yields six clears (three host-only + three domain-scoped) all with Secure and matching Domain=<app_domain>.
  • every cookie name appears exactly once per axis, so a future edit that adds a fourth auth cookie name to AuthCookies::set on the api side surfaces here.

Existing 232 domain-tests + 9 api-tests + 14 oidc-tests + 26 web-tests (three new) pass under just check-container. No change to the /oauth2/logout SSO surface, logout_all, or logout_redirect.

#BUNYIP-323

David tried to sign out and re-register with a corrected (lowercase) email but the logout did not clear his session; he stayed signed in. Root cause: bunyip-api's POST /v1/auth/logout required a valid `AuthenticatedUser` extractor. Once the access_token expired (up to 15 minutes after last refresh) the extractor 401'd BEFORE the handler ran, so no clearing `Set-Cookie` headers were emitted. bunyip-web's own /logout handler forwarded whatever bunyip-api returned via `.unwrap_or_default()`; a 401 body yielded an empty Set-Cookie vec, and only the `bunyip_2fa` challenge cookie was cleared. The browser kept access_token + refresh_token + bunyip_op_session intact, and the next request re-authenticated via the refresh cycle - the user was still signed in from every visible surface. Fix at both layers so the failure mode cannot recur through either path: 1. bunyip-api `POST /v1/auth/logout` now takes `OptionalUser` instead of `AuthenticatedUser`. With a valid user the pre-existing DB revoke + OIDC op-session fan-out still run (best-effort; a DB error inside the revoke logs a warn but does not block cookie clearing). Without a valid user those DB writes are skipped and the residual state ages out on its own schedule. Cookies are cleared via `AuthCookies::clear` on every response - the whole point of the endpoint from the browser's perspective is to emit clearing `Set-Cookie` headers, and that must happen whether or not the caller can prove who they are. 2. bunyip-web adds a defensive `bunyip_auth_cookie_clears(cfg)` helper that produces host-only + domain-scoped clears for the three auth cookie names (access_token, refresh_token, bunyip_op_session), mirroring `AuthCookies::clear`'s two-axis pattern in `crates/bunyip-domain/src/middleware/auth.rs`. The /logout handler always merges these into the response, so an unreachable / errored bunyip-api still ends with the browser losing its cookies. The existing `unwrap_or_default()` on the API call now degrades to the API's clears when present and the defensive clears when absent - either way the browser purges cookies. Three unit tests in `logout_clear_tests` pin the shape: - dev config (no app_domain, http api origin) yields exactly the three host-only clears with no `Secure` and no `Domain=`. - prod config yields six clears (three host-only + three domain-scoped) all with `Secure` and matching `Domain=<app_domain>`. - every cookie name appears exactly once per axis, so a future edit that adds a fourth auth cookie name to `AuthCookies::set` on the api side surfaces here. Existing 232 domain-tests + 9 api-tests + 14 oidc-tests + 26 web-tests (three new) pass under `just check-container`. No change to the /oauth2/logout SSO surface, `logout_all`, or `logout_redirect`. #BUNYIP-323
fix(auth): logout clears cookies unconditionally, even when access_token is stale
All checks were successful
E2E / Playwright against deployment (pull_request) Successful in 24s
Details
Check / fmt + clippy + build + tests (pull_request) Successful in 8m38s
Details
Create release / Create release from merged PR (pull_request) Has been skipped
Details
e5cb1647be 
David tried to sign out and re-register with a corrected (lowercase) email but the logout did not clear his session; he stayed signed in. Root cause: bunyip-api's POST /v1/auth/logout required a valid `AuthenticatedUser` extractor. Once the access_token expired (up to 15 minutes after last refresh) the extractor 401'd BEFORE the handler ran, so no clearing `Set-Cookie` headers were emitted. bunyip-web's own /logout handler forwarded whatever bunyip-api returned via `.unwrap_or_default()`; a 401 body yielded an empty Set-Cookie vec, and only the `bunyip_2fa` challenge cookie was cleared. The browser kept access_token + refresh_token + bunyip_op_session intact, and the next request re-authenticated via the refresh cycle - the user was still signed in from every visible surface.

Fix at both layers so the failure mode cannot recur through either path:

1. bunyip-api `POST /v1/auth/logout` now takes `OptionalUser` instead of `AuthenticatedUser`. With a valid user the pre-existing DB revoke + OIDC op-session fan-out still run (best-effort; a DB error inside the revoke logs a warn but does not block cookie clearing). Without a valid user those DB writes are skipped and the residual state ages out on its own schedule. Cookies are cleared via `AuthCookies::clear` on every response - the whole point of the endpoint from the browser's perspective is to emit clearing `Set-Cookie` headers, and that must happen whether or not the caller can prove who they are.

2. bunyip-web adds a defensive `bunyip_auth_cookie_clears(cfg)` helper that produces host-only + domain-scoped clears for the three auth cookie names (access_token, refresh_token, bunyip_op_session), mirroring `AuthCookies::clear`'s two-axis pattern in `crates/bunyip-domain/src/middleware/auth.rs`. The /logout handler always merges these into the response, so an unreachable / errored bunyip-api still ends with the browser losing its cookies. The existing `unwrap_or_default()` on the API call now degrades to the API's clears when present and the defensive clears when absent - either way the browser purges cookies.

Three unit tests in `logout_clear_tests` pin the shape:
- dev config (no app_domain, http api origin) yields exactly the three host-only clears with no `Secure` and no `Domain=`.
- prod config yields six clears (three host-only + three domain-scoped) all with `Secure` and matching `Domain=<app_domain>`.
- every cookie name appears exactly once per axis, so a future edit that adds a fourth auth cookie name to `AuthCookies::set` on the api side surfaces here.

Existing 232 domain-tests + 9 api-tests + 14 oidc-tests + 26 web-tests (three new) pass under `just check-container`. No change to the /oauth2/logout SSO surface, `logout_all`, or `logout_redirect`.

#BUNYIP-323
YousifShkara deleted branch fix/BUNYIP-323-logout-clears-cookies-unconditionally 2026-07-02 09:06:04 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
YousifShkara
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
psa-systems/bunyip!319
No description provided.