From 35d7edc2f6548c9c4608a43eabce3ea6cfb14cae Mon Sep 17 00:00:00 2001 From: 0xfandom <50949929+0xfandom@users.noreply.github.com> Date: Mon, 21 Sep 2026 06:37:40 +0530 Subject: [PATCH] fix(api): validate API_TIMEOUT_MS in the native Anthropic clients (#2169) * fix(api): validate API_TIMEOUT_MS in the native Anthropic clients The native Anthropic client path parsed API_TIMEOUT_MS with a raw parseInt: `parseInt(process.env.API_TIMEOUT_MS || String(600 * 1000), 10)` in client.ts and `parseInt(process.env.API_TIMEOUT_MS || '', 10)` in the non-streaming fallback. Because `||` only substitutes the default for an empty/unset value, any non-empty malformed value ("abc", "25ms") is truthy and parses to NaN, and a negative value passes straight through. A NaN or negative timeout on the SDK client is undefined behavior, and an oversized value overflows the 32-bit setTimeout ceiling into a tiny/immediate delay. The OpenAI-compatible shim already routes API_TIMEOUT_MS through getApiTimeoutMs, which rejects non-digit/negative input, falls back to a default, and clamps to the 32-bit ceiling. Reuse that single parser in the native clients too. getApiTimeoutMs gains an optional fallbackMs argument so the non-streaming path keeps its context-specific default (120s on remote, 300s otherwise) while still validating and clamping the override; existing shim callers are unaffected (the default remains 600s). Tests cover the fallback argument in the parser and assert the constructed native client's timeout for malformed, negative, zero, unset, valid, and oversized values. * test(api): cover the non-streaming fallback timeout and pin route env Add direct coverage for getNonstreamingFallbackTimeoutMs, which the previous tests only exercised indirectly through the shared parser. It now asserts the context-specific defaults survive the change (300s local, 120s remote) for missing and invalid API_TIMEOUT_MS, that a valid override is honored in both contexts, and that an oversized one is clamped. The function is exported for this; test files are knip entry points, so it does not read as dead code. Also clear every provider route-selection variable explicitly in the native client timeout test. The suite's afterEach already restores them, but pinning them up front keeps an inherited CLAUDE_CODE_USE_OPENAI (or another route flag) from diverting getAnthropicClient into the shim and measuring the wrong client's timeout. * test(api): pin the native client path and document the timeout contract The native timeout test cleared many provider variables but left NEARAI_API_KEY untouched, and the suite never snapshotted it at all. NEARAI_API_KEY alone selects the env-only Near AI route, and clearing the competing vendor keys removed the very credentials whose presence suppresses it, so an inherited value routed getAnthropicClient to the OpenAI shim. The shim is returned as a cast object with no SDK timeout, so the assertions read undefined and the regression silently stopped being covered. CLINE_API_KEY had the same gap. Snapshot, clear, and restore both alongside the other route-selection inputs, and assert the constructed object is an Anthropic instance before reading its timeout, so a stray variable or a future env-only route fails loudly instead of weakening the test. Verified by injecting NEARAI_API_KEY: the guard now fails on the shim object rather than passing on undefined. Also refresh the API_TIMEOUT_MS reference, which still said the Anthropic SDK kept its own timeout handling. It now describes both consumers: the headers-deadline semantics for OpenAI-compatible paths, and the native Anthropic SDK request timeout plus the non-streaming fallback's own defaults (120s remote, 300s otherwise), under the same validation and clamping. --- docs/advanced-setup.md | 2 +- src/services/api/claude.timeout.test.ts | 59 ++++++++++++++ src/services/api/claude.ts | 14 ++-- src/services/api/client.test.ts | 80 +++++++++++++++++++ src/services/api/client.ts | 7 +- src/services/api/openaiShim/transport.test.ts | 15 ++++ src/services/api/openaiShim/transport.ts | 5 +- 7 files changed, 171 insertions(+), 11 deletions(-) create mode 100644 src/services/api/claude.timeout.test.ts diff --git a/docs/advanced-setup.md b/docs/advanced-setup.md index 224d3399a..55db69772 100644 --- a/docs/advanced-setup.md +++ b/docs/advanced-setup.md @@ -500,7 +500,7 @@ host. Without this variable the behavior is unchanged. | `OPENAI_MODEL` | OpenAI-compatible only | Model name such as `gpt-4o`, `deepseek-v4-flash`, or `llama3.3:70b` | | `OPENAI_BASE_URL` | No | API endpoint, defaulting to `https://api.openai.com/v1` | | `OPENAI_API_BASE` | No | Compatibility alias for `OPENAI_BASE_URL` | -| `API_TIMEOUT_MS` | No | Time-to-response-headers deadline for generic OpenAI-compatible requests, direct GitHub Copilot Responses, and Copilot chat-to-Responses fallback requests, in milliseconds (default: `600000`, or 10 minutes). The value must be a safe positive integer; invalid, zero, negative, or fractional values use the default, and values above `2147483647` are capped. The deadline is disarmed after headers arrive, so it does not limit response streaming. Export this runtime setting from your shell or launcher; the provider env-file loader ignores runtime/debug settings, so a value configured only there leaves the default in effect. First-party Codex OAuth Responses and the Anthropic SDK retain their existing timeout handling. | +| `API_TIMEOUT_MS` | No | Request timeout in milliseconds, shared by the OpenAI-compatible and native Anthropic paths (default: `600000`, or 10 minutes). The value must be a safe positive integer; invalid, empty, zero, negative, or fractional values fall back to the applicable default, and values above `2147483647` are capped. For generic OpenAI-compatible requests, direct GitHub Copilot Responses, and Copilot chat-to-Responses fallback requests it is a time-to-response-headers deadline that is disarmed once headers arrive, so it does not limit response streaming. For native Anthropic clients it sets the Anthropic SDK request timeout, and for the non-streaming fallback it overrides that path's own default (`120000` on remote sessions, `300000` otherwise) under the same validation and clamping. Export this runtime setting from your shell or launcher; the provider env-file loader ignores runtime/debug settings, so a value configured only there leaves the default in effect. First-party Codex OAuth Responses retain their existing timeout handling. | | `OPENCLAUDE_OLLAMA_NUM_CTX` | Ollama only | Request-level Ollama context window. Defaults to `32768`; set a larger value for longer same-session history if your model and hardware can handle it. | | `OPENCLAUDE_NODE_MAX_OLD_SPACE_SIZE_MB` | No | Explicit V8 old-space heap cap in megabytes for the Node launcher. Used only when `--max-memory`, `--max-old-space-size-percentage`, and `OPENCLAUDE_NODE_MAX_OLD_SPACE_SIZE_PERCENTAGE` are all unset. Default `8192`. | | `OPENCLAUDE_NODE_MAX_OLD_SPACE_SIZE_PERCENTAGE` | No | Size the V8 old-space heap as a percentage (greater than 0, up to 100) of constrained/container memory when Node reports it, otherwise total system RAM. Converted to `--max-old-space-size` so Node 22.0.0 can start. Precedence, earlier wins: `--max-memory` → `--max-old-space-size-percentage` → this variable → `OPENCLAUDE_NODE_MAX_OLD_SPACE_SIZE_MB` → default `8192`. | diff --git a/src/services/api/claude.timeout.test.ts b/src/services/api/claude.timeout.test.ts new file mode 100644 index 000000000..88a78a5ac --- /dev/null +++ b/src/services/api/claude.timeout.test.ts @@ -0,0 +1,59 @@ +import { afterEach, expect, test } from 'bun:test' +import { getNonstreamingFallbackTimeoutMs } from './claude.js' + +const originalApiTimeoutMs = process.env.API_TIMEOUT_MS +const originalRemote = process.env.CLAUDE_CODE_REMOTE + +function restore(key: string, value: string | undefined): void { + if (value === undefined) delete process.env[key] + else process.env[key] = value +} + +afterEach(() => { + restore('API_TIMEOUT_MS', originalApiTimeoutMs) + restore('CLAUDE_CODE_REMOTE', originalRemote) +}) + +// The non-streaming fallback previously read API_TIMEOUT_MS with a raw +// parseInt and returned the override whenever it was truthy, so a malformed +// value produced NaN and a negative value passed straight through. It now +// shares the validated parser, which must still preserve the context-specific +// defaults: 120s on remote (to stay under CCR's container idle-kill) and 300s +// otherwise. +test('non-streaming fallback keeps the local default without an override', () => { + delete process.env.CLAUDE_CODE_REMOTE + delete process.env.API_TIMEOUT_MS + expect(getNonstreamingFallbackTimeoutMs()).toBe(300_000) +}) + +test('non-streaming fallback keeps the remote default without an override', () => { + process.env.CLAUDE_CODE_REMOTE = '1' + delete process.env.API_TIMEOUT_MS + expect(getNonstreamingFallbackTimeoutMs()).toBe(120_000) +}) + +test('non-streaming fallback rejects malformed and negative overrides', () => { + for (const invalid of ['not-a-number', '-1', '0', '1.5', '25ms', '']) { + delete process.env.CLAUDE_CODE_REMOTE + process.env.API_TIMEOUT_MS = invalid + expect(getNonstreamingFallbackTimeoutMs()).toBe(300_000) + + process.env.CLAUDE_CODE_REMOTE = '1' + expect(getNonstreamingFallbackTimeoutMs()).toBe(120_000) + } +}) + +test('non-streaming fallback honors a valid override in both contexts', () => { + process.env.API_TIMEOUT_MS = '45000' + delete process.env.CLAUDE_CODE_REMOTE + expect(getNonstreamingFallbackTimeoutMs()).toBe(45_000) + + process.env.CLAUDE_CODE_REMOTE = '1' + expect(getNonstreamingFallbackTimeoutMs()).toBe(45_000) +}) + +test('non-streaming fallback clamps an oversized override', () => { + process.env.API_TIMEOUT_MS = '3000000000' + delete process.env.CLAUDE_CODE_REMOTE + expect(getNonstreamingFallbackTimeoutMs()).toBe(2_147_483_647) +}) diff --git a/src/services/api/claude.ts b/src/services/api/claude.ts index e3c8d1a2a..ed12d01ca 100644 --- a/src/services/api/claude.ts +++ b/src/services/api/claude.ts @@ -113,7 +113,7 @@ import { extractQuotaStatusFromHeaders, } from '../claudeAiLimits.js' import { getAPIContextManagement } from '../compact/apiMicrocompact.js' -import { getStreamIdleTimeoutMs } from './openaiShim.js' +import { getApiTimeoutMs, getStreamIdleTimeoutMs } from './openaiShim.js' /* eslint-disable @typescript-eslint/no-require-imports */ const autoModeStateModule = feature('TRANSCRIPT_CLASSIFIER') @@ -897,10 +897,14 @@ function shouldDeferLspTool(tool: Tool): boolean { * Otherwise defaults to 300s — long enough for slow backends without * approaching the API's 10-minute non-streaming boundary. */ -function getNonstreamingFallbackTimeoutMs(): number { - const override = parseInt(process.env.API_TIMEOUT_MS || '', 10) - if (override) return override - return isEnvTruthy(process.env.CLAUDE_CODE_REMOTE) ? 120_000 : 300_000 +export function getNonstreamingFallbackTimeoutMs(): number { + // Validate and clamp API_TIMEOUT_MS through the shared parser so a malformed + // or negative override cannot yield a NaN/negative timeout; fall back to the + // context-specific default (shorter on remote to stay under CCR's idle-kill). + return getApiTimeoutMs( + process.env, + isEnvTruthy(process.env.CLAUDE_CODE_REMOTE) ? 120_000 : 300_000, + ) } /** diff --git a/src/services/api/client.test.ts b/src/services/api/client.test.ts index 13696df80..8ee68d851 100644 --- a/src/services/api/client.test.ts +++ b/src/services/api/client.test.ts @@ -1,3 +1,4 @@ +import Anthropic from '@anthropic-ai/sdk' import { afterEach, beforeEach, expect, mock, test } from 'bun:test' import { acquireSharedMutationLock, releaseSharedMutationLock } from '../../test/sharedMutationLock.js' import { @@ -83,6 +84,8 @@ const originalEnv = { MIMO_API_KEY: process.env.MIMO_API_KEY, VENICE_API_KEY: process.env.VENICE_API_KEY, FIREWORKS_API_KEY: process.env.FIREWORKS_API_KEY, + NEARAI_API_KEY: process.env.NEARAI_API_KEY, + CLINE_API_KEY: process.env.CLINE_API_KEY, LONGCAT_API_KEY: process.env.LONGCAT_API_KEY, AIMLAPI_API_KEY: process.env.AIMLAPI_API_KEY, APISMART_API_KEY: process.env.APISMART_API_KEY, @@ -201,6 +204,8 @@ beforeEach(async () => { delete process.env.MIMO_API_KEY delete process.env.VENICE_API_KEY delete process.env.FIREWORKS_API_KEY + delete process.env.NEARAI_API_KEY + delete process.env.CLINE_API_KEY delete process.env.LONGCAT_API_KEY delete process.env.AIMLAPI_API_KEY delete process.env.APISMART_API_KEY @@ -264,6 +269,8 @@ afterEach(() => { restoreEnv('MIMO_API_KEY', originalEnv.MIMO_API_KEY) restoreEnv('VENICE_API_KEY', originalEnv.VENICE_API_KEY) restoreEnv('FIREWORKS_API_KEY', originalEnv.FIREWORKS_API_KEY) + restoreEnv('NEARAI_API_KEY', originalEnv.NEARAI_API_KEY) + restoreEnv('CLINE_API_KEY', originalEnv.CLINE_API_KEY) restoreEnv('LONGCAT_API_KEY', originalEnv.LONGCAT_API_KEY) restoreEnv('AIMLAPI_API_KEY', originalEnv.AIMLAPI_API_KEY) restoreEnv('APISMART_API_KEY', originalEnv.APISMART_API_KEY) @@ -392,6 +399,79 @@ test('first-party Anthropic requests execute the configured fetch wrapper withou expect(capturedHeaders).toBeDefined() }) +test('native Anthropic client validates and clamps API_TIMEOUT_MS', async () => { + const originalApiTimeoutMs = process.env.API_TIMEOUT_MS + // Force the first-party native Anthropic constructor path. Every provider + // route-selection variable is cleared explicitly so an inherited value cannot + // divert getAnthropicClient into the shim and silently measure the wrong + // client's timeout. (The suite's afterEach restores all of these.) + delete process.env.CLAUDE_CODE_USE_OPENAI + delete process.env.CLAUDE_CODE_USE_BEDROCK + delete process.env.CLAUDE_CODE_USE_VERTEX + delete process.env.CLAUDE_CODE_USE_FOUNDRY + delete process.env.CLAUDE_CODE_USE_GITHUB + delete process.env.CLAUDE_CODE_USE_MISTRAL + delete process.env.CLAUDE_CODE_USE_GEMINI + delete process.env.GEMINI_API_KEY + delete process.env.GEMINI_MODEL + delete process.env.GEMINI_BASE_URL + delete process.env.GEMINI_AUTH_MODE + delete process.env.OPENAI_API_KEY + delete process.env.OPENAI_BASE_URL + delete process.env.OPENAI_API_BASE + delete process.env.OPENAI_MODEL + delete process.env.MINIMAX_API_KEY + delete process.env.XAI_API_KEY + delete process.env.MIMO_API_KEY + delete process.env.VENICE_API_KEY + delete process.env.FIREWORKS_API_KEY + delete process.env.NEARAI_API_KEY + delete process.env.CLINE_API_KEY + delete process.env.LONGCAT_API_KEY + delete process.env.AIMLAPI_API_KEY + delete process.env.APISMART_API_KEY + delete process.env.CONCENTRATE_API_KEY + delete process.env.NVIDIA_NIM + delete process.env.NVIDIA_API_KEY + delete process.env.ANTHROPIC_BASE_URL + delete process.env.ANTHROPIC_MODEL + process.env.ANTHROPIC_API_KEY = 'anthropic-test-key' + + try { + const timeoutOf = async (): Promise => { + const client = await getAnthropicClient({ + apiKey: 'anthropic-test-key', + maxRetries: 0, + model: 'claude-sonnet-4-6', + }) + // Guard the premise: only the native constructor exposes the SDK + // timeout. The OpenAI shim is returned as a cast object, so if a stray + // route-selection variable (or a future env-only route) diverts this + // call, fail here rather than silently reading undefined. + expect(client).toBeInstanceOf(Anthropic) + return (client as unknown as { timeout: number }).timeout + } + + // A malformed or negative override must not poison the client with a NaN or + // negative timeout; it falls back to the 600s default. + for (const invalid of ['not-a-number', '-1', '0', '1.5', '']) { + process.env.API_TIMEOUT_MS = invalid + expect(await timeoutOf()).toBe(600_000) + } + // Unset also yields the default. + delete process.env.API_TIMEOUT_MS + expect(await timeoutOf()).toBe(600_000) + // A valid override is honored, and an oversized one is clamped below the + // 32-bit setTimeout ceiling instead of overflowing to a tiny delay. + process.env.API_TIMEOUT_MS = '45000' + expect(await timeoutOf()).toBe(45_000) + process.env.API_TIMEOUT_MS = '3000000000' + expect(await timeoutOf()).toBe(2_147_483_647) + } finally { + restoreEnv('API_TIMEOUT_MS', originalApiTimeoutMs) + } +}) + test('routes a custom Anthropic endpoint with ANTHROPIC_AUTH_TOKEN without requiring an API key', async () => { let capturedUrl: string | undefined let capturedHeaders: Headers | undefined diff --git a/src/services/api/client.ts b/src/services/api/client.ts index ba3e585c3..b84f9dbac 100644 --- a/src/services/api/client.ts +++ b/src/services/api/client.ts @@ -58,6 +58,7 @@ import { type ProviderOverride, } from './authRouting.js' import { hasUsableOpenAICredential } from './credentialPool.js' +import { getApiTimeoutMs } from './openaiShim/transport.js' import { AnthropicVertex } from './vertexClient.js' import { importOptionalRuntimeModule } from '../../utils/optionalRuntimeModule.js' @@ -663,7 +664,7 @@ export async function getAnthropicClient({ const ARGS = { defaultHeaders, maxRetries, - timeout: parseInt(process.env.API_TIMEOUT_MS || String(600 * 1000), 10), + timeout: getApiTimeoutMs(), dangerouslyAllowBrowser: true, fetchOptions: getProxyFetchOptions({ forAnthropicAPI: true, @@ -686,7 +687,7 @@ export async function getAnthropicClient({ return createOpenAIShimClient({ defaultHeaders: safeHeaders, maxRetries, - timeout: parseInt(process.env.API_TIMEOUT_MS || String(600 * 1000), 10), + timeout: getApiTimeoutMs(), providerOverride, reasoningEffort: shimReasoningEffort, }) as unknown as Anthropic @@ -727,7 +728,7 @@ export async function getAnthropicClient({ return createOpenAIShimClient({ defaultHeaders, maxRetries, - timeout: parseInt(process.env.API_TIMEOUT_MS || String(600 * 1000), 10), + timeout: getApiTimeoutMs(), reasoningEffort: shimReasoningEffort, }) as unknown as Anthropic } diff --git a/src/services/api/openaiShim/transport.test.ts b/src/services/api/openaiShim/transport.test.ts index 7c5c79ffe..55bb4000f 100644 --- a/src/services/api/openaiShim/transport.test.ts +++ b/src/services/api/openaiShim/transport.test.ts @@ -41,6 +41,21 @@ test('API timeout parser can inspect a request-local environment', () => { expect(getApiTimeoutMs({})).toBe(600_000) }) +test('API timeout parser honors a caller-supplied fallback', () => { + // Native clients reuse this parser with their own context default, so an + // unset/malformed/negative override must resolve to the caller's fallback, + // never NaN or a negative timeout. + expect(getApiTimeoutMs({}, 300_000)).toBe(300_000) + for (const invalid of ['abc', '', '1.5', '25ms', '0', '-5']) { + expect(getApiTimeoutMs({ API_TIMEOUT_MS: invalid }, 120_000)).toBe(120_000) + } + // A valid override still wins over the fallback and stays capped. + expect(getApiTimeoutMs({ API_TIMEOUT_MS: '25' }, 120_000)).toBe(25) + expect(getApiTimeoutMs({ API_TIMEOUT_MS: '3000000000' }, 120_000)).toBe( + 2_147_483_647, + ) +}) + test('redacts configured and encoded secrets from diagnostic URLs', () => { process.env.OPENAI_API_KEY = 'secret/value' const diagnostic = redactUrlForDiagnostics( diff --git a/src/services/api/openaiShim/transport.ts b/src/services/api/openaiShim/transport.ts index 8bd4ab39a..3c1aec00c 100644 --- a/src/services/api/openaiShim/transport.ts +++ b/src/services/api/openaiShim/transport.ts @@ -65,13 +65,14 @@ export function preserveCallerAbortError( export function getApiTimeoutMs( processEnv: NodeJS.ProcessEnv = process.env, + fallbackMs: number = DEFAULT_API_TIMEOUT_MS, ): number { const raw = processEnv.API_TIMEOUT_MS?.trim() - if (!raw || !/^\d+$/.test(raw)) return DEFAULT_API_TIMEOUT_MS + if (!raw || !/^\d+$/.test(raw)) return fallbackMs const parsed = Number(raw) return Number.isSafeInteger(parsed) && parsed > 0 ? Math.min(parsed, MAX_API_TIMEOUT_MS) - : DEFAULT_API_TIMEOUT_MS + : fallbackMs } function combineRequestSignals(