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 <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
1939136eff
commit
1ad3edd96d
7 changed files with 210 additions and 34 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
<ResourceTemplatePanel
|
||||
template={{ name: "Broken", uriTemplate }}
|
||||
onReadResource={onReadResource}
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<ResourceTemplatePanel
|
||||
template={{ name: "Broken", uriTemplate: "x://items/{}" }}
|
||||
onReadResource={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("x://items/{}")).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
describe("completions", () => {
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
);
|
||||
})}
|
||||
</Stack>
|
||||
{expansion.error !== undefined && (
|
||||
<ExpansionErrorText>{expansion.error}</ExpansionErrorText>
|
||||
)}
|
||||
<FooterRow>
|
||||
<Button size="sm" disabled={!canSubmit} onClick={handleSubmit}>
|
||||
Read Resource
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import {
|
|||
parseUriTemplate,
|
||||
requiredGroups,
|
||||
templateVariables,
|
||||
tryExpandUriTemplate,
|
||||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
|
||||
describe("parseUriTemplate", () => {
|
||||
|
|
@ -506,3 +507,65 @@ describe("variable names that collide with Object.prototype", () => {
|
|||
expect(expandUriTemplate("x://a{?__proto__}", {})).toBe("x://a");
|
||||
});
|
||||
});
|
||||
|
||||
describe("an expression that declares no variable", () => {
|
||||
// RFC 6570 requires at least one varspec per expression and admits no empty
|
||||
// member, so each of these is a malformed template rather than one with a
|
||||
// member to skip. Skipping is the dangerous reading: `x://{}` would expand to
|
||||
// `x://` while the form rendered no inputs, so its "everything required is
|
||||
// filled" check is vacuously true and it submits a URI that is not the
|
||||
// template the server published.
|
||||
it.each([
|
||||
["an empty expression", "x://{}"],
|
||||
["only a separator", "x://{,}"],
|
||||
["a missing member", "x://{a,}"],
|
||||
["an operator and no name", "x://a{?}"],
|
||||
// `*` is an explode modifier, not a name -- it is stripped before the
|
||||
// emptiness test, so this must be rejected the same way.
|
||||
["only an explode modifier", "x://{*}"],
|
||||
])("strict rejects %s", (_label, template) => {
|
||||
expect(() => expandUriTemplateStrict(template, { a: "1" })).toThrow(
|
||||
/Invalid RFC 6570 varspec/,
|
||||
);
|
||||
});
|
||||
|
||||
it("declares no required group for it, so only the expansion gate reports it", () => {
|
||||
// An empty group could never be satisfied, and the form's "any one of"
|
||||
// message built from it would name no fields -- the accurate reason is the
|
||||
// malformed template, which tryExpandUriTemplate gives.
|
||||
expect(requiredGroups("x://{}")).toEqual([]);
|
||||
expect(tryExpandUriTemplate("x://{}", {}).error).toMatch(
|
||||
/Invalid RFC 6570 varspec/,
|
||||
);
|
||||
});
|
||||
|
||||
it("still reports the names a partly-empty expression does declare", () => {
|
||||
expect(templateVariables("x://{a,}").map((v) => v.name)).toEqual(["a"]);
|
||||
});
|
||||
|
||||
it("leaves the template unexpanded on the display path", () => {
|
||||
expect(expandUriTemplate("x://{}", {})).toBe("x://{}");
|
||||
});
|
||||
});
|
||||
|
||||
describe("tryExpandUriTemplate", () => {
|
||||
it("returns the URI and no error when the template expands", () => {
|
||||
expect(
|
||||
tryExpandUriTemplate("foobar://events/{topic}", { topic: "foo/bar" }),
|
||||
).toEqual({ uri: "foobar://events/foo%2Fbar" });
|
||||
});
|
||||
|
||||
it("returns the reason instead of a URI for a malformed template", () => {
|
||||
const result = tryExpandUriTemplate("x://{id:abc}", { id: "7" });
|
||||
expect(result.uri).toBeUndefined();
|
||||
expect(result.error).toMatch(/Invalid RFC 6570 varspec/);
|
||||
});
|
||||
|
||||
it("reports a value that cannot be encoded rather than throwing", () => {
|
||||
// A lone surrogate has no UTF-8 encoding, so encodeURIComponent throws
|
||||
// URIError on it -- and a text input can hold one via paste.
|
||||
const result = tryExpandUriTemplate("x://{v}", { v: "\ud800" });
|
||||
expect(result.uri).toBeUndefined();
|
||||
expect(result.error).toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -22,8 +22,10 @@ export {
|
|||
parseUriTemplate,
|
||||
requiredGroups,
|
||||
templateVariables,
|
||||
tryExpandUriTemplate,
|
||||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
export type {
|
||||
TemplateExpansion,
|
||||
TemplatePart,
|
||||
TemplateVariable,
|
||||
VarSpec,
|
||||
|
|
|
|||
|
|
@ -92,9 +92,11 @@ interface TemplateExpression {
|
|||
/** 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.
|
||||
* True when a varspec is not one RFC 6570 admits — an out-of-grammar modifier
|
||||
* (`{id:abc}`) or an empty member (`{}`, `{a,}`); see {@link parseVarSpec}.
|
||||
* Strict expansion rejects such a template and the form withholds the read,
|
||||
* while *discovery* stays lenient so the panel still renders whatever names
|
||||
* the template does declare rather than going blank.
|
||||
*/
|
||||
invalid: boolean;
|
||||
}
|
||||
|
|
@ -127,20 +129,28 @@ export interface TemplateVariable {
|
|||
const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/;
|
||||
|
||||
/**
|
||||
* Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix.
|
||||
* Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix,
|
||||
* or `"invalid"` for anything RFC 6570's `varspec` production does not admit.
|
||||
*
|
||||
* 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.
|
||||
* Two shapes reach that verdict, and neither is ignorable:
|
||||
*
|
||||
* - **A modifier outside the grammar** (`{id:abc}`). Treating it as a plain
|
||||
* `{id}` would send a URI that does not match what the server advertised,
|
||||
* with nothing to alert anyone.
|
||||
* - **An empty varspec** — `{}`, `{,}`, `{a,}`, `{?}`, `{*}`. RFC 6570 requires
|
||||
* at least one varspec per expression and admits no empty member, so this is
|
||||
* a malformed *template*, not an expression with a member to skip. Skipping
|
||||
* it is the more dangerous reading: `x://{}` would expand to `x://` while the
|
||||
* form rendered no inputs, so its "everything required is filled" check would
|
||||
* be vacuously true and it would submit a URI that is not the template the
|
||||
* server published.
|
||||
*/
|
||||
function parseVarSpec(raw: string): VarSpec | null | "invalid" {
|
||||
function parseVarSpec(raw: string): VarSpec | "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.
|
||||
const spec = raw.replace("*", "").trim();
|
||||
if (spec.length === 0) return null;
|
||||
if (spec.length === 0) return "invalid";
|
||||
|
||||
const colon = spec.indexOf(":");
|
||||
if (colon === -1) return { name: spec };
|
||||
|
|
@ -183,7 +193,7 @@ export function parseUriTemplate(uriTemplate: string): TemplatePart[] {
|
|||
const operator = OPERATORS.find((op) => body.startsWith(op)) ?? "";
|
||||
const parsed = body.slice(operator.length).split(",").map(parseVarSpec);
|
||||
const varspecs = parsed.filter(
|
||||
(spec): spec is VarSpec => spec !== null && spec !== "invalid",
|
||||
(spec): spec is VarSpec => spec !== "invalid",
|
||||
);
|
||||
parts.push({
|
||||
kind: "expression",
|
||||
|
|
@ -243,6 +253,12 @@ export function requiredGroups(uriTemplate: string): string[][] {
|
|||
for (const part of parseUriTemplate(uriTemplate)) {
|
||||
if (part.kind !== "expression") continue;
|
||||
if (OMITTABLE_OPERATORS.has(part.operator)) continue;
|
||||
// An expression naming no variable at all (`x://{}`) is a malformed
|
||||
// template, which {@link tryExpandUriTemplate} refuses with that as the
|
||||
// reason. Emitting an empty group here too would gate the form a second
|
||||
// time on a condition nothing can satisfy, and the "any one of" message
|
||||
// built from it would name no fields.
|
||||
if (part.names.length === 0) continue;
|
||||
groups.push(part.names);
|
||||
}
|
||||
return groups;
|
||||
|
|
@ -472,31 +488,63 @@ export function expandUriTemplateStrict(
|
|||
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.`,
|
||||
} — each varspec must name a variable, optionally with an explode (*) or a 1-9999 prefix modifier.`,
|
||||
);
|
||||
}
|
||||
return expandParts(parts, defined);
|
||||
}
|
||||
|
||||
/**
|
||||
* The outcome of an expansion: the URI, or the reason there isn't one.
|
||||
*
|
||||
* This is what a form gates its submit on. The lenient
|
||||
* {@link expandUriTemplate} below is for *display* — it answers with the raw
|
||||
* template so a panel can keep rendering — and a caller that mistook that
|
||||
* fallback for a URI would issue a `resources/read` for the template itself,
|
||||
* literal braces and all. Making the failure a value rather than a string the
|
||||
* caller has to recognize is what keeps that from being possible.
|
||||
*
|
||||
* Failure is not only the malformed-template case: a value carrying an
|
||||
* unpaired surrogate has no UTF-8 encoding, so `encodeURIComponent` throws
|
||||
* `URIError` on it, and a text input can hold one via paste.
|
||||
*/
|
||||
export type TemplateExpansion =
|
||||
| { uri: string; error?: undefined }
|
||||
| { uri?: undefined; error: string };
|
||||
|
||||
/**
|
||||
* {@link expandUriTemplateStrict} with the throw turned into a value, for
|
||||
* callers that must *decide* rather than degrade — chiefly the web panel, which
|
||||
* disables Read Resource and shows the reason instead of sending a URI it knows
|
||||
* is wrong.
|
||||
*/
|
||||
export function tryExpandUriTemplate(
|
||||
uriTemplate: string,
|
||||
values: Record<string, string>,
|
||||
): TemplateExpansion {
|
||||
try {
|
||||
return { uri: expandUriTemplateStrict(uriTemplate, values) };
|
||||
} catch (error) {
|
||||
return { error: error instanceof Error ? error.message : String(error) };
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* {@link expandUriTemplateStrict}, but falling back to the raw template string
|
||||
* instead of throwing.
|
||||
*
|
||||
* This is the form's variant: the template comes from the connected server, not
|
||||
* from the user, so an invalid one is not something the user can fix from the
|
||||
* panel — and what they already see in the URI preview is the raw template.
|
||||
* Letting it throw would take out the whole panel on render; returning it
|
||||
* unchanged sends the server a URI it rejects with a legible error instead.
|
||||
* Call sites that need the failure (`readResourceFromTemplate`, which wraps it
|
||||
* with the template name) use the strict variant.
|
||||
* This is the **display** variant, and only that: it runs during render (the
|
||||
* URI preview), where a throw would take out the whole panel, and the raw
|
||||
* template is the honest thing to show for a template nothing can expand.
|
||||
*
|
||||
* Do **not** submit what it returns. A caller deciding whether to issue a read
|
||||
* uses {@link tryExpandUriTemplate}, whose failure is a value it cannot
|
||||
* mistake for a URI; `readResourceFromTemplate` uses the strict variant and
|
||||
* wraps the error with the template's name.
|
||||
*/
|
||||
export function expandUriTemplate(
|
||||
uriTemplate: string,
|
||||
values: Record<string, string>,
|
||||
): string {
|
||||
try {
|
||||
return expandUriTemplateStrict(uriTemplate, values);
|
||||
} catch {
|
||||
return uriTemplate;
|
||||
}
|
||||
return tryExpandUriTemplate(uriTemplate, values).uri ?? uriTemplate;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue