feat(api): MCP proxy hardening — rate limit + real health probe (#200, #201) - #228
Conversation
The MCP credential proxy (apps/main/src/routes/mcp-proxy.ts) forwards session-authenticated calls to upstream MCP servers unbounded — one session, or several sessions sharing a tenant's vault credential, could hammer an upstream MCP server without any platform-side backstop (docs/mcp-credential-architecture.md's own "Rate limiting" future-work note flagged this gap). Add a per-tenant RL_MCP_PROXY_TENANT CF Rate Limiting binding (mcp:<tenantId> key, 300 req/60s), following the same pattern as RL_MAGICLINK_EMAIL / RL_SESSIONS_TENANT in apps/main/src/rate-limit.ts. The check lives inside forwardWithRefresh (now takes env as its first argument) rather than at each call site, so it covers all three callers uniformly: the HTTP /v1/mcp-proxy/:sid/:server endpoint and both McpProxyRpc RPC paths (mcpForward/fetch, outboundForward). Exceeding the budget returns 429 before any upstream traffic, still recorded through the existing mcp_proxy.forward audit log line (status 429, rate_limited: true) for observability. Fails open when the binding is absent, matching every other RL_* gate in this file. Keyed by tenant rather than session because the resource being protected (the vault credential) is shared tenant-wide — a per-session-only limit wouldn't catch several sessions each staying under an individual cap while collectively exhausting the upstream's own rate limit.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GET /v1/mcp-proxy/_health/:sid reported "ok" purely on credential resolution, never actually calling the upstream MCP server — so a green status could mask an unreachable/misconfigured/revoked upstream, exactly when a user is debugging why their agent's MCP tools aren't working. Add an opt-in `?probe=1` query param that performs a real upstream JSON-RPC round-trip per server — a `tools/list` POST matching the existing probeMcpServer/probeMcpEnabled precedent in routes/oauth.ts and packages/slack/src/provider.ts (same method, same 5s AbortController timeout, same "2xx only" success rule), run in parallel via Promise.allSettled so one hung upstream can't stall the others. Status vocabulary extends from "ok" | "unresolved" to "ok" | "unresolved" | "unreachable", with an added latency_ms on a completed probe. Default behavior (no ?probe=1) is unchanged: fast, free, presence-only check. The probe path reuses the same RL_MCP_PROXY_TENANT budget from #200 (one unit per server probed) so a health-page poller can't bypass the forward path's rate limit by hammering probes instead — when the budget's spent, that server quietly degrades to the presence-only "ok" rather than failing the whole response, since an observability endpoint going briefly stale is preferable to it erroring out. Updates AGENTS.md's Health check paragraph to document the new param and status values.
Summary
Two related hardening fixes to the MCP proxy (
apps/main/src/routes/mcp-proxy.ts), built on top of #221's mcp-server validation/redaction work.Fixes #200
Fixes #201
#200 — no rate limiting on the MCP proxy
RL_MCP_PROXY_TENANTCF Rate Limiting binding (mcp:<tenantId>key, 300 req/60s), following the exact pattern ofRL_MAGICLINK_EMAIL/RL_SESSIONS_TENANTinapps/main/src/rate-limit.ts.forwardWithRefresh(which now takesenvas its first argument) rather than duplicated per call site, so it covers all three callers uniformly: the HTTP/v1/mcp-proxy/:sid/:serverendpoint and bothMcpProxyRpcRPC paths (mcpForward/fetch,outboundForward).429before any upstream traffic. The blocked attempt is still recorded through the existingmcp_proxy.forwardaudit log line (status: 429, rate_limited: true) so it stays visible in the same observability stream as every other call.RL_*gate.docs/mcp-credential-architecture.md(a per-mcp:<tenantId>:<sessionId>sub-bucket was considered but deferred — flagging for a follow-up if production logs show one noisy session starving others on the same tenant).#201 —
_healthendpoint only checks credential presenceGET /v1/mcp-proxy/_health/:sidnow accepts?probe=1to perform a real upstream connectivity check per server — a cheap JSON-RPCtools/listPOST (matching the existingprobeMcpServer/probeMcpEnabledprecedent inroutes/oauth.tsandpackages/slack/src/provider.ts) with a 5s timeout, run in parallel viaPromise.allSettledso one hung upstream can't stall the response."ok" | "unresolved"to"ok" | "unresolved" | "unreachable", with an optionallatency_mson a completed probe.?probe=1) is unchanged — fast, free, presence-only check.RL_MCP_PROXY_TENANTbudget from security: no rate limiting on the MCP proxy — a single session can hammer any upstream MCP server unbounded #200 (one unit per server probed) so a health-page poller can't bypass the forward-path budget by hammering probes instead; when the budget's spent, that server quietly degrades to the presence-only"ok"rather than failing the whole health-check response.AGENTS.md's "Health check" paragraph anddocs/mcp-credential-architecture.mdupdated to match.main-node parity
apps/main-nodehas no per-tenant rate-limit binding of any kind today (confirmed via the existing// Node has no per-tenant rate-limit binding by default; soft-pass.comment inapps/main-node/src/index.ts) and no_healthroute for MCP at all. Per the task scope, I did not build new Node rate-limit/health infra — main-node's fail-open status quo is unchanged. Flagging here as a known gap rather than silently leaving it undocumented.Test plan
pnpm typecheck— cleanpnpm vitest run test/unit/mcp-proxy-refresh.test.ts— 6/6 passing (5 existing + 1 new rate-limit case)pnpm vitest run apps/main/src/routes/mcp-proxy-health.test.ts— probe test cases (added in the ux: MCP proxy _health endpoint only checks credential presence, not real upstream connectivity — misleading "ok" status #201 commit)pnpm test/ build run, per instructionshttps://claude.ai/code/session_01RJrW7pzndGtTitpKf1N3VT