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 f118081c3..dd57415f7 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 @@ -8,7 +8,7 @@ * (`get` returns null on failure, destructive ops no-op, `set` is the * one operation that hard-fails with `KeychainUnavailableError`). */ -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; // The mock must be hoisted above the `await import` of secret-store // inside the `KeyringSecretStore` describe block. Use `vi.hoisted` so @@ -24,6 +24,7 @@ const keyringMocks = vi.hoisted(() => { deleteThrows: false, findThrows: false, deleteThrowsNoEntry: false, + constructorThrows: false, }; const credentials = (): Array<{ account: string; password: string }> => { const out: Array<{ account: string; password: string }> = []; @@ -35,6 +36,12 @@ const keyringMocks = vi.hoisted(() => { class AsyncEntry { private readonly key: string; constructor(_service: string, username: string) { + // `AsyncEntry::new` performs the platform-store setup and throws + // when no secret service is reachable (#1848) — the stub must be + // able to fail here, not just in the methods. + if (failures.constructorThrows) { + throw new Error("Couldn't access platform storage: PermissionDenied"); + } this.key = username; } async getPassword(): Promise { @@ -215,6 +222,7 @@ describe("KeyringSecretStore (mocked native bindings)", () => { keyringMocks.failures.deleteThrows = false; keyringMocks.failures.findThrows = false; keyringMocks.failures.deleteThrowsNoEntry = false; + keyringMocks.failures.constructorThrows = false; store = new KeyringSecretStore(); }); @@ -311,6 +319,27 @@ describe("KeyringSecretStore (mocked native bindings)", () => { ).toBe("p"); }); + it("get returns null when the AsyncEntry constructor throws (#1848)", async () => { + keyringMocks.failures.constructorThrows = true; + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + }); + + it("set throws KeychainUnavailableError when the AsyncEntry constructor throws (#1848)", async () => { + keyringMocks.failures.constructorThrows = true; + await expect( + store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"), + ).rejects.toBeInstanceOf(KeychainUnavailableError); + }); + + it("delete silently no-ops when the AsyncEntry constructor throws (#1848)", async () => { + keyringMocks.failures.constructorThrows = true; + await expect( + store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).resolves.toBeUndefined(); + }); + it("KeychainUnavailableError stringifies a non-Error cause", () => { const err = new KeychainUnavailableError("plain string cause"); expect(err).toBeInstanceOf(KeychainUnavailableError); @@ -318,6 +347,19 @@ describe("KeyringSecretStore (mocked native bindings)", () => { expect(err.message).toMatch(/libsecret/); }); + it("KeychainUnavailableError steers a missing-native-binding cause to a reinstall hint (#1852)", () => { + const err = new KeychainUnavailableError( + new Error( + "Cannot find native binding. npm has a bug related to optional dependencies " + + "(https://github.com/npm/cli/issues/4828).", + ), + ); + expect(err.message).toMatch(/reinstall the Inspector/); + expect(err.message).toMatch(/Cannot find native binding/); + // The Linux keyring-daemon advice is irrelevant to a broken install. + expect(err.message).not.toMatch(/libsecret/); + }); + it("KeychainUnavailableError carries the underlying error message", async () => { keyringMocks.failures.setThrows = true; try { @@ -330,3 +372,63 @@ describe("KeyringSecretStore (mocked native bindings)", () => { } }); }); + +describe("KeyringSecretStore (module load failure — #1852)", () => { + // `loadKeyring` caches its result per module instance, so each test + // takes a fresh copy of secret-store whose dynamic import of + // `@napi-rs/keyring` rejects the way the napi-rs loader does when the + // platform binary package is missing (npm's optional-deps bug, + // npm/cli#4828). The statically-imported copy at the top of this file + // is untouched — these tests only use the fresh module's exports. + const importWithLoadFailure = async () => { + vi.resetModules(); + vi.doMock("@napi-rs/keyring", () => { + throw new Error( + "Cannot find native binding. npm has a bug related to optional dependencies " + + "(https://github.com/npm/cli/issues/4828). Please try `npm i` again after removing " + + "both package-lock.json and node_modules directory.", + ); + }); + return import("@inspector/core/auth/node/secret-store.js"); + }; + + afterEach(() => { + vi.doUnmock("@napi-rs/keyring"); + vi.resetModules(); + }); + + it("get returns null instead of propagating the load error", async () => { + const mod = await importWithLoadFailure(); + const store = new mod.KeyringSecretStore(); + expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe( + null, + ); + // The failed load is cached — a second call degrades the same way. + expect(await store.get("alpha", envSecretField("KEY"))).toBe(null); + }); + + it("delete and deleteAllForServer silently no-op", async () => { + const mod = await importWithLoadFailure(); + const store = new mod.KeyringSecretStore(); + await expect( + store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET), + ).resolves.toBeUndefined(); + await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined(); + }); + + it("set throws KeychainUnavailableError built from the cached load error", async () => { + // (vitest substitutes its own error text when a mock factory throws, + // so the loader's exact message can't be asserted here — the + // reinstall-hint wording is covered by the direct-construction test + // above. What matters here is the *typed* rejection: it's what the + // routes' 503 translation and the migratePlaintextSecrets skip + // branch dispatch on.) + const mod = await importWithLoadFailure(); + const store = new mod.KeyringSecretStore(); + await expect( + store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"), + // Instance check against the fresh module's class — the statically + // imported KeychainUnavailableError is a different module instance. + ).rejects.toBeInstanceOf(mod.KeychainUnavailableError); + }); +}); diff --git a/core/auth/node/secret-store.ts b/core/auth/node/secret-store.ts index dbc07d791..81c368ea1 100644 --- a/core/auth/node/secret-store.ts +++ b/core/auth/node/secret-store.ts @@ -13,10 +13,32 @@ * 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"; +type KeyringModule = typeof import("@napi-rs/keyring"); + +/** + * `@napi-rs/keyring` is loaded lazily because its top-level `require` + * throws when the platform binary package (an npm optionalDependency, + * e.g. `@napi-rs/keyring-win32-x64-msvc`) is missing — npm's + * optional-deps bug (npm/cli#4828) drops those on install, and the npx + * cache hits it on in-place upgrades. A static import here would take + * down the whole launcher at boot (#1852); loading on first use instead + * lets the load failure degrade through the same availability contract + * as an unreachable keychain (see `KeyringSecretStore`). + */ +let keyringLoad: Promise | undefined; +let keyringLoadError: unknown; + +function loadKeyring(): Promise { + keyringLoad ??= import("@napi-rs/keyring").catch((err: unknown) => { + keyringLoadError = err; + return null; + }); + return keyringLoad; +} + export { SECRET_FIELD_OAUTH_CLIENT_SECRET, SECRET_FIELD_IDP_CLIENT_SECRET, @@ -39,18 +61,23 @@ 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. + * Thrown when the OS keychain is unavailable. Two realistic causes: + * Linux without libsecret / gnome-keyring installed, and a missing + * `@napi-rs/keyring` platform binary package (npm's optional-deps bug, + * npm/cli#4828 — the module itself fails to load, any OS). Surfaced as + * a 503 by the API handlers so the UI can show an actionable error + * rather than a generic 500. */ 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)}`, - ); + const message = cause instanceof Error ? cause.message : String(cause); + // The napi-rs loader's throw for a missing platform package starts + // with this phrase — steer those users to a reinstall instead of + // the (irrelevant) Linux keyring-daemon advice. + const hint = message.includes("Cannot find native binding") + ? `The @napi-rs/keyring platform package for this OS is missing — reinstall the Inspector (for npx, clear the npx cache under your npm cache directory first).` + : `On Linux, install libsecret / gnome-keyring.`; + super(`OS keychain is not available. ${hint} Underlying error: ${message}`); this.name = "KeychainUnavailableError"; } } @@ -79,19 +106,29 @@ export interface SecretStore { * 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. + * **Availability behavior.** When the keychain is unavailable — the + * module failed to load (missing platform binary, #1852), the entry + * constructor threw (no reachable secret service, #1848), or the + * operation itself failed (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 / + * broken npx-cache install; 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 { - const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + const keyring = await loadKeyring(); + if (!keyring) return null; try { + // Constructed inside the try: `AsyncEntry::new` performs the + // platform-store setup and throws when no backend is reachable. + const entry = new keyring.AsyncEntry( + SERVICE_NAME, + buildAccount(serverId, field), + ); const v = await entry.getPassword(); return v ?? null; } catch { @@ -104,8 +141,13 @@ export class KeyringSecretStore implements SecretStore { } async set(serverId: string, field: string, value: string): Promise { - const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + const keyring = await loadKeyring(); + if (!keyring) throw new KeychainUnavailableError(keyringLoadError); try { + const entry = new keyring.AsyncEntry( + SERVICE_NAME, + buildAccount(serverId, field), + ); await entry.setPassword(value); } catch (err) { // The only operation that hard-fails — if we can't persist the @@ -116,8 +158,13 @@ export class KeyringSecretStore implements SecretStore { } async delete(serverId: string, field: string): Promise { - const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field)); + const keyring = await loadKeyring(); + if (!keyring) return; try { + const entry = new keyring.AsyncEntry( + SERVICE_NAME, + buildAccount(serverId, field), + ); await entry.deleteCredential(); } catch { // Both reasons for a throw collapse to the same desired outcome @@ -131,9 +178,11 @@ export class KeyringSecretStore implements SecretStore { } async deleteAllForServer(serverId: string): Promise { + const keyring = await loadKeyring(); + if (!keyring) return; let creds: Array<{ account: string; password: string }>; try { - creds = await findCredentialsAsync(SERVICE_NAME); + creds = await keyring.findCredentialsAsync(SERVICE_NAME); } catch { // Same reasoning as `delete`: nothing was written, nothing to sweep. return;