From 7d9410cbe004163d466d00bc123e702375ccf13c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 01:13:17 -0400 Subject: [PATCH] fix: keep a pct-encoded UTF-8 sequence whole, and bound the preview too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Copilot's round-16 review on #2035. Two suppressed comments, both real, and both refinements of fixes from the previous two rounds rather than defects in the original code. 1. Round 11 settled on "a pct-encoded triplet counts as one character", but a multi-octet character spans several triplets -- `%C3%A9` is one `é`. Measured on a3dd8563: x{+v:1} v = "%C3%A9x" => "x%C3" a lone lead byte, decoding to nothing `prefixUnits` now recognizes a well-formed pct-encoded UTF-8 sequence from its lead byte (RFC 3629: C2-DF two octets, E0-EF three, F0-F4 four; continuations 80-BF), longest alternative first so a four-octet sequence is not read 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 back to per-triplet counting. That is the conservative direction: it counts as MORE units, so truncation keeps less rather than emitting octets that were never in the value. 2. Round 14's per-value ceiling only covered the read. The panel calls `previewUriTemplate` on every keystroke, and that path expands expression by expression, so it reached the allocation-heavy encoders with the same oversized value -- submission refused while the UI froze on the paste anyway. Both paths now share one exported `valueLengthError`, checked before any encoding happens. npm run ci passes. Signed-off-by: cliffhall --- .../web/src/test/core/mcp/uriTemplate.test.ts | 33 +++++++++++ clients/web/src/utils/uriTemplate.test.ts | 16 +++++ clients/web/src/utils/uriTemplate.ts | 8 +++ core/mcp/uriTemplate.ts | 58 ++++++++++++++----- 4 files changed, 102 insertions(+), 13 deletions(-) diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index c5358b54..875508a3 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -9,6 +9,7 @@ import { templateVariables, tryExpandUriTemplate, unmetRequiredGroups, + valueLengthError, } from "@inspector/core/mcp/uriTemplate.js"; describe("parseUriTemplate", () => { @@ -749,6 +750,26 @@ describe("prefix modifier truncation", () => { }, ); + it.each([ + // A pct-encoded UTF-8 sequence is ONE character: `%C3%A9` is `é`. Counting + // per triplet returned `%C3`, a lone lead byte decoding to nothing. + ["x{+v:1}", "%C3%A9x", "x%C3%A9"], + ["x{+v:2}", "%C3%A9x", "x%C3%A9x"], + // Three-octet (`€`) and four-octet (an emoji) sequences, same rule. + ["x{+v:1}", "%E2%82%ACy", "x%E2%82%AC"], + ["x{+v:1}", "%F0%9F%98%80z", "x%F0%9F%98%80"], + // A malformed sequence -- a lead byte with too few continuations -- falls + // back to per-triplet counting rather than swallowing what follows. + ["x{+v:1}", "%C3x", "x%C3"], + // A stray continuation byte is not a sequence start. + ["x{+v:1}", "%A9%C3%A9", "x%A9"], + ])( + "keeps a multi-octet sequence whole (%s over %s)", + (template, value, expected) => { + expect(expandUriTemplate(template, { v: value })).toBe(expected); + }, + ); + it("counts a triplet as one character under a named operator too", () => { expect(expandUriTemplate("x{?v:1}", { v: "%61%62" })).toBe("x?v=%2561"); }); @@ -855,6 +876,18 @@ describe("the per-value length ceiling", () => { }); }); +describe("valueLengthError", () => { + it("names the first oversized value", () => { + expect(valueLengthError({ a: "1", b: "b".repeat(1_000_001) })).toMatch( + /"b" exceeds the 1000000-character limit/, + ); + }); + + it("passes values at or under the limit", () => { + expect(valueLengthError({ a: "a".repeat(1_000_000) })).toBeNull(); + }); +}); + describe("tryExpandUriTemplate", () => { it("returns the URI and no error when the template expands", () => { expect( diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 72e5f824..cdbac17c 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -126,3 +126,19 @@ describe("previewUriTemplate - it must never throw or over-promise", () => { ); }); }); + +describe("previewUriTemplate - the value ceiling", () => { + it("falls back before encoding an oversized value", () => { + // Without this the guard was half a guard: the read was refused while the + // preview still encoded the same value on every keystroke, so a large + // paste froze the UI anyway. + expect(previewUriTemplate("x://{v}", { v: "a".repeat(1_000_001) })).toBe( + "x://{v}", + ); + }); + + it("still previews a value at the limit", () => { + const value = "a".repeat(1_000_000); + expect(previewUriTemplate("x://{v}", { v: value })).toBe(`x://${value}`); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 81ecd3b3..46c8077a 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -15,6 +15,7 @@ import { encodeLiteral, expandTemplateExpression, parseUriTemplate, + valueLengthError, } from "@inspector/core/mcp/uriTemplate.js"; export { @@ -27,6 +28,7 @@ export { templateVariables, tryExpandUriTemplate, unmetRequiredGroups, + valueLengthError, } from "@inspector/core/mcp/uriTemplate.js"; export type { TemplateExpansion, @@ -71,6 +73,12 @@ export function previewUriTemplate( ): string { const defined = definedValues(values); + // The same ceiling `tryExpandUriTemplate` enforces for the read. Without it + // the guard was half a guard: submission was refused while this path still + // handed the oversized value to the encoders on every keystroke, so a large + // paste froze the UI anyway. Checked before any encoding happens. + if (valueLengthError(defined) !== null) return uriTemplate; + try { return parseUriTemplate(uriTemplate) .map((part) => { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 5ffc786d..a0a53cfb 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -512,6 +512,27 @@ function encodeValue(value: string, operator: string): string { : encodeUnreserved(value); } +/** + * One unit a prefix modifier counts: a code point, a pct-encoded octet, or a + * whole pct-encoded UTF-8 *sequence*. + * + * The sequence alternatives are what stop a multi-octet character being cut in + * half. `%C3%A9` is one character — `é` — so `{+v:1}` over `%C3%A9x` must keep + * both octets; counting per triplet returned `%C3`, a lone lead byte that + * decodes to nothing. The lead byte announces the length (RFC 3629: `C2-DF` + * two octets, `E0-EF` three, `F0-F4` four) and each continuation is `80-BF`, + * 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. + */ +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; + /** * Splits a value into the units a prefix modifier counts. * @@ -532,12 +553,12 @@ function encodeValue(value: string, operator: string): string { * still escapes the `%`, so the retained `%61` goes out as `%2561`), not what * counts as one. * - * The `u` flag makes the alternation match by code point, so an astral + * The `u` flag makes the final alternative match by code point, so an astral * character is one unit rather than a surrogate pair that truncation could * split. */ function prefixUnits(value: string): string[] { - return value.match(/%[0-9A-Fa-f]{2}|[\s\S]/gu) ?? []; + return value.match(PREFIX_UNIT) ?? []; } /** @@ -687,17 +708,8 @@ export function expandUriTemplateStrict( ); } - // Replacing `UriTemplate.expand` also dropped its per-value ceiling, and - // nothing else bounded what reaches the allocation-heavy encoders below: a - // pasted or programmatically supplied megabyte-plus value hit them unguarded. - // Kept at the SDK's own limit so this is the guard it was, not a new policy. - for (const [name, value] of Object.entries(values)) { - if (value.length > MAX_VALUE_LENGTH) { - throw new Error( - `The value for "${name}" exceeds the ${MAX_VALUE_LENGTH}-character limit.`, - ); - } - } + const tooLong = valueLengthError(values); + if (tooLong) throw new Error(tooLong); const bad = parts.find((part) => part.kind === "expression" && part.invalid); if (bad) { @@ -723,6 +735,26 @@ export function expandUriTemplateStrict( */ const MAX_VALUE_LENGTH = 1_000_000; +/** + * The first value exceeding {@link MAX_VALUE_LENGTH}, as a message, or `null`. + * + * Exported because the *preview* needs the same verdict and cannot get it from + * {@link expandUriTemplateStrict}: it expands expression by expression, so it + * would otherwise hand an oversized value straight to the encoders during + * render — the guard would hold for the read while the UI still froze on the + * paste that triggered it. + */ +export function valueLengthError( + values: Record, +): string | null { + for (const [name, value] of Object.entries(values)) { + if (value.length > MAX_VALUE_LENGTH) { + return `The value for "${name}" exceeds the ${MAX_VALUE_LENGTH}-character limit.`; + } + } + return null; +} + /** * The outcome of an expansion: the URI, or the reason there isn't one. *