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. *