fix: audit deactivate of an already-lapsed safety override grant (#2475) - #2483
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Guard keyed on the one field lazy expiry preserves fixes the root cause, and zeroing it closes the renew-resurrection hole — sound, proportionate, spec updated in-commit. [DESIGN-REVIEWED] 32bdb22 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
SafetyOverride.deactivate() returned early when _active was false, and that early return sat above the SEL audit write. Lazy expiry clears _active while leaving the rest of the grant state in place, so an operator switching back to normal mode after the TTL elapsed was recorded nowhere -- the common path, not an edge case. deactivate() now emits its safety_override:deactivate event for any grant that exists in any form, keyed on the _expires_at 0.0 sentinel (which lazy expiry does NOT clear) rather than _active (which it does). The pre-call state (was_active, was_permanent, remaining, prior_source) is snapshotted under _lock and written to resources outside the lock; operation/outcome are unchanged. A never-activated instance stays silent. Zeroing _expires_at on every existing grant also closes the renew grace window, so an explicitly revoked lapsed grant can no longer be resurrected by renew(). Contract decision: deactivate() logs every operator DECISION; deactivate_scope() stays on log-every-state-change (no decision is lost there). The revocation is deliberately non-critical, unlike the fail-closed _commit_activation: refusing to deactivate on a failed audit write would leave auto-approval ON. Also sorts imports in dashboard/token_auth.py -- a cross-PR merge artifact on main that fails the repo-wide isort gate. Closes #2475
98327bb to
32bdb22
Compare
…odotdev#2475) (kirodotdev#2483) SafetyOverride.deactivate() returned early when _active was false, and that early return sat above the SEL audit write. Lazy expiry clears _active while leaving the rest of the grant state in place, so an operator switching back to normal mode after the TTL elapsed was recorded nowhere -- the common path, not an edge case. deactivate() now emits its safety_override:deactivate event for any grant that exists in any form, keyed on the _expires_at 0.0 sentinel (which lazy expiry does NOT clear) rather than _active (which it does). The pre-call state (was_active, was_permanent, remaining, prior_source) is snapshotted under _lock and written to resources outside the lock; operation/outcome are unchanged. A never-activated instance stays silent. Zeroing _expires_at on every existing grant also closes the renew grace window, so an explicitly revoked lapsed grant can no longer be resurrected by renew(). Contract decision: deactivate() logs every operator DECISION; deactivate_scope() stays on log-every-state-change (no decision is lost there). The revocation is deliberately non-critical, unlike the fail-closed _commit_activation: refusing to deactivate on a failed audit write would leave auto-approval ON. Also sorts imports in dashboard/token_auth.py -- a cross-PR merge artifact on main that fails the repo-wide isort gate. Closes kirodotdev#2475
Summary
SafetyOverride.deactivate()returned early when_activewas false, and that early return sat above thesafety_override:deactivateSEL audit write. Lazy expiry (is_active()) clears_activewhile leaving_expires_at/_source/_permanentin place, so an operator switching back to normal mode on a grant whose TTL had already elapsed was recorded nowhere — the common path, not an edge case. The SEL stream is the only durable record of who changed the auto-approval posture; an auditor saw theactivateevent and then nothing.The contract decision (explicit, per the issue)
The issue frames this as a contract disagreement on the same object. This PR resolves it as:
deactivate()logs every operator DECISION — it now emits its SEL event whenever a grant exists in any form, live or lapsed. Only a never-activated instance stays silent.deactivate_scope()stays on "log every state change" — no operator decision is lost there (revoking an absent scope changes nothing), so it is intentionally left untouched.How
_expires_at0.0 sentinel — the one field lazy expiry does not clear (verified againstis_active(), and pinned by a test asserting_expires_at > 0after lazy expiry) — instead of_active, which is exactly what expiry zeroes._lock, then the SEL write happens outside the lock (same rule asrenew(): never hold the state lock across I/O).resourcesnow carries the pre-call state:source:<who>, was_active:<bool>, was_permanent:<bool>, remaining:<n>s, prior_source:<grant origin>, so a no-change deactivate is distinguishable from one that revoked a live grant.operation/outcome(safety_override:deactivate/disabled) are unchanged; nothing insrc/orwebsite/src/parses the resources string._commit_activation(and the fail-closed posture in SafetyOverride.renew() extends a grant before auditing it, and swallows audit failure #2453's area): this is a REVOCATION, not a grant. The audit stays non-critical and the state change unconditional — refusing to deactivate because an audit write failed would leave auto-approval ON, which is strictly worse. This is not an oversight; it is stated in the code comment.Latent bug fixed as a direct consequence
The old early return left
_expires_atintact on a lapsed grant, so arenew()inside the 300s grace window could resurrect a grant the operator had just explicitly revoked. Zeroing_expires_aton every existing-grant deactivate closes that;test_renew_after_deactivating_lapsed_grant_failsfails on unfixed main and pins it.Tests (mutation-verified: 4 of the new tests fail on unfixed code)
All inject at the SEL sink (
log_api_accesson the patchedsel()instance), not_log_sel, so assertions cannot pass vacuously:was_active:False, remaining:0s, prior_source:slack; also pins the discriminator (lazy expiry clears_activebut not_expires_at)was_active:True(existing signal not diluted)was_permanent:True, remaining:-1sOut-of-scope note
src/kiro_crew/dashboard/token_auth.pygets a mechanical import reorder: main HEAD (584bbb0) currently fails the repo-wideisort --check-onlygate on that file (a cross-PR merge artifact between the tailnet and revocation_gen import blocks). Without it this PR cannot pass lint. No code change, imports only.Coordination
Scoped strictly to
deactivate(). Sibling issue #2453 (renew()) and PR #2443 touch the same file in disjoint hunks; rebased onto freshorigin/mainimmediately before pushing. This PR does NOT attempt #2453.Closes #2475