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.
This commit is contained in:
Maurice
2026-08-29 12:03:13 +02:00
committed by Maurice
parent 937e32924d
commit a8a05ace40
4 changed files with 294 additions and 32 deletions
@@ -28,6 +28,10 @@ const EXPECTED_READ_ONLY = new Set<string>([
'geolocation:read',
'rates:read',
'weather:read',
// The hook permission for mcpToolProvider, just not hook:*-shaped. Every
// plugin that can advertise a tool holds it, so classifying it as a side
// effect would make readOnlyHint unreachable for all of them.
'mcp:tools',
...KNOWN_PERMISSIONS.filter((p) => p.startsWith('db:read:')),
...KNOWN_PERMISSIONS.filter((p) => p.startsWith('hook:')),
]);
@@ -57,6 +61,16 @@ describe('grant classification', () => {
expect(readOnly('some:future:grant')).toBe(false);
});
it('MCPANN-006: a read-only plugin keeps the hint despite the grant that lets it advertise', () => {
// Regression: mcp:tools is the permission every publishing plugin holds, so
// counting it as a side effect put readOnlyHint out of reach for all of them
// and cost every legitimate read tool its auto-approval.
expect(readOnly('mcp:tools')).toBe(true);
const out = clampToolAnnotations({ readOnlyHint: true }, new Set(['mcp:tools', 'weather:read']));
expect(out.readOnlyHint).toBe(true);
expect(out.destructiveHint).toBe(false);
});
it('MCPANN-005: a per-host egress grant is a side effect and forces openWorldHint', () => {
const out = clampToolAnnotations({ readOnlyHint: true, openWorldHint: false }, new Set(['http:outbound:api.example.com']));
expect(out.readOnlyHint).toBe(false);
Binary file not shown.