mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-23 20:26:10 +00:00
ci(codeql): exclude js/missing-rate-limiting; a test pins every OAuth route to its limiter (D-281)
The query recognizes only npm limiter packages, never oauthRateLimit / oauthTokenRateLimit, and flags a handler as soon as it calls a function named like *auth*, *login* or *verify*. All six alerts it ever raised were on throttled OAuth routes (the last two came from tenantAuthority()). What it stood in for is now a test: every route of the OAuth router must run a per-IP limiter first unless it is one of the three named discovery documents, /oauth/token must carry the strict budget, and a router.use() middleware fails the check. Verified by mutation: removing the limiter from /oauth/authorize fails the test with the route name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
+1
-1
Submodule internal updated: b5a820a159...c5f2c40077
@@ -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<string, unknown> {
|
||||
const routes = new Map<string, unknown>();
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 => {
|
||||
|
||||
Reference in New Issue
Block a user