From 99304f7e5920d895e72965c73e25f760cdf82548 Mon Sep 17 00:00:00 2001 From: Michael Gasperini Date: Sun, 1 Nov 2026 19:47:59 +0100 Subject: [PATCH 1/2] fix: address post-merge review findings Native registry: lock the temporary directory down before the file exists, abort when the restriction fails, clean up on SIGINT/SIGTERM/SIGHUP, report cleanup failures, and sweep stale registries from hard kills. Dry runs no longer write keys to disk. Legacy mode accepts a provider with only apiKeyCommand and reports it as legacy-only in --list-providers and doctor. The router steps down only when a 400 body mentions the reasoning effort. Doctor sanitizes the version probe environment, reports discovery failures and distinguishes keyed from authless providers. Redact COPILOT_PROVIDER_HEADERS in dry-run output and validate headers as a string map. --- schemas/providers.schema.json | 6 +- src/catalog.mjs | 4 +- src/cli.mjs | 97 ++++++++++++++++++++++++-------- src/config.mjs | 22 +++++--- src/doctor.mjs | 68 +++++++++++++++++----- src/keystore.mjs | 14 +++-- src/registry-file.mjs | 68 +++++++++++++++++++--- src/router.mjs | 80 ++++++++++++++++++-------- test/cli.test.mjs | 103 ++++++++++++++++++++++++++++++++++ test/config.test.mjs | 17 ++++++ test/doctor.test.mjs | 85 ++++++++++++++++++++++++---- test/models-command.test.mjs | 45 +++++++++++++++ test/registry-file.test.mjs | 38 ++++++++++++- test/router.test.mjs | 48 +++++++++++++++- 14 files changed, 595 insertions(+), 100 deletions(-) diff --git a/schemas/providers.schema.json b/schemas/providers.schema.json index 2f48300..5cad3b5 100644 --- a/schemas/providers.schema.json +++ b/schemas/providers.schema.json @@ -280,7 +280,7 @@ "xhigh", "max" ], - "description": "Reasoning effort sent to the provider. Copilot does not forward its own setting to BYOK models, so this is what actually takes effect. Levels differ per model; an unsupported one is stepped down automatically." + "description": "Reasoning effort the router applies when the request carries none. Current Copilot CLIs forward the level picked in the selector on their own; this value remains useful on older CLIs or to pin a model to a fixed level. Ignored by --native-byok, which sends the CLI's level directly. Levels differ per model; an unsupported one is stepped down automatically in router mode." }, "family": { "type": "string", @@ -288,7 +288,7 @@ }, "reasoningEffortPicker": { "type": "boolean", - "description": "Declare reasoning-effort support so Copilot shows its effort selector. Off by default: on CLI 1.0.79 the chosen level never reaches a BYOK provider (github/copilot-cli#4012). Turn it on once the CLI forwards it again." + "description": "Declares reasoning-effort support so Copilot shows its effort selector. On by default for models that report reasoning, since current CLIs forward the level to BYOK providers. Set it to false to hide the selector; on CLI 1.0.79 the selector was inert." }, "reasoningEffortLevels": { "type": "array", @@ -305,7 +305,7 @@ "max" ] }, - "description": "Levels offered in the selector, when it is enabled. Defaults to low/medium/high; models differ, so override where needed." + "description": "Levels offered in the selector. Defaults to low/medium/high; models differ, so override where needed. In --native-byok this is the only lever, because the router's step-down does not apply." } }, "additionalProperties": true diff --git a/src/catalog.mjs b/src/catalog.mjs index 9e33459..408609b 100644 --- a/src/catalog.mjs +++ b/src/catalog.mjs @@ -98,8 +98,8 @@ function buildEntry({ id, provider, model }) { // Declared for reasoning models by default: current CLIs forward the // level to BYOK providers, so the selector is meaningful. Older CLIs // accepted the choice without sending it (github/copilot-cli#4012, - // fixed in 1.0.91); there the selector is inert, and - // `reasoningEffortPicker: false` hides it. + // reported resolved in the 1.0.9x line); there the selector is inert, + // and `reasoningEffortPicker: false` hides it. ...(model.reasoning && model.reasoningEffortPicker !== false ? { reasoning_effort: model.reasoningEffortLevels || DEFAULT_EFFORT_LEVELS } : {}), diff --git a/src/cli.mjs b/src/cli.mjs index b8d6d3f..e2bb8b3 100644 --- a/src/cli.mjs +++ b/src/cli.mjs @@ -20,7 +20,7 @@ import { runKeysCommand } from './keys-command.mjs'; import { runDoctorCommand } from './doctor.mjs'; import { runModelsCommand } from './models-command.mjs'; import { buildNativeRegistry, nativeSelectionId } from './native-registry.mjs'; -import { createRegistryFile, inspectNativeRegistry } from './registry-file.mjs'; +import { createRegistryFile, inspectNativeRegistry, sweepStaleRegistries } from './registry-file.mjs'; import { loadKeys } from './keystore.mjs'; import { runExtensionCommand } from './extension-install.mjs'; import { runShimCommand } from './shim-install.mjs'; @@ -192,43 +192,34 @@ async function listProviders({ config, io }) { for (const provider of config.providers) { const usable = isQueryable(provider); const known = provider.models?.length || recallModels(diskCache, provider.id, Date.now)?.length || 0; - const keys = (provider.apiKeyEnv || []).join(' | ') || (provider.authRequired === false ? 'no key needed' : '-'); + const keys = (provider.apiKeyEnv || []).join(' | ') + || (provider.authRequired === false + ? 'no key needed' + : provider.apiKeyCommand ? 'apiKeyCommand (--legacy only)' : '-'); io.stdout.write(`${usable ? '*' : ' '} ${provider.id.padEnd(20)} ${String(known).padStart(2)} models ${keys}\n`); } io.stdout.write('\n* = key configured; live model lists load at startup. Counts show pinned or remembered lists.\n'); + io.stdout.write('apiKeyCommand providers are usable only with --legacy.\n'); return 0; } /** Native BYOK: the CLI reads a generated providers.json and talks to the providers itself. */ async function runNativeByokMode({ args, config, io }) { const sessionLog = createSessionLog(io); + // Best effort for sessions that could not clean up (SIGKILL, power loss). + await sweepStaleRegistries().catch(() => {}); const providers = args.noDiscovery ? await resolveOfflineModels(config.providers, { env: io.env }) : await discoverWithProgress({ config, io, sessionLog }); const active = selectActiveProviders(providers); - - if (active.length === 0) { - io.stderr.write( - 'No BYOK provider configured: starting Copilot with GitHub models only.\n' + - 'Run "copilot-byok --list-providers" to see which environment variables to set.\n\n' - ); - return runCopilot({ copilotArgs: nativeCopilotArgs(args, []), env: {}, io, dryRun: args.dryRun, native: true, config }); - } - - const registry = buildNativeRegistry(active); - const registryFile = await createRegistryFile(registry); - // A hard kill still leaves the file behind if only `finally` cleaned up. - const cleanupOnExit = () => rmSync(registryFile.dir, { recursive: true, force: true }); - process.once('exit', cleanupOnExit); - + const registry = active.length > 0 ? buildNativeRegistry(active) : { providers: [], models: [] }; const selectionIds = registry.models.map((model) => nativeSelectionId(model.provider, model.id)); const copilotArgs = nativeCopilotArgs(args, active); + // A dry run must not write provider keys to disk, not even temporarily. if (args.dryRun) { - process.removeListener('exit', cleanupOnExit); - await registryFile.cleanup(); io.stdout.write(`${JSON.stringify({ mode: 'native-byok', providers: active.map((provider) => provider.id), @@ -238,6 +229,32 @@ async function runNativeByokMode({ args, config, io }) { return 0; } + if (active.length === 0) { + io.stderr.write( + 'No provider configured by copilot-byok: starting Copilot without a generated registry.\n' + + 'Run "copilot-byok --list-providers" to see which environment variables to set.\n\n' + ); + return runCopilot({ copilotArgs, env: {}, io, native: true, config }); + } + + const registryFile = await createRegistryFile(registry); + const reportCleanupFailure = () => { + io.stderr.write(`copilot-byok: could not remove ${registryFile.path}; delete it manually.\n`); + }; + const cleanupSync = () => { + try { + rmSync(registryFile.dir, { recursive: true, force: true }); + return true; + } catch { + return false; + } + }; + const cleanupAndReport = () => { + if (!cleanupSync()) reportCleanupFailure(); + }; + process.once('exit', cleanupAndReport); + const removeSignalHandlers = installSignalCleanup(cleanupAndReport); + io.stderr.write( `copilot-byok: added ${registry.models.length} models from ${active.length} providers through the native BYOK registry\n` ); @@ -250,12 +267,42 @@ async function runNativeByokMode({ args, config, io }) { config, }); } finally { - process.removeListener('exit', cleanupOnExit); - await registryFile.cleanup(); + removeSignalHandlers(); + process.removeListener('exit', cleanupAndReport); + const cleaned = await registryFile.cleanup(); + if (!cleaned) reportCleanupFailure(); sessionLog.summarize(); } } +/** Removes the temporary registry, then lets the signal terminate the process. */ +function installSignalCleanup(cleanup) { + const handlers = []; + + for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { + const handler = () => { + cleanup(); + try { + process.removeListener(signal, handler); + process.kill(process.pid, signal); + } catch { + process.exit(1); + } + }; + + try { + process.once(signal, handler); + handlers.push([signal, handler]); + } catch { + // Signal not supported on this platform. + } + } + + return () => { + for (const [signal, handler] of handlers) process.removeListener(signal, handler); + }; +} + /** * Builds the Copilot arguments for native mode, translating a picker id from * this project (`byok-...`) into the `provider/model` id the CLI expects. @@ -307,8 +354,8 @@ async function runLegacyMode({ args, io }) { return runCopilot({ copilotArgs, env: {}, io, dryRun: args.dryRun, native: true, config }); } - if (!args.listModels && provider.authRequired !== false && !provider.apiKey && !provider.bearerToken) { - throw new Error(`Missing API key for ${provider.name}. Configure an environment variable listed in the provider's apiKeyEnv or bearerTokenEnv setting.`); + if (!args.listModels && provider.authRequired !== false && !provider.apiKey && !provider.bearerToken && !provider.apiKeyCommand) { + throw new Error(`Missing API key for ${provider.name}. Configure an environment variable listed in the provider's apiKeyEnv or bearerTokenEnv setting, or an apiKeyCommand.`); } if (args.wireApi && provider.type === 'anthropic') { @@ -556,7 +603,9 @@ function asArray(value) { function redactEnv(env) { return Object.fromEntries(Object.entries(env).map(([key, value]) => [ key, - /KEY|TOKEN|SECRET|PASSWORD/i.test(key) ? '' : value, + // Header values can carry a gateway signature even when the header name + // looks harmless; the value never belongs in dry-run output. + key === 'COPILOT_PROVIDER_HEADERS' || /KEY|TOKEN|SECRET|PASSWORD/i.test(key) ? '' : value, ])); } diff --git a/src/config.mjs b/src/config.mjs index 1123626..6c532ad 100644 --- a/src/config.mjs +++ b/src/config.mjs @@ -240,14 +240,22 @@ function validateProviderOptions(provider, name, type) { } if (provider.modelsHeaders != null) { - if (!isRecord(provider.modelsHeaders)) { - throw new Error(`Provider ${name} modelsHeaders must be an object`); - } + validateHeaders(provider.modelsHeaders, name, 'modelsHeaders'); + } - for (const [headerName, value] of Object.entries(provider.modelsHeaders)) { - if (!headerName.trim() || typeof value !== 'string') { - throw new Error(`Provider ${name} modelsHeaders must contain string header values`); - } + if (provider.headers != null) { + validateHeaders(provider.headers, name, 'headers'); + } +} + +function validateHeaders(value, providerName, field) { + if (!isRecord(value)) { + throw new Error(`Provider ${providerName} ${field} must be an object`); + } + + for (const [headerName, headerValue] of Object.entries(value)) { + if (!headerName.trim() || typeof headerValue !== 'string') { + throw new Error(`Provider ${providerName} ${field} must contain string header values`); } } } diff --git a/src/doctor.mjs b/src/doctor.mjs index a90a617..bd3c7b3 100644 --- a/src/doctor.mjs +++ b/src/doctor.mjs @@ -17,6 +17,7 @@ import { isQueryable, resolveLiveModels } from './launcher.mjs'; import { cachePath, readCache } from './model-cache.mjs'; import { nativeRegistryPath, inspectNativeRegistry } from './registry-file.mjs'; import { keystorePath, loadKeys } from './keystore.mjs'; +import { sanitizeCopilotEnvironment } from './process-env.mjs'; import { logPath } from './session-log.mjs'; import { shimDir } from './shim-install.mjs'; import { dataResidencyOrigin } from './upstream.mjs'; @@ -62,7 +63,12 @@ export function detectCopilotVersion(bin, env) { }); } -export async function runDoctorCommand({ argv = [], io, probeVersion = detectCopilotVersion }) { +export async function runDoctorCommand({ + argv = [], + io, + probeVersion = detectCopilotVersion, + discover = resolveLiveModels, +}) { const args = { json: false, online: false, configPath: null, help: false }; const rest = [...argv]; args.configPath = extractConfigPath(rest); @@ -83,14 +89,15 @@ export async function runDoctorCommand({ argv = [], io, probeVersion = detectCop const add = (level, name, detail) => checks.push({ level, name, detail }); await checkNode(add); - await checkCopilot({ add, io, probeVersion }); const { config, secrets } = await checkConfig({ add, io, configPath: args.configPath }); if (config) { checkProviders({ add, config }); - if (args.online) await checkOnline({ add, config, io }); + if (args.online) await checkOnline({ add, config, io, discover }); } + await checkCopilot({ add, io, probeVersion, config }); + checkKeystore({ add, secrets, env: io.env }); await checkNativeRegistry({ add, env: io.env }); await checkPaths({ add, env: io.env }); @@ -124,9 +131,12 @@ async function checkNode(add) { add('ok', 'node', minimum ? `${current} (minimum ${minimum})` : current); } -async function checkCopilot({ add, io, probeVersion }) { +async function checkCopilot({ add, io, probeVersion, config }) { const bin = resolveCopilotBin({ env: io.env }); - const version = await probeVersion(bin, io.env); + // The version probe is still a Copilot process: it must not inherit provider + // keys or stale COPILOT_PROVIDER_* variables. + const env = sanitizeCopilotEnvironment(io.env, {}, providerSecretNames(config)); + const version = await probeVersion(bin, env); if (!version) { add('warn', 'copilot', `${bin} did not answer --version; set COPILOT_BIN if the path is wrong`); return; @@ -159,25 +169,46 @@ async function checkConfig({ add, io, configPath }) { } function checkProviders({ add, config }) { - const active = config.providers.filter(isQueryable); + const keyed = config.providers.filter((provider) => provider.apiKey || provider.bearerToken); + const authless = config.providers.filter((provider) => provider.authRequired === false); + const commandOnly = config.providers.filter((provider) => + provider.authRequired !== false && !provider.apiKey && !provider.bearerToken && provider.apiKeyCommand); const missing = config.providers - .filter((provider) => provider.authRequired !== false && !provider.apiKey && !provider.bearerToken) + .filter((provider) => provider.authRequired !== false && !provider.apiKey && !provider.bearerToken && !provider.apiKeyCommand) .map((provider) => provider.id); - add(active.length > 0 ? 'ok' : 'warn', 'providers', `${active.length} of ${config.providers.length} have a credential`); - if (missing.length > 0 && active.length === 0) { + add(keyed.length + authless.length > 0 ? 'ok' : 'warn', 'providers', `${keyed.length} keyed, ${authless.length} need no key`); + if (commandOnly.length > 0) { + add('warn', 'apiKeyCommand', `${commandOnly.map((provider) => provider.id).join(', ')} work only with --legacy`); + } + if (missing.length > 0 && keyed.length === 0 && authless.length === 0) { add('warn', 'credentials', `set a key for one of: ${missing.slice(0, 6).join(', ')}${missing.length > 6 ? ', ...' : ''}`); } } -async function checkOnline({ add, config, io }) { +async function checkOnline({ add, config, io, discover }) { + const events = []; + let providers; try { - const providers = await resolveLiveModels(config.providers, { env: io.env }); - const withModels = providers.filter((provider) => provider.models?.length); - add('ok', 'discovery', `${withModels.length} providers returned models`); + providers = await discover(config.providers, { env: io.env, onEvent: (event) => events.push(event) }); } catch (error) { add('warn', 'discovery', error.message); + return; + } + + const queryable = config.providers.filter(isQueryable); + const withModels = providers.filter((provider) => provider.models?.length); + const errors = events + .filter((event) => event.type === 'error') + .map((event) => (event.provider ? `${event.provider}: ${event.message}` : event.message)) + .slice(0, 3); + + if (queryable.length > 0 && withModels.length === 0) { + add('warn', 'discovery', `no models from ${queryable.length} configured provider(s)${errors.length > 0 ? `; ${errors.join('; ')}` : ''}`); + return; } + + add('ok', 'discovery', `${withModels.length} providers returned models${errors.length > 0 ? `, ${errors.length} failed` : ''}`); } function checkKeystore({ add, secrets, env }) { @@ -255,6 +286,17 @@ function versionAtLeast(current, minimum) { return c >= z; } +function providerSecretNames(config) { + return (config?.providers || []).flatMap((provider) => [ + ...asArray(provider.apiKeyEnv), + ...asArray(provider.bearerTokenEnv), + ]); +} + +function asArray(value) { + return Array.isArray(value) ? value : value ? [value] : []; +} + function doctorHelpText() { return `copilot-byok doctor - report the state of the installation diff --git a/src/keystore.mjs b/src/keystore.mjs index 00e3a38..377a577 100644 --- a/src/keystore.mjs +++ b/src/keystore.mjs @@ -86,14 +86,15 @@ async function writeKeys(keys, env) { } /** - * Owner-only access. POSIX gets 0600; Windows needs an explicit ACL because file - * modes are advisory there and inherited permissions would otherwise apply. + * Owner-only access. POSIX gets 0600 by default; Windows needs an explicit ACL + * because file modes are advisory there and inherited permissions would + * otherwise apply. Callers can pass 0700 for a directory. * @returns {Promise} false when the restriction could not be applied. */ -export async function restrictPermissions(path) { +export async function restrictPermissions(path, { mode = 0o600 } = {}) { if (platform() !== 'win32') { try { - await chmod(path, 0o600); + await chmod(path, mode); return true; } catch { return false; @@ -106,7 +107,10 @@ export async function restrictPermissions(path) { if (!user) return false; return new Promise((resolve) => { - const child = spawn('icacls', [path, '/inheritance:r', '/grant:r', `${user}:F`], { + // Directories get inheritable permissions, so a file created inside them + // is owner-only from the first byte rather than after a second icacls run. + const grant = mode === 0o700 ? `${user}:(OI)(CI)F` : `${user}:F`; + const child = spawn('icacls', [path, '/inheritance:r', '/grant:r', grant], { stdio: 'ignore', shell: false, }); diff --git a/src/registry-file.mjs b/src/registry-file.mjs index 8f152be..c8a7083 100644 --- a/src/registry-file.mjs +++ b/src/registry-file.mjs @@ -4,13 +4,21 @@ // exit. Legacy mode uses the same writer to hand the CLI an empty registry, so // a providers.json in the user's config directory cannot silently override the // COPILOT_PROVIDER_* variables that mode is built on. +// +// The directory is locked down before the file exists: on Windows the file mode +// is advisory, and a registry holding provider keys must not rely on the ACL of +// a shared temp directory. If the owner-only restriction cannot be applied the +// writer refuses to keep the keys on disk. -import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'; import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import { restrictPermissions } from './keystore.mjs'; +const REGISTRY_DIR_PREFIX = 'copilot-byok-registry-'; +const STALE_REGISTRY_AGE_MS = 7 * 24 * 60 * 60 * 1000; + /** Where the CLI looks for the registry, honouring its own overrides. */ export function nativeRegistryPath(env = process.env) { if (env.COPILOT_PROVIDERS_CONFIG) return env.COPILOT_PROVIDERS_CONFIG; @@ -46,21 +54,65 @@ export async function inspectNativeRegistry(env = process.env) { } /** - * Writes a registry to a private temporary directory. - * @returns {Promise<{path: string, dir: string, cleanup: function(): Promise}>} + * Writes a registry to a private temporary directory. The directory is + * restricted before the file is created; a false from the restriction helper is + * a hard failure, because provider keys must not end up in a shared temp path. + * @returns {Promise<{path: string, dir: string, cleanup: function(): Promise}>} */ -export async function createRegistryFile(registry) { - const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-')); +export async function createRegistryFile(registry, { restrict = restrictPermissions, base = tmpdir() } = {}) { + const dir = await mkdtemp(join(base, REGISTRY_DIR_PREFIX)); const path = join(dir, 'providers.json'); - await writeFile(path, `${JSON.stringify(registry, null, 2)}\n`, { encoding: 'utf8', mode: 0o600 }); - await restrictPermissions(path); + try { + if (!await restrict(dir, { mode: 0o700 })) { + throw new Error(`could not restrict permissions on ${dir}; refusing to write provider keys there`); + } + + await writeFile(path, `${JSON.stringify(registry, null, 2)}\n`, { encoding: 'utf8', mode: 0o600 }); + + if (!await restrict(path)) { + throw new Error(`could not restrict permissions on ${path}; refusing to keep provider keys there`); + } + } catch (error) { + await rm(dir, { recursive: true, force: true }).catch(() => {}); + throw error; + } return { path, dir, async cleanup() { - await rm(dir, { recursive: true, force: true }).catch(() => {}); + try { + await rm(dir, { recursive: true, force: true }); + return true; + } catch { + return false; + } }, }; } + +/** + * Removes registries left behind by a session that could not clean up (SIGKILL, + * power loss). Only directories created by this module older than a week are + * touched, so a concurrent session is never removed. + * @returns {Promise} directories removed + */ +export async function sweepStaleRegistries({ + base = tmpdir(), + now = Date.now(), + maxAgeMs = STALE_REGISTRY_AGE_MS, +} = {}) { + let removed = 0; + const entries = await readdir(base, { withFileTypes: true }).catch(() => []); + + for (const entry of entries) { + if (!entry.isDirectory() || !entry.name.startsWith(REGISTRY_DIR_PREFIX)) continue; + const path = join(base, entry.name); + const info = await stat(path).catch(() => null); + if (!info || now - info.mtimeMs < maxAgeMs) continue; + if (await rm(path, { recursive: true, force: true }).then(() => true, () => false)) removed += 1; + } + + return removed; +} diff --git a/src/router.mjs b/src/router.mjs index 5a3f4f7..544283a 100644 --- a/src/router.mjs +++ b/src/router.mjs @@ -26,6 +26,10 @@ const MAX_BODY_BYTES = 64 * 1024 * 1024; // the turn indefinitely. Generous, because reasoning models legitimately take minutes. const PROVIDER_IDLE_TIMEOUT_MS = Number(process.env.COPILOT_BYOK_PROVIDER_TIMEOUT_MS) || 600_000; +// Error bodies are small. They are read only to tell an effort rejection from +// any other 400, and are forwarded unchanged when the ladder does not apply. +const MAX_ERROR_BYTES = 256 * 1024; + /** * @param {object} options * @param {object|function} options.catalog Result of buildCatalog(), or a function @@ -144,11 +148,21 @@ const EFFORT_LADDER = ['max', 'xhigh', 'high', 'medium', 'low', 'minimal', 'none const MAX_EFFORT_ATTEMPTS = EFFORT_LADDER.length + 1; const rejectedEfforts = new Map(); +/** Test hook: the rejection memory is per process, not per router. */ +export function clearRejectedEfforts() { + rejectedEfforts.clear(); +} + function nextEffortDown(effort) { const index = EFFORT_LADDER.indexOf(effort); return index === -1 || index === EFFORT_LADDER.length - 1 ? null : EFFORT_LADDER[index + 1]; } +/** Whether a provider's 400 body blames the reasoning effort. */ +function looksLikeEffortRejection(payload) { + return /reasoning[\s_-]*effort|\beffort\b/.test(payload.toString('utf8').toLowerCase()); +} + function forwardToProvider({ req, res, payload, route, onEvent, effortOverride, attempt = 0, state = { providerReq: null } }) { const { provider, model } = route; const target = new URL(`${provider.baseUrl.replace(/\/$/, '')}/chat/completions`); @@ -203,30 +217,48 @@ function forwardToProvider({ req, res, payload, route, onEvent, effortOverride, // remembered for the session, and the retry must carry the replacement: // the CLI sends its own level now, so replaying the original payload // would resend the same value and loop forever. - if (providerRes.statusCode === 400 && outbound.reasoning_effort && attempt < MAX_EFFORT_ATTEMPTS) { - const rejected = rejectedEfforts.get(effortKey) || new Set(); - rejected.add(outbound.reasoning_effort); - rejectedEfforts.set(effortKey, rejected); - - const fallback = nextEffortDown(outbound.reasoning_effort); - onEvent({ - type: 'notice', - provider: provider.id, - message: `${model.model} rejected reasoning effort "${outbound.reasoning_effort}"` - + (fallback ? `, retrying with "${fallback}"` : ', retrying without it'), - }); - - providerRes.resume(); // discard the error body - return forwardToProvider({ - req, - res, - payload, - route, - onEvent, - effortOverride: fallback, - attempt: attempt + 1, - state, - }); + // + // Not every 400 is about the effort. The body is read so an ordinary + // failure (context length, tool schema, content policy) is forwarded + // untouched instead of spending seven extra requests. + if (providerRes.statusCode === 400 && outbound.reasoning_effort) { + readAll(providerRes, MAX_ERROR_BYTES) + .then((errorBody) => { + const retryable = looksLikeEffortRejection(errorBody) && attempt < MAX_EFFORT_ATTEMPTS; + if (!retryable) { + if (!res.headersSent) res.writeHead(providerRes.statusCode, providerRes.headers); + res.end(errorBody); + return; + } + + const rejected = rejectedEfforts.get(effortKey) || new Set(); + rejected.add(outbound.reasoning_effort); + rejectedEfforts.set(effortKey, rejected); + + const fallback = nextEffortDown(outbound.reasoning_effort); + onEvent({ + type: 'notice', + provider: provider.id, + message: `${model.model} rejected reasoning effort "${outbound.reasoning_effort}"` + + (fallback ? `, retrying with "${fallback}"` : ', retrying without it'), + }); + + forwardToProvider({ + req, + res, + payload, + route, + onEvent, + effortOverride: fallback, + attempt: attempt + 1, + state, + }); + }) + .catch((error) => { + onEvent({ type: 'error', provider: provider.id, message: `400 response: ${error.message}` }); + respondJson(res, 502, { error: { message: `copilot-byok router: ${error.message}` } }); + }); + return; } res.writeHead(providerRes.statusCode, providerRes.headers); diff --git a/test/cli.test.mjs b/test/cli.test.mjs index ffaf84c..7d9f7d6 100644 --- a/test/cli.test.mjs +++ b/test/cli.test.mjs @@ -682,6 +682,109 @@ test('native BYOK dry-run publishes selection ids and translates picker ids', as assert.deepEqual(plan.args, ['--model', 'echo/m1']); }); +test('native BYOK dry-run keeps its shape when no provider is active', async () => { + const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-native-empty-')); + const configPath = join(dir, 'providers.json'); + await writeFile(configPath, JSON.stringify({ + providers: [{ + id: 'locked', + name: 'Locked', + baseUrl: 'http://127.0.0.1:1/v1', + apiKeyEnv: 'MISSING_KEY', + models: [{ model: 'm1' }], + }], + })); + + const output = captureWritable(); + const exitCode = await main(['--native-byok', '--no-discovery', '--dry-run', '--config', configPath], { + stdin: { isTTY: false }, + stdout: output, + stderr: captureWritable(), + env: { + COPILOT_BYOK_KEYSTORE: join(dir, 'keys.json'), + COPILOT_BYOK_MODEL_CACHE: join(dir, 'models.json'), + }, + }); + + assert.equal(exitCode, 0); + const plan = JSON.parse(output.text()); + assert.equal(plan.mode, 'native-byok'); + assert.deepEqual(plan.providers, []); + assert.deepEqual(plan.models, []); +}); + +test('native BYOK writes the registry for the session and removes it afterwards', async () => { + const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-native-session-')); + const configPath = join(dir, 'providers.json'); + const scriptPath = join(dir, 'fake-copilot.mjs'); + const markerPath = join(dir, 'session.json'); + await writeFile(configPath, JSON.stringify({ + providers: [{ + id: 'echo', + name: 'Echo', + baseUrl: 'http://127.0.0.1:1/v1', + authRequired: false, + models: [{ model: 'm1', contextWindow: 128000 }], + }], + })); + await writeFile(scriptPath, [ + "import { readFile, writeFile } from 'node:fs/promises';", + 'const registry = process.env.COPILOT_PROVIDERS_CONFIG;', + "const contents = await readFile(registry, 'utf8');", + "await writeFile(process.argv[2], JSON.stringify({ registry, contents }));", + ].join('\n')); + + const exitCode = await main(['--native-byok', '--no-discovery', '--config', configPath, '--', scriptPath, markerPath], { + stdin: { isTTY: false }, + stdout: captureWritable(), + stderr: captureWritable(), + env: { + COPILOT_BIN: process.execPath, + COPILOT_BYOK_KEYSTORE: join(dir, 'keys.json'), + COPILOT_BYOK_MODEL_CACHE: join(dir, 'models.json'), + COPILOT_BYOK_LOG: join(dir, 'router.log'), + }, + }); + + assert.equal(exitCode, 0); + const seen = JSON.parse(await readFile(markerPath, 'utf8')); + const registry = JSON.parse(seen.contents); + assert.equal(registry.providers[0].name, 'echo'); + assert.equal(registry.models[0].id, 'm1'); + await assert.rejects(() => stat(seen.registry), 'the temporary registry is removed'); +}); + +test('legacy mode accepts a provider with only a credential command', async () => { + const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-command-')); + const configPath = join(dir, 'providers.json'); + await writeFile(configPath, JSON.stringify({ + providers: [{ + id: 'gateway', + name: 'Gateway', + baseUrl: 'https://gateway.example.com/v1', + apiKeyCommand: 'vault read -field=key secret/llm', + headers: { 'X-Team': 'team-a' }, + }], + })); + + const output = captureWritable(); + const exitCode = await main(['--legacy', '--provider', 'gateway', '--model', 'm1', '--dry-run', '--config', configPath], { + stdin: { isTTY: false }, + stdout: output, + stderr: captureWritable(), + env: { + COPILOT_BYOK_KEYSTORE: join(dir, 'keys.json'), + COPILOT_BYOK_MODEL_CACHE: join(dir, 'models.json'), + }, + }); + + assert.equal(exitCode, 0); + const plan = JSON.parse(output.text()); + assert.equal(plan.env.COPILOT_PROVIDER_API_KEY_COMMAND, ''); + assert.equal(plan.env.COPILOT_PROVIDER_HEADERS, ''); + assert.equal(Object.hasOwn(plan.env, 'COPILOT_PROVIDER_API_KEY'), false); +}); + test('legacy mode isolates a native registry that would take precedence', async () => { const home = await mkdtemp(join(tmpdir(), 'copilot-byok-legacy-home-')); const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-legacy-')); diff --git a/test/config.test.mjs b/test/config.test.mjs index 46877e1..3c4d544 100644 --- a/test/config.test.mjs +++ b/test/config.test.mjs @@ -356,6 +356,23 @@ test('rejects header values with line breaks', async () => { await assert.rejects(() => loadConfigFromPath(configPath, {}), /must not contain line breaks/); }); +test('rejects request headers that are not an object of strings', async () => { + for (const headers of ['X-Team: a', ['a'], { 'X-Team': 42 }]) { + const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-')); + const configPath = join(dir, 'providers.json'); + await writeFile(configPath, JSON.stringify({ + providers: [{ + id: 'bad-headers', + name: 'Bad Headers', + baseUrl: 'https://api.example.com/v1', + headers, + }], + })); + + await assert.rejects(() => loadConfigFromPath(configPath, {}), /headers must be an object|string header values/); + } +}); + test('rejects an OpenAI wire API override on Anthropic providers', async () => { const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-')); const configPath = join(dir, 'providers.json'); diff --git a/test/doctor.test.mjs b/test/doctor.test.mjs index 2991b66..fdb8b26 100644 --- a/test/doctor.test.mjs +++ b/test/doctor.test.mjs @@ -21,7 +21,7 @@ test('reports a healthy installation as ok', async () => { const text = stdout.text(); assert.match(text, /copilot: 1\.0\.105/); assert.match(text, /config: 1 providers/); - assert.match(text, /providers: 1 of 1 have a credential/); + assert.match(text, /providers: 0 keyed, 1 need no key/); assert.match(text, /native registry: .*not present/); assert.match(text, /No failures\./); }); @@ -61,6 +61,69 @@ test('a broken configuration fails the doctor', async () => { assert.match(stdout.text(), /1 check\(s\) failed/); }); +test('the version probe runs without provider credentials', async () => { + const { io, configPath } = await fixture([{ + id: 'keyed', + name: 'Keyed', + baseUrl: 'https://api.example.com/v1', + apiKeyEnv: 'CUSTOM_PROVIDER_KEY', + models: [{ model: 'm1' }], + }]); + let seenEnv; + + const exitCode = await runDoctorCommand({ + argv: ['--config', configPath], + io: { ...io, stdout: captureWritable() }, + probeVersion: (bin, env) => { + seenEnv = env; + return '1.0.105'; + }, + }); + + assert.equal(exitCode, 0); + assert.equal(Object.hasOwn(seenEnv, 'CUSTOM_PROVIDER_KEY'), false); + assert.equal(Object.hasOwn(seenEnv, 'OPENAI_API_KEY'), false); + assert.equal(Object.hasOwn(seenEnv, 'COPILOT_PROVIDER_BASE_URL'), false); + assert.equal(seenEnv.COPILOT_BIN, 'copilot'); +}); + +test('--online reports discovery failures instead of an empty success', async () => { + const { io, configPath } = await fixture(); + const stdout = captureWritable(); + + const exitCode = await runDoctorCommand({ + argv: ['--online', '--config', configPath], + io: { ...io, stdout }, + probeVersion: () => '1.0.105', + discover: async (providers, { onEvent }) => { + onEvent({ type: 'error', provider: 'echo', message: '401 Unauthorized' }); + return providers.map((provider) => ({ ...provider, models: [] })); + }, + }); + + assert.equal(exitCode, 0); + assert.match(stdout.text(), /WARN discovery: no models from 1 configured provider\(s\); echo: 401 Unauthorized/); +}); + +test('a provider that only has a credential command is reported as legacy-only', async () => { + const { io, configPath } = await fixture([{ + id: 'gateway', + name: 'Gateway', + baseUrl: 'https://gateway.example.com/v1', + apiKeyCommand: 'vault read -field=key secret/llm', + }]); + const stdout = captureWritable(); + + const exitCode = await runDoctorCommand({ + argv: ['--config', configPath], + io: { ...io, stdout }, + probeVersion: () => '1.0.105', + }); + + assert.equal(exitCode, 0); + assert.match(stdout.text(), /WARN apiKeyCommand: gateway work only with --legacy/); +}); + test('a missing Copilot binary is a warning, not a failure', async () => { const { io, configPath } = await fixture(); const stdout = captureWritable(); @@ -75,18 +138,16 @@ test('a missing Copilot binary is a warning, not a failure', async () => { assert.match(stdout.text(), /WARN copilot:/); }); -async function fixture() { +async function fixture(providers = [{ + id: 'echo', + name: 'Echo', + baseUrl: 'http://127.0.0.1:1/v1', + authRequired: false, + models: [{ model: 'm1' }], +}]) { const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-doctor-')); const configPath = join(dir, 'providers.json'); - await writeFile(configPath, JSON.stringify({ - providers: [{ - id: 'echo', - name: 'Echo', - baseUrl: 'http://127.0.0.1:1/v1', - authRequired: false, - models: [{ model: 'm1' }], - }], - })); + await writeFile(configPath, JSON.stringify({ providers })); const env = { COPILOT_BIN: 'copilot', @@ -95,6 +156,8 @@ async function fixture() { COPILOT_BYOK_MODEL_CACHE: join(dir, 'models.json'), COPILOT_BYOK_LOG: join(dir, 'router.log'), COPILOT_BYOK_SHIM_DIR: join(dir, 'shim'), + OPENAI_API_KEY: 'secret', + COPILOT_PROVIDER_BASE_URL: 'https://stale.example.com/v1', }; return { diff --git a/test/models-command.test.mjs b/test/models-command.test.mjs index c5cc8a3..06e47df 100644 --- a/test/models-command.test.mjs +++ b/test/models-command.test.mjs @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import http from 'node:http'; import { mkdtemp, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -7,6 +8,50 @@ import test from 'node:test'; import { runModelsCommand } from '../src/models-command.mjs'; +test('lists live discovery results when --offline is not set', async () => { + const server = http.createServer((req, res) => { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ + data: [{ id: 'live-model', context_length: 64000, supported_features: ['tools'], created: 1 }], + })); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + + try { + const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-models-live-')); + const configPath = join(dir, 'providers.json'); + await writeFile(configPath, JSON.stringify({ + providers: [{ + id: 'live', + name: 'Live', + baseUrl: `http://127.0.0.1:${server.address().port}/v1`, + modelsUrl: `http://127.0.0.1:${server.address().port}/v1/models`, + authRequired: false, + }], + })); + + const stdout = captureWritable(); + const exitCode = await runModelsCommand({ + argv: ['--json', '--config', configPath], + io: { + stdout, + stderr: captureWritable(), + env: { + COPILOT_BYOK_KEYSTORE: join(dir, 'keys.json'), + COPILOT_BYOK_MODEL_CACHE: join(dir, 'models.json'), + }, + }, + }); + + assert.equal(exitCode, 0); + const rows = JSON.parse(stdout.text()); + assert.equal(rows[0].models[0].id, 'live-model'); + assert.equal(rows[0].models[0].contextWindow, 64000); + } finally { + await new Promise((resolve) => server.close(resolve)); + } +}); + test('lists pinned models without touching the network in offline mode', async () => { const { configPath, env } = await fixture(); const stdout = captureWritable(); diff --git a/test/registry-file.test.mjs b/test/registry-file.test.mjs index 7babc15..fd1613b 100644 --- a/test/registry-file.test.mjs +++ b/test/registry-file.test.mjs @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { mkdtemp, readFile, writeFile } from 'node:fs/promises'; +import { mkdir, mkdtemp, readFile, readdir, stat, utimes, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import test from 'node:test'; @@ -8,6 +8,7 @@ import { createRegistryFile, inspectNativeRegistry, nativeRegistryPath, + sweepStaleRegistries, } from '../src/registry-file.mjs'; test('writes a registry to a private temp directory and removes it on cleanup', async () => { @@ -19,6 +20,13 @@ test('writes a registry to a private temp directory and removes it on cleanup', const file = await createRegistryFile(registry); try { assert.deepEqual(JSON.parse(await readFile(file.path, 'utf8')), registry); + + if (process.platform !== 'win32') { + assert.equal((await stat(file.path)).mode & 0o777, 0o600, 'registry file is owner-only'); + assert.equal((await stat(file.dir)).mode & 0o777, 0o700, 'registry directory is owner-only'); + } + + assert.equal(await file.cleanup(), true); } finally { await file.cleanup(); } @@ -26,6 +34,34 @@ test('writes a registry to a private temp directory and removes it on cleanup', await assert.rejects(() => readFile(file.path, 'utf8')); }); +test('refuses to write keys when permissions cannot be restricted', async () => { + const base = await mkdtemp(join(tmpdir(), 'copilot-byok-registry-base-')); + + await assert.rejects( + () => createRegistryFile({ providers: [], models: [] }, { base, restrict: async () => false }), + /could not restrict permissions/, + ); + assert.deepEqual(await readdir(base), [], 'the temporary directory is removed'); +}); + +test('sweeps only stale registries from previous sessions', async () => { + const base = await mkdtemp(join(tmpdir(), 'copilot-byok-registry-base-')); + const stale = join(base, 'copilot-byok-registry-stale'); + const recent = join(base, 'copilot-byok-registry-recent'); + await mkdir(stale); + await mkdir(recent); + await writeFile(join(stale, 'providers.json'), '{}'); + await writeFile(join(recent, 'providers.json'), '{}'); + + const old = new Date(Date.now() - 8 * 24 * 60 * 60 * 1000); + await utimes(stale, old, old); + + const removed = await sweepStaleRegistries({ base }); + assert.equal(removed, 1); + await assert.rejects(() => stat(stale)); + assert.ok(await stat(recent)); +}); + test('honours COPILOT_PROVIDERS_CONFIG and counts the registry entries', async () => { const dir = await mkdtemp(join(tmpdir(), 'copilot-byok-registry-')); const path = join(dir, 'custom.json'); diff --git a/test/router.test.mjs b/test/router.test.mjs index 61e3250..282cebc 100644 --- a/test/router.test.mjs +++ b/test/router.test.mjs @@ -1,11 +1,15 @@ import assert from 'node:assert/strict'; import http from 'node:http'; -import test from 'node:test'; +import test, { beforeEach } from 'node:test'; import { buildCatalog } from '../src/catalog.mjs'; -import { createRouter, mergeModels } from '../src/router.mjs'; +import { clearRejectedEfforts, createRouter, mergeModels } from '../src/router.mjs'; import { createUpstreamResolver } from '../src/upstream.mjs'; +beforeEach(() => { + clearRejectedEfforts(); +}); + test('mergeModels appends BYOK entries to the GitHub list', () => { const payload = JSON.stringify({ data: [{ id: 'gpt-5.5' }] }); const merged = mergeModels(payload, [{ id: 'byok-acme-x' }]); @@ -485,6 +489,46 @@ test('a caller-sent effort the provider rejects steps down instead of looping', } }); +test('a 400 unrelated to the effort is forwarded without stepping down', async () => { + const attempts = []; + const provider = http.createServer((req, res) => { + let body = ''; + req.on('data', (chunk) => { body += chunk; }); + req.on('end', () => { + attempts.push(JSON.parse(body).reasoning_effort); + res.writeHead(400, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ error: { message: 'context_length_exceeded' } })); + }); + }); + await new Promise((resolve) => provider.listen(0, '127.0.0.1', resolve)); + + const catalog = buildCatalog([{ + id: 'delta', name: 'Delta', baseUrl: `http://127.0.0.1:${provider.address().port}/v1`, + apiKey: 'k', models: [{ model: 'context-limit' }], + }]); + const router = createRouter({ + catalog, + upstream: createUpstreamResolver({ explicit: 'https://upstream.invalid' }), + }); + await new Promise((resolve) => router.listen(0, '127.0.0.1', resolve)); + + try { + const response = await fetch(`http://127.0.0.1:${router.address().port}/chat/completions`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ model: 'byok-delta-context-limit', messages: [], reasoning_effort: 'high' }), + }); + + assert.equal(response.status, 400); + assert.match(await response.text(), /context_length_exceeded/); + assert.deepEqual(attempts, ['high'], 'one request, no effort ladder'); + } finally { + router.closeAllConnections(); + await new Promise((resolve) => router.close(resolve)); + await new Promise((resolve) => provider.close(resolve)); + } +}); + test('an effort every level rejects ends with the provider error', async () => { const attempts = []; const provider = http.createServer((req, res) => { From 583f5453b6d59cc9ff0ebcb9f7c5bd60e9a17837 Mon Sep 17 00:00:00 2001 From: Michael Gasperini Date: Sun, 1 Nov 2026 19:47:59 +0100 Subject: [PATCH 2/2] docs: document the hardened native registry and diagnostics Describe the permission, cleanup and stale-sweep guarantees, the legacy-only apiKeyCommand, the effort-400 behavior, and the new verification evidence. Update CHANGELOG and AGENTS. --- AGENTS.md | 16 ++++++++++------ CHANGELOG.md | 19 ++++++++++++++++++- SECURITY.md | 2 +- docs/configuration.md | 22 +++++++++++++++------- docs/how-it-works.md | 8 +++++--- docs/provider-verification.md | 6 +++++- docs/troubleshooting.md | 11 +++++++++-- 7 files changed, 63 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f1d0355..8b6acdc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,8 +17,10 @@ through the opt-in key store (`copilot-byok keys set` or `/byok`), never on its `--native-byok` publishes the same models through the CLI's supported `providers.json` registry instead: discovery runs as usual, a private temporary registry is written, `COPILOT_PROVIDERS_CONFIG` -points at it for the child process, and it is deleted when the session ends. That file contains the -provider credentials for the session, which is why the mode stays opt-in. +points at it for the child process, and it is deleted on normal exit, on failure, and on the usual +termination signals. That file contains the provider credentials for the session, which is why the +mode stays opt-in. Native mode needs a CLI that loads `providers.json`; 1.0.105 is the verified +version and 1.0.79 does not load it. `COPILOT_API_URL` is an internal Copilot CLI variable, absent from `copilot help environment`. Treat it as the project's main external risk: if a CLI release changes it, the router breaks and @@ -131,7 +133,8 @@ This project handles third-party provider API keys. Every change must preserve t interactive prompt: never accept a key from argv, stdin pipes, or an environment variable meant for something else. It must stay opt-in — nothing may create it implicitly. - Never print, log, or embed a credential value. `--dry-run` output must keep passing through `redactEnv()`, - which masks any key matching `/KEY|TOKEN|SECRET|PASSWORD/i`. + which masks any key matching `/KEY|TOKEN|SECRET|PASSWORD/i` and the whole `COPILOT_PROVIDER_HEADERS` + value. - Error messages must not reveal which environment variable holds a secret (covered by a test). - Model-catalog requests send credentials only when the catalog URL is same-origin with `baseUrl`, or when the provider explicitly sets an authenticated `modelsAuth` mode (`bearer`, `x-api-key`, or @@ -145,9 +148,10 @@ This project handles third-party provider API keys. Every change must preserve t - No `.env` file is used or expected; there is no `.env.example`. Do not add one. - Never commit real keys, tokens, or a `providers.json` containing credentials. - `--native-byok` writes provider credentials to a temporary registry because the CLI's supported - path requires them inline. That file must keep `0600` (owner-only ACL on Windows), live in the - system temp directory, and be removed on exit, including on failure. It must never be written to - the repository or to the user's Copilot config directory. + path requires them inline. The directory must be owner-only before the file exists, a failed + restriction must abort the session, and the file must be removed on exit, on failure to start, and + on SIGINT/SIGTERM/SIGHUP; a hard kill is covered only by the stale-registry sweep. It must never be + written to the repository or to the user's Copilot config directory. - `--legacy` must keep isolating itself from a user `providers.json` when one is present: that file takes precedence over `COPILOT_PROVIDER_*`, and silently using it would break the mode. diff --git a/CHANGELOG.md b/CHANGELOG.md index db638e8..7a88674 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,8 @@ All notable changes to this project are documented in this file. `copilot-api..ghe.com` instead of the public Copilot hosts. - Legacy mode now passes `COPILOT_PROVIDER_HEADERS` (from `headers`) and `COPILOT_PROVIDER_API_KEY_COMMAND` (from `apiKeyCommand`), and sets `COPILOT_PROVIDER_MODEL_ID` - when `catalogModelId` is configured. + when `catalogModelId` is configured. A provider with only `apiKeyCommand` is accepted in + `--legacy` (Copilot CLI 1.0.84 or newer) and reported as legacy-only elsewhere. ### Changed @@ -30,11 +31,27 @@ All notable changes to this project are documented in this file. - Legacy mode detects a user `providers.json` that would take precedence over `COPILOT_PROVIDER_*`, points the child at an empty temporary registry, and says so on stderr. +### Security + +- Native registry: the temporary directory is made owner-only before the file exists, and a failed + restriction aborts the session instead of leaving keys readable. Cleanup now covers SIGINT, + SIGTERM and SIGHUP, registry removal failures are reported, and stale registries from a hard kill + are swept at the next native startup. +- `--dry-run` no longer writes provider keys for `--native-byok`, and `COPILOT_PROVIDER_HEADERS` is + redacted in dry-run output. + ### Fixed - A reasoning-effort level sent by the CLI and rejected by the provider no longer loops: the retry now carries the stepped-down level instead of replaying the original request. The close listener is also registered once per client request instead of once per retry. +- The effort ladder runs only when a `400` body mentions the reasoning effort; ordinary context, + schema or content-policy errors are forwarded unchanged instead of consuming seven retries. +- `doctor` runs the Copilot version probe without provider credentials, reports discovery failures + in `--online`, counts keyed and authless providers separately, and marks `apiKeyCommand` providers + as legacy-only. +- `headers` must be an object of string values, like `modelsHeaders`, and the published schema no + longer describes the old reasoning-effort behavior. - `docs/troubleshooting.md` had a broken router log path. ## [2.0.1] - 2026-10-21 diff --git a/SECURITY.md b/SECURITY.md index 0a8e212..203dbb4 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -29,7 +29,7 @@ Out of scope: ## How credentials are handled -**Provider keys.** They are read from environment variables or, if you opt in, from the local key store. In router mode the keys stay in the router's own process: they are attached to that provider's outgoing request and stripped from the environment handed to Copilot CLI, so the agent process never sees them. In `--native-byok` mode the CLI reads them from a temporary `providers.json` written by this tool: the file is created in the system temp directory with `0600` and an owner-only ACL on Windows, passed to the CLI through `COPILOT_PROVIDERS_CONFIG`, and deleted when the session ends. In `--legacy` mode the selected credential is passed to Copilot itself, because that mode uses Copilot's built-in BYOK variables. +**Provider keys.** They are read from environment variables or, if you opt in, from the local key store. In router mode the keys stay in the router's own process: they are attached to that provider's outgoing request and stripped from the environment handed to Copilot CLI, so the agent process never sees them. In `--native-byok` mode the CLI reads them from a temporary `providers.json` written by this tool: the directory and the file are made owner-only before the file is written, the session is aborted if that restriction fails, and the registry is passed through `COPILOT_PROVIDERS_CONFIG`. It is deleted on normal exit, on failure to start, and on SIGINT/SIGTERM/SIGHUP; a hard kill can leave it behind, so registries older than a week are swept at the next native session. In `--legacy` mode the selected credential is passed to Copilot itself, because that mode uses Copilot's built-in BYOK variables. **The local key store.** `copilot-byok keys set` and the `/byok` command write to `~/.config/copilot-byok/keys.json` (`%APPDATA%\copilot-byok\keys.json` on Windows), created with mode `0600` and an owner-only ACL on Windows. **Keys stored there are plain text on disk**, protected by file permissions alone; if that does not fit your threat model, use environment variables, which always take precedence and are the default. The store is never created unless you use it. Keys are only accepted from an interactive prompt, never from command-line arguments or pipes, so they do not reach shell history or process listings. diff --git a/docs/configuration.md b/docs/configuration.md index c94eddb..e48f07d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -34,7 +34,9 @@ hold them. Inline `apiKey` fields are rejected. `apiKeyCommand` is the exception to environment variables: it names a command the Copilot CLI runs itself to obtain a rotating credential, passed through as `COPILOT_PROVIDER_API_KEY_COMMAND` in -legacy mode. `headers` adds gateway headers to the provider requests, and to the native registry in +legacy mode. It needs Copilot CLI 1.0.84 or newer and works only with `--legacy`: the CLI's +`providers.json` registry has no equivalent, so router and `--native-byok` sessions cannot use it. +`headers` adds gateway headers to the provider requests, and to the native registry in `--native-byok` mode. ## Commands @@ -78,9 +80,12 @@ Use it when you want the supported BYOK path, or when `COPILOT_API_URL` stops wo Requirements and trade-offs: -- a Copilot CLI that reads `providers.json` (verified on 1.0.105); +- a Copilot CLI that reads `providers.json` (verified on 1.0.105; 1.0.79 does not); - provider keys are written to that temporary file for the session, unlike router mode where they - never reach the Copilot process; + never reach the Copilot process. The directory and file are owner-only before the file is written; + if that restriction fails the session is aborted rather than leaving keys readable. The registry is + removed on normal exit and on SIGINT/SIGTERM/SIGHUP, and stale registries older than a week are + swept at the next native startup for the uncatchable cases (SIGKILL, power loss); - the router's reasoning-effort step-down does not apply because the CLI talks to the provider directly; the levels a model advertises are what the picker offers; - a `~/.copilot/providers.json` you maintain yourself is ignored for that session, since the @@ -161,8 +166,8 @@ with `low` and with `high` were byte-identical: that was the CLI bug tracked in The effort selector is therefore declared for models that report reasoning support, and can be hidden per model or per provider with `reasoningEffortPicker: false`. -You can still force a level from the config, useful on older CLIs or when a model should always run -at a fixed effort: +You can still force a level from the config in **router mode**, useful on older CLIs or when a model +should always run at a fixed effort: ```json { "model": "deepseek-reasoner", "reasoningEffort": "high" } @@ -170,11 +175,14 @@ at a fixed effort: `reasoningEffort` also works at provider level as a default. Values are the ones Copilot uses: `none`, `minimal`, `low`, `medium`, `high`, `xhigh`, `max`. `reasoningEffortLevels` narrows what -the picker offers for a model. +the picker offers for a model. `--native-byok` ignores `reasoningEffort`: the CLI sends the level +from the picker or `--reasoning-effort`, and only `reasoningEffortLevels` constrains it there. **Levels differ per model and providers do not fall back.** GLM-5.2 accepts `max` but answers `400` to `xhigh`. In router mode the router steps down one level at a time, applies the same ladder to a -level the CLI sent, records the rejection for the session, and logs the adjustment: +level the CLI sent, records the rejection for the session, and logs the adjustment. A `400` that +does not mention the effort is forwarded unchanged, so an ordinary context or schema error is never +retried through the ladder: ```text notice: chutes: zai-org/GLM-5.2-TEE rejected reasoning effort "xhigh", retrying with "high" diff --git a/docs/how-it-works.md b/docs/how-it-works.md index f1ab270..0b8c196 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -93,6 +93,8 @@ instead of the router. It runs the usual live discovery, writes a temporary `pro is no local proxy and no dependency on `COPILOT_API_URL`. The trade-off is the one the registry itself imposes: provider keys are in that temporary file while -the session runs. The file is created in the system temp directory with owner-only permissions and -removed on exit, including when the session fails to start. Model ids are `provider/model`; a -`--model byok-...` value is translated to the matching native id. +the session runs. The directory and file are owner-only before the file is written; if the +restriction cannot be applied the session is aborted. The file is removed on normal exit, when the +session fails to start, and on SIGINT/SIGTERM/SIGHUP; registries left by a hard kill are swept when +the next native session starts. Model ids are `provider/model`; a `--model byok-...` value is +translated to the matching native id. diff --git a/docs/provider-verification.md b/docs/provider-verification.md index 3cefc0f..3bb926b 100644 --- a/docs/provider-verification.md +++ b/docs/provider-verification.md @@ -36,6 +36,9 @@ Re-verified on 2026-11-01 against 1.0.105, with 1.0.79 as the baseline. | Reasoning effort to BYOK | not sent | sent | `reasoning_effort: "high"` on the wire with native provider variables, and through the router when the model entry advertises levels | | Native `providers.json` registry | not loaded | loaded | two providers in one file; `--model p2/m2` reached the second base URL with its `wireModel` | | Extension SDK | unchanged | unchanged | the 1.0.28 SDK still exports `joinSession`, `commands`, `ui.select` and `session.log`; extensions remain experimental and were not run interactively in this audit | +| Data residency origin | not exercised | derived, unit-tested | `dataResidencyOrigin` maps a `*.ghe.com` host to `copilot-api..ghe.com`; no live data residency tenant was available, so the host is inferred from upstream issue #4527 and the CLI's documented host handling, not verified end-to-end | +| `COPILOT_PROVIDER_HEADERS`, `_MODEL_ID`, `_API_KEY_COMMAND` | n/a | documented and passed through | 1.0.105 `copilot help providers` lists all three; `API_KEY_COMMAND` exists since 1.0.84 per upstream issue #4936; the environment pass-through is covered by unit tests, with no real provider run | +| `--native-byok` minimum CLI | does not load the registry | works | a manual `providers.json` was not loaded on 1.0.79; no bisect was done, so 1.0.105 is the verified version. Do not assume native mode works on older releases | `COPILOT_API_URL` remains undocumented: it is not listed in `copilot help environment` on 1.0.105. The native registry is the supported alternative, which is why `--native-byok` exists. @@ -186,7 +189,8 @@ On 2026-11-01, `copilot-byok --native-byok` ran on CLI 1.0.105 with an isolated The generated `providers.json` was accepted, Copilot sent `POST /v1/chat/completions` with the pinned model name, used the reply, and the temporary registry was gone after exit. This verifies the registry shape and the -cleanup path, not a real provider's inference. +clean-exit cleanup, not a real provider's inference. Signal cleanup and the +stale-registry sweep are covered by unit tests, not by this end-to-end run. ## Reproducing diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 1aebf42..7e8fbc0 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -90,8 +90,15 @@ pointing at it. Requirements: - model ids in `/model` are `provider/model`; `--model byok-...` is translated for you. If the session fails immediately, run `copilot-byok --native-byok --dry-run` to see the providers -and selection ids that would be written. The temporary file is removed when Copilot exits, and a -`providers.json` you maintain yourself is ignored for that session. +and selection ids that would be written. The temporary file is removed on normal exit, on failure to +start, and on Ctrl+C or a termination signal; after a hard kill, stale registries are removed at the +next native startup. A `providers.json` you maintain yourself is ignored for that session. + +## `apiKeyCommand` is ignored + +A provider that only has an `apiKeyCommand` works with `--legacy` and needs Copilot CLI 1.0.84 or +newer. Router mode and `--native-byok` cannot use it: the CLI's `providers.json` registry has no +credential-command equivalent. `--list-providers` marks such providers as `apiKeyCommand (--legacy only)`. ## `Unknown BYOK model`