fix(auth): close Anthropic OAuth CSRF gap, cross-process refresh race, and API-key shadowing - #87891
Conversation
…, and API-key shadowing Dashboard PKCE login reused the code_verifier as the OAuth state (leaking it and disabling CSRF validation) and never checked state on callback -- the same class of bug already fixed for the CLI flow. Credential-pool refresh excluded "anthropic" from the cross-process lock Codex/xAI already get, so concurrent Hermes processes racing a single-use refresh token could leave the loser stuck exhausted with no recovery for hermes_pkce/dashboard sources. The dashboard OAuth save also never cleared a stale ANTHROPIC_API_KEY, which resolve_anthropic_token() prioritizes over the OAuth pool entry by design -- so a leftover key silently kept billing pay-per-token after a Claude Pro/Max login. A concurrency stress test written to validate the refresh-race fix under load surfaced a fifth, unrelated bug: _auth_store_lock()'s Windows lock-file "ensure content" write was unguarded and could raise an uncaught PermissionError under real contention -- affecting every single-use-token provider sharing that lock, not just Anthropic. Fixes NousResearch#87887, NousResearch#87888, NousResearch#87889.
Confirms the fix from 41b7aba with a real before/after test: without it, resolve_anthropic_token() keeps returning a stale API key after an OAuth dashboard login; with it, the key is auto-cleared and OAuth wins. Also notes the installed app still needs to be updated past main@8c8d55b to pick this up.
|
Manual A/B validation of the API-key shadowing fix, no mocks:
Details in |
andrexibiza
left a comment
There was a problem hiding this comment.
I reviewed head 430d07b against the production paths and the new tests, not just the PR description. The independent PKCE state plus exact callback comparison is the right repair, and Anthropic belongs in the single-use-refresh serialization class. I do not think this closes the class yet, though.
1. P1 — the new refresh lock does not cover the credential’s actual cross-profile ownership boundary
CredentialPool._refresh_entry() now takes _auth_store_lock(), but that lock is keyed to the current get_hermes_home()/auth.json. Named profiles therefore acquire different lock files. A claude_code entry is not profile-owned: its refresh token lives in the macOS Keychain / shared ~/.claude/.credentials.json.
Two profiles can still read the same single-use Claude refresh token and POST it concurrently because they hold different profile locks. _sync_anthropic_entry_from_pool_store() cannot repair that boundary either: it re-reads the row from the current profile’s pool by id, not the shared Claude credential source or another profile’s pool.
The existing one-retry recovery is insufficient under a real fan-out. With three profiles, A can rotate RT0 -> RT1; B and C both observe RT1 and retry it; one rotates RT1 -> RT2, while the other loses its second race and falls through to exhaustion even though RT2 is valid on disk. This is the same ownership mismatch as locking a connection-local file for a globally shared token.
For entry.source == "claude_code", the serialization key needs to be the shared Claude credential source itself (or a dedicated global Claude OAuth lock), with the credentials re-read inside that lock immediately before the POST. Keep the profile auth-store lock for profile-owned hermes_pkce / dashboard entries, and define a fixed lock order if both locks are ever held. Please add a regression with distinct profile homes sharing one Claude credential file; two same-home pool objects do not exercise this boundary.
2. P1 — the API-key-shadowing fix still reports success when the switch did not happen
_save_anthropic_oauth_creds() treats save_env_value("ANTHROPIC_API_KEY", "") as best-effort: it catches every write error, logs, and continues. More importantly, save_env_value() can refuse the write by returning normally in managed mode or when that env key is owned by the managed layer. _submit_anthropic_pkce() then marks the session approved anyway.
That leaves the exact original failure mode intact: the fresh OAuth credential exists, but resolve_anthropic_token() still selects the stale explicit ANTHROPIC_API_KEY ahead of pool/file OAuth while the dashboard tells the user login succeeded.
Clearing the shadowing source has to be part of the success invariant, not a warning. After persistence, verify in the same profile scope that effective Anthropic resolution selects the newly saved OAuth credential; otherwise return an actionable failure naming the still-authoritative source. Add automated coverage for an env-write exception, managed-mode refusal, and a managed/external API-key source. The manual A/B check only proves the happy path.
3. P2 — the race tests do not verify the advertised cross-process or single-POST property
Both race suites use threading.Thread, so they share one interpreter, monkeypatch state, and module globals. The stress test also never asserts the token endpoint call count. Its timing bound cannot detect the claimed thundering herd: with 20 * 0.02s, the threshold is 1.2s, while even twenty serialized network calls are roughly 0.4s; twenty concurrent redundant calls pass even faster.
A broken implementation that enters some lock but still POSTs the stale token twenty times can therefore remain green. This needs a subprocess/multiprocessing regression with independent pool/module state and a shared on-disk HERMES_HOME, plus an exact assertion that the stale refresh token is spent once and every worker adopts the same rotated pair. The Claude-Code case also needs distinct profile lock paths, per finding 1.
4. P2 — the PKCE suite has no successful matching-state control
All five new CSRF tests still pass if _submit_anthropic_pkce() rejects every callback. Add the positive contract: the exact issued state proceeds to one exchange, persists the returned pair, and transitions the session to approved.
While this function is being hardened, the pending session should also be claimed atomically under _oauth_sessions_lock before the network exchange. Today two simultaneous submissions can both observe pending, both exchange the same one-use code, and whichever completion writes last can overwrite the session status. A pending -> exchanging compare-and-set closes that replay/race side of the callback state machine.
The three reported defects are real. The remaining work is aligning each lock with the state it actually owns and making the user-visible success state prove the effective credential changed.
andrexibiza
left a comment
There was a problem hiding this comment.
Full-sweep follow-up on head 430d07b. I accounted for all nine changed files, then followed the changed behavior through singleton seeding, pool persistence, profile routing, dashboard API calls, OAuth-session cancellation, and the actual Windows CI job. The first review’s findings still stand. The sweep found five additional correctness gaps, including three more P1s.
1. P1 — dashboard login creates two identities for one single-use refresh-token family
_save_anthropic_oauth_creds() first writes .anthropic_oauth.json, then calls load_pool("anthropic"), then inserts a new random-id manual:dashboard_pkce row containing the same access/refresh pair.
When Anthropic is explicitly configured, load_pool() reads the file and upserts the canonical hermes_pkce row before the manual row is added. The result is therefore:
hermes_pkce— singleton-seeded from.anthropic_oauth.jsonmanual:dashboard_pkce— independently persisted pool row- same single-use refresh token, different ids
Manual entries are normalized ahead of seeded entries, and the new _sync_anthropic_entry_from_pool_store() only adopts a persisted row with the same id. These two representations can never adopt one another’s rotation. Refreshing one leaves the sibling holding the consumed token; later failover/rotation reaches that sibling and gets invalid_grant.
The adjacent xAI dashboard path already states and tests the exact invariant this violates: do not create a parallel manual:dashboard_* row because duplicating a singleton’s single-use refresh token causes refresh_token_reused. Anthropic should use the same single-authority shape. Add a regression proving one dashboard login leaves exactly one authoritative OAuth row/token family.
Dashboard re-login makes this worse: it removes the old manual row and creates a new random id. A still-running process holding the prior id cannot adopt the replacement through the new by-id sync path even though reauthentication succeeded.
2. P1 — a successful hermes_pkce refresh is rolled back to the consumed token on the next pool load
The success path updates and persists the pool entry, then calls _sync_device_code_entry_to_auth_store(). That helper explicitly handles only source == "device_code" and only Nous/Codex/xAI singleton state. Anthropic hermes_pkce is therefore a no-op. The only Anthropic singleton write-back in _refresh_entry_impl() is for source == "claude_code".
Concrete sequence:
.anthropic_oauth.jsonand pool row containRT0.hermes_pkcerefresh succeeds: pool becomesRT1; the singleton file remainsRT0.- A new process calls
load_pool("anthropic"). _seed_from_singletons()reads fileRT0;_upsert_entry(..., source="hermes_pkce")overwrites the same-source pool row back toRT0.- The next refresh replays the already-consumed token and fails.
This is deterministic reload/restart corruption, not merely another concurrency window. The source that seeds a rotating credential must be updated in the same locked commit as the pool row, or it must stop being an independent source of truth. Add a regression that refreshes RT0 -> RT1, constructs a fresh pool, and proves RT1 survives and RT0 is never posted again.
3. P1 — the dashboard’s selected management profile is ignored by every OAuth UI operation
The backend already supports ?profile= for OAuth status, start, submit, persistence, and disconnect. The frontend does not use it:
/api/providers/oauthis absent fromPROFILE_SCOPED_PREFIXES.getOAuthProviders,disconnectOAuthProvider,startOAuthLogin,submitOAuthCode, poll, and cancel do not pass an explicit profile.OAuthProvidersCard/OAuthLoginModalcall those unscoped methods.
Selecting a named profile in the dashboard therefore leaves the Accounts/OAuth surface reading, authenticating, and disconnecting the dashboard process’s/default profile. This is a credential-placement and credential-deletion boundary error, not a display-only issue.
There is a second retargeting hole behind it: _oauth_session_profile() returns stored_profile or fallback. A session started for the default profile stores None, so a submit request carrying ?profile=worker can redirect the final credential save into worker. Bind a concrete profile identity/home at /start (including an explicit default-profile sentinel), propagate the selected profile through every UI operation, and reject any submit/poll/cancel profile mismatch.
4. P2 — cancelling an Anthropic PKCE submission does not cancel the credential write
_submit_anthropic_pkce() grabs a reference to sess, releases _oauth_sessions_lock, performs the network exchange and persistence, and never re-checks sess["cancelled"]. The DELETE endpoint marks that same dict cancelled before popping it, so a modal close during submitting removes the visible session but the in-flight submit still writes the OAuth credential and marks its detached dict approved.
The Codex worker and its existing regressions already demonstrate the required state machine: cancellation check plus final save must be atomic under _oauth_sessions_lock, with a defined point of no return. Apply that contract here. This is the concrete cancellation side of the duplicate-submit issue from my first review; the same atomic pending -> exchanging claim should also ensure only one matching-state submission can reach the token endpoint.
Add controls for successful submit, duplicate submit, cancel-before-exchange, cancel-during-exchange, and cancel-at-commit.
5. P2 — CI never exercises the Windows branch changed by this PR
The OS workflow discovers only files carrying windows_only, then runs pytest -m "windows_only and not integration". Neither tests/hermes_cli/test_auth_store_lock_concurrent.py nor tests/agent/test_anthropic_oauth_stress.py is marked. I checked the actual Windows job (95214793603): it selected 49 files / 142 tests, and neither new file was present.
Those tests green on Linux through fcntl; they do not execute the modified msvcrt.locking() path. So the PR currently has zero CI regression coverage for bug 5 on the OS where bug 5 exists. Mark the relevant tests windows_only and make the authoritative test process-based so it actually exercises independent file handles/process state.
There is also a source-shape fidelity error: the stress test uses source="manual:hermes_pkce", which production never emits. Production emits hermes_pkce or manual:dashboard_pkce. That difference is behaviorally material because refresh encoding is selected with entry.source.endswith("hermes_pkce"): the test takes the JSON path while the real dashboard row takes form encoding. Run the production source matrix explicitly rather than inventing a hybrid source that selects the convenient branch.
Receipt/documentation cleanup
The reports currently claim the class is closed and describe 20 “processes,” while the tests use threads. They also retain stale text saying no production code was changed, and the PR’s stated 13-test total does not match the four new files’ 5 + 3 + 2 + 1 = 11 tests. Please make those receipts describe the code and test topology that actually ships.
The minimal closure matrix now needs to prove:
- one authoritative Anthropic row/source after dashboard login;
- rotated
hermes_pkcetokens survive a freshload_pool(); - running processes adopt a dashboard reauthentication replacement;
- list/start/submit/poll/cancel/disconnect all stay on the selected profile and reject retargeting;
- one matching callback exchanges once, cancellation before commit prevents persistence;
- distinct OS processes serialize one refresh POST;
- the real Windows
msvcrtbranch runs in Windows CI; - actual production sources (
hermes_pkce,manual:dashboard_pkce,claude_code) are covered.
The original CSRF repair is correct. The remaining failures come from not yet choosing one owner for each rotating credential and one owner for each OAuth session/profile transition.
…opic OAuth Add a cross-process lock over the shared ~/.claude/.credentials.json file so concurrent Hermes processes racing a claude_code-sourced Anthropic refresh resync instead of losing the update (mirrors the existing per-profile auth-store lock, kept as the outer lock per the documented lock-ordering invariant). Remove the dashboard-triggered Anthropic PKCE OAuth flow entirely rather than continue patching it: an unattended HTTP endpoint minting Claude Pro/Max subscription tokens outside Anthropic's own client sits on the wrong side of Anthropic's OAuth usage policy. The provider catalog entry is now flow == "external", pointing at `hermes auth add anthropic` (terminal PKCE, unaffected, out of scope). Drop the now-dead PKCE functions/constants and the tests that exercised only that removed code.
|
Thanks for the review, @andrexibiza — pushed a follow-up commit addressing both rounds of feedback. Cross-process Dashboard Anthropic OAuth — removed instead of patched. On reflection, the dashboard's PKCE login ( This also resolves the deeper P1s from the second review (duplicate-identity dashboard row, singleton-reload corruption tied to dashboard writes, ignored Left out of scope, flag if you want it touched too: the terminal flow ( Tests: removed |
Re-review receipt — original blockers repaired; exact candidate still not greenRe-verified current head The repair materially changes the boundary rather than papering over the findings:
The exact candidate is not globally green yet. CI run So the original review findings are repaired at this head, but I am not marking the PR merge-ready while the exact merge candidate remains red and draft/unmergeable. The pre-existing terminal |
Context
Follow-up to a user report that Anthropic OAuth "wasn't working": logging in with a Claude Pro/Max subscription via the web dashboard, then selecting models, still resulted in Hermes billing pay-per-token against an old API key. Digging into that symptom surfaced two independent security bugs and a cross-process race condition in the same code paths; a stress test written to validate the race-condition fix under real concurrency then surfaced a fifth, unrelated bug in a primitive shared by every OAuth provider with single-use refresh tokens. Full root-cause write-up:
ANTHROPIC_ISSUES.md/RELATORIO_ANTHROPIC_OAUTH_BUGS.md(added by this PR).Bug 1 — PKCE
code_verifiersent as OAuthstate(dashboard) — fixes #87887File:
hermes_cli/web_server.py::_start_anthropic_pkce()The dashboard's PKCE login is a separate reimplementation of the same flow already hardened in
agent/anthropic_adapter.py::run_hermes_oauth_login_pure()(PR #10699 / issue #10693, seetests/agent/test_anthropic_oauth_pkce.py's history: PR #1775 fixed it, PR #2647 silently reintroduced it, PR #3107 dropped the old function leaving the regressed copy, PR #10699 fixed it again). The dashboard flow never got that fix:RFC 7636 section 7.2 requires the
code_verifierto stay confidential until the token exchange. Putting it in thestatequery parameter leaks it into browser history,Refererheaders on the authorization page, and Anthropic's own access logs.Bug 2 — callback never validates
state(dashboard) — fixes #87887File:
hermes_cli/web_server.py::_submit_anthropic_pkce()No comparison against
sess["state"]ever happens — any value (or none) is accepted, unlike the CLI flow'sif received_state != oauth_state: abort. This removes the CSRF protection RFC 6749 section 10.12 requires: an attacker who completes their own authorization could get a victim to paste that code/state pair, binding the attacker's Anthropic account to the victim's Hermes session.Bug 3 — Anthropic refresh not serialized across processes — fixes #87888
File:
agent/credential_pool.py::CredentialPool._refresh_entry()Anthropic's OAuth refresh tokens are single-use (
agent/anthropic_adapter.py::_refresh_oauth_token's own docstring: "a successful refresh rotates the pair and invalidates the old refresh token") — the exact property that made Codex/xAI need the cross-process flock in the first place."anthropic"was simply missing from that tuple. Worse, the only failure-recovery path (_sync_anthropic_entry_from_credentials_file) was hard-scoped toentry.source == "claude_code", so credentials from Hermes's own PKCE login (hermes_pkce) or the dashboard (manual:dashboard_pkce) had zero recovery — a lost race against a sibling Hermes process (fleet worker, cron job, second CLI session) permanently marked the credentialSTATUS_EXHAUSTEDeven though a valid token existed on disk.New
_sync_anthropic_entry_from_pool_store()mirrors the existing_sync_xai_oauth_entry_from_pool_store()pattern: re-reads the persisted pool row byidand adopts a rotated pair — provider-agnostic across every Anthropic source (claude_code,hermes_pkce,manual:dashboard_pkce), not justclaude_code. Also added as a failure-path backstop (mirroring the existing Codex/xAI/Nous "adopt winner on terminal failure" branches) in case the pre-lock sync still misses a narrow window.Bug 4 — stale
ANTHROPIC_API_KEYsilently shadows a fresh OAuth login — fixes #87888File:
hermes_cli/web_server.py::_save_anthropic_oauth_creds()resolve_anthropic_token()deliberately prioritizes an explicitANTHROPIC_API_KEYover the credential-pool OAuth entry (priority 3 vs. 5) — "an explicit user-configured key must not be shadowed by auto-discovered [...] credential-pool OAuth credentials," per its own docstring. The CLI OAuth flow (save_anthropic_oauth_tokeninhermes_cli/config.py) already clearsANTHROPIC_API_KEYwhen it saves a fresh OAuth token, keeping the two slots mutually exclusive. The dashboard flow never did — this is the concrete root cause of the reported symptom.Bug 5 — unhandled
PermissionErrorin the shared cross-process lock (Windows) — fixes #87889File:
hermes_cli/auth.py::_file_lock()Found by
tests/agent/test_anthropic_oauth_stress.py: 20 simulated concurrent Hermes processes racing the same stale Anthropic refresh token, added specifically to look for bottlenecks in the bug 3 fix. 16/20 threads raisedPermissionError: [Errno 13] Permission denied— reproduced deterministically across repeated runs.On Windows,
msvcrt.locking()requires the lock file to be non-empty, so_file_lock()writes a placeholder byte before opening it. That write is not protected by theexcept (BlockingIOError, OSError, PermissionError): retryloop immediately below it — it happens before that loop even starts. Under real contention, this write can collide with another thread/process's activemsvcrt.locking()byte-range lock on the same file, and thePermissionErrorpropagates uncaught. Not Anthropic-specific —_file_lock()/_auth_store_lock()is the same primitive Codex, xAI, and Nous already depend on for their own single-use-refresh-token protection; this was just never exercised at this concurrency level before.Files changed
hermes_cli/web_server.pystate(bug 1), state validation on callback (bug 2), clear staleANTHROPIC_API_KEYon OAuth save (bug 4)agent/credential_pool.py"anthropic"to the cross-process lock tuple + new_sync_anthropic_entry_from_pool_store()+ failure-path backstop (bug 3)hermes_cli/auth.pytests/hermes_cli/test_anthropic_dashboard_pkce_csrf.pytests/agent/test_credential_pool_anthropic_refresh_race.pyclaude_codevshermes_pkceasymmetry contrast test)tests/agent/test_anthropic_oauth_stress.pytests/hermes_cli/test_auth_store_lock_concurrent.py_auth_store_lock()concurrency coverage, independent of AnthropicANTHROPIC_ISSUES.md,RELATORIO_ANTHROPIC_OAUTH_BUGS.mdDiagram
%%{init: {'theme': 'dark', 'themeVariables': { 'primaryColor': '#00f0ff', 'mainBkg': '#0a0a16', 'primaryTextColor': '#ffffff', 'primaryBorderColor': '#ff007f', 'lineColor': '#00f0ff'}}}%% graph TD A[Dashboard PKCE Start] -->|state = verifier LEAK| B[Authorization URL] B -->|Callback, no state check| C[CSRF Bypass] A -.fix.-> A2[Independent state token] C -.fix.-> C2[state mismatch rejected] D[Two Hermes Processes] -->|Race stale refresh_token| E[Anthropic Token Endpoint] E -->|Winner rotates pair| F[Credential Pool Store] E -->|Loser: invalid_grant| G[Marked Exhausted] G -.fix.-> G2[Adopt winner via cross-process lock] H[Stale ANTHROPIC_API_KEY] -->|Priority 3 beats OAuth Priority 5| I[Pay-per-token, plan ignored] I -.fix.-> I2[Cleared on dashboard OAuth save] F -->|Stress test: 20 threads| J[PermissionError in file lock] J -.fix.-> J2[Guarded lock-file init write]Test plan
pytest tests/hermes_cli/test_anthropic_dashboard_pkce_csrf.py -v— 5/5 passed (state independent from verifier, verifier never in URL, CSRF rejected on mismatch and on missing state)pytest tests/agent/test_credential_pool_anthropic_refresh_race.py -v— 3/3 passed: structural lock check,hermes_pkceloser recovers (previously the reproduced bug),claude_codeloser still recovers (asymmetry contrast)pytest tests/agent/test_anthropic_oauth_stress.py -v— 2/2 passed: 20-way concurrent refresh race against the real cross-process file lock and real on-disk pool persistence (only the network call mocked), and a 500-session dashboard PKCE burst checking state/verifier uniqueness plus session-table GCpytest tests/hermes_cli/test_auth_store_lock_concurrent.py -v— dedicated, Anthropic-independent lock stress coveragetest_auth_store_lock_concurrent.pyand the stress test fail against the pre-fix code (git stashonhermes_cli/auth.py) — confirms they're real regression guards, not tautologiestest_credential_pool*.py(12 files), xAI OAuth, Codex OAuth, andtest_auth_commands.pysuites (164 tests total) — no regressions; one pre-existing, unrelated Windows/NTFSchmod-permission test failure reproduced identically onmainbefore this branch (os.chmodbits aren't reliably honored by NTFSstat())python -m py_compileon all four edited source files