From 41de91668b9927a378c46d5a6587aa03b1adc5a7 Mon Sep 17 00:00:00 2001 From: Renzo Beux Date: Fri, 28 Aug 2026 13:36:42 -0300 Subject: [PATCH] feat(plugins): normalise and sanitise plugin-authored assistant text The security control for plugin MCP tools, landed on its own so it is reviewable as one diff. Pure and Nest-free: no container, no DB, no running plugin needed to test it. The threat is not a plugin crashing. A tool name, title and description are a persistent, admin-invisible write into every user's assistant context, and .meta() puts the JSON Schema there verbatim. TREK's own code proves the channel is obeyed: the static-token deprecation notice is delivered in a tool result precisely because that is what an assistant reliably surfaces. sanitiseAssistantText is a sibling of stripEmoji, not a change to it. stripEmoji preserves newlines on purpose (PDF paragraphs) and ~15 render call sites depend on that. The new one collapses every whitespace run to a single space, so a description cannot forge "## System" as a heading that reads to the model like host text, and strips more than the brief asked for: the bidi overrides and isolates and U+2028/9 are neither C0 controls nor \n, so they defeat both halves of the stated rule. The schema is advertised rather than converted. z.looseObject({}).meta() passes the plugin's own schema through byte for byte, keeping pattern, oneOf and format, which a hand-rolled converter flattens to unknown. The .check() keeps required enforced, so the handler cannot receive a call missing arguments its own schema declared mandatory. That only works with a normaliser in front, since .meta() is a passthrough. It rejects a root type that is not object (which would advertise a non-object inputSchema while the runtime still passes an object), rejects $ref/$defs/definitions at any depth, and caps bytes, properties and nesting. It strips a top-level id, which vanishes from the advertisement and lands in z.globalRegistry._idmap, a strong Map unlike the WeakMap holding the rest. Sanitising recurses into every nested description, title and string enum member. That is the part a top-level cap misses, and where a property named trek_session_token with a plausible description would live. Annotations are clamped asymmetrically. readOnlyHint gates client auto-approval, so a plugin may lower its danger level and never raise it; openWorldHint goes the other way, since egress makes a tool open-world whether it admits it or not. db:own is deliberately not a side effect: counting the plugin's private SQLite would make nearly every plugin non-read-only and kill auto-approval for legitimate read tools. MCPSCHEMA-001 to MCPSCHEMA-030. --- server/src/nest/plugins/mcp-tool-schema.ts | 267 ++++++++++++++++++ server/src/nest/plugins/text-sanitize.ts | 32 +++ .../unit/plugins/mcp-tool-schema.test.ts | Bin 0 -> 12683 bytes 3 files changed, 299 insertions(+) create mode 100644 server/src/nest/plugins/mcp-tool-schema.ts create mode 100644 server/tests/unit/plugins/mcp-tool-schema.test.ts diff --git a/server/src/nest/plugins/mcp-tool-schema.ts b/server/src/nest/plugins/mcp-tool-schema.ts new file mode 100644 index 000000000..caa67324b --- /dev/null +++ b/server/src/nest/plugins/mcp-tool-schema.ts @@ -0,0 +1,267 @@ +/** + * Everything a plugin-authored MCP tool has to survive before it reaches an + * assistant: the advertised name, the bounded text, the JSON Schema, and the + * annotations a client gates auto-approval on. + * + * Pure and Nest-free on purpose. This is the security control for the whole + * feature, so it is testable without a container, a DB or a running plugin. + * + * The threat is not a plugin crashing. It is a plugin writing instructions into + * every user's assistant context, invisibly to the admin who granted it, and + * being obeyed. TREK's own code is the proof that the channel works: the + * static-token deprecation notice is delivered in a tool RESULT precisely + * because that is what an assistant reliably surfaces. + */ +import { sanitiseAssistantText } from './text-sanitize'; + +import { z } from 'zod'; + +import type { McpZodSchema } from '../../nest-mcp'; + +/** Tools one plugin may advertise. A long list crowds out the built-ins. */ +export const MCP_TOOLS_MAX = 8; + +/** + * Tools all plugins together may advertise. + * + * Not in the spec, added here: at 8 each, twenty installed plugins would put 160 + * tools in front of a model that already chooses from ~200, and tool selection + * degrades long before that. Truncation is logged, never silent. + */ +export const MCP_TOOLS_TOTAL_MAX = 32; + +/** Plugin-local tool name. No dot or dash, so the advertised name parses apart. */ +export const TOOL_NAME_RE = /^[a-z0-9_]{1,48}$/; + +export const TOOL_TITLE_MAX = 80; +export const TOOL_DESCRIPTION_MAX = 1024; + +/** Nested strings get their own, tighter caps; there can be many of them. */ +export const SCHEMA_TITLE_MAX = 80; +export const SCHEMA_DESCRIPTION_MAX = 512; +export const SCHEMA_ENUM_VALUE_MAX = 64; + +export const SCHEMA_BYTES_MAX = 8 * 1024; +export const SCHEMA_PROPERTIES_MAX = 64; +export const SCHEMA_ENUM_MAX = 32; +export const SCHEMA_DEPTH_MAX = 5; + +/** Raised for a declaration the host will not advertise. */ +export class McpToolSchemaError extends Error {} + +/** + * The name a plugin tool is advertised under. + * + * A plugin id matches /^[a-z][a-z0-9-]{2,39}$/, so it can never contain `_`, + * and a local name can never contain `-`. The prefix therefore parses apart + * unambiguously at the first `_` after `plugin_`. The prefix also makes an + * Object.prototype key like `constructor` impossible to advertise. + * + * Worst case is 7 + 40 + 1 + 48 = 96 characters, inside the SDK's 128 limit. + */ +export function mcpToolName(pluginId: string, name: string): string { + return `plugin_${pluginId}_${name}`; +} + +function isPlainObject(v: unknown): v is Record { + return typeof v === 'object' && v !== null && !Array.isArray(v); +} + +/** Keys that must not survive into the advertisement, each for its own reason. */ +const STRIPPED_ROOT_KEYS = [ + // Consumed by Zod's registry rather than advertised, and pinned into + // z.globalRegistry._idmap — a strong Map, unlike the WeakMap holding the rest. + 'id', + // Re-emitted by the SDK itself; a plugin-supplied one only ever disagrees. + '$id', + '$schema', +]; + +/** Rejected at any depth: the host cannot resolve them and clients disagree on them. */ +const FORBIDDEN_KEYS = ['$ref', '$defs', 'definitions']; + +interface Counters { + properties: number; +} + +function sanitiseNode(node: unknown, depth: number, counters: Counters): unknown { + if (Array.isArray(node)) return node.map((item) => sanitiseNode(item, depth, counters)); + if (!isPlainObject(node)) return node; + if (depth > SCHEMA_DEPTH_MAX) { + throw new McpToolSchemaError(`schema nests deeper than ${SCHEMA_DEPTH_MAX} levels`); + } + const out: Record = {}; + for (const [key, value] of Object.entries(node)) { + if (FORBIDDEN_KEYS.includes(key)) { + throw new McpToolSchemaError(`schema uses "${key}", which is not supported`); + } + if (key === 'description') { + out[key] = sanitiseAssistantText(value, SCHEMA_DESCRIPTION_MAX); + continue; + } + if (key === 'title') { + out[key] = sanitiseAssistantText(value, SCHEMA_TITLE_MAX); + continue; + } + if (key === 'const' && typeof value === 'string') { + out[key] = sanitiseAssistantText(value, SCHEMA_ENUM_VALUE_MAX); + continue; + } + if (key === 'enum' && Array.isArray(value)) { + if (value.length > SCHEMA_ENUM_MAX) { + throw new McpToolSchemaError(`an enum lists more than ${SCHEMA_ENUM_MAX} values`); + } + // Only the string members: a number or boolean carries no instructions. + out[key] = value.map((m) => (typeof m === 'string' ? sanitiseAssistantText(m, SCHEMA_ENUM_VALUE_MAX) : m)); + continue; + } + if (key === 'properties' && isPlainObject(value)) { + const props: Record = {}; + for (const [propName, propSchema] of Object.entries(value)) { + counters.properties += 1; + if (counters.properties > SCHEMA_PROPERTIES_MAX) { + throw new McpToolSchemaError(`schema declares more than ${SCHEMA_PROPERTIES_MAX} properties`); + } + props[propName] = sanitiseNode(propSchema, depth + 1, counters); + } + out[key] = props; + continue; + } + out[key] = sanitiseNode(value, depth + 1, counters); + } + return out; +} + +/** + * Validate and clean a plugin's JSON Schema so it can be advertised verbatim. + * + * `.meta()` is a passthrough straight into the assistant's context, which is + * what makes this function the control rather than a convenience. Returns + * undefined for an absent schema, meaning a tool that takes no arguments. + */ +export function normaliseToolSchema(raw: unknown): Record | undefined { + if (raw === undefined || raw === null) return undefined; + if (!isPlainObject(raw)) throw new McpToolSchemaError('schema must be an object'); + + // A root `type` of anything but object advertises a non-object inputSchema + // while the runtime still hands the handler an object. The advertisement + // would simply be a lie, and the model would act on it. + if (raw.type !== undefined && raw.type !== 'object') { + throw new McpToolSchemaError(`schema root type must be "object", got ${JSON.stringify(raw.type)}`); + } + + const bytes = Buffer.byteLength(JSON.stringify(raw) ?? '', 'utf8'); + if (bytes > SCHEMA_BYTES_MAX) { + throw new McpToolSchemaError(`schema is ${bytes} bytes, over the ${SCHEMA_BYTES_MAX} limit`); + } + + const cleaned = sanitiseNode(raw, 0, { properties: 0 }) as Record; + for (const key of STRIPPED_ROOT_KEYS) delete cleaned[key]; + cleaned.type = 'object'; + return cleaned; +} + +/** The `required` names a normalised schema declares. */ +function requiredKeys(schema: Record | undefined): string[] { + const req = schema?.required; + return Array.isArray(req) ? req.filter((k): k is string => typeof k === 'string') : []; +} + +/** + * The Zod schema handed to registerTool. + * + * A loose object rather than a converted one: Zod merges registry metadata into + * the toJSONSchema output the SDK renders tools/list from, so `.meta()` + * advertises the plugin's own schema byte for byte, including the `pattern`, + * `oneOf` and `format` keywords any hand-rolled converter would flatten away. + * + * The `.check()` is what keeps `required` real. Without it the loose object + * accepts anything and the handler receives a call missing the arguments its + * own schema said were mandatory. Extra keys still pass through, which is what + * "loose" buys: a plugin may accept more than it advertises. + */ +export function buildToolInputSchema(normalised: Record | undefined): McpZodSchema { + const required = requiredKeys(normalised); + const base = z.looseObject({}).check((ctx) => { + for (const key of required) { + if (!(key in (ctx.value as Record))) { + ctx.issues.push({ + code: 'custom', + input: ctx.value, + path: [key], + message: `Required property "${key}" is missing`, + }); + } + } + }); + return (normalised ? base.meta(normalised) : base) as unknown as McpZodSchema; +} + +/** The four hints the MCP spec defines. Anything else a plugin sends is dropped. */ +export interface McpToolAnnotations { + readOnlyHint: boolean; + destructiveHint: boolean; + idempotentHint: boolean; + openWorldHint: boolean; +} + +/** Grants that mean the plugin can change something outside its own sandbox. */ +const SIDE_EFFECT_EXACT = new Set([ + 'db:meta', + 'notify:send', + 'ai:invoke', + 'oauth:client', + 'ws:broadcast:trip', + 'ws:broadcast:user', + 'http:outbound', +]); +const SIDE_EFFECT_PREFIXES = ['db:write:', 'http:outbound:']; + +/** + * Reconcile a plugin's declared annotations with what its grants actually allow. + * + * The asymmetry is the point. `readOnlyHint` is what clients gate auto-approval + * on, so a plugin may lower its own danger level but never raise it: a plugin + * holding db:write:trips does not get to call itself read-only and be run + * without a prompt. `openWorldHint` goes the other way, since a plugin that can + * reach the internet is open-world whether it says so or not. + * + * `db:own` is deliberately NOT a side effect. It is the plugin's private SQLite, + * invisible to TREK; counting it would make nearly every plugin non-read-only + * and kill auto-approval for legitimate read tools. + */ +export function clampToolAnnotations(declared: unknown, grants: ReadonlySet): McpToolAnnotations { + const d = isPlainObject(declared) ? declared : {}; + const writes = [...grants].some( + (g) => SIDE_EFFECT_EXACT.has(g) || SIDE_EFFECT_PREFIXES.some((p) => g.startsWith(p)), + ); + const egress = [...grants].some((g) => g === 'http:outbound' || g.startsWith('http:outbound:')); + + const readOnlyHint = d.readOnlyHint === true && !writes; + return { + readOnlyHint, + // A read-only tool is not destructive; otherwise believe the plugin, and + // default to destructive when it says nothing. + destructiveHint: readOnlyHint ? false : d.destructiveHint !== false, + idempotentHint: readOnlyHint ? true : d.idempotentHint === true, + openWorldHint: d.openWorldHint === true || egress, + }; +} + +/** A tool declaration after the host has bounded every string a model will read. */ +export interface SanitisedToolText { + title?: string; + description: string; +} + +/** + * Bound the two free-text fields. Description is required because it is the only + * thing the model reads to decide whether to call the tool; a tool with none is + * invisible but still callable, which is worse than absent. + */ +export function sanitiseToolText(title: unknown, description: unknown): SanitisedToolText { + const cleanTitle = sanitiseAssistantText(title, TOOL_TITLE_MAX); + const cleanDescription = sanitiseAssistantText(description, TOOL_DESCRIPTION_MAX); + if (!cleanDescription) throw new McpToolSchemaError('description is required'); + return { ...(cleanTitle ? { title: cleanTitle } : {}), description: cleanDescription }; +} diff --git a/server/src/nest/plugins/text-sanitize.ts b/server/src/nest/plugins/text-sanitize.ts index b0eaabb64..fffe59f5a 100644 --- a/server/src/nest/plugins/text-sanitize.ts +++ b/server/src/nest/plugins/text-sanitize.ts @@ -41,3 +41,35 @@ export function hasEmoji(s: string): boolean { EMOJI_RE.lastIndex = 0; return EMOJI_RE.test(s); } + +// C0 and C1 controls, the Unicode line/paragraph separators, and the bidi +// overrides and isolates. Each group defeats a different half of the rule below: +// U+2028/9 are line breaks to a renderer but not to /\n/, and the bidi controls +// make a string render as one thing while it tokenises as another. \t is in the +// C0 range and goes with the rest — a tab is not meaningful in a tool description +// and is one more way to fake structure. +// eslint-disable-next-line no-control-regex +const UNSAFE_CONTROL_RE = /[\u0000-\u001F\u007F-\u009F\u2028\u2029\u202A-\u202E\u2066-\u2069]/g; + +/** + * The assistant-context flavour of the render-boundary sanitiser above. + * + * A plugin's tool name, title and description are a direct, persistent and + * admin-invisible write into every user's assistant context. That is a stronger + * surface than a badge or a PDF heading, which a person reads and discounts, so + * this goes further than stripEmoji: + * + * - every control character goes, not just the emoji glue; + * - every whitespace run collapses to ONE SPACE, newlines included, so a + * description cannot forge "\n\n## System\nIgnore previous instructions" — + * a markdown heading that reads to the model like host text; + * - then emoji, then a hard cap. + * + * Deliberately a sibling rather than a change to stripEmoji: that one preserves + * newlines on purpose (PDF paragraphs) and ~15 render call sites depend on it. + */ +export function sanitiseAssistantText(value: unknown, max: number): string { + const raw = typeof value === 'string' ? value : value == null ? '' : String(value); + const flattened = raw.replace(UNSAFE_CONTROL_RE, ' ').replace(/\s+/g, ' ').trim(); + return stripEmoji(flattened).trim().slice(0, max); +} diff --git a/server/tests/unit/plugins/mcp-tool-schema.test.ts b/server/tests/unit/plugins/mcp-tool-schema.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..d9e84af8f371c8471040ae85538a08a7c54c55fe GIT binary patch literal 12683 zcmbVT>vG%174C1pM0XO;0CY&|V%K%(x*5e*8@0BjN^;Yw?U9zil0*aoEH090jXHgZ zK4G7v-?tYK072T$Bok5~_UxW>zH{H~JbLs%JQ62yIgJwW=H-bf(li!@x-EoELXihE zHJ4(B?{Sn&#sB{EU%qwr@~6W$2ZP<+J-lCku>WKTdL(&1i*kW~iCvvX(qKFE-jEzYxRC$m^iy}%BAu~J{3zg+jUZ|w# z3mML%WDq6SQ67!4=mwi95hXaAx>ngzl)1{hoP=b~RWd0=kS1gktHI~xaIOmJ_b$bv zEX<+*a(4VyoEiSX1U3b!u`Hr_U*st{AiYfeq5`ZS5we<*)ttwi%c4-3n5S4-K!Ez^ znOrQEVj+te<+Sr4nlI9<5O)s*rFe0EeEj^Hhw(P2e(Tt;z7xwL|*s@A+d zdV6?rc6f4ddT@Sxx^>YrEV6v&@G-G6$iN!9ngW0T6w)z$mq+_uWGNhK5h?J`H{cK8 zukqczYtZNQ>z)`SUec@6UjtF3BSa{dq~X?qeKm#i?F+c*XkN}oVh`Wtt^NMQtka7# z^{+C@R5%jvJ#O!PBuN>Da4l&f2-B%-jfN?7;c*U0+b4Wbs%J%GgUwufM*0ttuF>%j(pRK913UDV#YSiSTd zr@5zY)(^FhL&+@1kt99~*twjKqiLCzdAt<#HV7|qnoQxIM69MV#($G(G0R)0-q~^S zJPLCWNcwXf?Ic3Hci{>D=+(vPAshf=R-6VPL=CRLI?&J%0t8LKL;TLLIcdlVgBy=z zB*{=O;~OYnKOCoJ63T3eC`T-5BacGB&y$|`PI%)%;0-gifH3{N$Gzv=_p3t^~_K@uE8s@Xh{xvA0X@*Y$^0y&>=pTYQQ94v6=?4~f_Dk}=lC z`9*bj@%3k~^o8TJHSoj)l?6-;hsu!R66i932?6aUW-ufca@;m%i_1=}`=0#G_(Ku= zmVRq*7tzJKASuQo5TRjvgS+`%btcL`SMWKqNHe3EaTG@GtTyt8iupQ#VG$&{$DnQEK4^6X{twA><0H69ii9bP0B^*EE8@-s8m&{1`-*&{?rOGy=aMmBJiKd0 z)BA^ajgWj_+$y-)ax%~4?9zz0fwcAX!6*2v3Uqkd28pMY_!aY6gcFscmeF~)n8wS+ z%xD7jLX4=a1s2G~y1*$fn-dojrVD%28S=$Rono5AOH;NmE3R+mB)y1&BAsT^ZJaOX z<225pK&ZtJdd%ji9hr%|%&wzrg%l9gQnpBQWn{k}*Jlc&#id-po4 zwi0)Mc57KKw2XO^WsCzL7UFvBRN~2Tu+<`dKQeVLy=0mU3=rYnY}Q)Lvh-%%BW`7y zQEUQti(s?2GS4sKAVDPh`c+T%8*85IZ{_&e2q}cTW;9r#NFv8T7F`7jW(hKm*OB+{ zz272~(SNhnA!)MxIeL|OYJhB|xj{60BazM{T^)lht$l=1)CYhA zDo2Ii3Zhv5^2GL&LhsL+|PuKq1zAQG>b@+8zCyO1q7N zwP3!7@N7gDXT{IksJ6&md#+7>af=#wnlZ2en)0*`#>Z&0n zZf5KhLXgfsWMzWR8xoZwf~#Ra(pVh7#q3BV_823sTJ13h#&m6hK-LUX1{@vo@T=n0 zo*UAn#Tb0-tYn~_(^!db3#$ig3@=Io1y2ni;(>)4=cTl{VEU=nYrLFjg= zZz`xO1prM@9g`B_p=t3;jgVaCLdA2-#574XleJ>gW{u&7-YRt z^9%=$iXa4H)q+(3(mniQTMo+>yM|N!s%cix-K{q)__Wcq5VF>FsJLmqGUh9k9Sr8% z9WA__f1q<={xSUvbQDP2TIFWR@q~3)d?4Me2tdYNq$%>;WGTi=#L9k+1Kll)5$NJ} z1z-d+0YD&+wj^p4$Xi_o7QF3_&21G=zTZWJF|29(seqzd(ho zJ9(GaTAwz=8@QQnd0oRBOn;81YRy4Z#=*mmggu#d1YamEvvVCAB(+sgC@>lU&yZ+| z>a&i=OVFzQnk=X*#5FUYAJscosQAqcs%tGP6V3P9vW`@`gFsdJva)c=UeoKfLkS9% z40SRq6*lf3qLs+S4~+!P2rp!+#B^ke)KDh@7=7S%INXl1`2fCnG0+ZITt?v~Ab|PB z3?%f}YCVVGelh;TK9=@($a6mpr*S%#@u`|(283?sMHFJUKGTN04y}~n{M>%q{GzNLhdZGlaQ=Ke@vXCZ4Hh9H65xQ8S(3#Fh?1211A!;Se9nKUMv0xa&0U@f%Z7kSayc% zh)pmYS<*pkLSK~1**EcSrmikwG_(=e7LSBYcn>Oal>~1fupkR(yq1RH%Yo_{T(e?y|cYZV!B+I>{_c_^=PYf zU%yV3@fA~OQ*~Bz)lB5$7E|6ww*-DPKqRsCvT=x}Y`&QROQ?A<42LSF=%OYq)3qY& zcCii3Q5ihv8(9RiV?E1V^TO7hb_C`wch)#cD+ceQ)lpq7=5Spj65j6++33<-PoQ_QCk+*Dxxl@-4t8_+g7;AS zqCcYRWEX~Xelqs7He z+)9fpnAJs4f1wdrxY!NJS4-WnylD)4w`+9Hb+Ek^_`4(0KKeZ>u|RfvnncUAU{Lnt zsFI6JQfdT5mX_05r}iyEUWiWhbwRx@j-GaHwdAI{)Y3>#7}wZp!_ZKxXvjGa8os%L zRhU}|kK5UWT{Fzf00x_{$)<7)?nMl%((H#Jwn8QIe)l7;jpezVD!bAo`F4YN~o&JnM@pmT+|iBHu)z!^d{rscW!pAb@cQFIXW=SzzN7x>?Jgj`7P+xUoaD z-mt+NplQmZO(n7-S;FaI9+{;mm(j@9^F-X7n@hH}aYuz?8s|5$S)AS=X$sNn(i!3` zMQ&6BzVYo{B7(ivPvgMeM`cGr`Cd34=_{Tij(l>|!W~|6#m#1`rjJDaXKp22E3$51 zu5hsd@aW|sx;tsQ_;Q7giET@3DY7wSVBeB=J-Yo`32^s~0w5dBykK3Tx-(ek9f^y+ zRfPf&L!Oj(Ybg>Ze=X$=@<0y=O2K_aj0&0ERK>WR>T4gsUVkOCmPA~|;w}O&vgn$i zJNw&feV3PIx4Yg8ET5kq{`EPcFQ)?V`9PtGzgL(JI0!cJ%2pczxFydEWlNgdJRNq$bVIMn8dVd@OVgwtzFYG*T1dt_k! z-v&M!w5`tBX>-;!D~iPkrlId}jpQO4s$1L*jumb+&dFedK%2wL`*!bM+XsN5s45J1 z(*10ng4PgAQ+4XG}c>)+b*P*TW~f|NeNakh|_fgFM1kmX%EK!31TDe!Qc`+u1%I~zBC9`@NZn_H`;68B{Vk) H-SPep<@5?3 literal 0 HcmV?d00001