modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

Merge pull request #2076 from modelcontextprotocol/v2/feat/1950-file-secret-store

Browse source 
feat(auth): file-backed and in-memory SecretStore for hosts with no keychain
This commit is contained in:
Cliff Hall 2026-08-23 02:01:27 -04:00 • committed by GitHub
commit 7701c68028
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
42 changed files with 8429 additions and 303 deletions
Download patch file Download diff file Expand all files Collapse all files

24
AGENTS.md
View file

@ -51,10 +51,30 @@ v2/main/
│ │ # wrapper that rewrites the discovered AS
│ │ # metadata document, the one seam SDK v2 routes
│ │ # BOTH endpoints through (neither reaches the
│ │ # OAuthClientProvider) — #1906)
│ │ # OAuthClientProvider) — #1906;
│ │ # secret-storage-info.ts browser-safe
│ │ # descriptor of WHERE a typed secret lands
│ │ # — kind/plaintext/durable plus the label,
│ │ # tone, caveat and summary helpers shared
│ │ # by the startup banner and the web UI, so
│ │ # terminal and browser cannot describe the
│ │ # same store differently — #1950)
│ │ ├── browser/ # Browser-side OAuth (sessionStorage, BrowserNavigation)
│ │ ├── node/ # Node-side OAuth (NodeOAuthStorage, OAuthCallbackServer,
│ │ │ # runner-interactive-oauth loopback callback flow)
│ │ │ # runner-interactive-oauth loopback callback flow);
│ │ │ # plus the SecretStore backends and their selection —
│ │ │ # secret-store.ts (KeyringSecretStore + InMemorySecretStore,
│ │ │ # the SecretStoreUnavailableError base every store's `set`
│ │ │ # throws and the routes turn into a 503, and the keychain
│ │ │ # probe), file-secret-store.ts (0600 JSON, AES-256-GCM when
│ │ │ # MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a
│ │ │ # file it cannot decrypt rather than destroying it), and
│ │ │ # secret-store-selection.ts (the POLICY: explicit
│ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain,
│ │ │ # else fall back LOUDLY — to memory in a container with
│ │ │ # nothing mounted, to file otherwise; cached per process so
│ │ │ # the banner, /api/config and the store doing the writing
│ │ │ # cannot disagree — #1950)
│ │ └── remote/ # Remote OAuth storage (delegates to the remote server)
│ ├── client/ # Install-level client config (`client.json`): browser-safe
│ │ # parse/validate (config-parse.ts) + Node load/save

71
README.md
View file

@ -30,7 +30,9 @@ inspector/
│ ├── tui/ # TUI client (Ink + React, tsup bundle)
│ └── launcher/ # Shared launcher — provides the `mcp-inspector` bin, dispatches to web/cli/tui
├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json)
│ ├── auth/ # OAuth: providers, discovery, storage, endpoint overrides, mid-session recovery (browser/node/remote backends)
│ ├── auth/ # OAuth: providers, discovery, storage, endpoint overrides, mid-session recovery (browser/node/remote backends);
│ │ # plus per-server secret storage — the keychain/file/memory SecretStore
│ │ # implementations, the selection policy, and the descriptor the banner and UI report
│ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets
│ ├── json/ # JSON + parameter/argument conversion utilities, and the nullable-union
│ │ # schema collapse shared by the web and TUI form builders
@ -137,8 +139,8 @@ 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) |
| `app-elicitation-http.json` **(legacy era)** | An MCP App rendering a form elicitation | [#1854](https://github.com/modelcontextprotocol/inspector/issues/1854) |
| `modern-mrtr-http.json` | A single MRTR round-trip | — |
@ -150,10 +152,10 @@ Each config below is a ready-made server for exercising one feature by hand. Loa
| `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
@ -284,23 +286,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`).
@ -389,7 +391,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 `<runner dir>/../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
@ -421,7 +423,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:**
@ -437,7 +439,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.
@ -475,6 +477,37 @@ 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:
| 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` 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 \
-v mcp-inspector-data:/home/node/.mcp-inspector \
-e MCP_INSPECTOR_SECRET_KEY="$MY_PASSPHRASE" \
ghcr.io/modelcontextprotocol/inspector
```
**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.
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.
The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true.
**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — there is deliberately **no lock**: writers are allowed to collide and the loser is made to notice. Each mutation reads the file, applies its change, writes, then reads back and compares the whole map; if another process wrote in between, it re-applies onto what they left and retries, and after five lost rounds it fails loudly rather than returning as though the value were saved.
This is **not mutual exclusion**, and the residual case is worth stating: the verify only catches a clobber that has already landed, so if one Inspector reads back *before* the other's write arrives, both report success and one value is gone. That needs two Inspectors writing the same file within the gap between one's write and its read-back — narrow, but not only a crash. An earlier build did take a lock (`secrets.json.lock`); it was removed because making a `mkdir` lock single-winner on a stale takeover needs a compare-and-swap on a directory entry that Node does not expose, so it had the same class of failure with several hundred more lines and no way to close it.
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:
```bash

14
clients/web/server/server.ts
View file

@ -18,6 +18,7 @@ import { formatHostForUrl } from "../../../core/node/hostUrl.ts";
import { createSandboxController } from "./sandbox-controller.js";
import { injectAuthToken } from "./inject-auth-token.js";
import type { WebServerConfig } from "./web-server-config.js";
import { getSecretStorageInfo } from "../../../core/auth/node/secret-store-selection.ts";
import {
webServerConfigToInitialPayload,
printServerBanner,
@ -49,6 +50,11 @@ export async function startHonoServer(
(config.dangerouslyOmitAuth ? "" : randomBytes(32).toString("hex"));
const rootPath = config.staticRoot ?? __dirname;
// Resolve the secret store before the API is built so the descriptor the
// browser reads (`/api/config`) and the one the banner prints come from the
// same resolution — the selection is cached, so this is also the only place
// the keychain probe actually runs.
const secretStorage = await getSecretStorageInfo();
const { app: apiApp, close: closeApi } = createRemoteApp({
authToken: config.dangerouslyOmitAuth ? undefined : resolvedAuthToken,
@ -60,7 +66,12 @@ export async function startHonoServer(
allowedOrigins: config.allowedOrigins,
sandboxUrl: sandboxController.getUrl() ?? undefined,
logger: config.logger,
initialConfig: webServerConfigToInitialPayload(config),
initialConfig: webServerConfigToInitialPayload(config, secretStorage),
// The startup value above is what the banner printed; the route
// re-resolves per request, because the first write under a newly-set
// passphrase encrypts a pre-existing plaintext file and a captured
// descriptor would keep reporting the old state until a restart.
secretStorageResolver: getSecretStorageInfo,
});
const app = new Hono();
@ -123,6 +134,7 @@ export async function startHonoServer(
info.port,
resolvedAuthToken,
sandboxUrl ?? undefined,
secretStorage,
);
if (config.autoOpen) {
// Never a bare `open(url)`: this callback is synchronous, so a

12
clients/web/server/vite-hono-plugin.ts
View file

@ -18,6 +18,7 @@ import { createRemoteApp } from "../../../core/mcp/remote/node/server.ts";
import { createSandboxController } from "./sandbox-controller.js";
import { injectAuthToken } from "./inject-auth-token.js";
import type { WebServerConfig } from "./web-server-config.js";
import { getSecretStorageInfo } from "../../../core/auth/node/secret-store-selection.ts";
import {
webServerConfigToInitialPayload,
printServerBanner,
@ -66,6 +67,9 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin {
allowedOrigins: config.allowedOrigins,
});
await sandboxController.start();
// Resolved before the API is built so `/api/config` and the banner
// report the same store (the selection itself is cached process-wide).
const secretStorage = await getSecretStorageInfo();
const {
app: honoApp,
@ -81,7 +85,12 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin {
allowedOrigins: config.allowedOrigins,
sandboxUrl: sandboxController.getUrl() ?? undefined,
logger: config.logger,
initialConfig: webServerConfigToInitialPayload(config),
initialConfig: webServerConfigToInitialPayload(config, secretStorage),
// The startup value above is what the banner printed; the route
// re-resolves per request, because the first write under a newly-set
// passphrase encrypts a pre-existing plaintext file and a captured
// descriptor would keep reporting the old state until a restart.
secretStorageResolver: getSecretStorageInfo,
});
// Expose the resolved token to `transformIndexHtml`. Left empty when
@ -111,6 +120,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin {
actualPort,
resolvedToken,
sandboxUrl ?? undefined,
secretStorage,
);
if (config.autoOpen) {

22
clients/web/server/web-server-config.ts
View file

@ -15,6 +15,8 @@ import {
LEGACY_AUTH_TOKEN_ENV,
} from "../../../core/mcp/remote/constants.ts";
import type { InitialConfigPayload } from "../../../core/mcp/remote/node/server.ts";
import type { SecretStorageInfo } from "../../../core/auth/secret-storage-info.ts";
import { secretStorageSummary } from "../../../core/auth/secret-storage-info.ts";
import { readInspectorVersionSafe } from "../../../core/node/version.ts";
import { resolveSandboxPort } from "./sandbox-controller.js";
import { resolveBindHostname } from "./resolve-bind-host.js";
@ -112,8 +114,17 @@ function defaultEnvironmentFromProcess(
*/
export function webServerConfigToInitialPayload(
config: WebServerConfig,
secretStorage?: SecretStorageInfo,
): InitialConfigPayload {
return { ...transportDefaults(config), version: inspectorVersion };
return {
...transportDefaults(config),
version: inspectorVersion,
// Omitted rather than defaulted when the caller didn't resolve a store:
// the UI treats an absent descriptor as "unknown" and shows nothing,
// which is the only honest rendering. A default of "keychain" would put a
// confident wrong answer under the field where a secret is typed.
...(secretStorage ? { secretStorage } : {}),
};
}
/** The transport-specific defaults half of the `/api/config` payload. */
@ -168,6 +179,7 @@ export function printServerBanner(
actualPort: number,
resolvedToken: string,
sandboxUrl: string | undefined,
secretStorage?: SecretStorageInfo,
): string {
// Advertise `localhost` for a wildcard bind (`http://0.0.0.0:PORT` is an
// awkward URL to click and points the user at a reachable, allow-listed
@ -197,6 +209,14 @@ export function printServerBanner(
console.log(` Auth token: ${resolvedToken}\n`);
}
// Where a secret typed into this session lands (#1950). Printed for every
// store, not only the surprising ones: the fallback already warns
// separately at selection time, and a line that appears only when
// something is wrong can't be used to confirm that nothing is.
if (secretStorage) {
console.log(` Secrets: ${secretStorageSummary(secretStorage)}\n`);
}
if (config.autoOpen) {
console.log("Opening browser...");
}

10
clients/web/src/App.css
View file

@ -24,6 +24,13 @@
light surfaces; a lighter red on the dark card/header (see dark override) —
`red-6` fell just under WCAG AA on both. */
--inspector-danger-text: var(--mantine-color-red-7);
/* Caution text (the secret-storage footer when secrets are unencrypted or
session-only, #1950). Yellow is the right *meaning* — this is "know what
you are doing", not "this failed" — but yellow is also the hardest hue to
make readable: `yellow-6`, the tone the log-level badges use on a filled
chip, is far under AA as body text on a light surface. `yellow-8` clears it
here, and the dark override lifts to `yellow-4` the same way danger does. */
--inspector-warning-text: var(--mantine-color-yellow-8);
/* ── Surfaces ──────────────────────────────────────────── */
/* Light-grey app background so the white cards read as raised. Drives
@ -204,6 +211,9 @@
--mantine-color-dimmed: var(--mantine-color-dark-1) !important;
/* Lighter danger red for the dark card/header (deep red would fail AA). */
--inspector-danger-text: var(--mantine-color-red-4);
/* Same inversion for caution text: the deep `yellow-8` is unreadable on a
dark surface. */
--inspector-warning-text: var(--mantine-color-yellow-4);
}
/* A11y contrast (light): Mantine sets these per-scheme on a selector more

61
clients/web/src/App.tsx
View file

@ -118,6 +118,7 @@ import { useMessageLog } from "@inspector/core/react/useMessageLog.js";
import { useFetchRequestLog } from "@inspector/core/react/useFetchRequestLog.js";
import { useStderrLog } from "@inspector/core/react/useStderrLog.js";
import { useInitialConfig } from "@inspector/core/react/useInitialConfig.js";
import { refreshingPersist } from "./lib/refreshingPersist";
import { usePendingClientRequests } from "@inspector/core/react/usePendingClientRequests.js";
import { InspectorView } from "./components/views/InspectorView/InspectorView";
import type {
@ -753,6 +754,8 @@ function App() {
sandboxUrl,
writable: serverListWritable,
version: inspectorVersion,
secretStorage,
refresh: refreshInitialConfig,
loading: initialConfigLoading,
} = useInitialConfig({
baseUrl: configBaseUrl,
@ -3750,7 +3753,14 @@ function App() {
runCommandInBackground(() => refreshResources(), "ambient");
}
}
void updateServerSettings(activeServerId, next).catch((err: unknown) => {
// Refreshed like every other secret-store mutation: this resends the
// server's rehydrated secrets, so it can trigger the pending
// plaintext-to-encrypted upgrade even though the user only toggled
// pagination (#1950 review r22).
void refreshingPersist(updateServerSettings, refreshInitialConfig)(
activeServerId,
next,
).catch((err: unknown) => {
// Persist failed: revert the optimistic override (the effect only
// clears it when the persisted value changes, which won't happen here)
// and roll the live client setting back, so the UI and client reflect
@ -3769,6 +3779,7 @@ function App() {
activeServerId,
inspectorClient,
updateServerSettings,
refreshInitialConfig,
connected,
loadToolsPage,
loadPromptsPage,
@ -4017,7 +4028,10 @@ function App() {
setStderrLogState(null);
setActiveServerId(undefined);
}
await removeServer(id);
// Deleting sweeps the server's secrets from the store, which for a
// file-backed store is a write — and a write is what performs the pending
// plaintext-to-encrypted upgrade (#1950 review r22).
await refreshingPersist(removeServer, refreshInitialConfig)(id);
setRemoveTarget(null);
}, [
removeTarget,
@ -4036,6 +4050,7 @@ function App() {
fetchRequestLogState,
stderrLogState,
removeServer,
refreshInitialConfig,
]);
// Submit handler for the Add / Edit / Clone modal. Add and Clone both go
@ -4121,7 +4136,14 @@ function App() {
targetId: settingsModalTargetId,
resolveInitial: (id) =>
servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS,
onPersist: updateServerSettings,
// The saved settings may include an OAuth client secret or a stdio `env:`
// value, and writing one can change what the secrets file *is* — the first
// save under a newly-set passphrase re-encrypts a pre-existing plaintext
// file. `refreshingPersist` re-asks the backend afterwards; it is a named
// unit rather than an inline `await …; refresh()` because this file is
// outside the coverage gate, so wiring written here is tested by nothing
// (#1950 review r17).
onPersist: refreshingPersist(updateServerSettings, refreshInitialConfig),
// Surface failures via toast — the modal usually closes
// immediately on user dismiss, so a silent fail-on-flush would
// leave the user thinking their last edits saved when they
@ -4145,7 +4167,10 @@ function App() {
} = useClientSettingsDraft({
opened: clientSettingsOpen,
resolveInitial: () => clientConfigToFormValues(clientConfig),
onPersist: async (values) => {
// Wrapped for the same reason as the server-settings persist above: this
// one can carry the enterprise IdP client secret, and that write is what
// flips a pending-encryption file to encrypted.
onPersist: refreshingPersist(async (values) => {
if (!canPersistClientSettingsDraft(values)) return;
const next = formValuesToClientConfig(values);
await saveClientConfigRemote(next, {
@ -4153,7 +4178,7 @@ function App() {
authToken: getAuthToken(),
});
setClientConfig(next);
},
}, refreshInitialConfig),
onError: (err) => {
notifications.show({
title: "Failed to save client settings",
@ -4713,12 +4738,27 @@ function App() {
initialConfig={configModalTarget?.config}
existingIds={existingIds}
onClose={() => setConfigModal(null)}
onSubmit={onConfigSubmit}
// Wrapped like the settings persists: this submit carries stdio `env`
// values, so it can perform the pending plaintext-to-encrypted
// upgrade and change the descriptor the footer reports.
onSubmit={refreshingPersist(onConfigSubmit, refreshInitialConfig)}
secretStorage={secretStorage}
/>
<ServerImportConfigModal
opened={importConfigOpen}
existingIds={existingIds}
onClose={() => setImportConfigOpen(false)}
// Refreshed once per import batch, not per entry. `useImportClientConfig`
// applies every addition and conflict sequentially, and on an encrypted
// file store each `/api/config` authenticates the whole file with a
// scrypt derivation — so wrapping the per-entry callbacks made a
// 20-server import pay 20 serialized KDFs and round trips. Closing is
// the batch boundary: the modal must be dismissed before the settings
// footer that reads this descriptor can be reached, and closing also
// covers a partially-failed batch (#1950 review r26).
onClose={() => {
setImportConfigOpen(false);
refreshInitialConfig();
}}
onFetchSource={importSource}
onAddServer={addServerHighlighted}
onUpdateServer={updateServer}
@ -4727,7 +4767,10 @@ function App() {
opened={importJsonOpen}
existingIds={existingIds}
onClose={() => setImportJsonOpen(false)}
onAddServer={addServerHighlighted}
onAddServer={refreshingPersist(
addServerHighlighted,
refreshInitialConfig,
)}
/>
<ServerSettingsModal
// Remount per open (and per target server) so the accordion resets to
@ -4753,6 +4796,7 @@ function App() {
onClearStoredOAuth={
settingsModalIsStdio ? undefined : handleClearStoredOAuthFromSettings
}
secretStorage={secretStorage}
/>
<ClientSettingsModal
key={
@ -4764,6 +4808,7 @@ function App() {
onSettingsChange={onClientSettingsChange}
emaIdpLoginState={emaIdpLoginState}
onEmaIdpLogout={logoutEmaIdp}
secretStorage={secretStorage}
/>
{initializeResult && activeServer && (
<ConnectionInfoModal

105
clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx Normal file
View file

@ -0,0 +1,105 @@
import type { Meta, StoryObj } from "@storybook/react-vite";
import { expect, within } from "storybook/test";
import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js";
import { SecretStorageFooter } from "./SecretStorageFooter";
const keyring: SecretStorageInfo = {
kind: "keyring",
reason: "default",
durable: true,
};
const meta: Meta<typeof SecretStorageFooter> = {
title: "Elements/SecretStorageFooter",
component: SecretStorageFooter,
args: { info: keyring },
};
export default meta;
type Story = StoryObj<typeof SecretStorageFooter>;
// The ordinary case on a desktop install: the OS keychain, stated quietly.
export const Keychain: Story = {
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
// Asserted on the band's full text: the "Secrets:" prefix is its own
// `<span>`, which a `findByText` string match would not see as one string.
const band = await canvas.findByTestId("secret-storage-footer");
await expect(band).toHaveTextContent("Secrets: OS keychain");
await expect(band).toHaveAttribute("data-tone", "neutral");
},
};
// A container with a mounted volume and `MCP_INSPECTOR_SECRET_KEY` set: durable,
// encrypted, and therefore still quiet — the path is the useful part.
export const EncryptedFile: Story = {
args: {
info: {
kind: "file",
reason: "fallback",
durable: true,
plaintext: false,
path: "/home/node/.mcp-inspector/secrets.json",
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const band = await canvas.findByTestId("secret-storage-footer");
await expect(band).toHaveTextContent(
"Secrets: Encrypted file. Owner-only permissions.",
);
// The path is no longer printed on the band — it goes to the clipboard,
// and stays readable in the button's accessible name.
await expect(
canvas.getByRole("button", {
name: "Secrets: Encrypted file. Owner-only permissions. Copy secrets file path: /home/node/.mcp-inspector/secrets.json",
}),
).toBeInTheDocument();
},
};
// The same container without a passphrase. The caveat is rendered inline and in
// the warning tone — this is what "plaintext gets a loud banner" looks like at
// the point where the secret is actually typed.
export const UnencryptedFile: Story = {
args: {
info: {
kind: "file",
reason: "fallback",
durable: true,
plaintext: true,
path: "/home/node/.mcp-inspector/secrets.json",
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const band = await canvas.findByTestId("secret-storage-footer");
await expect(band).toHaveTextContent(
"Secrets: Plaintext file. Owner-only permissions.",
);
await expect(band).toHaveAttribute("data-tone", "warn");
},
};
// The published container with nothing mounted: session-scoped, and it says so
// rather than promising a durability the writable layer can't keep.
export const InMemory: Story = {
args: { info: { kind: "memory", reason: "fallback", durable: false } },
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const band = await canvas.findByTestId("secret-storage-footer");
await expect(band).toHaveTextContent(
"Secrets: Memory (this session only):",
);
await expect(canvas.getByText(/lost on exit/)).toBeInTheDocument();
},
};
// A backend that reports no store (legacy build): the band is absent entirely.
export const Unknown: Story = {
args: { info: undefined },
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
await expect(canvas.queryByTestId("secret-storage-footer")).toBeNull();
},
};

246
clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx Normal file
View file

@ -0,0 +1,246 @@
import { describe, it, expect, vi } from "vitest";
import userEvent from "@testing-library/user-event";
import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js";
import { renderWithMantine, screen } from "../../../test/renderWithMantine";
import { SecretStorageFooter } from "./SecretStorageFooter";
const keyring: SecretStorageInfo = {
kind: "keyring",
reason: "default",
durable: true,
};
const encryptedFile: SecretStorageInfo = {
kind: "file",
reason: "fallback",
durable: true,
plaintext: false,
path: "/home/node/.mcp-inspector/secrets.json",
};
const plaintextFile: SecretStorageInfo = { ...encryptedFile, plaintext: true };
const memory: SecretStorageInfo = {
kind: "memory",
reason: "fallback",
durable: false,
};
describe("SecretStorageFooter", () => {
it("names the keychain with no caveat and no warning tone", () => {
renderWithMantine(<SecretStorageFooter info={keyring} />);
// Asserted on the band's full text rather than with `getByText`: the
// "Secrets:" prefix is its own `<span>`, and testing-library matches a
// string against an element's *direct* text nodes only, so the label now
// reads as two fragments even though it renders as one line.
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Secrets: OS keychain");
expect(band).toHaveAttribute("data-tone", "neutral");
});
it("states an encrypted file quietly, and offers its path for copying", () => {
// Nothing is wrong here, so the band must not shout. The path is no longer
// printed on the band — it goes to the clipboard — so the accessible name
// is where it stays readable.
renderWithMantine(<SecretStorageFooter info={encryptedFile} />);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent(
"Secrets: Encrypted file. Owner-only permissions.",
);
expect(band).toHaveAttribute("data-tone", "neutral");
// The accessible name must carry the *status* as well as the affordance:
// `aria-label` replaces descendant text, so labelling it only "Copy…"
// would announce the button and discard the warning the band exists for.
expect(
screen.getByRole("button", {
name: "Secrets: Encrypted file. Owner-only permissions. Copy secrets file path: /home/node/.mcp-inspector/secrets.json",
}),
).toBeInTheDocument();
});
it("says a plaintext file is plaintext, in the warning tone", () => {
// The "loud warning" decision made concrete: the state is on the band
// itself, not hidden behind a hover.
renderWithMantine(<SecretStorageFooter info={plaintextFile} />);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent(
"Secrets: Plaintext file. Owner-only permissions.",
);
expect(band).toHaveAttribute("data-tone", "warn");
});
it("copies the path on click", async () => {
const user = userEvent.setup();
const writeText = vi.fn().mockResolvedValue(undefined);
// happy-dom has no clipboard; define one so the copy path is exercised
// rather than merely not throwing.
Object.defineProperty(navigator, "clipboard", {
value: { writeText },
configurable: true,
});
renderWithMantine(<SecretStorageFooter info={plaintextFile} />);
await user.click(screen.getByRole("button", { name: /Copy secrets file/ }));
expect(writeText).toHaveBeenCalledWith(
"/home/node/.mcp-inspector/secrets.json",
);
});
it("is inert for a store with no file to point at", () => {
// A keychain or in-memory store has no path, so there is nothing to copy
// and the band must not pretend to be a control.
renderWithMantine(<SecretStorageFooter info={keyring} />);
expect(screen.queryByRole("button")).toBeNull();
});
it("declines to claim owner-only permissions when the mode says otherwise", () => {
// The one assertion the band must never get wrong: `looseMode` is set
// exactly when 0600 could not be reached, so "Owner-only permissions."
// there would state the opposite of the truth.
renderWithMantine(
<SecretStorageFooter info={{ ...plaintextFile, looseMode: 0o644 }} />,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Mode 0644 — not owner-only.");
expect(band).not.toHaveTextContent("Owner-only permissions.");
});
it("warns that an in-memory store loses secrets on exit", () => {
renderWithMantine(<SecretStorageFooter info={memory} />);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Secrets: Memory (this session only):");
expect(screen.getByText(/lost on exit/)).toBeInTheDocument();
expect(band).toHaveAttribute("data-tone", "warn");
});
it("does not claim exposure in the tooltip when encryption is unknown", async () => {
// Mirrors `secretStorageCaveat`: `plaintext` is absent alongside
// `encryptionUnknown`, and a two-way `!== false` test would assert that
// a reader gets the secrets out of a file we could not even classify.
// The claim lives in the *tooltip*, so that is what this hovers to read
// — the visible line reports the unreadable envelope, which is a
// different (and also correct) statement.
const user = userEvent.setup();
renderWithMantine(
<SecretStorageFooter
info={{
...plaintextFile,
plaintext: undefined,
looseMode: 0o644,
encryptionUnknown: "not valid JSON",
}}
/>,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveAttribute("data-tone", "warn");
// The tooltip's target is the copy button, not the band around it.
await user.hover(screen.getByRole("button", { name: /Copy secrets file/ }));
const tip = await screen.findByText(/Mode 0644 could not be tightened/);
expect(tip).toHaveTextContent("could not determine");
expect(tip).not.toHaveTextContent(
"anyone who can read the file can read the secrets in it",
);
});
it("announces the security status, not just the copy affordance", () => {
// The regression this pins: `aria-label` overrides the button's
// descendant text, so a screen-reader user heard "Copy secrets file
// path …" and lost "Plaintext file" entirely — the one sentence that
// should change their mind about typing a secret.
renderWithMantine(<SecretStorageFooter info={plaintextFile} />);
const button = screen.getByRole("button");
expect(button).toHaveAccessibleName(
/^Secrets: Plaintext file\. Owner-only permissions\./,
);
expect(button).toHaveAccessibleName(/Copy secrets file path/);
});
it("renders nothing when the backend didn't report a store", () => {
// A guessed answer under a secret field is worse than no answer, so the
// unknown case is silence rather than a default.
renderWithMantine(<SecretStorageFooter info={undefined} />);
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
});
it("says the permissions could not be checked rather than claiming owner-only", async () => {
// The third permission state. Falling into the verified-0600 branch here
// would have the band assert owner-only having read nothing — the same
// bug as claiming 0600 in the caveat, one layer up.
renderWithMantine(
<SecretStorageFooter
info={{
kind: "file",
reason: "fallback",
durable: true,
plaintext: true,
path: "/home/node/.mcp-inspector/secrets.json",
permissionsUnknown: "EACCES",
}}
/>,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Permissions could not be checked.");
expect(band).not.toHaveTextContent("Owner-only");
expect(band).toHaveAttribute("data-tone", "warn");
});
it("says an unreadable file is unreadable, not encrypted", async () => {
// The descriptor omits `plaintext` here, and the band must not read a
// default out of that absence.
renderWithMantine(
<SecretStorageFooter
info={{
kind: "file",
reason: "fallback",
durable: true,
path: "/home/node/.mcp-inspector/secrets.json",
encryptionUnknown: "not valid JSON",
}}
/>,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Unreadable file.");
expect(band).toHaveTextContent("Saving a secret will fail.");
expect(band).not.toHaveTextContent("Encrypted file");
expect(band).toHaveAttribute("data-tone", "warn");
});
it("does not claim a loose-mode encrypted file exposes its secrets", async () => {
renderWithMantine(
<SecretStorageFooter
info={{
kind: "file",
reason: "fallback",
durable: true,
plaintext: false,
path: "/home/node/.mcp-inspector/secrets.json",
looseMode: 0o644,
}}
/>,
);
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent("Mode 0644 — not owner-only.");
expect(band).toHaveAttribute("data-tone", "warn");
});
it("puts the path in the tooltip, not just an offer to copy it", async () => {
// The band no longer prints the path and the accessible name carries it
// for screen-reader users, which left a sighted user unable to see where
// a custom destination points without copying it out somewhere else.
renderWithMantine(
<SecretStorageFooter
info={{
kind: "file",
reason: "fallback",
durable: true,
plaintext: false,
path: "/custom/place/secrets.json",
}}
/>,
);
// Hover the control the tooltip is attached to, not the band around it.
await userEvent.hover(
screen.getByRole("button", { name: /Copy secrets file path/ }),
);
const tip = await screen.findByRole("tooltip");
expect(tip).toHaveTextContent("/custom/place/secrets.json");
expect(tip).toHaveTextContent("Click to copy.");
});
});

319
clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx Normal file
View file

@ -0,0 +1,319 @@
import type { ReactNode } from "react";
import {
CopyButton,
Group,
Text,
Tooltip,
UnstyledButton,
} from "@mantine/core";
import { TbAlertTriangle, TbDeviceDesktop, TbLock } from "react-icons/tb";
import {
secretStorageCaveat,
secretStorageLabel,
secretStorageTone,
type SecretStorageInfo,
} from "@inspector/core/auth/secret-storage-info.js";
/**
* Height of the footer band. Matches `InspectorView`'s `FOOTER_HEIGHT`
* deliberately: these are the app's only permanent status strips, and a
* modal footer standing a few pixels taller than the one behind it would
* read as a different kind of thing.
*/
export const SECRET_STORAGE_FOOTER_HEIGHT = 32;
export interface SecretStorageFooterProps {
/**
* The active store, from `GET /api/config`. Renders nothing when
* undefined — a backend that didn't report one leaves the question
* genuinely open, and an invented answer directly beneath a secret field
* is worse than silence.
*/
info?: SecretStorageInfo;
}
// A thin band, same height as the app footer. Mantine's compound Modal has no
// `Modal.Footer`, so it renders as the last child of `Modal.Content`, after the
// `Modal.Body`, and its theme variant pins it with `position: sticky` — the
// mirror of Mantine's own sticky header, because `Modal.Content` scrolls
// header, body and footer as one. `nowrap` keeps it to one line at any modal
// width; the truncating detail text on the right absorbs a narrow modal.
const FooterBand = Group.withProps({
variant: "stickyModalFooter",
h: SECRET_STORAGE_FOOTER_HEIGHT,
gap: 0,
align: "center",
wrap: "nowrap",
w: "100%",
});
// The icon + message row. Carries the horizontal padding (rather than the band)
// so that when the row is a copy button it covers the band edge to edge — a
// click target with dead margins is a click target people miss.
const FooterRow = Group.withProps({
h: "100%",
w: "100%",
px: "md",
gap: 6,
align: "center",
wrap: "nowrap",
});
// The whole strip becomes the copy affordance for a file-backed store. An
// `UnstyledButton` rather than a click handler on the Group: this is a real
// action, so it needs to be reachable by keyboard and announced as a button.
const CopyTarget = UnstyledButton.withProps({
h: "100%",
w: "100%",
});
// The whole statement, prefix included, as one clamped line.
//
// `lineClamp` rather than `truncate`, and the difference is load-bearing.
// `Modal.Content` sizes its scroll container to its children's **max-content**
// width, and `truncate` sets `white-space: nowrap`, whose max-content is the
// entire sentence — so the band grew the container to 770px inside a 620px
// modal and the *form above it* got clipped at the right edge (measured).
// Clamped text still wraps internally, so its max-content is merely its
// longest word, and the container stays the modal's width; the clamp then
// keeps it to the one line this 32px band has room for. `miw: 0` + `flex: 1`
// let it shrink to the leftover space rather than to its content.
const FooterMessage = Text.withProps({
size: "xs",
fw: 500,
lineClamp: 1,
miw: 0,
flex: 1,
ta: "left",
});
// "Secrets:" carries the heavier weight so the band reads as a labelled fact
// rather than as one run of prose — the eye lands on what the line is about
// before reading which store it names.
//
// A `span` inside the label rather than a sibling in the `Group`: `Text`
// renders a `<p>`, so a nested one would be a `<p>` inside a `<p>` (invalid,
// and the browser would close the outer paragraph early), and a sibling would
// take the Group's `gap` and sit too far from the value it labels. `inherit`
// keeps it at the label's own size and line height instead of Mantine's `Text`
// defaults.
const FooterLabelPrefix = Text.withProps({
span: true,
inherit: true,
fw: 700,
});
// `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;
/** Octal mode, as `stat` would print it (`0644`). */
const formatMode = (mode: number): string => mode.toString(8).padStart(4, "0");
/**
* How the band describes the file's permissions.
*
* Three states, not two, and the third is the point: "Owner-only
* permissions." is a claim of *fact*, so it may only be made when the mode
* was actually read and found to be 0600. `looseMode` says we looked and it
* was wrong; `permissionsUnknown` says we could not look at all. Treating
* the second like the first — which a plain `looseMode === undefined` test
* does — makes the footer assert owner-only having verified nothing, which
* is the failure this band exists to prevent, arrived at by omission rather
* than by error.
*/
function filePermissionsSentence(info: SecretStorageInfo): string {
if (info.looseMode !== undefined) {
return `Mode ${formatMode(info.looseMode)} — not owner-only.`;
}
if (info.permissionsUnknown !== undefined) {
return "Permissions could not be checked.";
}
return "Owner-only permissions.";
}
/**
* The visible statement, per store.
*
* Deliberately not built from `secretStorageLabel` for the file case: that
* label is parenthetical ("File (unencrypted)") because the startup banner
* appends " at <path>" to it, and the footer wants a sentence instead. The
* two surfaces have different shapes, so each phrases its own; the *tone*
* and the underlying facts still come from the shared helpers, which is
* what keeps them from disagreeing about anything that matters.
*/
function footerMessage(info: SecretStorageInfo): string {
if (info.kind === "keyring") return secretStorageLabel(info);
if (info.kind === "memory") {
// Derived rather than restated — memory's consequence is the caveat, and
// the banner prints the same sentence.
const caveat = secretStorageCaveat(info);
return caveat
? `${secretStorageLabel(info)}: ${caveat}`
: secretStorageLabel(info);
}
// "Unreadable" rather than guessing a mode: the envelope could not be
// parsed, so neither "Plaintext" nor "Encrypted" is a claim we can make.
if (info.encryptionUnknown !== undefined) {
return "Unreadable file. Saving a secret will fail.";
}
const encryption = info.plaintext ? "Plaintext" : "Encrypted";
const permissions = filePermissionsSentence(info);
return `${encryption} file. ${permissions}`;
}
/**
* The hover text, per store. Undefined leaves the band without a tooltip.
*
* For a file-backed store it always ends with the copy hint, because the
* path is no longer shown on the band — it is in the clipboard instead, and
* an affordance nobody knows about is not one.
*/
function footerTooltip(info: SecretStorageInfo): string | undefined {
if (info.kind !== "file" || !info.path) return undefined;
const parts: string[] = [];
if (info.encryptionUnknown !== undefined) {
parts.push(
`The file could not be read (${info.encryptionUnknown}), so its contents are unknown and saving a secret will fail rather than overwrite it.`,
);
} else if (info.plaintext) {
// Advice that can actually clear the condition: someone who has already
// set the passphrase is waiting on the next write, not on themselves.
parts.push(
info.pendingEncryption
? "Re-encrypted the next time a secret is saved."
: "Set MCP_INSPECTOR_SECRET_KEY to encrypt.",
);
}
if (info.looseMode !== undefined) {
// Encryption-aware: a reader of an encrypted file gets ciphertext, not
// secrets. Claiming otherwise overstates the exposure, and a warning
// that cries wolf is discounted on the occasion it is literally true.
parts.push(
// Three cases, mirroring `secretStorageCaveat`. With
// `encryptionUnknown` set, `plaintext` is absent and `!== false` would
// land on the "anyone can read the secrets" wording — asserting an
// exposure we did not establish, since the envelope may hold
// ciphertext we simply could not classify.
info.encryptionUnknown !== undefined
? `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — others can copy the file, and whether that exposes the secrets depends on encryption this build could not determine (${info.encryptionUnknown}).`
: info.plaintext === false
? `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — others can copy the file, and the passphrase is then the only thing protecting its contents.`
: `Mode ${formatMode(info.looseMode)} could not be tightened to 0600 — anyone who can read the file can read the secrets in it.`,
);
} else if (info.permissionsUnknown !== undefined) {
parts.push(
`The file's permissions could not be read (${info.permissionsUnknown}), so it is not known whether others can read it.`,
);
}
// The path itself, not just an offer to copy it. The band no longer prints
// it, and the accessible name carries it for screen-reader users — which
// left a sighted user unable to see *where* a custom destination points
// without copying it out to somewhere else first.
parts.push(info.path);
parts.push("Click to copy.");
return parts.join(" ");
}
/**
* The permanent "here is where your secrets go" strip at the bottom of the
* Client Settings and Server Settings modals (#1950).
*
* It appears wherever the Inspector accepts a secret — Client Settings (the
* enterprise IdP client secret), Server Settings (the per-server OAuth
* client secret and stdio `env:` values), and Server Config (stdio `env:`
* values again). This comment named only the first two until review round
* 20, and the dialog it omitted was the one taking secrets with no
* disclosure at all, so the wrong count here was the bug rather than a
* description of it. The alternatives were
* considered and rejected: a startup banner is seen once, by whoever
* started the process, possibly hours before anyone types anything; a
* toast is seen once and then gone; a dismissible banner is, by design,
* the thing a user makes disappear before doing the work it describes.
* This is always present, is not dismissible, and has no empty state
* beyond "the backend didn't say".
*
* When the store is lossy or unencrypted the caveat is rendered *inline*
* rather than left to the tooltip. That is the whole point of the
* "plaintext gets a loud warning" decision: a warning you have to hover to
* discover is not one. The tooltip carries the full sentence for the case
* where a narrow modal truncates it, and — for the quiet stores — the file
* path.
*
* Deliberately read-only for now. Choosing the store from here is the
* obvious follow-up, but the gap this closes is that users could not find
* out *at all*, and closing that does not require a control.
*/
export function SecretStorageFooter({ info }: SecretStorageFooterProps) {
if (!info) return null;
const warn = secretStorageTone(info) === "warn";
const color = warn
? "var(--inspector-warning-text)"
: "var(--inspector-text-secondary)";
const Icon = warn
? TbAlertTriangle
: info.kind === "keyring"
? TbDeviceDesktop
: TbLock;
const row = (
<FooterRow>
{/* A react-icons glyph, not a Mantine factory component — outside the
`.withProps()` rule, and it takes `color` as a prop rather than a
style. */}
<Icon size={ICON_SIZE} color={color} aria-hidden />
<FooterMessage c={color}>
<FooterLabelPrefix>Secrets:</FooterLabelPrefix> {footerMessage(info)}
</FooterMessage>
</FooterRow>
);
const tip = footerTooltip(info);
const band = (content: ReactNode) => (
<FooterBand
data-testid="secret-storage-footer"
data-tone={warn ? "warn" : "neutral"}
>
{content}
</FooterBand>
);
// A keychain or in-memory store has no path, so there is nothing to copy and
// the band stays inert — no button, no tooltip beyond what it already shows.
if (!info.path) return band(row);
return (
<CopyButton value={info.path} timeout={1500}>
{({ copied, copy }) =>
band(
<CaveatTooltip label={copied ? "Path copied" : tip}>
{/* The accessible name has to carry **both** halves. `aria-label`
replaces the button's descendant text, so labelling it only
"Copy secrets file path …" announced the affordance and threw
away the security status — "Plaintext file", "Unreadable
file", "not owner-only" — which is the entire reason this band
exists. The status leads, because it is the part a user needs
before deciding to type a secret; the path follows, since a
clipboard is no use to someone who cannot inspect it. Leading
with the visible text also keeps the accessible name a
superset of it (WCAG 2.5.3). */}
<CopyTarget
onClick={copy}
aria-label={`Secrets: ${footerMessage(info)} Copy secrets file path: ${info.path}`}
>
{row}
</CopyTarget>
</CaveatTooltip>,
)
}
</CopyButton>
);
}

42
clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx
View file

@ -471,4 +471,46 @@ describe("ClientSettingsModal", () => {
);
expect(screen.queryByText("Client Settings")).not.toBeInTheDocument();
});
describe("secret-storage footer (#1950)", () => {
// This dialog holds the enterprise IdP client secret, so it states where
// that secret is about to land — permanently, and beside the field rather
// than in a startup banner nobody watching this screen ever saw.
it("names the active store and warns when the file is unencrypted", () => {
renderWithMantine(
<ClientSettingsModal
opened
settings={EMPTY_CLIENT_SETTINGS}
onClose={vi.fn()}
onSettingsChange={vi.fn()}
secretStorage={{
kind: "file",
reason: "fallback",
durable: true,
plaintext: true,
path: "/home/node/.mcp-inspector/secrets.json",
}}
/>,
);
// Full-text assertion: the "Secrets:" prefix is its own `<span>`, which
// `getByText`'s direct-text-node matching would not see as one string.
const band = screen.getByTestId("secret-storage-footer");
expect(band).toHaveTextContent(
"Secrets: Plaintext file. Owner-only permissions.",
);
expect(band).toHaveAttribute("data-tone", "warn");
});
it("renders no footer when the backend didn't report a store", () => {
renderWithMantine(
<ClientSettingsModal
opened
settings={EMPTY_CLIENT_SETTINGS}
onClose={vi.fn()}
onSettingsChange={vi.fn()}
/>,
);
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
});
});
});

11
clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx
View file

@ -1,6 +1,8 @@
import { useState } from "react";
import { CloseButton, Group, Modal, ScrollArea } from "@mantine/core";
import { ListToggle } from "../../elements/ListToggle/ListToggle";
import { SecretStorageFooter } from "../../elements/SecretStorageFooter/SecretStorageFooter";
import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js";
import {
ClientSettingsForm,
type ClientSettingsSection,
@ -41,6 +43,13 @@ export interface ClientSettingsModalProps {
) => void;
emaIdpLoginState?: EmaIdpLoginState;
onEmaIdpLogout?: () => void;
/**
* Where secrets typed here end up (#1950). This dialog holds the
* enterprise IdP client secret, so the footer is not decoration — it is
* the answer to the question the field raises. Absent on a backend that
* doesn't report a store; the footer then renders nothing.
*/
secretStorage?: SecretStorageInfo;
}
export function ClientSettingsModal({
@ -50,6 +59,7 @@ export function ClientSettingsModal({
onSettingsChange,
emaIdpLoginState,
onEmaIdpLogout,
secretStorage,
}: ClientSettingsModalProps) {
const [expandedSections, setExpandedSections] = useState<
ClientSettingsSection[]
@ -109,6 +119,7 @@ export function ClientSettingsModal({
revealErrors={revealErrors}
/>
</Modal.Body>
<SecretStorageFooter info={secretStorage} />
</Modal.Content>
</AppModalLg>
);

39
clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.test.tsx
View file

@ -471,3 +471,42 @@ describe("ServerConfigModal", () => {
expect(props.onClose).toHaveBeenCalledOnce();
});
});
describe("secret-storage footer (#1950 review r20)", () => {
it("discloses where stdio env values will be kept", () => {
// This dialog takes stdio `env` values, which are extracted into the
// secret store exactly as Server Settings' are — so it is a secret-entry
// surface. It had no disclosure at all, which made it the one place a
// user could type a secret without being told where it goes.
renderWithMantine(
<ServerConfigModal
opened
mode="add"
existingIds={[]}
onClose={vi.fn()}
onSubmit={vi.fn()}
secretStorage={{
kind: "memory",
reason: "fallback",
durable: false,
}}
/>,
);
expect(screen.getByTestId("secret-storage-footer")).toHaveTextContent(
"Secrets: Memory (this session only)",
);
});
it("renders no footer when the backend didn't report a store", () => {
renderWithMantine(
<ServerConfigModal
opened
mode="add"
existingIds={[]}
onClose={vi.fn()}
onSubmit={vi.fn()}
/>,
);
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
});
});

12
clients/web/src/components/groups/ServerConfigModal/ServerConfigModal.tsx
View file

@ -10,6 +10,8 @@ import {
Textarea,
} from "@mantine/core";
import { ClearButton } from "../../elements/ClearButton/ClearButton";
import { SecretStorageFooter } from "../../elements/SecretStorageFooter/SecretStorageFooter";
import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js";
import { useValueChange } from "../../../hooks/useValueChange";
import type {
MCPServerConfig,
@ -33,6 +35,14 @@ export interface ServerConfigModalProps {
existingIds: string[];
onClose: () => void;
onSubmit: (id: string, config: MCPServerConfig) => Promise<void> | void;
/**
* Where secrets typed here end up (#1950 review r20). This dialog takes
* stdio `env` values, which are extracted into the secret store exactly as
* the ones in Server Settings are — so it is a secret-entry surface and
* needs the same permanent disclosure. It had none, which meant a user
* could type a value into the one dialog that never said where it goes.
*/
secretStorage?: SecretStorageInfo;
}
type TransportChoice = "stdio" | "sse" | "streamable-http";
@ -188,6 +198,7 @@ export function ServerConfigModal({
existingIds,
onClose,
onSubmit,
secretStorage,
}: ServerConfigModalProps) {
const initial = useMemo(
() => configToFormState(initialId, initialConfig, mode),
@ -446,6 +457,7 @@ export function ServerConfigModal({
</Button>
</Actions>
</SectionStack>
<SecretStorageFooter info={secretStorage} />
</AppModalLg>
);
}

42
clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx
View file

@ -899,4 +899,46 @@ describe("ServerSettingsModal", () => {
});
});
});
describe("secret-storage footer (#1950)", () => {
// The dialog holds the OAuth client secret and every stdio `env:` value, so
// the destination of what is typed here is stated permanently rather than
// in a toast that has long since gone.
it("names the active store", () => {
renderWithMantine(
<ServerSettingsModal
opened
settings={emptySettings}
serverType="streamable-http"
isStdio={false}
onClose={vi.fn()}
onSettingsChange={vi.fn()}
secretStorage={{
kind: "memory",
reason: "fallback",
durable: false,
}}
/>,
);
// Full-text assertion: the "Secrets:" prefix is its own `<span>`, which
// `getByText`'s direct-text-node matching would not see as one string.
expect(screen.getByTestId("secret-storage-footer")).toHaveTextContent(
"Secrets: Memory (this session only)",
);
});
it("renders no footer when the backend didn't report a store", () => {
renderWithMantine(
<ServerSettingsModal
opened
settings={emptySettings}
serverType="streamable-http"
isStdio={false}
onClose={vi.fn()}
onSettingsChange={vi.fn()}
/>,
);
expect(screen.queryByTestId("secret-storage-footer")).toBeNull();
});
});
});

11
clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx
View file

@ -11,6 +11,8 @@ import type {
import { isOAuthCapableServerType } from "@inspector/core/mcp/config.js";
import { ADVERTISABLE_EXTENSIONS } from "@inspector/core/mcp/extensions.js";
import { ListToggle } from "../../elements/ListToggle/ListToggle";
import { SecretStorageFooter } from "../../elements/SecretStorageFooter/SecretStorageFooter";
import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js";
import {
ServerSettingsForm,
type ServerSettingsSection,
@ -73,6 +75,13 @@ export interface ServerSettingsModalProps {
onClose: () => void;
onSettingsChange: (settings: InspectorServerSettings) => void;
onClearStoredOAuth?: () => void;
/**
* Where secrets typed here end up (#1950). This dialog holds the OAuth
* client secret and every stdio `env:` value, so the footer states the
* destination of what the user is about to type. Absent on a backend
* that doesn't report a store; the footer then renders nothing.
*/
secretStorage?: SecretStorageInfo;
}
export function ServerSettingsModal({
@ -84,6 +93,7 @@ export function ServerSettingsModal({
onClose,
onSettingsChange,
onClearStoredOAuth,
secretStorage,
}: ServerSettingsModalProps) {
const sections = allSectionsFor(serverType, isStdio);
// Initial expansion is the first ("options") section — where Network Log
@ -311,6 +321,7 @@ export function ServerSettingsModal({
onRootChange={handleRootChange}
/>
</Modal.Body>
<SecretStorageFooter info={secretStorage} />
</Modal.Content>
</AppModalLg>
);

64
clients/web/src/lib/refreshingPersist.test.ts Normal file
View file

@ -0,0 +1,64 @@
import { describe, it, expect, vi } from "vitest";
import { refreshingPersist } from "./refreshingPersist";
describe("refreshingPersist", () => {
it("refreshes after the persist resolves, not before", async () => {
// Ordering is the point: refreshing first would re-fetch the descriptor
// that the pending write is about to invalidate, which is the stale read
// this wrapper exists to prevent, one step earlier.
const order: string[] = [];
const persist = vi.fn(async () => {
order.push("persist");
});
const refresh = vi.fn(() => {
order.push("refresh");
});
await refreshingPersist(persist, refresh)();
expect(order).toEqual(["persist", "refresh"]);
});
it("passes every argument through unchanged", async () => {
// The server-settings caller is `(id, settings)`; the wrapper must be
// transparent or it silently drops the entry it was meant to save.
// Typed rather than given placeholder parameters: this scope's eslint
// does not honour the `_` prefix, and the signature is what matters.
const persist = vi
.fn<(id: string, settings: unknown) => Promise<void>>()
.mockResolvedValue(undefined);
const settings = { headers: [] };
await refreshingPersist(persist, vi.fn())("srv-1", settings);
expect(persist).toHaveBeenCalledWith("srv-1", settings);
});
it("refreshes even when the persist fails, and still propagates the error", async () => {
// This asserted the opposite until round 22, on reasoning that sounded
// right and was wrong for this write order: both persistence paths write
// the secret store *before* the file, so a rejected disk write can follow
// a `set` that already upgraded `secrets.json` from plaintext to
// encrypted. Skipping the refresh there left the footer describing a file
// that no longer exists in that form.
//
// The asymmetry decides it: a needless refresh costs one idempotent GET;
// a missed one leaves a security statement wrong until reload.
const boom = new Error("keychain unavailable");
const persist = vi.fn().mockRejectedValue(boom);
const refresh = vi.fn();
await expect(refreshingPersist(persist, refresh)()).rejects.toBe(boom);
expect(refresh).toHaveBeenCalledTimes(1);
});
it("returns a function usable as a persist callback more than once", async () => {
const persist = vi.fn(async () => {});
const refresh = vi.fn();
const wrapped = refreshingPersist(persist, refresh);
await wrapped();
await wrapped();
expect(persist).toHaveBeenCalledTimes(2);
expect(refresh).toHaveBeenCalledTimes(2);
});
});

51
clients/web/src/lib/refreshingPersist.ts Normal file
View file

@ -0,0 +1,51 @@
/**
* Wrap a persist callback so it refreshes derived state after it succeeds.
*
* Written for one case (#1950): saving a secret can change what the secrets
* file *is* — the first save under a newly-set passphrase re-encrypts a
* pre-existing plaintext file — so the `secretStorage` descriptor behind the
* settings-modal footer has to be re-fetched afterwards, or the footer keeps
* reporting the state it had at page load.
*
* Lives in `lib/` rather than `utils/`: the combinator itself computes
* nothing, but what it exists to do is *sequence side effects* — a persist
* that writes to disk and a refetch that hits the network — which is the
* `lib` half of the repo's split. Judged the other way at first on the
* grounds that the function is a pure higher-order one; the rule is about
* what a module is for, not about whether its own body touches I/O.
*
* It exists as a named unit rather than two inline `await …; refresh()` pairs
* in `App.tsx` because of where the coverage gate can reach. `App.tsx` is
* deliberately outside the coverage `include` (a ~4.5k-line composition root),
* so wiring written there is not exercised by anything: the hook's own tests
* and the modal's own tests can both pass while the callback that connects
* them is broken, leaving the security footer stale after exactly the write
* that changed it. Review round 17 of #1950 caught that, after three earlier
* rounds had each found a defect in this same path.
*
* `refresh` runs in a `finally`, **not only on success**, and the first
* version of this got that wrong on plausible-sounding reasoning: "a persist
* that threw did not write, so there is nothing new to describe". That is
* false for this write order. Both persistence paths write the secret store
* *before* the file — so a rejected disk write can follow a `set` that has
* already upgraded `secrets.json` from plaintext to encrypted. Skipping the
* refresh there leaves the footer describing a file that no longer exists in
* that form, which is the stale-descriptor bug this wrapper exists to
* prevent, reached through its own error path.
*
* The asymmetry is what settles it: refreshing after a failure that changed
* nothing costs one idempotent GET, while not refreshing after a failure that
* changed something leaves a security statement wrong until reload.
*/
export function refreshingPersist<Args extends unknown[]>(
persist: (...args: Args) => Promise<void>,
refresh: () => void,
): (...args: Args) => Promise<void> {
return async (...args: Args) => {
try {
await persist(...args);
} finally {
refresh();
}
};
}

204
clients/web/src/test/core/auth/secret-storage-info.test.ts Normal file
View file

@ -0,0 +1,204 @@
/**
* The secret-storage descriptor's presentation helpers (#1950).
*
* These are shared by the terminal banner and the browser footer, which is
* the whole reason they live in `core/auth/` rather than beside either
* consumer — so the cases below are about the *statements* they make,
* particularly which states are loud and which are quiet. A helper that
* quietly stopped warning about a plaintext file would leave both surfaces
* silently wrong at once.
*/
import { describe, it, expect } from "vitest";
import {
secretStorageCaveat,
secretStorageLabel,
secretStorageSummary,
secretStorageTone,
type SecretStorageInfo,
} from "@inspector/core/auth/secret-storage-info.js";
const keyring: SecretStorageInfo = {
kind: "keyring",
reason: "default",
durable: true,
};
const encryptedFile: SecretStorageInfo = {
kind: "file",
reason: "fallback",
durable: true,
plaintext: false,
path: "/home/node/.mcp-inspector/secrets.json",
};
const plaintextFile: SecretStorageInfo = {
...encryptedFile,
plaintext: true,
};
const memory: SecretStorageInfo = {
kind: "memory",
reason: "fallback",
durable: false,
};
describe("secretStorageLabel", () => {
it("names the keychain", () => {
expect(secretStorageLabel(keyring)).toBe("OS keychain");
});
it("distinguishes an encrypted file from an unencrypted one", () => {
// The distinction is the point — "File" alone would answer "where"
// while dropping the half a user needs to judge the risk.
expect(secretStorageLabel(encryptedFile)).toBe("File (encrypted)");
expect(secretStorageLabel(plaintextFile)).toBe("File (unencrypted)");
});
it("says memory is session-scoped in the label itself", () => {
expect(secretStorageLabel(memory)).toBe("Memory (this session only)");
});
});
describe("secretStorageTone", () => {
it("stays neutral for the keychain and an encrypted file", () => {
// A tone that fired for every store would carry no information.
expect(secretStorageTone(keyring)).toBe("neutral");
expect(secretStorageTone(encryptedFile)).toBe("neutral");
});
it("warns for an unencrypted file and for memory", () => {
expect(secretStorageTone(plaintextFile)).toBe("warn");
expect(secretStorageTone(memory)).toBe("warn");
});
});
describe("secretStorageCaveat", () => {
it("has none for the quiet stores", () => {
expect(secretStorageCaveat(keyring)).toBeUndefined();
expect(secretStorageCaveat(encryptedFile)).toBeUndefined();
});
it("names the loss for memory", () => {
expect(secretStorageCaveat(memory)).toContain("lost on exit");
});
it("names the fix for an unencrypted file, not just the problem", () => {
const caveat = secretStorageCaveat(plaintextFile);
expect(caveat).toContain("unencrypted");
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("reports a mode it could not tighten, ahead of the encryption caveat", () => {
// A file other users can read is a live exposure; "unencrypted" is a
// property of a file only its owner can open. When both hold, the
// permission problem is the one to say out loud.
const loose: SecretStorageInfo = { ...plaintextFile, looseMode: 0o644 };
expect(secretStorageTone(loose)).toBe("warn");
expect(secretStorageCaveat(loose)).toContain("0644");
expect(secretStorageCaveat(loose)).not.toContain(
"MCP_INSPECTOR_SECRET_KEY",
);
});
it("does not claim exposure when the encryption state is unknown", () => {
// `looseMode` with `encryptionUnknown` leaves `plaintext` absent, and a
// `!== false` test then lands on the plaintext wording — asserting that
// a reader gets the secrets. The envelope may hold ciphertext; we could
// not classify it. Claiming the worse of two unknowns is still claiming
// something we never established.
const loose: SecretStorageInfo = {
kind: "file",
reason: "fallback",
durable: true,
looseMode: 0o644,
encryptionUnknown: "not valid JSON",
path: "/tmp/secrets.json",
};
const caveat = secretStorageCaveat(loose);
expect(secretStorageTone(loose)).toBe("warn");
expect(caveat).toContain("0644");
expect(caveat).toContain("could not determine");
expect(caveat).not.toContain("anyone who can read it can read the secrets");
});
it("warns about a loose mode even on an encrypted file", () => {
// The passphrase is then the only thing between a reader and the values,
// which is worth saying rather than rendering the quiet neutral state.
const loose: SecretStorageInfo = { ...encryptedFile, looseMode: 0o640 };
expect(secretStorageTone(loose)).toBe("warn");
expect(secretStorageCaveat(loose)).toContain("0640");
});
it("does not overstate exposure for an encrypted file with a loose mode", () => {
// A reader of an encrypted file gets ciphertext, not secrets. Saying
// otherwise trains people to discount the warning on the occasion it is
// literally true.
const loose: SecretStorageInfo = { ...encryptedFile, looseMode: 0o644 };
const caveat = secretStorageCaveat(loose);
expect(caveat).toContain("0644");
expect(caveat).toContain("passphrase is then the only thing");
expect(caveat).not.toContain("can read the secrets in it");
});
it("keeps the blunt wording for a plaintext file with a loose mode", () => {
const loose: SecretStorageInfo = { ...plaintextFile, looseMode: 0o644 };
expect(secretStorageCaveat(loose)).toContain("can read the secrets in it");
});
it("says a file it could not read is unreadable, and warns about the next save", () => {
const unreadable: SecretStorageInfo = {
kind: "file",
reason: "fallback",
durable: true,
path: "/x/secrets.json",
encryptionUnknown: "not valid JSON",
};
// Neither "encrypted" nor "unencrypted" is a claim we can make here.
expect(secretStorageLabel(unreadable)).toBe("File (unreadable)");
expect(secretStorageTone(unreadable)).toBe("warn");
expect(secretStorageCaveat(unreadable)).toMatch(
/could not be read \(not valid JSON\)\. Saving a secret will fail/,
);
});
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.
expect(secretStorageCaveat({ ...memory, plaintext: true })).toContain(
"lost on exit",
);
});
});
describe("secretStorageSummary", () => {
it("is just the label when there is nothing else to say", () => {
expect(secretStorageSummary(keyring)).toBe("OS keychain");
});
it("includes the path so a file-backed store is findable", () => {
expect(secretStorageSummary(encryptedFile)).toContain(
"/home/node/.mcp-inspector/secrets.json",
);
});
it("marks a fallback as a fallback", () => {
// The banner has to distinguish "you configured this" from "we had to".
expect(secretStorageSummary(memory)).toContain("fell back");
expect(
secretStorageSummary({ ...memory, reason: "configured" }),
).not.toContain("fell back");
});
});

193
clients/web/src/test/core/client/node-persistence.test.ts
View file

@ -5,6 +5,7 @@ import * as os from "node:os";
import * as path from "node:path";
import {
InMemorySecretStore,
SessionSecretStore,
KeychainUnavailableError,
SECRET_FIELD_IDP_CLIENT_SECRET,
type SecretStore,
@ -62,6 +63,52 @@ describe("client node-persistence", () => {
).toBe("plain");
});
it("aborts rather than overwriting when the keychain read fails", async () => {
// The keychain-wins lookup used the tolerant `get`, which maps an
// unreadable store to `null` — and the branch below writes on `null`. So
// a transient read failure let the older client.json copy overwrite a
// newer stored secret, and the disk copy was then stripped.
const filePath = await makeTmpFile(
JSON.stringify(configWithPlaintextSecret),
);
let wrote = false;
const flaky: SecretStore = {
get: async () => null,
getStrict: async () => {
throw new KeychainUnavailableError(new Error("temporarily down"));
},
set: async () => {
wrote = true;
},
delete: async () => {},
deleteAllForServer: async () => {},
};
const loaded = await readClientConfigStore(filePath, flaky);
expect(wrote).toBe(false);
// The plaintext survives for the next attempt.
expect(readFileSync(filePath, "utf-8")).toContain("plain");
expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("plain");
});
it("keeps the plaintext on disk when the store is session-scoped", async () => {
// The container fallback. Migrating here would delete a secret that
// survives restarts and keep only a copy that dies with the process —
// and it happens on an ordinary read, so merely loading the app would
// do it. The session still works: the value is loaded into the store.
const filePath = await makeTmpFile(
JSON.stringify(configWithPlaintextSecret),
);
const secretStore = new SessionSecretStore();
const loaded = await readClientConfigStore(filePath, secretStore);
expect(loaded.enterpriseManagedAuth?.idp.clientSecret).toBe("plain");
// The disk copy is deliberately left alone.
expect(readFileSync(filePath, "utf-8")).toContain("plain");
});
it("does not overwrite an existing keychain secret during migration", async () => {
const filePath = await makeTmpFile(
JSON.stringify(configWithPlaintextSecret),
@ -177,3 +224,149 @@ describe("client node-persistence", () => {
).toBeNull();
});
});
describe("session-scoped store keeps client.json durable (#1950 review r19)", () => {
it("does not strip the IdP secret when the store cannot outlive the process", async () => {
// The read-path migration already withheld its strip for a session
// store, but the write path did not — and `readClientConfigStore` hands
// the rehydrated secret to the form, which resends the whole object when
// an unrelated field changes. Saving a CIMD URL therefore moved the only
// durable copy into RAM, to be lost at exit.
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-"));
const file = path.join(dir, "client.json");
try {
// Legacy state: the secret is *already* on disk in plaintext, which is
// what the durability guard exists to preserve.
await fs.writeFile(
file,
JSON.stringify({
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "must-survive",
},
},
}),
"utf-8",
);
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "must-survive",
},
},
},
new SessionSecretStore(),
);
expect(await fs.readFile(file, "utf-8")).toContain("must-survive");
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
it("does not write a newly entered IdP secret to disk on a session store", async () => {
// Same overshoot as the server path: preserving *legacy* plaintext is
// right, treating every submitted value as legacy is not — it would put a
// freshly typed secret in `client.json` while the footer says a session
// store writes secrets nowhere.
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-fresh-"));
const file = path.join(dir, "client.json");
try {
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "never-typed-before",
},
},
},
new SessionSecretStore(),
);
expect(await fs.readFile(file, "utf-8")).not.toContain(
"never-typed-before",
);
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
it("keeps a new secret off disk when the prior file is unreadable", async () => {
// Provenance cannot be established from a file that will not parse, and
// the conservative direction is to treat the value as new — writing a
// secret to disk on a guess is the failure that matters.
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-bad-"));
const file = path.join(dir, "client.json");
try {
await fs.writeFile(file, "{ not json", "utf-8");
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "unprovenanced",
},
},
},
new SessionSecretStore(),
);
expect(await fs.readFile(file, "utf-8")).not.toContain("unprovenanced");
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
it("writes nothing extra when a session store has no IdP secret at all", async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-none-"));
const file = path.join(dir, "client.json");
try {
await writeClientConfigStore(
file,
{ cimd: { enabled: true, clientMetadataUrl: "https://x.test/cimd" } },
new SessionSecretStore(),
);
expect(await fs.readFile(file, "utf-8")).toContain("cimd");
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
it("still strips against a durable store, so the guard is the difference", async () => {
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "client-durable-"));
const file = path.join(dir, "client.json");
try {
await writeClientConfigStore(
file,
{
enterpriseManagedAuth: {
enabled: true,
idp: {
issuer: "https://idp.example/",
clientId: "cid",
clientSecret: "goes-to-the-store",
},
},
},
new InMemorySecretStore(),
);
expect(await fs.readFile(file, "utf-8")).not.toContain(
"goes-to-the-store",
);
} finally {
await fs.rm(dir, { recursive: true, force: true });
}
});
});

Some files were not shown because too many files have changed in this diff Show more