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>

Author: cliffhall

clients/web/src/test/core/react/useServers.test.tsx

@@ -13,6 +13,7 @@ import { join } from "node:path";
 import { useServers } from "@inspector/core/react/useServers";
 import { createRemoteApp } from "@inspector/core/mcp/remote/node/server";
 import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList";
+import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store";
 import type { MCPConfig } from "@inspector/core/mcp/types";
 
 interface Harness {
@@ -29,6 +30,10 @@ function setupHarness(): Harness {
     dangerouslyOmitAuth: true,
     mcpConfigPath: configPath,
     initialConfig: { defaultEnvironment: {} },
+    // Inject an in-memory secret store so the test never touches the
+    // developer's real OS keychain (and so the suite passes on Linux CI
+    // runners without libsecret).
+    secretStore: new InMemorySecretStore(),
   });
   // Wrap so the typing matches Web fetch — Hono's app.fetch expects a Request
   // (not a URL string), so build one from string/URL inputs before dispatch.

clients/web/src/test/integration/auth/node/secret-store.test.ts

@@ -1,18 +1,78 @@
 /**
  * Contract tests for the secret-store abstraction.
  *
- * The keyring-backed default implementation needs platform native bindings
- * (libsecret on Linux) that aren't reliably present in CI, so the suite
- * exercises the `InMemorySecretStore` here — the same interface the
- * production code is tested against via the `/api/servers` integration
- * suite (which injects this in-memory impl). Coverage of the keyring
- * adapter itself is exercised by hand on macOS/Windows during development.
+ * `InMemorySecretStore` is exercised directly. `KeyringSecretStore` is
+ * exercised via a `vi.mock` of `@napi-rs/keyring` — the native bindings
+ * aren't reliably present in CI (Linux runners ship without libsecret),
+ * so the suite stubs the native side and asserts the tolerance contract
+ * (`get` returns null on failure, destructive ops no-op, `set` is the
+ * one operation that hard-fails with `KeychainUnavailableError`).
  */
-import { describe, it, expect, beforeEach } from "vitest";
+import { describe, it, expect, beforeEach, vi } from "vitest";
+
+// The mock must be hoisted above the `await import` of secret-store
+// inside the `KeyringSecretStore` describe block. Use `vi.hoisted` so
+// references are captured before the import is evaluated.
+const keyringMocks = vi.hoisted(() => {
+  const password = new Map<string, string | null>();
+  const reset = () => password.clear();
+  // Behavior hooks each test can flip to simulate keychain unavailability
+  // on specific operations. Defaults: real-ish in-memory behavior.
+  const failures = {
+    getThrows: false,
+    setThrows: false,
+    deleteThrows: false,
+    findThrows: false,
+    deleteThrowsNoEntry: false,
+  };
+  const credentials = (): Array<{ account: string; password: string }> => {
+    const out: Array<{ account: string; password: string }> = [];
+    for (const [k, v] of password.entries()) {
+      if (v !== null) out.push({ account: k, password: v });
+    }
+    return out;
+  };
+  class AsyncEntry {
+    private readonly key: string;
+    constructor(_service: string, username: string) {
+      this.key = username;
+    }
+    async getPassword(): Promise<string | undefined> {
+      if (failures.getThrows) throw new Error("keychain get unavailable");
+      const v = password.get(this.key);
+      return v === undefined || v === null ? undefined : v;
+    }
+    async setPassword(value: string): Promise<void> {
+      if (failures.setThrows) throw new Error("keychain set unavailable");
+      password.set(this.key, value);
+    }
+    async deleteCredential(): Promise<boolean> {
+      if (failures.deleteThrowsNoEntry) throw new Error("No entry found");
+      if (failures.deleteThrows) throw new Error("keychain delete unavailable");
+      return password.delete(this.key);
+    }
+  }
+  const findCredentialsAsync = async (): Promise<
+    Array<{ account: string; password: string }>
+  > => {
+    if (failures.findThrows) throw new Error("keychain find unavailable");
+    return credentials();
+  };
+  return { AsyncEntry, findCredentialsAsync, failures, password, reset };
+});
+
+vi.mock("@napi-rs/keyring", () => ({
+  AsyncEntry: keyringMocks.AsyncEntry,
+  findCredentialsAsync: keyringMocks.findCredentialsAsync,
+}));
+
 import {
   InMemorySecretStore,
+  KeyringSecretStore,
+  KeychainUnavailableError,
   SECRET_FIELD_OAUTH_CLIENT_SECRET,
   envSecretField,
+  parseAccount,
   type SecretStore,
 } from "@inspector/core/auth/node/secret-store.js";
 
@@ -119,3 +179,147 @@ describe("InMemorySecretStore", () => {
     expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe("");
   });
 });
+
+describe("parseAccount", () => {
+  it("splits `${serverId}:${field}` on the first colon", () => {
+    expect(parseAccount("srv:oauth-client-secret")).toEqual({
+      serverId: "srv",
+      field: "oauth-client-secret",
+    });
+  });
+
+  it("allows the field to contain colons (env:KEY uses one)", () => {
+    expect(parseAccount("srv:env:API_KEY")).toEqual({
+      serverId: "srv",
+      field: "env:API_KEY",
+    });
+  });
+
+  it("returns null when no separator is present", () => {
+    expect(parseAccount("noseparator")).toBe(null);
+  });
+
+  it("returns null for a leading or trailing colon (empty side)", () => {
+    expect(parseAccount(":field")).toBe(null);
+    expect(parseAccount("srv:")).toBe(null);
+  });
+});
+
+describe("KeyringSecretStore (mocked native bindings)", () => {
+  let store: KeyringSecretStore;
+
+  beforeEach(() => {
+    keyringMocks.reset();
+    keyringMocks.failures.getThrows = false;
+    keyringMocks.failures.setThrows = false;
+    keyringMocks.failures.deleteThrows = false;
+    keyringMocks.failures.findThrows = false;
+    keyringMocks.failures.deleteThrowsNoEntry = false;
+    store = new KeyringSecretStore();
+  });
+
+  it("round-trips a 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("get returns null when getPassword throws (keychain unavailable)", async () => {
+    // get is tolerant: there's no value to surface so degrading to "null"
+    // matches the absence semantic the caller already handles.
+    keyringMocks.failures.getThrows = true;
+    expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      null,
+    );
+  });
+
+  it("get returns null when the underlying entry is absent (no value set)", async () => {
+    expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      null,
+    );
+  });
+
+  it("set throws KeychainUnavailableError when setPassword throws", async () => {
+    // set is the one operation that hard-fails — losing data silently
+    // is worse than surfacing a clear error the user can act on.
+    keyringMocks.failures.setThrows = true;
+    await expect(
+      store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v"),
+    ).rejects.toBeInstanceOf(KeychainUnavailableError);
+  });
+
+  it("delete silently treats a 'no entry' error as success", async () => {
+    keyringMocks.failures.deleteThrowsNoEntry = true;
+    await expect(
+      store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET),
+    ).resolves.toBeUndefined();
+  });
+
+  it("delete silently no-ops when the keychain is unavailable", async () => {
+    keyringMocks.failures.deleteThrows = true;
+    await expect(
+      store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET),
+    ).resolves.toBeUndefined();
+  });
+
+  it("delete actually removes the value when the keychain is available", async () => {
+    await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v");
+    await store.delete("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET);
+    expect(await store.get("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      null,
+    );
+  });
+
+  it("deleteAllForServer no-ops when findCredentialsAsync throws", async () => {
+    // We don't even know what was written, so there's nothing to sweep.
+    // Critically, this must not throw — the route's defensive sweep on
+    // POST and DELETE depends on it.
+    keyringMocks.failures.findThrows = true;
+    await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
+  });
+
+  it("deleteAllForServer removes every entry under the given id", async () => {
+    await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "a");
+    await store.set("alpha", envSecretField("K"), "b");
+    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("K"))).toBe(null);
+    expect(await store.get("beta", SECRET_FIELD_OAUTH_CLIENT_SECRET)).toBe(
+      "untouched",
+    );
+  });
+
+  it("deleteAllForServer ignores entries on a different id that share a prefix", async () => {
+    // The `parseAccount` check guards against a literal startsWith match
+    // wrongly sweeping `alpha-prime:...` 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("KeychainUnavailableError carries the underlying error message", async () => {
+    keyringMocks.failures.setThrows = true;
+    try {
+      await store.set("alpha", SECRET_FIELD_OAUTH_CLIENT_SECRET, "v");
+      throw new Error("expected throw");
+    } catch (err) {
+      expect(err).toBeInstanceOf(KeychainUnavailableError);
+      expect((err as Error).message).toMatch(/keychain set unavailable/);
+      expect((err as Error).message).toMatch(/libsecret/);
+    }
+  });
+});

clients/web/src/test/integration/mcp/remote/servers-events.test.ts

@@ -27,6 +27,7 @@ import { join } from "node:path";
 import { serve } from "@hono/node-server";
 import type { ServerType } from "@hono/node-server";
 import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js";
+import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store.js";
 
 interface Harness {
   baseUrl: string;
@@ -43,6 +44,10 @@ async function setup(): Promise<Harness> {
     dangerouslyOmitAuth: true,
     mcpConfigPath: configPath,
     initialConfig: { defaultEnvironment: {} },
+    // Inject an in-memory secret store so the suite passes on Linux CI
+    // runners without libsecret (and so a developer running locally
+    // doesn't pollute their real OS keychain with test entries).
+    secretStore: new InMemorySecretStore(),
   });
   const { baseUrl, server } = await new Promise<{
     baseUrl: string;