Closes #1950 Before this, a box with no reachable OS keychain — the published container (no D-Bus session, #1848), Android/Termux (no platform binary, #1905), a minimal Linux install without libsecret — could not persist an OAuth client secret or a stdio `env:` value at all. `KeyringSecretStore` degraded into a store that silently holds nothing: `get` returned null, `delete` no-opped, and `set` hard-failed with a 503. That is the correct failure behavior, but it left the documented, supported way to run the Inspector with no secret persistence whatsoever. Adds the two missing implementations and, more importantly, the policy that picks one and the surfaces that say which was picked. - `FileSecretStore` — one `0600` JSON document at `~/.mcp-inspector/secrets.json`, encrypted with AES-256-GCM whenever `MCP_INSPECTOR_SECRET_KEY` is set (scrypt, per-file random salt). The whole map is encrypted as a unit rather than value-by-value so the account names — `${serverId}:${field}` — are hidden too; a per-value scheme would leave a readable index of which servers you hold a client secret for. A file that can no longer be decrypted reads as empty (matching the interface's read tolerance) but *refuses to be written*, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - `secret-store-selection.ts` — the policy. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright; otherwise probe the keychain and, if unreachable, fall back loudly. The fallback is `memory` in a container whose secrets directory is not on a mount, and `file` otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `--rm` will discard. Cached per process so the banner, `/api/config`, and the store doing the writing cannot disagree. - Automatic, but never silent. The downgrade prints at startup, rides `GET /api/config`, and is named in a permanent 32px footer at the bottom of both dialogs that accept a secret. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is the thing a user dismisses before doing the work it describes. The plaintext and memory cases render their caveat inline in a warning tone — a warning you must hover to discover is not one. - `SecretStoreUnavailableError` is now the base every store's `set` throws, so the routes translate one condition into a 503 instead of matching `KeychainUnavailableError` by name. Docs: the container section of the root README gains a table of which store you get and how to make secrets survive, plus the `MCP_INSPECTOR_SECRET_KEY` upgrade/mismatch rules. Tests: the existing interface contract block is extracted to `secretStoreContract.ts` and run against all three implementations, so none can drift on the availability contract. `FileSecretStore` is tested against a real temp filesystem — mode, cross-instance round-trip, that the ciphertext does not contain the plaintext, the read-modify-write race, and every refusal path. Full `npm run ci` passes, per-file gate included. Signed-off-by: cliffhall <cliff@futurescale.com>
463 lines
20 KiB
TypeScript
463 lines
20 KiB
TypeScript
/**
|
|
* Per-server secret storage backed by the OS keychain.
|
|
*
|
|
* Service name `mcp-inspector`; account `${serverId}:${field}`. Fields used
|
|
* by the current code: `oauth-client-secret` and `env:${KEY}` (one per
|
|
* stdio env variable). Keeping the account namespaced by `serverId` lets
|
|
* us drop every entry for a server in one sweep when DELETE
|
|
* /api/servers/:id runs, and lets `findCredentials(SERVICE)` enumerate
|
|
* everything we own for migration / debugging.
|
|
*
|
|
* Node-only — `@napi-rs/keyring` uses native bindings (Keychain Services
|
|
* on macOS, Credential Manager on Windows, libsecret on Linux). The
|
|
* browser side never imports this; it gets values rehydrated into the
|
|
* `/api/servers` response by the Hono handler.
|
|
*/
|
|
|
|
const SERVICE_NAME = "mcp-inspector";
|
|
|
|
/**
|
|
* `@napi-rs/keyring` ships one prebuilt binary per platform triple, and
|
|
* loading the package *throws* on a platform it has no binary for —
|
|
* Android / Termux is the reported case (#1905), where the import fails
|
|
* with "Cannot find native binding" / "Cannot find module
|
|
* '@napi-rs/keyring-android-arm64'".
|
|
*
|
|
* A static top-level import made that a startup crash: the module never
|
|
* evaluated, so the Inspector exited before any of the
|
|
* keychain-unavailable handling below could run. Loading it lazily, and
|
|
* caching the *outcome* rather than only the module, folds an
|
|
* unsupported platform into the same degradation contract as an
|
|
* unreachable keychain (see `KeyringSecretStore`) — the store is simply
|
|
* unavailable, and callers see it through the documented behavior
|
|
* instead of a crash.
|
|
*
|
|
* The failure is cached alongside the success so a box without a binary
|
|
* doesn't re-attempt (and re-throw) the resolution on every secret
|
|
* operation, and so `set` can name the underlying cause in its
|
|
* `KeychainUnavailableError`.
|
|
*
|
|
* The cache is process-lifetime by design — there is no reset seam, since
|
|
* a platform does not grow a native binary mid-run. Tests reach the
|
|
* unloadable path through `vi.resetModules()` + `vi.doMock`, which gives
|
|
* them a fresh module (and so a fresh cache) instead.
|
|
*
|
|
* A resolved import is also **shape-checked** before being accepted. The
|
|
* package is CJS, so the named exports we rely on come from interop, and
|
|
* a resolution that stopped yielding them (a default-only export in a
|
|
* future version, a bundler changing interop, a platform where the
|
|
* named-export detection fails) would land as `undefined`, making
|
|
* `new mod.AsyncEntry(...)` throw `TypeError: not a constructor` from
|
|
* inside the very `try` that implements graceful degradation.
|
|
*
|
|
* Be precise about what this buys, because it is narrower than it looks:
|
|
* it does **not** stop a bad shape from emptying the secret list. `get`
|
|
* returns `null` either way — that is its read-tolerance contract, and
|
|
* the `TypeError` lands in the same `catch` that a dead keychain does.
|
|
* What the check changes is *diagnosis*. Without it the only signal is
|
|
* `set` reporting "keyring.mod.AsyncEntry is not a constructor", an
|
|
* internal-looking message that reads like an Inspector bug; with it,
|
|
* `set` names the actual problem once, at the load boundary, in the same
|
|
* actionable 503 as every other flavor of unavailability. Detecting the
|
|
* silent-empty-list case itself would take a real round-trip against the
|
|
* unmocked package, which nothing in the suite does today.
|
|
*/
|
|
type KeyringModule = typeof import("@napi-rs/keyring");
|
|
type KeyringLoad =
|
|
| { ok: true; mod: KeyringModule }
|
|
| { ok: false; err: unknown };
|
|
|
|
let keyringLoad: Promise<KeyringLoad> | undefined;
|
|
|
|
/**
|
|
* Accept a resolved import only if it carries the two members we call.
|
|
*
|
|
* The member *access* is inside the `try` because reading a missing
|
|
* export is not always the harmless `undefined` a plain ESM namespace
|
|
* gives: a Proxy-based namespace can throw on an unknown key (vitest's
|
|
* module mocks do exactly that). Either way the answer is the same —
|
|
* unavailable — and returning it rather than throwing is what keeps the
|
|
* cached promise from ever rejecting.
|
|
*/
|
|
/**
|
|
* Marks the shape-check failure so `KeychainUnavailableError` can give
|
|
* advice that fits it. A distinct type rather than a string match on the
|
|
* message: this is our own error, so there is no reason to re-parse text
|
|
* we just wrote (the native-binding branch matches on a string only
|
|
* because that message comes from someone else's loader).
|
|
*/
|
|
export class KeyringModuleShapeError extends Error {
|
|
constructor(detail: string, options?: { cause?: unknown }) {
|
|
super(`@napi-rs/keyring loaded but ${detail}`, options);
|
|
this.name = "KeyringModuleShapeError";
|
|
}
|
|
}
|
|
|
|
const checkKeyringShape = (mod: KeyringModule): KeyringLoad => {
|
|
try {
|
|
if (
|
|
typeof mod.AsyncEntry === "function" &&
|
|
typeof mod.findCredentialsAsync === "function"
|
|
) {
|
|
return { ok: true, mod };
|
|
}
|
|
return {
|
|
ok: false,
|
|
err: new KeyringModuleShapeError(
|
|
"did not expose AsyncEntry / findCredentialsAsync",
|
|
),
|
|
};
|
|
} catch (err) {
|
|
// Both ways of failing the shape check are the same problem — the
|
|
// module is not the API we expect — so both carry the type that
|
|
// earns the packaging hint. Returning the raw error here instead
|
|
// would drop it back to the libsecret advice, which is what this
|
|
// whole branch exists to avoid.
|
|
return {
|
|
ok: false,
|
|
err: new KeyringModuleShapeError(
|
|
`its exports could not be read: ${err instanceof Error ? err.message : String(err)}`,
|
|
{ cause: err },
|
|
),
|
|
};
|
|
}
|
|
};
|
|
|
|
const loadKeyring = (): Promise<KeyringLoad> => {
|
|
keyringLoad ??= import("@napi-rs/keyring").then(
|
|
checkKeyringShape,
|
|
(err: unknown): KeyringLoad => ({ ok: false, err }),
|
|
);
|
|
return keyringLoad;
|
|
};
|
|
|
|
export {
|
|
SECRET_FIELD_OAUTH_CLIENT_SECRET,
|
|
SECRET_FIELD_IDP_CLIENT_SECRET,
|
|
envSecretField,
|
|
} from "../secret-fields.js";
|
|
|
|
/** Parse a stored account key back into its server id and field. */
|
|
export function parseAccount(
|
|
account: string,
|
|
): { serverId: string; field: string } | null {
|
|
const idx = account.indexOf(":");
|
|
if (idx <= 0 || idx === account.length - 1) return null;
|
|
return {
|
|
serverId: account.slice(0, idx),
|
|
field: account.slice(idx + 1),
|
|
};
|
|
}
|
|
|
|
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
|
|
* generic 500 — and "actionable" is the point: the causes need
|
|
* *different* fixes, so the message carries a hint chosen per cause
|
|
* (see `hintFor`). Three realistic ones:
|
|
*
|
|
* - **The keychain itself is missing** — Linux without libsecret /
|
|
* gnome-keyring. Install it.
|
|
* - **`@napi-rs/keyring` won't load** — no platform binary for this
|
|
* triple (Android/Termux, #1905) or npm's optional-deps bug dropping
|
|
* it on a supported one (npm/cli#4828). Reinstall / clear the npx
|
|
* cache; installing a keyring daemon would not help.
|
|
* - **It loads but exposes the wrong API** — a version or packaging
|
|
* mismatch (`KeyringModuleShapeError`). Also not a daemon problem.
|
|
*/
|
|
export class KeychainUnavailableError extends SecretStoreUnavailableError {
|
|
constructor(cause: unknown) {
|
|
const message = cause instanceof Error ? cause.message : String(cause);
|
|
super(
|
|
`OS keychain is not available. ${hintFor(cause, message)} Underlying error: ${message}`,
|
|
);
|
|
this.name = "KeychainUnavailableError";
|
|
}
|
|
}
|
|
|
|
/**
|
|
* The remediation that fits the cause. Wrong advice is worse than none —
|
|
* telling someone on Windows to install libsecret sends them down a path
|
|
* that cannot work — so every cause that has its own fix gets its own
|
|
* branch, and the libsecret line is the fallback rather than the default.
|
|
*/
|
|
const hintFor = (cause: unknown, message: string): string => {
|
|
// Our own error, so match on the type rather than re-parsing text we wrote.
|
|
if (cause instanceof KeyringModuleShapeError) {
|
|
return `The @napi-rs/keyring package loaded but does not expose the API this build expects — most likely a version or packaging mismatch; reinstall the Inspector, and report this if it persists.`;
|
|
}
|
|
// This phrasing comes from the napi-rs loader, not from us: it is what
|
|
// the package throws when the platform binary is missing.
|
|
if (message.includes("Cannot find native binding")) {
|
|
return `The @napi-rs/keyring platform package for this OS is missing or unavailable — reinstall the Inspector (for npx, clear the npx cache under your npm cache directory first).`;
|
|
}
|
|
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`.
|
|
*
|
|
* 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<string | null>;
|
|
set(serverId: string, field: string, value: string): Promise<void>;
|
|
/** No-op if no entry exists. */
|
|
delete(serverId: string, field: string): Promise<void>;
|
|
/** Remove every secret stored for this server id (called on DELETE /api/servers/:id). */
|
|
deleteAllForServer(serverId: string): Promise<void>;
|
|
}
|
|
|
|
/**
|
|
* Default implementation. Each operation constructs a fresh `AsyncEntry`;
|
|
* the native side is cheap and the alternative (caching entries by
|
|
* (serverId, field)) just trades native-handle bookkeeping for an
|
|
* allocation that's measured in microseconds. `getPassword` returns
|
|
* `undefined` for a missing entry — we normalize to `null` so callers
|
|
* can use `=== null` rather than truthiness (an empty-string secret is
|
|
* a real value and must round-trip).
|
|
*
|
|
* **Availability behavior.** When the keychain is unavailable, `set` is
|
|
* the only operation that throws `KeychainUnavailableError` — that's
|
|
* the moment where data would actually be lost. `get` returns `null`
|
|
* (as if no entry existed) and the destructive operations silently
|
|
* no-op (there's nothing to delete anyway). This keeps non-secret flows
|
|
* working on a stock CI runner / minimal Linux box / unsupported
|
|
* platform; the user only hits a hard error when they actually try to
|
|
* save a secret.
|
|
*
|
|
* "Unavailable" covers four distinct failures, all funneled into that
|
|
* one contract — the contract is only as good as its narrowest funnel,
|
|
* and each of these escaped it at some point:
|
|
*
|
|
* 1. **The package won't load at all** — no prebuilt binary for this
|
|
* platform (Android / Termux). A static top-level import made this a
|
|
* startup crash before any handling ran (#1905); `loadKeyring()`
|
|
* above defers and caches it instead.
|
|
* 2. **The package loads but exposes the wrong shape** — the named
|
|
* exports arrive via CJS interop, so a resolution that stopped
|
|
* yielding them would hand us `undefined` and fail as a `TypeError`
|
|
* swallowed by the degradation path. `loadKeyring()` shape-checks up
|
|
* front so `set` can name that cause instead of surfacing an
|
|
* "is not a constructor" message (see the note there — the check
|
|
* improves the diagnosis, it does not change what `get` returns).
|
|
* 3. **`AsyncEntry::new` throws** — it performs the platform-store setup
|
|
* (on Linux, the Secret Service connect with a keyutils fallback) and
|
|
* throws when no backend is reachable. Construction is therefore
|
|
* deliberately **inside** each method's `try`; outside it, the raw
|
|
* error escaped and 500'd every `GET /api/servers` before any secret
|
|
* was involved (#1848).
|
|
* 4. **The operation itself throws** — the original case, and the only
|
|
* one the first version of this contract actually handled.
|
|
*/
|
|
export class KeyringSecretStore implements SecretStore {
|
|
async get(serverId: string, field: string): Promise<string | null> {
|
|
try {
|
|
const keyring = await loadKeyring();
|
|
if (!keyring.ok) return null;
|
|
const entry = new keyring.mod.AsyncEntry(
|
|
SERVICE_NAME,
|
|
buildAccount(serverId, field),
|
|
);
|
|
const v = await entry.getPassword();
|
|
return v ?? null;
|
|
} catch {
|
|
// Tolerate keychain unavailability on reads: there's no value to
|
|
// surface either way. Hard-failing here would break GET flows
|
|
// that don't touch any secret material (most of the test suite,
|
|
// and most user sessions on a Linux box without libsecret).
|
|
return null;
|
|
}
|
|
}
|
|
|
|
async set(serverId: string, field: string, value: string): Promise<void> {
|
|
try {
|
|
const keyring = await loadKeyring();
|
|
// An unloadable package is as fatal to a write as an unreachable
|
|
// keychain, and for the same reason — the value would vanish.
|
|
if (!keyring.ok) throw new KeychainUnavailableError(keyring.err);
|
|
const entry = new keyring.mod.AsyncEntry(
|
|
SERVICE_NAME,
|
|
buildAccount(serverId, field),
|
|
);
|
|
await entry.setPassword(value);
|
|
} catch (err) {
|
|
// Already the typed error when the module failed to load — don't
|
|
// double-wrap it (that would bury the underlying cause one level
|
|
// deeper in the message).
|
|
if (err instanceof KeychainUnavailableError) throw err;
|
|
// The only operation that hard-fails — if we can't persist the
|
|
// secret, the user needs to know now rather than discover later
|
|
// that their value disappeared. Routes translate this to a 503.
|
|
throw new KeychainUnavailableError(err);
|
|
}
|
|
}
|
|
|
|
async delete(serverId: string, field: string): Promise<void> {
|
|
try {
|
|
const keyring = await loadKeyring();
|
|
if (!keyring.ok) return;
|
|
const entry = new keyring.mod.AsyncEntry(
|
|
SERVICE_NAME,
|
|
buildAccount(serverId, field),
|
|
);
|
|
await entry.deleteCredential();
|
|
} catch {
|
|
// Every reason for a throw collapses to the same desired outcome
|
|
// ("the entry isn't there anymore"): `deleteCredential` raises
|
|
// NoEntry for a missing credential, and both the constructor and
|
|
// the native binding raise a runtime error when the keychain
|
|
// itself is unavailable. We treat all of them as success — there's
|
|
// no value to lose either way, and `set` is the operation that
|
|
// hard-fails when the keychain is actually down.
|
|
}
|
|
}
|
|
|
|
async deleteAllForServer(serverId: string): Promise<void> {
|
|
let creds: Array<{ account: string; password: string }>;
|
|
try {
|
|
const keyring = await loadKeyring();
|
|
if (!keyring.ok) return;
|
|
creds = await keyring.mod.findCredentialsAsync(SERVICE_NAME);
|
|
} catch {
|
|
// Same reasoning as `delete`: nothing was written, nothing to sweep.
|
|
return;
|
|
}
|
|
const prefix = `${serverId}:`;
|
|
for (const c of creds) {
|
|
if (!c.account.startsWith(prefix)) continue;
|
|
const parsed = parseAccount(c.account);
|
|
if (!parsed || parsed.serverId !== serverId) continue;
|
|
await this.delete(serverId, parsed.field);
|
|
}
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Test double — substituted via the `secretStore` option on the remote
|
|
* server factory. Mirrors the keyring contract exactly so swapping it
|
|
* in/out doesn't change behavior beyond persistence.
|
|
*/
|
|
export class InMemorySecretStore implements SecretStore {
|
|
private readonly map = new Map<string, string>();
|
|
|
|
async get(serverId: string, field: string): Promise<string | null> {
|
|
return this.map.get(buildAccount(serverId, field)) ?? null;
|
|
}
|
|
|
|
async set(serverId: string, field: string, value: string): Promise<void> {
|
|
this.map.set(buildAccount(serverId, field), value);
|
|
}
|
|
|
|
async delete(serverId: string, field: string): Promise<void> {
|
|
this.map.delete(buildAccount(serverId, field));
|
|
}
|
|
|
|
async deleteAllForServer(serverId: string): Promise<void> {
|
|
const prefix = `${serverId}:`;
|
|
for (const key of [...this.map.keys()]) {
|
|
if (key.startsWith(prefix)) this.map.delete(key);
|
|
}
|
|
}
|
|
}
|