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 }; }