diff --git a/AGENTS.md b/AGENTS.md index ccb2935c..4636e17e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,10 +74,12 @@ v2/main/ │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime + state stores │ │ # (uriTemplate.ts: RFC 6570 parse/classify/expand. -│ │ # The ONE expander for every client — the web -│ │ # Resources form and readResourceFromTemplate -│ │ # (TUI + CLI) — and every client derives its FORM -│ │ # FIELDS from it too (clients/tui uriTemplateToForm), +│ │ # The ONE expander for the web Resources form and +│ │ # readResourceFromTemplate (TUI); the CLI is NOT a +│ │ # consumer — it has no template form and passes an +│ │ # already-expanded --uri to readResource. Both +│ │ # consumers derive their FORM FIELDS from it too +│ │ # (clients/tui uriTemplateToForm), │ │ # which is what makes the sharing real: a form │ │ # submits under the names it rendered, so a mangled │ │ # name silently drops the value at expansion. The diff --git a/README.md b/README.md index d4b0e61e..c9f662e5 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,7 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — and all three derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. +The web client and the TUI expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI via `InspectorClient.readResourceFromTemplate` — and both derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. (The CLI is not a consumer: it has no template form, and its `resources/read` passes the already-expanded `--uri` straight through.) The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: diff --git a/clients/tui/__tests__/ResourceTestModal.test.tsx b/clients/tui/__tests__/ResourceTestModal.test.tsx index d9de004d..0067d211 100644 --- a/clients/tui/__tests__/ResourceTestModal.test.tsx +++ b/clients/tui/__tests__/ResourceTestModal.test.tsx @@ -77,6 +77,53 @@ const renderAndSubmit = async ( }; describe("ResourceTestModal", () => { + // RFC 6570 keeps a required expression satisfied by any ONE of its names, so + // `{a,b}` cannot be expressed with ink-form's per-field `required` flag and + // its members are left optional there. Without the modal's own group check + // the TUI would submit blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + describe("required-group validation", () => { + it("refuses to submit when no name in a required group has a value", async () => { + const read = vi.fn(); + const { onClose, unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "", b: "" }, + ); + expect(read).not.toHaveBeenCalled(); + expect(onClose).not.toHaveBeenCalled(); + unmount(); + }); + + it("submits once any one name in the group has a value", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://only-a", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://{a,b}" }), + { a: "only-a", b: "" }, + ); + expect(read).toHaveBeenCalledWith("x://{a,b}", { a: "only-a", b: "" }); + unmount(); + }); + + it("does not block a template whose only expression is omittable", async () => { + const read = vi.fn().mockResolvedValue({ + result: { contents: [] }, + expandedUri: "x://events", + }); + const { unmount } = await renderAndSubmit( + fakeClient(read), + makeTemplate({ uriTemplate: "x://events{?topic}" }), + { topic: "" }, + ); + expect(read).toHaveBeenCalled(); + unmount(); + }); + }); + it("renders the form initially without invoking the client", async () => { const read = vi.fn(); const api = render( diff --git a/clients/tui/src/components/ResourceTestModal.tsx b/clients/tui/src/components/ResourceTestModal.tsx index 914b4bca..37d662a2 100644 --- a/clients/tui/src/components/ResourceTestModal.tsx +++ b/clients/tui/src/components/ResourceTestModal.tsx @@ -5,6 +5,10 @@ import { InspectorClient } from "@inspector/core/mcp/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; import type { ReadResourceResult } from "@modelcontextprotocol/client"; import { uriTemplateToForm } from "../utils/uriTemplateToForm.js"; +import { + hasRequiredValues, + requiredGroups, +} from "@inspector/core/mcp/uriTemplate.js"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; // Helper to extract error message from various error types @@ -133,6 +137,29 @@ export function ResourceTestModal({ const handleFormSubmit = async (values: Record) => { if (!inspectorClient || !template) return; + // RFC 6570 keeps a required expression satisfied by any ONE of its names, + // so `{a,b}` cannot be expressed with ink-form's per-field `required` flag + // and its members are left optional there. Without this check the TUI would + // submit `{a,b}` completely blank -- dropping the expression and reading a + // different resource -- while the web panel blocks the same request (#1919). + const groups = requiredGroups(template.uriTemplate); + if (!hasRequiredValues(groups, values)) { + const unmet = groups + .filter( + (names) => !names.some((name) => (values[name] ?? "").length > 0), + ) + .map((names) => names.join(" or ")); + setResult({ + input: values, + output: null, + error: `Missing required template variable(s): ${unmet.join(", ")}`, + duration: 0, + uri: template.uriTemplate, + }); + setState("results"); + return; + } + setState("loading"); const startTime = Date.now(); diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index f0384f90..4b847131 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -18,6 +18,7 @@ describe("parseUriTemplate", () => { operator: "", varspecs: [{ name: "topic" }], names: ["topic"], + invalid: false, }, ]); }); @@ -31,6 +32,7 @@ describe("parseUriTemplate", () => { operator: "?", varspecs: [{ name: "a" }, { name: "b" }], names: ["a", "b"], + invalid: false, }, ]); }); @@ -137,8 +139,19 @@ describe("expandUriTemplate", () => { ); }); - it("joins two query expressions with & rather than a second ?", () => { + it("expands each query expression independently, per RFC 6570", () => { + // NOT `?one=1&two=2`. The SDK rewrites the second `?` to `&`, but measured + // against the pinned SDK its own matcher then rejects the result: + // match("x?one=1&two=2") on `x{?one}{?two}` is null, while + // match("x?one=1?two=2") returns both variables. A server wanting a + // continuation advertises `{?one}{&two}` -- see the test below. expect(expandUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" })).toBe( + "x://a?one=1?two=2", + ); + }); + + it("emits & only where the template asks for the continuation operator", () => { + expect(expandUriTemplate("x://a{?one}{&two}", { one: "1", two: "2" })).toBe( "x://a?one=1&two=2", ); }); @@ -229,11 +242,6 @@ describe("varspec modifiers", () => { ); }); - it("ignores a malformed modifier rather than inventing a truncation", () => { - expect(templateVariables("x://{id:}")[0].name).toBe("id"); - expect(expandUriTemplate("x://{id:}", { id: "abcdef" })).toBe("x://abcdef"); - }); - it("strips the explode modifier from the name", () => { expect(templateVariables("x://{id*}")[0].name).toBe("id"); }); @@ -330,60 +338,24 @@ describe("requiredGroups / hasRequiredValues", () => { }); }); -describe("cross-expression query joining", () => { - it("rewrites a second ? to & on the own-expansion path too", () => { - // Forced onto the own-expansion path by the `;` expression; the `?`-to-`&` - // rewrite must still apply, exactly as the SDK does it. +describe("expression independence", () => { + it("does not rewrite a later query expression on the own-expansion path", () => { expect( expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", one: "1", two: "2", }), - ).toBe("x://a;k=v?one=1&two=2"); + ).toBe("x://a;k=v?one=1?two=2"); }); - it("uses ? for the first query expression that actually emits", () => { + it("omits an expression with no value without affecting its neighbours", () => { expect( expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", two: "2" }), ).toBe("x://a;k=v?two=2"); }); }); -describe("strict vs lenient expansion", () => { - // `readResourceFromTemplate` wraps the thrown error with the template name; - // the web panel instead needs the raw template back, because an invalid - // template comes from the server and throwing would take out the panel. - it.each(["file:///{unclosed", "{a,b,c"])( - "strict throws on the invalid template %s", - (template) => { - expect(() => expandUriTemplateStrict(template, { x: "1" })).toThrow(); - }, - ); - - it.each(["file:///{unclosed", "{a,b,c"])( - "lenient returns %s unchanged", - (template) => { - expect(expandUriTemplate(template, { x: "1" })).toBe(template); - }, - ); - - it("validates syntax even when taking the own-expansion path", () => { - // `{;a}` forces own-expansion, and this module's parser treats the - // unclosed tail as literal text -- so without the unconditional SDK - // construction nothing would reject this. - expect(() => expandUriTemplateStrict("x://{;a}{b,c", { a: "1" })).toThrow(); - }); - - it("agrees with the lenient variant on a valid template", () => { - const template = "foobar://events{?topic}"; - const values = { topic: "foo/bar" }; - expect(expandUriTemplateStrict(template, values)).toBe( - expandUriTemplate(template, values), - ); - }); -}); - describe("allow-reserved encoding under + and #", () => { // The SDK uses `encodeURI` for these operators, which corrupts two classes // of value rather than merely over-escaping: measured, encodeURI("[::1]") @@ -465,3 +437,34 @@ describe("unreserved encoding under the non-reserved operators", () => { expect(expandUriTemplate("x://{v}", { v: "aZ0-._~" })).toBe("x://aZ0-._~"); }); }); + +describe("prefix-modifier grammar", () => { + // RFC 6570: max-length = %x31-39 0*3DIGIT -- 1..9999, no leading zero. + // The SDK's constructor accepts these shapes, so nothing else rejects them; + // treating `{id:abc}` as a plain `{id}` would send a URI the server never + // advertised, with nothing to alert anyone. + it.each(["x://{id:}", "x://{id:0}", "x://{id:abc}", "x://{id:10000}"])( + "strict rejects the invalid template %s", + (template) => { + expect(() => expandUriTemplateStrict(template, { id: "abcdef" })).toThrow( + /Invalid RFC 6570 varspec/, + ); + }, + ); + + it.each(["x://{id:}", "x://{id:abc}"])( + "lenient returns %s unchanged rather than guessing", + (template) => { + expect(expandUriTemplate(template, { id: "abcdef" })).toBe(template); + }, + ); + + it.each([ + ["x://{id:1}", "a"], + ["x://{id:9999}", "abcdef"], + ])("accepts the in-range modifier %s", (template, expected) => { + expect(expandUriTemplate(template, { id: "abcdef" })).toBe( + `x://${expected}`, + ); + }); +}); diff --git a/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts new file mode 100644 index 00000000..5765c369 --- /dev/null +++ b/clients/web/src/test/integration/mcp/rfc6570-templates.test.ts @@ -0,0 +1,139 @@ +import { describe, it, expect, afterEach } from "vitest"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { expandUriTemplate } from "@inspector/core/mcp/uriTemplate.js"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, + loadConfig, + resolveConfig, +} from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of `test-servers/configs/rfc6570-templates-http.json` — the + * documented manual reproduction for #1919. + * + * The unit tests assert what `expandUriTemplate` *produces*. They cannot assert + * that what it produces is what a spec-compliant server *accepts*, and that + * second half is the entire bug: the old string substitution emitted a URI the + * Inspector was perfectly happy with and the server rejected. So this drives + * both directions over a real transport — the encoded URI must resolve, and the + * unencoded one the old code produced must still be refused, so a regression + * cannot pass by quietly loosening the server. + * + * The server is built by **resolving the checked-in config** rather than by + * calling the fixture factories, which is the difference between covering the + * wiring and merely asserting the factory: a misspelt preset name in + * `preset-registry.ts`, or a config naming a preset that no longer exists, + * fails here instead of only when someone runs the repro by hand. + */ +describe("RFC 6570 resource templates over the wire (#1919)", () => { + let client: InspectorClient | null = null; + let server: TestServerHttp | null = null; + + const configPath = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs/rfc6570-templates-http.json", + ); + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // ignore + } + client = null; + } + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + }); + + /** + * Boot the showcase config. The harness picks the port rather than using the + * config's fixed one, so this cannot collide with a showcase server someone + * is running by hand. + */ + async function connectToShowcase(): Promise { + const resolved = resolveConfig(loadConfig(configPath)); + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("rfc6570-templates-test", "1.0.0"), + tools: resolved.tools, + resources: resolved.resources, + resourceTemplates: resolved.resourceTemplates, + }); + await started.start(); + server = started; + + const connected = new InspectorClient( + { type: "streamable-http", url: started.url }, + { environment: { transport: createTransportNode } }, + ); + await connected.connect(); + client = connected; + return connected; + } + + it("advertises both templates from the checked-in config", async () => { + const connected = await connectToShowcase(); + const { resourceTemplates } = await connected.listAllResourceTemplates(); + expect(resourceTemplates.map((entry) => entry.uriTemplate).sort()).toEqual([ + "foobar://events/{topic}", + "foobar://events{?topic}", + ]); + }); + + it("resolves the encoded URI the simple expression expands to", async () => { + const connected = await connectToShowcase(); + const uri = expandUriTemplate("foobar://events/{topic}", { + topic: "foo/bar", + }); + expect(uri).toBe("foobar://events/foo%2Fbar"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); + + it("rejects the unencoded URI the old string substitution produced", async () => { + const connected = await connectToShowcase(); + // This is what the pre-fix client sent, and why #1919 was filed: the raw + // `/` creates a second path segment that the template cannot match. + await expect( + connected.readResource("foobar://events/foo/bar"), + ).rejects.toThrow(/not found/i); + }); + + it("resolves the encoded URI the query expression expands to", async () => { + const connected = await connectToShowcase(); + const uri = expandUriTemplate("foobar://events{?topic}", { + topic: "foo/bar", + }); + expect(uri).toBe("foobar://events?topic=foo%2Fbar"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); + + it("resolves the base URI a blank query expression expands to", async () => { + const connected = await connectToShowcase(); + // RFC 6570 drops the whole expression when the variable is undefined, so + // an unfilled `topic` legitimately requests the unfiltered collection. The + // SDK's matcher compiles `{?topic}` to a *required* `\?topic=([^&]+)`, so a + // template alone cannot serve this — the config registers the plain + // resource, which is what a real server would do. + const uri = expandUriTemplate("foobar://events{?topic}", { topic: "" }); + expect(uri).toBe("foobar://events"); + + const { result } = await connected.readResource(uri); + expect(result.contents[0]?.uri).toBe(uri); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 8a50ff7e..feba12fe 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -35,10 +35,12 @@ describe("previewUriTemplate", () => { expect(previewUriTemplate("x://a{?one,two}", {})).toBe("x://a{?one,two}"); }); - it("still rewrites the second ? to & when both query expressions resolve", () => { + it("expands each query expression independently, matching the submit", () => { + // Not `?one=1&two=2` -- see the expander's tests: the SDK's own matcher + // rejects that for this template. The preview must show what will be sent. expect( previewUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" }), - ).toBe("x://a?one=1&two=2"); + ).toBe("x://a?one=1?two=2"); }); it("restores a deferred expression that follows a resolved query expression", () => { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 248abd92..b68a5c42 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -1,11 +1,16 @@ /** * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * - * This lives in `core/` rather than in a client so the web Resources form, the - * TUI, and the CLI cannot disagree about what a template means. Every one of - * them derives its form fields from {@link templateVariables} and expands - * through {@link expandUriTemplate} — the web panel directly, the TUI and CLI - * via `InspectorClient.readResourceFromTemplate`. + * This lives in `core/` rather than in a client so the web Resources form and + * the TUI cannot disagree about what a template means. Both derive their form + * fields from {@link templateVariables} and expand through + * {@link expandUriTemplate} — the web panel directly, the TUI via + * `InspectorClient.readResourceFromTemplate`. + * + * The **CLI is deliberately not a consumer**: it has no template form, and its + * `resources/read` passes the already-expanded `--uri` straight to + * `readResource` (see `clients/cli/src/handlers/run-method.ts`). Nothing here + * runs for it. * * ## Why this is not simply `new UriTemplate(t).expand(v)` * @@ -86,6 +91,12 @@ interface TemplateExpression { varspecs: VarSpec[]; /** Bare variable names, `*` and any `:length` modifier stripped. */ names: string[]; + /** + * True when a varspec carried a modifier that is not valid RFC 6570. Strict + * expansion rejects the template; discovery stays lenient so the panel can + * still render something rather than going blank. + */ + invalid: boolean; } export type TemplatePart = TemplateLiteral | TemplateExpression; @@ -108,8 +119,23 @@ export interface TemplateVariable { required: boolean; } -/** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ -function parseVarSpec(raw: string): VarSpec | null { +/** + * RFC 6570's `max-length` production: `%x31-39 0*3DIGIT` — 1 to 9999, no + * leading zero. `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are all invalid + * *templates*, not templates with an ignorable modifier. + */ +const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/; + +/** + * Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. + * + * Returns `null` for an empty varspec (a stray comma) and `"invalid"` for one + * whose modifier does not match the grammar. The two are distinguished because + * the first is ignorable and the second must fail the template: silently + * treating `{id:abc}` as `{id}` would send a URI that does not match what the + * server advertised, with nothing to alert anyone. + */ +function parseVarSpec(raw: string): VarSpec | null | "invalid" { // The explode modifier is stripped rather than honored: it only changes how // a list or map value is joined, and every value reaching this module is a // single string. @@ -120,13 +146,9 @@ function parseVarSpec(raw: string): VarSpec | null { if (colon === -1) return { name: spec }; const name = spec.slice(0, colon); - const length = Number(spec.slice(colon + 1)); - // A malformed modifier (`{id:}`, `{id:abc}`) is not a valid varspec; keep the - // name and ignore the modifier rather than inventing a truncation. - if (name.length === 0) return null; - return Number.isInteger(length) && length > 0 - ? { name, maxLength: length } - : { name }; + const modifier = spec.slice(colon + 1); + if (name.length === 0 || !MAX_LENGTH_GRAMMAR.test(modifier)) return "invalid"; + return { name, maxLength: Number(modifier) }; } /** @@ -159,17 +181,17 @@ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { } const body = uriTemplate.slice(i + 1, end); const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const varspecs = body - .slice(operator.length) - .split(",") - .map(parseVarSpec) - .filter((spec): spec is VarSpec => spec !== null); + const parsed = body.slice(operator.length).split(",").map(parseVarSpec); + const varspecs = parsed.filter( + (spec): spec is VarSpec => spec !== null && spec !== "invalid", + ); parts.push({ kind: "expression", source: uriTemplate.slice(i, end + 1), operator, varspecs, names: varspecs.map((spec) => spec.name), + invalid: parsed.includes("invalid"), }); i = end + 1; } @@ -375,30 +397,31 @@ function expandExpression( } /** - * Expands a whole parsed template, mirroring the SDK's `expand` — including its - * rule that a second query expression switches its leading `?` to `&`. + * Expands a whole parsed template. + * + * Each expression is expanded **independently**, which is what RFC 6570 + * specifies — expansion carries no cross-expression state. The SDK instead + * tracks whether a query expression has already emitted and rewrites a later + * `{?two}`'s leading `?` to `&`. That looks friendlier and is wrong: measured + * against the pinned SDK, `x{?one}{?two}` expands to `x?one=1&two=2`, and + * `UriTemplate.match` on that same template *rejects* it — `match("x?one=1&two=2")` + * is `null`, while `match("x?one=1?two=2")` returns both variables. So the + * rewrite emits a URI the advertised template cannot match, which is precisely + * the failure #1919 is about, one level up. + * + * A server that wants a continuation advertises it: `{?one}{&two}` expands to + * `x?one=1&two=2` and matches. Producing that shape is the server's choice to + * declare, not ours to infer. */ export function expandParts( parts: TemplatePart[], values: Record, ): string { - let result = ""; - let hasQueryParam = false; - - for (const part of parts) { - if (part.kind === "literal") { - result += part.text; - continue; - } - const expanded = expandExpression(part, values); - if (!expanded) continue; - - const isQuery = part.operator === "?" || part.operator === "&"; - result += isQuery && hasQueryParam ? `&${expanded.slice(1)}` : expanded; - if (isQuery) hasQueryParam = true; - } - - return result; + return parts + .map((part) => + part.kind === "literal" ? part.text : expandExpression(part, values), + ) + .join(""); } /** @@ -406,29 +429,33 @@ export function expandParts( * each value according to its operator, and omitting expressions whose * variables were left blank. **Throws** on a template that is not valid. * - * Templates the SDK handles correctly still go through the SDK, so the two - * cannot drift on the ordinary path; only a template containing at least one - * expression from the table above is expanded here instead. That is a - * whole-template switch rather than a per-expression splice so the `?`-to-`&` - * rewrite always sees every expression that actually produced output. + * Every expression is expanded by {@link expandParts}; the SDK's `UriTemplate` + * is constructed only because that is what rejects an unclosed expression, and + * callers such as `readResourceFromTemplate` wrap the error it throws. Its own + * `expand` is deliberately unused — see the module comment for the five shapes + * it gets wrong. * - * The SDK template is constructed *before* choosing a path, and unconditionally: - * that construction is what validates the syntax and throws `Unclosed template - * expression`, and callers such as `readResourceFromTemplate` wrap that error. - * Skipping it on the own-expansion path would silently accept a template like - * `{;a}{b,c` — this module's parser treats the unclosed tail as literal text, - * so nothing else would object. + * Its constructor is not a complete validator either: it accepts `{id:abc}`, + * whose modifier is not RFC 6570's `max-length` production. Treating that as a + * plain `{id}` would send a URI the server never advertised, so the owned + * parser's verdict is checked too. */ export function expandUriTemplateStrict( uriTemplate: string, values: Record, ): string { const defined = definedValues(values); - // Constructed purely to validate: this is what throws `Unclosed template - // expression`, and callers such as `readResourceFromTemplate` wrap that - // error. Its `expand` is deliberately not used — see the module comment. new UriTemplate(uriTemplate); - return expandParts(parseUriTemplate(uriTemplate), defined); + const parts = parseUriTemplate(uriTemplate); + const bad = parts.find((part) => part.kind === "expression" && part.invalid); + if (bad) { + throw new Error( + `Invalid RFC 6570 varspec in "${uriTemplate}": ${ + bad.kind === "expression" ? bad.source : "" + } — a prefix modifier must be 1-9999 with no leading zero.`, + ); + } + return expandParts(parts, defined); } /**