From 2d0d35776c17ec4297d5bf1f134e18ee7d9d84f7 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 15 Oct 2026 13:16:09 +0200 Subject: [PATCH 1/2] fix(tasks): validate task options at registration Only sessionTtlMs, allowedOrigins and taskMaxRetainedPerPrincipal were checked at registration; taskLeaseMs, taskDefaultTtlMs and taskMaxTtlMs were used as given. A zero lease lapsed immediately and failed every task, and a NaN or negative ttl (e.g. Number(process.env.X) when unset) made every task expire at once, all surfacing only at runtime. Registration now requires each of them to be a positive safe integer and throws a TypeError naming the option otherwise. An explicit taskDefaultTtlMs above the resolved taskMaxTtlMs (default 1 hour) is also rejected, since it would be silently capped. Validation runs before any Redis connection is opened. Fixes #302 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UF4S3w7iA83V9DVsasLJzM --- README.md | 6 ++ src/handlers.ts | 2 +- src/index.ts | 14 +++++ test/task-options-validation.test.ts | 86 ++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 test/task-options-validation.test.ts diff --git a/README.md b/README.md index bfd15b35..4442b432 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,12 @@ A client-requested `ttl` must be a non-negative integer number of milliseconds; `ttl` is clamped between 1 second and `taskMaxTtlMs`, and the effective value is returned in the `CreateTaskResult`. +`taskDefaultTtlMs`, `taskMaxTtlMs` and `taskLeaseMs` must be positive integer numbers of +milliseconds, `taskMaxRetainedPerPrincipal` a positive integer, and `taskDefaultTtlMs` must not +exceed `taskMaxTtlMs` (default 1 hour). Registration throws a `TypeError` naming the option +otherwise, so a typo or a value read from an environment variable as a string fails at startup +rather than failing every task. + Tasks are stored in memory by default and in Redis when a `redis` option is given, so any instance can serve a poll for a task created on another. diff --git a/src/handlers.ts b/src/handlers.ts index 985240c9..bafd498e 100644 --- a/src/handlers.ts +++ b/src/handlers.ts @@ -923,7 +923,7 @@ const DEFAULT_POLL_INTERVAL = 1000 const DEFAULT_TASK_TTL = 60_000 /** Ceiling on retention, so a client cannot pin resources indefinitely */ -const MAX_TASK_TTL = 3600_000 +export const MAX_TASK_TTL = 3600_000 /** * Floor on retention. A smaller ttl (0 included) would let the task expire diff --git a/src/index.ts b/src/index.ts index a57f9b31..29e85ca7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -22,6 +22,7 @@ import oauthClientPlugin from './auth/oauth-client.ts' import authRoutesPlugin from './routes/auth-routes.ts' import { quitWithTimeout } from './redis-quit-with-timeout.ts' import { createJsonSchemaValidator } from './validation/json-schema-validator.ts' +import { MAX_TASK_TTL } from './handlers.ts' import { createMcpClient, type McpClient, @@ -102,6 +103,19 @@ const mcpPlugin = fp(async function (app: FastifyInstance, opts: MCPPluginOption // Checked before any Redis connection is opened, so a bad value cannot leak one validateTaskLimit('taskMaxRetainedPerPrincipal', opts.taskMaxRetainedPerPrincipal) } + // A zero lease lapses at once and a NaN or negative ttl expires every task, so + // reject them (and strings read from env variables) here rather than at runtime + for (const name of ['taskLeaseMs', 'taskDefaultTtlMs', 'taskMaxTtlMs'] as const) { + const value: unknown = opts[name] + if (value !== undefined && !(Number.isSafeInteger(value) && (value as number) > 0)) { + throw new TypeError(`${name} must be a positive integer number of milliseconds, got ${String(value)}`) + } + } + // An unset default is capped by the ceiling, but an explicit one above it is a mistake + const taskMaxTtlMs = opts.taskMaxTtlMs ?? MAX_TASK_TTL + if (opts.taskDefaultTtlMs !== undefined && opts.taskDefaultTtlMs > taskMaxTtlMs) { + throw new TypeError(`taskDefaultTtlMs (${opts.taskDefaultTtlMs}) must not exceed taskMaxTtlMs (${taskMaxTtlMs})`) + } const tools = new Map() const resources = new Map() const prompts = new Map() diff --git a/test/task-options-validation.test.ts b/test/task-options-validation.test.ts new file mode 100644 index 00000000..b487386e --- /dev/null +++ b/test/task-options-validation.test.ts @@ -0,0 +1,86 @@ +import { test, describe } from 'node:test' +import type { TestContext } from 'node:test' +import Fastify from 'fastify' +import mcpPlugin from '../src/index.ts' +import type { MCPPluginOptions } from '../src/types.ts' + +async function assertRegistrationRejects (t: TestContext, opts: Partial, pattern: RegExp): Promise { + const app = Fastify() + try { + await t.assert.rejects( + async () => { await app.register(mcpPlugin, { enableTasks: true, ...opts } as MCPPluginOptions) }, + (err: Error) => { + t.assert.ok(err instanceof TypeError, `expected a TypeError, got ${err}`) + t.assert.match(err.message, pattern) + return true + } + ) + } finally { + await app.close() + } +} + +async function assertRegistrationAccepts (t: TestContext, opts: Partial): Promise { + const app = Fastify() + try { + await app.register(mcpPlugin, { enableTasks: true, ...opts } as MCPPluginOptions) + await app.ready() + } finally { + await app.close() + } + t.assert.ok(true) +} + +// Values that would otherwise surface only at runtime: a lease that lapses at +// once, a ttl that is NaN or negative, or a string read from an env variable +const invalidValues: unknown[] = [0, -5, NaN, Infinity, -Infinity, 1.5, '30000', null, Number.MAX_SAFE_INTEGER + 1] + +describe('task options validation (#302)', () => { + for (const option of ['taskLeaseMs', 'taskDefaultTtlMs', 'taskMaxTtlMs'] as const) { + test(`registration throws a TypeError naming ${option} for invalid values`, async (t: TestContext) => { + for (const value of invalidValues) { + await assertRegistrationRejects(t, { [option]: value as any }, new RegExp(`^${option} must be a positive integer`)) + } + }) + + test(`registration throws for an invalid ${option} even when tasks are disabled`, async (t: TestContext) => { + await assertRegistrationRejects(t, { enableTasks: false, [option]: 0 }, new RegExp(`^${option} must be`)) + }) + + test(`registration accepts a positive integer or undefined ${option}`, async (t: TestContext) => { + await assertRegistrationAccepts(t, { [option]: undefined }) + await assertRegistrationAccepts(t, { [option]: 1000 }) + }) + } + + test('registration throws for an invalid taskMaxRetainedPerPrincipal', async (t: TestContext) => { + for (const value of [0, -1, NaN, 1.5, '20', null]) { + await assertRegistrationRejects(t, { taskMaxRetainedPerPrincipal: value as any }, /^taskMaxRetainedPerPrincipal must be a positive integer/) + } + await assertRegistrationAccepts(t, { taskMaxRetainedPerPrincipal: undefined }) + await assertRegistrationAccepts(t, { taskMaxRetainedPerPrincipal: 20 }) + }) + + test('registration throws when taskDefaultTtlMs exceeds taskMaxTtlMs', async (t: TestContext) => { + await assertRegistrationRejects(t, { taskDefaultTtlMs: 120_000, taskMaxTtlMs: 60_000 }, /taskDefaultTtlMs \(120000\) must not exceed taskMaxTtlMs \(60000\)/) + }) + + test('registration throws when taskDefaultTtlMs exceeds the default taskMaxTtlMs', async (t: TestContext) => { + await assertRegistrationRejects(t, { taskDefaultTtlMs: 2 * 3600_000 }, /taskDefaultTtlMs \(7200000\) must not exceed taskMaxTtlMs \(3600000\)/) + }) + + test('registration accepts taskDefaultTtlMs equal to taskMaxTtlMs', async (t: TestContext) => { + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 60_000, taskMaxTtlMs: 60_000 }) + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 15 * 60_000, taskMaxTtlMs: 60 * 60_000 }) + }) + + test('registration accepts a taskMaxTtlMs below the default taskDefaultTtlMs', async (t: TestContext) => { + // An unset taskDefaultTtlMs is capped by the ceiling, as before + await assertRegistrationAccepts(t, { taskMaxTtlMs: 30_000 }) + }) + + test('registration fails before connecting to Redis', async (t: TestContext) => { + // An unreachable port: had the plugin connected first, registration would hang or leak + await assertRegistrationRejects(t, { redis: { host: '127.0.0.1', port: 1, lazyConnect: true }, taskLeaseMs: 0 } as any, /^taskLeaseMs must be/) + }) +}) From afd1d9629bde7ec13a0687d801d60432744e0411 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 15 Oct 2026 16:11:56 +0200 Subject: [PATCH 2/2] fix(tasks): keep capping a taskDefaultTtlMs above taskMaxTtlMs Throwing when the default ttl exceeds the ceiling rejects configurations that work today (the default is silently capped), which would make this a breaking change. Keep the existing capping here and only reject values that already broke every task; enforcing the ceiling is left to a separate change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01UF4S3w7iA83V9DVsasLJzM --- README.md | 8 ++++---- src/handlers.ts | 2 +- src/index.ts | 6 ------ test/task-options-validation.test.ts | 9 +++------ 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 4442b432..c01ba993 100644 --- a/README.md +++ b/README.md @@ -382,10 +382,10 @@ A client-requested `ttl` must be a non-negative integer number of milliseconds; `CreateTaskResult`. `taskDefaultTtlMs`, `taskMaxTtlMs` and `taskLeaseMs` must be positive integer numbers of -milliseconds, `taskMaxRetainedPerPrincipal` a positive integer, and `taskDefaultTtlMs` must not -exceed `taskMaxTtlMs` (default 1 hour). Registration throws a `TypeError` naming the option -otherwise, so a typo or a value read from an environment variable as a string fails at startup -rather than failing every task. +milliseconds and `taskMaxRetainedPerPrincipal` a positive integer. Registration throws a +`TypeError` naming the option otherwise, so a typo or a value read from an environment variable +as a string fails at startup rather than failing every task. A `taskDefaultTtlMs` above +`taskMaxTtlMs` is capped to it. Tasks are stored in memory by default and in Redis when a `redis` option is given, so any instance can serve a poll for a task created on another. diff --git a/src/handlers.ts b/src/handlers.ts index bafd498e..985240c9 100644 --- a/src/handlers.ts +++ b/src/handlers.ts @@ -923,7 +923,7 @@ const DEFAULT_POLL_INTERVAL = 1000 const DEFAULT_TASK_TTL = 60_000 /** Ceiling on retention, so a client cannot pin resources indefinitely */ -export const MAX_TASK_TTL = 3600_000 +const MAX_TASK_TTL = 3600_000 /** * Floor on retention. A smaller ttl (0 included) would let the task expire diff --git a/src/index.ts b/src/index.ts index 29e85ca7..f255cb26 100644 --- a/src/index.ts +++ b/src/index.ts @@ -22,7 +22,6 @@ import oauthClientPlugin from './auth/oauth-client.ts' import authRoutesPlugin from './routes/auth-routes.ts' import { quitWithTimeout } from './redis-quit-with-timeout.ts' import { createJsonSchemaValidator } from './validation/json-schema-validator.ts' -import { MAX_TASK_TTL } from './handlers.ts' import { createMcpClient, type McpClient, @@ -111,11 +110,6 @@ const mcpPlugin = fp(async function (app: FastifyInstance, opts: MCPPluginOption throw new TypeError(`${name} must be a positive integer number of milliseconds, got ${String(value)}`) } } - // An unset default is capped by the ceiling, but an explicit one above it is a mistake - const taskMaxTtlMs = opts.taskMaxTtlMs ?? MAX_TASK_TTL - if (opts.taskDefaultTtlMs !== undefined && opts.taskDefaultTtlMs > taskMaxTtlMs) { - throw new TypeError(`taskDefaultTtlMs (${opts.taskDefaultTtlMs}) must not exceed taskMaxTtlMs (${taskMaxTtlMs})`) - } const tools = new Map() const resources = new Map() const prompts = new Map() diff --git a/test/task-options-validation.test.ts b/test/task-options-validation.test.ts index b487386e..2330872c 100644 --- a/test/task-options-validation.test.ts +++ b/test/task-options-validation.test.ts @@ -61,12 +61,9 @@ describe('task options validation (#302)', () => { await assertRegistrationAccepts(t, { taskMaxRetainedPerPrincipal: 20 }) }) - test('registration throws when taskDefaultTtlMs exceeds taskMaxTtlMs', async (t: TestContext) => { - await assertRegistrationRejects(t, { taskDefaultTtlMs: 120_000, taskMaxTtlMs: 60_000 }, /taskDefaultTtlMs \(120000\) must not exceed taskMaxTtlMs \(60000\)/) - }) - - test('registration throws when taskDefaultTtlMs exceeds the default taskMaxTtlMs', async (t: TestContext) => { - await assertRegistrationRejects(t, { taskDefaultTtlMs: 2 * 3600_000 }, /taskDefaultTtlMs \(7200000\) must not exceed taskMaxTtlMs \(3600000\)/) + test('registration accepts a taskDefaultTtlMs above taskMaxTtlMs, which is capped', async (t: TestContext) => { + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 120_000, taskMaxTtlMs: 60_000 }) + await assertRegistrationAccepts(t, { taskDefaultTtlMs: 2 * 3600_000 }) }) test('registration accepts taskDefaultTtlMs equal to taskMaxTtlMs', async (t: TestContext) => {