diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index b0d38f03..687d7a3f 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -880,6 +880,27 @@ describe("the per-value length ceiling", () => { ); }); + it("ignores an oversized value the template never references", () => { + // RFC 6570 ignores an extra variable, and the SDK path this replaced + // validated a value only after looking one up by declared name -- so + // checking the whole map refused `x://{id}` over a stale key beside it. + expect( + tryExpandUriTemplate("x://{id}", { + id: "7", + stale: "a".repeat(1_000_001), + }).uri, + ).toBe("x://7"); + }); + + it("scopes valueLengthError to the names it is given", () => { + const values = { id: "7", stale: "a".repeat(1_000_001) }; + expect(valueLengthError(values, ["id"])).toBeNull(); + expect(valueLengthError(values, ["id", "stale"])).not.toBeNull(); + // Unscoped, every entry is examined -- the behavior the preview and the + // read both moved off. + expect(valueLengthError(values)).not.toBeNull(); + }); + it("names the offending variable", () => { expect( tryExpandUriTemplate("x://{a}/{b}", { a: "1", b: "b".repeat(1_000_001) }) diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index a3133e22..1b9eacc4 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -144,6 +144,15 @@ describe("previewUriTemplate - the value ceiling", () => { ); }); + it("ignores an oversized value the template never references", () => { + expect( + previewUriTemplate("x://{id}", { + id: "7", + stale: "a".repeat(1_000_001), + }), + ).toBe("x://7"); + }); + 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 06ea88a1..7d55b8c2 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -11,6 +11,7 @@ */ import { + declaredNames, definedValues, encodeLiteral, expandTemplateExpression, @@ -79,20 +80,25 @@ 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; - // 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; + const parts = parseUriTemplate(uriTemplate); + + // The same ceiling `tryExpandUriTemplate` enforces for the read, scoped the + // same way -- to the names this template references. 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, declaredNames(parts)) !== null) { + return uriTemplate; + } + try { - return parseUriTemplate(uriTemplate) + return parts .map((part) => { if (part.kind === "literal") return encodeLiteral(part.text); // `Object.hasOwn`, not a bare lookup: `toString` and `constructor` are diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 025c95a8..a536dde3 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -709,13 +709,21 @@ export function expandUriTemplateStrict( uriTemplate: string, values: Record, ): string { - const invalid = templateError(uriTemplate); + const sdk = sdkTemplateError(uriTemplate); + if (sdk) throw new Error(sdk); + + // Parsed ONCE and reused for validation, the name list and the expansion -- + // the template is server-controlled and may be up to 1 MB, so re-parsing it + // per step is real work on the render thread. + const parts = parseUriTemplate(uriTemplate); + + const invalid = partsError(uriTemplate, parts); if (invalid) throw new Error(invalid); - const tooLong = valueLengthError(values); + const tooLong = valueLengthError(values, declaredNames(parts)); if (tooLong) throw new Error(tooLong); - return expandParts(parseUriTemplate(uriTemplate), values); + return expandParts(parts, values); } /** @@ -733,14 +741,27 @@ export function expandUriTemplateStrict( * literal, and a varspec this module's own grammar rejects. */ export function templateError(uriTemplate: string): string | null { + return ( + sdkTemplateError(uriTemplate) ?? + partsError(uriTemplate, parseUriTemplate(uriTemplate)) + ); +} + +/** + * What the SDK's constructor rejects: an unclosed `{`, and its template-length + * (1e6), expression-count (1e4) and variable-name-length (1e6) limits. + */ +function sdkTemplateError(uriTemplate: string): string | null { try { new UriTemplate(uriTemplate); + return null; } catch (error) { return error instanceof Error ? error.message : String(error); } +} - const parts = parseUriTemplate(uriTemplate); - +/** What this module's own grammar rejects, given the already-parsed template. */ +function partsError(uriTemplate: string, parts: TemplatePart[]): string | null { // A brace left in a literal is an unmatched one, and it is not a legal // literal character. The SDK's constructor rejects an unclosed `{` but reads // a stray `}` as text, and this parser does the same, so without this check @@ -779,6 +800,13 @@ const MAX_VALUE_LENGTH = 1_000_000; /** * The first value exceeding {@link MAX_VALUE_LENGTH}, as a message, or `null`. * + * `names` limits the check to the variables a template actually references, + * and callers should pass it. RFC 6570 ignores an extra variable, and the SDK + * path this replaced validated a value only *after* looking one up by declared + * name — so checking the whole map made an unrelated entry fail a template + * that never mentions it: `x://{id}` with a valid `id` was refused because + * some stale megabyte-long key sat beside it in the caller's map. + * * 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 @@ -787,15 +815,28 @@ const MAX_VALUE_LENGTH = 1_000_000; */ export function valueLengthError( values: Record, + names?: Iterable, ): string | null { - for (const [name, value] of Object.entries(values)) { - if (value.length > MAX_VALUE_LENGTH) { + const considered = + names === undefined ? Object.keys(values) : [...new Set(names)]; + for (const name of considered) { + const value = readValue(values, name); + if (value !== undefined && value.length > MAX_VALUE_LENGTH) { return `The value for "${name}" exceeds the ${MAX_VALUE_LENGTH}-character limit.`; } } return null; } +/** The variable names a parsed template references, in order, deduplicated. */ +export function declaredNames(parts: TemplatePart[]): string[] { + return [ + ...new Set( + parts.flatMap((part) => (part.kind === "expression" ? part.names : [])), + ), + ]; +} + /** * The outcome of an expansion: the URI, or the reason there isn't one. *