From 5f229d3ccd11f3349fd59f166ba8809eb2eb8800 Mon Sep 17 00:00:00 2001 From: FilipBl4gojevic <84703395+FilipBl4gojevic@users.noreply.github.com> Date: Fri, 31 Jul 2026 15:57:49 +0200 Subject: [PATCH] fix(builder): fail loud on malformed rate-limit and session env knobs; document them; refresh the transport diagram --- README.md | 57 +++++++------ packages/builder/src/env.ts | 34 ++++++++ packages/builder/src/index.ts | 35 ++++++-- packages/builder/src/vector/rate-limiter.ts | 11 ++- packages/builder/test/env.test.ts | 92 +++++++++++++++++++++ 5 files changed, 199 insertions(+), 30 deletions(-) create mode 100644 packages/builder/src/env.ts create mode 100644 packages/builder/test/env.test.ts diff --git a/README.md b/README.md index 4b13e6b..53acf8e 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,9 @@ Hosted instances run on both networks, exposing all 24 tools: > 11, "Rollout", for the full history). **Both hosted instances currently require a bearer > token from the operators to connect** - that is an access control, not a custody one: > neither instance accepts a mnemonic from any caller, token or not. Open public access -> (per-IP rate limiting, no token required) is a planned follow-up, not yet shipped. Self-host +> (per-IP rate limiting, no token required) is a planned follow-up, not yet enabled on the +> hosted instances - the per-IP rate-limiting code ships in this release; only the ops action +> of unsetting `MCP_AUTH_TOKENS` remains, coordinated separately after this deploys. Self-host > this release for tokenless access today. > **Transport: the hosted URLs above still answer on `/sse` only.** This codebase now serves @@ -245,9 +247,17 @@ If this instance will be reachable by anyone but you, set `MCP_AUTH_TOKENS` firs | `VECTOR_KOIOS_URL` | Koios REST API endpoint | `https://v2.koios.vector.testnet.apexfusion.org/` | | `VECTOR_SUBMIT_URL` | Transaction submit API | `https://submit.vector.testnet.apexfusion.org/api/submit/tx` | | `VECTOR_EXPLORER_URL` | Block explorer base URL | `https://vector.testnet.apexscan.org` | -| `VECTOR_RATE_LIMIT_PER_MINUTE` | Max tool calls per minute | `60` | +| `VECTOR_RATE_LIMIT_PER_MINUTE` | Max tool calls per minute, per identity (min `1`) | `60` | +| `VECTOR_MCP_SESSION_IDLE_MS` | `/mcp` session idle timeout before the reaper closes it, in ms (min `100`) | `600000` (10 min) | +| `VECTOR_MCP_SESSION_SWEEP_MS` | How often the `/mcp` idle reaper sweeps, in ms (min `100`) | `60000` (1 min) | +| `VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY` | Max concurrent `/mcp` sessions per identity before the oldest is evicted (min `1`) | `32` | | `MCP_AUTH_TOKENS` | Bearer tokens that may call this server. Comma-separated; each entry is `label:token` or a bare token. **When unset, the server is open to anyone who can reach it.** | _(unset — auth disabled)_ | +The four numeric knobs above fail loudly at startup on a malformed value: anything that is not +a plain integer, or is below its listed minimum, raises a startup error naming the variable and +the value it got, rather than silently falling back to some other behavior (an unparseable rate +limit silently disabling rate limiting, for example). + This server has no spend-limit or audit-log configuration of its own: it holds no key material for any family, so it has nothing left to limit. Every spend limit, and the audit log recording it, lives in your local signer instead - see `VECTOR_SIGNER_SPEND_LIMIT_PER_TX` / @@ -261,9 +271,10 @@ it, lives in your local signer instead - see `VECTOR_SIGNER_SPEND_LIMIT_PER_TX` > eviction resets a bucket's budget - a memory bound, not a security boundary, since an > attacker rotating enough source IPs can still defeat per-IP limiting at a tier only the > reverse proxy or network layer can police. `/mcp` sessions get the same per-identity -> treatment: idle sessions are reaped after `VECTOR_MCP_SESSION_IDLE_MS` (default 10 minutes), -> and concurrent sessions per identity are capped at `VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY` -> (default 32). +> treatment: idle sessions are reaped (`VECTOR_MCP_SESSION_IDLE_MS`, swept every +> `VECTOR_MCP_SESSION_SWEEP_MS`), and concurrent sessions per identity are capped +> (`VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY`) - see the Configuration table above for defaults and +> minimums. > **Malformed values fail loudly, not silently.** The server refuses to start if > `MCP_AUTH_TOKENS` contains an empty token, a token with embedded whitespace, a @@ -323,24 +334,24 @@ against live chain data. **Never submits.** Never runs in CI. See ## Architecture ``` -┌──────────────────────┐ ┌──────────────────────────┐ -│ Claude / GPT / etc. │◄────►│ vector-mcp-server │ -│ (any MCP client) │ SSE │ │ -└──────────────────────┘ │ ┌────────────────────┐ │ - │ │ Rate Limiter │ │ - │ │ (60 calls/min) │ │ - │ └────────┬───────────┘ │ - │ │ │ - │ ┌────────▼───────────┐ │ - │ │ Lucid + Ogmios │ │ - │ │ Provider │ │ - │ └────────┬───────────┘ │ - │ │ │ - │ ┌────────▼───────────┐ │ - │ │ Ogmios / Koios / │ │ - │ │ Submit API │ │ - │ └────────────────────┘ │ - └──────────────────────────┘ +┌──────────────────────┐ ┌──────────────────────────┐ +│ Claude / GPT / etc. │◄───────────►│ vector-mcp-server │ +│ (any MCP client) │ /sse + /mcp │ │ +└──────────────────────┘ │ ┌────────────────────┐ │ + │ │ Rate Limiter │ │ + │ │ (60 calls/min) │ │ + │ └────────┬───────────┘ │ + │ │ │ + │ ┌────────▼───────────┐ │ + │ │ Lucid + Ogmios │ │ + │ │ Provider │ │ + │ └────────┬───────────┘ │ + │ │ │ + │ ┌────────▼───────────┐ │ + │ │ Ogmios / Koios / │ │ + │ │ Submit API │ │ + │ └────────────────────┘ │ + └──────────────────────────┘ ``` No safety layer sits between the rate limiter and the provider: this server holds no key diff --git a/packages/builder/src/env.ts b/packages/builder/src/env.ts new file mode 100644 index 0000000..4376b5f --- /dev/null +++ b/packages/builder/src/env.ts @@ -0,0 +1,34 @@ +// Fail-loud parsing for integer environment variables that bound resource +// usage: rate limits, session timers, session caps. A malformed value must +// never silently degrade or disable the bound it configures - the same +// philosophy loadAuthConfig (auth.ts) already applies to MCP_AUTH_TOKENS, +// applied here to the numeric knobs instead. +// +// Pulled into its own file, rather than written inline in index.ts, for one +// concrete reason: index.ts is an executable entrypoint (importing it boots +// a real server as a side effect - see MAX_SESSIONS_PER_IDENTITY's comment +// there), so nothing defined at its module scope can be unit-tested by +// importing it directly. This function has no side effects of its own and +// takes an explicit `env` parameter (default `process.env`), matching +// loadAuthConfig's own testability pattern, so it - and every knob that +// calls it - can be verified directly, in isolation, the same way +// loadAuthConfig is. +export function intEnv( + name: string, + def: number, + min: number, + env: Record = process.env +): number { + const raw = env[name]; + if (raw === undefined) return def; + + // Number(), not parseInt: parseInt('10m') silently truncates to 10, so a + // typo ships a working-looking-but-wrong bound instead of failing to + // start. Number('10m') is NaN, which fails the isInteger check below, so + // trailing garbage is rejected rather than misread. + const n = Number(raw); + if (!Number.isInteger(n) || n < min) { + throw new Error(`${name} must be an integer >= ${min}, got ${JSON.stringify(raw)}.`); + } + return n; +} diff --git a/packages/builder/src/index.ts b/packages/builder/src/index.ts index 1561fd6..3508a06 100644 --- a/packages/builder/src/index.ts +++ b/packages/builder/src/index.ts @@ -29,6 +29,8 @@ import { SSEServerTransport } from "@modelcontextprotocol/sdk/server/sse.js"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; import { registerVectorTools } from "./vector/index.js"; import { loadAuthConfig, resolveIdentity, clientIpOf } from "./auth.js"; +import { intEnv } from "./env.js"; +import { RATE_LIMIT_PER_MINUTE } from "./vector/rate-limiter.js"; function createMcpServer(identity: string) { const server = new McpServer({ @@ -74,18 +76,39 @@ const streamableLastSeen = new Map(); // vector/rate-limiter.ts. Exported for visibility/tests; production tuning // is via the env var, since index.ts is an executable entrypoint (importing // it boots a real server as a side effect) rather than something a test can -// import and reconfigure in-process. -export const MAX_SESSIONS_PER_IDENTITY = parseInt(process.env.VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY || '32'); +// import and reconfigure in-process. Parsed with intEnv (env.ts), not a bare +// parseInt: a malformed value must fail loud at startup, not silently +// disable the cap - parseInt('thirty-two') is NaN, and every +// `length >= MAX_SESSIONS_PER_IDENTITY` comparison in the eviction check +// below is false whenever the right side is NaN, so the cap would silently +// stop evicting anything, forever. +export const MAX_SESSIONS_PER_IDENTITY = intEnv('VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY', 32, 1); // Idle bound: a session with no request for this long is reaped on the next // sweep, regardless of whether DELETE ever arrives. Default 10 minutes - // generous for a real client mid-conversation, short enough that an -// abandoned session does not outlive the process. -const STREAMABLE_SESSION_IDLE_MS = parseInt(process.env.VECTOR_MCP_SESSION_IDLE_MS || String(10 * 60 * 1000)); +// abandoned session does not outlive the process. intEnv, not parseInt, for +// the same fail-loud reason as above - and the failure mode here is worse in +// both directions: a fully non-numeric value is NaN, and `now - lastSeen > +// NaN` is always false, so the reaper would run on schedule forever while +// reaping nothing; a value with a numeric prefix and trailing garbage +// (e.g. a typo like '10m' meaning "10 minutes") would have parseInt-truncated +// to 10, reaping every session within 10ms of creation instead. +const STREAMABLE_SESSION_IDLE_MS = intEnv('VECTOR_MCP_SESSION_IDLE_MS', 10 * 60 * 1000, 100); // Sweep cadence, kept separate from the idle bound so a hermetic test can // shrink both independently without waiting anywhere near the real idle -// window. -const STREAMABLE_SESSION_SWEEP_MS = parseInt(process.env.VECTOR_MCP_SESSION_SWEEP_MS || '60000'); +// window. Same truncation risk as the idle bound: a typo like '60s' would +// have parseInt-truncated to 60, running the sweep every 60ms instead of +// every 60 seconds. +const STREAMABLE_SESSION_SWEEP_MS = intEnv('VECTOR_MCP_SESSION_SWEEP_MS', 60_000, 100); + +// Operators verify what's actually active from this one line - every DoS +// bound this file and rate-limiter.ts enforce, with the effective value each +// resolved to (default or override), not just the fact that they exist. +console.error( + `Config: rate limit ${RATE_LIMIT_PER_MINUTE}/min per identity, /mcp session idle ${STREAMABLE_SESSION_IDLE_MS}ms, ` + + `sweep every ${STREAMABLE_SESSION_SWEEP_MS}ms, cap ${MAX_SESSIONS_PER_IDENTITY} sessions/identity.` +); /** * Closes a /mcp session and drains it from every map, from any trigger other diff --git a/packages/builder/src/vector/rate-limiter.ts b/packages/builder/src/vector/rate-limiter.ts index 7e8c063..084cd2f 100644 --- a/packages/builder/src/vector/rate-limiter.ts +++ b/packages/builder/src/vector/rate-limiter.ts @@ -1,5 +1,7 @@ // Sliding-window rate limiter for Vector MCP tools +import { intEnv } from '../env.js'; + export class RateLimiter { private calls: number[] = []; private readonly windowMs: number; @@ -23,7 +25,14 @@ export class RateLimiter { } } -const RATE_LIMIT_PER_MINUTE = parseInt(process.env.VECTOR_RATE_LIMIT_PER_MINUTE || '60'); +// Parsed with intEnv (env.ts), not a bare parseInt: a malformed value must +// fail loud at startup, not silently disable rate limiting. parseInt('sixty') +// is NaN, and RateLimiter.check() above compares `calls.length >= maxCalls` - +// any comparison against NaN is false, so a typo here would have let every +// caller through with no limit at all, silently. Exported so index.ts's +// startup log can report the effective value operators are actually running +// under, alongside the /mcp session knobs. +export const RATE_LIMIT_PER_MINUTE = intEnv('VECTOR_RATE_LIMIT_PER_MINUTE', 60, 1); // One bucket per identity. A process-global limiter meant one busy caller // throttled everyone on the box; keying by identity fixes that. The registry diff --git a/packages/builder/test/env.test.ts b/packages/builder/test/env.test.ts new file mode 100644 index 0000000..9e776f7 --- /dev/null +++ b/packages/builder/test/env.test.ts @@ -0,0 +1,92 @@ +import { describe, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { intEnv } from '../src/env.ts'; + +// One case per real production call site: exact (name, default, min) triple +// as actually passed in index.ts / rate-limiter.ts, plus a validOverride +// already exercised by the integration suite for that var (see +// streamable-http-gating.test.ts / rate-limit-gating.test.ts) - proof this +// function accepts the values those hermetic tests already depend on, not +// just values chosen for this file. Keep these four rows in sync with the +// real call sites if either changes. +const KNOBS: Array<{ name: string; def: number; min: number; validOverride: string }> = [ + { name: 'VECTOR_MCP_SESSION_IDLE_MS', def: 10 * 60 * 1000, min: 100, validOverride: '400' }, + { name: 'VECTOR_MCP_SESSION_SWEEP_MS', def: 60_000, min: 100, validOverride: '100' }, + { name: 'VECTOR_MCP_MAX_SESSIONS_PER_IDENTITY', def: 32, min: 1, validOverride: '2' }, + { name: 'VECTOR_RATE_LIMIT_PER_MINUTE', def: 60, min: 1, validOverride: '3' }, +]; + +for (const { name, def, min, validOverride } of KNOBS) { + describe(`intEnv - ${name} (default ${def}, min ${min})`, () => { + test('uses the default when unset', () => { + assert.equal(intEnv(name, def, min, {}), def); + }); + + test('accepts a valid override (the exact value the integration suite uses for this var)', () => { + assert.equal(intEnv(name, def, min, { [name]: validOverride }), Number(validOverride)); + }); + + test('accepts the min value itself (inclusive boundary)', () => { + assert.equal(intEnv(name, def, min, { [name]: String(min) }), min); + }); + + test('rejects a non-integer value, throwing and naming the var', () => { + // A fully non-numeric string (Number(...) -> NaN), a real fractional + // number, and a numeric-prefix-plus-garbage string (the specific shape + // a bare parseInt would have silently truncated instead of rejecting - + // e.g. parseInt('100garbage') === 100). + for (const bad of ['not-a-number', `${min}.5`, `${min}garbage`, 'Infinity']) { + assert.throws( + () => intEnv(name, def, min, { [name]: bad }), + new RegExp(name), + `must throw naming ${name} for value ${JSON.stringify(bad)}` + ); + } + }); + + test('rejects a below-min value, throwing and naming the var', () => { + for (const bad of [String(min - 1), '-1']) { + assert.throws( + () => intEnv(name, def, min, { [name]: bad }), + new RegExp(name), + `must throw naming ${name} for value ${JSON.stringify(bad)}` + ); + } + }); + }); +} + +describe('intEnv - generic behavior', () => { + test('the thrown message names the var and echoes the offending raw value (never silently swallowed)', () => { + assert.throws( + () => intEnv('VECTOR_MCP_SESSION_IDLE_MS', 600_000, 100, { VECTOR_MCP_SESSION_IDLE_MS: '10m' }), + (err: unknown) => { + assert.ok(err instanceof Error); + assert.match(err.message, /VECTOR_MCP_SESSION_IDLE_MS/); + assert.match(err.message, /10m/); + return true; + } + ); + }); + + test('an explicitly empty value is not treated as "unset" - it is parsed (Number(\'\') is 0) and rejected by any min >= 1', () => { + // Distinguishes "unset" (env[name] === undefined -> default) from + // "set to an empty string" (env[name] === '' -> parsed, not defaulted). + // Every real knob's min is >= 1, so this always throws for them - but + // the throw must come from failing the min check, not from treating '' + // as somehow equivalent to undefined. + assert.throws(() => intEnv('VECTOR_RATE_LIMIT_PER_MINUTE', 60, 1, { VECTOR_RATE_LIMIT_PER_MINUTE: '' }), /VECTOR_RATE_LIMIT_PER_MINUTE/); + }); + + test('defaults to process.env when no explicit env object is passed (matches loadAuthConfig\'s own testability pattern in auth.ts)', () => { + const PROBE = 'VECTOR_TEST_INT_ENV_PROBE__DO_NOT_USE_ELSEWHERE'; + const prior = process.env[PROBE]; + process.env[PROBE] = '42'; + try { + assert.equal(intEnv(PROBE, 1, 1), 42); + } finally { + if (prior === undefined) delete process.env[PROBE]; + else process.env[PROBE] = prior; + } + }); +});