* fix(auth): offer explicit re-authorize when callback state is lost (#1808) A `/oauth/callback` that arrives with no recorded SEP-2352 discovery state made the SDK throw `AuthorizationServerMismatchError` and the Inspector dead-ended, surfacing the raw SDK text with no path forward. The SDK folds two very different failures into that one error class, so `core/auth/issuerBinding.ts` classifies them structurally: a recoverable "lost authorization state" (the SDK puts prose, not an issuer URL, in the `recordedIssuer` slot) versus a genuine cross-AS `issuer_mismatch`. Only the first gets a recovery affordance. Web: the callback failure now raises a dedicated banner ("Authorization state was lost" / "Authorize again") whose action clears the stale OAuth state for that server and starts a fresh authorization. A genuine mismatch stays a red, non-dismissible security notification with both issuers named and no one-click retry. Node runners (CLI/TUI): the same classification rewrites the callback rejection into the shared actionable copy instead of the SDK text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt * fix(auth): detect the SDK mismatch error via isInstance (#1808) The callback-leg classifier gated on `err.mcpBrand`, but the SDK declares that brand in a `static {}` block, so it lives on the constructor and is never reachable from an instance. The predicate always returned false, so `findIssuerBindingFailure()` never matched and the #1808 recovery banner was inert in production — the callback dead-ended exactly as before. Use the SDK's own `AuthorizationServerMismatchError.isInstance()`, which is cross-copy safe by construction (it consults a `Symbol.for()`-keyed brand set), with a `name` fallback for a serialization boundary. Correct the module doc, which cited cross-bundle safety as the reason to avoid `instanceof` — the very thing the SDK's branded `hasInstance` provides. Every fixture now constructs a real `AuthorizationServerMismatchError` instead of a hand-rolled object with an own `mcpBrand`, which is why a green `npm run ci` hid this. Adds a test pinning the brand's placement so the feature cannot silently go inert again. Review follow-ups: drop the unused `isLostAuthorizationStateError`, remove the duplicated banner title default, resolve the banner title/action label where the banner is raised, keep the clear-failure notification open, and bound remote-supplied issuers in user-facing copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016S7h83UKNQ7kvSiFDNksCt --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
01df9d81bb
commit
7784553674
3 changed files with 1003 additions and 7 deletions
191
clients/web/src/test/core/auth/issuerBinding.test.ts
Normal file
191
clients/web/src/test/core/auth/issuerBinding.test.ts
Normal file
|
|
@ -0,0 +1,191 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client";
|
||||
import { findIssuerBindingFailure } from "@inspector/core/auth/issuerBinding.js";
|
||||
|
||||
/**
|
||||
* Builds a **real** SDK `AuthorizationServerMismatchError`.
|
||||
*
|
||||
* This must stay the real class rather than a look-alike plain object. The SDK
|
||||
* declares `mcpBrand` in a `static {}` block, so it sits on the constructor and
|
||||
* is invisible from the instance; a fabricated fixture carrying an own
|
||||
* `mcpBrand` property would pass against a classifier that reads
|
||||
* `err.mcpBrand` — which no real thrown error would ever satisfy.
|
||||
*/
|
||||
function mismatchError(
|
||||
recordedIssuer: string,
|
||||
currentIssuer: string,
|
||||
): AuthorizationServerMismatchError {
|
||||
return new AuthorizationServerMismatchError(recordedIssuer, currentIssuer);
|
||||
}
|
||||
|
||||
/**
|
||||
* Documents why the classifier must not test `err.mcpBrand`: the SDK declares
|
||||
* that brand `static`, so it lives on the class and instances never carry it.
|
||||
* If a future SDK moves it onto the instance, this fails and the note in
|
||||
* `issuerBinding.ts` can be revisited.
|
||||
*/
|
||||
describe("SDK brand placement", () => {
|
||||
it("keeps `mcpBrand` on the class, not the instance", () => {
|
||||
const err = mismatchError("https://a.example.com", "https://b.example.com");
|
||||
expect("mcpBrand" in err).toBe(false);
|
||||
// The brand is defined at runtime via `Object.defineProperty` in a static
|
||||
// block, so it is absent from the SDK's published type declarations — hence
|
||||
// the cast to read it.
|
||||
const brandOnClass = (
|
||||
AuthorizationServerMismatchError as { mcpBrand?: unknown }
|
||||
).mcpBrand;
|
||||
expect(brandOnClass).toBe("mcp.AuthorizationServerMismatchError");
|
||||
});
|
||||
|
||||
it("matches via `isInstance`, which survives a foreign SDK copy", () => {
|
||||
const err = mismatchError("https://a.example.com", "https://b.example.com");
|
||||
expect(AuthorizationServerMismatchError.isInstance(err)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* An error that crossed a serialization boundary: neither the SDK's
|
||||
* `Symbol.for()`-keyed brand set nor the prototype survives, but `name` does.
|
||||
* This is the fallback arm of the classifier's predicate.
|
||||
*/
|
||||
function serializedMismatchError(
|
||||
recordedIssuer: string,
|
||||
currentIssuer: string,
|
||||
): Error {
|
||||
const err = new Error(
|
||||
"Authorization server changed between redirect and callback",
|
||||
);
|
||||
err.name = "AuthorizationServerMismatchError";
|
||||
Object.assign(err, { recordedIssuer, currentIssuer });
|
||||
return err;
|
||||
}
|
||||
|
||||
const MISSING_STATE_SENTINEL =
|
||||
"discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier";
|
||||
|
||||
describe("findIssuerBindingFailure", () => {
|
||||
it("classifies the missing-discovery-state sentinel as recoverable", () => {
|
||||
const failure = findIssuerBindingFailure(
|
||||
mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"),
|
||||
);
|
||||
expect(failure).toEqual({
|
||||
kind: "lost_authorization_state",
|
||||
currentIssuer: "https://as.example.com",
|
||||
});
|
||||
});
|
||||
|
||||
it("classifies non-URL prose in the recorded slot as recoverable", () => {
|
||||
const failure = findIssuerBindingFailure(
|
||||
mismatchError("nothing was recorded, sorry", "https://as.example.com"),
|
||||
);
|
||||
expect(failure?.kind).toBe("lost_authorization_state");
|
||||
});
|
||||
|
||||
it("classifies a non-http(s) URL in the recorded slot as recoverable", () => {
|
||||
const failure = findIssuerBindingFailure(
|
||||
mismatchError("urn:example:not-an-issuer", "https://as.example.com"),
|
||||
);
|
||||
expect(failure?.kind).toBe("lost_authorization_state");
|
||||
});
|
||||
|
||||
it("classifies two real issuers as a genuine mismatch", () => {
|
||||
const failure = findIssuerBindingFailure(
|
||||
mismatchError("https://old.example.com", "https://evil.example.com"),
|
||||
);
|
||||
expect(failure).toEqual({
|
||||
kind: "issuer_mismatch",
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: "https://evil.example.com",
|
||||
});
|
||||
});
|
||||
|
||||
it("accepts a plain http issuer as a genuine mismatch", () => {
|
||||
const failure = findIssuerBindingFailure(
|
||||
mismatchError("http://localhost:9000", "http://localhost:9001"),
|
||||
);
|
||||
expect(failure?.kind).toBe("issuer_mismatch");
|
||||
});
|
||||
|
||||
it("walks the `cause` chain", () => {
|
||||
const wrapped = new Error("negotiation failed", {
|
||||
cause: mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"),
|
||||
});
|
||||
expect(findIssuerBindingFailure(wrapped)?.kind).toBe(
|
||||
"lost_authorization_state",
|
||||
);
|
||||
});
|
||||
|
||||
it("walks `data.cause` (SdkError wrapper shape)", () => {
|
||||
const wrapped = Object.assign(new Error("ERA_NEGOTIATION_FAILED"), {
|
||||
data: {
|
||||
cause: mismatchError("https://a.example.com", "https://b.example.com"),
|
||||
},
|
||||
});
|
||||
expect(findIssuerBindingFailure(wrapped)?.kind).toBe("issuer_mismatch");
|
||||
});
|
||||
|
||||
it("walks multiple levels", () => {
|
||||
const wrapped = new Error("outer", {
|
||||
cause: new Error("inner", {
|
||||
cause: mismatchError(MISSING_STATE_SENTINEL, "https://as.example.com"),
|
||||
}),
|
||||
});
|
||||
expect(findIssuerBindingFailure(wrapped)?.kind).toBe(
|
||||
"lost_authorization_state",
|
||||
);
|
||||
});
|
||||
|
||||
it("ignores a non-object `data`", () => {
|
||||
const wrapped = Object.assign(new Error("outer"), { data: "nope" });
|
||||
expect(findIssuerBindingFailure(wrapped)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("ignores a null `data`", () => {
|
||||
const wrapped = Object.assign(new Error("outer"), { data: null });
|
||||
expect(findIssuerBindingFailure(wrapped)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("terminates on a cyclic cause chain", () => {
|
||||
const outer: { cause?: unknown } = new Error("outer");
|
||||
outer.cause = outer;
|
||||
expect(findIssuerBindingFailure(outer)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("returns undefined for unrelated errors and non-objects", () => {
|
||||
expect(findIssuerBindingFailure(new Error("boom"))).toBeUndefined();
|
||||
expect(findIssuerBindingFailure(undefined)).toBeUndefined();
|
||||
expect(findIssuerBindingFailure(null)).toBeUndefined();
|
||||
expect(findIssuerBindingFailure("string")).toBeUndefined();
|
||||
});
|
||||
|
||||
it("ignores a matching error missing the issuer fields", () => {
|
||||
const err = new Error("half-shaped");
|
||||
err.name = "AuthorizationServerMismatchError";
|
||||
expect(findIssuerBindingFailure(err)).toBeUndefined();
|
||||
|
||||
const halfShaped = new Error("only recorded");
|
||||
halfShaped.name = "AuthorizationServerMismatchError";
|
||||
Object.assign(halfShaped, { recordedIssuer: "https://as.example.com" });
|
||||
expect(findIssuerBindingFailure(halfShaped)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("classifies a serialized error via the `name` fallback", () => {
|
||||
expect(
|
||||
findIssuerBindingFailure(
|
||||
serializedMismatchError(
|
||||
MISSING_STATE_SENTINEL,
|
||||
"https://as.example.com",
|
||||
),
|
||||
)?.kind,
|
||||
).toBe("lost_authorization_state");
|
||||
});
|
||||
|
||||
it("ignores a different SDK error carrying the same fields", () => {
|
||||
const err = Object.assign(new Error("other"), {
|
||||
recordedIssuer: "https://a.example.com",
|
||||
currentIssuer: "https://b.example.com",
|
||||
});
|
||||
err.name = "IssuerMismatchError";
|
||||
expect(findIssuerBindingFailure(err)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
|
@ -19,6 +19,12 @@ import {
|
|||
stepUpInsufficientScopeMessage,
|
||||
stepUpModalTitle,
|
||||
stepUpAuthorizeActionLabel,
|
||||
issuerBindingFailureCopy,
|
||||
issuerMismatchMessage,
|
||||
issuerMismatchTitle,
|
||||
lostAuthorizationStateActionLabel,
|
||||
lostAuthorizationStateMessage,
|
||||
lostAuthorizationStateTitle,
|
||||
} from "@inspector/core/auth/oauthUx.js";
|
||||
import type { AuthChallenge } from "@inspector/core/auth/challenge.js";
|
||||
|
||||
|
|
@ -334,3 +340,79 @@ describe("oauthUx re-auth banner", () => {
|
|||
expect(reAuthBannerMessage({})).toBe("Authentication needs attention.");
|
||||
});
|
||||
});
|
||||
|
||||
describe("oauthUx issuer-binding copy", () => {
|
||||
it("explains lost authorization state in plain language, with the server name", () => {
|
||||
const withName = lostAuthorizationStateMessage({ serverName: "svc" });
|
||||
expect(withName).toContain('"svc"');
|
||||
expect(withName).toContain("was lost");
|
||||
expect(withName).toContain("Authorize again");
|
||||
// Never leaks the SDK's security-flavoured wording.
|
||||
expect(withName).not.toContain("discoveryState");
|
||||
expect(withName).not.toContain("AuthorizationServerMismatchError");
|
||||
expect(lostAuthorizationStateMessage()).toContain("this server");
|
||||
expect(lostAuthorizationStateTitle()).toBe("Authorization state was lost");
|
||||
expect(lostAuthorizationStateActionLabel()).toBe("Authorize again");
|
||||
});
|
||||
|
||||
it("names both issuers for a genuine mismatch and offers no retry nudge", () => {
|
||||
const message = issuerMismatchMessage({
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: "https://evil.example.com",
|
||||
serverName: "svc",
|
||||
});
|
||||
expect(message).toContain('"svc"');
|
||||
expect(message).toContain("https://old.example.com");
|
||||
expect(message).toContain("https://evil.example.com");
|
||||
expect(message).toContain("was not exchanged");
|
||||
expect(message).not.toContain("Authorize again");
|
||||
expect(
|
||||
issuerMismatchMessage({
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: "https://evil.example.com",
|
||||
}),
|
||||
).toContain("this server");
|
||||
expect(issuerMismatchTitle()).toBe("Authorization server mismatch");
|
||||
});
|
||||
|
||||
it("bounds an overlong remote-supplied issuer in the mismatch copy", () => {
|
||||
const overlong = `https://evil.example.com/${"a".repeat(400)}`;
|
||||
const message = issuerMismatchMessage({
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: overlong,
|
||||
});
|
||||
expect(message).not.toContain(overlong);
|
||||
expect(message).toContain("…");
|
||||
// The short issuer on the same call is passed through untouched.
|
||||
expect(message).toContain("https://old.example.com");
|
||||
});
|
||||
|
||||
it("issuerBindingFailureCopy dispatches on the failure kind", () => {
|
||||
expect(
|
||||
issuerBindingFailureCopy(
|
||||
{
|
||||
kind: "lost_authorization_state",
|
||||
currentIssuer: "https://as.example.com",
|
||||
},
|
||||
{ serverName: "svc" },
|
||||
),
|
||||
).toEqual({
|
||||
title: lostAuthorizationStateTitle(),
|
||||
message: lostAuthorizationStateMessage({ serverName: "svc" }),
|
||||
});
|
||||
|
||||
expect(
|
||||
issuerBindingFailureCopy({
|
||||
kind: "issuer_mismatch",
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: "https://evil.example.com",
|
||||
}),
|
||||
).toEqual({
|
||||
title: issuerMismatchTitle(),
|
||||
message: issuerMismatchMessage({
|
||||
recordedIssuer: "https://old.example.com",
|
||||
currentIssuer: "https://evil.example.com",
|
||||
}),
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { AuthorizationServerMismatchError } from "@modelcontextprotocol/client";
|
||||
import {
|
||||
runRunnerInteractiveOAuth,
|
||||
type RunnerInteractiveOAuthClient,
|
||||
|
|
@ -249,6 +250,76 @@ describe("runRunnerInteractiveOAuth", () => {
|
|||
).rejects.toThrow("token exchange failed");
|
||||
});
|
||||
|
||||
it("rewrites a lost-authorization-state callback failure into actionable copy (#1808)", async () => {
|
||||
const redirectUrlProvider = { redirectUrl: "" };
|
||||
// Real SDK error — its `mcpBrand` is static, so a fabricated
|
||||
// instance-branded look-alike would not exercise the real shape.
|
||||
const sdkError = new AuthorizationServerMismatchError(
|
||||
"discoveryState was not available on the callback leg; ensure your provider persists discoveryState alongside codeVerifier",
|
||||
"https://as.example",
|
||||
);
|
||||
const client = mockClient({
|
||||
authenticate: vi.fn(async () => {
|
||||
await simulateCallback(handlers.current);
|
||||
return new URL("https://as.example/authorize");
|
||||
}),
|
||||
completeOAuthFlow: vi.fn(async () => {
|
||||
throw sdkError;
|
||||
}),
|
||||
});
|
||||
|
||||
const failure = await runRunnerInteractiveOAuth({
|
||||
client,
|
||||
redirectUrlProvider,
|
||||
callbackListen: {
|
||||
hostname: "127.0.0.1",
|
||||
port: 6276,
|
||||
pathname: "/oauth/callback",
|
||||
},
|
||||
createCallbackServer: () => createMockCallbackServer(handlers),
|
||||
}).catch((err: unknown) => err);
|
||||
|
||||
expect(failure).toBeInstanceOf(Error);
|
||||
const message = (failure as Error).message;
|
||||
expect(message).toContain("Authorization state was lost");
|
||||
expect(message).toContain("Authorize again");
|
||||
expect(message).not.toContain("discoveryState");
|
||||
expect((failure as Error).cause).toBe(sdkError);
|
||||
});
|
||||
|
||||
it("keeps the security wording for a genuine issuer mismatch (#1808)", async () => {
|
||||
const redirectUrlProvider = { redirectUrl: "" };
|
||||
const sdkError = new AuthorizationServerMismatchError(
|
||||
"https://old.example",
|
||||
"https://evil.example",
|
||||
);
|
||||
const client = mockClient({
|
||||
authenticate: vi.fn(async () => {
|
||||
await simulateCallback(handlers.current);
|
||||
return new URL("https://as.example/authorize");
|
||||
}),
|
||||
completeOAuthFlow: vi.fn(async () => {
|
||||
throw sdkError;
|
||||
}),
|
||||
});
|
||||
|
||||
const failure = await runRunnerInteractiveOAuth({
|
||||
client,
|
||||
redirectUrlProvider,
|
||||
callbackListen: {
|
||||
hostname: "127.0.0.1",
|
||||
port: 6276,
|
||||
pathname: "/oauth/callback",
|
||||
},
|
||||
createCallbackServer: () => createMockCallbackServer(handlers),
|
||||
}).catch((err: unknown) => err);
|
||||
|
||||
const message = (failure as Error).message;
|
||||
expect(message).toContain("Authorization server mismatch");
|
||||
expect(message).toContain("https://evil.example");
|
||||
expect(message).not.toContain("Authorize again");
|
||||
});
|
||||
|
||||
it("propagates OAuth callback errors from the authorization server", async () => {
|
||||
const redirectUrlProvider = { redirectUrl: "" };
|
||||
const client = mockClient({
|
||||
|
|
|
|||
Loading…
Reference in a new issue