OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1694)
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
Some checks failed
Run install, format, lint, build, and test on every push / build (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish (push) Has been cancelled
Run install, format, lint, build, and test on every push / publish-github-container-registry (push) Has been cancelled
* OAuth: re-key store to (server, issuer) + SEP-837/2350/2352 auth UX (#1625) Re-key the persisted OAuth store to (server, issuer) and add the SEP-837/2350/2352 authorization-hardening UX. The SDK v2 auth() drives the SEP-2352 flow off the provider's ctx.issuer; the Inspector's job is to honor it and round-trip the issuer stamp. SEP-2352 — issuer-bound credentials - ServerOAuthState nests issuer-bound credentials under byIssuer[issuer] = { clientInformation, clientRegistrationKind, tokens }, with an activeIssuer pointer answering the transport's ctx-less per-request bearer read. - Fix a latent bug: OAuthStorageBase re-parsed tokens/client-info through OAuthTokensSchema/OAuthClientInformationSchema, which strip the non-RFC `issuer` field and silently defeat the SDK's discardIfIssuerMismatch. The store re-attaches the stamp from the byIssuer key. - BaseOAuthClientProvider threads ctx.issuer and implements discoveryState()/saveDiscoveryState() (activates the SDK's callback-leg AuthorizationServerMismatchError binding check) and invalidateCredentials(). - Lazy, migration-free upgrade: a legacy pre-1625 blob's top-level clientInformation/tokens are returned as an unkeyed fallback until the first issuer-stamped save promotes them into byIssuer. SEP-837 — application_type - DCR declares application_type: "native" (localhost Inspector). - ClientSettingsForm renders RegistrationRejectedError detail and reflects DCR's deprecated-in-favor-of-CIMD (SEP-991) status. SEP-2350 — step-up scope union - Per-server onInsufficientScope (reauthorize | throw) setting wired ServerSettingsForm -> StoredMCPServer.oauth -> StreamableHTTPClientTransport. - StepUpAuthModal visualizes the prior-union-challenged scope set, tagging each scope `new` vs `already granted`. mcpAuth simplification - Delete the hand-rolled authorizeWithoutRefresh (mishandled issuer stamping); forward forceReauthorization/skipIssuerMetadataValidation to native SDK auth() so both the initial and step-up paths get SEP-2352 stamping. Vocabulary + docs - OAuthStep gains cimd_fetch / issuer_comparison / scope_step_up (recorded on a step-up redirect); the Network tab labels auth requests by OAuth phase. - v2_auth_hardening.md gains an as-built status section. Deferred to the era-aware card: the SdkError(EraNegotiationFailed) -> UnauthorizedError unwrap and finishAuth(URLSearchParams) form (only fire under non-legacy negotiation). Closes #1625 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address @claude review: issuer-stamp source gating + clear/activeIssuer + phase over-match - SEP-2352: stamp the issuer only when the credential actually came from the byIssuer slot (`fromSlot`), not merely when a slot exists. A partially-populated slot alongside a surviving legacy field would otherwise stamp a legacy (possibly cross-AS) credential with the current issuer, defeating discardIfIssuerMismatch. - Issuer-scoped clears no longer promote the cleared issuer to `activeIssuer` (updateIssuerSlot gains a `setActive` flag; save paths keep the promotion). - oauthNetworkPhase matches register/token/authorize as the final path segment (trailing slash tolerated) so `/token/refresh` or `/api/register/foo` no longer over-match. - Tests for all three. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
f915889fc4
commit
7fecebed5e
10 changed files with 1620 additions and 601 deletions
|
|
@ -56,6 +56,9 @@ function createStorage(
|
|||
clearIdpSession: vi.fn(),
|
||||
clearEnterpriseManagedResourceServers: vi.fn(),
|
||||
getCodeVerifier: vi.fn(),
|
||||
getDiscoveryState: vi.fn().mockResolvedValue(undefined),
|
||||
saveDiscoveryState: vi.fn(),
|
||||
clearDiscoveryState: vi.fn(),
|
||||
};
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,20 +1,8 @@
|
|||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import type { OAuthClientProvider } from "@modelcontextprotocol/client";
|
||||
|
||||
const {
|
||||
sdkAuth,
|
||||
discoverOAuthServerInfo,
|
||||
startAuthorization,
|
||||
selectResourceURL,
|
||||
registerClient,
|
||||
isHttpsUrl,
|
||||
} = vi.hoisted(() => ({
|
||||
const { sdkAuth } = vi.hoisted(() => ({
|
||||
sdkAuth: vi.fn(),
|
||||
discoverOAuthServerInfo: vi.fn(),
|
||||
startAuthorization: vi.fn(),
|
||||
selectResourceURL: vi.fn(),
|
||||
registerClient: vi.fn(),
|
||||
isHttpsUrl: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@modelcontextprotocol/client", async (importOriginal) => {
|
||||
|
|
@ -23,20 +11,12 @@ vi.mock("@modelcontextprotocol/client", async (importOriginal) => {
|
|||
return {
|
||||
...actual,
|
||||
auth: sdkAuth,
|
||||
discoverOAuthServerInfo,
|
||||
selectResourceURL,
|
||||
startAuthorization,
|
||||
registerClient,
|
||||
isHttpsUrl,
|
||||
};
|
||||
});
|
||||
|
||||
import { mcpAuth } from "@inspector/core/auth/mcpAuth.js";
|
||||
import { InvalidClientMetadataError } from "@inspector/core/auth/mcpAuth.js";
|
||||
|
||||
function makeProvider(
|
||||
overrides: Partial<OAuthClientProvider> = {},
|
||||
): OAuthClientProvider {
|
||||
function makeProvider(): OAuthClientProvider {
|
||||
return {
|
||||
redirectUrl: "http://localhost/callback",
|
||||
clientMetadata: {
|
||||
|
|
@ -54,47 +34,22 @@ function makeProvider(
|
|||
redirectToAuthorization: vi.fn(),
|
||||
saveCodeVerifier: vi.fn(),
|
||||
codeVerifier: vi.fn().mockReturnValue("verifier"),
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Arrange the discovery + authorization mocks used by the
|
||||
* `forceReauthorization` (authorize-without-refresh) path.
|
||||
*/
|
||||
function stubForcePath(
|
||||
discovery: Partial<{
|
||||
authorizationServerUrl: string;
|
||||
authorizationServerMetadata: Record<string, unknown>;
|
||||
resourceMetadata: unknown;
|
||||
}> = {},
|
||||
): void {
|
||||
discoverOAuthServerInfo.mockResolvedValue({
|
||||
authorizationServerUrl: "https://as.example.com",
|
||||
authorizationServerMetadata: { issuer: "https://as.example.com" },
|
||||
resourceMetadata: undefined,
|
||||
...discovery,
|
||||
});
|
||||
selectResourceURL.mockResolvedValue(undefined);
|
||||
startAuthorization.mockResolvedValue({
|
||||
authorizationUrl: new URL("https://as.example.com/authorize"),
|
||||
codeVerifier: "cv",
|
||||
});
|
||||
}
|
||||
|
||||
describe("mcpAuth", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
isHttpsUrl.mockReturnValue(true);
|
||||
});
|
||||
|
||||
it("delegates to SDK auth() when forceReauthorization is not set", async () => {
|
||||
it("forwards the authorization-code exchange (incl. iss) to SDK auth()", async () => {
|
||||
sdkAuth.mockResolvedValue("AUTHORIZED");
|
||||
const provider = makeProvider();
|
||||
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
authorizationCode: "code",
|
||||
iss: "https://as.example.com",
|
||||
fetchFn: fetch,
|
||||
});
|
||||
|
||||
|
|
@ -102,14 +57,38 @@ describe("mcpAuth", () => {
|
|||
expect(sdkAuth).toHaveBeenCalledWith(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
authorizationCode: "code",
|
||||
iss: "https://as.example.com",
|
||||
scope: undefined,
|
||||
resourceMetadataUrl: undefined,
|
||||
fetchFn: fetch,
|
||||
skipIssuerMetadataValidation: undefined,
|
||||
forceReauthorization: undefined,
|
||||
});
|
||||
expect(discoverOAuthServerInfo).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects forceReauthorization combined with authorizationCode", async () => {
|
||||
it("forwards forceReauthorization + skipIssuerMetadataValidation to SDK auth()", async () => {
|
||||
sdkAuth.mockResolvedValue("REDIRECT");
|
||||
const provider = makeProvider();
|
||||
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
scope: "mcp weather:read",
|
||||
forceReauthorization: true,
|
||||
skipIssuerMetadataValidation: true,
|
||||
});
|
||||
|
||||
expect(result).toBe("REDIRECT");
|
||||
expect(sdkAuth).toHaveBeenCalledWith(
|
||||
provider,
|
||||
expect.objectContaining({
|
||||
scope: "mcp weather:read",
|
||||
forceReauthorization: true,
|
||||
skipIssuerMetadataValidation: true,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects forceReauthorization combined with authorizationCode without calling SDK auth()", async () => {
|
||||
const provider = makeProvider();
|
||||
|
||||
await expect(
|
||||
|
|
@ -122,192 +101,14 @@ describe("mcpAuth", () => {
|
|||
"forceReauthorization cannot be combined with authorizationCode",
|
||||
);
|
||||
expect(sdkAuth).not.toHaveBeenCalled();
|
||||
expect(discoverOAuthServerInfo).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("uses discovery + startAuthorization when forceReauthorization is true", async () => {
|
||||
stubForcePath();
|
||||
it("propagates the SDK auth() rejection", async () => {
|
||||
sdkAuth.mockRejectedValue(new Error("discovery failed"));
|
||||
const provider = makeProvider();
|
||||
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
scope: "mcp weather:read",
|
||||
forceReauthorization: true,
|
||||
fetchFn: fetch,
|
||||
});
|
||||
|
||||
expect(result).toBe("REDIRECT");
|
||||
expect(sdkAuth).not.toHaveBeenCalled();
|
||||
expect(startAuthorization).toHaveBeenCalled();
|
||||
expect(provider.saveCodeVerifier).toHaveBeenCalledWith("cv");
|
||||
expect(provider.redirectToAuthorization).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("persists discovery state and forwards the selected resource + state", async () => {
|
||||
stubForcePath({
|
||||
resourceMetadata: { resource: "https://mcp.example.com" },
|
||||
});
|
||||
selectResourceURL.mockResolvedValue(new URL("https://mcp.example.com/r"));
|
||||
const saveDiscoveryState = vi.fn().mockResolvedValue(undefined);
|
||||
const state = vi.fn().mockResolvedValue("state-123");
|
||||
const provider = makeProvider({ saveDiscoveryState, state });
|
||||
|
||||
await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
resourceMetadataUrl: new URL("https://mcp.example.com/.well-known/x"),
|
||||
forceReauthorization: true,
|
||||
});
|
||||
|
||||
expect(saveDiscoveryState).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
authorizationServerUrl: "https://as.example.com",
|
||||
resourceMetadataUrl: "https://mcp.example.com/.well-known/x",
|
||||
}),
|
||||
);
|
||||
expect(state).toHaveBeenCalled();
|
||||
expect(startAuthorization).toHaveBeenCalledWith(
|
||||
"https://as.example.com",
|
||||
expect.objectContaining({
|
||||
state: "state-123",
|
||||
resource: new URL("https://mcp.example.com/r"),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("derives scope from resourceMetadata.scopes_supported when no scope given", async () => {
|
||||
stubForcePath({
|
||||
resourceMetadata: { scopes_supported: ["mcp", "weather:read"] },
|
||||
});
|
||||
const provider = makeProvider();
|
||||
|
||||
await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
});
|
||||
|
||||
expect(startAuthorization).toHaveBeenCalledWith(
|
||||
"https://as.example.com",
|
||||
expect.objectContaining({ scope: "mcp weather:read" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back to clientMetadata.scope when nothing else provides scope", async () => {
|
||||
stubForcePath();
|
||||
const provider = makeProvider({
|
||||
clientMetadata: {
|
||||
...makeProvider().clientMetadata,
|
||||
scope: "fallback:scope",
|
||||
},
|
||||
});
|
||||
|
||||
await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
});
|
||||
|
||||
expect(startAuthorization).toHaveBeenCalledWith(
|
||||
"https://as.example.com",
|
||||
expect.objectContaining({ scope: "fallback:scope" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("dynamically registers a client when no client information exists", async () => {
|
||||
stubForcePath();
|
||||
registerClient.mockResolvedValue({ client_id: "registered" });
|
||||
const saveClientInformation = vi.fn().mockResolvedValue(undefined);
|
||||
const provider = makeProvider({
|
||||
clientInformation: vi.fn().mockResolvedValue(undefined),
|
||||
saveClientInformation,
|
||||
});
|
||||
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
scope: "mcp",
|
||||
forceReauthorization: true,
|
||||
});
|
||||
|
||||
expect(result).toBe("REDIRECT");
|
||||
expect(registerClient).toHaveBeenCalledWith(
|
||||
"https://as.example.com",
|
||||
expect.objectContaining({ scope: "mcp" }),
|
||||
);
|
||||
expect(saveClientInformation).toHaveBeenCalledWith({
|
||||
client_id: "registered",
|
||||
});
|
||||
});
|
||||
|
||||
it("uses a URL-based client id when the AS supports it", async () => {
|
||||
stubForcePath({
|
||||
authorizationServerMetadata: {
|
||||
issuer: "https://as.example.com",
|
||||
client_id_metadata_document_supported: true,
|
||||
},
|
||||
});
|
||||
isHttpsUrl.mockReturnValue(true);
|
||||
const saveClientInformation = vi.fn().mockResolvedValue(undefined);
|
||||
const provider = makeProvider({
|
||||
clientInformation: vi.fn().mockResolvedValue(undefined),
|
||||
clientMetadataUrl: "https://app.example.com/client.json",
|
||||
saveClientInformation,
|
||||
});
|
||||
|
||||
await mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
});
|
||||
|
||||
expect(registerClient).not.toHaveBeenCalled();
|
||||
expect(saveClientInformation).toHaveBeenCalledWith({
|
||||
client_id: "https://app.example.com/client.json",
|
||||
});
|
||||
});
|
||||
|
||||
it("rejects a non-HTTPS clientMetadataUrl", async () => {
|
||||
stubForcePath();
|
||||
isHttpsUrl.mockReturnValue(false);
|
||||
const provider = makeProvider({
|
||||
clientInformation: vi.fn().mockResolvedValue(undefined),
|
||||
clientMetadataUrl: "http://insecure.example.com/client.json",
|
||||
saveClientInformation: vi.fn(),
|
||||
});
|
||||
|
||||
await expect(
|
||||
mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
}),
|
||||
).rejects.toBeInstanceOf(InvalidClientMetadataError);
|
||||
expect(registerClient).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requires saveClientInformation for dynamic registration", async () => {
|
||||
stubForcePath();
|
||||
const provider = makeProvider({
|
||||
clientInformation: vi.fn().mockResolvedValue(undefined),
|
||||
saveClientInformation: undefined,
|
||||
});
|
||||
|
||||
await expect(
|
||||
mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
}),
|
||||
).rejects.toThrow(
|
||||
"OAuth client information must be saveable for dynamic registration",
|
||||
);
|
||||
expect(registerClient).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requires a redirectUrl for the authorization_code flow", async () => {
|
||||
stubForcePath();
|
||||
const provider = makeProvider({ redirectUrl: undefined });
|
||||
|
||||
await expect(
|
||||
mcpAuth(provider, {
|
||||
serverUrl: "https://mcp.example.com",
|
||||
forceReauthorization: true,
|
||||
}),
|
||||
).rejects.toThrow("redirectUrl is required for authorization_code flow");
|
||||
expect(startAuthorization).not.toHaveBeenCalled();
|
||||
mcpAuth(provider, { serverUrl: "https://mcp.example.com" }),
|
||||
).rejects.toThrow("discovery failed");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -223,7 +223,13 @@ describe("OAuthNavigation", () => {
|
|||
saveTokens: vi.fn(async () => undefined),
|
||||
saveCodeVerifier: vi.fn(async () => undefined),
|
||||
getCodeVerifier: vi.fn().mockResolvedValue(undefined),
|
||||
clear: vi.fn(),
|
||||
clear: vi.fn(async () => undefined),
|
||||
clearTokens: vi.fn(async () => undefined),
|
||||
clearClientInformation: vi.fn(async () => undefined),
|
||||
clearCodeVerifier: vi.fn(async () => undefined),
|
||||
clearDiscoveryState: vi.fn(async () => undefined),
|
||||
getDiscoveryState: vi.fn().mockResolvedValue(undefined),
|
||||
saveDiscoveryState: vi.fn(async () => undefined),
|
||||
getServerMetadata: vi.fn().mockResolvedValue(null),
|
||||
saveServerMetadata: vi.fn(async () => undefined),
|
||||
} as unknown as OAuthStorage;
|
||||
|
|
@ -362,17 +368,21 @@ describe("OAuthNavigation", () => {
|
|||
response_types_supported: ["code"],
|
||||
});
|
||||
|
||||
expect(storage.saveTokens).toHaveBeenCalledWith(SERVER, {
|
||||
access_token: "t",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
expect(storage.saveTokens).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
{
|
||||
access_token: "t",
|
||||
token_type: "Bearer",
|
||||
},
|
||||
{ issuer: undefined },
|
||||
);
|
||||
expect(storage.saveScope).toHaveBeenCalledWith(SERVER, "openid");
|
||||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
{
|
||||
client_id: "c",
|
||||
},
|
||||
{ registrationKind: "dcr" },
|
||||
{ registrationKind: "dcr", issuer: undefined },
|
||||
);
|
||||
expect(storage.savePreregisteredClientInformation).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
|
|
@ -386,5 +396,87 @@ describe("OAuthNavigation", () => {
|
|||
expect(typeof state).toBe("string");
|
||||
expect(state.length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it("declares application_type 'native' in DCR client metadata (SEP-837)", () => {
|
||||
const provider = makeProvider(makeStorage());
|
||||
expect(provider.clientMetadata.application_type).toBe("native");
|
||||
});
|
||||
|
||||
describe("SEP-2352 issuer threading", () => {
|
||||
it("forwards ctx.issuer to storage on clientInformation/tokens reads", async () => {
|
||||
const storage = makeStorage();
|
||||
const provider = makeProvider(storage);
|
||||
const issuer = "https://as.example.com";
|
||||
|
||||
await provider.clientInformation({ issuer });
|
||||
await provider.tokens({ issuer });
|
||||
|
||||
// Preregistered lookup (issuer-independent) then the per-issuer dynamic slot.
|
||||
expect(storage.getClientInformation).toHaveBeenCalledWith(SERVER, true);
|
||||
expect(storage.getClientInformation).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
false,
|
||||
issuer,
|
||||
);
|
||||
expect(storage.getTokens).toHaveBeenCalledWith(SERVER, issuer);
|
||||
});
|
||||
|
||||
it("keys saves by ctx.issuer and defaults registration kind to dcr", async () => {
|
||||
const storage = makeStorage();
|
||||
const provider = makeProvider(storage);
|
||||
const issuer = "https://as.example.com";
|
||||
|
||||
await provider.saveClientInformation({ client_id: "c" }, { issuer });
|
||||
await provider.saveTokens(
|
||||
{ access_token: "t", token_type: "Bearer" },
|
||||
{ issuer },
|
||||
);
|
||||
|
||||
expect(storage.saveClientInformation).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
{ client_id: "c" },
|
||||
{ registrationKind: "dcr", issuer },
|
||||
);
|
||||
expect(storage.saveTokens).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
{ access_token: "t", token_type: "Bearer" },
|
||||
{ issuer },
|
||||
);
|
||||
});
|
||||
|
||||
it("round-trips discovery state to storage", async () => {
|
||||
const storage = makeStorage();
|
||||
const provider = makeProvider(storage);
|
||||
const discoveryState = {
|
||||
authorizationServerUrl: "https://as.example.com",
|
||||
};
|
||||
|
||||
await provider.saveDiscoveryState(discoveryState);
|
||||
await provider.discoveryState();
|
||||
|
||||
expect(storage.saveDiscoveryState).toHaveBeenCalledWith(
|
||||
SERVER,
|
||||
discoveryState,
|
||||
);
|
||||
expect(storage.getDiscoveryState).toHaveBeenCalledWith(SERVER);
|
||||
});
|
||||
|
||||
it("maps invalidateCredentials scopes to the matching storage clear", async () => {
|
||||
const storage = makeStorage();
|
||||
const provider = makeProvider(storage);
|
||||
|
||||
await provider.invalidateCredentials("all");
|
||||
await provider.invalidateCredentials("client");
|
||||
await provider.invalidateCredentials("tokens");
|
||||
await provider.invalidateCredentials("verifier");
|
||||
await provider.invalidateCredentials("discovery");
|
||||
|
||||
expect(storage.clear).toHaveBeenCalledWith(SERVER);
|
||||
expect(storage.clearClientInformation).toHaveBeenCalledWith(SERVER);
|
||||
expect(storage.clearTokens).toHaveBeenCalledWith(SERVER);
|
||||
expect(storage.clearCodeVerifier).toHaveBeenCalledWith(SERVER);
|
||||
expect(storage.clearDiscoveryState).toHaveBeenCalledWith(SERVER);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -429,4 +429,216 @@ describe("BrowserOAuthStorage", () => {
|
|||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("SEP-2352 per-issuer keying", () => {
|
||||
const issuerA = "https://as-a.example.com";
|
||||
const issuerB = "https://as-b.example.com";
|
||||
|
||||
it("keeps separate client registrations per issuer and re-attaches the issuer stamp on read", async () => {
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "client-a" },
|
||||
{ registrationKind: "dcr", issuer: issuerA },
|
||||
);
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "client-b" },
|
||||
{ registrationKind: "cimd", issuer: issuerB },
|
||||
);
|
||||
|
||||
// Each issuer resolves its own credentials, stamped with its issuer
|
||||
// (SEP-2352) — the stamp is what lets the SDK reject cross-AS reuse.
|
||||
expect(
|
||||
await storage.getClientInformation(testServerUrl, false, issuerA),
|
||||
).toEqual({ client_id: "client-a", issuer: issuerA });
|
||||
expect(
|
||||
await storage.getClientInformation(testServerUrl, false, issuerB),
|
||||
).toEqual({ client_id: "client-b", issuer: issuerB });
|
||||
expect(
|
||||
await storage.getClientRegistrationKind(testServerUrl, issuerA),
|
||||
).toBe("dcr");
|
||||
expect(
|
||||
await storage.getClientRegistrationKind(testServerUrl, issuerB),
|
||||
).toBe("cimd");
|
||||
});
|
||||
|
||||
it("answers a ctx-less token read from the most-recently-saved (active) issuer", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-b", token_type: "Bearer" },
|
||||
{ issuer: issuerB },
|
||||
);
|
||||
|
||||
// No issuer → active issuer (last saved = B); the per-request bearer read.
|
||||
expect(await storage.getTokens(testServerUrl)).toEqual({
|
||||
access_token: "tok-b",
|
||||
token_type: "Bearer",
|
||||
issuer: issuerB,
|
||||
});
|
||||
// Explicit issuer selects that AS's tokens.
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toEqual({
|
||||
access_token: "tok-a",
|
||||
token_type: "Bearer",
|
||||
issuer: issuerA,
|
||||
});
|
||||
});
|
||||
|
||||
it("promotes the legacy unkeyed slot: an unstamped token read falls back, then a stamped save supersedes it", async () => {
|
||||
// Legacy pre-1625 blob: unstamped tokens at the top level.
|
||||
await storage.saveTokens(testServerUrl, {
|
||||
access_token: "legacy",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
// Read for a specific issuer falls back to the unkeyed slot (unstamped).
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toEqual({
|
||||
access_token: "legacy",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
|
||||
// First stamped save promotes into byIssuer and clears the fallback.
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "fresh", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toEqual({
|
||||
access_token: "fresh",
|
||||
token_type: "Bearer",
|
||||
issuer: issuerA,
|
||||
});
|
||||
});
|
||||
|
||||
it("clears a single issuer's tokens, leaving other issuers intact", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-b", token_type: "Bearer" },
|
||||
{ issuer: issuerB },
|
||||
);
|
||||
|
||||
await storage.clearTokens(testServerUrl, issuerA);
|
||||
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toBeUndefined();
|
||||
expect(await storage.getTokens(testServerUrl, issuerB)).toMatchObject({
|
||||
access_token: "tok-b",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not stamp a legacy fallback credential when the issuer slot lacks it", async () => {
|
||||
// Legacy unkeyed client info survives, but a byIssuer slot exists for
|
||||
// issuerA holding only tokens (no clientInformation). Reading client info
|
||||
// for issuerA must fall back to the legacy value WITHOUT stamping it with
|
||||
// issuerA — the legacy credential may have been minted by a different AS.
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "legacy-client" },
|
||||
{ registrationKind: "dcr" },
|
||||
);
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
|
||||
const clientInfo = await storage.getClientInformation(
|
||||
testServerUrl,
|
||||
false,
|
||||
issuerA,
|
||||
);
|
||||
// No issuer stamp — so the SDK's discardIfIssuerMismatch stays engaged.
|
||||
expect(clientInfo).toEqual({ client_id: "legacy-client" });
|
||||
|
||||
// Symmetric case: legacy tokens survive, slot has only clientInformation.
|
||||
await storage.clear(testServerUrl);
|
||||
await storage.saveTokens(testServerUrl, {
|
||||
access_token: "legacy-tok",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "client-a" },
|
||||
{ registrationKind: "dcr", issuer: issuerA },
|
||||
);
|
||||
const tokens = await storage.getTokens(testServerUrl, issuerA);
|
||||
expect(tokens).toEqual({
|
||||
access_token: "legacy-tok",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not promote a cleared issuer to the active (ctx-less) slot", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
// Clearing a *different* issuer must not make it the active issuer.
|
||||
await storage.clearTokens(testServerUrl, issuerB);
|
||||
|
||||
// The ctx-less (per-request bearer) read still resolves issuerA's token.
|
||||
expect(await storage.getTokens(testServerUrl)).toMatchObject({
|
||||
access_token: "tok-a",
|
||||
});
|
||||
});
|
||||
|
||||
it("clears every issuer's tokens/registration when no issuer is given", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "tok-a", token_type: "Bearer" },
|
||||
{ issuer: issuerA },
|
||||
);
|
||||
await storage.saveClientInformation(
|
||||
testServerUrl,
|
||||
{ client_id: "client-b" },
|
||||
{ registrationKind: "dcr", issuer: issuerB },
|
||||
);
|
||||
|
||||
await storage.clearTokens(testServerUrl);
|
||||
await storage.clearClientInformation(testServerUrl);
|
||||
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toBeUndefined();
|
||||
expect(
|
||||
await storage.getClientInformation(testServerUrl, false, issuerB),
|
||||
).toBeUndefined();
|
||||
});
|
||||
|
||||
it("tags EMA resource tokens even on the issuer-keyed save path", async () => {
|
||||
await storage.saveTokens(
|
||||
testServerUrl,
|
||||
{ access_token: "ema", token_type: "Bearer" },
|
||||
{ issuer: issuerA, enterpriseManaged: true },
|
||||
);
|
||||
|
||||
await storage.clearEnterpriseManagedResourceServers();
|
||||
|
||||
expect(await storage.getTokens(testServerUrl, issuerA)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("discovery state (SEP-2352)", () => {
|
||||
it("saves, reads, and clears the discovery state", async () => {
|
||||
expect(await storage.getDiscoveryState(testServerUrl)).toBeUndefined();
|
||||
|
||||
const discoveryState = {
|
||||
authorizationServerUrl: "https://as.example.com",
|
||||
resourceMetadataUrl: "https://mcp.example.com/.well-known/x",
|
||||
};
|
||||
await storage.saveDiscoveryState(testServerUrl, discoveryState);
|
||||
expect(await storage.getDiscoveryState(testServerUrl)).toEqual(
|
||||
discoveryState,
|
||||
);
|
||||
|
||||
await storage.clearDiscoveryState(testServerUrl);
|
||||
expect(await storage.getDiscoveryState(testServerUrl)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -58,6 +58,9 @@ function createMockParams(
|
|||
saveIdpSession: vi.fn().mockResolvedValue(undefined),
|
||||
clearIdpSession: vi.fn(),
|
||||
clearEnterpriseManagedResourceServers: vi.fn(),
|
||||
getDiscoveryState: vi.fn().mockResolvedValue(undefined),
|
||||
saveDiscoveryState: vi.fn().mockResolvedValue(undefined),
|
||||
clearDiscoveryState: vi.fn().mockResolvedValue(undefined),
|
||||
};
|
||||
|
||||
const redirectUrlProvider = {
|
||||
|
|
@ -1231,6 +1234,9 @@ describe("OAuthManager", () => {
|
|||
expect(
|
||||
params.initialConfig.navigation!.navigateToAuthorization,
|
||||
).not.toHaveBeenCalled();
|
||||
// SEP-2350: a step-up redirect is recorded as `scope_step_up`, distinct
|
||||
// from a first-time `authorization_code` login.
|
||||
expect(manager.getOAuthFlowStep()).toBe("scope_step_up");
|
||||
captureSpy.mockRestore();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -863,6 +863,43 @@ describe("enterpriseManaged oauth settings", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("oauthOnInsufficientScope (SEP-2350)", () => {
|
||||
const baseSettings = {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: [],
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
taskTtl: 60000,
|
||||
maxFetchRequests: 1000,
|
||||
roots: [],
|
||||
};
|
||||
|
||||
it("lifts oauth.onInsufficientScope to settings", () => {
|
||||
const settings = storedFieldsToInspectorSettings({
|
||||
oauth: { clientId: "cid", onInsufficientScope: "throw" },
|
||||
});
|
||||
expect(settings?.oauthOnInsufficientScope).toBe("throw");
|
||||
});
|
||||
|
||||
it("persists onInsufficientScope under oauth on disk", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthClientId: "cid",
|
||||
oauthOnInsufficientScope: "throw",
|
||||
});
|
||||
expect(stored.oauth?.onInsufficientScope).toBe("throw");
|
||||
});
|
||||
|
||||
it("omits onInsufficientScope when unset", () => {
|
||||
const stored = inspectorSettingsToStoredFields({
|
||||
...baseSettings,
|
||||
oauthClientId: "cid",
|
||||
});
|
||||
expect(stored.oauth?.onInsufficientScope).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("envRecordToPairs / envPairsToRecord", () => {
|
||||
it("envRecordToPairs preserves key insertion order", () => {
|
||||
expect(envRecordToPairs({ B: "2", A: "1" })).toEqual([
|
||||
|
|
|
|||
|
|
@ -1071,8 +1071,12 @@ describe("InspectorClient OAuth E2E", () => {
|
|||
|
||||
expect(client.getStatus()).toBe("connected");
|
||||
|
||||
type StoredTokens = { tokens?: { access_token?: string } };
|
||||
type StateShape = {
|
||||
servers?: Record<string, { tokens?: { access_token?: string } }>;
|
||||
servers?: Record<
|
||||
string,
|
||||
StoredTokens & { byIssuer?: Record<string, StoredTokens> }
|
||||
>;
|
||||
};
|
||||
// Persistence is fire-and-forget; await the write rather than polling.
|
||||
await flushStoreFileWrites(customPath);
|
||||
|
|
@ -1081,8 +1085,16 @@ describe("InspectorClient OAuth E2E", () => {
|
|||
) as StateShape;
|
||||
const servers = parsed.servers ?? {};
|
||||
expect(Object.keys(servers).length).toBeGreaterThan(0);
|
||||
// SEP-2352: tokens persist under `byIssuer[issuer].tokens`; accept the
|
||||
// legacy top-level slot too.
|
||||
expect(
|
||||
Object.values(servers).some((s) => !!s?.tokens?.access_token),
|
||||
Object.values(servers).some(
|
||||
(s) =>
|
||||
!!s?.tokens?.access_token ||
|
||||
Object.values(s?.byIssuer ?? {}).some(
|
||||
(slot) => !!slot?.tokens?.access_token,
|
||||
),
|
||||
),
|
||||
).toBe(true);
|
||||
} finally {
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -351,12 +351,24 @@ describe("InspectorClient OAuth E2E with Remote Storage", () => {
|
|||
"oauth",
|
||||
remoteAuthToken!,
|
||||
(body) => {
|
||||
// SEP-2352: tokens persist under `byIssuer[issuer].tokens`, keyed by
|
||||
// authorization-server issuer. Accept the legacy top-level slot too.
|
||||
type StoredTokens = { tokens?: { access_token?: string } };
|
||||
const b = body as {
|
||||
servers?: Record<string, { tokens?: { access_token?: string } }>;
|
||||
servers?: Record<
|
||||
string,
|
||||
StoredTokens & { byIssuer?: Record<string, StoredTokens> }
|
||||
>;
|
||||
};
|
||||
return !!(
|
||||
b?.servers &&
|
||||
Object.values(b.servers).some((s) => s?.tokens?.access_token)
|
||||
Object.values(b.servers).some(
|
||||
(s) =>
|
||||
s?.tokens?.access_token ||
|
||||
Object.values(s?.byIssuer ?? {}).some(
|
||||
(slot) => slot?.tokens?.access_token,
|
||||
),
|
||||
)
|
||||
);
|
||||
},
|
||||
);
|
||||
|
|
|
|||
|
|
@ -259,6 +259,38 @@ describe("Transport", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("forwards settings.oauthOnInsufficientScope to the streamable-http transport (SEP-2350)", () => {
|
||||
const config: MCPServerConfig = {
|
||||
type: "streamable-http",
|
||||
url: "https://mcp.example.com/mcp",
|
||||
};
|
||||
const baseSettings: InspectorServerSettings = {
|
||||
headers: [],
|
||||
env: [],
|
||||
metadata: [],
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
taskTtl: 0,
|
||||
maxFetchRequests: 1000,
|
||||
roots: [],
|
||||
};
|
||||
|
||||
const asThrow = createTransportNode(config, {
|
||||
settings: { ...baseSettings, oauthOnInsufficientScope: "throw" },
|
||||
});
|
||||
expect(
|
||||
(asThrow.transport as unknown as { _onInsufficientScope?: string })
|
||||
._onInsufficientScope,
|
||||
).toBe("throw");
|
||||
|
||||
// Unset → the SDK's default policy.
|
||||
const asDefault = createTransportNode(config, { settings: baseSettings });
|
||||
expect(
|
||||
(asDefault.transport as unknown as { _onInsufficientScope?: string })
|
||||
._onInsufficientScope,
|
||||
).toBe("reauthorize");
|
||||
});
|
||||
|
||||
it("applies settings.headers to the outgoing SSE request", async () => {
|
||||
const server = createTestServerHttp({
|
||||
serverInfo: createTestServerInfo(),
|
||||
|
|
|
|||
|
|
@ -1478,6 +1478,67 @@ describe("/api/servers routes", () => {
|
|||
expect(body.error).toMatch(/settings\.env/);
|
||||
});
|
||||
|
||||
it("rejects an invalid settings.oauthOnInsufficientScope with 400", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: { type: "streamable-http", url: "https://x.test/mcp" },
|
||||
},
|
||||
}),
|
||||
);
|
||||
const res = await fetch(`${h.baseUrl}/api/servers/srv`, {
|
||||
method: "PUT",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
settings: {
|
||||
headers: [],
|
||||
metadata: [],
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
env: [],
|
||||
oauthOnInsufficientScope: "nope",
|
||||
},
|
||||
}),
|
||||
});
|
||||
expect(res.status).toBe(400);
|
||||
const body = (await res.json()) as { error?: string };
|
||||
expect(body.error).toMatch(/oauthOnInsufficientScope/);
|
||||
});
|
||||
|
||||
it("persists a valid settings.oauthOnInsufficientScope onto the stored config", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
JSON.stringify({
|
||||
mcpServers: {
|
||||
srv: { type: "streamable-http", url: "https://x.test/mcp" },
|
||||
},
|
||||
}),
|
||||
);
|
||||
const res = await fetch(`${h.baseUrl}/api/servers/srv`, {
|
||||
method: "PUT",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({
|
||||
settings: {
|
||||
headers: [],
|
||||
metadata: [],
|
||||
connectionTimeout: 0,
|
||||
requestTimeout: 0,
|
||||
env: [],
|
||||
oauthOnInsufficientScope: "throw",
|
||||
},
|
||||
}),
|
||||
});
|
||||
expect(res.status).toBe(200);
|
||||
const stored = JSON.parse(readFileSync(h.configPath, "utf-8")) as {
|
||||
mcpServers: Record<
|
||||
string,
|
||||
{ oauth?: { onInsufficientScope?: string } }
|
||||
>;
|
||||
};
|
||||
expect(stored.mcpServers.srv?.oauth?.onInsufficientScope).toBe("throw");
|
||||
});
|
||||
|
||||
it("ignores settings env/cwd for a non-stdio server (no leak onto config)", async () => {
|
||||
writeFileSync(
|
||||
h.configPath,
|
||||
|
|
|
|||
Loading…
Reference in a new issue