fix(mcp-oauth): require client_id, reject empty callback code, test callback

Address three review findings on the OAuth proxy:

1. (Medium) client_id was optional at /oauth/authorize, falling back to the
   host allowlist alone — bypassing the per-client redirect_uri binding by
   simply omitting client_id. The cold-start reason for tolerating absence
   is gone now that client_id is a stateless, replica-portable signed token,
   and every conforming MCP client registers first. client_id is now
   REQUIRED (400 invalid_request when missing).

2. (Medium/Low) /oauth/callback signed-state + client enforcement had no
   route-level tests. Added coverage: valid signed state+client forwards the
   code, forged client_id, redirect_uri not registered to the client,
   tampered state, missing state, expired state (10-min replay window), and
   the missing-code case from finding 3.

3. (Low) /oauth/callback redirected with code=undefined when Entra returned
   neither error nor code. Now returns 400 invalid_request on a missing code
   instead of forwarding a malformed query.

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

src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts

@@ -11,14 +11,14 @@
  * 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 } from 'vitest';
+import { describe, it, expect, beforeAll, afterAll, vi } 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, signClientId, verifyClientId, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } =
+const { createOAuthRouter, signClientId, verifyClientId, signState, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } =
   await import('../oauth.js');
 
 let server: Server;
@@ -219,6 +219,92 @@ describe('/oauth/authorize — PKCE S256 enforcement', () => {
     // The client's challenge is forwarded unchanged.
     expect(location).toContain('E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM');
   });
+
+  it('rejects a request with no client_id (client_id is mandatory)', async () => {
+    const res = await authorize({
+      redirect_uri: 'http://127.0.0.1:33418/',
+      code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM',
+      code_challenge_method: 'S256',
+    });
+    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(/client_id is required/);
+  });
+});
+
+describe('/oauth/callback — signed state + client enforcement', () => {
+  const REDIRECT = 'http://127.0.0.1:33418/';
+  async function callback(query: Record<string, string>) {
+    const qs = new URLSearchParams(query).toString();
+    return fetch(`${baseUrl}/oauth/callback?${qs}`, { redirect: 'manual' });
+  }
+
+  it('forwards the code to the registered redirect_uri for a valid signed state + client', async () => {
+    const clientId = signClientId([REDIRECT], 'vscode');
+    const state = signState({ originalState: 'client-state', redirectUri: REDIRECT, clientId });
+    const res = await callback({ code: 'ENTRA_CODE', state });
+    expect(res.status).toBe(302);
+    const location = res.headers.get('location') ?? '';
+    expect(location.startsWith(`${REDIRECT}?`)).toBe(true);
+    expect(location).toContain('code=ENTRA_CODE');
+    expect(location).toContain('state=client-state');
+  });
+
+  it('rejects a forged client_id carried in the state', async () => {
+    const state = signState({ redirectUri: REDIRECT, clientId: 'forged.token' });
+    const res = await callback({ code: 'ENTRA_CODE', state });
+    expect(res.status).toBe(400);
+    expect(((await res.json()) as Record<string, unknown>).error).toBe('invalid_request');
+  });
+
+  it('rejects when the state redirect_uri is not registered to the signed client', async () => {
+    const clientId = signClientId(['http://127.0.0.1:1/cb'], 'vscode');
+    const state = signState({ redirectUri: 'http://127.0.0.1:2/cb', clientId });
+    const res = await callback({ code: 'ENTRA_CODE', state });
+    expect(res.status).toBe(400);
+    expect(((await res.json()) as Record<string, unknown>).error).toBe('invalid_request');
+  });
+
+  it('rejects a tampered state (HMAC mismatch)', async () => {
+    const clientId = signClientId([REDIRECT], 'vscode');
+    const state = signState({ redirectUri: REDIRECT, clientId });
+    const res = await callback({ code: 'ENTRA_CODE', state: `x${state}` });
+    expect(res.status).toBe(400);
+    expect(((await res.json()) as Record<string, unknown>).error).toBe('invalid_state');
+  });
+
+  it('rejects a missing state', async () => {
+    const res = await callback({ code: 'ENTRA_CODE' });
+    expect(res.status).toBe(400);
+    expect(((await res.json()) as Record<string, unknown>).error).toBe('invalid_state');
+  });
+
+  it('rejects an expired state (replay after the 10-min window)', async () => {
+    // Fake only Date so the express server / fetch keep using real timers.
+    vi.useFakeTimers({ toFake: ['Date'] });
+    try {
+      vi.setSystemTime(new Date('2020-01-01T00:00:00Z'));
+      const clientId = signClientId([REDIRECT], 'vscode');
+      const state = signState({ redirectUri: REDIRECT, clientId });
+      vi.setSystemTime(new Date('2020-01-01T00:11:00Z')); // +11 min > 10-min max age
+      const res = await callback({ code: 'ENTRA_CODE', state });
+      expect(res.status).toBe(400);
+      expect(((await res.json()) as Record<string, unknown>).error).toBe('invalid_state');
+    } finally {
+      vi.useRealTimers();
+    }
+  });
+
+  it('rejects a callback with neither error nor code', async () => {
+    const clientId = signClientId([REDIRECT], 'vscode');
+    const state = signState({ redirectUri: REDIRECT, clientId });
+    const res = await callback({ state });
+    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(/Missing authorization code/);
+  });
 });
 
 describe('/oauth/token — PKCE finishing move', () => {

src/McpServer/autopilot-monitor-mcp/src/oauth.ts

@@ -469,35 +469,41 @@ export function createOAuthRouter(): Router {
       });
       return;
     }
-    if (client_id) {
-      // The client_id is a self-describing signed token (see signClientId), so
-      // this verifies statelessly — no registry lookup, correct across cold
-      // starts and replicas. A forged/malformed token fails closed with
-      // invalid_client; a valid one must still carry the exact redirect_uri.
-      const registered = verifyClientId(client_id);
-      if (!registered) {
-        console.error('[oauth/authorize] client_id failed signature verification');
-        res.status(400).json({
-          error: 'invalid_client',
-          error_description: 'client_id is invalid — register via /oauth/register first',
-        });
-        return;
-      }
-      if (!registered.redirectUris.includes(redirect_uri)) {
-        console.error(
-          `[oauth/authorize] redirect_uri not registered for this client_id: ${redirect_uri}`,
-        );
-        res.status(400).json({
-          error: 'invalid_request',
-          error_description: 'redirect_uri does not match any URI registered for this client_id',
-        });
-        return;
-      }
-    } else {
-      // If client_id is missing we still allow the flow (the host allowlist
-      // already gates which destinations are reachable), but emit a warning so
-      // misbehaving clients are noticed.
-      console.warn('[oauth/authorize] No client_id supplied — proceeding on host-allowlist gate only');
+    // client_id is REQUIRED. Now that it is a stateless, replica-portable
+    // signed token, there is no cold-start reason to tolerate its absence —
+    // and tolerating it would let a caller bypass the per-client redirect_uri
+    // binding entirely, leaving only the (port-agnostic, loopback-permissive)
+    // host allowlist as the gate. Every conforming MCP client registers via
+    // /oauth/register first, so demanding client_id costs nothing.
+    if (!client_id) {
+      res.status(400).json({
+        error: 'invalid_request',
+        error_description: 'client_id is required — register via /oauth/register first',
+      });
+      return;
+    }
+    // The client_id is a self-describing signed token (see signClientId), so
+    // this verifies statelessly — no registry lookup, correct across cold
+    // starts and replicas. A forged/malformed token fails closed with
+    // invalid_client; a valid one must still carry the exact redirect_uri.
+    const registered = verifyClientId(client_id);
+    if (!registered) {
+      console.error('[oauth/authorize] client_id failed signature verification');
+      res.status(400).json({
+        error: 'invalid_client',
+        error_description: 'client_id is invalid — register via /oauth/register first',
+      });
+      return;
+    }
+    if (!registered.redirectUris.includes(redirect_uri)) {
+      console.error(
+        `[oauth/authorize] redirect_uri not registered for this client_id: ${redirect_uri}`,
+      );
+      res.status(400).json({
+        error: 'invalid_request',
+        error_description: 'redirect_uri does not match any URI registered for this client_id',
+      });
+      return;
     }
 
     // HMAC-signed state: tampering or replay (>10 min) fails verification at
@@ -535,6 +541,16 @@ export function createOAuthRouter(): Router {
       return;
     }
 
+    // No error and no code is a malformed callback — refuse rather than
+    // redirect the client a `code=undefined` query (clean OAuth boundary).
+    if (!code) {
+      res.status(400).json({
+        error: 'invalid_request',
+        error_description: 'Missing authorization code',
+      });
+      return;
+    }
+
     // verifyState rejects tampered, replayed-after-10-min, or malformed state.
     const decoded = state ? verifyState(state) : null;
     if (!decoded) {