From 8c46032db2e192c1bee0c70d21f5baca01b310bb Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 19 Oct 2026 13:17:55 +0800 Subject: [PATCH] fix(aimlapi): derive the by-key identity without hashing the API key (#2293) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves CodeQL js/insufficient-password-hash (alert 70): aimlapiByKeyIdentity fed the API key into a plain SHA-256. AI/ML API by-key top-ups (src/integrations/aimlapi, ProviderManager) - The identity is now `key:v2:` + HMAC-SHA256 keyed BY the credential over a fixed label, so the key is never the hashed input. It stays cheap on the interactive path (no KDF stalling Ink before the async lock), and a known-answer test pins the value so the derivation cannot drift silently. - Receipts written by earlier versions carry `key:`. That value is deliberately never recomputed, so they are handled without hashing: - one that stores this credential (a settled saved-key top-up waiting on its profile write, which a claim otherwise refuses to replace even with abandonExisting) is matched by its stored key and upgraded under the lock, in both reconcileSettledAimlapiTopupStateAsync and the by-key claim (new `byKeyApiKey` claim option, passed by ProviderManager); - one with no stored key (an in-flight by-key checkout, or a settled env-key top-up) cannot be attributed and holds nothing unrecoverable, since a by-key payment is credited to its key server-side, so the next claim may replace it instead of refusing a re-run that can never match. Both are skipped while a mint/exchange lease on the record is live. Skills watcher test (src/utils/skills) - `disabled sibling edits stop triggering hooks…` flaked on loaded CI runners ("Expected number of calls: 1, Received: 2"). Its edit helper retries writes every 50ms until a hook fires, and a retry already in flight could land one more hook after the exact-count assertion or after the sibling watcher closed. Assert that each phase's edit was observed, wait until hooks are quiet between phases, and give the real-filesystem test a 15s budget. Under 12 saturated cores: original 2/15 failures, fixed 45/45 passes. Co-authored-by: OpenClaude --- src/components/ProviderManager.test.tsx | 14 +- src/components/ProviderManager.tsx | 3 + src/integrations/aimlapi/topupState.test.ts | 150 +++++++++++++++++++ src/integrations/aimlapi/topupState.ts | 95 ++++++++++-- src/utils/skills/skillChangeDetector.test.ts | 38 ++++- 5 files changed, 282 insertions(+), 18 deletions(-) diff --git a/src/components/ProviderManager.test.tsx b/src/components/ProviderManager.test.tsx index 8099d570f..4620d6bf9 100644 --- a/src/components/ProviderManager.test.tsx +++ b/src/components/ProviderManager.test.tsx @@ -389,6 +389,8 @@ function mockProviderManagerDependencies( completeAimlapiCodeSignIn?: (...args: any[]) => Promise validateAimlapiApiKey?: (...args: any[]) => Promise claimAimlapiTopupStateAsync?: (...args: any[]) => unknown + // Observes the default (stateful) claim's arguments without replacing it. + onAimlapiTopupClaim?: (intent: Record, claimOptions: unknown) => void clearAimlapiTopupStateAsync?: (...args: any[]) => unknown recordAimlapiCheckoutSessionAsync?: (...args: any[]) => unknown resetAimlapiCheckoutSessionAsync?: (...args: any[]) => unknown @@ -547,7 +549,8 @@ function mockProviderManagerDependencies( (async () => ({ balance: 25, lowBalance: false, lowBalanceThreshold: 20 })), claimAimlapiTopupStateAsync: options?.claimAimlapiTopupStateAsync ?? - (async (intent: Record) => { + (async (intent: Record, claimOptions?: unknown) => { + options?.onAimlapiTopupClaim?.(intent, claimOptions) if (matchesAimlapiIntent(persistedAimlapiTopup, intent)) { return { paymentSessionId: persistedAimlapiTopup?.paymentSessionId, @@ -1574,11 +1577,15 @@ test('ProviderManager tops up a low-balance saved key by API key, not a new chec model: 'gpt-4o', } }) + const onAimlapiTopupClaim = mock( + (_intent: Record, _claimOptions: unknown) => {}, + ) mockProviderManagerDependencies(() => undefined, async () => undefined, { addProviderProfile, validateAimlapiApiKey, topUpAimlapiByApiKey, + onAimlapiTopupClaim, }) const nonce = `${Date.now()}-${Math.random()}` @@ -1618,6 +1625,11 @@ test('ProviderManager tops up a low-balance saved key by API key, not a new chec expect(topUpOptions.paymentSessionId).toEqual(expect.any(String)) expect(topUpOptions.paymentSessionId).not.toBe('') expect(topUpOptions.resumeSessionToken).toBe('') + // The claim carries the credential so it can adopt a receipt an earlier + // version wrote for it under the legacy identity. + const [claimIntent, claimOptions] = onAimlapiTopupClaim.mock.calls[0] ?? [] + expect(claimIntent?.email).toBe(aimlapiByKeyIdentity('aimlapi-test-key')) + expect(claimOptions).toEqual(expect.objectContaining({ byKeyApiKey: 'aimlapi-test-key' })) await waitForCondition(() => addProviderProfile.mock.calls.length > 0) expect(addProviderProfile).toHaveBeenCalledWith( diff --git a/src/components/ProviderManager.tsx b/src/components/ProviderManager.tsx index ee6927dcf..56c9bd239 100644 --- a/src/components/ProviderManager.tsx +++ b/src/components/ProviderManager.tsx @@ -3392,6 +3392,9 @@ export function ProviderManager({ mode, onDone }: Props): React.ReactNode { checkoutState = await claimAimlapiTopupStateAsync(intent, { abandonExisting: abandoningOpenedCheckout || abandoningPersistedIntent || forceAbandonExisting, + // Lets the claim adopt a receipt an earlier version wrote for this + // same credential under the legacy identity (see aimlapiByKeyIdentity). + ...(aimlapiTopupByKey ? { byKeyApiKey: aimlapiIssuedKey } : {}), }) } catch (error) { if (controller.signal.aborted) return diff --git a/src/integrations/aimlapi/topupState.test.ts b/src/integrations/aimlapi/topupState.test.ts index 69ee99768..e20fdc871 100644 --- a/src/integrations/aimlapi/topupState.test.ts +++ b/src/integrations/aimlapi/topupState.test.ts @@ -1164,6 +1164,156 @@ test('reconcileSettledAimlapiTopupStateAsync leaves an unsettled (still in-progr expect(loadAimlapiTopupState(keyAIntent)?.apiKey).toBe('key-a') }) +test('aimlapiByKeyIdentity is pinned: persisted receipts key on this exact value', () => { + // Known answer, not a recomputation: changing the derivation silently + // orphans every by-key receipt already on disk, so it must fail here first. + expect(aimlapiByKeyIdentity(' key-a\n')).toBe( + 'key:v2:64f9586e11869548ac185049673d8e13f98f7baf364d6a174ac7de4a41039e8e', + ) + expect(aimlapiByKeyIdentity('key-b')).not.toBe(aimlapiByKeyIdentity('key-a')) +}) + +// What versions before the HMAC identity wrote into a by-key intent's `email` +// field (a plain SHA-256 of the trimmed key), as literals: those receipts are +// already on disk, so the values to handle are fixed. +const LEGACY_BY_KEY_IDENTITIES = { + 'key-a': 'key:f10f781241e2246678b6b45c857069208152a53863e47fac33f607ab405006f4', + 'env-key-a': 'key:cd2374cd39e15cbed7e8a5c93f1c15ffb00b1a295b26fb00a181e91e9c6e2f03', +} as const + +function legacyByKeyIntent(apiKey: keyof typeof LEGACY_BY_KEY_IDENTITIES): AimlapiTopupIntent { + return { ...intent, email: LEGACY_BY_KEY_IDENTITIES[apiKey] } +} + +function persistedEmail(directory: string): string { + return JSON.parse(readFileSync(join(directory, 'aimlapi-topup.json'), 'utf8')).email +} + +// A settled saved-key top-up still waiting on its profile write: the one legacy +// receipt claimTopupStateOperation refuses to replace even with abandonExisting. +function seedLegacySettledKeyReceipt(): string { + const legacyIntent = legacyByKeyIntent('key-a') + const claimed = claimAimlapiTopupState(legacyIntent) + saveAimlapiTopupState({ + ...legacyIntent, + paymentSessionId: claimed.paymentSessionId, + resumeSessionToken: 'paid-session', + apiKey: 'key-a', + settled: true, + }) + return claimed.paymentSessionId +} + +test('re-running a by-key top-up adopts its settled legacy receipt by the stored key and upgrades it', async () => { + const directory = useTemporaryConfig() + const paymentSessionId = seedLegacySettledKeyReceipt() + const currentIntent = byKeyIntent('key-a') + + const resumed = await claimAimlapiTopupStateAsync(currentIntent, { byKeyApiKey: 'key-a' }) + + expect(resumed.paymentSessionId).toBe(paymentSessionId) + expect(resumed.settled).toBe(true) + expect(persistedEmail(directory)).toBe(currentIntent.email) + // The CAS operations after the claim key on the current identity and land. + clearAimlapiTopupState({ ...currentIntent, paymentSessionId }) + expect(existsSync(join(directory, 'aimlapi-topup.json'))).toBe(false) +}) + +test('reconcileSettledAimlapiTopupStateAsync clears a settled legacy receipt that stores the same credential', async () => { + const directory = useTemporaryConfig() + seedLegacySettledKeyReceipt() + + await reconcileSettledAimlapiTopupStateAsync('key-b') + expect(existsSync(join(directory, 'aimlapi-topup.json'))).toBe(true) + + await reconcileSettledAimlapiTopupStateAsync('key-a') + expect(existsSync(join(directory, 'aimlapi-topup.json'))).toBe(false) +}) + +test('a settled legacy receipt storing a DIFFERENT credential is neither adopted nor replaced', async () => { + const directory = useTemporaryConfig() + seedLegacySettledKeyReceipt() + + await expect( + claimAimlapiTopupStateAsync(byKeyIntent('key-b'), { byKeyApiKey: 'key-b' }), + ).rejects.toThrow(/already succeeded and is waiting to be saved/) + expect(persistedEmail(directory)).toBe(LEGACY_BY_KEY_IDENTITIES['key-a']) +}) + +test('a legacy by-key checkout with no stored key is released to the next claim', async () => { + // Nothing ties it to a credential without re-hashing one, and a by-key + // payment is credited to its key server-side, so refusing would only block + // a re-run that can never match it. + const directory = useTemporaryConfig() + const legacyIntent = legacyByKeyIntent('key-a') + const claimed = claimAimlapiTopupState(legacyIntent) + saveAimlapiTopupState({ + ...legacyIntent, + paymentSessionId: claimed.paymentSessionId, + resumeSessionToken: 'open-session', + }) + const currentIntent = byKeyIntent('key-a') + + const fresh = await claimAimlapiTopupStateAsync(currentIntent, { byKeyApiKey: 'key-a' }) + + expect(fresh.paymentSessionId).not.toBe(claimed.paymentSessionId) + expect(fresh.resumeSessionToken).toBe('') + expect(persistedEmail(directory)).toBe(currentIntent.email) +}) + +test('a settled legacy env-credential receipt (never stores its key) is released to the next claim', async () => { + const directory = useTemporaryConfig() + const legacyIntent = legacyByKeyIntent('env-key-a') + const claimed = claimAimlapiTopupState(legacyIntent) + saveAimlapiTopupState({ + ...legacyIntent, + paymentSessionId: claimed.paymentSessionId, + resumeSessionToken: 'paid-session', + settled: true, + }) + + const fresh = await claimAimlapiTopupStateAsync(byKeyIntent('env-key-b'), { + byKeyApiKey: 'env-key-b', + }) + + expect(fresh.paymentSessionId).not.toBe(claimed.paymentSessionId) + expect(persistedEmail(directory)).toBe(byKeyIntent('env-key-b').email) +}) + +test('a current-identity keyless receipt for another credential is still refused, not released', async () => { + // The release is only for legacy receipts that cannot be attributed; a + // current one can be, so its owner's re-run must still be able to resume it. + useTemporaryConfig() + const envKeyAIntent = byKeyIntent('env-key-a') + const claimed = claimAimlapiTopupState(envKeyAIntent) + saveAimlapiTopupState({ + ...envKeyAIntent, + paymentSessionId: claimed.paymentSessionId, + resumeSessionToken: 'paid-session', + settled: true, + }) + + await expect( + claimAimlapiTopupStateAsync(byKeyIntent('env-key-b'), { byKeyApiKey: 'env-key-b' }), + ).rejects.toThrow(/hasn't finished and may already be paid/) + expect(loadAimlapiTopupState(envKeyAIntent)?.resumeSessionToken).toBe('paid-session') +}) + +test('a legacy receipt is neither adopted nor released while a key-mint lease on it is live', async () => { + const directory = useTemporaryConfig() + const legacyIntent = legacyByKeyIntent('key-a') + const claimed = claimAimlapiTopupState(legacyIntent) + const expected = { ...legacyIntent, paymentSessionId: claimed.paymentSessionId } + // A holder that claimed under the legacy identity has a mint in flight; its + // eventual CAS save must still find the record it claimed. + expect((await acquireAimlapiKeyMintLeaseAsync(expected, 'legacy-owner')).status).toBe('acquired') + + await expect( + claimAimlapiTopupStateAsync(byKeyIntent('key-a'), { byKeyApiKey: 'key-a' }), + ).rejects.toThrow(/minting or exchanging a key right now/) + expect(persistedEmail(directory)).toBe(legacyIntent.email) +}) + test('recordAimlapiSettledKeyAsync persists the key and clears the lease under the CAS', async () => { useTemporaryConfig() const claimed = claimAimlapiTopupState(intent) diff --git a/src/integrations/aimlapi/topupState.ts b/src/integrations/aimlapi/topupState.ts index 028867679..e4d24e32a 100644 --- a/src/integrations/aimlapi/topupState.ts +++ b/src/integrations/aimlapi/topupState.ts @@ -7,7 +7,7 @@ import { writeFileSync, } from 'node:fs' import { dirname, join } from 'node:path' -import { createHash, randomUUID } from 'node:crypto' +import { createHmac, randomUUID } from 'node:crypto' import { logForDebugging } from '../../utils/debug.js' import { getClaudeConfigHomeDir } from '../../utils/envUtils.js' @@ -557,6 +557,8 @@ function toCheckoutState(state: AimlapiPersistedTopup): AimlapiCheckoutState { } } +const BY_KEY_IDENTITY_LABEL = 'aimlapi-by-key-identity-v2' + /** * Stable, non-secret identity for a by-key checkout intent: a saved-profile * or env-sourced credential has no account email to key the intent on (the @@ -567,9 +569,72 @@ function toCheckoutState(state: AimlapiPersistedTopup): AimlapiCheckoutState { * maintained hash computation could silently drift (a different algorithm, * different trimming) and break the match either by never matching a real * receipt or, worse, by colliding across different keys. + * + * An HMAC keyed BY the credential over a fixed label, so the credential is + * never itself the hashed input. Cheap enough for the interactive path (a slow + * KDF here would stall Ink before the async lock is even reached), and an API + * key is already high-entropy, so a work factor would add latency without + * making the fingerprint any harder to reverse. */ export function aimlapiByKeyIdentity(apiKey: string): string { - return `key:${createHash('sha256').update(apiKey.trim()).digest('hex')}` + const fingerprint = createHmac('sha256', apiKey.trim()) + .update(BY_KEY_IDENTITY_LABEL) + .digest('hex') + return `key:v2:${fingerprint}` +} + +/** + * Identity shape written by versions before aimlapiByKeyIdentity moved to + * HMAC: a plain SHA-256 of the key. Deliberately not recomputed (that would + * hash the credential again), so such a receipt can only be tied back to a + * credential through the key stored in it. + */ +const LEGACY_BY_KEY_IDENTITY = /^key:[0-9a-f]{64}$/ + +function isLegacyByKeyReceipt(state: AimlapiPersistedTopup): boolean { + return LEGACY_BY_KEY_IDENTITY.test(state.email) +} + +/** + * A legacy by-key receipt with no stored key cannot be attributed to any + * credential: an in-flight by-key checkout, or a settled top-up of an ambient + * env key (which is never written to disk). It holds nothing that is not + * already recoverable — a by-key payment is credited to its key server-side, + * and an env key is still in the environment — so the next claim may replace + * it instead of refusing a re-run that could never match it. + */ +function isUnattributableLegacyReceipt(state: AimlapiPersistedTopup): boolean { + return isLegacyByKeyReceipt(state) && !state.apiKey?.trim() +} + +/** + * Upgrade a legacy by-key receipt that stores THIS credential to the current + * identity, under the caller's lock, and return the (possibly rewritten) + * record. That covers a settled saved-key top-up waiting on its profile write, + * which claimTopupStateOperation otherwise refuses to replace even with + * abandonExisting. Every CAS after the claim keys on the current identity, so + * the record is rewritten rather than merely matched. Left alone while a + * mint/exchange lease on it is live: that holder's eventual CAS save still + * expects the identity it claimed under. + */ +function adoptLegacyByKeyReceiptUnlocked(apiKey: string): AimlapiPersistedTopup | null { + const current = readAimlapiTopupStateUnlocked() + const trimmedApiKey = apiKey.trim() + if ( + !current || + !trimmedApiKey || + !isLegacyByKeyReceipt(current) || + current.apiKey?.trim() !== trimmedApiKey || + hasLiveMintOrExchangeLease(current) + ) { + return current + } + const adopted: AimlapiPersistedTopup = { + ...current, + email: aimlapiByKeyIdentity(trimmedApiKey), + } + writeAimlapiTopupStateUnlocked(adopted) + return adopted } /** @@ -595,14 +660,16 @@ export function aimlapiByKeyIdentity(apiKey: string): string { * different env credentials' keyless receipts apart, and would let checking * credential B's balance delete credential A's still-unrecovered receipt. * The identity fingerprint is present and stable for a by-key receipt - * regardless of whether its key ended up stored, so it works for both. + * regardless of whether its key ended up stored, so it works for both. A + * legacy receipt that stores this credential is upgraded first so it matches + * too (see adoptLegacyByKeyReceiptUnlocked). */ export function reconcileSettledAimlapiTopupStateAsync(apiKey: string): Promise { const trimmedApiKey = apiKey.trim() if (!trimmedApiKey) return Promise.resolve() const identity = aimlapiByKeyIdentity(trimmedApiKey) return withStateLockAsync(() => { - const current = readAimlapiTopupStateUnlocked() + const current = adoptLegacyByKeyReceiptUnlocked(trimmedApiKey) if (!current?.settled || current.email !== identity) return rmSync(statePath(), { force: true }) }) @@ -866,6 +933,8 @@ function recordCheckoutSessionOperation(state: AimlapiPersistedTopup): AimlapiCh return toCheckoutState(recorded) } +type ClaimTopupStateOptions = { abandonExisting?: boolean; byKeyApiKey?: string } + /** * Adopt the stored checkout for this intent, or start a new one. This is a single * slot, so claiming a DIFFERENT intent would replace the stored record. That is @@ -883,10 +952,14 @@ function recordCheckoutSessionOperation(state: AimlapiPersistedTopup): AimlapiCh * (e.g. a re-edited amount the user chose to submit anyway). The overwrite still * happens under this same lock acquisition, so the caller never observes a * window where the slot is cleared but not yet re-claimed. + * + * `byKeyApiKey` is the credential a by-key intent's identity was derived from. + * With it, a legacy receipt storing that credential is upgraded before + * matching, so re-running that top-up after upgrading still resumes it. */ export function claimAimlapiTopupState( intent: AimlapiTopupIntent, - options: { abandonExisting?: boolean } = {}, + options: ClaimTopupStateOptions = {}, ): AimlapiCheckoutState { return withStateLock(() => claimTopupStateOperation(intent, options)) } @@ -899,16 +972,18 @@ export function claimAimlapiTopupState( */ export function claimAimlapiTopupStateAsync( intent: AimlapiTopupIntent, - options: { abandonExisting?: boolean } = {}, + options: ClaimTopupStateOptions = {}, ): Promise { return withStateLockAsync(() => claimTopupStateOperation(intent, options)) } function claimTopupStateOperation( intent: AimlapiTopupIntent, - options: { abandonExisting?: boolean }, + options: ClaimTopupStateOptions, ): AimlapiCheckoutState { - const existing = readAimlapiTopupStateUnlocked() + const existing = options.byKeyApiKey + ? adoptLegacyByKeyReceiptUnlocked(options.byKeyApiKey) + : readAimlapiTopupStateUnlocked() if (existing && matchesIntent(existing, intent)) { return toCheckoutState(existing) } @@ -947,6 +1022,8 @@ function claimTopupStateOperation( if ( !options.abandonExisting && existing && + // No re-run could ever match it; see isUnattributableLegacyReceipt. + !isUnattributableLegacyReceipt(existing) && (Boolean(existing.resumeSessionToken?.trim()) || existing.settled === true || Boolean(existing.apiKey?.trim())) @@ -967,7 +1044,7 @@ function claimTopupStateOperation( // resetAimlapiCheckoutSession's retain-key pattern. Deliberately // excludes `settled`/`exchange`: those describe the OLD payment // intent's outcome, not this new one's. `email` doubles as the account/ - // key identity (a `key:` id for the by-key flow), so it must match + // key identity (a `key:v2:` id for the by-key flow), so it must match // too — abandonExisting also covers "switch account", where the retained // key belongs to a DIFFERENT identity and carrying it into the new claim // would let a later resume pay account A but save A's key into B's diff --git a/src/utils/skills/skillChangeDetector.test.ts b/src/utils/skills/skillChangeDetector.test.ts index af1dda354..d756696ea 100644 --- a/src/utils/skills/skillChangeDetector.test.ts +++ b/src/utils/skills/skillChangeDetector.test.ts @@ -379,37 +379,59 @@ describe('skillChangeDetector reload batching', () => { stabilityThreshold: 10, pollInterval: 10, chokidarInterval: 10, reloadDebounce: 10, reloadCooldown: 10, }) - const editUntilHook = async (path: string, content: string, count: number) => { + const hookCalls = () => executeConfigChangeHooks.mock.calls.length + // Settle until no hook has fired for a full window, so a late hook from an + // earlier phase cannot be attributed to the next one. + const waitForQuietHooks = async () => { + const deadline = Date.now() + 2000 + let seen = hookCalls() + let quietSince = Date.now() + while (Date.now() - quietSince < 200 && Date.now() < deadline) { + await sleep(25) + if (hookCalls() !== seen) { + seen = hookCalls() + quietSince = Date.now() + } + } + } + const editUntilHook = async (path: string, content: string) => { // Chokidar's ready marks traversal completion, but Bun may still be // initializing its polling baseline. Retry edits until one is observed. + // A retry already in flight when the first hook lands can still produce + // one more, so assert that the edit was seen rather than an exact count. + const before = hookCalls() const deadline = Date.now() + 2000 let edit = 0 - while (executeConfigChangeHooks.mock.calls.length < count && Date.now() < deadline) { + while (hookCalls() === before && Date.now() < deadline) { writeFileSync(path, `${content} ${edit++}`) await sleep(50) } - expect(executeConfigChangeHooks).toHaveBeenCalledTimes(count) + expect(hookCalls()).toBeGreaterThan(before) + await waitForQuietHooks() } try { siblingSkillsHomePaths = [sibling] await detector.initialize() await new Promise(resolve => realWatcher!.once('ready', resolve)) - await editUntilHook(siblingFile, 'enabled edit', 1) + await editUntilHook(siblingFile, 'enabled edit') siblingSkillsHomePaths = [] await detector.reloadAfterSkillSourceSettingsChange() + // Drain anything the sibling watcher emitted before it was closed. + await waitForQuietHooks() + const settled = hookCalls() writeFileSync(siblingFile, 'disabled edit') await sleep(150) - expect(executeConfigChangeHooks).toHaveBeenCalledTimes(1) - await editUntilHook(ownFile, 'owned edit', 2) + expect(hookCalls()).toBe(settled) + await editUntilHook(ownFile, 'owned edit') siblingSkillsHomePaths = [sibling] await detector.reloadAfterSkillSourceSettingsChange() await new Promise(resolve => realWatcher!.once('ready', resolve)) - await editUntilHook(siblingFile, 're-enabled edit', 3) + await editUntilHook(siblingFile, 're-enabled edit') } finally { await detector.dispose() rmSync(root, { recursive: true, force: true }) } - }) + }, 15_000) test('batches rapid reload requests into one hook/cache clear/notification', async () => { const detector = await importFreshModule()