diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml index 900b33b2..89e7ff65 100644 --- a/.github/codeql/codeql-config.yml +++ b/.github/codeql/codeql-config.yml @@ -1,8 +1,8 @@ # CodeQL configuration. # -# The default query suites stay active — this file only filters out two C# queries -# that fire structurally on this codebase, where the flagged behavior is the feature, -# not a defect: +# The default query suites stay active — this file only filters out queries that +# fire structurally on this codebase, where the flagged behavior is the feature +# or the protection exists in a form the query cannot see: # # cs/exposure-of-sensitive-information # Audit and operational logs record WHO performed an action (actor UPN / email) @@ -15,10 +15,20 @@ # key. The IP is an identifier for throttling, not a secret at rest; storing it in # the in-memory / table rate-limit key is by design. # +# js/missing-rate-limiting +# Only the MCP server has Express routes. Every one that does work is throttled by +# our own limiters (oauthRateLimit / oauthTokenRateLimit on /oauth/*, accessGuard +# on /mcp); /health and the OAuth discovery documents return static JSON. +# The query only recognizes limiters from npm packages (express-rate-limit and +# similar) and flags a route handler as soon as it calls a function named +# like *auth*, *login* or *verify*. So it reports throttled and unthrottled +# routes the same way, and every alert it ever raised was a false positive. The +# coverage check it stood in for is a test: every OAuth router route must carry +# a limiter or be named as exempt (MCP oauth-rate-limit.test.ts). +# # Genuine one-off false positives from OTHER queries (e.g. an open-redirect the taint -# model can't see is allow-listed, a rate-limited route it doesn't recognize) are -# dismissed individually in the Security tab so those queries stay sharp — they are -# NOT suppressed here. +# model can't see is allow-listed) are dismissed individually in the Security tab so +# those queries stay sharp — they are NOT suppressed here. # # Log-injection (cs/log-forging) is deliberately NOT filtered: user-controlled values # are sanitized at the sink via Helpers/LogSanitizer, so the query stays useful for @@ -29,3 +39,5 @@ query-filters: id: cs/exposure-of-sensitive-information - exclude: id: cs/cleartext-storage-of-sensitive-information + - exclude: + id: js/missing-rate-limiting diff --git a/internal b/internal index b5a820a1..c5f2c400 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit b5a820a1595e1b555affaec1c84a402a6874eaef +Subproject commit c5f2c400774f1d39d9a87558a22304d98ec1e075 diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts index 3aa7a206..41322910 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts @@ -10,6 +10,10 @@ * suites. No network is reached: every request fails validation (unsupported * grant / missing code) before the handler would call Entra, and the limiter * runs before the handler anyway. + * + * The last suite is the coverage check CodeQL's js/missing-rate-limiting stood + * in for (excluded, D-281): it cannot recognize these limiters, so every route + * of the OAuth router must carry one or be named as exempt here. */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import express from 'express'; @@ -20,6 +24,7 @@ process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000' process.env.MCP_OAUTH_RATE_LIMIT_PER_MINUTE = '3'; process.env.MCP_OAUTH_TOKEN_RATE_LIMIT_PER_MINUTE = '2'; const { createOAuthRouter } = await import('../oauth.js'); +const { oauthRateLimit, oauthTokenRateLimit } = await import('../access-guard.js'); let server: Server; let baseUrl: string; @@ -89,3 +94,44 @@ describe('/oauth/* per-source-IP rate limiting (R-3)', () => { expect((await token()).status).toBe(429); }); }); + +describe('every OAuth router route is throttled (D-281)', () => { + // The discovery documents are the only exemption: static JSON built from config and the request + // host, no outbound call, no state, no logging. Anything else needs a limiter as its first handler. + const UNTHROTTLED = [ + 'GET /.well-known/oauth-protected-resource/mcp', + 'GET /.well-known/oauth-protected-resource', + 'GET /.well-known/oauth-authorization-server', + ]; + + /** Each route of the OAuth router as `METHOD path` → the handler that runs first. */ + function firstHandlers(): Map { + const routes = new Map(); + for (const layer of createOAuthRouter().stack) { + // A router.use() middleware has no route and would escape the check below. + expect(layer.route, 'the OAuth router mounts routes only').toBeDefined(); + for (const handler of layer.route!.stack) { + const key = `${(handler.method ?? 'all').toUpperCase()} ${layer.route!.path}`; + if (!routes.has(key)) routes.set(key, handler.handle); + } + } + return routes; + } + + it('runs a per-IP limiter before every handler except the discovery documents', () => { + const routes = firstHandlers(); + // Introspection sanity: the known surface is found (an Express upgrade that moves the stack fails here). + expect([...routes.keys()]).toEqual(expect.arrayContaining([ + ...UNTHROTTLED, 'POST /oauth/register', 'GET /oauth/authorize', 'GET /oauth/callback', 'POST /oauth/token', + ])); + + const unthrottled = [...routes].filter(([key, first]) => + !UNTHROTTLED.includes(key) && first !== oauthRateLimit && first !== oauthTokenRateLimit, + ).map(([key]) => key); + expect(unthrottled).toEqual([]); + }); + + it('gives /oauth/token the strict budget — it is the route that calls Entra with the client secret', () => { + expect(firstHandlers().get('POST /oauth/token')).toBe(oauthTokenRateLimit); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index 2bc35905..4f924d75 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -394,7 +394,8 @@ function respondRateLimited(res: Response, kind: RateLimitKind, key: string, rpc * map keyed `${kind}:${ip}` so the two surfaces cannot starve each other and * the strict token budget stays independent of the general one. Applied * per-route inside createOAuthRouter so any future /oauth endpoint must opt in - * consciously rather than inherit silence. + * consciously rather than inherit silence — oauth-rate-limit.test.ts fails any + * router route that has no limiter and is not a named exemption (D-281). */ function makeOAuthIpLimiter(kind: 'oauth' | 'oauth-token', limit: number) { return (req: Request, res: Response, next: NextFunction): void => {