fix(auth): keep oauth-persist browser-safe (move Node file backend out)
The refactor routed the browser's RemoteOAuthStorage through `oauth-persist.ts`, which value-imported the Node-only `store-io.js` (`atomically` -> `stubborn-fs` -> `node:process.getuid`, plus `node:fs/promises`). That dragged Node built-ins into the browser bundle and the web app crashed to a blank page at runtime — a regression from v2/main, where browser OAuth storage used a Zustand JSON adapter with no Node imports. Unit/integration tests run in node/happy-dom and `smoke:web` only checks the served HTML, so none of them execute the app in a real browser and the break slipped through. Fix, following the existing `store-id.ts` precedent: - Split the pure JSON (de)serializers into Node-free `core/storage/store-serialize.ts`; `store-io.ts` re-exports them so existing importers are unaffected. - Move the Node-only `createFileOAuthPersistBackend` into `core/auth/node/oauth-persist-file.ts`; update `storage-node.ts` and the storage adapters test to import from there. - `oauth-persist.ts` now imports only the Node-free serializers, so the browser bundle no longer pulls in `node:fs`/`atomically`. Tests: - oauth-persist-browser-safe.test.ts: source-level guard that `oauth-persist.ts` never imports store-io/node:/atomically. - Direct unit tests for the isomorphic remote + session backends (restoring oauth-persist.ts coverage after the file backend moved out). - store.test.ts: OAuthMemoryStore unit coverage incl. replace() fallbacks. - storage.test.ts: clearAllOAuthClientState null-snapshot (no file) path. Verified end-to-end in a real browser (Playwright): DCR OAuth flow completes, tokens persist via the remote store, and after a full page reload the server reconnects with no re-authentication. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
cdeb2316d0
commit
1e6a2825d4
10 changed files with 368 additions and 52 deletions
|
|
@ -0,0 +1,47 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { readFileSync } from "node:fs";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { dirname, resolve } from "node:path";
|
||||
|
||||
/**
|
||||
* Regression guard for the browser bundle. `core/auth/oauth-persist.ts` is
|
||||
* imported by browser code (App.tsx -> RemoteOAuthStorage -> oauth-persist),
|
||||
* so it must stay Node-free. It previously imported `store-io.js`, which pulls
|
||||
* `node:fs` + `atomically` (-> `stubborn-fs` -> `node:process.getuid`) into the
|
||||
* browser bundle and blanked the app at runtime. The Node-only file backend
|
||||
* now lives in `core/auth/node/oauth-persist-file.ts`; the isomorphic module
|
||||
* must only import the Node-free `store-serialize` helpers.
|
||||
*/
|
||||
|
||||
const here = dirname(fileURLToPath(import.meta.url));
|
||||
const coreAuth = resolve(here, "../../../../../../core/auth");
|
||||
|
||||
function importSpecifiers(source: string): string[] {
|
||||
const specs: string[] = [];
|
||||
const re = /(?:import|export)[^;]*?from\s*["']([^"']+)["']/g;
|
||||
let m: RegExpExecArray | null;
|
||||
while ((m = re.exec(source)) !== null) {
|
||||
specs.push(m[1]);
|
||||
}
|
||||
return specs;
|
||||
}
|
||||
|
||||
describe("oauth-persist.ts stays browser-safe", () => {
|
||||
const source = readFileSync(resolve(coreAuth, "oauth-persist.ts"), "utf-8");
|
||||
const specs = importSpecifiers(source);
|
||||
|
||||
it("does not import the Node-only store-io module", () => {
|
||||
expect(specs).not.toContain("../storage/store-io.js");
|
||||
});
|
||||
|
||||
it("does not import node: builtins or atomically", () => {
|
||||
const forbidden = specs.filter(
|
||||
(s) => s.startsWith("node:") || s === "atomically",
|
||||
);
|
||||
expect(forbidden).toEqual([]);
|
||||
});
|
||||
|
||||
it("gets its (de)serializers from the Node-free store-serialize module", () => {
|
||||
expect(specs).toContain("../storage/store-serialize.js");
|
||||
});
|
||||
});
|
||||
|
|
@ -1,8 +1,21 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import {
|
||||
parseOAuthPersistBlob,
|
||||
serializeOAuthPersistBlob,
|
||||
createRemoteOAuthPersistBackend,
|
||||
createSessionOAuthPersistBackend,
|
||||
OAUTH_PERSIST_STORAGE_KEY,
|
||||
} from "@inspector/core/auth/oauth-persist.js";
|
||||
import type { OAuthPersistSnapshot } from "@inspector/core/auth/oauth-persist.js";
|
||||
|
||||
const SNAPSHOT: OAuthPersistSnapshot = {
|
||||
servers: { "http://s": { scope: "read" } },
|
||||
idpSessions: {},
|
||||
};
|
||||
|
||||
function jsonResponse(body: unknown, status = 200): Response {
|
||||
return new Response(JSON.stringify(body), { status });
|
||||
}
|
||||
|
||||
describe("parseOAuthPersistBlob", () => {
|
||||
it("returns null for empty input", () => {
|
||||
|
|
@ -69,3 +82,141 @@ describe("serializeOAuthPersistBlob", () => {
|
|||
expect(raw).not.toMatch(/"state"\s*:/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("createRemoteOAuthPersistBackend", () => {
|
||||
const baseUrl = "http://remote.example/";
|
||||
const storeId = "oauth";
|
||||
const url = "http://remote.example/api/storage/oauth";
|
||||
|
||||
it("read() returns the parsed snapshot and sends the auth header", async () => {
|
||||
const fetchFn = vi.fn(async () => jsonResponse(SNAPSHOT));
|
||||
const backend = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
authToken: "tok",
|
||||
fetchFn: fetchFn as unknown as typeof fetch,
|
||||
});
|
||||
expect(await backend.read()).toEqual(SNAPSHOT);
|
||||
expect(fetchFn).toHaveBeenCalledWith(url, {
|
||||
method: "GET",
|
||||
headers: { "x-mcp-remote-auth": "Bearer tok" },
|
||||
});
|
||||
});
|
||||
|
||||
it("read() returns null for the empty-object missing-file response", async () => {
|
||||
const backend = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () => jsonResponse({})) as unknown as typeof fetch,
|
||||
});
|
||||
expect(await backend.read()).toBeNull();
|
||||
});
|
||||
|
||||
it("read() returns null on 404 and throws on other errors", async () => {
|
||||
const notFound = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 404 })) as unknown as typeof fetch,
|
||||
});
|
||||
expect(await notFound.read()).toBeNull();
|
||||
|
||||
const failing = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 500 })) as unknown as typeof fetch,
|
||||
});
|
||||
await expect(failing.read()).rejects.toThrow(/Failed to read store: 500/);
|
||||
});
|
||||
|
||||
it("write() POSTs the serialized snapshot and throws on failure", async () => {
|
||||
const ok = vi.fn(async () => new Response("", { status: 200 }));
|
||||
const backend = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: ok as unknown as typeof fetch,
|
||||
});
|
||||
await backend.write(SNAPSHOT);
|
||||
expect(ok).toHaveBeenCalledWith(
|
||||
url,
|
||||
expect.objectContaining({ method: "POST" }),
|
||||
);
|
||||
const body = ok.mock.calls[0][1].body;
|
||||
expect(JSON.parse(body)).toEqual(SNAPSHOT);
|
||||
|
||||
const failing = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 500 })) as unknown as typeof fetch,
|
||||
});
|
||||
await expect(failing.write(SNAPSHOT)).rejects.toThrow(
|
||||
/Failed to write store: 500/,
|
||||
);
|
||||
});
|
||||
|
||||
it("remove() DELETEs, tolerates 404, and throws on other errors", async () => {
|
||||
const ok = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
authToken: "tok",
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 200 })) as unknown as typeof fetch,
|
||||
});
|
||||
await expect(ok.remove!()).resolves.toBeUndefined();
|
||||
|
||||
const gone = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 404 })) as unknown as typeof fetch,
|
||||
});
|
||||
await expect(gone.remove!()).resolves.toBeUndefined();
|
||||
|
||||
const failing = createRemoteOAuthPersistBackend({
|
||||
baseUrl,
|
||||
storeId,
|
||||
fetchFn: (async () =>
|
||||
new Response("", { status: 500 })) as unknown as typeof fetch,
|
||||
});
|
||||
await expect(failing.remove!()).rejects.toThrow(
|
||||
/Failed to delete store: 500/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("createSessionOAuthPersistBackend", () => {
|
||||
function fakeStorage() {
|
||||
const map = new Map<string, string>();
|
||||
return {
|
||||
getItem: (k: string) => map.get(k) ?? null,
|
||||
setItem: (k: string, v: string) => void map.set(k, v),
|
||||
removeItem: (k: string) => void map.delete(k),
|
||||
map,
|
||||
} as unknown as Storage & { map: Map<string, string> };
|
||||
}
|
||||
|
||||
it("round-trips a snapshot through the default storage key", async () => {
|
||||
const storage = fakeStorage();
|
||||
const backend = createSessionOAuthPersistBackend({
|
||||
getStorage: () => storage,
|
||||
});
|
||||
expect(await backend.read()).toBeNull();
|
||||
await backend.write(SNAPSHOT);
|
||||
expect(storage.map.has(OAUTH_PERSIST_STORAGE_KEY)).toBe(true);
|
||||
expect(await backend.read()).toEqual(SNAPSHOT);
|
||||
await backend.remove!();
|
||||
expect(await backend.read()).toBeNull();
|
||||
});
|
||||
|
||||
it("honors a custom storage key", async () => {
|
||||
const storage = fakeStorage();
|
||||
const backend = createSessionOAuthPersistBackend({
|
||||
storageKey: "custom-key",
|
||||
getStorage: () => storage,
|
||||
});
|
||||
await backend.write(SNAPSHOT);
|
||||
expect(storage.map.has("custom-key")).toBe(true);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
84
clients/web/src/test/core/auth/store.test.ts
Normal file
84
clients/web/src/test/core/auth/store.test.ts
Normal file
|
|
@ -0,0 +1,84 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { OAuthMemoryStore } from "@inspector/core/auth/store.js";
|
||||
|
||||
describe("OAuthMemoryStore", () => {
|
||||
it("returns empty defaults for unknown server / issuer", () => {
|
||||
const store = new OAuthMemoryStore();
|
||||
expect(store.getState().getServerState("http://x")).toEqual({});
|
||||
expect(store.getState().getIdpSession("http://idp")).toEqual({});
|
||||
});
|
||||
|
||||
it("merges partial updates immutably via setServerState / setIdpSession", () => {
|
||||
const store = new OAuthMemoryStore();
|
||||
const s = store.getState();
|
||||
s.setServerState("http://x", { scope: "read" });
|
||||
s.setServerState("http://x", { codeVerifier: "v" });
|
||||
expect(store.getState().getServerState("http://x")).toEqual({
|
||||
scope: "read",
|
||||
codeVerifier: "v",
|
||||
});
|
||||
s.setIdpSession("http://idp", { idToken: "t" });
|
||||
expect(store.getState().getIdpSession("http://idp")).toEqual({
|
||||
idToken: "t",
|
||||
});
|
||||
});
|
||||
|
||||
it("clears a single server and a single idp session", () => {
|
||||
const store = new OAuthMemoryStore({
|
||||
servers: { "http://x": { scope: "read" } },
|
||||
idpSessions: { "http://idp": { idToken: "t" } },
|
||||
});
|
||||
store.getState().clearServerState("http://x");
|
||||
store.getState().clearIdpSession("http://idp");
|
||||
expect(store.getState().getServerState("http://x")).toEqual({});
|
||||
expect(store.getState().getIdpSession("http://idp")).toEqual({});
|
||||
});
|
||||
|
||||
it("clears only enterprise-managed servers", () => {
|
||||
const store = new OAuthMemoryStore({
|
||||
servers: {
|
||||
"http://managed": { enterpriseManaged: true },
|
||||
"http://plain": { scope: "read" },
|
||||
},
|
||||
idpSessions: {},
|
||||
});
|
||||
store.getState().clearEnterpriseManagedResourceServers();
|
||||
expect(store.getState().getServerState("http://managed")).toEqual({});
|
||||
expect(store.getState().getServerState("http://plain")).toEqual({
|
||||
scope: "read",
|
||||
});
|
||||
});
|
||||
|
||||
it("snapshot() returns a shallow copy of current state", () => {
|
||||
const store = new OAuthMemoryStore({
|
||||
servers: { "http://x": { scope: "read" } },
|
||||
idpSessions: {},
|
||||
});
|
||||
const snap = store.snapshot();
|
||||
expect(snap).toEqual({
|
||||
servers: { "http://x": { scope: "read" } },
|
||||
idpSessions: {},
|
||||
});
|
||||
// Mutating the store afterward must not change the earlier snapshot.
|
||||
store.getState().setServerState("http://y", { scope: "write" });
|
||||
expect(snap.servers["http://y"]).toBeUndefined();
|
||||
});
|
||||
|
||||
it("replace() tolerates a snapshot missing servers or idpSessions", () => {
|
||||
// Exercises both `?? {}` fallbacks in replace().
|
||||
const noServers = new OAuthMemoryStore({
|
||||
idpSessions: { "http://idp": { idToken: "t" } },
|
||||
} as never);
|
||||
expect(noServers.snapshot()).toEqual({
|
||||
servers: {},
|
||||
idpSessions: { "http://idp": { idToken: "t" } },
|
||||
});
|
||||
const noSessions = new OAuthMemoryStore({
|
||||
servers: { "http://x": { scope: "read" } },
|
||||
} as never);
|
||||
expect(noSessions.snapshot()).toEqual({
|
||||
servers: { "http://x": { scope: "read" } },
|
||||
idpSessions: {},
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -579,6 +579,18 @@ describe("NodeOAuthStorage with custom storagePath", () => {
|
|||
resetNodeOAuthStorageCache();
|
||||
});
|
||||
|
||||
it("clearAllOAuthClientState is a no-op when no store file exists", async () => {
|
||||
// Exercises the null-snapshot branch: read() returns null, so
|
||||
// `snapshot?.servers ?? {}` yields no urls and nothing is cleared.
|
||||
const filePath = getStateFilePath();
|
||||
resetNodeOAuthStorageCache();
|
||||
await fs.unlink(filePath).catch(() => {});
|
||||
|
||||
await expect(clearAllOAuthClientState()).resolves.toBeUndefined();
|
||||
|
||||
resetNodeOAuthStorageCache();
|
||||
});
|
||||
|
||||
it("should isolate state from default store", async () => {
|
||||
const customPath = path.join(
|
||||
os.tmpdir(),
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ import type { ServerType } from "@hono/node-server";
|
|||
import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js";
|
||||
import { RemoteOAuthStorage } from "@inspector/core/auth/remote/storage-remote.js";
|
||||
import { OAuthMemoryStore } from "@inspector/core/auth/store.js";
|
||||
import { createFileOAuthPersistBackend } from "@inspector/core/auth/oauth-persist.js";
|
||||
import { createFileOAuthPersistBackend } from "@inspector/core/auth/node/oauth-persist-file.js";
|
||||
import { createRemoteApp } from "@inspector/core/mcp/remote/node/server.js";
|
||||
import {
|
||||
writeStoreFile,
|
||||
|
|
|
|||
42
core/auth/node/oauth-persist-file.ts
Normal file
42
core/auth/node/oauth-persist-file.ts
Normal file
|
|
@ -0,0 +1,42 @@
|
|||
/**
|
||||
* Node-only file backend for OAuth persistence. Kept out of the isomorphic
|
||||
* `core/auth/oauth-persist.ts` because it imports `store-io.js` (which pulls
|
||||
* `node:fs`/`atomically`); the browser must never load that. Node consumers
|
||||
* (e.g. `NodeOAuthStorage`) import the file backend from here, while the
|
||||
* shared blob (de)serialization and browser/remote backends stay isomorphic.
|
||||
*/
|
||||
|
||||
import {
|
||||
readStoreFile,
|
||||
writeStoreFile,
|
||||
deleteStoreFile,
|
||||
} from "../../storage/store-io.js";
|
||||
import {
|
||||
parseOAuthPersistBlob,
|
||||
serializeOAuthPersistBlob,
|
||||
type OAuthPersistBackend,
|
||||
} from "../oauth-persist.js";
|
||||
|
||||
export interface FileOAuthPersistBackendOptions {
|
||||
filePath: string;
|
||||
}
|
||||
|
||||
export function createFileOAuthPersistBackend(
|
||||
options: FileOAuthPersistBackendOptions,
|
||||
): OAuthPersistBackend {
|
||||
return {
|
||||
async read() {
|
||||
const raw = await readStoreFile(options.filePath);
|
||||
return parseOAuthPersistBlob(raw);
|
||||
},
|
||||
async write(snapshot) {
|
||||
await writeStoreFile(
|
||||
options.filePath,
|
||||
serializeOAuthPersistBlob(snapshot),
|
||||
);
|
||||
},
|
||||
async remove() {
|
||||
await deleteStoreFile(options.filePath);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
@ -1,6 +1,6 @@
|
|||
import { OAuthStorageBase } from "../oauth-storage.js";
|
||||
import { OAuthMemoryStore } from "../store.js";
|
||||
import { createFileOAuthPersistBackend } from "../oauth-persist.js";
|
||||
import { createFileOAuthPersistBackend } from "./oauth-persist-file.js";
|
||||
import {
|
||||
getDefaultStorageDir,
|
||||
getStoreFilePath,
|
||||
|
|
|
|||
|
|
@ -1,17 +1,15 @@
|
|||
/**
|
||||
* OAuth persistence format and backends (file, remote HTTP, sessionStorage).
|
||||
* Writes plain JSON `{ servers, idpSessions }`. On read, accepts legacy
|
||||
* persist envelopes `{ state: { servers, idpSessions }, version }` and
|
||||
* promotes the inner payload.
|
||||
* OAuth persistence format and isomorphic backends (remote HTTP,
|
||||
* sessionStorage). Writes plain JSON `{ servers, idpSessions }`. On read,
|
||||
* accepts legacy persist envelopes `{ state: { servers, idpSessions },
|
||||
* version }` and promotes the inner payload.
|
||||
*
|
||||
* This module must stay browser-safe: it imports only the Node-free
|
||||
* `store-serialize` helpers, never `store-io` (which pulls `node:fs`). The
|
||||
* Node-only file backend lives in `./node/oauth-persist-file.ts`.
|
||||
*/
|
||||
|
||||
import {
|
||||
readStoreFile,
|
||||
writeStoreFile,
|
||||
deleteStoreFile,
|
||||
serializeStore,
|
||||
parseStore,
|
||||
} from "../storage/store-io.js";
|
||||
import { serializeStore, parseStore } from "../storage/store-serialize.js";
|
||||
import type { IdpSessionState } from "./storage.js";
|
||||
import type { ServerOAuthState } from "./store.js";
|
||||
|
||||
|
|
@ -76,30 +74,6 @@ export interface OAuthPersistBackend {
|
|||
remove?(): Promise<void>;
|
||||
}
|
||||
|
||||
export interface FileOAuthPersistBackendOptions {
|
||||
filePath: string;
|
||||
}
|
||||
|
||||
export function createFileOAuthPersistBackend(
|
||||
options: FileOAuthPersistBackendOptions,
|
||||
): OAuthPersistBackend {
|
||||
return {
|
||||
async read() {
|
||||
const raw = await readStoreFile(options.filePath);
|
||||
return parseOAuthPersistBlob(raw);
|
||||
},
|
||||
async write(snapshot) {
|
||||
await writeStoreFile(
|
||||
options.filePath,
|
||||
serializeOAuthPersistBlob(snapshot),
|
||||
);
|
||||
},
|
||||
async remove() {
|
||||
await deleteStoreFile(options.filePath);
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export interface RemoteOAuthPersistBackendOptions {
|
||||
baseUrl: string;
|
||||
storeId: string;
|
||||
|
|
|
|||
|
|
@ -12,6 +12,10 @@ import { readFile, writeFile } from "atomically";
|
|||
// isomorphic code can reuse it without pulling Node deps into the browser.
|
||||
export { validateStoreId } from "./store-id.js";
|
||||
|
||||
// Likewise re-export the pure JSON (de)serializers from the Node-free
|
||||
// `store-serialize` module so existing `store-io` importers are unaffected.
|
||||
export { serializeStore, parseStore } from "./store-serialize.js";
|
||||
|
||||
/**
|
||||
* Default storage directory (~/.mcp-inspector/storage or %USERPROFILE%\.mcp-inspector\storage on Windows).
|
||||
*/
|
||||
|
|
@ -120,17 +124,3 @@ export async function deleteStoreFile(filePath: string): Promise<void> {
|
|||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Serialize store data to JSON string (consistent format for server writes).
|
||||
*/
|
||||
export function serializeStore(data: unknown): string {
|
||||
return JSON.stringify(data, null, 2);
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse store JSON string. Use after readStoreFile when returning parsed object.
|
||||
*/
|
||||
export function parseStore(raw: string): unknown {
|
||||
return JSON.parse(raw);
|
||||
}
|
||||
|
|
|
|||
16
core/storage/store-serialize.ts
Normal file
16
core/storage/store-serialize.ts
Normal file
|
|
@ -0,0 +1,16 @@
|
|||
/**
|
||||
* Pure, Node-free JSON (de)serialization for store files. Split from
|
||||
* `store-io.ts` (which imports `node:fs`/`node:path`/`atomically`) so
|
||||
* isomorphic code — e.g. the browser OAuth persistence flow in
|
||||
* `core/auth/oauth-persist.ts` — can (de)serialize store blobs without
|
||||
* pulling Node deps into the browser bundle. `store-io.ts` re-exports these
|
||||
* so existing importers are unaffected.
|
||||
*/
|
||||
|
||||
export function serializeStore(data: unknown): string {
|
||||
return JSON.stringify(data, null, 2);
|
||||
}
|
||||
|
||||
export function parseStore(raw: string): unknown {
|
||||
return JSON.parse(raw);
|
||||
}
|
||||
Loading…
Reference in a new issue