From 3452a6310144529a4112938eb555ece79b75bd8e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 12:26:33 -0400 Subject: [PATCH] fix(auth): address Copilot review on the secret-store PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings, all taken. - The descriptor reported the *write policy*, not the file. Adding `MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext file flipped `store.encrypted` immediately, so the banner and footer claimed "File (encrypted)" while the existing secrets sat readable until the next `set`. `FileSecretStore.readOnDiskEncryption()` now reads the envelope (never the payload, so it needs no passphrase) and the descriptor is built from that. The transitional state gets its own `pendingEncryption` flag, which changes the advice without softening the verdict — "set a passphrase" is the wrong thing to tell someone who already has, so the caveat names the pending write instead. - The secrets file ignored the configured storage directory. A container mounting only that directory was judged unmounted, fell back to `memory`, and lost secrets the user had arranged to keep. `defaultSecretFilePath()` now follows `MCP_STORAGE_DIR` — the variable that already relocates OAuth tokens and `client.json` — between the explicit `MCP_INSPECTOR_SECRET_FILE` and the `~/.mcp-inspector` default. (The issue comment called it `MCP_INSPECTOR_STORAGE_DIR`; no such variable exists.) - `version` was never checked before the plaintext branch, so a `{ version: 2, encryption: "none" }` file was read and silently rewritten as version 1, discarding whatever the newer format added — the same destroy-to-satisfy-an-additive-request case the encrypted branch already refuses. Checked first now, so the answer no longer depends on which encryption mode the newer writer chose. - The doc comment claimed a per-file salt made "a short, memorable value safe to use". It does not: the salt defeats precomputation, not guessing, and the scrypt cost is deliberately a few milliseconds because it runs on every read and write. Both the comment and the README now say to use a high-entropy value, generated rather than chosen. - `usableSecretStorage` validated only `kind`, so a partial `{ kind: "file" }` was cast through and — because a missing `plaintext` is falsy — rendered as the quiet, neutral *encrypted* file. That is the confident half-answer the guard exists to reject, produced by the absence of information rather than by any claim the backend made. `reason`, `durable`, and each kind's own required fields are checked. - The footer's inline `Tooltip` carried four static props; extracted to a `CaveatTooltip` constant per the `.withProps()` rule. Tests cover each: the policy/file disagreement in both directions and the no-file-yet fallback, `MCP_STORAGE_DIR` and its precedence against `MCP_INSPECTOR_SECRET_FILE`, the version refusal (asserting the bytes survive it) split from the unrecognized-cipher refusal it used to be conflated with, `readOnDiskEncryption` including the keyless and unparseable cases, five new rejected descriptor shapes, and the changed caveat. Full `npm run ci` green. Signed-off-by: cliffhall --- README.md | 96 ++++++++++--------- .../SecretStorageFooter.tsx | 17 ++-- .../core/auth/secret-storage-info.test.ts | 16 ++++ .../test/core/react/useInitialConfig.test.tsx | 21 ++++ .../auth/node/file-secret-store.test.ts | 91 +++++++++++++++++- .../auth/node/secret-store-selection.test.ts | 83 ++++++++++++++++ core/auth/node/file-secret-store.ts | 68 ++++++++++++- core/auth/node/secret-store-selection.ts | 41 ++++++-- core/auth/secret-storage-info.ts | 26 ++++- core/react/useInitialConfig.ts | 27 +++++- 10 files changed, 411 insertions(+), 75 deletions(-) diff --git a/README.md b/README.md index 97e75e05..eb0624d7 100644 --- a/README.md +++ b/README.md @@ -139,22 +139,22 @@ This is what lets an Inspector connection negotiating `protocolEra: "auto" | "mo Each config below is a ready-made server for exercising one feature by hand. Load one with `--config`, and unless noted, connect with **Protocol Era = Modern**. -| Config | Demonstrates | Issue | -| ----------------------------------------- | -------------------------------------------------- | ---------------------------------------------------------------------- | +| Config | Demonstrates | Issue | +| ----------------------------------------- | --------------------------------------------------- | ---------------------------------------------------------------------- | | `mcp-app-http.json` **(legacy era)** | An MCP App (UI resource + app tool) in the Apps tab | [#1859](https://github.com/modelcontextprotocol/inspector/issues/1859) | -| `modern-mrtr-http.json` | A single MRTR round-trip | — | -| `mrtr-showcase-http.json` | Every MRTR preset in one server | [#1860](https://github.com/modelcontextprotocol/inspector/issues/1860) | -| `modern-network-http.json` | Network tab: `Mcp-*` headers + error taxonomy | [#1628](https://github.com/modelcontextprotocol/inspector/issues/1628) | -| `xmcpheader-modern-http.json` | Tools tab: `x-mcp-header` mirroring and exclusions | [#1632](https://github.com/modelcontextprotocol/inspector/issues/1632) | -| `pagination-http.json` | Page-by-page list fetching | [#1721](https://github.com/modelcontextprotocol/inspector/issues/1721) | -| `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | -| `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | -| `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | +| `modern-mrtr-http.json` | A single MRTR round-trip | — | +| `mrtr-showcase-http.json` | Every MRTR preset in one server | [#1860](https://github.com/modelcontextprotocol/inspector/issues/1860) | +| `modern-network-http.json` | Network tab: `Mcp-*` headers + error taxonomy | [#1628](https://github.com/modelcontextprotocol/inspector/issues/1628) | +| `xmcpheader-modern-http.json` | Tools tab: `x-mcp-header` mirroring and exclusions | [#1632](https://github.com/modelcontextprotocol/inspector/issues/1632) | +| `pagination-http.json` | Page-by-page list fetching | [#1721](https://github.com/modelcontextprotocol/inspector/issues/1721) | +| `structured-output-http.json` | Tools tab: a result's `structuredContent` section | [#1908](https://github.com/modelcontextprotocol/inspector/issues/1908) | +| `duplicate-tool-names-http.json` | A `tools/list` that repeats a tool name | [#1957](https://github.com/modelcontextprotocol/inspector/issues/1957) | +| `nullable-fields-http.json` | Tools tab: nullable (`anyOf` + `null`) arguments | [#1928](https://github.com/modelcontextprotocol/inspector/issues/1928) | | `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | -| `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | -| `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | -| `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | -| `tasks-{legacy,modern}-http.json` | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | +| `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | +| `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | +| `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | +| `tasks-{legacy,modern}-http.json` | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | #### MCP Apps @@ -266,23 +266,23 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r The web client and the TUI expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI via `InspectorClient.readResourceFromTemplate` — and both derive their **form fields** from its parser too, which is the half that makes the sharing real: a form submits values under the names it rendered, so a parser that mangles a name silently drops the value at expansion time. (The CLI is not a consumer: it has no template form, and its `resources/read` passes the already-expanded `--uri` straight through.) -The SDK's `UriTemplate` is still used, but only to *validate* a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: +The SDK's `UriTemplate` is still used, but only to _validate_ a template (constructing it is what rejects an unclosed expression). Its expander is not, because it is incomplete in five ways — each measured against the pinned SDK, not inferred: -| Shape | SDK behavior | -| --- | --- | -| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | -| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | -| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | +| Shape | SDK behavior | +| --------------- | -------------------------------------------------------------------------------------------------------------- | +| `{a,b}` | raw-joins the values — no encoding, operator prefix dropped | +| `{;id}` | `;` is missing from its operator list, so the variable parses as `;id` | +| `{id:3}` | the prefix modifier is folded into the name, giving `id:3` | | `{+v}` / `{#v}` | `encodeURI` mangles reserved `[`/`]` (`[::1]` → `%5B::1%5D`) and double-encodes pct-triplets (`%2F` → `%252F`) | -| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | +| `{v}` | `encodeURIComponent` leaves the sub-delims `!'()*` bare, which RFC 6570 requires encoded | 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. 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. -Literals are pct-encoded on expansion too (RFC 6570 §3.1): `café/{var}` sends `caf%C3%A9/value`, not raw UTF-8 in the path — something the SDK's expander does not do either. And the *names* a template may use are RFC 6570's `varchar` plus a labelled tolerance for `-` and `~`: the conformance suite rejects `{default-graph-uri}`, but real servers publish such names and the SDK's matcher round-trips them, so the Inspector expands them and marks the variable `conforming: false` rather than refusing a resource that demonstrably works. +Literals are pct-encoded on expansion too (RFC 6570 §3.1): `café/{var}` sends `caf%C3%A9/value`, not raw UTF-8 in the path — something the SDK's expander does not do either. And the _names_ a template may use are RFC 6570's `varchar` plus a labelled tolerance for `-` and `~`: the conformance suite rejects `{default-graph-uri}`, but real servers publish such names and the SDK's matcher round-trips them, so the Inspector expands them and marks the variable `conforming: false` rather than refusing a resource that demonstrably works. -An **undefined** variable is what omits its expression — a variable defined as the empty string expands (`x{?q}` gives `x?q=`, `x{;q}` gives `x;q`, per RFC 6570 §3.2.7). The expander honors that distinction, so a caller such as `readResourceFromTemplate` can request either URI. Collapsing the two is a *form* concern, not a template one: both clients seed every declared variable with `""` and a text input cannot express "defined but empty", so each form drops its blanks (`definedValues`) on the way in. +An **undefined** variable is what omits its expression — a variable defined as the empty string expands (`x{?q}` gives `x?q=`, `x{;q}` gives `x;q`, per RFC 6570 §3.2.7). The expander honors that distinction, so a caller such as `readResourceFromTemplate` can request either URI. Collapsing the two is a _form_ concern, not a template one: both clients seed every declared variable with `""` and a text input cannot express "defined but empty", so each form drops its blanks (`definedValues`) on the way in. 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`). @@ -341,18 +341,18 @@ Individual clients: `build:web`, `build:cli`, `build:tui`, `build:launcher`. The Each client self-validates from its own folder; the root scripts chain them. There is **no** aggregate root `test` script — use `validate` (fast) or `coverage` (the gate). -| Script | What it does | -| ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | -| `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | -| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus two headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge. | -| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | -| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | -| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | -| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. -| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | -| `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | +| Script | What it does | +| ----------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `npm run validate` | Runs the three durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:typecheck-coverage` (every one lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | +| `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus two headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge. | +| `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | +| `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards _this_ gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | +| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only _within a major version_. Runs in `validate`. | +| `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | +| `npm run pack:verify` | Publish smoke — see [Publishing](#publishing). | Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. @@ -371,7 +371,7 @@ The root `package.json` `"files"` allowlist is the source of truth for the tarba - **No source maps.** The client bundlers set `sourcemap: false` (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`); Vite and the launcher's `tsc` already emit none. Maps are ~half the unpacked size and aren't needed at runtime — debug via `npm run dev` on the source. - **`clients/web/build` ships via `clients/web/.npmignore`.** `clients/web/.gitignore` lists `build/`, and npm's packlist honors that nested `.gitignore` over the root `"files"` allowlist — so the prod web-server runner was silently missing from the tarball while `clients/web/dist` slipped through (its `.gitignore` only lists `dist-ssr`). `clients/web/.npmignore` overrides the `.gitignore` for publishing so both `build/` (runner) and `dist/` (SPA) ship. The other clients don't need this — none ship a nested `.gitignore`. - **`clients/web/static` ships the MCP Apps sandbox proxy.** `clients/web/static/sandbox_proxy.html` is a committed source file (not a build artifact), read from disk at runtime by `clients/web/server/sandbox-controller.ts` as `/../static/sandbox_proxy.html`. It was missing from the root `"files"` allowlist entirely, so every published build failed the Apps tab with **"Sandbox not loaded"** ([#1859](https://github.com/modelcontextprotocol/inspector/issues/1859)) while working fine in the repo. Because the path is resolved _relative to_ `clients/web/build`, the directory must ship at that exact location — `pack:verify` asserts both the tarball entry and the installed-on-disk path. -- **A dependency that renders React is bundled, not externalized.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, which is not necessarily where the bundle resolves ours — npm places a package beside a React satisfying *its* peer range, and those ranges are looser than ours. `ink-form` and `ink-scroll-view` declare `">=18"`, so a project holding React 18 satisfies them and gets them hoisted while the Inspector's React 19 nests underneath: two React copies, and the TUI dies with `TypeError: Cannot read properties of null (reading 'useState')` the moment a tool test form or a scroll view mounts ([#1952](https://github.com/modelcontextprotocol/inspector/issues/1952)). Both are therefore inlined by `clients/tui/tsup.config.ts` and are **not** root dependencies: the tarball ships their code inside `clients/tui/build/index.js` rather than having consumers install them. Bundling also pins their transitive deps to what this repo's install resolved (notably `ink-select-input@6` via `overrides`, which npm ignores for a package installed as a dependency). **`ink` is the one exception, on cost:** bundling it works but adds ~1.4 MB (`react-reconciler` and `yoga-layout` come along, plus a `createRequire` banner for the inlined CJS), so it stays external — *not* because its `">=19"` peer makes it safe, which it does not. What keeps that tolerable is the root `react` range: `"^19.0.0"` is deliberately open to the whole major so npm can dedupe our React with whatever React 19 a consumer pins, leaving an external `ink` on the same copy the bundle uses. **Narrowing that range reopens the bug for the renderer itself** — `clients/tui/__tests__/tsupConfig.test.ts` pins it to `ink`'s peer floor, and guards the rest of the split; see the [TUI README](./clients/tui/README.md#bundling-react-rendering-dependencies-must-be-inlined-1952). +- **A dependency that renders React is bundled, not externalized.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, which is not necessarily where the bundle resolves ours — npm places a package beside a React satisfying _its_ peer range, and those ranges are looser than ours. `ink-form` and `ink-scroll-view` declare `">=18"`, so a project holding React 18 satisfies them and gets them hoisted while the Inspector's React 19 nests underneath: two React copies, and the TUI dies with `TypeError: Cannot read properties of null (reading 'useState')` the moment a tool test form or a scroll view mounts ([#1952](https://github.com/modelcontextprotocol/inspector/issues/1952)). Both are therefore inlined by `clients/tui/tsup.config.ts` and are **not** root dependencies: the tarball ships their code inside `clients/tui/build/index.js` rather than having consumers install them. Bundling also pins their transitive deps to what this repo's install resolved (notably `ink-select-input@6` via `overrides`, which npm ignores for a package installed as a dependency). **`ink` is the one exception, on cost:** bundling it works but adds ~1.4 MB (`react-reconciler` and `yoga-layout` come along, plus a `createRequire` banner for the inlined CJS), so it stays external — _not_ because its `">=19"` peer makes it safe, which it does not. What keeps that tolerable is the root `react` range: `"^19.0.0"` is deliberately open to the whole major so npm can dedupe our React with whatever React 19 a consumer pins, leaving an external `ink` on the same copy the bundle uses. **Narrowing that range reopens the bug for the renderer itself** — `clients/tui/__tests__/tsupConfig.test.ts` pins it to `ink`'s peer floor, and guards the rest of the split; see the [TUI README](./clients/tui/README.md#bundling-react-rendering-dependencies-must-be-inlined-1952). - **A single version number, read from the root `package.json`.** The Inspector ships as one package with one version, so only the **root** `package.json` carries a `version` — the four `clients/*/package.json`s deliberately have none. Every Node client (CLI, TUI, and the web backend) resolves the version through the shared `readInspectorVersion()` reader in `core/node/version.ts`, which walks up to the root manifest (always present in the tarball). No client `package.json` is read at runtime, so none needs to ship. The web **browser** can't read the filesystem; it gets its version from the backend via `GET /api/config` (see [#1639](https://github.com/modelcontextprotocol/inspector/issues/1639)). ### `npm run pack:verify` — publish smoke against the real tarball @@ -403,7 +403,7 @@ npm version minor --no-git-tag-version # or major / patch; bump only, no tag **2. Merge `v2/main` → `main`** through the usual milestone-merge branch. It now carries the bump, so the release lands on `main` with the version already correct. -Between steps 1 and 2 the two branches **do** differ, and that is expected, not drift: `v2/main` reads the version being built while `main` still reads the one currently released. What this ordering removes is *post-release* drift — once the milestone merge lands they agree again, and `v2/main` is never left **behind** `main`. If you see `v2/main` ahead of `main`, a release is in flight; if you see it behind, something went wrong. +Between steps 1 and 2 the two branches **do** differ, and that is expected, not drift: `v2/main` reads the version being built while `main` still reads the one currently released. What this ordering removes is _post-release_ drift — once the milestone merge lands they agree again, and `v2/main` is never left **behind** `main`. If you see `v2/main` ahead of `main`, a release is in flight; if you see it behind, something went wrong. **3. Tag the `main` commit and draft the Release:** @@ -419,7 +419,7 @@ git tag 2.3.0 origin/main && git push origin 2.3.0 The release's target commit selects which workflow runs, so this only publishes when a release is cut from a commit carrying this (v2) workflow. -**Why the bump goes on `v2/main` first ([#2010](https://github.com/modelcontextprotocol/inspector/issues/2010)).** It used to happen on the milestone-merge branch, which is cut from `main` — so the bump existed only *downstream* of `v2/main` and nothing carried it back. `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases. That is not cosmetic: a branch cut from a milestone-merge branch silently carries the bump into an unrelated PR (this happened on [#2009](https://github.com/modelcontextprotocol/inspector/issues/2009), where a container bugfix arrived with a `2.0.0 → 2.2.0` diff), and anything reading the version in development — `readInspectorVersion()`, `--version`, `GET /api/config` — reported a version two releases old. +**Why the bump goes on `v2/main` first ([#2010](https://github.com/modelcontextprotocol/inspector/issues/2010)).** It used to happen on the milestone-merge branch, which is cut from `main` — so the bump existed only _downstream_ of `v2/main` and nothing carried it back. `v2/main` sat at `2.0.0` through both the 2.1.0 and 2.2.0 releases. That is not cosmetic: a branch cut from a milestone-merge branch silently carries the bump into an unrelated PR (this happened on [#2009](https://github.com/modelcontextprotocol/inspector/issues/2009), where a container bugfix arrived with a `2.0.0 → 2.2.0` diff), and anything reading the version in development — `readInspectorVersion()`, `--version`, `GET /api/config` — reported a version two releases old. Do **not** "fix" a future drift by merging `main` back into `v2/main`. `main` carries the entire pre-v2 v1 history (retained through `ec5d8e13 chore: replace main's tree with v2` — ~230 commits `v2/main` does not have), so a back-merge grafts all of it into the develop branch's log permanently in order to deliver a two-file change. Bumping first means there is nothing to back-merge. @@ -457,17 +457,17 @@ docker run --rm -p 127.0.0.1:6274:6274 \ The same volume also persists OAuth tokens and stored state, so an authorized server stays authorized across runs. Use `-e MCP_CATALOG_PATH=/some/other/path.json` to put the catalog somewhere else — mount a volume covering whatever directory you point it at. If you **bind-mount a host directory** instead of a named volume (`-v "$PWD/inspector-data:/home/node/.mcp-inspector"`), the directory keeps its host ownership, so on Linux add `--user "$(id -u):$(id -g)"` or `chown` it to uid `1000` — otherwise the non-root `node` user can't write and adding a server fails with `EACCES`. -**Where secrets go, and how to make them survive (#1950).** The Inspector keeps the values it deliberately does *not* write to `mcp.json` — an OAuth client secret, an enterprise IdP client secret, each stdio `env:` value — in the **OS keychain**. A container has no keychain (the published image has no D-Bus session), so on startup the Inspector probes for one and falls back, saying so in the logs and in a permanent footer at the bottom of the Client Settings and Server Settings dialogs. Which fallback you get depends on whether the directory it would write to is going to survive: +**Where secrets go, and how to make them survive (#1950).** The Inspector keeps the values it deliberately does _not_ write to `mcp.json` — an OAuth client secret, an enterprise IdP client secret, each stdio `env:` value — in the **OS keychain**. A container has no keychain (the published image has no D-Bus session), so on startup the Inspector probes for one and falls back, saying so in the logs and in a permanent footer at the bottom of the Client Settings and Server Settings dialogs. Which fallback you get depends on whether the directory it would write to is going to survive: -| Situation | Store | Secrets survive a restart? | -| --- | --- | --- | -| Keychain reachable (a normal desktop install) | OS keychain | Yes | -| Container, **no volume** on `/home/node/.mcp-inspector` | Memory | No — session only | -| Container **with** that volume, or any host without a keychain | `~/.mcp-inspector/secrets.json`, mode `0600` | Yes | +| Situation | Store | Secrets survive a restart? | +| -------------------------------------------------------------- | -------------------------------------------- | -------------------------- | +| Keychain reachable (a normal desktop install) | OS keychain | Yes | +| Container, **no volume** on `/home/node/.mcp-inspector` | Memory | No — session only | +| Container **with** that volume, or any host without a keychain | `~/.mcp-inspector/secrets.json`, mode `0600` | Yes | So the same volume that keeps your server list also switches secrets from session-scoped to durable — nothing extra to configure. The in-memory default for an unmounted container is deliberate: a file in the writable layer is discarded by `--rm` and by every image update, and promising durability it can't deliver is worse than declining to. -**A file-backed store is unencrypted unless you give it a key.** Set `MCP_INSPECTOR_SECRET_KEY` to any passphrase and the file is encrypted with AES-256-GCM (the passphrase is stretched with scrypt against a per-file random salt). Without it the file is still `0600`, but the values are readable to anyone who can read the file — which the startup log and the settings footer both say, every session, in a warning tone: +**A file-backed store is unencrypted unless you give it a key.** Set `MCP_INSPECTOR_SECRET_KEY` and the file is encrypted with AES-256-GCM (the passphrase is stretched with scrypt against a per-file random salt). Without it the file is still `0600`, but the values are readable to anyone who can read the file — which the startup log and the settings footer both say, every session, in a warning tone: ```bash docker run --rm -p 127.0.0.1:6274:6274 \ @@ -476,9 +476,11 @@ docker run --rm -p 127.0.0.1:6274:6274 \ ghcr.io/modelcontextprotocol/inspector ``` -Setting the passphrase later is safe — the next write upgrades an existing plaintext file in place. **Changing or losing it is not**: a file that can no longer be decrypted is read as empty and *refuses to be written*, rather than being silently replaced with a new one holding only your latest secret. Restore the original passphrase, or delete `secrets.json` and re-enter the values. +**Use a high-entropy passphrase — generated, not chosen.** The random salt stops an attacker precomputing a table across files; it does nothing against _guessing_, and the scrypt cost is deliberately low because the derivation runs on every read and write. Anyone who obtains `secrets.json` can therefore test candidate passphrases quickly and offline, so treat this value like any other credential rather than like a memorable password. -Two env vars override the automatic choice: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, and `MCP_INSPECTOR_SECRET_FILE` moves the file. +Setting the passphrase later is safe — the next write upgrades an existing plaintext file in place. Until that write happens the existing values really are still readable, and the banner and footer keep saying so rather than reporting the file as encrypted the moment the variable appears. **Changing or losing the passphrase is not safe**: a file that can no longer be decrypted is read as empty and _refuses to be written_, rather than being silently replaced with a new one holding only your latest secret. Restore the original passphrase, or delete `secrets.json` and re-enter the values. + +Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there. **Upgrading from an image before this fix?** Earlier images did not create `/home/node/.mcp-inspector`, so Docker created the volume's mount point as `root` and the non-root `node` user couldn't write to it. An **empty** volume repairs itself on the first run of a current image (Docker applies the image directory's ownership to an empty volume), but one that already has files in it keeps its old `root` ownership and still fails with `EACCES`. Fix it once: diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx index 252e6a66..008787f8 100644 --- a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -66,6 +66,15 @@ const FooterDetail = Text.withProps({ flex: 1, }); +// `multiline` + a fixed width so the sentence wraps instead of forming a +// tooltip wider than the modal it explains. Only the `label` is dynamic. +const CaveatTooltip = Tooltip.withProps({ + multiline: true, + w: 320, + position: "top", + withArrow: true, +}); + const ICON_SIZE = 14; /** @@ -129,11 +138,5 @@ export function SecretStorageFooter({ info }: SecretStorageFooterProps) { const tip = [caveat, info.path].filter(Boolean).join(" "); if (!tip) return band; - return ( - // `multiline` + a width so the sentence wraps instead of forming a tooltip - // wider than the modal it explains. - - {band} - - ); + return {band}; } diff --git a/clients/web/src/test/core/auth/secret-storage-info.test.ts b/clients/web/src/test/core/auth/secret-storage-info.test.ts index 8c55c157..1d60a92d 100644 --- a/clients/web/src/test/core/auth/secret-storage-info.test.ts +++ b/clients/web/src/test/core/auth/secret-storage-info.test.ts @@ -87,6 +87,22 @@ describe("secretStorageCaveat", () => { expect(caveat).toContain("MCP_INSPECTOR_SECRET_KEY"); }); + it("changes the advice once a passphrase is set but not yet applied", () => { + // Telling someone to set MCP_INSPECTOR_SECRET_KEY when they already have + // is advice that cannot clear the condition. The condition here is the + // pending write, so that is what the sentence names — while the verdict + // (still unencrypted, still a warning) stays exactly the same. + const pending: SecretStorageInfo = { + ...plaintextFile, + pendingEncryption: true, + }; + expect(secretStorageTone(pending)).toBe("warn"); + expect(secretStorageLabel(pending)).toBe("File (unencrypted)"); + const caveat = secretStorageCaveat(pending); + expect(caveat).toContain("next time a secret is saved"); + expect(caveat).not.toContain("MCP_INSPECTOR_SECRET_KEY"); + }); + it("prefers the memory caveat when a memory store somehow carries the flag", () => { // Defensive ordering: `plaintext` is meaningless for memory, and the // durability loss is the more consequential of the two statements. diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index ecf3aad4..ac70b6a1 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -289,6 +289,27 @@ describe("useInitialConfig", () => { ["null", null], ["an unknown kind", { kind: "vault", durable: true }], ["a missing kind", { durable: true, path: "/x" }], + // The dangerous one: `kind` alone passed the old check, and a missing + // `plaintext` is falsy, so it rendered as the quiet *encrypted* file — + // the most misleading thing this footer can say, produced by the absence + // of information rather than by anything the backend claimed. + ["a file descriptor with nothing but a kind", { kind: "file" }], + [ + "a file descriptor missing its path", + { kind: "file", reason: "fallback", durable: true, plaintext: false }, + ], + [ + "a file descriptor missing plaintext", + { kind: "file", reason: "fallback", durable: true, path: "/x" }, + ], + [ + "a descriptor with an unknown reason", + { kind: "keyring", reason: "vibes", durable: true }, + ], + [ + "a descriptor with a non-boolean durable", + { kind: "memory", reason: "fallback", durable: "no" }, + ], ])("rejects %s", async (_label, value) => { const fetchFn = vi .fn() diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts index 96152517..369546fa 100644 --- a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -225,6 +225,31 @@ describe("FileSecretStore failure handling", () => { ); }); + it("refuses a file written by a newer format version", async () => { + // Readable as far as the plaintext branch is concerned, which is the trap: + // accepting it means the next write rewrites it as version 1 and silently + // drops whatever the newer format added. Same refusal the encrypted branch + // already gives, so the answer doesn't depend on which mode the newer + // writer happened to choose. + await fs.writeFile( + filePath(), + JSON.stringify({ + version: 2, + encryption: "none", + secrets: { "a:b": "1" }, + }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.get("a", "b")).toBe(null); + await expect(store.set("a", "b", "2")).rejects.toThrow(/version 2/); + // The bytes are still there — nothing was destroyed to answer the write. + const raw = JSON.parse(await fs.readFile(filePath(), "utf-8")) as { + version: number; + }; + expect(raw.version).toBe(2); + }); + it("delete stays silent on a file it cannot decrypt", async () => { await writeEncryptedFixture(); const store = new FileSecretStore({ @@ -246,15 +271,34 @@ describe("FileSecretStore failure handling", () => { expect(await store.get("alpha", "env:A")).toBe(null); }); - it("set reports a file written in a format this build doesn't know", async () => { + it("set reports a file written by a newer format version", async () => { // The forward-compat case: a newer Inspector's file, opened by an older // one. Refusing beats truncating it to the entries we happen to parse. + // The version is checked before the encryption mode, so this is the + // message even though the cipher is also unrecognized — the version is + // the more actionable of the two ("upgrade"), and it is the reason the + // file is unreadable regardless of what the cipher turned out to be. await fs.writeFile( filePath(), JSON.stringify({ version: 99, encryption: "chacha20-poly1305" }), "utf-8", ); const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /format version 99/, + ); + }); + + it("set reports an unrecognized cipher at a version it does understand", async () => { + // The other half: a file this build's version check accepts, whose + // encryption mode it has no code for. A different failure with different + // advice, so it must not collapse into the version message. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "chacha20-poly1305" }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( /unsupported format/, ); @@ -409,3 +453,48 @@ describe("tightenSecretFilePermissions", () => { ).resolves.toBeUndefined(); }); }); + +describe("readOnDiskEncryption", () => { + it("reports what the file is, not what the store would write", async () => { + // The whole reason it exists: these two disagree for a full session after + // a passphrase is added to an install that already has a plaintext file. + const plain = new FileSecretStore({ filePath: filePath() }); + await plain.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + + const withKey = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + expect(withKey.encrypted).toBe(true); + expect(await withKey.readOnDiskEncryption()).toBe("none"); + + await withKey.set("alpha", "env:A", "1"); + expect(await withKey.readOnDiskEncryption()).toBe("aes-256-gcm"); + }); + + it("answers null for an absent, unparseable, or unrecognized file", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.readOnDiskEncryption()).toBe(null); + + await fs.writeFile(filePath(), "{not json", "utf-8"); + expect(await store.readOnDiskEncryption()).toBe(null); + + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "rot13" }), + "utf-8", + ); + expect(await store.readOnDiskEncryption()).toBe(null); + }); + + it("needs no passphrase — it reads the envelope, never the payload", async () => { + // A store whose key is wrong still has to be describable in the UI. + const writer = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + await writer.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + const keyless = new FileSecretStore({ filePath: filePath() }); + expect(await keyless.readOnDiskEncryption()).toBe("aes-256-gcm"); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 90b96bfe..b77dec18 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -33,6 +33,7 @@ const ENV_KEYS = [ "MCP_INSPECTOR_SECRET_STORE", "MCP_INSPECTOR_SECRET_FILE", "MCP_INSPECTOR_SECRET_KEY", + "MCP_STORAGE_DIR", "KUBERNETES_SERVICE_HOST", ]; @@ -381,4 +382,86 @@ describe("defaultSecretFilePath", () => { path.join(".mcp-inspector", "secrets.json"), ); }); + + it("follows MCP_STORAGE_DIR when no explicit file is named", async () => { + // The variable that already relocates OAuth tokens and client.json. A + // container mounting only that directory must not be judged unmounted and + // demoted to `memory` — the user arranged for exactly this directory to + // survive. + process.env.MCP_STORAGE_DIR = tmpDir; + const mod = await loadWithProbe(true); + expect(mod.defaultSecretFilePath()).toBe(path.join(tmpDir, "secrets.json")); + }); + + it("lets an explicit MCP_INSPECTOR_SECRET_FILE outrank MCP_STORAGE_DIR", async () => { + // Naming the file outright is the more specific statement of the two. + process.env.MCP_STORAGE_DIR = tmpDir; + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "elsewhere.json"); + const mod = await loadWithProbe(true); + expect(mod.defaultSecretFilePath()).toBe( + path.join(tmpDir, "elsewhere.json"), + ); + }); +}); + +describe("the descriptor reports the file, not the write policy", () => { + it("says plaintext while a pre-existing plaintext file is only *going* to be encrypted", async () => { + // The transitional state: the passphrase is set, so the next write will + // encrypt, but the bytes on disk are still readable. Reporting "File + // (encrypted)" here is the one false reassurance this subsystem exists to + // avoid. + const filePath = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const plain = await loadWithProbe(false); + await plain.defaultSecretStore().set("alpha", "env:A", "1"); + + // Same file, now with a passphrase configured. + process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2"; + const withKey = await loadWithProbe(false); + const info = await withKey.getSecretStorageInfo(); + expect(info.plaintext).toBe(true); + expect(info.pendingEncryption).toBe(true); + + // And the advice changes with it: telling someone to set a passphrase + // they have already set is worse than saying nothing. + const { secretStorageCaveat, secretStorageLabel } = + await import("@inspector/core/auth/secret-storage-info.js"); + expect(secretStorageLabel(info)).toBe("File (unencrypted)"); + expect(secretStorageCaveat(info)).toMatch(/next time a secret is saved/); + }); + + it("says encrypted once the upgrading write has actually landed", async () => { + const filePath = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const plain = await loadWithProbe(false); + await plain.defaultSecretStore().set("alpha", "env:A", "1"); + + process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2"; + const withKey = await loadWithProbe(false); + await withKey.defaultSecretStore().set("alpha", "env:B", "2"); + + const after = await loadWithProbe(false); + const info = await after.getSecretStorageInfo(); + expect(info.plaintext).toBe(false); + expect(info.pendingEncryption).toBeUndefined(); + }); + + it("falls back to the write policy when there is no file yet", async () => { + // Nothing on disk to describe, and the policy is then accurate: the first + // write creates the file in exactly that mode. + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "absent.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(false); + const info = await mod.getSecretStorageInfo(); + expect(info.plaintext).toBe(false); + expect(info.pendingEncryption).toBeUndefined(); + }); }); diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index a61e2fc2..7c5e419d 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -32,8 +32,16 @@ * * **Key.** `MCP_INSPECTOR_SECRET_KEY` is a passphrase, not a key: it is * stretched with scrypt against a per-file random salt stored beside the - * ciphertext. That means a short, memorable value is safe to use, and it - * means the same passphrase produces a different key for a different file. + * ciphertext, so the same passphrase produces a different key for a + * different file and a precomputed table buys an attacker nothing. + * + * That is **not** a licence to use a short, memorable one. The salt + * defeats precomputation; it does nothing against guessing, and the cost + * parameters below are deliberately cheap (a few milliseconds, since the + * derivation runs on every read and write). Anyone who steals + * `secrets.json` can therefore try candidate passphrases quickly and + * offline. Use a high-entropy value — generated, not chosen — the same as + * you would for any other credential. * * **Availability contract — identical to `KeyringSecretStore`'s**, because * callers must not have to know which store they got. `get` is tolerant @@ -183,11 +191,47 @@ export class FileSecretStore implements SecretStore { this.passphrase = raw && raw.trim() ? raw : undefined; } - /** True when this store encrypts. Drives the `plaintext` descriptor flag. */ + /** + * True when the *next write* will encrypt. This is the configured policy, + * not the state of the file — see {@link readOnDiskEncryption}, which is + * what the user-facing descriptor is built from. + */ get encrypted(): boolean { return this.passphrase !== undefined; } + /** + * What the file on disk is *currently* written as — `null` when there is + * no file yet, or when it cannot be read or parsed. + * + * Separate from {@link encrypted} because the two genuinely disagree for a + * whole session: adding `MCP_INSPECTOR_SECRET_KEY` to an install that + * already has a plaintext file flips `encrypted` to true immediately, + * while the bytes stay readable until the next `set`. A descriptor built + * from the policy would tell that user "File (encrypted)" while their + * existing secrets sat in the clear — the precise reassurance this whole + * subsystem exists not to give. + * + * Reads the envelope only. It never decrypts, so it needs no passphrase + * and answers correctly even when the key is wrong or missing — which is + * also a state the UI has to describe rather than throw on. + */ + async readOnDiskEncryption(): Promise { + try { + const raw = await readStoreFile(this.filePath); + if (raw === null) return null; + const parsed = JSON.parse(raw) as SecretFile; + return parsed.encryption === "none" || parsed.encryption === CIPHER + ? parsed.encryption + : null; + } catch { + // Absent, unreadable, or not JSON. "Unknown" is the honest answer, and + // the caller falls back to reporting the configured policy — the only + // thing that is knowable about a file nobody can read. + return null; + } + } + private async deriveKey(kdf: KdfParams): Promise { return scrypt( // Only reached when the passphrase is set — every caller checks first, @@ -221,10 +265,26 @@ export class FileSecretStore implements SecretStore { ); } + // Version first, before either branch. A `{ version: 2, encryption: + // "none" }` file is readable as far as the plaintext branch is concerned, + // but reading it means the next write rewrites it as version 1 — silently + // discarding whatever fields version 2 added. That is exactly the + // destroy-to-satisfy-an-additive-request case the encrypted branch already + // refuses, so it gets the same answer rather than a different one that + // happens to depend on which encryption mode the newer writer chose. + if (parsed.version !== FORMAT_VERSION) { + throw new SecretStoreUnavailableError( + `The secrets file at ${this.filePath} declares format version ${String(parsed.version)}, but this Inspector only understands version ${FORMAT_VERSION}. It was probably written by a newer Inspector; upgrade, or move the file aside to start over.`, + ); + } + if (parsed.encryption === "none") { // A plaintext file read by a store that now has a passphrase is fine and // deliberately not an error: the next write re-encrypts it, so setting - // the env var upgrades an existing file in place. + // the env var upgrades an existing file in place. Until that write + // lands the file really is still readable, which is why + // `readOnDiskEncryption` exists — the descriptor must report the file, + // not the intent. return { ...(parsed.secrets ?? {}) }; } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 9f07d903..a135d3d1 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -61,9 +61,19 @@ import { /** Env var naming an explicit store, bypassing the probe entirely. */ export const SECRET_STORE_ENV = "MCP_INSPECTOR_SECRET_STORE"; -/** Env var overriding the secrets file location. */ +/** Env var overriding the secrets file location outright. */ export const SECRET_FILE_ENV = "MCP_INSPECTOR_SECRET_FILE"; +/** + * Env var relocating the Inspector's durable state directory. Already + * honored by the web backend for OAuth tokens and `client.json`; the + * secrets file follows it for the same reason, and because the mount check + * below asks whether *that* directory survives. A container mounting only + * the configured storage directory would otherwise be judged unmounted, + * fall back to `memory`, and lose secrets the user had arranged to keep. + */ +export const STORAGE_DIR_ENV = "MCP_STORAGE_DIR"; + const KINDS: SecretStoreKind[] = ["keyring", "file", "memory"]; export interface ResolvedSecretStore { @@ -89,10 +99,16 @@ export function parseSecretStoreEnv( return undefined; } -/** Default secrets file: beside the catalog, in `~/.mcp-inspector`. */ +/** + * Default secrets file: `MCP_INSPECTOR_SECRET_FILE` if named outright, + * otherwise `secrets.json` in the configured storage directory, otherwise + * beside the catalog in `~/.mcp-inspector`. + */ export function defaultSecretFilePath(): string { const override = process.env[SECRET_FILE_ENV]?.trim(); if (override) return path.resolve(override); + const storageDir = process.env[STORAGE_DIR_ENV]?.trim(); + if (storageDir) return path.resolve(storageDir, "secrets.json"); const homeDir = process.env.HOME || process.env.USERPROFILE || "."; return path.join(homeDir, ".mcp-inspector", "secrets.json"); } @@ -172,11 +188,11 @@ export function isOnMountPoint(dir: string): boolean { } /** Build the store named by `kind`, plus the descriptor that explains it. */ -function buildStore( +async function buildStore( kind: SecretStoreKind, reason: SecretStorageInfo["reason"], detail?: string, -): ResolvedSecretStore { +): Promise { if (kind === "keyring") { return { store: new KeyringSecretStore(), @@ -196,13 +212,24 @@ function buildStore( // every failure), and blocking selection on a chmod would make an // unwritable file a startup problem rather than a `set`-time one. void tightenSecretFilePermissions(filePath); + // Report the FILE, not the intent. `store.encrypted` is what the next write + // will do; a file already on disk in the clear stays readable until that + // write happens, and saying "File (encrypted)" in the meantime is the one + // false reassurance this footer must never give. With no file yet (or one we + // cannot parse) the policy is all there is to report, and it is then + // accurate — the first write creates the file in that mode. + const onDisk = await store.readOnDiskEncryption(); + const plaintext = onDisk === null ? !store.encrypted : onDisk === "none"; return { store, info: { kind, reason, path: filePath, - plaintext: !store.encrypted, + plaintext, + // Only meaningful while the two disagree; omitted otherwise so the + // payload does not carry a flag that says nothing. + ...(plaintext && store.encrypted ? { pendingEncryption: true } : {}), durable: true, detail, }, @@ -260,7 +287,7 @@ export function resolveSecretStore(): Promise { resolved ??= (async (): Promise => { const configured = parseSecretStoreEnv(process.env[SECRET_STORE_ENV]); if (configured) { - const result = buildStore(configured, "configured"); + const result = await buildStore(configured, "configured"); warnAboutSecretStorage(result.info); return result; } @@ -271,7 +298,7 @@ export function resolveSecretStore(): Promise { container: isContainer(), mounted: isOnMountPoint(path.dirname(defaultSecretFilePath())), }); - const result = buildStore(kind, "fallback", probe.detail); + const result = await buildStore(kind, "fallback", probe.detail); warnAboutSecretStorage(result.info); return result; })(); diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts index 054cf3b4..38c9ca79 100644 --- a/core/auth/secret-storage-info.ts +++ b/core/auth/secret-storage-info.ts @@ -42,12 +42,26 @@ export interface SecretStorageInfo { */ path?: string; /** - * True when a file-backed store is writing secrets in the clear (no - * `MCP_INSPECTOR_SECRET_KEY`). Always false for the other kinds — a - * keychain is opaque to us and RAM is not "at rest" in the sense this - * flag is about. + * True when the secrets file is *currently* in the clear. Read off the + * file's own envelope rather than off whether a passphrase is + * configured, because those two disagree for a whole session: adding + * `MCP_INSPECTOR_SECRET_KEY` to an install that already has a plaintext + * file makes the next write encrypt, while the existing bytes stay + * readable until then. Reporting the intent would tell that user their + * secrets were encrypted while they were not. + * + * Always false for the other kinds — a keychain is opaque to us and RAM + * is not "at rest" in the sense this flag is about. */ plaintext?: boolean; + /** + * True in exactly the transitional state above: `plaintext` is still + * true, but a passphrase is set and the next write will encrypt. It + * changes the *advice* rather than the verdict — "set a passphrase" is + * the wrong thing to tell someone who already has, so the caveat says + * what will actually clear the condition. + */ + pendingEncryption?: boolean; /** * True when secrets outlive the process. False only for `memory`, and * it is the *promise* being made rather than an implementation detail: @@ -104,7 +118,9 @@ export function secretStorageCaveat( return "Secrets are not written anywhere and are lost when the Inspector exits."; } if (info.plaintext) { - return "Secrets are stored unencrypted (file mode 0600). Set MCP_INSPECTOR_SECRET_KEY to encrypt them."; + return info.pendingEncryption + ? "Existing secrets in this file are still unencrypted (file mode 0600). They are re-encrypted the next time a secret is saved." + : "Secrets are stored unencrypted (file mode 0600). Set MCP_INSPECTOR_SECRET_KEY to encrypt them."; } return undefined; } diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index a17ecbc8..88c26c5c 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -71,15 +71,34 @@ interface ConfigPayload { * * The footer states, in the UI, where a user's secret is about to go, so a * malformed or partial descriptor must render as "unknown" rather than as a - * confident half-answer — the `kind` alone is what every label and tone - * derives from, so it is the field the check turns on. + * confident half-answer. + * + * Every field the footer reads is checked, not just `kind`. A partial + * `{ kind: "file" }` would otherwise pass, and because a missing `plaintext` + * is falsy it would render as the quiet, neutral *encrypted* file — the + * single most misleading thing this component can say, produced by the + * absence of information rather than by any claim the backend made. Each + * kind's own required fields are checked for the same reason: a file store + * with no `path` cannot answer the question a file store exists to answer. */ function usableSecretStorage(value: unknown): SecretStorageInfo | undefined { if (typeof value !== "object" || value === null) return undefined; - const kind = (value as { kind?: unknown }).kind; - if (kind !== "keyring" && kind !== "file" && kind !== "memory") { + const v = value as Record; + if (v.kind !== "keyring" && v.kind !== "file" && v.kind !== "memory") { return undefined; } + if ( + v.reason !== "configured" && + v.reason !== "default" && + v.reason !== "fallback" + ) { + return undefined; + } + if (typeof v.durable !== "boolean") return undefined; + if (v.kind === "file") { + if (typeof v.path !== "string" || v.path === "") return undefined; + if (typeof v.plaintext !== "boolean") return undefined; + } return value as SecretStorageInfo; }