From 83658db2dd1c81ec49192a68378072c93d4d84f5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:31:35 -0400 Subject: [PATCH 01/21] fix(web): expand resource templates per RFC 6570 The Resources screen discovered and substituted template variables with a bare `/\{(\w+)\}/g` regex, which is wrong two ways: it cannot see an expression carrying an operator, so `foobar://events{?topic}` rendered no input at all; and it splices the raw value in, so a `/`, `?`, `#`, `%`, space or non-ASCII character in a simple `{topic}` landed unencoded and changed the URI's structure -- `foo/bar` produced an extra path segment that a conforming matcher rejects with `-32602 Resource not found`. Delegate expansion to the SDK's `UriTemplate`, the same implementation `InspectorClient.readResourceFromTemplate` (and so the TUI) already expands through, so the two clients cannot disagree about what a template means. The new `utils/uriTemplate` supplies only what that class does not: which variables to render an input for, which of them a read cannot proceed without, and a partially-expanded preview. Required-ness follows the operator. Under `?`, `&`, `.` or `/` the whole expression is omitted when the variable is undefined, so those fields are marked Optional and reading with them blank is a legitimate request for the unfiltered resource; under `""`, `+` or `#` the variable sits mid-URI, so it stays required. Blank fields are dropped before expanding so an untouched optional field reads as undefined rather than as the empty string, which would expand to a valueless `?topic=`. Adds the `rfc6570_templates` preset and a `rfc6570-templates-http.json` showcase server serving the two templates from the issue. Closes #1919 Signed-off-by: cliffhall --- README.md | 11 + .../ResourceTemplatePanel.test.tsx | 91 ++++++++ .../ResourceTemplatePanel.tsx | 61 +++-- clients/web/src/utils/uriTemplate.test.ts | 168 ++++++++++++++ clients/web/src/utils/uriTemplate.ts | 219 ++++++++++++++++++ .../configs/rfc6570-templates-http.json | 12 + test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 47 ++++ 8 files changed, 578 insertions(+), 34 deletions(-) create mode 100644 clients/web/src/utils/uriTemplate.test.ts create mode 100644 clients/web/src/utils/uriTemplate.ts create mode 100644 test-servers/configs/rfc6570-templates-http.json diff --git a/README.md b/README.md index cb57dd359..de94130ed 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | | `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | | `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | +| `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | @@ -237,6 +238,16 @@ Open the Tools tab and select `record_shipment`: `direction` must render as a ** The **TUI** had the same gap and is worth checking against the same server (`--tui`, then test `record_shipment`): `direction` is a select, `quantity` an integer field, `express` a boolean. Both clients now share one collapse step — `normalizeNullableUnion` in [`core/json/nullableUnion.ts`](./core/json/nullableUnion.ts) — precisely so they cannot drift on which schemas they can render. +#### RFC 6570 resource templates + +`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against and the variables the server decoded. Plain streamable-HTTP; connect with the **default (legacy)** protocol era. + +Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The request must go out as `foobar://events/foo%2Fbar`, and the result echoes back the URI the server matched. On the broken build the value was spliced in raw, so the slash created a second path segment and the SDK's matcher answered `-32602 Resource not found: foobar://events/foo/bar` — the exact failure in the issue. The same holds for `?`, `#`, `%`, spaces, and non-ASCII text. + +**events_by_query** is the half that was invisible: the old `/\{(\w+)\}/g` scan could not see an expression carrying an operator, so no `topic` input was rendered at all. It now appears, marked **Optional** — RFC 6570 drops the whole expression when the variable is undefined, so reading with the field blank requests `foobar://events`, and filling it in requests `foobar://events?topic=foo%2Fbar`. The URI preview beside the title shows the partially-expanded form as you type, leaving unfilled expressions standing as written. + +Both clients expand through the SDK's `UriTemplate` — the web client via [`clients/web/src/utils/uriTemplate.ts`](./clients/web/src/utils/uriTemplate.ts), the TUI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. + #### Advertised extensions `advertised-extensions-http.json` serves `echo` (always) and a `get_weather` tool **gated on the `io.modelcontextprotocol/tasks` extension** (`extensionGatedTools`): the tool is registered but starts disabled, and the server enables it on `notifications/initialized` only when the client declared that extension in its `capabilities.extensions`. diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 596a9340f..4955b385f 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -27,6 +27,11 @@ const noVarTemplate: ResourceTemplate = { uriTemplate: "file:///static.txt", }; +const queryTemplate: ResourceTemplate = { + name: "Events", + uriTemplate: "foobar://events{?topic}", +}; + describe("ResourceTemplatePanel", () => { it("renders the template title (or name) and description", () => { renderWithMantine( @@ -154,6 +159,92 @@ describe("ResourceTemplatePanel", () => { ).not.toBeDisabled(); }); + describe("RFC 6570 expansion (#1919)", () => { + it("renders an input for a query expression the old regex could not see", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("topic")).toBeInTheDocument(); + }); + + it("percent-encodes a reserved character in a simple variable", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("userId"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "file:///users/foo%2Fbar/profile", + ); + }); + + it("builds an encoded query expression for {?topic}", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("topic"), "foo/bar"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "foobar://events?topic=foo%2Fbar", + ); + }); + + it("marks a query variable Optional and does not gate the read on it", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect(screen.getByText("Optional")).toBeInTheDocument(); + const button = screen.getByRole("button", { name: "Read Resource" }); + expect(button).not.toBeDisabled(); + // Left blank, the whole expression drops out per RFC 6570. + await user.click(button); + expect(onReadResource).toHaveBeenCalledWith("foobar://events"); + }); + + it("does not mark a required simple variable Optional", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Optional")).not.toBeInTheDocument(); + }); + + it("previews the query expression verbatim until it is filled", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + expect(screen.getByText("foobar://events{?topic}")).toBeInTheDocument(); + await user.type(screen.getByLabelText("topic"), "news"); + expect( + screen.getByText("foobar://events?topic=news"), + ).toBeInTheDocument(); + }); + }); + describe("completions", () => { it("fires a completion immediately on focus before any keystroke", async () => { const user = userEvent.setup(); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 6b506228b..eafd16f47 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -14,6 +14,11 @@ import { useValueChange } from "../../../hooks/useValueChange"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; +import { + expandUriTemplate, + previewUriTemplate, + templateVariables, +} from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { template: ResourceTemplate; @@ -39,34 +44,6 @@ export interface ResourceTemplatePanelProps { const COMPLETION_DEBOUNCE_MS = 300; -function parseVariableNames(uriTemplate: string): string[] { - const names: string[] = []; - const regex = /\{(\w+)\}/g; - let match: RegExpExecArray | null; - - while ((match = regex.exec(uriTemplate)) !== null) { - names.push(match[1]); - } - - return names; -} - -function resolveUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (_, key: string) => variables[key]); -} - -function previewUri( - uriTemplate: string, - variables: Record, -): string { - return uriTemplate.replace(/\{(\w+)\}/g, (match, key: string) => - variables[key]?.length > 0 ? variables[key] : match, - ); -} - const HeaderRow = Group.withProps({ justify: "space-between", wrap: "nowrap", @@ -108,10 +85,16 @@ export function ResourceTemplatePanel({ }: ResourceTemplatePanelProps) { const { name, title, uriTemplate, description, annotations } = template; - const variableNames = useMemo( - () => parseVariableNames(uriTemplate), + // Every variable the template declares, with the operator it appears under + // and whether omitting it would change the URI's shape (see `utils/uriTemplate`). + const declaredVariables = useMemo( + () => templateVariables(uriTemplate), [uriTemplate], ); + const variableNames = useMemo( + () => declaredVariables.map((v) => v.name), + [declaredVariables], + ); const [variables, setVariables] = useState>(() => Object.fromEntries(variableNames.map((n) => [n, ""])), @@ -232,13 +215,18 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - const canSubmit = variableNames.every((n) => variables[n]?.length > 0); + // Only the variables whose absence would change the URI's shape gate the + // read; an unfilled `{?topic}` is a legitimate request for the unfiltered + // resource, and RFC 6570 drops the whole expression for it. + const canSubmit = declaredVariables.every( + (v) => !v.required || variables[v.name]?.length > 0, + ); function handleSubmit() { - onReadResource(resolveUri(uriTemplate, variables)); + onReadResource(expandUriTemplate(uriTemplate, variables)); } - const preview = previewUri(uriTemplate, variables); + const preview = previewUriTemplate(uriTemplate, variables); return ( @@ -251,13 +239,17 @@ export function ResourceTemplatePanel({ {description && {description}} - {variableNames.map((varName) => { + {declaredVariables.map(({ name: varName, required }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; + // RFC 6570 omits an undefined variable under a query/path-segment + // operator entirely, so those fields are genuinely optional. + const description = required ? undefined : "Optional"; return useAutocomplete ? ( diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts new file mode 100644 index 000000000..b337a42b5 --- /dev/null +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -0,0 +1,168 @@ +import { describe, it, expect } from "vitest"; +import { + expandUriTemplate, + parseUriTemplate, + previewUriTemplate, + templateVariables, +} from "./uriTemplate"; + +describe("parseUriTemplate", () => { + it("splits literals from expressions", () => { + expect(parseUriTemplate("foobar://events/{topic}")).toEqual([ + { kind: "literal", text: "foobar://events/" }, + { + kind: "expression", + source: "{topic}", + operator: "", + names: ["topic"], + }, + ]); + }); + + it("reads the operator and the comma-separated name list", () => { + expect(parseUriTemplate("x://{?a,b*}")).toEqual([ + { kind: "literal", text: "x://" }, + { + kind: "expression", + source: "{?a,b*}", + operator: "?", + names: ["a", "b"], + }, + ]); + }); + + it("treats an unclosed expression as trailing literal text", () => { + expect(parseUriTemplate("x://a/{oops")).toEqual([ + { kind: "literal", text: "x://a/{oops" }, + ]); + }); + + it("returns nothing for an empty template", () => { + expect(parseUriTemplate("")).toEqual([]); + }); +}); + +describe("templateVariables", () => { + it("finds a simple variable and marks it required", () => { + expect(templateVariables("foobar://events/{topic}")).toEqual([ + { name: "topic", operator: "", required: true }, + ]); + }); + + it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { + expect(templateVariables("foobar://events{?topic}")).toEqual([ + { name: "topic", operator: "?", required: false }, + ]); + }); + + it.each([ + ["{+path}", "+", true], + ["{#frag}", "#", true], + ["{.label}", ".", false], + ["{/segment}", "/", false], + ["{&extra}", "&", false], + ])("classifies %s", (expression, operator, required) => { + const [variable] = templateVariables(`x://a${expression}`); + expect(variable.operator).toBe(operator); + expect(variable.required).toBe(required); + }); + + it("deduplicates a repeated name and keeps it required if any use is", () => { + expect(templateVariables("x://{?id}/{id}")).toEqual([ + { name: "id", operator: "?", required: true }, + ]); + }); + + it("returns an empty list for a template with no expressions", () => { + expect(templateVariables("file:///static.txt")).toEqual([]); + }); +}); + +describe("expandUriTemplate", () => { + it("percent-encodes a reserved character in a simple variable (#1919)", () => { + expect( + expandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it.each([ + ["?", "a?b", "a%3Fb"], + ["#", "a#b", "a%23b"], + ["%", "a%b", "a%25b"], + ["space", "a b", "a%20b"], + ["unicode", "caffè", "caff%C3%A8"], + ])("encodes %s", (_label, value, encoded) => { + expect(expandUriTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); + }); + + it("builds an encoded query expression", () => { + expect( + expandUriTemplate("foobar://events{?topic}", { topic: "foo/bar" }), + ).toBe("foobar://events?topic=foo%2Fbar"); + }); + + it("leaves reserved characters intact under the + operator", () => { + expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); + }); + + it("omits an expression whose variable was left blank", () => { + expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( + "foobar://events", + ); + }); + + it("joins two query expressions with & rather than a second ?", () => { + expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( + "x://a?one=1&two=2", + ); + }); + + it("falls back to the raw template when the SDK cannot parse it", () => { + expect(expandUriTemplate("x://a/{oops", { oops: "v" })).toBe("x://a/{oops"); + }); +}); + +describe("previewUriTemplate", () => { + it("shows the template verbatim before anything is entered", () => { + expect(previewUriTemplate("file:///users/{userId}/profile", {})).toBe( + "file:///users/{userId}/profile", + ); + }); + + it("substitutes only the expressions that are filled", () => { + expect( + previewUriTemplate("db://{tableName}/rows/{rowId}", { + tableName: "users", + rowId: "", + }), + ).toBe("db://users/rows/{rowId}"); + }); + + it("encodes the filled values the same way expansion does", () => { + expect( + previewUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it("keeps a multi-name expression whole until every name is filled", () => { + expect(previewUriTemplate("x://a{?one,two}", { one: "1" })).toBe( + "x://a{?one,two}", + ); + }); + + it("still rewrites the second ? to & when both query expressions resolve", () => { + expect( + previewUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" }), + ).toBe("x://a?one=1&two=2"); + }); + + it("restores a deferred expression that follows a resolved query expression", () => { + expect(previewUriTemplate("x://a{?one}{?two}", { one: "1" })).toBe( + "x://a?one=1{?two}", + ); + }); + + it("falls back to the raw template when the SDK cannot parse it", () => { + expect(previewUriTemplate("x://a/{oops", {})).toBe("x://a/{oops"); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts new file mode 100644 index 000000000..25cd027eb --- /dev/null +++ b/clients/web/src/utils/uriTemplate.ts @@ -0,0 +1,219 @@ +/** + * RFC 6570 URI Template support for the Resources screen. + * + * The web client used to discover and substitute variables with a bare + * `/\{(\w+)\}/g` regex, which is wrong in two ways (#1919): it cannot see an + * expression carrying an operator (`{?topic}`, `{/path}`, `{#frag}`, ...), so + * no input is rendered for it; and it splices the raw value in, so a `/`, `?`, + * `#` or space in a simple `{var}` lands unencoded and silently changes the + * URI's structure. + * + * Expansion itself is delegated to the SDK's `UriTemplate` -- the same + * implementation `InspectorClient.readResourceFromTemplate` (and therefore the + * TUI) already expands through, so the two clients cannot disagree about what a + * template means. What lives here is the surrounding form/preview logic the SDK + * class does not provide: which variables to render an input for, which of them + * a read cannot proceed without, and a partially-expanded preview string. + */ + +import { UriTemplate } from "@modelcontextprotocol/client"; + +/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ +const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; + +/** + * Operators whose expansion omits cleanly when the variables in it are + * undefined -- the whole expression, separator included, simply disappears. + * A variable under any *other* operator is interpolated into the middle of the + * URI, so leaving it out produces a different resource path rather than a + * shorter one; those we require before allowing a read. + */ +const OMITTABLE_OPERATORS = new Set([".", "/", "?", "&"]); + +interface TemplateLiteral { + kind: "literal"; + text: string; +} + +interface TemplateExpression { + kind: "expression"; + /** The expression including its braces, e.g. `{?topic}`. */ + source: string; + /** The RFC 6570 operator, or `""` for a simple expression. */ + operator: string; + /** Variable names in the expression, `*` (explode) and whitespace stripped. */ + names: string[]; +} + +export type TemplatePart = TemplateLiteral | TemplateExpression; + +export interface TemplateVariable { + name: string; + /** The operator of the expression the variable was first seen in. */ + operator: string; + /** + * True when omitting the variable would change the URI's structure rather + * than shorten it -- see {@link OMITTABLE_OPERATORS}. + */ + required: boolean; +} + +/** + * Splits a template into literal runs and expressions. + * + * Deliberately mirrors the SDK parser's own scanning rules (first matching + * operator character wins, names split on `,`, `*` stripped) so this never + * disagrees with the class that ultimately does the expanding. An unclosed + * `{` yields a trailing literal, which is what makes the callers below degrade + * to "render no inputs, show the template verbatim" rather than throw. + */ +export function parseUriTemplate(uriTemplate: string): TemplatePart[] { + const parts: TemplatePart[] = []; + let literal = ""; + let i = 0; + + while (i < uriTemplate.length) { + if (uriTemplate[i] !== "{") { + literal += uriTemplate[i]; + i += 1; + continue; + } + const end = uriTemplate.indexOf("}", i); + if (end === -1) { + // Unclosed expression -- the rest is not a template, treat it as text. + literal += uriTemplate.slice(i); + break; + } + if (literal) { + parts.push({ kind: "literal", text: literal }); + literal = ""; + } + const body = uriTemplate.slice(i + 1, end); + const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; + const names = body + .slice(operator.length) + .split(",") + .map((name) => name.replace("*", "").trim()) + .filter((name) => name.length > 0); + parts.push({ + kind: "expression", + source: uriTemplate.slice(i, end + 1), + operator, + names, + }); + i = end + 1; + } + + if (literal) parts.push({ kind: "literal", text: literal }); + return parts; +} + +/** + * The variables a form should render an input for, in template order and + * deduplicated by name. A name appearing under more than one operator is + * required if *any* of its occurrences is. + */ +export function templateVariables(uriTemplate: string): TemplateVariable[] { + const byName = new Map(); + + for (const part of parseUriTemplate(uriTemplate)) { + if (part.kind !== "expression") continue; + const required = !OMITTABLE_OPERATORS.has(part.operator); + for (const name of part.names) { + const existing = byName.get(name); + if (existing) { + existing.required = existing.required || required; + } else { + byName.set(name, { name, operator: part.operator, required }); + } + } + } + + return [...byName.values()]; +} + +/** + * Drops empty entries so an untouched optional field reads as *undefined* to + * the SDK (the expression disappears) rather than as the empty string (which + * would expand to a valueless `?topic=`). + */ +function definedValues(values: Record): Record { + return Object.fromEntries( + Object.entries(values).filter(([, value]) => value.length > 0), + ); +} + +/** + * Expands a template against the entered values per RFC 6570 -- percent- + * encoding each value according to its operator, and omitting expressions whose + * variables were left blank. + * + * A template the SDK refuses to parse falls back to the raw template string, + * which is what the user already sees in the preview and what the server will + * reject with a legible error; throwing here would take out the whole panel. + */ +export function expandUriTemplate( + uriTemplate: string, + values: Record, +): string { + try { + return new UriTemplate(uriTemplate).expand(definedValues(values)); + } catch { + return uriTemplate; + } +} + +/** + * A placeholder standing in for an expression the user has not filled in yet. + * + * `U+0000` cannot appear in a URI template, so a token built from it can never + * collide with real template text; and because it is emitted as *literal* text + * rather than as a variable value, expansion passes it through unencoded. + */ +const deferredToken = (index: number) => `\u0000${index}\u0000`; + +/** + * A partially-expanded template for display: expressions whose variables are + * all filled are expanded exactly as {@link expandUriTemplate} would, and the + * rest are left standing as written so the user can see what is still needed. + * + * Unfilled expressions are swapped for an inert token and restored after + * expansion -- rather than expanding each filled expression in isolation -- so + * the SDK still sees one whole template and applies its cross-expression rules + * (notably rewriting a second `?` query expression to `&`). + */ +export function previewUriTemplate( + uriTemplate: string, + values: Record, +): string { + const parts = parseUriTemplate(uriTemplate); + const deferred: string[] = []; + const defined = definedValues(values); + + const rewritten = parts + .map((part) => { + if (part.kind === "literal") return part.text; + if (part.names.every((name) => defined[name] !== undefined)) { + return part.source; + } + deferred.push(part.source); + return deferredToken(deferred.length - 1); + }) + .join(""); + + let expanded: string; + try { + expanded = new UriTemplate(rewritten).expand(defined); + } catch { + return uriTemplate; + } + + // Restored by exact-string replacement rather than by a pattern: a regex + // matching the token would have to embed U+0000 literally, which `eslint` + // rejects (`no-control-regex`) -- and each token's text is already known + // here, so there is nothing to match on. + return deferred.reduce( + (uri, source, index) => uri.replaceAll(deferredToken(index), source), + expanded, + ); +} diff --git a/test-servers/configs/rfc6570-templates-http.json b/test-servers/configs/rfc6570-templates-http.json new file mode 100644 index 000000000..c6949773e --- /dev/null +++ b/test-servers/configs/rfc6570-templates-http.json @@ -0,0 +1,12 @@ +{ + "serverInfo": { + "name": "rfc6570-template-showcase", + "version": "1.0.0" + }, + "resourceTemplates": [{ "preset": "rfc6570_templates" }], + "tools": [{ "preset": "echo" }], + "transport": { + "type": "streamable-http", + "port": 6603 + } +} diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index d78998ec6..34fd53ad7 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -62,6 +62,7 @@ import { createFileResourceTemplate, createUserResourceTemplate, createNumberedResourceTemplates, + createRfc6570ResourceTemplates, createSimplePrompt, createArgsPrompt, createNumberedPrompts, @@ -260,6 +261,8 @@ function resolveResourceTemplatePreset( return createUserResourceTemplate(); case "numbered_resource_templates": return createNumberedResourceTemplates(Number(get("count")) || 3); + case "rfc6570_templates": + return createRfc6570ResourceTemplates(); default: throw new Error(`Unknown resource template preset: ${name}`); } diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 070b57506..89283e494 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1474,6 +1474,53 @@ export function createFileResourceTemplate( }; } +/** + * Resource templates that exercise RFC 6570 expansion (#1919). + * + * `events_by_topic` is the simple `{topic}` expression from the issue: a `/`, + * `?`, `#` or space in the value MUST be percent-encoded, or the URI gains a + * path segment and a conforming matcher rejects it. `events_by_query` is the + * `{?topic}` form, which the web client could not even render an input for. + * + * Both handlers echo the URI they were matched against plus the variables the + * server decoded, so the round-trip is visible in the read result. + */ +export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] { + const echo = + (label: string) => async (uri: URL, params: Record) => ({ + contents: [ + { + uri: uri.toString(), + mimeType: "application/json", + text: JSON.stringify( + { template: label, matchedUri: uri.toString(), variables: params }, + null, + 2, + ), + }, + ], + }); + + return [ + { + name: "events_by_topic", + uriTemplate: "foobar://events/{topic}", + description: + "Simple expression - a reserved character in `topic` must be percent-encoded", + inputSchema: { topic: z.string().describe("Topic name") }, + handler: echo("foobar://events/{topic}"), + }, + { + name: "events_by_query", + uriTemplate: "foobar://events{?topic}", + description: + "Query expression - optional, and omitted entirely when `topic` is blank", + inputSchema: { topic: z.string().describe("Topic name") }, + handler: echo("foobar://events{?topic}"), + }, + ]; +} + /** * Create a "user" resource template that returns user data by ID */ From 409dce37be4647bef46f2df75972b876804fa897 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:51:12 -0400 Subject: [PATCH 02/21] fix(core): share URI-template expansion and correct the SDK's multi-name branch Addresses Copilot's review on #2035. All three findings reproduced against the pinned SDK before acting. 1. `#` was misclassified as required. Measured: `x://a{#frag}` with no `frag` expands to exactly `x://a`, a well-formed URI naming a real resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}` (`file:///users//profile`), which leave an empty path segment. Moved `#` into the omittable set; the required cases now assert what the URI *becomes* when blank, so the rule is checked rather than asserted. 2. Delegating to the SDK did not actually give RFC 6570 expansion. `UriTemplate.expandPart` takes an early `names.length > 1` branch that raw-joins values, skipping both `encodeValue` and the operator prefix: `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` -- the very unencoded-slash defect this PR is about -- and `x://a{/p,q}` to `x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier. Fixing that in the web client alone would have left the TUI and CLI wrong, since `readResourceFromTemplate` expands through the same class. So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both call sites use it. The correction is surgical: a multi-name non-query expression is expanded here and spliced in as literal text before the SDK sees the template (safe -- both encoders escape `{`/`}`), while every single-name and query expression still goes through the SDK untouched. The preview applies the same correction, so it cannot promise a URI that submitting would not send. 3. The showcase promised a blank `{?topic}` read that did not work. `UriTemplate.match()` compiles `{?topic}` to a *required* `\?topic=([^&]+)`, so `match("foobar://events")` returns null and the read 404s. A real server exposes the unfiltered collection as its own resource; the showcase now registers `foobar://events` so the documented step resolves. Verified end to end through the CLI. Signed-off-by: cliffhall --- AGENTS.md | 9 +- README.md | 9 +- .../web/src/test/core/mcp/uriTemplate.test.ts | 194 +++++++++++++ clients/web/src/utils/uriTemplate.test.ts | 133 +-------- clients/web/src/utils/uriTemplate.ts | 187 ++----------- core/mcp/inspectorClient.ts | 20 +- core/mcp/uriTemplate.ts | 257 ++++++++++++++++++ .../configs/rfc6570-templates-http.json | 19 +- test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 21 ++ 10 files changed, 559 insertions(+), 293 deletions(-) create mode 100644 clients/web/src/test/core/mcp/uriTemplate.test.ts create mode 100644 core/mcp/uriTemplate.ts diff --git a/AGENTS.md b/AGENTS.md index d07e84f4d..5005d6d87 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,14 @@ v2/main/ │ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores -│ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand +│ │ # shared by the web Resources form and +│ │ # readResourceFromTemplate (TUI + CLI), so the +│ │ # clients cannot drift on what a template means; +│ │ # delegates to the SDK's UriTemplate but corrects +│ │ # its multi-name `{a,b}` branch, which skips both +│ │ # encoding and the operator prefix — #1919; +│ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; │ │ # listSalvage.ts: per-item salvage for list results — diff --git a/README.md b/README.md index de94130ed..0bc30868b 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,8 @@ inspector/ │ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union │ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton -│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import +│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, +│ │ # and the RFC 6570 URI-template helpers all three clients expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends @@ -240,13 +241,15 @@ The **TUI** had the same gap and is worth checking against the same server (`--t #### RFC 6570 resource templates -`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against and the variables the server decoded. Plain streamable-HTTP; connect with the **default (legacy)** protocol era. +`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era. Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The request must go out as `foobar://events/foo%2Fbar`, and the result echoes back the URI the server matched. On the broken build the value was spliced in raw, so the slash created a second path segment and the SDK's matcher answered `-32602 Resource not found: foobar://events/foo/bar` — the exact failure in the issue. The same holds for `?`, `#`, `%`, spaces, and non-ASCII text. **events_by_query** is the half that was invisible: the old `/\{(\w+)\}/g` scan could not see an expression carrying an operator, so no `topic` input was rendered at all. It now appears, marked **Optional** — RFC 6570 drops the whole expression when the variable is undefined, so reading with the field blank requests `foobar://events`, and filling it in requests `foobar://events?topic=foo%2Fbar`. The URI preview beside the title shows the partially-expanded form as you type, leaving unfilled expressions standing as written. -Both clients expand through the SDK's `UriTemplate` — the web client via [`clients/web/src/utils/uriTemplate.ts`](./clients/web/src/utils/uriTemplate.ts), the TUI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. +> The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. + +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` with one correction applied first: the SDK's `expandPart` takes an early `names.length > 1` branch that raw-joins the values, skipping both percent-encoding **and** the operator prefix, so `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` and `x://a{/p,q}` to `x://ax y,z`. Those expressions are expanded by the helper and spliced in as literal text before the SDK sees them; every single-name and query expression still goes through the SDK untouched. #### Advertised extensions diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts new file mode 100644 index 000000000..570caffc8 --- /dev/null +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -0,0 +1,194 @@ +import { describe, it, expect } from "vitest"; +import { + expandUriTemplate, + parseUriTemplate, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; + +describe("parseUriTemplate", () => { + it("splits literals from expressions", () => { + expect(parseUriTemplate("foobar://events/{topic}")).toEqual([ + { kind: "literal", text: "foobar://events/" }, + { + kind: "expression", + source: "{topic}", + operator: "", + names: ["topic"], + }, + ]); + }); + + it("reads the operator and the comma-separated name list", () => { + expect(parseUriTemplate("x://{?a,b*}")).toEqual([ + { kind: "literal", text: "x://" }, + { + kind: "expression", + source: "{?a,b*}", + operator: "?", + names: ["a", "b"], + }, + ]); + }); + + it("treats an unclosed expression as trailing literal text", () => { + expect(parseUriTemplate("x://a/{oops")).toEqual([ + { kind: "literal", text: "x://a/{oops" }, + ]); + }); + + it("returns nothing for an empty template", () => { + expect(parseUriTemplate("")).toEqual([]); + }); +}); + +describe("templateVariables", () => { + it("finds a simple variable and marks it required", () => { + expect(templateVariables("foobar://events/{topic}")).toEqual([ + { name: "topic", operator: "", required: true }, + ]); + }); + + it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { + expect(templateVariables("foobar://events{?topic}")).toEqual([ + { name: "topic", operator: "?", required: false }, + ]); + }); + + // Required iff omitting the variable leaves an empty slot mid-URI rather + // than a shorter, still-well-formed URI. Verified against the pinned SDK: + // `x://a/{+path}` with no `path` expands to "x://a/" (empty segment), while + // `x://a{#frag}` expands to exactly "x://a". + it.each([ + ["{+path}", "+", true], + ["{#frag}", "#", false], + ["{.label}", ".", false], + ["{/segment}", "/", false], + ["{&extra}", "&", false], + ])("classifies %s", (expression, operator, required) => { + const [variable] = templateVariables(`x://a${expression}`); + expect(variable.operator).toBe(operator); + expect(variable.required).toBe(required); + }); + + it("deduplicates a repeated name and keeps it required if any use is", () => { + expect(templateVariables("x://{?id}/{id}")).toEqual([ + { name: "id", operator: "?", required: true }, + ]); + }); + + it.each([ + ["file:///users/{userId}/profile", "file:///users//profile"], + ["x://a/{+path}", "x://a/"], + ])( + "requires %s because omitting it leaves an empty slot (%s)", + (template, omitted) => { + expect(templateVariables(template)[0].required).toBe(true); + // The reason, asserted rather than asserted-about: this is what the URI + // would become if the field were left blank. + expect(expandUriTemplate(template, {})).toBe(omitted); + }, + ); + + it("does not require {#frag}, which omits to a well-formed URI", () => { + expect(templateVariables("x://a{#frag}")[0].required).toBe(false); + expect(expandUriTemplate("x://a{#frag}", {})).toBe("x://a"); + }); + + it("returns an empty list for a template with no expressions", () => { + expect(templateVariables("file:///static.txt")).toEqual([]); + }); +}); + +describe("expandUriTemplate", () => { + it("percent-encodes a reserved character in a simple variable (#1919)", () => { + expect( + expandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it.each([ + ["?", "a?b", "a%3Fb"], + ["#", "a#b", "a%23b"], + ["%", "a%b", "a%25b"], + ["space", "a b", "a%20b"], + ["unicode", "caffè", "caff%C3%A8"], + ])("encodes %s", (_label, value, encoded) => { + expect(expandUriTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); + }); + + it("builds an encoded query expression", () => { + expect( + expandUriTemplate("foobar://events{?topic}", { topic: "foo/bar" }), + ).toBe("foobar://events?topic=foo%2Fbar"); + }); + + it("leaves reserved characters intact under the + operator", () => { + expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); + }); + + it("omits an expression whose variable was left blank", () => { + expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( + "foobar://events", + ); + }); + + it("joins two query expressions with & rather than a second ?", () => { + expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( + "x://a?one=1&two=2", + ); + }); + + it("falls back to the raw template when the SDK cannot parse it", () => { + expect(expandUriTemplate("x://a/{oops", { oops: "v" })).toBe("x://a/{oops"); + }); +}); + +describe("expandUriTemplate - multi-name expressions (SDK correction)", () => { + // The pinned SDK's `expandPart` takes an early `names.length > 1` branch that + // raw-joins the values, skipping BOTH encodeValue and the operator prefix. + // Measured directly: `x://{a,b}` -> "x://foo/bar,q", `x://a{/p,q}` -> + // "x://ax y,z". These assert the corrected output. + it("encodes each value in a simple multi-name expression", () => { + expect(expandUriTemplate("x://{a,b}", { a: "foo/bar", b: "q" })).toBe( + "x://foo%2Fbar,q", + ); + }); + + it("keeps the / operator prefix and separator, and encodes", () => { + expect(expandUriTemplate("x://a{/p,q}", { p: "x y", q: "z" })).toBe( + "x://a/x%20y/z", + ); + }); + + it("keeps the . operator prefix and separator", () => { + expect(expandUriTemplate("x://a{.p,q}", { p: "x/y", q: "z" })).toBe( + "x://a.x%2Fy.z", + ); + }); + + it("keeps the # prefix and leaves reserved characters under it", () => { + expect(expandUriTemplate("x://a{#p,q}", { p: "x/y", q: "z" })).toBe( + "x://a#x/y,z", + ); + }); + + it("leaves reserved characters under the + operator", () => { + expect(expandUriTemplate("x://{+a,b}", { a: "x/y", b: "z" })).toBe( + "x://x/y,z", + ); + }); + + it("drops only the undefined names, keeping the rest", () => { + expect(expandUriTemplate("x://a{/p,q}", { q: "z" })).toBe("x://a/z"); + }); + + it("omits the whole expression when no name has a value", () => { + expect(expandUriTemplate("x://a{/p,q}", {})).toBe("x://a"); + }); + + it("leaves multi-name query expressions to the SDK, which handles them", () => { + expect(expandUriTemplate("x://a{?p,q}", { p: "x/y", q: "z" })).toBe( + "x://a?p=x%2Fy&q=z", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index b337a42b5..7a1ead921 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -1,126 +1,5 @@ import { describe, it, expect } from "vitest"; -import { - expandUriTemplate, - parseUriTemplate, - previewUriTemplate, - templateVariables, -} from "./uriTemplate"; - -describe("parseUriTemplate", () => { - it("splits literals from expressions", () => { - expect(parseUriTemplate("foobar://events/{topic}")).toEqual([ - { kind: "literal", text: "foobar://events/" }, - { - kind: "expression", - source: "{topic}", - operator: "", - names: ["topic"], - }, - ]); - }); - - it("reads the operator and the comma-separated name list", () => { - expect(parseUriTemplate("x://{?a,b*}")).toEqual([ - { kind: "literal", text: "x://" }, - { - kind: "expression", - source: "{?a,b*}", - operator: "?", - names: ["a", "b"], - }, - ]); - }); - - it("treats an unclosed expression as trailing literal text", () => { - expect(parseUriTemplate("x://a/{oops")).toEqual([ - { kind: "literal", text: "x://a/{oops" }, - ]); - }); - - it("returns nothing for an empty template", () => { - expect(parseUriTemplate("")).toEqual([]); - }); -}); - -describe("templateVariables", () => { - it("finds a simple variable and marks it required", () => { - expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true }, - ]); - }); - - it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { - expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false }, - ]); - }); - - it.each([ - ["{+path}", "+", true], - ["{#frag}", "#", true], - ["{.label}", ".", false], - ["{/segment}", "/", false], - ["{&extra}", "&", false], - ])("classifies %s", (expression, operator, required) => { - const [variable] = templateVariables(`x://a${expression}`); - expect(variable.operator).toBe(operator); - expect(variable.required).toBe(required); - }); - - it("deduplicates a repeated name and keeps it required if any use is", () => { - expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true }, - ]); - }); - - it("returns an empty list for a template with no expressions", () => { - expect(templateVariables("file:///static.txt")).toEqual([]); - }); -}); - -describe("expandUriTemplate", () => { - it("percent-encodes a reserved character in a simple variable (#1919)", () => { - expect( - expandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), - ).toBe("foobar://events/foo%2Fbar"); - }); - - it.each([ - ["?", "a?b", "a%3Fb"], - ["#", "a#b", "a%23b"], - ["%", "a%b", "a%25b"], - ["space", "a b", "a%20b"], - ["unicode", "caffè", "caff%C3%A8"], - ])("encodes %s", (_label, value, encoded) => { - expect(expandUriTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); - }); - - it("builds an encoded query expression", () => { - expect( - expandUriTemplate("foobar://events{?topic}", { topic: "foo/bar" }), - ).toBe("foobar://events?topic=foo%2Fbar"); - }); - - it("leaves reserved characters intact under the + operator", () => { - expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); - }); - - it("omits an expression whose variable was left blank", () => { - expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( - "foobar://events", - ); - }); - - it("joins two query expressions with & rather than a second ?", () => { - expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( - "x://a?one=1&two=2", - ); - }); - - it("falls back to the raw template when the SDK cannot parse it", () => { - expect(expandUriTemplate("x://a/{oops", { oops: "v" })).toBe("x://a/{oops"); - }); -}); +import { previewUriTemplate } from "./uriTemplate"; describe("previewUriTemplate", () => { it("shows the template verbatim before anything is entered", () => { @@ -166,3 +45,13 @@ describe("previewUriTemplate", () => { expect(previewUriTemplate("x://a/{oops", {})).toBe("x://a/{oops"); }); }); + +describe("previewUriTemplate - multi-name expressions", () => { + it("applies the same correction the real expansion does", () => { + // Must match expandUriTemplate("x://{a,b}", ...) exactly, or the preview + // would promise a URI that submitting does not send. + expect(previewUriTemplate("x://{a,b}", { a: "foo/bar", b: "q" })).toBe( + "x://foo%2Fbar,q", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 25cd027eb..4c3f8ea2c 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -1,167 +1,29 @@ /** - * RFC 6570 URI Template support for the Resources screen. + * The web Resources form's view of an RFC 6570 URI template (#1919). * - * The web client used to discover and substitute variables with a bare - * `/\{(\w+)\}/g` regex, which is wrong in two ways (#1919): it cannot see an - * expression carrying an operator (`{?topic}`, `{/path}`, `{#frag}`, ...), so - * no input is rendered for it; and it splices the raw value in, so a `/`, `?`, - * `#` or space in a simple `{var}` lands unencoded and silently changes the - * URI's structure. - * - * Expansion itself is delegated to the SDK's `UriTemplate` -- the same - * implementation `InspectorClient.readResourceFromTemplate` (and therefore the - * TUI) already expands through, so the two clients cannot disagree about what a - * template means. What lives here is the surrounding form/preview logic the SDK - * class does not provide: which variables to render an input for, which of them - * a read cannot proceed without, and a partially-expanded preview string. + * Parsing, variable classification, and expansion live in + * `@inspector/core/mcp/uriTemplate.js` so the web form, the TUI, and the CLI + * cannot disagree about what a template means -- they are re-exported here so + * the panel has a single import. What this module adds is the one piece that is + * purely a display concern: the partially-expanded preview string. */ +import { + applyMultiNameCorrection, + definedValues, + parseUriTemplate, +} from "@inspector/core/mcp/uriTemplate.js"; import { UriTemplate } from "@modelcontextprotocol/client"; -/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ -const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; - -/** - * Operators whose expansion omits cleanly when the variables in it are - * undefined -- the whole expression, separator included, simply disappears. - * A variable under any *other* operator is interpolated into the middle of the - * URI, so leaving it out produces a different resource path rather than a - * shorter one; those we require before allowing a read. - */ -const OMITTABLE_OPERATORS = new Set([".", "/", "?", "&"]); - -interface TemplateLiteral { - kind: "literal"; - text: string; -} - -interface TemplateExpression { - kind: "expression"; - /** The expression including its braces, e.g. `{?topic}`. */ - source: string; - /** The RFC 6570 operator, or `""` for a simple expression. */ - operator: string; - /** Variable names in the expression, `*` (explode) and whitespace stripped. */ - names: string[]; -} - -export type TemplatePart = TemplateLiteral | TemplateExpression; - -export interface TemplateVariable { - name: string; - /** The operator of the expression the variable was first seen in. */ - operator: string; - /** - * True when omitting the variable would change the URI's structure rather - * than shorten it -- see {@link OMITTABLE_OPERATORS}. - */ - required: boolean; -} - -/** - * Splits a template into literal runs and expressions. - * - * Deliberately mirrors the SDK parser's own scanning rules (first matching - * operator character wins, names split on `,`, `*` stripped) so this never - * disagrees with the class that ultimately does the expanding. An unclosed - * `{` yields a trailing literal, which is what makes the callers below degrade - * to "render no inputs, show the template verbatim" rather than throw. - */ -export function parseUriTemplate(uriTemplate: string): TemplatePart[] { - const parts: TemplatePart[] = []; - let literal = ""; - let i = 0; - - while (i < uriTemplate.length) { - if (uriTemplate[i] !== "{") { - literal += uriTemplate[i]; - i += 1; - continue; - } - const end = uriTemplate.indexOf("}", i); - if (end === -1) { - // Unclosed expression -- the rest is not a template, treat it as text. - literal += uriTemplate.slice(i); - break; - } - if (literal) { - parts.push({ kind: "literal", text: literal }); - literal = ""; - } - const body = uriTemplate.slice(i + 1, end); - const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const names = body - .slice(operator.length) - .split(",") - .map((name) => name.replace("*", "").trim()) - .filter((name) => name.length > 0); - parts.push({ - kind: "expression", - source: uriTemplate.slice(i, end + 1), - operator, - names, - }); - i = end + 1; - } - - if (literal) parts.push({ kind: "literal", text: literal }); - return parts; -} - -/** - * The variables a form should render an input for, in template order and - * deduplicated by name. A name appearing under more than one operator is - * required if *any* of its occurrences is. - */ -export function templateVariables(uriTemplate: string): TemplateVariable[] { - const byName = new Map(); - - for (const part of parseUriTemplate(uriTemplate)) { - if (part.kind !== "expression") continue; - const required = !OMITTABLE_OPERATORS.has(part.operator); - for (const name of part.names) { - const existing = byName.get(name); - if (existing) { - existing.required = existing.required || required; - } else { - byName.set(name, { name, operator: part.operator, required }); - } - } - } - - return [...byName.values()]; -} - -/** - * Drops empty entries so an untouched optional field reads as *undefined* to - * the SDK (the expression disappears) rather than as the empty string (which - * would expand to a valueless `?topic=`). - */ -function definedValues(values: Record): Record { - return Object.fromEntries( - Object.entries(values).filter(([, value]) => value.length > 0), - ); -} - -/** - * Expands a template against the entered values per RFC 6570 -- percent- - * encoding each value according to its operator, and omitting expressions whose - * variables were left blank. - * - * A template the SDK refuses to parse falls back to the raw template string, - * which is what the user already sees in the preview and what the server will - * reject with a legible error; throwing here would take out the whole panel. - */ -export function expandUriTemplate( - uriTemplate: string, - values: Record, -): string { - try { - return new UriTemplate(uriTemplate).expand(definedValues(values)); - } catch { - return uriTemplate; - } -} +export { + expandUriTemplate, + parseUriTemplate, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; +export type { + TemplatePart, + TemplateVariable, +} from "@inspector/core/mcp/uriTemplate.js"; /** * A placeholder standing in for an expression the user has not filled in yet. @@ -174,8 +36,8 @@ const deferredToken = (index: number) => `\u0000${index}\u0000`; /** * A partially-expanded template for display: expressions whose variables are - * all filled are expanded exactly as {@link expandUriTemplate} would, and the - * rest are left standing as written so the user can see what is still needed. + * all filled are expanded exactly as `expandUriTemplate` would, and the rest + * are left standing as written so the user can see what is still needed. * * Unfilled expressions are swapped for an inert token and restored after * expansion -- rather than expanding each filled expression in isolation -- so @@ -194,7 +56,10 @@ export function previewUriTemplate( .map((part) => { if (part.kind === "literal") return part.text; if (part.names.every((name) => defined[name] !== undefined)) { - return part.source; + // Route the kept expression through the same multi-name correction the + // real expansion applies, so the preview can never promise a URI that + // submitting would not actually send. + return applyMultiNameCorrection([part], defined); } deferred.push(part.source); return deferredToken(deferred.length - 1); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index a635582d0..9cdedd1cb 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -191,6 +191,11 @@ import { convertPromptArguments, } from "../json/jsonUtils.js"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + applyMultiNameCorrection, + definedValues, + parseUriTemplate, +} from "./uriTemplate.js"; import { InspectorClientEventTarget, type TaskWithOptionalCreatedAt, @@ -4971,11 +4976,20 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; - // Expand the template's uriTemplate using the provided params + // Expand the template's uriTemplate using the provided params. Routed + // through the shared helpers in ./uriTemplate.js so this and the web + // Resources form cannot disagree: `definedValues` makes a blank field read + // as *undefined* (the expression drops out) rather than as the empty string + // (a valueless `?topic=`), and `applyMultiNameCorrection` fixes the SDK's + // mis-expansion of `{a,b}`-style expressions before it sees them (#1919). let expandedUri: string; try { - const uriTemplate = new UriTemplate(uriTemplateString); - expandedUri = uriTemplate.expand(params); + const defined = definedValues(params); + const corrected = applyMultiNameCorrection( + parseUriTemplate(uriTemplateString), + defined, + ); + expandedUri = new UriTemplate(corrected).expand(defined); } catch (error) { throw new Error( `Failed to expand URI template "${uriTemplate}": ${error instanceof Error ? error.message : String(error)}`, diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts new file mode 100644 index 000000000..0518b6085 --- /dev/null +++ b/core/mcp/uriTemplate.ts @@ -0,0 +1,257 @@ +/** + * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). + * + * Expansion is delegated to the SDK's `UriTemplate` — with one correction + * applied first, documented on {@link expandMultiNameExpression} below. This + * lives in `core/` rather than in a client so the web form, the TUI, and the + * CLI cannot disagree about what a template means; `InspectorClient + * .readResourceFromTemplate` and the web Resources form both expand through + * {@link expandUriTemplate}. + */ + +import { UriTemplate } from "@modelcontextprotocol/client"; + +/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ +const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; + +/** + * Operators whose expansion omits cleanly when the variables in it are + * undefined — the whole expression, separator and all, simply disappears and + * what is left is still a well-formed URI naming a real (broader) resource. + * + * The remaining operators — `""` (simple) and `+` (reserved) — interpolate the + * value into the *middle* of the URI, so omitting one leaves an empty path + * segment rather than a shorter URI: measured against the pinned SDK, + * `file:///users/{userId}/profile` expands to `file:///users//profile` and + * `x://a/{+path}` to `x://a/`, both of which name a different resource. Those + * are the ones a form must require before allowing a read. + * + * `#` is in the omittable set on the same measurement: `x://a{#frag}` with no + * `frag` expands to exactly `x://a`. A fragment is optional by construction. + */ +const OMITTABLE_OPERATORS = new Set(["#", ".", "/", "?", "&"]); + +interface TemplateLiteral { + kind: "literal"; + text: string; +} + +interface TemplateExpression { + kind: "expression"; + /** The expression including its braces, e.g. `{?topic}`. */ + source: string; + /** The RFC 6570 operator, or `""` for a simple expression. */ + operator: string; + /** Variable names in the expression, `*` (explode) and whitespace stripped. */ + names: string[]; +} + +export type TemplatePart = TemplateLiteral | TemplateExpression; + +export interface TemplateVariable { + name: string; + /** The operator of the expression the variable was first seen in. */ + operator: string; + /** + * True when omitting the variable would change the URI's structure rather + * than shorten it — see {@link OMITTABLE_OPERATORS}. + */ + required: boolean; +} + +/** + * Splits a template into literal runs and expressions. + * + * Deliberately mirrors the SDK parser's own scanning rules (first matching + * operator character wins, names split on `,`, `*` stripped) so this never + * disagrees with the class that ultimately does the expanding. An unclosed + * `{` yields a trailing literal, which is what makes the callers below degrade + * to "render no inputs, show the template verbatim" rather than throw. + */ +export function parseUriTemplate(uriTemplate: string): TemplatePart[] { + const parts: TemplatePart[] = []; + let literal = ""; + let i = 0; + + while (i < uriTemplate.length) { + if (uriTemplate[i] !== "{") { + literal += uriTemplate[i]; + i += 1; + continue; + } + const end = uriTemplate.indexOf("}", i); + if (end === -1) { + // Unclosed expression — the rest is not a template, treat it as text. + literal += uriTemplate.slice(i); + break; + } + if (literal) { + parts.push({ kind: "literal", text: literal }); + literal = ""; + } + const body = uriTemplate.slice(i + 1, end); + const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; + const names = body + .slice(operator.length) + .split(",") + .map((name) => name.replace("*", "").trim()) + .filter((name) => name.length > 0); + parts.push({ + kind: "expression", + source: uriTemplate.slice(i, end + 1), + operator, + names, + }); + i = end + 1; + } + + if (literal) parts.push({ kind: "literal", text: literal }); + return parts; +} + +/** + * The variables a form should render an input for, in template order and + * deduplicated by name. A name appearing under more than one operator is + * required if *any* of its occurrences is. + */ +export function templateVariables(uriTemplate: string): TemplateVariable[] { + const byName = new Map(); + + for (const part of parseUriTemplate(uriTemplate)) { + if (part.kind !== "expression") continue; + const required = !OMITTABLE_OPERATORS.has(part.operator); + for (const name of part.names) { + const existing = byName.get(name); + if (existing) { + existing.required = existing.required || required; + } else { + byName.set(name, { name, operator: part.operator, required }); + } + } + } + + return [...byName.values()]; +} + +/** + * Drops empty entries so an untouched optional field reads as *undefined* to + * the SDK (the expression disappears) rather than as the empty string (which + * would expand to a valueless `?topic=`). + */ +export function definedValues( + values: Record, +): Record { + return Object.fromEntries( + Object.entries(values).filter(([, value]) => value.length > 0), + ); +} + +/** The SDK's `encodeValue`: reserved characters survive under `+` and `#`. */ +function encodeValue(value: string, operator: string): string { + return operator === "+" || operator === "#" + ? encodeURI(value) + : encodeURIComponent(value); +} + +/** + * Expands a **multi-name, non-query** expression (`{a,b}`, `{/a,b}`, …). + * + * The pinned SDK gets this branch wrong: `UriTemplate.expandPart` takes an + * early `part.names.length > 1` path that raw-joins the values with `,` — + * skipping `encodeValue` *and* the operator prefix entirely. Measured against + * the pinned SDK, `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` + * (unencoded, so the slash creates a path segment — the very defect #1919 is + * about), and `x://a{/p,q}` expands to `x://ax y,z` — no leading `/`, spaces + * intact. Only the `?`/`&` operators are handled correctly there, because they + * are dispatched before that branch. + * + * So those expressions are expanded here and spliced into the template as + * *literal* text before the SDK ever sees them. This is deliberately surgical: + * every other shape — the overwhelmingly common single-name expression, and + * every query expression — still goes through the SDK untouched, so the two + * cannot drift on the ordinary path, and if the SDK fixes its branch this + * correction keeps producing the same (correct) answer. + * + * Splicing is safe because both encoders escape `{` and `}` (to `%7B`/`%7D`), + * so an expanded value can never be re-parsed as an expression. + * + * Returns `""` when no name in the expression has a value, matching RFC 6570's + * rule that an expression with only undefined variables expands to nothing. + */ +function expandMultiNameExpression( + part: TemplateExpression, + values: Record, +): string { + const encoded = part.names + .map((name) => values[name]) + .filter((value) => value !== undefined) + .map((value) => encodeValue(value, part.operator)); + + if (encoded.length === 0) return ""; + + switch (part.operator) { + case "#": + return `#${encoded.join(",")}`; + case ".": + return `.${encoded.join(".")}`; + case "/": + return `/${encoded.join("/")}`; + // "" and "+" — a bare comma-joined list, no prefix. + default: + return encoded.join(","); + } +} + +/** + * True for the expressions {@link expandMultiNameExpression} has to take over: + * more than one name, and not a query operator (which the SDK dispatches before + * its broken branch and therefore handles correctly). + */ +function needsMultiNameCorrection(part: TemplateExpression): boolean { + return ( + part.names.length > 1 && part.operator !== "?" && part.operator !== "&" + ); +} + +/** + * Rebuilds `uriTemplate` with every mis-expanded multi-name expression already + * resolved to literal text, leaving the rest for the SDK. + */ +export function applyMultiNameCorrection( + parts: TemplatePart[], + values: Record, +): string { + return parts + .map((part) => { + if (part.kind === "literal") return part.text; + return needsMultiNameCorrection(part) + ? expandMultiNameExpression(part, values) + : part.source; + }) + .join(""); +} + +/** + * Expands a template against the entered values per RFC 6570 — percent-encoding + * each value according to its operator, and omitting expressions whose + * variables were left blank. + * + * A template the SDK refuses to parse falls back to the raw template string, + * which is what the user already sees in the preview and what the server will + * reject with a legible error; throwing here would take out the whole panel. + */ +export function expandUriTemplate( + uriTemplate: string, + values: Record, +): string { + const defined = definedValues(values); + try { + const corrected = applyMultiNameCorrection( + parseUriTemplate(uriTemplate), + defined, + ); + return new UriTemplate(corrected).expand(defined); + } catch { + return uriTemplate; + } +} diff --git a/test-servers/configs/rfc6570-templates-http.json b/test-servers/configs/rfc6570-templates-http.json index c6949773e..61b552c93 100644 --- a/test-servers/configs/rfc6570-templates-http.json +++ b/test-servers/configs/rfc6570-templates-http.json @@ -3,10 +3,23 @@ "name": "rfc6570-template-showcase", "version": "1.0.0" }, - "resourceTemplates": [{ "preset": "rfc6570_templates" }], - "tools": [{ "preset": "echo" }], + "resourceTemplates": [ + { + "preset": "rfc6570_templates" + } + ], + "tools": [ + { + "preset": "echo" + } + ], "transport": { "type": "streamable-http", "port": 6603 - } + }, + "resources": [ + { + "preset": "rfc6570_base" + } + ] } diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index 34fd53ad7..2def3a1a9 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -63,6 +63,7 @@ import { createUserResourceTemplate, createNumberedResourceTemplates, createRfc6570ResourceTemplates, + createRfc6570BaseResource, createSimplePrompt, createArgsPrompt, createNumberedPrompts, @@ -243,6 +244,8 @@ function resolveResourcePreset( return createNumberedResources(Number(get("count")) || 3); case "mcp_app_demo_widget": return createMcpAppDemoResource(); + case "rfc6570_base": + return createRfc6570BaseResource(); default: throw new Error(`Unknown resource preset: ${name}`); } diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 89283e494..9e58cfaa2 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1485,6 +1485,27 @@ export function createFileResourceTemplate( * Both handlers echo the URI they were matched against plus the variables the * server decoded, so the round-trip is visible in the read result. */ +/** + * The plain `foobar://events` resource that a blank `{?topic}` read lands on. + * + * Registering it is not decoration. RFC 6570 omits a query expression whose + * variable is undefined, so leaving `topic` blank legitimately requests the + * unfiltered collection -- but the SDK's own matcher cannot serve that from the + * template: `UriTemplate.partToRegExp` compiles `{?topic}` to a **required** + * `\?topic=([^&]+)`, so `match("foobar://events")` returns null and the read + * would 404. A real server would expose the collection as its own resource; + * this fixture does the same so the showcase's "read it blank" step works. + */ +export function createRfc6570BaseResource(): ResourceDefinition { + return { + uri: "foobar://events", + name: "events", + description: "All events - what a blank `{?topic}` read resolves to", + mimeType: "application/json", + text: JSON.stringify({ collection: "events", filtered: false }, null, 2), + }; +} + export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] { const echo = (label: string) => async (uri: URL, params: Record) => ({ From 25c3ad272e97960336dec630bcbaca6c1767a260 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 19:17:05 -0400 Subject: [PATCH 03/21] fix(core): support ; and :length, and scope requiredness to the expression Addresses Copilot's round-2 review on #2035. It reported "no new comments" but carried three *suppressed* ones; all three reproduced against the pinned SDK. 1. The `;` path-parameter operator is absent from the SDK's operator list, so `{;id}` parsed as a variable literally named ";id" and expanded to nothing. Added the operator and its named expansion (`;a=1;b=2`). 2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}` yielded a variable called "id:3" and expanded to nothing. Varspecs are now parsed properly and the value truncated before encoding. Truncation is by code point, since `String.prototype.slice` counts UTF-16 units and would split an astral character into a lone surrogate. For both of these the wrong URI is the lesser problem: a form has to *name* the variables it asks the user to fill, so the panel was rendering fields labelled `;id` and `id:3` that nobody could use. 3. Requiredness was applied per variable, but it is a property of the *expression*: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled expands to `a`'s value -- the SDK does this too. The panel was refusing input the expander would have accepted. `hasRequiredValues` now encodes "any one name in a required group suffices", and such a field reads "Any one of: a, b" rather than falsely claiming each is mandatory. Two structural consequences, each pinned by a test: - Takeover is now per TEMPLATE rather than per expression. Splicing corrected fragments into a template the SDK re-expands would leave its cross-expression `?`-to-`&` rewrite blind to the fragments already resolved. - Expansion is split into a strict variant that throws and a lenient one that returns the raw template. `readResourceFromTemplate` wraps the thrown error with the template name -- three pre-existing integration tests assert that -- while the form must not throw on a server-supplied template, since that would take out the panel on render. The strict variant constructs the SDK template unconditionally, because that construction is what validates syntax: otherwise `x://{;a}{b,c` would take the own-expansion path and its unclosed tail would pass as literal text with nothing objecting. Signed-off-by: cliffhall --- AGENTS.md | 13 +- README.md | 12 +- .../ResourceTemplatePanel.tsx | 24 +- .../web/src/test/core/mcp/uriTemplate.test.ts | 168 ++++++++- clients/web/src/utils/uriTemplate.test.ts | 10 +- clients/web/src/utils/uriTemplate.ts | 25 +- core/mcp/inspectorClient.ts | 26 +- core/mcp/uriTemplate.ts | 328 +++++++++++++----- 8 files changed, 467 insertions(+), 139 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5005d6d87..527eccbc9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,9 +77,16 @@ v2/main/ │ │ # shared by the web Resources form and │ │ # readResourceFromTemplate (TUI + CLI), so the │ │ # clients cannot drift on what a template means; -│ │ # delegates to the SDK's UriTemplate but corrects -│ │ # its multi-name `{a,b}` branch, which skips both -│ │ # encoding and the operator prefix — #1919; +│ │ # delegates to the SDK's UriTemplate for what it +│ │ # gets right, and takes over a whole template +│ │ # containing any of the three shapes it does not: +│ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), +│ │ # `{;id}` (operator absent from its list), and +│ │ # `{id:3}` (prefix modifier folded into the name) — +│ │ # the last two would render form fields literally +│ │ # labelled `;id` / `id:3`. Requiredness is per +│ │ # EXPRESSION, not per variable (hasRequiredValues): +│ │ # `{a,b}` with only `a` filled is expandable — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index 0bc30868b..f274f4118 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,17 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` with one correction applied first: the SDK's `expandPart` takes an early `names.length > 1` branch that raw-joins the values, skipping both percent-encoding **and** the operator prefix, so `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` and `x://a{/p,q}` to `x://ax y,z`. Those expressions are expanded by the helper and spliced in as literal text before the SDK sees them; every single-name and query expression still goes through the SDK untouched. +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): + +| Shape | SDK `variableNames` | SDK expansion | Correct | +| --- | --- | --- | --- | +| `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, operator prefix dropped | `foo%2Fbar,q` | +| `{;id}` | `[";id"]` | `""` — the `;` operator is not in its list | `;id=7` | +| `{id:3}` | `["id:3"]` | `""` — the prefix modifier is folded into the name | `abc` | + +The last two matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render fields literally labelled `;id` and `id:3`. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. + +One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. #### Advertised extensions diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index eafd16f47..f2ac2b8e0 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -16,6 +16,7 @@ import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge" import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { expandUriTemplate, + hasRequiredValues, previewUriTemplate, templateVariables, } from "../../../utils/uriTemplate"; @@ -215,12 +216,12 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - // Only the variables whose absence would change the URI's shape gate the + // Only the expressions whose absence would change the URI's shape gate the // read; an unfilled `{?topic}` is a legitimate request for the unfiltered - // resource, and RFC 6570 drops the whole expression for it. - const canSubmit = declaredVariables.every( - (v) => !v.required || variables[v.name]?.length > 0, - ); + // resource, and RFC 6570 drops the whole expression for it. The rule is + // per-expression rather than per-variable -- `{a,b}` with only `a` filled + // expands to `a`'s value -- so it lives in core beside the expander. + const canSubmit = hasRequiredValues(declaredVariables, variables); function handleSubmit() { onReadResource(expandUriTemplate(uriTemplate, variables)); @@ -239,12 +240,19 @@ export function ResourceTemplatePanel({ {description && {description}} - {declaredVariables.map(({ name: varName, required }) => { + {declaredVariables.map(({ name: varName, required, groupNames }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; // RFC 6570 omits an undefined variable under a query/path-segment - // operator entirely, so those fields are genuinely optional. - const description = required ? undefined : "Optional"; + // operator entirely, so those fields are genuinely optional. In a + // required multi-name expression no single field is mandatory either + // -- any one of them satisfies it -- so say which, rather than + // marking each one required and blocking valid input. + const description = !required + ? "Optional" + : groupNames.length > 1 + ? `Any one of: ${groupNames.join(", ")}` + : undefined; return useAutocomplete ? ( { kind: "expression", source: "{topic}", operator: "", + varspecs: [{ name: "topic" }], names: ["topic"], }, ]); @@ -25,6 +28,7 @@ describe("parseUriTemplate", () => { kind: "expression", source: "{?a,b*}", operator: "?", + varspecs: [{ name: "a" }, { name: "b" }], names: ["a", "b"], }, ]); @@ -44,13 +48,13 @@ describe("parseUriTemplate", () => { describe("templateVariables", () => { it("finds a simple variable and marks it required", () => { expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true }, + { name: "topic", operator: "", required: true, groupNames: ["topic"] }, ]); }); it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false }, + { name: "topic", operator: "?", required: false, groupNames: ["topic"] }, ]); }); @@ -72,7 +76,7 @@ describe("templateVariables", () => { it("deduplicates a repeated name and keeps it required if any use is", () => { expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true }, + { name: "id", operator: "?", required: true, groupNames: ["id"] }, ]); }); @@ -192,3 +196,161 @@ describe("expandUriTemplate - multi-name expressions (SDK correction)", () => { ); }); }); + +describe("varspec modifiers", () => { + // The pinned SDK folds a `:length` modifier into the variable name -- + // `new UriTemplate("x://a/{id:3}").variableNames` is `["id:3"]`, and it + // expands to "x://a/" -- so a form built on it would render a field the + // user cannot usefully fill. + it("parses a prefix modifier off the variable name", () => { + expect(templateVariables("x://a/{id:3}")).toEqual([ + { name: "id", operator: "", required: true, groupNames: ["id"] }, + ]); + }); + + it("truncates the value to the prefix length before encoding", () => { + expect(expandUriTemplate("x://a/{id:3}", { id: "abcdef" })).toBe( + "x://a/abc", + ); + }); + + it("encodes what survives truncation", () => { + expect(expandUriTemplate("x://a/{id:3}", { id: "a/bcdef" })).toBe( + "x://a/a%2Fb", + ); + }); + + it("truncates by code point, never splitting an astral character", () => { + // "\u{1F600}" is one code point but two UTF-16 units, so a naive + // `slice(0, 1)` would emit a lone surrogate. + expect(expandUriTemplate("x://{v:1}", { v: "\u{1F600}x" })).toBe( + `x://${encodeURIComponent("\u{1F600}")}`, + ); + }); + + it("ignores a malformed modifier rather than inventing a truncation", () => { + expect(templateVariables("x://{id:}")[0].name).toBe("id"); + expect(expandUriTemplate("x://{id:}", { id: "abcdef" })).toBe("x://abcdef"); + }); + + it("strips the explode modifier from the name", () => { + expect(templateVariables("x://{id*}")[0].name).toBe("id"); + }); +}); + +describe("the ; (path-parameter) operator", () => { + // Absent from the SDK's operator list entirely: it parses `{;id}` as a + // variable named ";id" and expands to "". + it("is recognised as an operator, not part of the name", () => { + expect(templateVariables("x://a{;id}")).toEqual([ + { name: "id", operator: ";", required: false, groupNames: ["id"] }, + ]); + }); + + it("expands to a named path parameter", () => { + expect(expandUriTemplate("x://a{;id}", { id: "7" })).toBe("x://a;id=7"); + }); + + it("repeats its separator per pair", () => { + expect(expandUriTemplate("x://a{;a,b}", { a: "1", b: "2" })).toBe( + "x://a;a=1;b=2", + ); + }); + + it("encodes the value", () => { + expect(expandUriTemplate("x://a{;p}", { p: "x/y" })).toBe("x://a;p=x%2Fy"); + }); + + it("omits cleanly when undefined", () => { + expect(expandUriTemplate("x://a{;id}", {})).toBe("x://a"); + }); +}); + +describe("hasRequiredValues", () => { + // A required *expression* is satisfied by any one of its names, because + // RFC 6570 drops the undefined ones -- verified against the SDK: + // `x://{a,b}` with only `a` expands to "x://only-a". + it("accepts a multi-name expression with only one name filled", () => { + const vars = templateVariables("x://{a,b}"); + expect(hasRequiredValues(vars, { a: "only-a", b: "" })).toBe(true); + expect(expandUriTemplate("x://{a,b}", { a: "only-a", b: "" })).toBe( + "x://only-a", + ); + }); + + it("rejects a multi-name expression with nothing filled", () => { + const vars = templateVariables("x://{a,b}"); + expect(hasRequiredValues(vars, { a: "", b: "" })).toBe(false); + }); + + it("still requires a lone required variable", () => { + const vars = templateVariables("file:///users/{userId}/profile"); + expect(hasRequiredValues(vars, { userId: "" })).toBe(false); + expect(hasRequiredValues(vars, { userId: "alice" })).toBe(true); + }); + + it("never blocks on an omittable expression", () => { + const vars = templateVariables("foobar://events{?topic}"); + expect(hasRequiredValues(vars, { topic: "" })).toBe(true); + }); + + it("is satisfied by a template with no variables at all", () => { + expect(hasRequiredValues(templateVariables("file:///static.txt"), {})).toBe( + true, + ); + }); +}); + +describe("cross-expression query joining", () => { + it("rewrites a second ? to & on the own-expansion path too", () => { + // Forced onto the own-expansion path by the `;` expression; the `?`-to-`&` + // rewrite must still apply, exactly as the SDK does it. + expect( + expandUriTemplate("x://a{;k}{?one}{?two}", { + k: "v", + one: "1", + two: "2", + }), + ).toBe("x://a;k=v?one=1&two=2"); + }); + + it("uses ? for the first query expression that actually emits", () => { + expect( + expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", two: "2" }), + ).toBe("x://a;k=v?two=2"); + }); +}); + +describe("strict vs lenient expansion", () => { + // `readResourceFromTemplate` wraps the thrown error with the template name; + // the web panel instead needs the raw template back, because an invalid + // template comes from the server and throwing would take out the panel. + it.each(["file:///{unclosed", "{a,b,c"])( + "strict throws on the invalid template %s", + (template) => { + expect(() => expandUriTemplateStrict(template, { x: "1" })).toThrow(); + }, + ); + + it.each(["file:///{unclosed", "{a,b,c"])( + "lenient returns %s unchanged", + (template) => { + expect(expandUriTemplate(template, { x: "1" })).toBe(template); + }, + ); + + it("validates syntax even when taking the own-expansion path", () => { + // `{;a}` forces own-expansion, and this module's parser treats the + // unclosed tail as literal text -- so without the unconditional SDK + // construction nothing would reject this. + expect(() => expandUriTemplateStrict("x://{;a}{b,c", { a: "1" })).toThrow(); + }); + + it("agrees with the lenient variant on a valid template", () => { + const template = "foobar://events{?topic}"; + const values = { topic: "foo/bar" }; + expect(expandUriTemplateStrict(template, values)).toBe( + expandUriTemplate(template, values), + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 7a1ead921..8a50ff7e7 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -23,12 +23,18 @@ describe("previewUriTemplate", () => { ).toBe("foobar://events/foo%2Fbar"); }); - it("keeps a multi-name expression whole until every name is filled", () => { + it("expands a partially-filled multi-name expression, as submitting would", () => { + // RFC 6570 drops the undefined names rather than the whole expression, so + // showing `{?one,two}` here would promise a URI the submit does not send. expect(previewUriTemplate("x://a{?one,two}", { one: "1" })).toBe( - "x://a{?one,two}", + "x://a?one=1", ); }); + it("keeps a multi-name expression whole while none of its names is filled", () => { + expect(previewUriTemplate("x://a{?one,two}", {})).toBe("x://a{?one,two}"); + }); + it("still rewrites the second ? to & when both query expressions resolve", () => { expect( previewUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" }), diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 4c3f8ea2c..0a14a33d4 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -9,20 +9,21 @@ */ import { - applyMultiNameCorrection, definedValues, + expandUriTemplate, parseUriTemplate, } from "@inspector/core/mcp/uriTemplate.js"; -import { UriTemplate } from "@modelcontextprotocol/client"; export { expandUriTemplate, + hasRequiredValues, parseUriTemplate, templateVariables, } from "@inspector/core/mcp/uriTemplate.js"; export type { TemplatePart, TemplateVariable, + VarSpec, } from "@inspector/core/mcp/uriTemplate.js"; /** @@ -41,8 +42,10 @@ const deferredToken = (index: number) => `\u0000${index}\u0000`; * * Unfilled expressions are swapped for an inert token and restored after * expansion -- rather than expanding each filled expression in isolation -- so - * the SDK still sees one whole template and applies its cross-expression rules - * (notably rewriting a second `?` query expression to `&`). + * the expander still sees one whole template and applies its cross-expression + * rules (notably rewriting a second `?` query expression to `&`). Routing the + * rewritten template back through `expandUriTemplate` is what keeps the preview + * honest: it can never promise a URI that submitting would not send. */ export function previewUriTemplate( uriTemplate: string, @@ -55,23 +58,15 @@ export function previewUriTemplate( const rewritten = parts .map((part) => { if (part.kind === "literal") return part.text; - if (part.names.every((name) => defined[name] !== undefined)) { - // Route the kept expression through the same multi-name correction the - // real expansion applies, so the preview can never promise a URI that - // submitting would not actually send. - return applyMultiNameCorrection([part], defined); + if (part.names.some((name) => defined[name] !== undefined)) { + return part.source; } deferred.push(part.source); return deferredToken(deferred.length - 1); }) .join(""); - let expanded: string; - try { - expanded = new UriTemplate(rewritten).expand(defined); - } catch { - return uriTemplate; - } + const expanded = expandUriTemplate(rewritten, defined); // Restored by exact-string replacement rather than by a pattern: a regex // matching the token would have to embed U+0000 literally, which `eslint` diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 9cdedd1cb..2a98dc2f8 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -190,12 +190,7 @@ import { convertToolParameters, convertPromptArguments, } from "../json/jsonUtils.js"; -import { UriTemplate } from "@modelcontextprotocol/client"; -import { - applyMultiNameCorrection, - definedValues, - parseUriTemplate, -} from "./uriTemplate.js"; +import { expandUriTemplateStrict } from "./uriTemplate.js"; import { InspectorClientEventTarget, type TaskWithOptionalCreatedAt, @@ -4976,20 +4971,15 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; - // Expand the template's uriTemplate using the provided params. Routed - // through the shared helpers in ./uriTemplate.js so this and the web - // Resources form cannot disagree: `definedValues` makes a blank field read - // as *undefined* (the expression drops out) rather than as the empty string - // (a valueless `?topic=`), and `applyMultiNameCorrection` fixes the SDK's - // mis-expansion of `{a,b}`-style expressions before it sees them (#1919). + // Expand through the shared helper in ./uriTemplate.js so this and the web + // Resources form cannot disagree. It drops blank values so an unfilled + // optional field reads as *undefined* (the expression disappears) rather + // than as the empty string (a valueless `?topic=`), and it covers the + // expression shapes the SDK's own expander gets wrong -- `{a,b}`, `{;id}`, + // and the `{id:3}` prefix modifier (#1919). let expandedUri: string; try { - const defined = definedValues(params); - const corrected = applyMultiNameCorrection( - parseUriTemplate(uriTemplateString), - defined, - ); - expandedUri = new UriTemplate(corrected).expand(defined); + expandedUri = expandUriTemplateStrict(uriTemplateString, params); } catch (error) { throw new Error( `Failed to expand URI template "${uriTemplate}": ${error instanceof Error ? error.message : String(error)}`, diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 0518b6085..28afd7e09 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -1,18 +1,45 @@ /** * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * - * Expansion is delegated to the SDK's `UriTemplate` — with one correction - * applied first, documented on {@link expandMultiNameExpression} below. This - * lives in `core/` rather than in a client so the web form, the TUI, and the - * CLI cannot disagree about what a template means; `InspectorClient - * .readResourceFromTemplate` and the web Resources form both expand through - * {@link expandUriTemplate}. + * This lives in `core/` rather than in a client so the web Resources form, the + * TUI, and the CLI cannot disagree about what a template means: the form calls + * {@link templateVariables} / {@link expandUriTemplate} directly, and the TUI + * and CLI reach the same code through `InspectorClient.readResourceFromTemplate`. + * + * ## Why this is not simply `new UriTemplate(t).expand(v)` + * + * Expansion is delegated to the SDK's `UriTemplate` for every expression it + * handles correctly — which is the overwhelmingly common case, and keeping it + * there means we cannot drift from the SDK on the ordinary path. But its parser + * and expander are incomplete in three ways that a *form* makes visible, + * because a form has to name the variables it is asking the user to fill in. + * Each was measured against the pinned SDK, not inferred: + * + * | Shape | SDK `variableNames` | SDK expansion | Correct | + * | ------------ | ------------------- | --------------------------------- | -------------- | + * | `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, no prefix| `foo%2Fbar,q` | + * | `{;id}` | `[";id"]` | `""` — operator unknown | `;id=7` | + * | `{id:3}` | `["id:3"]` | `""` — modifier folded into name | `abc` | + * + * For the last two the damage is not just a wrong URI: the form would render + * fields literally labelled `;id` and `id:3`, which the user cannot fill in + * usefully. So this module parses varspecs properly and, **when a template + * contains any expression the SDK gets wrong, expands that whole template + * itself** in {@link expandParts} rather than splicing corrected fragments into + * a template the SDK then re-expands — splicing would leave the SDK's + * cross-expression `?`-to-`&` rewrite unaware of the fragments we resolved. */ import { UriTemplate } from "@modelcontextprotocol/client"; -/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ -const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; +/** + * The RFC 6570 operators. + * + * `;` is here but **not** in the SDK's own list, which is why `{;id}` parses + * there as a variable literally named `;id`. Order matters only in that each is + * a distinct single character; the first match wins. + */ +const OPERATORS = ["+", "#", ".", "/", ";", "?", "&"] as const; /** * Operators whose expansion omits cleanly when the variables in it are @@ -29,7 +56,20 @@ const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; * `#` is in the omittable set on the same measurement: `x://a{#frag}` with no * `frag` expands to exactly `x://a`. A fragment is optional by construction. */ -const OMITTABLE_OPERATORS = new Set(["#", ".", "/", "?", "&"]); +const OMITTABLE_OPERATORS = new Set(["#", ".", "/", ";", "?", "&"]); + +/** Operators that expand to `name=value` pairs rather than bare values. */ +const NAMED_OPERATORS = new Set([";", "?", "&"]); + +/** A single variable reference inside an expression, e.g. `id` or `id:3`. */ +export interface VarSpec { + name: string; + /** + * The RFC 6570 prefix modifier (`{id:3}`), a maximum length in *characters*. + * Applied to the value before percent-encoding, per §3.2.1. + */ + maxLength?: number; +} interface TemplateLiteral { kind: "literal"; @@ -42,7 +82,9 @@ interface TemplateExpression { source: string; /** The RFC 6570 operator, or `""` for a simple expression. */ operator: string; - /** Variable names in the expression, `*` (explode) and whitespace stripped. */ + /** The variable references, in order. */ + varspecs: VarSpec[]; + /** Bare variable names, `*` and any `:length` modifier stripped. */ names: string[]; } @@ -53,20 +95,50 @@ export interface TemplateVariable { /** The operator of the expression the variable was first seen in. */ operator: string; /** - * True when omitting the variable would change the URI's structure rather - * than shorten it — see {@link OMITTABLE_OPERATORS}. + * True when the expression this variable belongs to cannot be omitted + * without changing the URI's structure — see {@link OMITTABLE_OPERATORS}. + * + * Note this is a property of the *expression*, not of the single variable: + * RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` + * with only `a` filled expands to `a`'s value. Use {@link hasRequiredValues} + * rather than testing every required variable individually, or a form will + * refuse input the expander would have accepted. */ required: boolean; + /** + * Every name in the expression this variable belongs to, itself included. + * A single-name expression yields a one-element array. + */ + groupNames: string[]; +} + +/** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ +function parseVarSpec(raw: string): VarSpec | null { + // 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 null; + + const colon = spec.indexOf(":"); + if (colon === -1) return { name: spec }; + + const name = spec.slice(0, colon); + const length = Number(spec.slice(colon + 1)); + // A malformed modifier (`{id:}`, `{id:abc}`) is not a valid varspec; keep the + // name and ignore the modifier rather than inventing a truncation. + if (name.length === 0) return null; + return Number.isInteger(length) && length > 0 + ? { name, maxLength: length } + : { name }; } /** * Splits a template into literal runs and expressions. * - * Deliberately mirrors the SDK parser's own scanning rules (first matching - * operator character wins, names split on `,`, `*` stripped) so this never - * disagrees with the class that ultimately does the expanding. An unclosed - * `{` yields a trailing literal, which is what makes the callers below degrade - * to "render no inputs, show the template verbatim" rather than throw. + * An unclosed `{` yields a trailing literal, which is what makes the callers + * below degrade to "render no inputs, show the template verbatim" rather than + * throw at the user. */ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { const parts: TemplatePart[] = []; @@ -91,16 +163,17 @@ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { } const body = uriTemplate.slice(i + 1, end); const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const names = body + const varspecs = body .slice(operator.length) .split(",") - .map((name) => name.replace("*", "").trim()) - .filter((name) => name.length > 0); + .map(parseVarSpec) + .filter((spec): spec is VarSpec => spec !== null); parts.push({ kind: "expression", source: uriTemplate.slice(i, end + 1), operator, - names, + varspecs, + names: varspecs.map((spec) => spec.name), }); i = end + 1; } @@ -125,7 +198,12 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { if (existing) { existing.required = existing.required || required; } else { - byName.set(name, { name, operator: part.operator, required }); + byName.set(name, { + name, + operator: part.operator, + required, + groupNames: part.names, + }); } } } @@ -134,9 +212,28 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { } /** - * Drops empty entries so an untouched optional field reads as *undefined* to - * the SDK (the expression disappears) rather than as the empty string (which - * would expand to a valueless `?topic=`). + * Whether `values` supplies everything expansion structurally needs. + * + * A required *expression* is satisfied by any one of its names having a value, + * because RFC 6570 drops the undefined ones — `{a,b}` with only `a` filled + * expands to `a`'s value, which the SDK does too. Testing each required + * variable individually would block that valid input. + */ +export function hasRequiredValues( + variables: TemplateVariable[], + values: Record, +): boolean { + return variables.every( + (variable) => + !variable.required || + variable.groupNames.some((name) => (values[name] ?? "").length > 0), + ); +} + +/** + * Drops empty entries so an untouched optional field reads as *undefined* + * (the expression disappears) rather than as the empty string (which would + * expand to a valueless `?topic=`). */ export function definedValues( values: Record, @@ -154,103 +251,156 @@ function encodeValue(value: string, operator: string): string { } /** - * Expands a **multi-name, non-query** expression (`{a,b}`, `{/a,b}`, …). - * - * The pinned SDK gets this branch wrong: `UriTemplate.expandPart` takes an - * early `part.names.length > 1` path that raw-joins the values with `,` — - * skipping `encodeValue` *and* the operator prefix entirely. Measured against - * the pinned SDK, `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` - * (unencoded, so the slash creates a path segment — the very defect #1919 is - * about), and `x://a{/p,q}` expands to `x://ax y,z` — no leading `/`, spaces - * intact. Only the `?`/`&` operators are handled correctly there, because they - * are dispatched before that branch. - * - * So those expressions are expanded here and spliced into the template as - * *literal* text before the SDK ever sees them. This is deliberately surgical: - * every other shape — the overwhelmingly common single-name expression, and - * every query expression — still goes through the SDK untouched, so the two - * cannot drift on the ordinary path, and if the SDK fixes its branch this - * correction keeps producing the same (correct) answer. - * - * Splicing is safe because both encoders escape `{` and `}` (to `%7B`/`%7D`), - * so an expanded value can never be re-parsed as an expression. + * Applies a prefix modifier, then encodes. * - * Returns `""` when no name in the expression has a value, matching RFC 6570's - * rule that an expression with only undefined variables expands to nothing. + * Truncation is by *code point* (`Array.from`), not by `slice`: RFC 6570 counts + * the prefix in characters, and `String.prototype.slice` counts UTF-16 code + * units, so it can cut an astral character in half and yield a lone surrogate. + */ +function renderValue(value: string, spec: VarSpec, operator: string): string { + const truncated = + spec.maxLength === undefined + ? value + : Array.from(value).slice(0, spec.maxLength).join(""); + return encodeValue(truncated, operator); +} + +/** + * True for an expression the SDK would get wrong, and which this module must + * therefore expand itself. See the table in the module comment. */ -function expandMultiNameExpression( +function needsOwnExpansion(part: TemplatePart): boolean { + if (part.kind !== "expression") return false; + return ( + // Multi-name, non-query: the SDK raw-joins, skipping encoding and prefix. + (part.varspecs.length > 1 && + part.operator !== "?" && + part.operator !== "&") || + // The SDK has no `;` operator at all. + part.operator === ";" || + // The SDK folds everything after a `:` into the variable name. Keyed on the + // raw source rather than on a parsed `maxLength` so a *malformed* modifier + // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and + // looks up `id`, while the SDK would look up `id:` and find nothing. + part.source.includes(":") + ); +} + +/** Expands one expression per RFC 6570. Returns "" if no name has a value. */ +function expandExpression( part: TemplateExpression, values: Record, ): string { - const encoded = part.names - .map((name) => values[name]) - .filter((value) => value !== undefined) - .map((value) => encodeValue(value, part.operator)); + const present = part.varspecs.filter( + (spec) => values[spec.name] !== undefined, + ); + if (present.length === 0) return ""; + + const { operator } = part; + + if (NAMED_OPERATORS.has(operator)) { + const pairs = present.map( + (spec) => + `${spec.name}=${renderValue(values[spec.name], spec, operator)}`, + ); + // `;` repeats its separator per pair; `?`/`&` join with `&`. + return operator === ";" + ? `;${pairs.join(";")}` + : `${operator}${pairs.join("&")}`; + } - if (encoded.length === 0) return ""; + const rendered = present.map((spec) => + renderValue(values[spec.name], spec, operator), + ); - switch (part.operator) { + switch (operator) { case "#": - return `#${encoded.join(",")}`; + return `#${rendered.join(",")}`; case ".": - return `.${encoded.join(".")}`; + return `.${rendered.join(".")}`; case "/": - return `/${encoded.join("/")}`; + return `/${rendered.join("/")}`; // "" and "+" — a bare comma-joined list, no prefix. default: - return encoded.join(","); + return rendered.join(","); } } /** - * True for the expressions {@link expandMultiNameExpression} has to take over: - * more than one name, and not a query operator (which the SDK dispatches before - * its broken branch and therefore handles correctly). + * Expands a whole parsed template, mirroring the SDK's `expand` — including its + * rule that a second query expression switches its leading `?` to `&`. */ -function needsMultiNameCorrection(part: TemplateExpression): boolean { - return ( - part.names.length > 1 && part.operator !== "?" && part.operator !== "&" - ); +export function expandParts( + parts: TemplatePart[], + values: Record, +): string { + let result = ""; + let hasQueryParam = false; + + for (const part of parts) { + if (part.kind === "literal") { + result += part.text; + continue; + } + const expanded = expandExpression(part, values); + if (!expanded) continue; + + const isQuery = part.operator === "?" || part.operator === "&"; + result += isQuery && hasQueryParam ? `&${expanded.slice(1)}` : expanded; + if (isQuery) hasQueryParam = true; + } + + return result; } /** - * Rebuilds `uriTemplate` with every mis-expanded multi-name expression already - * resolved to literal text, leaving the rest for the SDK. + * Expands a template against the entered values per RFC 6570 — percent-encoding + * each value according to its operator, and omitting expressions whose + * variables were left blank. **Throws** on a template that is not valid. + * + * Templates the SDK handles correctly still go through the SDK, so the two + * cannot drift on the ordinary path; only a template containing at least one + * expression from the table above is expanded here instead. That is a + * whole-template switch rather than a per-expression splice so the `?`-to-`&` + * rewrite always sees every expression that actually produced output. + * + * The SDK template is constructed *before* choosing a path, and unconditionally: + * that construction is what validates the syntax and throws `Unclosed template + * expression`, and callers such as `readResourceFromTemplate` wrap that error. + * Skipping it on the own-expansion path would silently accept a template like + * `{;a}{b,c` — this module's parser treats the unclosed tail as literal text, + * so nothing else would object. */ -export function applyMultiNameCorrection( - parts: TemplatePart[], +export function expandUriTemplateStrict( + uriTemplate: string, values: Record, ): string { - return parts - .map((part) => { - if (part.kind === "literal") return part.text; - return needsMultiNameCorrection(part) - ? expandMultiNameExpression(part, values) - : part.source; - }) - .join(""); + const defined = definedValues(values); + const sdkTemplate = new UriTemplate(uriTemplate); + const parts = parseUriTemplate(uriTemplate); + return parts.some(needsOwnExpansion) + ? expandParts(parts, defined) + : sdkTemplate.expand(defined); } /** - * Expands a template against the entered values per RFC 6570 — percent-encoding - * each value according to its operator, and omitting expressions whose - * variables were left blank. + * {@link expandUriTemplateStrict}, but falling back to the raw template string + * instead of throwing. * - * A template the SDK refuses to parse falls back to the raw template string, - * which is what the user already sees in the preview and what the server will - * reject with a legible error; throwing here would take out the whole panel. + * This is the form's variant: the template comes from the connected server, not + * from the user, so an invalid one is not something the user can fix from the + * panel — and what they already see in the URI preview is the raw template. + * Letting it throw would take out the whole panel on render; returning it + * unchanged sends the server a URI it rejects with a legible error instead. + * Call sites that need the failure (`readResourceFromTemplate`, which wraps it + * with the template name) use the strict variant. */ export function expandUriTemplate( uriTemplate: string, values: Record, ): string { - const defined = definedValues(values); try { - const corrected = applyMultiNameCorrection( - parseUriTemplate(uriTemplate), - defined, - ); - return new UriTemplate(corrected).expand(defined); + return expandUriTemplateStrict(uriTemplate, values); } catch { return uriTemplate; } From e7d1246c86589f76adfa5fc6a3011e6b08d5f72e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 19:35:33 -0400 Subject: [PATCH 04/21] fix(core): scope requiredness to expressions, and encode + / # per RFC 6570 Addresses Copilot's round-3 review on #2035. It again reported "no new comments" while carrying two suppressed ones; both reproduced. 1. Requiredness could not live on a variable at all. Deduplication kept the first occurrence's group, so in `x{?a}{?b}{a,b}` both names ended up required with singleton groups and the form demanded both -- while the SDK expands that template with only `a` to "x?a=11". Widening the stored group would not have been enough either: in `{a,b}{a,c}`, filling `b` and `c` satisfies both expressions, which no per-variable flag can express. So requiredness is now returned per expression by `requiredGroups`, and `hasRequiredValues` asks that each group be satisfied by any one of its names. `TemplateVariable.groupNames` is gone rather than left as a field that quietly means something narrower than it reads; `required` remains, documented as driving the "Optional" marker and nothing else. 2. `encodeURI` is not the allow-reserved encoder `+` and `#` call for. It escapes `[` and `]`, which are reserved and must survive, and it escapes `%`, so an already-encoded value is double-encoded. Measured: "[::1]" -> "%5B::1%5D" and "%2F" -> "%252F". Both corrupt the URI rather than merely over-escaping it -- an IPv6 literal or a pre-encoded path reaches the server altered, which is the same class of defect #1919 is about. Added an RFC 6570 3.2.1 encoder that preserves reserved characters and existing pct-triplets, splitting on `%XX` so a lone `%` is still encoded to `%25`, and matching with the `u` flag so an astral character is encoded whole. `+` and `#` expressions are now taken over even for a single name, so both expansion paths agree on what those operators mean. Signed-off-by: cliffhall --- AGENTS.md | 12 +- README.md | 15 ++- .../ResourceTemplatePanel.tsx | 16 ++- .../web/src/test/core/mcp/uriTemplate.test.ts | 110 +++++++++++++++--- clients/web/src/utils/uriTemplate.ts | 1 + core/mcp/uriTemplate.ts | 110 +++++++++++++----- 6 files changed, 204 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 527eccbc9..3b5df7836 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,11 +82,15 @@ v2/main/ │ │ # containing any of the three shapes it does not: │ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), │ │ # `{;id}` (operator absent from its list), and -│ │ # `{id:3}` (prefix modifier folded into the name) — -│ │ # the last two would render form fields literally +│ │ # `{id:3}` (prefix modifier folded into the name), +│ │ # and `{+v}`/`{#v}` (encodeURI mangles reserved +│ │ # `[`/`]` and double-encodes pct-triplets). The `;` +│ │ # and `:N` cases would render form fields literally │ │ # labelled `;id` / `id:3`. Requiredness is per -│ │ # EXPRESSION, not per variable (hasRequiredValues): -│ │ # `{a,b}` with only `a` filled is expandable — #1919; +│ │ # EXPRESSION, not per variable — requiredGroups + +│ │ # hasRequiredValues: `{a,b}` with only `a` filled is +│ │ # expandable, and a name recurring across +│ │ # expressions needs each group checked — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index f274f4118..af8917c5a 100644 --- a/README.md +++ b/README.md @@ -251,13 +251,16 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): -| Shape | SDK `variableNames` | SDK expansion | Correct | -| --- | --- | --- | --- | -| `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, operator prefix dropped | `foo%2Fbar,q` | -| `{;id}` | `[";id"]` | `""` — the `;` operator is not in its list | `;id=7` | -| `{id:3}` | `["id:3"]` | `""` — the prefix modifier is folded into the name | `abc` | +| Shape | SDK behavior | Correct | +| --- | --- | --- | +| `{a,b}` | `foo/bar,q` — raw-joined, unencoded, operator prefix dropped | `foo%2Fbar,q` | +| `{;id}` | `""` — the `;` operator is not in its list, so the variable parses as `;id` | `;id=7` | +| `{id:3}` | `""` — the prefix modifier is folded into the name, giving `id:3` | `abc` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | `[::1]`, `%2F` | -The last two matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render fields literally labelled `;id` and `id:3`. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. +The `;` and `:3` rows matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render 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. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. + +Requiredness is likewise 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 the 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 a per-variable flag cannot express once a name recurs across expressions. One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index f2ac2b8e0..7b39b4ad9 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -18,6 +18,7 @@ import { expandUriTemplate, hasRequiredValues, previewUriTemplate, + requiredGroups, templateVariables, } from "../../../utils/uriTemplate"; @@ -96,6 +97,10 @@ export function ResourceTemplatePanel({ () => declaredVariables.map((v) => v.name), [declaredVariables], ); + // The names of each expression that cannot be omitted. Kept separate from + // `declaredVariables` (which is deduplicated for rendering) because each + // required expression has to be satisfied on its own — see `requiredGroups`. + const groups = useMemo(() => requiredGroups(uriTemplate), [uriTemplate]); const [variables, setVariables] = useState>(() => Object.fromEntries(variableNames.map((n) => [n, ""])), @@ -221,7 +226,7 @@ export function ResourceTemplatePanel({ // resource, and RFC 6570 drops the whole expression for it. The rule is // per-expression rather than per-variable -- `{a,b}` with only `a` filled // expands to `a`'s value -- so it lives in core beside the expander. - const canSubmit = hasRequiredValues(declaredVariables, variables); + const canSubmit = hasRequiredValues(groups, variables); function handleSubmit() { onReadResource(expandUriTemplate(uriTemplate, variables)); @@ -240,7 +245,7 @@ export function ResourceTemplatePanel({ {description && {description}} - {declaredVariables.map(({ name: varName, required, groupNames }) => { + {declaredVariables.map(({ name: varName, required }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; // RFC 6570 omits an undefined variable under a query/path-segment @@ -248,10 +253,13 @@ export function ResourceTemplatePanel({ // required multi-name expression no single field is mandatory either // -- any one of them satisfies it -- so say which, rather than // marking each one required and blocking valid input. + const sharedGroup = groups.find( + (names) => names.length > 1 && names.includes(varName), + ); const description = !required ? "Optional" - : groupNames.length > 1 - ? `Any one of: ${groupNames.join(", ")}` + : sharedGroup + ? `Any one of: ${sharedGroup.join(", ")}` : undefined; return useAutocomplete ? ( { describe("templateVariables", () => { it("finds a simple variable and marks it required", () => { expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true, groupNames: ["topic"] }, + { name: "topic", operator: "", required: true }, ]); }); it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false, groupNames: ["topic"] }, + { name: "topic", operator: "?", required: false }, ]); }); @@ -76,7 +77,7 @@ describe("templateVariables", () => { it("deduplicates a repeated name and keeps it required if any use is", () => { expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true, groupNames: ["id"] }, + { name: "id", operator: "?", required: true }, ]); }); @@ -204,7 +205,7 @@ describe("varspec modifiers", () => { // user cannot usefully fill. it("parses a prefix modifier off the variable name", () => { expect(templateVariables("x://a/{id:3}")).toEqual([ - { name: "id", operator: "", required: true, groupNames: ["id"] }, + { name: "id", operator: "", required: true }, ]); }); @@ -243,7 +244,7 @@ describe("the ; (path-parameter) operator", () => { // variable named ";id" and expands to "". it("is recognised as an operator, not part of the name", () => { expect(templateVariables("x://a{;id}")).toEqual([ - { name: "id", operator: ";", required: false, groupNames: ["id"] }, + { name: "id", operator: ";", required: false }, ]); }); @@ -266,39 +267,67 @@ describe("the ; (path-parameter) operator", () => { }); }); -describe("hasRequiredValues", () => { +describe("requiredGroups / hasRequiredValues", () => { // A required *expression* is satisfied by any one of its names, because // RFC 6570 drops the undefined ones -- verified against the SDK: // `x://{a,b}` with only `a` expands to "x://only-a". it("accepts a multi-name expression with only one name filled", () => { - const vars = templateVariables("x://{a,b}"); - expect(hasRequiredValues(vars, { a: "only-a", b: "" })).toBe(true); + const groups = requiredGroups("x://{a,b}"); + expect(groups).toEqual([["a", "b"]]); + expect(hasRequiredValues(groups, { a: "only-a", b: "" })).toBe(true); expect(expandUriTemplate("x://{a,b}", { a: "only-a", b: "" })).toBe( "x://only-a", ); }); it("rejects a multi-name expression with nothing filled", () => { - const vars = templateVariables("x://{a,b}"); - expect(hasRequiredValues(vars, { a: "", b: "" })).toBe(false); + expect( + hasRequiredValues(requiredGroups("x://{a,b}"), { a: "", b: "" }), + ).toBe(false); }); it("still requires a lone required variable", () => { - const vars = templateVariables("file:///users/{userId}/profile"); - expect(hasRequiredValues(vars, { userId: "" })).toBe(false); - expect(hasRequiredValues(vars, { userId: "alice" })).toBe(true); + const groups = requiredGroups("file:///users/{userId}/profile"); + expect(hasRequiredValues(groups, { userId: "" })).toBe(false); + expect(hasRequiredValues(groups, { userId: "alice" })).toBe(true); }); it("never blocks on an omittable expression", () => { - const vars = templateVariables("foobar://events{?topic}"); - expect(hasRequiredValues(vars, { topic: "" })).toBe(true); + expect(requiredGroups("foobar://events{?topic}")).toEqual([]); + expect( + hasRequiredValues(requiredGroups("foobar://events{?topic}"), { + topic: "", + }), + ).toBe(true); }); it("is satisfied by a template with no variables at all", () => { - expect(hasRequiredValues(templateVariables("file:///static.txt"), {})).toBe( + expect(hasRequiredValues(requiredGroups("file:///static.txt"), {})).toBe( true, ); }); + + it("tracks a name that recurs under a different operator", () => { + // A per-variable model keeping only the first occurrence's group would + // mark both names required with singleton groups and refuse this input; + // the SDK expands the same template with just `a` to "x?a=11". + const groups = requiredGroups("x{?a}{?b}{a,b}"); + expect(groups).toEqual([["a", "b"]]); + expect(hasRequiredValues(groups, { a: "1", b: "" })).toBe(true); + expect(expandUriTemplate("x{?a}{?b}{a,b}", { a: "1" })).toBe("x?a=11"); + }); + + it("satisfies two required expressions sharing a name, from the others", () => { + // `{a,b}{a,c}`: filling only b and c satisfies both groups. No + // per-variable flag can express this, which is why groups are separate. + const groups = requiredGroups("x://{a,b}{a,c}"); + expect(groups).toEqual([ + ["a", "b"], + ["a", "c"], + ]); + expect(hasRequiredValues(groups, { b: "B", c: "C" })).toBe(true); + expect(hasRequiredValues(groups, { b: "B" })).toBe(false); + }); }); describe("cross-expression query joining", () => { @@ -354,3 +383,52 @@ describe("strict vs lenient expansion", () => { ); }); }); + +describe("allow-reserved encoding under + and #", () => { + // The SDK uses `encodeURI` for these operators, which corrupts two classes + // of value rather than merely over-escaping: measured, encodeURI("[::1]") + // is "%5B::1%5D" and encodeURI("%2F") is "%252F". + it.each(["+", "#"])("leaves reserved [ and ] intact under %s", (operator) => { + const prefix = operator === "#" ? "#" : ""; + expect(expandUriTemplate(`x://{${operator}v}`, { v: "[::1]" })).toBe( + `x://${prefix}[::1]`, + ); + }); + + it.each(["+", "#"])( + "does not double-encode an existing pct-triplet under %s", + (operator) => { + const prefix = operator === "#" ? "#" : ""; + expect(expandUriTemplate(`x://{${operator}v}`, { v: "a%2Fb" })).toBe( + `x://${prefix}a%2Fb`, + ); + }, + ); + + it("still encodes a lone % that is not a triplet", () => { + expect(expandUriTemplate("x://{+v}", { v: "100%" })).toBe("x://100%25"); + }); + + it("still encodes characters outside the allowed set", () => { + expect(expandUriTemplate("x://{+v}", { v: "a b" })).toBe("x://a%20b"); + }); + + it("encodes an astral character whole rather than as surrogates", () => { + expect(expandUriTemplate("x://{+v}", { v: "\u{1F600}" })).toBe( + `x://${encodeURIComponent("\u{1F600}")}`, + ); + }); + + it("applies the same encoding in a multi-name + expression", () => { + expect(expandUriTemplate("x://{+a,b}", { a: "[::1]", b: "%2F" })).toBe( + "x://[::1],%2F", + ); + }); + + it("still percent-encodes reserved characters under the simple operator", () => { + // Only + and # allow reserved through; the default path is unchanged. + expect(expandUriTemplate("x://{v}", { v: "[::1]" })).toBe( + "x://%5B%3A%3A1%5D", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 0a14a33d4..56b2b8258 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -18,6 +18,7 @@ export { expandUriTemplate, hasRequiredValues, parseUriTemplate, + requiredGroups, templateVariables, } from "@inspector/core/mcp/uriTemplate.js"; export type { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 28afd7e09..0a790cf08 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -95,21 +95,17 @@ export interface TemplateVariable { /** The operator of the expression the variable was first seen in. */ operator: string; /** - * True when the expression this variable belongs to cannot be omitted - * without changing the URI's structure — see {@link OMITTABLE_OPERATORS}. + * True when this variable appears in at least one expression that cannot be + * omitted without changing the URI's structure — see + * {@link OMITTABLE_OPERATORS}. Drives the form's "Optional" marker. * - * Note this is a property of the *expression*, not of the single variable: - * RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` - * with only `a` filled expands to `a`'s value. Use {@link hasRequiredValues} - * rather than testing every required variable individually, or a form will - * refuse input the expander would have accepted. + * It does **not** mean "the user must fill this field in". Requiredness is a + * property of the *expression*: RFC 6570 drops undefined names from a + * multi-name expression, so `{a,b}` with only `a` filled expands to `a`'s + * value. Gate submission on {@link hasRequiredValues} over + * {@link requiredGroups}, never by testing this flag per variable. */ required: boolean; - /** - * Every name in the expression this variable belongs to, itself included. - * A single-name expression yields a one-element array. - */ - groupNames: string[]; } /** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ @@ -198,12 +194,7 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { if (existing) { existing.required = existing.required || required; } else { - byName.set(name, { - name, - operator: part.operator, - required, - groupNames: part.names, - }); + byName.set(name, { name, operator: part.operator, required }); } } } @@ -212,21 +203,39 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { } /** - * Whether `values` supplies everything expansion structurally needs. + * The variable names of each expression that cannot be omitted, in template + * order — one entry per expression, not per variable. * - * A required *expression* is satisfied by any one of its names having a value, - * because RFC 6570 drops the undefined ones — `{a,b}` with only `a` filled - * expands to `a`'s value, which the SDK does too. Testing each required - * variable individually would block that valid input. + * This is deliberately *not* folded onto {@link TemplateVariable}. A name can + * appear in several expressions with different operators, and each required + * expression has to be satisfied on its own: in `x{?a}{?b}{a,b}` the only + * required expression is `{a,b}`, which either `a` or `b` satisfies (the SDK + * expands that template with just `a` to `x?a=11`), while a per-variable model + * that kept only the first occurrence's group would mark both names required + * with singleton groups and refuse it. And in `{a,b}{a,c}` — two required + * expressions sharing `a` — filling `b` and `c` satisfies both, which no + * per-variable flag can express at all. + */ +export function requiredGroups(uriTemplate: string): string[][] { + const groups: string[][] = []; + for (const part of parseUriTemplate(uriTemplate)) { + if (part.kind !== "expression") continue; + if (OMITTABLE_OPERATORS.has(part.operator)) continue; + groups.push(part.names); + } + return groups; +} + +/** + * Whether `values` supplies everything expansion structurally needs: every + * required expression has at least one of its names filled in. */ export function hasRequiredValues( - variables: TemplateVariable[], + groups: string[][], values: Record, ): boolean { - return variables.every( - (variable) => - !variable.required || - variable.groupNames.some((name) => (values[name] ?? "").length > 0), + return groups.every((names) => + names.some((name) => (values[name] ?? "").length > 0), ); } @@ -243,10 +252,45 @@ export function definedValues( ); } -/** The SDK's `encodeValue`: reserved characters survive under `+` and `#`. */ +/** + * The characters RFC 6570 leaves alone under the `+` and `#` operators: + * RFC 3986 *unreserved* plus *reserved* (gen-delims and sub-delims). + */ +const ALLOW_RESERVED = /[^A-Za-z0-9\-._~:/?#[\]@!$&'()*+,;=]/gu; + +/** + * The `allow-reserved` value encoding of RFC 6570 §3.2.1, used by `+` and `#`. + * + * The SDK reaches for `encodeURI` here, which is close but wrong twice over, + * and both cases corrupt the URI rather than merely over-escaping it: + * + * - It escapes `[` and `]`, which are *reserved* and must survive — so an IPv6 + * literal `[::1]` becomes `%5B::1%5D`. + * - It escapes `%`, so an already-encoded value is double-encoded: `%2F` + * becomes `%252F`, and the server sees a literal "%2F" rather than a slash. + * + * The spec instead keeps existing pct-triplets intact, which is what the split + * below does: odd chunks are whole `%XX` triplets and pass through untouched, + * even chunks are scanned for anything outside the allowed set. A lone `%` is + * not a triplet, so it lands in an even chunk and is correctly encoded to + * `%25`. The `u` flag makes the class match by code point, so an astral + * character is handed to `encodeURIComponent` whole rather than as surrogates. + */ +function encodeAllowReserved(value: string): string { + return value + .split(/(%[0-9A-Fa-f]{2})/g) + .map((chunk, index) => + index % 2 === 1 + ? chunk + : chunk.replace(ALLOW_RESERVED, (char) => encodeURIComponent(char)), + ) + .join(""); +} + +/** Encodes one value for its operator: reserved characters survive `+` and `#`. */ function encodeValue(value: string, operator: string): string { return operator === "+" || operator === "#" - ? encodeURI(value) + ? encodeAllowReserved(value) : encodeURIComponent(value); } @@ -278,6 +322,12 @@ function needsOwnExpansion(part: TemplatePart): boolean { part.operator !== "&") || // The SDK has no `;` operator at all. part.operator === ";" || + // The SDK encodes `+`/`#` values with `encodeURI`, which mangles reserved + // `[`/`]` and double-encodes existing pct-triplets — see + // `encodeAllowReserved`. Taken over even for a single name so both paths + // agree on what these operators mean. + part.operator === "+" || + part.operator === "#" || // The SDK folds everything after a `:` into the variable name. Keyed on the // raw source rather than on a parsed `maxLength` so a *malformed* modifier // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and From 935113d1e06211a9d2e6bd6b33269981581ac27d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:04:20 -0400 Subject: [PATCH 05/21] fix: encode per RFC 3986, and derive the TUI form from the shared parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot's round-4 review on #2035 — again reported as "no new comments" while carrying three suppressed ones. All three reproduced. 1. The TUI form still named its fields from the SDK. `uriTemplateToForm` read `UriTemplate.variableNames`, which mangles a name: `{;id}` yields ";id" and `{id:3}` yields "id:3". The form therefore submitted `{ ";id": "7" }` while the shared expander looked up `id`, found nothing, and dropped the expression — the value vanished silently. This is the finding that mattered most, because it falsified the claim this change is built on. Moving the expander into core only makes the clients agree if each client's FORM derives its names from the same parser: a form submits under the names it rendered. The TUI now reads `templateVariables`, and the SDK template is constructed only to validate, preserving the existing malformed-template diagnostic. Its `required` flag comes from `requiredGroups`, and only a variable that is the sole member of a non-omittable expression is marked: ink- form cannot express "any one of these", so marking every member of `{a,b}` required would refuse input the expander accepts. 2. `encodeURIComponent` is not RFC 3986's unreserved set — it leaves the sub-delims !'()* bare, which RFC 6570 requires encoded for every operator except + and #. Fixing that settled a design question left open in the previous round. With two encoders, the SAME value encoded differently depending on whether its expression happened to carry a modifier, since only the modifier pushed it onto our path. So delegation is gone entirely: one expander, one set of rules. The SDK's `UriTemplate` is still constructed, but only to validate a template. 3. The "Any one of: a, b" hint could contradict the disabled submit button. A name can sit in a singleton required group AND a shared one (`x://{a}/{a,b}`), where the singleton demands that exact field. The hint is suppressed in that case. Signed-off-by: cliffhall --- AGENTS.md | 34 +++--- README.md | 21 ++-- .../tui/__tests__/uriTemplateToForm.test.ts | 33 +++++- clients/tui/src/utils/uriTemplateToForm.ts | 52 ++++++--- .../ResourceTemplatePanel.tsx | 13 ++- .../web/src/test/core/mcp/uriTemplate.test.ts | 33 ++++++ core/mcp/uriTemplate.ts | 106 +++++++++--------- 7 files changed, 190 insertions(+), 102 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3b5df7836..ccb2935c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,24 +73,22 @@ v2/main/ │ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores -│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand -│ │ # shared by the web Resources form and -│ │ # readResourceFromTemplate (TUI + CLI), so the -│ │ # clients cannot drift on what a template means; -│ │ # delegates to the SDK's UriTemplate for what it -│ │ # gets right, and takes over a whole template -│ │ # containing any of the three shapes it does not: -│ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), -│ │ # `{;id}` (operator absent from its list), and -│ │ # `{id:3}` (prefix modifier folded into the name), -│ │ # and `{+v}`/`{#v}` (encodeURI mangles reserved -│ │ # `[`/`]` and double-encodes pct-triplets). The `;` -│ │ # and `:N` cases would render form fields literally -│ │ # labelled `;id` / `id:3`. Requiredness is per -│ │ # EXPRESSION, not per variable — requiredGroups + -│ │ # hasRequiredValues: `{a,b}` with only `a` filled is -│ │ # expandable, and a name recurring across -│ │ # expressions needs each group checked — #1919; +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. +│ │ # The ONE expander for every client — the web +│ │ # Resources form and readResourceFromTemplate +│ │ # (TUI + CLI) — and every client derives its FORM +│ │ # FIELDS from it too (clients/tui uriTemplateToForm), +│ │ # which is what makes the sharing real: a form +│ │ # submits under the names it rendered, so a mangled +│ │ # name silently drops the value at expansion. The +│ │ # SDK's UriTemplate is used only to VALIDATE; its +│ │ # expander is wrong for `{a,b}` (raw-joined), +│ │ # `{;id}` (operator missing), `{id:3}` (modifier in +│ │ # the name), `{+v}`/`{#v}` (encodeURI mangles `[`/`]` +│ │ # and double-encodes pct-triplets), and `{v}` +│ │ # (encodeURIComponent leaves `!'()*` bare). +│ │ # Requiredness is per EXPRESSION, not per variable — +│ │ # requiredGroups + hasRequiredValues — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index af8917c5a..d4b0e61e0 100644 --- a/README.md +++ b/README.md @@ -249,20 +249,21 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — and all three derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. -| Shape | SDK behavior | Correct | -| --- | --- | --- | -| `{a,b}` | `foo/bar,q` — raw-joined, unencoded, operator prefix dropped | `foo%2Fbar,q` | -| `{;id}` | `""` — the `;` operator is not in its list, so the variable parses as `;id` | `;id=7` | -| `{id:3}` | `""` — the prefix modifier is folded into the name, giving `id:3` | `abc` | -| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | `[::1]`, `%2F` | +The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: -The `;` and `:3` rows matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render 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. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. +| Shape | SDK behavior | +| --- | --- | +| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | +| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | +| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | +| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | -Requiredness is likewise 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 the 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 a per-variable flag cannot express once a name recurs across expressions. +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. -One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. +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`). #### Advertised extensions diff --git a/clients/tui/__tests__/uriTemplateToForm.test.ts b/clients/tui/__tests__/uriTemplateToForm.test.ts index 1c522c659..9dc1a32c6 100644 --- a/clients/tui/__tests__/uriTemplateToForm.test.ts +++ b/clients/tui/__tests__/uriTemplateToForm.test.ts @@ -11,7 +11,38 @@ describe("uriTemplateToForm", () => { expect(form.title).toBe("Read Resource: file"); const fields = form.sections[0]!.fields; expect(fields.map((f) => f.name)).toEqual(["path", "name"]); - expect(fields[0]).toMatchObject({ type: "string", required: false }); + // Simple `{path}` sits mid-URI, so omitting it would leave an empty path + // segment rather than a shorter URI -- it is mandatory, matching the web + // panel. Both clients read this from core's `requiredGroups`. + expect(fields[0]).toMatchObject({ type: "string", required: true }); + }); + + it("names fields as the expander looks them up, not as the SDK parses them", () => { + // The SDK reports these as ";id" and "id:3"; submitting under those keys + // would make the expander find nothing and drop the expression (#1919). + expect( + uriTemplateToForm("x://a{;id}", "matrix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + expect( + uriTemplateToForm("x://a/{id:3}", "prefix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + }); + + it("leaves a shared required group optional rather than demanding every name", () => { + // `{a,b}` is satisfied by either name, and ink-form cannot say "any one + // of"; marking both required would refuse input the expander accepts. + const fields = uriTemplateToForm("x://{a,b}", "pair").sections[0]!.fields; + expect(fields.map((f) => f.name)).toEqual(["a", "b"]); + expect(fields.every((f) => f.required === false)).toBe(true); + }); + + it("marks an omittable variable optional", () => { + const fields = uriTemplateToForm("x://a{?topic}", "q").sections[0]!.fields; + expect(fields[0]).toMatchObject({ name: "topic", required: false }); }); it("returns an empty Template Variables section for a static URI", () => { diff --git a/clients/tui/src/utils/uriTemplateToForm.ts b/clients/tui/src/utils/uriTemplateToForm.ts index c8a027e9c..af580a984 100644 --- a/clients/tui/src/utils/uriTemplateToForm.ts +++ b/clients/tui/src/utils/uriTemplateToForm.ts @@ -4,32 +4,50 @@ import type { FormStructure, FormSection, FormField } from "ink-form"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + requiredGroups, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; /** - * Converts a URI Template to ink-form structure + * Converts a URI Template to ink-form structure. + * + * Fields come from core's `templateVariables`, the same parser + * `InspectorClient.readResourceFromTemplate` expands through, so the key this + * form submits is the key the expander looks up. Using the SDK's + * `variableNames` here instead is not merely untidy -- it mangles the name: + * `{;id}` yields `";id"` and `{id:3}` yields `"id:3"`, so the form would submit + * `{ ";id": "7" }` while the expander looks for `id`, silently dropping the + * value and the whole expression with it (#1919). + * + * The SDK template is still constructed, but only to validate: core's parser is + * deliberately lenient (an unclosed `{` becomes literal text), so this is what + * still surfaces a malformed template as an empty form plus a logged error. */ export function uriTemplateToForm( uriTemplate: string, templateName: string, ): FormStructure { - const fields: FormField[] = []; + let fields: FormField[] = []; try { - const template = new UriTemplate(uriTemplate); - /* v8 ignore next -- UriTemplate.variableNames is a getter that always - returns a string[]; the `|| []` fallback is an unreachable guard. */ - const variableNames = template.variableNames || []; - - for (const variableName of variableNames) { - const field: FormField = { - name: variableName, - label: variableName, - type: "string", - required: false, // URI template variables are typically optional - }; - - fields.push(field); - } + new UriTemplate(uriTemplate); + // Only a variable that is the *sole* member of a non-omittable expression + // is genuinely mandatory. RFC 6570 drops undefined names from a multi-name + // expression, so `{a,b}` needs only one of the two -- ink-form cannot + // express "any one of these", and marking both required would refuse input + // the expander accepts. + const mandatory = new Set( + requiredGroups(uriTemplate) + .filter((names) => names.length === 1) + .map(([name]) => name), + ); + fields = templateVariables(uriTemplate).map(({ name }) => ({ + name, + label: name, + type: "string", + required: mandatory.has(name), + })); } catch (error) { // If parsing fails, return empty form console.error("Failed to parse URI template:", error); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 7b39b4ad9..9c1048217 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -253,9 +253,18 @@ export function ResourceTemplatePanel({ // required multi-name expression no single field is mandatory either // -- any one of them satisfies it -- so say which, rather than // marking each one required and blocking valid input. - const sharedGroup = groups.find( - (names) => names.length > 1 && names.includes(varName), + // A name can sit in a singleton required group *and* a shared one + // (`x://{a}/{a,b}`). The singleton demands this exact field, so the + // "any one of" hint would contradict the disabled submit button -- + // suppress it and let the field read as plainly required. + const individuallyRequired = groups.some( + (names) => names.length === 1 && names[0] === varName, ); + const sharedGroup = individuallyRequired + ? undefined + : groups.find( + (names) => names.length > 1 && names.includes(varName), + ); const description = !required ? "Optional" : sharedGroup diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 4c84d8073..f0384f905 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -432,3 +432,36 @@ describe("allow-reserved encoding under + and #", () => { ); }); }); + +describe("unreserved encoding under the non-reserved operators", () => { + // `encodeURIComponent` leaves the sub-delims !'()* bare, but RFC 6570 only + // allows *unreserved* characters through for these operators. + it.each([ + ["", "x://"], + [".", "x://a."], + ["/", "x://a/"], + ])("encodes !'()* under the %s operator", (operator, prefix) => { + const base = operator === "" ? "x://" : "x://a"; + expect( + expandUriTemplate(`${base}{${operator}v}`, { v: "a!b'c(d)e*f" }), + ).toBe(`${prefix}a%21b%27c%28d%29e%2Af`); + }); + + it("encodes them in a named (query) expression too", () => { + expect(expandUriTemplate("x://a{?v}", { v: "a!b" })).toBe("x://a?v=a%21b"); + }); + + it("encodes them in a matrix expression too", () => { + expect(expandUriTemplate("x://a{;v}", { v: "a!b" })).toBe("x://a;v=a%21b"); + }); + + it("leaves them alone under + and #, where reserved characters are allowed", () => { + expect(expandUriTemplate("x://{+v}", { v: "a!b'c(d)e*f" })).toBe( + "x://a!b'c(d)e*f", + ); + }); + + it("still leaves the unreserved set itself untouched", () => { + expect(expandUriTemplate("x://{v}", { v: "aZ0-._~" })).toBe("x://aZ0-._~"); + }); +}); diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 0a790cf08..248abd920 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -2,32 +2,32 @@ * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * * This lives in `core/` rather than in a client so the web Resources form, the - * TUI, and the CLI cannot disagree about what a template means: the form calls - * {@link templateVariables} / {@link expandUriTemplate} directly, and the TUI - * and CLI reach the same code through `InspectorClient.readResourceFromTemplate`. + * TUI, and the CLI cannot disagree about what a template means. Every one of + * them derives its form fields from {@link templateVariables} and expands + * through {@link expandUriTemplate} — the web panel directly, the TUI and CLI + * via `InspectorClient.readResourceFromTemplate`. * * ## Why this is not simply `new UriTemplate(t).expand(v)` * - * Expansion is delegated to the SDK's `UriTemplate` for every expression it - * handles correctly — which is the overwhelmingly common case, and keeping it - * there means we cannot drift from the SDK on the ordinary path. But its parser - * and expander are incomplete in three ways that a *form* makes visible, - * because a form has to name the variables it is asking the user to fill in. - * Each was measured against the pinned SDK, not inferred: + * The SDK's `UriTemplate` is still used, but only to *validate* a template — + * constructing it is what rejects an unclosed expression. Its expander is not, + * because it is incomplete in ways a form makes visible: a form has to *name* + * the variables it asks the user to fill in, so a parser that mangles a name + * produces a field nobody can use. Each of these was measured against the + * pinned SDK, not inferred: * - * | Shape | SDK `variableNames` | SDK expansion | Correct | - * | ------------ | ------------------- | --------------------------------- | -------------- | - * | `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, no prefix| `foo%2Fbar,q` | - * | `{;id}` | `[";id"]` | `""` — operator unknown | `;id=7` | - * | `{id:3}` | `["id:3"]` | `""` — modifier folded into name | `abc` | + * | Shape | SDK behavior | + * | ---------------- | --------------------------------------------------------- | + * | `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | + * | `{;id}` | `;` is not in its operator list, so the variable is `;id` | + * | `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | + * | `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` and double-encodes `%` | + * | `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare | * - * For the last two the damage is not just a wrong URI: the form would render - * fields literally labelled `;id` and `id:3`, which the user cannot fill in - * usefully. So this module parses varspecs properly and, **when a template - * contains any expression the SDK gets wrong, expands that whole template - * itself** in {@link expandParts} rather than splicing corrected fragments into - * a template the SDK then re-expands — splicing would leave the SDK's - * cross-expression `?`-to-`&` rewrite unaware of the fragments we resolved. + * An earlier revision delegated the shapes the SDK got right and took over only + * the rest. That split is gone: once the encoders themselves differed, the two + * paths would have encoded the *same value* differently depending on whether + * the expression happened to carry a modifier. One expander, one set of rules. */ import { UriTemplate } from "@modelcontextprotocol/client"; @@ -287,11 +287,36 @@ function encodeAllowReserved(value: string): string { .join(""); } +/** + * Percent-encodes everything outside RFC 3986's *unreserved* set, which is what + * every operator except `+` and `#` calls for. + * + * `encodeURIComponent` alone is not that set: it leaves `!`, `'`, `(`, `)` and + * `*` unescaped. Those are sub-delims, not unreserved, so RFC 6570 requires + * them encoded for simple, label, path, matrix and query expansion. They are + * substituted afterwards rather than hand-rolled, so `encodeURIComponent` still + * does the UTF-8 work for everything else. + */ +const FORCE_ENCODED: Record = { + "!": "%21", + "'": "%27", + "(": "%28", + ")": "%29", + "*": "%2A", +}; + +function encodeUnreserved(value: string): string { + return encodeURIComponent(value).replace( + /[!'()*]/g, + (char) => FORCE_ENCODED[char], + ); +} + /** Encodes one value for its operator: reserved characters survive `+` and `#`. */ function encodeValue(value: string, operator: string): string { return operator === "+" || operator === "#" ? encodeAllowReserved(value) - : encodeURIComponent(value); + : encodeUnreserved(value); } /** @@ -309,33 +334,6 @@ function renderValue(value: string, spec: VarSpec, operator: string): string { return encodeValue(truncated, operator); } -/** - * True for an expression the SDK would get wrong, and which this module must - * therefore expand itself. See the table in the module comment. - */ -function needsOwnExpansion(part: TemplatePart): boolean { - if (part.kind !== "expression") return false; - return ( - // Multi-name, non-query: the SDK raw-joins, skipping encoding and prefix. - (part.varspecs.length > 1 && - part.operator !== "?" && - part.operator !== "&") || - // The SDK has no `;` operator at all. - part.operator === ";" || - // The SDK encodes `+`/`#` values with `encodeURI`, which mangles reserved - // `[`/`]` and double-encodes existing pct-triplets — see - // `encodeAllowReserved`. Taken over even for a single name so both paths - // agree on what these operators mean. - part.operator === "+" || - part.operator === "#" || - // The SDK folds everything after a `:` into the variable name. Keyed on the - // raw source rather than on a parsed `maxLength` so a *malformed* modifier - // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and - // looks up `id`, while the SDK would look up `id:` and find nothing. - part.source.includes(":") - ); -} - /** Expands one expression per RFC 6570. Returns "" if no name has a value. */ function expandExpression( part: TemplateExpression, @@ -426,11 +424,11 @@ export function expandUriTemplateStrict( values: Record, ): string { const defined = definedValues(values); - const sdkTemplate = new UriTemplate(uriTemplate); - const parts = parseUriTemplate(uriTemplate); - return parts.some(needsOwnExpansion) - ? expandParts(parts, defined) - : sdkTemplate.expand(defined); + // Constructed purely to validate: this is what throws `Unclosed template + // expression`, and callers such as `readResourceFromTemplate` wrap that + // error. Its `expand` is deliberately not used — see the module comment. + new UriTemplate(uriTemplate); + return expandParts(parseUriTemplate(uriTemplate), defined); } /** From ebc18e1d56eef13098deb2b8bbc757521eb3f330 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:38:52 -0400 Subject: [PATCH 06/21] fix: expand each expression independently, and reject invalid varspecs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot's round-5 review on #2035 — eight suppressed comments, all of them real. The consequential one overturns behavior I copied from the SDK and defended in two earlier rounds. RFC 6570 expands each expression independently; it carries no cross-expression state. The SDK tracks whether a query expression has already emitted and rewrites a later `{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and `match("x?one=1&two=2")` on that same template returns **null**, while `match("x?one=1?two=2")` returns both variables. So the rewrite emitted a URI the advertised template cannot match — #1919 one level up. A server that wants a continuation advertises `{?one}{&two}`, which expands to `&` and matches; producing that shape is the server's choice to declare, not ours to infer. Also: - `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC 6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are invalid templates rather than templates with an ignorable modifier — and the SDK's constructor accepts them all, so nothing else rejects them. Strict expansion now throws; lenient returns the raw template. Discovery stays lenient so the panel still renders rather than going blank. - The TUI could submit `{a,b}` completely blank while the web panel blocked the same request: ink-form has no way to express "any one of these", so its members are left optional and the modal needs its own group check. Added, with tests — it was also below the coverage gate without them (functions 76.92%). - Added the integration test the config never had: it resolves the checked-in config and drives all four reads over a real transport, so a misspelt preset fails there rather than only when someone runs the repro by hand. - "All three clients" was false. The CLI has no template form and its `resources/read` passes the already-expanded `--uri` straight to `readResource`, so nothing here runs for it. Corrected in the README, AGENTS.md, and the module comment, and dropped a docblock still describing the SDK-delegation path removed last round. Signed-off-by: cliffhall --- AGENTS.md | 10 +- README.md | 2 +- .../tui/__tests__/ResourceTestModal.test.tsx | 47 ++++++ .../tui/src/components/ResourceTestModal.tsx | 27 ++++ .../web/src/test/core/mcp/uriTemplate.test.ts | 95 ++++++------ .../integration/mcp/rfc6570-templates.test.ts | 139 ++++++++++++++++++ clients/web/src/utils/uriTemplate.test.ts | 6 +- core/mcp/uriTemplate.ts | 133 ++++++++++------- 8 files changed, 353 insertions(+), 106 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/rfc6570-templates.test.ts diff --git a/AGENTS.md b/AGENTS.md index ccb2935c0..4636e17e4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,10 +74,12 @@ v2/main/ │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. -│ │ # The ONE expander for every client — the web -│ │ # Resources form and readResourceFromTemplate -│ │ # (TUI + CLI) — and every client derives its FORM -│ │ # FIELDS from it too (clients/tui uriTemplateToForm), +│ │ # The ONE expander for the web Resources form and +│ │ # readResourceFromTemplate (TUI); the CLI is NOT a +│ │ # consumer — it has no template form and passes an +│ │ # already-expanded --uri to readResource. Both +│ │ # consumers derive their FORM FIELDS from it too +│ │ # (clients/tui uriTemplateToForm), │ │ # which is what makes the sharing real: a form │ │ # submits under the names it rendered, so a mangled │ │ # name silently drops the value at expansion. The diff --git a/README.md b/README.md index d4b0e61e0..c9f662e50 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,7 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — and all three derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. +The web client and the TUI expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI via `InspectorClient.readResourceFromTemplate` — and both derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. (The CLI is not a consumer: it has no template form, and its `resources/read` passes the already-expanded `--uri` straight through.) The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index d9de004d6..0067d2118 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -77,6 +77,53 @@ const renderAndSubmit = async ( }; describe("ResourceTestModal", () => { + // RFC 6570 keeps a required expression satisfied by any ONE of its names, so + // `{a,b}` cannot be expressed with ink-form's per-field `required` flag and + // its members are left optional there. Without the modal's own group check + // the TUI would submit blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + describe("required-group validation", () => { + it("refuses to submit when no name in a required group has a value", async () => { + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "", b: "" }, + ); + 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: [] }, + expandedUri: "x://only-a", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "only-a", b: "" }, + ); + expect(read).toHaveBeenCalledWith("x://{a,b}", { a: "only-a", b: "" }); + unmount(); + }); + + it("does not block a template whose only expression is omittable", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://events", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://events{?topic}" }), + { topic: "" }, + ); + expect(read).toHaveBeenCalled(); + unmount(); + }); + }); + it("renders the form initially without invoking the client", async () => { const read = vi.fn(); const api = render( diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 914b4bca6..37d662a25 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -5,6 +5,10 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { ReadResourceResult } from "@modelcontextprotocol/client"; import { uriTemplateToForm } from "../utils/uriTemplateToForm.js"; +import { + hasRequiredValues, + requiredGroups, +} from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; // Helper to extract error message from various error types @@ -133,6 +137,29 @@ export function ResourceTestModal({ const handleFormSubmit = async (values: Record) => { if (!inspectorClient || !template) return; + // RFC 6570 keeps a required expression satisfied by any ONE of its names, + // so `{a,b}` cannot be expressed with ink-form's per-field `required` flag + // 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 ")); + setResult({ + input: values, + output: null, + error: `Missing required template variable(s): ${unmet.join(", ")}`, + duration: 0, + uri: template.uriTemplate, + }); + setState("results"); + return; + } + setState("loading"); const startTime = Date.now(); diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index f0384f905..4b8471319 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -18,6 +18,7 @@ describe("parseUriTemplate", () => { operator: "", varspecs: [{ name: "topic" }], names: ["topic"], + invalid: false, }, ]); }); @@ -31,6 +32,7 @@ describe("parseUriTemplate", () => { operator: "?", varspecs: [{ name: "a" }, { name: "b" }], names: ["a", "b"], + invalid: false, }, ]); }); @@ -137,8 +139,19 @@ describe("expandUriTemplate", () => { ); }); - it("joins two query expressions with & rather than a second ?", () => { + it("expands each query expression independently, per RFC 6570", () => { + // NOT `?one=1&two=2`. The SDK rewrites the second `?` to `&`, but measured + // against the pinned SDK its own matcher then rejects the result: + // match("x?one=1&two=2") on `x{?one}{?two}` is null, while + // match("x?one=1?two=2") returns both variables. A server wanting a + // continuation advertises `{?one}{&two}` -- see the test below. expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( + "x://a?one=1?two=2", + ); + }); + + it("emits & only where the template asks for the continuation operator", () => { + expect(expandUriTemplate("x://a{?one}{&two}", { one: "1", two: "2" })).toBe( "x://a?one=1&two=2", ); }); @@ -229,11 +242,6 @@ describe("varspec modifiers", () => { ); }); - it("ignores a malformed modifier rather than inventing a truncation", () => { - expect(templateVariables("x://{id:}")[0].name).toBe("id"); - expect(expandUriTemplate("x://{id:}", { id: "abcdef" })).toBe("x://abcdef"); - }); - it("strips the explode modifier from the name", () => { expect(templateVariables("x://{id*}")[0].name).toBe("id"); }); @@ -330,60 +338,24 @@ describe("requiredGroups / hasRequiredValues", () => { }); }); -describe("cross-expression query joining", () => { - it("rewrites a second ? to & on the own-expansion path too", () => { - // Forced onto the own-expansion path by the `;` expression; the `?`-to-`&` - // rewrite must still apply, exactly as the SDK does it. +describe("expression independence", () => { + it("does not rewrite a later query expression on the own-expansion path", () => { expect( expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", one: "1", two: "2", }), - ).toBe("x://a;k=v?one=1&two=2"); + ).toBe("x://a;k=v?one=1?two=2"); }); - it("uses ? for the first query expression that actually emits", () => { + it("omits an expression with no value without affecting its neighbours", () => { expect( expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", two: "2" }), ).toBe("x://a;k=v?two=2"); }); }); -describe("strict vs lenient expansion", () => { - // `readResourceFromTemplate` wraps the thrown error with the template name; - // the web panel instead needs the raw template back, because an invalid - // template comes from the server and throwing would take out the panel. - it.each(["file:///{unclosed", "{a,b,c"])( - "strict throws on the invalid template %s", - (template) => { - expect(() => expandUriTemplateStrict(template, { x: "1" })).toThrow(); - }, - ); - - it.each(["file:///{unclosed", "{a,b,c"])( - "lenient returns %s unchanged", - (template) => { - expect(expandUriTemplate(template, { x: "1" })).toBe(template); - }, - ); - - it("validates syntax even when taking the own-expansion path", () => { - // `{;a}` forces own-expansion, and this module's parser treats the - // unclosed tail as literal text -- so without the unconditional SDK - // construction nothing would reject this. - expect(() => expandUriTemplateStrict("x://{;a}{b,c", { a: "1" })).toThrow(); - }); - - it("agrees with the lenient variant on a valid template", () => { - const template = "foobar://events{?topic}"; - const values = { topic: "foo/bar" }; - expect(expandUriTemplateStrict(template, values)).toBe( - expandUriTemplate(template, values), - ); - }); -}); - describe("allow-reserved encoding under + and #", () => { // The SDK uses `encodeURI` for these operators, which corrupts two classes // of value rather than merely over-escaping: measured, encodeURI("[::1]") @@ -465,3 +437,34 @@ describe("unreserved encoding under the non-reserved operators", () => { expect(expandUriTemplate("x://{v}", { v: "aZ0-._~" })).toBe("x://aZ0-._~"); }); }); + +describe("prefix-modifier grammar", () => { + // RFC 6570: max-length = %x31-39 0*3DIGIT -- 1..9999, no leading zero. + // The SDK's constructor accepts these shapes, so nothing else rejects them; + // treating `{id:abc}` as a plain `{id}` would send a URI the server never + // advertised, with nothing to alert anyone. + it.each(["x://{id:}", "x://{id:0}", "x://{id:abc}", "x://{id:10000}"])( + "strict rejects the invalid template %s", + (template) => { + expect(() => expandUriTemplateStrict(template, { id: "abcdef" })).toThrow( + /Invalid RFC 6570 varspec/, + ); + }, + ); + + it.each(["x://{id:}", "x://{id:abc}"])( + "lenient returns %s unchanged rather than guessing", + (template) => { + expect(expandUriTemplate(template, { id: "abcdef" })).toBe(template); + }, + ); + + it.each([ + ["x://{id:1}", "a"], + ["x://{id:9999}", "abcdef"], + ])("accepts the in-range modifier %s", (template, expected) => { + expect(expandUriTemplate(template, { id: "abcdef" })).toBe( + `x://${expected}`, + ); + }); +}); diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts new file mode 100644 index 000000000..5765c3690 --- /dev/null +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -0,0 +1,139 @@ +import { describe, it, expect, afterEach } from "vitest"; +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 { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + loadConfig, + resolveConfig, +} from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of `test-servers/configs/rfc6570-templates-http.json` — the + * documented manual reproduction for #1919. + * + * The unit tests assert what `expandUriTemplate` *produces*. They cannot assert + * that what it produces is what a spec-compliant server *accepts*, and that + * second half is the entire bug: the old string substitution emitted a URI the + * Inspector was perfectly happy with and the server rejected. So this drives + * both directions over a real transport — the encoded URI must resolve, and the + * unencoded one the old code produced must still be refused, so a regression + * cannot pass by quietly loosening the server. + * + * The server is built by **resolving the checked-in config** rather than by + * calling the fixture factories, which is the difference between covering the + * wiring and merely asserting the factory: a misspelt preset name in + * `preset-registry.ts`, or a config naming a preset that no longer exists, + * fails here instead of only when someone runs the repro by hand. + */ +describe("RFC 6570 resource templates over the wire (#1919)", () => { + let client: InspectorClient | null = null; + let server: TestServerHttp | null = null; + + const configPath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs/rfc6570-templates-http.json", + ); + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // ignore + } + client = null; + } + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + }); + + /** + * Boot the showcase config. The harness picks the port rather than using the + * config's fixed one, so this cannot collide with a showcase server someone + * is running by hand. + */ + async function connectToShowcase(): Promise { + const resolved = resolveConfig(loadConfig(configPath)); + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("rfc6570-templates-test", "1.0.0"), + tools: resolved.tools, + resources: resolved.resources, + resourceTemplates: resolved.resourceTemplates, + }); + await started.start(); + server = started; + + const connected = new InspectorClient( + { type: "streamable-http", url: started.url }, + { environment: { transport: createTransportNode } }, + ); + await connected.connect(); + client = connected; + return connected; + } + + it("advertises both templates from the checked-in config", async () => { + const connected = await connectToShowcase(); + const { resourceTemplates } = await connected.listAllResourceTemplates(); + expect(resourceTemplates.map((entry) => entry.uriTemplate).sort()).toEqual([ + "foobar://events/{topic}", + "foobar://events{?topic}", + ]); + }); + + it("resolves the encoded URI the simple expression expands to", async () => { + const connected = await connectToShowcase(); + const uri = expandUriTemplate("foobar://events/{topic}", { + topic: "foo/bar", + }); + expect(uri).toBe("foobar://events/foo%2Fbar"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); + + it("rejects the unencoded URI the old string substitution produced", async () => { + const connected = await connectToShowcase(); + // This is what the pre-fix client sent, and why #1919 was filed: the raw + // `/` creates a second path segment that the template cannot match. + await expect( + connected.readResource("foobar://events/foo/bar"), + ).rejects.toThrow(/not found/i); + }); + + it("resolves the encoded URI the query expression expands to", async () => { + const connected = await connectToShowcase(); + const uri = expandUriTemplate("foobar://events{?topic}", { + topic: "foo/bar", + }); + expect(uri).toBe("foobar://events?topic=foo%2Fbar"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); + + it("resolves the base URI a blank query expression expands to", async () => { + const connected = await connectToShowcase(); + // RFC 6570 drops the whole expression when the variable is undefined, so + // an unfilled `topic` legitimately requests the unfiltered collection. The + // SDK's matcher compiles `{?topic}` to a *required* `\?topic=([^&]+)`, so a + // template alone cannot serve this — the config registers the plain + // resource, which is what a real server would do. + const uri = expandUriTemplate("foobar://events{?topic}", { topic: "" }); + expect(uri).toBe("foobar://events"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 8a50ff7e7..feba12fe0 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -35,10 +35,12 @@ describe("previewUriTemplate", () => { expect(previewUriTemplate("x://a{?one,two}", {})).toBe("x://a{?one,two}"); }); - it("still rewrites the second ? to & when both query expressions resolve", () => { + it("expands each query expression independently, matching the submit", () => { + // Not `?one=1&two=2` -- see the expander's tests: the SDK's own matcher + // rejects that for this template. The preview must show what will be sent. expect( previewUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" }), - ).toBe("x://a?one=1&two=2"); + ).toBe("x://a?one=1?two=2"); }); it("restores a deferred expression that follows a resolved query expression", () => { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 248abd920..b68a5c422 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -1,11 +1,16 @@ /** * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * - * This lives in `core/` rather than in a client so the web Resources form, the - * TUI, and the CLI cannot disagree about what a template means. Every one of - * them derives its form fields from {@link templateVariables} and expands - * through {@link expandUriTemplate} — the web panel directly, the TUI and CLI - * via `InspectorClient.readResourceFromTemplate`. + * This lives in `core/` rather than in a client so the web Resources form and + * the TUI cannot disagree about what a template means. Both derive their form + * fields from {@link templateVariables} and expand through + * {@link expandUriTemplate} — the web panel directly, the TUI via + * `InspectorClient.readResourceFromTemplate`. + * + * The **CLI is deliberately not a consumer**: it has no template form, and its + * `resources/read` passes the already-expanded `--uri` straight to + * `readResource` (see `clients/cli/src/handlers/run-method.ts`). Nothing here + * runs for it. * * ## Why this is not simply `new UriTemplate(t).expand(v)` * @@ -86,6 +91,12 @@ interface TemplateExpression { varspecs: VarSpec[]; /** Bare variable names, `*` and any `:length` modifier stripped. */ names: string[]; + /** + * True when a varspec carried a modifier that is not valid RFC 6570. Strict + * expansion rejects the template; discovery stays lenient so the panel can + * still render something rather than going blank. + */ + invalid: boolean; } export type TemplatePart = TemplateLiteral | TemplateExpression; @@ -108,8 +119,23 @@ export interface TemplateVariable { required: boolean; } -/** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ -function parseVarSpec(raw: string): VarSpec | null { +/** + * 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. + */ +const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/; + +/** + * Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. + * + * Returns `null` for an empty varspec (a stray comma) and `"invalid"` for one + * whose modifier does not match the grammar. The two are distinguished because + * the first is ignorable and the second must fail the template: silently + * treating `{id:abc}` as `{id}` would send a URI that does not match what the + * server advertised, with nothing to alert anyone. + */ +function parseVarSpec(raw: string): VarSpec | null | "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. @@ -120,13 +146,9 @@ function parseVarSpec(raw: string): VarSpec | null { if (colon === -1) return { name: spec }; const name = spec.slice(0, colon); - const length = Number(spec.slice(colon + 1)); - // A malformed modifier (`{id:}`, `{id:abc}`) is not a valid varspec; keep the - // name and ignore the modifier rather than inventing a truncation. - if (name.length === 0) return null; - return Number.isInteger(length) && length > 0 - ? { name, maxLength: length } - : { name }; + const modifier = spec.slice(colon + 1); + if (name.length === 0 || !MAX_LENGTH_GRAMMAR.test(modifier)) return "invalid"; + return { name, maxLength: Number(modifier) }; } /** @@ -159,17 +181,17 @@ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { } const body = uriTemplate.slice(i + 1, end); const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const varspecs = body - .slice(operator.length) - .split(",") - .map(parseVarSpec) - .filter((spec): spec is VarSpec => spec !== null); + const parsed = body.slice(operator.length).split(",").map(parseVarSpec); + const varspecs = parsed.filter( + (spec): spec is VarSpec => spec !== null && spec !== "invalid", + ); parts.push({ kind: "expression", source: uriTemplate.slice(i, end + 1), operator, varspecs, names: varspecs.map((spec) => spec.name), + invalid: parsed.includes("invalid"), }); i = end + 1; } @@ -375,30 +397,31 @@ function expandExpression( } /** - * Expands a whole parsed template, mirroring the SDK's `expand` — including its - * rule that a second query expression switches its leading `?` to `&`. + * Expands a whole parsed template. + * + * Each expression is expanded **independently**, which is what RFC 6570 + * specifies — expansion carries no cross-expression state. The SDK instead + * tracks whether a query expression has already emitted and rewrites a later + * `{?two}`'s leading `?` to `&`. That looks friendlier and is wrong: measured + * against the pinned SDK, `x{?one}{?two}` expands to `x?one=1&two=2`, and + * `UriTemplate.match` on that same template *rejects* it — `match("x?one=1&two=2")` + * is `null`, while `match("x?one=1?two=2")` returns both variables. So the + * rewrite emits a URI the advertised template cannot match, which is precisely + * the failure #1919 is about, one level up. + * + * A server that wants a continuation advertises it: `{?one}{&two}` expands to + * `x?one=1&two=2` and matches. Producing that shape is the server's choice to + * declare, not ours to infer. */ export function expandParts( parts: TemplatePart[], values: Record, ): string { - let result = ""; - let hasQueryParam = false; - - for (const part of parts) { - if (part.kind === "literal") { - result += part.text; - continue; - } - const expanded = expandExpression(part, values); - if (!expanded) continue; - - const isQuery = part.operator === "?" || part.operator === "&"; - result += isQuery && hasQueryParam ? `&${expanded.slice(1)}` : expanded; - if (isQuery) hasQueryParam = true; - } - - return result; + return parts + .map((part) => + part.kind === "literal" ? part.text : expandExpression(part, values), + ) + .join(""); } /** @@ -406,29 +429,33 @@ export function expandParts( * each value according to its operator, and omitting expressions whose * variables were left blank. **Throws** on a template that is not valid. * - * Templates the SDK handles correctly still go through the SDK, so the two - * cannot drift on the ordinary path; only a template containing at least one - * expression from the table above is expanded here instead. That is a - * whole-template switch rather than a per-expression splice so the `?`-to-`&` - * rewrite always sees every expression that actually produced output. + * Every expression is expanded by {@link expandParts}; the SDK's `UriTemplate` + * is constructed only because that is what rejects an unclosed expression, and + * callers such as `readResourceFromTemplate` wrap the error it throws. Its own + * `expand` is deliberately unused — see the module comment for the five shapes + * it gets wrong. * - * The SDK template is constructed *before* choosing a path, and unconditionally: - * that construction is what validates the syntax and throws `Unclosed template - * expression`, and callers such as `readResourceFromTemplate` wrap that error. - * Skipping it on the own-expansion path would silently accept a template like - * `{;a}{b,c` — this module's parser treats the unclosed tail as literal text, - * so nothing else would object. + * Its constructor is not a complete validator either: it accepts `{id:abc}`, + * whose modifier is not RFC 6570's `max-length` production. Treating that as a + * plain `{id}` would send a URI the server never advertised, so the owned + * parser's verdict is checked too. */ export function expandUriTemplateStrict( uriTemplate: string, values: Record, ): string { const defined = definedValues(values); - // Constructed purely to validate: this is what throws `Unclosed template - // expression`, and callers such as `readResourceFromTemplate` wrap that - // error. Its `expand` is deliberately not used — see the module comment. new UriTemplate(uriTemplate); - return expandParts(parseUriTemplate(uriTemplate), defined); + const parts = parseUriTemplate(uriTemplate); + const bad = parts.find((part) => part.kind === "expression" && part.invalid); + if (bad) { + throw new Error( + `Invalid RFC 6570 varspec in "${uriTemplate}": ${ + bad.kind === "expression" ? bad.source : "" + } — a prefix modifier must be 1-9999 with no leading zero.`, + ); + } + return expandParts(parts, defined); } /** From 1939136effaca7dc5b46b61556e8d22e9fd4653d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:02:39 -0400 Subject: [PATCH 07/21] fix: read template variables as own properties only Addresses Copilot's round-6 review on #2035. `toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC 6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded -- and every lookup here was a bare `values[name]`, which finds `Object.prototype`'s member for each of them. Measured: `({})["toString"] !== undefined` is true with typeof "function", and `({}["constructor"] ?? "").length` is 1. So a *blank* `{?toString}` field read as supplied and expanded a function body into the URI instead of omitting the expression, and `hasRequiredValues([["constructor"]], {})` returned true because `Object.length` is 1 -- a required group satisfied by nothing. All reads now go through an own-property `readValue`, and the web preview's filled-check uses `Object.hasOwn`. `expandExpression` carries the value through its filter rather than re-reading it afterwards, so the "is it defined" test and the read cannot disagree and nothing needs a non-null assertion. The remaining six findings were documentation drifting behind the code, which matters here because the comments are the argument for why this module exists: - The web helper still described the SDK's second-`?`-to-`&` rewrite as something the preview relies on. That was removed last round. - Three places still named the CLI as a consumer. It has no template form and passes an already-expanded `--uri` to `readResource`. - The PR description still described the superseded architecture (expansion delegated to the SDK), i.e. the opposite of what now ships. Signed-off-by: cliffhall --- README.md | 2 +- .../web/src/test/core/mcp/uriTemplate.test.ts | 38 +++++++++++++++++++ clients/web/src/utils/uriTemplate.test.ts | 16 ++++++++ clients/web/src/utils/uriTemplate.ts | 33 +++++++++------- core/mcp/uriTemplate.ts | 36 ++++++++++++++---- 5 files changed, 102 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index c9f662e50..c7fa48e09 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ inspector/ │ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, -│ │ # and the RFC 6570 URI-template helpers all three clients expand through +│ │ # and the RFC 6570 URI-template helpers the web form and TUI expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 4b8471319..f9bfb9b1f 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -468,3 +468,41 @@ describe("prefix-modifier grammar", () => { ); }); }); + +describe("variable names that collide with Object.prototype", () => { + // `toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC 6570 + // varnames. A bare `values[name]` lookup finds the prototype's member for + // every one of them, so a *blank* field read as supplied: measured, + // `({})["toString"] !== undefined` is true and its typeof is "function". + it.each(["toString", "constructor", "valueOf", "hasOwnProperty"])( + "omits a blank {?%s} instead of expanding a prototype member", + (name) => { + expect(expandUriTemplate(`x://a{?${name}}`, { [name]: "" })).toBe( + "x://a", + ); + }, + ); + + it("omits the expression when the key is absent entirely", () => { + expect(expandUriTemplate("x://a{?toString}", {})).toBe("x://a"); + }); + + it("still expands such a variable when it really has a value", () => { + expect(expandUriTemplate("x://a{?toString}", { toString: "v" })).toBe( + "x://a?toString=v", + ); + }); + + it("does not treat an inherited member as satisfying a required group", () => { + // `Object` (the inherited constructor) has length 1, so the old + // `(values[name] ?? "").length > 0` test reported this as satisfied. + expect(hasRequiredValues([["constructor"]], {})).toBe(false); + expect(hasRequiredValues([["constructor"]], { constructor: "c" })).toBe( + true, + ); + }); + + it("handles __proto__ as an ordinary variable name", () => { + expect(expandUriTemplate("x://a{?__proto__}", {})).toBe("x://a"); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index feba12fe0..3e9ca93b2 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -63,3 +63,19 @@ describe("previewUriTemplate - multi-name expressions", () => { ); }); }); + +describe("preview with Object.prototype-colliding names", () => { + it("leaves a blank {?toString} standing rather than treating it as filled", () => { + // A bare `defined[name] !== undefined` finds Object.prototype.toString and + // would expand the expression instead of showing the placeholder. + expect(previewUriTemplate("x://a{?toString}", { toString: "" })).toBe( + "x://a{?toString}", + ); + }); + + it("expands it once it really has a value", () => { + expect(previewUriTemplate("x://a{?toString}", { toString: "v" })).toBe( + "x://a?toString=v", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 56b2b8258..ea6c089b2 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -2,10 +2,12 @@ * The web Resources form's view of an RFC 6570 URI template (#1919). * * Parsing, variable classification, and expansion live in - * `@inspector/core/mcp/uriTemplate.js` so the web form, the TUI, and the CLI - * cannot disagree about what a template means -- they are re-exported here so - * the panel has a single import. What this module adds is the one piece that is - * purely a display concern: the partially-expanded preview string. + * `@inspector/core/mcp/uriTemplate.js` so the web form and the TUI cannot + * disagree about what a template means -- they are re-exported here so the + * panel has a single import. (The CLI is not a consumer: it has no template + * form, and its `resources/read` passes an already-expanded `--uri` straight to + * `readResource`.) What this module adds is the one piece that is purely a + * display concern: the partially-expanded preview string. */ import { @@ -37,16 +39,16 @@ export type { const deferredToken = (index: number) => `\u0000${index}\u0000`; /** - * A partially-expanded template for display: expressions whose variables are - * all filled are expanded exactly as `expandUriTemplate` would, and the rest - * are left standing as written so the user can see what is still needed. + * A partially-expanded template for display: expressions with at least one + * value are expanded exactly as `expandUriTemplate` would, and the rest are + * left standing as written so the user can see what is still needed. * - * Unfilled expressions are swapped for an inert token and restored after - * expansion -- rather than expanding each filled expression in isolation -- so - * the expander still sees one whole template and applies its cross-expression - * rules (notably rewriting a second `?` query expression to `&`). Routing the - * rewritten template back through `expandUriTemplate` is what keeps the preview - * honest: it can never promise a URI that submitting would not send. + * Unfilled expressions are swapped for an inert token and restored afterwards, + * and the rewritten template is expanded by `expandUriTemplate` itself. Routing + * it back through the real expander is what keeps the preview honest: it cannot + * promise a URI that submitting would not send. (Expansion carries no + * cross-expression state — see `expandParts` — so this is a per-expression + * substitution, not a whole-template rewrite that some later pass depends on.) */ export function previewUriTemplate( uriTemplate: string, @@ -59,7 +61,10 @@ export function previewUriTemplate( const rewritten = parts .map((part) => { if (part.kind === "literal") return part.text; - if (part.names.some((name) => defined[name] !== undefined)) { + // `Object.hasOwn`, not a bare lookup: `toString` and `constructor` are + // valid RFC 6570 variable names, and a plain lookup would find + // `Object.prototype`'s member and treat a blank field as filled. + if (part.names.some((name) => Object.hasOwn(defined, name))) { return part.source; } deferred.push(part.source); diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index b68a5c422..7e5c1d87d 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -257,10 +257,27 @@ export function hasRequiredValues( values: Record, ): boolean { return groups.every((names) => - names.some((name) => (values[name] ?? "").length > 0), + names.some((name) => (readValue(values, name) ?? "").length > 0), ); } +/** + * Reads a variable, ignoring anything inherited from `Object.prototype`. + * + * `toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC 6570 + * variable names (`varname` allows ALPHA / DIGIT / `_` / pct-encoded), and a + * plain object lookup finds the prototype's member for every one of them. A + * bare `values[name] !== undefined` therefore reports a *blank* `{?toString}` + * as supplied and expands a function body into the URI; `hasRequiredValues` + * likewise saw `constructor` as satisfied because `Object.length` is 1. + */ +function readValue( + values: Record, + name: string, +): string | undefined { + return Object.hasOwn(values, name) ? values[name] : undefined; +} + /** * Drops empty entries so an untouched optional field reads as *undefined* * (the expression disappears) rather than as the empty string (which would @@ -361,17 +378,20 @@ function expandExpression( part: TemplateExpression, values: Record, ): string { - const present = part.varspecs.filter( - (spec) => values[spec.name] !== undefined, - ); + // The value is carried through the filter rather than re-read afterwards, so + // the "is it defined" test and the read cannot disagree — and so nothing + // downstream needs a non-null assertion to convince the compiler. + const present = part.varspecs.flatMap((spec) => { + const value = readValue(values, spec.name); + return value === undefined ? [] : [{ spec, value }]; + }); if (present.length === 0) return ""; const { operator } = part; if (NAMED_OPERATORS.has(operator)) { const pairs = present.map( - (spec) => - `${spec.name}=${renderValue(values[spec.name], spec, operator)}`, + ({ spec, value }) => `${spec.name}=${renderValue(value, spec, operator)}`, ); // `;` repeats its separator per pair; `?`/`&` join with `&`. return operator === ";" @@ -379,8 +399,8 @@ function expandExpression( : `${operator}${pairs.join("&")}`; } - const rendered = present.map((spec) => - renderValue(values[spec.name], spec, operator), + const rendered = present.map(({ spec, value }) => + renderValue(value, spec, operator), ); switch (operator) { From 1ad3edd96d4fab3ff1f37d8102db21556f5a5609 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:32:27 -0400 Subject: [PATCH 08/21] fix: refuse a template that cannot expand, instead of reading the raw one Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed as a duplicate) got right and this branch did not. 1. An expression declaring no variable is a malformed template, not one with a member to skip. RFC 6570 requires at least one varspec per expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}` and `{*}` (an explode modifier is not a name) now make the template invalid. Skipping them was the more dangerous reading: `x://{}` expanded to `x://` while the form rendered no inputs, so its "everything required is filled" check was vacuously true and it submitted a URI that is not the template the server published. 2. The read is withheld when expansion fails. `expandUriTemplate`'s raw-template fallback exists for the preview, which runs during render -- but the submit path used it too, so an invalid template was read with its braces intact and the server answered with a confusing "not found" for a defect that is not the user's. `tryExpandUriTemplate` returns the URI or the reason as a value the caller cannot mistake for one; the panel gates Read Resource on it and prints the reason. This also covers a value that cannot be encoded -- an unpaired surrogate has no UTF-8 encoding, so `encodeURIComponent` throws `URIError` on it, and a text input can hold one via paste. `requiredGroups` now skips an expression naming no variable: an empty group can never be satisfied, so it would gate the form a second time on a condition nothing can meet, and the "any one of" hint built from it would name no fields. The accurate reason is the malformed template, which the expansion gate reports. npm run ci passes. Signed-off-by: cliffhall --- AGENTS.md | 7 +- README.md | 2 + .../ResourceTemplatePanel.test.tsx | 32 ++++++ .../ResourceTemplatePanel.tsx | 40 ++++++-- .../web/src/test/core/mcp/uriTemplate.test.ts | 63 ++++++++++++ clients/web/src/utils/uriTemplate.ts | 2 + core/mcp/uriTemplate.ts | 98 ++++++++++++++----- 7 files changed, 210 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4636e17e4..aa442e662 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,12 @@ v2/main/ │ │ # and double-encodes pct-triplets), and `{v}` │ │ # (encodeURIComponent leaves `!'()*` bare). │ │ # Requiredness is per EXPRESSION, not per variable — -│ │ # requiredGroups + hasRequiredValues — #1919; +│ │ # requiredGroups + hasRequiredValues. A template that +│ │ # cannot expand ({id:abc}, {}, {a,}) WITHHOLDS the read: +│ │ # tryExpandUriTemplate returns the reason as a value, and +│ │ # the panel disables Read Resource rather than sending the +│ │ # raw template — expandUriTemplate's raw-template fallback +│ │ # is for DISPLAY (the preview) only — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index c7fa48e09..ec2af96c0 100644 --- a/README.md +++ b/README.md @@ -263,6 +263,8 @@ 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. + 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`). #### Advertised extensions diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 4955b385f..87b2ba15d 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -243,6 +243,38 @@ describe("ResourceTemplatePanel", () => { screen.getByText("foobar://events?topic=news"), ).toBeInTheDocument(); }); + + // A template the server advertised can be malformed. There is no URI to + // send for one, so the read is withheld and the reason shown -- reading the + // raw template with its braces intact would draw a confusing "not found" + // from the server for a defect that is not the user's. + it.each([ + ["an out-of-grammar prefix modifier", "x://items/{id:abc}"], + ["an expression declaring no variable", "x://items/{}"], + ])("refuses to read a template with %s", (_label, uriTemplate) => { + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + expect(screen.getByText(/Invalid RFC 6570 varspec/)).toBeInTheDocument(); + expect(onReadResource).not.toHaveBeenCalled(); + }); + + it("shows the malformed template unexpanded in the preview", () => { + renderWithMantine( + , + ); + expect(screen.getByText("x://items/{}")).toBeInTheDocument(); + }); }); describe("completions", () => { diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 9c1048217..cb6a15893 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -15,11 +15,11 @@ import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprot import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { - expandUriTemplate, hasRequiredValues, previewUriTemplate, requiredGroups, templateVariables, + tryExpandUriTemplate, } from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { @@ -69,6 +69,13 @@ const DescriptionText = Text.withProps({ c: "dimmed", }); +// Why Read Resource is disabled when the template itself cannot be expanded. +// Without it the button is inert with nothing on screen explaining the refusal. +const ExpansionErrorText = Text.withProps({ + size: "sm", + c: "red", +}); + // Left-aligned so the action sits closest to the sidebar controls / the form // fields above; annotation badges trail it. const FooterRow = Group.withProps({ @@ -221,15 +228,29 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - // Only the expressions whose absence would change the URI's shape gate the - // read; an unfilled `{?topic}` is a legitimate request for the unfiltered - // resource, and RFC 6570 drops the whole expression for it. The rule is - // per-expression rather than per-variable -- `{a,b}` with only `a` filled - // expands to `a`'s value -- so it lives in core beside the expander. - const canSubmit = hasRequiredValues(groups, variables); + // Two independent gates on the read. + // + // First, whether the values cover what expansion structurally needs. Only the + // expressions whose absence would change the URI's shape count; an unfilled + // `{?topic}` is a legitimate request for the unfiltered resource, and RFC 6570 + // drops the whole expression for it. The rule is per-expression rather than + // per-variable -- `{a,b}` with only `a` filled expands to `a`'s value -- so it + // lives in core beside the expander. + // + // Second, whether the template expands at all. A template the server + // advertised can be malformed (`{id:abc}`, `{a,}`) and a pasted value can be + // unencodable, and in neither case is there a URI to send -- so withhold the + // request and say why, rather than reading the raw template with its braces + // intact and letting the server answer with a confusing "not found". + const expansion = tryExpandUriTemplate(uriTemplate, variables); + const canSubmit = + expansion.error === undefined && hasRequiredValues(groups, variables); function handleSubmit() { - onReadResource(expandUriTemplate(uriTemplate, variables)); + /* v8 ignore next -- unreachable: the button is disabled whenever the + expansion failed, which is the only way `uri` is undefined. */ + if (expansion.uri === undefined) return; + onReadResource(expansion.uri); } const preview = previewUriTemplate(uriTemplate, variables); @@ -308,6 +329,9 @@ export function ResourceTemplatePanel({ ); })} + {expansion.error !== undefined && ( + {expansion.error} + )}