From b7877579595d01fb7ffb745ff40e2632bb435e42 Mon Sep 17 00:00:00 2001 From: rp-maintainers Date: Mon, 27 Jul 2026 23:04:41 +0000 Subject: [PATCH] fix(sdk): read MCP verdicts fail-closed; complete the prompt options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from cross-checking the TypeScript SDK against the Python one. Verdict reading was fail-open. The verdict methods returned the gateway's body verbatim, so `if (verdict.outcome === 'deny')` was false for an empty 200, an unknown outcome value, or a proxy error page — and the caller proceeded with the tool call. A default-deny policy boundary has to survive the client, not just the gateway: only an explicit allow is now an allow, and anything unparseable denies (run steps stop) with the payload preserved under `response` so the cause stays debuggable. Same change in the MCP server's shared helper. A 4xx carrying no verdict still throws rather than reading as a deny. Axum rejects a malformed body with a plain-text 422, and turning the caller's own bug into a policy refusal would bury it. Fail-closed applies to reading a decision the gateway actually made, not to inventing one it never sent. Prompt options were incomplete. `missing` ("error" | "empty") was unreachable, so callers could not opt out of the default hard failure on an unsupplied variable; the `x-routeplane-cohort` header had no typed route, leaving sticky A/B assignment unusable; and `complete()` threaded only `model`, so temperature, max_tokens and the rest of the chat body were unreachable. `variables` widens to any JSON, matching the gateway. All additive — existing call sites are unchanged. Verified the fail-closed tests fail against the old pass-through: reverting the normalizers turns exactly those 12 red, and the 422-still-throws case stays green. --- README.md | 6 ++ packages/mcp-server/src/tools/security.ts | 43 +++++++--- packages/sdk/src/core/resources/index.ts | 7 +- packages/sdk/src/core/resources/mcp.ts | 60 ++++++++++--- packages/sdk/src/core/resources/prompts.ts | 65 ++++++++++++-- .../sdk/src/core/resources/resources.test.ts | 85 +++++++++++++++++++ 6 files changed, 230 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 1627d34..fecf272 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,12 @@ const inspection = await rp.mcp.inspectToolResult(result); anomaly operator surface (`anomalyStatus`, `clearAnomaly`), and the enforcement-event feed (`securityEvents`). +Reading a verdict is **fail-closed**: only an explicit allow is an allow. An empty body, an +unknown `outcome`, or a proxy's error page all read as a deny, so a response the client +cannot parse can never fall through as permission granted. (A 4xx carrying no verdict at +all still throws — that is a malformed request, and turning your own bug into a policy deny +would hide it.) + All of it is gated on the tenant's `AgenticSecurity` entitlement. The gateway hides the surface rather than refusing it, so an un-entitled key gets `RouteplaneError` with status **404** — not a 403. diff --git a/packages/mcp-server/src/tools/security.ts b/packages/mcp-server/src/tools/security.ts index f009809..87cf773 100644 --- a/packages/mcp-server/src/tools/security.ts +++ b/packages/mcp-server/src/tools/security.ts @@ -10,7 +10,6 @@ */ import { RouteplaneError } from '@routeplane/sdk/core'; -import type { McpVerdict } from '@routeplane/sdk/core'; import { type ToolDef, EMPTY_SCHEMA, @@ -28,27 +27,41 @@ import { enumStr, } from './common.js'; +/** Whether a payload is an object carrying `key` set to one of `values`. */ +function hasLiteral(payload: unknown, key: string, values: string[]): boolean { + if (payload === null || typeof payload !== 'object') return false; + const actual = (payload as Record)[key]; + return typeof actual === 'string' && values.includes(actual); +} + /** * Run a policy call, returning the deny body rather than an error result when * the gateway refuses. A deny is the enforcement point working — surfacing it * as a tool error would tell the assistant the gateway broke, when in fact it * answered. Statuses outside the deny envelope (404 un-entitled, 401 bad key) * still propagate to the caller's error handling. + * + * Reading the answer is fail-closed. `key` names the verdict field and `allow` + * the one value that means "proceed"; anything else the assistant is shown as a + * refusal, so a response we cannot parse can never read as permission granted. */ -async function verdict(call: () => Promise): Promise { +async function verdict( + call: () => Promise, + key: 'outcome' | 'decision', + allow: 'allow' | 'continue', +): Promise { + const refuse = key === 'outcome' ? 'deny' : 'stop'; + let payload: unknown; try { - return await call(); + payload = await call(); } catch (err) { if (err instanceof RouteplaneError && (err.status === 422 || err.status === 429)) { - const body = err.body; - if (body !== null && typeof body === 'object' && (body as McpVerdict).outcome === 'deny') { - return body; - } - // run/step reports a refusal as `decision: "stop"` rather than a deny. - if (body !== null && typeof body === 'object' && 'decision' in body) return body; + if (hasLiteral(err.body, key, [refuse])) return err.body; } throw err; } + if (hasLiteral(payload, key, [allow, refuse])) return payload; + return { [key]: refuse, reason: 'unrecognized gateway response', response: payload }; } const getGuardrailOutcomes: ToolDef = { @@ -98,7 +111,8 @@ const authorizeToolCall: ToolDef = { if (manifest !== undefined) body.server_manifest = manifest; const runId = optString(args, 'run_id'); if (runId !== undefined) body.run_id = runId; - return jsonResult(await verdict(() => make().post('/v1/mcp/tool-call/authorize', body))); + const call = () => make().post('/v1/mcp/tool-call/authorize', body); + return jsonResult(await verdict(call, 'outcome', 'allow')); }, }; @@ -109,7 +123,8 @@ const inspectToolResult: ToolDef = { inputSchema: objectSchema({ content: str('The tool-result content to inspect.') }, ['content']), handler: async (make, args) => { const content = requireString(args, 'content'); - return jsonResult(await verdict(() => make().post('/v1/mcp/tool-result/inspect', { content }))); + const call = () => make().post('/v1/mcp/tool-result/inspect', { content }); + return jsonResult(await verdict(call, 'outcome', 'allow')); }, }; @@ -132,7 +147,8 @@ const evaluateSampling: ToolDef = { }; const agentId = optString(args, 'agent_id'); if (agentId !== undefined) body.agent_id = agentId; - return jsonResult(await verdict(() => make().post('/v1/mcp/sampling/evaluate', body))); + const call = () => make().post('/v1/mcp/sampling/evaluate', body); + return jsonResult(await verdict(call, 'outcome', 'allow')); }, }; @@ -154,7 +170,8 @@ const mcpRunStep: ToolDef = { if (agentId !== undefined) body.agent_id = agentId; const cost = optNumber(args, 'cost_micro_usd'); if (cost !== undefined) body.cost_micro_usd = cost; - return jsonResult(await verdict(() => make().post('/v1/mcp/run/step', body))); + const call = () => make().post('/v1/mcp/run/step', body); + return jsonResult(await verdict(call, 'decision', 'continue')); }, }; diff --git a/packages/sdk/src/core/resources/index.ts b/packages/sdk/src/core/resources/index.ts index bd73383..9f58fbf 100644 --- a/packages/sdk/src/core/resources/index.ts +++ b/packages/sdk/src/core/resources/index.ts @@ -1,5 +1,10 @@ export { PromptResource } from './prompts.js'; -export type { PromptCompleteOptions } from './prompts.js'; +export type { + MissingVariablePolicy, + PromptCompleteOptions, + PromptRenderOptions, + PromptVariables, +} from './prompts.js'; export { LogResource } from './logs.js'; export type { LogListOptions } from './logs.js'; export { FinOpsResource } from './finops.js'; diff --git a/packages/sdk/src/core/resources/mcp.ts b/packages/sdk/src/core/resources/mcp.ts index 1bcff3f..3d8bf42 100644 --- a/packages/sdk/src/core/resources/mcp.ts +++ b/packages/sdk/src/core/resources/mcp.ts @@ -8,9 +8,13 @@ * * Enforcement points are default-deny, and a deny is a decision rather than a * failure — the gateway returns it as HTTP 422 (429 for a quota deny) with a - * structured body. The three verdict methods decode those into an `McpVerdict` + * structured body. The verdict methods decode those into an `McpVerdict` * instead of throwing, so calling code branches on `outcome` and only has to * catch genuine transport or entitlement errors. + * + * Reading a verdict is fail-closed: only an explicit allow is an allow, so a + * response this client cannot parse denies rather than falling through. The + * default-deny posture has to survive the client, not just the gateway. */ import { RouteplaneError, type RouteplaneCoreClient } from '../client.js'; @@ -82,17 +86,50 @@ export interface ReceiptIssueOptions { /** Statuses the gateway answers a deny with, rather than failing the request. */ const VERDICT_STATUSES = new Set([422, 429]); +const UNRECOGNIZED = 'unrecognized gateway response'; + +/** Whether a payload is an object carrying `key` set to one of `values`. */ +function hasLiteral(payload: unknown, key: string, values: string[]): boolean { + if (payload === null || typeof payload !== 'object') return false; + const actual = (payload as Record)[key]; + return typeof actual === 'string' && values.includes(actual); +} + +/** + * Read a response as a verdict, fail-closed. + * + * Only an explicit `outcome: 'allow' | 'deny'` is honoured. An empty 200, a + * proxy's error page, a shape the gateway changed under us — anything else + * reads as a deny, because a policy boundary that opens when it is confused is + * worse than one that refuses. The unparsed payload rides along under + * `response` so the cause is still debuggable. + */ +function asVerdict(payload: unknown): McpVerdict { + if (hasLiteral(payload, 'outcome', ['allow', 'deny'])) return payload as McpVerdict; + return { outcome: 'deny', reason: UNRECOGNIZED, response: payload }; +} + +/** The run-step analogue of `asVerdict` — an unreadable answer stops the loop. */ +function asRunStep(payload: unknown): McpRunStepDecision { + if (hasLiteral(payload, 'decision', ['continue', 'stop'])) { + return payload as McpRunStepDecision; + } + return { decision: 'stop', reason: UNRECOGNIZED, iterations: 0, response: payload }; +} + /** * Decode a structured deny into a value. The gateway signals a policy refusal * with a 4xx carrying `{ outcome: 'deny', ... }`; anything else — 404 for an * un-entitled tenant, 401 for a bad key, a real transport error — rethrows. + * + * Note this deliberately does *not* fail closed: a 422 that carries no verdict + * is a rejected request (a malformed body), and turning the caller's own bug + * into a policy deny would hide it. Fail-closed applies to reading a decision + * the gateway actually made, which is `asVerdict`'s job. */ function verdictOrThrow(err: unknown): McpVerdict { if (err instanceof RouteplaneError && VERDICT_STATUSES.has(err.status)) { - const body = err.body; - if (body !== null && typeof body === 'object' && (body as McpVerdict).outcome === 'deny') { - return body as McpVerdict; - } + if (hasLiteral(err.body, 'outcome', ['deny'])) return err.body as McpVerdict; } throw err; } @@ -100,10 +137,7 @@ function verdictOrThrow(err: unknown): McpVerdict { /** The run-step analogue of `verdictOrThrow` — a refused step reports `decision: 'stop'`. */ function stopOrThrow(err: unknown): McpRunStepDecision { if (err instanceof RouteplaneError && err.status === 422) { - const body = err.body; - if (body !== null && typeof body === 'object' && (body as McpRunStepDecision).decision === 'stop') { - return body as McpRunStepDecision; - } + if (hasLiteral(err.body, 'decision', ['stop'])) return err.body as McpRunStepDecision; } throw err; } @@ -125,7 +159,7 @@ export class McpResource { if (opts.serverManifest !== undefined) body.server_manifest = opts.serverManifest; if (opts.runId !== undefined) body.run_id = opts.runId; try { - return await this.client.post('/v1/mcp/tool-call/authorize', body); + return asVerdict(await this.client.post('/v1/mcp/tool-call/authorize', body)); } catch (err) { return verdictOrThrow(err); } @@ -138,7 +172,7 @@ export class McpResource { */ async inspectToolResult(content: string): Promise { try { - return await this.client.post('/v1/mcp/tool-result/inspect', { content }); + return asVerdict(await this.client.post('/v1/mcp/tool-result/inspect', { content })); } catch (err) { return verdictOrThrow(err); } @@ -153,7 +187,7 @@ export class McpResource { if (opts.agentId !== undefined) body.agent_id = opts.agentId; if (opts.costMicroUsd !== undefined) body.cost_micro_usd = opts.costMicroUsd; try { - return await this.client.post('/v1/mcp/run/step', body); + return asRunStep(await this.client.post('/v1/mcp/run/step', body)); } catch (err) { return stopOrThrow(err); } @@ -180,7 +214,7 @@ export class McpResource { const body: Record = { server: opts.server, prompt: opts.prompt }; if (opts.agentId !== undefined) body.agent_id = opts.agentId; try { - return await this.client.post('/v1/mcp/sampling/evaluate', body); + return asVerdict(await this.client.post('/v1/mcp/sampling/evaluate', body)); } catch (err) { return verdictOrThrow(err); } diff --git a/packages/sdk/src/core/resources/prompts.ts b/packages/sdk/src/core/resources/prompts.ts index d72bee7..7162134 100644 --- a/packages/sdk/src/core/resources/prompts.ts +++ b/packages/sdk/src/core/resources/prompts.ts @@ -4,13 +4,48 @@ import type { RouteplaneCoreClient } from '../client.js'; import { createHeaders } from '../headers.js'; import type { Completion, Prompt, RenderedPrompt } from '../models.js'; -export interface PromptCompleteOptions { +/** + * Template variables. Values are any JSON — the gateway substitutes objects and + * numbers as readily as strings. + */ +export type PromptVariables = Record; + +/** + * What to do when the template references a variable the caller did not supply. + * The gateway defaults to `error`; `empty` substitutes an empty string instead. + */ +export type MissingVariablePolicy = 'error' | 'empty'; + +export interface PromptRenderOptions { + /** Missing-variable policy. Defaults to the gateway's `error`. */ + missing?: MissingVariablePolicy; + /** + * A/B cohort key, sent as `x-routeplane-cohort`. Assignment is sticky per + * cohort key, so pass a stable caller-chosen identity. Absent means the + * experiment serves its control arm. + */ + cohort?: string; +} + +export interface PromptCompleteOptions extends PromptRenderOptions { /** Template variables to substitute. */ - variables?: Record; + variables?: PromptVariables; /** Model override, threaded into the completion request body. */ model?: string; /** Provider (or fallback chain) override, sent as `x-routeplane-provider`. */ provider?: string; + /** + * Further chat-request fields merged into the body (`temperature`, + * `max_tokens`, `stream`, `user`, …). Body fields win over the prompt + * version's `default_params` and `default_model`; `messages` is always the + * rendered template and cannot be overridden. + * + * Routing options do not belong here. The gateway flattens this body into a + * chat request, which ignores fields it does not know — so a `provider` put + * here would be dropped silently rather than rejected. Use the typed + * `provider` and `cohort` options, which travel as headers. + */ + overrides?: Record; } export class PromptResource { @@ -22,24 +57,36 @@ export class PromptResource { } /** Render a template with variables, without running a completion. */ - render(reference: string, variables?: Record): Promise { - return this.client.post( + async render( + reference: string, + variables?: PromptVariables, + opts: PromptRenderOptions = {}, + ): Promise { + const body: Record = { variables: variables ?? {} }; + if (opts.missing !== undefined) body.missing = opts.missing; + const { data } = await this.client.postWithMeta( `/v1/prompts/${encodeURIComponent(reference)}/render`, - { variables: variables ?? {} }, + body, + opts.cohort !== undefined ? createHeaders({ cohort: opts.cohort }) : undefined, ); + return data; } /** Render and run a completion in one call. */ async complete(reference: string, opts: PromptCompleteOptions = {}): Promise { - const body: Record = {}; + const body: Record = { ...(opts.overrides ?? {}) }; if (opts.variables !== undefined) body.variables = opts.variables; + if (opts.missing !== undefined) body.missing = opts.missing; if (opts.model !== undefined) body.model = opts.model; - const extraHeaders = - opts.provider !== undefined ? createHeaders({ provider: opts.provider }) : undefined; + + const headers = createHeaders({ + ...(opts.provider !== undefined ? { provider: opts.provider } : {}), + ...(opts.cohort !== undefined ? { cohort: opts.cohort } : {}), + }); const { data } = await this.client.postWithMeta( `/v1/prompts/${encodeURIComponent(reference)}/completions`, body, - extraHeaders, + Object.keys(headers).length > 0 ? headers : undefined, ); return data; } diff --git a/packages/sdk/src/core/resources/resources.test.ts b/packages/sdk/src/core/resources/resources.test.ts index 1c7a70d..5bbcacb 100644 --- a/packages/sdk/src/core/resources/resources.test.ts +++ b/packages/sdk/src/core/resources/resources.test.ts @@ -76,6 +76,13 @@ describe('PromptResource', () => { expect(captured[0]?.body).toEqual({ variables: { name: 'bob' } }); }); + it('render() forwards the missing policy in the body and the cohort as a header', async () => { + stubFetch({ text: 'hi' }); + await client().prompts.render('greet', { name: 'bob' }, { missing: 'empty', cohort: 'user-7' }); + expect(captured[0]?.body).toEqual({ variables: { name: 'bob' }, missing: 'empty' }); + expect(captured[0]?.headers['x-routeplane-cohort']).toBe('user-7'); + }); + it('complete() → POST /v1/prompts/{reference}/completions, model in body, provider as header', async () => { stubFetch({ id: 'cmpl_1' }); const out = await client().prompts.complete('greet', { @@ -89,6 +96,33 @@ describe('PromptResource', () => { expect(captured[0]?.headers['x-routeplane-provider']).toBe('anthropic'); expect(out).toEqual({ id: 'cmpl_1' }); }); + + it('complete() never routes via the body — provider and cohort go as headers only', async () => { + stubFetch({ id: 'cmpl_1' }); + await client().prompts.complete('greet', { provider: 'groq', cohort: 'user-7' }); + // The gateway flattens this body into a chat request that ignores unknown + // fields, so a routing option left here would be a silent no-op. + expect(captured[0]?.body).toEqual({}); + expect(captured[0]?.headers['x-routeplane-provider']).toBe('groq'); + expect(captured[0]?.headers['x-routeplane-cohort']).toBe('user-7'); + }); + + it('complete() merges chat overrides into the body, with typed fields winning', async () => { + stubFetch({ id: 'cmpl_1' }); + await client().prompts.complete('greet', { + variables: { name: 'bob' }, + model: 'gpt-4o-mini', + missing: 'empty', + overrides: { temperature: 0.2, max_tokens: 512, model: 'ignored' }, + }); + expect(captured[0]?.body).toEqual({ + variables: { name: 'bob' }, + missing: 'empty', + model: 'gpt-4o-mini', + temperature: 0.2, + max_tokens: 512, + }); + }); }); describe('LogResource', () => { @@ -300,6 +334,57 @@ describe('McpResource — enforcement points', () => { }); }); +describe('McpResource — fail-closed verdict reading', () => { + // A policy boundary that opens when it cannot parse the answer is worse than + // one that refuses. Only an explicit allow may read as an allow. + const unreadable: [string, unknown][] = [ + ['an empty 200', {}], + ['an unknown outcome', { outcome: 'maybe' }], + ['a non-string outcome', { outcome: true }], + ['an error page', 'Bad Gateway'], + ['a null body', null], + ]; + + for (const [label, body] of unreadable) { + it(`authorizeToolCall() denies on ${label}`, async () => { + stubFetch(body); + const v = await client().mcp.authorizeToolCall({ server: 'github', tool: 'x' }); + expect(v.outcome).toBe('deny'); + expect(v.reason).toBe('unrecognized gateway response'); + }); + + it(`runStep() stops on ${label}`, async () => { + stubFetch(body); + const out = await client().mcp.runStep({ runId: 'run-1' }); + expect(out.decision).toBe('stop'); + expect(out.reason).toBe('unrecognized gateway response'); + }); + } + + it('inspectToolResult() and evaluateSampling() deny on an unreadable 200', async () => { + stubFetch({ status: 'fine' }); + const c = client(); + expect((await c.mcp.inspectToolResult('x')).outcome).toBe('deny'); + expect((await c.mcp.evaluateSampling({ server: 's', prompt: 'p' })).outcome).toBe('deny'); + }); + + it('preserves the unparsed payload for debugging', async () => { + stubFetch({ unexpected: 'shape' }); + const v = await client().mcp.authorizeToolCall({ server: 'github', tool: 'x' }); + expect(v.response).toEqual({ unexpected: 'shape' }); + }); + + it('a 422 carrying no verdict still throws, so a malformed request is not hidden', async () => { + // Axum rejects an unparseable body with a plain-text 422. Reading that as a + // policy deny would bury the caller's own bug. + stubFetchStatus(422, 'Failed to deserialize the JSON body'); + await expect(client().mcp.authorizeToolCall({ server: 'github', tool: 'x' })).rejects.toThrow( + RouteplaneError, + ); + await expect(client().mcp.runStep({ runId: 'run-1' })).rejects.toThrow(RouteplaneError); + }); +}); + describe('McpResource — run governance', () => { it('runStep() → POST /v1/mcp/run/step with run_id and cost', async () => { stubFetch({ decision: 'continue', iterations: 3 });