diff --git a/CHANGELOG.md b/CHANGELOG.md index dd1baab..0a87c26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ versions adhere to [Semantic Versioning](https://semver.org). ## [Unreleased] +### Fixed + +- Hardened `/mcp` rate-limit env parsing: malformed or negative + `MCP_HTTP_RATE_LIMIT_*` values now fall back to safe defaults, and + over-large windows clamp before reaching express-rate-limit's + timer-backed MemoryStore. +- Made `scripts/wire-trace-tasks.sh` match the documented Cloud Run + deployment path: it now accepts `CLIENT_ID` / `CLIENT_SECRET` from + env first and falls back to both stack-specific and DEPLOY.md Secret + Manager names. + ## [1.6.0-beta.5] — 2026-05-20 CI + docs hygiene release on top of beta.4. No runtime behaviour diff --git a/DEPLOY.md b/DEPLOY.md index 833ad9f..cabecfc 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -52,8 +52,8 @@ If neither mode is configured, the server **refuses to start**. There's no path | `CAPSULE_MCP_READONLY` | optional | Set to `1` (or `true` / `yes` / `on`) to skip registering all write/delete tools at the MCP layer | | `CAPSULE_API_BASE_URL` | optional | Override the Capsule API base URL (default `https://api.capsulecrm.com/api/v2`). Useful for testing | | `MCP_HTTP_JSON_LIMIT` | optional | Inbound JSON body limit for `/mcp` (default `35mb`). Leaves headroom for the base64 expansion of a 25 MB attachment binary; bump if you raise the attachment cap | -| `MCP_HTTP_RATE_LIMIT_MAX` | optional | Max `/mcp` requests per client per window (default `600`). The connector tracks usage per authenticated client_id, so one runaway caller can't burn the shared Capsule quota | -| `MCP_HTTP_RATE_LIMIT_WINDOW_MS` | optional | Window in ms for the above (default `60000`) | +| `MCP_HTTP_RATE_LIMIT_MAX` | optional | Max `/mcp` requests per client per window (default `600`). The connector tracks usage per authenticated client_id, so one runaway caller can't burn the shared Capsule quota. Non-positive / malformed values fall back to the default | +| `MCP_HTTP_RATE_LIMIT_WINDOW_MS` | optional | Window in ms for the above (default `60000`). Non-positive / malformed values fall back to the default; values above Node's timer ceiling clamp before reaching express-rate-limit's in-memory store | | `MCP_HTTP_RATE_LIMIT_DISABLED` | optional | Set to `1` to disable the `/mcp` rate limiter entirely. Test-only | | `MCP_HTTP_DEBUG` | optional | Set to `1` to include full `err.message` in `/mcp` error logs (default: low-cardinality summary only, to avoid Capsule response bodies smearing across log aggregators) | | `MCP_HTTP_TRUST_PROXY` | optional | Integer 0..10 (default `1`). Number of proxy hops to trust when deriving the client IP for the unauthenticated-path rate limiter. `1` is correct for single-frontend deployments like Cloud Run. Set `2+` for multi-hop ingress (Cloudflare → Cloud Run). Set `0` for bare-IP deployments with no proxy in front, otherwise a client can spoof `X-Forwarded-For` to dodge their per-IP bucket. Authenticated `/mcp` is unaffected — it keys by `client_id` | diff --git a/HOWTO.md b/HOWTO.md index 71cfdba..0e7f907 100644 --- a/HOWTO.md +++ b/HOWTO.md @@ -11,7 +11,7 @@ npm install npm test ``` -460 tests, all mocked — no Capsule API calls happen, no token needed. The suite has three layers: +463 tests, all mocked — no Capsule API calls happen, no token needed. The suite has three layers: - **Per-tool unit tests** (e.g. `tests/parties.test.ts`): import the tool function, mock `undici.fetch`, assert on the URL, method, body, and response handling. Most tests live here. - **MCP-protocol integration tests** (`tests/mcp-integration.test.ts`): drive a real `McpServer` through the wire protocol via the SDK's in-memory transport pair, with `undici.fetch` still mocked. Catches the layer between "tool function works" and "MCP correctly registers and dispatches the tool". Includes the `get_attachment` content-type routing logic (which lives in `server.ts`, not the tool function). @@ -46,7 +46,7 @@ for the contributor-facing summary. npm run build ``` -Produces `dist/index.js` (stdio entry, ~145 KB, with `#!/usr/bin/env node` shebang and the executable bit set) and `dist/http.js` (HTTP entry, ~171 KB, no shebang). Each is fully self-contained — tsup runs as two separate configs so the stdio entry can be invoked directly via npx while the HTTP entry isn't a CLI. tsup target is Node 22 (undici 8 requires Node 22+ for the `webidl.util.markAsUncloneable` runtime API). +Produces `dist/index.js` (stdio entry, 144.74 KB, with `#!/usr/bin/env node` shebang and the executable bit set) and `dist/http.js` (HTTP entry, 171.21 KB, no shebang). Each is fully self-contained — tsup runs as two separate configs so the stdio entry can be invoked directly via npx while the HTTP entry isn't a CLI. tsup target is Node 22 (undici 8 requires Node 22+ for the `webidl.util.markAsUncloneable` runtime API). `npm run build` also chains `npm run build:icon` (`scripts/build-icon.mjs`), which regenerates `src/icon.ts` from the canonical `assets/icon.svg`. The TypeScript file is committed (so typecheck works without a build step) but is **generated** — edit the SVG, then run the build. A drift-guard test (`tests/icon-source.test.ts`) fails CI if the two ever fall out of sync. diff --git a/scripts/wire-trace-tasks.sh b/scripts/wire-trace-tasks.sh index 35824e6..6eb117e 100755 --- a/scripts/wire-trace-tasks.sh +++ b/scripts/wire-trace-tasks.sh @@ -23,7 +23,11 @@ # STACK=production REGION=europe-west1 PROJECT= \ # ./scripts/wire-trace-tasks.sh # -# Reads CLIENT_ID and CLIENT_SECRET from Secret Manager. +# Reads CLIENT_ID / CLIENT_SECRET from env when provided. If absent, +# falls back to Secret Manager: +# CLIENT_ID_SECRET (default: capsulemcp-${STACK}-oauth-client-id) +# CLIENT_SECRET_SECRET (defaults: capsulemcp-${STACK}-oauth-client-secret, +# then DEPLOY.md's capsulemcp-client-secret) # # Exits 0 if the lifecycle round-trips cleanly, 1 otherwise. @@ -50,8 +54,36 @@ PROJECT_NUMBER=$(gcloud projects describe "${PROJECT}" --format='value(projectNu URL="https://${SERVICE}-${PROJECT_NUMBER}.${REGION}.run.app" TAG_NAME="mcp-tasks-trace-$(date +%Y%m%d-%H%M%S)" -CLIENT_ID=$(gcloud secrets versions access latest --secret="capsulemcp-${STACK}-oauth-client-id") -CLIENT_SECRET=$(gcloud secrets versions access latest --secret="capsulemcp-${STACK}-oauth-client-secret") +read_secret() { + gcloud secrets versions access latest --secret="$1" 2>/dev/null +} + +CLIENT_ID="${CLIENT_ID:-${MCP_OAUTH_CLIENT_ID:-}}" +CLIENT_SECRET="${CLIENT_SECRET:-${MCP_OAUTH_CLIENT_SECRET:-}}" + +if [ -z "$CLIENT_ID" ]; then + CLIENT_ID_SECRET="${CLIENT_ID_SECRET:-capsulemcp-${STACK}-oauth-client-id}" + if ! CLIENT_ID=$(read_secret "$CLIENT_ID_SECRET"); then + echo "error: CLIENT_ID is not set and Secret Manager secret '$CLIENT_ID_SECRET' was not readable" >&2 + echo " Set CLIENT_ID=... (DEPLOY.md keeps client_id as a Cloud Run env var, not a secret)." >&2 + exit 1 + fi +fi + +if [ -z "$CLIENT_SECRET" ]; then + if [ -n "${CLIENT_SECRET_SECRET:-}" ]; then + if ! CLIENT_SECRET=$(read_secret "$CLIENT_SECRET_SECRET"); then + echo "error: CLIENT_SECRET_SECRET='$CLIENT_SECRET_SECRET' was not readable" >&2 + exit 1 + fi + elif ! CLIENT_SECRET=$(read_secret "capsulemcp-${STACK}-oauth-client-secret"); then + if ! CLIENT_SECRET=$(read_secret "capsulemcp-client-secret"); then + echo "error: CLIENT_SECRET is not set and no default Secret Manager secret was readable" >&2 + echo " Tried 'capsulemcp-${STACK}-oauth-client-secret' and 'capsulemcp-client-secret'." >&2 + exit 1 + fi + fi +fi PASS=0 FAIL=0 diff --git a/src/http/app.ts b/src/http/app.ts index c9f6f76..3e59111 100644 --- a/src/http/app.ts +++ b/src/http/app.ts @@ -20,6 +20,7 @@ import { import { requireBearerAuth } from "@modelcontextprotocol/sdk/server/auth/middleware/bearerAuth.js"; import { SUPPORTED_PROTOCOL_VERSIONS } from "@modelcontextprotocol/sdk/types.js"; import type { OAuthProvider } from "../auth/provider.js"; +import { readPositiveInt } from "../env.js"; import { createCapsuleMcpServer } from "../server.js"; import { ICON_SVG } from "../icon.js"; import { withRequestContext } from "../log.js"; @@ -57,6 +58,30 @@ function timingSafeSecretEqual(provided: string, expected: string): boolean { return timingSafeEqual(secretDigest(provided), secretDigest(expected)); } +const DEFAULT_MCP_RATE_LIMIT_WINDOW_MS = 60_000; +const DEFAULT_MCP_RATE_LIMIT_MAX = 600; +const MAX_MEMORY_STORE_WINDOW_MS = 2 ** 31 - 1; + +export function resolveMcpRateLimitConfig(): { + windowMs: number; + limit: number; + disabled: boolean; +} { + // express-rate-limit's default MemoryStore backs `windowMs` with + // setInterval, so over-large or negative values get coerced by Node + // timers after only a logged validation error. Parse defensively here + // so operator typos fall back or clamp before they reach the store. + const windowMs = Math.min( + readPositiveInt("MCP_HTTP_RATE_LIMIT_WINDOW_MS", DEFAULT_MCP_RATE_LIMIT_WINDOW_MS), + MAX_MEMORY_STORE_WINDOW_MS, + ); + return { + windowMs, + limit: readPositiveInt("MCP_HTTP_RATE_LIMIT_MAX", DEFAULT_MCP_RATE_LIMIT_MAX), + disabled: process.env["MCP_HTTP_RATE_LIMIT_DISABLED"] === "1", + }; +} + export function createApp(opts: AppOptions): express.Express { const { oauthProvider, issuerUrl, jsonLimit, allowedOrigins } = opts; const resourceName = opts.resourceName ?? "Capsule CRM MCP"; @@ -212,9 +237,11 @@ export function createApp(opts: AppOptions): express.Express { // that a runaway loop trips before the upstream 4000-rph cap. // Operators on heavy-tenant deployments can override via env. Tests // disable it via MCP_HTTP_RATE_LIMIT_DISABLED. - const rateLimitWindowMs = Number(process.env["MCP_HTTP_RATE_LIMIT_WINDOW_MS"]) || 60_000; - const rateLimitMax = Number(process.env["MCP_HTTP_RATE_LIMIT_MAX"]) || 600; - const rateLimitDisabled = process.env["MCP_HTTP_RATE_LIMIT_DISABLED"] === "1"; + const { + windowMs: rateLimitWindowMs, + limit: rateLimitMax, + disabled: rateLimitDisabled, + } = resolveMcpRateLimitConfig(); const mcpRateLimit = rateLimit({ windowMs: rateLimitWindowMs, limit: rateLimitMax, diff --git a/tests/http-app.test.ts b/tests/http-app.test.ts index 1836d2c..b8fda7a 100644 --- a/tests/http-app.test.ts +++ b/tests/http-app.test.ts @@ -11,11 +11,11 @@ * of that or mock undici.fetch. */ -import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; import type { Server } from "node:http"; import type { AddressInfo } from "node:net"; import { createHash } from "node:crypto"; -import { createApp } from "../src/http/app.js"; +import { createApp, resolveMcpRateLimitConfig } from "../src/http/app.js"; import { OAuthProvider, FixedClientStore, InMemoryClientsStore } from "../src/auth/provider.js"; vi.mock("undici", () => ({ fetch: vi.fn() })); @@ -563,6 +563,42 @@ describe("/mcp per-client rate limit", () => { }); }); +describe("/mcp rate-limit env parsing", () => { + const keys = [ + "MCP_HTTP_RATE_LIMIT_MAX", + "MCP_HTTP_RATE_LIMIT_WINDOW_MS", + "MCP_HTTP_RATE_LIMIT_DISABLED", + ]; + + afterEach(() => { + for (const key of keys) delete process.env[key]; + }); + + it("falls back on malformed or unsafe values before constructing express-rate-limit", () => { + process.env["MCP_HTTP_RATE_LIMIT_MAX"] = "-1"; + process.env["MCP_HTTP_RATE_LIMIT_WINDOW_MS"] = "-500"; + const cfg = resolveMcpRateLimitConfig(); + expect(cfg.limit).toBe(600); + expect(cfg.windowMs).toBe(60_000); + }); + + it("clamps windows above Node's timer ceiling", () => { + process.env["MCP_HTTP_RATE_LIMIT_WINDOW_MS"] = String(2 ** 31 + 1000); + expect(resolveMcpRateLimitConfig().windowMs).toBe(2 ** 31 - 1); + }); + + it("honours positive overrides and the explicit test-only disable flag", () => { + process.env["MCP_HTTP_RATE_LIMIT_MAX"] = "7"; + process.env["MCP_HTTP_RATE_LIMIT_WINDOW_MS"] = "12345"; + process.env["MCP_HTTP_RATE_LIMIT_DISABLED"] = "1"; + expect(resolveMcpRateLimitConfig()).toEqual({ + limit: 7, + windowMs: 12345, + disabled: true, + }); + }); +}); + describe("Icon endpoints (cosmetic)", () => { it("/icon.svg returns the SVG with correct content-type", async () => { const res = await fetch(`${baseUrl}/icon.svg`);