inspector/core/auth/node/secret-store.ts
Cliff Hall 037225a9fb
feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356) (#1360)
* feat(servers): move oauthClientSecret and stdio env values to OS keychain (#1356)

mcp.json is designed to be tool-shareable (symlinked from Claude Desktop's
config, pasted into bug reports, synced via dotfiles), so storing OAuth
client secrets and stdio env values in plaintext there meant any of those
flows could leak them. Lift both into the OS keychain via @napi-rs/keyring
(active replacement for the archived keytar). The wire shape is unchanged:
the GET /api/servers handler rehydrates from the keychain so browser code
sees the same JSON; the on-disk file no longer contains the secret
material. Includes an idempotent migration that lifts plaintext from older
mcp.json files (or hand-edited ones) into the keychain on first read.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): tolerate keychain unavailability on non-set ops (#1356)

CI on Linux failed because `@napi-rs/keyring` hard-errors without
libsecret, and the route handlers were touching the keychain even for
no-secret flows (defensive `deleteAllForServer` on POST/DELETE, sweep
on PUT). The user's intent for the Linux fallback was "hard fail when a
secret is actually involved" — not on every keychain touch.

Make `KeyringSecretStore.get / delete / deleteAllForServer` silently
degrade when the keychain is unavailable (return null / no-op); `set`
remains the one operation that hard-fails with
`KeychainUnavailableError`. The migration path catches the error and
preserves the disk plaintext rather than losing the secret.

Also inject `InMemorySecretStore` in `useServers.test.tsx` and
`servers-events.test.ts` so tests never touch a developer's real
keychain even when libsecret is present.

Adds `KeyringSecretStore` tests (vi.mock'd native bindings) and an
integration suite that simulates Linux-without-libsecret to verify the
tolerance contract end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(servers): close GET migration race; reverse keychain/disk order (#1356)

Addresses code review:

1. **Concurrency**: GET /api/servers was reading + migrating outside the
   write lock, then writing inside it — a concurrent POST/PUT/DELETE
   between the read and the lock acquisition could be clobbered by the
   migration write. Also affected the first-launch seed write. Fix:
   take a fast unlocked peek; if the file is absent (seed branch) or
   carries plaintext to migrate (migration branch), re-read inside the
   write lock and decide there. Added `hasPlaintextSecrets()` as a
   pure predicate so the common no-migration path stays lock-free.

2. **Write ordering**: POST and PUT wrote disk first, then keychain.
   A keychain `set` failure mid-write left a half-configured entry on
   disk and trapped retries at 409. Reversed to keychain-first so a
   503 leaves both stores in their pre-write state. PUT in-place now
   sets new fields, writes disk, then deletes obsolete fields (a
   failed disk write leaves orphan keychain entries — recoverable —
   rather than premature deletions of fields the user still expects
   to see).

3. **Tolerance simplification**: dropped the no-entry regex in
   `KeyringSecretStore.delete` — both "no entry" and "keychain
   unavailable" collapse to the same desired outcome (the entry isn't
   there anymore), so a uniform swallow is clearer.

4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the
   per-field `get` calls. macOS Keychain round-trips are 10-50ms;
   serializing 5+ env vars per server × N servers is a meaningful
   wall-clock cost on GET.

5. **Spec doc**: reflects the tolerance contract (only `set`
   hard-fails) and the keychain-before-disk write ordering rationale.

New regression test in servers-route.test.ts covers the retry-after-503
contract directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(servers): parallel keychain writes + migration partial-state comment (#1356)

Two nice-to-have observations from the re-review:

- `writeKeychainEntriesFor` now `Promise.all`s its per-field sets,
  matching the read side. No ordering requirement among distinct
  (id, field) keys; on unavailability every set rejects and Promise.all
  surfaces the first rejection (still 503 from the route handler).
- `migratePlaintextSecrets` catch block now spells out the
  partial-migration semantics: we return the original config so the
  disk plaintext stays intact, and the next successful GET retries
  with the keychain-wins branch absorbing already-set entries.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(servers): parallel keychain deletes for symmetry (#1356)

Picks up the symmetry observation from the third review: `deleteKeychainFields`
now uses `Promise.all` like its read and write siblings. Distinct (id, field)
deletes have no ordering requirement, and `secretStore.delete` is already a
silent no-op on unavailability so Promise.all has no failure-mode surprise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-26 10:33:46 -04:00

179 lines
6.9 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.
*/
import {
AsyncEntry,
findCredentialsAsync,
} from "@napi-rs/keyring";
const SERVICE_NAME = "mcp-inspector";
export {
SECRET_FIELD_OAUTH_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}`;
/**
* Thrown when the OS keychain is unavailable — typically Linux without
* libsecret / gnome-keyring installed. Surfaced as a 503 by the API
* handlers so the UI can show an actionable error rather than a generic
* 500. macOS and Windows always have a working keychain, so this only
* realistically fires on minimal Linux installs.
*/
export class KeychainUnavailableError extends Error {
constructor(cause: unknown) {
super(
`OS keychain is not available. On Linux, install libsecret / gnome-keyring. ` +
`Underlying error: ${cause instanceof Error ? cause.message : String(cause)}`,
);
this.name = "KeychainUnavailableError";
}
}
/**
* 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).
*/
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 (the
* typical case is Linux without libsecret / gnome-keyring), `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; the user
* only hits a hard error when they actually try to save a secret.
*/
export class KeyringSecretStore implements SecretStore {
async get(serverId: string, field: string): Promise<string | null> {
const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
try {
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> {
const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
try {
await entry.setPassword(value);
} catch (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> {
const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
try {
await entry.deleteCredential();
} catch {
// Both reasons for a throw collapse to the same desired outcome
// ("the entry isn't there anymore"): `deleteCredential` raises
// NoEntry for a missing credential, and the native binding
// raises a runtime error when the keychain itself is unavailable.
// We treat both 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 {
creds = await 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);
}
}
}