From 467b7bed73d0dada89f467561cc74c25cea48239 Mon Sep 17 00:00:00 2001 From: Nguyen Van Long Date: Tue, 6 Oct 2026 11:56:58 +0700 Subject: [PATCH] fix(api): sign-out fails closed when the session delete does not commit (FIX-LOGOUT) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /v1/logout logged a session-store Delete error, expired the cookie and answered 204/200 while the server-side session stayed live — a copied cookie kept working. The delete is now the ordering's point of no return: on failure the handler answers retryable 503 UNAVAILABLE + Retry-After and tears nothing down (no cookie expiry, no lease/ticket revoke), so the response never claims a sign-out that did not commit and the still-valid session keeps its live leases until a retry completes the destroy. Revoke-all already fails closed (single transaction, 500, cookie kept); pinned by a retry test that mirrors the real row deletion. Portal copy (en/vi) now states the user is still signed in and can retry. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- docs/security/threat-model.md | 2 +- internal/api/auth.go | 19 +++++++ internal/api/logout_test.go | 81 ++++++++++++++++++++++++++++ internal/api/openapi.yaml | 7 +++ internal/api/revokeall_test.go | 65 ++++++++++++++++++++++ web/src/api/generated/schema.d.ts | 7 +++ web/src/i18n/en/app.ts | 3 +- web/src/i18n/vi/app.ts | 3 +- web/tests/unit/auth/signout.test.tsx | 33 ++++++++++++ 9 files changed, 217 insertions(+), 3 deletions(-) diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 0763c69b..05583fbb 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -515,7 +515,7 @@ items that landed since. | S14 | App-layer rate limit for `/v1/login`, `/v1/launch` | Implemented (v0.4: `internal/ratelimit` + Postgres fixed-minute windows, ADR 0006 — FX-R30 keying, exact shared-window bound with the undivided local bucket as a healthy-mode store-protection prefilter and the divided local bucket as the degraded-mode floor (v0.5 RL-CEILING amendment), fail-open on store error). Since then: a circuit breaker bounds degraded-mode cost (500 ms per-call deadline, 10 s cool-down skipping the store, single-flight probe — `internal/ratelimit/shared.go`, `shared_test.go`), the v0.4.0 defaults restore the v0.3.x effective budgets as window bounds (`TestRateLimitDefaults_V040Budgets`), and `/v1/me/sessions:revoke-all` joined the session-keyed limiter. Reviewer verifies coverage, ceilings, bypass resistance and the outage degradation path. Residual: IP-keyed login budgets multiply with a client's address pool — a dual-stack client, IPv6 temporary addresses or several egress addresses each get their own local and window budget (per-address limiting as designed; authenticated routes key by session) | | S15 | Operator mTLS client cert / listener client-CA hot reload | **Implemented since** (`opclient` reload loop + `hotReloadClientCAs`; FX-R33 test); reviewer confirms rotation edge cases | | S16 | `runtime.appArmor.requireRuntimeDefault` opt-out | Implemented (`AppArmorNotRequired`); review docs/default/preflight detection on AppArmor-less nodes | -| S17 | Sign-out vs live desktop streams | Implemented (#107 + #123; `LogoutHandler` → `Broker.RevokePortalSession`, `internal/api/auth.go:685`, `internal/broker/sessions.go:109`): sign-out revokes the session's digest-bound leases and outstanding tickets in one store tx — every replica's renew loop closes the bound stream within one renew cycle, and a replayed workspace cookie resolves to a revoked lease (401) on any replica; ticket-redemption re-checks the session row (`internal/broker/tickets.go`). Since the SIGNOUT-FIX pass both revoke sweeps pre-lock their covered rows in a deterministic key order (`lockByKeysInOrderTx`, `sessions.go:418` — tickets by `ticket_hash`, sessions by `id`, leases by `id`), so overlapping sweeps serialize on the first contested row instead of crossing waits. Defence-in-depth (#123, DR): every lease read that feeds renew/attach/claim or the cookie→lease resolve (`liveLease`/`LeaseBySession`, `internal/broker/leases.go`) re-validates the bound portal session row — epoch + absolute expiry, same semantics as the redeem-time check — in the same indexed read, and revokes the lease in one statement when it fails (`tinycdi_lease_session_missing_total{reason=absent\|invalid}` + one log line). A lease resurrected as `active` by a Postgres restore therefore dies at its next renew or attach even when the sign-out revoke itself was lost in the dump window; leases with a NULL `portal_session_digest` (pre-018 rows) are exempt — nothing to verify against, and revoking them on sight would mass-kill sessions mid-rolling-upgrade — they keep the TTL lifecycle and are covered by the DR runbook's post-restore lease sweep. Tests `internal/broker/lease_session_test.go`, `signout_test.go` (api, broker, gateway) | +| S17 | Sign-out vs live desktop streams | Implemented (#107 + #123; `LogoutHandler` → `Broker.RevokePortalSession`, `internal/api/auth.go:685`, `internal/broker/sessions.go:109`): sign-out revokes the session's digest-bound leases and outstanding tickets in one store tx — every replica's renew loop closes the bound stream within one renew cycle, and a replayed workspace cookie resolves to a revoked lease (401) on any replica; ticket-redemption re-checks the session row (`internal/broker/tickets.go`). Since the SIGNOUT-FIX pass both revoke sweeps pre-lock their covered rows in a deterministic key order (`lockByKeysInOrderTx`, `sessions.go:418` — tickets by `ticket_hash`, sessions by `id`, leases by `id`), so overlapping sweeps serialize on the first contested row instead of crossing waits. Defence-in-depth (#123, DR): every lease read that feeds renew/attach/claim or the cookie→lease resolve (`liveLease`/`LeaseBySession`, `internal/broker/leases.go`) re-validates the bound portal session row — epoch + absolute expiry, same semantics as the redeem-time check — in the same indexed read, and revokes the lease in one statement when it fails (`tinycdi_lease_session_missing_total{reason=absent\|invalid}` + one log line). A lease resurrected as `active` by a Postgres restore therefore dies at its next renew or attach even when the sign-out revoke itself was lost in the dump window; leases with a NULL `portal_session_digest` (pre-018 rows) are exempt — nothing to verify against, and revoking them on sight would mass-kill sessions mid-rolling-upgrade — they keep the TTL lifecycle and are covered by the DR runbook's post-restore lease sweep. Since the FIX-LOGOUT pass the session-row delete is the ordering's point of no return: a store failure on it answers retryable `503 UNAVAILABLE` (+ `Retry-After`) and tears nothing down — no cookie expiry, no lease/ticket revoke — so a response never claims a sign-out that did not commit, and the still-valid session keeps its live leases until a retry completes the destroy (revoking first would leave a live session with dead streams, a half-revoked state the refusal would be lying about). Tests `internal/broker/lease_session_test.go`, `signout_test.go` (api, broker, gateway), `TestLogout_StoreDeleteFailureReturns503` | | S18 | Client address chain gateway → KasmVNC | Closed (ADR 0008): client XFF never reaches the runtime and only trusted proxies shift the forwarded keys (`forwarded_test.go`); the pod-side 5/10 blacklist is correctly keyed on the derived client address and stays as defence-in-depth — credential guesses are unreachable by construction since `Authorization` is broker-injected on every proxied request (`TestProxy_StripsClientAuth`) | | S19 | Sign-out-everywhere vs the principal's other sessions | Implemented (ADR 0007; `RevokeAllSessionsHandler` → `Broker.RevokePrincipalSessions`, `internal/api/revokeall.go`, `internal/broker/sessions.go`): `POST /v1/me/sessions:revoke-all` destroys every portal session of the principal **in the caller's tenant** — caller's own session included — and revokes its active leases and outstanding tickets in one store transaction (lock order tickets → sessions → leases, identical to `RedeemTicket`/`RevokePortalSession` and deterministic within each kind via `lockByKeysInOrderTx`; both redeem interleavings, a multi-ticket inversion and a `-race` three-way run are pinned on real row locks). Streams die within one renew cycle on every replica; every revoked session's cookie replays to 401. Failure rolls back whole (500, caller stays signed in — never a partial revoke reported as success). No IdP back-channel logout; the provider session ends only via the same RP-initiated `endSessionUrl` as logout. Tests `revokeall_test.go` (api, broker, gateway) | | S20 | Per-principal running-workspace limits | Implemented (#121, migration 022): `tenant_user_limit_default` + `user_session_limit` tables written through tenant-admin routes `/v1/admin/tenants/{tenant}/user-limits[/default]` (`internal/api/adminuserlimit.go` — auth + audit + CSRF, set/clear audited under `admin.user_limit.*`); admission enforces the effective limit inside the reservation transaction under the `tenant_quota` row lock (`checkUserLimit`, `internal/provisioning/quota.go:433-492`), counting only `held` running slots — disk-only holds and retained disks are exempt, a missing workspace row fails closed, refusal maps to 409 `UserLimitReached`. No rows means unlimited, so upgrades change nothing until an admin acts. Tests `adminuserlimit_test.go`, `provisioning/userlimit_test.go`; reviewer checks the owner→principal identity mapping (`issuer\|sub`) and cross-tenant authz | diff --git a/internal/api/auth.go b/internal/api/auth.go index 1d947208..afbff79b 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -661,6 +661,11 @@ type LogoutResult struct { // response. const sessionRevokeTimeout = 5 * time.Second +// logoutRetryAfter is the Retry-After hint on the 503 a sign-out answers +// when the session store could not delete the session row: a transient +// store failure, safe to retry once the store is back. +const logoutRetryAfter = "5" + // LogoutHandler destroys the server-side session and expires all login- and // session-scoped cookies. Route it behind RequireAuth + RequireCSRF. // @@ -674,6 +679,17 @@ const sessionRevokeTimeout = 5 * time.Second // audited but never kept back the sign-out: the portal cookie is cleared // and the session destroyed regardless. // +// The session-row delete is the point of no return and is ordered first for +// exactly that reason: when it fails the handler answers a retryable 503 +// (UNAVAILABLE + Retry-After) and tears NOTHING down — no cookie expiry, no +// lease or ticket revoke. The answer "not signed out, retry" then matches +// the world: the session still validates and its desktops are still alive. +// Revoking material first would leave a live session with dead streams — a +// half-revoked state a 503 would be lying about — and expiring the cookie +// would tell the browser it is signed out while a copied cookie still works. +// A retry re-runs the whole destroy; the material revoke is a no-op re-run +// when it already committed. +// // RP-initiated logout: when EndSession is on and the provider advertises // end_session_endpoint, it answers 200 {"endSessionUrl"} so the portal can // continue there and end the provider session — otherwise the next visit @@ -695,6 +711,9 @@ func (a *Authenticator) LogoutHandler(w http.ResponseWriter, r *http.Request) { if err := a.sessions.Delete(ctx, c.Value); err != nil { a.log.Warn("sign-out: session delete failed", "request_id", RequestIDFromContext(ctx), "err", err) + w.Header().Set("Retry-After", logoutRetryAfter) + writeError(w, r, CodeUnavailable, "could not sign out; retry") + return } a.revokeSessionMaterial(r, c.Value) } diff --git a/internal/api/logout_test.go b/internal/api/logout_test.go index 59fe51b4..e21e0eed 100644 --- a/internal/api/logout_test.go +++ b/internal/api/logout_test.go @@ -507,3 +507,84 @@ func TestLogout_NoRevokerKeepsSignOut(t *testing.T) { t.Fatalf("logout status = %d, want 204", resp.StatusCode) } } + +// failingDeleteStore is a SessionStore whose Delete fails while fail is +// set — a transient store outage — and passes through once it clears. +type failingDeleteStore struct { + SessionStore + fail bool +} + +func (s *failingDeleteStore) Delete(ctx context.Context, id string) error { + if s.fail { + return errors.New("session store down") + } + return s.SessionStore.Delete(ctx, id) +} + +// TestLogout_StoreDeleteFailureReturns503: when the session store cannot +// delete the session row the sign-out must fail closed — 503 UNAVAILABLE, +// retryable, with Retry-After — and tear nothing down: the cookie is not +// expired (no success claim the state contradicts), the session still +// validates (a copied cookie keeps working), and the session-bound +// material revoke never ran (no half-revoked live session). Once the +// store recovers a plain retry completes the sign-out and the session no +// longer validates. +func TestLogout_StoreDeleteFailureReturns503(t *testing.T) { + env := newTestEnv(t, nil) // no end-session endpoint: the 204 path + rv := &fakeSessionRevoker{n: 1} + env.auth.WithSessionRevoker(rv) + fs := &failingDeleteStore{SessionStore: env.store, fail: true} + env.auth.sessions = fs + sess := env.loginSession(t) + + resp := env.postLogout(t, sess, csrfTokenFor(sess.Value), nil) + defer resp.Body.Close() + if resp.StatusCode != http.StatusServiceUnavailable { + t.Fatalf("logout status = %d, want 503", resp.StatusCode) + } + if resp.Header.Get("Retry-After") == "" { + t.Fatal("503 sign-out refusal carries no Retry-After") + } + var body Error + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + t.Fatalf("decode error body: %v", err) + } + if body.Code != CodeUnavailable || !body.Retryable { + t.Fatalf("error body = %+v, want UNAVAILABLE retryable", body) + } + // No expired-cookie header: the response must not claim a sign-out + // that never committed. + for _, h := range resp.Header.Values("Set-Cookie") { + if strings.Contains(h, "Max-Age=0") { + t.Fatal("session cookie cleared despite failed delete") + } + } + // The session still validates — the state matches the refusal. + r := env.authedGet(t, sess, "/v1/me") + r.Body.Close() + if r.StatusCode != http.StatusOK { + t.Fatalf("session invalidated by a failed logout: %d", r.StatusCode) + } + // Nothing else was torn down: no lease/ticket revoke on a live + // session. + if len(rv.calls) != 0 { + t.Fatalf("session material revoked despite failed delete: %v", rv.calls) + } + + // The store recovers: the same request, retried, signs out for real. + fs.fail = false + resp = env.postLogout(t, sess, csrfTokenFor(sess.Value), nil) + resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("retry status = %d, want 204", resp.StatusCode) + } + if len(rv.calls) != 1 || rv.calls[0] != sess.Value { + t.Fatalf("revocations after retry = %v, want exactly [%q]", rv.calls, sess.Value) + } + r = env.authedGet(t, sess, "/v1/me") + r.Body.Close() + if r.StatusCode != http.StatusUnauthorized { + t.Fatalf("session still valid after retried logout: %d", r.StatusCode) + } +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 3c7fbafe..d8620311 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -151,6 +151,13 @@ paths: `oidc.postLogoutRedirect` — never from the request (no open redirect). The session keeps no ID token, so `id_token_hint` is not sent. Otherwise the answer is `204` and the portal stays local. + + The server-side session delete is the point of no return: when the + session store cannot delete it, the answer is `503` `UNAVAILABLE` + with `Retry-After` — the cookie is NOT expired and no session-bound + leases or launch tickets are revoked, so the caller is still fully + signed in and may simply retry. A success status is only ever sent + after the session row is gone. tags: [session] responses: "200": diff --git a/internal/api/revokeall_test.go b/internal/api/revokeall_test.go index 742c5dae..8f6ff19a 100644 --- a/internal/api/revokeall_test.go +++ b/internal/api/revokeall_test.go @@ -195,6 +195,71 @@ func TestRevokeAll_NoRevokerIs503(t *testing.T) { } } +// txPrincipalRevoker mirrors what the wired broker revoker actually does: +// on success the principal's session rows are gone for real (the in-memory +// store stands in for the transaction), on error nothing changes. +type txPrincipalRevoker struct { + fakePrincipalRevoker + store *InMemorySessionStore +} + +func (f *txPrincipalRevoker) RevokePrincipalSessions(ctx context.Context, tenantID, issuer, subject string) (RevokeAllResult, error) { + res, err := f.fakePrincipalRevoker.RevokePrincipalSessions(ctx, tenantID, issuer, subject) + if err != nil { + return res, err + } + f.store.mu.Lock() + defer f.store.mu.Unlock() + for key, s := range f.store.sessions { + if s.Principal.Issuer == issuer && s.Principal.Subject == subject && s.Principal.TenantID == tenantID { + delete(f.store.sessions, key) + res.Sessions++ + } + } + return res, nil +} + +// TestRevokeAll_StoreFailureThenRetrySignsOut: a revoke-all that fails +// answers a non-success status and destroys nothing — the caller stays +// signed in and retries. Once the store recovers, the same call commits: +// the caller's session no longer validates and the cookie expires. +func TestRevokeAll_StoreFailureThenRetrySignsOut(t *testing.T) { + env := newTestEnv(t, nil) + rv := &txPrincipalRevoker{fakePrincipalRevoker: fakePrincipalRevoker{err: errors.New("lease store down")}, store: env.store} + env.auth.WithPrincipalRevoker(rv) + sess := env.loginSession(t) + + resp := env.postRevokeAll(t, sess, csrfTokenFor(sess.Value)) + resp.Body.Close() + if resp.StatusCode == http.StatusOK || resp.StatusCode == http.StatusNoContent { + t.Fatalf("failed revocation answered success: %d", resp.StatusCode) + } + for _, h := range resp.Header.Values("Set-Cookie") { + if strings.Contains(h, "Max-Age=0") { + t.Fatal("session cookie cleared despite failed revoke") + } + } + r := env.authedGet(t, sess, "/v1/me") + r.Body.Close() + if r.StatusCode != http.StatusOK { + t.Fatalf("session gone after failed revoke-all: %d", r.StatusCode) + } + + // The store recovers: a retry commits — the caller's session no + // longer validates and the cookie dies with the response. + rv.err = nil + resp = env.postRevokeAll(t, sess, csrfTokenFor(sess.Value)) + defer resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Fatalf("retry status = %d, want 204", resp.StatusCode) + } + r = env.authedGet(t, sess, "/v1/me") + r.Body.Close() + if r.StatusCode != http.StatusUnauthorized { + t.Fatalf("session still valid after retried revoke-all: %d", r.StatusCode) + } +} + // TestRevokeAll_RevokeSurvivesClientDisconnect: a client that disconnects // mid-call must not abort the revocation — the revoker runs on a detached, // bounded context. diff --git a/web/src/api/generated/schema.d.ts b/web/src/api/generated/schema.d.ts index 2535f722..49d0a3bd 100644 --- a/web/src/api/generated/schema.d.ts +++ b/web/src/api/generated/schema.d.ts @@ -83,6 +83,13 @@ export interface paths { * `oidc.postLogoutRedirect` — never from the request (no open redirect). * The session keeps no ID token, so `id_token_hint` is not sent. * Otherwise the answer is `204` and the portal stays local. + * + * The server-side session delete is the point of no return: when the + * session store cannot delete it, the answer is `503` `UNAVAILABLE` + * with `Retry-After` — the cookie is NOT expired and no session-bound + * leases or launch tickets are revoked, so the caller is still fully + * signed in and may simply retry. A success status is only ever sent + * after the session row is gone. */ post: operations["logout"]; delete?: never; diff --git a/web/src/i18n/en/app.ts b/web/src/i18n/en/app.ts index c588dc2a..9cdea5cc 100644 --- a/web/src/i18n/en/app.ts +++ b/web/src/i18n/en/app.ts @@ -28,7 +28,8 @@ export default { "app.user.signOutAll.confirm": "Sign out everywhere", "app.user.signOutAll.busy": "Signing out…", "app.user.signOutFailed.title": "Could not sign out", - "app.user.signOutFailed.body": "Try again. If it keeps failing, close this browser window.", + "app.user.signOutFailed.body": + "You are still signed in. Try again — if it keeps failing, close this browser window.", "auth.signedOut.title": "You have signed out", "auth.signedOut.body": "Your session has ended. Close this window or sign in again.", diff --git a/web/src/i18n/vi/app.ts b/web/src/i18n/vi/app.ts index cd1fa39e..1f584c1b 100644 --- a/web/src/i18n/vi/app.ts +++ b/web/src/i18n/vi/app.ts @@ -30,7 +30,8 @@ export default { "app.user.signOutAll.confirm": "Đăng xuất khỏi mọi nơi", "app.user.signOutAll.busy": "Đang đăng xuất…", "app.user.signOutFailed.title": "Không đăng xuất được", - "app.user.signOutFailed.body": "Thử lại. Nếu vẫn lỗi, hãy đóng cửa sổ trình duyệt này.", + "app.user.signOutFailed.body": + "Bạn vẫn đang đăng nhập. Thử lại — nếu vẫn lỗi, hãy đóng cửa sổ trình duyệt này.", "auth.signedOut.title": "Bạn đã đăng xuất", "auth.signedOut.body": "Phiên của bạn đã kết thúc. Đóng cửa sổ này hoặc đăng nhập lại.", diff --git a/web/tests/unit/auth/signout.test.tsx b/web/tests/unit/auth/signout.test.tsx index 75e6eb10..d751e6cf 100644 --- a/web/tests/unit/auth/signout.test.tsx +++ b/web/tests/unit/auth/signout.test.tsx @@ -282,4 +282,37 @@ describe("user menu", () => { Object.defineProperty(window, "location", { value: loc, configurable: true }); } }); + + it("a refused sign-out (503 UNAVAILABLE) can simply be retried", async () => { + const api = createMockApi(); + const base = api.handle; + let storeDown = true; + api.handle = (req) => + storeDown && req.path === "/v1/logout" + ? { + status: 503, + headers: { "content-type": "application/json", "retry-after": "5" }, + body: { code: "UNAVAILABLE", message: "could not sign out; retry", retryable: true }, + } + : base(req); + renderShell(api); + const loc = window.location; + const assign = vi.fn(); + Object.defineProperty(window, "location", { value: { ...loc, assign, pathname: loc.pathname }, configurable: true }); + try { + fireEvent.click(await screen.findByRole("button", { name: "Ada Admin" })); + fireEvent.click(screen.getByRole("menuitem", { name: "Sign out" })); + // Still on the page, still signed in — the failure is surfaced. + expect(await screen.findByText("Could not sign out")).toBeInTheDocument(); + expect(assign).not.toHaveBeenCalled(); + + // The store recovers: the same menu action retries and signs out. + storeDown = false; + fireEvent.click(await screen.findByRole("button", { name: "Ada Admin" })); + fireEvent.click(screen.getByRole("menuitem", { name: "Sign out" })); + await waitFor(() => expect(assign).toHaveBeenCalledWith(SIGNED_OUT_PATH)); + } finally { + Object.defineProperty(window, "location", { value: loc, configurable: true }); + } + }); });