fix(auth): bind refresh chains to the tailnet peer that opened them

[iamwhatever]

Problem / Motivation

A dashboard refresh cookie stolen from allowed tailnet node A and replayed from allowed node B rotates successfully, and the replacement access token comes back pinned to B.

Phase 3 pins ACCESS sessions to a daemon-verified tailnet peer, and #2411 made rotation carry that pin forward. The peer-bound rotation mechanism (require_peer / peer_key) exists and works — /api/auth/refresh verifies the carried key against the daemon-verified peer and refuses with peer_mismatch / peer_unverified / peer_binding_missing before any mint. But it was armed for exactly one producer: the persistent QR phone session in tailnet_mobile.py. Every ordinary Phase-3 session opened an unbound chain and took the else branch into _rebind_rotated_token_to_peer, which re-binds the rotated token to whoever presents it next.

Proven on origin/main before writing the fix: an ordinary ?token= exchange with a verified laptop peer mints a chain with require_peer absent; replaying that cookie with the phone resolving returns 200 and check_token_peer(new_access, PHONE_KEY) is True.

Why it matters

The refresh cookie is a 30-day bearer credential, and the chain was the laundering path around the access token's own device pin. Anyone who obtains one — a backup, a shared browser profile, a second machine the user also owns and someone else can reach — converts it into a fresh, fully-pinned session on their own device, and the audit trail attributes it to them as a legitimate peer. kirocrew logout is the only thing that ends it, which the remote-and-mobile guide already documents as a known gap.

What changed (motivation → approach → change)

The existing mechanism is extended to ordinary Phase-3 sessions; no second mechanism is introduced.

• token_auth — an exchange that resolves a daemon-verified, allowlisted peer now opens its chain with require_peer + that peer's peer_key. Gated on a resolved peer, not on the pin key: that key is ip:<addr> when nothing resolved, and binding a chain to the tunnel's shared loopback address would read as a pin while excluding nobody.
• refresh_tokens — refresh_chains.json gains a chain_peers record (chain_id → peer_key, exp): evicted with its chain, dropped on revocation, and re-stamped inside the same locked write as the consumption, so one rotation still costs one state write and can never record a spent jti while losing the binding that decides who may spend its replacement. Malformed records are skipped rather than fatal, mirroring the existing exp guard.
• auth_refresh — the signed claim and the server-side record are two independent authorities, and the request must satisfy every key either of them names. The record earns its place because this gap was one mint path carrying the claim while the others did not: a rotation path that drops the claim is caught by the record it cannot influence. It does not cover a future mint path that omits both — such a chain has no claim and no record, which is indistinguishable from a legitimate pre-upgrade chain and so rotates unbound. Making that detectable would need a positive marker on every unbound chain (a schema version, or an explicit unbound record) so absence stops meaning "legacy"; that is a wider design move than this PR, and moot while the legacy-chain question below is still open. Credit to Design Review for catching the overclaim.
• dashboard.tailscale.bind_refresh_chains (default true) is the operator's opt-out. Narrowing-only like the two existing tailscale load rules — a non-boolean resolves to true — so a typo can only ever leave the binding on.

Security property now enforced: a refresh chain opened by a daemon-verified tailnet peer can only be rotated for that same peer key. A different node (node scope) or a different login is refused with peer_identity_mismatch and audited. The refusal does not revoke the chain: identity resolution fails transiently as often as maliciously, and burning a 30-day credential over a daemon blip would turn a recoverable hiccup into a re-mint.

The roaming tradeoff @NicholasRBowers raised

pin_scope already owns roaming, and the binding respects it rather than overriding it:

• At login scope the pin key is ts:login:<login>, so the same person's other device rotates normally. Roaming works with the binding on. (Test: test_login_scope_still_roams_between_the_users_own_devices.)
• At node scope (the default) the ACCESS token was already device-pinned in-memory. So an unbound chain was the only thing making cross-device use appear to work — by laundering a fresh pin on rotation, which is the defect itself, not a feature the guide promised.
• bind_refresh_chains: false covers the operator who genuinely needs cross-device roaming at node scope and accepts that a stolen refresh cookie then renews from any allowed node. Documented as that tradeoff in both the guide and the spec.

Migration

Absence, per the issue's scope note. A chain with neither the signed claim nor a chain_peers record is unbound — that is every chain outstanding at upgrade, and every session opened with no verified peer — so the change logs nobody out by itself.

Unreadable persisted state fails closed, and does not crash

Three review rounds landed on this one function from opposite directions. The
invariant now shipped satisfies all of them:

Absence is fine; corruption of a PRESENT security record is not.

State of refresh_chains.json	Reading	Why
missing / unparseable	start empty (unchanged, pinned)	atomic_write makes a torn write impossible, so a file that will not parse is "not our state", not "our records, lost"
key absent	no records	a fresh install has revoked nothing; refusing here would deny rotation on every healthy system
key present, not a list	degraded — refuse	the records existed and cannot be read, so no control can be proven satisfied
top-level valid-JSON non-dict	degraded — refuse	it parsed, so this is not the unreadable-file case: something replaced our records with a document that cannot hold any
one element not a mapping	filtered	junk beside good entries is not evidence the good ones are untrustworthy

Round 1 (GPT, BLOCKING) was that a malformed container raised inside the
RefreshStateManager constructor — which runs from _get_state() — so one bad
byte-range 500'd every /api/auth/refresh until the file was hand-repaired.
A .get(key, []) default does not cover it: the key exists, so the default is
never consulted.

Round 3 (GPT, upheld by the Opus adjudication) was that reading it as empty
is the opposite defect, and the more serious one: revoked_chains read as empty
makes is_chain_revoked answer False for a chain POST /api/auth/logout
already killed, so a stolen cookie rotates into fresh credentials. Nothing
self-corrects it — reuse detection only re-fires on a jti replay the attacker
need not cause.

So _record_list returns None for a present-but-unreadable list, _load
records which keys were unreadable, and validate_refresh_token refuses with
"refresh state unavailable" — placed before is_chain_revoked, since that
is the check being bypassed. It is the same posture, and the same wording shape,
the function already applies twelve lines down when the revocation counter cannot
be read. _persist also refuses while degraded: writing our empty in-memory
state over the operator's file would destroy the records and let the next start
load a clean store and resume rotating, converting the refusal into the bypass it
exists to prevent.

Availability cost, stated plainly: all rotation stops until the file is
repaired or removed. The ~20h access cookie masks it, revocation_gen lives in
a separate file so kirocrew logout still works, and the condition is loud
(logger.error naming the path and the affected keys).

The seam this leaves, named rather than shipped quietly. An unparseable or
deleted file still starts empty, so {"revoked_chains": null} refuses all
rotation while rm refresh_chains.json silently drops the same revocations. That
asymmetry is deliberate and is scoped to accidental corruption — a hand-edit,
a foreign or legacy format, a bad restore. It is not a defence against an
adversarial write, and cannot be: this file lives beside token_signing.key in
the same 0700 data home, so anyone who can write one can write the other and
mint arbitrary tokens outright. Closing the seam properly would need a positive
"I have no records" marker (a schema version, or a sentinel) so that absence
stops being ambiguous with fresh install — a wider format change than this PR
should carry, and the reason the unparseable-file branch is left exactly as
test_tr_i_17_corrupted_state_file_starts_empty pins it. A regression test now
asserts that boundary so a future round of this argument cannot quietly widen
into it.

⚠️ Residual tradeoff, flagged for a maintainer call

The require_peer claim is carried onto both halves of the rotated pair. Binding only the chain would leave the access cookie with the same laundering shape one credential over: after a restart the in-memory pin map is empty, so a stolen access cookie is re-pinned to whichever allowed node presents it first. Binding both closes that sibling hole for free and keeps the two credentials consistent.

The cost: an access cookie carrying require_peer fails closed when no peer resolves, so a session bound this way needs one re-mint via the kirocrew token URL after an operator switches identity trust off. That is the same fail-closed posture the QR require_peer shape already has, but it is new for ordinary sessions, so it is a maintainer decision rather than mine. The narrower alternative — bind the chain only, leave the rotated access token as today — is a two-line change if you prefer it; I did not take it because it leaves the post-restart takeover hole open on the access cookie.

Tests

test/test_refresh_peer_binding.py — 23 tests, red before / green after. The exploit was reproduced against unmodified main first (see Manual verification).

Test	Locks in
test_stolen_refresh_cookie_cannot_rotate_from_another_allowed_node	the exploit end to end: steal from laptop, replay from phone → peer_identity_mismatch, no Set-Cookie, jti not consumed, chain not revoked
test_the_original_node_still_rotates_its_own_chain	the legitimate device keeps its session, and the binding survives onto the rotated pair (so the second rotation is not silently unbound)
test_ordinary_phase3_exchange_binds_the_chain_it_opens	the mint side — the gap was here
test_the_chain_binding_is_persisted_for_the_next_gateway	the on-disk chain_peers shape, and a fresh manager reading it back (the restart case)
test_a_non_tailnet_exchange_still_opens_an_unbound_chain	ordinary token+IP installs are untouched
test_the_rotated_access_token_carries_the_signed_device	both halves of the rotated pair carry the binding
test_a_rotated_access_token_is_refused_for_another_node_after_restart	no first-arrival takeover with an empty pin map, and the original device still authenticates
test_login_scope_still_roams_between_the_users_own_devices	the roaming resolution above
test_the_opt_out_restores_the_unbound_chain	bind_refresh_chains: false
test_a_pre_upgrade_chain_keeps_todays_unbound_semantics	the migration rule
test_the_persisted_record_alone_refuses_a_different_peer	the record as an independent authority, with no claim on the token at all
test_a_bound_chain_cannot_rotate_while_identity_is_unverifiable	fail closed on an unresolvable peer, without revoking
test_revoking_a_chain_drops_its_peer_record	no binding outliving its chain
test_an_expired_chain_binding_is_evicted	the state file stays bounded
test_a_malformed_chain_binding_does_not_brick_the_store	one bad record does not take every refresh with it
test_a_wrong_shaped_state_file_does_not_500_every_refresh	the round-1 blocker, parametrized over all three record keys and three non-object documents
test_a_wrong_shaped_state_file_still_serves_a_rotation	the same finding at the endpoint it broke — /api/auth/refresh answers 200, not 500

Regression sweep — this is exactly the code where a narrow fix breaks a legitimate flow, so the sweep is wider than the new file: 875 passed, 3 skipped, 0 failed across the auth / refresh / tailnet / token_auth / config-loader / config-baseline / peer-auth / mobile-link selection, including all 50 existing test_auth_refresh_handlers_cov80.py tests and the full test_refresh_tokens.py and test_token_auth.py files. An earlier sweep on the pre-rebase tree ran 4444 tests over a wider selection with the same result.

Gates, all green on the rebased tree: black (baselined), isort, flake8, mypy (1288 files, no issues), docs-lint (261 files), plus the repo's check_* gates for subprocess encoding, agent-SDK boundary, sync-IO-in-async, lockdown-before-publish, loop-bound locks, builtin-skill scope, testpaths coverage, brand name, harness parity, feature map, changelog history, and focus cue. config-baseline.json was regenerated for the new config field.

The diff-scoped runner escalates to the full backend suite for this surface; that full run is deferred to CI's sharded Backend Tests lanes, which is where it completes in reasonable time.

Manual verification

The exploit was reproduced as an executable probe against unmodified origin/main before any source change — the ordinary ?token= link exchange minted require_peer-absent, the phone's replay returned 200, and check_token_peer(new_access, PHONE_KEY) was True. That probe is not committed; its assertions live on as test_stolen_refresh_cookie_cannot_rotate_from_another_allowed_node with the outcome inverted.

One nosemgrep added, for a proven false positive. python-logger-credential-disclosure
fired on the two new logger.warning calls in refresh_tokens.py, and the
"potential hardcoded secret" it names is the format string itself — the
literal begins "refresh_tokens: ...", which contains "token". Neither call logs
a credential: one logs a JSON field name ("chain_peers" / "consumed_jtis" /
"revoked_chains") plus a type name, the other a file path plus a type name.
Suppressed in the form this repo already uses for this exact rule
(sel.py:1388, slack/scope_probe.py:65, providers/acp.py:1108,
mcp_gateway/credwatch.py:206): a comment naming what is logged, then the rule
id inline. The module's four pre-existing "refresh_tokens: ..." log calls carry
the same false positive and are simply baselined, since the gate scans diff-only.
Rewording was the alternative, but it would break the six-call prefix convention
this module relies on for log grepping.

Not exercised against a live tailnet: every test fakes resolve_forwarded_peer, which is how the existing peer tests in this area work too, so the daemon-facing half (tailscale whois behind tailscale serve) is unchanged and uncovered here as before.

Screenshots / video

Why no screenshot: backend-only change — auth handlers, the refresh-token module, config schema, and docs. No frontend file is touched and no rendered surface changes (CI's Frontend Lint & Type Check and Bundle Size Gate both skip on this diff).

Related Issues

Follow-up from #2411 (issue #1762, RFC rfc-tailnet-dashboard-access Phase 3).

Closes #2417

Pattern harvest

Rule candidate: review-prompt

Pattern: a security mechanism whose enforcement is complete but whose arming has exactly one producer.

The verify side here was correct and well-tested from day one; the defect was that only one of several mint paths set the claim it verifies, so the check was unreachable for every other session shape. Worth asking on review of any new signed claim: which mint paths can reach this check, and which silently bypass it by omitting the claim? The same question applies to boot, no_refresh, and embed_parent_port in this file, each of which carries its own "carried, never re-derived" comment for the same reason.

Design Review sharpened this into the form worth keeping: a second authority protects the paths that carry part of the state, but absence is not detectable — a producer that omits every marker is indistinguishable from a legitimate legacy record. So the durable guard for the next producer is the review question above, not the record.

Also generalizable as a design rule, applied in this PR: when a signed claim gates a security decision, keep a server-side record the credential cannot influence as a second authority, so the next mint path that forgets the claim fails closed instead of silently unbound.

Review dispositions

Every raised concern is answered in its own comment on this PR. Summary:

Lane	Finding	Disposition
GPT r1	malformed chain_peers crashes every refresh	fixed — shared chokepoint, 7 shapes, 5 pre-existing
GPT r2	tests leak process-global auth state	fixed — the isolation fixture test_token_auth.py already uses; leak measured (10 bindings + a pinned store)
GPT r3	malformed security lists must not read as empty	needs-a-decision — hit #2 on this span, and it asks the opposite of r1; the coherent invariant reverses a pinned test
GPT r3	legacy chains replayable across peers	needs-a-decision — the migration ruling #2417 deferred to a named owner
Design	access-token binding is a wider change	needs-a-decision — escalated, not merged by default
Design	record does not cover a forgetful mint path	accepted — overclaim removed from this body
First Principles	chain_peer() wrapper has 0 consumers	fixed — deleted
First Principles	the opt-out serves the case the PR calls the defect	needs-a-decision — keep with an explicit yes, or drop it

Four decisions are with the maintainer; nothing else is outstanding.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 2031ba8c21c8d86fecac4fef29d52afe8138207c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound extension of an existing mechanism, but the shipped corrupt-state posture contradicts the PR body, and two flagged tradeoffs genuinely need the maintainer call.

Watch

• Description ↔ diff conflict on corrupt state. The round-1 section claims wrong-shaped records "resolve to 'no records'" because a lockout is worse than lost reuse state — but the shipped code does the opposite: _corrupt_keys makes validate_refresh_token refuse every rotation and _persist stops writing until the file is hand-repaired. The fail-closed direction is defensible for security lists, but the body promises the fail-open one; a maintainer reading the description will approve behavior the code doesn't have.
• The split posture is one deletion from fail-open. An unparseable file still loads empty (pinned by test_tr_i_17), so {"revoked_chains": null} locks out all rotation while rm refresh_chains.json or truncation silently drops the same revocations. The guard only covers accidental corruption, not the adversarial write it's phrased against — fine, but the queued GPT-r3 decision should settle one coherent invariant, not ship the seam.
• Access-cookie binding fail-closed is a new operational requirement: ordinary sessions now need a kirocrew token re-mint after an operator disables identity trust. Correctly escalated in the body; confirm it before merge since it changes behavior for every Phase-3 session, not just the exploit path.

Suggestions

• Rewrite the "Malformed persisted state cannot deny auth" section to describe the fail-closed behavior actually shipped, and rename it — its current title asserts the inverse of the code.

[DESIGN-REVIEWED] 2031ba8

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 2031ba8c21c8d86fecac4fef29d52afe8138207c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

The fix earns its place; the server-side chain_peers record's own description concedes it cannot catch the defect class it was added for.

What this change ships

Intent: stop a stolen refresh cookie from renewing a session on a different allowed device — a FIX.

1. Stolen refresh cookie no longer rotates from another allowed node — justified (the fix).
2. Ordinary tailnet exchanges now mint peer-bound chains via the existing require_peer mechanism — justified.
3. Rotated access cookie carries the device claim; identity-trust-off now ends bound sessions — declared tradeoff.
4. Server-side chain_peers record in refresh_chains.json — one writer, one reader; conceded-narrow value.
5. New config key dashboard.tailscale.bind_refresh_chains, default on — declared opt-out.
6. That key is editable from the dashboard settings API (core.py:1850) — undeclared.
7. A corrupt state file now refuses rotation (401) instead of 500ing every refresh — justified, general fix (5 pre-existing shapes, pinned by tests).
8. Degraded store never overwrites the unreadable file — justified.
9. Peer-mismatch refusal neither consumes nor revokes the chain — justified.
10. Pre-upgrade chains stay unbound and keep rotating — justified migration.

Watch

• The record's justification is "a record the presented token cannot influence makes the next forgetful mint path fail closed" — but the same description says "It does not cover a future mint path that omits both", and the claim is HMAC-signed, so an attacker cannot strip it. What remains is a claimless server-minted token for a recorded chain: there is exactly one rotation path, and it is the function that reads the record. Grepped chain_peer|bind_chain_peer: 1 producer (token_auth.py:3027), 1 reader (auth_refresh.py:493); everything else is the record's own persistence machinery. Its zero option costs nothing observable today, and its cost is a persisted schema field whose absence-means-unbound semantics must hold forever.
• Roaming now has two knobs: pin_scope: "login" (which the description says "already owns roaming") and bind_refresh_chains: false (1 decision site, token_auth.py:3004). The opt-out's distinct beneficiary — node-scope pinning plus a chain that roams — renews by re-pinning on rotation, the very laundering shape the PR names as the defect; and sustained two-device use of one chain trips same-IP reuse detection anyway.

Subtractions

• Drop the chain_peers server-side record: bind_chain_peer (both forms), chain_peer, mark_consumed's peer_key parameter, and the persist/load/evict/revoke handling — the signed claim alone refuses the exploit end to end (the module's own exploit test exercises only claim-bound chains minted by the exchange). The _record_list chokepoint stays: it fixes five pre-existing crash shapes independent of this record.

[FIRST-PRINCIPLES-REVIEWED] 2031ba8

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 2031ba8c21c8d86fecac4fef29d52afe8138207c and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 2031ba8

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2031ba8c21c8d86fecac4fef29d52afe8138207c — this comment is updated in place on each push.

Review details

The candidate list contained no candidates, and my independent trace of the security-critical path confirms the logic is sound: the degraded store fails closed in validate_refresh_token before any credential is issued (line 443); the peer check is sited ahead of both reuse detection and mint (line 505); tightest-wins requires the verified peer to match every key either authority names; and a refusal neither consumes the jti nor revokes the chain. The migration/absent-record, opt-out, and disagreement cases all resolve correctly. Nothing survives to report.

No findings.

[OPUS-REVIEWED] 2031ba8

Verdict parsed from the review's SHA-scoped output markers for commit 2031ba8c21c8d86fecac4fef29d52afe8138207c.

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

[iamwhatever]

BLOCKING — Malformed chain-peer state crashes refresh authentication (span=dc2f58772b80, src/kiro_crew/dashboard/refresh_tokens.py:383) — fixed in 0fe61d63ad3c13ae2ee5cc84329d0434601e402f.

{"chain_peers": null} -> state initialization -> TypeError -> every refresh returns 500 until manual repair.
Fix: Validate chain_peers is a list before iterating; otherwise treat it as empty.

Legitimate, and the consequence is worse than a crash in isolation: the raise happens inside the RefreshStateManager constructor, which runs from _get_state(), so one malformed byte-range in an operator-visible state file denies /api/auth/refresh for every user until someone hand-repairs the file. Worth stating explicitly for the next reader: .get(key, []) does not protect against this, because the key is present — the default is only consulted for a missing key.

Fixed at a shared chokepoint (_record_list) rather than at the reported key, because the defect was never specific to chain_peers. Proven red-before across seven shapes, three of which are pre-existing on main:

Shape	Raised	Introduced by
{"chain_peers": null}	TypeError	this PR (as reported)
{"chain_peers": {...}}	— (old per-entry isinstance already skipped it)	this PR
{"consumed_jtis": null}	TypeError	pre-existing
{"revoked_chains": null}	TypeError	pre-existing
[] (document is a list)	AttributeError on .get	pre-existing
"nonsense"	AttributeError on .get	pre-existing
null	AttributeError on .get	pre-existing

Guarding only the reported key would have left the identical crash on its two siblings three lines away, and left the whole-document case unguarded ahead of all three.

All seven now resolve to "no records" — the same direction _load's existing JSONDecodeError handler already takes for an unreadable file. That choice is deliberate: losing reuse-detection state is bounded and self-healing (the affected tokens simply rotate again), while an auth endpoint that 500s until manual repair is a total dashboard lockout. A wrong-shaped container is logged at warning so it is diagnosable rather than silent.

Coverage: test_a_wrong_shaped_state_file_does_not_500_every_refresh (parametrized over all seven shapes) plus test_a_wrong_shaped_state_file_still_serves_a_rotation, which asserts the endpoint answers 200 rather than 500 — i.e. the consequence you named, not just the constructor. Verified red on the reverted production hunk and green after; 895 tests pass across the auth/refresh/tailnet/config families.

[iamwhatever]

BLOCKING — exchanges leak process-global authentication state (span=4a263decb765, test/test_refresh_peer_binding.py:145) — fixed in 45cf3e37082924f2b2dc49b558eec3a24a170185.

Repeated tests -> shared peer bindings and revoked-nonce singleton persist -> later posture reads stale sessions and writes into a prior test's isolated home.
Fix: Add an autouse fixture that isolates/restores ta._state and resets ta._revoked_store_singleton.

Correct on both halves, and measured rather than reasoned about. I removed the proposed fixture and ran this file followed by a canary asserting the globals are clean:

AssertionError: LEAKED 10 peer binding(s) into the global map
AssertionError: LEAKED a revoked-nonce store singleton pinned to a prior test's tmp home:
  .../pytest-0/popen-gw0/i0/1-kirocrew-home/token_revoked_nonces.json

The second line is the sharper half and is exactly what you described: _get_revoked_store() builds its singleton once per process from whichever KIROCREW_HOME the first caller happened to see, so the store outlived the test that created it and a later test would write through it into an already-deleted tmp home. conftest pins KIROCREW_HOME per test, which bounds the blast radius to the suite — but it does not re-derive a singleton that is already built, so the leak survives it.

Root cause is deliberate and stays: _exchange drives the real token_auth_middleware rather than a stub, because the mint-side gap this PR fixes lives in that middleware — a stubbed exchange could not have caught it. So the fix is isolation, not avoidance.

Fixed with the fixture test/test_token_auth.py:46 already uses for these exact singletons — same pinned _gen, same _revoked_store_singleton = None, same _state.clear_all(), same _app_perms_cache.clear() — rather than a second isolation shape for the same state. Two details worth naming: it resets on the way in as well as out, since the leak can arrive from an earlier module just as easily as leave for a later one; and it uses _state.clear_all() rather than revoke_all_sessions(), because bumping the revocation generation between unrelated tests would reject every token minted before the bump.

Canary green after the fix, and it also contains the one test that deliberately clears _peer_bindings to simulate a gateway restart — that mutation is now scoped to its own test. 875 passed / 3 skipped / 0 failed across the auth, refresh, tailnet, token_auth, config-baseline, peer-auth and mobile-link families.

[iamwhatever]

• BLOCKING — Malformed state silently disables refresh revocation (span=dc2f58772b80, src/kiro_crew/dashboard/refresh_tokens.py:168) — fixed in bbfb364fda2e2a787eee0d5f781cc39e75bf7979. Hit refactor: remove dead legacy compatibility shims #3 on this span; last round I called it needs-a-decision and that was my error, which the adjudication is what exposed.

Silently dropping malformed revoked_chains/consumed_jtis is a non-self-healing revocation bypass; though the malformed shape is not writer-producible, the stolen-live-chain window persists silently with no automatic recovery, so the unbounded security-class risk stands.

What I got wrong. I argued this could not be fixed without reversing test_tr_i_17_corrupted_state_file_starts_empty, a behaviour a maintainer pinned in an earlier review round, and routed it to the maintainer on that basis. Re-reading _load, that is simply false: the JSONDecodeError handler returns at line 398-404, before any record decoding. The unparseable-file path and the malformed-container path are different branches, so the second can fail closed while the first keeps its pinned behaviour untouched. The adjudication's insistence that the recovery record "cannot be completed as clean self-heal" is what sent me back to look properly.

The invariant now implemented, which satisfies every round on this span at once rather than trading one off against another:

Absence is fine; corruption of a PRESENT security record is not.

State of the file	Reading	Why
missing / unparseable	start empty (unchanged, pinned)	atomic_write makes a torn write impossible, so a file that will not parse is "not our state", not "our records, lost"
key absent	no records	a fresh install has revoked nothing; refusing here would deny rotation on every healthy system
key present, not a list	DEGRADED — refuse	the records existed and cannot be read, so no control can be proven satisfied
top-level valid-JSON non-dict	DEGRADED — refuse	it parsed, so this is not the unreadable-file case: something replaced our records with a document that cannot hold any
one element not a mapping	filtered	junk beside good entries is not evidence the good ones are untrustworthy

_record_list returns None for the degraded cases, _load records which keys were unreadable, and validate_refresh_token refuses with "refresh state unavailable" — placed before is_chain_revoked, since that is the check being bypassed. It is the same posture and the same wording shape the function already applies twelve lines down when the revocation counter cannot be read.

Two consequences worth calling out because neither is obvious. First, _persist now refuses while degraded: writing our empty in-memory state over the operator's file would destroy the records and let the next start load a clean store and resume rotating, converting the refusal into precisely the bypass it exists to prevent — so the file is left for inspection and the log says so. Second, the availability cost is bounded and named: all rotation stops until the file is repaired or removed, but the ~20h access cookie masks it, revocation_gen lives in a separate file so kirocrew logout still works, and the condition is loud (logger.error naming the path and the keys).

Red-before, on the exact harm you described — revoke a chain, then corrupt the list that recorded it:

FAILED test_a_corrupt_revoked_chains_list_cannot_revive_a_revoked_chain
  - AssertionError: a revoked chain came back to life through a corrupt file
FAILED test_a_wrong_shaped_state_file_refuses_rotation_without_a_500
  - assert 200 == 401

Green after. Coverage added: the revival case above; the endpoint answering 401 rather than 500 (round 1) or 200 (round 3); the degraded store refusing to overwrite the file; absence not counting as corruption; and test_an_unparseable_file_still_starts_empty, which pins the boundary explicitly so a future round of this argument cannot quietly widen into the behaviour test_tr_i_17 owns.

887 passed / 3 skipped / 0 failed across the auth, refresh, tailnet, token_auth, config-baseline, peer-auth, mobile-link and diagnostics families, test_tr_i_17 and 17a included and still green.

[iamwhatever]

• Design Review Watch — description ↔ diff conflict on corrupt state — fixed; the PR body now describes the fail-closed behaviour actually shipped, and the section is renamed as suggested.

The round-1 section claims wrong-shaped records "resolve to 'no records'" [...] but the shipped code does the opposite [...] a maintainer reading the description will approve behavior the code doesn't have.

Entirely my error, and the worst kind on a security PR: the body was written for round 1's fix and I did not rewrite it when round 3 inverted the direction. A reviewer approving that text would have been approving fail-open while the diff shipped fail-closed.

The section is now titled "Unreadable persisted state fails closed, and does not crash" and leads with the invariant the code implements — absence is fine; corruption of a PRESENT security record is not — followed by a five-row table giving the actual reading for each file state, both rounds' findings as the reasons, and the availability cost named outright ("all rotation stops until the file is repaired or removed"). Your suggestion to rename it was right: the old title asserted the inverse of the code.

[iamwhatever]

• Design Review Watch — the split posture is one deletion from fail-open — rebutted on threat model, and the seam is now named explicitly in the PR body rather than shipped quietly.

An unparseable file still loads empty (pinned by test_tr_i_17), so {"revoked_chains": null} locks out all rotation while rm refresh_chains.json or truncation silently drops the same revocations. The guard only covers accidental corruption, not the adversarial write it's phrased against.

The asymmetry is real and you have described it exactly. Where I disagree is that it matters: the adversarial case is already lost before this file is reached. refresh_chains.json lives in the 0700 data home beside token_signing.key — the HMAC secret every access and refresh token is signed with. Anyone who can delete or truncate the records can equally write that key and mint whatever tokens they like, without touching revocation state at all. So a guard that resisted an adversarial delete would buy nothing: it would be defending one file in a directory whose compromise is total.

What the guard is genuinely for is accidental corruption, which is the shape that actually occurs: a hand-edit, a foreign or legacy format, a partial restore. Those produce a file that still parses as JSON but whose records cannot be read — and that is precisely the branch that now refuses. I have re-phrased the body away from adversarial language, since you are right that it invited exactly this objection.

On not shipping the seam silently: the body now carries it as a named paragraph — that the unparseable and deleted cases still start empty, that this is scoped to accidental corruption, why the adversarial framing does not apply, and what closing it properly would take (a positive "I have no records" marker — a schema version or sentinel — so absence stops being ambiguous with fresh install). That is a wider format change than this PR should make, and it is the same reason the unparseable branch is left exactly as test_tr_i_17 pins it.

One correction to the note: the GPT-r3 decision is no longer queued — I fixed it in bbfb364fd after re-reading _load and finding the two branches separable, so there is no pending decision left to settle this alongside. That is why the seam is documented here and now rather than deferred to it.

[iamwhatever]

• Design Review Watch — access-cookie fail-closed is a new operational requirement — needs-a-decision, unchanged and still with the maintainer; restated here because it is now the last thing between this PR and merge.

ordinary sessions now need a kirocrew token re-mint after an operator disables identity trust. Correctly escalated in the body; confirm it before merge since it changes behavior for every Phase-3 session, not just the exploit path.

Agreed on all three points, including that it reaches every Phase-3 session rather than only the exploit path — that is the part a reader is most likely to miss, and it is why I have not treated the body note as an acceptance.

The decision, unchanged from the earlier round: keep both halves (as shipped) also closes the post-restart first-arrival re-pin on the access cookie, at the cost you name; chain-only is two lines at the require_peer carry site, leaves access-cookie behaviour untouched and needs no re-mint on a trust toggle, but leaves the rotated access token re-pinnable by whichever allowed node presents it first after a restart — the same laundering shape as the reported defect, one credential over.

My recommendation is still to keep both, because the sibling hole is the same bug and shipping with it open invites the next round. But it is a user-visible availability change and the call is the module owner's. I will make the chain-only change on request; it does not need another review cycle to describe.

[iamwhatever]

• First Principles Watch — the record's justification does not hold — accepted; the analysis is correct and it is what makes your subtraction a real question rather than a style preference.

the claim is HMAC-signed, so an attacker cannot strip it. What remains is a claimless server-minted token for a recorded chain: there is exactly one rotation path, and it is the function that reads the record. Grepped chain_peer|bind_chain_peer: 1 producer (token_auth.py:3027), 1 reader (auth_refresh.py:493) [...] Its zero option costs nothing observable today, and its cost is a persisted schema field whose absence-means-unbound semantics must hold forever.

I checked each step rather than taking it on trust, and it holds. The claim is inside the HMAC-signed payload, so an attacker cannot strip it and present a claimless token for a chain the record covers — the signature check rejects that before _record_list is ever consulted. Nor does the record help the restart case I had in mind when I added it: the signed claim travels in the token, so it survives a restart on its own; the in-memory pin map being empty changes nothing about it.

That leaves exactly the case you name — a future mint path that writes the record and omits the claim — and the producer/reader counts (1 and 1) are what make that hypothetical rather than latent. Your framing of the cost is also the part I had underweighted: not the lines, but a persisted schema field whose absence-means-unbound semantics have to stay true forever, in a file whose other keys now fail closed when unreadable.

I am not defending it beyond that. Where it goes is the subtraction, answered in its own comment.

[iamwhatever]

• First Principles Subtraction — drop the chain_peers server-side record — needs-a-decision, with my recommendation to accept it. I am not deleting it unasked only because issue Bind refresh chains to the tailnet peer identity that opened them #2417 specifies the field by name.

Drop the chain_peers server-side record: bind_chain_peer (both forms), chain_peer, mark_consumed's peer_key parameter, and the persist/load/evict/revoke handling — the signed claim alone refuses the exploit end to end. The _record_list chokepoint stays: it fixes five pre-existing crash shapes independent of this record.

I think you are right, and the split you drew is the right one — _record_list earns its place independently (five pre-existing crash shapes, none of them about this record), while the record itself is justified only by a mint path nobody has written. Confirmed against the module's own exploit test: it mints through the real ?token= exchange, so every chain it exercises is claim-bound; deleting the record would not weaken a single assertion in it.

Why I am asking rather than doing it. The field is not incidental to the issue, it is written into its scope note: "Persisted-format change to refresh_chains.json (add peer_key), with migration for existing chains (absent = unbound, today's semantics)." Deleting something the issue names is as much a scope decision as adding one, and this is the second reviewer-recommended subtraction on this PR that collides with the issue text — the first being bind_refresh_chains. I would rather surface that pattern than quietly resolve it twice in opposite directions.

Worth noting what dropping it also buys: it removes one of the open decisions outright. The record is why chain_peers participates in the new fail-closed degraded state, and why the body has to explain an absence-means-unbound schema semantic that must hold forever. Without it, the degraded logic covers only consumed_jtis and revoked_chains — the two genuine security controls — and the PR gets meaningfully smaller.

Ready to do it on a yes: delete both bind_chain_peer forms, mark_consumed's peer_key parameter, the chain_peers persist/load/evict/revoke handling and its _RECORD_KEYS entry, the auth_refresh reader plus the two-authority reconciliation (leaving the signed claim as the single authority), and the six tests that cover the record. _record_list and the degraded posture stay for the other two keys.

[iamwhatever]

• First Principles Watch — roaming now has two knobs — needs-a-decision, still with the maintainer, and your new argument makes the case for deleting the flag stronger than when I first raised it.

The opt-out's distinct beneficiary — node-scope pinning plus a chain that roams — renews by re-pinning on rotation, the very laundering shape the PR names as the defect; and sustained two-device use of one chain trips same-IP reuse detection anyway.

The second clause is the part I had not accounted for. Even granting the opt-out its intended beneficiary, two devices sharing one chain do not coexist quietly: the grace window is chain-head-only and same-IP, so the second device's rotation presents a jti the first has already consumed from a different address, which is reuse detection doing its job and revoking the chain. So bind_refresh_chains: false does not actually deliver durable cross-device roaming — it delivers re-pinning on each rotation until the two devices race, and then a revocation. That is a worse deal than I represented, and it narrows the flag's honest beneficiary close to nothing.

Unchanged from the earlier round is why I am not deleting it myself: the flag exists as the answer to @NicholasRBowers' routing comment on #2417 — "it also stops legitimate roaming between allowed nodes, which is user-visible behavior that the remote-and-mobile guide currently documents as working". Removing the escape hatch that comment asked for is the same kind of call as adding it was, and it is now the second subtraction on this PR that collides with the issue text.

My recommendation, updated by your argument: drop it, and let pin_scope: "login" be the single roaming knob. On a yes it is a clean deletion — the config field, the TailnetTrust field, the governed_tailnet_trust parameter, the seven plumbing sites, the core.py write-schema entry, the baseline entry, and one test.