diff --git a/AGENTS.md b/AGENTS.md index 3b5df783..ccb2935c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,24 +73,22 @@ v2/main/ │ │ # a nullable field entirely — #1928/#2015) │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores -│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand -│ │ # shared by the web Resources form and -│ │ # readResourceFromTemplate (TUI + CLI), so the -│ │ # clients cannot drift on what a template means; -│ │ # delegates to the SDK's UriTemplate for what it -│ │ # gets right, and takes over a whole template -│ │ # containing any of the three shapes it does not: -│ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), -│ │ # `{;id}` (operator absent from its list), and -│ │ # `{id:3}` (prefix modifier folded into the name), -│ │ # and `{+v}`/`{#v}` (encodeURI mangles reserved -│ │ # `[`/`]` and double-encodes pct-triplets). The `;` -│ │ # and `:N` cases would render form fields literally -│ │ # labelled `;id` / `id:3`. Requiredness is per -│ │ # EXPRESSION, not per variable — requiredGroups + -│ │ # hasRequiredValues: `{a,b}` with only `a` filled is -│ │ # expandable, and a name recurring across -│ │ # expressions needs each group checked — #1919; +│ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. +│ │ # The ONE expander for every client — the web +│ │ # Resources form and readResourceFromTemplate +│ │ # (TUI + CLI) — and every client derives its FORM +│ │ # FIELDS from it too (clients/tui uriTemplateToForm), +│ │ # which is what makes the sharing real: a form +│ │ # submits under the names it rendered, so a mangled +│ │ # name silently drops the value at expansion. The +│ │ # SDK's UriTemplate is used only to VALIDATE; its +│ │ # expander is wrong for `{a,b}` (raw-joined), +│ │ # `{;id}` (operator missing), `{id:3}` (modifier in +│ │ # the name), `{+v}`/`{#v}` (encodeURI mangles `[`/`]` +│ │ # and double-encodes pct-triplets), and `{v}` +│ │ # (encodeURIComponent leaves `!'()*` bare). +│ │ # Requiredness is per EXPRESSION, not per variable — +│ │ # requiredGroups + hasRequiredValues — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index af8917c5..d4b0e61e 100644 --- a/README.md +++ b/README.md @@ -249,20 +249,21 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — and all three derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. -| Shape | SDK behavior | Correct | -| --- | --- | --- | -| `{a,b}` | `foo/bar,q` — raw-joined, unencoded, operator prefix dropped | `foo%2Fbar,q` | -| `{;id}` | `""` — the `;` operator is not in its list, so the variable parses as `;id` | `;id=7` | -| `{id:3}` | `""` — the prefix modifier is folded into the name, giving `id:3` | `abc` | -| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | `[::1]`, `%2F` | +The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: -The `;` and `:3` rows matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. +| Shape | SDK behavior | +| --- | --- | +| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | +| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | +| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | +| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | -Requiredness is likewise a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and the form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which a per-variable flag cannot express once a name recurs across expressions. +The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the form renders fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. -One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. +Requiredness is a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and a form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which no per-variable flag can express once a name recurs across expressions (`{a,b}{a,c}` is satisfied by filling `b` and `c`). #### Advertised extensions diff --git a/clients/tui/__tests__/uriTemplateToForm.test.ts b/clients/tui/__tests__/uriTemplateToForm.test.ts index 1c522c65..9dc1a32c 100644 --- a/clients/tui/__tests__/uriTemplateToForm.test.ts +++ b/clients/tui/__tests__/uriTemplateToForm.test.ts @@ -11,7 +11,38 @@ describe("uriTemplateToForm", () => { expect(form.title).toBe("Read Resource: file"); const fields = form.sections[0]!.fields; expect(fields.map((f) => f.name)).toEqual(["path", "name"]); - expect(fields[0]).toMatchObject({ type: "string", required: false }); + // Simple `{path}` sits mid-URI, so omitting it would leave an empty path + // segment rather than a shorter URI -- it is mandatory, matching the web + // panel. Both clients read this from core's `requiredGroups`. + expect(fields[0]).toMatchObject({ type: "string", required: true }); + }); + + it("names fields as the expander looks them up, not as the SDK parses them", () => { + // The SDK reports these as ";id" and "id:3"; submitting under those keys + // would make the expander find nothing and drop the expression (#1919). + expect( + uriTemplateToForm("x://a{;id}", "matrix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + expect( + uriTemplateToForm("x://a/{id:3}", "prefix").sections[0]!.fields.map( + (f) => f.name, + ), + ).toEqual(["id"]); + }); + + it("leaves a shared required group optional rather than demanding every name", () => { + // `{a,b}` is satisfied by either name, and ink-form cannot say "any one + // of"; marking both required would refuse input the expander accepts. + const fields = uriTemplateToForm("x://{a,b}", "pair").sections[0]!.fields; + expect(fields.map((f) => f.name)).toEqual(["a", "b"]); + expect(fields.every((f) => f.required === false)).toBe(true); + }); + + it("marks an omittable variable optional", () => { + const fields = uriTemplateToForm("x://a{?topic}", "q").sections[0]!.fields; + expect(fields[0]).toMatchObject({ name: "topic", required: false }); }); it("returns an empty Template Variables section for a static URI", () => { diff --git a/clients/tui/src/utils/uriTemplateToForm.ts b/clients/tui/src/utils/uriTemplateToForm.ts index c8a027e9..af580a98 100644 --- a/clients/tui/src/utils/uriTemplateToForm.ts +++ b/clients/tui/src/utils/uriTemplateToForm.ts @@ -4,32 +4,50 @@ import type { FormStructure, FormSection, FormField } from "ink-form"; import { UriTemplate } from "@modelcontextprotocol/client"; +import { + requiredGroups, + templateVariables, +} from "@inspector/core/mcp/uriTemplate.js"; /** - * Converts a URI Template to ink-form structure + * Converts a URI Template to ink-form structure. + * + * Fields come from core's `templateVariables`, the same parser + * `InspectorClient.readResourceFromTemplate` expands through, so the key this + * form submits is the key the expander looks up. Using the SDK's + * `variableNames` here instead is not merely untidy -- it mangles the name: + * `{;id}` yields `";id"` and `{id:3}` yields `"id:3"`, so the form would submit + * `{ ";id": "7" }` while the expander looks for `id`, silently dropping the + * value and the whole expression with it (#1919). + * + * The SDK template is still constructed, but only to validate: core's parser is + * deliberately lenient (an unclosed `{` becomes literal text), so this is what + * still surfaces a malformed template as an empty form plus a logged error. */ export function uriTemplateToForm( uriTemplate: string, templateName: string, ): FormStructure { - const fields: FormField[] = []; + let fields: FormField[] = []; try { - const template = new UriTemplate(uriTemplate); - /* v8 ignore next -- UriTemplate.variableNames is a getter that always - returns a string[]; the `|| []` fallback is an unreachable guard. */ - const variableNames = template.variableNames || []; - - for (const variableName of variableNames) { - const field: FormField = { - name: variableName, - label: variableName, - type: "string", - required: false, // URI template variables are typically optional - }; - - fields.push(field); - } + new UriTemplate(uriTemplate); + // Only a variable that is the *sole* member of a non-omittable expression + // is genuinely mandatory. RFC 6570 drops undefined names from a multi-name + // expression, so `{a,b}` needs only one of the two -- ink-form cannot + // express "any one of these", and marking both required would refuse input + // the expander accepts. + const mandatory = new Set( + requiredGroups(uriTemplate) + .filter((names) => names.length === 1) + .map(([name]) => name), + ); + fields = templateVariables(uriTemplate).map(({ name }) => ({ + name, + label: name, + type: "string", + required: mandatory.has(name), + })); } catch (error) { // If parsing fails, return empty form console.error("Failed to parse URI template:", error); diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 7b39b4ad..9c104821 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -253,9 +253,18 @@ export function ResourceTemplatePanel({ // required multi-name expression no single field is mandatory either // -- any one of them satisfies it -- so say which, rather than // marking each one required and blocking valid input. - const sharedGroup = groups.find( - (names) => names.length > 1 && names.includes(varName), + // A name can sit in a singleton required group *and* a shared one + // (`x://{a}/{a,b}`). The singleton demands this exact field, so the + // "any one of" hint would contradict the disabled submit button -- + // suppress it and let the field read as plainly required. + const individuallyRequired = groups.some( + (names) => names.length === 1 && names[0] === varName, ); + const sharedGroup = individuallyRequired + ? undefined + : groups.find( + (names) => names.length > 1 && names.includes(varName), + ); const description = !required ? "Optional" : sharedGroup diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 4c84d807..f0384f90 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -432,3 +432,36 @@ describe("allow-reserved encoding under + and #", () => { ); }); }); + +describe("unreserved encoding under the non-reserved operators", () => { + // `encodeURIComponent` leaves the sub-delims !'()* bare, but RFC 6570 only + // allows *unreserved* characters through for these operators. + it.each([ + ["", "x://"], + [".", "x://a."], + ["/", "x://a/"], + ])("encodes !'()* under the %s operator", (operator, prefix) => { + const base = operator === "" ? "x://" : "x://a"; + expect( + expandUriTemplate(`${base}{${operator}v}`, { v: "a!b'c(d)e*f" }), + ).toBe(`${prefix}a%21b%27c%28d%29e%2Af`); + }); + + it("encodes them in a named (query) expression too", () => { + expect(expandUriTemplate("x://a{?v}", { v: "a!b" })).toBe("x://a?v=a%21b"); + }); + + it("encodes them in a matrix expression too", () => { + expect(expandUriTemplate("x://a{;v}", { v: "a!b" })).toBe("x://a;v=a%21b"); + }); + + it("leaves them alone under + and #, where reserved characters are allowed", () => { + expect(expandUriTemplate("x://{+v}", { v: "a!b'c(d)e*f" })).toBe( + "x://a!b'c(d)e*f", + ); + }); + + it("still leaves the unreserved set itself untouched", () => { + expect(expandUriTemplate("x://{v}", { v: "aZ0-._~" })).toBe("x://aZ0-._~"); + }); +}); diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 0a790cf0..248abd92 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -2,32 +2,32 @@ * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * * This lives in `core/` rather than in a client so the web Resources form, the - * TUI, and the CLI cannot disagree about what a template means: the form calls - * {@link templateVariables} / {@link expandUriTemplate} directly, and the TUI - * and CLI reach the same code through `InspectorClient.readResourceFromTemplate`. + * TUI, and the CLI cannot disagree about what a template means. Every one of + * them derives its form fields from {@link templateVariables} and expands + * through {@link expandUriTemplate} — the web panel directly, the TUI and CLI + * via `InspectorClient.readResourceFromTemplate`. * * ## Why this is not simply `new UriTemplate(t).expand(v)` * - * Expansion is delegated to the SDK's `UriTemplate` for every expression it - * handles correctly — which is the overwhelmingly common case, and keeping it - * there means we cannot drift from the SDK on the ordinary path. But its parser - * and expander are incomplete in three ways that a *form* makes visible, - * because a form has to name the variables it is asking the user to fill in. - * Each was measured against the pinned SDK, not inferred: + * The SDK's `UriTemplate` is still used, but only to *validate* a template — + * constructing it is what rejects an unclosed expression. Its expander is not, + * because it is incomplete in ways a form makes visible: a form has to *name* + * the variables it asks the user to fill in, so a parser that mangles a name + * produces a field nobody can use. Each of these was measured against the + * pinned SDK, not inferred: * - * | Shape | SDK `variableNames` | SDK expansion | Correct | - * | ------------ | ------------------- | --------------------------------- | -------------- | - * | `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, no prefix| `foo%2Fbar,q` | - * | `{;id}` | `[";id"]` | `""` — operator unknown | `;id=7` | - * | `{id:3}` | `["id:3"]` | `""` — modifier folded into name | `abc` | + * | Shape | SDK behavior | + * | ---------------- | --------------------------------------------------------- | + * | `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | + * | `{;id}` | `;` is not in its operator list, so the variable is `;id` | + * | `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | + * | `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` and double-encodes `%` | + * | `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare | * - * For the last two the damage is not just a wrong URI: the form would render - * fields literally labelled `;id` and `id:3`, which the user cannot fill in - * usefully. So this module parses varspecs properly and, **when a template - * contains any expression the SDK gets wrong, expands that whole template - * itself** in {@link expandParts} rather than splicing corrected fragments into - * a template the SDK then re-expands — splicing would leave the SDK's - * cross-expression `?`-to-`&` rewrite unaware of the fragments we resolved. + * An earlier revision delegated the shapes the SDK got right and took over only + * the rest. That split is gone: once the encoders themselves differed, the two + * paths would have encoded the *same value* differently depending on whether + * the expression happened to carry a modifier. One expander, one set of rules. */ import { UriTemplate } from "@modelcontextprotocol/client"; @@ -287,11 +287,36 @@ function encodeAllowReserved(value: string): string { .join(""); } +/** + * Percent-encodes everything outside RFC 3986's *unreserved* set, which is what + * every operator except `+` and `#` calls for. + * + * `encodeURIComponent` alone is not that set: it leaves `!`, `'`, `(`, `)` and + * `*` unescaped. Those are sub-delims, not unreserved, so RFC 6570 requires + * them encoded for simple, label, path, matrix and query expansion. They are + * substituted afterwards rather than hand-rolled, so `encodeURIComponent` still + * does the UTF-8 work for everything else. + */ +const FORCE_ENCODED: Record = { + "!": "%21", + "'": "%27", + "(": "%28", + ")": "%29", + "*": "%2A", +}; + +function encodeUnreserved(value: string): string { + return encodeURIComponent(value).replace( + /[!'()*]/g, + (char) => FORCE_ENCODED[char], + ); +} + /** Encodes one value for its operator: reserved characters survive `+` and `#`. */ function encodeValue(value: string, operator: string): string { return operator === "+" || operator === "#" ? encodeAllowReserved(value) - : encodeURIComponent(value); + : encodeUnreserved(value); } /** @@ -309,33 +334,6 @@ function renderValue(value: string, spec: VarSpec, operator: string): string { return encodeValue(truncated, operator); } -/** - * True for an expression the SDK would get wrong, and which this module must - * therefore expand itself. See the table in the module comment. - */ -function needsOwnExpansion(part: TemplatePart): boolean { - if (part.kind !== "expression") return false; - return ( - // Multi-name, non-query: the SDK raw-joins, skipping encoding and prefix. - (part.varspecs.length > 1 && - part.operator !== "?" && - part.operator !== "&") || - // The SDK has no `;` operator at all. - part.operator === ";" || - // The SDK encodes `+`/`#` values with `encodeURI`, which mangles reserved - // `[`/`]` and double-encodes existing pct-triplets — see - // `encodeAllowReserved`. Taken over even for a single name so both paths - // agree on what these operators mean. - part.operator === "+" || - part.operator === "#" || - // The SDK folds everything after a `:` into the variable name. Keyed on the - // raw source rather than on a parsed `maxLength` so a *malformed* modifier - // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and - // looks up `id`, while the SDK would look up `id:` and find nothing. - part.source.includes(":") - ); -} - /** Expands one expression per RFC 6570. Returns "" if no name has a value. */ function expandExpression( part: TemplateExpression, @@ -426,11 +424,11 @@ export function expandUriTemplateStrict( values: Record, ): string { const defined = definedValues(values); - const sdkTemplate = new UriTemplate(uriTemplate); - const parts = parseUriTemplate(uriTemplate); - return parts.some(needsOwnExpansion) - ? expandParts(parts, defined) - : sdkTemplate.expand(defined); + // Constructed purely to validate: this is what throws `Unclosed template + // expression`, and callers such as `readResourceFromTemplate` wrap that + // error. Its `expand` is deliberately not used — see the module comment. + new UriTemplate(uriTemplate); + return expandParts(parseUriTemplate(uriTemplate), defined); } /**