modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 4

fix: accept only well-formed UTF-8 sequences, and validate before previewing

Browse source 
Addresses Copilot's round-18 review on #2035. Two suppressed comments, both
holes in my own recent fixes rather than in the original code.

1. Round 16's grouping keyed off the length-announcing lead byte alone, so
   it accepted sequences RFC 3629 forbids -- and the comment claiming they
   fell back to per-triplet counting was simply wrong. Measured on 05bbfe50:

     x{+v:1}  "%C0%80x"        =>  "x%C0%80"        overlong
              "%E0%80%80x"     =>  "x%E0%80%80"     overlong
              "%ED%A0%80x"     =>  "x%ED%A0%80"     UTF-16 surrogate
              "%F4%90%80%80x"  =>  "x%F4%90%80%80"  past U+10FFFF

   The lead-byte ranges are now the well-formed ones -- C2-DF, E0 only with
   A0-BF, ED only with 80-9F, F0 only with 90-BF, F4 only with 80-8F -- so
   each of the above falls back to one triplet, which is what the fallback
   was always documented to do. The legal boundary cases (%E0%A0%80,
   %ED%9F%BF, %F4%8F%BF%BF) are tested alongside, so tightening the ranges
   cannot quietly start splitting valid characters.

2. The preview never ran the read's template validation. It expands
   expression by expression, so `x://{a}}/{b}` with both fields filled
   previewed `x://1}/2` -- a URI whose stray `}` makes every read refuse.

   Validation moved into an exported `templateError` that both callers
   consult, so they cannot drift. It subsumes the narrower rule round 12
   added here ("leave the invalid part standing"), which still let the REST
   of such a template expand: the refusal is per template, not per
   expression, so the preview now shows the template verbatim -- literals
   unencoded included, since half-normalizing an unreadable template invents
   a string the server never published. Two existing tests asserted the
   weaker behavior and now assert this one.

npm run ci passes.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-17 01:40:50 -04:00
commit 9caac43da4
4 changed files with 130 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

38
clients/web/src/test/core/mcp/uriTemplate.test.ts
View file

@ -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(

25
clients/web/src/utils/uriTemplate.test.ts
View file

@ -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", () => {

19
clients/web/src/utils/uriTemplate.ts
View file

@ -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.

81
core/mcp/uriTemplate.ts
View file

@ -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, string>,
): 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;
}
/**