From 1c24eda42921b37bcdefc829a11c0a8f694fc470 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:48:36 -0400 Subject: [PATCH] fix: validate the whole varspec, and derive the TUI's message from its gate Addresses Copilot's round-7 review on #2035. Two suppressed comments, both real; each reproduced against this branch before acting. 1. `parseVarSpec` was a "strip the first `*`, trim, split on `:`" pass, so it accepted shapes RFC 6570's varspec production does not admit -- measured on the previous commit: x://{*id} name "id" => "x://abcdef" explode is a TRAILING modifier x://{id*:3} name "id" => "x://abc" the two modifiers are exclusive x://{ id } name "id" => "x://abcdef" whitespace is not a varchar x://{a b} name "a b" => "x://1" ditto -- and it emits into the URI `{id*:3}` is the worst of them: it silently truncated the value through a combination the grammar forbids. Parsing is now one anchored regex -- a varname, then at most one trailing `*` or `:1-9999`. The name set is `varchar` widened by `-` and `~`, deliberately. A name is the one thing expansion emits into the URI WITHOUT encoding it (`;`, `?` and `&` write `name=value`), so the rule it has to satisfy is "needs no encoding" -- RFC 3986 unreserved plus pct-encoded, of which the RFC's own varchar is a subset. Strict varchar would refuse `{user-id}`, which real servers publish and which the SDK's matcher round-trips; refusing to read a resource that demonstrably works is a worse outcome for a debugging tool than a name one character outside the production. Everything now rejected is a shape we would otherwise have to guess at. 2. The TUI built its "Missing required template variable(s): ..." list with its own bare `values[name]` filter while gating on `hasRequiredValues`, which reads own properties only. For a variable legitimately named `constructor` the two disagreed: `({})["constructor"]` is `Object`, whose `.length` is 1, so the list came out empty and the error named no field at all while the submit stayed blocked. Both now come from one exported `unmetRequiredGroups`, with `hasRequiredValues` defined in terms of it. Also renames a test whose title still said multi-name query expressions were left "to the SDK, which handles them" -- expansion has been owned here since 935113d1; the behavior it asserts is naming, not delegation. npm run ci passes. Signed-off-by: cliffhall --- README.md | 2 +- .../tui/__tests__/ResourceTestModal.test.tsx | 17 +++ .../tui/src/components/ResourceTestModal.tsx | 19 ++-- .../web/src/test/core/mcp/uriTemplate.test.ts | 82 +++++++++++++- .../integration/mcp/rfc6570-templates.test.ts | 23 +++- core/mcp/uriTemplate.ts | 103 ++++++++++++++---- test-servers/src/test-server-fixtures.ts | 18 +++ 7 files changed, 228 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index ec2af96c..0f29e549 100644 --- a/README.md +++ b/README.md @@ -263,7 +263,7 @@ The SDK's `UriTemplate` is still used, but only to *validate* a template (constr The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the form renders fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. -A template that cannot be expanded at all — an out-of-grammar modifier (`{id:abc}`), or an expression declaring no variable (`{}`, `{a,}`, `{?}`) — **withholds the read** rather than sending something. The panel disables Read Resource, prints the reason, and leaves the raw template in the preview. The alternative is worse than it looks: `x://{}` would otherwise expand to `x://` with no inputs rendered, so the form's "everything required is filled" check passes vacuously and it reads a URI that is not the template the server published. +A template that cannot be expanded at all — an out-of-grammar modifier (`{id:abc}`), or an expression declaring no variable (`{}`, `{a,}`, `{?}`) — **withholds the read** rather than sending something. Pick **events_malformed** (`foobar://events/{topic:abc}`) to see it: Read Resource is disabled, the reason is printed under the form, and the preview shows the template as the server declared it. The alternative is worse than it looks: `x://{}` would otherwise expand to `x://` with no inputs rendered, so the form's "everything required is filled" check passes vacuously and it reads a URI that is not the template the server published. Requiredness is a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and a form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which no per-variable flag can express once a name recurs across expressions (`{a,b}{a,c}` is satisfied by filling `b` and `c`). diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index 0067d211..26dfdc4d 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -95,6 +95,23 @@ describe("ResourceTestModal", () => { unmount(); }); + it("blocks a required variable whose name collides with Object.prototype", async () => { + // `constructor` is a valid RFC 6570 varname, and the modal's own filter + // used to read `Object` (length 1) as a filled value. The *message* built + // from that filter came out naming no field; since frames render empty + // here (see the header note), that half is asserted on + // `unmetRequiredGroups` in the core suite. This asserts the gate. + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{constructor}" }), + { constructor: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + it("submits once any one name in the group has a value", async () => { const read = vi.fn().mockResolvedValue({ result: { contents: [] }, diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 37d662a2..0ac77a5d 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -6,8 +6,8 @@ import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { ReadResourceResult } from "@modelcontextprotocol/client"; import { uriTemplateToForm } from "../utils/uriTemplateToForm.js"; import { - hasRequiredValues, requiredGroups, + unmetRequiredGroups, } from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; @@ -142,13 +142,16 @@ export function ResourceTestModal({ // and its members are left optional there. Without this check the TUI would // submit `{a,b}` completely blank -- dropping the expression and reading a // different resource -- while the web panel blocks the same request (#1919). - const groups = requiredGroups(template.uriTemplate); - if (!hasRequiredValues(groups, values)) { - const unmet = groups - .filter( - (names) => !names.some((name) => (values[name] ?? "").length > 0), - ) - .map((names) => names.join(" or ")); + // The message and the gate come from the SAME pass. Filtering the groups + // again here with a bare `values[name]` disagreed with the gate for a + // variable named `constructor` or `toString`: the inherited member read as + // filled, so the list came out empty and the error named no field at all. + const unmetGroups = unmetRequiredGroups( + requiredGroups(template.uriTemplate), + values, + ); + if (unmetGroups.length > 0) { + const unmet = unmetGroups.map((names) => names.join(" or ")); setResult({ input: values, output: null, diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 26a4c9ee..7c9d5241 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -7,6 +7,7 @@ import { requiredGroups, templateVariables, tryExpandUriTemplate, + unmetRequiredGroups, } from "@inspector/core/mcp/uriTemplate.js"; describe("parseUriTemplate", () => { @@ -205,7 +206,10 @@ describe("expandUriTemplate - multi-name expressions (SDK correction)", () => { expect(expandUriTemplate("x://a{/p,q}", {})).toBe("x://a"); }); - it("leaves multi-name query expressions to the SDK, which handles them", () => { + // Naming, not delegation: a multi-name QUERY expression pairs each name with + // its own value and joins with `&` (RFC 6570 §3.2.8), unlike the bare + // comma-joined list every non-named operator above produces. + it("pairs each name with its value in a multi-name query expression", () => { expect(expandUriTemplate("x://a{?p,q}", { p: "x/y", q: "z" })).toBe( "x://a?p=x%2Fy&q=z", ); @@ -548,6 +552,82 @@ describe("an expression that declares no variable", () => { }); }); +describe("varspec grammar", () => { + // Each of these expanded rather than being refused before the anchored + // grammar landed -- measured on the previous commit: `{*id}` and `{ id }` + // both yielded "x://abcdef", and `{id*:3}` silently truncated to "x://abc" + // through a modifier combination RFC 6570 does not allow. + it.each([ + ["explode in the leading position", "x://{*id}"], + ["explode combined with a prefix", "x://{id*:3}"], + ["whitespace around the name", "x://{ id }"], + ["whitespace inside the name", "x://{a b}"], + ["a name outside the character set", "x://{a/b}"], + ])("strict rejects %s", (_label, template) => { + expect(() => + expandUriTemplateStrict(template, { + id: "abcdef", + a: "1", + "a/b": "1", + "a b": "1", + }), + ).toThrow(/Invalid RFC 6570 varspec/); + }); + + it.each([ + // RFC 6570 varchar, plus the two RFC 3986 unreserved characters the set is + // deliberately widened by -- a name is emitted verbatim by `?`/`&`/`;`, so + // the rule is "needs no encoding", and `{user-id}` is common in the wild. + ["a plain name", "x://{id}", { id: "7" }, "x://7"], + ["a hyphen", "x://{user-id}", { "user-id": "7" }, "x://7"], + ["a tilde", "x://{a~b}", { "a~b": "7" }, "x://7"], + ["an underscore", "x://{a_b}", { a_b: "7" }, "x://7"], + ["a dot separator", "x://{a.b}", { "a.b": "7" }, "x://7"], + ["a pct-encoded varchar", "x://{a%2Fb}", { "a%2Fb": "7" }, "x://7"], + ["a trailing explode", "x://{id*}", { id: "7" }, "x://7"], + ["a trailing prefix", "x://{id:3}", { id: "abcdef" }, "x://abc"], + ])("accepts %s", (_label, template, values, expected) => { + expect(expandUriTemplate(template, values)).toBe(expected); + }); + + it.each(["x://{a.}", "x://{a..b}"])( + "rejects the misplaced dot in %s", + (template) => { + // varname = varchar *( ["."] varchar ) -- a dot separates, it cannot + // trail or double. + expect(() => expandUriTemplateStrict(template, {})).toThrow( + /Invalid RFC 6570 varspec/, + ); + }, + ); + + it("reads a leading dot as the label operator, not a malformed name", () => { + // `{.a}` is label expansion of `a` -- the one "misplaced dot" that is not + // one, since the operator is stripped before the varspec is parsed. + expect(expandUriTemplate("x://a{.ext}", { ext: "json" })).toBe( + "x://a.json", + ); + }); +}); + +describe("unmetRequiredGroups", () => { + it("names the groups that are missing", () => { + expect(unmetRequiredGroups([["a", "b"], ["c"]], { c: "1" })).toEqual([ + ["a", "b"], + ]); + }); + + it("agrees with hasRequiredValues on a prototype-collision name", () => { + // The TUI derived its "Missing required template variable(s)" list with its + // own bare `values[name]` filter: `({})["constructor"]` is `Object`, whose + // `.length` is 1, so the group read as satisfied and the message named no + // field while the gate still blocked the submit. + const groups = [["constructor"]]; + expect(hasRequiredValues(groups, {})).toBe(false); + expect(unmetRequiredGroups(groups, {})).toEqual([["constructor"]]); + }); +}); + describe("tryExpandUriTemplate", () => { it("returns the URI and no error when the template expands", () => { expect( diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts index 5765c369..c79872c8 100644 --- a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -3,7 +3,10 @@ import path from "node:path"; import { fileURLToPath } from "node:url"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; -import { expandUriTemplate } from "@inspector/core/mcp/uriTemplate.js"; +import { + expandUriTemplate, + tryExpandUriTemplate, +} from "@inspector/core/mcp/uriTemplate.js"; import { createTestServerHttp, type TestServerHttp, @@ -83,15 +86,31 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { return connected; } - it("advertises both templates from the checked-in config", async () => { + it("advertises every template from the checked-in config", async () => { const connected = await connectToShowcase(); const { resourceTemplates } = await connected.listAllResourceTemplates(); expect(resourceTemplates.map((entry) => entry.uriTemplate).sort()).toEqual([ + // The malformed one is advertised precisely so a client has to decide + // what to do with it; the SDK's own constructor accepts it, so nothing + // upstream of the Inspector rejects it. See the refusal test below. + "foobar://events/{topic:abc}", "foobar://events/{topic}", "foobar://events{?topic}", ]); }); + it("refuses to expand the malformed template rather than guessing", async () => { + // `abc` is not RFC 6570's `max-length` production, so there is no URI to + // send. Guessing `{topic}` would read something the server never + // advertised -- and this server would answer it, which is exactly why the + // check has to happen client-side. + const result = tryExpandUriTemplate("foobar://events/{topic:abc}", { + topic: "news", + }); + expect(result.uri).toBeUndefined(); + expect(result.error).toMatch(/Invalid RFC 6570 varspec/); + }); + it("resolves the encoded URI the simple expression expands to", async () => { const connected = await connectToShowcase(); const uri = expandUriTemplate("foobar://events/{topic}", { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 805f33a1..5150ac37 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -122,21 +122,62 @@ export interface TemplateVariable { } /** - * RFC 6570's `max-length` production: `%x31-39 0*3DIGIT` — 1 to 9999, no - * leading zero. `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are all invalid - * *templates*, not templates with an ignorable modifier. + * One character of a variable's name. + * + * RFC 6570's `varchar` is `ALPHA / DIGIT / "_" / pct-encoded`. This admits two + * more — `-` and `~` — and the widening is deliberate, on a rule the RFC's own + * set is a subset of: **a name is the one thing expansion emits into the URI + * without encoding it** (the `;`, `?` and `&` operators write `name=value`), so + * a name must consist of characters that need no encoding. That is RFC 3986's + * *unreserved* set — `ALPHA / DIGIT / -._~` — plus pct-encoded. + * + * Strict `varchar` would refuse `{user-id}`, which real servers publish and + * which the SDK's own matcher round-trips (`match()` compiles the hyphen + * literally). Refusing to read a resource that demonstrably works is a worse + * outcome for a debugging tool than tolerating a name one character outside the + * production — whereas everything this *does* reject is a shape we would + * otherwise have to **guess** at, which is the line the rest of this module + * draws too. */ -const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/; +const VARCHAR = String.raw`(?:[A-Za-z0-9_\-~]|%[0-9A-Fa-f]{2})`; + +/** + * RFC 6570's `varspec`: a `varname` with at most one trailing modifier, either + * the explode `*` or a `:max-length`, never both. + * + * `varname = varchar *( ["."] varchar )`, so a `.` may separate name segments + * but cannot lead, trail, or double. `max-length` is `%x31-39 0*3DIGIT` — 1 to + * 9999, no leading zero — so `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are + * invalid *templates*, not templates with an ignorable modifier. + * + * Anchoring the whole varspec is what rejects the shapes a looser "strip the + * `*`, split on `:`" pass waves through. Measured against this branch before + * the change, each expanded rather than being refused: + * + * | Varspec | Was | Why it is invalid | + * | --------- | -------------------- | ----------------------------------- | + * | `{*id}` | name `id` | explode is a *trailing* modifier | + * | `{id*:3}` | name `id`, truncated | the two modifiers are exclusive | + * | `{ id }` | name `id` | whitespace is not a `varchar` | + * | `{a b}` | name `a b` | ditto — and it emits into the URI | + */ +const VARSPEC_GRAMMAR = new RegExp( + `^(${VARCHAR}(?:\\.?${VARCHAR})*)(?:\\*|:([1-9][0-9]{0,3}))?$`, +); /** * Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix, - * or `"invalid"` for anything RFC 6570's `varspec` production does not admit. + * or `"invalid"` for anything {@link VARSPEC_GRAMMAR} does not admit. * - * Two shapes reach that verdict, and neither is ignorable: + * Three classes reach that verdict, none of them ignorable: * - * - **A modifier outside the grammar** (`{id:abc}`). Treating it as a plain + * - **A modifier outside the grammar** (`{id:abc}`), or two at once (`{id*:3}`), + * or one in the wrong position (`{*id}`). Treating any of them as a plain * `{id}` would send a URI that does not match what the server advertised, - * with nothing to alert anyone. + * with nothing to alert anyone — and `{id*:3}` silently *truncated* the value. + * - **A name outside the character set** (`{a b}`, `{a/b}`). A named operator + * emits the name verbatim, so this is the one place an unencodable character + * reaches the URI as structure. * - **An empty varspec** — `{}`, `{,}`, `{a,}`, `{?}`, `{*}`. RFC 6570 requires * at least one varspec per expression and admits no empty member, so this is * a malformed *template*, not an expression with a member to skip. Skipping @@ -144,21 +185,15 @@ const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/; * form rendered no inputs, so its "everything required is filled" check would * be vacuously true and it would submit a URI that is not the template the * server published. + * + * The explode modifier parses but is not honored: it only changes how a list or + * map value is joined, and every value reaching this module is a single string. */ function parseVarSpec(raw: string): VarSpec | "invalid" { - // The explode modifier is stripped rather than honored: it only changes how - // a list or map value is joined, and every value reaching this module is a - // single string. - const spec = raw.replace("*", "").trim(); - if (spec.length === 0) return "invalid"; - - const colon = spec.indexOf(":"); - if (colon === -1) return { name: spec }; - - const name = spec.slice(0, colon); - const modifier = spec.slice(colon + 1); - if (name.length === 0 || !MAX_LENGTH_GRAMMAR.test(modifier)) return "invalid"; - return { name, maxLength: Number(modifier) }; + const match = VARSPEC_GRAMMAR.exec(raw); + if (match === null) return "invalid"; + const [, name, maxLength] = match; + return maxLength === undefined ? { name } : { name, maxLength: +maxLength }; } /** @@ -264,6 +299,28 @@ export function requiredGroups(uriTemplate: string): string[][] { return groups; } +/** + * The required expressions `values` does **not** satisfy — none of their names + * filled in — in template order. + * + * Exported so a caller that has to *name* the missing fields derives them from + * the same pass that decides whether anything is missing at all. The TUI built + * its "Missing required template variable(s): …" list with its own bare + * `values[name]` filter, which for a variable legitimately named `constructor` + * or `toString` found `Object.prototype`'s member and judged the group + * satisfied — so {@link hasRequiredValues} blocked the submit while the message + * listed nothing. + */ +export function unmetRequiredGroups( + groups: string[][], + values: Record, +): string[][] { + return groups.filter( + (names) => + !names.some((name) => (readValue(values, name) ?? "").length > 0), + ); +} + /** * Whether `values` supplies everything expansion structurally needs: every * required expression has at least one of its names filled in. @@ -272,9 +329,7 @@ export function hasRequiredValues( groups: string[][], values: Record, ): boolean { - return groups.every((names) => - names.some((name) => (readValue(values, name) ?? "").length > 0), - ); + return unmetRequiredGroups(groups, values).length === 0; } /** diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 9e58cfaa..5dd8cd29 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1539,6 +1539,24 @@ export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] { inputSchema: { topic: z.string().describe("Topic name") }, handler: echo("foobar://events{?topic}"), }, + { + // A template no client can honor: `abc` is not RFC 6570's `max-length` + // production (`%x31-39 0*3DIGIT`), so `{topic:abc}` is a malformed + // *template* rather than one with an ignorable modifier. The SDK's + // `UriTemplate` constructor accepts it, which is exactly why the + // Inspector checks the grammar itself and refuses the read -- guessing + // `{topic}` would send a URI this server never advertised. + // + // The handler is therefore unreachable through the Inspector by design. + // It is registered anyway so the template appears in + // `resources/templates/list`, which is what puts the refusal on screen. + name: "events_malformed", + uriTemplate: "foobar://events/{topic:abc}", + description: + "Malformed template - an out-of-grammar prefix modifier; the read is withheld", + inputSchema: { topic: z.string().describe("Topic name") }, + handler: echo("foobar://events/{topic:abc}"), + }, ]; }