From 409dce37be4647bef46f2df75972b876804fa897 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 18:51:12 -0400 Subject: [PATCH] fix(core): share URI-template expansion and correct the SDK's multi-name branch Addresses Copilot's review on #2035. All three findings reproduced against the pinned SDK before acting. 1. `#` was misclassified as required. Measured: `x://a{#frag}` with no `frag` expands to exactly `x://a`, a well-formed URI naming a real resource -- unlike `{+path}` (`x://a/`) or a simple `{userId}` (`file:///users//profile`), which leave an empty path segment. Moved `#` into the omittable set; the required cases now assert what the URI *becomes* when blank, so the rule is checked rather than asserted. 2. Delegating to the SDK did not actually give RFC 6570 expansion. `UriTemplate.expandPart` takes an early `names.length > 1` branch that raw-joins values, skipping both `encodeValue` and the operator prefix: `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` -- the very unencoded-slash defect this PR is about -- and `x://a{/p,q}` to `x://ax y,z`. Only `?`/`&` are correct, being dispatched earlier. Fixing that in the web client alone would have left the TUI and CLI wrong, since `readResourceFromTemplate` expands through the same class. So parse/classify/expand now live in `core/mcp/uriTemplate.ts` and both call sites use it. The correction is surgical: a multi-name non-query expression is expanded here and spliced in as literal text before the SDK sees the template (safe -- both encoders escape `{`/`}`), while every single-name and query expression still goes through the SDK untouched. The preview applies the same correction, so it cannot promise a URI that submitting would not send. 3. The showcase promised a blank `{?topic}` read that did not work. `UriTemplate.match()` compiles `{?topic}` to a *required* `\?topic=([^&]+)`, so `match("foobar://events")` returns null and the read 404s. A real server exposes the unfiltered collection as its own resource; the showcase now registers `foobar://events` so the documented step resolves. Verified end to end through the CLI. Signed-off-by: cliffhall --- AGENTS.md | 9 +- README.md | 9 +- .../web/src/test/core/mcp/uriTemplate.test.ts | 194 +++++++++++++ clients/web/src/utils/uriTemplate.test.ts | 133 +-------- clients/web/src/utils/uriTemplate.ts | 187 ++----------- core/mcp/inspectorClient.ts | 20 +- core/mcp/uriTemplate.ts | 257 ++++++++++++++++++ .../configs/rfc6570-templates-http.json | 19 +- test-servers/src/preset-registry.ts | 3 + test-servers/src/test-server-fixtures.ts | 21 ++ 10 files changed, 559 insertions(+), 293 deletions(-) create mode 100644 clients/web/src/test/core/mcp/uriTemplate.test.ts create mode 100644 core/mcp/uriTemplate.ts diff --git a/AGENTS.md b/AGENTS.md index d07e84f4..5005d6d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,7 +73,14 @@ v2/main/ │ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores -│ │ # (modernTaskSchemas.ts: SEP-2663 modern Tasks +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand +│ │ # shared by the web Resources form and +│ │ # readResourceFromTemplate (TUI + CLI), so the +│ │ # clients cannot drift on what a template means; +│ │ # delegates to the SDK's UriTemplate but corrects +│ │ # its multi-name `{a,b}` branch, which skips both +│ │ # encoding and the operator prefix — #1919; +│ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; │ │ # listSalvage.ts: per-item salvage for list results — diff --git a/README.md b/README.md index de94130e..0bc30868 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,8 @@ inspector/ │ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union │ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton -│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import +│ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, +│ │ # and the RFC 6570 URI-template helpers all three clients expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends @@ -240,13 +241,15 @@ The **TUI** had the same gap and is worth checking against the same server (`--t #### RFC 6570 resource templates -`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against and the variables the server decoded. Plain streamable-HTTP; connect with the **default (legacy)** protocol era. +`rfc6570-templates-http.json` serves two resource templates straight out of [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) — `events_by_topic` (`foobar://events/{topic}`) and `events_by_query` (`foobar://events{?topic}`) — each echoing the URI it was matched against, plus a plain `foobar://events` resource (see below). Plain streamable-HTTP; connect with the **default (legacy)** protocol era. Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The request must go out as `foobar://events/foo%2Fbar`, and the result echoes back the URI the server matched. On the broken build the value was spliced in raw, so the slash created a second path segment and the SDK's matcher answered `-32602 Resource not found: foobar://events/foo/bar` — the exact failure in the issue. The same holds for `?`, `#`, `%`, spaces, and non-ASCII text. **events_by_query** is the half that was invisible: the old `/\{(\w+)\}/g` scan could not see an expression carrying an operator, so no `topic` input was rendered at all. It now appears, marked **Optional** — RFC 6570 drops the whole expression when the variable is undefined, so reading with the field blank requests `foobar://events`, and filling it in requests `foobar://events?topic=foo%2Fbar`. The URI preview beside the title shows the partially-expanded form as you type, leaving unfilled expressions standing as written. -Both clients expand through the SDK's `UriTemplate` — the web client via [`clients/web/src/utils/uriTemplate.ts`](./clients/web/src/utils/uriTemplate.ts), the TUI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. +> The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. + +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` with one correction applied first: the SDK's `expandPart` takes an early `names.length > 1` branch that raw-joins the values, skipping both percent-encoding **and** the operator prefix, so `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` and `x://a{/p,q}` to `x://ax y,z`. Those expressions are expanded by the helper and spliced in as literal text before the SDK sees them; every single-name and query expression still goes through the SDK untouched. #### Advertised extensions diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts new file mode 100644 index 00000000..570caffc --- /dev/null +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -0,0 +1,194 @@ +import { describe, it, expect } from "vitest"; +import { + expandUriTemplate, + parseUriTemplate, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; + +describe("parseUriTemplate", () => { + it("splits literals from expressions", () => { + expect(parseUriTemplate("foobar://events/{topic}")).toEqual([ + { kind: "literal", text: "foobar://events/" }, + { + kind: "expression", + source: "{topic}", + operator: "", + names: ["topic"], + }, + ]); + }); + + it("reads the operator and the comma-separated name list", () => { + expect(parseUriTemplate("x://{?a,b*}")).toEqual([ + { kind: "literal", text: "x://" }, + { + kind: "expression", + source: "{?a,b*}", + operator: "?", + names: ["a", "b"], + }, + ]); + }); + + it("treats an unclosed expression as trailing literal text", () => { + expect(parseUriTemplate("x://a/{oops")).toEqual([ + { kind: "literal", text: "x://a/{oops" }, + ]); + }); + + it("returns nothing for an empty template", () => { + expect(parseUriTemplate("")).toEqual([]); + }); +}); + +describe("templateVariables", () => { + it("finds a simple variable and marks it required", () => { + expect(templateVariables("foobar://events/{topic}")).toEqual([ + { name: "topic", operator: "", required: true }, + ]); + }); + + it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { + expect(templateVariables("foobar://events{?topic}")).toEqual([ + { name: "topic", operator: "?", required: false }, + ]); + }); + + // Required iff omitting the variable leaves an empty slot mid-URI rather + // than a shorter, still-well-formed URI. Verified against the pinned SDK: + // `x://a/{+path}` with no `path` expands to "x://a/" (empty segment), while + // `x://a{#frag}` expands to exactly "x://a". + it.each([ + ["{+path}", "+", true], + ["{#frag}", "#", false], + ["{.label}", ".", false], + ["{/segment}", "/", false], + ["{&extra}", "&", false], + ])("classifies %s", (expression, operator, required) => { + const [variable] = templateVariables(`x://a${expression}`); + expect(variable.operator).toBe(operator); + expect(variable.required).toBe(required); + }); + + it("deduplicates a repeated name and keeps it required if any use is", () => { + expect(templateVariables("x://{?id}/{id}")).toEqual([ + { name: "id", operator: "?", required: true }, + ]); + }); + + it.each([ + ["file:///users/{userId}/profile", "file:///users//profile"], + ["x://a/{+path}", "x://a/"], + ])( + "requires %s because omitting it leaves an empty slot (%s)", + (template, omitted) => { + expect(templateVariables(template)[0].required).toBe(true); + // The reason, asserted rather than asserted-about: this is what the URI + // would become if the field were left blank. + expect(expandUriTemplate(template, {})).toBe(omitted); + }, + ); + + it("does not require {#frag}, which omits to a well-formed URI", () => { + expect(templateVariables("x://a{#frag}")[0].required).toBe(false); + expect(expandUriTemplate("x://a{#frag}", {})).toBe("x://a"); + }); + + it("returns an empty list for a template with no expressions", () => { + expect(templateVariables("file:///static.txt")).toEqual([]); + }); +}); + +describe("expandUriTemplate", () => { + it("percent-encodes a reserved character in a simple variable (#1919)", () => { + expect( + expandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), + ).toBe("foobar://events/foo%2Fbar"); + }); + + it.each([ + ["?", "a?b", "a%3Fb"], + ["#", "a#b", "a%23b"], + ["%", "a%b", "a%25b"], + ["space", "a b", "a%20b"], + ["unicode", "caffè", "caff%C3%A8"], + ])("encodes %s", (_label, value, encoded) => { + expect(expandUriTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); + }); + + it("builds an encoded query expression", () => { + expect( + expandUriTemplate("foobar://events{?topic}", { topic: "foo/bar" }), + ).toBe("foobar://events?topic=foo%2Fbar"); + }); + + it("leaves reserved characters intact under the + operator", () => { + expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); + }); + + it("omits an expression whose variable was left blank", () => { + expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( + "foobar://events", + ); + }); + + it("joins two query expressions with & rather than a second ?", () => { + expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( + "x://a?one=1&two=2", + ); + }); + + it("falls back to the raw template when the SDK cannot parse it", () => { + expect(expandUriTemplate("x://a/{oops", { oops: "v" })).toBe("x://a/{oops"); + }); +}); + +describe("expandUriTemplate - multi-name expressions (SDK correction)", () => { + // The pinned SDK's `expandPart` takes an early `names.length > 1` branch that + // raw-joins the values, skipping BOTH encodeValue and the operator prefix. + // Measured directly: `x://{a,b}` -> "x://foo/bar,q", `x://a{/p,q}` -> + // "x://ax y,z". These assert the corrected output. + it("encodes each value in a simple multi-name expression", () => { + expect(expandUriTemplate("x://{a,b}", { a: "foo/bar", b: "q" })).toBe( + "x://foo%2Fbar,q", + ); + }); + + it("keeps the / operator prefix and separator, and encodes", () => { + expect(expandUriTemplate("x://a{/p,q}", { p: "x y", q: "z" })).toBe( + "x://a/x%20y/z", + ); + }); + + it("keeps the . operator prefix and separator", () => { + expect(expandUriTemplate("x://a{.p,q}", { p: "x/y", q: "z" })).toBe( + "x://a.x%2Fy.z", + ); + }); + + it("keeps the # prefix and leaves reserved characters under it", () => { + expect(expandUriTemplate("x://a{#p,q}", { p: "x/y", q: "z" })).toBe( + "x://a#x/y,z", + ); + }); + + it("leaves reserved characters under the + operator", () => { + expect(expandUriTemplate("x://{+a,b}", { a: "x/y", b: "z" })).toBe( + "x://x/y,z", + ); + }); + + it("drops only the undefined names, keeping the rest", () => { + expect(expandUriTemplate("x://a{/p,q}", { q: "z" })).toBe("x://a/z"); + }); + + it("omits the whole expression when no name has a value", () => { + expect(expandUriTemplate("x://a{/p,q}", {})).toBe("x://a"); + }); + + it("leaves multi-name query expressions to the SDK, which handles them", () => { + expect(expandUriTemplate("x://a{?p,q}", { p: "x/y", q: "z" })).toBe( + "x://a?p=x%2Fy&q=z", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index b337a42b..7a1ead92 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -1,126 +1,5 @@ import { describe, it, expect } from "vitest"; -import { - expandUriTemplate, - parseUriTemplate, - previewUriTemplate, - templateVariables, -} from "./uriTemplate"; - -describe("parseUriTemplate", () => { - it("splits literals from expressions", () => { - expect(parseUriTemplate("foobar://events/{topic}")).toEqual([ - { kind: "literal", text: "foobar://events/" }, - { - kind: "expression", - source: "{topic}", - operator: "", - names: ["topic"], - }, - ]); - }); - - it("reads the operator and the comma-separated name list", () => { - expect(parseUriTemplate("x://{?a,b*}")).toEqual([ - { kind: "literal", text: "x://" }, - { - kind: "expression", - source: "{?a,b*}", - operator: "?", - names: ["a", "b"], - }, - ]); - }); - - it("treats an unclosed expression as trailing literal text", () => { - expect(parseUriTemplate("x://a/{oops")).toEqual([ - { kind: "literal", text: "x://a/{oops" }, - ]); - }); - - it("returns nothing for an empty template", () => { - expect(parseUriTemplate("")).toEqual([]); - }); -}); - -describe("templateVariables", () => { - it("finds a simple variable and marks it required", () => { - expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true }, - ]); - }); - - it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { - expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false }, - ]); - }); - - it.each([ - ["{+path}", "+", true], - ["{#frag}", "#", true], - ["{.label}", ".", false], - ["{/segment}", "/", false], - ["{&extra}", "&", false], - ])("classifies %s", (expression, operator, required) => { - const [variable] = templateVariables(`x://a${expression}`); - expect(variable.operator).toBe(operator); - expect(variable.required).toBe(required); - }); - - it("deduplicates a repeated name and keeps it required if any use is", () => { - expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true }, - ]); - }); - - it("returns an empty list for a template with no expressions", () => { - expect(templateVariables("file:///static.txt")).toEqual([]); - }); -}); - -describe("expandUriTemplate", () => { - it("percent-encodes a reserved character in a simple variable (#1919)", () => { - expect( - expandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }), - ).toBe("foobar://events/foo%2Fbar"); - }); - - it.each([ - ["?", "a?b", "a%3Fb"], - ["#", "a#b", "a%23b"], - ["%", "a%b", "a%25b"], - ["space", "a b", "a%20b"], - ["unicode", "caffè", "caff%C3%A8"], - ])("encodes %s", (_label, value, encoded) => { - expect(expandUriTemplate("x://{v}", { v: value })).toBe(`x://${encoded}`); - }); - - it("builds an encoded query expression", () => { - expect( - expandUriTemplate("foobar://events{?topic}", { topic: "foo/bar" }), - ).toBe("foobar://events?topic=foo%2Fbar"); - }); - - it("leaves reserved characters intact under the + operator", () => { - expect(expandUriTemplate("x://{+path}", { path: "a/b" })).toBe("x://a/b"); - }); - - it("omits an expression whose variable was left blank", () => { - expect(expandUriTemplate("foobar://events{?topic}", { topic: "" })).toBe( - "foobar://events", - ); - }); - - it("joins two query expressions with & rather than a second ?", () => { - expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( - "x://a?one=1&two=2", - ); - }); - - it("falls back to the raw template when the SDK cannot parse it", () => { - expect(expandUriTemplate("x://a/{oops", { oops: "v" })).toBe("x://a/{oops"); - }); -}); +import { previewUriTemplate } from "./uriTemplate"; describe("previewUriTemplate", () => { it("shows the template verbatim before anything is entered", () => { @@ -166,3 +45,13 @@ describe("previewUriTemplate", () => { expect(previewUriTemplate("x://a/{oops", {})).toBe("x://a/{oops"); }); }); + +describe("previewUriTemplate - multi-name expressions", () => { + it("applies the same correction the real expansion does", () => { + // Must match expandUriTemplate("x://{a,b}", ...) exactly, or the preview + // would promise a URI that submitting does not send. + expect(previewUriTemplate("x://{a,b}", { a: "foo/bar", b: "q" })).toBe( + "x://foo%2Fbar,q", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 25cd027e..4c3f8ea2 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -1,167 +1,29 @@ /** - * RFC 6570 URI Template support for the Resources screen. + * The web Resources form's view of an RFC 6570 URI template (#1919). * - * The web client used to discover and substitute variables with a bare - * `/\{(\w+)\}/g` regex, which is wrong in two ways (#1919): it cannot see an - * expression carrying an operator (`{?topic}`, `{/path}`, `{#frag}`, ...), so - * no input is rendered for it; and it splices the raw value in, so a `/`, `?`, - * `#` or space in a simple `{var}` lands unencoded and silently changes the - * URI's structure. - * - * Expansion itself is delegated to the SDK's `UriTemplate` -- the same - * implementation `InspectorClient.readResourceFromTemplate` (and therefore the - * TUI) already expands through, so the two clients cannot disagree about what a - * template means. What lives here is the surrounding form/preview logic the SDK - * class does not provide: which variables to render an input for, which of them - * a read cannot proceed without, and a partially-expanded preview string. + * Parsing, variable classification, and expansion live in + * `@inspector/core/mcp/uriTemplate.js` so the web form, the TUI, and the CLI + * cannot disagree about what a template means -- they are re-exported here so + * the panel has a single import. What this module adds is the one piece that is + * purely a display concern: the partially-expanded preview string. */ +import { + applyMultiNameCorrection, + definedValues, + parseUriTemplate, +} from "@inspector/core/mcp/uriTemplate.js"; import { UriTemplate } from "@modelcontextprotocol/client"; -/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ -const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; - -/** - * Operators whose expansion omits cleanly when the variables in it are - * undefined -- the whole expression, separator included, simply disappears. - * A variable under any *other* operator is interpolated into the middle of the - * URI, so leaving it out produces a different resource path rather than a - * shorter one; those we require before allowing a read. - */ -const OMITTABLE_OPERATORS = new Set([".", "/", "?", "&"]); - -interface TemplateLiteral { - kind: "literal"; - text: string; -} - -interface TemplateExpression { - kind: "expression"; - /** The expression including its braces, e.g. `{?topic}`. */ - source: string; - /** The RFC 6570 operator, or `""` for a simple expression. */ - operator: string; - /** Variable names in the expression, `*` (explode) and whitespace stripped. */ - names: string[]; -} - -export type TemplatePart = TemplateLiteral | TemplateExpression; - -export interface TemplateVariable { - name: string; - /** The operator of the expression the variable was first seen in. */ - operator: string; - /** - * True when omitting the variable would change the URI's structure rather - * than shorten it -- see {@link OMITTABLE_OPERATORS}. - */ - required: boolean; -} - -/** - * Splits a template into literal runs and expressions. - * - * Deliberately mirrors the SDK parser's own scanning rules (first matching - * operator character wins, names split on `,`, `*` stripped) so this never - * disagrees with the class that ultimately does the expanding. An unclosed - * `{` yields a trailing literal, which is what makes the callers below degrade - * to "render no inputs, show the template verbatim" rather than throw. - */ -export function parseUriTemplate(uriTemplate: string): TemplatePart[] { - const parts: TemplatePart[] = []; - let literal = ""; - let i = 0; - - while (i < uriTemplate.length) { - if (uriTemplate[i] !== "{") { - literal += uriTemplate[i]; - i += 1; - continue; - } - const end = uriTemplate.indexOf("}", i); - if (end === -1) { - // Unclosed expression -- the rest is not a template, treat it as text. - literal += uriTemplate.slice(i); - break; - } - if (literal) { - parts.push({ kind: "literal", text: literal }); - literal = ""; - } - const body = uriTemplate.slice(i + 1, end); - const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const names = body - .slice(operator.length) - .split(",") - .map((name) => name.replace("*", "").trim()) - .filter((name) => name.length > 0); - parts.push({ - kind: "expression", - source: uriTemplate.slice(i, end + 1), - operator, - names, - }); - i = end + 1; - } - - if (literal) parts.push({ kind: "literal", text: literal }); - return parts; -} - -/** - * The variables a form should render an input for, in template order and - * deduplicated by name. A name appearing under more than one operator is - * required if *any* of its occurrences is. - */ -export function templateVariables(uriTemplate: string): TemplateVariable[] { - const byName = new Map(); - - for (const part of parseUriTemplate(uriTemplate)) { - if (part.kind !== "expression") continue; - const required = !OMITTABLE_OPERATORS.has(part.operator); - for (const name of part.names) { - const existing = byName.get(name); - if (existing) { - existing.required = existing.required || required; - } else { - byName.set(name, { name, operator: part.operator, required }); - } - } - } - - return [...byName.values()]; -} - -/** - * Drops empty entries so an untouched optional field reads as *undefined* to - * the SDK (the expression disappears) rather than as the empty string (which - * would expand to a valueless `?topic=`). - */ -function definedValues(values: Record): Record { - return Object.fromEntries( - Object.entries(values).filter(([, value]) => value.length > 0), - ); -} - -/** - * Expands a template against the entered values per RFC 6570 -- percent- - * encoding each value according to its operator, and omitting expressions whose - * variables were left blank. - * - * A template the SDK refuses to parse falls back to the raw template string, - * which is what the user already sees in the preview and what the server will - * reject with a legible error; throwing here would take out the whole panel. - */ -export function expandUriTemplate( - uriTemplate: string, - values: Record, -): string { - try { - return new UriTemplate(uriTemplate).expand(definedValues(values)); - } catch { - return uriTemplate; - } -} +export { + expandUriTemplate, + parseUriTemplate, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; +export type { + TemplatePart, + TemplateVariable, +} from "@inspector/core/mcp/uriTemplate.js"; /** * A placeholder standing in for an expression the user has not filled in yet. @@ -174,8 +36,8 @@ const deferredToken = (index: number) => `\u0000${index}\u0000`; /** * A partially-expanded template for display: expressions whose variables are - * all filled are expanded exactly as {@link expandUriTemplate} would, and the - * rest are left standing as written so the user can see what is still needed. + * all filled are expanded exactly as `expandUriTemplate` would, and the rest + * are left standing as written so the user can see what is still needed. * * Unfilled expressions are swapped for an inert token and restored after * expansion -- rather than expanding each filled expression in isolation -- so @@ -194,7 +56,10 @@ export function previewUriTemplate( .map((part) => { if (part.kind === "literal") return part.text; if (part.names.every((name) => defined[name] !== undefined)) { - return part.source; + // Route the kept expression through the same multi-name correction the + // real expansion applies, so the preview can never promise a URI that + // submitting would not actually send. + return applyMultiNameCorrection([part], defined); } deferred.push(part.source); return deferredToken(deferred.length - 1); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index a635582d..9cdedd1c 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -191,6 +191,11 @@ import { convertPromptArguments, } from "../json/jsonUtils.js"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + applyMultiNameCorrection, + definedValues, + parseUriTemplate, +} from "./uriTemplate.js"; import { InspectorClientEventTarget, type TaskWithOptionalCreatedAt, @@ -4971,11 +4976,20 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; - // Expand the template's uriTemplate using the provided params + // Expand the template's uriTemplate using the provided params. Routed + // through the shared helpers in ./uriTemplate.js so this and the web + // Resources form cannot disagree: `definedValues` makes a blank field read + // as *undefined* (the expression drops out) rather than as the empty string + // (a valueless `?topic=`), and `applyMultiNameCorrection` fixes the SDK's + // mis-expansion of `{a,b}`-style expressions before it sees them (#1919). let expandedUri: string; try { - const uriTemplate = new UriTemplate(uriTemplateString); - expandedUri = uriTemplate.expand(params); + const defined = definedValues(params); + const corrected = applyMultiNameCorrection( + parseUriTemplate(uriTemplateString), + defined, + ); + expandedUri = new UriTemplate(corrected).expand(defined); } catch (error) { throw new Error( `Failed to expand URI template "${uriTemplate}": ${error instanceof Error ? error.message : String(error)}`, diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts new file mode 100644 index 00000000..0518b608 --- /dev/null +++ b/core/mcp/uriTemplate.ts @@ -0,0 +1,257 @@ +/** + * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). + * + * Expansion is delegated to the SDK's `UriTemplate` — with one correction + * applied first, documented on {@link expandMultiNameExpression} below. This + * lives in `core/` rather than in a client so the web form, the TUI, and the + * CLI cannot disagree about what a template means; `InspectorClient + * .readResourceFromTemplate` and the web Resources form both expand through + * {@link expandUriTemplate}. + */ + +import { UriTemplate } from "@modelcontextprotocol/client"; + +/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ +const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; + +/** + * Operators whose expansion omits cleanly when the variables in it are + * undefined — the whole expression, separator and all, simply disappears and + * what is left is still a well-formed URI naming a real (broader) resource. + * + * The remaining operators — `""` (simple) and `+` (reserved) — interpolate the + * value into the *middle* of the URI, so omitting one leaves an empty path + * segment rather than a shorter URI: measured against the pinned SDK, + * `file:///users/{userId}/profile` expands to `file:///users//profile` and + * `x://a/{+path}` to `x://a/`, both of which name a different resource. Those + * are the ones a form must require before allowing a read. + * + * `#` is in the omittable set on the same measurement: `x://a{#frag}` with no + * `frag` expands to exactly `x://a`. A fragment is optional by construction. + */ +const OMITTABLE_OPERATORS = new Set(["#", ".", "/", "?", "&"]); + +interface TemplateLiteral { + kind: "literal"; + text: string; +} + +interface TemplateExpression { + kind: "expression"; + /** The expression including its braces, e.g. `{?topic}`. */ + source: string; + /** The RFC 6570 operator, or `""` for a simple expression. */ + operator: string; + /** Variable names in the expression, `*` (explode) and whitespace stripped. */ + names: string[]; +} + +export type TemplatePart = TemplateLiteral | TemplateExpression; + +export interface TemplateVariable { + name: string; + /** The operator of the expression the variable was first seen in. */ + operator: string; + /** + * True when omitting the variable would change the URI's structure rather + * than shorten it — see {@link OMITTABLE_OPERATORS}. + */ + required: boolean; +} + +/** + * Splits a template into literal runs and expressions. + * + * Deliberately mirrors the SDK parser's own scanning rules (first matching + * operator character wins, names split on `,`, `*` stripped) so this never + * disagrees with the class that ultimately does the expanding. An unclosed + * `{` yields a trailing literal, which is what makes the callers below degrade + * to "render no inputs, show the template verbatim" rather than throw. + */ +export function parseUriTemplate(uriTemplate: string): TemplatePart[] { + const parts: TemplatePart[] = []; + let literal = ""; + let i = 0; + + while (i < uriTemplate.length) { + if (uriTemplate[i] !== "{") { + literal += uriTemplate[i]; + i += 1; + continue; + } + const end = uriTemplate.indexOf("}", i); + if (end === -1) { + // Unclosed expression — the rest is not a template, treat it as text. + literal += uriTemplate.slice(i); + break; + } + if (literal) { + parts.push({ kind: "literal", text: literal }); + literal = ""; + } + const body = uriTemplate.slice(i + 1, end); + const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; + const names = body + .slice(operator.length) + .split(",") + .map((name) => name.replace("*", "").trim()) + .filter((name) => name.length > 0); + parts.push({ + kind: "expression", + source: uriTemplate.slice(i, end + 1), + operator, + names, + }); + i = end + 1; + } + + if (literal) parts.push({ kind: "literal", text: literal }); + return parts; +} + +/** + * The variables a form should render an input for, in template order and + * deduplicated by name. A name appearing under more than one operator is + * required if *any* of its occurrences is. + */ +export function templateVariables(uriTemplate: string): TemplateVariable[] { + const byName = new Map(); + + for (const part of parseUriTemplate(uriTemplate)) { + if (part.kind !== "expression") continue; + const required = !OMITTABLE_OPERATORS.has(part.operator); + for (const name of part.names) { + const existing = byName.get(name); + if (existing) { + existing.required = existing.required || required; + } else { + byName.set(name, { name, operator: part.operator, required }); + } + } + } + + return [...byName.values()]; +} + +/** + * Drops empty entries so an untouched optional field reads as *undefined* to + * the SDK (the expression disappears) rather than as the empty string (which + * would expand to a valueless `?topic=`). + */ +export function definedValues( + values: Record, +): Record { + return Object.fromEntries( + Object.entries(values).filter(([, value]) => value.length > 0), + ); +} + +/** The SDK's `encodeValue`: reserved characters survive under `+` and `#`. */ +function encodeValue(value: string, operator: string): string { + return operator === "+" || operator === "#" + ? encodeURI(value) + : encodeURIComponent(value); +} + +/** + * Expands a **multi-name, non-query** expression (`{a,b}`, `{/a,b}`, …). + * + * The pinned SDK gets this branch wrong: `UriTemplate.expandPart` takes an + * early `part.names.length > 1` path that raw-joins the values with `,` — + * skipping `encodeValue` *and* the operator prefix entirely. Measured against + * the pinned SDK, `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` + * (unencoded, so the slash creates a path segment — the very defect #1919 is + * about), and `x://a{/p,q}` expands to `x://ax y,z` — no leading `/`, spaces + * intact. Only the `?`/`&` operators are handled correctly there, because they + * are dispatched before that branch. + * + * So those expressions are expanded here and spliced into the template as + * *literal* text before the SDK ever sees them. This is deliberately surgical: + * every other shape — the overwhelmingly common single-name expression, and + * every query expression — still goes through the SDK untouched, so the two + * cannot drift on the ordinary path, and if the SDK fixes its branch this + * correction keeps producing the same (correct) answer. + * + * Splicing is safe because both encoders escape `{` and `}` (to `%7B`/`%7D`), + * so an expanded value can never be re-parsed as an expression. + * + * Returns `""` when no name in the expression has a value, matching RFC 6570's + * rule that an expression with only undefined variables expands to nothing. + */ +function expandMultiNameExpression( + part: TemplateExpression, + values: Record, +): string { + const encoded = part.names + .map((name) => values[name]) + .filter((value) => value !== undefined) + .map((value) => encodeValue(value, part.operator)); + + if (encoded.length === 0) return ""; + + switch (part.operator) { + case "#": + return `#${encoded.join(",")}`; + case ".": + return `.${encoded.join(".")}`; + case "/": + return `/${encoded.join("/")}`; + // "" and "+" — a bare comma-joined list, no prefix. + default: + return encoded.join(","); + } +} + +/** + * True for the expressions {@link expandMultiNameExpression} has to take over: + * more than one name, and not a query operator (which the SDK dispatches before + * its broken branch and therefore handles correctly). + */ +function needsMultiNameCorrection(part: TemplateExpression): boolean { + return ( + part.names.length > 1 && part.operator !== "?" && part.operator !== "&" + ); +} + +/** + * Rebuilds `uriTemplate` with every mis-expanded multi-name expression already + * resolved to literal text, leaving the rest for the SDK. + */ +export function applyMultiNameCorrection( + parts: TemplatePart[], + values: Record, +): string { + return parts + .map((part) => { + if (part.kind === "literal") return part.text; + return needsMultiNameCorrection(part) + ? expandMultiNameExpression(part, values) + : part.source; + }) + .join(""); +} + +/** + * Expands a template against the entered values per RFC 6570 — percent-encoding + * each value according to its operator, and omitting expressions whose + * variables were left blank. + * + * A template the SDK refuses to parse falls back to the raw template string, + * which is what the user already sees in the preview and what the server will + * reject with a legible error; throwing here would take out the whole panel. + */ +export function expandUriTemplate( + uriTemplate: string, + values: Record, +): string { + const defined = definedValues(values); + try { + const corrected = applyMultiNameCorrection( + parseUriTemplate(uriTemplate), + defined, + ); + return new UriTemplate(corrected).expand(defined); + } catch { + return uriTemplate; + } +} diff --git a/test-servers/configs/rfc6570-templates-http.json b/test-servers/configs/rfc6570-templates-http.json index c6949773..61b552c9 100644 --- a/test-servers/configs/rfc6570-templates-http.json +++ b/test-servers/configs/rfc6570-templates-http.json @@ -3,10 +3,23 @@ "name": "rfc6570-template-showcase", "version": "1.0.0" }, - "resourceTemplates": [{ "preset": "rfc6570_templates" }], - "tools": [{ "preset": "echo" }], + "resourceTemplates": [ + { + "preset": "rfc6570_templates" + } + ], + "tools": [ + { + "preset": "echo" + } + ], "transport": { "type": "streamable-http", "port": 6603 - } + }, + "resources": [ + { + "preset": "rfc6570_base" + } + ] } diff --git a/test-servers/src/preset-registry.ts b/test-servers/src/preset-registry.ts index 34fd53ad..2def3a1a 100644 --- a/test-servers/src/preset-registry.ts +++ b/test-servers/src/preset-registry.ts @@ -63,6 +63,7 @@ import { createUserResourceTemplate, createNumberedResourceTemplates, createRfc6570ResourceTemplates, + createRfc6570BaseResource, createSimplePrompt, createArgsPrompt, createNumberedPrompts, @@ -243,6 +244,8 @@ function resolveResourcePreset( return createNumberedResources(Number(get("count")) || 3); case "mcp_app_demo_widget": return createMcpAppDemoResource(); + case "rfc6570_base": + return createRfc6570BaseResource(); default: throw new Error(`Unknown resource preset: ${name}`); } diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 89283e49..9e58cfaa 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -1485,6 +1485,27 @@ export function createFileResourceTemplate( * Both handlers echo the URI they were matched against plus the variables the * server decoded, so the round-trip is visible in the read result. */ +/** + * The plain `foobar://events` resource that a blank `{?topic}` read lands on. + * + * Registering it is not decoration. RFC 6570 omits a query expression whose + * variable is undefined, so leaving `topic` blank legitimately requests the + * unfiltered collection -- but the SDK's own matcher cannot serve that from the + * template: `UriTemplate.partToRegExp` compiles `{?topic}` to a **required** + * `\?topic=([^&]+)`, so `match("foobar://events")` returns null and the read + * would 404. A real server would expose the collection as its own resource; + * this fixture does the same so the showcase's "read it blank" step works. + */ +export function createRfc6570BaseResource(): ResourceDefinition { + return { + uri: "foobar://events", + name: "events", + description: "All events - what a blank `{?topic}` read resolves to", + mimeType: "application/json", + text: JSON.stringify({ collection: "events", filtered: false }, null, 2), + }; +} + export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] { const echo = (label: string) => async (uri: URL, params: Record) => ({