From c6b17c52419198c691ef5928b67aaf9ad6f46712 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 00:47:19 -0400 Subject: [PATCH] fix: refuse a stray brace, bound value length, and drop a quadratic scan Addresses Copilot's round-14 review on #2035. Three suppressed comments, all real; each measured on 1c595e96 before acting. 1. A brace surviving in a literal is an unmatched one, and not a legal literal character. The SDK's constructor rejects an unclosed `{` but reads a stray `}` as text, and this parser did the same: expandUriTemplateStrict("x://a}", {}) => "x://a}" -- a "URI" carrying a brace, with the panel enabling the read on it. Both halves are now refused the same way, so the withhold behavior covers the whole class rather than the half the SDK happened to catch. 2. Replacing `UriTemplate.expand` also removed its 1,000,000-character per-value ceiling. Nothing else bounded what reached the allocation-heavy encoders, so a pasted or programmatically supplied megabyte-plus value went straight through. Restored at the SDK's own limit -- the guard it was, not a new policy -- and the error names the offending variable. 3. `templateVariables` re-scanned an expression's whole varspec list for every one of its names to compute `conforming`, which is quadratic in a multi-name expression. Templates are server-controlled and the SDK admits one up to 1 MB, so a single expression can carry thousands of short comma-separated names, and this runs on the render thread. Iterating the varspecs supplies the name and its conformance in one pass. npm run ci passes. Signed-off-by: cliffhall --- .../web/src/test/core/mcp/uriTemplate.test.ts | 54 +++++++++++++++++++ core/mcp/uriTemplate.ts | 44 +++++++++++++-- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index a560eed0..c5358b54 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -801,6 +801,60 @@ describe("a value defined as the empty string", () => { }); }); +describe("unmatched braces", () => { + // The SDK's constructor rejects an unclosed `{` but reads a stray `}` as + // literal text, and this parser does the same -- so `x://a}` used to expand + // to itself, a "URI" carrying a brace, with the panel enabling the read. + it.each([ + ["a stray closing brace", "x://a}"], + ["a closing brace after an expression", "x://{a}}"], + ["an unclosed opening brace", "x://a/{oops"], + ])("strict refuses %s", (_label, template) => { + // The stray `}` is ours to catch; the unclosed `{` the SDK's constructor + // already rejects ("Unclosed template expression"). Either way the read is + // withheld, which is the property under test. + expect(() => expandUriTemplateStrict(template, { a: "1" })).toThrow( + /Unmatched brace|Unclosed template expression/, + ); + }); + + it("leaves such a template unexpanded on the display path", () => { + expect(expandUriTemplate("x://a}", {})).toBe("x://a}"); + expect(expandUriTemplate("x://a/{oops", {})).toBe("x://a/{oops"); + }); + + it("still accepts a well-formed expression", () => { + expect(expandUriTemplate("x://{a}/b", { a: "1" })).toBe("x://1/b"); + }); +}); + +describe("the per-value length ceiling", () => { + // Replacing `UriTemplate.expand` dropped the SDK's own 1,000,000-character + // guard, leaving the allocation-heavy encoders unbounded for a pasted or + // programmatically supplied value. + it("refuses a value past the limit", () => { + const result = tryExpandUriTemplate("x://{v}", { + v: "a".repeat(1_000_001), + }); + expect(result.uri).toBeUndefined(); + expect(result.error).toMatch(/exceeds the 1000000-character limit/); + }); + + it("accepts a value at the limit", () => { + const value = "a".repeat(1_000_000); + expect(tryExpandUriTemplate("x://{v}", { v: value }).uri).toBe( + `x://${value}`, + ); + }); + + it("names the offending variable", () => { + expect( + tryExpandUriTemplate("x://{a}/{b}", { a: "1", b: "b".repeat(1_000_001) }) + .error, + ).toMatch(/"b"/); + }); +}); + describe("tryExpandUriTemplate", () => { it("returns the URI and no error when the template expands", () => { expect( diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 55f16543..485d5a79 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -270,10 +270,13 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { for (const part of parseUriTemplate(uriTemplate)) { if (part.kind !== "expression") continue; const required = !OMITTABLE_OPERATORS.has(part.operator); - for (const name of part.names) { - const conforming = !part.varspecs.some( - (spec) => spec.name === name && spec.conforming === false, - ); + // Iterating the varspecs (rather than the names, re-scanning the varspecs + // for each) keeps discovery linear. A template is server-controlled and the + // SDK admits one up to 1 MB, so a single expression can carry thousands of + // short comma-separated names -- and this runs on the render thread. + for (const spec of part.varspecs) { + const { name } = spec; + const conforming = spec.conforming !== false; const existing = byName.get(name); if (existing) { existing.required = existing.required || required; @@ -668,6 +671,33 @@ export function expandUriTemplateStrict( ): string { new UriTemplate(uriTemplate); const parts = parseUriTemplate(uriTemplate); + + // 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 + // `x://a}` expanded to itself -- a "URI" carrying a brace, with the panel + // happily enabling the read. Both halves are now refused the same way. + const stray = parts.find( + (part) => part.kind === "literal" && /[{}]/.test(part.text), + ); + if (stray) { + throw new Error( + `Unmatched brace in "${uriTemplate}" — an expression must be a complete {…}.`, + ); + } + + // 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 bad = parts.find((part) => part.kind === "expression" && part.invalid); if (bad) { throw new Error( @@ -679,6 +709,12 @@ export function expandUriTemplateStrict( return expandParts(parts, values); } +/** + * The pinned SDK's per-value ceiling, enforced here because this module no + * longer calls `UriTemplate.expand` — which is where that check lived. + */ +const MAX_VALUE_LENGTH = 1_000_000; + /** * The outcome of an expansion: the URI, or the reason there isn't one. *