From bc408f58692af5950aab16dd8e85f6d509c6f80d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 12:06:31 -0400 Subject: [PATCH] feat(auth): file-backed and in-memory SecretStore for hosts with no keychain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall --- AGENTS.md | 24 +- README.md | 27 +- clients/web/server/server.ts | 9 +- clients/web/server/vite-hono-plugin.ts | 7 +- clients/web/server/web-server-config.ts | 22 +- clients/web/src/App.css | 10 + clients/web/src/App.tsx | 3 + .../SecretStorageFooter.stories.tsx | 105 +++++ .../SecretStorageFooter.test.tsx | 84 ++++ .../SecretStorageFooter.tsx | 139 ++++++ .../ClientSettingsModal.test.tsx | 42 ++ .../ClientSettingsModal.tsx | 11 + .../ServerSettingsModal.test.tsx | 40 ++ .../ServerSettingsModal.tsx | 11 + .../core/auth/secret-storage-info.test.ts | 117 +++++ .../test/core/react/useInitialConfig.test.tsx | 51 +++ .../auth/node/file-secret-store.test.ts | 411 ++++++++++++++++++ .../auth/node/secret-store-selection.test.ts | 384 ++++++++++++++++ .../auth/node/secret-store.test.ts | 193 ++++---- .../auth/node/secretStoreContract.ts | 146 +++++++ .../server/web-server-config.test.ts | 54 +++ clients/web/src/theme/Group.ts | 28 ++ core/auth/node/file-secret-store.ts | 400 +++++++++++++++++ core/auth/node/secret-store-selection.ts | 323 ++++++++++++++ core/auth/node/secret-store.ts | 105 ++++- core/auth/secret-storage-info.ts | 124 ++++++ core/client/node-persistence.ts | 4 +- core/client/runner.ts | 13 +- core/mcp/node/servers.ts | 11 +- core/mcp/remote/node/server.ts | 43 +- core/react/useInitialConfig.ts | 32 +- 31 files changed, 2827 insertions(+), 146 deletions(-) create mode 100644 clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.stories.tsx create mode 100644 clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx create mode 100644 clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx create mode 100644 clients/web/src/test/core/auth/secret-storage-info.test.ts create mode 100644 clients/web/src/test/integration/auth/node/file-secret-store.test.ts create mode 100644 clients/web/src/test/integration/auth/node/secret-store-selection.test.ts create mode 100644 clients/web/src/test/integration/auth/node/secretStoreContract.ts create mode 100644 core/auth/node/file-secret-store.ts create mode 100644 core/auth/node/secret-store-selection.ts create mode 100644 core/auth/secret-storage-info.ts diff --git a/AGENTS.md b/AGENTS.md index 0771175e..04f365f2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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 diff --git a/README.md b/README.md index 2c420c00..97e75e05 100644 --- a/README.md +++ b/README.md @@ -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 @@ -455,6 +457,29 @@ 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` to any passphrase and the file is encrypted with AES-256-GCM (the passphrase is stretched with scrypt against a per-file random salt). Without it the file is still `0600`, but the values are readable to anyone who can read the file — which the startup log and the settings footer both say, every session, in a warning tone: + +```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 +``` + +Setting the passphrase later is safe — the next write upgrades an existing plaintext file in place. **Changing or losing it is not**: a file that can no longer be decrypted is read as empty and *refuses to be written*, rather than being silently replaced with a new one holding only your latest secret. Restore the original passphrase, or delete `secrets.json` and re-enter the values. + +Two env vars override the automatic choice: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, and `MCP_INSPECTOR_SECRET_FILE` moves the file. + **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 diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index fe465530..39476682 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -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,7 @@ export async function startHonoServer( allowedOrigins: config.allowedOrigins, sandboxUrl: sandboxController.getUrl() ?? undefined, logger: config.logger, - initialConfig: webServerConfigToInitialPayload(config), + initialConfig: webServerConfigToInitialPayload(config, secretStorage), }); const app = new Hono(); @@ -123,6 +129,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 diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 880913e6..1be0674b 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -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,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { allowedOrigins: config.allowedOrigins, sandboxUrl: sandboxController.getUrl() ?? undefined, logger: config.logger, - initialConfig: webServerConfigToInitialPayload(config), + initialConfig: webServerConfigToInitialPayload(config, secretStorage), }); // Expose the resolved token to `transformIndexHtml`. Left empty when @@ -111,6 +115,7 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { actualPort, resolvedToken, sandboxUrl ?? undefined, + secretStorage, ); if (config.autoOpen) { diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 6bb13be2..a4359728 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -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..."); } diff --git a/clients/web/src/App.css b/clients/web/src/App.css index fb5d1aab..b208285c 100644 --- a/clients/web/src/App.css +++ b/clients/web/src/App.css @@ -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 diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index db29e06b..bae88517 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -741,6 +741,7 @@ function App() { sandboxUrl, writable: serverListWritable, version: inspectorVersion, + secretStorage, } = useInitialConfig({ baseUrl: configBaseUrl, authToken: getAuthToken(), @@ -4618,6 +4619,7 @@ function App() { onClearStoredOAuth={ settingsModalIsStdio ? undefined : handleClearStoredOAuthFromSettings } + secretStorage={secretStorage} /> {initializeResult && activeServer && ( = { + title: "Elements/SecretStorageFooter", + component: SecretStorageFooter, + args: { info: keyring }, +}; + +export default meta; +type Story = StoryObj; + +// The ordinary case on a desktop install: the OS keychain, stated quietly. +export const Keychain: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await expect( + await canvas.findByText("Secrets: OS keychain"), + ).toBeInTheDocument(); + await expect(canvas.getByTestId("secret-storage-footer")).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); + await expect( + await canvas.findByText("Secrets: File (encrypted)"), + ).toBeInTheDocument(); + await expect( + canvas.getByText("/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); + await expect( + await canvas.findByText("Secrets: File (unencrypted)"), + ).toBeInTheDocument(); + await expect(canvas.getByTestId("secret-storage-footer")).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); + await expect( + await canvas.findByText("Secrets: Memory (this session only)"), + ).toBeInTheDocument(); + await expect( + canvas.getByText(/lost when the Inspector exits/), + ).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(); + }, +}; diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx new file mode 100644 index 00000000..90dca4a2 --- /dev/null +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.test.tsx @@ -0,0 +1,84 @@ +import { describe, it, expect } from "vitest"; +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(); + expect(screen.getByText("Secrets: OS keychain")).toBeInTheDocument(); + expect(screen.getByTestId("secret-storage-footer")).toHaveAttribute( + "data-tone", + "neutral", + ); + }); + + it("shows the path for an encrypted file, quietly", () => { + // Nothing is wrong here, so the band must not shout — the path is the + // useful part and the tone stays neutral. + renderWithMantine(); + expect(screen.getByText("Secrets: File (encrypted)")).toBeInTheDocument(); + expect( + screen.getByText("/home/node/.mcp-inspector/secrets.json"), + ).toBeInTheDocument(); + expect(screen.getByTestId("secret-storage-footer")).toHaveAttribute( + "data-tone", + "neutral", + ); + }); + + it("renders the plaintext caveat inline, not only on hover", () => { + // This is the "loud banner" decision made concrete: a warning you have + // to hover to discover is not a warning. The caveat takes the visible + // slot, displacing the path (which stays in the tooltip). + renderWithMantine(); + expect(screen.getByText("Secrets: File (unencrypted)")).toBeInTheDocument(); + expect( + screen.getByText(/Set MCP_INSPECTOR_SECRET_KEY to encrypt them/), + ).toBeInTheDocument(); + expect(screen.getByTestId("secret-storage-footer")).toHaveAttribute( + "data-tone", + "warn", + ); + }); + + it("warns that an in-memory store loses secrets on exit", () => { + renderWithMantine(); + expect( + screen.getByText("Secrets: Memory (this session only)"), + ).toBeInTheDocument(); + expect( + screen.getByText(/lost when the Inspector exits/), + ).toBeInTheDocument(); + expect(screen.getByTestId("secret-storage-footer")).toHaveAttribute( + "data-tone", + "warn", + ); + }); + + 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(); + expect(screen.queryByTestId("secret-storage-footer")).toBeNull(); + }); +}); diff --git a/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx new file mode 100644 index 00000000..252e6a66 --- /dev/null +++ b/clients/web/src/components/elements/SecretStorageFooter/SecretStorageFooter.tsx @@ -0,0 +1,139 @@ +import { Group, Text, Tooltip } 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: "secretStorageFooter", + h: SECRET_STORAGE_FOOTER_HEIGHT, + px: "md", + gap: 6, + align: "center", + wrap: "nowrap", + w: "100%", +}); + +const FooterLabel = Text.withProps({ + size: "xs", + fw: 500, + variant: "nowrap", +}); + +// The detail — a path, or the caveat sentence — is the part allowed to lose +// characters, since the label before it carries the answer. +// +// `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 FooterDetail = Text.withProps({ + size: "xs", + lineClamp: 1, + miw: 0, + flex: 1, +}); + +const ICON_SIZE = 14; + +/** + * The permanent "here is where your secrets go" strip at the bottom of the + * Client Settings and Server Settings modals (#1950). + * + * Those two dialogs are the only places the Inspector accepts a secret — + * an OAuth client secret, an enterprise IdP client secret, a stdio `env:` + * value — so they are where the answer belongs. 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 caveat = secretStorageCaveat(info); + const warn = secretStorageTone(info) === "warn"; + const color = warn + ? "var(--inspector-warning-text)" + : "var(--inspector-text-secondary)"; + // The caveat wins the visible slot when there is one; otherwise the path, + // which only the (quiet) encrypted-file case has. + const detail = caveat ?? info.path; + const Icon = warn + ? TbAlertTriangle + : info.kind === "keyring" + ? TbDeviceDesktop + : TbLock; + + const band = ( + + {/* A react-icons glyph, not a Mantine factory component — outside the + `.withProps()` rule, and it takes `color` as a prop rather than a + style. */} + + Secrets: {secretStorageLabel(info)} + {detail && {detail}} + + ); + + // Tooltip content: the caveat plus the path when both exist (the unencrypted + // file case), since the visible slot can only show one of them. + const tip = [caveat, info.path].filter(Boolean).join(" "); + if (!tip) return band; + + return ( + // `multiline` + a width so the sentence wraps instead of forming a tooltip + // wider than the modal it explains. + + {band} + + ); +} diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx index 19cc880b..2e02df15 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx @@ -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( + , + ); + expect( + screen.getByText("Secrets: File (unencrypted)"), + ).toBeInTheDocument(); + expect(screen.getByTestId("secret-storage-footer")).toHaveAttribute( + "data-tone", + "warn", + ); + }); + + it("renders no footer when the backend didn't report a store", () => { + renderWithMantine( + , + ); + expect(screen.queryByTestId("secret-storage-footer")).toBeNull(); + }); + }); }); diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx index 17320037..7e4d1e12 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx @@ -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} /> + ); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index e627beb7..5479f22b 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -899,4 +899,44 @@ 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( + , + ); + expect( + screen.getByText("Secrets: Memory (this session only)"), + ).toBeInTheDocument(); + }); + + it("renders no footer when the backend didn't report a store", () => { + renderWithMantine( + , + ); + expect(screen.queryByTestId("secret-storage-footer")).toBeNull(); + }); + }); }); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index 35c1fb12..e2af62f5 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -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} /> + ); diff --git a/clients/web/src/test/core/auth/secret-storage-info.test.ts b/clients/web/src/test/core/auth/secret-storage-info.test.ts new file mode 100644 index 00000000..8c55c157 --- /dev/null +++ b/clients/web/src/test/core/auth/secret-storage-info.test.ts @@ -0,0 +1,117 @@ +/** + * 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 when the Inspector exits", + ); + }); + + 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("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 when the Inspector exits", + ); + }); +}); + +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"); + }); +}); diff --git a/clients/web/src/test/core/react/useInitialConfig.test.tsx b/clients/web/src/test/core/react/useInitialConfig.test.tsx index 3d246e88..ecf3aad4 100644 --- a/clients/web/src/test/core/react/useInitialConfig.test.tsx +++ b/clients/web/src/test/core/react/useInitialConfig.test.tsx @@ -249,4 +249,55 @@ describe("useInitialConfig", () => { expect(result.current.version).toBeUndefined(); expect(result.current.writable).toBe(true); }); + + describe("secretStorage (#1950)", () => { + // The footer under a secret field reads this, so the hook's job is to hand + // back only a descriptor it can vouch for. Every rejected shape must land + // on `undefined` — which the footer renders as nothing — rather than on a + // half-populated object that would render a confident wrong answer. + it("passes through a well-formed descriptor", async () => { + const info = { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/home/node/.mcp-inspector/secrets.json", + }; + const fetchFn = vi + .fn() + .mockResolvedValue(jsonResponse({ secretStorage: info })); + + const { result } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.secretStorage).toEqual(info); + }); + + it("is undefined on a backend that omits the field", async () => { + const fetchFn = vi.fn().mockResolvedValue(jsonResponse({})); + const { result } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.secretStorage).toBeUndefined(); + }); + + it.each([ + ["a non-object", "keyring"], + ["null", null], + ["an unknown kind", { kind: "vault", durable: true }], + ["a missing kind", { durable: true, path: "/x" }], + ])("rejects %s", async (_label, value) => { + const fetchFn = vi + .fn() + .mockResolvedValue(jsonResponse({ secretStorage: value })); + const { result } = renderHook(() => + useInitialConfig({ baseUrl: "http://test.local", fetchFn }), + ); + await waitFor(() => expect(result.current.loading).toBe(false)); + expect(result.current.secretStorage).toBeUndefined(); + }); + }); }); diff --git a/clients/web/src/test/integration/auth/node/file-secret-store.test.ts b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts new file mode 100644 index 00000000..96152517 --- /dev/null +++ b/clients/web/src/test/integration/auth/node/file-secret-store.test.ts @@ -0,0 +1,411 @@ +/** + * `FileSecretStore` against a real filesystem (#1950). + * + * A real temp directory rather than a mocked `fs`: the properties worth + * asserting here are properties *of the file* — its mode, that a second + * store reading it back sees the same secrets, that the ciphertext does + * not contain the plaintext — and a mocked filesystem can be made to + * agree with any of those without them being true. + */ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { + FileSecretStore, + SecretFileKeyMismatchError, + tightenSecretFilePermissions, +} from "@inspector/core/auth/node/file-secret-store.js"; +import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js"; +import { SECRET_FIELD_OAUTH_CLIENT_SECRET } from "@inspector/core/auth/secret-fields.js"; +import { describeSecretStoreContract } from "./secretStoreContract.js"; + +let tmpDir: string; +const filePath = (): string => path.join(tmpDir, "secrets.json"); + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "inspector-secrets-")); +}); + +afterEach(async () => { + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +// Both modes must satisfy the same contract — encryption is a property of +// the bytes on disk, not of the interface, and a caller must not be able to +// tell which one it got. +describeSecretStoreContract( + "FileSecretStore (plaintext)", + () => new FileSecretStore({ filePath: filePath() }), +); +describeSecretStoreContract( + "FileSecretStore (encrypted)", + () => new FileSecretStore({ filePath: filePath(), passphrase: "hunter2" }), +); + +describe("FileSecretStore persistence", () => { + it("a second store instance reads what the first wrote", async () => { + const a = new FileSecretStore({ filePath: filePath() }); + await a.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + const b = new FileSecretStore({ filePath: filePath() }); + expect(await b.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe("shh"); + }); + + it("round-trips through a real encrypt/decrypt cycle across instances", async () => { + const a = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + await a.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + const b = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + expect(await b.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe("shh"); + }); + + it("writes the file 0600", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + const stat = await fs.stat(filePath()); + // Windows does not model POSIX modes; asserting there would fail for a + // reason that has nothing to do with this code. + if (process.platform !== "win32") { + expect(stat.mode & 0o777).toBe(0o600); + } + }); + + it("concurrent sets do not lose each other (the read-modify-write race)", async () => { + // The route handlers write a server's fields with `Promise.all`, so this + // is the ordinary path. Without the internal queue both calls read the + // same empty map and the second write drops the first entry. + const store = new FileSecretStore({ filePath: filePath() }); + await Promise.all([ + store.set("alpha", "env:A", "1"), + store.set("alpha", "env:B", "2"), + store.set("alpha", "env:C", "3"), + ]); + expect(await store.get("alpha", "env:A")).toBe("1"); + expect(await store.get("alpha", "env:B")).toBe("2"); + expect(await store.get("alpha", "env:C")).toBe("3"); + }); + + it("keeps the file after the last entry is deleted", async () => { + // The file's presence records which encryption mode this install settled + // on; unlinking it would let a later write silently switch modes. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + await store.deleteAllForServer("alpha"); + await expect(fs.stat(filePath())).resolves.toBeDefined(); + }); +}); + +describe("FileSecretStore at rest", () => { + it("plaintext mode stores the value readably (the state the warning is about)", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + expect(store.encrypted).toBe(false); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "super-secret"); + const raw = await fs.readFile(filePath(), "utf-8"); + expect(raw).toContain("super-secret"); + expect(JSON.parse(raw).encryption).toBe("none"); + }); + + it("encrypted mode leaves neither the value nor the account name on disk", async () => { + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + expect(store.encrypted).toBe(true); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "super-secret"); + const raw = await fs.readFile(filePath(), "utf-8"); + expect(raw).not.toContain("super-secret"); + // The account names are encrypted too — that's why the map is encrypted + // as a unit rather than value-by-value, since the key list would + // otherwise index which servers hold which kind of secret. + expect(raw).not.toContain("alpha"); + expect(raw).not.toContain(SECRET_FIELD_OAUTH_CLIENT_SECRET); + const parsed = JSON.parse(raw); + expect(parsed.encryption).toBe("aes-256-gcm"); + expect(parsed.kdf.algorithm).toBe("scrypt"); + }); + + it("treats a blank passphrase as no passphrase", async () => { + // `MCP_INSPECTOR_SECRET_KEY=` in a compose file means "off", not a + // zero-length key — and silently encrypting with "" would be worse than + // either reading. + expect( + new FileSecretStore({ filePath: filePath(), passphrase: " " }) + .encrypted, + ).toBe(false); + }); + + it("upgrades an existing plaintext file to encrypted on the next write", async () => { + const plain = new FileSecretStore({ filePath: filePath() }); + await plain.set("alpha", "env:A", "1"); + const encrypted = new FileSecretStore({ + filePath: filePath(), + passphrase: "hunter2", + }); + // Reads the plaintext file it inherited... + expect(await encrypted.get("alpha", "env:A")).toBe("1"); + // ...and the next write carries the old entry into the encrypted form, + // so setting the env var does not strand the secrets already stored. + await encrypted.set("alpha", "env:B", "2"); + const raw = await fs.readFile(filePath(), "utf-8"); + expect(JSON.parse(raw).encryption).toBe("aes-256-gcm"); + expect(await encrypted.get("alpha", "env:A")).toBe("1"); + }); +}); + +describe("FileSecretStore failure handling", () => { + const writeEncryptedFixture = async (): Promise => { + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + }; + + it("get returns null when the passphrase is wrong (read tolerance)", async () => { + await writeEncryptedFixture(); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", + }); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("set refuses rather than overwriting a file it cannot decrypt", async () => { + await writeEncryptedFixture(); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( + SecretFileKeyMismatchError, + ); + // The original secrets survive: the refusal is the whole point. + const original = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + expect(await original.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "shh", + ); + }); + + it("set names the missing passphrase when the key was removed entirely", async () => { + await writeEncryptedFixture(); + const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /MCP_INSPECTOR_SECRET_KEY is not set/, + ); + }); + + it("reads a plaintext file that carries no secrets key as empty", async () => { + // A hand-edited (or hand-created) file is the realistic source of this + // shape, and it must read as "no secrets yet" rather than throwing: the + // document is well-formed and declares its encryption mode, so the only + // thing missing is entries. Flattening it to `{}` is also what lets the + // next `set` add one instead of refusing. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 1, encryption: "none" }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "shh", + ); + }); + + it("delete stays silent on a file it cannot decrypt", async () => { + await writeEncryptedFixture(); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", + }); + await expect( + store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + }); + + it("set reports a corrupt file rather than silently replacing it", async () => { + await fs.writeFile(filePath(), "{ not json", "utf-8"); + const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, + ); + expect(await store.get("alpha", "env:A")).toBe(null); + }); + + it("set reports a file written in a format this build doesn't know", async () => { + // The forward-compat case: a newer Inspector's file, opened by an older + // one. Refusing beats truncating it to the entries we happen to parse. + await fs.writeFile( + filePath(), + JSON.stringify({ version: 99, encryption: "chacha20-poly1305" }), + "utf-8", + ); + const store = new FileSecretStore({ filePath: filePath() }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /unsupported format/, + ); + }); + + it("set reports a truncated ciphertext payload", async () => { + await writeEncryptedFixture(); + const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8")); + parsed.data = "only-one-part"; + await fs.writeFile(filePath(), JSON.stringify(parsed), "utf-8"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( + SecretFileKeyMismatchError, + ); + }); + + it("reports a file whose KDF parameters are unusable", async () => { + // scrypt rejects on out-of-grammar cost parameters (`N` must be a power of + // two > 1). Those parameters come off disk, so this is a corrupt-file case + // reachable without touching the crypto call site — and it must land as a + // refusal, not as an exception escaping the derivation. + await writeEncryptedFixture(); + const parsed = JSON.parse(await fs.readFile(filePath(), "utf-8")); + parsed.kdf.N = 3; + await fs.writeFile(filePath(), JSON.stringify(parsed), "utf-8"); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "right-key", + }); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( + SecretFileKeyMismatchError, + ); + }); + + it("reports a read failure that isn't a missing file", async () => { + // A directory where the file should be: `readStoreFile` only translates + // ENOENT into "no file yet", so anything else must surface as a refusal + // rather than being read as an empty store and overwritten. + const asDir = path.join(tmpDir, "as-dir"); + await fs.mkdir(asDir); + const store = new FileSecretStore({ filePath: asDir }); + await expect(store.set("alpha", "env:A", "1")).rejects.toBeInstanceOf( + SecretStoreUnavailableError, + ); + }); + + it("a rejected set leaves the same store usable for later operations", async () => { + // The internal queue swallows failures on its chain so one rejection can't + // strand every subsequent call on that store — the returned promise still + // rejects, which is what the caller sees. + await writeEncryptedFixture(); + const store = new FileSecretStore({ + filePath: filePath(), + passphrase: "wrong-key", + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow(); + expect(await store.get("alpha", "env:A")).toBe(null); + await expect(store.delete("alpha", "env:A")).resolves.toBeUndefined(); + await expect(store.set("alpha", "env:B", "2")).rejects.toThrow(); + }); + + it("delete is a no-op when the file exists but holds no matching entry", async () => { + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + await store.delete("beta", "env:A"); + await store.deleteAllForServer("gamma"); + expect(await store.get("alpha", "env:A")).toBe("1"); + }); + + it("set reports an unreachable location instead of failing silently", async () => { + // A path whose parent is a *file*: the read fails with ENOTDIR rather + // than the ENOENT that means "no file yet", so this lands on the read + // refusal. It is the class the 503 exists for — fixable outside the + // process. + const blocker = path.join(tmpDir, "blocker"); + await fs.writeFile(blocker, "x", "utf-8"); + const store = new FileSecretStore({ + filePath: path.join(blocker, "secrets.json"), + }); + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /Could not read the secrets file/, + ); + }); + + // A read that succeeds (or reports "no file yet") followed by a write that + // fails — a read-only mount, a full disk, a permissions change since + // startup. Reproduced with a non-writable directory, which requires not + // being root: `chmod` is advisory to uid 0, so under root the write would + // succeed and the assertion would be wrong rather than merely unexercised. + // CI runs as an unprivileged user, which is where the coverage gate is + // enforced. + const asRoot = process.getuid?.() === 0; + it.skipIf(asRoot || process.platform === "win32")( + "set reports a write failure distinctly from a read failure", + async () => { + const dir = path.join(tmpDir, "locked"); + await fs.mkdir(dir); + await fs.chmod(dir, 0o555); + try { + const store = new FileSecretStore({ + filePath: path.join(dir, "secrets.json"), + }); + // Nothing is there to read, so the failure is unambiguously the write. + await expect(store.set("alpha", "env:A", "1")).rejects.toThrow( + /Could not write the secrets file/, + ); + } finally { + // Restore write permission or the afterEach `rm -rf` can't clean up. + await fs.chmod(dir, 0o755); + } + }, + ); + + it("a rejected set does not poison later sets on the same store", async () => { + // The internal queue swallows on the chain but not on the returned + // promise; without that, one failure would strand every subsequent write. + const blocked = path.join(tmpDir, "blocker"); + await fs.writeFile(blocked, "x", "utf-8"); + const bad = new FileSecretStore({ + filePath: path.join(blocked, "secrets.json"), + }); + await expect(bad.set("alpha", "env:A", "1")).rejects.toThrow(); + + // A different store on a good path still works, and so does a second + // write on the failing one's successor. + const store = new FileSecretStore({ filePath: filePath() }); + await store.set("alpha", "env:A", "1"); + await store.set("alpha", "env:B", "2"); + expect(await store.get("alpha", "env:A")).toBe("1"); + expect(await store.get("alpha", "env:B")).toBe("2"); + }); +}); + +describe("tightenSecretFilePermissions", () => { + it("repairs a loosened mode on an existing file", async () => { + if (process.platform === "win32") return; + await fs.writeFile(filePath(), "{}", "utf-8"); + await fs.chmod(filePath(), 0o644); + await tightenSecretFilePermissions(filePath()); + expect((await fs.stat(filePath())).mode & 0o777).toBe(0o600); + }); + + it("is a no-op for a file that doesn't exist yet (the first-run case)", async () => { + await expect( + tightenSecretFilePermissions(path.join(tmpDir, "nope.json")), + ).resolves.toBeUndefined(); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts new file mode 100644 index 00000000..90b96bfe --- /dev/null +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -0,0 +1,384 @@ +/** + * The store-selection policy (#1950). + * + * The two questions this file exists to pin down are the ones a reviewer + * would want proof of, because getting either wrong silently changes where + * a user's secret goes: + * + * 1. an unreachable keychain falls back rather than failing, and says so; + * 2. the fallback is `memory` in a container with nothing durable + * mounted, and `file` otherwise. + * + * `resolveSecretStore` caches its answer for the process, so each case + * that exercises it goes through `vi.resetModules()` + a fresh dynamic + * import — the same technique the keyring-unloadable cases in + * `secret-store.test.ts` use, and for the same reason. The keychain probe + * is mocked because CI has no libsecret; the container and mount + * predicates are driven directly rather than through a real container, + * which is what makes them worth exporting. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { + chooseFallbackKind, + isOnMountPoint, + parseSecretStoreEnv, + warnAboutSecretStorage, +} from "@inspector/core/auth/node/secret-store-selection.js"; +import type { SecretStorageInfo } from "@inspector/core/auth/secret-storage-info.js"; + +const ENV_KEYS = [ + "MCP_INSPECTOR_SECRET_STORE", + "MCP_INSPECTOR_SECRET_FILE", + "MCP_INSPECTOR_SECRET_KEY", + "KUBERNETES_SERVICE_HOST", +]; + +let saved: Record; +let tmpDir: string; + +beforeEach(async () => { + saved = Object.fromEntries(ENV_KEYS.map((k) => [k, process.env[k]])); + for (const k of ENV_KEYS) delete process.env[k]; + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "inspector-selection-")); +}); + +afterEach(async () => { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + await fs.rm(tmpDir, { recursive: true, force: true }); + vi.restoreAllMocks(); +}); + +/** + * Load a fresh copy of the selection module with the keychain probe forced + * to a given answer. Fresh because the resolution is cached per module + * instance, which is exactly the behavior we don't want leaking between + * cases. + */ +async function loadWithProbe(available: boolean, detail = "no D-Bus session") { + vi.resetModules(); + vi.doMock("@inspector/core/auth/node/secret-store.js", async () => { + const actual = await vi.importActual< + typeof import("@inspector/core/auth/node/secret-store.js") + >("@inspector/core/auth/node/secret-store.js"); + return { + ...actual, + probeKeyringAvailable: vi.fn(async () => + available ? { available: true } : { available: false, detail }, + ), + }; + }); + return import("@inspector/core/auth/node/secret-store-selection.js"); +} + +describe("parseSecretStoreEnv", () => { + it("accepts each kind, case- and whitespace-insensitively", () => { + expect(parseSecretStoreEnv("keyring")).toBe("keyring"); + expect(parseSecretStoreEnv(" FILE ")).toBe("file"); + expect(parseSecretStoreEnv("Memory")).toBe("memory"); + }); + + it("treats unset and blank as unset", () => { + expect(parseSecretStoreEnv(undefined)).toBeUndefined(); + expect(parseSecretStoreEnv(" ")).toBeUndefined(); + }); + + it("warns and falls through on a typo rather than refusing to start", () => { + // A misspelled env var in a compose file should not be the reason the + // Inspector won't boot — the automatic policy is a working default. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + expect(parseSecretStoreEnv("keychain")).toBeUndefined(); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("keyring, file, memory"), + ); + }); +}); + +describe("chooseFallbackKind", () => { + it("uses memory in a container with nothing mounted", () => { + // The honest answer: a file in the writable layer is discarded by + // `docker run --rm` and by any image update, so promising durability + // there is worse than declining to. + expect(chooseFallbackKind({ container: true, mounted: false })).toBe( + "memory", + ); + }); + + it("uses file in a container once a volume is mounted", () => { + // Mounting the volume *is* the user saying this directory should + // survive, so no extra configuration should be needed to honor it. + expect(chooseFallbackKind({ container: true, mounted: true })).toBe("file"); + }); + + it("uses file outside a container, mounted or not", () => { + expect(chooseFallbackKind({ container: false, mounted: false })).toBe( + "file", + ); + expect(chooseFallbackKind({ container: false, mounted: true })).toBe( + "file", + ); + }); +}); + +describe("isContainer", () => { + /** + * Load the module with `node:fs` stubbed, so each detection signal can be + * exercised on a developer's laptop. The three exist because no single one + * covers the runtimes people use — Docker, Podman/containerd, and + * Kubernetes each announce themselves differently. + */ + async function loadWithFs(opts: { + dockerenv?: boolean; + cgroup?: string | Error; + }) { + vi.resetModules(); + vi.doMock("node:fs", async () => { + const actual = await vi.importActual("node:fs"); + return { + ...actual, + existsSync: (p: string) => + p === "/.dockerenv" ? !!opts.dockerenv : actual.existsSync(p), + readFileSync: (p: string, enc?: unknown) => { + if (p === "/proc/1/cgroup") { + if (opts.cgroup instanceof Error) throw opts.cgroup; + if (opts.cgroup === undefined) throw new Error("ENOENT"); + return opts.cgroup; + } + return actual.readFileSync(p, enc as never); + }, + }; + }); + return import("@inspector/core/auth/node/secret-store-selection.js"); + } + + afterEach(() => { + vi.doUnmock("node:fs"); + vi.resetModules(); + }); + + it("is true inside Kubernetes, without touching the filesystem", async () => { + process.env.KUBERNETES_SERVICE_HOST = "10.0.0.1"; + const mod = await loadWithFs({}); + expect(mod.isContainer()).toBe(true); + }); + + it("is true when /.dockerenv exists", async () => { + const mod = await loadWithFs({ dockerenv: true }); + expect(mod.isContainer()).toBe(true); + }); + + it("is true when a container runtime appears in /proc/1/cgroup", async () => { + const mod = await loadWithFs({ + cgroup: "0::/system.slice/containerd.service\n", + }); + expect(mod.isContainer()).toBe(true); + }); + + it("is false for a host cgroup line", async () => { + const mod = await loadWithFs({ + cgroup: "0::/user.slice/user-1000.slice\n", + }); + expect(mod.isContainer()).toBe(false); + }); + + it("is false when neither signal is readable (macOS, or no /proc)", async () => { + // Not a container as far as we know — and a false negative is the cheap + // direction, since it only biases the fallback between two working stores. + const mod = await loadWithFs({ cgroup: new Error("ENOENT") }); + expect(mod.isContainer()).toBe(false); + }); +}); + +describe("isOnMountPoint", () => { + it("is false for an ordinary directory", () => { + expect(isOnMountPoint(tmpDir)).toBe(false); + }); + + it("is false for a path that doesn't exist yet, walking to its parent", () => { + // The first-run shape: `~/.mcp-inspector` has not been created. Answering + // from the nearest existing ancestor is what keeps this from being a + // missing-file error at startup. + expect(isOnMountPoint(path.join(tmpDir, "nope", "deeper"))).toBe(false); + }); + + it("is false when the path itself can't be resolved", () => { + // A NUL byte makes `path.resolve` throw. Any failure to answer the + // question resolves to "not a mount", which keeps a container on the + // conservative side — memory, which never promises what it can't keep. + expect(isOnMountPoint("bad\0path")).toBe(false); + }); + + it("is false at the filesystem root", () => { + // `/` is its own parent and is trivially a mount — reporting true there + // would make every container look like it had a volume. + expect(isOnMountPoint(path.parse(process.cwd()).root)).toBe(false); + }); +}); + +describe("warnAboutSecretStorage", () => { + const base: SecretStorageInfo = { + kind: "keyring", + reason: "default", + durable: true, + }; + + it("says nothing for the ordinary keychain case", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + warnAboutSecretStorage(base); + expect(warn).not.toHaveBeenCalled(); + }); + + it("announces a fallback, names the cause, and points at the override", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + warnAboutSecretStorage({ + kind: "memory", + reason: "fallback", + durable: false, + detail: "no D-Bus session", + }); + const output = warn.mock.calls.flat().join("\n"); + expect(output).toContain("OS keychain is not available"); + expect(output).toContain("no D-Bus session"); + expect(output).toContain("MCP_INSPECTOR_SECRET_STORE"); + // The lossy-store caveat rides along — this is the "loud" half of + // "automatic fallback with a loud banner". + expect(output).toContain("lost when the Inspector exits"); + }); + + it("warns about an unencrypted file even when it was explicitly chosen", () => { + // `reason: "configured"` means the user picked `file`; it does not mean + // they knew it would be unencrypted. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + warnAboutSecretStorage({ + kind: "file", + reason: "configured", + durable: true, + plaintext: true, + path: "/tmp/secrets.json", + }); + expect(warn.mock.calls.flat().join("\n")).toContain( + "MCP_INSPECTOR_SECRET_KEY", + ); + }); +}); + +describe("resolveSecretStore", () => { + it("uses the keychain when the probe succeeds", async () => { + const mod = await loadWithProbe(true); + const { info } = await mod.resolveSecretStore(); + expect(info).toMatchObject({ kind: "keyring", reason: "default" }); + }); + + it("caches the resolution so every consumer describes the same store", async () => { + // The banner, `/api/config`, and the store doing the writing are three + // readers of one decision; a re-probe that flipped would have the UI + // naming a store nobody is using. + const mod = await loadWithProbe(true); + expect(await mod.resolveSecretStore()).toBe(await mod.resolveSecretStore()); + }); + + it("falls back with a reason and a detail when the probe fails", async () => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + const mod = await loadWithProbe(false, "Couldn't access platform storage"); + const { info, store } = await mod.resolveSecretStore(); + expect(info.reason).toBe("fallback"); + expect(info.detail).toContain("Couldn't access platform storage"); + // And the store it hands back actually works — the point of falling back. + await store.set("alpha", "env:A", "1"); + expect(await store.get("alpha", "env:A")).toBe("1"); + }); + + it("honors an explicit memory store without probing", async () => { + process.env.MCP_INSPECTOR_SECRET_STORE = "memory"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const { info } = await mod.resolveSecretStore(); + expect(info).toMatchObject({ + kind: "memory", + reason: "configured", + durable: false, + }); + }); + + it("honors an explicit file store, reporting its path and encryption state", async () => { + const target = path.join(tmpDir, "custom", "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + process.env.MCP_INSPECTOR_SECRET_FILE = target; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const { info } = await mod.resolveSecretStore(); + expect(info).toMatchObject({ + kind: "file", + reason: "configured", + path: target, + plaintext: true, + durable: true, + }); + }); + + it("reports an encrypted file when the passphrase is set", async () => { + process.env.MCP_INSPECTOR_SECRET_STORE = "file"; + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_KEY = "hunter2"; + const mod = await loadWithProbe(true); + const { info } = await mod.resolveSecretStore(); + expect(info.plaintext).toBe(false); + }); + + it("getSecretStorageInfo returns the same descriptor as the resolution", async () => { + const mod = await loadWithProbe(true); + const { info } = await mod.resolveSecretStore(); + expect(await mod.getSecretStorageInfo()).toBe(info); + }); +}); + +describe("defaultSecretStore", () => { + it("delegates every operation to the resolved store", async () => { + // The deferred wrapper exists so the synchronous `?? defaultSecretStore()` + // call sites keep working; if it stopped forwarding, secrets would vanish + // into an object nobody reads. + process.env.MCP_INSPECTOR_SECRET_STORE = "memory"; + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const store = mod.defaultSecretStore(); + + await store.set("alpha", "env:A", "1"); + await store.set("alpha", "env:B", "2"); + await store.set("beta", "env:A", "3"); + expect(await store.get("alpha", "env:A")).toBe("1"); + + await store.delete("alpha", "env:A"); + expect(await store.get("alpha", "env:A")).toBe(null); + + await store.deleteAllForServer("alpha"); + expect(await store.get("alpha", "env:B")).toBe(null); + expect(await store.get("beta", "env:A")).toBe("3"); + + // And it is the *same* underlying store the resolution reports. + const { store: resolvedStore } = await mod.resolveSecretStore(); + expect(await resolvedStore.get("beta", "env:A")).toBe("3"); + }); +}); + +describe("defaultSecretFilePath", () => { + it("resolves a relative override to an absolute path", async () => { + // The path is shown in the UI footer, so a relative one would be + // ambiguous about which directory it is relative to. + process.env.MCP_INSPECTOR_SECRET_FILE = "./rel/secrets.json"; + const mod = await loadWithProbe(true); + expect(path.isAbsolute(mod.defaultSecretFilePath())).toBe(true); + }); + + it("defaults to ~/.mcp-inspector/secrets.json", async () => { + const mod = await loadWithProbe(true); + expect(mod.defaultSecretFilePath()).toContain( + path.join(".mcp-inspector", "secrets.json"), + ); + }); +}); diff --git a/clients/web/src/test/integration/auth/node/secret-store.test.ts b/clients/web/src/test/integration/auth/node/secret-store.test.ts index 2d66858e..a2e079ed 100644 --- a/clients/web/src/test/integration/auth/node/secret-store.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store.test.ts @@ -17,6 +17,7 @@ * its own describe built on `vi.resetModules()`. */ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { describeSecretStoreContract } from "./secretStoreContract.js"; // The mock must be hoisted above the `await import` of secret-store // inside the `KeyringSecretStore` describe block. Use `vi.hoisted` so @@ -24,6 +25,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; const keyringMocks = vi.hoisted(() => { const password = new Map(); const reset = () => password.clear(); + const storedAccounts = (): string[] => [...password.keys()]; // Behavior hooks each test can flip to simulate keychain unavailability // on specific operations. Defaults: real-ish in-memory behavior. const failures = { @@ -77,7 +79,14 @@ const keyringMocks = vi.hoisted(() => { if (failures.findThrows) throw new Error("keychain find unavailable"); return credentials(); }; - return { AsyncEntry, findCredentialsAsync, failures, password, reset }; + return { + AsyncEntry, + findCredentialsAsync, + failures, + password, + reset, + storedAccounts, + }; }); vi.mock("@napi-rs/keyring", () => ({ @@ -90,115 +99,20 @@ import { KeyringSecretStore, KeychainUnavailableError, KeyringModuleShapeError, + probeKeyringAvailable, SECRET_FIELD_OAUTH_CLIENT_SECRET, envSecretField, parseAccount, - type SecretStore, } from "@inspector/core/auth/node/secret-store.js"; -describe("InMemorySecretStore", () => { - let store: SecretStore; - - beforeEach(() => { - store = new InMemorySecretStore(); - }); - - it("returns null for a missing entry", async () => { - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - null, - ); - }); - - it("round-trips a value set then get", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - "shh", - ); - }); - - it("treats different server ids as separate namespaces", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "alpha-secret"); - await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "beta-secret"); - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - "alpha-secret", - ); - expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - "beta-secret", - ); - }); - - it("treats different fields under the same server id as separate entries", async () => { - await store.set("alpha", envSecretField("API_KEY"), "k1"); - await store.set("alpha", envSecretField("DB_PASS"), "k2"); - expect(await store.get("alpha", envSecretField("API_KEY"))).toBe("k1"); - expect(await store.get("alpha", envSecretField("DB_PASS"))).toBe("k2"); - }); - - it("overwrites an existing entry on set", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v1"); - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v2"); - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - "v2", - ); - }); - - it("delete is a no-op for a missing entry", async () => { - await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); - // No throw, no state change. - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - null, - ); - }); - - it("delete removes only the targeted (id, field)", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); - await store.set("alpha", envSecretField("KEY"), "b"); - await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - null, - ); - expect(await store.get("alpha", envSecretField("KEY"))).toBe("b"); - }); - - it("deleteAllForServer removes every field under that id", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); - await store.set("alpha", envSecretField("KEY1"), "b"); - await store.set("alpha", envSecretField("KEY2"), "c"); - await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "untouched"); - - await store.deleteAllForServer("alpha"); - - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - null, - ); - expect(await store.get("alpha", envSecretField("KEY1"))).toBe(null); - expect(await store.get("alpha", envSecretField("KEY2"))).toBe(null); - expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - "untouched", - ); - }); - - it("deleteAllForServer does not delete entries on a different id that happens to share a prefix", async () => { - // The account scheme is `${serverId}:${field}` — a literal prefix match - // would incorrectly sweep "alpha-prime" entries when deleting "alpha". - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); - await store.set("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET, "p"); - - await store.deleteAllForServer("alpha"); - - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( - null, - ); - expect( - await store.get("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET), - ).toBe("p"); - }); - - it("round-trips an empty-string value (set + get returns '')", async () => { - await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, ""); - expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(""); - }); -}); +// The generic cases live in `secretStoreContract.ts` and are shared with +// `FileSecretStore` (#1950) — a contract asserted against one implementation +// is not a contract. `KeyringSecretStore` keeps its own (failure-focused) +// block below, since it has no reachable backend in CI. +describeSecretStoreContract( + "InMemorySecretStore", + () => new InMemorySecretStore(), +); describe("parseAccount", () => { it("splits `${serverId}:${field}` on the first colon", () => { @@ -332,6 +246,22 @@ describe("KeyringSecretStore (mocked native bindings)", () => { ).toBe("p"); }); + it("deleteAllForServer skips an account it cannot parse", async () => { + // `findCredentialsAsync` returns whatever is in the keychain, including + // an entry this build didn't write. An empty field yields the account + // "alpha:", which `parseAccount` rejects — the guard is what keeps a + // stray entry from being treated as belonging to this server. + await store.set("alpha", "", "orphan"); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "real"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", "")).toBe("orphan"); + }); + it("KeychainUnavailableError stringifies a non-Error cause", () => { const err = new KeychainUnavailableError("plain string cause"); expect(err).toBeInstanceOf(KeychainUnavailableError); @@ -447,6 +377,44 @@ describe("KeyringSecretStore (mocked native bindings)", () => { await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); }); }); + + describe("probeKeyringAvailable (#1950)", () => { + // The input to the automatic-fallback policy. It has to answer "is the + // keychain *usable*", not "did the package import" — the container that + // motivated #1848 imports the package fine and only fails when + // `AsyncEntry::new` reaches for a Secret Service that isn't there. + it("reports available when a read round-trips", async () => { + await expect(probeKeyringAvailable()).resolves.toEqual({ + available: true, + }); + }); + + it("reports unavailable, with the cause, when construction throws", async () => { + keyringMocks.failures.constructorThrows = true; + const result = await probeKeyringAvailable(); + expect(result.available).toBe(false); + // The detail is what the fallback banner prints, and the three + // realistic causes need different fixes — so it is surfaced rather + // than summarized. + expect(result).toHaveProperty("detail"); + expect((result as { detail: string }).detail).toContain( + "Couldn't access platform storage", + ); + }); + + it("reports unavailable when the read itself throws", async () => { + keyringMocks.failures.getThrows = true; + expect((await probeKeyringAvailable()).available).toBe(false); + }); + + it("never writes to the user's keychain", async () => { + // Deliberate: a write probe would be a stronger signal but would + // deposit a value in someone's login keyring at every startup, for a + // store they may never use. + await probeKeyringAvailable(); + expect(keyringMocks.storedAccounts()).toEqual([]); + }); + }); }); describe("@napi-rs/keyring unloadable on this platform (#1905)", () => { @@ -539,6 +507,21 @@ describe("@napi-rs/keyring unloadable on this platform (#1905)", () => { await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); }); + it("the availability probe reports unavailable and names the load error", async () => { + // The fallback policy's input on Android/Termux: nothing to probe, so + // the answer must come from the load failure rather than from an + // attempted construction that would throw somewhere less legible. + const mod = await importWithUnloadableKeyring(); + const result = await mod.probeKeyringAvailable(); + expect(result.available).toBe(false); + // The detail carries the loader's own message. Asserted as non-empty + // rather than by content: vitest replaces whatever a throwing `doMock` + // factory threw with its own "error when mocking a module" text (see the + // hint-branch note above), so the literal `Cannot find module` string + // never reaches us here even though it does in production. + expect((result as { detail: string }).detail).not.toBe(""); + }); + it("attempts the load once and caches the failure", async () => { // Without the cache every secret operation re-attempts (and re-throws) // the resolution — `expectedSecretFields` means that is once per diff --git a/clients/web/src/test/integration/auth/node/secretStoreContract.ts b/clients/web/src/test/integration/auth/node/secretStoreContract.ts new file mode 100644 index 00000000..e92079a3 --- /dev/null +++ b/clients/web/src/test/integration/auth/node/secretStoreContract.ts @@ -0,0 +1,146 @@ +/** + * The behavioral contract every `SecretStore` implementation must satisfy, + * as a reusable block of `it()`s. + * + * These assertions were written once against `InMemorySecretStore` and are + * now shared with `FileSecretStore` (#1950), which is the point: the whole + * design rests on callers not knowing which store they got, and a contract + * asserted against only one implementation cannot support that claim. The + * cases that matter most are the two that look like edge cases and are + * not — the empty-string value (a real value that must round-trip, which + * is why the interface returns `null` for "absent" rather than falsy) and + * the shared-prefix server id (`alpha` vs `alpha-prime`, where a literal + * prefix sweep in `deleteAllForServer` deletes the wrong server's + * secrets). + * + * `KeyringSecretStore` is deliberately *not* run through this block: it + * has no reachable backend in CI, so its own file drives a mocked native + * layer and asserts the failure half of the contract instead. + */ +import { describe, it, expect, beforeEach } from "vitest"; +import type { SecretStore } from "@inspector/core/auth/node/secret-store.js"; +import { + SECRET_FIELD_OAUTH_CLIENT_SECRET, + envSecretField, +} from "@inspector/core/auth/secret-fields.js"; + +/** + * @param name Suite label. + * @param makeStore Produces a *fresh, empty* store. Called per test — a + * file-backed implementation uses this to point at a new temp path, so + * the shared cases stay ignorant of where the state lives. + */ +export function describeSecretStoreContract( + name: string, + makeStore: () => SecretStore | Promise, +): void { + describe(`${name} (SecretStore contract)`, () => { + let store: SecretStore; + + beforeEach(async () => { + store = await makeStore(); + }); + + it("returns null for a missing entry", async () => { + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("round-trips a value set then get", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "shh"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "shh", + ); + }); + + it("treats different server ids as separate namespaces", async () => { + await store.set( + "alpha", + SECRET_FIELD_OAUTH_CLIENT_SECRET, + "alpha-secret", + ); + await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "beta-secret"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "alpha-secret", + ); + expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "beta-secret", + ); + }); + + it("treats different fields under the same server id as separate entries", async () => { + await store.set("alpha", envSecretField("API_KEY"), "k1"); + await store.set("alpha", envSecretField("DB_PASS"), "k2"); + expect(await store.get("alpha", envSecretField("API_KEY"))).toBe("k1"); + expect(await store.get("alpha", envSecretField("DB_PASS"))).toBe("k2"); + }); + + it("overwrites an existing entry on set", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v1"); + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v2"); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "v2", + ); + }); + + it("delete is a no-op for a missing entry", async () => { + await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); + // No throw, no state change. + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("delete removes only the targeted (id, field)", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha", envSecretField("KEY"), "b"); + await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", envSecretField("KEY"))).toBe("b"); + }); + + it("deleteAllForServer removes every field under that id", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha", envSecretField("KEY1"), "b"); + await store.set("alpha", envSecretField("KEY2"), "c"); + await store.set("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET, "untouched"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect(await store.get("alpha", envSecretField("KEY1"))).toBe(null); + expect(await store.get("alpha", envSecretField("KEY2"))).toBe(null); + expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "untouched", + ); + }); + + it("deleteAllForServer does not delete entries on a different id that happens to share a prefix", async () => { + // The account scheme is `${serverId}:${field}` — a literal prefix match + // would incorrectly sweep "alpha-prime" entries when deleting "alpha". + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a"); + await store.set("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET, "p"); + + await store.deleteAllForServer("alpha"); + + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + expect( + await store.get("alpha-prime", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).toBe("p"); + }); + + it("round-trips an empty-string value (set + get returns '')", async () => { + await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, ""); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + "", + ); + }); + }); +} diff --git a/clients/web/src/test/integration/server/web-server-config.test.ts b/clients/web/src/test/integration/server/web-server-config.test.ts index 7dbb2e4a..df51e8ab 100644 --- a/clients/web/src/test/integration/server/web-server-config.test.ts +++ b/clients/web/src/test/integration/server/web-server-config.test.ts @@ -583,6 +583,28 @@ describe("webServerConfigToInitialPayload", () => { expect(payload.defaultTransport).toBe("streamable-http"); expect(payload.defaultServerUrl).toBe("https://srv/other"); }); + + it("omits secretStorage when the caller didn't resolve a store", () => { + // The UI treats an absent descriptor as "unknown" and shows nothing. A + // defaulted value would put a confident wrong answer under a secret field. + expect(webServerConfigToInitialPayload(baseConfig()).secretStorage).toBe( + undefined, + ); + }); + + it("passes the resolved store through verbatim (#1950)", () => { + const secretStorage = { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/home/node/.mcp-inspector/secrets.json", + } as const; + expect( + webServerConfigToInitialPayload(baseConfig(), secretStorage) + .secretStorage, + ).toEqual(secretStorage); + }); }); describe("printServerBanner", () => { @@ -668,6 +690,38 @@ describe("printServerBanner", () => { printServerBanner(withAuto, 6274, "tok", undefined); expect(logSpy.lines.some((l) => l.includes("Opening browser"))).toBe(true); }); + + it("prints the secret store for every kind, not only the alarming ones", () => { + // A line that appears only when something is wrong can't be used to + // confirm that nothing is. + printServerBanner(baseConfig(), 6274, "tok", undefined, { + kind: "keyring", + reason: "default", + durable: true, + }); + expect(logSpy.lines.some((l) => l.includes("Secrets: OS keychain"))).toBe( + true, + ); + }); + + it("names the path and the fallback for a file-backed store", () => { + printServerBanner(baseConfig(), 6274, "tok", undefined, { + kind: "file", + reason: "fallback", + durable: true, + plaintext: true, + path: "/home/node/.mcp-inspector/secrets.json", + }); + const line = logSpy.lines.find((l) => l.includes("Secrets:")); + expect(line).toContain("File (unencrypted)"); + expect(line).toContain("/home/node/.mcp-inspector/secrets.json"); + expect(line).toContain("fell back"); + }); + + it("prints no secrets line when no store was resolved", () => { + printServerBanner(baseConfig(), 6274, "tok", undefined); + expect(logSpy.lines.some((l) => l.includes("Secrets:"))).toBe(false); + }); }); // Minimal console.log capture so banner-format assertions don't pollute stdout. diff --git a/clients/web/src/theme/Group.ts b/clients/web/src/theme/Group.ts index de6623cf..d6e5b0df 100644 --- a/clients/web/src/theme/Group.ts +++ b/clients/web/src/theme/Group.ts @@ -13,4 +13,32 @@ export const ThemeGroup = Group.extend({ if (props.variant === "sectionHeader") return { root: "filter-toggle" }; return {}; }, + styles: (_theme, props) => { + // `secretStorageFooter` is the permanent band at the bottom of the + // Client/Server Settings modals naming where secrets are stored (#1950). + // Flat properties, so they belong here rather than in App.css. + // + // `position: sticky` is the load-bearing part, and it is the mirror of how + // Mantine pins the modal header. `Modal.Content` wraps *all* of its + // children — header, body, and this band — in one scroll area sized to the + // viewport, so a plain block at the end simply scrolls off the bottom with + // the body (measured: at 900px tall the band landed 140px below the fold). + // A footer the user has to scroll to find is not the permanent statement + // this is supposed to be, so it sticks to the bottom edge instead, with + // the same opaque body background and z-index the sticky header uses so + // content passes underneath rather than through it. + if (props.variant === "secretStorageFooter") { + return { + root: { + position: "sticky", + bottom: 0, + zIndex: 1000, + backgroundColor: "var(--mantine-color-body)", + borderTop: "1px solid var(--inspector-border-default)", + flexShrink: 0, + }, + }; + } + return { root: {} }; + }, }); diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts new file mode 100644 index 00000000..a61e2fc2 --- /dev/null +++ b/core/auth/node/file-secret-store.ts @@ -0,0 +1,400 @@ +/** + * File-backed {@link SecretStore} — the persistence answer for a box with + * no reachable OS keychain (#1950): the published container (no D-Bus + * session, #1848), Android/Termux (no platform binary, #1905), and a + * minimal Linux install without libsecret. + * + * **This partially reverses #1356 on purpose.** That change moved + * `oauthClientSecret` *out of* `mcp.json` and into the keychain to get it + * off disk. Writing secrets back to disk here is therefore a decision, not + * an oversight, and it is bounded three ways: the file is separate from + * `mcp.json` (so a user pasting their catalog into an issue does not paste + * their secrets), it is written `0600`, and it is encrypted whenever + * `MCP_INSPECTOR_SECRET_KEY` is set. What it buys is the alternative: + * before this, those users could not persist a secret at all — `set` threw + * and the route answered 503. + * + * **Format.** One JSON document, whose whole secret map is encrypted as a + * unit rather than value-by-value: + * + * ```json + * { "version": 1, "encryption": "none", "secrets": { "srv:field": "s3cr3t" } } + * { "version": 1, "encryption": "aes-256-gcm", + * "kdf": { "algorithm": "scrypt", "salt": "…", "N": 16384, "r": 8, "p": 1 }, + * "data": ".." } + * ``` + * + * Encrypting the map as a unit — not each value — is what hides the + * *account names* too. Those are `${serverId}:${field}`, so a per-value + * scheme would leave a readable index of which servers you use and which + * of them you hold an OAuth client secret for. That index is worth + * roughly as much to an attacker as some of the values. + * + * **Key.** `MCP_INSPECTOR_SECRET_KEY` is a passphrase, not a key: it is + * stretched with scrypt against a per-file random salt stored beside the + * ciphertext. That means a short, memorable value is safe to use, and it + * means the same passphrase produces a different key for a different file. + * + * **Availability contract — identical to `KeyringSecretStore`'s**, because + * callers must not have to know which store they got. `get` is tolerant + * (returns `null` on any failure), `delete` no-ops, and `set` is the only + * operation that hard-fails, throwing {@link SecretStoreUnavailableError} + * — the moment where a value would actually be lost. Routes translate that + * to a 503 the same way they already do for the keychain. + * + * The one failure worth calling out is the *key mismatch*: a file written + * encrypted, opened later with the passphrase changed or removed. Reads go + * quiet (`null`, per the contract) but writes must not — writing would + * replace a file full of still-valid secrets with one holding a single new + * one, destroying data to satisfy a request that was only ever additive. + * So `set` refuses and says which of the two things to fix. + */ + +import * as crypto from "node:crypto"; +import * as fs from "node:fs/promises"; +import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; +import { + SecretStoreUnavailableError, + type SecretStore, +} from "./secret-store.js"; + +/** + * Promisified `scrypt`. Hand-wrapped rather than `promisify`d because the + * overload carrying the cost parameters is not the one `promisify`'s types + * pick up, and the alternative — `scryptSync` — blocks the event loop for + * the duration of the derivation on every read and write. + */ +const scrypt = ( + passphrase: string, + salt: Buffer, + keylen: number, + options: crypto.ScryptOptions, +): Promise => + new Promise((resolve, reject) => { + crypto.scrypt(passphrase, salt, keylen, options, (err, key) => { + /* v8 ignore next -- @preserve: node validates the cost parameters + synchronously (an out-of-grammar `N` throws from the call itself, + which the corrupt-KDF test exercises), so this asynchronous channel + is left for allocation failure — not provokable deterministically. */ + if (err) reject(err); + else resolve(key); + }); + }); + +const FORMAT_VERSION = 1; +const CIPHER = "aes-256-gcm"; +const KEY_BYTES = 32; +const IV_BYTES = 12; +/** scrypt defaults. `N` is the only one that meaningfully costs time; 2^14 + * derives in a few milliseconds, which is invisible next to the file I/O it + * accompanies and is derived once per read/write rather than per secret. */ +const SCRYPT_N = 16384; +const SCRYPT_R = 8; +const SCRYPT_P = 1; + +/** Env var holding the passphrase. Absent → the file is written in the clear. */ +export const SECRET_KEY_ENV = "MCP_INSPECTOR_SECRET_KEY"; + +interface KdfParams { + algorithm: "scrypt"; + salt: string; + N: number; + r: number; + p: number; +} + +interface SecretFile { + version: number; + encryption: "none" | "aes-256-gcm"; + kdf?: KdfParams; + secrets?: Record; + data?: string; +} + +const buildAccount = (serverId: string, field: string): string => + `${serverId}:${field}`; + +/** + * Thrown when a file was written encrypted and the current passphrase + * cannot open it. A distinct type so `set` can refuse with advice that + * fits, instead of the generic "could not write" that would send someone + * looking at permissions. + */ +export class SecretFileKeyMismatchError extends SecretStoreUnavailableError { + constructor(filePath: string, hasKey: boolean) { + super( + hasKey + ? `The secrets file at ${filePath} could not be decrypted with the current ${SECRET_KEY_ENV}. Refusing to write, which would overwrite the existing secrets. Restore the original passphrase, or delete the file to start over.` + : `The secrets file at ${filePath} is encrypted but ${SECRET_KEY_ENV} is not set. Refusing to write, which would overwrite the existing secrets. Set the passphrase this file was written with, or delete the file to start over.`, + ); + this.name = "SecretFileKeyMismatchError"; + } +} + +const encodeParts = (iv: Buffer, tag: Buffer, body: Buffer): string => + `${iv.toString("base64")}.${tag.toString("base64")}.${body.toString("base64")}`; + +/** + * Split a `..` payload, rejecting anything that is not + * exactly three parts. `split(".")` on a truncated or extended payload + * otherwise yields buffers of the wrong length, and `createDecipheriv` + * reports that as a generic error further away from the cause. + */ +function decodeParts( + payload: string, +): { iv: Buffer; tag: Buffer; body: Buffer } | null { + const parts = payload.split("."); + if (parts.length !== 3) return null; + const [iv, tag, body] = parts as [string, string, string]; + return { + iv: Buffer.from(iv, "base64"), + tag: Buffer.from(tag, "base64"), + body: Buffer.from(body, "base64"), + }; +} + +export interface FileSecretStoreOptions { + /** Absolute path of the secrets file. */ + filePath: string; + /** + * Passphrase. Defaults to `process.env[SECRET_KEY_ENV]`; an empty or + * whitespace-only value counts as absent, since `MCP_INSPECTOR_SECRET_KEY=` + * in a compose file is a user who meant "off", not a one-character key. + */ + passphrase?: string; +} + +export class FileSecretStore implements SecretStore { + readonly filePath: string; + private readonly passphrase: string | undefined; + /** + * Serializes read-modify-write cycles within this process. `writeStoreFile` + * already makes each individual write atomic and ordered, but that is not + * enough here: two concurrent `set`s would each read the same "before" map + * and the second write would drop the first one's entry. The route handlers + * write a server's fields with `Promise.all`, so this is the ordinary path, + * not a race that needs contriving. + */ + private queue: Promise = Promise.resolve(); + + constructor(options: FileSecretStoreOptions) { + this.filePath = options.filePath; + const raw = options.passphrase ?? process.env[SECRET_KEY_ENV]; + this.passphrase = raw && raw.trim() ? raw : undefined; + } + + /** True when this store encrypts. Drives the `plaintext` descriptor flag. */ + get encrypted(): boolean { + return this.passphrase !== undefined; + } + + private async deriveKey(kdf: KdfParams): Promise { + return scrypt( + // Only reached when the passphrase is set — every caller checks first, + // which is what makes the non-null assertion safe here rather than a + // widened `string | undefined` threaded through the crypto helpers. + this.passphrase as string, + Buffer.from(kdf.salt, "base64"), + KEY_BYTES, + { N: kdf.N, r: kdf.r, p: kdf.p }, + ); + } + + /** + * Read and decode the file into a plain map. + * + * Distinguishes three outcomes that callers treat differently: `null` for + * "no file yet" (a first write should create one), a map for success, and a + * throw for "there is a file and we cannot read it" — which must never be + * flattened into an empty map, or `set` would cheerfully overwrite it. + */ + private async readMap(): Promise | null> { + const raw = await readStoreFile(this.filePath); + if (raw === null) return null; + + let parsed: SecretFile; + try { + parsed = JSON.parse(raw) as SecretFile; + } catch (err) { + throw new SecretStoreUnavailableError( + `The secrets file at ${this.filePath} is not valid JSON: ${err instanceof Error ? err.message : String(err)}`, + ); + } + + if (parsed.encryption === "none") { + // A plaintext file read by a store that now has a passphrase is fine and + // deliberately not an error: the next write re-encrypts it, so setting + // the env var upgrades an existing file in place. + return { ...(parsed.secrets ?? {}) }; + } + + if (parsed.encryption !== CIPHER || !parsed.kdf || !parsed.data) { + throw new SecretStoreUnavailableError( + `The secrets file at ${this.filePath} declares an unsupported format (encryption="${String(parsed.encryption)}", version=${String(parsed.version)}). It may have been written by a newer Inspector.`, + ); + } + if (!this.passphrase) + throw new SecretFileKeyMismatchError(this.filePath, false); + + const parts = decodeParts(parsed.data); + if (!parts) throw new SecretFileKeyMismatchError(this.filePath, true); + try { + const key = await this.deriveKey(parsed.kdf); + const decipher = crypto.createDecipheriv(CIPHER, key, parts.iv); + decipher.setAuthTag(parts.tag); + const plain = Buffer.concat([ + decipher.update(parts.body), + decipher.final(), + ]).toString("utf-8"); + return JSON.parse(plain) as Record; + } catch { + // GCM authentication failure is what a wrong passphrase looks like, and + // it is indistinguishable from a tampered/corrupt file — both mean "this + // key does not open this file", which is what the message says. + throw new SecretFileKeyMismatchError(this.filePath, true); + } + } + + private async writeMap(map: Record): Promise { + let file: SecretFile; + if (this.passphrase) { + const kdf: KdfParams = { + algorithm: "scrypt", + // A fresh salt per write, not per file: rewriting with the same salt + // and a fresh IV would still be safe for GCM, but re-salting costs one + // scrypt derivation we were doing anyway and removes the question. + salt: crypto.randomBytes(16).toString("base64"), + N: SCRYPT_N, + r: SCRYPT_R, + p: SCRYPT_P, + }; + const key = await this.deriveKey(kdf); + const iv = crypto.randomBytes(IV_BYTES); + const cipher = crypto.createCipheriv(CIPHER, key, iv); + const body = Buffer.concat([ + cipher.update(JSON.stringify(map), "utf-8"), + cipher.final(), + ]); + file = { + version: FORMAT_VERSION, + encryption: CIPHER, + kdf, + data: encodeParts(iv, cipher.getAuthTag(), body), + }; + } else { + file = { version: FORMAT_VERSION, encryption: "none", secrets: map }; + } + // `writeStoreFile` creates the parent dir, writes atomically, and sets + // 0600 — the same helper the OAuth token store uses, so the two files on + // disk have identical permissions by construction rather than by + // convention. + await writeStoreFile(this.filePath, `${JSON.stringify(file, null, 2)}\n`); + } + + /** Run `fn` with the file to itself, so concurrent callers can't lose writes. */ + private serialize(fn: () => Promise): Promise { + const run = this.queue.then(fn); + // Swallow on the chain only — the returned promise still rejects, so a + // failed `set` surfaces to its caller while a later `set` is not poisoned + // by it. This assignment is also why `this.queue` needs no `.catch()` of + // its own above: it is only ever `Promise.resolve()` or an + // already-swallowed promise, so it cannot reject. + this.queue = run.then( + () => {}, + () => {}, + ); + return run; + } + + async get(serverId: string, field: string): Promise { + try { + const map = await this.serialize(() => this.readMap()); + return map?.[buildAccount(serverId, field)] ?? null; + } catch { + // Read tolerance, matching `KeyringSecretStore.get`: an unreadable store + // and an absent entry are the same answer to the caller, and hard-failing + // here would break every `GET /api/servers` on a box whose passphrase + // changed. + return null; + } + } + + async set(serverId: string, field: string, value: string): Promise { + await this.serialize(async () => { + let map: Record; + try { + map = (await this.readMap()) ?? {}; + } catch (err) { + // Already a typed refusal (unreadable file / key mismatch) — rethrow + // rather than re-wrapping, so the specific advice survives. + if (err instanceof SecretStoreUnavailableError) throw err; + throw new SecretStoreUnavailableError( + `Could not read the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + map[buildAccount(serverId, field)] = value; + try { + await this.writeMap(map); + } catch (err) { + throw new SecretStoreUnavailableError( + `Could not write the secrets file at ${this.filePath}: ${err instanceof Error ? err.message : String(err)}`, + ); + } + }); + } + + async delete(serverId: string, field: string): Promise { + await this.deleteWhere((key) => key === buildAccount(serverId, field)); + } + + async deleteAllForServer(serverId: string): Promise { + const prefix = `${serverId}:`; + await this.deleteWhere((key) => key.startsWith(prefix)); + } + + /** + * Shared delete body. Silent on every failure, matching the keyring store: + * every reason a delete can fail collapses to "the entry isn't there + * anymore", and `set` is the operation that reports a broken store. + */ + private async deleteWhere(match: (key: string) => boolean): Promise { + try { + await this.serialize(async () => { + const map = await this.readMap(); + if (!map) return; + const keys = Object.keys(map).filter(match); + if (keys.length === 0) return; + for (const key of keys) delete map[key]; + // An emptied map keeps the file rather than unlinking it: its presence + // is what records which encryption mode this install settled on, so a + // later write can't silently switch modes on the user. + await this.writeMap(map); + }); + } catch { + // Intentionally silent — see the doc comment above. + } + } +} + +/** + * Best-effort permission repair for a pre-existing secrets file. + * + * `writeStoreFile` passes `mode: 0o600`, but a mode argument only applies + * when the file is *created* — an atomic rename over a file someone made + * `0644` by hand (or restored from a backup, or copied out of an image) + * keeps the loose mode forever. Called once at store selection so the + * guarantee the docs make is the guarantee on disk. + * + * Failures are swallowed: a file we cannot chmod is one we probably cannot + * write either, and `set` is where that gets reported with context. + */ +export async function tightenSecretFilePermissions( + filePath: string, +): Promise { + try { + await fs.chmod(filePath, 0o600); + } catch { + // Non-existent (the common case — nothing written yet), read-only mount, + // or a platform without POSIX modes. Nothing actionable here. + } +} diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts new file mode 100644 index 00000000..9f07d903 --- /dev/null +++ b/core/auth/node/secret-store-selection.ts @@ -0,0 +1,323 @@ +/** + * Which {@link SecretStore} this process uses, and how the user finds out + * (#1950). + * + * ## The policy + * + * `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. With it + * unset: + * + * 1. Probe the OS keychain. Reachable → use it. This is the overwhelming + * majority of runs and nothing changes for them. + * 2. Unreachable → fall back, **loudly**. Which way we fall back depends + * on whether the file we would write is going to survive: + * - in a container whose secrets directory is *not* on a mount → + * `memory`; + * - otherwise → `file`. + * + * ## Why automatic + * + * Automatic fallback silently changes where a secret lives, which is a + * real objection to make of a security-adjacent tool. But the objection + * is to *silently*, not to *automatic*: requiring an env var before any + * persistence works at all means the container user's first encounter is + * a 503 with no hint that a working configuration exists — and the store + * they would then be told to set is the one this code would have picked + * anyway. So we take the friendly default and pay for it in noise. The + * downgrade prints a banner, rides `GET /api/config`, and is named in the + * footer of both settings modals — permanently, where the secret is + * typed, rather than once at startup where the person typing it 20 + * minutes later will never see it. + * + * ## Why memory can be the better answer + * + * A file in a container's writable layer looks like persistence and isn't: + * `docker run --rm` discards it, and so does any image update. An + * in-memory store makes no such promise, and says so. Mounting a volume — + * which the README already recommends for the catalog — flips the same + * run to `file` with no configuration, because the mount is the user + * stating that this directory is meant to survive. + */ + +import * as fsSync from "node:fs"; +import * as path from "node:path"; +import { + type SecretStorageInfo, + type SecretStoreKind, + secretStorageCaveat, + secretStorageSummary, +} from "../secret-storage-info.js"; +import { + FileSecretStore, + tightenSecretFilePermissions, +} from "./file-secret-store.js"; +import { + InMemorySecretStore, + KeyringSecretStore, + probeKeyringAvailable, + type SecretStore, +} from "./secret-store.js"; + +/** Env var naming an explicit store, bypassing the probe entirely. */ +export const SECRET_STORE_ENV = "MCP_INSPECTOR_SECRET_STORE"; + +/** Env var overriding the secrets file location. */ +export const SECRET_FILE_ENV = "MCP_INSPECTOR_SECRET_FILE"; + +const KINDS: SecretStoreKind[] = ["keyring", "file", "memory"]; + +export interface ResolvedSecretStore { + store: SecretStore; + info: SecretStorageInfo; +} + +/** + * Parse {@link SECRET_STORE_ENV}. An unrecognized value returns undefined + * (falling through to the automatic policy) after a warning rather than + * throwing: a typo in a compose file should not stop the Inspector from + * starting, and the user needs the correct spellings more than a stack. + */ +export function parseSecretStoreEnv( + raw: string | undefined, +): SecretStoreKind | undefined { + const value = raw?.trim().toLowerCase(); + if (!value) return undefined; + if ((KINDS as string[]).includes(value)) return value as SecretStoreKind; + console.warn( + `[mcp-inspector] Ignoring ${SECRET_STORE_ENV}="${raw}": expected one of ${KINDS.join(", ")}.`, + ); + return undefined; +} + +/** Default secrets file: beside the catalog, in `~/.mcp-inspector`. */ +export function defaultSecretFilePath(): string { + const override = process.env[SECRET_FILE_ENV]?.trim(); + if (override) return path.resolve(override); + const homeDir = process.env.HOME || process.env.USERPROFILE || "."; + return path.join(homeDir, ".mcp-inspector", "secrets.json"); +} + +/** + * Are we in a container? + * + * Three signals, any of which is sufficient, because no single one covers + * the runtimes people actually use: `/.dockerenv` is Docker-specific and + * absent under Podman and containerd; the `/proc/1/cgroup` scan catches + * those but reads like the host's under cgroup v2 on some setups; and + * `KUBERNETES_SERVICE_HOST` catches a pod regardless of runtime. + * + * A false positive here is cheap and a false negative is cheaper still — + * this only *biases the fallback* between two working stores, and the + * mount check below is what actually decides. Nothing about correctness + * rests on getting this exactly right, which is why it is allowed to be a + * heuristic at all. + */ +export function isContainer(): boolean { + if (process.env.KUBERNETES_SERVICE_HOST) return true; + try { + if (fsSync.existsSync("/.dockerenv")) return true; + } catch { + // A sandboxed / restricted filesystem. Fall through to the cgroup read. + } + try { + const cgroup = fsSync.readFileSync("/proc/1/cgroup", "utf-8"); + return /\b(docker|containerd|kubepods|podman|lxc)\b/.test(cgroup); + } catch { + // Not Linux, or /proc is not mounted. Not a container as far as we know. + return false; + } +} + +/** + * Is `dir` — or the nearest existing ancestor of it — a mount point? + * + * The test is `st_dev`: a mount is exactly the case where a directory sits + * on a different device from its parent. That is what makes this work + * regardless of *what* was mounted (named volume, bind mount, tmpfs) and + * without parsing `/proc/mounts`, whose format and container-namespace + * visibility vary. + * + * It walks to the nearest existing ancestor because the directory usually + * does not exist yet on a first run — `~/.mcp-inspector` is created on + * demand. A volume mounted *at* that path exists by definition, so the + * common positive case is found immediately; the walk is what keeps the + * common negative case from being answered by a missing-file error. + * + * `/` is its own parent and is trivially a "mount", so the loop stops + * there and reports false: the container's root filesystem is the writable + * layer this whole check exists to distinguish *against*. + */ +export function isOnMountPoint(dir: string): boolean { + try { + let current = path.resolve(dir); + // Walk up to the first path that exists. + while (!fsSync.existsSync(current)) { + const parent = path.dirname(current); + /* v8 ignore next -- @preserve: the walk can only run out of ancestors + if the filesystem root itself does not exist, which would mean the + process has no cwd to have resolved against. Kept as the loop's + termination guard rather than trusting that invariant. */ + if (parent === current) return false; + current = parent; + } + const parent = path.dirname(current); + if (parent === current) return false; // reached `/` + return fsSync.statSync(current).dev !== fsSync.statSync(parent).dev; + } catch { + // Permission denied, a racing unmount, or a platform where `dev` is not + // meaningful. "Not a mount" keeps the fallback on the conservative side + // (memory in a container), which never loses a secret it promised to keep. + return false; + } +} + +/** Build the store named by `kind`, plus the descriptor that explains it. */ +function buildStore( + kind: SecretStoreKind, + reason: SecretStorageInfo["reason"], + detail?: string, +): ResolvedSecretStore { + if (kind === "keyring") { + return { + store: new KeyringSecretStore(), + info: { kind, reason, durable: true }, + }; + } + if (kind === "memory") { + return { + store: new InMemorySecretStore(), + info: { kind, reason, durable: false, detail }, + }; + } + const filePath = defaultSecretFilePath(); + const store = new FileSecretStore({ filePath }); + // Repair a loosened mode on a pre-existing file before anything reads or + // writes it. Fire-and-forget: it is a best-effort tightening (it swallows + // every failure), and blocking selection on a chmod would make an + // unwritable file a startup problem rather than a `set`-time one. + void tightenSecretFilePermissions(filePath); + return { + store, + info: { + kind, + reason, + path: filePath, + plaintext: !store.encrypted, + durable: true, + detail, + }, + }; +} + +/** + * Pick the fallback store for a host with no keychain. Exported for the + * tests, which drive the container/mount predicates directly rather than + * trying to make a real container appear. + */ +export function chooseFallbackKind(opts: { + container: boolean; + mounted: boolean; +}): SecretStoreKind { + return opts.container && !opts.mounted ? "memory" : "file"; +} + +/** + * Print the fallback / plaintext warnings. + * + * `console.warn` rather than the pino file logger deliberately: the person + * who needs this is watching a terminal or `docker logs`, and store + * selection happens before (and independently of) any logger being + * configured. + */ +export function warnAboutSecretStorage(info: SecretStorageInfo): void { + if (info.reason === "fallback") { + console.warn( + `\n[mcp-inspector] The OS keychain is not available, so secrets will be kept in: ${secretStorageSummary(info)}.` + + (info.detail + ? `\n[mcp-inspector] Keychain error: ${info.detail}` + : "") + + `\n[mcp-inspector] Set ${SECRET_STORE_ENV}=keyring|file|memory to choose explicitly.`, + ); + } + const caveat = secretStorageCaveat(info); + if (caveat) console.warn(`[mcp-inspector] ${caveat}`); +} + +let resolved: Promise | undefined; + +/** + * Resolve the active store, once per process. + * + * Cached because the probe is a native round-trip and, more importantly, + * because the answer must not differ between two callers: the banner, the + * `/api/config` payload, and the store actually doing the writing are + * three consumers of one decision, and a re-probe that flipped would have + * the UI describing a store nobody is using. There is no reset seam for + * the same reason a platform does not grow a keychain mid-run; tests get a + * fresh module (and so a fresh cache) via `vi.resetModules()`. + */ +export function resolveSecretStore(): Promise { + resolved ??= (async (): Promise => { + const configured = parseSecretStoreEnv(process.env[SECRET_STORE_ENV]); + if (configured) { + const result = buildStore(configured, "configured"); + warnAboutSecretStorage(result.info); + return result; + } + const probe = await probeKeyringAvailable(); + if (probe.available) return buildStore("keyring", "default"); + + const kind = chooseFallbackKind({ + container: isContainer(), + mounted: isOnMountPoint(path.dirname(defaultSecretFilePath())), + }); + const result = buildStore(kind, "fallback", probe.detail); + warnAboutSecretStorage(result.info); + return result; + })(); + return resolved; +} + +/** The active store's descriptor — for the banner and `GET /api/config`. */ +export async function getSecretStorageInfo(): Promise { + return (await resolveSecretStore()).info; +} + +/** + * A `SecretStore` that resolves the real one on first use. + * + * Selection is async (it probes the keychain) but every existing default + * is a synchronous `new KeyringSecretStore()` in the middle of an options + * object — `core/mcp/node/servers.ts`, `core/client/runner.ts`, + * `core/mcp/remote/node/server.ts`. Making those async would ripple + * through their callers to no benefit, since each method is already async + * and can simply await the selection it needs. So the indirection lives + * here, in one place, instead of in three signatures. + */ +class DeferredSecretStore implements SecretStore { + private async target(): Promise { + return (await resolveSecretStore()).store; + } + async get(serverId: string, field: string): Promise { + return (await this.target()).get(serverId, field); + } + async set(serverId: string, field: string, value: string): Promise { + await (await this.target()).set(serverId, field, value); + } + async delete(serverId: string, field: string): Promise { + await (await this.target()).delete(serverId, field); + } + async deleteAllForServer(serverId: string): Promise { + await (await this.target()).deleteAllForServer(serverId); + } +} + +/** + * The default store for production call sites — what `new + * KeyringSecretStore()` used to be. Every one of them now routes through + * the same selection, so the CLI, the TUI, and the web backend cannot end + * up writing to different places on the same box. + */ +export function defaultSecretStore(): SecretStore { + return new DeferredSecretStore(); +} diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index 2ccefb43..7944a23b 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -152,6 +152,28 @@ export function parseAccount( const buildAccount = (serverId: string, field: string): string => `${serverId}:${field}`; +/** + * Base type for "the active secret store cannot do this right now". + * + * Every store implementation hard-fails on `set` and only on `set` (see + * the {@link SecretStore} contract), and the API routes translate that + * one condition into a 503. Before #1950 there was only one store, so the + * routes could match on `KeychainUnavailableError` directly; now that a + * file-backed store can fail for entirely different reasons (an + * unreadable file, a changed passphrase) they match on this base instead, + * and a future store gets the same treatment by extending it rather than + * by every route learning its name. + * + * The message is the user-facing text — these are surfaced verbatim in + * the 503 body — so subclasses write advice, not diagnostics. + */ +export class SecretStoreUnavailableError extends Error { + constructor(message: string, options?: { cause?: unknown }) { + super(message, options); + this.name = "SecretStoreUnavailableError"; + } +} + /** * Thrown when the OS keychain is unavailable. Surfaced as a 503 by the * API handlers so the UI can show an actionable error rather than a @@ -168,7 +190,7 @@ const buildAccount = (serverId: string, field: string): string => * - **It loads but exposes the wrong API** — a version or packaging * mismatch (`KeyringModuleShapeError`). Also not a daemon problem. */ -export class KeychainUnavailableError extends Error { +export class KeychainUnavailableError extends SecretStoreUnavailableError { constructor(cause: unknown) { const message = cause instanceof Error ? cause.message : String(cause); super( @@ -197,11 +219,86 @@ const hintFor = (cause: unknown, message: string): string => { return `On Linux, install libsecret / gnome-keyring.`; }; +/** + * Probe whether the OS keychain is actually usable, rather than merely + * importable. + * + * This is the input to the automatic-fallback policy in + * `secret-store-selection.ts`, and it deliberately does more than + * {@link loadKeyring}: on the container that motivated #1848 the package + * imports and shape-checks perfectly well, and the failure only appears + * when `AsyncEntry::new` tries to reach a Secret Service that isn't + * there. So the probe constructs an entry and performs a real read. + * + * A **read** and not a write, on purpose. A write would be the stronger + * signal — a keychain can in principle be readable and not writable — but + * it means depositing a value in the user's login keyring at every + * startup, for a store they may never use, that we would then have to + * clean up (and would leave behind if the process died between the two + * calls). Writing into someone's keychain uninvited to answer a question + * about our own configuration is not a trade worth making; a write that + * fails after a passing probe still surfaces as the documented 503. + * + * Never throws — the whole point is to answer a question, and a probe + * that could fail its caller would just move the crash it exists to + * prevent. + */ +export async function probeKeyringAvailable(): Promise< + { available: true } | { available: false; detail: string } +> { + const keyring = await loadKeyring(); + if (!keyring.ok) { + const err = keyring.err; + return { + available: false, + detail: err instanceof Error ? err.message : String(err), + }; + } + try { + const entry = new keyring.mod.AsyncEntry(SERVICE_NAME, PROBE_ACCOUNT); + await entry.getPassword(); + return { available: true }; + } catch (err) { + return { + available: false, + detail: err instanceof Error ? err.message : String(err), + }; + } +} + +/** + * Account used by {@link probeKeyringAvailable}. Namespaced under a server + * id that cannot collide with a real one (`:` is the account separator, so + * a real id never contains one) and never written — it only ever appears + * as the argument to a lookup that is expected to miss. + */ +const PROBE_ACCOUNT = "__inspector:probe"; + /** * Storage interface for the per-server secrets we lift off - * `~/.mcp-inspector/mcp.json`. Implemented by `KeyringSecretStore` (the - * production impl) and `InMemorySecretStore` (used in tests so the suite - * doesn't require libsecret in CI). + * `~/.mcp-inspector/mcp.json`. + * + * Three implementations, selected per run by + * `secret-store-selection.ts` (#1950) and described to the user by the + * `SecretStorageInfo` it returns: + * + * - `KeyringSecretStore` — the OS keychain, and the default wherever one + * is reachable. + * - `FileSecretStore` — `~/.mcp-inspector/secrets.json`, `0600`, + * encrypted when `MCP_INSPECTOR_SECRET_KEY` is set. The fallback on a + * host with no keychain, and on a container with a mounted volume. + * - `InMemorySecretStore` — the session-scoped store. Used by the test + * suite (so CI needs no libsecret), and as the container fallback when + * nothing durable is mounted, where it is the honest choice rather than + * the degraded one: a file in the writable layer promises persistence + * the next `docker run` will not honor. + * + * All three share one availability contract, which is what lets callers + * stay ignorant of which they got: `get` is tolerant (`null` on any + * failure), `delete` no-ops, and `set` is the only operation that + * hard-fails — throwing a {@link SecretStoreUnavailableError} the API + * routes turn into a 503 — because it is the only one where a value would + * be lost. */ export interface SecretStore { get(serverId: string, field: string): Promise; diff --git a/core/auth/secret-storage-info.ts b/core/auth/secret-storage-info.ts new file mode 100644 index 00000000..054cf3b4 --- /dev/null +++ b/core/auth/secret-storage-info.ts @@ -0,0 +1,124 @@ +/** + * The description of *where a typed secret ends up* — the one fact this + * whole subsystem exists to make unmissable (#1950). + * + * Pure data plus its presentation helpers, deliberately kept out of + * `core/auth/node/` so the browser can import it. The Node side resolves + * the active store (`core/auth/node/secret-store-selection.ts`) and + * produces one of these; it then travels three ways, and all three read + * the same object so they cannot disagree: + * + * 1. the startup banner the launcher prints, + * 2. `GET /api/config` → the web UI's settings-modal footers, + * 3. the `SecretStore` doc comment's contract, in prose. + * + * A store's *kind* answers "where", `plaintext` answers "readable by + * anyone who can read the file", and `durable` answers "will it still be + * there next run". Those are three separate questions and a user asking + * one of them is rarely helped by the answer to another, so none is + * folded into the others. + */ + +/** Which backend is holding secrets this session. */ +export type SecretStoreKind = "keyring" | "file" | "memory"; + +/** + * Why the active store is the active store. `configured` means the user + * named it via `MCP_INSPECTOR_SECRET_STORE`; `default` means the keychain + * was reachable and we used it; `fallback` means the keychain was *not* + * reachable and we picked something that works instead — the case that + * earns a loud banner, because it is the one where the answer to "where + * did my secret go" changed without the user asking for it. + */ +export type SecretStoreReason = "configured" | "default" | "fallback"; + +export interface SecretStorageInfo { + kind: SecretStoreKind; + reason: SecretStoreReason; + /** + * Absolute path of the secrets file, for `kind: "file"` only. Shown in + * the UI and the banner: a file-backed store whose location is a + * mystery is barely better than no answer at all. + */ + path?: string; + /** + * True when a file-backed store is writing secrets in the clear (no + * `MCP_INSPECTOR_SECRET_KEY`). Always false for the other kinds — a + * keychain is opaque to us and RAM is not "at rest" in the sense this + * flag is about. + */ + plaintext?: boolean; + /** + * True when secrets outlive the process. False only for `memory`, and + * it is the *promise* being made rather than an implementation detail: + * an in-memory store is honest precisely because it never claims + * durability it cannot deliver in a container's writable layer. + */ + durable: boolean; + /** + * Why the keychain was unreachable, when `reason === "fallback"`. Free + * text from the underlying loader/binding — surfaced rather than + * summarized, because the three realistic causes (missing libsecret, no + * platform binary, a packaging mismatch) need different fixes and only + * the raw message distinguishes them. + */ + detail?: string; +} + +/** Short label naming the store — the footer's primary text. */ +export function secretStorageLabel(info: SecretStorageInfo): string { + switch (info.kind) { + case "keyring": + return "OS keychain"; + case "file": + return info.plaintext ? "File (unencrypted)" : "File (encrypted)"; + case "memory": + return "Memory (this session only)"; + } +} + +/** + * How loudly to say it. `warn` is reserved for the two states a user + * would want to know about before typing a secret — it is on disk in the + * clear, or it is about to be lost on exit — so that the ordinary + * keychain and encrypted-file cases stay quiet and the warning keeps its + * meaning. + */ +export type SecretStorageTone = "neutral" | "warn"; + +export function secretStorageTone(info: SecretStorageInfo): SecretStorageTone { + if (info.kind === "memory") return "warn"; + if (info.plaintext) return "warn"; + return "neutral"; +} + +/** + * One sentence of consequence, or undefined when there is none worth + * saying. Consequence, not mechanism: "will be lost when the Inspector + * exits" is what a user can act on; "uses an in-memory Map" is not. + */ +export function secretStorageCaveat( + info: SecretStorageInfo, +): string | undefined { + if (info.kind === "memory") { + return "Secrets are not written anywhere and are lost when the Inspector exits."; + } + if (info.plaintext) { + return "Secrets are stored unencrypted (file mode 0600). Set MCP_INSPECTOR_SECRET_KEY to encrypt them."; + } + return undefined; +} + +/** + * The full sentence, for the startup banner and any single-line surface. + * Built from the same three helpers the UI uses so the terminal and the + * browser cannot drift into describing the same store differently. + */ +export function secretStorageSummary(info: SecretStorageInfo): string { + const parts = [secretStorageLabel(info)]; + if (info.path) parts.push(`at ${info.path}`); + if (info.reason === "fallback") { + parts.push("— fell back from the OS keychain"); + } + return parts.join(" "); +} diff --git a/core/client/node-persistence.ts b/core/client/node-persistence.ts index fdb7c7af..0d769d38 100644 --- a/core/client/node-persistence.ts +++ b/core/client/node-persistence.ts @@ -3,7 +3,7 @@ */ import { - KeychainUnavailableError, + SecretStoreUnavailableError, type SecretStore, } from "../auth/node/secret-store.js"; import { SECRET_FIELD_IDP_CLIENT_SECRET } from "../auth/secret-fields.js"; @@ -58,7 +58,7 @@ async function migrateClientPlaintextSecret( await writeStoreFile(filePath, serializeStore(stripped)); return stripped; } catch (err) { - if (err instanceof KeychainUnavailableError) { + if (err instanceof SecretStoreUnavailableError) { return config; } throw err; diff --git a/core/client/runner.ts b/core/client/runner.ts index 5192b674..a77db074 100644 --- a/core/client/runner.ts +++ b/core/client/runner.ts @@ -3,10 +3,8 @@ * for Node runners (TUI, CLI). */ -import { - KeyringSecretStore, - type SecretStore, -} from "../auth/node/secret-store.js"; +import type { SecretStore } from "../auth/node/secret-store.js"; +import { defaultSecretStore } from "../auth/node/secret-store-selection.js"; import type { InspectorClientOptions, InspectorServerSettings, @@ -25,8 +23,9 @@ import { export interface LoadRunnerClientConfigOptions { /** Explicit path from `--client-config` (or MCP_CLIENT_CONFIG_PATH when unset). */ clientConfigPath?: string; - /** Secret store for the IdP clientSecret; defaults to the OS keychain. Tests - * inject an in-memory store for determinism. */ + /** Secret store for the IdP clientSecret; defaults to the store selected for + * this host (OS keychain, or its file/memory fallback). Tests inject an + * in-memory store for determinism. */ secretStore?: SecretStore; } @@ -38,7 +37,7 @@ export async function loadRunnerClientConfig( options?.clientConfigPath?.trim() || process.env.MCP_CLIENT_CONFIG_PATH?.trim() || undefined; - const secretStore = options?.secretStore ?? new KeyringSecretStore(); + const secretStore = options?.secretStore ?? defaultSecretStore(); return loadClientConfig({ filePath: customPath, secretStore }); } diff --git a/core/mcp/node/servers.ts b/core/mcp/node/servers.ts index 532f2976..9e10c69a 100644 --- a/core/mcp/node/servers.ts +++ b/core/mcp/node/servers.ts @@ -1,7 +1,5 @@ -import { - KeyringSecretStore, - type SecretStore, -} from "../../auth/node/secret-store.js"; +import type { SecretStore } from "../../auth/node/secret-store.js"; +import { defaultSecretStore } from "../../auth/node/secret-store-selection.js"; import type { InspectorServerSettings, MCPServerConfig } from "../types.js"; import { DEFAULT_MAX_FETCH_REQUESTS, DEFAULT_TASK_TTL_MS } from "../types.js"; import { mcpConfigToServerEntries } from "../serverList.js"; @@ -32,7 +30,8 @@ export type ResolvedServer = { * broadcast into every resolved server's settings. */ export type ServerLoadOptions = ServerConfigOptions & { headers?: Record; - /** Test injection; defaults to {@link KeyringSecretStore} for catalog/config loads. */ + /** Test injection; defaults to the selected store (see + * `secret-store-selection.ts`) for catalog/config loads. */ secretStore?: SecretStore; }; @@ -104,7 +103,7 @@ export async function loadServerEntries( if (source) { let config = readServerListFile(source.path, source.writable); - const secretStore = serverOptions.secretStore ?? new KeyringSecretStore(); + const secretStore = serverOptions.secretStore ?? defaultSecretStore(); config = await rehydrateMcpConfigFromKeychain(config, secretStore); const entries = mcpConfigToServerEntries(config); const result: Record = {}; diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 5bc58f5e..4341954f 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -65,10 +65,10 @@ import { RemoteSession } from "./remote-session.js"; import { createRemoteAuthProvider } from "./tokenAuthProvider.js"; import { API_SERVER_ENV_VARS } from "../constants.js"; import { - KeychainUnavailableError, - KeyringSecretStore, + SecretStoreUnavailableError, type SecretStore, } from "../../../auth/node/secret-store.js"; +import { defaultSecretStore } from "../../../auth/node/secret-store-selection.js"; import { deleteClientConfigStore, readClientConfigStore, @@ -76,6 +76,7 @@ import { } from "../../../client/node-persistence.js"; import { formatClientConfigLoadError } from "../../../client/config-parse.js"; import { envSecretField } from "../../../auth/secret-fields.js"; +import type { SecretStorageInfo } from "../../../auth/secret-storage-info.js"; import { ZodError } from "zod"; /** @@ -189,6 +190,15 @@ export interface InitialConfigPayload { * a legacy backend that predates the field; the UI just shows nothing then. */ version?: string; + /** + * Where secrets typed into this session end up (#1950): the OS keychain, + * a file, or memory. The browser cannot work this out for itself — the + * store lives entirely on the Node side — and it is what the permanent + * footer in the Client/Server Settings modals reports. Absent on a + * legacy backend, in which case the footer renders nothing rather than + * guessing "keychain", since a wrong answer here is worse than none. + */ + secretStorage?: SecretStorageInfo; } export interface RemoteServerOptions { @@ -243,9 +253,11 @@ export interface RemoteServerOptions { /** * Backend for per-server secret values that we keep out of mcp.json * (OAuth client secret, stdio env values). Defaults to a - * `KeyringSecretStore` that talks to the OS keychain via - * `@napi-rs/keyring`. Tests inject `InMemorySecretStore` so the suite - * doesn't need libsecret on Linux CI runners. + * store selected for this host — the OS keychain where one is + * reachable, otherwise the file or in-memory fallback chosen by + * `secret-store-selection.ts` (#1950). Tests inject + * `InMemorySecretStore` so the suite doesn't need libsecret on Linux CI + * runners. */ secretStore?: SecretStore; } @@ -490,8 +502,7 @@ export function createRemoteApp( const { logger: fileLogger, allowedOrigins } = options; const storageDir = options.storageDir ?? getDefaultStorageDir(); const mcpConfigPath = options.mcpConfigPath ?? getDefaultMcpConfigPath(); - const secretStore: SecretStore = - options.secretStore ?? new KeyringSecretStore(); + const secretStore: SecretStore = options.secretStore ?? defaultSecretStore(); // Read-only session support (#1481/#1483). `writable: false` rejects every // /api/servers mutation and suppresses the seed/migrate writes on read. @@ -1830,9 +1841,11 @@ export function createRemoteApp( // Centralizes the "write the secrets in this entry to the keychain" and // "fetch all secrets for this entry from the keychain" steps so the // POST/PUT/DELETE/GET handlers stay readable. Each operation translates a - // `KeychainUnavailableError` from the underlying store into a Hono 503 - // — the only realistic trigger is Linux without libsecret, and the user - // can install it without restarting. + // `SecretStoreUnavailableError` from the underlying store into a Hono + // 503 — a keychain the user can install without restarting (Linux + // without libsecret), or a secrets file we refuse to overwrite because + // the passphrase changed (#1950). Both are fixable outside the process, + // which is what makes 503 the right code rather than 500. // Same Promise.all reasoning as `readKeychainEntriesFor`: each set // is a native round-trip and there's no ordering requirement among @@ -1896,7 +1909,7 @@ export function createRemoteApp( // knows whether to persist the cleanup. // // When the keychain is unavailable (Linux without libsecret), the - // first `secretStore.set` throws `KeychainUnavailableError`. We catch + // first `secretStore.set` throws `SecretStoreUnavailableError`. We catch // it and abandon the migration for this GET — the on-disk file stays // as-is so the user's secret isn't lost, and the next GET retries // (e.g. after the user installs libsecret). @@ -1926,7 +1939,7 @@ export function createRemoteApp( changed = true; } } catch (err) { - if (err instanceof KeychainUnavailableError) { + if (err instanceof SecretStoreUnavailableError) { if (fileLogger) { fileLogger.warn( { err: err.message }, @@ -2016,7 +2029,7 @@ export function createRemoteApp( c: Context, err: unknown, ): Response | undefined => { - if (err instanceof KeychainUnavailableError) { + if (err instanceof SecretStoreUnavailableError) { return c.json({ error: err.message }, 503); } return undefined; @@ -2182,7 +2195,7 @@ export function createRemoteApp( // goes to disk, the values go to the keychain. const { stripped, secrets } = extractSecretsFromStored(built); // Order: sweep → keychain → disk. The keychain write is the - // only step that can hard-fail (KeychainUnavailableError on + // only step that can hard-fail (SecretStoreUnavailableError on // `set`); doing it before the disk write means a 503 leaves no // disk entry behind, so a retry POST isn't trapped at 409. The // initial sweep handles the case where a previous DELETE failed @@ -2439,7 +2452,7 @@ export function createRemoteApp( // disk file, then clean up obsolete keychain entries. // // - Keychain set is the only hard-fail step (it raises 503 on - // `KeychainUnavailableError`). Doing it first means a failed + // `SecretStoreUnavailableError`). Doing it first means a failed // set leaves both disk and keychain in their pre-PUT state; // the user retries and nothing is half-applied. // - The disk write happens after the keychain is fully primed, diff --git a/core/react/useInitialConfig.ts b/core/react/useInitialConfig.ts index 884b2370..a17ecbc8 100644 --- a/core/react/useInitialConfig.ts +++ b/core/react/useInitialConfig.ts @@ -18,6 +18,11 @@ * `package.json` (the browser can't read it off disk). `undefined` until the * fetch resolves, and whenever the backend omits it (legacy backend) — the UI * renders nothing then. + * - `secretStorage` — which store this session's secrets go to (#1950), + * resolved on the Node side where the keychain probe happens. `undefined` + * until the fetch resolves and on a backend that omits it; the settings-modal + * footers render nothing then, because a guessed answer under a secret field + * is worse than no answer. * * This consolidates the three former single-field hooks (`useSandboxUrl`, * `useServerListWritable`, `useInspectorVersion`), each of which fetched the @@ -29,6 +34,7 @@ */ import { useCallback, useEffect, useState } from "react"; +import type { SecretStorageInfo } from "../auth/secret-storage-info.js"; export interface UseInitialConfigOptions { /** Base URL of the remote server (typically `window.location.origin`). */ @@ -46,6 +52,8 @@ export interface UseInitialConfigResult { sandboxUrl: string | undefined; /** Whether the server list is writable (catalog) or read-only (session). */ writable: boolean; + /** Where this session's secrets are stored, or undefined when unknown. */ + secretStorage: SecretStorageInfo | undefined; /** True while the initial fetch is in flight. */ loading: boolean; } @@ -55,6 +63,24 @@ interface ConfigPayload { version?: unknown; sandboxUrl?: unknown; writable?: unknown; + secretStorage?: unknown; +} + +/** + * Narrow the payload's `secretStorage` before trusting it. + * + * The footer states, in the UI, where a user's secret is about to go, so a + * malformed or partial descriptor must render as "unknown" rather than as a + * confident half-answer — the `kind` alone is what every label and tone + * derives from, so it is the field the check turns on. + */ +function usableSecretStorage(value: unknown): SecretStorageInfo | undefined { + if (typeof value !== "object" || value === null) return undefined; + const kind = (value as { kind?: unknown }).kind; + if (kind !== "keyring" && kind !== "file" && kind !== "memory") { + return undefined; + } + return value as SecretStorageInfo; } /** Coerce a payload field to a usable non-empty string, else undefined. */ @@ -74,6 +100,9 @@ export function useInitialConfig( // Default writable so the common (catalog) case shows CRUD immediately and a // legacy backend that omits the field keeps working. const [writable, setWritable] = useState(true); + const [secretStorage, setSecretStorage] = useState< + SecretStorageInfo | undefined + >(undefined); const [loading, setLoading] = useState(true); // `isCancelled` lets the effect drop a response that resolves after unmount or @@ -100,6 +129,7 @@ export function useInitialConfig( // Only an explicit `false` makes the list read-only; a missing field // (legacy backend) stays writable. setWritable(body.writable !== false); + setSecretStorage(usableSecretStorage(body.secretStorage)); } catch { // Network error / aborted fetch: leave every field at its default // (version/sandboxUrl undefined, writable true). @@ -118,5 +148,5 @@ export function useInitialConfig( }; }, [load]); - return { version, sandboxUrl, writable, loading }; + return { version, sandboxUrl, writable, secretStorage, loading }; }