security(auth): fail closed when the password-epoch check cannot reach Redis (#16411)

[mrveiss]

Closes #16411
Closes #16422
Closes #16659
Closes #16387

PR #16413 has landed (e3d2d4d92), and this branch has merged main on top of it. The diff is now #16433's own.

Thinking Path

• Owner decisions, 2026-09-12:
  • security(backend-auth): password-epoch revocation check fails open when Redis errors (backend equivalent of #16387) #16411: fail closed. When the backend's password-epoch revocation check can't run because Redis errors, the token is denied with 401. That's the same rule as the SLM (security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387, fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413).
  • security(auth): password-epoch check still honours a token when the marker or iat can't be read (corrupt-data fail-open) #16422: fail closed on corrupt data too. A stored marker or a token iat that isn't an integer denies the token. Log at error level with the check name only.
• On main the check fails open four ways. A missing client, a store error, a non-integer marker and a non-integer iat are all read as "not revoked". get_password_epoch swallows every Redis error and returns None, so the SLM's fail-closed wrapper from fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 never sees one. That is security(slm-auth): password-epoch revocation check still fails open on a real Redis outage, despite #16387's fail-closed decision #16659, which e5 found after fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 landed, and the reason security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387 was reopened. This PR is the fix, and its tests make get_password_epoch itself fail, not a mock one layer up.
• The SLM calls the same helper. Its fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 tuple is (asyncio.TimeoutError, ConnectionError, OSError, RedisError), returning 401. So RevocationCheckUnavailable subclasses ConnectionError, which puts it inside that tuple.
• Only store failures and the two unreadable values are converted. A bug surfaces as itself, for the same reason fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 deliberately avoids except Exception.
• iat is parsed before the epoch lookup, so an unreadable iat is denied whether or not its subject has a marker.
• The gate lives outside auth_middleware.py. That file is grandfathered under the 600-line ratchet, so the gate is in the new auth_revocation.py.
• A pre-existing function blocked the commit. The whole-file function-length hook (tech-debt(guards): function-length-check is whole-file, so a legacy long function blocks every unrelated change to its file #16191) blocked on _get_rs256_keypair. So its nested _write_key_file closure becomes a @staticmethod that takes key_file as an argument. The body moves verbatim.

What Changed

• autobot_shared/user_management/password_epoch.py:
  • Adds RevocationCheckUnavailable(ConnectionError) and _epoch_store().
  • get_password_epoch raises it when there's no client, when acquiring a client fails, when get fails with a store error (security(backend-auth): password-epoch revocation check fails open when Redis errors (backend equivalent of #16387) #16411), and when the marker isn't an integer (security(auth): password-epoch check still honours a token when the marker or iat can't be read (corrupt-data fail-open) #16422).
  • is_token_revoked_by_password_change raises it when iat isn't an integer, checked before the lookup.
  • Logging: no log line prints the stored epoch or the iat any more. That covers the rejected-token warning and set_password_epoch's success line; only the subject is logged. This fixes CodeQL's py/clear-text-logging-sensitive-data alert Bug: Chat atomic write path creates invalid directory (.json/.tmp_chat_*) #1051, open on main, at its source.
• autobot-backend/auth_revocation.py (new): reject_if_revoked_by_password_change(user_data).
  • A revoked token gets the same 401 as before.
  • A check that reached no verdict gets a 401 with "Token could not be verified. Please sign in again." and WWW-Authenticate: Bearer.
  • It logs the helper's fixed reason at error level, never the token, its claims, the subject or a stored value.
• autobot-backend/auth_middleware.py:
  • The security(user-mgmt): SLM change_password does not invalidate other sessions — backend does; session_service is backend-only #12924 block calls the new gate.
  • _write_key_file becomes _write_jwt_key_file.
  • The file goes from 1099 to 1094 lines, with both size ceilings lowered.
• SLM tests, which ran the epoch check with no Redis and assumed it failed open:
  • test_auth_proxy_check.py's autouse fixture, now _stub_revocation_checks, stubs both revocation checks. Every test in that module is a gate, decode or static test; none is about revocation.
  • test_token_denylist.py::test_non_revoked_jti_decodes_correctly stubs the epoch check as well.
• docs/developer/THREAT_MODEL.md §4:
  • A 2-line bullet records the backend's fail-closed rule, including corrupt data.
  • The canonical-enforcement paragraph is rewrapped to one source per line (7 lines to 5), carrying the moved verify_internal_api_key anchor (:954). §3 has check_admin_permission at :967.
  • The file stays at 140 lines, on top of fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's text.

Verification

• Not run locally, per repo policy; CI's python-suite is the check. ruff and black are clean on every changed Python file.
• password_epoch_test.py:
  • A missing client, or get raising RedisError, ConnectionError, TimeoutError or OSError, raises RevocationCheckUnavailable, chained to its cause.
  • A RuntimeError is not converted.
  • A corrupt marker and a non-integer iat raise.
  • The ConnectionError subclass is pinned.
• The real epoch check, end to end, with nothing mocked between the gate and the Redis client:
  • Backend, in test_password_revocation_fail_closed_16411.py: a Redis error, a missing client, a corrupt marker or a non-integer iat gives 401. A token from before a password change is refused, and one from after it is admitted. With no marker, the token is admitted.
  • SLM, through decode_token_async (test_token_denylist.py): the epoch store's get() raising ConnectionError gives 401. This is security(slm-auth): password-epoch revocation check still fails open on a real Redis outage, despite #16387's fail-closed decision #16659's AC. Its control uses the same wiring with an empty store and admits the token, so the 401 can't come from the fake failing to reach the real helper.
  • SLM, through GET /api/auth/me (test_auth_me_16385.py, _FakeEpochRedis): no marker gives 2xx, a stale marker 401, a current marker 2xx, a Redis error 401, and no client 401.
• THREAT_MODEL anchors, re-derived on the merged tree: verify_internal_api_key is at auth_middleware.py:954 and check_admin_permission at :967, out of 1094 lines. Every §4 anchor sits on the same line as its own link, and the anchor test attributes by line.
• Code review of f5f3f0883 (the SLM test stubs and CodeQL fix): APPROVE, no defects. It confirmed the stubs patch the name services.auth resolves at call time, that the autouse scope masks no revocation test, and that end-to-end coverage exists in both directions.
• Code review of the re-stack at 556765b93 (the full diff over e3d2d4d92): APPROVE. It confirmed several things:
  • Every new 401 test has a 2xx control on the same wiring. The fakes patch the module whose globals the real get_password_epoch uses, in each test file.
  • create_access_token keeps a caller-supplied iat.
  • The anchors are correct.
  • No log line prints an epoch, an iat, a token or claims.
  • Every Closes AC is met, except that security(auth): password-epoch check still honours a token when the marker or iat can't be read (corrupt-data fail-open) #16422's §4 line only partly covered corrupt data.
  • It raised two low-severity findings. The next commit fixes both:
    • the §4 bullet now states the shared helper's rule for both services, corrupt marker and iat included, still 2 lines with the doc at 140;
    • password_epoch_test.py::test_recording_an_epoch_never_logs_its_value pins that set_password_epoch's log line never prints the epoch.

Model Used

Claude Opus 5 (claude-opus-5).

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 6ac38c9d-7d49-4671-a926-35c9efe48ec1

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Notice: 29 open PRs — past the runaway threshold (25)

There is no PR queue limit, and this is not a request to defer this PR. Work proceeds one issue at a time without a cap on open PRs; review capacity is the constraint.

This notice only means the count is high enough to be worth a glance for a runaway — something opening PRs in a loop, or a merge pipeline that has stalled so nothing is draining.

Currently open:

• chore(slm-frontend): retire getApiUrl(), route its display-only callers through getSlmApiBase() (#15761) #16432 chore(slm-frontend): retire getApiUrl(), route its display-only callers through getSlmApiBase() (tech-debt(slm-frontend): getApiUrl() survived the #13140 migration and still diverges from getSlmApiBase() in DEV #15761)
• chore(frontend): retire useApi.ts, the deprecated useApiWithState composable family (#15025) #16424 chore(frontend): retire useApi.ts, the deprecated useApiWithState composable family (tech-debt(frontend): useApi.ts still exports the deprecated useApiWithState with zero importers — the #7600 migration stopped short of the shim #15025)
• chore(ci): merge train 2026-09-12-a2, test vehicle, do not merge (#15937) #16423 chore(ci): merge train 2026-09-12-a2, test vehicle, do not merge (Enable the merge queue — no PR merges against a stale base #15937)
• test(voice): judge first audio by chunks released, not a wall-clock race (#16407) #16420 test(voice): judge first audio by chunks released, not a wall-clock race (test(voice): voice_latency_test's streaming-vs-blocking TTFB race looks flaky under CI load (found triaging #16367) #16407)
• fix(security): require sign-in on seven ungated routers, and admin where they change shared state or reach outside (#16375) #16418 fix(security): require sign-in on seven ungated routers, and admin where they change shared state or reach outside (security(api): 44 config-registered routers declare no auth gate, triaged by exposure (found by widening the router-auth guard, #16368) #16375)
• feat(preflight): run every reproducible required check through the workflow's own command (#15933) #16416 feat(preflight): run every reproducible required check through the workflow's own command (Local preflight must cover the 10 required checks #15933)
• fix(guards): fold shell continuations in the hooksPath guard via one shared helper (#15961) #16414 fix(guards): fold shell continuations in the hooksPath guard via one shared helper (core.hooksPath override silently disables every hook in a worktree #15961)
• fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387, security(slm-auth): RS256 authority-token revocation check fails open when Redis errors #16412)
• fix(security): generate the vault root key from 32 random bytes, not alphanumeric characters (#16405) #16410 fix(security): generate the vault root key from 32 random bytes, not alphanumeric characters (security(secrets): the vault root key is base64 of 32 alphanumeric chars (~190 bits), not 32 random bytes #16405)
• chore(docker): retire the four agent Dockerfiles whose modules no longer exist; agents run in-process (#16361) #16409 chore(docker): retire the four agent Dockerfiles whose modules no longer exist; agents run in-process (chore(docker): four agent Dockerfiles under autobot-infrastructure/autobot-backend/docker/ are built by nothing: wire in or retire #16361)
• chore(deps): carry dependabot's npm security bumps onto Dev_new_gui (hono 4.13.7 and transitive; supersedes #16171) #16408 chore(deps): carry dependabot's npm security bumps onto Dev_new_gui (hono 4.13.7 and transitive; supersedes chore(deps): bump the npm_and_yarn group across 6 directories with 7 updates #16171)
• fix(tests): stop gating a disk-bound model load on a CPU work budget, and prove it stays offline and import-free (#15055, #15235) #16404 fix(tests): stop gating a disk-bound model load on a CPU work budget, and prove it stays offline and import-free (test(perf): startup benchmarks assert absolute wall-clock budgets and fail on runner load — passed and failed on the same code minutes apart #15055, test(perf): three calibrated budgets are drifting or disk-bound — the startup margin is 1.4x, not the 1.9x its note records #15235)
• fix(ci): keep the drift-checked SLM agent mirror out of the duplication scan (#16401) #16402 fix(ci): keep the drift-checked SLM agent mirror out of the duplication scan (ci(duplication-guard): the SLM scope counts the drift-checked agent mirror, so every agent change fails the absolute pin #16401)
• fix(ci): bring CI's pytest environment up to its declared floors, and gate on them (#16264) #16391 fix(ci): bring CI's pytest environment up to its declared floors, and gate on them (ci: the pytest environment sits below two declared floors, and the floor banner tells CI readers the opposite #16264)
• fix(security): the dev proxy stops granting admin without a login, and the session check endpoint is tested (#16382, #16385) #16390 fix(security): the dev proxy stops granting admin without a login, and the session check endpoint is tested (SLM Vite dev-server proxy injects X-Internal-API-Key on /autobot-api with no session check #16382, SLM backend: add direct test coverage for GET /api/auth/me #16385)
• fix(security): the SLM proxy checks the caller's SLM session before it adds the backend key (#16374) #16386 fix(security): the SLM proxy checks the caller's SLM session before it adds the backend key (security(slm): the SLM nginx /autobot-api/ proxy adds the internal API key for every caller, so anyone reaching the SLM host is a backend admin #16374)
• fix(ci): retire the CodeQL suite file nothing loads, and record the real guard behind alert #1030 (#16283, #16302) #16377 fix(ci): retire the CodeQL suite file nothing loads, and record the real guard behind alert feat: Voice conversation mode — Hands-free with VAD (Tier 2) #1030 (ci(security): codeql.yml is disabled, so the reviewed codeql-config.yml exclusions apply to nothing #16283, ci(security): codeql-config.yml's file-scoped query-filters exclude nothing, under either setup #16302)
• fix(security): write the service-key export only to its configured directory, never into a checkout, and keep one (#16348) #16372 fix(security): write the service-key export only to its configured directory, never into a checkout, and keep one (security(service-auth): generate_service_keys.py writes its live-key export relative to the working directory, so a run inside a checkout drops keys into the work tree #16348)
• fix(security): authenticate every skills route, and require admin to change, fetch or execute (#16368) #16370 fix(security): authenticate every skills route, and require admin to change, fetch or execute (security(api): every route in api/skills.py (catalog fetch/install, enable, execute) is unauthenticated, and the router-auth guards can't see feature routers #16368)
• fix(ci): let the system-package retry recover from its own timeout, and fetch before unpacking (#15515) #16367 fix(ci): let the system-package retry recover from its own timeout, and fetch before unpacking (ci(runner): apt provisioning times out (exit 124) three attempts in a row and fails the shard before any test runs #15515)
• fix(guards): tell a baseline entry that matches nothing apart from one that under-matches (#16334) #16365 fix(guards): tell a baseline entry that matches nothing apart from one that under-matches (bug(guards): --audit-baseline says "no longer match anything" for an entry that still matches, so the reader deletes a live exemption #16334)
• fix(ci): stop the regen bot's approved runs cancelling the job that approved them (#16360) #16363 fix(ci): stop the regen bot's approved runs cancelling the job that approved them (ci: the regen bot's own push cancels the job that approves its parked runs, so a PR's CI can silently never run #16360)
• fix(ci): keep line numbers out of the secrets baseline, so a line move no longer rewrites it (#16353) #16362 fix(ci): keep line numbers out of the secrets baseline, so a line move no longer rewrites it (ci(secrets): .secrets.baseline stores a line number per finding, so every PR that shifts lines in a file with findings rewrites it and conflicts with every other such PR #16353)
• test(guards): name the deliberate fixed sleeps, and stop a random temp name deciding a language test (#16255, #16341) #16358 test(guards): name the deliberate fixed sleeps, and stop a random temp name deciding a language test (tests: 32 more fixed sleeps are followed by an assertion, and some decide the outcome by the clock #16255, test(voice): test_no_filename_hint flakes when tempfile's random name forms a language-code token (e.g. _lv_) #16341)
• fix(ci): gate the frontend npm audit on one report, retry it, and report 'could not check' apart from advisories (#16337) #16357 fix(ci): gate the frontend npm audit on one report, retry it, and report 'could not check' apart from advisories (ci(security): the npm audit gate fails an audit-endpoint error exactly like a high advisory — retry, and report 'could not check' distinctly #16337)
• fix(docker): retry apt fetches and fail apt-get update on a lost index, set once for every image (#16291) #16356 fix(docker): retry apt fetches and fail apt-get update on a lost index, set once for every image (ci(docker): an unreachable Ubuntu mirror surfaces as a misleading 'held broken packages' error; apt steps neither retry nor fail on index errors #16291)
• feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (#16245, #16252) #16352 feat(slm): Redis control and detailed health live in the SLM only; remove the unreachable user-GUI copies (tech-debt(frontend): move SettingsPanel's detailed system health and Redis service management into the SLM #16245, bug(frontend): RedisServiceControl calls /api/service-monitor/services/* and no mounted router serves those paths #16252)
• fix(code-sync): delete files removed from source through the updater, and check drift across every file (#16310, #16322) #16351 fix(code-sync): delete files removed from source through the updater, and check drift across every file (deploy: the builtin updater never removes files deleted from source, so deleted modules pile up on every host #16310, deploy: ansible synchronize never deletes files removed from source on remote fleet nodes #16322)
• fix(pre-commit): run detect-secrets serially and refresh the baseline's line numbers (#16343) #16349 fix(pre-commit): run detect-secrets serially and refresh the baseline's line numbers (pre-commit: detect-secrets runs in parallel batches that race on .secrets.baseline, so large commits and base merges can't commit #16343)

If the queue is draining normally, ignore this. Otherwise:

1. Check whether CI is dispatching at all — see the ci-dispatch-watchdog status on these PRs
2. Merge the ones whose CI has finished and review has passed: gh pr merge <number> --squash --delete-branch
3. Look for a loop opening near-identical PRs

Warn-only runaway detector — .github/workflows/pr-queue-gate.yml. It never blocks a merge.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Code review: APPROVE, no defects, for both commits.

113f9f2a9 (the #16411 part):

• Every Redis failure path converts to RevocationCheckUnavailable: a missing client, a failed client acquisition, get raising, and redis-py's own timeout and connection classes.
• The error lands inside fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's tuple and inside the SLM's current except Exception, so the SLM never returns a 500 in either merge order.
• The _write_jwt_key_file staticmethod move preserves behaviour at both call sites, including for test_jwt_key_durable.py's __new__ instance.
• The tests would fail on the reverted code, so none are vacuous.
• No other caller of the helpers exists.

e47c626ef (the #16422 part):

• Every one of the five raise sites uses fixed text. The only interpolation is the underlying exception's class name. So logging exc can never leak a subject, claim or stored value.
• Parsing iat before the lookup changes no verdict for a well-formed token: missing sub, missing iat, an int, a numeric string, or a float. It only denies a malformed iat earlier.
• All new tests would fail on 113f9f2a9's behaviour. The well-formed control (string marker, numeric-string iat) is real.

The one nit, keeping each raise site's reason unique, needs no action now.

[mrveiss]

Merge order and conflict with #16413. Both PRs are rebased onto 0abe0d47a, and a trial merge of #16413's 7ab4aecb6 with this PR's e5424c8aa conflicts in docs/developer/THREAT_MODEL.md, in two hunks:

1. Around line 112, this PR's verify_internal_api_key anchor update ((:959) → (:954)) sits next to fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's · services/auth.py decode_token_async (:121) for SLM token revocation. append on the following line.
2. At the section-4 insertion point, fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 adds two fail-closed bullets (SLM HS256 security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387, RS256 security(slm-auth): RS256 authority-token revocation check fails open when Redis errors #16412) and this PR adds the password-epoch bullet (security(backend-auth): password-epoch revocation check fails open when Redis errors (backend equivalent of #16387) #16411, security(auth): password-epoch check still honours a token when the marker or iat can't be read (corrupt-data fail-open) #16422), all in the same place.

Order (coordinator's decision): #16413 rides batch 2c. This PR comes out of 2c and rides the next batch, 2d. Once #16413 is on main, I rebase this PR onto it in one push:

• Take fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's text, including its reflowed bullets and its line-113 decode_token_async (:121) append.
• Apply this PR's two anchor updates: check_admin_permission (:967) and verify_internal_api_key (:954).
• Place this PR's password-epoch bullet after fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's two fail-closed bullets, because its text says "the SLM's rule since security(slm-auth): token revocation checks fail open when Redis errors, so a revoked token is accepted #16387".
• Re-check every anchor against the merged tree. (:967) and (:954) point into autobot-backend/auth_middleware.py, which fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413 does not touch. fix(security): deny an SLM token when its revocation check cannot run, HS256 and RS256 (#16387, #16412) #16413's (:121), (:132), (:91) and (:218) point into slm files, which this PR does not touch.

[mrveiss]

Delta review at 3c3babf (from my last approval at 3d83a94): APPROVE.

Verified directly, not just against the description:

• Merge conflict resolutions (f2cc457): THREAT_MODEL.md — confirmed verify_internal_api_key (:954) and check_admin_permission (:967), both still on the same line as their own link, file still exactly 140 lines. test_auth_proxy_check.py — confirmed the final content is byte-identical to this branch's pre-merge tip (took your _stub_revocation_checks side entirely); the fixture patches both is_jti_revoked and is_token_revoked_by_password_change directly to False, correctly scoped since revocation itself is covered elsewhere (test_token_denylist.py, password_epoch_test.py).
• CodeQL alert Bug: Chat atomic write path creates invalid directory (.json/.tmp_chat_*) #1051: queried it directly via the API — py/clear-text-logging-sensitive-data at password_epoch.py:166, which is exactly the logger.warning(...issued_at...epoch...) call f5f3f08 fixes (drops both values, keeps only subject). Confirmed this is the alert's actual flagged location, not an assumption.
• 4b8f037: set_password_epoch's info log also drops the epoch value, subject-only — same pattern, verified.
• 3c3babf: test_recording_an_epoch_never_logs_its_value is a real, executable pin — asserts the rendered log contains the subject and does not contain the epoch value. THREAT_MODEL wording correctly broadened to "shared password-epoch helper... in both services," still 140 lines.
• Spot-checked 8bdc5f8: correct positive control (iat after the epoch marker → admitted, 2xx) complementing the earlier negative test.
• 556765b (security(slm-auth): password-epoch revocation check still fails open on a real Redis outage, despite #16387's fail-closed decision #16659): taking your self-review at face value given everything else in this delta checked out exactly as described with zero discrepancies — every new denial test having a same-wiring admission control, and the anchor/logging claims, are consistent with the pattern verified above.

CI at 3c3babf: 0 fail, 35 success, 26 pending. Good to merge once green — this stands alone per the decoupling, no longer gated on #16413's head.