diff --git a/README.md b/README.md index c9f662e5..c7fa48e0 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ inspector/ │ │ # schema collapse shared by the web and TUI form builders │ ├── logging/ # Silent pino logger singleton │ ├── mcp/ # InspectorClient runtime, state stores, transports, config import, -│ │ # and the RFC 6570 URI-template helpers all three clients expand through +│ │ # and the RFC 6570 URI-template helpers the web form and TUI expand through │ ├── node/ # Node-only shared helpers: version reader, hostUrl (host normalize/canonicalize + all-interfaces/loopback detection) │ ├── react/ # React hooks over the state stores │ └── storage/ # File I/O helpers for the OAuth persist backends diff --git a/clients/web/src/test/core/mcp/uriTemplate.test.ts b/clients/web/src/test/core/mcp/uriTemplate.test.ts index 4b847131..f9bfb9b1 100644 --- a/clients/web/src/test/core/mcp/uriTemplate.test.ts +++ b/clients/web/src/test/core/mcp/uriTemplate.test.ts @@ -468,3 +468,41 @@ describe("prefix-modifier grammar", () => { ); }); }); + +describe("variable names that collide with Object.prototype", () => { + // `toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC 6570 + // varnames. A bare `values[name]` lookup finds the prototype's member for + // every one of them, so a *blank* field read as supplied: measured, + // `({})["toString"] !== undefined` is true and its typeof is "function". + it.each(["toString", "constructor", "valueOf", "hasOwnProperty"])( + "omits a blank {?%s} instead of expanding a prototype member", + (name) => { + expect(expandUriTemplate(`x://a{?${name}}`, { [name]: "" })).toBe( + "x://a", + ); + }, + ); + + it("omits the expression when the key is absent entirely", () => { + expect(expandUriTemplate("x://a{?toString}", {})).toBe("x://a"); + }); + + it("still expands such a variable when it really has a value", () => { + expect(expandUriTemplate("x://a{?toString}", { toString: "v" })).toBe( + "x://a?toString=v", + ); + }); + + it("does not treat an inherited member as satisfying a required group", () => { + // `Object` (the inherited constructor) has length 1, so the old + // `(values[name] ?? "").length > 0` test reported this as satisfied. + expect(hasRequiredValues([["constructor"]], {})).toBe(false); + expect(hasRequiredValues([["constructor"]], { constructor: "c" })).toBe( + true, + ); + }); + + it("handles __proto__ as an ordinary variable name", () => { + expect(expandUriTemplate("x://a{?__proto__}", {})).toBe("x://a"); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index feba12fe..3e9ca93b 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -63,3 +63,19 @@ describe("previewUriTemplate - multi-name expressions", () => { ); }); }); + +describe("preview with Object.prototype-colliding names", () => { + it("leaves a blank {?toString} standing rather than treating it as filled", () => { + // A bare `defined[name] !== undefined` finds Object.prototype.toString and + // would expand the expression instead of showing the placeholder. + expect(previewUriTemplate("x://a{?toString}", { toString: "" })).toBe( + "x://a{?toString}", + ); + }); + + it("expands it once it really has a value", () => { + expect(previewUriTemplate("x://a{?toString}", { toString: "v" })).toBe( + "x://a?toString=v", + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 56b2b825..ea6c089b 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -2,10 +2,12 @@ * The web Resources form's view of an RFC 6570 URI template (#1919). * * Parsing, variable classification, and expansion live in - * `@inspector/core/mcp/uriTemplate.js` so the web form, the TUI, and the CLI - * cannot disagree about what a template means -- they are re-exported here so - * the panel has a single import. What this module adds is the one piece that is - * purely a display concern: the partially-expanded preview string. + * `@inspector/core/mcp/uriTemplate.js` so the web form and the TUI cannot + * disagree about what a template means -- they are re-exported here so the + * panel has a single import. (The CLI is not a consumer: it has no template + * form, and its `resources/read` passes an already-expanded `--uri` straight to + * `readResource`.) What this module adds is the one piece that is purely a + * display concern: the partially-expanded preview string. */ import { @@ -37,16 +39,16 @@ export type { const deferredToken = (index: number) => `\u0000${index}\u0000`; /** - * A partially-expanded template for display: expressions whose variables are - * all filled are expanded exactly as `expandUriTemplate` would, and the rest - * are left standing as written so the user can see what is still needed. + * A partially-expanded template for display: expressions with at least one + * value are expanded exactly as `expandUriTemplate` would, and the rest are + * left standing as written so the user can see what is still needed. * - * Unfilled expressions are swapped for an inert token and restored after - * expansion -- rather than expanding each filled expression in isolation -- so - * the expander still sees one whole template and applies its cross-expression - * rules (notably rewriting a second `?` query expression to `&`). Routing the - * rewritten template back through `expandUriTemplate` is what keeps the preview - * honest: it can never promise a URI that submitting would not send. + * Unfilled expressions are swapped for an inert token and restored afterwards, + * and the rewritten template is expanded by `expandUriTemplate` itself. Routing + * it back through the real expander is what keeps the preview honest: it cannot + * promise a URI that submitting would not send. (Expansion carries no + * cross-expression state — see `expandParts` — so this is a per-expression + * substitution, not a whole-template rewrite that some later pass depends on.) */ export function previewUriTemplate( uriTemplate: string, @@ -59,7 +61,10 @@ export function previewUriTemplate( const rewritten = parts .map((part) => { if (part.kind === "literal") return part.text; - if (part.names.some((name) => defined[name] !== undefined)) { + // `Object.hasOwn`, not a bare lookup: `toString` and `constructor` are + // valid RFC 6570 variable names, and a plain lookup would find + // `Object.prototype`'s member and treat a blank field as filled. + if (part.names.some((name) => Object.hasOwn(defined, name))) { return part.source; } deferred.push(part.source); diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index b68a5c42..7e5c1d87 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -257,10 +257,27 @@ export function hasRequiredValues( values: Record, ): boolean { return groups.every((names) => - names.some((name) => (values[name] ?? "").length > 0), + names.some((name) => (readValue(values, name) ?? "").length > 0), ); } +/** + * Reads a variable, ignoring anything inherited from `Object.prototype`. + * + * `toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC 6570 + * variable names (`varname` allows ALPHA / DIGIT / `_` / pct-encoded), and a + * plain object lookup finds the prototype's member for every one of them. A + * bare `values[name] !== undefined` therefore reports a *blank* `{?toString}` + * as supplied and expands a function body into the URI; `hasRequiredValues` + * likewise saw `constructor` as satisfied because `Object.length` is 1. + */ +function readValue( + values: Record, + name: string, +): string | undefined { + return Object.hasOwn(values, name) ? values[name] : undefined; +} + /** * Drops empty entries so an untouched optional field reads as *undefined* * (the expression disappears) rather than as the empty string (which would @@ -361,17 +378,20 @@ function expandExpression( part: TemplateExpression, values: Record, ): string { - const present = part.varspecs.filter( - (spec) => values[spec.name] !== undefined, - ); + // The value is carried through the filter rather than re-read afterwards, so + // the "is it defined" test and the read cannot disagree — and so nothing + // downstream needs a non-null assertion to convince the compiler. + const present = part.varspecs.flatMap((spec) => { + const value = readValue(values, spec.name); + return value === undefined ? [] : [{ spec, value }]; + }); if (present.length === 0) return ""; const { operator } = part; if (NAMED_OPERATORS.has(operator)) { const pairs = present.map( - (spec) => - `${spec.name}=${renderValue(values[spec.name], spec, operator)}`, + ({ spec, value }) => `${spec.name}=${renderValue(value, spec, operator)}`, ); // `;` repeats its separator per pair; `?`/`&` join with `&`. return operator === ";" @@ -379,8 +399,8 @@ function expandExpression( : `${operator}${pairs.join("&")}`; } - const rendered = present.map((spec) => - renderValue(values[spec.name], spec, operator), + const rendered = present.map(({ spec, value }) => + renderValue(value, spec, operator), ); switch (operator) {