Skip to content

Commit 7d9410c

Browse files
committed
fix: keep a pct-encoded UTF-8 sequence whole, and bound the preview too
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 a3dd856: 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 <cliff@futurescale.com>
1 parent a3dd856 commit 7d9410c

4 files changed

Lines changed: 102 additions & 13 deletions

File tree

‎clients/web/src/test/core/mcp/uriTemplate.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
templateVariables,
1010
tryExpandUriTemplate,
1111
unmetRequiredGroups,
12+
valueLengthError,
1213
} from "@inspector/core/mcp/uriTemplate.js";
1314

1415
describe("parseUriTemplate", () => {
@@ -749,6 +750,26 @@ describe("prefix modifier truncation", () => {
749750
},
750751
);
751752

753+
it.each([
754+
// A pct-encoded UTF-8 sequence is ONE character: `%C3%A9` is `é`. Counting
755+
// per triplet returned `%C3`, a lone lead byte decoding to nothing.
756+
["x{+v:1}", "%C3%A9x", "x%C3%A9"],
757+
["x{+v:2}", "%C3%A9x", "x%C3%A9x"],
758+
// Three-octet (`€`) and four-octet (an emoji) sequences, same rule.
759+
["x{+v:1}", "%E2%82%ACy", "x%E2%82%AC"],
760+
["x{+v:1}", "%F0%9F%98%80z", "x%F0%9F%98%80"],
761+
// A malformed sequence -- a lead byte with too few continuations -- falls
762+
// back to per-triplet counting rather than swallowing what follows.
763+
["x{+v:1}", "%C3x", "x%C3"],
764+
// A stray continuation byte is not a sequence start.
765+
["x{+v:1}", "%A9%C3%A9", "x%A9"],
766+
])(
767+
"keeps a multi-octet sequence whole (%s over %s)",
768+
(template, value, expected) => {
769+
expect(expandUriTemplate(template, { v: value })).toBe(expected);
770+
},
771+
);
772+
752773
it("counts a triplet as one character under a named operator too", () => {
753774
expect(expandUriTemplate("x{?v:1}", { v: "%61%62" })).toBe("x?v=%2561");
754775
});
@@ -855,6 +876,18 @@ describe("the per-value length ceiling", () => {
855876
});
856877
});
857878

879+
describe("valueLengthError", () => {
880+
it("names the first oversized value", () => {
881+
expect(valueLengthError({ a: "1", b: "b".repeat(1_000_001) })).toMatch(
882+
/"b" exceeds the 1000000-character limit/,
883+
);
884+
});
885+
886+
it("passes values at or under the limit", () => {
887+
expect(valueLengthError({ a: "a".repeat(1_000_000) })).toBeNull();
888+
});
889+
});
890+
858891
describe("tryExpandUriTemplate", () => {
859892
it("returns the URI and no error when the template expands", () => {
860893
expect(

‎clients/web/src/utils/uriTemplate.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,3 +126,19 @@ describe("previewUriTemplate - it must never throw or over-promise", () => {
126126
);
127127
});
128128
});
129+
130+
describe("previewUriTemplate - the value ceiling", () => {
131+
it("falls back before encoding an oversized value", () => {
132+
// Without this the guard was half a guard: the read was refused while the
133+
// preview still encoded the same value on every keystroke, so a large
134+
// paste froze the UI anyway.
135+
expect(previewUriTemplate("x://{v}", { v: "a".repeat(1_000_001) })).toBe(
136+
"x://{v}",
137+
);
138+
});
139+
140+
it("still previews a value at the limit", () => {
141+
const value = "a".repeat(1_000_000);
142+
expect(previewUriTemplate("x://{v}", { v: value })).toBe(`x://${value}`);
143+
});
144+
});

‎clients/web/src/utils/uriTemplate.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
encodeLiteral,
1616
expandTemplateExpression,
1717
parseUriTemplate,
18+
valueLengthError,
1819
} from "@inspector/core/mcp/uriTemplate.js";
1920

2021
export {
@@ -27,6 +28,7 @@ export {
2728
templateVariables,
2829
tryExpandUriTemplate,
2930
unmetRequiredGroups,
31+
valueLengthError,
3032
} from "@inspector/core/mcp/uriTemplate.js";
3133
export type {
3234
TemplateExpansion,
@@ -71,6 +73,12 @@ export function previewUriTemplate(
7173
): string {
7274
const defined = definedValues(values);
7375

76+
// The same ceiling `tryExpandUriTemplate` enforces for the read. Without it
77+
// the guard was half a guard: submission was refused while this path still
78+
// handed the oversized value to the encoders on every keystroke, so a large
79+
// paste froze the UI anyway. Checked before any encoding happens.
80+
if (valueLengthError(defined) !== null) return uriTemplate;
81+
7482
try {
7583
return parseUriTemplate(uriTemplate)
7684
.map((part) => {

‎core/mcp/uriTemplate.ts‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,27 @@ function encodeValue(value: string, operator: string): string {
512512
: encodeUnreserved(value);
513513
}
514514

515+
/**
516+
* One unit a prefix modifier counts: a code point, a pct-encoded octet, or a
517+
* whole pct-encoded UTF-8 *sequence*.
518+
*
519+
* The sequence alternatives are what stop a multi-octet character being cut in
520+
* half. `%C3%A9` is one character — `é` — so `{+v:1}` over `%C3%A9x` must keep
521+
* both octets; counting per triplet returned `%C3`, a lone lead byte that
522+
* decodes to nothing. The lead byte announces the length (RFC 3629: `C2-DF`
523+
* two octets, `E0-EF` three, `F0-F4` four) and each continuation is `80-BF`,
524+
* so a well-formed sequence is recognizable without decoding.
525+
*
526+
* Order matters: longest first, so a four-octet sequence is not matched as a
527+
* two-octet one followed by loose triplets. A malformed sequence — a lead byte
528+
* with too few continuations, or a stray continuation — matches no sequence
529+
* alternative and falls through to the single-triplet rule, which is the
530+
* conservative behavior: it counts as more units, so truncation keeps less
531+
* rather than emitting something that was never in the value.
532+
*/
533+
const PREFIX_UNIT =
534+
/%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;
535+
515536
/**
516537
* Splits a value into the units a prefix modifier counts.
517538
*
@@ -532,12 +553,12 @@ function encodeValue(value: string, operator: string): string {
532553
* still escapes the `%`, so the retained `%61` goes out as `%2561`), not what
533554
* counts as one.
534555
*
535-
* The `u` flag makes the alternation match by code point, so an astral
556+
* The `u` flag makes the final alternative match by code point, so an astral
536557
* character is one unit rather than a surrogate pair that truncation could
537558
* split.
538559
*/
539560
function prefixUnits(value: string): string[] {
540-
return value.match(/%[0-9A-Fa-f]{2}|[\s\S]/gu) ?? [];
561+
return value.match(PREFIX_UNIT) ?? [];
541562
}
542563

543564
/**
@@ -687,17 +708,8 @@ export function expandUriTemplateStrict(
687708
);
688709
}
689710

690-
// Replacing `UriTemplate.expand` also dropped its per-value ceiling, and
691-
// nothing else bounded what reaches the allocation-heavy encoders below: a
692-
// pasted or programmatically supplied megabyte-plus value hit them unguarded.
693-
// Kept at the SDK's own limit so this is the guard it was, not a new policy.
694-
for (const [name, value] of Object.entries(values)) {
695-
if (value.length > MAX_VALUE_LENGTH) {
696-
throw new Error(
697-
`The value for "${name}" exceeds the ${MAX_VALUE_LENGTH}-character limit.`,
698-
);
699-
}
700-
}
711+
const tooLong = valueLengthError(values);
712+
if (tooLong) throw new Error(tooLong);
701713

702714
const bad = parts.find((part) => part.kind === "expression" && part.invalid);
703715
if (bad) {
@@ -723,6 +735,26 @@ export function expandUriTemplateStrict(
723735
*/
724736
const MAX_VALUE_LENGTH = 1_000_000;
725737

738+
/**
739+
* The first value exceeding {@link MAX_VALUE_LENGTH}, as a message, or `null`.
740+
*
741+
* Exported because the *preview* needs the same verdict and cannot get it from
742+
* {@link expandUriTemplateStrict}: it expands expression by expression, so it
743+
* would otherwise hand an oversized value straight to the encoders during
744+
* render — the guard would hold for the read while the UI still froze on the
745+
* paste that triggered it.
746+
*/
747+
export function valueLengthError(
748+
values: Record<string, string>,
749+
): string | null {
750+
for (const [name, value] of Object.entries(values)) {
751+
if (value.length > MAX_VALUE_LENGTH) {
752+
return `The value for "${name}" exceeds the ${MAX_VALUE_LENGTH}-character limit.`;
753+
}
754+
}
755+
return null;
756+
}
757+
726758
/**
727759
* The outcome of an expansion: the URI, or the reason there isn't one.
728760
*

0 commit comments

Comments
 (0)