fix: harden /mcp rate-limit env parsing + wire-trace-tasks portability

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

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` |

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.
 

scripts/wire-trace-tasks.sh

@@ -23,7 +23,11 @@
 #   STACK=production REGION=europe-west1 PROJECT=<your-gcp-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

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,

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`);