mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
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.
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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<string | null> {
|
||||
const managedAuthPath = await this.getOwnedManagedAuthPath(account)
|
||||
@@ -103,15 +114,30 @@ export class ClaudeRuntimeAuthManagedCredentials extends ClaudeRuntimeAuthCreden
|
||||
}
|
||||
|
||||
protected async readManagedOauthAccount(account: ClaudeManagedAccount): Promise<unknown> {
|
||||
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<ClaudeManagedOauthRead> {
|
||||
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' }
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<Store['getSettings']>,
|
||||
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
|
||||
}
|
||||
@@ -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<void> {
|
||||
@@ -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<typeof this.store.getSettings>,
|
||||
target: ReturnType<typeof normalizeClaudeAccountSelectionTarget>
|
||||
): 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<void> {
|
||||
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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user