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

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
1 changed files with 130 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

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;
}
/**