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

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>

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(

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", () => {
+  it("shows a template the read would refuse exactly as the server wrote it", () => {
-    expect(previewUriTemplate("x://café/{oops", {})).toBe(
+    // Not even the literal is encoded: an unreadable template is displayed
-      "x://caf%C3%A9/{oops",
+    // 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", () => {
+  it.each([
-    expect(previewUriTemplate("x://{a,}/{b}", { a: "1", b: "2" })).toBe(
+    ["an invalid varspec", "x://{a,}/{b}"],
-      "x://{a,}/2",
+    ["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", () => {

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
+ * - **A template the read would refuse is shown verbatim.** `{a,}` still
- *   its varspecs, so filling `a` would preview `x://1` for a template whose
+ *   parses `a` into its varspecs, and `x://{a}}/{b}` parses two perfectly good
- *   submission is refused outright -- a URI the user could never send.
+ *   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.

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
+ * two-octet one followed by loose triplets.
- * with too few continuations, or a stray continuation — matches no sequence
+ *
- * alternative and falls through to the single-triplet rule, which is the
+ * The lead-byte ranges are RFC 3629's **well-formed** ones, not merely the
- * conservative behavior: it counts as more units, so truncation keeps less
+ * length-announcing prefixes: `C2-DF` (so the overlong `%C0%80` is excluded),
- * rather than emitting something that was never in the value.
+ * `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 =
+const CONTINUATION = String.raw`%[89AB][0-9A-F]`;
-  /%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;
+/** 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(
+    return `Unmatched brace in "${uriTemplate}" — an expression must be a complete {…}.`;
-      `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(
+    return `Invalid RFC 6570 varspec in "${uriTemplate}": ${
-      `Invalid RFC 6570 varspec in "${uriTemplate}": ${
+      bad.kind === "expression" ? bad.source : ""
-        bad.kind === "expression" ? bad.source : ""
+    } — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`;
-      } — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`,
-    );
   }
-  return expandParts(parts, values);
+
+  return null;
 }
 
 /**