fix: dashboard-auth session revocation, .env lost-update race, and WS reattach-race test gap - #77174
Closed
JoaoMarcos44 wants to merge 4 commits into
Closed
Conversation
…d session.resume reattach _close_sessions_for_transport() snapshotted sessions owned by the disconnecting transport under _sessions_lock, released the lock, then mutated each session (close or repoint to the detached sentinel) without re-checking ownership. session.resume's warm-reuse path (_reuse_live_payload -> _live_session_payload) rebinds session["transport"] under _session_resume_lock independently, so a reconnect landing in that window got silently undone: the old transport's teardown either force-closed the just-reattached session or stomped its new transport back to _detached_ws_transport, making a live reconnect look orphaned and eligible for grace-reap. Revalidate transport ownership under the same resume_lock -> sessions_lock ordering already used by the orphan-reap timer, immediately before claiming (close) or repointing (detach) each session. Slow teardown work still runs after releasing both locks. Fixes #HPA-01
…AH-01) BasicAuthProvider's tokens are stateless HMAC blobs with no server-side session record, so revoke_session() was a documented no-op and rotating the password (with an explicit, restart-stable secret) never invalidated tokens already issued — verify_session() only checked the signature and expiry, never anything derived from the current credential. Give every token an "epoch" claim checked at verification time. The epoch is persisted (atomic JSON write) and keyed to a fingerprint of the credential source material (not the salted hash, which changes every restart even when the password doesn't): register() detects a fingerprint change and bumps the epoch, so both an explicit revoke_session() and a credential rotation across a restart invalidate every previously issued token. Same fingerprint across a restart leaves the epoch untouched, preserving the documented explicit-secret multi-worker/restart-survival contract. Direct construction (tests, or any future caller) without a credential_fingerprint keeps the previous in-memory-only behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…race (RAH-02) save_env_value()/remove_env_value() each read the whole .env file, mutate an in-memory line list, and atomic-replace the file. atomic_replace() rules out a torn/partial write, but two concurrent callers (two CLI/desktop processes editing credentials at once) can both read the same snapshot and each write their own version — the loser's change vanishes silently, with no error and a perfectly valid resulting file. Add a cross-process advisory lock (reusing auth.py's existing fcntl/ msvcrt _file_lock helper) around the read-transform-write cycle in both functions. Verified the race reproduces reliably (3/3 runs) against the unlocked code with a threading.Barrier-synchronized test, and is closed by the lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…RAH-04); add real WS reattach-race regression test (RAH-05) RAH-04: model.save_key rebuilt the provider inventory via build_models_payload(picker_hints=True), which already computes a real `authenticated` value per row, then unconditionally overwrote it with `True` right after — even for the row it had just found in that payload. The comment above the line said it was only meant to cover the synthetic fallback (provider not found in the rebuilt inventory at all); the code ran unconditionally instead, silently discarding the real computed value whenever it happened to be False. RAH-05: the existing WS disconnect/reconnect race regression test constructed both sessions already pointing at new_transport, so they never entered `owned_sids` (filtered by old_transport) and the revalidation logic added by the TOCTOU fix was never exercised — the test passed identically before and after that fix and proved nothing about it. Added a test that starts the session on old_transport (so the snapshot captures it) and injects the reattach strictly between the snapshot and the per-sid claim under _session_resume_lock, matching the actual race window. Confirmed it fails against the pre-fix implementation and passes against the current one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
alt-glitch
added
type/bug
This was referenced Aug 3, 2026
Open
Contributor
Author
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes four confirmed bugs and one test-coverage gap found during a deep
race/auth/consistency investigation, each tracked as its own issue with
full root-cause analysis. Brings in an already-authored WS TOCTOU fix
that turned out to exist only on an unrelated, unmerged branch.
Closes #77186, #77187, #77190, #77191, #77192.
BasicAuthProvider.revoke_session()was adocumented no-op and password rotation (with a stable explicit
secret) never invalidated prior tokens. Added a persisted,fingerprint-gated session epoch checked at verification time.
save_env_value()/remove_env_value()had nolock around their read-modify-write cycle; concurrent writers silently
lost each other's changes. Reused the existing cross-process
_file_lockfromauth.py.model.save_keyforce-overwrote a realauthenticatedvalue computed by the inventory builder withTrue,even when the provider was found in the rebuilt payload (the override
was only ever meant for the not-found fallback). Scoped the override
to that branch.
both sessions already on the new transport, so it never entered the
code path the fix under test actually revalidates. Rewrote it to
trigger the real snapshot → reattach → stale-teardown interleaving.
TOCTOU fix (
9d5579460), which was missing frommainentirely.#77188 (RAH-03: credential lifecycle across
.env/auth.json/cache/config.yamlisn't transactional) isintentionally not included here — larger blast radius, left open as
follow-up work.
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[🔒 revoke_session / rotate password] -->|bump + persist| B[⚡ Session Epoch Store] B -->|epoch claim checked| C{Token Verify} C -->|epoch mismatch| D[🚫 Rejected] C -->|epoch match| E[✅ Session Valid] F[💾 Two concurrent .env writers] -->|read snapshot| G[⚡ _file_lock cross-process] G -->|serialized read-modify-write| H[🚀 Both keys survive] I[🔑 model.save_key] -->|rebuild inventory| J[⚡ real authenticated value] J -->|found in payload| K[🚀 kept as computed] J -->|not found: fallback| L[🚀 forced True] M[🔌 WS disconnect teardown] -->|snapshot sids| N[⚡ _session_resume_lock] O[🔁 session.resume reattach] -->|rebind transport| N N -->|revalidate ownership| P[✅ live reconnect preserved]Test plan
tests/plugins/dashboard_auth/test_basic_provider.py— 22 passed; 3 new RAH-01 tests each confirmed to fail against pre-fix codetests/hermes_cli/test_config.py(env save/remove suite) — passes incl. newTestSaveEnvValueConcurrency, confirmed to reproduce the lost update 3/3 runs pre-fixtests/test_tui_gateway_server.py -k "close_sessions_for_transport or model_save_key"— passes; new RAH-04/RAH-05 tests each confirmed to fail pre-fixtests/plugins/dashboard_auth/,test_dashboard_auth_provider_base.py,test_dashboard_auth_plugin_hook.py,test_dashboard_auth_password_login.py,test_dashboard_admin_endpoints.py— 157 passed, no regressionspython -m py_compileon every edited filetests/test_tui_gateway_server.pysuite (large file, run separately — targeted subsets above cover every touched code path)🤖 Generated with Claude Code