diff --git a/.changeset/28550-mcp-aggregate-entry-filter.md b/.changeset/28550-mcp-aggregate-entry-filter.md new file mode 100644 index 00000000000..3125708a287 --- /dev/null +++ b/.changeset/28550-mcp-aggregate-entry-filter.md @@ -0,0 +1,19 @@ +--- +'@objectstack/mcp': patch +--- + +fix(mcp)!: an `aggregate_records` aggregation entry carries its `filter` to the engine and refuses an undeclared key by name (#28550) + +Clause-②: yes (narrowing) — the aggregation entry now accepts `filter` (a widening of the tool's input), and refuses any other undeclared entry key, which it stripped and ignored before (a narrowing). +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the MCP `aggregate_records` tool (`@objectstack/mcp`), on both the HTTP and the stdio transport. + +- **Before.** Each aggregation entry was a non-strict `{ function, field, alias }`. A per-aggregation `filter` was stripped before the engine was asked, so `{"function":"count","alias":"n","filter":{"status":"active"}}` beside an unfiltered count answered the total twice, while the REST query door answered the filtered count for the same body. Any other unknown entry key was dropped the same way, with no error. +- **Now.** The entry is built from the engine's `AggregationNodeSchema` members (`function`, `field`, `alias`, `filter`) and is closed. The `filter` is forwarded unchanged to `engine.aggregate`, the path REST uses, so both doors give one answer, including the security middleware's 403 `PERMISSION_DENIED` for a filter on a field the caller cannot read. An undeclared entry key (the retired `distinct` included) is refused by name before anything runs, as an undeclared top-level key already was. +- **Unchanged.** Entries without a filter answer as before; the REST query door is untouched. +- **Who sees it.** An MCP client that sent an extra key inside an aggregation entry: the call was answered with that key ignored, and it is now refused naming the key. + +**The remedy.** Drop the key, or for a per-aggregation condition send it as the entry's `filter`. diff --git a/packages/mcp/src/mcp-http-tools.aggregation-entry.test.ts b/packages/mcp/src/mcp-http-tools.aggregation-entry.test.ts new file mode 100644 index 00000000000..938db499515 --- /dev/null +++ b/packages/mcp/src/mcp-http-tools.aggregation-entry.test.ts @@ -0,0 +1,182 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `aggregate_records`: each aggregation entry carries its `filter` to the + * engine and refuses an undeclared key by name (#28550). + * + * THE DEFECT. The entry was a plain `z.object({ function, field, alias })`, so + * zod's `.strip` default deleted a per-aggregation `filter` before the handler + * ran. `{ function: 'count', alias: 'n', filter: { status: 'active' } }` beside + * an unfiltered `count` answered the same number twice (5 and 5 on the stock + * showcase), while the REST query door answered 2 and 5 for the same body. An + * unknown entry key (`bogusKey`) was dropped the same way. The top-level + * closure (#16913) never reached inside the array. + * + * WHAT IS PINNED HERE, at the tool layer over a recording bridge: the entry's + * `filter` reaches the bridge byte-for-byte (the bridge hands `aggregations` to + * `engine.aggregate`, which applies it, so nothing in this package evaluates + * it); a malformed filter gets the engine node's own refusal at the door; an + * undeclared entry key is refused by name before the bridge is asked; and the + * advertised JSON Schema declares both facts. The MCP-equals-REST answer over a + * real engine, and the field-permission refusal, are pinned end to end in + * `packages/qa/dogfood/test/showcase-mcp-aggregate-filter.dogfood.test.ts`. + * + * CONTROLS. Entries without a filter reach the bridge exactly as before, and + * the top-level unknown-key posture is unchanged. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; + +import { MCPServerRuntime } from './mcp-server-runtime.js'; +import type { McpDataBridge } from './mcp-http-tools.js'; + +interface RecordingBridge extends McpDataBridge { + calls: any[]; +} + +function makeBridge(): RecordingBridge { + const calls: any[] = []; + return { + calls, + async listObjects() { return [{ name: 'showcase_project' }]; }, + async describeObject(name: string) { return { name, fields: [] }; }, + async query(object: string) { return { object, records: [] }; }, + async get() { return null; }, + async create(object: string) { return { object, id: 'n1' }; }, + async update(object: string, id: string) { return { object, id }; }, + async remove(object: string, id: string) { return { object, id, success: true }; }, + async aggregate(object: string, opts: any) { + calls.push([object, opts]); + return [{ n: 2, all: 5 }]; + }, + }; +} + +let nextId = 1; + +async function rpc(runtime: MCPServerRuntime, bridge: RecordingBridge, method: string, params?: unknown) { + const body = { jsonrpc: '2.0', id: nextId++, method, ...(params === undefined ? {} : { params }) }; + const req = new Request('http://localhost/api/v1/mcp', { + method: 'POST', + headers: { 'content-type': 'application/json', accept: 'application/json, text/event-stream' }, + body: JSON.stringify(body), + }); + const res = await runtime.handleHttpRequest(req, { bridge, parsedBody: body }); + return (await res.json()) as any; +} + +async function aggregate(runtime: MCPServerRuntime, bridge: RecordingBridge, args: Record) { + const json = await rpc(runtime, bridge, 'tools/call', { name: 'aggregate_records', arguments: args }); + const text = json.result?.content?.map((c: any) => c.text).join('\n') ?? json.error?.message ?? ''; + return { refused: json.result?.isError === true || Boolean(json.error), text: String(text) }; +} + +describe('aggregate_records — the aggregation entry (#28550)', () => { + let runtime: MCPServerRuntime; + let bridge: RecordingBridge; + + beforeEach(() => { + runtime = new MCPServerRuntime({ name: 't', version: '1.0.0' }); + bridge = makeBridge(); + }); + + it('the reported call: an entry `filter` reaches the bridge unchanged, beside an unfiltered sibling', async () => { + const aggregations = [ + { function: 'count', alias: 'n', filter: { status: 'active' } }, + { function: 'count', alias: 'all' }, + ]; + const { refused, text } = await aggregate(runtime, bridge, { objectName: 'showcase_project', aggregations }); + expect(refused, text).toBe(false); + expect(bridge.calls).toHaveLength(1); + expect(bridge.calls[0][0]).toBe('showcase_project'); + // Exactly what was sent: the filter on the first entry, none on the second. + expect(bridge.calls[0][1].aggregations).toEqual(aggregations); + }); + + it('an operator filter reaches the bridge as sent', async () => { + const aggregations = [ + { function: 'sum', field: 'budget', alias: 'big', filter: { budget: { $gte: 1000 }, status: { $in: ['active', 'planning'] } } }, + ]; + const { refused, text } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + aggregations, + groupBy: ['status'], + where: { owner: 'u1' }, + }); + expect(refused, text).toBe(false); + expect(bridge.calls[0][1]).toMatchObject({ aggregations, groupBy: ['status'], where: { owner: 'u1' } }); + }); + + it('CONTROL — entries without a filter reach the bridge exactly as before', async () => { + const aggregations = [ + { function: 'count', alias: 'n' }, + { function: 'sum', field: 'budget', alias: 'total' }, + ]; + const { refused, text } = await aggregate(runtime, bridge, { objectName: 'showcase_project', aggregations }); + expect(refused, text).toBe(false); + expect(bridge.calls[0][1].aggregations).toEqual(aggregations); + }); + + it('an undeclared entry key is refused by name, and the bridge is never asked', async () => { + const { refused, text } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + aggregations: [{ function: 'count', alias: 'n', bogusKey: 1 }], + }); + expect(refused).toBe(true); + expect(text).toContain('bogusKey'); + expect(bridge.calls).toHaveLength(0); + }); + + it('an entry `where` is refused, and the refusal names `filter` as the spelling to send', async () => { + const { refused, text } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + aggregations: [{ function: 'count', alias: 'n', where: { status: 'active' } }], + }); + expect(refused).toBe(true); + expect(text).toContain('where'); + expect(text).toContain('filter'); + expect(bridge.calls).toHaveLength(0); + }); + + it('the retired `distinct` entry key is refused, not dropped', async () => { + const { refused, text } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + aggregations: [{ function: 'sum', field: 'budget', alias: 's', distinct: true }], + }); + expect(refused).toBe(true); + expect(text).toContain('distinct'); + expect(bridge.calls).toHaveLength(0); + }); + + it('a filter that is not a filter object is refused at the door by the engine node\'s own shape', async () => { + for (const filter of ["status = 'active'", ['status', '=', 'active'], 7]) { + const { refused } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + aggregations: [{ function: 'count', alias: 'n', filter }], + }); + expect(refused, JSON.stringify(filter)).toBe(true); + } + expect(bridge.calls).toHaveLength(0); + }); + + it('CONTROL — an undeclared TOP-LEVEL key is still refused, naming the declared spelling', async () => { + const { refused, text } = await aggregate(runtime, bridge, { + objectName: 'showcase_project', + metrics: [{ function: 'count', alias: 'n' }], + }); + expect(refused).toBe(true); + expect(text).toContain('metrics'); + expect(text).toContain('aggregations'); + expect(bridge.calls).toHaveLength(0); + }); + + it('`tools/list` advertises the entry as closed, with `filter` declared', async () => { + const json = await rpc(runtime, bridge, 'tools/list'); + const tool = (json.result?.tools ?? []).find((t: any) => t.name === 'aggregate_records'); + expect(tool).toBeDefined(); + const entry = tool.inputSchema?.properties?.aggregations?.items; + expect(entry?.additionalProperties).toBe(false); + expect(Object.keys(entry?.properties ?? {}).sort()).toEqual(['alias', 'field', 'filter', 'function']); + expect(entry?.required).toEqual(expect.arrayContaining(['function', 'alias'])); + }); +}); diff --git a/packages/mcp/src/mcp-http-tools.ts b/packages/mcp/src/mcp-http-tools.ts index f1c100f1b74..72a5e1b6fd3 100644 --- a/packages/mcp/src/mcp-http-tools.ts +++ b/packages/mcp/src/mcp-http-tools.ts @@ -39,6 +39,10 @@ import { z } from 'zod'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import type { EngineAggregateOptions } from '@objectstack/spec/data'; +// [#28550] The engine's aggregation entry. Each `aggregate_records` entry is +// built from ITS members, so this door and the REST query door (which parses +// `FindDataRequestSchema`, whose `aggregations` are this node) judge one shape. +import { AggregationNodeSchema } 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'; @@ -689,6 +693,56 @@ function strictToolInput( .strict(); } +/** + * [#28550] One `aggregate_records` aggregation entry: closed, and carrying the + * per-aggregation `filter`. + * + * The entry used to be a plain `z.object({ function, field, alias })`. Zod's + * default for an object is `.strip`, and {@link strictToolInput} closes only the + * tool's TOP-LEVEL keys, so an entry's `filter` was deleted before the handler + * ran: `{ function: 'count', alias: 'n', filter: { status: 'active' } }` answered + * the unconditional count, beside the REST query door answering the filtered + * one for the same body. Any other unknown entry key vanished the same way. + * + * Both halves of the fix are the entry's shape, nothing else: + * + * - **`filter` is declared**, and it is the engine's own member, + * `AggregationNodeSchema.shape.filter` (`FilterConditionSchema`), the slot the + * REST door parses. The handler forwards `aggregations` to the bridge + * unchanged, and every bridge hands them to `engine.aggregate`, which applies + * the filter and runs it through the same refusal doors as `where` (shape, + * unknown field, and the security middleware's field-read check). ⛔ No filter + * is evaluated here: a second evaluator in this package would be a second + * answer to the same question. + * - **The entry is strict**, through {@link strictToolInput}, so an undeclared + * entry key is refused by name, as an undeclared top-level key already is. + * The aliases are semantic near-misses for `filter` and `alias`; they are + * named in the refusal and never parsed. + * + * `function`, `field` and `alias` are the engine's members too. Each keeps the + * description this tool advertised before; `.describe` copies the schema and + * leaves its parse unchanged. + */ +function aggregationEntrySchema() { + const node = AggregationNodeSchema.shape; + return strictToolInput( + { + surface: 'this aggregate_records aggregation entry', + aliases: { where: 'filter', filters: 'filter', conditions: 'filter', as: 'alias' }, + }, + { + function: node.function.describe('Aggregation function'), + field: node.field.describe('Field to aggregate (omit for count(*))'), + alias: node.alias.describe('Result column name'), + filter: node.filter.describe( + 'Optional per-aggregation filter, e.g. {"status":"active"}: only rows matching it feed THIS ' + + 'aggregation, while the other aggregations in the call still read every row. Same operators ' + + 'as where.', + ), + }, + ); +} + /** * The spellings every object-scoped tool shares, split out because they are the * same guess wherever an object name is taken. @@ -1179,17 +1233,12 @@ export function registerObjectTools( { objectName: z.string().describe('The object/table name'), aggregations: z - .array( - z.object({ - function: z - .enum(['count', 'sum', 'avg', 'min', 'max', 'count_distinct']) - .describe('Aggregation function'), - field: z.string().optional().describe('Field to aggregate (omit for count(*))'), - alias: z.string().describe('Result column name'), - }), - ) + .array(aggregationEntrySchema()) .min(1) - .describe('Metrics to compute, e.g. [{"function":"sum","field":"amount","alias":"total"}]'), + .describe( + 'Metrics to compute, e.g. [{"function":"sum","field":"amount","alias":"total"},' + + '{"function":"count","alias":"open_count","filter":{"status":"open"}}]', + ), groupBy: z .array( z.union([ diff --git a/packages/qa/dogfood/test/showcase-mcp-aggregate-filter.dogfood.test.ts b/packages/qa/dogfood/test/showcase-mcp-aggregate-filter.dogfood.test.ts new file mode 100644 index 00000000000..b8e28ad184c --- /dev/null +++ b/packages/qa/dogfood/test/showcase-mcp-aggregate-filter.dogfood.test.ts @@ -0,0 +1,200 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #28550 — MCP `aggregate_records` and the REST query door give ONE answer to a +// per-aggregation `filter`, through the real showcase + security + MCP stack. +// +// THE DEFECT, measured by QA on the stock showcase: the MCP tool's aggregation +// entry was a non-strict zod object, so an entry's `filter` was stripped before +// the call reached the engine. The same body answered +// +// MCP `rows: [{ n: 5, all: 5 }]` (the filter dropped) +// REST `records: [{ n: 2, all: 5 }]` (the filter applied) +// +// and a per-aggregation filter on a field the caller cannot read answered the +// unfiltered count through MCP, where REST refused it 403 PERMISSION_DENIED. +// +// WHAT IS PINNED. Both answers are read here, from the same principal, on the +// same rows, and compared — never a hand-computed constant on one side only: +// +// 1. the filtered and unfiltered counts agree between MCP and REST, and are +// DIFFERENT from each other (non-vacuous: a dropped filter would make them +// equal, which is exactly how the defect looked); +// 2. both equal the count the engine reads in system context; +// 3. a member whose permission set marks `budget` `readable: false` gets the +// security middleware's own refusal on BOTH doors (403 PERMISSION_DENIED), +// not an unfiltered count — the refusal comes from the engine path, since +// `packages/mcp` evaluates no filter; +// 4. an undeclared entry key is refused by MCP, naming the key. +// +// CONTROLS. The same member's UNFILTERED count is served by both doors and +// agrees, so the refusal in (3) is the predicate's and not the member's read of +// the object. +// +// Why a private boot rather than the worker-shared showcase: this file saves a +// permission set and loads the MCP plugin, which the shared harness excludes. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { type VerifyStack } from '@objectstack/verify'; +import { MCPServerPlugin } from '@objectstack/mcp'; +import { bootShowcase } from './showcase-boot.js'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; +import { catalogPermissionSet } from './catalog-permission-set.js'; + +const SYS = { isSystem: true } as const; +const OBJECT = 'showcase_project'; +const SCRATCH_SET = 'showcase_scratch_mcp_aggregate_budget_deny'; +const MEMBER = 'mcp-aggregate-filter@verify.test'; + +/** The reported body: a filtered count beside an unfiltered one. */ +const FILTERED_PAIR = [ + { function: 'count', alias: 'n', filter: { status: 'active' } }, + { function: 'count', alias: 'all' }, +]; + +function mcpBody(method: string, params?: unknown, id = 1) { + return { jsonrpc: '2.0', id, method, ...(params !== undefined ? { params } : {}) }; +} + +describe('showcase: MCP aggregate_records honours a per-aggregation filter as REST does (#28550)', () => { + let stack: VerifyStack; + let ql: any; + let adminTok: string; + let memberTok: string; + + /** POST /api/v1/mcp tools/call; returns `{ isError, payload }` with the text content parsed. */ + async function mcpAggregate(token: string, args: Record) { + const res = await stack.api('/mcp', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + Authorization: `Bearer ${token}`, + }, + body: JSON.stringify(mcpBody('tools/call', { name: 'aggregate_records', arguments: args })), + }); + expect(res.status, `MCP transport: ${await res.clone().text()}`).toBe(200); + const rpc: any = await res.json(); + expect(rpc.error, `JSON-RPC error: ${JSON.stringify(rpc.error)}`).toBeUndefined(); + const text: string = rpc.result?.content?.[0]?.text ?? ''; + let payload: any; + try { + payload = JSON.parse(text); + } catch { + payload = text; + } + return { isError: rpc.result?.isError === true, payload, text }; + } + + /** POST /api/v1/data/:object/query with the same aggregation body. */ + async function restAggregate(token: string, body: Record) { + const res = await stack.apiAs(token, 'POST', `/data/${OBJECT}/query`, body); + return { status: res.status, body: (await res.json()) as any }; + } + + async function systemCount(where: Record): Promise { + return Number(await ql.count(OBJECT, { where, context: SYS })); + } + + beforeAll(async () => { + stack = await bootShowcase({ + extraPlugins: [new MCPServerPlugin()], + security: showcaseAppDefaultSecurity(), + }); + adminTok = await stack.signIn(); + memberTok = await stack.signUp(MEMBER); + ql = await stack.kernel.getServiceAsync('objectql'); + + // A scratch set that denies READ on `budget` only: the app's everyone + // baseline already grants read on `showcase_project`, so the set adds + // nothing but the field deny. Object-qualified key (a bare `budget` key + // enforces nothing). + const authored = await stack.apiAs(adminTok, 'PUT', `/meta/permission/${SCRATCH_SET}`, { + name: SCRATCH_SET, + label: 'Scratch: FLS read deny on showcase_project.budget (#28550)', + objects: {}, + fields: { [`${OBJECT}.budget`]: { readable: false, editable: false } }, + }); + expect(authored.status, 'scratch set authored through the metadata door').toBeLessThan(300); + const ps = await catalogPermissionSet(ql, SCRATCH_SET); + expect(ps, 'scratch permission set served by the catalog').toBeTruthy(); + const memberId = (await ql.findOne('sys_user', { where: { email: MEMBER }, context: SYS }))?.id; + expect(memberId, 'member provisioned').toBeTruthy(); + await ql.insert('sys_user_permission_set', { user_id: memberId, permission_set: ps.name }, { context: SYS }); + }, 180_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('the reported body: MCP and REST give one answer, and the filter narrows it', async () => { + const active = await systemCount({ status: 'active' }); + const total = await systemCount({}); + // Non-vacuous fixture: the seed must hold rows the filter excludes. + expect(active, 'seeded active projects').toBeGreaterThan(0); + expect(active, 'seeded projects that are not active').toBeLessThan(total); + + const mcp = await mcpAggregate(adminTok, { objectName: OBJECT, aggregations: FILTERED_PAIR }); + expect(mcp.isError, mcp.text).toBe(false); + const rest = await restAggregate(adminTok, { aggregations: FILTERED_PAIR }); + expect(rest.status, JSON.stringify(rest.body)).toBe(200); + + const mcpRow = mcp.payload.rows?.[0]; + const restRow = (rest.body.records ?? rest.body.data)?.[0]; + expect({ n: Number(mcpRow?.n), all: Number(mcpRow?.all) }).toEqual({ n: active, all: total }); + expect({ n: Number(restRow?.n), all: Number(restRow?.all) }).toEqual({ n: active, all: total }); + }); + + it('grouped: each group\'s filtered count agrees between MCP and REST', async () => { + const body = { + groupBy: ['health'], + aggregations: [ + { function: 'count', alias: 'active_n', filter: { status: 'active' } }, + { function: 'count', alias: 'all' }, + ], + }; + const mcp = await mcpAggregate(adminTok, { objectName: OBJECT, ...body }); + expect(mcp.isError, mcp.text).toBe(false); + const rest = await restAggregate(adminTok, body); + expect(rest.status, JSON.stringify(rest.body)).toBe(200); + const norm = (rows: any[]) => + [...rows] + .map((r) => ({ health: r.health ?? null, active_n: Number(r.active_n), all: Number(r.all) })) + .sort((a, b) => String(a.health).localeCompare(String(b.health))); + expect(norm(mcp.payload.rows ?? [])).toEqual(norm(rest.body.records ?? rest.body.data ?? [])); + }); + + it('an undeclared entry key is refused by MCP, naming the key', async () => { + const mcp = await mcpAggregate(adminTok, { + objectName: OBJECT, + aggregations: [{ function: 'count', alias: 'n', bogusKey: 1 }], + }); + expect(mcp.isError).toBe(true); + expect(mcp.text).toContain('bogusKey'); + }); + + it('a filter on a field the caller cannot read is refused 403 PERMISSION_DENIED on BOTH doors', async () => { + const aggregations = [ + { function: 'count', alias: 'big', filter: { budget: { $gt: 0 } } }, + { function: 'count', alias: 'all' }, + ]; + const rest = await restAggregate(memberTok, { aggregations }); + expect(rest.status, JSON.stringify(rest.body)).toBe(403); + expect(rest.body?.error?.code ?? rest.body?.code).toBe('PERMISSION_DENIED'); + + const mcp = await mcpAggregate(memberTok, { objectName: OBJECT, aggregations }); + expect(mcp.isError, `MCP served an answer: ${mcp.text}`).toBe(true); + expect(mcp.payload?.error?.code).toBe('PERMISSION_DENIED'); + expect(mcp.payload?.error?.status).toBe(403); + }); + + it('CONTROL — the same member\'s unfiltered count is served by both doors and agrees', async () => { + const aggregations = [{ function: 'count', alias: 'all' }]; + const mcp = await mcpAggregate(memberTok, { objectName: OBJECT, aggregations }); + expect(mcp.isError, mcp.text).toBe(false); + const rest = await restAggregate(memberTok, { aggregations }); + expect(rest.status, JSON.stringify(rest.body)).toBe(200); + const restAll = Number((rest.body.records ?? rest.body.data)?.[0]?.all); + expect(Number(mcp.payload.rows?.[0]?.all)).toBe(restAll); + expect(restAll).toBeGreaterThan(0); + }); +});