From 384ac0d3804cc4337cd0af190dbb0f56aedacc8f Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Mon, 29 Jun 2026 21:19:45 +0200 Subject: [PATCH] fix(mcp-oauth): require client_id, reject empty callback code, test callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/__tests__/oauth-register.test.ts | 90 ++++++++++++++++++- .../autopilot-monitor-mcp/src/oauth.ts | 74 +++++++++------ 2 files changed, 133 insertions(+), 31 deletions(-) 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 index ffc8abd4..3fec3c23 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts +++ b/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; + 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) { + 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).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).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).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).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).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; + expect(json.error).toBe('invalid_request'); + expect(String(json.error_description)).toMatch(/Missing authorization code/); + }); }); describe('/oauth/token — PKCE finishing move', () => { diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 0d61b170..13a7e130 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/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) {