diff --git a/src/auth/claude-code-creds.ts b/src/auth/claude-code-creds.ts index 6e412a2..c2f581d 100644 --- a/src/auth/claude-code-creds.ts +++ b/src/auth/claude-code-creds.ts @@ -28,7 +28,12 @@ interface CredentialBlob { const DEFAULT_SCOPES = 'user:profile user:inference user:sessions:claude_code user:mcp_servers' function toSession(data: RawCreds): ClaudeSession | undefined { - if (typeof data.accessToken !== 'string' || typeof data.refreshToken !== 'string' || typeof data.expiresAt !== 'number') { + // Empty-string tokens (seen from a corrupted Keychain item left by a Claude + // Code logout) pass the typeof gate but are useless and would poison the + // auth store — a single such entry fails every provider's status read. + if (typeof data.accessToken !== 'string' || data.accessToken.length === 0 + || typeof data.refreshToken !== 'string' || data.refreshToken.length === 0 + || typeof data.expiresAt !== 'number' || !Number.isFinite(data.expiresAt)) { return undefined } const scopes = Array.isArray(data.scopes) ? data.scopes.join(' ') : typeof data.scopes === 'string' ? data.scopes : DEFAULT_SCOPES diff --git a/src/auth/rpc.ts b/src/auth/rpc.ts index bcee4db..9932d37 100644 --- a/src/auth/rpc.ts +++ b/src/auth/rpc.ts @@ -409,8 +409,15 @@ async function dispatch( ): Promise> { switch (endpoint) { case 'status': { + // One provider's failure (a corrupt store entry, a broken flow) must not + // blind the whole page: it degrades to an error detail on that provider + // while the others still report their real status. const entries = await Promise.all(PROVIDER_IDS.map( - async provider => [provider, await controller.status(provider)] as const, + async provider => [provider, await controller.status(provider).catch((error: unknown) => ({ + busy: false, + accounts: [], + detail: error instanceof Error ? error.message : String(error), + }) satisfies ProviderStatus)] as const, )) return ok({ providers: Object.fromEntries(entries) }) } diff --git a/src/auth/store.ts b/src/auth/store.ts index 36bb60a..2e1c0ca 100644 --- a/src/auth/store.ts +++ b/src/auth/store.ts @@ -203,7 +203,15 @@ export async function loadStore(path = authFilePath()): Promise { return parseStore(text, path) } -/** Parse, validate, and migrate store JSON read from `path`. */ +/** + * Parse and migrate store JSON read from `path`. An ACCOUNT entry whose shape + * is invalid (empty or missing tokens — corruption seen in the wild from a + * broken keychain import) is SKIPPED instead of rejected: one bad entry must + * not blind every provider's status read, and a session without tokens is + * unusable by definition, so nothing of value is discarded. The next write + * persists the store without the skipped entry. Structural failures (invalid + * JSON, a non-object file) still throw — those say the file itself is broken. + */ function parseStore(text: string, path: string): SessionMap { let parsed: unknown try { @@ -220,12 +228,16 @@ function parseStore(text: string, path: string): SessionMap { const entry = raw[provider] if (entry === undefined) continue if (typeof entry !== 'object' || entry === null || Array.isArray(entry)) { - throw new Error(`subscriptions auth store: entry "${provider}" is not an object; fix or delete the store file`) + console.warn(`subscriptions auth store: entry "${provider}" is not an object; skipped`) + continue } const record = entry as Record if (typeof record.accessToken === 'string') { // Single-account format: wrap the bare session, preserving every field. - assertSessionShape(provider, '(legacy)', record) + if (!isValidSessionShape(record)) { + console.warn(`subscriptions auth store: legacy entry "${provider}" has no usable tokens; skipped`) + continue + } const session = record as unknown as StoredSession const key = accountKeyOf(provider, session) ;(store as Record)[provider] = { default: key, accounts: { [key]: session } } @@ -233,21 +245,41 @@ function parseStore(text: string, path: string): SessionMap { } const accounts = record.accounts if (typeof accounts !== 'object' || accounts === null || Array.isArray(accounts)) { - throw new Error( - `subscriptions auth store: entry "${provider}" has no accounts map; fix or delete the store file`, - ) + console.warn(`subscriptions auth store: entry "${provider}" has no accounts map; skipped`) + continue } if (record.default !== undefined && typeof record.default !== 'string') { - throw new Error(`subscriptions auth store: entry "${provider}" default is not a string; fix or delete the store file`) + console.warn(`subscriptions auth store: entry "${provider}" default is not a string; skipped`) + continue } + const kept: Record = {} for (const [account, session] of Object.entries(accounts)) { - assertSessionShape(provider, account, session) + if (isValidSessionShape(session)) { + kept[account] = session as StoredSession + } else { + console.warn( + `subscriptions auth store: entry "${provider}/${account}" has no usable accessToken/refreshToken/expiresAt; skipped`, + ) + } } - ;(store as Record)[provider] = record + if (Object.keys(kept).length === 0) continue + const validDefault = record.default === undefined || record.default in kept + ? record.default as string | undefined + : Object.keys(kept)[0] + ;(store as Record)[provider] = { ...record, default: validDefault, accounts: kept } } return store } +/** Whether a value carries the fields every stored session needs (non-empty tokens). */ +function isValidSessionShape(value: unknown): boolean { + if (typeof value !== 'object' || value === null) return false + const entry = value as Record + return typeof entry.accessToken === 'string' && entry.accessToken.length > 0 + && typeof entry.refreshToken === 'string' && entry.refreshToken.length > 0 + && typeof entry.expiresAt === 'number' && Number.isFinite(entry.expiresAt) +} + /** Persist the whole store atomically with owner-only permissions. */ async function writeStore(store: SessionMap, path: string): Promise { await mkdir(dirname(path), { recursive: true }) @@ -335,10 +367,15 @@ export async function getAccountSession( /** * Write one account's session, preserving the others. The first account of a * provider becomes its default. + * + * The session is validated before it lands: a corrupt entry written here + * would fail every later read of the whole store (one bad entry breaks all + * providers' status), so the write path must be as strict as the read path. * @param provider - the provider route. * @param account - the account key (see {@link accountKeyOf}). * @param session - the fresh session from a login or refresh. * @param path - store file path; defaults to {@link authFilePath}. + * @throws when the session is missing accessToken/refreshToken/expiresAt. */ export async function saveAccountSession( provider: K, @@ -346,6 +383,7 @@ export async function saveAccountSession( session: SessionOf, path = authFilePath(), ): Promise { + assertSessionShape(provider, account, session) return serialize(path, async () => { const store = await loadStore(path) const entry = store[provider] as ProviderAccounts> | undefined diff --git a/src/client/SubscriptionsSection.tsx b/src/client/SubscriptionsSection.tsx index db16242..80f582b 100644 --- a/src/client/SubscriptionsSection.tsx +++ b/src/client/SubscriptionsSection.tsx @@ -589,13 +589,18 @@ export function SubscriptionsSection(props: SubscriptionsSectionProps) { let response: StatusResponse try { response = await callSubscriptionsAuth(rpc, 'status', {}) - } catch { + } catch (error) { // A failed poll must not kill the page; busy providers keep polling and - // the action paths report their own errors. + // the action paths report their own errors. But staying silent turns a + // persistent failure into an endless "Checking…" — show it instead. + const message = error instanceof Error ? error.message : String(error) + for (const { id } of PROVIDERS) setProviderError(id, message) return } if (!mountedRef.current) return setStatuses(response.providers) + // The poll recovered: drop any error line a previous failed poll left. + for (const { id } of PROVIDERS) setProviderError(id, undefined) for (const { id } of PROVIDERS) { const status = response.providers[id] if (status.accounts.length > 0 || !status.busy) { @@ -609,7 +614,7 @@ export function SubscriptionsSection(props: SubscriptionsSectionProps) { }) } } - }, [rpc, stopPolling]) + }, [rpc, stopPolling, setProviderError]) const startPolling = useCallback((provider: SubscriptionProvider): void => { if (pollersRef.current.has(provider)) return diff --git a/test/login.spec.ts b/test/login.spec.ts index ea93bae..2535e87 100644 --- a/test/login.spec.ts +++ b/test/login.spec.ts @@ -186,6 +186,22 @@ test('readClaudeCodeCredentials returns undefined for incomplete credentials', n }) }) +test('readClaudeCodeCredentials returns undefined for EMPTY-string credentials', needsFileStore, async () => { + // A corrupted Keychain item (observed in the wild after a Claude Code + // logout) holds the right keys with empty strings — importing it used to + // poison the auth store and blind every provider's status page. + const blob = JSON.stringify({ + claudeAiOauth: { + accessToken: '', refreshToken: '', expiresAt: 0, + scopes: ['user:profile'], subscriptionType: 'pro', + }, + }) + await withEnv('CLAUDE_CONFIG_DIR', credentialsDir('claude-empty-str-', blob), () => { + assert.equal(readClaudeCodeCredentials(), undefined, 'empty tokens = undefined') + return Promise.resolve() + }) +}) + test('readClaudeCodeCredentials reads bare fields (no claudeAiOauth wrapper)', needsFileStore, async () => { const blob = JSON.stringify({ accessToken: 'bare-at', refreshToken: 'bare-rt', expiresAt: Date.now() + 3600_000, diff --git a/test/rpc.spec.ts b/test/rpc.spec.ts index ad5f21c..086811d 100644 --- a/test/rpc.spec.ts +++ b/test/rpc.spec.ts @@ -2,7 +2,8 @@ * Unit tests for the `/subscriptions-auth` `image` endpoint: payload * validation, the base64 round trip through a fake attachment store, and the * no-service / read-failure error results. Drives the real plugin wiring with - * a fake host connection; DSH_HOME is redirected to a temp dir. + * a fake host connection; DSH_HOME is redirected to a temp dir. Also covers + * the `status` endpoint degrading per provider when one store entry is corrupt. */ import { test } from 'node:test' @@ -179,3 +180,25 @@ test('speed endpoints: per-session tier round trip and payload validation', asyn } } }) + +test('status endpoint: one corrupt provider entry degrades alone, others still report', async () => { + // The exact corruption seen in the wild: empty tokens under a claude key. + // Before the fix this rejected the WHOLE status call and the UI sat on + // "Checking…" forever with every provider blind. + const { mkdirSync: mkDir } = await import('node:fs') + const home = process.env.DSH_HOME as string + mkDir(join(home, 'plugins', 'subscriptions'), { recursive: true }) + writeFileSync(join(home, 'plugins', 'subscriptions', 'auth.json'), JSON.stringify({ + codex: { default: 'acct-1', accounts: { 'acct-1': { + accessToken: 'at', refreshToken: 'rt', expiresAt: Date.now() + 3600_000, accountId: 'acct-1', + } } }, + claude: { default: 'corrupt', accounts: { corrupt: { accessToken: '', refreshToken: '', expiresAt: 0 } } }, + }), { mode: 0o600 }) + const handler = await mount() + const result = await handler('status', {}, new AbortController().signal) + assert.ok(result.ok, 'the status call itself must succeed') + if (!result.ok) return + const providers = (result.value as { providers: Record }).providers + assert.equal(providers.codex.accounts.length, 1, 'codex still reports its account') + assert.equal(providers.claude.accounts.length, 0, 'the corrupt claude entry is skipped') +}) diff --git a/test/store.spec.ts b/test/store.spec.ts index 951510a..70595fa 100644 --- a/test/store.spec.ts +++ b/test/store.spec.ts @@ -153,3 +153,46 @@ test('a single-account store migrates on read, preserving every field', async () const onDisk = JSON.parse(readFileSync(path, 'utf8')) as Record assert.ok(onDisk.codex?.accounts !== undefined, 'the file now uses the accounts shape') }) + +test('saveAccountSession rejects an empty-token session before it can poison the store', async () => { + const path = storePath() + await saveAccountSession('codex', 'acct-1', CODEX, path) + const corrupt = { ...CLAUDE, accessToken: '', refreshToken: '', expiresAt: 0 } + await saveAccountSession('claude', 'corrupt', corrupt, path).then( + () => assert.fail('saving an empty-token session must throw'), + (error: unknown) => assert.match(String(error), /missing accessToken\/refreshToken\/expiresAt/), + ) + // The store survives intact: the earlier valid account is still readable. + assert.equal((await getAccountSession('codex', undefined, path))?.accessToken, CODEX.accessToken) +}) + +test('one corrupt provider entry does not blind the other providers', async () => { + const path = storePath() + // The exact corruption seen in the wild: empty tokens under a claude key. + writeFileSync(path, JSON.stringify({ + codex: { default: 'acct-1', accounts: { 'acct-1': CODEX } }, + claude: { default: 'corrupt', accounts: { corrupt: { accessToken: '', refreshToken: '', expiresAt: 0 } } }, + }), { mode: 0o600 }) + // Codex keeps its account; the corrupt claude entry is skipped, not fatal. + assert.equal((await listAccounts('codex', path)).length, 1) + assert.equal((await listAccounts('claude', path)).length, 0) + // …and the next write persists the store without the corrupt entry. + await saveAccountSession('codex', 'acct-1', CODEX, path) + const onDisk = JSON.parse(readFileSync(path, 'utf8')) as Record + assert.equal(onDisk.claude, undefined, 'the corrupt entry is dropped on the next write') +}) + +test('a valid account survives alongside a corrupt sibling of the same provider', async () => { + const path = storePath() + writeFileSync(path, JSON.stringify({ + codex: { + default: 'acct-1', + accounts: { + 'acct-1': CODEX, + corrupt: { accessToken: '', refreshToken: '', expiresAt: 0 }, + }, + }, + }), { mode: 0o600 }) + const entries = await listAccounts('codex', path) + assert.deepEqual(entries.map((entry) => entry.key), ['acct-1'], 'only the valid account is listed') +})