diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 596f3aae..57b18154 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -641,6 +641,44 @@ describe("unmetRequiredGroups", () => { }); }); +describe("prefix modifier truncation", () => { + // RFC 6570 2.4.1 counts characters, and says a pct-encoded triplet counts as + // ONE. Truncating by code point cut the sixth triplet in half -- measured + // before the fix, `{+v:5}` over "%61%62%63%64%65%66" produced "x%61%256", + // because the orphaned "%6" was no longer a triplet and its `%` was then + // (correctly) encoded. A byte the caller never asked for. + it.each([ + ["+", "x{+v:5}", "x%61%62%63%64%65"], + ["#", "x{#v:5}", "x#%61%62%63%64%65"], + ])("keeps pct-triplets whole under %s", (_op, template, expected) => { + expect(expandUriTemplate(template, { v: "%61%62%63%64%65%66" })).toBe( + expected, + ); + }); + + it("counts a triplet as three characters where the encoder escapes it", () => { + // Under a non-reserved operator `%` is escaped anyway (`%61` -> `%2561`), + // so the three characters really are three and grouping them would + // mis-count the prefix. + expect(expandUriTemplate("x/{v:3}", { v: "%61%62" })).toBe("x/%2561"); + }); + + it("truncates by code point, not UTF-16 code unit", () => { + // An astral character is one character; `slice` would cut the surrogate + // pair in half and yield a lone surrogate. + expect( + expandUriTemplate("x/{v:2}", { v: "\u{1F600}\u{1F601}\u{1F602}" }), + ).toBe("x/%F0%9F%98%80%F0%9F%98%81"); + expect(expandUriTemplate("x{+v:2}", { v: "\u{1F600}\u{1F601}" })).toBe( + "x%F0%9F%98%80%F0%9F%98%81", + ); + }); + + it("leaves a value shorter than the prefix untouched", () => { + expect(expandUriTemplate("x{+v:9}", { v: "%61%62" })).toBe("x%61%62"); + }); +}); + describe("a value defined as the empty string", () => { // RFC 6570 distinguishes an *undefined* variable (the expression is omitted) // from one defined as "". The expander used to collapse the two, which made diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 2a98dc2f..6ca58821 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -4972,11 +4972,17 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; // Expand through the shared helper in ./uriTemplate.js so this and the web - // Resources form cannot disagree. It drops blank values so an unfilled - // optional field reads as *undefined* (the expression disappears) rather - // than as the empty string (a valueless `?topic=`), and it covers the - // expression shapes the SDK's own expander gets wrong -- `{a,b}`, `{;id}`, - // and the `{id:3}` prefix modifier (#1919). + // Resources form cannot disagree. It covers the expression shapes the SDK's + // own expander gets wrong -- `{a,b}`, `{;id}`, and the `{id:3}` prefix + // modifier (#1919). + // + // `params` is passed through AS GIVEN. Only an *absent* key omits its + // expression; a key present with `""` is a defined RFC 6570 value and + // expands (`{?q}` -> `?q=`, `{;q}` -> `;q`), which is what lets a caller + // request those URIs deliberately. A caller whose values come from a form + // -- where an untouched field is indistinguishable from a deliberately + // empty one -- drops its blanks with `definedValues` first, as the TUI's + // ResourceTestModal does. let expandedUri: string; try { expandedUri = expandUriTemplateStrict(uriTemplateString, params); diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index a9278e81..4fd7fcfd 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -434,25 +434,56 @@ function encodeUnreserved(value: string): string { ); } +/** + * The two operators whose expansion preserves RFC 3986 *reserved* characters + * and existing pct-triplets rather than encoding them (RFC 6570 §3.2.3, §3.2.4). + */ +function allowsReserved(operator: string): boolean { + return operator === "+" || operator === "#"; +} + /** Encodes one value for its operator: reserved characters survive `+` and `#`. */ function encodeValue(value: string, operator: string): string { - return operator === "+" || operator === "#" + return allowsReserved(operator) ? encodeAllowReserved(value) : encodeUnreserved(value); } +/** + * Splits a value into the units a prefix modifier counts. + * + * RFC 6570 §2.4.1 counts *characters*, and is explicit that "a pct-encoded + * triplet counts as a single character" — so `{+v:5}` over `%61%62%63%64%65%66` + * must stop after five whole triplets. Splitting on code points alone cut the + * sixth in half, and the trailing `%6` was then no longer a triplet, so + * `encodeAllowReserved` correctly encoded its `%`: the value went out as + * `%61%256`, a byte the server never asked for. + * + * Triplets are units only under `+` and `#`. Everywhere else the encoder + * escapes a `%` anyway (`%61` becomes `%2561`), so the three characters are + * three characters and grouping them would mis-count the prefix. + * + * The `u` flag makes the alternation match by code point, so an astral + * character is one unit rather than a surrogate pair that truncation could + * split — the reason `Array.from` was used here in the first place. + */ +function prefixUnits(value: string, operator: string): string[] { + if (!allowsReserved(operator)) return Array.from(value); + return value.match(/%[0-9A-Fa-f]{2}|[\s\S]/gu) ?? []; +} + /** * Applies a prefix modifier, then encodes. * - * Truncation is by *code point* (`Array.from`), not by `slice`: RFC 6570 counts - * the prefix in characters, and `String.prototype.slice` counts UTF-16 code - * units, so it can cut an astral character in half and yield a lone surrogate. + * Truncation counts the units {@link prefixUnits} defines rather than using + * `String.prototype.slice`, which counts UTF-16 code units and so can cut an + * astral character in half and yield a lone surrogate. */ function renderValue(value: string, spec: VarSpec, operator: string): string { const truncated = spec.maxLength === undefined ? value - : Array.from(value).slice(0, spec.maxLength).join(""); + : prefixUnits(value, operator).slice(0, spec.maxLength).join(""); return encodeValue(truncated, operator); }