fix: expand each expression independently, and reject invalid varspecs
Addresses Copilot's round-5 review on #2035 — eight suppressed comments, all of them real. The consequential one overturns behavior I copied from the SDK and defended in two earlier rounds. RFC 6570 expands each expression independently; it carries no cross-expression state. The SDK tracks whether a query expression has already emitted and rewrites a later `{?two}`'s `?` to `&`. That looks friendlier and is wrong, and its own matcher says so: for `x{?one}{?two}` it expands to `x?one=1&two=2`, and `match("x?one=1&two=2")` on that same template returns **null**, while `match("x?one=1?two=2")` returns both variables. So the rewrite emitted a URI the advertised template cannot match — #1919 one level up. A server that wants a continuation advertises `{?one}{&two}`, which expands to `&` and matches; producing that shape is the server's choice to declare, not ours to infer. Also: - `{id:abc}` was silently treated as `{id}`, and a test codified it. RFC 6570's max-length is `%x31-39 0*3DIGIT`, so `{id:}`, `{id:0}`, `{id:abc}` and `{id:10000}` are invalid templates rather than templates with an ignorable modifier — and the SDK's constructor accepts them all, so nothing else rejects them. Strict expansion now throws; lenient returns the raw template. Discovery stays lenient so the panel still renders rather than going blank. - The TUI could submit `{a,b}` completely blank while the web panel blocked the same request: ink-form has no way to express "any one of these", so its members are left optional and the modal needs its own group check. Added, with tests — it was also below the coverage gate without them (functions 76.92%). - Added the integration test the config never had: it resolves the checked-in config and drives all four reads over a real transport, so a misspelt preset fails there rather than only when someone runs the repro by hand. - "All three clients" was false. The CLI has no template form and its `resources/read` passes the already-expanded `--uri` straight to `readResource`, so nothing here runs for it. Corrected in the README, AGENTS.md, and the module comment, and dropped a docblock still describing the SDK-delegation path removed last round. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
935113d1e0
commit
ebc18e1d56
2 changed files with 353 additions and 106 deletions
|
|
@ -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}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
139
clients/web/src/test/integration/mcp/rfc6570-templates.test.ts
Normal file
139
clients/web/src/test/integration/mcp/rfc6570-templates.test.ts
Normal file
|
|
@ -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<InspectorClient> {
|
||||
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);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue