From 84fe7575baece874d3743aa89754b58e7e245212 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 30 Jun 2026 12:30:36 +0200 Subject: [PATCH] fix(mcp-security): host-spoof fail-fast, auth-fetch timeout, layered rate-limit + bounded cache Addresses the 3 findings from the 2026-06-30 MCP security review (v1.4.3 -> 1.4.4). M3 host-header spoofing: centralize getPublicBaseUrl into config.ts (deletes the duplicated copies in oauth.ts + access-guard.ts) and fail-fast at boot in production when MCP_PUBLIC_URL is unset, instead of silently trusting caller-supplied X-Forwarded-Host. Safe for the stage-1 deploy (minReplicas=0 -> no container runs). M2 access-check hang: add AbortSignal.timeout (default 15s, env-overridable) to the /api/auth/mcp fetch -- it was the only fetch without a timeout, on the path every request traverses. Timeout falls through to the existing fail-closed catch. M1 pre-auth flood + unbounded cache: - boundedSet caps accessCache (default 10k, oldest-evict) -> no OOM under a distinct-token flood on the 0.5GiB container. - per-source-IP pre-auth limiter (isPreAuthRateLimited, default 120/min) on the cache-MISS path only, keyed on IP not UPN so a forged token never burns a victim's per-UPN budget. Kept LAYERED with the existing per-UPN limiter (IP for pre-auth, identity for authenticated -- the SaaS-standard split). - app.set('trust proxy', 1) for a spoof-resistant req.ip (single Envoy hop). - parsePositiveInt fixes the RATE_LIMIT NaN-disables-limiter bug and guards all new tunables; cleanup interval prunes the pre-auth bucket map. Monitoring/UX: both 429 paths go through respondRateLimited -> structured greppable log (kind=pre-auth|upn), Retry-After header, and an actionable body message. Tests: parsePositiveInt, boundedSet, isPreAuthRateLimited, getPublicBaseUrl fallback (security-guards.test.ts) + MCP_PUBLIC_URL pin (config-public-url.test.ts). tsc clean; vitest 327 passed / 41 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../autopilot-monitor-mcp/package.json | 2 +- .../src/__tests__/config-public-url.test.ts | 22 ++ .../src/__tests__/security-guards.test.ts | 112 +++++++++- .../autopilot-monitor-mcp/src/access-guard.ts | 199 ++++++++++++++---- .../autopilot-monitor-mcp/src/config.ts | 57 +++++ .../autopilot-monitor-mcp/src/index.ts | 20 +- .../autopilot-monitor-mcp/src/oauth.ts | 14 +- 7 files changed, 361 insertions(+), 65 deletions(-) create mode 100644 src/McpServer/autopilot-monitor-mcp/src/__tests__/config-public-url.test.ts diff --git a/src/McpServer/autopilot-monitor-mcp/package.json b/src/McpServer/autopilot-monitor-mcp/package.json index cff046de..a77a117b 100644 --- a/src/McpServer/autopilot-monitor-mcp/package.json +++ b/src/McpServer/autopilot-monitor-mcp/package.json @@ -1,6 +1,6 @@ { "name": "autopilot-monitor-mcp", - "version": "1.4.3", + "version": "1.4.4", "description": "MCP server for Autopilot-Monitor — enables AI agents to query enrollment sessions, device properties, events and vulnerabilities", "type": "module", "bin": { diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-public-url.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-public-url.test.ts new file mode 100644 index 00000000..c6b1f8b4 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-public-url.test.ts @@ -0,0 +1,22 @@ +/** + * M3 — MCP_PUBLIC_URL pin path. + * + * config.ts captures MCP_PUBLIC_URL into a module-level const at import time, so + * the pin-wins behaviour must be exercised in its own file with the env var set + * BEFORE the dynamic import resolves. The forwarded-header fallback (pin unset) + * is covered in security-guards.test.ts. + */ +import { describe, it, expect } from 'vitest'; + +process.env.MCP_PUBLIC_URL = 'https://pinned.example.net'; +const { getPublicBaseUrl } = await import('../config.js'); + +describe('M3 — getPublicBaseUrl honors the MCP_PUBLIC_URL pin', () => { + it('returns the pin verbatim and ignores spoofable forwarded headers', () => { + const req = { + headers: { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'attacker.tld', host: 'attacker.tld' }, + protocol: 'http', + } as unknown as Parameters[0]; + expect(getPublicBaseUrl(req)).toBe('https://pinned.example.net'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts index b6c1b2e0..0f326cf2 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -294,7 +294,12 @@ describe('L3 — error-handler 5xx body redaction', () => { // 2026-05-08 security review fixes // --------------------------------------------------------------------------- -const { buildCacheKey } = await import('../access-guard.js'); +// Pin a small pre-auth budget BEFORE access-guard loads (it reads the env at +// module init) so the throttle tests are cheap and limit-independent. +const PRE_AUTH_TEST_LIMIT = 5; +process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE = String(PRE_AUTH_TEST_LIMIT); +const { buildCacheKey, boundedSet, isPreAuthRateLimited } = await import('../access-guard.js'); +const { parsePositiveInt, getPublicBaseUrl } = await import('../config.js'); describe('F1 — access-guard cache key includes token hash', () => { it('produces stable keys for the same (upn, token) pair', () => { @@ -476,3 +481,108 @@ describe('list_tenants — extractTenantList keep-list projection', () => { }); }); +// --------------------------------------------------------------------------- +// 2026-06-30 security review fixes (M1 / M2 / M3) +// --------------------------------------------------------------------------- + +describe('M1 — parsePositiveInt env guard (rate-limit NaN bug)', () => { + it('returns the fallback for an undefined env var', () => { + expect(parsePositiveInt(undefined, 60)).toBe(60); + }); + + it('parses a valid positive integer', () => { + expect(parsePositiveInt('120', 60)).toBe(120); + }); + + it('returns the fallback for non-numeric input (would be NaN — disables limiter)', () => { + // The actual bug: a bare parseInt('abc') is NaN, and `count >= NaN` is + // always false, silently disabling the rate limiter. + expect(parsePositiveInt('abc', 60)).toBe(60); + expect(parsePositiveInt('', 60)).toBe(60); + }); + + it('returns the fallback for zero and negative values', () => { + expect(parsePositiveInt('0', 60)).toBe(60); + expect(parsePositiveInt('-5', 60)).toBe(60); + }); +}); + +describe('M1 — boundedSet size cap (accessCache memory bound)', () => { + it('evicts the oldest entry once the cap is reached', () => { + const m = new Map(); + boundedSet(m, 'a', 1, 2); + boundedSet(m, 'b', 2, 2); + boundedSet(m, 'c', 3, 2); // over cap → evict 'a' (oldest) + expect(m.size).toBe(2); + expect(m.has('a')).toBe(false); + expect(m.has('b')).toBe(true); + expect(m.has('c')).toBe(true); + }); + + it('updates an existing key in place without evicting or growing', () => { + const m = new Map(); + boundedSet(m, 'a', 1, 2); + boundedSet(m, 'b', 2, 2); + boundedSet(m, 'a', 99, 2); // re-set existing key — must not evict 'b' + expect(m.size).toBe(2); + expect(m.get('a')).toBe(99); + expect(m.has('b')).toBe(true); + }); + + it('stays bounded under a flood far exceeding the cap', () => { + const m = new Map(); + for (let i = 0; i < 1000; i++) boundedSet(m, `k${i}`, i, 10); + expect(m.size).toBe(10); + // Only the most recent 10 keys survive. + expect(m.has('k999')).toBe(true); + expect(m.has('k989')).toBe(false); + }); +}); + +describe('M1 — isPreAuthRateLimited (per-IP pre-auth throttle)', () => { + // Budget pinned to PRE_AUTH_TEST_LIMIT above. Use a unique IP per test so the + // module-level bucket map does not leak across cases. + let ipSeed = 0; + const uniqueIp = () => `203.0.113.${ipSeed++}`; + + it('allows requests up to the budget, then throttles', () => { + const ip = uniqueIp(); + let throttledAt = -1; + for (let i = 0; i < PRE_AUTH_TEST_LIMIT + 5; i++) { + if (isPreAuthRateLimited(ip)) { throttledAt = i; break; } + } + // The (limit+1)th call is the first to exceed the budget. + expect(throttledAt).toBe(PRE_AUTH_TEST_LIMIT); + }); + + it('tracks distinct IPs independently', () => { + const a = uniqueIp(); + const b = uniqueIp(); + // Burn a's budget; b must be unaffected. + for (let i = 0; i < PRE_AUTH_TEST_LIMIT; i++) isPreAuthRateLimited(a); + expect(isPreAuthRateLimited(a)).toBe(true); + expect(isPreAuthRateLimited(b)).toBe(false); + }); +}); + +describe('M3 — getPublicBaseUrl forwarded-header fallback (no MCP_PUBLIC_URL pin)', () => { + // This file is imported with MCP_PUBLIC_URL unset and NODE_ENV != production, + // so the config module captured no pin and getPublicBaseUrl derives from + // forwarded headers. The pin-wins path is covered in config-public-url.test.ts. + it('prefers X-Forwarded-Host / X-Forwarded-Proto', () => { + const req = { + headers: { 'x-forwarded-proto': 'https', 'x-forwarded-host': 'mcp.example.com', host: 'internal:8080' }, + protocol: 'http', + } as unknown as Parameters[0]; + expect(getPublicBaseUrl(req)).toBe('https://mcp.example.com'); + }); + + it('falls back to req.protocol + Host when no forwarded headers', () => { + const req = { + headers: { host: 'localhost:8080' }, + protocol: 'http', + } as unknown as Parameters[0]; + expect(getPublicBaseUrl(req)).toBe('http://localhost:8080'); + }); +}); + diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index 6193b220..9ef2284a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -11,20 +11,42 @@ import type { Request, Response, NextFunction } from 'express'; import crypto from 'node:crypto'; import { extractTokenClaims, isTokenExpired } from './auth.js'; import { runWithCaller } from './client.js'; -import { API_BASE_URL } from './config.js'; +import { API_BASE_URL, getPublicBaseUrl, parsePositiveInt } from './config.js'; const BASE_URL = API_BASE_URL; -/** - * Derives the public base URL for the WWW-Authenticate header. - */ -function getPublicBaseUrl(req: Request): string { - if (process.env.MCP_PUBLIC_URL) return process.env.MCP_PUBLIC_URL; - const proto = (req.headers['x-forwarded-proto'] as string) ?? req.protocol; - const host = (req.headers['x-forwarded-host'] as string) ?? req.headers.host; - return `${proto}://${host}`; -} -const RATE_LIMIT = parseInt(process.env.MCP_RATE_LIMIT_PER_MINUTE ?? '60', 10); +// Per-UPN request budget (sliding 60s window), applied AFTER a token is +// validated. parsePositiveInt guards against a non-numeric override: a bare +// parseInt would yield NaN, and `count >= NaN` is always false — silently +// disabling the limiter. +const RATE_LIMIT = parsePositiveInt(process.env.MCP_RATE_LIMIT_PER_MINUTE, 60); + +// Per-source-IP budget for UNvalidated requests (sliding 60s window), applied +// only on the cache-MISS path that would call the backend. Bounds forged- / +// distinct-token floods at the source without ever touching a victim's per-UPN +// budget (different key space). It counts ONLY cache misses — a legitimate user +// misses at most ~once per token per 5-min TTL (~0.2/min), and a NAT'd org of N +// active users ~N/5 per min — so the default 120 (= 2× the per-UPN rate) clears +// even a ~600-user shared-IP org while capping flood amplification to ~2 backend +// calls/sec/IP. NOTE: this is per-replica; with maxReplicas=1 it is effectively +// global, but if the app is ever scaled out the effective per-IP ceiling becomes +// 120×replicas (a shared store would be needed for a true global cap). Tune via +// MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE; raise it (or allowlist the egress IP) only +// if a very large shared-IP org reports false 429s. +const PRE_AUTH_RATE_LIMIT = parsePositiveInt(process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE, 120); + +// Hard ceiling on access-cache cardinality. The key is UPN+tokenHash, so a flood +// of distinct (valid-signature) tokens would otherwise grow the map unbounded +// between the 5-min reaper passes — a memory-exhaustion vector on the 0.5 GiB +// container. Oldest entries are evicted on write once the cap is hit. +const MAX_ACCESS_CACHE_ENTRIES = parsePositiveInt(process.env.MCP_ACCESS_CACHE_MAX_ENTRIES, 10_000); + +// Timeout for the backend access-check fetch. Without it a hung/cold backend +// stalls the path EVERY request must traverse. Generous enough to survive a +// cold Functions backend (→ no false 403s), bounded so an unresponsive backend +// cannot pile up held requests. +const ACCESS_CHECK_TIMEOUT_MS = parsePositiveInt(process.env.MCP_ACCESS_CHECK_TIMEOUT_MS, 15_000); + const ACCESS_CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes // --- Access check cache --- @@ -65,6 +87,10 @@ interface AccessCheckResult { // only the former should be surfaced to the user as "you are not enabled — // ask to be whitelisted"; an infra error is not the user's fault. infraError: boolean; + // Set when the per-source-IP pre-auth budget was exhausted on a cache-miss: + // the middleware maps this to 429 (not 403), since it is throttling, not a + // verdict. Never set on a cache hit (a hit consumes no pre-auth budget). + rateLimited?: boolean; } export function buildCacheKey(upn: string, token: string): string { @@ -72,7 +98,21 @@ export function buildCacheKey(upn: string, token: string): string { return `${upn}:${h}`; } -async function checkAccess(upn: string, token: string): Promise { +/** + * Insert into a size-bounded Map, evicting the oldest entry (insertion order) + * when the cap is reached. Keeps memory bounded under a distinct-key flood. + * Exported for unit testing. Re-inserting an existing key updates in place + * without growing the map (the size check is skipped when the key already exists). + */ +export function boundedSet(map: Map, key: K, value: V, cap: number): void { + if (!map.has(key) && map.size >= cap) { + const oldest = map.keys().next().value as K | undefined; + if (oldest !== undefined) map.delete(oldest); + } + map.set(key, value); +} + +async function checkAccess(upn: string, token: string, clientIp: string): Promise { const cacheKey = buildCacheKey(upn, token); const cached = accessCache.get(cacheKey); if (cached && Date.now() < cached.expiresAt) { @@ -89,9 +129,19 @@ async function checkAccess(upn: string, token: string): Promise(); +// Pre-validation, per-source-IP budget (cache-miss path only). Separate key +// space from rateBuckets so throttling unvalidated floods never affects a +// validated user's per-UPN budget. +const preAuthBuckets = new Map(); -// Cleanup stale entries every 5 minutes. Both maps share this interval — -// rateBuckets evicts empty windows, accessCache evicts expired entries -// (the cache key now includes a token hash, so cardinality is one entry per +/** Drop timestamps outside the 60s window from a bucket map; delete empty buckets. */ +function pruneBuckets(buckets: Map, cutoff: number): void { + for (const [key, entry] of buckets) { + entry.timestamps = entry.timestamps.filter((t) => t > cutoff); + if (entry.timestamps.length === 0) buckets.delete(key); + } +} + +// Cleanup stale entries every 5 minutes. All three maps share this interval — +// the rate buckets evict empty windows, accessCache evicts expired entries +// (the cache key includes a token hash, so cardinality is one entry per // user×token-rotation; without proactive eviction the map would only shrink -// when a key is re-fetched). +// when a key is re-fetched or the size cap evicts it). setInterval(() => { const now = Date.now(); const cutoff = now - 60_000; - for (const [key, entry] of rateBuckets) { - entry.timestamps = entry.timestamps.filter((t) => t > cutoff); - if (entry.timestamps.length === 0) rateBuckets.delete(key); - } + pruneBuckets(rateBuckets, cutoff); + pruneBuckets(preAuthBuckets, cutoff); for (const [key, entry] of accessCache) { if (entry.expiresAt <= now) accessCache.delete(key); } }, 5 * 60_000); -function isRateLimited(upn: string): boolean { +/** + * Sliding-window check against a bucket map. Returns true when the key has + * already used its full budget in the last 60s; otherwise records the hit. + */ +function isWindowExceeded(buckets: Map, key: string, limit: number): boolean { const now = Date.now(); const windowStart = now - 60_000; - let entry = rateBuckets.get(upn); + let entry = buckets.get(key); if (!entry) { entry = { timestamps: [] }; - rateBuckets.set(upn, entry); + buckets.set(key, entry); } - - // Remove timestamps outside the window entry.timestamps = entry.timestamps.filter((t) => t > windowStart); - if (entry.timestamps.length >= RATE_LIMIT) { - return true; - } - + if (entry.timestamps.length >= limit) return true; entry.timestamps.push(now); return false; } +/** + * Per-source-IP throttle for UNvalidated, cache-missing requests. Exported for + * unit testing. 'unknown' (no resolvable IP) shares a single bucket — acceptable + * since the limit is generous and this is a backstop, not the primary gate. + */ +export function isPreAuthRateLimited(clientIp: string): boolean { + return isWindowExceeded(preAuthBuckets, clientIp, PRE_AUTH_RATE_LIMIT); +} + +/** + * Best-effort client IP for the pre-auth throttle. With `app.set('trust proxy', 1)` + * (index.ts) and the single Container Apps Envoy ingress hop, req.ip is the real + * client address and is not spoofable by prepending X-Forwarded-For entries. + * Falls back to the socket address, then 'unknown'. + */ +function getClientIp(req: Request): string { + return req.ip ?? req.socket?.remoteAddress ?? 'unknown'; +} + +function isRateLimited(upn: string): boolean { + return isWindowExceeded(rateBuckets, upn, RATE_LIMIT); +} + +/** + * Emit a 429 with the conventional Retry-After header, a structured + greppable + * log line, and an actionable message. `kind` distinguishes the per-source-IP + * pre-auth throttle from the per-UPN throttle so a Log Analytics alert can break + * the two out and so an unexpected false-positive surfaces to the user (who is + * told to contact the administrator). + */ +function respondRateLimited(res: Response, kind: 'pre-auth' | 'upn', key: string, rpcMethod: string): void { + console.error(`[mcp-auth] 429 kind=${kind} key=${key} method=${rpcMethod}`); + res.setHeader('Retry-After', '60'); + res.status(429).json({ + error: 'Rate limit exceeded', + retryAfterSeconds: 60, + message: + kind === 'pre-auth' + ? 'Too many requests from your network in a short window. Wait ~60s and retry. If this keeps ' + + 'happening unexpectedly, you may be sharing an IP via a corporate proxy/NAT — contact the MCP ' + + 'server administrator.' + : 'You are sending requests faster than the per-user limit. Wait ~60s and retry. If this keeps ' + + 'happening unexpectedly, contact the MCP server administrator.', + }); +} + // --- Express middleware --- export function accessGuard(req: Request, res: Response, next: NextFunction): void { @@ -237,19 +344,25 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo } const upn = claims.upn.toLowerCase(); + const clientIp = getClientIp(req); - // Access check (async — calls backend, cached). Must run BEFORE rate-limit - // accounting: the JWT signature is not verified here (only the backend has - // JWKS access), so an attacker can forge an unsigned token with a victim's - // UPN. If rate-limit incremented before validation, those forgeries would - // burn the victim's per-minute budget and 429 their legitimate calls. - // checkAccess delegates signature verification to the backend; both - // allow- and deny-decisions are cached for 5 min keyed on UPN+tokenHash, - // so a forged token cannot piggyback on a legitimate user's cached - // `allowed: true`, and forged-token floods cost one backend call per - // distinct token. - checkAccess(upn, token) + // Access check (async — calls backend, cached). Must run BEFORE the per-UPN + // rate-limit accounting: the JWT signature is not verified here (only the + // backend has JWKS access), so an attacker can forge an unsigned token with a + // victim's UPN. If the per-UPN limiter incremented before validation, those + // forgeries would burn the victim's per-minute budget and 429 their + // legitimate calls. checkAccess delegates signature verification to the + // backend; both allow- and deny-decisions are cached for 5 min keyed on + // UPN+tokenHash, so a forged token cannot piggyback on a legitimate user's + // cached `allowed: true`. On a cache MISS, checkAccess additionally applies a + // per-source-IP throttle (keyed on clientIp, not UPN) so a distinct-token + // flood cannot drive unbounded backend calls. + checkAccess(upn, token, clientIp) .then((result) => { + if (result.rateLimited) { + respondRateLimited(res, 'pre-auth', clientIp, rpcMethod); + return; + } if (!result.allowed) { if (result.infraError) { // Backend could not reach a verdict (unreachable / malformed). Fail @@ -279,7 +392,7 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo // else's UPN was already rejected above; only a real, allow-listed // user reaches this counter. if (isRateLimited(upn)) { - res.status(429).json({ error: 'Rate limit exceeded', retryAfterSeconds: 60 }); + respondRateLimited(res, 'upn', upn, rpcMethod); return; } diff --git a/src/McpServer/autopilot-monitor-mcp/src/config.ts b/src/McpServer/autopilot-monitor-mcp/src/config.ts index e4313867..480e9d47 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/config.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/config.ts @@ -9,6 +9,8 @@ * fast in production rather than defaulting (matching the fail-fast posture in * oauth.ts). */ +import type { Request } from 'express'; + const DEFAULT_API_BASE_URL = 'https://autopilotmonitor-api.azurewebsites.net'; function resolveApiBaseUrl(): string { @@ -25,3 +27,58 @@ function resolveApiBaseUrl(): string { /** Resolved backend API base URL (no trailing slash). */ export const API_BASE_URL = resolveApiBaseUrl(); + +/** + * Parse a positive-integer environment variable, falling back to `fallback` + * for missing, non-numeric, or non-positive values. A bare `parseInt` returns + * NaN for garbage input, and `NaN` silently breaks every `>=` comparison it + * feeds — e.g. an unguarded rate-limit cap of NaN makes `count >= NaN` always + * false, disabling the limiter entirely. Centralized so every tunable (rate + * limits, cache caps, timeouts) is validated the same way. + */ +export function parsePositiveInt(raw: string | undefined, fallback: number): number { + if (raw === undefined) return fallback; + const n = Number.parseInt(raw, 10); + return Number.isFinite(n) && n > 0 ? n : fallback; +} + +/** + * Public base URL of this MCP server, used verbatim for OAuth issuer / + * WWW-Authenticate / redirect metadata. + * + * Pinned via MCP_PUBLIC_URL. When unset, it is derived from the + * X-Forwarded-Host / X-Forwarded-Proto headers set by the Container Apps + * ingress — but those headers are caller-supplied and spoofable, so a request + * with a forged Host could make the server advertise an attacker-chosen issuer + * / redirect / WWW-Authenticate metadata URL. In production we therefore REFUSE + * to start without the pin (mirrors the AUTOPILOT_API_URL fail-fast above). + * + * This does not break the documented two-stage deploy (infra/mcp-server.bicep): + * the stage-1 deploy that exists only to emit the containerAppUrl runs with + * minReplicas=0, so no container boots and this check never fires; the operator + * then re-deploys with mcpPublicUrl pinned before any traffic arrives. + */ +const MCP_PUBLIC_URL = process.env.MCP_PUBLIC_URL?.trim() || undefined; +if (!MCP_PUBLIC_URL && process.env.NODE_ENV === 'production') { + throw new Error( + 'MCP_PUBLIC_URL must be set in production — refusing to derive the OAuth issuer / ' + + 'redirect_uri / WWW-Authenticate metadata from caller-supplied X-Forwarded-Host headers ' + + '(host-spoofing risk). Pin it via the bicep `mcpPublicUrl` parameter (two-stage deploy).', + ); +} + +export { MCP_PUBLIC_URL }; + +/** + * Derives the public base URL of this MCP server for the current request. + * Prefers the MCP_PUBLIC_URL pin (production); falls back to the forwarded + * headers only in dev, where the boot-time guard above is not enforced. + * Typed structurally on the two fields it reads so config.ts needs no runtime + * express dependency. + */ +export function getPublicBaseUrl(req: Pick): string { + if (MCP_PUBLIC_URL) return MCP_PUBLIC_URL; + const proto = (req.headers['x-forwarded-proto'] as string) ?? req.protocol; + const host = (req.headers['x-forwarded-host'] as string) ?? (req.headers.host as string); + return `${proto}://${host}`; +} diff --git a/src/McpServer/autopilot-monitor-mcp/src/index.ts b/src/McpServer/autopilot-monitor-mcp/src/index.ts index 0e7e1eaf..6c1b5c1d 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/index.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/index.ts @@ -26,16 +26,15 @@ const SERVER_VERSION: string = pkg.version; const PORT = parseInt(process.env.PORT ?? '8080', 10); const RULES_DIR = process.env.RULES_DIR ?? resolve(__dirname, '..', '..', '..', '..', 'rules'); -// Surface the MCP_PUBLIC_URL state at boot so a missing public-URL pin is -// visible in container logs, not silently fallen back to forwarded-headers -// in production. Two-stage deploy expectation: first deploy → containerAppUrl -// output → second deploy with mcpPublicUrl pinned. See infra/mcp-server.bicep. +// Surface the MCP_PUBLIC_URL state at boot. In production a missing pin is a +// hard boot failure (config.ts throws — host-spoofing defense); this dev-only +// notice flags the forwarded-header fallback. Two-stage deploy expectation: +// first deploy → containerAppUrl output → second deploy with mcpPublicUrl +// pinned. See infra/mcp-server.bicep. if (!process.env.MCP_PUBLIC_URL) { console.error( '[startup] MCP_PUBLIC_URL is not set — OAuth issuer / WWW-Authenticate / ' + - 'redirect metadata will be derived from X-Forwarded-* headers. This is ' + - 'acceptable for local dev only; in production, set MCP_PUBLIC_URL to the ' + - 'Container App FQDN (re-deploy with the bicep `mcpPublicUrl` parameter).', + 'redirect metadata will be derived from X-Forwarded-* headers (dev only).', ); } @@ -182,6 +181,13 @@ function createMcpServer(ga: boolean, strictGa: boolean, delegated: boolean, man const app = express(); +// Single Container Apps Envoy ingress hop sits in front of this server. Trusting +// exactly one proxy makes req.ip the real downstream client address (the entry +// the ingress appended to X-Forwarded-For), and NOT spoofable by a client +// prepending its own X-Forwarded-For entries — which the per-source-IP pre-auth +// throttle in access-guard relies on. +app.set('trust proxy', 1); + // Tight body-size limit for /oauth/register, registered BEFORE the global // parser so the smaller limit wins (the global parser's body-already-parsed // short-circuit then skips re-parsing). RFC 7591 registration requests carry diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 8f29901a..ff3e44ec 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -18,6 +18,7 @@ */ import { Router } from 'express'; import crypto from 'node:crypto'; +import { getPublicBaseUrl } from './config.js'; const CLIENT_ID: string = (() => { const v = process.env.AUTOPILOT_ENTRA_CLIENT_ID; @@ -342,19 +343,6 @@ export function redirectUriMatches(registered: string[], requested: string): boo return registered.some((r) => canonicalizeRedirectUri(r) === target); } -/** - * Derives the public base URL of this MCP server. - * In production: from X-Forwarded-Host / X-Forwarded-Proto (set by Container Apps ingress). - * Fallback: MCP_PUBLIC_URL env var or localhost. - */ -function getPublicBaseUrl(req: import('express').Request): string { - if (process.env.MCP_PUBLIC_URL) return process.env.MCP_PUBLIC_URL; - - const proto = (req.headers['x-forwarded-proto'] as string) ?? req.protocol; - const host = (req.headers['x-forwarded-host'] as string) ?? req.headers.host; - return `${proto}://${host}`; -} - export function createOAuthRouter(): Router { const router = Router();