From 6784bd20ff2a0fca77d9b3687bed9642904485ce Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:25:07 -0400 Subject: [PATCH 01/12] fix: expand resource templates per RFC 6570 in the web client (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Resources tab discovered template variables with `/\{(\w+)\}/g` and substituted them with a plain `String.replace`. That regex only ever matched a bare `{name}` expression, so a query expression like `foobar://events{?topic}` declared a variable the form never offered an input for — and the substitution inserted values verbatim, so a `topic` of `foo/bar` became a second path segment instead of `foo%2Fbar`, which a spec-compliant matcher rejects with `-32602 Resource not found`. Discovery, expansion, and preview now go through the SDK's `UriTemplate` — the same RFC 6570 implementation the TUI's form builder and `InspectorClient.readResourceFromTemplate` already used, so all three clients agree on a template's variables and on how a value is encoded. The preview keeps showing `{name}` for a variable that hasn't been filled yet (via an unreserved sentinel that survives expansion untouched), so the shape of the URI stays legible while the form is being completed, while a filled value is rendered exactly as it will be sent. Adds `rfc6570-templates-http.json` (preset `rfc6570_templates`) serving both templates from the issue, each echoing back the topic it received and the URI that matched. Signed-off-by: cliffhall --- README.md | 11 ++ .../ResourceTemplatePanel.stories.tsx | 17 +++ .../ResourceTemplatePanel.test.tsx | 64 ++++++++ .../ResourceTemplatePanel.tsx | 39 +---- clients/web/src/utils/uriTemplate.test.ts | 137 ++++++++++++++++++ clients/web/src/utils/uriTemplate.ts | 119 +++++++++++++++ .../configs/rfc6570-templates-http.json | 11 ++ test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 45 ++++++ 9 files changed, 415 insertions(+), 31 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..198692ebe 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 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 the two templates from [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) (preset `rfc6570_templates`): `foobar://events/{topic}` (simple expression) and `foobar://events{?topic}` (query expression). Each echoes back the `topic` it received and the URI that matched. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Open the Resources tab and select **events-by-path**, enter `foo/bar` for `topic`, and read it: the preview and the `resources/read` request must both show `foobar://events/foo%2Fbar`. On the broken build the web client substituted the value verbatim, producing `foobar://events/foo/bar` — a second path segment, which a spec-compliant matcher rejects with `-32602 Resource not found` (this server does exactly that, so the failure is visible rather than silent). + +Then select **events-by-query**: it must render a `topic` input at all. The old scan was `/\{(\w+)\}/g`, which sees only bare `{name}` expressions, so a query expression declared a variable the form never offered. + +Both surfaces now go through the SDK's `UriTemplate` — the same RFC 6570 implementation the TUI's form builder and `InspectorClient.readResourceFromTemplate` already used — so web, CLI, and TUI agree on a template's variables and on how a value is encoded. + #### 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.stories.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.stories.tsx index 8098dbf1a..c38c206a7 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.stories.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.stories.tsx @@ -46,6 +46,23 @@ export const WithAnnotations: Story = { }, }; +/** + * An RFC 6570 query expression. The variable lives inside `{?…}` rather than a + * bare `{…}`, so it only produces an input once discovery goes through a real + * RFC 6570 parser (#1919); the preview shows where the value lands in the + * query string. + */ +export const QueryExpression: Story = { + args: { + template: { + name: "Events", + uriTemplate: "foobar://events{?topic}", + description: + "Filter the event stream by topic. The value is percent-encoded into the query string.", + }, + }, +}; + export const NoDescription: Story = { args: { template: { diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 596a9340f..051665249 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -27,6 +27,18 @@ const noVarTemplate: ResourceTemplate = { uriTemplate: "file:///static.txt", }; +// #1919: a query expression declares a variable the old `/\{(\w+)\}/g` scan +// could not see, so it rendered no input at all. +const queryVarTemplate: ResourceTemplate = { + name: "Events", + uriTemplate: "foobar://events{?topic}", +}; + +const simpleVarTemplate: ResourceTemplate = { + name: "Events", + uriTemplate: "foobar://events/{topic}", +}; + describe("ResourceTemplatePanel", () => { it("renders the template title (or name) and description", () => { renderWithMantine( @@ -105,6 +117,58 @@ describe("ResourceTemplatePanel", () => { expect(screen.getByText("file:///users/bob/profile")).toBeInTheDocument(); }); + it("renders an input for a variable declared by a query expression", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("topic")).toBeInTheDocument(); + }); + + it("expands a query expression per RFC 6570 when submitted", async () => { + const user = userEvent.setup(); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("topic"), "weather"); + await user.click(screen.getByRole("button", { name: "Read Resource" })); + expect(onReadResource).toHaveBeenCalledWith( + "foobar://events?topic=weather", + ); + }); + + it("percent-encodes a reserved character rather than emitting a new path segment", 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/foo%2Fbar"); + }); + + it("previews the encoded value, not the raw input", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.type(screen.getByLabelText("topic"), "a b"); + expect(screen.getByText("foobar://events/a%20b")).toBeInTheDocument(); + }); + it("clears a variable via its Clear button (non-autocomplete branch)", async () => { const user = userEvent.setup(); renderWithMantine( diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 6b506228b..e3266bb90 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 { + expandTemplate, + previewTemplate, + templateVariableNames, +} 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", @@ -109,7 +86,7 @@ export function ResourceTemplatePanel({ const { name, title, uriTemplate, description, annotations } = template; const variableNames = useMemo( - () => parseVariableNames(uriTemplate), + () => templateVariableNames(uriTemplate), [uriTemplate], ); @@ -235,10 +212,10 @@ export function ResourceTemplatePanel({ const canSubmit = variableNames.every((n) => variables[n]?.length > 0); function handleSubmit() { - onReadResource(resolveUri(uriTemplate, variables)); + onReadResource(expandTemplate(uriTemplate, variables)); } - const preview = previewUri(uriTemplate, variables); + const preview = previewTemplate(uriTemplate, variables); return ( diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts new file mode 100644 index 000000000..4ab75a418 --- /dev/null +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { + expandTemplate, + previewTemplate, + templateVariableNames, +} from "./uriTemplate"; + +afterEach(() => { + vi.restoreAllMocks(); +}); + +/** Suppress the console.warn a malformed template emits. */ +function silenceWarn() { + return vi.spyOn(console, "warn").mockImplementation(() => {}); +} + +describe("templateVariableNames", () => { + it("finds a simple variable", () => { + expect(templateVariableNames("foobar://events/{topic}")).toEqual(["topic"]); + }); + + it("finds a variable inside a query expression", () => { + expect(templateVariableNames("foobar://events{?topic}")).toEqual(["topic"]); + }); + + it.each([ + ["reserved", "x://{+path}", ["path"]], + ["fragment", "x://a{#frag}", ["frag"]], + ["path segment", "x://a{/seg}", ["seg"]], + ["label", "x://a{.ext}", ["ext"]], + ["query continuation", "x://a?x=1{&y}", ["y"]], + ])("finds a variable in a %s expression", (_label, template, expected) => { + expect(templateVariableNames(template)).toEqual(expected); + }); + + it("finds every variable in a multi-variable expression", () => { + expect(templateVariableNames("foobar://e{?a,b}")).toEqual(["a", "b"]); + }); + + it("returns a repeated name once", () => { + expect(templateVariableNames("x://{a}/{b}/{a}")).toEqual(["a", "b"]); + }); + + it("returns an empty list for a template with no expressions", () => { + expect(templateVariableNames("foobar://events")).toEqual([]); + }); + + it("returns an empty list for a malformed template", () => { + const warn = silenceWarn(); + expect(templateVariableNames("x://{unterminated")).toEqual([]); + expect(warn).toHaveBeenCalled(); + }); +}); + +describe("expandTemplate", () => { + it("percent-encodes a reserved character in a simple variable", () => { + expect( + expandTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it("expands a query expression", () => { + expect( + expandTemplate("foobar://events{?topic}", { topic: "foo/bar" }), + ).toBe("foobar://events?topic=foo%2Fbar"); + }); + + it.each([ + ["?", "x://e/%3F"], + ["#", "x://e/%23"], + ["%", "x://e/%25"], + [" ", "x://e/%20"], + ["café", "x://e/caf%C3%A9"], + ])("encodes %j", (value, expected) => { + expect(expandTemplate("x://e/{v}", { v: value })).toBe(expected); + }); + + it("leaves a reserved-expansion value's sub-delimiters intact", () => { + expect(expandTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); + }); + + it("omits a variable with no value rather than emitting a dangling key", () => { + expect(expandTemplate("foobar://events{?topic}", { topic: "" })).toBe( + "foobar://events", + ); + }); + + it("omits a variable that is absent from the values entirely", () => { + expect(expandTemplate("foobar://e{?a,b}", { a: "1" })).toBe( + "foobar://e?a=1", + ); + }); + + it("returns the raw template when it cannot be parsed", () => { + silenceWarn(); + expect(expandTemplate("x://{unterminated", { a: "1" })).toBe( + "x://{unterminated", + ); + }); +}); + +describe("previewTemplate", () => { + it("shows an unfilled simple variable as its expression", () => { + expect(previewTemplate("foobar://events/{topic}", { topic: "" })).toBe( + "foobar://events/{topic}", + ); + }); + + it("shows an unfilled query variable as its expression", () => { + expect(previewTemplate("foobar://events{?topic}", { topic: "" })).toBe( + "foobar://events?topic={topic}", + ); + }); + + it("shows a filled value encoded exactly as it will be sent", () => { + expect( + previewTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it("mixes filled and unfilled variables", () => { + expect(previewTemplate("x://{a}/{b}", { a: "one", b: "" })).toBe( + "x://one/{b}", + ); + }); + + it("substitutes every occurrence of a repeated unfilled name", () => { + expect(previewTemplate("x://{a}/{b}/{a}", { a: "", b: "2" })).toBe( + "x://{a}/2/{a}", + ); + }); + + it("returns the raw template when it cannot be parsed", () => { + silenceWarn(); + expect(previewTemplate("x://{unterminated", {})).toBe("x://{unterminated"); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts new file mode 100644 index 000000000..3f45ffcf2 --- /dev/null +++ b/clients/web/src/utils/uriTemplate.ts @@ -0,0 +1,119 @@ +/** + * RFC 6570 URI Template helpers for the Resources screen. + * + * The web client used to discover variables with `/\{(\w+)\}/g` and expand them + * with a plain `String.replace`. That only ever saw simple expressions — a + * query expression like `{?topic}` produced no input at all — and it inserted + * values verbatim, so a `topic` of `foo/bar` silently became a second path + * segment instead of `foo%2Fbar` (#1919). + * + * These wrap the SDK's `UriTemplate`, which is the same RFC 6570 implementation + * the TUI's form builder and `InspectorClient.readResourceFromTemplate` already + * use, so all three surfaces agree on what a template's variables are and on + * how a value is encoded. + */ +import { UriTemplate } from "@modelcontextprotocol/client"; + +/** + * Parses `uriTemplate` once so a caller can discover, expand, and preview + * without re-parsing. Returns `null` when the template is malformed (the SDK + * throws on, e.g., an unterminated expression) so callers can degrade to + * rendering the raw string rather than crashing the panel. + */ +function parseTemplate(uriTemplate: string): UriTemplate | null { + try { + return new UriTemplate(uriTemplate); + } catch (error) { + console.warn(`Failed to parse URI template "${uriTemplate}":`, error); + return null; + } +} + +/** + * The variable names declared by `uriTemplate`, in declaration order — + * including those inside non-simple expressions (`{?topic}`, `{+path}`, + * `{#frag}`, `{/seg*}`, …), which the old regex missed entirely. + */ +export function templateVariableNames(uriTemplate: string): string[] { + const template = parseTemplate(uriTemplate); + return template ? uniqueNames(template) : []; +} + +/** + * A name repeated across expressions (`x://{a}/{b}/{a}`) is one input, not two — + * and the preview's sentinel bookkeeping is keyed by position, so a duplicate + * would otherwise leave one occurrence un-substituted. + */ +function uniqueNames(template: UriTemplate): string[] { + return [...new Set(template.variableNames)]; +} + +/** + * Expands `uriTemplate` per RFC 6570, percent-encoding each value according to + * its expression's operator. Variables with no value are omitted, which is what + * the spec prescribes and what keeps `{?topic}` from expanding to a dangling + * `?topic=`. + */ +export function expandTemplate( + uriTemplate: string, + variables: Record, +): string { + const template = parseTemplate(uriTemplate); + if (!template) return uriTemplate; + return template.expand(withoutEmptyValues(variables)); +} + +/** + * A token used to stand in for a variable the user hasn't filled yet, so the + * preview can show `{topic}` in its place instead of silently dropping it. + * + * Every character is RFC 3986 *unreserved*, so `expand` passes it through + * verbatim under every operator and it survives to be swapped back out. + */ +const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; + +/** + * Keyed by the variable's position rather than its name: a name may legally + * contain characters (`%`-encoded triplets) that `expand` would re-encode, + * which would keep the sentinel from surviving the round trip. + */ +function sentinelFor(index: number): string { + return `${UNFILLED_SENTINEL}${index}zz`; +} + +function withoutEmptyValues( + variables: Record, +): Record { + return Object.fromEntries( + Object.entries(variables).filter(([, value]) => value.length > 0), + ); +} + +/** + * A human-readable rendering of the template with the values entered so far: + * filled variables are expanded (and encoded) exactly as they would be on the + * wire, while unfilled ones are shown as `{name}` so the shape of the URI stays + * legible while the form is still being completed. + */ +export function previewTemplate( + uriTemplate: string, + variables: Record, +): string { + const template = parseTemplate(uriTemplate); + if (!template) return uriTemplate; + + const names = uniqueNames(template); + const filled = withoutEmptyValues(variables); + const values: Record = { ...filled }; + names.forEach((name, index) => { + if (values[name] === undefined) values[name] = sentinelFor(index); + }); + + let preview = template.expand(values); + names.forEach((name, index) => { + if (filled[name] !== undefined) return; + // The sentinel is unreserved, so it appears in the expansion unencoded. + preview = preview.split(sentinelFor(index)).join(`{${name}}`); + }); + return preview; +} diff --git a/test-servers/configs/rfc6570-templates-http.json b/test-servers/configs/rfc6570-templates-http.json new file mode 100644 index 000000000..1e30477e0 --- /dev/null +++ b/test-servers/configs/rfc6570-templates-http.json @@ -0,0 +1,11 @@ +{ + "serverInfo": { + "name": "rfc6570-templates", + "version": "1.0.0" + }, + "resourceTemplates": [{ "preset": "rfc6570_templates" }], + "transport": { + "type": "streamable-http", + "port": 3143 + } +} 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..5082e3e9e 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1474,6 +1474,51 @@ export function createFileResourceTemplate( }; } +/** + * Create the pair of RFC 6570 templates from #1919: one simple expression whose + * value must be percent-encoded rather than injected verbatim, and one query + * expression — which a naive `/\{(\w+)\}/g` scan cannot see at all. + * + * Both echo the received variable back, so the rendered result shows whether the + * client encoded and routed the value the server actually expected. + */ +export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] { + const describe = (uri: URL, topic: unknown) => ({ + contents: [ + { + uri: uri.toString(), + mimeType: "application/json", + text: JSON.stringify({ topic, matchedUri: uri.toString() }, null, 2), + }, + ], + }); + + return [ + { + name: "events-by-path", + uriTemplate: "foobar://events/{topic}", + description: + "Simple expression. A topic containing `/` must arrive percent-encoded, or it becomes a second path segment and no longer matches.", + inputSchema: { + topic: z.string().describe("Topic name — try `foo/bar`"), + }, + handler: async (uri: URL, params: Record) => + describe(uri, params.topic), + }, + { + name: "events-by-query", + uriTemplate: "foobar://events{?topic}", + description: + "Query expression. The variable is only discoverable through an RFC 6570 parser.", + inputSchema: { + topic: z.string().describe("Topic name"), + }, + handler: async (uri: URL, params: Record) => + describe(uri, params.topic), + }, + ]; +} + /** * Create a "user" resource template that returns user data by ID */ From 7111b5cd5789d2b673ca3ff8ab6f320ead13b425 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:28:00 -0400 Subject: [PATCH 02/12] test: cover the sentinel's double-digit position boundary (#1919) Signed-off-by: cliffhall --- clients/web/src/utils/uriTemplate.test.ts | 9 +++++++++ clients/web/src/utils/uriTemplate.ts | 3 +++ 2 files changed, 12 insertions(+) diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 4ab75a418..a13e2aa45 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -130,6 +130,15 @@ describe("previewTemplate", () => { ); }); + // The placeholder token for variable 1 must not be a prefix of the one for + // variable 11, or substituting the former would corrupt the latter. + it("keeps double-digit variable positions distinct", () => { + const names = Array.from({ length: 12 }, (_, i) => `v${i}`); + const template = `x://${names.map((n) => `{${n}}`).join("/")}`; + const empty = Object.fromEntries(names.map((n) => [n, ""])); + expect(previewTemplate(template, empty)).toBe(template); + }); + it("returns the raw template when it cannot be parsed", () => { silenceWarn(); expect(previewTemplate("x://{unterminated", {})).toBe("x://{unterminated"); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 3f45ffcf2..fd990eb26 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -76,6 +76,9 @@ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; * Keyed by the variable's position rather than its name: a name may legally * contain characters (`%`-encoded triplets) that `expand` would re-encode, * which would keep the sentinel from surviving the round trip. + * + * The trailing delimiter is load-bearing — without it index 1's token would be + * a prefix of index 11's, and substituting the first would corrupt the second. */ function sentinelFor(index: number): string { return `${UNFILLED_SENTINEL}${index}zz`; From 14874041204867afb4aaa7c7fa60fcf7201a9344 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:30:54 -0400 Subject: [PATCH 03/12] test: pin the SDK's prefix-modifier boundary for URI templates (#1919) Signed-off-by: cliffhall --- clients/web/src/utils/uriTemplate.test.ts | 18 ++++++++++++++++++ clients/web/src/utils/uriTemplate.ts | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index a13e2aa45..2aaaea55c 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -41,6 +41,14 @@ describe("templateVariableNames", () => { expect(templateVariableNames("x://{a}/{b}/{a}")).toEqual(["a", "b"]); }); + // The SDK's UriTemplate does not implement prefix modifiers: it treats + // `topic:3` as the whole variable name rather than a 3-char prefix of + // `topic`. Pinned here because that boundary is what the panel renders as a + // field label, and it is shared with the TUI and readResourceFromTemplate. + it("treats a prefix modifier as part of the variable name", () => { + expect(templateVariableNames("x://{topic:3}")).toEqual(["topic:3"]); + }); + it("returns an empty list for a template with no expressions", () => { expect(templateVariableNames("foobar://events")).toEqual([]); }); @@ -139,6 +147,16 @@ describe("previewTemplate", () => { expect(previewTemplate(template, empty)).toBe(template); }); + // A prefix modifier does not truncate here — the SDK folds it into the + // variable name (see the discovery test above) — so the sentinel survives + // expansion intact and the placeholder is restored like any other. + it("restores the placeholder for a variable carrying a prefix modifier", () => { + expect(previewTemplate("x://{topic:3}", {})).toBe("x://{topic:3}"); + expect(previewTemplate("x://{?topic:3}", {})).toBe( + "x://?topic:3={topic:3}", + ); + }); + it("returns the raw template when it cannot be parsed", () => { silenceWarn(); expect(previewTemplate("x://{unterminated", {})).toBe("x://{unterminated"); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index fd990eb26..877c210b4 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -33,6 +33,11 @@ function parseTemplate(uriTemplate: string): UriTemplate | null { * The variable names declared by `uriTemplate`, in declaration order — * including those inside non-simple expressions (`{?topic}`, `{+path}`, * `{#frag}`, `{/seg*}`, …), which the old regex missed entirely. + * + * One boundary is inherited from the SDK rather than chosen here: it does not + * implement prefix modifiers, so `{topic:3}` yields the name `"topic:3"` and + * expands without truncating. That behavior is shared with the TUI's form + * builder and `readResourceFromTemplate`, which parse through the same class. */ export function templateVariableNames(uriTemplate: string): string[] { const template = parseTemplate(uriTemplate); From 0cc492d0de01f5b920da1475ab1cab9f6ab07a25 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:45:39 -0400 Subject: [PATCH 04/12] fix: make the preview placeholder collision-proof, cover the repro server (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from review. The preview's placeholder token was a fixed constant, so a user who typed that exact string as *another* variable's value would see it rewritten into a `{name}` on substitution — a preview disagreeing with the URI actually submitted. The base is now extended until it appears in neither the template's literal text nor any filled value, making the token unambiguous per call. The new `rfc6570-templates-http.json` preset had no automated coverage: the helper's unit tests assert what `expandTemplate` produces, but not that the produced URI is what a spec-compliant server accepts — which is the whole bug, since the old substitution emitted a URI the Inspector was happy with and the server rejected. The integration test drives both directions against a real server over a real transport, and pins the rejection of the unencoded URI so a regression cannot pass by loosening the server. It resolves the checked-in config rather than calling the factory, so a misspelt preset name fails there too. Signed-off-by: cliffhall --- .../integration/mcp/rfc6570-templates.test.ts | 155 ++++++++++++++++++ clients/web/src/utils/uriTemplate.test.ts | 15 ++ clients/web/src/utils/uriTemplate.ts | 31 +++- 3 files changed, 195 insertions(+), 6 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/rfc6570-templates.test.ts 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..67fc2c6d4 --- /dev/null +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -0,0 +1,155 @@ +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 { + expandTemplate, + templateVariableNames, +} from "../../../utils/uriTemplate"; +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 helper's unit tests assert what `expandTemplate` *produces*; they cannot + * assert that the produced URI is what a spec-compliant server *accepts*. That + * second half is the whole bug: the old string substitution emitted a URI the + * Inspector was perfectly happy with and the server rejected. So this test + * drives both directions against a real server 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 loosening the server. + * + * The server is built by **resolving the checked-in config** rather than by + * calling the fixture factory, so 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"), + 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; + } + + /** + * Read the sole content block as JSON. `contents[]` is a text-or-blob union, + * so narrow rather than cast — a fixture that started returning a blob should + * fail here with a clear message, not at `JSON.parse(undefined)`. + */ + async function readJson( + connected: InspectorClient, + uri: string, + ): Promise { + const { result } = await connected.readResource(uri); + const [content] = result.contents; + expect(content).toBeDefined(); + if (!("text" in content)) { + throw new Error(`expected a text content block for ${uri}`); + } + return JSON.parse(content.text); + } + + it("resolves the preset the config names", () => { + const resolved = resolveConfig(loadConfig(configPath)); + expect(resolved.resourceTemplates?.map((t) => t.uriTemplate)).toEqual([ + "foobar://events/{topic}", + "foobar://events{?topic}", + ]); + }); + + it("advertises both templates, including the query expression", async () => { + const connected = await connectToShowcase(); + const { resourceTemplates } = await connected.listAllResourceTemplates(); + const byName = Object.fromEntries( + resourceTemplates.map((t) => [t.name, t.uriTemplate]), + ); + expect(byName["events-by-path"]).toBe("foobar://events/{topic}"); + expect(byName["events-by-query"]).toBe("foobar://events{?topic}"); + }); + + it("discovers a variable in each expression form", () => { + expect(templateVariableNames("foobar://events/{topic}")).toEqual(["topic"]); + expect(templateVariableNames("foobar://events{?topic}")).toEqual(["topic"]); + }); + + it("reads a reserved-character value through the simple expression", async () => { + const connected = await connectToShowcase(); + const uri = expandTemplate("foobar://events/{topic}", { topic: "foo/bar" }); + expect(uri).toBe("foobar://events/foo%2Fbar"); + + expect(await readJson(connected, uri)).toEqual({ + topic: "foo%2Fbar", + matchedUri: uri, + }); + }); + + it("reads through the query expression", async () => { + const connected = await connectToShowcase(); + const uri = expandTemplate("foobar://events{?topic}", { topic: "weather" }); + expect(uri).toBe("foobar://events?topic=weather"); + + expect(await readJson(connected, uri)).toMatchObject({ topic: "weather" }); + }); + + // The old behavior, pinned from the server's side. If this ever starts + // succeeding, the repro server has stopped reproducing and the test above + // would keep passing while proving nothing. + it("rejects the unencoded URI the old string substitution produced", async () => { + const connected = await connectToShowcase(); + await expect( + connected.readResource("foobar://events/foo/bar"), + ).rejects.toThrow(/not found/i); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 2aaaea55c..73efdc766 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -157,6 +157,21 @@ describe("previewTemplate", () => { ); }); + // A filled value that happens to be the placeholder token must not be + // rewritten into a `{name}` — that would make the preview disagree with the + // URI actually submitted. + it("does not mistake a filled value for its own placeholder", () => { + expect( + previewTemplate("x://{a}/{b}", { a: "zzInspectorUnfilledzz1zz", b: "" }), + ).toBe("x://zzInspectorUnfilledzz1zz/{b}"); + }); + + it("does not mistake the template's own literal text for a placeholder", () => { + expect(previewTemplate("x://zzInspectorUnfilledzz0zz/{a}", { a: "" })).toBe( + "x://zzInspectorUnfilledzz0zz/{a}", + ); + }); + it("returns the raw template when it cannot be parsed", () => { silenceWarn(); expect(previewTemplate("x://{unterminated", {})).toBe("x://{unterminated"); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 877c210b4..3c7f2406e 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -69,14 +69,32 @@ export function expandTemplate( } /** - * A token used to stand in for a variable the user hasn't filled yet, so the - * preview can show `{topic}` in its place instead of silently dropping it. + * Base of the token used to stand in for a variable the user hasn't filled yet, + * so the preview can show `{topic}` in its place instead of silently dropping it. * * Every character is RFC 3986 *unreserved*, so `expand` passes it through * verbatim under every operator and it survives to be swapped back out. + * Percent-encoding can never *produce* this sequence either — it only emits + * `%` plus hex digits, and the base contains characters outside that set — so + * checking the raw inputs for a collision (below) is sufficient. */ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; +/** + * Pick a sentinel base that appears nowhere in the expansion's other inputs. + * + * Without this, a user who types the literal token as *another* variable's + * value would see that value rewritten into a `{placeholder}` on substitution — + * the preview would disagree with the URI actually submitted. Extending the + * base until it is absent makes the token unambiguous for this call. + */ +function uncollidingBase(uriTemplate: string, filled: Record) { + const haystack = [uriTemplate, ...Object.values(filled)].join("\n"); + let base = UNFILLED_SENTINEL; + while (haystack.includes(base)) base += "z"; + return base; +} + /** * Keyed by the variable's position rather than its name: a name may legally * contain characters (`%`-encoded triplets) that `expand` would re-encode, @@ -85,8 +103,8 @@ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; * The trailing delimiter is load-bearing — without it index 1's token would be * a prefix of index 11's, and substituting the first would corrupt the second. */ -function sentinelFor(index: number): string { - return `${UNFILLED_SENTINEL}${index}zz`; +function sentinelFor(base: string, index: number): string { + return `${base}${index}zz`; } function withoutEmptyValues( @@ -112,16 +130,17 @@ export function previewTemplate( const names = uniqueNames(template); const filled = withoutEmptyValues(variables); + const base = uncollidingBase(uriTemplate, filled); const values: Record = { ...filled }; names.forEach((name, index) => { - if (values[name] === undefined) values[name] = sentinelFor(index); + if (values[name] === undefined) values[name] = sentinelFor(base, index); }); let preview = template.expand(values); names.forEach((name, index) => { if (filled[name] !== undefined) return; // The sentinel is unreserved, so it appears in the expansion unencoded. - preview = preview.split(sentinelFor(index)).join(`{${name}}`); + preview = preview.split(sentinelFor(base, index)).join(`{${name}}`); }); return preview; } From bc1237c2cf958e0affb923fffd7de277cc35dd2f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:57:20 -0400 Subject: [PATCH 05/12] fix: preserve RFC 6570's undefined-vs-empty-string distinction (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `expandTemplate` filtered empty values before expanding, which collapsed two states the spec keeps apart — and the SDK already honors the difference: {?topic} with { topic: "" } → ?topic= {?topic} with {} → (omitted) Filtering made a deliberately-empty value unexpressible through the helper. Values now pass through untouched, so the expansion is a faithful RFC 6570 expansion for any caller. Treating an empty string as "not entered yet" is a *preview* concern, not an expansion one: the panel seeds every declared variable with "" and a text input cannot express "defined but empty", so the placeholder substitution keeps that notion — now named `enteredValues` and documented as deliberately divergent, with a test asserting the two helpers differ here on purpose. Signed-off-by: cliffhall --- clients/web/src/utils/uriTemplate.test.ts | 14 +++++++++++-- clients/web/src/utils/uriTemplate.ts | 25 +++++++++++++++++------ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 73efdc766..2f5aeab21 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -87,9 +87,11 @@ describe("expandTemplate", () => { expect(expandTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); }); - it("omits a variable with no value rather than emitting a dangling key", () => { + // RFC 6570 distinguishes an *undefined* variable from one defined as the + // empty string, and the expansion must not collapse the two. + it("keeps a variable defined as the empty string", () => { expect(expandTemplate("foobar://events{?topic}", { topic: "" })).toBe( - "foobar://events", + "foobar://events?topic=", ); }); @@ -108,6 +110,14 @@ describe("expandTemplate", () => { }); describe("previewTemplate", () => { + // The preview's own notion, deliberately different from expandTemplate's: a + // text input cannot express "defined but empty", so within the preview an + // empty string means "not entered yet". + it("treats an empty string as unfilled rather than as an empty expansion", () => { + expect(expandTemplate("x://e{?t}", { t: "" })).toBe("x://e?t="); + expect(previewTemplate("x://e{?t}", { t: "" })).toBe("x://e?t={t}"); + }); + it("shows an unfilled simple variable as its expression", () => { expect(previewTemplate("foobar://events/{topic}", { topic: "" })).toBe( "foobar://events/{topic}", diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 3c7f2406e..a1a7a2819 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -55,9 +55,13 @@ function uniqueNames(template: UriTemplate): string[] { /** * Expands `uriTemplate` per RFC 6570, percent-encoding each value according to - * its expression's operator. Variables with no value are omitted, which is what - * the spec prescribes and what keeps `{?topic}` from expanding to a dangling - * `?topic=`. + * its expression's operator. + * + * Values are passed through untouched, which preserves the spec's distinction + * between an *undefined* variable and one defined as the empty string: an + * absent key drops out of the expansion entirely, while `{ topic: "" }` against + * `{?topic}` yields `?topic=`. Collapsing the two here would silently make a + * deliberately-empty value unexpressible. */ export function expandTemplate( uriTemplate: string, @@ -65,7 +69,7 @@ export function expandTemplate( ): string { const template = parseTemplate(uriTemplate); if (!template) return uriTemplate; - return template.expand(withoutEmptyValues(variables)); + return template.expand(variables); } /** @@ -107,7 +111,16 @@ function sentinelFor(base: string, index: number): string { return `${base}${index}zz`; } -function withoutEmptyValues( +/** + * The subset of `variables` the user has actually typed something into. + * + * This is a *preview* notion, not an RFC 6570 one — `expandTemplate` + * deliberately does not collapse `""` this way. The panel seeds every declared + * variable with `""` and a text input cannot express "defined but empty", so + * within the preview an empty string means "not entered yet" and earns a + * `{name}` placeholder rather than an empty expansion. + */ +function enteredValues( variables: Record, ): Record { return Object.fromEntries( @@ -129,7 +142,7 @@ export function previewTemplate( if (!template) return uriTemplate; const names = uniqueNames(template); - const filled = withoutEmptyValues(variables); + const filled = enteredValues(variables); const base = uncollidingBase(uriTemplate, filled); const values: Record = { ...filled }; names.forEach((name, index) => { From 81fbaacac09a0a6419da6496551fbcd232281141 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 19:16:11 -0400 Subject: [PATCH 06/12] fix: correct multi-name expansion, and make both helpers total (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from review, all confirmed against the SDK. **Multi-name expressions lost their encoding and operator.** The SDK's `expandPart` has a `part.names.length > 1` branch that joins the raw values and returns before reaching the encode/operator switch, so `x://{a,b}` with `a = "foo/bar"` produced `x://foo/bar,x y`, and `{#a,b}` dropped its `#`. Only the query operators (`?`, `&`) take a different, correct path. This PR newly exposes it: the old regex never matched `{a,b}`, so no inputs were rendered, whereas discovery now offers them and lets the URI be submitted. Rather than reimplement RFC 6570, such an expression is rewritten to a single synthetic variable carrying an *array* of the defined values — which the SDK's single-name branch encodes elementwise and joins with the operator's separator, which is exactly the spec's rule. Templates without the shape are untouched. Note the SDK's `UriTemplate.match` cannot match a multi-name expression either (it returns null for every URI), so an SDK-based server can never route one and there is no round trip to add to the integration suite. The client can still emit the spec-correct URI a conforming server needs, and that is what the unit tests assert; the comment records why the coverage sits there. **Expansion could throw after parsing succeeded.** The SDK enforces a 1,000,000-character per-value ceiling at expansion time, and the inputs have no matching limit. The preview expands during render, so a pasted value took the panel down instead of showing a problem. Both helpers are now total: `expandTemplate` returns `null` and the panel disables Read Resource, and `previewTemplate` falls back to the raw template. **The collision loop was quadratic.** A server-supplied template (up to 1 MB) holding the token followed by a long run of `z`s made every extended candidate collide in turn, each rescanning the whole input. It now measures the longest following run in a single pass and clears it by one. Signed-off-by: cliffhall --- .../ResourceTemplatePanel.test.tsx | 30 +++- .../ResourceTemplatePanel.tsx | 12 +- .../integration/mcp/rfc6570-templates.test.ts | 2 + clients/web/src/utils/uriTemplate.test.ts | 78 ++++++++- clients/web/src/utils/uriTemplate.ts | 159 ++++++++++++++++-- 5 files changed, 261 insertions(+), 20 deletions(-) diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 051665249..a2f9f25cd 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -1,7 +1,11 @@ import { describe, it, expect, vi } from "vitest"; import userEvent from "@testing-library/user-event"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; -import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { + renderWithMantine, + screen, + fireEvent, +} from "../../../test/renderWithMantine"; import { ResourceTemplatePanel } from "./ResourceTemplatePanel"; const singleVarTemplate: ResourceTemplate = { @@ -169,6 +173,30 @@ describe("ResourceTemplatePanel", () => { expect(screen.getByText("foobar://events/a%20b")).toBeInTheDocument(); }); + // The SDK refuses a value past its 1,000,000-character ceiling at expansion + // time, and the input has no matching limit. Withhold the request rather than + // send a URI we know is wrong. + it("keeps Read Resource disabled when the value cannot be expanded", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + // fireEvent, not user.type — typing a million characters key by key would + // take longer than the suite's timeout. + fireEvent.change(screen.getByLabelText("topic"), { + target: { value: "z".repeat(1_000_001) }, + }); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + expect(onReadResource).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + it("clears a variable via its Clear button (non-autocomplete branch)", async () => { const user = userEvent.setup(); renderWithMantine( diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index e3266bb90..f5f9e6d9b 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -209,10 +209,18 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - const canSubmit = variableNames.every((n) => variables[n]?.length > 0); + const allFilled = variableNames.every((n) => variables[n]?.length > 0); + // `null` means the template or a value could not be expanded (a malformed + // template, or a value past the SDK's length ceiling). Withhold the request + // rather than send a URI we know is wrong. + const expandedUri = allFilled ? expandTemplate(uriTemplate, variables) : null; + const canSubmit = expandedUri !== null; function handleSubmit() { - onReadResource(expandTemplate(uriTemplate, variables)); + /* v8 ignore next -- unreachable: the button is disabled unless + `expandedUri` is non-null. */ + if (expandedUri === null) return; + onReadResource(expandedUri); } const preview = previewTemplate(uriTemplate, variables); diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts index 67fc2c6d4..adfb41262 100644 --- a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -128,6 +128,7 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { const connected = await connectToShowcase(); const uri = expandTemplate("foobar://events/{topic}", { topic: "foo/bar" }); expect(uri).toBe("foobar://events/foo%2Fbar"); + if (uri === null) throw new Error("unreachable — asserted above"); expect(await readJson(connected, uri)).toEqual({ topic: "foo%2Fbar", @@ -139,6 +140,7 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { const connected = await connectToShowcase(); const uri = expandTemplate("foobar://events{?topic}", { topic: "weather" }); expect(uri).toBe("foobar://events?topic=weather"); + if (uri === null) throw new Error("unreachable — asserted above"); expect(await readJson(connected, uri)).toMatchObject({ topic: "weather" }); }); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 2f5aeab21..67e0f2ceb 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -101,11 +101,58 @@ describe("expandTemplate", () => { ); }); - it("returns the raw template when it cannot be parsed", () => { + // The SDK's multi-name branch skips both encoding and the operator, so these + // expressions are rewritten to a single array-valued variable, which takes + // the branch that applies them. + // + // Covered here rather than in the integration suite on purpose: the SDK's + // `UriTemplate.match` cannot match a multi-name expression *either* (it + // returns null for every URI), so an SDK-based server can never route one and + // there is no round trip to drive. What the client can do is emit the + // spec-correct URI, which is what a conforming server needs — so that is what + // these assert. + describe("multi-name expressions", () => { + it.each([ + ["simple", "x://{a,b}", "x://foo%2Fbar,x%20y"], + // `#`, like `+`, is a *reserved* expansion — `/` survives, a space does not. + ["fragment", "x://e{#a,b}", "x://e#foo/bar,x%20y"], + ["label", "x://e{.a,b}", "x://e.foo%2Fbar.x%20y"], + ["path segment", "x://e{/a,b}", "x://e/foo%2Fbar/x%20y"], + ])("encodes and applies the operator for a %s group", (_l, t, expected) => { + expect(expandTemplate(t, { a: "foo/bar", b: "x y" })).toBe(expected); + }); + + it("preserves reserved characters under the + operator", () => { + expect(expandTemplate("x://{+a,b}", { a: "foo/bar", b: "x y" })).toBe( + "x://foo/bar,x%20y", + ); + }); + + it("still expands a multi-name query expression correctly", () => { + expect(expandTemplate("x://e{?a,b}", { a: "foo/bar", b: "x y" })).toBe( + "x://e?a=foo%2Fbar&b=x%20y", + ); + }); + + it("drops an undefined member from the group", () => { + expect(expandTemplate("x://{a,b}", { b: "two" })).toBe("x://two"); + }); + + it("omits the whole expression when no member is defined", () => { + expect(expandTemplate("x://e{#a,b}", {})).toBe("x://e"); + }); + }); + + it("returns null when the template cannot be parsed", () => { silenceWarn(); - expect(expandTemplate("x://{unterminated", { a: "1" })).toBe( - "x://{unterminated", - ); + expect(expandTemplate("x://{unterminated", { a: "1" })).toBeNull(); + }); + + // Parsing succeeding does not mean expanding will — the SDK checks its + // per-value length ceiling at expansion time. + it("returns null when a value cannot be expanded", () => { + silenceWarn(); + expect(expandTemplate("x://{a}", { a: "z".repeat(1_000_001) })).toBeNull(); }); }); @@ -182,8 +229,31 @@ describe("previewTemplate", () => { ); }); + it("shows a placeholder per member of a multi-name group", () => { + expect(previewTemplate("x://{a,b}", { a: "one", b: "" })).toBe( + "x://one,{b}", + ); + }); + it("returns the raw template when it cannot be parsed", () => { silenceWarn(); expect(previewTemplate("x://{unterminated", {})).toBe("x://{unterminated"); }); + + // Must not throw out of render — the panel expands its preview while + // rendering, so an escaping error would unmount the panel. + it("returns the raw template when a value cannot be expanded", () => { + silenceWarn(); + expect(previewTemplate("x://{a}", { a: "z".repeat(1_000_001) })).toBe( + "x://{a}", + ); + }); + + // A template holding the token followed by a long run of `z`s used to make + // every extended candidate collide in turn, rescanning the whole input each + // time. The base is now cleared in a single pass. + it("resolves a padded-run collision without rescanning", () => { + const template = `x://zzInspectorUnfilledzz${"z".repeat(5000)}/{a}`; + expect(previewTemplate(template, { a: "" })).toBe(template); + }); }); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index a1a7a2819..a33f8e938 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -11,6 +11,11 @@ * the TUI's form builder and `InspectorClient.readResourceFromTemplate` already * use, so all three surfaces agree on what a template's variables are and on * how a value is encoded. + * + * Neither helper throws: a template the SDK rejects, or a value it refuses to + * expand, yields `null` from `expandTemplate` (so the caller can withhold the + * request) and the raw template from `previewTemplate` (which runs during + * render, where a throw would take the panel down with it). */ import { UriTemplate } from "@modelcontextprotocol/client"; @@ -29,6 +34,27 @@ function parseTemplate(uriTemplate: string): UriTemplate | null { } } +/** + * Expand, converting a throw into `null`. + * + * Parsing succeeding does not mean expanding will: the SDK also enforces a + * 1,000,000-character ceiling per *value*, which is checked at expansion time. + * The inputs have no matching limit, so a paste can reach it — and the preview + * expands during render, where an escaping throw unmounts the panel instead of + * showing a problem with the value. + */ +function tryExpand( + template: UriTemplate, + values: Record, +): string | null { + try { + return template.expand(values); + } catch (error) { + console.warn("Failed to expand URI template:", error); + return null; + } +} + /** * The variable names declared by `uriTemplate`, in declaration order — * including those inside non-simple expressions (`{?topic}`, `{+path}`, @@ -53,9 +79,83 @@ function uniqueNames(template: UriTemplate): string[] { return [...new Set(template.variableNames)]; } +/** + * Matches a multi-name expression whose operator is *not* `?` or `&`. + * + * The SDK expands this shape through a branch that returns the raw values + * joined by `,`, skipping both `encodeValue` and the operator prefix — so + * `x://{a,b}` with `a = "foo/bar"` yields `x://foo/bar,x y` rather than + * `x://foo%2Fbar,x%20y`, and `{#a,b}` loses its `#` entirely. Its query + * counterpart (`{?a,b}`) takes a different, correct branch. + * + * `groupMultiNameExpressions` rewrites this shape so the SDK's *single*-name + * branch — which does apply the operator's encoding and separator, over an + * array value — handles it instead. + */ +const MULTI_NAME_EXPRESSION = /\{([+#./;]?)([^{}?&][^{}]*)\}/g; + +/** Name given to the synthetic single variable a rewritten group expands from. */ +function groupName(index: number): string { + return `__inspectorGroup${index}__`; +} + +interface GroupedTemplate { + /** The rewritten template string, safe to hand to the SDK. */ + text: string; + /** Synthetic variable name → the real names it stands for, in order. */ + groups: Map; +} + +/** + * Rewrite each multi-name non-query expression into a single synthetic + * variable, so the SDK applies the operator's encoding and separator. + * + * `x://{a,b}` becomes `x://{__inspectorGroup0__}`, expanded with an *array* of + * the defined values — which the SDK's single-name branch encodes elementwise + * and joins with the operator's separator, exactly as RFC 6570 prescribes. + * Templates without this shape are returned unchanged and pay nothing. + */ +function groupMultiNameExpressions(uriTemplate: string): GroupedTemplate { + const groups = new Map(); + const text = uriTemplate.replace( + MULTI_NAME_EXPRESSION, + (match, operator: string, body: string) => { + if (!body.includes(",")) return match; + const names = body.split(",").map((name) => name.trim()); + const synthetic = groupName(groups.size); + groups.set(synthetic, names); + return `{${operator}${synthetic}}`; + }, + ); + return { text, groups }; +} + +/** + * Project the caller's per-name values onto the synthetic group variables. + * + * A group is omitted when none of its names has a value, matching RFC 6570's + * rule that an expression with no defined variable contributes nothing. + */ +function applyGroups( + variables: Record, + groups: Map, +): Record { + if (groups.size === 0) return variables; + const projected: Record = { ...variables }; + for (const [synthetic, names] of groups) { + const present = names + .filter((name) => variables[name] !== undefined) + .map((name) => variables[name]); + if (present.length > 0) projected[synthetic] = present; + } + return projected; +} + /** * Expands `uriTemplate` per RFC 6570, percent-encoding each value according to - * its expression's operator. + * its expression's operator. Returns `null` when the template cannot be parsed + * or a value cannot be expanded, so a caller can decline to issue the request + * rather than send a URI it knows is wrong. * * Values are passed through untouched, which preserves the spec's distinction * between an *undefined* variable and one defined as the empty string: an @@ -66,10 +166,11 @@ function uniqueNames(template: UriTemplate): string[] { export function expandTemplate( uriTemplate: string, variables: Record, -): string { - const template = parseTemplate(uriTemplate); - if (!template) return uriTemplate; - return template.expand(variables); +): string | null { + const { text, groups } = groupMultiNameExpressions(uriTemplate); + const template = parseTemplate(text); + if (!template) return null; + return tryExpand(template, applyGroups(variables, groups)); } /** @@ -89,14 +190,33 @@ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; * * Without this, a user who types the literal token as *another* variable's * value would see that value rewritten into a `{placeholder}` on substitution — - * the preview would disagree with the URI actually submitted. Extending the - * base until it is absent makes the token unambiguous for this call. + * the preview would disagree with the URI actually submitted. + * + * Done in one pass rather than by extending the base until it stops colliding: + * the template is server-supplied and may be up to 1 MB, and a template holding + * the token followed by a long run of `z`s would make every extended candidate + * collide in turn, each rescanning the whole input — quadratic work on the + * render thread. Instead, measure the longest run of `z` that follows any + * occurrence and clear it by one, which no occurrence can then match. */ function uncollidingBase(uriTemplate: string, filled: Record) { const haystack = [uriTemplate, ...Object.values(filled)].join("\n"); - let base = UNFILLED_SENTINEL; - while (haystack.includes(base)) base += "z"; - return base; + let longestRun = -1; + for ( + let at = haystack.indexOf(UNFILLED_SENTINEL); + at !== -1; + at = haystack.indexOf(UNFILLED_SENTINEL, at + UNFILLED_SENTINEL.length) + ) { + let run = 0; + let cursor = at + UNFILLED_SENTINEL.length; + while (haystack[cursor] === "z") { + run++; + cursor++; + } + if (run > longestRun) longestRun = run; + } + // -1 means the token is absent, so the base needs no padding at all. + return UNFILLED_SENTINEL + "z".repeat(longestRun + 1); } /** @@ -133,15 +253,25 @@ function enteredValues( * filled variables are expanded (and encoded) exactly as they would be on the * wire, while unfilled ones are shown as `{name}` so the shape of the URI stays * legible while the form is still being completed. + * + * Falls back to the raw template when it cannot be parsed or expanded — this + * runs during render, so it must not throw. */ export function previewTemplate( uriTemplate: string, variables: Record, ): string { - const template = parseTemplate(uriTemplate); + const { text, groups } = groupMultiNameExpressions(uriTemplate); + const template = parseTemplate(text); if (!template) return uriTemplate; - const names = uniqueNames(template); + // Map each synthetic group back to the real names it stands for, so the + // placeholders the user sees are the ones the form is asking them for. + const names = [ + ...new Set( + uniqueNames(template).flatMap((name) => groups.get(name) ?? [name]), + ), + ]; const filled = enteredValues(variables); const base = uncollidingBase(uriTemplate, filled); const values: Record = { ...filled }; @@ -149,7 +279,10 @@ export function previewTemplate( if (values[name] === undefined) values[name] = sentinelFor(base, index); }); - let preview = template.expand(values); + const expanded = tryExpand(template, applyGroups(values, groups)); + if (expanded === null) return uriTemplate; + + let preview = expanded; names.forEach((name, index) => { if (filled[name] !== undefined) return; // The sentinel is unreserved, so it appears in the expansion unencoded. From 722f24d2e8a7a851a0032b406704b141ecfc553f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 19:32:24 -0400 Subject: [PATCH 07/12] fix: correct group member names, synthetic collisions, ; and overlap (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-ups, all in the multi-name machinery added last round, all confirmed against the SDK. **Exploded members were looked up under the wrong key.** `variableNames` strips a trailing `*` (`{/a*,b}` → `["a","b"]`) so the form stores `a`, while the group kept `a*` and `applyGroups` found nothing — silently dropping a filled value. Member names are now normalized exactly as the SDK does: strip a trailing `*`, keep a prefix modifier. **The synthetic group name could collide with a real variable.** A template declaring `__inspectorGroup0__` had the group overwrite the user's value and emit itself twice. The prefix is now padded past any occurrence in the template. **The `;` operator was made worse, not better.** The SDK does not implement it — `{;a}` parses as a variable literally named ";a" and expands to the bare value, dropping the required `;a=` — and the rewrite turned `{;a,b}` into a synthetic that expanded to nothing. `;` is out of the rewrite's operator class, and a template using it is now declined outright: `expandTemplate` returns null (so the panel withholds the request rather than sending a knowingly invalid URI) and the preview shows the template as declared. **The collision scan skipped overlapping occurrences.** The token begins and ends with `zz`, so it can overlap itself, and `zzInspectorUnfilledzzInspectorUnfilledzzz0zz` holds a second occurrence whose trailing run is the longer one. Advancing by the token's length missed it and chose a colliding placeholder. The scan advances one character at a time, and the logic is now one helper shared with the group-prefix padding so the two cannot drift. Signed-off-by: cliffhall --- clients/web/src/utils/uriTemplate.test.ts | 53 +++++++++++++ clients/web/src/utils/uriTemplate.ts | 95 +++++++++++++++++++---- 2 files changed, 135 insertions(+), 13 deletions(-) diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 67e0f2ceb..136be014d 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -141,6 +141,50 @@ describe("expandTemplate", () => { it("omits the whole expression when no member is defined", () => { expect(expandTemplate("x://e{#a,b}", {})).toBe("x://e"); }); + + // `variableNames` strips a trailing `*`, so the form stores `a`. A group + // that kept `a*` would look up a key the form never sets and silently drop + // a filled value. + it("matches the SDK's name for an exploded member", () => { + expect(templateVariableNames("x://e{/a*,b}")).toEqual(["a", "b"]); + expect(expandTemplate("x://e{/a*,b}", { a: "one", b: "two" })).toBe( + "x://e/one/two", + ); + }); + + // A template may legitimately declare a variable named like the synthetic + // one; the rewrite must not overwrite the user's value with the group's. + it("does not collide with a variable named like the synthetic one", () => { + expect( + expandTemplate("x://{a,b}/{__inspectorGroup0__}", { + a: "one", + b: "two", + __inspectorGroup0__: "mine", + }), + ).toBe("x://one,two/mine"); + }); + }); + + // The SDK does not implement `;`: it reads `{;a}` as a variable literally + // named ";a" and expands it to the bare value, dropping the required `;a=`. + // No arrangement of its branches produces the right output, so decline rather + // than hand back a URI known to be invalid. + describe("the unsupported ; (path-parameter) operator", () => { + it.each([ + ["single-name", "x://e{;a}"], + ["multi-name", "x://e{;a,b}"], + ])("returns null for a %s expression", (_label, template) => { + silenceWarn(); + expect(expandTemplate(template, { a: "1", b: "2" })).toBeNull(); + }); + + it("previews it as the template the server declared", () => { + expect(previewTemplate("x://e{;a,b}", { a: "1" })).toBe("x://e{;a,b}"); + }); + + it("does not mistake a literal semicolon for the operator", () => { + expect(expandTemplate("x://e;q/{a}", { a: "1" })).toBe("x://e;q/1"); + }); }); it("returns null when the template cannot be parsed", () => { @@ -256,4 +300,13 @@ describe("previewTemplate", () => { const template = `x://zzInspectorUnfilledzz${"z".repeat(5000)}/{a}`; expect(previewTemplate(template, { a: "" })).toBe(template); }); + + // The token starts and ends with `zz`, so it can overlap itself: this literal + // holds a second occurrence at index 19 whose trailing run is the longer one. + // A scan advancing by the token's length would miss it and pick a colliding + // placeholder, rewriting literal URI text into `{a}`. + it("measures an overlapping occurrence of the token", () => { + const template = "x://zzInspectorUnfilledzzInspectorUnfilledzzz0zz/{a}"; + expect(previewTemplate(template, { a: "" })).toBe(template); + }); }); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index a33f8e938..8a3f2d4ad 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -92,13 +92,42 @@ function uniqueNames(template: UriTemplate): string[] { * branch — which does apply the operator's encoding and separator, over an * array value — handles it instead. */ -const MULTI_NAME_EXPRESSION = /\{([+#./;]?)([^{}?&][^{}]*)\}/g; +const MULTI_NAME_EXPRESSION = /\{([+#./]?)([^{}?&;][^{}]*)\}/g; -/** Name given to the synthetic single variable a rewritten group expands from. */ -function groupName(index: number): string { - return `__inspectorGroup${index}__`; +/** + * RFC 6570's path-parameter operator, which the SDK does not implement: it + * treats `{;a}` as a variable literally named `";a"` and expands it to the bare + * value, dropping the required `;a=`. There is no arrangement of the SDK's own + * branches that produces the right output, so a template using it cannot be + * expanded correctly here — `expandTemplate` declines rather than returning a + * URI known to be wrong, and the panel keeps its submit disabled. + */ +const PATH_PARAM_EXPRESSION = /\{;/; + +/** + * The name the SDK will parse out of a group member, so `applyGroups` looks up + * the same key the form stores. + * + * It strips a trailing explode modifier (`{a*}` → `a`) but *keeps* a prefix + * modifier (`{a:3}` → `a:3`, see `templateVariableNames`). Mirroring it exactly + * is the point: normalizing differently would silently drop a filled value. + */ +function memberName(raw: string): string { + return raw.trim().replace(/\*$/, ""); } +/** + * Name for the synthetic variable a rewritten group expands from, chosen so it + * cannot collide with a variable the template already declares — otherwise a + * template like `x://{a,b}/{__inspectorGroup0__}` would have the group's value + * overwrite the user's own, emitting the group twice. + */ +function groupName(prefix: string, index: number): string { + return `${prefix}${index}__`; +} + +const GROUP_PREFIX = "__inspectorGroup"; + interface GroupedTemplate { /** The rewritten template string, safe to hand to the SDK. */ text: string; @@ -117,12 +146,15 @@ interface GroupedTemplate { */ function groupMultiNameExpressions(uriTemplate: string): GroupedTemplate { const groups = new Map(); + // The prefix must not occur in the template, or a template declaring a + // variable of that name would have the group overwrite the user's value. + const prefix = padPastCollisions(GROUP_PREFIX, "_", uriTemplate); const text = uriTemplate.replace( MULTI_NAME_EXPRESSION, (match, operator: string, body: string) => { if (!body.includes(",")) return match; - const names = body.split(",").map((name) => name.trim()); - const synthetic = groupName(groups.size); + const names = body.split(",").map(memberName); + const synthetic = groupName(prefix, groups.size); groups.set(synthetic, names); return `{${operator}${synthetic}}`; }, @@ -167,6 +199,12 @@ export function expandTemplate( uriTemplate: string, variables: Record, ): string | null { + if (PATH_PARAM_EXPRESSION.test(uriTemplate)) { + console.warn( + `Cannot expand "${uriTemplate}": the ; (path-parameter) operator is unsupported.`, + ); + return null; + } const { text, groups } = groupMultiNameExpressions(uriTemplate); const template = parseTemplate(text); if (!template) return null; @@ -200,23 +238,51 @@ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; * occurrence and clear it by one, which no occurrence can then match. */ function uncollidingBase(uriTemplate: string, filled: Record) { - const haystack = [uriTemplate, ...Object.values(filled)].join("\n"); + return padPastCollisions( + UNFILLED_SENTINEL, + "z", + [uriTemplate, ...Object.values(filled)].join("\n"), + ); +} + +/** + * Extend `token` with `pad` characters until it cannot occur in `haystack`. + * + * Done by measuring, in a single pass, the longest run of `pad` that follows + * any occurrence, then clearing it by one. The obvious `while + * (haystack.includes(candidate)) candidate += pad` is quadratic on exactly the + * input that motivates the check: a haystack holding the token followed by a + * long run of `pad` makes every successive candidate collide, each rescanning + * the whole string — and the template is server-supplied, up to 1 MB, scanned + * on the render thread. + * + * The scan advances one character at a time rather than by the token's length, + * because a token that begins and ends with the same characters can overlap + * itself: `zzInspectorUnfilledzzInspectorUnfilledzzz0zz` holds a second + * occurrence at index 19 whose trailing run is longer than the first's. Skipping + * it would choose a colliding token after all. + */ +function padPastCollisions( + token: string, + pad: string, + haystack: string, +): string { let longestRun = -1; for ( - let at = haystack.indexOf(UNFILLED_SENTINEL); + let at = haystack.indexOf(token); at !== -1; - at = haystack.indexOf(UNFILLED_SENTINEL, at + UNFILLED_SENTINEL.length) + at = haystack.indexOf(token, at + 1) ) { let run = 0; - let cursor = at + UNFILLED_SENTINEL.length; - while (haystack[cursor] === "z") { + let cursor = at + token.length; + while (haystack[cursor] === pad) { run++; cursor++; } if (run > longestRun) longestRun = run; } - // -1 means the token is absent, so the base needs no padding at all. - return UNFILLED_SENTINEL + "z".repeat(longestRun + 1); + // -1 means the token is absent, so it needs no padding at all. + return token + pad.repeat(longestRun + 1); } /** @@ -261,6 +327,9 @@ export function previewTemplate( uriTemplate: string, variables: Record, ): string { + // Nothing truthful to render for an expression the expander declines, so + // show the template as the server declared it. + if (PATH_PARAM_EXPRESSION.test(uriTemplate)) return uriTemplate; const { text, groups } = groupMultiNameExpressions(uriTemplate); const template = parseTemplate(text); if (!template) return uriTemplate; From 00549c949c40641d46fad40f16313f099d86cc5b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 20:45:29 -0400 Subject: [PATCH 08/12] refactor: move the RFC 6570 helper into core/uri, share it across clients (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The multi-name and `;` corrections were web-only, which undercut this PR's own premise. The TUI and CLI submit through `InspectorClient.readResourceFromTemplate`, which still called the SDK directly and therefore took the raw multi-name branch — so `x://{a,b}` resolved to `x://foo%2Fbar,x%20y` in the web panel and `x://foo/bar,x y` everywhere else. The helper now lives in `core/uri/uriTemplate.ts` and all three consumers route through it: - the web `ResourceTemplatePanel` (discovery, expansion, preview), - `InspectorClient.readResourceFromTemplate` (the CLI/TUI submit path) — the last direct `new UriTemplate` call in `core/` is gone, - the TUI's `uriTemplateToForm`, which now shares *discovery* too, so both clients offer the same fields for a template: the variables inside non-simple expressions, and one field rather than two for a repeated name. `expandTemplate` returning null becomes a thrown error on the client path, which keeps the existing "Failed to expand URI template" contract its callers and tests rely on. `core/uri/**` is added to the web coverage `include` so it stays under the ≥90 gate, its tests move to `clients/web/src/test/core/uri/` per the placement rule, and the structure trees plus the coverage-include list in AGENTS.md and the README are updated to match. Signed-off-by: cliffhall --- AGENTS.md | 14 ++++++-- README.md | 5 +-- .../tui/__tests__/uriTemplateToForm.test.ts | 21 ++++++++--- clients/tui/src/utils/uriTemplateToForm.ts | 36 +++++++------------ .../ResourceTemplatePanel.tsx | 2 +- .../core/uri}/uriTemplate.test.ts | 2 +- .../integration/mcp/rfc6570-templates.test.ts | 19 +++++++++- clients/web/vite.config.ts | 1 + core/mcp/inspectorClient.ts | 17 +++++---- .../web/src/utils => core/uri}/uriTemplate.ts | 15 +++++--- 10 files changed, 83 insertions(+), 49 deletions(-) rename clients/web/src/{utils => test/core/uri}/uriTemplate.test.ts (99%) rename {clients/web/src/utils => core/uri}/uriTemplate.ts (95%) diff --git a/AGENTS.md b/AGENTS.md index d07e84f4d..c9a12d3b3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -98,7 +98,17 @@ v2/main/ │ │ # callback listener; also stripBrackets. Used across │ │ # clients/web/server, clients/cli, and core/auth/node — #1795) │ ├── react/ # React hooks over the state stores -│ └── storage/ # File I/O helpers (store-io.ts) used by OAuth persist backends +│ ├── storage/ # File I/O helpers (store-io.ts) used by OAuth persist backends +│ └── uri/ # RFC 6570 URI Template discovery/expansion/preview +│ # (uriTemplate.ts) — wraps the SDK's UriTemplate and +│ # corrects the two places its expander departs from the +│ # RFC (multi-name expressions skip encoding + the +│ # operator; `;` is unimplemented). Shared by the web +│ # ResourceTemplatePanel, the TUI's uriTemplateToForm, +│ # and InspectorClient.readResourceFromTemplate, so a +│ # template cannot resolve differently per client — #1919. +│ # Gated by the web coverage `include`; tests live in +│ # clients/web/src/test/core/uri/. ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests. │ ├── src/ # TypeScript sources. (modern-tasks.ts: SEP-2663 modern │ │ # Tasks extension runtime + tasks/* Express interceptor @@ -666,7 +676,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **The test tiers, shallowest first:** unit (`test`, per client) → web integration (`test:integration`, real transports/servers) → out-of-process (`clients/cli/__tests__/e2e.test.ts`, spawns the built binary) → smokes through the built launcher (`npm run smoke`) → Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts` (it's part of cli's `test`), but **not** web's integration project, which runs inside the `coverage` gate. Everything from `smoke` rightward is `npm run ci` only, and is described under [Mandatory pre-push gate](#mandatory-pre-push-gate). - The repo root has no aggregate `test` script — each client self-validates, so run `npm run validate` from the root (all clients, fast) or `cd clients/ && npm run validate` (one client). Each client still exposes its own `test` / `test:coverage` for quick iteration. - **`validate` is fast: it runs `test`, not `test:coverage`.** The coverage gate (slower — adds v8 instrumentation, and for web the integration project) is a **separate** top-level `npm run coverage` (and per-client `coverage:web` / `coverage:cli` / `coverage:tui` / `coverage:launcher`, each delegating to that client's `test:coverage`). Run `npm run coverage` when you want to reproduce the gate locally before pushing. **CI runs `coverage`** on every push (#1550): the per-file ≥90 gate is CI-enforced, so a PR that drops any file below 90 on lines/statements/functions/branches fails the job. CI runs `validate` (fast) for format/lint/build/unit tests, then `coverage` for the instrumented gate. Because web's `test:coverage` already runs the integration project, CI has no separate `test:integration` step — the integration paths are exercised inside the coverage gate. -- Each client's `test:coverage` enforces a **uniform per-file gate of ≥ 90 on all four dimensions** — lines, statements, functions, and branches — across `clients/web`, `clients/cli`, `clients/tui`, and `clients/launcher` (CI enforces this gate). This is the result of a codebase-wide audit: the branch floor was first lifted 50 → 70 for web (#1271), then the whole gate raised to 90 with real tests added for every outlier. Genuinely-unreachable branches are **not** waved through by lowering the gate — they are annotated at the source with a justified `/* v8 ignore … -- */` comment. Acceptable reasons are happy-dom-inherent paths (Mantine portal mount points, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards (e.g. a `?? fallback` for a value the types guarantee non-null, or a `Select.onChange` receiving a value outside the allowed list). New code must clear 90 on every dimension; reach for a justified `v8 ignore` only when a branch is genuinely impossible to exercise. The web coverage `include` (in `clients/web/vite.config.ts`) covers the shared `core/` runtime consumed by the browser — `core/mcp`, `core/react`, `core/auth`, `core/storage`, `core/logging`, `core/node`, **`core/json`, and `core/client`** (the last two folded in by #1689). When adding a `core/json/*` or `core/client/*` module, its tests live under `clients/web/src/test/core/…` and are gated the same ≥90 way. +- Each client's `test:coverage` enforces a **uniform per-file gate of ≥ 90 on all four dimensions** — lines, statements, functions, and branches — across `clients/web`, `clients/cli`, `clients/tui`, and `clients/launcher` (CI enforces this gate). This is the result of a codebase-wide audit: the branch floor was first lifted 50 → 70 for web (#1271), then the whole gate raised to 90 with real tests added for every outlier. Genuinely-unreachable branches are **not** waved through by lowering the gate — they are annotated at the source with a justified `/* v8 ignore … -- */` comment. Acceptable reasons are happy-dom-inherent paths (Mantine portal mount points, `useMediaQuery` fallbacks, `typeof window` SSR guards), React StrictMode effect-replay blocks, and provably-dead defensive guards (e.g. a `?? fallback` for a value the types guarantee non-null, or a `Select.onChange` receiving a value outside the allowed list). New code must clear 90 on every dimension; reach for a justified `v8 ignore` only when a branch is genuinely impossible to exercise. The web coverage `include` (in `clients/web/vite.config.ts`) covers the shared `core/` runtime consumed by the browser — `core/mcp`, `core/react`, `core/auth`, `core/storage`, `core/logging`, `core/node`, **`core/json`, and `core/client`** (the last two folded in by #1689), plus **`core/uri`** (#1919). When adding a `core/json/*`, `core/client/*`, or `core/uri/*` module, its tests live under `clients/web/src/test/core/…` and are gated the same ≥90 way. - The **same per-file gate** is enforced for the CLI and TUI (#1484), not just web: - **CLI** (`clients/cli`): tests run **in-process** by importing `runCli()` (see `__tests__/helpers/cli-runner.ts`) so `clients/cli/src` is measured under v8 instrumentation. A thin out-of-process layer (`__tests__/e2e.test.ts` + `scripts/smoke-cli.mjs`) still spawns the built binary for the shebang/`process.exit` paths; `src/index.ts` (binary bootstrap) is the only coverage exclusion. `commander` uses `.exitOverride()` so a parse error throws instead of tearing down the test worker. - **TUI** (`clients/tui`): the gate now covers **all of `src/**`, React surface included** — the former interim exclusion of the Ink components, `App.tsx`, and `hooks/` was lifted in #1501. Components mount through `ink-testing-library` with the `ink-scroll-view` / `ink-form` passthrough doubles in `__tests__/helpers/`, `App.tsx` mounts against a controllable mock of the `@inspector/core` surface, and keypresses are driven through stdin. The **only** coverage exclusion left in `clients/tui/vitest.config.ts` is `src/tui-servers.ts` — a pure re-export + type alias of core's server resolver with no runtime statements of its own (the logic is measured in `core/` via the web suite; `tui-servers.test.ts` still exercises it behaviorally, and it's excluded only so it doesn't surface as a misleading 0/0 row). Any new logic under `clients/tui/src`, React or not, is held to the gate automatically. diff --git a/README.md b/README.md index 198692ebe..0821d4f90 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,8 @@ inspector/ │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import │ ├── 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 +│ ├── storage/ # File I/O helpers for the OAuth persist backends +│ └── uri/ # RFC 6570 URI Template discovery/expansion/preview, shared by all clients ├── test-servers/ # Composable MCP test servers + fixtures used by integration tests ├── scripts/ # Root build/verify tooling (install cascade, smokes, verify-build-gate, verify-format-coverage, verify-dep-lockstep, pack:verify) ├── docs/ # Task-oriented guides (v1→v2 migration, server configuration, MCP App review, launcher/config plan) @@ -246,7 +247,7 @@ Open the Resources tab and select **events-by-path**, enter `foo/bar` for `topic Then select **events-by-query**: it must render a `topic` input at all. The old scan was `/\{(\w+)\}/g`, which sees only bare `{name}` expressions, so a query expression declared a variable the form never offered. -Both surfaces now go through the SDK's `UriTemplate` — the same RFC 6570 implementation the TUI's form builder and `InspectorClient.readResourceFromTemplate` already used — so web, CLI, and TUI agree on a template's variables and on how a value is encoded. +All three clients now go through one shared helper, [`core/uri/uriTemplate.ts`](./core/uri/uriTemplate.ts) — the web panel, the TUI's form builder, and `InspectorClient.readResourceFromTemplate` — so a template cannot resolve differently depending on where it is driven from. It wraps the SDK's `UriTemplate` and corrects the two places that expander departs from RFC 6570: a multi-name expression (`{a,b}`) skips both encoding and its operator, and the `;` path-parameter operator is unimplemented (such a template is declined rather than expanded to a knowingly invalid URI). #### Advertised extensions diff --git a/clients/tui/__tests__/uriTemplateToForm.test.ts b/clients/tui/__tests__/uriTemplateToForm.test.ts index 1c522c659..a888b8cf4 100644 --- a/clients/tui/__tests__/uriTemplateToForm.test.ts +++ b/clients/tui/__tests__/uriTemplateToForm.test.ts @@ -23,13 +23,24 @@ describe("uriTemplateToForm", () => { }); it("logs and returns an empty form when the template cannot be parsed", () => { - const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + // The shared core/uri helper warns and yields no names; this file no longer + // does its own try/catch, so the assertion is on that warning (#1919). + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}); const form = uriTemplateToForm("file:///{unclosed", "broken"); - expect(errorSpy).toHaveBeenCalledWith( - "Failed to parse URI template:", - expect.any(Error), - ); + expect(warnSpy).toHaveBeenCalled(); expect(form.sections[0]!.fields).toEqual([]); }); + + // Shared with the web panel's field list: the old scan saw only bare + // `{name}` expressions, and a repeated name produced two identical fields. + it("creates a field for a variable inside a query expression", () => { + const form = uriTemplateToForm("foobar://events{?topic}", "events"); + expect(form.sections[0]!.fields.map((f) => f.name)).toEqual(["topic"]); + }); + + it("creates one field for a name repeated across expressions", () => { + const form = uriTemplateToForm("x://{a}/{b}/{a}", "repeat"); + expect(form.sections[0]!.fields.map((f) => f.name)).toEqual(["a", "b"]); + }); }); diff --git a/clients/tui/src/utils/uriTemplateToForm.ts b/clients/tui/src/utils/uriTemplateToForm.ts index c8a027e9c..b8d6a9424 100644 --- a/clients/tui/src/utils/uriTemplateToForm.ts +++ b/clients/tui/src/utils/uriTemplateToForm.ts @@ -3,7 +3,7 @@ */ import type { FormStructure, FormSection, FormField } from "ink-form"; -import { UriTemplate } from "@modelcontextprotocol/client"; +import { templateVariableNames } from "@inspector/core/uri/uriTemplate.js"; /** * Converts a URI Template to ink-form structure @@ -12,28 +12,18 @@ export function uriTemplateToForm( uriTemplate: string, templateName: string, ): FormStructure { - const 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); - } - } catch (error) { - // If parsing fails, return empty form - console.error("Failed to parse URI template:", error); - } + // Shared with the web panel's field list (#1919), so the two clients offer + // the same inputs for a given template — including the variables inside + // non-simple expressions, and one field (not two) for a repeated name. It + // does not throw: a malformed template yields no names, so the form is empty. + const fields: FormField[] = templateVariableNames(uriTemplate).map( + (variableName) => ({ + name: variableName, + label: variableName, + type: "string", + required: false, // URI template variables are typically optional + }), + ); const sections: FormSection[] = [ { diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index f5f9e6d9b..cd9d5b18f 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -18,7 +18,7 @@ import { expandTemplate, previewTemplate, templateVariableNames, -} from "../../../utils/uriTemplate"; +} from "@inspector/core/uri/uriTemplate.js"; export interface ResourceTemplatePanelProps { template: ResourceTemplate; diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/test/core/uri/uriTemplate.test.ts similarity index 99% rename from clients/web/src/utils/uriTemplate.test.ts rename to clients/web/src/test/core/uri/uriTemplate.test.ts index 136be014d..584e7d965 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/test/core/uri/uriTemplate.test.ts @@ -3,7 +3,7 @@ import { expandTemplate, previewTemplate, templateVariableNames, -} from "./uriTemplate"; +} from "@inspector/core/uri/uriTemplate.js"; afterEach(() => { vi.restoreAllMocks(); diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts index adfb41262..555395fc0 100644 --- a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -6,7 +6,7 @@ import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; import { expandTemplate, templateVariableNames, -} from "../../../utils/uriTemplate"; +} from "@inspector/core/uri/uriTemplate.js"; import { createTestServerHttp, type TestServerHttp, @@ -145,6 +145,23 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => { expect(await readJson(connected, uri)).toMatchObject({ topic: "weather" }); }); + // `readResourceFromTemplate` is the path the TUI and CLI submit through, and + // it used to call the SDK directly — so the same template could resolve one + // way in the web panel and another here. Both now route through + // `core/uri/uriTemplate`; this asserts the client-level path end to end. + it("encodes correctly through readResourceFromTemplate", async () => { + const connected = await connectToShowcase(); + const invocation = await connected.readResourceFromTemplate( + "foobar://events/{topic}", + { topic: "foo/bar" }, + ); + expect(invocation.expandedUri).toBe("foobar://events/foo%2Fbar"); + // Same URI the panel's `expandTemplate` produces — the two cannot drift. + expect(invocation.expandedUri).toBe( + expandTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ); + }); + // The old behavior, pinned from the server's side. If this ever starts // succeeding, the repro server has stopped reproducing and the test above // would keep passing while proving nothing. diff --git a/clients/web/vite.config.ts b/clients/web/vite.config.ts index a31c01693..178e3bbe7 100644 --- a/clients/web/vite.config.ts +++ b/clients/web/vite.config.ts @@ -212,6 +212,7 @@ export default defineConfig(({ command }) => { path.join(repoRoot, "core/storage/**/*.{ts,tsx}"), path.join(repoRoot, "core/logging/**/*.{ts,tsx}"), path.join(repoRoot, "core/node/**/*.{ts,tsx}"), + path.join(repoRoot, "core/uri/**/*.{ts,tsx}"), ], exclude: [ "**/*.stories.{ts,tsx}", diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index a635582d0..e987f5344 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -190,7 +190,7 @@ import { convertToolParameters, convertPromptArguments, } from "../json/jsonUtils.js"; -import { UriTemplate } from "@modelcontextprotocol/client"; +import { expandTemplate } from "../uri/uriTemplate.js"; import { InspectorClientEventTarget, type TaskWithOptionalCreatedAt, @@ -4971,15 +4971,14 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; - // Expand the template's uriTemplate using the provided params - let expandedUri: string; - try { - const uriTemplate = new UriTemplate(uriTemplateString); - expandedUri = uriTemplate.expand(params); - } catch (error) { + // Expand through the shared RFC 6570 helper rather than the SDK directly, + // so this path and the web panel's cannot resolve the same template to + // different URIs (#1919). `null` means the template or a value could not be + // expanded correctly — better to fail loudly than to read a wrong URI. + const expandedUri = expandTemplate(uriTemplateString, params); + if (expandedUri === null) { throw new Error( - `Failed to expand URI template "${uriTemplate}": ${error instanceof Error ? error.message : String(error)}`, - { cause: error }, + `Failed to expand URI template "${uriTemplateString}": the template or one of its values cannot be expanded per RFC 6570.`, ); } diff --git a/clients/web/src/utils/uriTemplate.ts b/core/uri/uriTemplate.ts similarity index 95% rename from clients/web/src/utils/uriTemplate.ts rename to core/uri/uriTemplate.ts index 8a3f2d4ad..087800199 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/core/uri/uriTemplate.ts @@ -1,5 +1,7 @@ /** - * RFC 6570 URI Template helpers for the Resources screen. + * RFC 6570 URI Template discovery, expansion, and preview — shared by every + * client so a template cannot resolve differently depending on where it is + * driven from. * * The web client used to discover variables with `/\{(\w+)\}/g` and expand them * with a plain `String.replace`. That only ever saw simple expressions — a @@ -7,10 +9,13 @@ * values verbatim, so a `topic` of `foo/bar` silently became a second path * segment instead of `foo%2Fbar` (#1919). * - * These wrap the SDK's `UriTemplate`, which is the same RFC 6570 implementation - * the TUI's form builder and `InspectorClient.readResourceFromTemplate` already - * use, so all three surfaces agree on what a template's variables are and on - * how a value is encoded. + * These wrap the SDK's `UriTemplate`, correcting the two places its expander + * departs from RFC 6570 (see `groupMultiNameExpressions` and + * `PATH_PARAM_EXPRESSION`). Living in `core/` is what makes the correction + * uniform: the web panel, the TUI's form builder, and + * `InspectorClient.readResourceFromTemplate` all route through here rather than + * calling the SDK directly, so web, CLI, and TUI agree on what a template's + * variables are and on how a value is encoded. * * Neither helper throws: a template the SDK rejects, or a value it refuses to * expand, yields `null` from `expandTemplate` (so the caller can withhold the From 892914ff53afa113d96400b5006d29607f3d279d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:03:40 -0400 Subject: [PATCH 09/12] fix: reject empty expressions, and stop a synthetic key standing in for a group (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups, both confirmed against the SDK. **An empty expression was accepted.** `new UriTemplate("x://{}")` parses, reports no variable names, and expands to `x://` — so the panel rendered no inputs, its "every variable is filled" check was vacuously true, and Read Resource would submit a URI that is not the template the server advertised. `{ }`, `{,}`, and `{a,}` are the same defect with some members missing. Validation now runs on the template *as the server declared it*, before any rewriting: putting the check inside `parseTemplate` was not enough, because the multi-name grouping folds `{a,}` into a synthetic single-name expression and masks the empty member. The `;` check moves into the same place, so the two "cannot handle this template" cases are one function with one reason string rather than two guards at two layers. **A caller value could stand in for a group.** `applyGroups` starts from a copy of every supplied variable, so a caller passing a key equal to the generated synthetic name had it expand the group's expression: `expandTemplate("x://{a,b}", { __inspectorGroup0__: "injected" })` emitted `x://injected` for an expression whose real members are all undefined, where the SDK ignores undeclared variables entirely. The omitted-group branch now deletes the key rather than leaving the caller's value under it. Signed-off-by: cliffhall --- .../ResourceTemplatePanel.test.tsx | 20 +++++ .../web/src/test/core/uri/uriTemplate.test.ts | 25 ++++++ core/uri/uriTemplate.ts | 79 ++++++++++++++----- 3 files changed, 104 insertions(+), 20 deletions(-) diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index a2f9f25cd..8d5ac88ba 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -197,6 +197,26 @@ describe("ResourceTemplatePanel", () => { warn.mockRestore(); }); + // `x://{}` parses in the SDK and reports no variables, so the panel renders + // no inputs and "every variable is filled" is vacuously true. Read Resource + // must stay disabled rather than submit a URI that is not the advertised + // template. + it("keeps Read Resource disabled for a template with an empty expression", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + // And it shows the template as declared, not the `x://` the SDK expands to. + expect(screen.getByText("x://{}")).toBeInTheDocument(); + warn.mockRestore(); + }); + it("clears a variable via its Clear button (non-autocomplete branch)", async () => { const user = userEvent.setup(); renderWithMantine( diff --git a/clients/web/src/test/core/uri/uriTemplate.test.ts b/clients/web/src/test/core/uri/uriTemplate.test.ts index 584e7d965..0350c6810 100644 --- a/clients/web/src/test/core/uri/uriTemplate.test.ts +++ b/clients/web/src/test/core/uri/uriTemplate.test.ts @@ -58,6 +58,22 @@ describe("templateVariableNames", () => { expect(templateVariableNames("x://{unterminated")).toEqual([]); expect(warn).toHaveBeenCalled(); }); + + // The SDK *accepts* these: `new UriTemplate("x://{}")` parses, reports no + // variables, and expands to `x://`. Left alone, the panel would render no + // inputs, find its "all filled" check vacuously true, and submit a URI that + // is not the template the server advertised. + it.each([ + ["no name", "x://{}"], + ["a blank name", "x://{ }"], + ["only a separator", "x://{,}"], + ["a missing member", "x://{a,}"], + ["an operator and no name", "x://{?}"], + ])("rejects an expression with %s", (_label, template) => { + silenceWarn(); + expect(templateVariableNames(template)).toEqual([]); + expect(expandTemplate(template, { a: "1" })).toBeNull(); + }); }); describe("expandTemplate", () => { @@ -142,6 +158,15 @@ describe("expandTemplate", () => { expect(expandTemplate("x://e{#a,b}", {})).toBe("x://e"); }); + // The projection copies every supplied variable, so a caller passing a key + // equal to the generated synthetic name must not have it stand in for the + // group — the SDK ignores undeclared variables, and so must this. + it("ignores a caller value keyed like the synthetic group name", () => { + expect( + expandTemplate("x://{a,b}", { __inspectorGroup0__: "injected" }), + ).toBe("x://"); + }); + // `variableNames` strips a trailing `*`, so the form stores `a`. A group // that kept `a*` would look up a key the form never sets and silently drop // a filled value. diff --git a/core/uri/uriTemplate.ts b/core/uri/uriTemplate.ts index 087800199..de9d01e02 100644 --- a/core/uri/uriTemplate.ts +++ b/core/uri/uriTemplate.ts @@ -11,7 +11,7 @@ * * These wrap the SDK's `UriTemplate`, correcting the two places its expander * departs from RFC 6570 (see `groupMultiNameExpressions` and - * `PATH_PARAM_EXPRESSION`). Living in `core/` is what makes the correction + * `unsupportedReason`). Living in `core/` is what makes the correction * uniform: the web panel, the TUI's form builder, and * `InspectorClient.readResourceFromTemplate` all route through here rather than * calling the SDK directly, so web, CLI, and TUI agree on what a template's @@ -39,6 +39,43 @@ function parseTemplate(uriTemplate: string): UriTemplate | null { } } +/** Any `{…}` expression in the template, with its body captured. */ +const ANY_EXPRESSION = /\{([^{}]*)\}/g; + +/** + * Why this template cannot be handled, or `null` if it can. + * + * Checked on the template *as the server declared it*, before any rewriting — + * the multi-name grouping would otherwise mask an empty member by folding + * `{a,}` into a synthetic single-name expression. + * + * Two cases, both of which the SDK accepts and mishandles rather than + * rejecting: + * + * - **An expression declaring no variable.** `new UriTemplate("x://{}")` + * parses, reports no variable names, and expands to `x://`. That defeats the + * null-on-malformed contract in the most dangerous way: the panel renders no + * inputs, so its "every variable is filled" check is vacuously true and it + * submits a URI that is not the template the server advertised. `{ }`, + * `{,}`, and `{a,}` are the same defect with some members missing. + * - **The `;` path-parameter operator**, which the SDK does not implement: it + * reads `{;a}` as a variable literally named `";a"` and expands it to the + * bare value, dropping the required `;a=`. No arrangement of its own branches + * produces the right output. + */ +function unsupportedReason(uriTemplate: string): string | null { + for (const [, body] of uriTemplate.matchAll(ANY_EXPRESSION)) { + if (body.startsWith(";")) { + return "the ; (path-parameter) operator is unsupported"; + } + const names = body.replace(/^[+#./?&]/, "").split(","); + if (names.some((name) => name.trim().length === 0)) { + return "an expression declares no variable"; + } + } + return null; +} + /** * Expand, converting a throw into `null`. * @@ -71,10 +108,19 @@ function tryExpand( * builder and `readResourceFromTemplate`, which parse through the same class. */ export function templateVariableNames(uriTemplate: string): string[] { + if (warnIfUnsupported(uriTemplate)) return []; const template = parseTemplate(uriTemplate); return template ? uniqueNames(template) : []; } +/** Warn once with the reason, and report whether the template is unsupported. */ +function warnIfUnsupported(uriTemplate: string): boolean { + const reason = unsupportedReason(uriTemplate); + if (reason === null) return false; + console.warn(`Cannot handle URI template "${uriTemplate}": ${reason}.`); + return true; +} + /** * A name repeated across expressions (`x://{a}/{b}/{a}`) is one input, not two — * and the preview's sentinel bookkeeping is keyed by position, so a duplicate @@ -99,16 +145,6 @@ function uniqueNames(template: UriTemplate): string[] { */ const MULTI_NAME_EXPRESSION = /\{([+#./]?)([^{}?&;][^{}]*)\}/g; -/** - * RFC 6570's path-parameter operator, which the SDK does not implement: it - * treats `{;a}` as a variable literally named `";a"` and expands it to the bare - * value, dropping the required `;a=`. There is no arrangement of the SDK's own - * branches that produces the right output, so a template using it cannot be - * expanded correctly here — `expandTemplate` declines rather than returning a - * URI known to be wrong, and the panel keeps its submit disabled. - */ -const PATH_PARAM_EXPRESSION = /\{;/; - /** * The name the SDK will parse out of a group member, so `applyGroups` looks up * the same key the form stores. @@ -183,7 +219,15 @@ function applyGroups( const present = names .filter((name) => variables[name] !== undefined) .map((name) => variables[name]); + // Assign or *delete* — never leave the caller's own value under this key. + // `projected` starts as a copy of every supplied variable, so a caller that + // happens to pass a key equal to the generated name would otherwise have it + // expand the group's expression: `expandTemplate("x://{a,b}", + // { __inspectorGroup0__: "injected" })` would emit `x://injected` for an + // expression whose real members are all undefined, where the SDK ignores + // undeclared variables entirely. if (present.length > 0) projected[synthetic] = present; + else delete projected[synthetic]; } return projected; } @@ -204,12 +248,7 @@ export function expandTemplate( uriTemplate: string, variables: Record, ): string | null { - if (PATH_PARAM_EXPRESSION.test(uriTemplate)) { - console.warn( - `Cannot expand "${uriTemplate}": the ; (path-parameter) operator is unsupported.`, - ); - return null; - } + if (warnIfUnsupported(uriTemplate)) return null; const { text, groups } = groupMultiNameExpressions(uriTemplate); const template = parseTemplate(text); if (!template) return null; @@ -332,9 +371,9 @@ export function previewTemplate( uriTemplate: string, variables: Record, ): string { - // Nothing truthful to render for an expression the expander declines, so - // show the template as the server declared it. - if (PATH_PARAM_EXPRESSION.test(uriTemplate)) return uriTemplate; + // Nothing truthful to render for a template the expander declines, so show it + // as the server declared it. + if (unsupportedReason(uriTemplate) !== null) return uriTemplate; const { text, groups } = groupMultiNameExpressions(uriTemplate); const template = parseTemplate(text); if (!template) return uriTemplate; From 24345d6a8e5324ca4f4812d7182f5f60d383ca51 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:23:21 -0400 Subject: [PATCH 10/12] fix: encode values against the RFC 3986 sets, substitute in one pass (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The helper delegated value encoding to the SDK, which is not RFC 6570 conformant in three ways — all verified against the pinned version, all reachable from a plain text input, and all now shared by every client: {v} with "!" → x://! RFC: x://%21 {+v} with "[a]" → x://%5Ba%5D RFC: x://[a] {+v} with "%41" → x://%2541 RFC: x://%41 Simple and query expansions use `encodeURIComponent`, which leaves `!*'()` bare; `+` and `#` use `encodeURI`, which escapes the gen-delims `[` and `]` that reserved expansion exists to pass through, and re-encodes a well-formed pct-triplet. Rather than patch around it, the split is made explicit: each variable now reaches the SDK as a sentinel of unreserved characters and is replaced by its real rendering afterwards, so *structure* — operators, separators, which expressions appear at all — stays the SDK's job while *encoding* becomes ours, done against the explicit RFC 3986 unreserved and reserved sets. That also answers the separate performance point: substitution is a single regex pass over the expansion rather than one rescan per variable, which was O(variables × length) during render on a template the SDK will accept at 1 MB with 10,000 expressions. `expandTemplate` and `previewTemplate` collapse into one routine differing only in how an unset variable renders, and the sentinel base now only has to avoid the template's own text, since values never appear in the string being substituted. One consequence handled explicitly: the SDK's per-value length ceiling no longer sees the real value, so the same bound is enforced here — otherwise the helper would emit a URI the SDK itself refuses to build, and the panel would lose its "withhold rather than send something absurd" behavior. Signed-off-by: cliffhall --- .../web/src/test/core/uri/uriTemplate.test.ts | 74 ++++++ core/uri/uriTemplate.ts | 217 ++++++++++++++---- 2 files changed, 243 insertions(+), 48 deletions(-) diff --git a/clients/web/src/test/core/uri/uriTemplate.test.ts b/clients/web/src/test/core/uri/uriTemplate.test.ts index 0350c6810..4ffe09109 100644 --- a/clients/web/src/test/core/uri/uriTemplate.test.ts +++ b/clients/web/src/test/core/uri/uriTemplate.test.ts @@ -103,6 +103,64 @@ describe("expandTemplate", () => { expect(expandTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); }); + // The SDK's own encoding is not RFC-conformant, and every case below is + // reachable from a plain text input. Simple and query expansions go through + // `encodeURIComponent`, which leaves these five bare; RFC 6570 §3.2.1 encodes + // everything outside the *unreserved* set. + describe("RFC 3986 character sets", () => { + it.each([ + ["!", "%21"], + ["*", "%2A"], + ["'", "%27"], + ["(", "%28"], + [")", "%29"], + ])("encodes %j in a simple expansion", (value, encoded) => { + expect(expandTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); + }); + + it("encodes them in a query expansion too", () => { + expect(expandTemplate("x://e{?v}", { v: "!*'()" })).toBe( + "x://e?v=%21%2A%27%28%29", + ); + }); + + // `+` and `#` use `encodeURI`, which escapes `[` and `]` — but those are + // gen-delims, exactly what reserved expansion exists to pass through. + it.each([ + ["reserved", "x://{+v}", "x://[a]"], + ["fragment", "x://{#v}", "x://#[a]"], + ])("keeps gen-delims in a %s expansion", (_label, template, expected) => { + expect(expandTemplate(template, { v: "[a]" })).toBe(expected); + }); + + it("keeps every reserved character under the + operator", () => { + const reserved = ":/?#[]@!$&'()*+,;="; + expect(expandTemplate("x://{+v}", { v: reserved })).toBe( + `x://${reserved}`, + ); + }); + + // A well-formed triplet is already pct-encoded; the SDK re-encoded it to + // `%2541`, which changes the value. + it("passes an existing pct-triplet through a reserved expansion", () => { + expect(expandTemplate("x://{+v}", { v: "%41" })).toBe("x://%41"); + }); + + it("encodes a bare percent that is not a triplet", () => { + expect(expandTemplate("x://{+v}", { v: "100%" })).toBe("x://100%25"); + expect(expandTemplate("x://{+v}", { v: "%zz" })).toBe("x://%25zz"); + }); + + // A simple expansion has no triplet passthrough — the value is literal. + it("encodes a percent in a simple expansion", () => { + expect(expandTemplate("x://{v}", { v: "%41" })).toBe("x://%2541"); + }); + + it("encodes a code point outside the BMP as its UTF-8 octets", () => { + expect(expandTemplate("x://{v}", { v: "😀" })).toBe("x://%F0%9F%98%80"); + }); + }); + // RFC 6570 distinguishes an *undefined* variable from one defined as the // empty string, and the expansion must not collapse the two. it("keeps a variable defined as the empty string", () => { @@ -326,6 +384,22 @@ describe("previewTemplate", () => { expect(previewTemplate(template, { a: "" })).toBe(template); }); + // Substitution is one regex pass over the expansion rather than one pass per + // variable — the SDK accepts a 1 MB template with up to 10,000 expressions, + // and a per-variable rescan is O(variables × length) during render. This also + // covers the multi-digit index boundary at scale. + it("substitutes many variables correctly", () => { + const names = Array.from({ length: 200 }, (_, i) => `v${i}`); + const template = `x://${names.map((n) => `{${n}}`).join("/")}`; + const values = Object.fromEntries( + names.map((n, i) => [n, i % 2 === 0 ? `val${i}` : ""]), + ); + const expected = `x://${names + .map((n, i) => (i % 2 === 0 ? `val${i}` : `{${n}}`)) + .join("/")}`; + expect(previewTemplate(template, values)).toBe(expected); + }); + // The token starts and ends with `zz`, so it can overlap itself: this literal // holds a second occurrence at index 19 whose trailing run is the longer one. // A scan advancing by the token's length would miss it and pick a colliding diff --git a/core/uri/uriTemplate.ts b/core/uri/uriTemplate.ts index de9d01e02..5af443a3d 100644 --- a/core/uri/uriTemplate.ts +++ b/core/uri/uriTemplate.ts @@ -232,27 +232,173 @@ function applyGroups( return projected; } +/** RFC 3986 §2.3 unreserved: the set never percent-encoded, under any operator. */ +const UNRESERVED = /[A-Za-z0-9\-._~]/; + +/** RFC 3986 §2.2 reserved (gen-delims + sub-delims), allowed by `+` and `#`. */ +const RESERVED = /[:/?#[\]@!$&'()*+,;=]/; + +/** `encodeURIComponent` leaves these alone; RFC 6570 requires them encoded. */ +const UNDER_ENCODED_BY_ENCODE_URI_COMPONENT = /[!'()*]/g; + +/** + * Percent-encode `value` for an expression using `operator`, per RFC 6570 §3.2.1. + * + * The SDK's own encoding is not RFC-conformant in three ways, all verified + * against the pinned version, and all reachable from a plain text input: + * + * - simple and query expansions use `encodeURIComponent`, which leaves `!`, + * `*`, `'`, `(` and `)` bare — `{v}` with `!` gives `x://!` where the RFC + * requires `x://%21`; + * - `+` and `#` use `encodeURI`, which escapes `[` and `]` — but those are + * gen-delims, which reserved expansion is specifically meant to pass through; + * - `+` and `#` also re-encode an existing pct-triplet, turning `%41` into + * `%2541`, where the RFC keeps a well-formed triplet as-is. + * + * So this module owns value encoding and leaves *structure* — operators, + * separators, which expressions appear at all — to the SDK. See + * `expandWithPlaceholders` for how the two are combined. + */ +function encodeValue(value: string, operator: string): string { + const allowReserved = operator === "+" || operator === "#"; + let out = ""; + for (let at = 0; at < value.length; ) { + const char = value[at]; + if ( + allowReserved && + char === "%" && + /^[0-9A-Fa-f]{2}$/.test(value.slice(at + 1, at + 3)) + ) { + // A well-formed triplet is already pct-encoded; reserved expansion keeps it. + out += value.slice(at, at + 3); + at += 3; + continue; + } + if (UNRESERVED.test(char) || (allowReserved && RESERVED.test(char))) { + out += char; + at += 1; + continue; + } + // Encode a whole code point, so a surrogate pair yields its UTF-8 octets + // rather than two lone-surrogate errors. + const codePoint = String.fromCodePoint(value.codePointAt(at) as number); + out += encodeURIComponent(codePoint).replace( + UNDER_ENCODED_BY_ENCODE_URI_COMPONENT, + (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ); + at += codePoint.length; + } + return out; +} + +/** + * The SDK's per-value ceiling, enforced here instead of by it. + * + * Because a value now reaches the SDK as a short sentinel and is substituted in + * afterwards, the SDK's own `validateLength` no longer sees it — so without this + * the helper would happily emit a URI the SDK itself refuses to build. Keeping + * the same bound preserves that guard, and with it the panel's behavior of + * withholding the request rather than sending something absurd. + */ +const MAX_VALUE_LENGTH = 1_000_000; + +/** Operator declared for each variable name, read off the original template. */ +function operatorsByName(uriTemplate: string): Map { + const operators = new Map(); + for (const [, body] of uriTemplate.matchAll(ANY_EXPRESSION)) { + const operator = /^[+#./?&]/.test(body) ? body[0] : ""; + const names = body.slice(operator.length).split(",").map(memberName); + for (const name of names) { + if (!operators.has(name)) operators.set(name, operator); + } + } + return operators; +} + +/** + * Expand `uriTemplate`, letting the SDK build the *structure* while this module + * supplies each variable's *rendering*. + * + * Every variable that will appear is handed to the SDK as a sentinel token made + * only of unreserved characters, so it survives whichever encoding the SDK + * applies, and is swapped for its real rendering afterwards. That split is what + * lets the operators, separators, and omission rules stay the SDK's job while + * the encoding — which the SDK gets wrong (see `encodeValue`) — becomes ours. + * + * `renderUnset` decides what an absent-or-empty variable becomes: `null` omits + * it (the wire behavior), while returning a string substitutes it (the preview's + * `{name}` placeholder). + * + * The substitution is a single regex pass over the expansion, not one pass per + * variable: the SDK accepts a 1 MB template with up to 10,000 expressions, and a + * per-variable rescan would be O(variables × length) on the render thread. + */ +function expandWithPlaceholders( + uriTemplate: string, + variables: Record, + renderUnset: (name: string) => string | null, +): string | null { + const { text, groups } = groupMultiNameExpressions(uriTemplate); + const template = parseTemplate(text); + if (!template) return null; + + // Map each synthetic group back to the real names it stands for, so both the + // rendering and the preview's placeholders speak in the form's own names. + const names = [ + ...new Set( + uniqueNames(template).flatMap((name) => groups.get(name) ?? [name]), + ), + ]; + const operators = operatorsByName(uriTemplate); + const base = uncollidingBase(uriTemplate); + + const renderings = new Map(); + const values: Record = {}; + for (const [index, name] of names.entries()) { + const value = variables[name]; + if (value !== undefined && value.length > MAX_VALUE_LENGTH) { + console.warn( + `Cannot expand URI template "${uriTemplate}": the value for "${name}" exceeds ${MAX_VALUE_LENGTH} characters.`, + ); + return null; + } + const rendered = + value === undefined + ? renderUnset(name) + : encodeValue(value, operators.get(name) ?? ""); + if (rendered === null) continue; // leave unset, so the expression omits it + renderings.set(index, rendered); + values[name] = sentinelFor(base, index); + } + + const expanded = tryExpand(template, applyGroups(values, groups)); + if (expanded === null) return null; + + return expanded.replace( + new RegExp(`${base}(\\d+)zz`, "g"), + /* v8 ignore next -- the `?? match` fallback is unreachable: every sentinel + in the expansion was minted from `renderings` just above. */ + (match, index: string) => renderings.get(Number(index)) ?? match, + ); +} + /** * Expands `uriTemplate` per RFC 6570, percent-encoding each value according to - * its expression's operator. Returns `null` when the template cannot be parsed + * its expression's operator. Returns `null` when the template cannot be handled * or a value cannot be expanded, so a caller can decline to issue the request * rather than send a URI it knows is wrong. * - * Values are passed through untouched, which preserves the spec's distinction - * between an *undefined* variable and one defined as the empty string: an - * absent key drops out of the expansion entirely, while `{ topic: "" }` against - * `{?topic}` yields `?topic=`. Collapsing the two here would silently make a - * deliberately-empty value unexpressible. + * An absent variable is omitted, while one defined as the empty string is kept — + * preserving the spec's distinction between the two, so `{ topic: "" }` against + * `{?topic}` yields `?topic=`. Collapsing them would make a deliberately-empty + * value unexpressible. */ export function expandTemplate( uriTemplate: string, variables: Record, ): string | null { if (warnIfUnsupported(uriTemplate)) return null; - const { text, groups } = groupMultiNameExpressions(uriTemplate); - const template = parseTemplate(text); - if (!template) return null; - return tryExpand(template, applyGroups(variables, groups)); + return expandWithPlaceholders(uriTemplate, variables, () => null); } /** @@ -268,11 +414,11 @@ export function expandTemplate( const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; /** - * Pick a sentinel base that appears nowhere in the expansion's other inputs. + * Pick a sentinel base that appears nowhere in the template's literal text. * - * Without this, a user who types the literal token as *another* variable's - * value would see that value rewritten into a `{placeholder}` on substitution — - * the preview would disagree with the URI actually submitted. + * Only the template needs checking: values never reach the expansion — they are + * substituted in afterwards, in a single pass that does not rescan what it + * inserts — so a value equal to the token cannot be mistaken for one. * * Done in one pass rather than by extending the base until it stops colliding: * the template is server-supplied and may be up to 1 MB, and a template holding @@ -281,12 +427,8 @@ const UNFILLED_SENTINEL = "zzInspectorUnfilledzz"; * render thread. Instead, measure the longest run of `z` that follows any * occurrence and clear it by one, which no occurrence can then match. */ -function uncollidingBase(uriTemplate: string, filled: Record) { - return padPastCollisions( - UNFILLED_SENTINEL, - "z", - [uriTemplate, ...Object.values(filled)].join("\n"), - ); +function uncollidingBase(uriTemplate: string) { + return padPastCollisions(UNFILLED_SENTINEL, "z", uriTemplate); } /** @@ -374,32 +516,11 @@ export function previewTemplate( // Nothing truthful to render for a template the expander declines, so show it // as the server declared it. if (unsupportedReason(uriTemplate) !== null) return uriTemplate; - const { text, groups } = groupMultiNameExpressions(uriTemplate); - const template = parseTemplate(text); - if (!template) return uriTemplate; - - // Map each synthetic group back to the real names it stands for, so the - // placeholders the user sees are the ones the form is asking them for. - const names = [ - ...new Set( - uniqueNames(template).flatMap((name) => groups.get(name) ?? [name]), - ), - ]; - const filled = enteredValues(variables); - const base = uncollidingBase(uriTemplate, filled); - const values: Record = { ...filled }; - names.forEach((name, index) => { - if (values[name] === undefined) values[name] = sentinelFor(base, index); - }); - - const expanded = tryExpand(template, applyGroups(values, groups)); - if (expanded === null) return uriTemplate; - - let preview = expanded; - names.forEach((name, index) => { - if (filled[name] !== undefined) return; - // The sentinel is unreserved, so it appears in the expansion unencoded. - preview = preview.split(sentinelFor(base, index)).join(`{${name}}`); - }); - return preview; + return ( + expandWithPlaceholders( + uriTemplate, + enteredValues(variables), + (name) => `{${name}}`, + ) ?? uriTemplate + ); } From 0a96cc1700ffc628a5a4d7820992b70d76bdc4c2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:45:14 -0400 Subject: [PATCH 11/12] fix: encode per occurrence, and decline an unpaired surrogate (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more, both consequences of the previous round's restructure. **An unpaired surrogate threw out of render.** `encodeURIComponent("\uD800")` raises `URIError`, and value encoding runs outside the SDK's try/catch — so both helpers could throw, and `previewTemplate` runs during React render, which would take the panel down rather than disable its submit. A lone surrogate has no UTF-8 encoding under any operator, so it is now detected up front and fails the whole expansion: `expandTemplate` returns null, the preview falls back to the template, and the panel withholds the request. **A name repeated under different operators got one encoding.** RFC 6570 allows `x://{+a}-{a}`, where `a = "/"` must stay `/` under the reserved operator and become `%2F` in the simple expansion. The per-name operator map applied the first occurrence's operator to both — and the underlying reason one map cannot work is that the SDK looks values up *by name*, so a single key cannot carry two renderings. So the rewrite generalizes from "multi-name expressions" to **every non-query expression**, each becoming its own synthetic variable. An occurrence is then the unit of rendering, which is what RFC 6570 actually specifies. Query expressions stay untouched — `?`/`&` emit the variable's *name* into the URI, so renaming `{?topic}` would produce `?__inspectorGroup0__=` — and they need no rewrite, since `?` and `&` share one encoding and their branch is already correct. Name discovery now comes from that same scan rather than from `UriTemplate.variableNames`, so the form's fields and the expander's lookups are one list by construction instead of two paths that happen to agree. Signed-off-by: cliffhall --- .../web/src/test/core/uri/uriTemplate.test.ts | 46 ++++ core/uri/uriTemplate.ts | 231 +++++++++--------- 2 files changed, 162 insertions(+), 115 deletions(-) diff --git a/clients/web/src/test/core/uri/uriTemplate.test.ts b/clients/web/src/test/core/uri/uriTemplate.test.ts index 4ffe09109..2adc29952 100644 --- a/clients/web/src/test/core/uri/uriTemplate.test.ts +++ b/clients/web/src/test/core/uri/uriTemplate.test.ts @@ -159,6 +159,52 @@ describe("expandTemplate", () => { it("encodes a code point outside the BMP as its UTF-8 octets", () => { expect(expandTemplate("x://{v}", { v: "😀" })).toBe("x://%F0%9F%98%80"); }); + + // An unpaired surrogate has no UTF-8 encoding and makes + // `encodeURIComponent` throw `URIError`. Encoding runs outside the SDK's + // try/catch — and, through the preview, during render — so it must be + // reported rather than thrown. + it.each([ + ["a lone high surrogate", "\uD800"], + ["a lone low surrogate", "\uDC00"], + ["a surrogate among valid text", "ok\uD800ok"], + ])("declines %s instead of throwing", (_label, value) => { + silenceWarn(); + expect(() => expandTemplate("x://{v}", { v: value })).not.toThrow(); + expect(expandTemplate("x://{v}", { v: value })).toBeNull(); + }); + + it("previews a lone surrogate as the raw template instead of throwing", () => { + silenceWarn(); + expect(previewTemplate("x://{v}", { v: "\uD800" })).toBe("x://{v}"); + }); + }); + + // RFC 6570 allows one name in expressions with different operators, and each + // occurrence encodes per *its* operator. A single value keyed by name cannot + // express that, so each occurrence gets its own rendering. + describe("a name repeated under different operators", () => { + // `-` as the literal separator, so the slashes in the output are only ever + // the ones the expansion produced. + it("encodes each occurrence per its own operator", () => { + expect(expandTemplate("x://{+a}-{a}", { a: "/" })).toBe("x:///-%2F"); + }); + + it("still offers the repeated name as one field", () => { + expect(templateVariableNames("x://{+a}-{a}")).toEqual(["a"]); + }); + + it("handles a simple/query pair", () => { + expect(expandTemplate("x://{+a}{?a}", { a: "a/b" })).toBe( + "x://a/b?a=a%2Fb", + ); + }); + + it("handles a fragment/simple pair", () => { + expect(expandTemplate("x://{a}{#a}", { a: "[x]" })).toBe( + "x://%5Bx%5D#[x]", + ); + }); }); // RFC 6570 distinguishes an *undefined* variable from one defined as the diff --git a/core/uri/uriTemplate.ts b/core/uri/uriTemplate.ts index 5af443a3d..bb8796de2 100644 --- a/core/uri/uriTemplate.ts +++ b/core/uri/uriTemplate.ts @@ -109,8 +109,11 @@ function tryExpand( */ export function templateVariableNames(uriTemplate: string): string[] { if (warnIfUnsupported(uriTemplate)) return []; - const template = parseTemplate(uriTemplate); - return template ? uniqueNames(template) : []; + // Parsed only to reject what the SDK rejects (an unterminated expression); + // the names themselves come from the same scan the expander uses, so the + // form's fields and its lookups are one list by construction. + if (!parseTemplate(uriTemplate)) return []; + return rewriteExpressions(uriTemplate).order; } /** Warn once with the reason, and report whether the template is unsupported. */ @@ -122,32 +125,8 @@ function warnIfUnsupported(uriTemplate: string): boolean { } /** - * A name repeated across expressions (`x://{a}/{b}/{a}`) is one input, not two — - * and the preview's sentinel bookkeeping is keyed by position, so a duplicate - * would otherwise leave one occurrence un-substituted. - */ -function uniqueNames(template: UriTemplate): string[] { - return [...new Set(template.variableNames)]; -} - -/** - * Matches a multi-name expression whose operator is *not* `?` or `&`. - * - * The SDK expands this shape through a branch that returns the raw values - * joined by `,`, skipping both `encodeValue` and the operator prefix — so - * `x://{a,b}` with `a = "foo/bar"` yields `x://foo/bar,x y` rather than - * `x://foo%2Fbar,x%20y`, and `{#a,b}` loses its `#` entirely. Its query - * counterpart (`{?a,b}`) takes a different, correct branch. - * - * `groupMultiNameExpressions` rewrites this shape so the SDK's *single*-name - * branch — which does apply the operator's encoding and separator, over an - * array value — handles it instead. - */ -const MULTI_NAME_EXPRESSION = /\{([+#./]?)([^{}?&;][^{}]*)\}/g; - -/** - * The name the SDK will parse out of a group member, so `applyGroups` looks up - * the same key the form stores. + * The name the SDK will parse out of an expression member, so the values handed + * back to it are keyed the way the form stores them. * * It strips a trailing explode modifier (`{a*}` → `a`) but *keeps* a prefix * modifier (`{a:3}` → `a:3`, see `templateVariableNames`). Mirroring it exactly @@ -169,67 +148,67 @@ function groupName(prefix: string, index: number): string { const GROUP_PREFIX = "__inspectorGroup"; -interface GroupedTemplate { +/** One expression of the template, as rewritten for the SDK. */ +interface Slot { + /** The real variable names it declares, in order. */ + names: string[]; + /** Its RFC 6570 operator (`""` for a simple expression). */ + operator: string; +} + +interface RewrittenTemplate { /** The rewritten template string, safe to hand to the SDK. */ text: string; - /** Synthetic variable name → the real names it stands for, in order. */ - groups: Map; + /** Synthetic variable name → the expression it stands for. */ + slots: Map; + /** Names left under their own key, because a query expression emits them. */ + queryNames: Set; + /** Every declared name, in template order, deduplicated. */ + order: string[]; } /** - * Rewrite each multi-name non-query expression into a single synthetic - * variable, so the SDK applies the operator's encoding and separator. + * Rewrite each **non-query** expression to a single synthetic variable. + * + * Two problems this solves at once, both stemming from the SDK looking values up + * by variable name: + * + * - a *multi-name* expression takes a branch that returns the values joined raw, + * skipping both `encodeValue` and the operator; giving the rewritten + * expression an **array** value routes it through the single-name branch, + * which applies them (`x://{a,b}` → `x://{__inspectorGroup0__}`); + * - a name **repeated under different operators** — `x://{+a}/{a}` — must be + * encoded differently per occurrence (`/` preserved by `+`, `%2F` by the + * simple expansion), which one value keyed by name cannot express. A synthetic + * name per *occurrence* gives each its own value, hence its own encoding. * - * `x://{a,b}` becomes `x://{__inspectorGroup0__}`, expanded with an *array* of - * the defined values — which the SDK's single-name branch encodes elementwise - * and joins with the operator's separator, exactly as RFC 6570 prescribes. - * Templates without this shape are returned unchanged and pay nothing. + * Query expressions are deliberately left alone: `?`/`&` emit the variable's + * **name** into the URI, so renaming `{?topic}` would produce `?__inspectorGroup0__=`. + * They need no rewrite anyway — their branch already encodes correctly, and + * `?` and `&` share one encoding, so every query occurrence of a name can share + * a single value keyed by that name. */ -function groupMultiNameExpressions(uriTemplate: string): GroupedTemplate { - const groups = new Map(); +function rewriteExpressions(uriTemplate: string): RewrittenTemplate { + const slots = new Map(); + const queryNames = new Set(); + // A Set, so the order is the template's and each name appears once. + const order = new Set(); // The prefix must not occur in the template, or a template declaring a - // variable of that name would have the group overwrite the user's value. + // variable of that name would have the slot overwrite the user's value. const prefix = padPastCollisions(GROUP_PREFIX, "_", uriTemplate); - const text = uriTemplate.replace( - MULTI_NAME_EXPRESSION, - (match, operator: string, body: string) => { - if (!body.includes(",")) return match; - const names = body.split(",").map(memberName); - const synthetic = groupName(prefix, groups.size); - groups.set(synthetic, names); - return `{${operator}${synthetic}}`; - }, - ); - return { text, groups }; -} - -/** - * Project the caller's per-name values onto the synthetic group variables. - * - * A group is omitted when none of its names has a value, matching RFC 6570's - * rule that an expression with no defined variable contributes nothing. - */ -function applyGroups( - variables: Record, - groups: Map, -): Record { - if (groups.size === 0) return variables; - const projected: Record = { ...variables }; - for (const [synthetic, names] of groups) { - const present = names - .filter((name) => variables[name] !== undefined) - .map((name) => variables[name]); - // Assign or *delete* — never leave the caller's own value under this key. - // `projected` starts as a copy of every supplied variable, so a caller that - // happens to pass a key equal to the generated name would otherwise have it - // expand the group's expression: `expandTemplate("x://{a,b}", - // { __inspectorGroup0__: "injected" })` would emit `x://injected` for an - // expression whose real members are all undefined, where the SDK ignores - // undeclared variables entirely. - if (present.length > 0) projected[synthetic] = present; - else delete projected[synthetic]; - } - return projected; + const text = uriTemplate.replace(ANY_EXPRESSION, (match, body: string) => { + const operator = /^[+#./?&]/.test(body) ? body[0] : ""; + const names = body.slice(operator.length).split(",").map(memberName); + for (const name of names) order.add(name); + if (operator === "?" || operator === "&") { + for (const name of names) queryNames.add(name); + return match; + } + const synthetic = groupName(prefix, slots.size); + slots.set(synthetic, { names, operator }); + return `{${operator}${synthetic}}`; + }); + return { text, slots, queryNames, order: [...order] }; } /** RFC 3986 §2.3 unreserved: the set never percent-encoded, under any operator. */ @@ -259,7 +238,7 @@ const UNDER_ENCODED_BY_ENCODE_URI_COMPONENT = /[!'()*]/g; * separators, which expressions appear at all — to the SDK. See * `expandWithPlaceholders` for how the two are combined. */ -function encodeValue(value: string, operator: string): string { +function encodeValue(value: string, operator: string): string | null { const allowReserved = operator === "+" || operator === "#"; let out = ""; for (let at = 0; at < value.length; ) { @@ -281,7 +260,13 @@ function encodeValue(value: string, operator: string): string { } // Encode a whole code point, so a surrogate pair yields its UTF-8 octets // rather than two lone-surrogate errors. - const codePoint = String.fromCodePoint(value.codePointAt(at) as number); + const point = value.codePointAt(at) as number; + // An *unpaired* surrogate has no UTF-8 encoding, and `encodeURIComponent` + // throws `URIError` on it. Report it instead: this runs outside `tryExpand` + // and, through the preview, during React render — an escaping throw would + // take the panel down rather than disable its submit. + if (point >= 0xd800 && point <= 0xdfff) return null; + const codePoint = String.fromCodePoint(point); out += encodeURIComponent(codePoint).replace( UNDER_ENCODED_BY_ENCODE_URI_COMPONENT, (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, @@ -302,19 +287,6 @@ function encodeValue(value: string, operator: string): string { */ const MAX_VALUE_LENGTH = 1_000_000; -/** Operator declared for each variable name, read off the original template. */ -function operatorsByName(uriTemplate: string): Map { - const operators = new Map(); - for (const [, body] of uriTemplate.matchAll(ANY_EXPRESSION)) { - const operator = /^[+#./?&]/.test(body) ? body[0] : ""; - const names = body.slice(operator.length).split(",").map(memberName); - for (const name of names) { - if (!operators.has(name)) operators.set(name, operator); - } - } - return operators; -} - /** * Expand `uriTemplate`, letting the SDK build the *structure* while this module * supplies each variable's *rendering*. @@ -325,6 +297,11 @@ function operatorsByName(uriTemplate: string): Map { * lets the operators, separators, and omission rules stay the SDK's job while * the encoding — which the SDK gets wrong (see `encodeValue`) — becomes ours. * + * Renderings are per *occurrence*, not per name, which is what lets the same + * variable be encoded two ways in one template: `x://{+a}/{a}` with `a = "/"` + * must keep the `/` under the reserved operator and encode it as `%2F` in the + * simple expansion. + * * `renderUnset` decides what an absent-or-empty variable becomes: `null` omits * it (the wire behavior), while returning a string substitutes it (the preview's * `{name}` placeholder). @@ -338,23 +315,11 @@ function expandWithPlaceholders( variables: Record, renderUnset: (name: string) => string | null, ): string | null { - const { text, groups } = groupMultiNameExpressions(uriTemplate); + const { text, slots, queryNames, order } = rewriteExpressions(uriTemplate); const template = parseTemplate(text); if (!template) return null; - // Map each synthetic group back to the real names it stands for, so both the - // rendering and the preview's placeholders speak in the form's own names. - const names = [ - ...new Set( - uniqueNames(template).flatMap((name) => groups.get(name) ?? [name]), - ), - ]; - const operators = operatorsByName(uriTemplate); - const base = uncollidingBase(uriTemplate); - - const renderings = new Map(); - const values: Record = {}; - for (const [index, name] of names.entries()) { + for (const name of order) { const value = variables[name]; if (value !== undefined && value.length > MAX_VALUE_LENGTH) { console.warn( @@ -362,16 +327,52 @@ function expandWithPlaceholders( ); return null; } + } + + const base = uncollidingBase(uriTemplate); + const renderings = new Map(); + const values: Record = {}; + let nextIndex = 0; + + // An unpaired surrogate has no encoding under any operator, so it fails the + // whole expansion rather than silently dropping one variable. Checked up + // front, which also lets `place` treat a null rendering as "omit this one". + for (const name of order) { + const value = variables[name]; + if (value !== undefined && encodeValue(value, "") === null) { + console.warn( + `Cannot expand URI template "${uriTemplate}": the value for "${name}" contains an unpaired surrogate.`, + ); + return null; + } + } + + /** Mint a sentinel for one occurrence, or report that it contributes nothing. */ + function place(name: string, operator: string): string | null { + const value = variables[name]; const rendered = - value === undefined - ? renderUnset(name) - : encodeValue(value, operators.get(name) ?? ""); - if (rendered === null) continue; // leave unset, so the expression omits it + value === undefined ? renderUnset(name) : encodeValue(value, operator); + if (rendered === null) return null; + const index = nextIndex++; renderings.set(index, rendered); - values[name] = sentinelFor(base, index); + return sentinelFor(base, index); + } + + for (const [synthetic, slot] of slots) { + const placed = slot.names + .map((name) => place(name, slot.operator)) + .filter((sentinel): sentinel is string => sentinel !== null); + // An expression with no defined variable contributes nothing (RFC 6570). + if (placed.length > 0) values[synthetic] = placed; + } + for (const name of queryNames) { + // `?` and `&` share one encoding, so every query occurrence of a name can + // share the single value the SDK will look up under that name. + const sentinel = place(name, "?"); + if (sentinel !== null) values[name] = sentinel; } - const expanded = tryExpand(template, applyGroups(values, groups)); + const expanded = tryExpand(template, values); if (expanded === null) return null; return expanded.replace( From ae7b34cdb2b45958e11027703d9cdd24bef2de63 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:00:26 -0400 Subject: [PATCH 12/12] fix: validate the original template, and refresh the stale rationale (#1919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `expandTemplate` parsed only the *rewritten* text. The rewrite replaces variable names with short synthetics, shrinking both the template and every name — and the SDK enforces a length limit on each at construction — so a template it would reject could slip through here while `templateVariableNames`, which parses the original, rejected it: an empty form beside an expansion happily producing some shorter URI. The original is now parsed first, so the two agree. The second parse (of the rewritten text) can no longer fail, since rewriting only ever shortens; that branch is marked unreachable rather than left as dead-looking code. The module header and the README also still described the state after the *first* round of this work — "the two places the expander departs from RFC 6570", pointing at a `groupMultiNameExpressions` that no longer exists. Both now describe what the helper actually maintains: value encoding against the RFC 3986 sets, the per-occurrence rewrite that fixes multi-name expressions and a name repeated under different operators, and the shapes that are declined rather than expanded into a knowingly invalid URI. The AGENTS.md entry is updated in the same pass so a third copy isn't left stale. Signed-off-by: cliffhall --- AGENTS.md | 12 ++++--- README.md | 8 ++++- .../web/src/test/core/uri/uriTemplate.test.ts | 13 ++++++++ core/uri/uriTemplate.ts | 31 +++++++++++++++++-- 4 files changed, 56 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c9a12d3b3..8ac65f05e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,10 +100,14 @@ v2/main/ │ ├── react/ # React hooks over the state stores │ ├── storage/ # File I/O helpers (store-io.ts) used by OAuth persist backends │ └── uri/ # RFC 6570 URI Template discovery/expansion/preview -│ # (uriTemplate.ts) — wraps the SDK's UriTemplate and -│ # corrects the two places its expander departs from the -│ # RFC (multi-name expressions skip encoding + the -│ # operator; `;` is unimplemented). Shared by the web +│ # (uriTemplate.ts) — wraps the SDK's UriTemplate, keeping +│ # its parse/operators/separators and correcting where it +│ # departs from the RFC: value encoding is done here +│ # against the RFC 3986 sets, each non-query expression is +│ # rewritten to a synthetic variable (so multi-name +│ # expressions encode, and a name repeated under different +│ # operators encodes per occurrence), and the shapes it +│ # mishandles (`{}`, `;`) are declined. Shared by the web │ # ResourceTemplatePanel, the TUI's uriTemplateToForm, │ # and InspectorClient.readResourceFromTemplate, so a │ # template cannot resolve differently per client — #1919. diff --git a/README.md b/README.md index 0821d4f90..924c69923 100644 --- a/README.md +++ b/README.md @@ -247,7 +247,13 @@ Open the Resources tab and select **events-by-path**, enter `foo/bar` for `topic Then select **events-by-query**: it must render a `topic` input at all. The old scan was `/\{(\w+)\}/g`, which sees only bare `{name}` expressions, so a query expression declared a variable the form never offered. -All three clients now go through one shared helper, [`core/uri/uriTemplate.ts`](./core/uri/uriTemplate.ts) — the web panel, the TUI's form builder, and `InspectorClient.readResourceFromTemplate` — so a template cannot resolve differently depending on where it is driven from. It wraps the SDK's `UriTemplate` and corrects the two places that expander departs from RFC 6570: a multi-name expression (`{a,b}`) skips both encoding and its operator, and the `;` path-parameter operator is unimplemented (such a template is declined rather than expanded to a knowingly invalid URI). +All three clients now go through one shared helper, [`core/uri/uriTemplate.ts`](./core/uri/uriTemplate.ts) — the web panel, the TUI's form builder, and `InspectorClient.readResourceFromTemplate` — so a template cannot resolve differently depending on where it is driven from. + +It wraps the SDK's `UriTemplate`, keeping what that gets right (the parse, the operators and separators, which expressions appear at all) and correcting where it departs from RFC 6570: + +- **Value encoding** is done against the explicit RFC 3986 character sets. The SDK's `encodeURIComponent` leaves the sub-delimiters `!*'()` bare, its `encodeURI` escapes the gen-delims `[` and `]` that reserved expansion exists to pass through, and it double-encodes an existing percent triplet (`%41` → `%2541`). +- **Multi-name expressions** (`{a,b}`) take an SDK branch that skips both encoding and the operator, and a **name repeated under different operators** (`{+a}-{a}`) has to encode differently per occurrence — which values looked up by name cannot express. Each non-query expression is rewritten to its own synthetic variable so both work. +- Two shapes the SDK accepts but mishandles — an expression declaring no variable (`{}`), and the unimplemented `;` path-parameter operator — are **declined** rather than expanded into a knowingly invalid URI, so the panel withholds the request instead of sending it. #### Advertised extensions diff --git a/clients/web/src/test/core/uri/uriTemplate.test.ts b/clients/web/src/test/core/uri/uriTemplate.test.ts index 2adc29952..440951c76 100644 --- a/clients/web/src/test/core/uri/uriTemplate.test.ts +++ b/clients/web/src/test/core/uri/uriTemplate.test.ts @@ -321,6 +321,19 @@ describe("expandTemplate", () => { expect(expandTemplate("x://{unterminated", { a: "1" })).toBeNull(); }); + // The rewrite replaces variable names with short synthetics, shrinking both + // the template and every name — so validating only the rewritten text would + // accept a template the SDK rejects, and disagree with + // `templateVariableNames`, which parses the original. + it.each([ + ["an over-long variable name", `x://{${"n".repeat(1_000_001)}}`], + ["an over-long template", `x://${"p".repeat(1_000_001)}/{a}`], + ])("returns null for %s, as discovery does", (_label, template) => { + silenceWarn(); + expect(templateVariableNames(template)).toEqual([]); + expect(expandTemplate(template, { a: "1" })).toBeNull(); + }); + // Parsing succeeding does not mean expanding will — the SDK checks its // per-value length ceiling at expansion time. it("returns null when a value cannot be expanded", () => { diff --git a/core/uri/uriTemplate.ts b/core/uri/uriTemplate.ts index bb8796de2..2adb37785 100644 --- a/core/uri/uriTemplate.ts +++ b/core/uri/uriTemplate.ts @@ -9,9 +9,24 @@ * values verbatim, so a `topic` of `foo/bar` silently became a second path * segment instead of `foo%2Fbar` (#1919). * - * These wrap the SDK's `UriTemplate`, correcting the two places its expander - * departs from RFC 6570 (see `groupMultiNameExpressions` and - * `unsupportedReason`). Living in `core/` is what makes the correction + * These wrap the SDK's `UriTemplate`, keeping what it gets right — the parse, + * the operators, the separators, which expressions appear at all — and + * correcting where it departs from RFC 6570: + * + * - **value encoding** is done here instead, against the explicit RFC 3986 + * character sets; the SDK's `encodeURIComponent` / `encodeURI` leave `!*'()` + * bare, escape the gen-delims `[` `]` that reserved expansion should pass + * through, and double-encode an existing pct-triplet (see `encodeValue`); + * - **multi-name expressions** (`{a,b}`) take a branch that skips both encoding + * and the operator, and a **name repeated under different operators** cannot + * be encoded per-occurrence when values are looked up by name — both are + * handled by rewriting each non-query expression into its own synthetic + * variable (see `rewriteExpressions`); + * - two shapes it accepts but mishandles — an expression declaring no variable, + * and the unimplemented `;` operator — are **declined** rather than expanded + * into a knowingly wrong URI (see `unsupportedReason`). + * + * Living in `core/` is what makes the correction * uniform: the web panel, the TUI's form builder, and * `InspectorClient.readResourceFromTemplate` all route through here rather than * calling the SDK directly, so web, CLI, and TUI agree on what a template's @@ -315,8 +330,18 @@ function expandWithPlaceholders( variables: Record, renderUnset: (name: string) => string | null, ): string | null { + // Validate the template **as the server sent it**, before rewriting. The + // rewrite replaces variable names with short synthetics, which shrinks both + // the template and every name — so a template the SDK would reject for + // exceeding its length limits could otherwise slip through here while + // `templateVariableNames` (which parses the original) rejected it, leaving the + // form empty and the expansion happily producing some shorter URI. + if (!parseTemplate(uriTemplate)) return null; + const { text, slots, queryNames, order } = rewriteExpressions(uriTemplate); const template = parseTemplate(text); + /* v8 ignore next -- unreachable: the rewrite only ever shortens the template + and its names, so anything the original parse accepted parses here too. */ if (!template) return null; for (const name of order) {