k1nq/TREK
1
0
Fork 0
mirror of https://github.com/mauriceboe/TREK.git synced 2026-10-18 21:16:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): refuse an argument a tool never declared instead of dropping it

Browse Source 
The SDK wraps a raw shape in a plain z.object, which strips an undeclared
key and runs the handler as if nothing was wrong: an assistant that wrote
stopType for stop_type was told the place was updated while nothing had
changed. The registry registers every shape as z.strictObject, so the call
answers "Unrecognized key" and tools/list advertises additionalProperties
false. The nested objects a tool takes, authored inline or spread from a
shared schema (legs, stops, links, transit places), are strict at this
boundary for the same reason; the shared schemas themselves stay open for
REST. A whole Zod schema, the way a plugin's manifest-built tool arrives,
passes through as its author wrote it.
This commit is contained in:
Maurice
2026-09-22 09:55:45 +02:00
parent 34f3aeee5f
commit e510ff25ab
18 changed files with 113 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/tests/unit/mcp/tools-places.test.ts
+17
View File
@@ -247,6 +247,23 @@ describe('Tool: update_place', () => {
});
});
it('refuses a misspelt field instead of silently updating nothing', async () => {
// An assistant wrote stopType for stop_type; the call used to come back as a
// success with the place untouched, and the road trip never got its fuel stop.
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
const place = createPlace(testDb, trip.id, { name: 'Aral Bad Segeberg' });
await withHarness(user.id, async (h) => {
const refused = await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: place.id, stopType: 'fuel' } });
expect(refused.isError).toBe(true);
expect((refused.content as Array<{ text: string }>)[0].text).toMatch(/Unrecognized key.*stopType/);
expect((testDb.prepare('SELECT stop_type FROM places WHERE id = ?').get(place.id) as { stop_type: string | null }).stop_type).toBeNull();
expect(broadcastMock).not.toHaveBeenCalled();
const fixed = parseToolResult(await h.client.callTool({ name: 'update_place', arguments: { tripId: trip.id, placeId: place.id, stop_type: 'fuel' } })) as any;
expect(fixed.place.stop_type).toBe('fuel');
});
});
it('sets image_url on an existing place (#37)', async () => {
const { user } = createUser(testDb);
const trip = createTrip(testDb, user.id);
server/tests/unit/nest-mcp/attach.test.ts
+36 -3
View File
@@ -43,6 +43,13 @@ class FixtureMcp {
return { content: [{ type: 'text', text: `wrote as ${ctx.userId}` }] };
}
// A whole schema, the way a plugin's manifest-built tool arrives: its author
// decided how open it is, and the registry must not tighten it.
@Tool({ name: 'loose_tool', inputSchema: z.looseObject({ name: z.string() }) })
async looseTool(args: Record<string, unknown>, ctx: TestCtx) {
return { content: [{ type: 'text', text: JSON.stringify({ args, userId: ctx.userId }) }] };
}
@Tool({ name: 'predicate_tool', access: (ctx: McpContext) => (ctx as TestCtx).allow === true })
async predicateTool(args: Record<string, never>, ctx: TestCtx) {
return { content: [{ type: 'text', text: JSON.stringify({ args, userId: ctx.userId }) }] };
@@ -150,7 +157,7 @@ describe('McpRegistry.attach', () => {
it('registers everything for a ctx passing all access checks', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 7, canRead: true, canWrite: true, allow: true });
const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort();
expect(tools).toEqual(['open_tool', 'predicate_tool', 'read_tool', 'write_tool']);
expect(tools).toEqual(['loose_tool', 'open_tool', 'predicate_tool', 'read_tool', 'write_tool']);
const resources = (await harness.client.listResources()).resources.map((r) => r.name);
expect(resources).toEqual(['fixture_doc']);
const templates = (await harness.client.listResourceTemplates()).resourceTemplates.map((t) => t.name);
@@ -162,13 +169,13 @@ describe('McpRegistry.attach', () => {
it('filters declarative access through the policy per mode', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 7, canRead: true, canWrite: false, allow: false });
const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort();
expect(tools).toEqual(['open_tool', 'read_tool']);
expect(tools).toEqual(['loose_tool', 'open_tool', 'read_tool']);
});
it('filters everything gated when the ctx grants nothing, keeping ungated entries', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 7 });
const tools = (await harness.client.listTools()).tools.map((t) => t.name).sort();
expect(tools).toEqual(['open_tool']);
expect(tools).toEqual(['loose_tool', 'open_tool']);
expect((await harness.client.listResources()).resources).toEqual([]);
const templates = (await harness.client.listResourceTemplates()).resourceTemplates.map((t) => t.name);
expect(templates).toEqual(['fixture_item']);
@@ -188,6 +195,32 @@ describe('McpRegistry.attach', () => {
expect(JSON.parse(textOf(result))).toEqual({ greeting: 'hello trek', userId: 42 });
});
it('refuses an argument the tool never declared instead of stripping it', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 42, canRead: true, canWrite: true, allow: true });
// A shape is registered strict: the SDK would otherwise drop `nmae` and run
// the handler as if the caller had sent nothing wrong.
const refused = await harness.client.callTool({ name: 'read_tool', arguments: { name: 'trek', nmae: 'typo' } });
expect(refused.isError).toBe(true);
expect(textOf(refused)).toMatch(/Unrecognized key.*nmae/);
// An empty shape is strict too: nothing declared means nothing accepted.
const stray = await harness.client.callTool({ name: 'write_tool', arguments: { anything: 1 } });
expect(stray.isError).toBe(true);
expect(textOf(stray)).toMatch(/Unrecognized key.*anything/);
expect(textOf(await harness.client.callTool({ name: 'write_tool', arguments: {} }))).toBe('wrote as 42');
// tools/list says so up front, so a client that reads the schema never sends it.
const tools = (await harness.client.listTools()).tools;
expect(tools.find((t) => t.name === 'read_tool')?.inputSchema).toMatchObject({ additionalProperties: false });
expect(tools.find((t) => t.name === 'write_tool')?.inputSchema).toMatchObject({ additionalProperties: false });
});
it('hands a whole schema to the SDK as written, however open its author left it', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 42 });
const result = await harness.client.callTool({ name: 'loose_tool', arguments: { name: 'trek', extra: 'kept' } });
expect(JSON.parse(textOf(result))).toEqual({ args: { name: 'trek', extra: 'kept' }, userId: 42 });
const loose = (await harness.client.listTools()).tools.find((t) => t.name === 'loose_tool');
expect(loose?.inputSchema).not.toHaveProperty('additionalProperties', false);
});
it('normalizes args to {} for tools declared without an inputSchema', async () => {
harness = await createAttachHarness(buildRegistry(), { userId: 42 });
const result = await harness.client.callTool({ name: 'open_tool', arguments: {} });