fix: validate the whole varspec, and derive the TUI's message from its gate
Addresses Copilot's round-7 review on #2035. Two suppressed comments, both
real; each reproduced against this branch before acting.
1. `parseVarSpec` was a "strip the first `*`, trim, split on `:`" pass, so
it accepted shapes RFC 6570's varspec production does not admit --
measured on the previous commit:
x://{*id} name "id" => "x://abcdef" explode is a TRAILING modifier
x://{id*:3} name "id" => "x://abc" the two modifiers are exclusive
x://{ id } name "id" => "x://abcdef" whitespace is not a varchar
x://{a b} name "a b" => "x://1" ditto -- and it emits into the URI
`{id*:3}` is the worst of them: it silently truncated the value through a
combination the grammar forbids. Parsing is now one anchored regex --
a varname, then at most one trailing `*` or `:1-9999`.
The name set is `varchar` widened by `-` and `~`, deliberately. A name is
the one thing expansion emits into the URI WITHOUT encoding it (`;`, `?`
and `&` write `name=value`), so the rule it has to satisfy is "needs no
encoding" -- RFC 3986 unreserved plus pct-encoded, of which the RFC's own
varchar is a subset. Strict varchar would refuse `{user-id}`, which real
servers publish and which the SDK's matcher round-trips; refusing to read
a resource that demonstrably works is a worse outcome for a debugging
tool than a name one character outside the production. Everything now
rejected is a shape we would otherwise have to guess at.
2. The TUI built its "Missing required template variable(s): ..." list with
its own bare `values[name]` filter while gating on `hasRequiredValues`,
which reads own properties only. For a variable legitimately named
`constructor` the two disagreed: `({})["constructor"]` is `Object`, whose
`.length` is 1, so the list came out empty and the error named no field
at all while the submit stayed blocked. Both now come from one exported
`unmetRequiredGroups`, with `hasRequiredValues` defined in terms of it.
Also renames a test whose title still said multi-name query expressions were
left "to the SDK, which handles them" -- expansion has been owned here since
935113d1; the behavior it asserts is naming, not delegation.
npm run ci passes.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
1ad3edd96d
commit
1c24eda429
7 changed files with 228 additions and 36 deletions
|
|
@ -263,7 +263,7 @@ 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.
|
||||
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. Pick **events_malformed** (`foobar://events/{topic:abc}`) to see it: Read Resource is disabled, the reason is printed under the form, and the preview shows the template as the server declared it. 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`).
|
||||
|
||||
|
|
|
|||
|
|
@ -95,6 +95,23 @@ describe("ResourceTestModal", () => {
|
|||
unmount();
|
||||
});
|
||||
|
||||
it("blocks a required variable whose name collides with Object.prototype", async () => {
|
||||
// `constructor` is a valid RFC 6570 varname, and the modal's own filter
|
||||
// used to read `Object` (length 1) as a filled value. The *message* built
|
||||
// from that filter came out naming no field; since frames render empty
|
||||
// here (see the header note), that half is asserted on
|
||||
// `unmetRequiredGroups` in the core suite. This asserts the gate.
|
||||
const read = vi.fn();
|
||||
const { onClose, unmount } = await renderAndSubmit(
|
||||
fakeClient(read),
|
||||
makeTemplate({ uriTemplate: "x://{constructor}" }),
|
||||
{ constructor: "" },
|
||||
);
|
||||
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: [] },
|
||||
|
|
|
|||
|
|
@ -6,8 +6,8 @@ import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
|
|||
import type { ReadResourceResult } from "@modelcontextprotocol/client";
|
||||
import { uriTemplateToForm } from "../utils/uriTemplateToForm.js";
|
||||
import {
|
||||
hasRequiredValues,
|
||||
requiredGroups,
|
||||
unmetRequiredGroups,
|
||||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
import { ScrollView, type ScrollViewRef } from "ink-scroll-view";
|
||||
|
||||
|
|
@ -142,13 +142,16 @@ export function ResourceTestModal({
|
|||
// 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 "));
|
||||
// The message and the gate come from the SAME pass. Filtering the groups
|
||||
// again here with a bare `values[name]` disagreed with the gate for a
|
||||
// variable named `constructor` or `toString`: the inherited member read as
|
||||
// filled, so the list came out empty and the error named no field at all.
|
||||
const unmetGroups = unmetRequiredGroups(
|
||||
requiredGroups(template.uriTemplate),
|
||||
values,
|
||||
);
|
||||
if (unmetGroups.length > 0) {
|
||||
const unmet = unmetGroups.map((names) => names.join(" or "));
|
||||
setResult({
|
||||
input: values,
|
||||
output: null,
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import {
|
|||
requiredGroups,
|
||||
templateVariables,
|
||||
tryExpandUriTemplate,
|
||||
unmetRequiredGroups,
|
||||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
|
||||
describe("parseUriTemplate", () => {
|
||||
|
|
@ -205,7 +206,10 @@ describe("expandUriTemplate - multi-name expressions (SDK correction)", () => {
|
|||
expect(expandUriTemplate("x://a{/p,q}", {})).toBe("x://a");
|
||||
});
|
||||
|
||||
it("leaves multi-name query expressions to the SDK, which handles them", () => {
|
||||
// Naming, not delegation: a multi-name QUERY expression pairs each name with
|
||||
// its own value and joins with `&` (RFC 6570 §3.2.8), unlike the bare
|
||||
// comma-joined list every non-named operator above produces.
|
||||
it("pairs each name with its value in a multi-name query expression", () => {
|
||||
expect(expandUriTemplate("x://a{?p,q}", { p: "x/y", q: "z" })).toBe(
|
||||
"x://a?p=x%2Fy&q=z",
|
||||
);
|
||||
|
|
@ -548,6 +552,82 @@ describe("an expression that declares no variable", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("varspec grammar", () => {
|
||||
// Each of these expanded rather than being refused before the anchored
|
||||
// grammar landed -- measured on the previous commit: `{*id}` and `{ id }`
|
||||
// both yielded "x://abcdef", and `{id*:3}` silently truncated to "x://abc"
|
||||
// through a modifier combination RFC 6570 does not allow.
|
||||
it.each([
|
||||
["explode in the leading position", "x://{*id}"],
|
||||
["explode combined with a prefix", "x://{id*:3}"],
|
||||
["whitespace around the name", "x://{ id }"],
|
||||
["whitespace inside the name", "x://{a b}"],
|
||||
["a name outside the character set", "x://{a/b}"],
|
||||
])("strict rejects %s", (_label, template) => {
|
||||
expect(() =>
|
||||
expandUriTemplateStrict(template, {
|
||||
id: "abcdef",
|
||||
a: "1",
|
||||
"a/b": "1",
|
||||
"a b": "1",
|
||||
}),
|
||||
).toThrow(/Invalid RFC 6570 varspec/);
|
||||
});
|
||||
|
||||
it.each([
|
||||
// RFC 6570 varchar, plus the two RFC 3986 unreserved characters the set is
|
||||
// deliberately widened by -- a name is emitted verbatim by `?`/`&`/`;`, so
|
||||
// the rule is "needs no encoding", and `{user-id}` is common in the wild.
|
||||
["a plain name", "x://{id}", { id: "7" }, "x://7"],
|
||||
["a hyphen", "x://{user-id}", { "user-id": "7" }, "x://7"],
|
||||
["a tilde", "x://{a~b}", { "a~b": "7" }, "x://7"],
|
||||
["an underscore", "x://{a_b}", { a_b: "7" }, "x://7"],
|
||||
["a dot separator", "x://{a.b}", { "a.b": "7" }, "x://7"],
|
||||
["a pct-encoded varchar", "x://{a%2Fb}", { "a%2Fb": "7" }, "x://7"],
|
||||
["a trailing explode", "x://{id*}", { id: "7" }, "x://7"],
|
||||
["a trailing prefix", "x://{id:3}", { id: "abcdef" }, "x://abc"],
|
||||
])("accepts %s", (_label, template, values, expected) => {
|
||||
expect(expandUriTemplate(template, values)).toBe(expected);
|
||||
});
|
||||
|
||||
it.each(["x://{a.}", "x://{a..b}"])(
|
||||
"rejects the misplaced dot in %s",
|
||||
(template) => {
|
||||
// varname = varchar *( ["."] varchar ) -- a dot separates, it cannot
|
||||
// trail or double.
|
||||
expect(() => expandUriTemplateStrict(template, {})).toThrow(
|
||||
/Invalid RFC 6570 varspec/,
|
||||
);
|
||||
},
|
||||
);
|
||||
|
||||
it("reads a leading dot as the label operator, not a malformed name", () => {
|
||||
// `{.a}` is label expansion of `a` -- the one "misplaced dot" that is not
|
||||
// one, since the operator is stripped before the varspec is parsed.
|
||||
expect(expandUriTemplate("x://a{.ext}", { ext: "json" })).toBe(
|
||||
"x://a.json",
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("unmetRequiredGroups", () => {
|
||||
it("names the groups that are missing", () => {
|
||||
expect(unmetRequiredGroups([["a", "b"], ["c"]], { c: "1" })).toEqual([
|
||||
["a", "b"],
|
||||
]);
|
||||
});
|
||||
|
||||
it("agrees with hasRequiredValues on a prototype-collision name", () => {
|
||||
// The TUI derived its "Missing required template variable(s)" list with its
|
||||
// own bare `values[name]` filter: `({})["constructor"]` is `Object`, whose
|
||||
// `.length` is 1, so the group read as satisfied and the message named no
|
||||
// field while the gate still blocked the submit.
|
||||
const groups = [["constructor"]];
|
||||
expect(hasRequiredValues(groups, {})).toBe(false);
|
||||
expect(unmetRequiredGroups(groups, {})).toEqual([["constructor"]]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("tryExpandUriTemplate", () => {
|
||||
it("returns the URI and no error when the template expands", () => {
|
||||
expect(
|
||||
|
|
|
|||
|
|
@ -3,7 +3,10 @@ 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 {
|
||||
expandUriTemplate,
|
||||
tryExpandUriTemplate,
|
||||
} from "@inspector/core/mcp/uriTemplate.js";
|
||||
import {
|
||||
createTestServerHttp,
|
||||
type TestServerHttp,
|
||||
|
|
@ -83,15 +86,31 @@ describe("RFC 6570 resource templates over the wire (#1919)", () => {
|
|||
return connected;
|
||||
}
|
||||
|
||||
it("advertises both templates from the checked-in config", async () => {
|
||||
it("advertises every template from the checked-in config", async () => {
|
||||
const connected = await connectToShowcase();
|
||||
const { resourceTemplates } = await connected.listAllResourceTemplates();
|
||||
expect(resourceTemplates.map((entry) => entry.uriTemplate).sort()).toEqual([
|
||||
// The malformed one is advertised precisely so a client has to decide
|
||||
// what to do with it; the SDK's own constructor accepts it, so nothing
|
||||
// upstream of the Inspector rejects it. See the refusal test below.
|
||||
"foobar://events/{topic:abc}",
|
||||
"foobar://events/{topic}",
|
||||
"foobar://events{?topic}",
|
||||
]);
|
||||
});
|
||||
|
||||
it("refuses to expand the malformed template rather than guessing", async () => {
|
||||
// `abc` is not RFC 6570's `max-length` production, so there is no URI to
|
||||
// send. Guessing `{topic}` would read something the server never
|
||||
// advertised -- and this server would answer it, which is exactly why the
|
||||
// check has to happen client-side.
|
||||
const result = tryExpandUriTemplate("foobar://events/{topic:abc}", {
|
||||
topic: "news",
|
||||
});
|
||||
expect(result.uri).toBeUndefined();
|
||||
expect(result.error).toMatch(/Invalid RFC 6570 varspec/);
|
||||
});
|
||||
|
||||
it("resolves the encoded URI the simple expression expands to", async () => {
|
||||
const connected = await connectToShowcase();
|
||||
const uri = expandUriTemplate("foobar://events/{topic}", {
|
||||
|
|
|
|||
|
|
@ -122,21 +122,62 @@ export interface TemplateVariable {
|
|||
}
|
||||
|
||||
/**
|
||||
* 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.
|
||||
* One character of a variable's name.
|
||||
*
|
||||
* RFC 6570's `varchar` is `ALPHA / DIGIT / "_" / pct-encoded`. This admits two
|
||||
* more — `-` and `~` — and the widening is deliberate, on a rule the RFC's own
|
||||
* set is a subset of: **a name is the one thing expansion emits into the URI
|
||||
* without encoding it** (the `;`, `?` and `&` operators write `name=value`), so
|
||||
* a name must consist of characters that need no encoding. That is RFC 3986's
|
||||
* *unreserved* set — `ALPHA / DIGIT / -._~` — plus pct-encoded.
|
||||
*
|
||||
* Strict `varchar` would refuse `{user-id}`, which real servers publish and
|
||||
* which the SDK's own matcher round-trips (`match()` compiles the hyphen
|
||||
* literally). Refusing to read a resource that demonstrably works is a worse
|
||||
* outcome for a debugging tool than tolerating a name one character outside the
|
||||
* production — whereas everything this *does* reject is a shape we would
|
||||
* otherwise have to **guess** at, which is the line the rest of this module
|
||||
* draws too.
|
||||
*/
|
||||
const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/;
|
||||
const VARCHAR = String.raw`(?:[A-Za-z0-9_\-~]|%[0-9A-Fa-f]{2})`;
|
||||
|
||||
/**
|
||||
* RFC 6570's `varspec`: a `varname` with at most one trailing modifier, either
|
||||
* the explode `*` or a `:max-length`, never both.
|
||||
*
|
||||
* `varname = varchar *( ["."] varchar )`, so a `.` may separate name segments
|
||||
* but cannot lead, trail, or double. `max-length` is `%x31-39 0*3DIGIT` — 1 to
|
||||
* 9999, no leading zero — so `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are
|
||||
* invalid *templates*, not templates with an ignorable modifier.
|
||||
*
|
||||
* Anchoring the whole varspec is what rejects the shapes a looser "strip the
|
||||
* `*`, split on `:`" pass waves through. Measured against this branch before
|
||||
* the change, each expanded rather than being refused:
|
||||
*
|
||||
* | Varspec | Was | Why it is invalid |
|
||||
* | --------- | -------------------- | ----------------------------------- |
|
||||
* | `{*id}` | name `id` | explode is a *trailing* modifier |
|
||||
* | `{id*:3}` | name `id`, truncated | the two modifiers are exclusive |
|
||||
* | `{ id }` | name `id` | whitespace is not a `varchar` |
|
||||
* | `{a b}` | name `a b` | ditto — and it emits into the URI |
|
||||
*/
|
||||
const VARSPEC_GRAMMAR = new RegExp(
|
||||
`^(${VARCHAR}(?:\\.?${VARCHAR})*)(?:\\*|:([1-9][0-9]{0,3}))?$`,
|
||||
);
|
||||
|
||||
/**
|
||||
* 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.
|
||||
* or `"invalid"` for anything {@link VARSPEC_GRAMMAR} does not admit.
|
||||
*
|
||||
* Two shapes reach that verdict, and neither is ignorable:
|
||||
* Three classes reach that verdict, none of them ignorable:
|
||||
*
|
||||
* - **A modifier outside the grammar** (`{id:abc}`). Treating it as a plain
|
||||
* - **A modifier outside the grammar** (`{id:abc}`), or two at once (`{id*:3}`),
|
||||
* or one in the wrong position (`{*id}`). Treating any of them as a plain
|
||||
* `{id}` would send a URI that does not match what the server advertised,
|
||||
* with nothing to alert anyone.
|
||||
* with nothing to alert anyone — and `{id*:3}` silently *truncated* the value.
|
||||
* - **A name outside the character set** (`{a b}`, `{a/b}`). A named operator
|
||||
* emits the name verbatim, so this is the one place an unencodable character
|
||||
* reaches the URI as structure.
|
||||
* - **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
|
||||
|
|
@ -144,21 +185,15 @@ const MAX_LENGTH_GRAMMAR = /^[1-9][0-9]{0,3}$/;
|
|||
* 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.
|
||||
*
|
||||
* The explode modifier parses but is not honored: it only changes how a list or
|
||||
* map value is joined, and every value reaching this module is a single string.
|
||||
*/
|
||||
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 "invalid";
|
||||
|
||||
const colon = spec.indexOf(":");
|
||||
if (colon === -1) return { name: spec };
|
||||
|
||||
const name = spec.slice(0, colon);
|
||||
const modifier = spec.slice(colon + 1);
|
||||
if (name.length === 0 || !MAX_LENGTH_GRAMMAR.test(modifier)) return "invalid";
|
||||
return { name, maxLength: Number(modifier) };
|
||||
const match = VARSPEC_GRAMMAR.exec(raw);
|
||||
if (match === null) return "invalid";
|
||||
const [, name, maxLength] = match;
|
||||
return maxLength === undefined ? { name } : { name, maxLength: +maxLength };
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -264,6 +299,28 @@ export function requiredGroups(uriTemplate: string): string[][] {
|
|||
return groups;
|
||||
}
|
||||
|
||||
/**
|
||||
* The required expressions `values` does **not** satisfy — none of their names
|
||||
* filled in — in template order.
|
||||
*
|
||||
* Exported so a caller that has to *name* the missing fields derives them from
|
||||
* the same pass that decides whether anything is missing at all. The TUI built
|
||||
* its "Missing required template variable(s): …" list with its own bare
|
||||
* `values[name]` filter, which for a variable legitimately named `constructor`
|
||||
* or `toString` found `Object.prototype`'s member and judged the group
|
||||
* satisfied — so {@link hasRequiredValues} blocked the submit while the message
|
||||
* listed nothing.
|
||||
*/
|
||||
export function unmetRequiredGroups(
|
||||
groups: string[][],
|
||||
values: Record<string, string>,
|
||||
): string[][] {
|
||||
return groups.filter(
|
||||
(names) =>
|
||||
!names.some((name) => (readValue(values, name) ?? "").length > 0),
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether `values` supplies everything expansion structurally needs: every
|
||||
* required expression has at least one of its names filled in.
|
||||
|
|
@ -272,9 +329,7 @@ export function hasRequiredValues(
|
|||
groups: string[][],
|
||||
values: Record<string, string>,
|
||||
): boolean {
|
||||
return groups.every((names) =>
|
||||
names.some((name) => (readValue(values, name) ?? "").length > 0),
|
||||
);
|
||||
return unmetRequiredGroups(groups, values).length === 0;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -1539,6 +1539,24 @@ export function createRfc6570ResourceTemplates(): ResourceTemplateDefinition[] {
|
|||
inputSchema: { topic: z.string().describe("Topic name") },
|
||||
handler: echo("foobar://events{?topic}"),
|
||||
},
|
||||
{
|
||||
// A template no client can honor: `abc` is not RFC 6570's `max-length`
|
||||
// production (`%x31-39 0*3DIGIT`), so `{topic:abc}` is a malformed
|
||||
// *template* rather than one with an ignorable modifier. The SDK's
|
||||
// `UriTemplate` constructor accepts it, which is exactly why the
|
||||
// Inspector checks the grammar itself and refuses the read -- guessing
|
||||
// `{topic}` would send a URI this server never advertised.
|
||||
//
|
||||
// The handler is therefore unreachable through the Inspector by design.
|
||||
// It is registered anyway so the template appears in
|
||||
// `resources/templates/list`, which is what puts the refusal on screen.
|
||||
name: "events_malformed",
|
||||
uriTemplate: "foobar://events/{topic:abc}",
|
||||
description:
|
||||
"Malformed template - an out-of-grammar prefix modifier; the read is withheld",
|
||||
inputSchema: { topic: z.string().describe("Topic name") },
|
||||
handler: echo("foobar://events/{topic:abc}"),
|
||||
},
|
||||
];
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue