diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 875508a3..b0d38f03 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -7,6 +7,7 @@ import { parseUriTemplate, requiredGroups, templateVariables, + templateError, tryExpandUriTemplate, unmetRequiredGroups, valueLengthError, @@ -763,6 +764,17 @@ describe("prefix modifier truncation", () => { ["x{+v:1}", "%C3x", "x%C3"], // A stray continuation byte is not a sequence start. ["x{+v:1}", "%A9%C3%A9", "x%A9"], + // RFC 3629 well-formedness, not just the length-announcing lead byte: an + // overlong encoding, a UTF-16 surrogate, and a code point past U+10FFFF + // are each refused the grouping and counted per triplet. + ["x{+v:1}", "%C0%80x", "x%C0"], + ["x{+v:1}", "%E0%80%80x", "x%E0"], + ["x{+v:1}", "%ED%A0%80x", "x%ED"], + ["x{+v:1}", "%F4%90%80%80x", "x%F4"], + // The boundary cases that ARE well-formed stay whole. + ["x{+v:1}", "%E0%A0%80x", "x%E0%A0%80"], + ["x{+v:1}", "%ED%9F%BFx", "x%ED%9F%BF"], + ["x{+v:1}", "%F4%8F%BF%BFx", "x%F4%8F%BF%BF"], ])( "keeps a multi-octet sequence whole (%s over %s)", (template, value, expected) => { @@ -876,6 +888,32 @@ describe("the per-value length ceiling", () => { }); }); +describe("templateError", () => { + it.each([ + ["a stray closing brace", "x://{a}}"], + ["an unclosed brace", "x://{oops"], + ["an invalid varspec", "x://{a,}"], + ["an out-of-grammar modifier", "x://{id:abc}"], + ])("reports %s", (_label, template) => { + expect(templateError(template)).not.toBeNull(); + }); + + it.each(["x://{a}", "x://a{?b,c}", "x://a{;d:3}", "café/{var}"])( + "passes the valid template %s", + (template) => { + expect(templateError(template)).toBeNull(); + }, + ); + + it("gives strict expansion its verdict, so the two cannot disagree", () => { + // Every template the error reports must also refuse to expand. + for (const template of ["x://{a}}", "x://{oops", "x://{a,}"]) { + expect(templateError(template)).not.toBeNull(); + expect(() => expandUriTemplateStrict(template, { a: "1" })).toThrow(); + } + }); +}); + describe("valueLengthError", () => { it("names the first oversized value", () => { expect(valueLengthError({ a: "1", b: "b".repeat(1_000_001) })).toMatch( diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index cdbac17c..a3133e22 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -93,10 +93,10 @@ describe("previewUriTemplate - literals", () => { expect(previewUriTemplate("café/{var}", {})).toBe("caf%C3%A9/{var}"); }); - it("keeps a malformed template legible rather than encoding its brace", () => { - expect(previewUriTemplate("x://café/{oops", {})).toBe( - "x://caf%C3%A9/{oops", - ); + it("shows a template the read would refuse exactly as the server wrote it", () => { + // Not even the literal is encoded: an unreadable template is displayed + // verbatim rather than half-normalized into something never published. + expect(previewUriTemplate("x://café/{oops", {})).toBe("x://café/{oops"); }); }); @@ -120,11 +120,18 @@ describe("previewUriTemplate - it must never throw or over-promise", () => { ); }); - it("still previews the valid expressions around an invalid one", () => { - expect(previewUriTemplate("x://{a,}/{b}", { a: "1", b: "2" })).toBe( - "x://{a,}/2", - ); - }); + it.each([ + ["an invalid varspec", "x://{a,}/{b}"], + ["a stray closing brace", "x://{a}}/{b}"], + ])( + "does not expand the valid expressions around %s either", + (_label, template) => { + // The refusal is per TEMPLATE, not per expression: every read of these + // is refused, so previewing `x://{a,}/2` or `x://1}/2` would promise a + // URI this form can never send. An earlier revision did exactly that. + expect(previewUriTemplate(template, { a: "1", b: "2" })).toBe(template); + }, + ); }); describe("previewUriTemplate - the value ceiling", () => { diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 46c8077a..06ea88a1 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -15,6 +15,7 @@ import { encodeLiteral, expandTemplateExpression, parseUriTemplate, + templateError, valueLengthError, } from "@inspector/core/mcp/uriTemplate.js"; @@ -25,6 +26,7 @@ export { hasRequiredValues, parseUriTemplate, requiredGroups, + templateError, templateVariables, tryExpandUriTemplate, unmetRequiredGroups, @@ -58,9 +60,13 @@ export type { * expand-the-whole-template implementation got for free from the lenient * `expandUriTemplate` it called: * - * - **An invalid expression is left standing.** `{a,}` still parses `a` into - * its varspecs, so filling `a` would preview `x://1` for a template whose - * submission is refused outright -- a URI the user could never send. + * - **A template the read would refuse is shown verbatim.** `{a,}` still + * parses `a` into its varspecs, and `x://{a}}/{b}` parses two perfectly good + * expressions around a stray `}`, so expanding either previewed a URI whose + * submission is refused outright. `templateError` is the read's own verdict, + * so consulting it first is what keeps the two from disagreeing -- and it + * subsumes the narrower "skip the invalid part" rule this had at first, + * which left the *rest* of such a template expanding. * - **The whole thing is wrapped.** This runs during render, where a throw * unmounts the panel instead of disabling its button, and encoding can throw * for real input: `encodeURIComponent` raises `URIError` on an unpaired @@ -79,11 +85,16 @@ export function previewUriTemplate( // paste froze the UI anyway. Checked before any encoding happens. if (valueLengthError(defined) !== null) return uriTemplate; + // The same verdict the read is gated on. Without it this expanded expression + // by expression regardless: `x://{a}}/{b}` with both fields filled previewed + // `x://1}/2`, a URI whose stray `}` makes every read refuse -- the preview + // promising something the form it sits in can never send. + if (templateError(uriTemplate) !== null) return uriTemplate; + try { return parseUriTemplate(uriTemplate) .map((part) => { if (part.kind === "literal") return encodeLiteral(part.text); - if (part.invalid) return part.source; // `Object.hasOwn`, not a bare lookup: `toString` and `constructor` are // valid RFC 6570 variable names, and a plain lookup would find // `Object.prototype`'s member and treat a blank field as filled. diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index a0a53cfb..025c95a8 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -524,14 +524,32 @@ function encodeValue(value: string, operator: string): string { * so a well-formed sequence is recognizable without decoding. * * Order matters: longest first, so a four-octet sequence is not matched as a - * two-octet one followed by loose triplets. A malformed sequence — a lead byte - * with too few continuations, or a stray continuation — matches no sequence - * alternative and falls through to the single-triplet rule, which is the - * conservative behavior: it counts as more units, so truncation keeps less - * rather than emitting something that was never in the value. + * two-octet one followed by loose triplets. + * + * The lead-byte ranges are RFC 3629's **well-formed** ones, not merely the + * length-announcing prefixes: `C2-DF` (so the overlong `%C0%80` is excluded), + * `E0` only with `A0-BF`, `ED` only with `80-9F` (excluding UTF-16 + * surrogates), `F0` only with `90-BF`, `F4` only with `80-8F` (nothing past + * U+10FFFF). An earlier revision accepted the whole `C0-DF`/`E0-EF`/`F0-F4` + * space and so grouped those invalid sequences as one character — the + * opposite of the fallback this comment promised. + * + * A malformed sequence — an excluded lead byte, too few continuations, or a + * stray continuation — now matches no sequence alternative and falls through + * to the single-triplet rule, which is the conservative behavior: it counts as + * more units, so truncation keeps less rather than emitting something that was + * never in the value. */ -const PREFIX_UNIT = - /%F[0-4](?:%[89AB][0-9A-F]){3}|%E[0-9A-F](?:%[89AB][0-9A-F]){2}|%[CD][0-9A-F]%[89AB][0-9A-F]|%[0-9A-F]{2}|[\s\S]/giu; +const CONTINUATION = String.raw`%[89AB][0-9A-F]`; +/** RFC 3629 well-formed sequences only — `C2-DF`, and the restricted second + * bytes that exclude overlongs (`E0%A0-BF`, `F0%90-BF`), UTF-16 surrogates + * (`ED%80-9F`) and anything past U+10FFFF (`F4%80-8F`). */ +const UTF8_SEQUENCE = [ + String.raw`(?:%F0%(?:9[0-9A-F]|[AB][0-9A-F])|%F[1-3]${CONTINUATION}|%F4%8[0-9A-F])${CONTINUATION}${CONTINUATION}`, + String.raw`(?:%E0%[AB][0-9A-F]|%E[1-9ABCEF]${CONTINUATION}|%ED%[89][0-9A-F])${CONTINUATION}`, + String.raw`%(?:C[2-9A-F]|D[0-9A-F])${CONTINUATION}`, +].join("|"); +const PREFIX_UNIT = new RegExp(`${UTF8_SEQUENCE}|%[0-9A-F]{2}|[\\s\\S]`, "giu"); /** * Splits a value into the units a prefix modifier counts. @@ -691,7 +709,36 @@ export function expandUriTemplateStrict( uriTemplate: string, values: Record, ): string { - new UriTemplate(uriTemplate); + const invalid = templateError(uriTemplate); + if (invalid) throw new Error(invalid); + + const tooLong = valueLengthError(values); + if (tooLong) throw new Error(tooLong); + + return expandParts(parseUriTemplate(uriTemplate), values); +} + +/** + * Why `uriTemplate` cannot be expanded **at all**, independent of any values, + * or `null` if it can. + * + * Exported because the preview needs the same verdict before it expands + * anything: it goes expression by expression, so without this it happily + * rendered `x://1}/2` for `x://{a}}/{b}` — a template whose stray `}` makes + * every read refuse, i.e. a URI the form it sits in can never send. + * + * Three sources, deliberately in one place so the two callers cannot drift: + * the SDK's constructor (an unclosed `{`, and its template-length, + * expression-count and variable-name-length limits), a brace surviving in a + * literal, and a varspec this module's own grammar rejects. + */ +export function templateError(uriTemplate: string): string | null { + try { + new UriTemplate(uriTemplate); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + const parts = parseUriTemplate(uriTemplate); // A brace left in a literal is an unmatched one, and it is not a legal @@ -703,23 +750,17 @@ export function expandUriTemplateStrict( (part) => part.kind === "literal" && /[{}]/.test(part.text), ); if (stray) { - throw new Error( - `Unmatched brace in "${uriTemplate}" — an expression must be a complete {…}.`, - ); + return `Unmatched brace in "${uriTemplate}" — an expression must be a complete {…}.`; } - const tooLong = valueLengthError(values); - if (tooLong) throw new Error(tooLong); - const bad = parts.find((part) => part.kind === "expression" && part.invalid); if (bad) { - throw new Error( - `Invalid RFC 6570 varspec in "${uriTemplate}": ${ - bad.kind === "expression" ? bad.source : "" - } — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`, - ); + return `Invalid RFC 6570 varspec in "${uriTemplate}": ${ + bad.kind === "expression" ? bad.source : "" + } — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`; } - return expandParts(parts, values); + + return null; } /**