fix(auth): prevent credential resurrection after Anthropic logout#47
Merged
gsxdsm merged 5 commits intoRunfusion:mainfrom May 6, 2026
Merged
Conversation
The logout flow had two bugs causing credentials to reappear immediately: 1. The codebase has two separate auth storage Proxy chains: - createFusionAuthStorage (engine, for agents) - mergeAuthStorageReads (CLI, for dashboard UI) Neither had a logout trap, so supplemental credentials from ~/.claude/.credentials.json were never excluded after logout. 2. The upstream AuthStorage.hasAuth() checks environment variables (ANTHROPIC_API_KEY), which always returns true regardless of logout. Fix: Add loggedOutProviders tracking to both Proxy chains. All query traps (has, hasAuth, get, getAll, list, getApiKey) return false/undefined for logged-out providers instead of delegating to the underlying storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTracks providers explicitly logged out to prevent supplemental/fallback credentials from being rehydrated; hydrates fallback credentials for non-logged-out providers; updates auth storage proxy traps and resolution to respect logout state; clears usage cache on logout; tests added to validate behaviors. ChangesLogout-Aware Supplemental Credential Handling
Sequence DiagramsequenceDiagram
participant User as User
participant Auth as AuthStorage
participant Supplemental as SupplementalStorage
participant Cache as UsageCache
User->>Auth: logout(provider)
Auth->>Auth: delegate target.logout(provider)
Auth->>Auth: add provider to loggedOutProviders
Auth->>Cache: clearUsageCache()
User->>Auth: has(provider) / get(provider) / getApiKey(provider)
Auth->>Auth: check loggedOutProviders
Auth-->>User: false / undefined
User->>Auth: reload()
Auth->>Supplemental: read fallback creds (skip logged-out)
Auth->>Auth: hydrate non-logged-out credentials
User->>Auth: set(provider, credential)
Auth->>Auth: target.set(provider, credential)
Auth->>Auth: remove provider from loggedOutProviders
Auth-->>User: provider available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR fixes credential resurrection after Anthropic logout by introducing an in-memory
Confidence Score: 5/5Safe to merge; the logic is correct, both proxy layers are symmetric, and the 15 new tests cover all key paths including reload-resurrection, env-var suppression, and re-authentication clearing. All credential-reading traps in both proxy layers consistently check No files require special attention; the only observation is the absence of a Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Dashboard UI
participant Route as POST /auth/logout
participant Merged as mergeAuthStorageReads proxy
participant Fusion as createFusionAuthStorage proxy
participant Primary as AuthStorage (primary)
participant Supp as Supplemental Credentials
UI->>Route: POST /api/auth/logout { provider }
Route->>Merged: logout("anthropic")
Merged->>Merged: loggedOutProviders.add("anthropic")
Merged->>Fusion: logout("anthropic")
Fusion->>Fusion: loggedOutProviders.add("anthropic")
Fusion->>Primary: logout("anthropic")
Primary-->>Fusion: credential removed from auth.json
Route->>Route: clearUsageCache()
Route-->>UI: { success: true }
Note over UI,Supp: Subsequent reads
UI->>Merged: hasAuth("anthropic")
Merged->>Merged: loggedOutProviders.has("anthropic") true
Merged-->>UI: false (short-circuit, Supp never queried)
Note over UI,Supp: On reload()
UI->>Merged: reload()
Merged->>Fusion: reload()
Fusion->>Supp: readSupplementalCredentials()
Fusion->>Fusion: syncSupplementalOauthCredentials() skips anthropic
Merged->>Merged: syncFallbackOauthCredentials() skips anthropic
UI->>Merged: hasAuth("anthropic")
Merged-->>UI: false (credential not re-hydrated)
Note over UI,Supp: On re-authentication
UI->>Merged: set("anthropic", newCred)
Merged->>Merged: loggedOutProviders.delete("anthropic")
Merged->>Fusion: set("anthropic", newCred)
Fusion->>Fusion: loggedOutProviders.delete("anthropic")
Fusion->>Primary: set("anthropic", newCred)
UI->>Merged: hasAuth("anthropic")
Merged-->>UI: true
Reviews (5): Last reviewed commit: "fix(auth): add remove() trap to engine a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/provider-auth.ts`:
- Around line 186-198: Add a proxy branch for the "remove" trap that mirrors the
"logout" behavior: when prop === "remove" return a function that tombstones the
given provider by adding it to loggedOutProviders and then calls
target.remove(provider); reference the existing symbols loggedOutProviders and
target.remove so the API-key deletion (clearApiKey -> mergedAuthStorage.remove)
behaves like logout and prevents resurrection on reads.
- Around line 158-163: getCredential currently delegates to
authStorage.get(providerId) for providers in loggedOutProviders causing
tombstoned providers to still return credentials from underlying storages;
change getCredential so that if loggedOutProviders.has(providerId) it returns
undefined (do not call authStorage.get), otherwise keep calling
selectCredential(providerId, readAuthStorages); update any types if needed to
keep the return type StoredCredential | undefined and ensure
wrapAuthStorageWithApiKeyProviders.hasApiKey/mergedAuthStorage.get no longer see
tombstoned providers' credentials.
In `@packages/engine/src/auth-storage.ts`:
- Around line 254-263: The provider set returned by the proxy handler for "list"
incorrectly includes keys from modelsJsonApiKeys even if they've been tombstoned
in loggedOutProviders; update the "list" branch (the function that builds
providers using modelsJsonApiKeys, supplementalCredentials, and
loggedOutProviders) to exclude any key present in loggedOutProviders — e.g.,
filter modelsJsonApiKeys.keys() (or remove loggedOutProviders from the Set after
creation) so the resulting Array.from(providers) no longer contains logged-out
providers and stays consistent with has() and getApiKey().
- Around line 202-211: In the "get" proxy branch (prop === "get") change the
logged-out behavior to return undefined instead of delegating to
target.get(provider); specifically, where the code currently checks
loggedOutProviders.has(provider) and calls target.get(provider), update it to
immediately return undefined so tombstoned providers cannot be exposed, and
otherwise continue returning
choosePreferredStoredCredential(target.get(provider) as StoredCredential |
undefined, supplementalCredentials[provider]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ead21b60-bf52-42f8-bf10-cf7382984fc2
📒 Files selected for processing (5)
packages/cli/src/commands/__tests__/provider-auth.test.tspackages/cli/src/commands/provider-auth.tspackages/dashboard/src/routes/register-auth-routes.tspackages/engine/src/__tests__/auth-storage.test.tspackages/engine/src/auth-storage.ts
- get() now returns undefined for logged-out providers instead of delegating to target.get() which could bypass the guard - getCredential() in provider-auth returns undefined for logged-out providers instead of falling through to authStorage.get() - getAll() skips logged-out providers at top of loop - list() filters modelsJsonApiKeys against loggedOutProviders - Added remove() trap in provider-auth for clearApiKey flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/auth-storage.ts (1)
257-271: 💤 Low valueConsider filtering
target.list()againstloggedOutProvidersfor consistency.The initial set is built from
target.list()without filtering, whilemodelsJsonApiKeysandsupplementalCredentialsare filtered. This differs from the CLI layer'slist()which filters all providers. Whiletarget.logout()should remove the provider from the underlying storage, adding a final filter would be more defensive and consistent with other traps.♻️ Optional: filter final result for defensive consistency
for (const p of Object.keys(supplementalCredentials)) { if (!loggedOutProviders.has(p)) { providers.add(p); } } - return Array.from(providers); + return Array.from(providers).filter((p) => !loggedOutProviders.has(p)); }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/auth-storage.ts` around lines 257 - 271, The current proxy handler for the "list" trap builds providers from target.list() without filtering out loggedOutProviders, causing inconsistency with how modelsJsonApiKeys and supplementalCredentials are handled; update the returned array in the list() trap to filter out any provider present in loggedOutProviders (in addition to the existing additions from modelsJsonApiKeys and supplementalCredentials) so the final Array.from(providers) excludes loggedOutProviders—this mirrors the CLI behavior and remains defensive even if target.logout() is expected to remove entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/engine/src/auth-storage.ts`:
- Around line 257-271: The current proxy handler for the "list" trap builds
providers from target.list() without filtering out loggedOutProviders, causing
inconsistency with how modelsJsonApiKeys and supplementalCredentials are
handled; update the returned array in the list() trap to filter out any provider
present in loggedOutProviders (in addition to the existing additions from
modelsJsonApiKeys and supplementalCredentials) so the final
Array.from(providers) excludes loggedOutProviders—this mirrors the CLI behavior
and remains defensive even if target.logout() is expected to remove entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 98706a7f-e4c9-4491-af72-44c118065d9b
📒 Files selected for processing (2)
packages/cli/src/commands/provider-auth.tspackages/engine/src/auth-storage.ts
…tency The list() trap now applies a final filter against loggedOutProviders, matching the defensive approach used in the CLI layer. While target.logout() removes entries from underlying storage, this prevents any edge case where a logged-out provider could appear in list() results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/auth-storage.ts`:
- Around line 179-183: The proxy currently mutates the in-memory tombstone sets
(loggedOutProviders / loggedOutUsers) before calling the underlying storage
methods (target.logout and target.set), which can cause state drift if those
calls throw; change the flow in the handlers for the "logout" and "set"
properties so you first call await/execute target.logout(provider) or
target.set(user, value) and only after that succeeds update loggedOutProviders
or loggedOutUsers (and return the original result), and make the same fix in the
other similar block (around the set handler at the 186-190 region) so tombstone
updates happen only after successful underlying storage writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 230b041c-22db-4d41-ae63-3c800592066a
📒 Files selected for processing (1)
packages/engine/src/auth-storage.ts
Reorder logout/set/remove traps so in-memory loggedOutProviders is only updated after the underlying storage write succeeds. If target.logout() or target.set() throws, the tombstone set now stays consistent with the actual storage state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/commands/provider-auth.ts (1)
136-278: 🏗️ Heavy liftConsider extracting shared proxy merge+tombstone logic to one reusable helper.
mergeAuthStorageReads()andcreateFusionAuthStorage()now carry near-identical tombstone/read-merge behavior. Centralizing this logic would reduce regression risk and keep semantics aligned across engine/CLI paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/provider-auth.ts` around lines 136 - 278, Extract the duplicated tombstone + read-merge proxy logic into a single helper (e.g., createMergedAuthStorageProxy) and have both mergeAuthStorageReads and createFusionAuthStorage call it; specifically move the loggedOutProviders Set, selectCredential, getCredential, syncFallbackOauthCredentials, and the Proxy handler implementation into that helper so the helper accepts the primary AuthStorage and an array of ReadFallbackAuthStorage and returns the merged AuthStorage Proxy, then update mergeAuthStorageReads to call the new helper (and modify createFusionAuthStorage to use it) to avoid duplicated logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/auth-storage.ts`:
- Around line 178-191: The proxy get trap handles "logout" and "set" but skips
"remove", so removals don't mark providers in loggedOutProviders; update the get
handler to also intercept prop === "remove" and return a function (provider:
string) that calls target.remove(provider) and then
loggedOutProviders.add(provider) (mirroring the logout behavior) so a removed
provider is tombstoned the same as logout.
---
Nitpick comments:
In `@packages/cli/src/commands/provider-auth.ts`:
- Around line 136-278: Extract the duplicated tombstone + read-merge proxy logic
into a single helper (e.g., createMergedAuthStorageProxy) and have both
mergeAuthStorageReads and createFusionAuthStorage call it; specifically move the
loggedOutProviders Set, selectCredential, getCredential,
syncFallbackOauthCredentials, and the Proxy handler implementation into that
helper so the helper accepts the primary AuthStorage and an array of
ReadFallbackAuthStorage and returns the merged AuthStorage Proxy, then update
mergeAuthStorageReads to call the new helper (and modify createFusionAuthStorage
to use it) to avoid duplicated logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd1e0828-90f1-4419-97dd-0b59cc585c55
📒 Files selected for processing (2)
packages/cli/src/commands/provider-auth.tspackages/engine/src/auth-storage.ts
The CLI proxy already had a remove() trap, but the engine's createFusionAuthStorage was missing it. Without this trap, calling remove() on a provider would delete the credential from storage but not add it to loggedOutProviders, allowing fallback credentials to resurrect the provider on the next read. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 #46 — Anthropic (Claude Pro/Max) logout doesn't work. The provider immediately reappears as authenticated after clicking "Logout".
loggedOutProviderstracking to both auth storage Proxy chains (createFusionAuthStorageandmergeAuthStorageReads) so supplemental credentials from~/.claude/.credentials.jsonand env vars (ANTHROPIC_API_KEY) are excluded after logoutfalse/undefineddirectly from all query traps for logged-out providers, rather than delegating to the underlyingAuthStoragewhich checks env varsset()clearUsageCache()to the logout routeTest plan
packages/cli(9 new logout tests)packages/engine🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements
Tests