diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts new file mode 100644 index 00000000..fcb516f7 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts @@ -0,0 +1,278 @@ +/** + * Unit tests for the MCP access guard — the security-critical request gate + * (review findings H1 / tests-1 / tests-2). + * + * accessGuard decides 401 / 403 / 429 and the next()-allow path; checkAccess + * (exercised through it) is the fail-closed backend check. The code is correct + * today — these tests exist purely to catch a regression that would open the + * server: e.g. inverting `!result.allowed`, moving next() into the fail branch, + * or dropping the fail-closed default on a backend error. With scale-to-zero, + * "backend unreachable" is a routine condition, not an edge case. + * + * No live backend or real token is needed: fetch is stubbed and JWTs are + * hand-built (the proxy does not verify signatures — the backend does). + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Request, Response, NextFunction } from 'express'; + +// RATE_LIMIT is read from the environment at module import. Set a low cap so +// the 429 path is cheap to drive, BEFORE the dynamic import resolves. +process.env.MCP_RATE_LIMIT_PER_MINUTE = '2'; + +const { accessGuard } = await import('../access-guard.js'); +// Same ESM singleton the guard uses internally — so the context set by +// runWithCaller inside next() is observable here. +const { isGlobalAdmin, getCurrentToken } = await import('../client.js'); + +/** Build an unsigned JWT-shaped token carrying the given claims. */ +function makeToken(claims: Record): string { + const header = Buffer.from(JSON.stringify({ alg: 'none', typ: 'JWT' })).toString('base64url'); + const payload = Buffer.from(JSON.stringify(claims)).toString('base64url'); + return `${header}.${payload}.sig`; +} + +const futureExp = () => Math.floor(Date.now() / 1000) + 3600; + +/** A valid (well-formed, unexpired) token for the given upn. */ +function validToken(upn: string): string { + return makeToken({ upn, exp: futureExp() }); +} + +function mockReq(authHeader?: string): Request { + return { + headers: { host: 'mcp.example.com', ...(authHeader ? { authorization: authHeader } : {}) }, + protocol: 'https', + } as unknown as Request; +} + +interface CapturedRes { + res: Response; + statusCode: number | null; + body: unknown; + headers: Record; +} + +function mockRes(): CapturedRes { + const captured: CapturedRes = { res: null as unknown as Response, statusCode: null, body: undefined, headers: {} }; + const res: Partial = { + setHeader(name: string, value: string | number | readonly string[]) { + captured.headers[name] = String(value); + return this as Response; + }, + status(code: number) { + captured.statusCode = code; + return this as Response; + }, + json(payload: unknown) { + captured.body = payload; + return this as Response; + }, + }; + captured.res = res as Response; + return captured; +} + +interface GuardOutcome { + nextCalled: boolean; + /** caller context captured INSIDE next() (only meaningful when nextCalled) */ + ctx?: { token: string | undefined; ga: boolean }; + status: number | null; + body: unknown; + headers: Record; +} + +/** + * Drives accessGuard and resolves once it either sends a response (res.json) + * or calls next(). The guard's work is async (checkAccess), so we await. + */ +function runGuard(req: Request): Promise { + return new Promise((resolve, reject) => { + const cap = mockRes(); + let settled = false; + const done = (o: GuardOutcome) => { + if (settled) return; + settled = true; + resolve(o); + }; + // Capture the response when json() is called. + const origJson = cap.res.json.bind(cap.res); + cap.res.json = ((payload: unknown) => { + const r = origJson(payload); + done({ nextCalled: false, status: cap.statusCode, body: cap.body, headers: cap.headers }); + return r; + }) as Response['json']; + + const next: NextFunction = () => { + // Read the per-request caller context from inside the async scope. + done({ + nextCalled: true, + ctx: { token: getCurrentToken(), ga: isGlobalAdmin() }, + status: cap.statusCode, + body: cap.body, + headers: cap.headers, + }); + }; + + try { + accessGuard(req, cap.res, next); + } catch (err) { + reject(err); + } + // Safety net so a hung guard fails the test rather than the runner timing out. + setTimeout(() => done({ nextCalled: false, status: cap.statusCode, body: cap.body, headers: cap.headers }), 2_000); + }); +} + +/** Stub global fetch to return a backend access-check response. */ +function stubBackend(opts: { status?: number; body?: unknown; rawText?: string; reject?: boolean }) { + const fn = vi.fn(async () => { + if (opts.reject) throw new Error('network down'); + const text = opts.rawText !== undefined ? opts.rawText : JSON.stringify(opts.body ?? {}); + return { status: opts.status ?? 200, text: async () => text } as unknown as Response; + }); + vi.stubGlobal('fetch', fn); + return fn; +} + +let upnCounterSeed = 0; +/** Unique upn per test → isolates the module-level access cache + rate buckets. */ +function uniqueUpn(): string { + upnCounterSeed += 1; + return `user${upnCounterSeed}@contoso.com`; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('accessGuard — 401 (authentication failures)', () => { + beforeEach(() => stubBackend({ body: { allowed: true } })); + + it('rejects a request with no Authorization header', async () => { + const out = await runGuard(mockReq()); + expect(out.nextCalled).toBe(false); + expect(out.status).toBe(401); + expect(out.headers['WWW-Authenticate']).toContain('oauth-protected-resource'); + }); + + it('rejects a non-Bearer Authorization header', async () => { + const out = await runGuard(mockReq('Basic dXNlcjpwYXNz')); + expect(out.status).toBe(401); + }); + + it('rejects a token missing the required upn claim', async () => { + const out = await runGuard(mockReq(`Bearer ${makeToken({ exp: futureExp() })}`)); + expect(out.status).toBe(401); + expect(out.headers['WWW-Authenticate']).toContain('error="invalid_token"'); + }); + + it('rejects a malformed (non-decodable) token', async () => { + const out = await runGuard(mockReq('Bearer not-a-jwt')); + expect(out.status).toBe(401); + }); + + it('rejects an expired token before ever calling the backend', async () => { + const fetchFn = stubBackend({ body: { allowed: true } }); + const expired = makeToken({ upn: 'alice@contoso.com', exp: Math.floor(Date.now() / 1000) - 100 }); + const out = await runGuard(mockReq(`Bearer ${expired}`)); + expect(out.status).toBe(401); + expect(out.headers['WWW-Authenticate']).toContain('error="invalid_token"'); + expect(fetchFn).not.toHaveBeenCalled(); + }); +}); + +describe('accessGuard — 403 (authorization + fail-closed)', () => { + it('denies when the backend reports allowed:false and surfaces the reason', async () => { + stubBackend({ body: { allowed: false, reason: 'not on the MCP whitelist' } }); + const out = await runGuard(mockReq(`Bearer ${validToken(uniqueUpn())}`)); + expect(out.nextCalled).toBe(false); + expect(out.status).toBe(403); + expect((out.body as { reason: string }).reason).toBe('not on the MCP whitelist'); + }); + + it('fail-closed: denies when the backend fetch throws (tests-2)', async () => { + // scale-to-zero ⇒ a cold/unreachable backend is routine. checkAccess must + // resolve allowed:false, never allowed:true. + stubBackend({ reject: true }); + const out = await runGuard(mockReq(`Bearer ${validToken(uniqueUpn())}`)); + expect(out.nextCalled).toBe(false); + expect(out.status).toBe(403); + expect((out.body as { reason: string }).reason).toMatch(/unavailable/i); + }); + + it('fail-closed: denies when the backend returns an empty body (tests-2)', async () => { + stubBackend({ status: 502, rawText: '' }); + const out = await runGuard(mockReq(`Bearer ${validToken(uniqueUpn())}`)); + expect(out.nextCalled).toBe(false); + expect(out.status).toBe(403); + expect((out.body as { reason: string }).reason).toMatch(/empty body/i); + }); + + it('fail-closed: treats allowed values other than boolean true as denied', async () => { + // `allowed: "true"` (string) or `allowed: 1` must NOT be coerced to allow. + stubBackend({ body: { allowed: 'true' } }); + const out = await runGuard(mockReq(`Bearer ${validToken(uniqueUpn())}`)); + expect(out.status).toBe(403); + }); +}); + +describe('accessGuard — allow path', () => { + it('calls next() exactly once and scopes the caller context (token + GA flag)', async () => { + const upn = uniqueUpn(); + stubBackend({ body: { allowed: true, accessGrant: 'whitelisted', isGlobalAdmin: true } }); + const token = validToken(upn); + const out = await runGuard(mockReq(`Bearer ${token}`)); + + expect(out.nextCalled).toBe(true); + expect(out.status).toBeNull(); // no response sent on the allow path + expect(out.ctx).toEqual({ token, ga: true }); + }); + + it('propagates isGlobalAdmin:false through the caller context', async () => { + stubBackend({ body: { allowed: true, isGlobalAdmin: false } }); + const out = await runGuard(mockReq(`Bearer ${validToken(uniqueUpn())}`)); + expect(out.nextCalled).toBe(true); + expect(out.ctx?.ga).toBe(false); + }); +}); + +describe('accessGuard — 429 (rate limit) and caching', () => { + it('429s once the per-minute budget is exhausted (cap=2)', async () => { + const upn = uniqueUpn(); + const token = validToken(upn); + stubBackend({ body: { allowed: true } }); + + const first = await runGuard(mockReq(`Bearer ${token}`)); + const second = await runGuard(mockReq(`Bearer ${token}`)); + const third = await runGuard(mockReq(`Bearer ${token}`)); + + expect(first.nextCalled).toBe(true); + expect(second.nextCalled).toBe(true); + expect(third.nextCalled).toBe(false); + expect(third.status).toBe(429); + expect((third.body as { retryAfterSeconds: number }).retryAfterSeconds).toBe(60); + }); + + it('serves the cached access decision (one backend call across repeat requests)', async () => { + const token = validToken(uniqueUpn()); + const fetchFn = stubBackend({ body: { allowed: true } }); + + await runGuard(mockReq(`Bearer ${token}`)); + await runGuard(mockReq(`Bearer ${token}`)); + + // Second request hits the 5-min UPN+tokenHash cache — no second backend call. + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it('does not let a forged token reuse a legitimate token cache entry', async () => { + // Same upn, two different token strings ⇒ distinct cache keys ⇒ two backend + // calls. This is the F1 guarantee, observed end-to-end through the guard. + const upn = uniqueUpn(); + const fetchFn = stubBackend({ body: { allowed: true } }); + + await runGuard(mockReq(`Bearer ${makeToken({ upn, exp: futureExp(), jti: 'legit' })}`)); + await runGuard(mockReq(`Bearer ${makeToken({ upn, exp: futureExp(), jti: 'forged' })}`)); + + expect(fetchFn).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/auth.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/auth.test.ts new file mode 100644 index 00000000..f9c9c786 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/auth.test.ts @@ -0,0 +1,92 @@ +/** + * Unit tests for the token-claim helpers (review finding tests-3 / tests-4). + * + * extractTokenClaims + isTokenExpired are the first gate every authenticated + * request passes through in accessGuard. The code is correct; these tests pin + * the contract so a regression (e.g. inverting the expiry comparison or losing + * the skew buffer) fails loudly rather than silently weakening auth. + * + * No signature validation happens here by design — the backend re-validates the + * same token. These helpers only decode + check the unix `exp`. + */ +import { describe, it, expect } from 'vitest'; +import { extractTokenClaims, isTokenExpired } from '../auth.js'; + +/** Build an unsigned JWT-shaped string with the given payload claims. */ +function makeToken(claims: Record): string { + const header = Buffer.from(JSON.stringify({ alg: 'none', typ: 'JWT' })).toString('base64url'); + const payload = Buffer.from(JSON.stringify(claims)).toString('base64url'); + return `${header}.${payload}.sig`; +} + +const nowSec = () => Math.floor(Date.now() / 1000); + +describe('extractTokenClaims', () => { + it('decodes upn / oid / tid / exp / aud from a well-formed token', () => { + const claims = extractTokenClaims( + makeToken({ + upn: 'alice@contoso.com', + oid: 'object-id-123', + tid: 'tenant-id-456', + exp: nowSec() + 3600, + aud: 'api://00000000-0000-0000-0000-000000000000', + }), + ); + expect(claims).not.toBeNull(); + expect(claims!.upn).toBe('alice@contoso.com'); + expect(claims!.oid).toBe('object-id-123'); + expect(claims!.tid).toBe('tenant-id-456'); + expect(typeof claims!.exp).toBe('number'); + }); + + it('surfaces the aud claim so it is observable (tests-4: parsed, not locally validated)', () => { + // The proxy does not validate audience — the backend does. But it must + // still decode it, e.g. for diagnostics. This pins that aud is parsed. + const claims = extractTokenClaims(makeToken({ upn: 'a@b.c', aud: 'api://some-resource' })); + expect(claims!.aud).toBe('api://some-resource'); + }); + + it('returns null for a token that is not three dot-separated parts', () => { + expect(extractTokenClaims('only.two')).toBeNull(); + expect(extractTokenClaims('no-dots-at-all')).toBeNull(); + expect(extractTokenClaims('a.b.c.d')).toBeNull(); + expect(extractTokenClaims('')).toBeNull(); + }); + + it('returns null when the payload segment is not valid JSON', () => { + const header = Buffer.from('{}').toString('base64url'); + const garbagePayload = Buffer.from('this-is-not-json').toString('base64url'); + expect(extractTokenClaims(`${header}.${garbagePayload}.sig`)).toBeNull(); + }); + + it('returns null for completely malformed input', () => { + expect(extractTokenClaims('!@#$%^&*()')).toBeNull(); + }); +}); + +describe('isTokenExpired', () => { + it('treats a token with no exp claim as expired (fail-closed)', () => { + expect(isTokenExpired({})).toBe(true); + expect(isTokenExpired({ upn: 'a@b.c' })).toBe(true); + }); + + it('reports a token whose exp is in the past as expired', () => { + expect(isTokenExpired({ exp: nowSec() - 100 })).toBe(true); + }); + + it('reports a token expiring within the 60s skew buffer as expired', () => { + // exp is technically still in the future, but inside the 60s buffer the + // helper conservatively reports expired so a borderline token is not used. + expect(isTokenExpired({ exp: nowSec() + 30 })).toBe(true); + }); + + it('reports a token comfortably in the future as not expired', () => { + expect(isTokenExpired({ exp: nowSec() + 3600 })).toBe(false); + }); + + it('treats a token exactly at the skew boundary as still valid', () => { + // now > exp - 60 ⇒ expired. At exp = now + 61, (exp - 60) = now + 1 > now, + // so it is NOT expired. Pins the boundary direction. + expect(isTokenExpired({ exp: nowSec() + 61 })).toBe(false); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/client-routing.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/client-routing.test.ts new file mode 100644 index 00000000..399dddd8 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/client-routing.test.ts @@ -0,0 +1,88 @@ +/** + * Unit tests for the GA-gating / caller-context routing (review finding tests-6). + * + * pickGlobalOrTenantPath is the hinge of the MCP server's tenant isolation: + * a Global Admin is routed to /api/global/* (cross-tenant), everyone else to + * the tenant-scoped /api/* variant where the backend resolves the tenant from + * the JWT. A regression that flips this default would either expose + * cross-tenant endpoints to a tenant-admin or break GA cross-tenant queries. + * + * runWithCaller scopes that decision per async context so concurrent sessions + * cannot observe each other's token / GA flag — this is asserted explicitly. + */ +import { describe, it, expect } from 'vitest'; +import { + pickGlobalOrTenantPath, + isGlobalAdmin, + getCurrentToken, + runWithCaller, +} from '../client.js'; + +const GLOBAL = '/api/global/devices/blocked'; +const TENANT = '/api/devices/blocked'; + +describe('pickGlobalOrTenantPath — GA gating', () => { + it('defaults to the tenant path when no caller context is active', () => { + // No runWithCaller wrapper ⇒ isGlobalAdmin() is false by design. + expect(isGlobalAdmin()).toBe(false); + expect(getCurrentToken()).toBeUndefined(); + expect(pickGlobalOrTenantPath(GLOBAL, TENANT)).toBe(TENANT); + }); + + it('routes a Global Admin to the /api/global/* path', () => { + runWithCaller({ token: 'ga-token', isGlobalAdmin: true }, () => { + expect(isGlobalAdmin()).toBe(true); + expect(getCurrentToken()).toBe('ga-token'); + expect(pickGlobalOrTenantPath(GLOBAL, TENANT)).toBe(GLOBAL); + }); + }); + + it('routes a non-GA caller to the tenant-scoped path', () => { + runWithCaller({ token: 'tenant-token', isGlobalAdmin: false }, () => { + expect(isGlobalAdmin()).toBe(false); + expect(pickGlobalOrTenantPath(GLOBAL, TENANT)).toBe(TENANT); + }); + }); + + it('reverts to the no-context default after the callback returns', () => { + runWithCaller({ token: 't', isGlobalAdmin: true }, () => { + expect(isGlobalAdmin()).toBe(true); + }); + // Context must not leak past the run() boundary. + expect(isGlobalAdmin()).toBe(false); + expect(getCurrentToken()).toBeUndefined(); + }); +}); + +describe('runWithCaller — context isolation', () => { + it('keeps concurrent contexts from bleeding into each other', async () => { + // Interleave two async callers on the event loop; each must continue to see + // its own token + GA flag despite the other running between awaits. + const ga = runWithCaller({ token: 'ga', isGlobalAdmin: true }, async () => { + await Promise.resolve(); + return { token: getCurrentToken(), ga: isGlobalAdmin(), path: pickGlobalOrTenantPath(GLOBAL, TENANT) }; + }); + const tenant = runWithCaller({ token: 'tn', isGlobalAdmin: false }, async () => { + await Promise.resolve(); + return { token: getCurrentToken(), ga: isGlobalAdmin(), path: pickGlobalOrTenantPath(GLOBAL, TENANT) }; + }); + + const [gaResult, tenantResult] = await Promise.all([ga, tenant]); + + expect(gaResult).toEqual({ token: 'ga', ga: true, path: GLOBAL }); + expect(tenantResult).toEqual({ token: 'tn', ga: false, path: TENANT }); + }); + + it('nested contexts restore the parent context on exit', () => { + runWithCaller({ token: 'outer', isGlobalAdmin: true }, () => { + expect(getCurrentToken()).toBe('outer'); + runWithCaller({ token: 'inner', isGlobalAdmin: false }, () => { + expect(getCurrentToken()).toBe('inner'); + expect(isGlobalAdmin()).toBe(false); + }); + // Inner run() must not clobber the outer context. + expect(getCurrentToken()).toBe('outer'); + expect(isGlobalAdmin()).toBe(true); + }); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts new file mode 100644 index 00000000..fe368368 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts @@ -0,0 +1,172 @@ +/** + * Endpoint tests for the OAuth proxy router (review finding tests-5). + * + * The file header of security-guards.test.ts claims /oauth/register + PKCE-S256 + * enforcement are covered, but those tests only assert constant magnitudes (F2) + * and signState/verifyState (F4) — never the route handlers themselves. This + * suite closes that gap by mounting createOAuthRouter() on an ephemeral Express + * server and exercising the actual request paths. + * + * No external network is reached: every asserted path returns before the proxy + * would call Entra ID (the /oauth/authorize success case yields a 302 we read + * with redirect:'manual'; the token error case returns before fetch). + */ +import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest'; +import express from 'express'; +import type { Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; + +// createOAuthRouter() throws at import unless the Entra client id is present. +process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +const { createOAuthRouter, _registryForTest, MAX_REGISTERED_CLIENTS, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } = + await import('../oauth.js'); + +let server: Server; +let baseUrl: string; + +beforeAll(async () => { + const app = express(); + app.use(express.json()); + app.use(express.urlencoded({ extended: true })); + app.use(createOAuthRouter()); + await new Promise((resolve) => { + server = app.listen(0, '127.0.0.1', () => resolve()); + }); + const { port } = server.address() as AddressInfo; + baseUrl = `http://127.0.0.1:${port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +afterEach(() => { + // Reset the registry between tests (the cap test fills it to capacity). + _registryForTest.clear(); +}); + +async function register(body: unknown) { + const res = await fetch(`${baseUrl}/oauth/register`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + const json = await res.json().catch(() => ({})); + return { status: res.status, json: json as Record }; +} + +describe('/oauth/register — dynamic client registration (RFC 7591)', () => { + it('registers a client with a loopback redirect_uri and returns a client_id', async () => { + const { status, json } = await register({ + client_name: 'Claude Code', + redirect_uris: ['http://localhost:54321/callback'], + }); + expect(status).toBe(201); + expect(typeof json.client_id).toBe('string'); + expect(json.redirect_uris).toEqual(['http://localhost:54321/callback']); + // RFC 7591 default for a public client. + expect(json.token_endpoint_auth_method).toBe('none'); + }); + + it('rejects a hostile redirect_uri host (allowlist defense-in-depth)', async () => { + const { status, json } = await register({ + client_name: 'evil', + redirect_uris: ['https://attacker.tld/cb'], + }); + expect(status).toBe(400); + expect(json.error).toBe('invalid_redirect_uri'); + }); + + it('rejects more than MAX_REDIRECT_URIS_PER_CLIENT redirect_uris', async () => { + const uris = Array.from({ length: MAX_REDIRECT_URIS_PER_CLIENT + 1 }, (_, i) => `http://localhost:${1000 + i}/cb`); + const { status, json } = await register({ client_name: 'greedy', redirect_uris: uris }); + expect(status).toBe(400); + expect(json.error).toBe('invalid_redirect_uri'); + }); + + it('rejects an oversized redirect_uri', async () => { + const huge = `http://localhost:1/${'a'.repeat(MAX_REDIRECT_URI_LENGTH + 1)}`; + const { status, json } = await register({ client_name: 'x', redirect_uris: [huge] }); + expect(status).toBe(400); + expect(json.error).toBe('invalid_redirect_uri'); + }); + + it('rejects an over-long client_name', async () => { + const { status, json } = await register({ + client_name: 'n'.repeat(MAX_CLIENT_NAME_LENGTH + 1), + redirect_uris: ['http://localhost:1/cb'], + }); + expect(status).toBe(400); + expect(json.error).toBe('invalid_client_metadata'); + }); + + it('returns 503 once the registry is at its hard cap', async () => { + _registryForTest.fill(MAX_REGISTERED_CLIENTS); + expect(_registryForTest.size()).toBeGreaterThanOrEqual(MAX_REGISTERED_CLIENTS); + const { status, json } = await register({ + client_name: 'overflow', + redirect_uris: ['http://localhost:1/cb'], + }); + expect(status).toBe(503); + expect(json.error).toBe('temporarily_unavailable'); + }); +}); + +describe('/oauth/authorize — PKCE S256 enforcement', () => { + async function authorize(query: Record) { + const qs = new URLSearchParams(query).toString(); + return fetch(`${baseUrl}/oauth/authorize?${qs}`, { redirect: 'manual' }); + } + + it('rejects a request with no code_challenge (PKCE mandatory)', async () => { + const res = await authorize({ redirect_uri: 'http://localhost:1/cb' }); + expect(res.status).toBe(400); + const json = (await res.json()) as Record; + expect(json.error).toBe('invalid_request'); + expect(String(json.error_description)).toMatch(/code_challenge is required/); + }); + + it('rejects a downgrade to code_challenge_method=plain', async () => { + const res = await authorize({ + redirect_uri: 'http://localhost:1/cb', + code_challenge: 'abc123', + code_challenge_method: 'plain', + }); + expect(res.status).toBe(400); + const json = (await res.json()) as Record; + expect(String(json.error_description)).toMatch(/must be S256/); + }); + + it('rejects a missing redirect_uri', async () => { + const res = await authorize({ code_challenge: 'abc123' }); + expect(res.status).toBe(400); + }); + + it('redirects a valid S256 request to the Entra authorize endpoint', async () => { + const res = await authorize({ + redirect_uri: 'http://localhost:54321/cb', + code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + code_challenge_method: 'S256', + state: 'client-state', + }); + expect(res.status).toBe(302); + const location = res.headers.get('location') ?? ''; + expect(location).toContain('login.microsoftonline.com'); + expect(location).toContain('code_challenge_method=S256'); + // The client's challenge is forwarded unchanged. + expect(location).toContain('E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM'); + }); +}); + +describe('/oauth/token — PKCE finishing move', () => { + it('rejects an authorization_code grant with no code_verifier', async () => { + const res = await fetch(`${baseUrl}/oauth/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ grant_type: 'authorization_code', code: 'abc' }).toString(), + }); + expect(res.status).toBe(400); + const json = (await res.json()) as Record; + expect(String(json.error_description)).toMatch(/code_verifier is required/); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-admin.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-admin.test.ts index e7f8213d..aed0ce0c 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-admin.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-admin.test.ts @@ -5,11 +5,17 @@ import { describe, it, expect, beforeAll } from 'vitest'; import { apiFetch, getToken } from './helpers.js'; +// Integration suite: only runs when a backend token is supplied. Without one it +// SKIPS (not errors), so an unattended CI run still goes green on the unit +// suites instead of failing the whole file. (review finding tests-7) +const RUN_INTEGRATION = !!process.env.AUTOPILOT_API_TOKEN; +const suite = describe.skipIf(!RUN_INTEGRATION); + beforeAll(() => { - getToken(); + if (RUN_INTEGRATION) getToken(); }); -describe('get_metrics', () => { +suite('get_metrics', () => { it('should return global enrollment metrics', async () => { const data = await apiFetch(`/api/global/metrics/summary?days=30`); expect(data.success).toBe(true); @@ -30,7 +36,7 @@ describe('get_metrics', () => { }); }); -describe('get_usage_metrics', () => { +suite('get_usage_metrics', () => { it('should return platform usage overview', async () => { const data = await apiFetch(`/api/global/metrics/usage`); expect(data).toHaveProperty('sessions'); @@ -41,7 +47,7 @@ describe('get_usage_metrics', () => { }); }); -describe('get_geographic_metrics', () => { +suite('get_geographic_metrics', () => { it('should return geographic distribution (by country)', async () => { const data = await apiFetch(`/api/global/metrics/geographic?groupBy=country&days=30`); expect(data.success).toBe(true); @@ -55,7 +61,7 @@ describe('get_geographic_metrics', () => { }); }); -describe('get_geographic_sessions', () => { +suite('get_geographic_sessions', () => { it('should return sessions for a country', async () => { const data = await apiFetch(`/api/global/metrics/geographic/sessions?country=DE&days=7`); expect(data.success).toBe(true); @@ -63,7 +69,7 @@ describe('get_geographic_sessions', () => { }); }); -describe('get_platform_metrics', () => { +suite('get_platform_metrics', () => { it('should return agent performance metrics', async () => { const data = await apiFetch(`/api/global/metrics/platform`); expect(data).toHaveProperty('sessionsAnalyzed'); @@ -75,7 +81,7 @@ describe('get_platform_metrics', () => { }); }); -describe('get_api_usage', () => { +suite('get_api_usage', () => { it('should return daily usage summary', async () => { const data = await apiFetch(`/api/global/metrics/mcp-usage/daily`); expect(Array.isArray(data.summaries)).toBe(true); @@ -92,7 +98,7 @@ describe('get_api_usage', () => { }); }); -describe('get_audit_logs', () => { +suite('get_audit_logs', () => { it('should return audit log entries (cross-tenant)', async () => { const data = await apiFetch(`/api/global/audit/logs`); expect(data.success).toBe(true); @@ -107,7 +113,7 @@ describe('get_audit_logs', () => { }); }); -describe('get_ops_events', () => { +suite('get_ops_events', () => { it('should return ops events', async () => { const data = await apiFetch(`/api/global/ops-events?maxResults=5`); expect(data.success).toBe(true); @@ -123,7 +129,7 @@ describe('get_ops_events', () => { }); }); -describe('list_session_reports', () => { +suite('list_session_reports', () => { it('should return session reports', async () => { const data = await apiFetch(`/api/global/session-reports`); expect(data.success).toBe(true); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-raw.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-raw.test.ts index 6ed8a09a..49c471bf 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-raw.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-raw.test.ts @@ -7,14 +7,21 @@ import { apiFetch, getToken } from './helpers.js'; let knownTenantId: string; +// Integration suite: only runs when a backend token is supplied. Without one it +// SKIPS (not errors), so an unattended CI run still goes green on the unit +// suites instead of failing the whole file. (review finding tests-7) +const RUN_INTEGRATION = !!process.env.AUTOPILOT_API_TOKEN; +const suite = describe.skipIf(!RUN_INTEGRATION); + beforeAll(async () => { + if (!RUN_INTEGRATION) return; getToken(); // Discover a tenant with data const data = await apiFetch(`/api/global/search/sessions?limit=1`); knownTenantId = data.sessions[0]?.tenantId ?? ''; }); -describe('list_tables', () => { +suite('list_tables', () => { it('should return list of Azure Table Storage tables', async () => { const data = await apiFetch(`/api/global/raw/tables`); expect(data).toHaveProperty('tables'); @@ -26,7 +33,7 @@ describe('list_tables', () => { }); }); -describe('query_table', () => { +suite('query_table', () => { it('should query TenantConfiguration table', async () => { const data = await apiFetch(`/api/global/raw/tables/TenantConfiguration?limit=2`); expect(data).toHaveProperty('entities'); @@ -58,7 +65,7 @@ describe('query_table', () => { }); }); -describe('query_raw_sessions', () => { +suite('query_raw_sessions', () => { it('should return raw sessions (cross-tenant)', async () => { const data = await apiFetch(`/api/global/raw/sessions?limit=3`); expect(data).toHaveProperty('sessions'); @@ -84,7 +91,7 @@ describe('query_raw_sessions', () => { }); }); -describe('query_raw_events', () => { +suite('query_raw_events', () => { it('should return events for a tenant', async () => { if (!knownTenantId) return; const data = await apiFetch( @@ -95,7 +102,7 @@ describe('query_raw_events', () => { }); }); -describe('query_backend_logs', () => { +suite('query_backend_logs', () => { it('should execute KQL query (POST)', async () => { const data = await apiFetch(`/api/global/raw/logs`, { method: 'POST', @@ -111,7 +118,7 @@ describe('query_backend_logs', () => { }); }); -describe('read-only verification', () => { +suite('read-only verification', () => { it('should reject PUT on session endpoint', async () => { // Verify the API doesn't accept mutation methods on read endpoints try { diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-sessions.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-sessions.test.ts index 9d66519b..71f27f6f 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-sessions.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tools-sessions.test.ts @@ -9,11 +9,17 @@ import { apiFetch, buildQuery, getToken } from './helpers.js'; let knownSessionId: string; let knownTenantId: string; +// Integration suite: only runs when a backend token is supplied. Without one it +// SKIPS (not errors), so an unattended CI run still goes green on the unit +// suites instead of failing the whole file. (review finding tests-7) +const RUN_INTEGRATION = !!process.env.AUTOPILOT_API_TOKEN; +const suite = describe.skipIf(!RUN_INTEGRATION); + beforeAll(() => { - getToken(); // Fail fast if no token + if (RUN_INTEGRATION) getToken(); // Fail fast if no token }); -describe('search_sessions', () => { +suite('search_sessions', () => { it('should return sessions (cross-tenant, no filters)', async () => { const data = await apiFetch(`/api/global/search/sessions?limit=3`); expect(data.success).toBe(true); @@ -50,7 +56,7 @@ describe('search_sessions', () => { }); }); -describe('search_sessions_by_event', () => { +suite('search_sessions_by_event', () => { it('should find sessions with enrollment_complete events', async () => { const data = await apiFetch( `/api/global/search/sessions-by-event?eventType=enrollment_complete&limit=3`, @@ -68,7 +74,7 @@ describe('search_sessions_by_event', () => { }); }); -describe('get_session', () => { +suite('get_session', () => { it('should return session details', async () => { const data = await apiFetch(`/api/sessions/${knownSessionId}`); expect(data.success).toBe(true); @@ -93,7 +99,7 @@ describe('get_session', () => { }); }); -describe('get_session_events', () => { +suite('get_session_events', () => { it('should return events for a session', async () => { const data = await apiFetch(`/api/sessions/${knownSessionId}/events`); expect(data.success).toBe(true); @@ -128,7 +134,7 @@ describe('get_session_events', () => { }); }); -describe('get_session_analysis', () => { +suite('get_session_analysis', () => { it('should return analysis results', async () => { const data = await apiFetch(`/api/sessions/${knownSessionId}/analysis`); expect(data.success).toBe(true); @@ -137,7 +143,7 @@ describe('get_session_analysis', () => { }); }); -describe('search_sessions_by_cve', () => { +suite('search_sessions_by_cve', () => { it('should return results (possibly empty) for a CVE search', async () => { const data = await apiFetch( `/api/global/search/sessions-by-cve?cveId=CVE-2024-21447&limit=3`, @@ -155,7 +161,7 @@ describe('search_sessions_by_cve', () => { }); }); -describe('list_blocked_devices', () => { +suite('list_blocked_devices', () => { it('should return blocked devices list (cross-tenant)', async () => { const data = await apiFetch(`/api/global/devices/blocked`); expect(data.success).toBe(true); diff --git a/src/McpServer/autopilot-monitor-mcp/src/auth.ts b/src/McpServer/autopilot-monitor-mcp/src/auth.ts index 34835418..a2364eab 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/auth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/auth.ts @@ -20,7 +20,18 @@ export interface TokenClaims { tid?: string; /** Token expiry (unix timestamp) */ exp?: number; - /** Audience */ + /** + * Audience. Parsed for observability/diagnostics but intentionally NOT + * validated here — the same Bearer token is passed through to the backend + * API, which enforces the audience cryptographically alongside the + * signature/issuer/lifetime checks. See + * `src/Backend/AutopilotMonitor.Functions/Middleware/AuthenticationMiddleware.cs:200` + * (`ValidateAudience = true`, `ValidAudiences = { clientId, api://clientId }`). + * Duplicating that gate here would only add a second, drift-prone copy of the + * accepted-audience list (RFC 8707 resource indicators are honored by the + * backend, not the proxy). This is spec-conformant token pass-through: the + * user's token is forwarded to the resource it was issued for. + */ aud?: string; }