From 45d78c92722155922a87e16a69d7bfe40d5c4eda Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 6 Aug 2026 22:12:50 +0800 Subject: [PATCH] fix(llm-pi-ai): refuse an unusable API key before the header is built --- docs/config-catalog.md | 8 +++- packages/llm/llm-pi-ai/src/config.ts | 23 ++++++++-- packages/llm/llm-pi-ai/src/discovery.ts | 26 ++++++++++- packages/llm/llm-pi-ai/src/index.ts | 4 +- packages/llm/llm-pi-ai/tests/config.spec.ts | 24 ++++++++++ .../llm/llm-pi-ai/tests/discovery.spec.ts | 45 ++++++++++++++++++- 6 files changed, 119 insertions(+), 11 deletions(-) create mode 100644 packages/llm/llm-pi-ai/tests/config.spec.ts diff --git a/docs/config-catalog.md b/docs/config-catalog.md index a3e63d994d..6c860cbd9d 100644 --- a/docs/config-catalog.md +++ b/docs/config-catalog.md @@ -704,7 +704,11 @@ export interface Config { /** Configuration for one pi-ai provider route; the `providers` dict key IS the route. */ export interface PiAiProviderProfile { - /** Literal provider credential; prefer {@link apiKeyEnv}. With both absent pi-ai uses its provider-native ambient discovery. */ + /** + * Literal provider credential; prefer {@link apiKeyEnv}. With both absent pi-ai uses its + * provider-native ambient discovery. Trimmed and format-checked by {@link resolveProfiles}; a + * value no HTTP header can carry fails there rather than inside `fetch`. + */ apiKey?: string /** Credential reference (environment-variable name) resolved per request through `ctx.credentials`. */ apiKeyEnv?: string @@ -776,7 +780,7 @@ export interface PiAiModelProfile { Depends on: `CacheRetention` (`@earendil-works/pi-ai`) · `ModelThinkingLevel` (`@earendil-works/pi-ai`) · [`RetryPolicyConfig`](../packages/llm/llm/src/index.ts) · `ThinkingBudgets` (`@earendil-works/pi-ai`) · `Transport` (`@earendil-works/pi-ai`) -Source: [`packages/llm/llm-pi-ai/src/config.ts:122`](../packages/llm/llm-pi-ai/src/config.ts) +Source: [`packages/llm/llm-pi-ai/src/config.ts:126`](../packages/llm/llm-pi-ai/src/config.ts) ## `@deepseek-ai/dsh-llm-replay` diff --git a/packages/llm/llm-pi-ai/src/config.ts b/packages/llm/llm-pi-ai/src/config.ts index 7473dbb7ae..7e8374ab9f 100644 --- a/packages/llm/llm-pi-ai/src/config.ts +++ b/packages/llm/llm-pi-ai/src/config.ts @@ -19,7 +19,7 @@ import z from 'schemastery' import { credentialRef } from '@deepseek-ai/dsh-credentials' import type { CredentialRef } from '@deepseek-ai/dsh-credentials' import { MAX_TIMER_DELAY_MS } from '@deepseek-ai/dsh-timeout' -import { resolveRetryPolicy, RetryPolicySchema } from '@deepseek-ai/dsh-llm' +import { normalizeApiKey, resolveRetryPolicy, RetryPolicySchema } from '@deepseek-ai/dsh-llm' import type { ResolvedRetryPolicy, RetryPolicyConfig } from '@deepseek-ai/dsh-llm' import { resolveRouteModels } from './catalog.ts' import type { PiAiModelProfile } from './catalog.ts' @@ -38,7 +38,11 @@ export type { PiAiModelProfile } from './catalog.ts' /** Configuration for one pi-ai provider route; the `providers` dict key IS the route. */ export interface PiAiProviderProfile { - /** Literal provider credential; prefer {@link apiKeyEnv}. With both absent pi-ai uses its provider-native ambient discovery. */ + /** + * Literal provider credential; prefer {@link apiKeyEnv}. With both absent pi-ai uses its + * provider-native ambient discovery. Trimmed and format-checked by {@link resolveProfiles}; a + * value no HTTP header can carry fails there rather than inside `fetch`. + */ apiKey?: string /** Credential reference (environment-variable name) resolved per request through `ctx.credentials`. */ apiKeyEnv?: string @@ -220,8 +224,18 @@ export function resolveProfiles( for (const [provider, source] of entries) { rejectRemovedFields(provider, source) if (provider.length === 0) throw new Error('llm-pi-ai: provider names must be non-empty') - if (source.apiKey !== undefined && source.apiKey.trim().length === 0) { - throw new Error(`llm-pi-ai: provider "${provider}" has an empty apiKey; omit it to use ambient authentication`) + // Omission selects the installed provider's own auth — ambient discovery + // or OAuth — so only a supplied key is judged. + let apiKey: string | undefined + if (source.apiKey !== undefined) { + const checked = normalizeApiKey(source.apiKey) + if (!checked.ok) { + throw new Error(checked.reason === 'empty' + ? `llm-pi-ai: provider "${provider}" has an empty apiKey; omit it to use ambient authentication` + : `llm-pi-ai: provider "${provider}" has an apiKey containing characters no HTTP header can carry;` + + ' paste the raw key only') + } + apiKey = checked.value } if (source.baseURL !== undefined && source.baseURL.length === 0) { throw new Error(`llm-pi-ai: provider "${provider}" has an empty baseURL`) @@ -252,6 +266,7 @@ export function resolveProfiles( const { apiKeyEnv, retryPolicy, models: _models, displayName: _displayName, ...rest } = source resolved.set(provider, { ...rest, + ...apiKey === undefined ? {} : { apiKey }, provider, displayName, ...apiKeyEnv === undefined ? {} : { apiKeyEnv: credentialRef(apiKeyEnv) }, diff --git a/packages/llm/llm-pi-ai/src/discovery.ts b/packages/llm/llm-pi-ai/src/discovery.ts index bff2c9a7ca..014c9c2f3e 100644 --- a/packages/llm/llm-pi-ai/src/discovery.ts +++ b/packages/llm/llm-pi-ai/src/discovery.ts @@ -22,7 +22,7 @@ * @module dsh-llm-pi-ai/discovery */ -import { LlmError } from '@deepseek-ai/dsh-llm' +import { INVALID_CREDENTIAL_CODE, LlmError, normalizeApiKey } from '@deepseek-ai/dsh-llm' import type { LlmDiscoveredModel, LlmModelDiscoveryRequest } from '@deepseek-ai/dsh-llm' import { attributionHeaders } from '@deepseek-ai/dsh-llm' import { catalogModels } from './catalog.ts' @@ -161,6 +161,25 @@ function readListing(body: unknown): LlmDiscoveredModel[] { return models } +/** + * Accept one probe key, or refuse it before the header is built. Without this + * the `fetch` below would throw a ByteString `TypeError` that this function's + * catch reports as `could not reach ` — blaming the network for a local, + * deterministic fault. + * @param raw - the key typed into the form or read from storage. + * @returns the trimmed, usable key. + */ +function usableProbeKey(raw: string): string { + const checked = normalizeApiKey(raw) + if (checked.ok) return checked.value + throw new LlmError( + checked.reason === 'empty' + ? 'this provider\'s API key is blank; enter it on the Models page, or clear it to probe unauthenticated' + : 'this provider\'s API key contains characters no HTTP header can carry; paste the raw key only', + INVALID_CREDENTIAL_CODE, + ) +} + /** * Interrogate one draft provider endpoint for the models it advertises. * @param request - the endpoint, protocol, and one-shot credential to use. @@ -216,7 +235,10 @@ export async function discoverModels( // stored one is only asked for here, past the catalog short-circuit and the // protocol check, so a route answered from the registry costs no credential // lookup — and no diagnostic about a credential it never needed. - const apiKey = request.apiKey ?? await storedApiKey?.() + // A probe carrying no key stays unauthenticated, which is how a route that + // relies on the provider's own ambient discovery is meant to be asked. + const supplied = request.apiKey ?? await storedApiKey?.() + const apiKey = supplied === undefined ? undefined : usableProbeKey(supplied) let response: Response try { response = await fetch(url, { diff --git a/packages/llm/llm-pi-ai/src/index.ts b/packages/llm/llm-pi-ai/src/index.ts index 0d058e94ac..c30fd3db6f 100644 --- a/packages/llm/llm-pi-ai/src/index.ts +++ b/packages/llm/llm-pi-ai/src/index.ts @@ -43,7 +43,7 @@ */ import type { Context } from 'cordis' -import { LlmError } from '@deepseek-ai/dsh-llm' +import { assertUsableApiKey, LlmError } from '@deepseek-ai/dsh-llm' import type { AdapterRegistrationHandle, DirectoryRegistrationHandle, LlmConfigurableProvider } from '@deepseek-ai/dsh-llm' import { deepEqualJson, installSettingsSection, settingsNamespace } from '@deepseek-ai/dsh-settings' import { PiAiAdapter } from './adapter.ts' @@ -145,7 +145,7 @@ export function apply(ctx: Context, config: Config): void { // Without the seam, read exactly the named variable so a plain // cordis.yml composition works from the environment alone. : process.env[ref] - if (hit !== undefined && hit.length > 0) return hit + if (hit !== undefined && hit.length > 0) return assertUsableApiKey(hit, 'llm-pi-ai', ref) throw new LlmError( `llm-pi-ai: no credential for provider route "${provider}"; its profile resolves ${ref}, which is not` + ` set — store ${ref} through the credentials service (the web Models page writes it) or export it,` diff --git a/packages/llm/llm-pi-ai/tests/config.spec.ts b/packages/llm/llm-pi-ai/tests/config.spec.ts new file mode 100644 index 0000000000..90f8487ad8 --- /dev/null +++ b/packages/llm/llm-pi-ai/tests/config.spec.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest' +import { resolveProfiles } from '../src/config.ts' + +describe('API key format', () => { + it('trims a padded literal apiKey into the resolved profile', () => { + const resolved = resolveProfiles({ openai: { apiKey: ' sk-abc ', baseURL: 'https://acme.test' } }) + expect(resolved.get('openai')?.apiKey).toBe('sk-abc') + }) + + it('keeps an omitted apiKey absent so ambient authentication still applies', () => { + const resolved = resolveProfiles({ openai: { baseURL: 'https://acme.test' } }) + expect(resolved.get('openai')?.apiKey).toBeUndefined() + }) + + it('still tells an empty apiKey to omit itself', () => { + expect(() => resolveProfiles({ openai: { apiKey: ' ', baseURL: 'https://acme.test' } })) + .toThrow(/omit it to use ambient authentication/) + }) + + it('rejects an apiKey no header can carry', () => { + expect(() => resolveProfiles({ openai: { apiKey: 'sk-\u{1F600}', baseURL: 'https://acme.test' } })) + .toThrow(/no HTTP header can carry/) + }) +}) diff --git a/packages/llm/llm-pi-ai/tests/discovery.spec.ts b/packages/llm/llm-pi-ai/tests/discovery.spec.ts index 916700fbbf..63b43ecdab 100644 --- a/packages/llm/llm-pi-ai/tests/discovery.spec.ts +++ b/packages/llm/llm-pi-ai/tests/discovery.spec.ts @@ -1,6 +1,6 @@ import { createServer } from 'node:http' import type { IncomingMessage, Server, ServerResponse } from 'node:http' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { Context } from 'cordis' import LlmService, { userAgent } from '@deepseek-ai/dsh-llm' import * as LlmPiAi from '@deepseek-ai/dsh-llm-pi-ai' @@ -12,6 +12,9 @@ const servers: Server[] = [] const touchedEnv: string[] = [] afterEach(async () => { + // A no-op when the test never stubbed `fetch`; only 'probe key format' + // below installs one. + vi.unstubAllGlobals() for (const name of touchedEnv.splice(0)) Reflect.deleteProperty(process.env, name) await Promise.all(servers.splice(0).map(server => new Promise(resolve => server.close(resolve)))) }) @@ -311,3 +314,43 @@ describe('draft-provider model discovery', () => { .rejects.toMatchObject({ code: 'NO_DISCOVERY' }) }) }) + +describe('probe key format', () => { + it('reports an illegal probe key as a credential fault, not an unreachable endpoint', async () => { + await expect(discoverModels({ + baseURL: 'https://acme.test', + api: 'openai-completions', + apiKey: 'sk-\u{1F600}', + })).rejects.toMatchObject({ code: 'INVALID_CREDENTIAL' }) + }) + + it('reports a blank probe key as a credential fault too', async () => { + // A cleared form field arrives as '', not an absent key; it must fail the + // same way a typed-in illegal key does, rather than probing unauthenticated. + await expect(discoverModels({ + baseURL: 'https://acme.test', + api: 'openai-completions', + apiKey: '', + })).rejects.toMatchObject({ code: 'INVALID_CREDENTIAL' }) + }) + + it('leaves a probe with no key unauthenticated', async () => { + // The file's other cases capture headers through a real local HTTP server + // (`listingServer`); this one has no route or stored key to resolve, so + // the smallest real double is a `fetch` stub, scoped to this test and + // unstubbed by the shared `afterEach` above. + const requests: RequestInit[] = [] + vi.stubGlobal('fetch', async (_url: string | URL, init?: RequestInit) => { + requests.push(init ?? {}) + return new Response(JSON.stringify({ data: [] }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }) + }) + + await discoverModels({ baseURL: 'https://acme.test', api: 'openai-completions' }) + + const headers = new Headers(requests[0]?.headers) + expect(headers.has('authorization')).toBe(false) + }) +})