3 Commits

Author	SHA1	Message	Date
Maurice	a8a05ace40	fix(plugins): close the gaps between what a tool schema advertises and what it enforces ... `.meta()` advertises every node of the declaration; `nodeToZod` only walks the ones it can type. Everything below follows from that gap, so the keyword allowlist moves into `sanitiseNode`, which is the one walker that sees every node. A `pattern` was compiled straight into a RegExp and run on the main thread against caller input, with no timeout and nothing to abort it: `^(a+)+$` against 33 characters stops the whole server, not just MCP. That is an escape from the supervised child into the host event loop, and it does not take malice, since a hand-written email pattern is the classic accident. Quantified groups that repeat or alternate inside are refused, and the source is capped. `mcp:tools` counted as a side effect, so `readOnlyHint` was structurally unreachable: holding that grant is what lets a plugin advertise a tool at all. Every plugin tool was therefore advertised as potentially destructive and every client's auto-approval was off. It is a host entry point like `hook:*`, and is classified as one. Property names, `default` and `examples` were the strings that never met the sanitiser. A key may carry newlines, so `\n## SYSTEM\n...` in one defeated the newline collapse everything else goes through. Names are identifiers now; sample values keep their own keys but have their strings bounded. Also refused at parse time, which is what the rest of the file already does: an object `enum` member carrying `$ref` past the deep scan, a non-boolean `additionalProperties` advertised and checked by nothing, `enum` beside `properties` skipping the children, a root `enum`/`const`/`nullable` producing a non-object schema the SDK cannot read, and a draft-4 `exclusiveMinimum: true` dropped from the check while staying in the advertisement. `required` used `in`, so a field named `toString` read as present on every call. Nodes are built with a null prototype, and the three names Zod cannot round-trip are refused rather than silently lost. A result block that had to be shortened got no marker and no log line, which reads to the model like the whole answer. It is counted now, like a dropped one, and `isError` survives every envelope path. The security test file carried two raw NUL bytes, so git, GitHub and ripgrep all treated it as binary: no diff, no blame, no review. Escaped, same runtime value.	2026-08-29 12:22:59 +02:00
Renzo Beux	937e32924d	fix(plugins): close six gaps in the MCP tool boundary ... Found in review of the branch. Each one is a case where the host told the model or the admin something it did not enforce. Advertised schemas are now enforced. buildToolInputSchema built a loose object that checked only which keys were present, so a tool advertising an integer, an enum and additionalProperties: false accepted a string, an unlisted value and an extra property. .meta() still advertises the declaration verbatim, and the schema underneath it is now built from that same declaration, so the two cannot disagree. Anything the builder cannot enforce (oneOf, unknown formats, non-primitive enum members) is refused by parseManifest rather than advertised, and the failure lands at install time where an admin sees it. Writing the test for that turned up a live bug: z.custom() cannot be represented in JSON Schema, so building enums that way would have thrown inside tools/list for the whole session. Enums and consts are real literal types now. readOnlyHint classification fails closed. The side-effect list spelled out db:write: and missed db:create:trips entirely, letting a plugin that can create trips advertise a tool as read-only and idempotent, which is what clients gate auto-approval on. It is an allowlist of read-only grants now, and mcp-tool-annotations.test.ts walks the whole catalog so a new permission fails a test rather than producing a wrong hint. The result cap covers the whole result. It bounded each text block, so a hundred blocks cost a hundred times the limit: 6.5M characters in a probe, now 65K. Large objects are serialised with a budget instead of stringified whole and then sliced, and the block count is capped. The admin tool list was dead code. The registry preview carries only widget and tripPage, so the section added for it never rendered, and the UI hid the description behind the title when both existed. The preview carries sanitised tools now (previewManifest is exported and tested, which is how the gap survived), and both fields render, description always. plugins:use is out of the DCR default. Adding it to ALL_SCOPES meant a registration that names no scopes got it and had it pre-selected at consent. Defaults come from DEFAULT_CLIENT_SCOPES now; asking for it by name still works. The public ManifestCapabilities gained mcpTools, so a typed manifest can express what the runtime already validates. MCPANN-001..005, MCPENF-001..010, MCPPREV-001..005, MCPTOOLS-020..022, SDKP-015b/c.	2026-08-29 12:22:59 +02:00
Renzo Beux	41de91668b	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-08-29 12:22:59 +02:00