diff --git a/AGENTS.md b/AGENTS.md index 527eccbc..3b5df783 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,11 +82,15 @@ v2/main/ │ │ # containing any of the three shapes it does not: │ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), │ │ # `{;id}` (operator absent from its list), and -│ │ # `{id:3}` (prefix modifier folded into the name) — -│ │ # the last two would render form fields literally +│ │ # `{id:3}` (prefix modifier folded into the name), +│ │ # and `{+v}`/`{#v}` (encodeURI mangles reserved +│ │ # `[`/`]` and double-encodes pct-triplets). The `;` +│ │ # and `:N` cases would render form fields literally │ │ # labelled `;id` / `id:3`. Requiredness is per -│ │ # EXPRESSION, not per variable (hasRequiredValues): -│ │ # `{a,b}` with only `a` filled is expandable — #1919; +│ │ # EXPRESSION, not per variable — requiredGroups + +│ │ # hasRequiredValues: `{a,b}` with only `a` filled is +│ │ # expandable, and a name recurring across +│ │ # expressions needs each group checked — #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 f274f411..af8917c5 100644 --- a/README.md +++ b/README.md @@ -251,13 +251,16 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r 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` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): -| Shape | SDK `variableNames` | SDK expansion | Correct | -| --- | --- | --- | --- | -| `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, operator prefix dropped | `foo%2Fbar,q` | -| `{;id}` | `[";id"]` | `""` — the `;` operator is not in its list | `;id=7` | -| `{id:3}` | `["id:3"]` | `""` — the prefix modifier is folded into the name | `abc` | +| Shape | SDK behavior | Correct | +| --- | --- | --- | +| `{a,b}` | `foo/bar,q` — raw-joined, unencoded, operator prefix dropped | `foo%2Fbar,q` | +| `{;id}` | `""` — the `;` operator is not in its list, so the variable parses as `;id` | `;id=7` | +| `{id:3}` | `""` — the prefix modifier is folded into the name, giving `id:3` | `abc` | +| `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | `[::1]`, `%2F` | -The last two matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render fields literally labelled `;id` and `id:3`. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. +The `;` and `:3` rows matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render 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. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. + +Requiredness is likewise 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 the 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 a per-variable flag cannot express once a name recurs across expressions. One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index f2ac2b8e..7b39b4ad 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -18,6 +18,7 @@ import { expandUriTemplate, hasRequiredValues, previewUriTemplate, + requiredGroups, templateVariables, } from "../../../utils/uriTemplate"; @@ -96,6 +97,10 @@ export function ResourceTemplatePanel({ () => declaredVariables.map((v) => v.name), [declaredVariables], ); + // The names of each expression that cannot be omitted. Kept separate from + // `declaredVariables` (which is deduplicated for rendering) because each + // required expression has to be satisfied on its own — see `requiredGroups`. + const groups = useMemo(() => requiredGroups(uriTemplate), [uriTemplate]); const [variables, setVariables] = useState>(() => Object.fromEntries(variableNames.map((n) => [n, ""])), @@ -221,7 +226,7 @@ export function ResourceTemplatePanel({ // 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(declaredVariables, variables); + const canSubmit = hasRequiredValues(groups, variables); function handleSubmit() { onReadResource(expandUriTemplate(uriTemplate, variables)); @@ -240,7 +245,7 @@ export function ResourceTemplatePanel({ {description && {description}} - {declaredVariables.map(({ name: varName, required, groupNames }) => { + {declaredVariables.map(({ name: varName, required }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; // RFC 6570 omits an undefined variable under a query/path-segment @@ -248,10 +253,13 @@ export function ResourceTemplatePanel({ // required multi-name expression no single field is mandatory either // -- any one of them satisfies it -- so say which, rather than // marking each one required and blocking valid input. + const sharedGroup = groups.find( + (names) => names.length > 1 && names.includes(varName), + ); const description = !required ? "Optional" - : groupNames.length > 1 - ? `Any one of: ${groupNames.join(", ")}` + : sharedGroup + ? `Any one of: ${sharedGroup.join(", ")}` : undefined; return useAutocomplete ? ( { describe("templateVariables", () => { it("finds a simple variable and marks it required", () => { expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true, groupNames: ["topic"] }, + { name: "topic", operator: "", required: true }, ]); }); it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false, groupNames: ["topic"] }, + { name: "topic", operator: "?", required: false }, ]); }); @@ -76,7 +77,7 @@ describe("templateVariables", () => { it("deduplicates a repeated name and keeps it required if any use is", () => { expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true, groupNames: ["id"] }, + { name: "id", operator: "?", required: true }, ]); }); @@ -204,7 +205,7 @@ describe("varspec modifiers", () => { // user cannot usefully fill. it("parses a prefix modifier off the variable name", () => { expect(templateVariables("x://a/{id:3}")).toEqual([ - { name: "id", operator: "", required: true, groupNames: ["id"] }, + { name: "id", operator: "", required: true }, ]); }); @@ -243,7 +244,7 @@ describe("the ; (path-parameter) operator", () => { // variable named ";id" and expands to "". it("is recognised as an operator, not part of the name", () => { expect(templateVariables("x://a{;id}")).toEqual([ - { name: "id", operator: ";", required: false, groupNames: ["id"] }, + { name: "id", operator: ";", required: false }, ]); }); @@ -266,39 +267,67 @@ describe("the ; (path-parameter) operator", () => { }); }); -describe("hasRequiredValues", () => { +describe("requiredGroups / hasRequiredValues", () => { // A required *expression* is satisfied by any one of its names, because // RFC 6570 drops the undefined ones -- verified against the SDK: // `x://{a,b}` with only `a` expands to "x://only-a". it("accepts a multi-name expression with only one name filled", () => { - const vars = templateVariables("x://{a,b}"); - expect(hasRequiredValues(vars, { a: "only-a", b: "" })).toBe(true); + const groups = requiredGroups("x://{a,b}"); + expect(groups).toEqual([["a", "b"]]); + expect(hasRequiredValues(groups, { a: "only-a", b: "" })).toBe(true); expect(expandUriTemplate("x://{a,b}", { a: "only-a", b: "" })).toBe( "x://only-a", ); }); it("rejects a multi-name expression with nothing filled", () => { - const vars = templateVariables("x://{a,b}"); - expect(hasRequiredValues(vars, { a: "", b: "" })).toBe(false); + expect( + hasRequiredValues(requiredGroups("x://{a,b}"), { a: "", b: "" }), + ).toBe(false); }); it("still requires a lone required variable", () => { - const vars = templateVariables("file:///users/{userId}/profile"); - expect(hasRequiredValues(vars, { userId: "" })).toBe(false); - expect(hasRequiredValues(vars, { userId: "alice" })).toBe(true); + const groups = requiredGroups("file:///users/{userId}/profile"); + expect(hasRequiredValues(groups, { userId: "" })).toBe(false); + expect(hasRequiredValues(groups, { userId: "alice" })).toBe(true); }); it("never blocks on an omittable expression", () => { - const vars = templateVariables("foobar://events{?topic}"); - expect(hasRequiredValues(vars, { topic: "" })).toBe(true); + expect(requiredGroups("foobar://events{?topic}")).toEqual([]); + expect( + hasRequiredValues(requiredGroups("foobar://events{?topic}"), { + topic: "", + }), + ).toBe(true); }); it("is satisfied by a template with no variables at all", () => { - expect(hasRequiredValues(templateVariables("file:///static.txt"), {})).toBe( + expect(hasRequiredValues(requiredGroups("file:///static.txt"), {})).toBe( true, ); }); + + it("tracks a name that recurs under a different operator", () => { + // A per-variable model keeping only the first occurrence's group would + // mark both names required with singleton groups and refuse this input; + // the SDK expands the same template with just `a` to "x?a=11". + const groups = requiredGroups("x{?a}{?b}{a,b}"); + expect(groups).toEqual([["a", "b"]]); + expect(hasRequiredValues(groups, { a: "1", b: "" })).toBe(true); + expect(expandUriTemplate("x{?a}{?b}{a,b}", { a: "1" })).toBe("x?a=11"); + }); + + it("satisfies two required expressions sharing a name, from the others", () => { + // `{a,b}{a,c}`: filling only b and c satisfies both groups. No + // per-variable flag can express this, which is why groups are separate. + const groups = requiredGroups("x://{a,b}{a,c}"); + expect(groups).toEqual([ + ["a", "b"], + ["a", "c"], + ]); + expect(hasRequiredValues(groups, { b: "B", c: "C" })).toBe(true); + expect(hasRequiredValues(groups, { b: "B" })).toBe(false); + }); }); describe("cross-expression query joining", () => { @@ -354,3 +383,52 @@ describe("strict vs lenient expansion", () => { ); }); }); + +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]") + // is "%5B::1%5D" and encodeURI("%2F") is "%252F". + it.each(["+", "#"])("leaves reserved [ and ] intact under %s", (operator) => { + const prefix = operator === "#" ? "#" : ""; + expect(expandUriTemplate(`x://{${operator}v}`, { v: "[::1]" })).toBe( + `x://${prefix}[::1]`, + ); + }); + + it.each(["+", "#"])( + "does not double-encode an existing pct-triplet under %s", + (operator) => { + const prefix = operator === "#" ? "#" : ""; + expect(expandUriTemplate(`x://{${operator}v}`, { v: "a%2Fb" })).toBe( + `x://${prefix}a%2Fb`, + ); + }, + ); + + it("still encodes a lone % that is not a triplet", () => { + expect(expandUriTemplate("x://{+v}", { v: "100%" })).toBe("x://100%25"); + }); + + it("still encodes characters outside the allowed set", () => { + expect(expandUriTemplate("x://{+v}", { v: "a b" })).toBe("x://a%20b"); + }); + + it("encodes an astral character whole rather than as surrogates", () => { + expect(expandUriTemplate("x://{+v}", { v: "\u{1F600}" })).toBe( + `x://${encodeURIComponent("\u{1F600}")}`, + ); + }); + + it("applies the same encoding in a multi-name + expression", () => { + expect(expandUriTemplate("x://{+a,b}", { a: "[::1]", b: "%2F" })).toBe( + "x://[::1],%2F", + ); + }); + + it("still percent-encodes reserved characters under the simple operator", () => { + // Only + and # allow reserved through; the default path is unchanged. + expect(expandUriTemplate("x://{v}", { v: "[::1]" })).toBe( + "x://%5B%3A%3A1%5D", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 0a14a33d..56b2b825 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -18,6 +18,7 @@ export { expandUriTemplate, hasRequiredValues, parseUriTemplate, + requiredGroups, templateVariables, } from "@inspector/core/mcp/uriTemplate.js"; export type { diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 28afd7e0..0a790cf0 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -95,21 +95,17 @@ export interface TemplateVariable { /** The operator of the expression the variable was first seen in. */ operator: string; /** - * True when the expression this variable belongs to cannot be omitted - * without changing the URI's structure — see {@link OMITTABLE_OPERATORS}. + * True when this variable appears in at least one expression that cannot be + * omitted without changing the URI's structure — see + * {@link OMITTABLE_OPERATORS}. Drives the form's "Optional" marker. * - * Note this is a property of the *expression*, not of the single variable: - * RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` - * with only `a` filled expands to `a`'s value. Use {@link hasRequiredValues} - * rather than testing every required variable individually, or a form will - * refuse input the expander would have accepted. + * It does **not** mean "the user must fill this field in". Requiredness is a + * property of the *expression*: RFC 6570 drops undefined names from a + * multi-name expression, so `{a,b}` with only `a` filled expands to `a`'s + * value. Gate submission on {@link hasRequiredValues} over + * {@link requiredGroups}, never by testing this flag per variable. */ required: boolean; - /** - * Every name in the expression this variable belongs to, itself included. - * A single-name expression yields a one-element array. - */ - groupNames: string[]; } /** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ @@ -198,12 +194,7 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { if (existing) { existing.required = existing.required || required; } else { - byName.set(name, { - name, - operator: part.operator, - required, - groupNames: part.names, - }); + byName.set(name, { name, operator: part.operator, required }); } } } @@ -212,21 +203,39 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { } /** - * Whether `values` supplies everything expansion structurally needs. + * The variable names of each expression that cannot be omitted, in template + * order — one entry per expression, not per variable. * - * A required *expression* is satisfied by any one of its names having a value, - * because RFC 6570 drops the undefined ones — `{a,b}` with only `a` filled - * expands to `a`'s value, which the SDK does too. Testing each required - * variable individually would block that valid input. + * This is deliberately *not* folded onto {@link TemplateVariable}. A name can + * appear in several expressions with different operators, and each required + * expression has to be satisfied on its own: in `x{?a}{?b}{a,b}` the only + * required expression is `{a,b}`, which either `a` or `b` satisfies (the SDK + * expands that template with just `a` to `x?a=11`), while a per-variable model + * that kept only the first occurrence's group would mark both names required + * with singleton groups and refuse it. And in `{a,b}{a,c}` — two required + * expressions sharing `a` — filling `b` and `c` satisfies both, which no + * per-variable flag can express at all. + */ +export function requiredGroups(uriTemplate: string): string[][] { + const groups: string[][] = []; + for (const part of parseUriTemplate(uriTemplate)) { + if (part.kind !== "expression") continue; + if (OMITTABLE_OPERATORS.has(part.operator)) continue; + groups.push(part.names); + } + return groups; +} + +/** + * Whether `values` supplies everything expansion structurally needs: every + * required expression has at least one of its names filled in. */ export function hasRequiredValues( - variables: TemplateVariable[], + groups: string[][], values: Record, ): boolean { - return variables.every( - (variable) => - !variable.required || - variable.groupNames.some((name) => (values[name] ?? "").length > 0), + return groups.every((names) => + names.some((name) => (values[name] ?? "").length > 0), ); } @@ -243,10 +252,45 @@ export function definedValues( ); } -/** The SDK's `encodeValue`: reserved characters survive under `+` and `#`. */ +/** + * The characters RFC 6570 leaves alone under the `+` and `#` operators: + * RFC 3986 *unreserved* plus *reserved* (gen-delims and sub-delims). + */ +const ALLOW_RESERVED = /[^A-Za-z0-9\-._~:/?#[\]@!$&'()*+,;=]/gu; + +/** + * The `allow-reserved` value encoding of RFC 6570 §3.2.1, used by `+` and `#`. + * + * The SDK reaches for `encodeURI` here, which is close but wrong twice over, + * and both cases corrupt the URI rather than merely over-escaping it: + * + * - It escapes `[` and `]`, which are *reserved* and must survive — so an IPv6 + * literal `[::1]` becomes `%5B::1%5D`. + * - It escapes `%`, so an already-encoded value is double-encoded: `%2F` + * becomes `%252F`, and the server sees a literal "%2F" rather than a slash. + * + * The spec instead keeps existing pct-triplets intact, which is what the split + * below does: odd chunks are whole `%XX` triplets and pass through untouched, + * even chunks are scanned for anything outside the allowed set. A lone `%` is + * not a triplet, so it lands in an even chunk and is correctly encoded to + * `%25`. The `u` flag makes the class match by code point, so an astral + * character is handed to `encodeURIComponent` whole rather than as surrogates. + */ +function encodeAllowReserved(value: string): string { + return value + .split(/(%[0-9A-Fa-f]{2})/g) + .map((chunk, index) => + index % 2 === 1 + ? chunk + : chunk.replace(ALLOW_RESERVED, (char) => encodeURIComponent(char)), + ) + .join(""); +} + +/** Encodes one value for its operator: reserved characters survive `+` and `#`. */ function encodeValue(value: string, operator: string): string { return operator === "+" || operator === "#" - ? encodeURI(value) + ? encodeAllowReserved(value) : encodeURIComponent(value); } @@ -278,6 +322,12 @@ function needsOwnExpansion(part: TemplatePart): boolean { part.operator !== "&") || // The SDK has no `;` operator at all. part.operator === ";" || + // The SDK encodes `+`/`#` values with `encodeURI`, which mangles reserved + // `[`/`]` and double-encodes existing pct-triplets — see + // `encodeAllowReserved`. Taken over even for a single name so both paths + // agree on what these operators mean. + part.operator === "+" || + part.operator === "#" || // The SDK folds everything after a `:` into the variable name. Keyed on the // raw source rather than on a parsed `maxLength` so a *malformed* modifier // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and