mirror of
https://github.com/Sea-Haven-Industries/sh-mcp.git
synced 2026-10-07 06:58:59 +00:00
Address CodeQL findings: bound ajv error work + edge rate limiting
GHAS code-scanning alerts on this PR: - dispatch.ts (js/resource-exhaustion): ajv ran with allErrors:true on untrusted input, letting a crafted payload force unbounded error enumeration. Switch to allErrors:false (default) so validation short-circuits on the first failure; the 400 still names that path. - http.ts (js/missing-rate-limiting): the authenticated routes (/mcp, /tools/:name) had no edge throttle — auth/JWT verification ran on every request before the per-sub dispatch limiter could apply. Add an IP-keyed express-rate-limit in front of authenticate (120/60s default, configurable), returning the standard 429 shape. Defense-in-depth over the per-sub + per-tool limiter in executeTool; API GW/WAF remains the production edge. Tests: +2 cases proving the edge limiter throttles before auth (429, not 401) on /tools and /mcp. 407 pass; tsc/eslint/prettier clean.
This commit is contained in:
parent
d4574309bc
commit
00762de0a6
5 changed files with 62 additions and 6 deletions
1
package-lock.json
generated
1
package-lock.json
generated
|
|
@ -8483,6 +8483,7 @@
|
|||
"ajv": "8.20.0",
|
||||
"ajv-formats": "3.0.1",
|
||||
"express": "5.2.1",
|
||||
"express-rate-limit": "^8.2.1",
|
||||
"jose": "^6.2.3"
|
||||
},
|
||||
"devDependencies": {
|
||||
|
|
|
|||
|
|
@ -34,6 +34,7 @@
|
|||
"ajv": "8.20.0",
|
||||
"ajv-formats": "3.0.1",
|
||||
"express": "5.2.1",
|
||||
"express-rate-limit": "^8.2.1",
|
||||
"jose": "^6.2.3"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -91,7 +91,15 @@ export interface DispatchDeps {
|
|||
// One Ajv instance for the process. Schemas are JSON Schema (draft-07 / the
|
||||
// OpenAPI 3.1 subset our tools use). `strict: false` because tool authors use
|
||||
// vocabulary (e.g. `description`) liberally; we only need structural validation.
|
||||
const ajv = new Ajv({ allErrors: true, strict: false, coerceTypes: false });
|
||||
//
|
||||
// `allErrors: false` (the default) is deliberate and security-relevant: the
|
||||
// input is UNTRUSTED, and `allErrors: true` makes ajv enumerate every schema
|
||||
// violation, which an attacker can weaponize into CPU/memory exhaustion by
|
||||
// sending a large/deeply-nested payload that fails many constraints at once
|
||||
// (CodeQL js/resource-exhaustion). Short-circuiting on the first error caps the
|
||||
// work per request; the 400 still names the first failing path, which is enough
|
||||
// for a caller to fix their input.
|
||||
const ajv = new Ajv({ allErrors: false, strict: false, coerceTypes: false });
|
||||
addFormats(ajv);
|
||||
|
||||
const validatorCache = new WeakMap<object, ValidateFunction>();
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ import { NoopAuditLogger } from './audit.js';
|
|||
import { InMemoryRateLimiter } from './rate-limit.js';
|
||||
import type { DispatchDeps } from './dispatch.js';
|
||||
|
||||
function build() {
|
||||
function build(opts: { edgeRateLimit?: { windowMs: number; limit: number } } = {}) {
|
||||
const registry = new ToolRegistry();
|
||||
registry.register(
|
||||
defineTool<{ id: string }, { id: string; ok: boolean }>({
|
||||
|
|
@ -71,6 +71,7 @@ function build() {
|
|||
deps,
|
||||
mcpInfo: { name: 'sh-mcp-test', version: '0.0.1' },
|
||||
openApi: { info: { title: 'Test', version: '0.0.1' }, servers: [{ url: 'http://x' }] },
|
||||
edgeRateLimit: opts.edgeRateLimit,
|
||||
});
|
||||
}
|
||||
|
||||
|
|
@ -145,6 +146,25 @@ describe('createApp routes', () => {
|
|||
expect(res.status).toBe(429);
|
||||
});
|
||||
|
||||
it('edge rate limiter — throttles by IP BEFORE auth (429 on an unauthenticated flood)', async () => {
|
||||
// limit 2/window. Unauthenticated requests would normally 401, but the edge
|
||||
// limiter sits in front of `authenticate`, so the 3rd request is throttled
|
||||
// (429), not 401 — proving floods are capped before any JWT verification.
|
||||
const app = build({ edgeRateLimit: { windowMs: 60_000, limit: 2 } });
|
||||
expect((await request(app).post('/tools/lookup_thing').send({ id: '1' })).status).toBe(401);
|
||||
expect((await request(app).post('/tools/lookup_thing').send({ id: '2' })).status).toBe(401);
|
||||
const res = await request(app).post('/tools/lookup_thing').send({ id: '3' });
|
||||
expect(res.status).toBe(429);
|
||||
expect(res.body).toEqual({ error: 'rate_limited' });
|
||||
});
|
||||
|
||||
it('edge rate limiter — also fronts /mcp', async () => {
|
||||
const app = build({ edgeRateLimit: { windowMs: 60_000, limit: 1 } });
|
||||
expect((await request(app).post('/mcp').send({})).status).toBe(401);
|
||||
const res = await request(app).post('/mcp').send({});
|
||||
expect(res.status).toBe(429);
|
||||
});
|
||||
|
||||
it('POST /mcp — 401 without a token', async () => {
|
||||
const res = await request(build())
|
||||
.post('/mcp')
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@
|
|||
*/
|
||||
|
||||
import express, { type Express, type Request, type Response, type NextFunction } from 'express';
|
||||
import { rateLimit, type RateLimitRequestHandler } from 'express-rate-limit';
|
||||
import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js';
|
||||
|
||||
import { AuthError } from './cognito-auth.js';
|
||||
|
|
@ -43,6 +44,16 @@ export interface CreateAppOptions {
|
|||
/** MCP handshake identity + OpenAPI `info`/`servers`. */
|
||||
mcpInfo: McpServerInfo;
|
||||
openApi: BuildOpenApiOptions;
|
||||
/**
|
||||
* Per-IP edge rate limit for the authenticated routes (`/mcp`, `/tools/:name`).
|
||||
* This is a coarse abuse/DoS guard that runs BEFORE `authProvider.authenticate`
|
||||
* — so it throttles unauthenticated and invalid-token floods before the (more
|
||||
* expensive) JWT verification and dispatch. It complements, and does not
|
||||
* replace, the per-`sub` + per-tool limiter inside `executeTool` (design.md
|
||||
* §7.3). Defaults: 120 requests / 60s / IP. In production the API Gateway/WAF
|
||||
* is the first edge; this is defense-in-depth.
|
||||
*/
|
||||
edgeRateLimit?: { windowMs: number; limit: number };
|
||||
}
|
||||
|
||||
/** Express `Request` augmented with the authenticated context. */
|
||||
|
|
@ -58,6 +69,21 @@ export function createApp(options: CreateAppOptions): Express {
|
|||
const app = express();
|
||||
app.use(express.json());
|
||||
|
||||
// --- Per-IP edge rate limiter for the authenticated routes ---
|
||||
// Runs ahead of auth so request floods are throttled before JWT verification
|
||||
// and dispatch. Keyed by client IP (express-rate-limit default). Returns the
|
||||
// same 429 shape adapters use elsewhere; standard RateLimit headers, no legacy.
|
||||
const { windowMs = 60_000, limit = 120 } = options.edgeRateLimit ?? {};
|
||||
const edgeRateLimit: RateLimitRequestHandler = rateLimit({
|
||||
windowMs,
|
||||
limit,
|
||||
standardHeaders: 'draft-7',
|
||||
legacyHeaders: false,
|
||||
handler: (_req: Request, res: Response) => {
|
||||
res.status(429).json({ error: 'rate_limited' });
|
||||
},
|
||||
});
|
||||
|
||||
// --- Unauthenticated liveness ---
|
||||
app.get('/healthz', (_req: Request, res: Response) => {
|
||||
res.status(200).json({ status: 'ok' });
|
||||
|
|
@ -103,12 +129,12 @@ export function createApp(options: CreateAppOptions): Express {
|
|||
await server.connect(transport);
|
||||
await transport.handleRequest(req, res, req.body);
|
||||
};
|
||||
app.post('/mcp', authenticate, (req, res) => void handleMcp(req, res));
|
||||
app.get('/mcp', authenticate, (req, res) => void handleMcp(req, res));
|
||||
app.delete('/mcp', authenticate, (req, res) => void handleMcp(req, res));
|
||||
app.post('/mcp', edgeRateLimit, authenticate, (req, res) => void handleMcp(req, res));
|
||||
app.get('/mcp', edgeRateLimit, authenticate, (req, res) => void handleMcp(req, res));
|
||||
app.delete('/mcp', edgeRateLimit, authenticate, (req, res) => void handleMcp(req, res));
|
||||
|
||||
// --- One-shot OpenAPI tool call ---
|
||||
app.post('/tools/:name', authenticate, (req: AuthedRequest, res: Response) => {
|
||||
app.post('/tools/:name', edgeRateLimit, authenticate, (req: AuthedRequest, res: Response) => {
|
||||
const ctx = req.authContext!;
|
||||
const rawName = req.params['name'];
|
||||
const name = Array.isArray(rawName) ? (rawName[0] ?? '') : (rawName ?? '');
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue