diff --git a/.claude/commands b/.claude/commands index 79ab9e76..b3903978 160000 --- a/.claude/commands +++ b/.claude/commands @@ -1 +1 @@ -Subproject commit 79ab9e7658029abf4262d2358fde79aec5730bf0 +Subproject commit b39039789197c83e3911603fdd7951c6e25af41e diff --git a/infra b/infra index 55a8eaf9..742249e7 160000 --- a/infra +++ b/infra @@ -1 +1 @@ -Subproject commit 55a8eaf9290c3cf1ee574593789d300a89876123 +Subproject commit 742249e7e308159df0eaa2942aad8873dc088f1a diff --git a/internal b/internal index aecb8467..398d32a4 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit aecb8467c4c99a3526ed83a3dda237a04fdc9073 +Subproject commit 398d32a48bd2f0fabca8746ab17272357a1d7241 diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-401-and-cancel.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-401-and-cancel.test.ts new file mode 100644 index 00000000..2acc6deb --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-401-and-cancel.test.ts @@ -0,0 +1,139 @@ +/** + * Two request-gate contracts (2026-09-10): + * - a token the backend refuses (401) is answered 401 + WWW-Authenticate error="invalid_token" + * (spec: invalid or expired tokens MUST receive 401), cached like a deny, never 403 "not enabled"; + * - the caller context carries a cancellation signal that fires when the client closes the request + * before the response is written (Streamable HTTP: closing the response stream cancels the request). + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import type { Request, Response, NextFunction } from 'express'; +import { accessGuard } from '../access-guard.js'; +import { getCallerSignal } from '../client.js'; + +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`; +} + +let seed = 0; +/** A fresh, unexpired token per call — the guard caches verdicts per UPN+token. */ +function request(): Request { + seed += 1; + const token = makeToken({ upn: `user${seed}@contoso.com`, exp: Math.floor(Date.now() / 1000) + 3600 }); + return { + headers: { host: 'mcp.example.com', authorization: `Bearer ${token}` }, + protocol: 'https', + body: { jsonrpc: '2.0', method: 'tools/list', id: 1 }, + } as unknown as Request; +} + +interface Outcome { + nextCalled: boolean; + status: number | null; + body: unknown; + headers: Record; + /** The caller context's signal, read INSIDE next(). */ + signal?: AbortSignal; + res: { writableFinished: boolean }; + /** Emits the response's 'close' event the way Node does when the socket goes away. */ + close: () => void; +} + +function run(req: Request): Promise { + return new Promise((resolve) => { + const headers: Record = {}; + const closeHandlers: Array<() => void> = []; + let status: number | null = null; + let settled = false; + const state = { writableFinished: false }; + const close = () => closeHandlers.forEach((h) => h()); + const finish = (o: Omit) => { + if (settled) return; + settled = true; + resolve({ ...o, res: state, close }); + }; + const res = { + get writableFinished() { return state.writableFinished; }, + setHeader(name: string, value: string) { headers[name] = String(value); return this; }, + status(code: number) { status = code; return this; }, + json(payload: unknown) { finish({ nextCalled: false, status, body: payload, headers }); return this; }, + on(event: string, handler: () => void) { if (event === 'close') closeHandlers.push(handler); return this; }, + } as unknown as Response; + const next: NextFunction = () => finish({ nextCalled: true, status, body: undefined, headers, signal: getCallerSignal() }); + accessGuard(req, res, next); + setTimeout(() => finish({ nextCalled: false, status, body: undefined, headers }), 2_000); + }); +} + +function stubBackend(status: number, body: unknown) { + const fn = vi.fn(async () => ({ status, text: async () => JSON.stringify(body) } as unknown as Response)); + vi.stubGlobal('fetch', fn); + return fn; +} + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('accessGuard — a backend 401 becomes 401 + challenge', () => { + it('answers 401 with error="invalid_token" and the backend reason, not 403', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + stubBackend(401, { error: 'Token signature is invalid', code: 'unauthorized' }); + const out = await run(request()); + expect(out.nextCalled).toBe(false); + expect(out.status).toBe(401); + expect(out.headers['WWW-Authenticate']).toContain('error="invalid_token"'); + expect(out.headers['WWW-Authenticate']).toContain('oauth-protected-resource/mcp'); + expect(out.body).toMatchObject({ error: 'Invalid token', reason: 'Token signature is invalid' }); + }); + + it('caches the rejection per token: a replay costs no second backend call and stays 401', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const fetchMock = stubBackend(401, { error: 'Token expired' }); + const req = request(); + const first = await run(req); + const second = await run(req); + expect(first.status).toBe(401); + expect(second.status).toBe(401); + expect(second.headers['WWW-Authenticate']).toContain('error="invalid_token"'); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('keeps a 401 without an envelope body at 401 with a generic reason', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + stubBackend(401, 'Unauthorized'); + const out = await run(request()); + expect(out.status).toBe(401); + expect(out.body).toMatchObject({ reason: 'The backend rejected the token' }); + }); + + it('keeps a genuine authorization deny (200, allowed:false) at 403 without a challenge', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + stubBackend(200, { allowed: false, upn: 'x', accessGrant: '', reason: 'not on the MCP allowlist' }); + const out = await run(request()); + expect(out.status).toBe(403); + expect(out.headers['WWW-Authenticate']).toBeUndefined(); + }); +}); + +describe('accessGuard — cancellation signal', () => { + it('hands the caller context a signal that fires when the client closes the request early', async () => { + stubBackend(200, { allowed: true, upn: 'x', accessGrant: 'ok' }); + const out = await run(request()); + expect(out.nextCalled).toBe(true); + expect(out.signal).toBeInstanceOf(AbortSignal); + expect(out.signal?.aborted).toBe(false); + out.close(); + expect(out.signal?.aborted).toBe(true); + }); + + it('does not fire when the response completed normally before the socket closed', async () => { + stubBackend(200, { allowed: true, upn: 'x', accessGrant: 'ok' }); + const out = await run(request()); + out.res.writableFinished = true; + out.close(); + expect(out.signal?.aborted).toBe(false); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts index 13761c9d..bdf8286d 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts @@ -45,6 +45,7 @@ function runGuard(req: Request): Promise { setHeader() { return this as Response; }, status(code: number) { statusCode = code; return this as Response; }, json() { done({ nextCalled: false, status: statusCode }); return this as Response; }, + on() { return this as Response; }, }; const next: NextFunction = () => done({ nextCalled: true, status: statusCode }); accessGuard(req, res as Response, next); 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 index 158c6808..211a394a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard.test.ts @@ -68,6 +68,10 @@ function mockRes(): CapturedRes { captured.body = payload; return this as Response; }, + // The guard subscribes to 'close' for the cancellation signal; nothing here ever emits it. + on() { + return this as Response; + }, }; captured.res = res as Response; return captured; diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/apifetch-abort.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/apifetch-abort.test.ts new file mode 100644 index 00000000..4cc194e3 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/apifetch-abort.test.ts @@ -0,0 +1,59 @@ +/** + * Client cancellation reaches the backend: the caller context's signal (aborted by the access + * guard when the client closes the request) is folded into every apiFetch attempt and into the + * automatic 429/503 retry wait, so an abandoned tool call stops its backend work at once. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { apiFetch, runWithCaller } from '../client.js'; + +afterEach(() => vi.restoreAllMocks()); + +function caller(signal: AbortSignal) { + return { token: 'test-token', isGlobalAdmin: true, isGlobalReader: false, signal }; +} + +describe('apiFetch — caller cancellation', () => { + it("aborts the in-flight backend fetch when the caller's signal fires", async () => { + let seen: AbortSignal | undefined; + vi.spyOn(globalThis, 'fetch').mockImplementation((_url, init) => new Promise((_resolve, reject) => { + seen = init?.signal ?? undefined; + seen?.addEventListener('abort', () => reject(seen?.reason), { once: true }); + })); + const controller = new AbortController(); + const pending = runWithCaller(caller(controller.signal), () => apiFetch('/api/sessions')); + + expect(seen).toBeDefined(); + expect(seen?.aborted).toBe(false); + controller.abort(); + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + expect(seen?.aborted).toBe(true); + }); + + it('ends the automatic retry wait at once on cancellation instead of sleeping out Retry-After', async () => { + const fetchMock = vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: false, + status: 429, + headers: { get: (name: string) => (name.toLowerCase() === 'retry-after' ? '5' : null) }, + text: async () => '', + } as unknown as Response); + const controller = new AbortController(); + const started = Date.now(); + const pending = runWithCaller(caller(controller.signal), () => apiFetch('/api/sessions')); + setTimeout(() => controller.abort(), 20); + + await expect(pending).rejects.toMatchObject({ name: 'AbortError' }); + expect(Date.now() - started).toBeLessThan(2000); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('keeps the plain per-attempt timeout when the caller context carries no signal', async () => { + let seen: AbortSignal | undefined; + vi.spyOn(globalThis, 'fetch').mockImplementation(async (_url, init) => { + seen = init?.signal ?? undefined; + return { ok: true, status: 200, headers: { get: () => null }, text: async () => '{"ok":true}' } as unknown as Response; + }); + await runWithCaller({ token: 'test-token', isGlobalAdmin: true }, () => apiFetch('/api/sessions')); + expect(seen).toBeDefined(); + expect(seen?.aborted).toBe(false); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/arg-policy-error-text.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/arg-policy-error-text.test.ts new file mode 100644 index 00000000..a135917c --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/arg-policy-error-text.test.ts @@ -0,0 +1,49 @@ +/** + * The per-argument echo policy a tool declares at its withToolTelemetry call site governs BOTH + * sinks that quote the arguments: the tool_call log line and the "Parameters used" block of an + * error result (whose first lines the log quotes). Without this, a clear-text config value masked + * in the args summary still reached the container log through the error text. + */ +import { describe, it, expect } from 'vitest'; +import { toolError } from '../tools/error-handler.js'; +import { createToolCallContext, runWithToolCallContext } from '../client.js'; + +const TENANT = 'a1b2c3d4-e5f6-7890-abcd-ef1234567890'; +const SECRET = 'https://hooks.example/services/secret-token-123'; + +function errorText(result: unknown): string { + return (result as { content: Array<{ text: string }> }).content[0].text; +} + +describe('toolError — parameter echo honours the call\'s ArgPolicy', () => { + it("renders only the field NAMES of a 'keys' argument, so no config value enters the error text", () => { + const args = { tenantId: TENANT, fields: { teamsWebhookUrl: SECRET, dataRetentionDays: 4242 }, reason: 'rotate' }; + const result = runWithToolCallContext( + createToolCallContext('update_tenant_config', 'cid', { fields: 'keys' }), + () => toolError('update_tenant_config', args, new Error('boom')), + ); + const text = errorText(result); + expect(text).toContain('**Parameters used**'); + expect(text).toContain('fields: teamsWebhookUrl,dataRetentionDays'); + expect(text).toContain('reason: rotate'); + expect(text).not.toContain(SECRET); + expect(text).not.toContain('4242'); + }); + + it("omits a 'drop' argument entirely", () => { + const result = runWithToolCallContext( + createToolCallContext('t', 'cid', { secret: 'drop' }), + () => toolError('t', { secret: 'hunter2', q: 'x' }, new Error('boom')), + ); + expect(errorText(result)).not.toContain('hunter2'); + expect(errorText(result)).not.toContain('secret'); + expect(errorText(result)).toContain('q: x'); + }); + + it('renders values verbatim (size-capped) without a policy — the pre-existing behaviour', () => { + const text = errorText(toolError('search_sessions', { status: 'failed', pageSize: 50, days: null }, new Error('boom'))); + expect(text).toContain('status: failed'); + expect(text).toContain('pageSize: 50'); + expect(text).not.toContain('days'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-write-tools.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-write-tools.test.ts index a83fad60..d1a7b828 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-write-tools.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/config-write-tools.test.ts @@ -101,6 +101,7 @@ describe('revert_tenant_config — request shape', () => { apiFetchMock.mockResolvedValueOnce({ success: true }); await captureToolHandlers(true, true).revert_tenant_config({ tenantId: TENANT, + backupId: '20260910T000000Z_abcd1234', reason: 'undo bad change', }); @@ -108,7 +109,7 @@ describe('revert_tenant_config — request shape', () => { expect(path).toBe(`/api/config/${TENANT}/revert`); expect(options.method).toBe('POST'); expect(JSON.parse(options.body as string)).toEqual({ - backupId: undefined, + backupId: '20260910T000000Z_abcd1234', includeProtectedFields: false, reason: 'undo bad change', }); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/mcp-http-origin.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/mcp-http-origin.test.ts new file mode 100644 index 00000000..c526f829 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/mcp-http-origin.test.ts @@ -0,0 +1,97 @@ +/** + * The Origin gate in front of the dual-era MCP entry, driven over loopback HTTP the way index.ts + * mounts it (express.json → originGuard → handler): a foreign Origin is refused with 403 on a 2025 + * initialize and on a 2026-07-28 request alike; the server's own origin and no Origin pass. + */ +import { describe, it, expect, beforeAll, afterAll, vi, afterEach } from 'vitest'; +import express from 'express'; +import type { Server } from 'node:http'; +import { createMcpRequestHandler } from '../mcp-http.js'; +import { createServerForCaller, type ServerDeps } from '../mcp-server-factory.js'; +import { runWithCaller } from '../client.js'; +import { originGuard } from '../origin-guard.js'; + +const DEPS: ServerDeps = { serverVersion: '0.0.0-test', knowledgeBase: undefined, eventTypeIndex: undefined, docs: undefined }; + +let httpServer: Server; +let url: URL; + +beforeAll(async () => { + const app = express(); + app.use('/mcp', express.json({ limit: '256kb' })); + app.use('/mcp', originGuard); + const handler = createMcpRequestHandler(() => createServerForCaller(DEPS), {}); + app.post('/mcp', (req, res) => + runWithCaller({ token: 'test-token', isGlobalAdmin: true, isGlobalReader: false, upn: 'caller@contoso.com' }, () => handler(req, res)), + ); + await new Promise((resolve) => { + httpServer = app.listen(0, '127.0.0.1', () => resolve()); + }); + const addr = httpServer.address(); + if (typeof addr !== 'object' || addr === null) throw new Error('no address'); + url = new URL(`http://127.0.0.1:${addr.port}/mcp`); +}); + +afterAll(async () => { + await new Promise((resolve) => httpServer.close(() => resolve())); +}); + +afterEach(() => vi.restoreAllMocks()); + +const LEGACY_INITIALIZE = { + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { protocolVersion: '2025-11-25', capabilities: {}, clientInfo: { name: 'origin-test', version: '1.0.0' } }, +}; + +const MODERN_TOOLS_LIST = { + jsonrpc: '2.0', + id: 1, + method: 'tools/list', + params: { + _meta: { + 'io.modelcontextprotocol/protocolVersion': '2026-07-28', + 'io.modelcontextprotocol/clientInfo': { name: 'origin-test', version: '1.0.0' }, + 'io.modelcontextprotocol/clientCapabilities': {}, + }, + }, +}; + +function post(body: unknown, headers: Record): Promise { + return fetch(url, { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream', ...headers }, + body: JSON.stringify(body), + }); +} + +describe('Origin gate on /mcp — both protocol eras', () => { + it('refuses a foreign Origin on a 2025 initialize: 403 + JSON-RPC error without id, and a log line', async () => { + const log = vi.spyOn(console, 'error').mockImplementation(() => {}); + const res = await post(LEGACY_INITIALIZE, { origin: 'https://attacker.example' }); + expect(res.status).toBe(403); + expect(await res.json()).toMatchObject({ jsonrpc: '2.0', error: { code: -32600 }, id: null }); + expect(log).toHaveBeenCalledWith(expect.stringContaining('403 origin-rejected origin=https://attacker.example method=initialize')); + }); + + it('refuses a foreign Origin on a 2026-07-28 request', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const res = await post(MODERN_TOOLS_LIST, { + origin: 'https://attacker.example', + 'mcp-protocol-version': '2026-07-28', + 'mcp-method': 'tools/list', + }); + expect(res.status).toBe(403); + }); + + it("serves the server's own origin", async () => { + const res = await post(LEGACY_INITIALIZE, { origin: url.origin, 'mcp-protocol-version': '2025-11-25' }); + expect(res.status).toBe(200); + }); + + it('serves a request without Origin (every non-browser client)', async () => { + const res = await post(LEGACY_INITIALIZE, { 'mcp-protocol-version': '2025-11-25' }); + expect(res.status).toBe(200); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-signing-key.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-signing-key.test.ts new file mode 100644 index 00000000..779e9201 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-signing-key.test.ts @@ -0,0 +1,36 @@ +/** + * OAuthStateSigningKey boot guard: production refuses to boot without the key (the signed + * client_ids and the OAuth state derive from it and must survive restarts and replicas); the + * per-process random fallback exists for development and tests only; a short key fails everywhere. + */ +import { describe, it, expect } from 'vitest'; + +// oauth.ts throws at import unless the Entra client id is present. +process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +const { loadSigningKey } = await import('../oauth.js'); + +const KEY = Buffer.alloc(32, 7).toString('base64'); + +describe('loadSigningKey', () => { + it('decodes a valid base64 key of 32+ bytes in production', () => { + expect(loadSigningKey(KEY, 'production').length).toBe(32); + }); + + it('refuses to boot in production without a key', () => { + expect(() => loadSigningKey(undefined, 'production')).toThrow('OAuthStateSigningKey is not set'); + expect(() => loadSigningKey('', 'production')).toThrow('OAuthStateSigningKey is not set'); + }); + + it('falls back to a fresh random key outside production', () => { + const a = loadSigningKey(undefined, 'test'); + const b = loadSigningKey(undefined, undefined); + expect(a.length).toBe(32); + expect(b.length).toBe(32); + expect(a.equals(b)).toBe(false); + }); + + it('rejects a key shorter than 32 bytes in every environment', () => { + expect(() => loadSigningKey(Buffer.alloc(16).toString('base64'), 'test')).toThrow('32 bytes'); + expect(() => loadSigningKey('not base64 at all', 'production')).toThrow('32 bytes'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-token-error-body.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-token-error-body.test.ts new file mode 100644 index 00000000..4a1776c1 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-token-error-body.test.ts @@ -0,0 +1,61 @@ +/** + * /oauth/token relays Entra's error body through an allowlist: the OAuth error code, the human + * message (cut before the trace-id tail, capped) and the numeric AADSTS codes. Trace, correlation, + * timestamp, error_uri and claims never reach an unauthenticated caller (review R-5). + */ +import { describe, it, expect } from 'vitest'; + +// oauth.ts throws at import unless the Entra client id is present. +process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +const { sanitizeTokenErrorBody } = await import('../oauth.js'); + +const ENTRA_INVALID_GRANT = { + error: 'invalid_grant', + error_description: + 'AADSTS70008: The provided authorization code or refresh token has expired due to inactivity. ' + + 'Send a new interactive authorization request for this user and resource. Trace ID: 5d1a2b3c-0000-4000-8000-000000000000\r\n' + + 'Correlation ID: 7e9f0a1b-0000-4000-8000-000000000000\r\nTimestamp: 2026-09-10 00:00:00Z', + error_codes: [70008], + timestamp: '2026-09-10 00:00:00Z', + trace_id: '5d1a2b3c-0000-4000-8000-000000000000', + correlation_id: '7e9f0a1b-0000-4000-8000-000000000000', + error_uri: 'https://login.microsoftonline.com/error?code=70008', + claims: '{"access_token":{"capolids":{"essential":true,"values":["x"]}}}', +}; + +describe('sanitizeTokenErrorBody', () => { + it('keeps the OAuth error, the message before the trace tail, and the numeric codes only', () => { + expect(sanitizeTokenErrorBody(ENTRA_INVALID_GRANT)).toEqual({ + error: 'invalid_grant', + error_description: + 'AADSTS70008: The provided authorization code or refresh token has expired due to inactivity. ' + + 'Send a new interactive authorization request for this user and resource.', + error_codes: [70008], + }); + }); + + it('never carries trace, correlation, timestamp, error_uri or claims', () => { + const out = sanitizeTokenErrorBody(ENTRA_INVALID_GRANT); + for (const key of ['trace_id', 'correlation_id', 'timestamp', 'error_uri', 'claims']) expect(out).not.toHaveProperty(key); + expect(JSON.stringify(out)).not.toContain('5d1a2b3c'); + expect(JSON.stringify(out)).not.toContain('7e9f0a1b'); + }); + + it('maps an unknown error code to invalid_request', () => { + expect(sanitizeTokenErrorBody({ error: 'something_entra_specific', error_description: 'x' }).error).toBe('invalid_request'); + }); + + it('caps a long description', () => { + const out = sanitizeTokenErrorBody({ error: 'invalid_request', error_description: 'y'.repeat(1000) }); + expect((out.error_description as string).length).toBe(303); + expect(out.error_description as string).toMatch(/\.\.\.$/); + }); + + it('tolerates a non-object body and non-numeric codes', () => { + expect(sanitizeTokenErrorBody('not json')).toEqual({ error: 'invalid_request' }); + expect(sanitizeTokenErrorBody({ error: 'invalid_client', error_codes: ['7000218', 7000218] })).toEqual({ + error: 'invalid_client', + error_codes: [7000218], + }); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/origin-guard.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/origin-guard.test.ts new file mode 100644 index 00000000..ccc55976 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/origin-guard.test.ts @@ -0,0 +1,91 @@ +/** + * Origin gate on /mcp (spec 2026-07-28 Streamable HTTP "Security & Endpoint": a present, invalid + * Origin MUST be answered 403). Non-browser clients send no Origin and pass; only the server's own + * origin passes with one; everything else — foreign hosts, scheme downgrades, suffix tricks, the + * opaque `null` — is refused before any token work. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import type { Request, Response } from 'express'; +import { isAllowedOrigin, originGuard } from '../origin-guard.js'; + +const SELF = 'https://mcp.example.com'; + +describe('isAllowedOrigin', () => { + it('passes a request without Origin (every non-browser MCP client)', () => { + expect(isAllowedOrigin(undefined, SELF)).toBe(true); + }); + + it("accepts the server's own origin, port-normalised and case-insensitive on the host", () => { + expect(isAllowedOrigin('https://mcp.example.com', SELF)).toBe(true); + expect(isAllowedOrigin('https://MCP.example.com:443', SELF)).toBe(true); + }); + + it.each([ + 'https://evil.example', + 'http://mcp.example.com', + 'https://mcp.example.com.evil.example', + 'https://mcp.example.com:8443', + 'null', + 'not a url', + '', + ])('rejects a present, invalid Origin: %j', (origin) => { + expect(isAllowedOrigin(origin, SELF)).toBe(false); + }); +}); + +describe('originGuard middleware', () => { + afterEach(() => vi.restoreAllMocks()); + + function run(origin?: string) { + const req = { + headers: { host: 'mcp.example.com', ...(origin !== undefined ? { origin } : {}) }, + protocol: 'https', + body: { jsonrpc: '2.0', method: 'tools/list', id: 1 }, + } as unknown as Request; + let status: number | null = null; + let body: unknown; + const res = { + status(code: number) { status = code; return this; }, + json(payload: unknown) { body = payload; return this; }, + } as unknown as Response; + const next = vi.fn(); + originGuard(req, res, next); + return { status: status as number | null, body, next }; + } + + it('lets a request without Origin through untouched', () => { + const { status, next } = run(); + expect(next).toHaveBeenCalledOnce(); + expect(status).toBeNull(); + }); + + it("lets the server's own origin through", () => { + const { status, next } = run('https://mcp.example.com'); + expect(next).toHaveBeenCalledOnce(); + expect(status).toBeNull(); + }); + + it('answers 403 with a JSON-RPC error without id for a foreign origin, and logs it', () => { + const log = vi.spyOn(console, 'error').mockImplementation(() => {}); + const { status, body, next } = run('https://attacker.example'); + expect(next).not.toHaveBeenCalled(); + expect(status).toBe(403); + expect(body).toEqual({ jsonrpc: '2.0', error: { code: -32600, message: 'Origin not allowed' }, id: null }); + expect(log).toHaveBeenCalledWith(expect.stringContaining('[mcp-auth] 403 origin-rejected origin=https://attacker.example method=tools/list')); + }); + + it('treats the opaque "null" origin as present and invalid', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { status, next } = run('null'); + expect(next).not.toHaveBeenCalled(); + expect(status).toBe(403); + }); + + it('renders a hostile origin value log-safe (control characters replaced, length capped)', () => { + const log = vi.spyOn(console, 'error').mockImplementation(() => {}); + run('https://x.example/\r\nINJECTED' + 'a'.repeat(500)); + const line = log.mock.calls[0][0] as string; + expect(line).not.toContain('\n'); + expect(line.length).toBeLessThan(320); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/raw-events-page-size.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/raw-events-page-size.test.ts new file mode 100644 index 00000000..ca68b469 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/raw-events-page-size.test.ts @@ -0,0 +1,123 @@ +/** + * query_raw_events pageSize follows the effective projection: a page that carries DataJson is + * bounded by rows × payload (measured p90 3.3 KB, outliers 21-29 KB per row), so it gets a + * smaller default and a ceiling of 100, refused BEFORE the backend does the work; a lean page + * keeps the schema's 1000. A nextLink is judged by its echoed fields. + * + * The handler reaches the backend through scanWithTimeoutFallback (client.ts), so that is the + * seam the test observes: its first argument is the path the backend would receive. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; + +const { scanMock } = vi.hoisted(() => ({ scanMock: vi.fn() })); + +vi.mock('../client.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, scanWithTimeoutFallback: scanMock }; +}); + +import { registerAdminTools, rawEventsPayloadIncluded, RAW_EVENTS_PAYLOAD_PAGE_DEFAULT, RAW_EVENTS_PAYLOAD_PAGE_MAX } from '../tools/admin.js'; +import { DEFAULT_FIRST_PAGE_SIZE } from '../client.js'; + +type Handler = (args: Record) => Promise<{ content: Array<{ text: string }>; isError?: boolean }>; + +function queryRawEvents(): Handler { + const handlers: Record = {}; + const fake = { registerTool: (name: string, _def: unknown, handler: Handler) => { handlers[name] = handler; } }; + registerAdminTools(fake as never, true, true, false); + return handlers.query_raw_events; +} + +const PAGE = { count: 1, events: [{ EventType: 'app_install_failed' }] }; + +function sentPath(): string { + return scanMock.mock.calls[0][0] as string; +} + +function sentPageSize(): string | null { + const path = sentPath(); + return new URLSearchParams(path.slice(path.indexOf('?') + 1)).get('pageSize'); +} + +const FULL_ROWS_NEXTLINK = '/api/raw/events?eventType=app_install_failed&pageSize=1000&continuation=c1'; +const LEAN_NEXTLINK = '/api/raw/events?eventType=app_install_failed&fields=EventType%2CSeverity%2COccurredUtc&pageSize=1000&continuation=c1'; + +describe('rawEventsPayloadIncluded', () => { + it('follows an explicit fields list, case-insensitively', () => { + expect(rawEventsPayloadIncluded('PartitionKey,DataJson', undefined, false)).toBe(true); + expect(rawEventsPayloadIncluded('EventType, datajson', undefined, false)).toBe(true); + expect(rawEventsPayloadIncluded('EventType,Message', undefined, true)).toBe(false); + }); + + it('a filtered first page without fields carries the payload; an unfiltered one is lean by default', () => { + expect(rawEventsPayloadIncluded(undefined, undefined, true)).toBe(true); + expect(rawEventsPayloadIncluded(undefined, undefined, false)).toBe(false); + }); + + it('judges a nextLink by its echoed fields — none echoed means full rows', () => { + expect(rawEventsPayloadIncluded(undefined, FULL_ROWS_NEXTLINK, false)).toBe(true); + expect(rawEventsPayloadIncluded(undefined, LEAN_NEXTLINK, false)).toBe(false); + expect(rawEventsPayloadIncluded(undefined, '/api/raw/events?fields=EventType%2CDataJson&continuation=c1', false)).toBe(true); + }); +}); + +describe('query_raw_events — pageSize follows the payload projection', () => { + beforeEach(() => { + scanMock.mockReset(); + scanMock.mockResolvedValue(PAGE); + }); + + it('refuses pageSize 1000 on a filtered read (full rows) before any backend call', async () => { + const result = await queryRawEvents()({ eventType: 'app_install_failed', pageSize: 1000 }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain(`exceeds the maximum of ${RAW_EVENTS_PAYLOAD_PAGE_MAX}`); + expect(scanMock).not.toHaveBeenCalled(); + }); + + it('refuses an explicit DataJson projection above the ceiling', async () => { + const result = await queryRawEvents()({ fields: 'EventType,DataJson', pageSize: RAW_EVENTS_PAYLOAD_PAGE_MAX + 1 }); + expect(result.isError).toBe(true); + expect(scanMock).not.toHaveBeenCalled(); + }); + + it('accepts the ceiling itself with the payload', async () => { + const result = await queryRawEvents()({ eventType: 'app_install_failed', pageSize: RAW_EVENTS_PAYLOAD_PAGE_MAX }); + expect(result.isError).toBeUndefined(); + expect(sentPageSize()).toBe(String(RAW_EVENTS_PAYLOAD_PAGE_MAX)); + }); + + it('keeps the large page for a lean projection', async () => { + const result = await queryRawEvents()({ eventType: 'app_install_failed', fields: 'EventType,Severity,OccurredUtc', pageSize: 1000 }); + expect(result.isError).toBeUndefined(); + expect(sentPageSize()).toBe('1000'); + }); + + it('defaults a payload page to the smaller size and a lean page to the tool default', async () => { + await queryRawEvents()({ eventType: 'app_install_failed' }); + expect(sentPageSize()).toBe(String(RAW_EVENTS_PAYLOAD_PAGE_DEFAULT)); + + scanMock.mockClear(); + await queryRawEvents()({}); + expect(sentPageSize()).toBe(String(DEFAULT_FIRST_PAGE_SIZE)); + }); + + it('refuses a nextLink whose embedded pageSize exceeds the ceiling for full rows', async () => { + const result = await queryRawEvents()({ continuation: FULL_ROWS_NEXTLINK }); + expect(result.isError).toBe(true); + expect(result.content[0].text).toContain('overrides the value embedded in a nextLink'); + expect(scanMock).not.toHaveBeenCalled(); + }); + + it('lets an explicit smaller pageSize override the nextLink and pass', async () => { + const result = await queryRawEvents()({ continuation: FULL_ROWS_NEXTLINK, pageSize: RAW_EVENTS_PAYLOAD_PAGE_MAX }); + expect(result.isError).toBeUndefined(); + expect(sentPath()).toContain('continuation=c1'); + expect(sentPageSize()).toBe(String(RAW_EVENTS_PAYLOAD_PAGE_MAX)); + }); + + it('keeps the embedded 1000 of a lean nextLink', async () => { + const result = await queryRawEvents()({ continuation: LEAN_NEXTLINK }); + expect(result.isError).toBeUndefined(); + expect(sentPageSize()).toBe('1000'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/telemetry-overflow-cancel.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/telemetry-overflow-cancel.test.ts new file mode 100644 index 00000000..ea31deab --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/telemetry-overflow-cancel.test.ts @@ -0,0 +1,66 @@ +/** + * The tool_call line under the two 2026-09-10 result contracts: an overflow answer (page refused + * by toolResultText) is logged at the size the page WOULD have had and as overCap; a call the + * client abandoned is logged as cancelled, never as an error. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; + +async function loadTelemetry() { + vi.resetModules(); + vi.stubEnv('MCP_TOOL_LOGGING', 'true'); + return await import('../telemetry.js'); +} + +function lastLoggedJson(spy: ReturnType): Record { + expect(spy).toHaveBeenCalled(); + return JSON.parse(spy.mock.calls[spy.mock.calls.length - 1][0] as string) as Record; +} + +afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); +}); + +describe('withToolTelemetry — overflow and cancellation', () => { + it('logs an overflow answer at the size the page would have had, as overCap and isError', async () => { + const { withToolTelemetry } = await loadTelemetry(); + const { toolResultText } = await import('../tools/shared.js'); + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const page = { events: Array.from({ length: 50 }, (_, i) => ({ i, payload: 'x'.repeat(200) })) }; + + const result = await withToolTelemetry('query_raw_events', { eventType: 'x' }, () => toolResultText(page, 1_000)); + + expect(result.isError).toBe(true); + const line = lastLoggedJson(spy); + expect(line.overCap).toBe(true); + expect(line.isError).toBe(true); + expect(line.resultChars).toBe(JSON.stringify(page).length); + expect(String(line.errorMessage)).toContain('"overflow":true'); + }); + + it('logs a call the client abandoned as cancelled, not as an error', async () => { + const { withToolTelemetry } = await loadTelemetry(); + const { runWithCaller } = await import('../client.js'); + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const controller = new AbortController(); + + await runWithCaller({ token: 't', isGlobalAdmin: true, signal: controller.signal }, () => + withToolTelemetry('get_session', { sessionId: 's' }, () => { + controller.abort(); + return { isError: true, content: [{ type: 'text', text: '**Timeout in get_session**' }] }; + }), + ); + + const line = lastLoggedJson(spy); + expect(line.cancelled).toBe(true); + expect(line.isError).toBe(false); + expect(line.errorMessage).toBeUndefined(); + }); + + it('leaves a completed call without a cancelled field', async () => { + const { withToolTelemetry } = await loadTelemetry(); + const spy = vi.spyOn(console, 'error').mockImplementation(() => {}); + await withToolTelemetry('get_session', { sessionId: 's' }, () => ({ content: [{ type: 'text', text: '{}' }] })); + expect(lastLoggedJson(spy)).not.toHaveProperty('cancelled'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/tool-result-overflow.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tool-result-overflow.test.ts new file mode 100644 index 00000000..283d895c --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/tool-result-overflow.test.ts @@ -0,0 +1,83 @@ +/** + * The response-size cap is enforced, not advisory: a page above the tool's cap is never sent — + * the answer is an error naming what the page held and the exact pageSize that fits, computed from + * THIS page's bytes per row. The oversized page's nextLink is withheld (following it would skip + * rows), and the telemetry marker carries the size the page would have had. + */ +import { describe, it, expect } from 'vitest'; +import { toolResultText, OVERFLOW_META_KEY } from '../tools/shared.js'; + +const CAP = 10_000; + +function rows(n: number, payloadChars: number): Array<{ id: number; payload: string }> { + return Array.from({ length: n }, (_, i) => ({ id: i, payload: 'x'.repeat(payloadChars) })); +} + +function bodyOf(result: { content: Array<{ text: string }> }): Record { + return JSON.parse(result.content[0].text) as Record; +} + +describe('toolResultText — hard cap', () => { + it('sends a page under the cap unchanged, with the size hint and no overflow marker', () => { + const data = { count: 3, events: rows(3, 100), nextLink: '/api/x?continuation=abc' }; + const r = toolResultText(data, CAP); + expect(r.isError).toBeUndefined(); + expect(bodyOf(r)).toEqual(data); + expect(r._meta['anthropic/maxResultSizeChars']).toBe(CAP); + expect(r._meta[OVERFLOW_META_KEY]).toBeUndefined(); + }); + + it('refuses a page over the cap: no rows, no nextLink, the pageSize that fits, the marker', () => { + const data = { count: 100, events: rows(100, 500), nextLink: '/api/x?continuation=abc' }; + const chars = JSON.stringify(data).length; + const r = toolResultText(data, CAP); + + expect(r.isError).toBe(true); + const body = bodyOf(r); + expect(body.overflow).toBe(true); + expect(body.responseChars).toBe(chars); + expect(body.maxResultSizeChars).toBe(CAP); + expect(body.rowsField).toBe('events'); + expect(body.rowsFetched).toBe(100); + expect(body.rowsThatFit).toBe(Math.floor((CAP * 0.9) / (chars / 100))); + expect(body.rowsThatFit as number).toBeGreaterThanOrEqual(1); + expect(body.rowsThatFit as number).toBeLessThan(100); + expect(body.events).toBeUndefined(); + expect(body.nextLink).toBeUndefined(); + expect(body.advice).toContain(`pageSize=${body.rowsThatFit}`); + expect(r.content[0].text.length).toBeLessThan(CAP); + expect(r._meta[OVERFLOW_META_KEY]).toEqual({ responseChars: chars }); + expect(r._meta['anthropic/maxResultSizeChars']).toBe(CAP); + }); + + it('gives an honest advice: a re-send at rowsThatFit lands under the cap', () => { + const fit = bodyOf(toolResultText({ events: rows(100, 500) }, CAP)).rowsThatFit as number; + const again = toolResultText({ events: rows(fit, 500) }, CAP); + expect(again.isError).toBeUndefined(); + }); + + it('never advises fewer than one row', () => { + const body = bodyOf(toolResultText({ events: rows(2, CAP) }, CAP)); + expect(body.rowsThatFit).toBe(1); + }); + + it('gives only narrowing advice when the page carries no list', () => { + const body = bodyOf(toolResultText({ blob: 'x'.repeat(CAP + 1) }, CAP)); + expect(body.overflow).toBe(true); + expect(body.rowsThatFit).toBeUndefined(); + expect(body.rowsField).toBeUndefined(); + expect(body.advice).toContain('Narrow the query'); + }); + + it('does not project a pageSize from a one-element list (the KQL tables[] shape)', () => { + const body = bodyOf(toolResultText({ tables: [{ name: 'PrimaryResult', rows: rows(50, 300) }] }, CAP)); + expect(body.overflow).toBe(true); + expect(body.rowsThatFit).toBeUndefined(); + }); + + it('picks the largest top-level list when a page carries several', () => { + const body = bodyOf(toolResultText({ warnings: ['a', 'b'], sessions: rows(60, 400) }, CAP)); + expect(body.rowsField).toBe('sessions'); + expect(body.rowsFetched).toBe(60); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index 5afcf34b..33181640 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -89,6 +89,9 @@ interface AccessCacheEntry { // scope and downgrade the caller to home-tenant-only routing. delegatedTenantIds?: string[]; delegatedRole?: string; + // The backend refused the TOKEN itself (401) — cached like a deny so a replayed bad token costs + // one backend call per TTL, but answered 401 + challenge, never "not enabled". + unauthenticated?: boolean; expiresAt: number; } @@ -109,6 +112,11 @@ interface AccessCheckResult { // routing bounded to this set; every delegated tool call must name one of these tenants. delegatedTenantIds?: string[]; delegatedRole?: string; + // The backend — the only party holding the JWKS — rejected the token (signature, issuer, + // audience, lifetime). That is "invalid token": the spec says it MUST be answered 401 with a + // WWW-Authenticate challenge so the client re-authenticates, not 403 ("insufficient + // permissions"), which a client reads as final. + unauthenticated?: boolean; // Distinguishes a genuine authorization denial (the backend reached a verdict // and said "no" — e.g. user not on the MCP whitelist) from an infrastructure // failure (backend unreachable / malformed response). Both fail closed, but @@ -161,6 +169,7 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis isGlobalReader: cached.isGlobalReader, delegatedTenantIds: cached.delegatedTenantIds, delegatedRole: cached.delegatedRole, + unauthenticated: cached.unauthenticated, // Only genuine backend verdicts are ever cached (infra errors return early // below without caching), so a cache hit is never an infra error. infraError: false, @@ -183,6 +192,22 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis }); const text = await res.text(); + if (res.status === 401) { + // The token itself was refused (see AccessCheckResult.unauthenticated). The envelope's + // message names the cause (expired, wrong audience, bad signature) for the log line. + let reason = 'The backend rejected the token'; + try { + const parsed = JSON.parse(text) as Partial; + if (typeof parsed.error === 'string' && parsed.error) reason = parsed.error; + } catch { + /* not an error envelope */ + } + boundedSet(accessCache, cacheKey, { + allowed: false, reason, isGlobalAdmin: false, isGlobalReader: false, unauthenticated: true, + expiresAt: Date.now() + ACCESS_CACHE_TTL_MS, + }, MAX_ACCESS_CACHE_ENTRIES); + return { allowed: false, reason, isGlobalAdmin: false, isGlobalReader: false, infraError: false, unauthenticated: true }; + } if (!text) { console.error(`[access-guard] Backend returned empty body for ${upn} (status=${res.status})`); return { allowed: false, reason: `Backend returned ${res.status} with empty body`, isGlobalAdmin: false, isGlobalReader: false, infraError: true }; @@ -451,6 +476,14 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo return; } if (!result.allowed) { + if (result.unauthenticated) { + // Spec: an invalid or expired token MUST get 401 with a challenge, so the client + // re-authenticates instead of reading a final "forbidden". + console.error(`[mcp-auth] 401 backend-rejected-token (method=${rpcMethod}, upn=${upn})`); + res.setHeader('WWW-Authenticate', `Bearer resource_metadata="${resourceMetadataUrl}", error="invalid_token"`); + res.status(401).json({ error: 'Invalid token', reason: result.reason }); + return; + } if (result.infraError) { // Backend could not reach a verdict (unreachable / malformed). Fail // closed, but do NOT tell the user they are "not whitelisted" — it is @@ -484,12 +517,21 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo return; } + // Cancellation (Streamable HTTP: closing the response stream cancels the request): the + // caller context carries a signal that fires when the client goes away before the + // response is written, so the backend work of an abandoned call stops (client.ts apiFetch). + const cancellation = new AbortController(); + res.on('close', () => { + if (!res.writableFinished) cancellation.abort(); + }); + // Scope the caller context (token + platform role) to this async context // so concurrent sessions cannot overwrite each other on the event loop, // and so tools can route based on role without re-checking the JWT. runWithCaller( { token, + signal: cancellation.signal, isGlobalAdmin: result.isGlobalAdmin, isGlobalReader: result.isGlobalReader, delegatedTenantIds: result.delegatedTenantIds, diff --git a/src/McpServer/autopilot-monitor-mcp/src/auth.ts b/src/McpServer/autopilot-monitor-mcp/src/auth.ts index bcb4a958..f73c82be 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/auth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/auth.ts @@ -1,12 +1,20 @@ /** * Auth module for the remote MCP server. * - * In remote mode, the Claude Code client handles OAuth and sends a Bearer token - * with each MCP request. This module provides helpers for token validation and - * extracting user info from the JWT claims. + * The MCP client obtains a Bearer token through this server's OAuth proxy (oauth.ts) and sends it + * with each MCP request. This module reads the JWT claims; the token itself is validated by the + * backend. * - * The user's token is passed through to the backend API (access_as_user scope), - * so no service principal or separate credentials are needed. + * Token model — a DELIBERATE deviation from the MCP authorization spec, recorded as a decision in + * the internal decision register: the token's audience is the backend API app (access_as_user), + * not this server, and the same token is forwarded to the backend. The spec requires an MCP + * server to accept only tokens issued for itself and never to transit them downstream. The + * compensations: the backend validates signature, issuer, audience and lifetime on EVERY + * forwarded call (this server never treats a token as valid on its own), /api/auth/mcp decides + * MCP eligibility per principal, and every tool reaches only its own backend path (followNextLink + * basePath pinning). Audience separation would need a second app registration with consent in + * every customer tenant plus an on-behalf-of exchange, and would break the app-only + * service-principal path — not built until a reviewer requires it. */ import { createDecoder } from './jwt-decode.js'; @@ -30,16 +38,12 @@ export interface TokenClaims { /** Calling application's client id on a v2.0 token. */ azp?: string; /** - * 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. + * Audience. Parsed for observability/diagnostics but intentionally NOT validated here — the + * backend enforces it cryptographically alongside the signature/issuer/lifetime checks + * (`AuthenticationMiddleware.cs`: `ValidateAudience = true`, `ValidAudiences = { clientId, + * api://clientId }`); duplicating that gate here would only add a second, drift-prone copy of + * the accepted-audience list. The audience is the backend API, not this server — see the + * module header for why that deviation from the MCP spec is deliberate. */ aud?: string; } diff --git a/src/McpServer/autopilot-monitor-mcp/src/client.ts b/src/McpServer/autopilot-monitor-mcp/src/client.ts index 7e6a8299..9c4b0880 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/client.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/client.ts @@ -116,6 +116,13 @@ interface CallerContext { * wire's gzip does not remove from a model's context. */ prettyJson?: boolean; + /** + * Fires when the client closes the HTTP request before the response is written — the transport's + * cancellation signal (spec 2026-07-28: closing the response stream cancels the request). apiFetch + * folds it into every backend call so an abandoned tool call stops its backend work instead of + * running to its own deadline; the timeout-fallback retry and the telemetry read it too. + */ + signal?: AbortSignal; } const callerStore = new AsyncLocalStorage(); @@ -145,6 +152,11 @@ export function getCurrentToken(): string | undefined { return callerStore.getStore()?.token; } +/** The current request's cancellation signal (see CallerContext.signal), or undefined without a context. */ +export function getCallerSignal(): AbortSignal | undefined { + return callerStore.getStore()?.signal; +} + /** * Returns true if the current request is being made by a Global Admin. * Defaults to false when no context is active (e.g. unit tests without @@ -329,12 +341,30 @@ export interface ToolCallContext { correlationId: string; /** Automatic 429/503 retries apiFetch performed during this tool call (counted here, logged on the tool_call line). */ retries: number; + /** + * How the call's arguments are rendered wherever they are echoed — the `tool_call` log line AND the + * "Parameters used" block of an error result (which the log quotes). One declaration at the + * withToolTelemetry call site covers both sinks; without it a clear-text config value masked in + * the args summary would still reach the log through the error text. + */ + argPolicy?: ArgPolicy; } +/** + * Per-argument echo policy: 'keys' renders only the property NAMES of an object-valued argument + * (update_tenant_config.fields — which fields changed, never their values), 'drop' omits the + * argument entirely. Arguments without a policy are rendered verbatim, size-capped. + */ +export type ArgPolicy = Record; + const toolCallStore = new AsyncLocalStorage(); -export function createToolCallContext(toolName: string, correlationId: string): ToolCallContext { - return { toolName, correlationId, retries: 0 }; +export function createToolCallContext(toolName: string, correlationId: string, argPolicy?: ArgPolicy): ToolCallContext { + return { toolName, correlationId, retries: 0, argPolicy }; +} + +export function getCurrentArgPolicy(): ArgPolicy | undefined { + return toolCallStore.getStore()?.argPolicy; } /** Run `fn` inside a tool-call context the caller keeps a reference to (withToolTelemetry reads `retries` afterwards). */ @@ -372,7 +402,27 @@ export interface ApiFetchOptions extends RequestInit { retry?: boolean; } -const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +/** Sleeps `ms`, or rejects at once with the signal's reason when the caller cancels meanwhile. */ +const delay = (ms: number, signal?: AbortSignal) => new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason); + return; + } + const onAbort = () => { + clearTimeout(timer); + reject(signal?.reason); + }; + const timer = setTimeout(() => { + signal?.removeEventListener('abort', onAbort); + resolve(); + }, ms); + signal?.addEventListener('abort', onAbort, { once: true }); +}); + +/** The per-attempt signal: the call's own deadline plus, when the request has one, the caller's cancellation. */ +function withCallerSignal(own: AbortSignal, caller: AbortSignal | undefined): AbortSignal { + return caller ? AbortSignal.any([own, caller]) : own; +} /** Seconds to wait before the one automatic retry, or null when this answer gets none. */ function autoRetryDelaySeconds(status: number, body: string, retryAfterHeader: string | null): number | null { @@ -421,8 +471,15 @@ async function apiFetch(path: string, options: ApiFetchOptions = {}): Promise const method = (init.method ?? 'GET').toUpperCase(); const mayRetry = retry ?? (method === 'GET' && !init.signal); - // Apply timeout to prevent hanging on unresponsive backend; a retry gets a fresh one. - const attempt = () => fetch(url, { ...init, headers, signal: init.signal ?? AbortSignal.timeout(API_TIMEOUT_MS) }); + // Every attempt runs under the call's own deadline (a retry gets a fresh one) AND the caller's + // cancellation: when the client has closed the request, the backend work stops here instead of + // running to its timeout — pagination scans and the retry wait included. + const callerSignal = getCallerSignal(); + const attempt = () => fetch(url, { + ...init, + headers, + signal: withCallerSignal(init.signal ?? AbortSignal.timeout(API_TIMEOUT_MS), callerSignal), + }); let res = await attempt(); if (!res.ok) { @@ -433,7 +490,7 @@ async function apiFetch(path: string, options: ApiFetchOptions = {}): Promise throw new ApiError(res.status, text, headerOf(res, 'x-correlation-id'), retryAfterHeader); } if (call) call.retries += 1; - await delay(wait * 1000); + await delay(wait * 1000, callerSignal); res = await attempt(); if (!res.ok) { throw new ApiError(res.status, await res.text(), headerOf(res, 'x-correlation-id'), headerOf(res, 'retry-after')); @@ -728,7 +785,9 @@ export async function scanWithTimeoutFallback( try { return await scan(firstPath); } catch (error: unknown) { - if (!isTimeoutError(error) || pageSize <= MIN_FALLBACK_PAGE_SIZE) throw error; + // A caller-cancelled fetch surfaces as the same abort shape as a timeout; never re-fetch for + // a client that has already gone away. + if (!isTimeoutError(error) || pageSize <= MIN_FALLBACK_PAGE_SIZE || getCallerSignal()?.aborted) throw error; const reduced = Math.max(MIN_FALLBACK_PAGE_SIZE, Math.floor(pageSize / 2)); const page = await scan(withQueryOverrides(firstPath, { pageSize: reduced })); return { diff --git a/src/McpServer/autopilot-monitor-mcp/src/index.ts b/src/McpServer/autopilot-monitor-mcp/src/index.ts index 105f1530..899be8da 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/index.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/index.ts @@ -15,6 +15,7 @@ import { validatePrecomputedIndex } from './precomputed-index.js'; import { buildEventTypeSearchDocs } from './resource-catalog.js'; import { createOAuthRouter } from './oauth.js'; import { accessGuard } from './access-guard.js'; +import { originGuard } from './origin-guard.js'; import { toolLoggingEnabled, attachToolCallRejectionSniffer } from './telemetry.js'; import { API_BASE_URL } from './config.js'; import { SERVER_VERSION, BUILD_COMMIT, BUILD_UTC, DOCS_COMMIT } from './build-info.js'; @@ -274,6 +275,10 @@ app.get('/health', (_req, res) => { }); }); +// Origin gate for /mcp (spec MUST): a browser Origin other than this server's own is refused +// before any token work — see origin-guard.ts. +app.use('/mcp', originGuard); + // Access guard for /mcp — validates JWT, checks backend whitelist, enforces rate limits app.use('/mcp', accessGuard); diff --git a/src/McpServer/autopilot-monitor-mcp/src/mcp-server-factory.ts b/src/McpServer/autopilot-monitor-mcp/src/mcp-server-factory.ts index 0df8d41a..9ed4d81f 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/mcp-server-factory.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/mcp-server-factory.ts @@ -82,7 +82,7 @@ export function buildInstructions(deps: ServerDeps, ga: boolean, strictGa: boole : []), 'Searching events: use search_events (hybrid keyword+semantic ranking; depth="fast" then "deep" for exhaustive recall) for ranked hits, or get_session_events / query_raw_events for the raw unranked stream.', 'Counting / aggregating: pass a lean `fields=` projection and use `agentVersionPrefix=`/`imeAgentVersionPrefix=` sweeps to stay under the per-response size cap.', - 'Pagination: when a response carries `nextLink`, pass that whole string back as `continuation`; stop when it is absent. Results are never silently truncated.', + 'Pagination: when a response carries `nextLink`, pass that whole string back as `continuation`; stop when it is absent. Results are never silently truncated: a page above a tool\'s response cap is refused with an overflow error that names the pageSize that fits — re-send the same call with it.', 'Catalogs: call get_resource(name="event_types"|"device_properties") to discover valid eventType strings and deviceProperties keys before filtering.', 'Error codes: lookup_error_code explains one HRESULT / MSI exit code / symbol / IME enforcement state from the shared catalog; search_knowledge finds the rules that name a code.', scopeLine, diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index e63786fb..79e15c53 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -124,16 +124,26 @@ const ENTRA_TOKEN_TIMEOUT_MS = 15_000; // binding on every callback, and the client validates its own originalState // exactly once. Decision reviewed 2026-08-15. // -// HMAC key: prefer OAuthStateSigningKey from the environment so all replicas -// agree on the signature; fall back to a per-instance random key when unset -// (state has a 10 min lifetime, so per-instance is acceptable for single- -// replica scale=0..1). Format and naming match the backend's -// PaginationTokenSigningKey for consistency: PascalCase env var, base64- -// encoded random bytes, ≥32 bytes after decode. Generate on PowerShell: +// HMAC key: OAuthStateSigningKey from the environment so every replica AND every +// cold start agree on the signature. The signed client_id (below) is derived from +// this key and is meant to outlive the process — with a per-boot random key a +// scale-to-zero restart would invalidate every minted client_id (invalid_client) +// and every in-flight state. So production refuses to boot without the key; the +// per-instance random fallback exists for local development and tests only. +// Format and naming match the backend's PaginationTokenSigningKey: PascalCase env +// var, base64-encoded random bytes, ≥32 bytes after decode. Generate on PowerShell: // [Convert]::ToBase64String([Security.Cryptography.RandomNumberGenerator]::GetBytes(32)) -const OAUTH_STATE_SIGNING_KEY: Buffer = (() => { - const raw = process.env.OAuthStateSigningKey; - if (!raw) return crypto.randomBytes(32); +export function loadSigningKey(raw: string | undefined, nodeEnv: string | undefined): Buffer { + if (!raw) { + if (nodeEnv === 'production') { + throw new Error( + 'OAuthStateSigningKey is not set. Production requires it: signed client_ids and OAuth state must ' + + 'survive restarts and replicas. Generate one with PowerShell: ' + + '[Convert]::ToBase64String([Security.Cryptography.RandomNumberGenerator]::GetBytes(32))', + ); + } + return crypto.randomBytes(32); + } // Buffer.from(s, 'base64') silently drops invalid chars — the only useful // signal is the resulting byte length. Anything below 32 bytes is either a // malformed input or a too-weak key; either way we want to fail loud at @@ -148,7 +158,9 @@ const OAUTH_STATE_SIGNING_KEY: Buffer = (() => { ); } return decoded; -})(); +} + +const OAUTH_STATE_SIGNING_KEY: Buffer = loadSigningKey(process.env.OAuthStateSigningKey, process.env.NODE_ENV); const STATE_MAX_AGE_SECONDS = 600; interface ProxyStatePayload { @@ -204,6 +216,38 @@ export function sanitizeForLog(value: unknown, maxLength = 200): string { return cleaned.length > maxLength ? `${cleaned.slice(0, maxLength)}...` : cleaned; } +/** OAuth error codes (RFC 6749 §5.2, OIDC Core §3.1.2.6) a client acts on; anything else is relayed as invalid_request. */ +const OAUTH_ERROR_CODES = new Set([ + 'invalid_request', 'invalid_client', 'invalid_grant', 'unauthorized_client', 'unsupported_grant_type', + 'invalid_scope', 'access_denied', 'server_error', 'temporarily_unavailable', + 'interaction_required', 'login_required', 'consent_required', +]); +const TOKEN_ERROR_DESCRIPTION_CAP = 300; + +/** + * The token endpoint's error body as the proxy relays it. Entra's own body is a fingerprinting + * surface for an unauthenticated caller — `trace_id`, `correlation_id`, `timestamp`, `error_uri`, + * a `claims` challenge, and an `error_description` that repeats the trace ids in prose — so only + * what a client acts on passes: the OAuth `error` code, the human message (cut before the + * "Trace ID:" tail Entra appends, size-capped) and the numeric AADSTS `error_codes`. The server + * log line keeps the correlation id for a Microsoft support case. + */ +export function sanitizeTokenErrorBody(data: unknown): Record { + const err = (typeof data === 'object' && data !== null ? data : {}) as Record; + const error = typeof err.error === 'string' && OAUTH_ERROR_CODES.has(err.error) ? err.error : 'invalid_request'; + const out: Record = { error }; + if (typeof err.error_description === 'string' && err.error_description) { + const description = err.error_description.split(/\s*Trace ID:/i)[0].trim(); + out.error_description = description.length > TOKEN_ERROR_DESCRIPTION_CAP + ? `${description.slice(0, TOKEN_ERROR_DESCRIPTION_CAP)}...` + : description; + } + if (Array.isArray(err.error_codes)) { + out.error_codes = err.error_codes.filter((c): c is number => typeof c === 'number').slice(0, 10); + } + return out; +} + // ---- Client-id signing (HMAC) --------------------------------------------- // // The dynamic-registration client_id is a self-describing signed token rather @@ -909,7 +953,7 @@ export function createOAuthRouter(): Router { `correlation_id=${sanitizeForLog(err.correlation_id)}`, ); } - res.status(tokenResponse.status).json(data); + res.status(tokenResponse.status).json(tokenResponse.status === 200 ? data : sanitizeTokenErrorBody(data)); } catch (err) { // AbortSignal.timeout() rejects with a DOMException named 'TimeoutError' // (see error-handler.ts) — report that as 504 so a stalled identity diff --git a/src/McpServer/autopilot-monitor-mcp/src/origin-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/origin-guard.ts new file mode 100644 index 00000000..6eb47e11 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/origin-guard.ts @@ -0,0 +1,43 @@ +/** + * Origin gate for the MCP endpoint (Streamable HTTP, spec 2026-07-28 "Security & Endpoint"): + * "Servers MUST validate the Origin header on all incoming connections to prevent DNS rebinding + * attacks. If the Origin header is present and invalid, servers MUST respond with HTTP 403." + * + * Every legitimate client of this server is a non-browser process (Claude Code, the Claude.ai + * connector, VS Code) and sends no Origin at all — such requests pass untouched. A request that + * DOES carry an Origin comes from a browser context; the only one this server can serve is its + * own (there is no CORS layer, so no cross-origin page could complete a call anyway), everything + * else — including the opaque `null` origin — is refused. Mounted on /mcp only: the /oauth + * endpoints are browser-navigated by design. + */ +import type { Request, Response, NextFunction } from 'express'; +import { getPublicBaseUrl } from './config.js'; + +/** True when the request may proceed: no Origin (non-browser client) or exactly this server's origin. */ +export function isAllowedOrigin(origin: string | undefined, publicBaseUrl: string): boolean { + if (origin === undefined) return true; + let candidate: URL; + try { + candidate = new URL(origin); + } catch { + return false; // 'null' and malformed values are "present and invalid" + } + return candidate.origin === new URL(publicBaseUrl).origin; +} + +/** Log-safe rendering of an attacker-controlled header value. */ +function forLog(origin: string): string { + return origin.replace(/[^\x20-\x7e]/g, '?').slice(0, 200); +} + +export function originGuard(req: Request, res: Response, next: NextFunction): void { + const origin = req.headers.origin; + if (isAllowedOrigin(origin, getPublicBaseUrl(req))) { + next(); + return; + } + const rpcMethod = (req.body as { method?: string } | undefined)?.method ?? '?'; + console.error(`[mcp-auth] 403 origin-rejected origin=${forLog(origin as string)} method=${rpcMethod}`); + // The spec allows a JSON-RPC error without id as the 403 body. + res.status(403).json({ jsonrpc: '2.0', error: { code: -32600, message: 'Origin not allowed' }, id: null }); +} diff --git a/src/McpServer/autopilot-monitor-mcp/src/telemetry.ts b/src/McpServer/autopilot-monitor-mcp/src/telemetry.ts index 8f8573e1..760fffd2 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/telemetry.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/telemetry.ts @@ -1,5 +1,9 @@ import { randomUUID } from 'node:crypto'; -import { createToolCallContext, runWithToolCallContext, hasGlobalScope, isDelegated, getCallerUpn } from './client.js'; +import { + createToolCallContext, runWithToolCallContext, hasGlobalScope, isDelegated, getCallerUpn, getCallerSignal, + type ArgPolicy, +} from './client.js'; +import { OVERFLOW_META_KEY } from './tools/shared.js'; export const toolLoggingEnabled = process.env.MCP_TOOL_LOGGING === 'true'; @@ -15,18 +19,16 @@ function cap(s: string, max: number): string { } /** - * Per-argument logging policy, declared at the tool's withToolTelemetry call - * site so schema, handler and log policy are reviewed together. - * - 'keys': log only the property NAMES of an object-valued argument. For - * update_tenant_config.fields this keeps the useful signal (which fields a - * Global Admin changes) while the clear-text values (webhook URLs, SAS URLs, - * custom headers) never reach the container log stream — the backend keeps - * them in the backup snapshot and the audit log, both GA-gated. - * - 'drop': omit the argument entirely (for arguments whose key alone reveals - * something, e.g. a plain-string secret). - * Arguments without a policy are rendered verbatim (capped). + * Per-argument echo policy (see client.ts ArgPolicy), declared at the tool's + * withToolTelemetry call site so schema, handler and policy are reviewed + * together. It travels in the tool-call context, so the SAME policy governs the + * `tool_call` log line and the "Parameters used" block toolError appends to an + * error result. For update_tenant_config.fields this keeps the useful signal + * (which fields a Global Admin changes) while the clear-text values (webhook + * URLs, SAS URLs, custom headers) never reach the container log stream — the + * backend keeps them in the backup snapshot and the audit log, both GA-gated. */ -export type ArgPolicy = Record; +export type { ArgPolicy }; function renderKeysOnly(value: unknown): string { if (Array.isArray(value)) return `[array:${value.length}]`; @@ -98,7 +100,7 @@ export async function withToolTelemetry( // One correlation id per tool call — sent on every backend request the call makes and written // into the tool_call line below, so an MCP log line and the backend request rows join on it. const correlationId = randomUUID(); - const context = createToolCallContext(toolName, correlationId); + const context = createToolCallContext(toolName, correlationId, argPolicy); if (!toolLoggingEnabled) { return runWithToolCallContext(context, fn) as Promise; } @@ -117,9 +119,16 @@ export async function withToolTelemetry( } finally { try { const r = result as ToolResultShape | undefined; - const resultChars = r?.content?.reduce((sum, c) => sum + (c.text?.length ?? 0), 0) ?? 0; + // A page the server refused to send (toolResultText overflow) is logged at the size it + // WOULD have had, so the size distribution keeps naming the offending calls. + const overflow = r?._meta?.[OVERFLOW_META_KEY] as { responseChars?: number } | undefined; + const resultChars = overflow?.responseChars + ?? (r?.content?.reduce((sum, c) => sum + (c.text?.length ?? 0), 0) ?? 0); const capValue = Number(r?._meta?.['anthropic/maxResultSizeChars']); - const isError = threw || r?.isError === true; + // The client closed the request before the result was written: the call was abandoned, + // not failed — it must not count towards the tool's error rate. + const cancelled = getCallerSignal()?.aborted === true; + const isError = !cancelled && (threw || r?.isError === true); // What actually failed — without this every error drilldown ends at // guessing from args. Soft errors carry the toolError text (its first // lines name the error class: HTTP status / timeout / auth / not found). @@ -133,10 +142,12 @@ export async function withToolTelemetry( durationMs: Date.now() - start, isError, errorMessage, + cancelled: cancelled ? true : undefined, resultChars, - // Result exceeds the inline-size hint → the host truncates it. A tool - // that is frequently overCap needs tighter defaults or projections. - overCap: Number.isFinite(capValue) && capValue > 0 ? resultChars > capValue : false, + // Result exceeds the tool's size cap: the server answered with an overflow error + // instead of the page (marker), or the advisory hint was exceeded and the host + // truncates. A tool that is frequently overCap needs tighter defaults or projections. + overCap: overflow !== undefined || (Number.isFinite(capValue) && capValue > 0 ? resultChars > capValue : false), // Automatic 429/503 retries apiFetch made for this call (absent when none): a tool that // retries often is hitting a rate limit its defaults should avoid. retries: context.retries > 0 ? context.retries : undefined, diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts index 0d3ce590..49efc03a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts @@ -33,6 +33,42 @@ export const GA_STRICT_TOOL_NAMES: ReadonlySet = new Set([ /** Default first-page size of get_fleet_overview's session list (a fleet snapshot, not a sweep). */ const FLEET_OVERVIEW_PAGE_SIZE = 25; +/** + * query_raw_events rows per page when the projection carries DataJson. Measured over 30 days of + * tool telemetry: with the payload a row is p50 0.6 KB, p90 3.3 KB, outliers 21-29 KB, so 100 rows + * keep the p90 page (330 KB) under the 500 KB cap and the outliers are the hard cap's job + * (toolResultText names the exact pageSize that fits). Without DataJson the schema's 1000 stands. + */ +export const RAW_EVENTS_PAYLOAD_PAGE_DEFAULT = 50; +export const RAW_EVENTS_PAYLOAD_PAGE_MAX = 100; + +/** + * Whether the EFFECTIVE projection of a query_raw_events call includes DataJson: an explicit + * `fields` naming it; a filtered first page without `fields` (full rows by default, see + * leanFieldSelection); or a nextLink whose echoed `fields` names it — or carries none, which is + * what a filtered page 1 leaves behind and means full rows on the follow-up as well. + */ +export function rawEventsPayloadIncluded(explicitFields: string | undefined, continuation: string | undefined, targeted: boolean): boolean { + const names = (list: string) => list.split(',').map((f) => f.trim().toLowerCase()); + if (explicitFields !== undefined) return names(explicitFields).includes('datajson'); + if (continuation?.startsWith('/api/')) { + const q = continuation.indexOf('?'); + const echoed = q === -1 ? null : new URLSearchParams(continuation.slice(q + 1)).get('fields'); + return echoed === null || names(echoed).includes('datajson'); + } + return targeted; +} + +function assertPayloadPageSize(pageSize: number): void { + if (pageSize <= RAW_EVENTS_PAYLOAD_PAGE_MAX) return; + throw new Error( + `pageSize ${pageSize} exceeds the maximum of ${RAW_EVENTS_PAYLOAD_PAGE_MAX} for a read that includes DataJson: ` + + 'the raw payload carries kilobytes per row, so such a page is bounded by rows × payload, not rows. ' + + `Re-send with pageSize ≤ ${RAW_EVENTS_PAYLOAD_PAGE_MAX} (an explicit pageSize overrides the value embedded in a ` + + 'nextLink; the cursor stays valid), or pass a fields= projection without DataJson to keep the larger page.', + ); +} + /** * Pure: the get_fleet_overview result. The two backend responses each carry their own * quotaExcludedTenants (the quota layer decides per request); the union is what the model must know is @@ -1510,7 +1546,10 @@ export function registerAdminTools(server: McpServer, ga: boolean, strictGa: boo 'nextLink as "continuation" to keep scanning). ' + 'This endpoint is fully paginated — there is no truncation. The default pageSize=' + DEFAULT_FIRST_PAGE_SIZE + ' is tuned for typical ' + 'interactive queries; raise it (up to 1000) for forensics-grade exact recall. For broad analysis, use ' + - 'pageSize=1000 and follow nextLink repeatedly until absent. Pass the whole nextLink string as "continuation" ' + + 'pageSize=1000 and follow nextLink repeatedly until absent. EXCEPTION: a page that includes DataJson (named in ' + + 'fields, or a filtered read without fields) is capped at pageSize=' + RAW_EVENTS_PAYLOAD_PAGE_MAX + ' (default ' + + RAW_EVENTS_PAYLOAD_PAGE_DEFAULT + ') — the payload carries kilobytes per row, and a larger value is refused before ' + + 'any work is done. Pass the whole nextLink string as "continuation" ' + 'so all backend-echoed query params round-trip correctly. Note: pageSize is the index-scan cadence — a single ' + 'indexed session can contribute multiple events, so total events per page may exceed pageSize. ' + 'The server bounds every page by a scan budget: a page that ends early carries "partial": true — nothing is ' + @@ -1544,9 +1583,16 @@ export function registerAdminTools(server: McpServer, ga: boolean, strictGa: boo async (args) => withToolTelemetry('query_raw_events', args, async () => { try { const { tenantId: rawTenantId, sessionId, eventType, severity, source, startedAfter, startedBefore, fields: explicitFields, continuation } = args; - const pageSize = pageSizeForCall(args.pageSize, continuation, DEFAULT_FIRST_PAGE_SIZE); + const targeted = Boolean(eventType || severity || source); + // A page that carries DataJson is bounded by rows × payload, not rows (measured over 30 d: + // p90 3.3 KB, outliers 21-29 KB per row), so 1000 such rows exceed the cap many times over + // while 1000 lean rows fit. Default and ceiling therefore follow the effective projection, + // and an oversized request is refused BEFORE the backend does the work. + const payload = rawEventsPayloadIncluded(explicitFields, continuation, targeted); + const pageSize = pageSizeForCall(args.pageSize, continuation, payload ? RAW_EVENTS_PAYLOAD_PAGE_DEFAULT : DEFAULT_FIRST_PAGE_SIZE); + if (payload) assertPayloadPageSize(effectivePageSize(pageSize, continuation)); // Default projection follows intent — see leanFieldSelection (DataJson is the raw payload column). - const { fields, leanDefaultApplied } = leanFieldSelection(explicitFields, continuation, Boolean(eventType || severity || source), LEAN_RAW_EVENT_FIELDS); + const { fields, leanDefaultApplied } = leanFieldSelection(explicitFields, continuation, targeted, LEAN_RAW_EVENT_FIELDS); const tenantId = enforceDelegatedTenantForPage(rawTenantId, continuation); if (eventType) assertKnownEventType(eventType); const basePath = pickGlobalOrTenantPath('/api/global/raw/events', '/api/raw/events', tenantId); @@ -1917,15 +1963,18 @@ export function registerAdminTools(server: McpServer, ga: boolean, strictGa: boo { title: 'Revert Tenant Configuration', description: - 'Restore a tenant\'s configuration from a pre-write snapshot (latest by default). Global Admin only. ' + - 'The revert snapshots the CURRENT state first, so a revert is itself revertible. Protected fields ' + - '(plan/trial, homedAppClientId, auth provenance, onboarded*) keep their CURRENT values unless ' + - 'includeProtectedFields is explicitly true — time-traveling those via an old snapshot is almost never ' + - 'intended. Same transactional verify-and-rollback machinery as update_tenant_config.', + 'Restore a tenant\'s configuration from a named pre-write snapshot. Global Admin only. ' + + 'The revert snapshots the CURRENT state first, so a revert is itself revertible — which is exactly why ' + + 'backupId is required and there is no "latest" default: a repeated revert without an id would restore the ' + + 'snapshot the previous revert just created and undo it. Reverting the same backupId twice is a no-op. ' + + 'Protected fields (plan/trial, homedAppClientId, auth provenance, onboarded*) keep their CURRENT values ' + + 'unless includeProtectedFields is explicitly true — time-traveling those via an old snapshot is almost ' + + 'never intended. Same transactional verify-and-rollback machinery as update_tenant_config.', inputSchema: { tenantId: TenantGuidSchema.describe('Tenant ID (GUID) whose configuration to revert.'), - backupId: z.string().optional() - .describe('Snapshot to restore (from list_tenant_config_backups). Omit for the most recent snapshot.'), + backupId: z.string().min(1) + .describe('REQUIRED: the snapshot to restore — from list_tenant_config_backups, or the backupId an ' + + 'update_tenant_config response returned. Never omitted: see the description.'), includeProtectedFields: z.boolean().optional() .describe('DANGEROUS, default false: also restore plan/trial, homedAppClientId and auth-provenance fields ' + 'from the snapshot. Only set when the snapshot\'s values for those are explicitly wanted.'), diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts index 4a5a5b44..57f4bb46 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts @@ -1,5 +1,6 @@ -import { ApiError, isTimeoutError, type ParsedErrorBody } from '../client.js'; +import { ApiError, getCurrentArgPolicy, isTimeoutError, type ParsedErrorBody } from '../client.js'; import type { McpQuotaExceededResponse } from '../generated/wire-types.generated.js'; +import { summarizeArgs } from '../telemetry.js'; interface ToolErrorResult { [x: string]: unknown; @@ -153,13 +154,13 @@ export function toolError( } } - // Include parameter summary so the AI can see what it sent - const argsSummary = Object.entries(args) - .filter(([, v]) => v != null && v !== undefined) - .map(([k, v]) => ` ${k}: ${JSON.stringify(v)}`) - .join('\n'); + // Parameter summary so the AI can see what it sent — rendered under the SAME per-argument + // policy as the tool_call log line (withToolTelemetry): an argument declared 'keys' shows only + // its property names here too, so a clear-text config value never travels in an error text + // (the log quotes the first lines of this text). + const argsSummary = summarizeArgs(args, getCurrentArgPolicy()); if (argsSummary) { - parts.push(`**Parameters used**:\n${argsSummary}`); + parts.push(`**Parameters used**:\n${Object.entries(argsSummary).map(([k, v]) => ` ${k}: ${v}`).join('\n')}`); } return { diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts index 8ee54bdc..47d78f85 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts @@ -230,18 +230,92 @@ export function stringifyResult(data: unknown): string { return PRETTY_JSON || isPrettyJsonRequested() ? JSON.stringify(data, null, 2) : JSON.stringify(data); } +/** + * `_meta` key of an overflow answer (see toolResultText). The telemetry reads it to log the + * page at the size it WOULD have had, so the size distribution keeps naming the offending calls. + */ +export const OVERFLOW_META_KEY = 'autopilotmonitor.com/overflow'; + +/** Share of the cap an overflow advice aims at, so the re-sent page lands under it with headroom. */ +const OVERFLOW_TARGET_RATIO = 0.9; + +/** Fewest rows a page needs before its per-row size is worth projecting into a pageSize advice. */ +const OVERFLOW_MIN_ROWS_FOR_ADVICE = 2; + +export interface ToolResult { + // Index signature: the SDK's CallToolResult is indexable, and an interface without one is not + // assignable to it (an inline object type would be, implicitly). + [x: string]: unknown; + isError?: true; + content: Array<{ type: 'text'; text: string }>; + _meta: Record; +} + /** * Wraps a tool response payload with the Anthropic maxResultSizeChars * annotation and a single text content block. Use in preference to bare * { content: [...] } so the cap travels with every call without * client-side configuration. + * + * The cap is ENFORCED here, not merely advertised: the hint only tells an Anthropic host how + * much to accept — other hosts cut the JSON somewhere in the middle, and even that host has a + * ceiling. Either way the model would read a page with rows missing and no way to tell which + * (the nextLink at the end is usually the first casualty). So a page above the cap is not sent + * at all: the answer is an error carrying what the page held and, when the payload is a list, + * the exact pageSize that fits — computed from THIS page's measured bytes per row, not guessed. + * Nothing is lost because backend pages are re-requestable: the same continuation with the + * smaller pageSize returns the first rows plus a nextLink to the rest. The oversized page's + * own nextLink is deliberately withheld — following it would skip the rows that were dropped. */ -export function toolResultText( - data: unknown, - maxResultSizeChars: number, -): { content: Array<{ type: 'text'; text: string }>; _meta: Record } { +export function toolResultText(data: unknown, maxResultSizeChars: number): ToolResult { + const text = stringifyResult(data); + if (text.length <= maxResultSizeChars) { + return { + content: [{ type: 'text' as const, text }], + _meta: { 'anthropic/maxResultSizeChars': maxResultSizeChars }, + }; + } + return overflowResult(data, text.length, maxResultSizeChars); +} + +/** The top-level list a page carries (its largest array-valued property), if any. */ +function largestTopLevelList(data: unknown): { field: string; rows: number } | undefined { + if (typeof data !== 'object' || data === null || Array.isArray(data)) return undefined; + let best: { field: string; rows: number } | undefined; + for (const [field, value] of Object.entries(data as Record)) { + if (Array.isArray(value) && (best === undefined || value.length > best.rows)) best = { field, rows: value.length }; + } + return best; +} + +function overflowResult(data: unknown, responseChars: number, maxResultSizeChars: number): ToolResult { + const list = largestTopLevelList(data); + const rowAdvice = list !== undefined && list.rows >= OVERFLOW_MIN_ROWS_FOR_ADVICE; + const rowsThatFit = rowAdvice + ? Math.max(1, Math.floor((maxResultSizeChars * OVERFLOW_TARGET_RATIO) / (responseChars / list.rows))) + : undefined; + const body = { + overflow: true, + error: + `Result too large: ${responseChars} characters exceed this tool's ${maxResultSizeChars}-character response cap. ` + + 'The page was NOT sent — a cut page would have lost rows without telling you which.', + responseChars, + maxResultSizeChars, + ...(rowAdvice ? { rowsField: list.field, rowsFetched: list.rows, rowsThatFit } : {}), + advice: rowAdvice + ? `This page held ${list.rows} rows in "${list.field}" at about ${Math.round(responseChars / list.rows)} characters each; ` + + `${rowsThatFit} rows fit the cap. If this tool takes pageSize, re-send the SAME call (same filters, same ` + + `continuation if any) with pageSize=${rowsThatFit} and follow its nextLink for the rest — nothing is skipped. ` + + 'Otherwise narrow the query: tighter filters, a fields= projection without the payload column, a smaller date window.' + : 'Narrow the query: tighter filters, a fields= projection without the payload column, a smaller date window, ' + + 'or a smaller pageSize if this tool takes one.', + }; return { - content: [{ type: 'text' as const, text: stringifyResult(data) }], - _meta: { 'anthropic/maxResultSizeChars': maxResultSizeChars }, + isError: true, + content: [{ type: 'text' as const, text: stringifyResult(body) }], + _meta: { + 'anthropic/maxResultSizeChars': maxResultSizeChars, + [OVERFLOW_META_KEY]: { responseChars }, + }, }; }