From e5b751e4afa40ba386173d9b24be2cda1247788c Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Tue, 1 Sep 2026 17:07:03 -0700 Subject: [PATCH] fix(claude): don't clear a managed selection when the outgoing oauth identity is unreadable Item 18 of the enumeration, which I had deferred as "clears nothing durable". That was wrong, and the review's reading of runtime-auth-sync is right. `readManagedOauthAccount`'s null is the third disjunct of the test that decides whether Orca restores the user's system default before dropping a managed selection. `runtimeOauthAccountMatches(null)` is false, so a failed read did not cause a wrong restore -- it caused NO restore, while the `updateSettings({ activeClaudeManagedAccountId: null })` a few lines later ran regardless. The runtime kept holding the managed account's credentials with nothing in Orca pointing at them: not a wrong undo, a missing one. It is also not a rare corner. The disjunct only decides when the account is unchanged AND `hasMaterializedRuntimeAuth` is false, and the service constructor seeds `lastSyncedAccountId` from persisted settings without materializing -- so that is the state of the FIRST sync after every app start with a selected host managed account, and the oauth identity is the only evidence available. The read is now tri-state, and an indeterminate one defers the whole transition: neither the restore nor the clear. Deferring rather than restoring-anyway is deliberate -- `restoreSystemDefaultSnapshotForMissingManagedCredentials` consumes the same oauth value for its own ownership checks, so running it on a failed read would degrade exactly the checks that decide what is safe to overwrite. Malformed JSON stays dispositive: that is a completed observation of the file. The two teardown blocks were byte-identical once both used the hoisted read, so they are now one method, and the selection-clearing helpers moved to their own module to stay under the line cap. Refs STA-5674. --- ...auth-service-wsl-ownership-verdict.test.ts | 99 +++++++++++++++- .../runtime-auth-managed-credentials.ts | 40 +++++-- .../runtime-auth-selection-teardown.ts | 50 ++++++++ .../runtime-auth/runtime-auth-sync.ts | 111 +++++++++--------- 4 files changed, 236 insertions(+), 64 deletions(-) create mode 100644 src/main/claude-accounts/runtime-auth/runtime-auth-selection-teardown.ts diff --git a/src/main/claude-accounts/runtime-auth-service-wsl-ownership-verdict.test.ts b/src/main/claude-accounts/runtime-auth-service-wsl-ownership-verdict.test.ts index b186ea323d5..c561098de63 100644 --- a/src/main/claude-accounts/runtime-auth-service-wsl-ownership-verdict.test.ts +++ b/src/main/claude-accounts/runtime-auth-service-wsl-ownership-verdict.test.ts @@ -20,7 +20,9 @@ import { const fsFaults = vi.hoisted(() => ({ lockedRealpathSuffix: null as string | null, - lockedReadSuffix: null as string | null + lockedReadSuffix: null as string | null, + lockedReadHits: 0, + lockedRealpathHits: 0 })) vi.mock('node:fs', async (importOriginal) => { @@ -30,6 +32,7 @@ vi.mock('node:fs', async (importOriginal) => { fsFaults.lockedRealpathSuffix !== null && String(path).endsWith(fsFaults.lockedRealpathSuffix) ) { + fsFaults.lockedRealpathHits += 1 const error = new Error(`EBUSY: resource busy or locked, realpath`) as NodeJS.ErrnoException error.code = 'EBUSY' throw error @@ -39,6 +42,7 @@ vi.mock('node:fs', async (importOriginal) => { realpathSync.native = original.realpathSync.native const readFileSync = ((path: never, options: never) => { if (fsFaults.lockedReadSuffix !== null && String(path).endsWith(fsFaults.lockedReadSuffix)) { + fsFaults.lockedReadHits += 1 const error = new Error('EBUSY: resource busy or locked, read') as NodeJS.ErrnoException error.code = 'EBUSY' throw error @@ -237,6 +241,8 @@ describe('runtime-auth host ownership probe', () => { resetRuntimeAuthTestState() fsFaults.lockedRealpathSuffix = null fsFaults.lockedReadSuffix = null + fsFaults.lockedReadHits = 0 + fsFaults.lockedRealpathHits = 0 setPlatform('linux') }) @@ -294,6 +300,97 @@ describe('runtime-auth host ownership probe', () => { } }) + it('keeps the active host account when the outgoing oauth identity cannot be read', async () => { + // Reaching the teardown needs the credentials gone (a dispositive absence); + // the oauth read is then the only thing deciding whether the user's default + // gets restored before the selection is dropped. + const managedAuthPath = createManagedClaudeAuth( + testState.userDataDir, + 'host-account', + createClaudeCredentialsJson('alice@example.com', 'alice-token') + ) + rmSync(join(managedAuthPath, '.credentials.json')) + fsFaults.lockedReadSuffix = 'oauth-account.json' + const settings = createSettings({ + claudeManagedAccounts: [createClaudeAccount('host-account', managedAuthPath)], + activeClaudeManagedAccountId: 'host-account', + activeClaudeManagedAccountIdsByRuntime: { host: 'host-account', wsl: {} } + }) + const store = createStore(settings) + const { ClaudeRuntimeAuthService } = await import('./runtime-auth-service') + await new ClaudeRuntimeAuthService(store as never).syncForCurrentSelection() + + // Prove the injected fault was actually consumed, not merely armed. + expect(fsFaults.lockedReadHits).toBeGreaterThan(0) + // Clearing here without restoring would leave the runtime holding this + // account's credentials with nothing in Orca pointing at them. + expect(store.getSettings().activeClaudeManagedAccountId).toBe('host-account') + }) + + it('clears the active host account when the outgoing oauth file is malformed', async () => { + // Control for the case above: malformed JSON is a completed observation, so + // the teardown must proceed rather than defer forever on a corrupt file. + const managedAuthPath = createManagedClaudeAuth( + testState.userDataDir, + 'host-account', + createClaudeCredentialsJson('alice@example.com', 'alice-token') + ) + rmSync(join(managedAuthPath, '.credentials.json')) + writeFileSync(join(managedAuthPath, 'oauth-account.json'), '{ not json') + const settings = createSettings({ + claudeManagedAccounts: [createClaudeAccount('host-account', managedAuthPath)], + activeClaudeManagedAccountId: 'host-account', + activeClaudeManagedAccountIdsByRuntime: { host: 'host-account', wsl: {} } + }) + const store = createStore(settings) + const { ClaudeRuntimeAuthService } = await import('./runtime-auth-service') + await new ClaudeRuntimeAuthService(store as never).syncForCurrentSelection() + + expect(store.getSettings().activeClaudeManagedAccountId).toBeNull() + }) + + it('keeps the selection when the OUTGOING account directory cannot be read during a switch', async () => { + // Switching away: the outgoing account is not the active one, so the restore + // is warranted -- but it consumes the outgoing oauth identity, so an + // unreadable directory means the restore cannot be run safely either. + const outgoing = createManagedClaudeAuth( + testState.userDataDir, + 'outgoing-account', + createClaudeCredentialsJson('alice@example.com', 'alice-token') + ) + const incoming = createManagedClaudeAuth( + testState.userDataDir, + 'incoming-account', + createClaudeCredentialsJson('bob@example.com', 'bob-token') + ) + let settings = createSettings({ + claudeManagedAccounts: [ + createClaudeAccount('outgoing-account', outgoing), + createClaudeAccount('incoming-account', incoming) + ], + activeClaudeManagedAccountId: 'outgoing-account', + activeClaudeManagedAccountIdsByRuntime: { host: 'outgoing-account', wsl: {} } + }) + const store = createStore(settings) + const { ClaudeRuntimeAuthService } = await import('./runtime-auth-service') + const service = new ClaudeRuntimeAuthService(store as never) + await service.syncForCurrentSelection() + + // Now select the incoming account and make it dispositively unusable, so the + // teardown runs with the outgoing account as `previousAccount`. + settings = store.updateSettings({ + activeClaudeManagedAccountId: 'incoming-account', + activeClaudeManagedAccountIdsByRuntime: { host: 'incoming-account', wsl: {} } + }) as typeof settings + rmSync(incoming, { recursive: true, force: true }) + fsFaults.lockedRealpathSuffix = join('outgoing-account', 'auth') + + await service.syncForCurrentSelection() + + expect(fsFaults.lockedRealpathHits).toBeGreaterThan(0) + expect(store.getSettings().activeClaudeManagedAccountId).toBe('incoming-account') + }) + it('still clears the active host account when its directory is proven gone', async () => { const store = await (async () => { const managedAuthPath = createManagedClaudeAuth( diff --git a/src/main/claude-accounts/runtime-auth/runtime-auth-managed-credentials.ts b/src/main/claude-accounts/runtime-auth/runtime-auth-managed-credentials.ts index 50ffd6518b7..4fb4d049b91 100644 --- a/src/main/claude-accounts/runtime-auth/runtime-auth-managed-credentials.ts +++ b/src/main/claude-accounts/runtime-auth/runtime-auth-managed-credentials.ts @@ -5,7 +5,6 @@ import { type ClaudeManagedAuthVerdict } from '../claude-managed-auth-ownership' import { - readClaudeManagedAuthFile, readClaudeManagedAuthFileResult, resolveClaudeManagedAuthVerdict, writeClaudeManagedAuthFile, @@ -19,6 +18,18 @@ import { } from '../keychain' import { ClaudeRuntimeAuthCredentialIdentity } from './runtime-auth-credential-identity' +/** + * Why this needs a result too: the null it replaces is the third disjunct of the + * decision that restores the user's system default before Orca clears a managed + * selection. A failed read made that disjunct false, so no restore ran and the + * selection was cleared anyway -- the runtime kept holding managed credentials + * for an account Orca had forgotten. + */ +export type ClaudeManagedOauthRead = + | { kind: 'present'; value: unknown } + | { kind: 'absent' } + | { kind: 'indeterminate'; error: unknown } + export class ClaudeRuntimeAuthManagedCredentials extends ClaudeRuntimeAuthCredentialIdentity { protected async readManagedCredentials(account: ClaudeManagedAccount): Promise { const managedAuthPath = await this.getOwnedManagedAuthPath(account) @@ -103,15 +114,30 @@ export class ClaudeRuntimeAuthManagedCredentials extends ClaudeRuntimeAuthCreden } protected async readManagedOauthAccount(account: ClaudeManagedAccount): Promise { - const managedAuthPath = await this.getOwnedManagedAuthPath(account) - if (!managedAuthPath) { - return null + const read = await this.readManagedOauthAccountResult(account) + return read.kind === 'present' ? read.value : null + } + + protected async readManagedOauthAccountResult( + account: ClaudeManagedAccount + ): Promise { + const verdict = await this.resolveManagedAuthVerdict(account) + if (verdict.kind === 'indeterminate') { + return { kind: 'indeterminate', error: verdict.error } + } + if (verdict.kind === 'untrusted') { + return { kind: 'absent' } + } + const read = readClaudeManagedAuthFileResult(verdict.authPath, 'oauth-account.json') + if (read.kind !== 'present') { + return read } try { - const contents = readClaudeManagedAuthFile(managedAuthPath, 'oauth-account.json') - return contents ? (JSON.parse(contents) as unknown) : null + return { kind: 'present', value: JSON.parse(read.contents) as unknown } } catch { - return null + // Malformed JSON is a completed observation of the file, not a failure to + // read it. + return { kind: 'absent' } } } diff --git a/src/main/claude-accounts/runtime-auth/runtime-auth-selection-teardown.ts b/src/main/claude-accounts/runtime-auth/runtime-auth-selection-teardown.ts new file mode 100644 index 00000000000..70c2f60cd7a --- /dev/null +++ b/src/main/claude-accounts/runtime-auth/runtime-auth-selection-teardown.ts @@ -0,0 +1,50 @@ +import type { Store } from '../../persistence' +import { + normalizeClaudeRuntimeSelection, + setSelectedClaudeAccountIdForTarget, + type ClaudeAccountSelectionTarget +} from '../runtime-selection' +import type { ClaudeManagedOauthRead } from './runtime-auth-managed-credentials' + +/** + * Drop the account selected for `target`, keeping the legacy host field in step. + * Three call sites in the sync did this identically. + */ +export function clearClaudeSelectionForTarget( + store: Store, + settings: ReturnType, + target: ClaudeAccountSelectionTarget +): void { + store.updateSettings({ + activeClaudeManagedAccountId: + target.runtime === 'host' ? null : settings.activeClaudeManagedAccountId, + activeClaudeManagedAccountIdsByRuntime: setSelectedClaudeAccountIdForTarget( + normalizeClaudeRuntimeSelection(settings), + null, + target + ) + }) +} + +/** + * The outgoing account's oauth identity is the only evidence that the runtime is + * still holding THIS account's credentials, and it is the sole decider on the + * first sync after a restart: the service seeds `lastSyncedAccountId` from + * settings without materializing, so the account is unchanged and + * `hasMaterializedRuntimeAuth` is false. + * + * If it could not be read we can neither prove a restore is needed nor safely + * run one -- the restore consumes the same value for its own ownership checks -- + * so the caller must leave the whole transition for the next sync rather than + * clear the selection without putting the user's default back. + */ +export function shouldDeferOnUnreadableOauth(read: ClaudeManagedOauthRead | null): boolean { + if (read?.kind !== 'indeterminate') { + return false + } + console.warn( + '[claude-runtime-auth] Could not read the outgoing account oauth identity; leaving the selection in place', + read.error + ) + return true +} diff --git a/src/main/claude-accounts/runtime-auth/runtime-auth-sync.ts b/src/main/claude-accounts/runtime-auth/runtime-auth-sync.ts index 0d6c8200b5e..2b1c338fc0a 100644 --- a/src/main/claude-accounts/runtime-auth/runtime-auth-sync.ts +++ b/src/main/claude-accounts/runtime-auth/runtime-auth-sync.ts @@ -2,14 +2,18 @@ import { existsSync, readFileSync } from 'node:fs' import { getSelectedClaudeAccountIdForTarget, normalizeClaudeAccountSelectionTarget, - normalizeClaudeRuntimeSelection, - setSelectedClaudeAccountIdForTarget, type ClaudeAccountSelectionTarget } from '../runtime-selection' import { hasLiveClaudePtys } from '../live-pty-gate' import { isOauthTokenExpiring } from '../oauth-refresh' import { writeActiveClaudeKeychainCredentialsForRuntime } from '../keychain' import { ClaudeRuntimeAuthPreparationService } from './runtime-auth-preparation' +import type { ClaudeManagedAccount } from '../../../shared/managed-account-types' +import type { ClaudeManagedOauthRead } from './runtime-auth-managed-credentials' +import { + clearClaudeSelectionForTarget, + shouldDeferOnUnreadableOauth +} from './runtime-auth-selection-teardown' export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { protected async doSyncForCurrentSelection(target?: ClaudeAccountSelectionTarget): Promise { @@ -26,9 +30,11 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { const previousManagedCredentialsJson = previousAccount ? await this.readManagedCredentials(previousAccount) : null - const previousManagedOauthAccount = previousAccount - ? await this.readManagedOauthAccount(previousAccount) + const previousManagedOauthRead = previousAccount + ? await this.readManagedOauthAccountResult(previousAccount) : null + const previousManagedOauthAccount = + previousManagedOauthRead?.kind === 'present' ? previousManagedOauthRead.value : null if (previousAccount && previousAccount.id !== activeAccount?.id) { if (previousManagedCredentialsJson) { const outgoingReadBackResult = await this.readBackRefreshedTokens( @@ -67,7 +73,7 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { } if (!activeAccount) { if (activeAccountId) { - this.clearSelectionForTarget(settings, normalizedTarget) + clearClaudeSelectionForTarget(this.store, settings, normalizedTarget) } if (normalizedTarget.runtime === 'wsl') { return @@ -100,7 +106,7 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { console.warn( '[claude-runtime-auth] Active WSL managed account is not owned by Orca, restoring system default' ) - this.clearSelectionForTarget(settings, normalizedTarget) + clearClaudeSelectionForTarget(this.store, settings, normalizedTarget) return } const wslCredentials = await this.readManagedCredentialsResultAt( @@ -119,7 +125,7 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { console.warn( '[claude-runtime-auth] Active WSL managed account is missing or has invalid credentials, restoring system default' ) - this.clearSelectionForTarget(settings, normalizedTarget) + clearClaudeSelectionForTarget(this.store, settings, normalizedTarget) return } // Why: WSL managed accounts are isolated by their Linux CLAUDE_CONFIG_DIR; materializing into Windows ~/.claude would mix two auth stores. @@ -139,23 +145,12 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { console.warn( '[claude-runtime-auth] Active managed account is not owned by Orca, restoring system default' ) - if (this.lastSyncedAccountId !== null) { - if ( - previousAccount && - (previousAccount.id !== activeAccount.id || - this.hasMaterializedRuntimeAuth || - this.runtimeOauthAccountMatches(await this.readManagedOauthAccount(previousAccount))) - ) { - await this.restoreSystemDefaultSnapshotForMissingManagedCredentials( - previousAccount, - previousManagedOauthAccount - ) - } else if (!previousAccount && this.hasMaterializedRuntimeAuth) { - await this.restoreSystemDefaultSnapshot(this.lastWrittenCredentialsJson, undefined) - } - } - this.store.updateSettings({ activeClaudeManagedAccountId: null }) - this.lastSyncedAccountId = null + await this.restoreDefaultAndDropHostSelection( + activeAccount, + previousAccount, + previousManagedOauthRead, + previousManagedOauthAccount + ) return } @@ -175,23 +170,12 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { console.warn( '[claude-runtime-auth] Active managed account is missing or has invalid credentials, restoring system default' ) - if (this.lastSyncedAccountId !== null) { - if ( - previousAccount && - (previousAccount.id !== activeAccount.id || - this.hasMaterializedRuntimeAuth || - this.runtimeOauthAccountMatches(previousManagedOauthAccount)) - ) { - await this.restoreSystemDefaultSnapshotForMissingManagedCredentials( - previousAccount, - previousManagedOauthAccount - ) - } else if (!previousAccount && this.hasMaterializedRuntimeAuth) { - await this.restoreSystemDefaultSnapshot(this.lastWrittenCredentialsJson, undefined) - } - } - this.store.updateSettings({ activeClaudeManagedAccountId: null }) - this.lastSyncedAccountId = null + await this.restoreDefaultAndDropHostSelection( + activeAccount, + previousAccount, + previousManagedOauthRead, + previousManagedOauthAccount + ) return } @@ -297,21 +281,36 @@ export class ClaudeRuntimeAuthSync extends ClaudeRuntimeAuthPreparationService { } /** - * Drop the account selected for `target`, keeping the legacy host field in - * step. Three call sites did this identically. + * Undo the managed materialization, if we can establish that it happened, then + * drop the host selection. When the oauth identity cannot be read this does + * nothing at all -- neither the restore nor the clear -- so the next sync + * retries the whole transition rather than completing half of it. */ - private clearSelectionForTarget( - settings: ReturnType, - target: ReturnType - ): void { - this.store.updateSettings({ - activeClaudeManagedAccountId: - target.runtime === 'host' ? null : settings.activeClaudeManagedAccountId, - activeClaudeManagedAccountIdsByRuntime: setSelectedClaudeAccountIdForTarget( - normalizeClaudeRuntimeSelection(settings), - null, - target - ) - }) + private async restoreDefaultAndDropHostSelection( + activeAccount: ClaudeManagedAccount, + previousAccount: ClaudeManagedAccount | null, + previousManagedOauthRead: ClaudeManagedOauthRead | null, + previousManagedOauthAccount: unknown + ): Promise { + if (this.lastSyncedAccountId !== null) { + if (shouldDeferOnUnreadableOauth(previousManagedOauthRead)) { + return + } + if ( + previousAccount && + (previousAccount.id !== activeAccount.id || + this.hasMaterializedRuntimeAuth || + this.runtimeOauthAccountMatches(previousManagedOauthAccount)) + ) { + await this.restoreSystemDefaultSnapshotForMissingManagedCredentials( + previousAccount, + previousManagedOauthAccount + ) + } else if (!previousAccount && this.hasMaterializedRuntimeAuth) { + await this.restoreSystemDefaultSnapshot(this.lastWrittenCredentialsJson, undefined) + } + } + this.store.updateSettings({ activeClaudeManagedAccountId: null }) + this.lastSyncedAccountId = null } }