fix(aimlapi): derive the by-key identity without hashing the API key (#2293)

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:<sha256>`. 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 <openclaude@gitlawb.com>
This commit is contained in:
Kevin Codex
2026-10-19 13:17:55 +08:00
committed by GitHub
co-authored by OpenClaude
parent 933019b6a9
commit 8c46032db2
5 changed files with 282 additions and 18 deletions
+13 -1
View File
@@ -389,6 +389,8 @@ function mockProviderManagerDependencies(
completeAimlapiCodeSignIn?: (...args: any[]) => Promise<unknown>
validateAimlapiApiKey?: (...args: any[]) => Promise<unknown>
claimAimlapiTopupStateAsync?: (...args: any[]) => unknown
// Observes the default (stateful) claim's arguments without replacing it.
onAimlapiTopupClaim?: (intent: Record<string, unknown>, 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<string, unknown>) => {
(async (intent: Record<string, unknown>, 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<string, unknown>, _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(
+3
View File
@@ -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
+150
View File
@@ -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)
+86 -9
View File
@@ -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<void> {
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<AimlapiCheckoutState> {
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:<hash>` id for the by-key flow), so it must match
// key identity (a `key:v2:<hmac>` 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
+30 -8
View File
@@ -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<void>(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<void>(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()