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.
2026-10-18 21:16:47 +00:00 · 2026-08-28 13:36:42 -03:00
parent 21f4a1241b
commit 41de91668b
3 changed files with 299 additions and 0 deletions
@@ -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<string, unknown> {
+  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<string, unknown> = {};
+  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<string, unknown> = {};
+      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<string, unknown> | 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<string, unknown>;
+  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<string, unknown> | 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<string, unknown> | undefined): McpZodSchema {
+  const required = requiredKeys(normalised);
+  const base = z.looseObject({}).check((ctx) => {
+    for (const key of required) {
+      if (!(key in (ctx.value as Record<string, unknown>))) {
+        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<string>): 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 };
+}
@@ -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);
+}