fix: read template variables as own properties only

Addresses Copilot's round-6 review on #2035.

`toString`, `constructor`, `valueOf` and `__proto__` are all valid RFC
6570 variable names -- `varname` allows ALPHA / DIGIT / `_` / pct-encoded
-- and every lookup here was a bare `values[name]`, which finds
`Object.prototype`'s member for each of them. Measured: `({})["toString"]
!== undefined` is true with typeof "function", and `({}["constructor"] ??
"").length` is 1.

So a *blank* `{?toString}` field read as supplied and expanded a function
body into the URI instead of omitting the expression, and
`hasRequiredValues([["constructor"]], {})` returned true because
`Object.length` is 1 -- a required group satisfied by nothing.

All reads now go through an own-property `readValue`, and the web
preview's filled-check uses `Object.hasOwn`. `expandExpression` carries
the value through its filter rather than re-reading it afterwards, so the
"is it defined" test and the read cannot disagree and nothing needs a
non-null assertion.

The remaining six findings were documentation drifting behind the code,
which matters here because the comments are the argument for why this
module exists:

- The web helper still described the SDK's second-`?`-to-`&` rewrite as
  something the preview relies on. That was removed last round.
- Three places still named the CLI as a consumer. It has no template form
  and passes an already-expanded `--uri` to `readResource`.
- The PR description still described the superseded architecture
  (expansion delegated to the SDK), i.e. the opposite of what now ships.

Signed-off-by: cliffhall <cliff@futurescale.com>

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

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");
+  });
+});

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",
+    );
+  });
+});

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
+ * `@inspector/core/mcp/uriTemplate.js` so the web form and the TUI cannot
- * cannot disagree about what a template means -- they are re-exported here so
+ * disagree about what a template means -- they are re-exported here so the
- * the panel has a single import. What this module adds is the one piece that is
+ * panel has a single import. (The CLI is not a consumer: it has no template
- * purely a display concern: the partially-expanded preview string.
+ * 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
+ * A partially-expanded template for display: expressions with at least one
- * all filled are expanded exactly as `expandUriTemplate` would, and the rest
+ * value are expanded exactly as `expandUriTemplate` would, and the rest are
- * are left standing as written so the user can see what is still needed.
+ * left standing as written so the user can see what is still needed.
  *
- * Unfilled expressions are swapped for an inert token and restored after
+ * Unfilled expressions are swapped for an inert token and restored afterwards,
- * expansion -- rather than expanding each filled expression in isolation -- so
+ * and the rewritten template is expanded by `expandUriTemplate` itself. Routing
- * the expander still sees one whole template and applies its cross-expression
+ * it back through the real expander is what keeps the preview honest: it cannot
- * rules (notably rewriting a second `?` query expression to `&`). Routing the
+ * promise a URI that submitting would not send. (Expansion carries no
- * rewritten template back through `expandUriTemplate` is what keeps the preview
+ * cross-expression state — see `expandParts` — so this is a per-expression
- * honest: it can never promise a URI that submitting would not send.
+ * 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);

core/mcp/uriTemplate.ts

@@ -257,10 +257,27 @@ export function hasRequiredValues(
   values: Record<string, string>,
 ): 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<string, string>,
+  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, string>,
 ): string {
-  const present = part.varspecs.filter(
+  // The value is carried through the filter rather than re-read afterwards, so
-    (spec) => values[spec.name] !== undefined,
+  // 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, value }) => `${spec.name}=${renderValue(value, spec, operator)}`,
-        `${spec.name}=${renderValue(values[spec.name], 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) =>
+  const rendered = present.map(({ spec, value }) =>
-    renderValue(values[spec.name], spec, operator),
+    renderValue(value, spec, operator),
   );
 
   switch (operator) {