diff --git a/.changeset/23529-mcp-action-tools-withhold-permission-details.md b/.changeset/23529-mcp-action-tools-withhold-permission-details.md new file mode 100644 index 00000000000..5d70bc19ee9 --- /dev/null +++ b/.changeset/23529-mcp-action-tools-withhold-permission-details.md @@ -0,0 +1,11 @@ +--- +'@objectstack/mcp': patch +--- + +fix(mcp): `run_action` and `resume_run` withhold a `PERMISSION_DENIED` refusal's `details`, as the data tools already do (#23529) + +**What was wrong.** The two action tools render a thrown coded refusal through `errorResultFromThrown`, which served the refusal's `details` verbatim. The security gate's `PERMISSION_DENIED` carries the caller's own positions and permission-set names in `details`. Those are server-side diagnostics. The runtime dispatcher and the REST actions door withhold them, and the data tools and the record resource never serve a refusal's `details`. The action tools were the one place on the MCP surface that still served them, and `executeDeclarativeUpdateAction` writes as the caller, so the gate's write-side refusal reaches that renderer. + +**What a client reads now.** A `PERMISSION_DENIED` on `run_action` or `resume_run` answers the shape the data tools answer: `isError: true`, and the text is the JSON `{ "error": { "code", "message", "status" } }` with no `details`. The code, the message and the status are unchanged. + +Every other coded refusal on the two tools is unchanged. `ACTION_CONFIRMATION_REQUIRED` keeps the `details` that name the action and the member to set, and `resume_run`'s other refusals keep theirs. A throw with no code still answers its bare message. diff --git a/packages/mcp/src/mcp-action-refusal-details.test.ts b/packages/mcp/src/mcp-action-refusal-details.test.ts new file mode 100644 index 00000000000..18164a96266 --- /dev/null +++ b/packages/mcp/src/mcp-action-refusal-details.test.ts @@ -0,0 +1,186 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23529] `run_action` and `resume_run` WITHHOLD a permission refusal's + * `details`, as the data tools do (#23509) and as the runtime dispatcher does + * (the ruling on #7450). + * + * ## What was open + * + * The security gate's `PERMISSION_DENIED` carries the caller's `positions` and + * `permissionSets` in `details`. Those are server-side diagnostics: the + * dispatcher drops them from the wire, the REST actions door answers through + * the closed `details` list of `resolveThrownHttpError`, and the data tools + * render through `errorResultWithCode`, which never reads `details`. The two + * action tools rendered through `errorResultFromThrown`, which spread a thrown + * refusal's `details` verbatim into a tool result. The tool result rides a + * JSON-RPC 200, so the dispatcher's withholding never saw it. + * + * The reachable producer, read from source: a declarative update action + * (`operation: 'update'`) writes as the caller. A caller who may read the row + * but not update the object passes the subject load and meets the gate's + * write-side refusal, and nothing between the gate and this package catches + * it. A host script handler that rethrows a caller-scope refusal surfaces the + * same way. + * + * ## What is pinned here + * + * - The HTTP `/mcp` face, through `handleHttpRequest` and the real Streamable + * HTTP transport, as `mcp-coded-refusal.test.ts` drives it. The bridge is a + * double, because this package does not depend on `@objectstack/runtime`: + * it lets the caller read the subject row and refuses the declarative + * update with the gate's `PermissionDeniedError` shape. + * - The refusal answers `code` `PERMISSION_DENIED` and status 403, and the + * caller's position and permission-set NAMES appear nowhere in the HTTP + * response body: not under `details`, not anywhere else. A key-presence + * check would pass a renderer that moved the names to another key. + * - `resume_run` renders through the same function. No caller-scope write of + * its own reaches it today (it re-reads the subject through the not-found + * door), so its row pins the renderer, not a measured producer. + * - The control: on the same two tools, `ACTION_CONFIRMATION_REQUIRED` still + * serves its `details` (#15942). So the withholding is keyed on the + * refusal's code, and the `details` channel is shown to be live. + */ + +import { describe, it, expect } from 'vitest'; + +import { AI_ACTION_CONFIRMATION_MEMBER } from '@objectstack/spec/contracts'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { McpActionBridge, McpDataBridge } from './mcp-http-tools.js'; + +type Bridge = McpDataBridge & McpActionBridge; + +const OBJECT = 'crm_case'; +const RECORD_ID = 'case_1'; +/** A declarative update action: `operation: 'update'` writes the patch as the caller. */ +const ACTION = 'close_case'; +const RUN_ID = 'run_1'; + +/** Distinctive names, so a hit in the response body can only be a disclosure. */ +const POSITIONS = ['pin23529_position_case_reader', 'pin23529_position_everyone']; +const PERMISSION_SETS = ['pin23529_set_case_read_only', 'pin23529_set_member_default']; + +const DENIAL_MESSAGE = + 'You do not have permission to perform this action. Contact your administrator if you need access.'; + +/** + * The security gate's write-side refusal, in the shape `PermissionDeniedError` + * gives it: the localized sentence, `code`, both status spellings, the + * diagnostics in `details`, and the operator's sentence as a SIBLING of + * `details`. + */ +function writeSideDenial(): Error { + const err = Object.assign(new Error(DENIAL_MESSAGE), { + code: 'PERMISSION_DENIED', + status: 403, + statusCode: 403, + details: { operation: 'update', object: OBJECT, positions: POSITIONS, permissionSets: PERMISSION_SETS }, + developerMessage: + `[Security] Access denied: operation 'update' on object '${OBJECT}' ` + + `is not permitted for positions [${POSITIONS.join(', ')}]`, + }); + err.name = 'PermissionDeniedError'; + return err; +} + +/** The runtime's confirmation refusal (#15942): its `details` are the retry contract. */ +function confirmationRefusal(): Error { + return Object.assign( + new Error(`Action '${ACTION}' on '${OBJECT}' declares ai.requiresConfirmation: true — nothing was run.`), + { + code: 'ACTION_CONFIRMATION_REQUIRED', + status: 428, + details: { actionName: ACTION, objectName: OBJECT, confirmationMember: AI_ACTION_CONFIRMATION_MEMBER }, + }, + ); +} + +/** + * A caller who may READ the subject row and may not UPDATE the object. The + * subject read answers the row; the declarative update (and the run it would + * resume) is refused by the gate with `thrown`. + */ +function readerBridge(thrown: () => Error): Bridge { + return { + async listObjects() { return [{ name: OBJECT, label: 'Case', fieldCount: 1 }]; }, + async describeObject() { return { name: OBJECT, fields: [{ name: 'status', type: 'text' }] }; }, + async query() { return { object: OBJECT, records: [], total: 0 }; }, + async get(_object, id) { return { id, status: 'open' }; }, + async create() { return {}; }, + async update() { throw thrown(); }, + async remove() { return {}; }, + async listActions() { + return [{ name: ACTION, objectName: OBJECT, type: 'script', requiresRecord: true, requiresConfirmation: false }]; + }, + async runAction() { throw thrown(); }, + async resumeRun() { throw thrown(); }, + }; +} + +let nextId = 1; + +/** One `tools/call` over the real HTTP transport; answers the raw body text and the parsed tool result. */ +async function callOverHttp(bridge: Bridge, name: string, args: Record) { + const body = { jsonrpc: '2.0', id: nextId++, method: 'tools/call', params: { name, arguments: args } }; + const runtime = new MCPServerRuntime({ name: 'pin', version: '0.0.0' }); + const res = await runtime.handleHttpRequest( + new Request('http://localhost/api/v1/mcp', { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: JSON.stringify(body), + }), + { bridge, parsedBody: body }, + ); + expect(res.status, `${name}: the HTTP transport itself answered ${res.status}`).toBe(200); + const raw = await res.text(); + const json = JSON.parse(raw) as { result?: any; error?: unknown }; + expect(json.error, `${name}: JSON-RPC error ${JSON.stringify(json.error)}`).toBeUndefined(); + return { raw, result: json.result }; +} + +const DOORS = [ + { tool: 'run_action', args: { actionName: ACTION, objectName: OBJECT, recordId: RECORD_ID } }, + { tool: 'resume_run', args: { runId: RUN_ID, values: {} } }, +] as const; + +describe('run_action / resume_run over HTTP /mcp: a permission refusal withholds its details (#23529)', () => { + it('control: the caller may read the subject row', async () => { + const { result } = await callOverHttp(readerBridge(writeSideDenial), 'get_record', { + objectName: OBJECT, + recordId: RECORD_ID, + }); + expect(result.isError).toBeFalsy(); + expect(JSON.parse(result.content[0].text)).toEqual({ id: RECORD_ID, status: 'open' }); + }); + + for (const door of DOORS) { + it(`${door.tool}: PERMISSION_DENIED answers code, message and status, and the caller's grants appear nowhere`, async () => { + const { raw, result } = await callOverHttp(readerBridge(writeSideDenial), door.tool, door.args); + + expect(result.isError).toBe(true); + expect(result.content).toHaveLength(1); + expect(JSON.parse(result.content[0].text)).toEqual({ + error: { code: 'PERMISSION_DENIED', message: DENIAL_MESSAGE, status: 403 }, + }); + // Every name, against the whole HTTP response body, not one key. + for (const name of [...POSITIONS, ...PERMISSION_SETS]) { + expect(raw, `${door.tool}: '${name}' reached the wire`).not.toContain(name); + } + }); + + it(`${door.tool}: control — ACTION_CONFIRMATION_REQUIRED still serves its retry details (#15942)`, async () => { + const { result } = await callOverHttp(readerBridge(confirmationRefusal), door.tool, door.args); + + expect(result.isError).toBe(true); + expect(JSON.parse(result.content[0].text)).toEqual({ + error: { + code: 'ACTION_CONFIRMATION_REQUIRED', + message: `Action '${ACTION}' on '${OBJECT}' declares ai.requiresConfirmation: true — nothing was run.`, + status: 428, + details: { actionName: ACTION, objectName: OBJECT, confirmationMember: AI_ACTION_CONFIRMATION_MEMBER }, + }, + }); + }); + } +}); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index b7a491d6641..232f1b56c01 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -35,6 +35,9 @@ import { z } from 'zod'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import type { EngineAggregateOptions } from '@objectstack/spec/data'; +// [#23529] The catalogue's spelling of `PERMISSION_DENIED`, the code whose +// `details` `errorResultFromThrown` withholds. Imported, never hand-spelled. +import { StandardErrorCode } from '@objectstack/spec/api'; import { MCP_OAUTH_SCOPE_DATA_READ, MCP_OAUTH_SCOPE_DATA_WRITE, @@ -302,16 +305,35 @@ function errorResult(message: string) { * nobody and the agent is back to guessing the member's spelling from * documentation. * - * Uncoded throws fall through to `errorResult` unchanged, so this widens what a - * caller can read and narrows nothing. + * Uncoded throws fall through to `errorResult` unchanged. * * `run_action` and `resume_run` only. The other catches render through * {@link errorResultWithCode}, which serves the same envelope WITHOUT `details`; * that function says why. + * + * [#23529] ⛔ A `PERMISSION_DENIED` is the exception: it renders through + * {@link errorResultWithCode}, so its `details` are WITHHELD here too. The + * security gate puts the caller's `positions` and `permissionSets` in those + * `details`; the ruling on #7450 keeps them off the wire, and the runtime + * dispatcher and the REST actions door both drop them. These two tools are + * reachable by that refusal: a declarative update action writes as the caller, + * so a caller who may read the row but not update it meets the gate's + * write-side refusal, and a host script handler can rethrow one. The answer is + * a tool result inside a JSON-RPC 200, which the dispatcher's withholding never + * sees, so it is withheld at this renderer. + * + * The question is asked on the DECLARED code, spelled from the spec's standard + * catalogue rather than restated, and answered with the data tools' own + * function rather than a second filter of `details`. Every `PermissionDeniedError` + * declares this code (it is a class constant), and a throw with no code never + * reaches the `details` read at all. Every other coded refusal keeps its + * `details` as before, so #15942's confirmation contract and `resume_run`'s + * REST-built `details` are unchanged. */ function errorResultFromThrown(err: unknown) { const envelope = thrownRefusalEnvelope(err); if (!envelope) return errorResult(messageOf(err)); + if (envelope.code === StandardErrorCode.enum.PERMISSION_DENIED) return errorResultWithCode(err); const details = (err as { details?: unknown }).details; return codedErrorResult({ ...envelope, ...(details !== undefined ? { details } : {}) }); }