From 1ad3edd96d4fab3ff1f37d8102db21556f5a5609 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:32:27 -0400 Subject: [PATCH] fix: refuse a template that cannot expand, instead of reading the raw one Ports the two behaviors PR #2033 (the parallel attempt at #1919, now closed as a duplicate) got right and this branch did not. 1. An expression declaring no variable is a malformed template, not one with a member to skip. RFC 6570 requires at least one varspec per expression and admits no empty member, so `{}`, `{,}`, `{a,}`, `{?}` and `{*}` (an explode modifier is not a name) now make the template invalid. Skipping them was the more dangerous reading: `x://{}` expanded to `x://` while the form rendered no inputs, so its "everything required is filled" check was vacuously true and it submitted a URI that is not the template the server published. 2. The read is withheld when expansion fails. `expandUriTemplate`'s raw-template fallback exists for the preview, which runs during render -- but the submit path used it too, so an invalid template was read with its braces intact and the server answered with a confusing "not found" for a defect that is not the user's. `tryExpandUriTemplate` returns the URI or the reason as a value the caller cannot mistake for one; the panel gates Read Resource on it and prints the reason. This also covers a value that cannot be encoded -- an unpaired surrogate has no UTF-8 encoding, so `encodeURIComponent` throws `URIError` on it, and a text input can hold one via paste. `requiredGroups` now skips an expression naming no variable: an empty group can never be satisfied, so it would gate the form a second time on a condition nothing can meet, and the "any one of" hint built from it would name no fields. The accurate reason is the malformed template, which the expansion gate reports. npm run ci passes. Signed-off-by: cliffhall --- AGENTS.md | 7 +- README.md | 2 + .../ResourceTemplatePanel.test.tsx | 32 ++++++ .../ResourceTemplatePanel.tsx | 40 ++++++-- .../web/src/test/core/mcp/uriTemplate.test.ts | 63 ++++++++++++ clients/web/src/utils/uriTemplate.ts | 2 + core/mcp/uriTemplate.ts | 98 ++++++++++++++----- 7 files changed, 210 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4636e17e..aa442e66 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,12 @@ v2/main/ │ │ # and double-encodes pct-triplets), and `{v}` │ │ # (encodeURIComponent leaves `!'()*` bare). │ │ # Requiredness is per EXPRESSION, not per variable — -│ │ # requiredGroups + hasRequiredValues — #1919; +│ │ # requiredGroups + hasRequiredValues. A template that +│ │ # cannot expand ({id:abc}, {}, {a,}) WITHHOLDS the read: +│ │ # tryExpandUriTemplate returns the reason as a value, and +│ │ # the panel disables Read Resource rather than sending the +│ │ # raw template — expandUriTemplate's raw-template fallback +│ │ # is for DISPLAY (the preview) only — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index c7fa48e0..ec2af96c 100644 --- a/README.md +++ b/README.md @@ -263,6 +263,8 @@ The SDK's `UriTemplate` is still used, but only to *validate* a template (constr The `;` and `:3` rows are the ones a user sees directly: on the SDK's parse the form renders fields literally labelled `;id` and `id:3`. The `+`/`#` row is silent corruption rather than over-escaping — an IPv6 literal or an already-encoded path arrives at the server altered. +A template that cannot be expanded at all — an out-of-grammar modifier (`{id:abc}`), or an expression declaring no variable (`{}`, `{a,}`, `{?}`) — **withholds the read** rather than sending something. The panel disables Read Resource, prints the reason, and leaves the raw template in the preview. The alternative is worse than it looks: `x://{}` would otherwise expand to `x://` with no inputs rendered, so the form's "everything required is filled" check passes vacuously and it reads a URI that is not the template the server published. + Requiredness is a property of the **expression**, not the variable: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled is expandable and a form must not block it. `requiredGroups` returns one entry per non-omittable expression and `hasRequiredValues` asks that each be satisfied by any one of its names — which no per-variable flag can express once a name recurs across expressions (`{a,b}{a,c}` is satisfied by filling `b` and `c`). #### Advertised extensions diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx index 4955b385..87b2ba15 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.test.tsx @@ -243,6 +243,38 @@ describe("ResourceTemplatePanel", () => { screen.getByText("foobar://events?topic=news"), ).toBeInTheDocument(); }); + + // A template the server advertised can be malformed. There is no URI to + // send for one, so the read is withheld and the reason shown -- reading the + // raw template with its braces intact would draw a confusing "not found" + // from the server for a defect that is not the user's. + it.each([ + ["an out-of-grammar prefix modifier", "x://items/{id:abc}"], + ["an expression declaring no variable", "x://items/{}"], + ])("refuses to read a template with %s", (_label, uriTemplate) => { + const onReadResource = vi.fn(); + renderWithMantine( + , + ); + expect( + screen.getByRole("button", { name: "Read Resource" }), + ).toBeDisabled(); + expect(screen.getByText(/Invalid RFC 6570 varspec/)).toBeInTheDocument(); + expect(onReadResource).not.toHaveBeenCalled(); + }); + + it("shows the malformed template unexpanded in the preview", () => { + renderWithMantine( + , + ); + expect(screen.getByText("x://items/{}")).toBeInTheDocument(); + }); }); describe("completions", () => { diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index 9c104821..cb6a1589 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -15,11 +15,11 @@ import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprot import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge"; import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { - expandUriTemplate, hasRequiredValues, previewUriTemplate, requiredGroups, templateVariables, + tryExpandUriTemplate, } from "../../../utils/uriTemplate"; export interface ResourceTemplatePanelProps { @@ -69,6 +69,13 @@ const DescriptionText = Text.withProps({ c: "dimmed", }); +// Why Read Resource is disabled when the template itself cannot be expanded. +// Without it the button is inert with nothing on screen explaining the refusal. +const ExpansionErrorText = Text.withProps({ + size: "sm", + c: "red", +}); + // Left-aligned so the action sits closest to the sidebar controls / the form // fields above; annotation badges trail it. const FooterRow = Group.withProps({ @@ -221,15 +228,29 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - // Only the expressions whose absence would change the URI's shape gate the - // read; an unfilled `{?topic}` is a legitimate request for the unfiltered - // resource, and RFC 6570 drops the whole expression for it. The rule is - // per-expression rather than per-variable -- `{a,b}` with only `a` filled - // expands to `a`'s value -- so it lives in core beside the expander. - const canSubmit = hasRequiredValues(groups, variables); + // Two independent gates on the read. + // + // First, whether the values cover what expansion structurally needs. Only the + // expressions whose absence would change the URI's shape count; an unfilled + // `{?topic}` is a legitimate request for the unfiltered resource, and RFC 6570 + // drops the whole expression for it. The rule is per-expression rather than + // per-variable -- `{a,b}` with only `a` filled expands to `a`'s value -- so it + // lives in core beside the expander. + // + // Second, whether the template expands at all. A template the server + // advertised can be malformed (`{id:abc}`, `{a,}`) and a pasted value can be + // unencodable, and in neither case is there a URI to send -- so withhold the + // request and say why, rather than reading the raw template with its braces + // intact and letting the server answer with a confusing "not found". + const expansion = tryExpandUriTemplate(uriTemplate, variables); + const canSubmit = + expansion.error === undefined && hasRequiredValues(groups, variables); function handleSubmit() { - onReadResource(expandUriTemplate(uriTemplate, variables)); + /* v8 ignore next -- unreachable: the button is disabled whenever the + expansion failed, which is the only way `uri` is undefined. */ + if (expansion.uri === undefined) return; + onReadResource(expansion.uri); } const preview = previewUriTemplate(uriTemplate, variables); @@ -308,6 +329,9 @@ export function ResourceTemplatePanel({ ); })} + {expansion.error !== undefined && ( + {expansion.error} + )}