fix(api): sign-out fails closed when the session delete does not commit (FIX-LOGOUT)

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 |

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)
 	}

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)
+	}
+}

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":

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.

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;

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.",

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.",