test(mcp): cover auth core — accessGuard, token claims, oauth, GA-gating

Close the regression-coverage gaps flagged in the MCP review (H1, tests-1..7).
The auth core was correct but entirely untested; a regression that inverted a
fail-closed decision or moved next() into the error branch would silently open
the server with no failing test. All new tests run without a live backend or
token (stubbed fetch, hand-built JWTs, ephemeral express server).

- access-guard.test.ts: 401/403/429 decisions, fail-closed paths (fetch throws /
  empty body / non-boolean allowed -> denied), allow-path caller context, cache,
  F1 forged-token distinct-key. (H1, tests-1, tests-2)
- auth.test.ts: extractTokenClaims + isTokenExpired incl. 60s-skew boundary. (tests-3)
- auth.ts: comment documenting aud is parsed but enforced by the backend
  (AuthenticationMiddleware.cs:200 ValidateAudience=true); no behavior change. (tests-4)
- oauth-register.test.ts: /oauth/register caps + cap-503, PKCE-S256 enforcement on
  /oauth/authorize, code_verifier requirement on /oauth/token. (tests-5)
- client-routing.test.ts: pickGlobalOrTenantPath GA-gating + runWithCaller isolation. (tests-6)
- tools-admin/raw/sessions: describe.skipIf(no token) so integration suites skip
  cleanly unattended instead of throwing in beforeAll. (tests-7)

npm test (no token): 100 passed / 0 failed / 39 skipped (was 3 suites failing).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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, unknown>): 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<string, string>;
+}
+
+function mockRes(): CapturedRes {
+  const captured: CapturedRes = { res: null as unknown as Response, statusCode: null, body: undefined, headers: {} };
+  const res: Partial<Response> = {
+    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<string, string>;
+}
+
+/**
+ * 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<GuardOutcome> {
+  return new Promise<GuardOutcome>((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);
+  });
+});

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, unknown>): 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);
+  });
+});

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);
+    });
+  });
+});

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<void>((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<void>((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<string, unknown> };
+}
+
+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<string, string>) {
+    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<string, unknown>;
+    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<string, unknown>;
+    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<string, unknown>;
+    expect(String(json.error_description)).toMatch(/code_verifier is required/);
+  });
+});

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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(`/api/global/session-reports`);
     expect(data.success).toBe(true);

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<any>(`/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<any>(`/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<any>(`/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<any>(`/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<any>(
@@ -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<any>(`/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 {

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<any>(`/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<any>(
       `/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<any>(`/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<any>(`/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<any>(`/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<any>(
       `/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<any>(`/api/global/devices/blocked`);
     expect(data.success).toBe(true);

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;
 }