diff --git a/.changeset/brave-moons-repeat.md b/.changeset/brave-moons-repeat.md new file mode 100644 index 0000000..038ca7c --- /dev/null +++ b/.changeset/brave-moons-repeat.md @@ -0,0 +1,9 @@ +--- +"@call-e/cli": patch +--- + +Reject duration flags that are not plain numbers, and bound the ones backed by a timer. + +`--timeout-seconds 30s` used to resolve to `NaN`, which reached the MCP timeout arithmetic where `Math.max(NaN, 1000)` stays `NaN` and `setTimeout` substitutes 1ms, so every call aborted before it left. Durations are now validated where they are read, with the offending flag named in the error. + +The constraints are per option rather than shared: `--min-ttl-seconds 0` keeps working, since zero disables the minimum remaining-lifetime window, while the timeout flags stay strictly positive. Timer-backed values are also capped at 2147483 seconds, because `setTimeout` collapses any longer delay to 1ms and would reproduce the same immediate abort. diff --git a/.changeset/shaggy-taxis-shout.md b/.changeset/shaggy-taxis-shout.md new file mode 100644 index 0000000..ef89046 --- /dev/null +++ b/.changeset/shaggy-taxis-shout.md @@ -0,0 +1,7 @@ +--- +"@call-e/core": patch +--- + +Guard the MCP request timeout against a non-numeric duration. + +`Math.max(NaN, 1000)` is `NaN`, and `setTimeout(fn, NaN)` fires after 1ms, so an unreadable timeout aborted every request immediately instead of falling back to a usable one. diff --git a/packages/cli/lib/config.js b/packages/cli/lib/config.js index 32b1082..5379ef2 100644 --- a/packages/cli/lib/config.js +++ b/packages/cli/lib/config.js @@ -45,6 +45,39 @@ function firstOptionValue(value) { return Array.isArray(value) ? value[0] : value; } +// Node turns any setTimeout delay above this into 1ms, which brings back the very +// immediate-abort failure this validator exists to prevent: --timeout-seconds 2147484 +// is 2,147,484,000ms, so the request would abort at once instead of waiting. +// https://nodejs.org/api/timers.html#settimeoutcallback-delay-args +const MAX_TIMER_DELAY_MS = 2_147_483_647; +const MAX_TIMER_SECONDS = Math.floor(MAX_TIMER_DELAY_MS / 1000); + +// Zero is meaningful for --min-ttl-seconds (it disables the minimum remaining-lifetime +// window) and meaningless for a timeout, so the bounds are per option rather than shared. +function secondsOption(value, fallback, flag, { allowZero = false, timerBacked = true } = {}) { + const provided = firstOptionValue(value); + // Not `value || fallback`: a numeric 0 is falsy, and for --min-ttl-seconds it is a + // value the user asked for, not an absent one. + const raw = provided === undefined || provided === null || provided === "" ? fallback : provided; + const parsed = Number(raw); + const wanted = allowZero ? "a non-negative" : "a positive"; + + if (!Number.isFinite(parsed) || parsed < 0 || (!allowZero && parsed === 0)) { + throw new Error( + `${flag} expects ${wanted} number of seconds, got "${raw}". Use "30", not "30s".`, + ); + } + + if (timerBacked && parsed > MAX_TIMER_SECONDS) { + throw new Error( + `${flag} expects at most ${MAX_TIMER_SECONDS} seconds, got "${raw}". ` + + `Node collapses a longer timer to 1ms, which would abort immediately.`, + ); + } + + return parsed; +} + function isDisabledFlag(value) { return ["0", "false", "no", "off", "disabled"].includes(String(value || "").trim().toLowerCase()); } @@ -139,16 +172,28 @@ export function resolveRuntimeConfig(options = {}, env = process.env) { scope: options.scope || DEFAULT_SCOPE, clientName: options.clientName || DEFAULT_CLIENT_NAME, cacheRoot: expandHomePath(options.cacheRoot || DEFAULT_CACHE_ROOT), - timeoutSeconds: Number(options.timeoutSeconds || DEFAULT_TIMEOUT_SECONDS), - pollTimeoutSeconds: Number(options.pollTimeoutSeconds || DEFAULT_POLL_TIMEOUT_SECONDS), - minTtlSeconds: Number(options.minTtlSeconds || DEFAULT_MIN_TTL_SECONDS), + timeoutSeconds: secondsOption( + options.timeoutSeconds, + DEFAULT_TIMEOUT_SECONDS, + "--timeout-seconds", + ), + pollTimeoutSeconds: secondsOption( + options.pollTimeoutSeconds, + DEFAULT_POLL_TIMEOUT_SECONDS, + "--poll-timeout-seconds", + ), + // Not timer-backed, and 0 disables the minimum remaining-lifetime window. + minTtlSeconds: secondsOption(options.minTtlSeconds, DEFAULT_MIN_TTL_SECONDS, "--min-ttl-seconds", { + allowZero: true, + timerBacked: false, + }), serverName: options.serverName || DEFAULT_SERVER_NAME, telemetryEnabled: resolveTelemetryEnabled(options, env), telemetryUrl: resolveTelemetryUrl({ telemetryUrl: options.telemetryUrl, baseUrl }, env), - telemetryTimeoutSeconds: Number( - firstOptionValue(options.telemetryTimeoutSeconds) || - env.CALLE_TELEMETRY_TIMEOUT_SECONDS || - DEFAULT_TELEMETRY_TIMEOUT_SECONDS, + telemetryTimeoutSeconds: secondsOption( + firstOptionValue(options.telemetryTimeoutSeconds) || env.CALLE_TELEMETRY_TIMEOUT_SECONDS, + DEFAULT_TELEMETRY_TIMEOUT_SECONDS, + "--telemetry-timeout-seconds", ), }; } diff --git a/packages/cli/test/cli.test.js b/packages/cli/test/cli.test.js index f270bd7..c7761ba 100644 --- a/packages/cli/test/cli.test.js +++ b/packages/cli/test/cli.test.js @@ -6,7 +6,7 @@ import assert from "node:assert/strict"; import { POST_AUTH_HELP_MESSAGE, preAuthHelpMessage, runCli } from "../lib/cli.js"; import { pendingCachePath, tokenCachePath, writePrivateJson } from "../lib/cache.js"; -import { CLI_VERSION } from "../lib/config.js"; +import { CLI_VERSION, resolveRuntimeConfig } from "../lib/config.js"; const defaultIntegrationHeader = `cli/cli/${CLI_VERSION}`; @@ -1876,3 +1876,96 @@ test("telemetry opt-out flags and failures do not affect command output", async assert.equal(result.code, 0); assert.equal(JSON.parse(result.stdout).usable, false); }); + +test("resolveRuntimeConfig rejects durations that are not plain numbers", () => { + // "30s" and "1m" are how people write durations, and Number() turns both into + // NaN. Previously that NaN reached mcp-client's timeout arithmetic, where + // Math.max(NaN, 1000) stays NaN and setTimeout substitutes 1ms, so every MCP + // call aborted before it left. Fail at the edge instead, with the flag named. + for (const bad of ["30s", "1m", "abc", "-5", "0"]) { + assert.throws( + () => resolveRuntimeConfig({ timeoutSeconds: bad }), + /--timeout-seconds expects a positive number of seconds/, + `expected "${bad}" to be rejected`, + ); + } + + assert.throws( + () => resolveRuntimeConfig({ pollTimeoutSeconds: "5m" }), + /--poll-timeout-seconds expects a positive number of seconds/, + ); + assert.throws( + () => resolveRuntimeConfig({ minTtlSeconds: "60s" }), + /--min-ttl-seconds expects a non-negative number of seconds/, + ); + assert.throws( + () => resolveRuntimeConfig({ telemetryTimeoutSeconds: "1.5s" }, {}), + /--telemetry-timeout-seconds expects a positive number of seconds/, + ); +}); + +test("resolveRuntimeConfig keeps --min-ttl-seconds 0 working", () => { + // Zero disables the minimum remaining-lifetime window, which is a documented + // way to use the flag. The duration validator is strictly positive, so sharing + // it across every setting would have taken that away. + assert.equal(resolveRuntimeConfig({ minTtlSeconds: "0" }).minTtlSeconds, 0); + assert.equal(resolveRuntimeConfig({ minTtlSeconds: 0 }).minTtlSeconds, 0); + + // Still not a free pass: a negative minimum is meaningless. + assert.throws( + () => resolveRuntimeConfig({ minTtlSeconds: "-1" }), + /--min-ttl-seconds expects a non-negative number of seconds/, + ); + + // And zero stays rejected where it would mean "abort immediately". + assert.throws( + () => resolveRuntimeConfig({ timeoutSeconds: "0" }), + /--timeout-seconds expects a positive number of seconds/, + ); +}); + +test("resolveRuntimeConfig rejects timer values Node would collapse to 1ms", () => { + // setTimeout stores its delay in a signed 32-bit int. Anything over + // 2,147,483,647ms is silently replaced by 1ms, so a very large --timeout-seconds + // recreated the same immediate-abort bug the validator was added to stop. + const maxSeconds = Math.floor(2_147_483_647 / 1000); // 2147483 + + assert.equal(resolveRuntimeConfig({ timeoutSeconds: String(maxSeconds) }).timeoutSeconds, maxSeconds); + + for (const flag of ["timeoutSeconds", "pollTimeoutSeconds"]) { + assert.throws( + () => resolveRuntimeConfig({ [flag]: String(maxSeconds + 1) }), + /expects at most 2147483 seconds/, + `expected ${flag} to reject ${maxSeconds + 1}`, + ); + } + + assert.throws( + () => resolveRuntimeConfig({ telemetryTimeoutSeconds: String(maxSeconds + 1) }, {}), + /expects at most 2147483 seconds/, + ); + + // --min-ttl-seconds never reaches setTimeout, so it is not bounded by it. + assert.equal( + resolveRuntimeConfig({ minTtlSeconds: String(maxSeconds + 1) }).minTtlSeconds, + maxSeconds + 1, + ); +}); + +test("resolveRuntimeConfig keeps accepting valid values and defaults", () => { + const explicit = resolveRuntimeConfig({ timeoutSeconds: "30" }); + assert.equal(explicit.timeoutSeconds, 30); + + const defaults = resolveRuntimeConfig({}, {}); + assert.equal(Number.isFinite(defaults.timeoutSeconds), true); + assert.equal(Number.isFinite(defaults.pollTimeoutSeconds), true); + assert.equal(Number.isFinite(defaults.minTtlSeconds), true); + assert.equal(Number.isFinite(defaults.telemetryTimeoutSeconds), true); + + // Falsy values still fall back to the default, as before. + assert.equal(resolveRuntimeConfig({ timeoutSeconds: "" }).timeoutSeconds, defaults.timeoutSeconds); + assert.equal( + resolveRuntimeConfig({ timeoutSeconds: undefined }).timeoutSeconds, + defaults.timeoutSeconds, + ); +}); diff --git a/packages/core/lib/mcp-client.js b/packages/core/lib/mcp-client.js index 8c45fc6..6620bb9 100644 --- a/packages/core/lib/mcp-client.js +++ b/packages/core/lib/mcp-client.js @@ -142,7 +142,7 @@ function accessTokenFromCache(config) { async function openMcpSession({ config, fetchImpl }) { requireFetch(fetchImpl); const accessToken = accessTokenFromCache(config); - const timeoutMs = Math.max(Math.ceil(config.timeoutSeconds * 1000), 1000); + const timeoutMs = Math.max(Math.ceil(Number(config.timeoutSeconds || 15) * 1000), 1000); const commonHeaders = { Accept: "application/json, text/event-stream", "Content-Type": "application/json",