fix: make revocation generation cover refresh tokens (#2028)

[bolichen97]

Problem

kirocrew logout (CLI → POST /api/logout → revoke_all_sessions()) ends access sessions but not refresh chains: validate_refresh_token() checks HMAC, kind, session_exp, and per-chain revocation, but never consults the revocation generation, and generate_refresh_token() embeds no gen claim. A browser still holding a valid mc_refresh_<port> cookie can call POST /api/auth/refresh after logout and mint a fresh access cookie carrying the new generation. There is also no global refresh-chain revocation path — revoke_chain is per-chain and only reachable from the target browser's own cookie.

Why it matters

An operator running kirocrew logout reasonably expects every outstanding session to end. Sessions that silently survive the documented "revoke all sessions" control are a behavior/consistency gap (documented as a known limitation in the docs; this PR removes the gap the docs describe).

Fix (symptom → root cause → change)

Symptom: sessions survive kirocrew logout via the refresh path. Root cause: the persisted revocation-generation counter is embedded and checked only in access tokens. Change: make the counter authoritative over BOTH cookie types — the issue's preferred option, reusing the already-persisted counter with no chain enumeration.

• New src/kiro_crew/dashboard/revocation_gen.py — the counter's canonical home (current_revocation_gen() / bump_revocation_gen()), extracted from token_auth.py so refresh_tokens.py can consult it without recreating the known token_auth ↔ refresh_tokens import cycle (same seam as token_secret.py). Loading is lazy + lock-guarded; both validators read the LIVE value through the accessor. A failed disk read is not memoized (answers 0 for that call, retries next call), so a transient startup read error cannot permanently un-revoke sessions. warm_auth_singletons() primes it off the event loop for both server entry points.
• token_auth.py — re-exports the old names (_REVOCATION_FILE, _load_revocation_gen, _bump_revocation_gen) for backwards compatibility; generate_token / validate_token now go through the accessor.
• refresh_tokens.py — generate_refresh_token() embeds "gen": current_revocation_gen(); validate_refresh_token() rejects gen < current with reason "session revoked", mirroring the access-cookie semantics. Also corrects the stale RefreshStateManager.clear_all() docstring (logout never calls it — and must not, since it would clear _revoked_chains).
• Docs — dashboard-token-auth.md, remote-and-mobile.md, slack-setup.md, cli.md updated in the same commit: the generation counter is authoritative over both cookie types; restart semantics unchanged (counter reloads unchanged, logs nobody out); POST /api/auth/logout remains the narrower per-browser control.

Upgrade consequence (deliberate fail-closed posture): refresh tokens minted before this claim existed default to gen 0, so on installs where the operator has ever run kirocrew logout, existing refresh chains are rejected once and users re-mint via the kirocrew token URL. Installs that never ran a logout (gen still 0) are unaffected. Access-cookie behavior is unchanged.

Tests

Extends test/test_refresh_tokens.py (TR-U-28..32):

• Refresh token minted, then revoke_all_sessions() → invalid with reason "session revoked" (identity/claims still surfaced for audit).
• Refresh token minted after the bump validates.
• Legacy payload with no gen claim: valid while gen==0, rejected once gen>0 (fail-closed).
• Handler-level: POST /api/auth/refresh with a pre-logout refresh cookie → 401.
• A failed counter read is not memoized (retry restores the persisted gen).

Existing chain-revocation, reuse-detection, and access-cookie gen tests pass; fixtures in test_token_auth.py / test_refresh_tokens.py updated to pin the moved counter.

Manual verification

N/A — unit coverage sufficient: the change is confined to token mint/validate paths exercised end-to-end by the handler-level test.

Closes #2028

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7b2c5b550796d53e94fbea8fb4ec27220b7583a9 — this comment is updated in place on each push.

Review details

This is a well-scoped, well-tested security fix (extending the persisted revocation generation to cover refresh tokens, with fail-closed I/O handling). Let me verify the two things that could be residual defects: a new blocking call on the event loop, and whether any production caller is broken by the new OSError.

The only production caller of revoke_all_sessions() is api_logout (core.py:1995), which now wraps it in try/except OSError. The synchronous file write in bump_revocation_gen() pre-existed (the old _bump_revocation_gen() also did p.write_text), so no new blocking call is introduced. Validation-path disk reads are primed by warm_auth_singletons() before the loop serves connections, and the fallback is a few-byte read, not "large" I/O. Fail-closed posture is deliberate and tested. No frontend changes.

No findings.

[OPUS-REVIEWED] 7b2c5b5

Verdict parsed from the review's SHA-scoped output markers for commit 7b2c5b550796d53e94fbea8fb4ec27220b7583a9.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7b2c5b550796d53e94fbea8fb4ec27220b7583a9: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of 7b2c5b550796d53e94fbea8fb4ec27220b7583a9 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Root-cause fix in the right seam: one persisted counter now governs both token kinds, mirroring the existing token_secret.py cycle-breaker, with docs updated in-commit.

Suggestions

• The fail-closed unreadable-counter path ("revocation state unavailable") has no self-heal: a pre-upgrade torn write leaves an empty file that rejects every non-loopback request until someone hand-deletes token_revocation.gen — surface the remediation in the log/error so a remotely locked-out operator knows the one-file fix.

[DESIGN-REVIEWED] 7b2c5b5

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7b2c5b550796d53e94fbea8fb4ec27220b7583a9 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7b2c5b5

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7b2c5b550796d53e94fbea8fb4ec27220b7583a9: <one-sentence reason>

[bolichen97]

Dispositions for review findings on 70a115db7f5a0bbac07bbece3e0abd5855e9da15 (superseded by d30dde2):

GPT 5.6 BLOCKING — revocation_gen.py: counter I/O failures reopen revoked sessions → fixed in d30dde2. Both validators now call current_revocation_gen_or_none() and reject with "revocation state unavailable" when the persisted counter cannot be read (the next validation retries); bump_revocation_gen() raises OSError instead of bumping from an assumed base when the read fails, and re-raises on a failed write so POST /api/logout returns 500 revocation_persist_failed rather than a false success (in-memory revocation still holds for the process). Locked in by test_tr_u_33/34/35 in test/test_refresh_tokens.py.

Design Review suggestion — bump from base 0 can overwrite a higher on-disk counter (5 → 1) → fixed by the same change: the bump refuses to proceed without a readable base, so a lower value can never be persisted over a higher one.

[bolichen97]

Disposition for the finding on d30dde2710c2e677d0e45f7dad15490418fc9bd0 (superseded by 0b32d16):

GPT 5.6 BLOCKING — revocation_gen.py: torn counter writes can resurrect revoked sessions → fixed in 0b32d16, exactly as suggested: bump_revocation_gen() now persists via a same-directory temporary file + os.replace (the on-disk file always carries either the old or the new complete value), and _load_revocation_gen_or_none() treats an existing-but-EMPTY file as unreadable (evidence of an interrupted write) rather than gen 0 — validators then fail closed with "revocation state unavailable". A missing file remains a definitive gen 0 (fresh install). Locked in by test_tr_u_36 (empty file → reject) and test_tr_u_37 (atomic persist, no tmp residue) in test/test_refresh_tokens.py.

[bolichen97]

Dispositions for findings on 0b32d16ed4943a4e956673d0d7caaebe565aa5e2 (superseded by 7b2c5b5):

GPT 5.6 BLOCKING — revocation_gen.py: failed persistence publishes a generation that can survive later logout → fixed in 7b2c5b5 exactly as suggested: bump_revocation_gen() now holds _gen_lock through persistence and assigns _gen only AFTER os.replace succeeds. On a failed write the counter is unchanged in memory and on disk, so no token can ever be minted with an unpersisted generation. test_tr_u_35 rewritten to pin the new invariant (counter unchanged after a failed persist).

SAST (Semgrep) — python-logger-credential-disclosure on the empty-file warning → rebutted as false positive, suppressed inline: the log line emits only the counter file PATH (%s = Path), never a token or secret value; the rule fires on the credential-adjacent wording of the static message. Annotated with the repo's established # nosemgrep: python-logger-credential-disclosure pattern (same convention as token_secret.py and refresh_tokens.py).

[bolichen97]

Disposition for the Design Review suggestion on 7b2c5b5 (advisory, PASS verdict):

No self-heal / remediation not surfaced for the unreadable-counter lockout → accepted-and-deferred: #2393. Rationale for deferring rather than amending this PR: the scenario requires state corrupted BEFORE this upgrade (post-upgrade writes are atomic via tmp + os.replace, so this PR cannot create the empty file it fails closed on), the gateway log already names the file path, and the fix wanted (operator-facing remediation text, possibly on the 403 surface) deserves its own small change rather than another full CI round on a review-ready security fix.