modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: load @napi-rs/keyring lazily so an unsupported platform degrades instead of crashing (#1905) (#1948)

Browse source 
* fix: load @napi-rs/keyring lazily so an unsupported platform degrades

`@napi-rs/keyring` ships one prebuilt binary per platform triple and
throws on import where it has none. On Android / Termux there is no
`@napi-rs/keyring-android-arm64`, so the static top-level import in
`core/auth/node/secret-store.ts` threw during module evaluation and the
Inspector exited at startup with "Cannot find native binding" — before
any of the keychain-unavailable handling in that same file could run.

Replaces the static import with a cached dynamic one. The outcome is
cached, not just the module, so a box without a binary doesn't
re-attempt (and re-throw) resolution on every secret operation —
`expectedSecretFields` means that would otherwise be once per server per
`GET /api/servers` — and so `set` can name the underlying cause in its
`KeychainUnavailableError`.

An unloadable package now folds into the same degradation contract as an
unreachable keychain: `get` returns null, `delete` and
`deleteAllForServer` no-op, and `set` throws `KeychainUnavailableError`
(not double-wrapped — the load failure is already typed by the time the
catch sees it). The class doc comment now enumerates all three ways
"unavailable" can arrive, since each escaped the contract at some point.

The shared `vi.mock` stub can't express this — it models a keyring that
loads — so the new tests build on `vi.resetModules()` + `vi.doMock` with
a throwing factory and re-import the module, covering the load failure,
each method under it, and the load-once caching.

Verified the dynamic import survives bundling in all three clients
(cli, tui, and the web runner) rather than being inlined back to a
static one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XAR2Nr9kXbrywFNUVoTe9F

* fix: steer a missing native binding to a reinstall hint

`KeychainUnavailableError` gave the Linux libsecret / gnome-keyring
advice for every cause, including the one where the keychain is fine and
the @napi-rs/keyring platform package is what's missing — an unsupported
triple (#1905) or npm's optional-deps bug dropping it on a supported one
(npm/cli#4828, the Windows report on #1852). Detect the loader's "Cannot
find native binding" phrasing and point those users at a reinstall /
npx-cache clear instead.

Ported from the superseded #1943, which fixed this alongside the same
lazy-load change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3

* fix: name a bad keyring namespace instead of "is not a constructor"

The lazy import accepted whatever the resolution produced. The named
exports arrive through CJS interop, so a resolution that stopped
yielding them — a default-only export upstream, a bundler changing
interop, a platform where named-export detection fails — would hand us
`undefined`, and `new mod.AsyncEntry(...)` would throw a TypeError
inside the very try that implements graceful degradation.

Shape-check the resolved module and treat a bad shape as unavailable.

Scope, because it is narrower than it first appears: this does NOT
prevent 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 a dead keychain does. What changes is the
diagnosis. Without the check the only signal is `set` reporting
"keyring.mod.AsyncEntry is not a constructor", which reads like an
Inspector bug; with it, `set` names the real problem once, at the load
boundary, in the same actionable 503 as every other unavailability.
Catching the silent-empty-list case itself would need a round-trip
against the unmocked package, which nothing in the suite does today.

The member access sits inside the try because reading a missing export
is not always a harmless undefined — a Proxy-backed namespace can throw,
as vitest's module mocks do — and letting that escape would reject the
cached promise, which is otherwise guaranteed to always resolve.

Of the four new tests only the cause-message one fails without the
guard; the rest pin the surrounding contract.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3

* docs: correct the test header's count of unavailability entry points

The shape check added a fourth way "unavailable" can arrive, and the
enumeration in secret-store.ts was updated to match, but the test file's
header still said three — so it read as an exhaustive list that silently
omitted the case a whole describe below it covers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3

* fix: give the wrong-shape cause its own remediation hint

The shape check added a cause that fell through to the libsecret
advice — so a packaging mismatch told the user to install a keyring
daemon, which cannot help, and on Windows or macOS points somewhere that
does not exist. That is the same wrong-advice bug the native-binding
branch was added to fix, reintroduced by the guard itself.

Hint selection moves into `hintFor`, where libsecret is the explicit
fallback rather than the default, and the shape failure becomes a
distinct `KeyringModuleShapeError` matched by type — it is our own error,
so there is no reason to re-parse text we wrote (the native-binding
branch matches a string only because that message comes from napi-rs).

Also retitles the wrong-shape test: it claimed the guard avoids
"returning null secrets", but `get` returns null either way by the
read-tolerance contract. The title now says what actually differs —
`set` hard-fails — matching the correction already made in the comments.

Both raised by Copilot review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3

* fix: give both shape-check failure paths the packaging hint

The shape check can fail two ways — the members are absent, or reading
them throws — and only the first carried KeyringModuleShapeError. The
catch path returned the raw error, so a Proxy-backed namespace throwing
on access fell through to the libsecret advice: the same wrong-remedy
bug as the previous two commits, one branch further along.

Both paths now carry the type. KeyringModuleShapeError takes a detail
string plus a standard `cause`, so the throwing path reads "its exports
could not be read: <original>" and keeps the underlying failure legible
while still earning the packaging hint.

Three instances of one mistake in a row (a new cause added without
deciding which remediation it inherits) says the shape was wrong, not
just the branch: making libsecret an explicit fallback in `hintFor` did
not stop me adding a second exit that skipped it. The tests now pin the
invariant directly — one asserts both construction paths produce the
same hint — so the next cause that forgets is a failing test rather than
a reviewer's catch.

Raised by Copilot review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall 2026-08-08 21:15:20 -04:00 • committed by GitHub
commit a09f2dcf1d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
1 changed files with 525 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -7,8 +7,16 @@
* 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`).
*
* The contract has four entry points for "unavailable" and the suite
* covers all four: the operation throwing, `AsyncEntry`'s constructor
* throwing (#1848), the package failing to load at all (#1905), and the
* package loading but exposing the wrong shape. The last two can't use
* the shared stub — one needs the *import* to reject, the other needs it
* to resolve to a namespace the stub can't express — so each lives in
* its own describe built on `vi.resetModules()`.
*/
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
@ -81,6 +89,7 @@ import {
InMemorySecretStore,
KeyringSecretStore,
KeychainUnavailableError,
KeyringModuleShapeError,
SECRET_FIELD_OAUTH_CLIENT_SECRET,
envSecretField,
parseAccount,
@ -330,6 +339,59 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
expect(err.message).toMatch(/libsecret/);
});
// The two hint branches are asserted by direct construction rather than
// through the store: the unloadable-package path can only be reached via
// a throwing `vi.doMock` factory, and vitest substitutes its own "error
// when mocking a module" message for whatever that factory throws — so
// the real loader text never reaches the constructor from there. These
// two cases pin the wording against the message the napi-rs loader
// actually produces.
it("KeychainUnavailableError steers a missing native binding to a reinstall", () => {
const err = new KeychainUnavailableError(
new Error(
"Cannot find native binding. npm has a bug related to optional dependencies…",
),
);
expect(err.message).toMatch(/reinstall the Inspector/);
expect(err.message).toMatch(/npx cache/);
// The Linux keyring-daemon advice is irrelevant to this cause.
expect(err.message).not.toMatch(/libsecret/);
expect(err.message).toMatch(/Cannot find native binding/);
});
it("KeychainUnavailableError points a wrong-shape module at a reinstall, not at libsecret", () => {
// A packaging/version mismatch is not a missing keyring daemon, so
// the libsecret line would send the user somewhere that cannot help.
const err = new KeychainUnavailableError(
new KeyringModuleShapeError("did not expose AsyncEntry"),
);
expect(err.message).toMatch(/does not expose the API this build expects/);
expect(err.message).not.toMatch(/libsecret/);
});
it("KeychainUnavailableError gives the same hint however the shape check failed", () => {
// The shape check fails two ways — members absent, or reading them
// throws. Both are the same underlying problem, so both must earn the
// packaging hint; only one of them carrying it was the original bug.
const err = new KeychainUnavailableError(
new KeyringModuleShapeError("its exports could not be read: boom", {
cause: new Error("boom"),
}),
);
expect(err.message).toMatch(/does not expose the API this build expects/);
expect(err.message).not.toMatch(/libsecret/);
// The original failure is still legible in the message.
expect(err.message).toMatch(/boom/);
});
it("KeychainUnavailableError keeps the libsecret advice for other causes", () => {
const err = new KeychainUnavailableError(
new Error("failed to unlock the default collection"),
);
expect(err.message).toMatch(/libsecret/);
expect(err.message).not.toMatch(/reinstall the Inspector/);
});
it("KeychainUnavailableError carries the underlying error message", async () => {
keyringMocks.failures.setThrows = true;
try {
@ -386,3 +448,254 @@ describe("KeyringSecretStore (mocked native bindings)", () => {
});
});
});
describe("@napi-rs/keyring unloadable on this platform (#1905)", () => {
// `@napi-rs/keyring` ships one prebuilt binary per platform triple and
// throws on import where it has none — Android / Termux is the reported
// case. A static top-level import made that a startup crash: the module
// never evaluated, so none of the tolerance handling could run.
//
// The shared stub above can't express this: it models a keyring that
// loads. Here the *import itself* rejects, which needs a fresh module
// registry — hence `vi.resetModules()` + `vi.doMock` and a re-import
// rather than a flag. The re-imported module is a distinct instance, so
// its `KeychainUnavailableError` is a distinct class identity too; the
// assertions below use the freshly imported one.
// The real-world message on Termux. Documentary here — see the cause
// assertion below for why vitest doesn't let it through verbatim.
const LOAD_ERROR = "Cannot find module '@napi-rs/keyring-android-arm64'";
/** Fresh secret-store module whose `@napi-rs/keyring` import rejects. */
const importWithUnloadableKeyring = async (
onLoadAttempt: () => void = () => {},
) => {
vi.resetModules();
vi.doMock("@napi-rs/keyring", () => {
onLoadAttempt();
throw new Error(LOAD_ERROR);
});
return await import("@inspector/core/auth/node/secret-store.js");
};
afterEach(() => {
vi.doUnmock("@napi-rs/keyring");
vi.resetModules();
});
it("the module still evaluates — importing it must not throw", async () => {
// The regression itself: with a static top-level import this rejects,
// and the Inspector exits before reaching any fallback.
await expect(importWithUnloadableKeyring()).resolves.toHaveProperty(
"KeyringSecretStore",
);
});
it("get returns null", async () => {
const mod = await importWithUnloadableKeyring();
const store = new mod.KeyringSecretStore();
expect(await store.get("alpha", "oauth-client-secret")).toBe(null);
});
it("set throws KeychainUnavailableError carrying the load failure as its cause", async () => {
const mod = await importWithUnloadableKeyring();
const store = new mod.KeyringSecretStore();
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toBeInstanceOf(mod.KeychainUnavailableError);
// Asserted by shape, not by the literal text: vitest substitutes its
// own "error when mocking a module" message for a throwing `doMock`
// factory, so `LOAD_ERROR` never reaches the store here. What matters
// — and what is testable — is that the load failure is appended as
// the cause rather than swallowed. In production the real
// "Cannot find native binding" text lands in that slot.
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toThrow(/Underlying error: .+/);
});
it("set does not double-wrap the typed error", async () => {
// The load failure is already a `KeychainUnavailableError` by the time
// the catch sees it; re-wrapping would bury the cause a level deeper.
const mod = await importWithUnloadableKeyring();
const store = new mod.KeyringSecretStore();
try {
await store.set("alpha", "oauth-client-secret", "v");
throw new Error("expected throw");
} catch (err) {
expect(err).toBeInstanceOf(mod.KeychainUnavailableError);
// One "OS keychain is not available" prefix, not two nested.
expect(
(err as Error).message.match(/OS keychain is not available/g),
).toHaveLength(1);
}
});
it("delete and deleteAllForServer silently no-op", async () => {
const mod = await importWithUnloadableKeyring();
const store = new mod.KeyringSecretStore();
await expect(
store.delete("alpha", "oauth-client-secret"),
).resolves.toBeUndefined();
await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
});
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
// server per `GET /api/servers`.
const onLoadAttempt = vi.fn();
const mod = await importWithUnloadableKeyring(onLoadAttempt);
const store = new mod.KeyringSecretStore();
await store.get("alpha", "oauth-client-secret");
await store.get("beta", "oauth-client-secret");
await store.delete("alpha", "oauth-client-secret");
expect(onLoadAttempt).toHaveBeenCalledTimes(1);
});
});
describe("@napi-rs/keyring loads but exposes the wrong shape", () => {
// The package is CJS, so `AsyncEntry` / `findCredentialsAsync` reach us
// through named-export interop. That holds today — verified against the
// real package — but if it ever stopped (a default-only export upstream,
// a bundler changing interop, a platform where named-export detection
// fails), the members would be `undefined` and `new undefined(...)` would
// throw a TypeError *inside* the try that implements degradation.
//
// What the shape check fixes is the *diagnosis*, not the data loss: `get`
// returns null with or without it (read-tolerance is its contract), so
// the empty secret list looks the same either way. The difference is that
// `set` names the shape problem at the load boundary instead of reporting
// "keyring.mod.AsyncEntry is not a constructor". Only the cause-message
// test below actually fails without the guard — the others pin the
// surrounding contract and would pass either way.
const importWithKeyringShape = async (shape: Record<string, unknown>) => {
vi.resetModules();
vi.doMock("@napi-rs/keyring", () => shape);
return await import("@inspector/core/auth/node/secret-store.js");
};
afterEach(() => {
vi.doUnmock("@napi-rs/keyring");
vi.resetModules();
});
// The members are declared but not callable, rather than absent. That is
// the shape a default-only export would leave behind, and it keeps these
// cases on the `typeof !== "function"` branch deterministically: vitest's
// module mock *throws* on reading a key the factory never returned, which
// is a different path (covered by its own case at the end).
const ENTRY_NOT_A_FUNCTION = {
AsyncEntry: undefined,
findCredentialsAsync: async () => [],
};
it("hard-fails set for a namespace without a callable AsyncEntry (get still returns null by contract)", async () => {
const mod = await importWithKeyringShape(ENTRY_NOT_A_FUNCTION);
const store = new mod.KeyringSecretStore();
// `get` returning null is the read-tolerance contract, not something
// the shape check changes — it reads the same as "no secret stored",
// which is exactly why `set` has to be the operation that hard-fails.
expect(await store.get("alpha", "oauth-client-secret")).toBe(null);
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toBeInstanceOf(mod.KeychainUnavailableError);
});
it("names the shape problem as the cause, not a bare unavailability", async () => {
const mod = await importWithKeyringShape(ENTRY_NOT_A_FUNCTION);
const store = new mod.KeyringSecretStore();
try {
await store.set("alpha", "oauth-client-secret", "v");
throw new Error("expected throw");
} catch (err) {
expect((err as Error).message).toMatch(
/did not expose AsyncEntry \/ findCredentialsAsync/,
);
// Not double-wrapped: the load failure is already typed by the time
// `set`'s catch sees it.
expect(
(err as Error).message.match(/OS keychain is not available/g),
).toHaveLength(1);
}
});
it("treats a namespace without a callable findCredentialsAsync as unavailable too", async () => {
// `deleteAllForServer` is the only caller of it, so a shape check on
// `AsyncEntry` alone would let this one through to a TypeError.
class StubEntry {
async getPassword(): Promise<string | undefined> {
return undefined;
}
}
const mod = await importWithKeyringShape({
AsyncEntry: StubEntry,
findCredentialsAsync: undefined,
});
const store = new mod.KeyringSecretStore();
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toBeInstanceOf(mod.KeychainUnavailableError);
await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
});
it("treats a namespace that throws on member access as unavailable", async () => {
// Reading a missing export is not always a harmless `undefined` — a
// Proxy-backed namespace can throw, and vitest's module mock does. If
// that throw escaped the shape check it would reject the *cached*
// promise, so the check has to absorb it and report unavailable.
const mod = await importWithKeyringShape({});
const store = new mod.KeyringSecretStore();
expect(await store.get("alpha", "oauth-client-secret")).toBe(null);
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toBeInstanceOf(mod.KeychainUnavailableError);
// …and it reaches the user as a packaging problem, not as "install
// libsecret". Returning the raw access error here would have been
// typed correctly but hinted wrongly.
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.toThrow(/does not expose the API this build expects/);
await expect(
store.set("alpha", "oauth-client-secret", "v"),
).rejects.not.toThrow(/libsecret/);
// Absorbed, not escaped: a rejected cached promise would surface here
// as the raw access error instead of the typed one.
await expect(store.deleteAllForServer("alpha")).resolves.toBeUndefined();
});
it("accepts a well-formed namespace", async () => {
// The guard must not reject the shape the package actually ships —
// otherwise it would turn a working keychain into a permanent 503.
const stored = new Map<string, string>();
class StubEntry {
// Declared explicitly rather than as a constructor parameter
// property: those are disallowed under `erasableSyntaxOnly`.
private readonly account: string;
constructor(_service: string, account: string) {
this.account = account;
}
async getPassword(): Promise<string | undefined> {
return stored.get(this.account);
}
async setPassword(value: string): Promise<void> {
stored.set(this.account, value);
}
async deleteCredential(): Promise<boolean> {
return stored.delete(this.account);
}
}
const mod = await importWithKeyringShape({
AsyncEntry: StubEntry,
findCredentialsAsync: async () => [],
});
const store = new mod.KeyringSecretStore();
await store.set("alpha", "oauth-client-secret", "shh");
expect(await store.get("alpha", "oauth-client-secret")).toBe("shh");
});
});