fix(auth): guard OAuth-storage async paths against unhandled rejections
Review follow-ups on the RemoteOAuthStorage web migration: - useEmaIdpLoginState.logout(): add a .catch to the now-async clearEmaIdpSession call so a storage failure no longer surfaces as an unhandled rejection; leave loginState unchanged on failure (the session was not actually cleared). Add a test asserting no unhandled rejection. - createOAuthStore: attach a no-op catch to the eager persistLoadPromise so a hydration read that fails before any load() awaits it does not become an unhandled rejection; real awaiters still observe the rejection via the shared reference. - storage-remote.test: replace `as unknown as typeof fetch` double-casts with the typed vi.fn<typeof fetch>() pattern the same PR established. - remoteOAuthStorage: correct the doc comment to describe the cache-of-one accurately instead of implying a keyed map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
512f45d357
commit
ba48e06ffd
5 changed files with 80 additions and 9 deletions
|
|
@ -17,8 +17,12 @@ const defaultFetch: typeof fetch = (...args) => globalThis.fetch(...args);
|
||||||
* Shared web OAuth store: `RemoteOAuthStorage` → `GET/POST /api/storage/oauth`
|
* Shared web OAuth store: `RemoteOAuthStorage` → `GET/POST /api/storage/oauth`
|
||||||
* → `~/.mcp-inspector/storage/oauth.json` (same file as CLI/TUI).
|
* → `~/.mcp-inspector/storage/oauth.json` (same file as CLI/TUI).
|
||||||
*
|
*
|
||||||
* Memoized by `{ baseUrl, authToken }` so connect, EMA IdP session, and
|
* Caches a single instance keyed by `{ baseUrl, authToken }`: repeat calls with
|
||||||
* per-server clear all mutate the same in-memory view.
|
* the same key return that instance, so connect, EMA IdP session, and per-server
|
||||||
|
* clear all mutate the same in-memory view. A call with a different key replaces
|
||||||
|
* the cached instance (cache of one). That is sufficient because the web app
|
||||||
|
* uses a stable origin + page-lifetime API token, so the key never changes
|
||||||
|
* within a session.
|
||||||
*/
|
*/
|
||||||
export function getRemoteOAuthStorage(
|
export function getRemoteOAuthStorage(
|
||||||
options: WebRemoteOAuthStorageOptions,
|
options: WebRemoteOAuthStorageOptions,
|
||||||
|
|
|
||||||
|
|
@ -142,12 +142,12 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => {
|
||||||
version: 0,
|
version: 0,
|
||||||
};
|
};
|
||||||
let resolveFetch!: (value: Response) => void;
|
let resolveFetch!: (value: Response) => void;
|
||||||
const delayedFetch = vi.fn(
|
const delayedFetch = vi.fn<typeof fetch>(
|
||||||
() =>
|
() =>
|
||||||
new Promise<Response>((resolve) => {
|
new Promise<Response>((resolve) => {
|
||||||
resolveFetch = resolve;
|
resolveFetch = resolve;
|
||||||
}),
|
}),
|
||||||
) as unknown as typeof fetch;
|
);
|
||||||
|
|
||||||
const delayedStorage = new RemoteOAuthStorage({
|
const delayedStorage = new RemoteOAuthStorage({
|
||||||
baseUrl: "http://remote.example",
|
baseUrl: "http://remote.example",
|
||||||
|
|
@ -167,9 +167,9 @@ describe("RemoteOAuthStorage (unit, mocked fetch)", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
it("load() rejects when remote GET fails instead of hanging", async () => {
|
it("load() rejects when remote GET fails instead of hanging", async () => {
|
||||||
const failingFetch = vi.fn(
|
const failingFetch = vi.fn<typeof fetch>(
|
||||||
async () => new Response("server error", { status: 500 }),
|
async () => new Response("server error", { status: 500 }),
|
||||||
) as unknown as typeof fetch;
|
);
|
||||||
|
|
||||||
const failingStorage = new RemoteOAuthStorage({
|
const failingStorage = new RemoteOAuthStorage({
|
||||||
baseUrl: "http://remote.example",
|
baseUrl: "http://remote.example",
|
||||||
|
|
|
||||||
|
|
@ -67,6 +67,58 @@ describe("useEmaIdpLoginState", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("logout swallows a clear failure without an unhandled rejection and keeps state", async () => {
|
||||||
|
const exp = Math.floor(Date.now() / 1000) + 3600;
|
||||||
|
const payload = btoa(JSON.stringify({ exp }))
|
||||||
|
.replace(/\+/g, "-")
|
||||||
|
.replace(/\//g, "_")
|
||||||
|
.replace(/=+$/, "");
|
||||||
|
vi.mocked(storage.getIdpSession).mockResolvedValue({
|
||||||
|
idToken: `h.${payload}.s`,
|
||||||
|
});
|
||||||
|
vi.mocked(storage.clearIdpSession).mockRejectedValue(
|
||||||
|
new Error("storage backend unreachable"),
|
||||||
|
);
|
||||||
|
const unhandled = vi.fn();
|
||||||
|
const onUnhandled = (event: PromiseRejectionEvent) => {
|
||||||
|
event.preventDefault();
|
||||||
|
unhandled();
|
||||||
|
};
|
||||||
|
window.addEventListener("unhandledrejection", onUnhandled);
|
||||||
|
|
||||||
|
try {
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useEmaIdpLoginState(storage, "https://idp.test", true),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(result.current.loginState).toBe("logged_in");
|
||||||
|
});
|
||||||
|
|
||||||
|
act(() => {
|
||||||
|
result.current.logout();
|
||||||
|
});
|
||||||
|
|
||||||
|
await waitFor(() => {
|
||||||
|
expect(storage.clearIdpSession).toHaveBeenCalledWith(
|
||||||
|
"https://idp.test",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
// Give the rejected promise a turn to settle so any unhandled rejection
|
||||||
|
// would have fired.
|
||||||
|
await act(async () => {
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
// Clear failed, so the session is still present: state stays "logged_in"
|
||||||
|
// and no unhandled rejection escaped.
|
||||||
|
expect(result.current.loginState).toBe("logged_in");
|
||||||
|
expect(unhandled).not.toHaveBeenCalled();
|
||||||
|
} finally {
|
||||||
|
window.removeEventListener("unhandledrejection", onUnhandled);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
it("reports 'expired' for an expired token with no refresh token", async () => {
|
it("reports 'expired' for an expired token with no refresh token", async () => {
|
||||||
const exp = Math.floor(Date.now() / 1000) - 3600;
|
const exp = Math.floor(Date.now() / 1000) - 3600;
|
||||||
const payload = btoa(JSON.stringify({ exp }))
|
const payload = btoa(JSON.stringify({ exp }))
|
||||||
|
|
|
||||||
|
|
@ -78,6 +78,13 @@ export function createOAuthStore(
|
||||||
resolvePersistLoad = resolve;
|
resolvePersistLoad = resolve;
|
||||||
rejectPersistLoad = reject;
|
rejectPersistLoad = reject;
|
||||||
});
|
});
|
||||||
|
// Zustand kicks off async hydration as soon as the store is created, before
|
||||||
|
// any consumer calls `OAuthStorageBase.load()`. If that initial read fails
|
||||||
|
// and no one is awaiting yet, `persistLoadPromise` would reject with no
|
||||||
|
// handler and surface as an unhandled rejection. Attach a no-op handler so
|
||||||
|
// the promise is always "handled"; real awaiters still observe the rejection
|
||||||
|
// through the reference returned by `waitForOAuthStorePersistLoad`.
|
||||||
|
void persistLoadPromise.catch(() => {});
|
||||||
|
|
||||||
const settlePersistLoad = (error?: unknown) => {
|
const settlePersistLoad = (error?: unknown) => {
|
||||||
if (persistLoadSettled) {
|
if (persistLoadSettled) {
|
||||||
|
|
|
||||||
|
|
@ -40,9 +40,17 @@ export function useEmaIdpLoginState(
|
||||||
|
|
||||||
const logout = useCallback(() => {
|
const logout = useCallback(() => {
|
||||||
if (!normalizedIssuer) return;
|
if (!normalizedIssuer) return;
|
||||||
void clearEmaIdpSession(storage, normalizedIssuer).then(() => {
|
void clearEmaIdpSession(storage, normalizedIssuer)
|
||||||
setLoginState("none");
|
.then(() => {
|
||||||
});
|
setLoginState("none");
|
||||||
|
})
|
||||||
|
.catch(() => {
|
||||||
|
// Clearing the persisted IdP session failed (e.g. the storage backend
|
||||||
|
// is unreachable). Swallow the rejection so it does not surface as an
|
||||||
|
// unhandled promise, and leave loginState unchanged so the UI keeps
|
||||||
|
// reflecting the still-present session rather than falsely showing
|
||||||
|
// signed-out.
|
||||||
|
});
|
||||||
}, [storage, normalizedIssuer]);
|
}, [storage, normalizedIssuer]);
|
||||||
|
|
||||||
return { loginState, refresh, logout };
|
return { loginState, refresh, logout };
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue