fix(oauth): record every redirect's scope, and what EMA really minted
Two more paths had the same defect, both found in review. `handleStandardAuthChallenge` returns an interactive redirect for `unauthorized` / `token_expired` / `invalid_token` as well, and its callback lands in the same completeOAuthFlow -- but the recording was gated on `insufficient_scope`. Those challenges compute their own `scopeForAuth`, which can differ from what storage holds, so a silent token response left the stale scope standing. The read is now one private helper both entry points call, applied to every redirect. And `scopeForMint` is not always what the EMA mint asks for: with neither config nor storage supplying a scope, discoverEmaResourceContext resolves it from the protected resource metadata's `scopes_supported`, which is what both exchanges send. `completeEmaIdpAuthorizationAndMint` now resolves that context itself -- passing it into the mint, so no extra discovery -- and returns it as `requestedScope` for the manager to use as the RFC 6749 5.1 fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
8be48eb742
commit
9a2616652c
4 changed files with 192 additions and 35 deletions
|
|
@ -378,7 +378,7 @@ describe("emaFlow", () => {
|
|||
// Seed code verifier / client info / metadata for the callback exchange.
|
||||
await startEmaIdpAuthorization(config);
|
||||
|
||||
const tokens = await completeEmaIdpAuthorizationAndMint(
|
||||
const { tokens } = await completeEmaIdpAuthorizationAndMint(
|
||||
config,
|
||||
"auth-code",
|
||||
IDP_ISSUER,
|
||||
|
|
@ -392,6 +392,43 @@ describe("emaFlow", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("completeEmaIdpAuthorizationAndMint reports a metadata-derived scope (#2117)", async () => {
|
||||
// With no configured scope, discoverEmaResourceContext resolves the
|
||||
// request from the protected resource metadata's `scopes_supported`. That
|
||||
// is what both exchanges send, so it is what an omitted `scope` in the
|
||||
// response implies was granted -- and the caller can only apply RFC 6749
|
||||
// 5.1 if the flow hands it back.
|
||||
const idToken = jwtWithExp(Math.floor(Date.now() / 1000) + 3600);
|
||||
const storage = createMemoryStorage();
|
||||
const inner = mockEmaFetch(idToken);
|
||||
const fetchFn = vi.fn(
|
||||
async (input: RequestInfo | URL, init?: RequestInit) => {
|
||||
if (String(input).includes("/.well-known/oauth-protected-resource")) {
|
||||
return new Response(
|
||||
JSON.stringify({
|
||||
resource: MCP_RESOURCE,
|
||||
authorization_servers: [AS_ISSUER],
|
||||
scopes_supported: ["mcp:read", "mcp:write"],
|
||||
}),
|
||||
);
|
||||
}
|
||||
return inner(input, init);
|
||||
},
|
||||
);
|
||||
const config = { ...baseConfig(storage), scope: undefined, fetchFn };
|
||||
|
||||
await startEmaIdpAuthorization(config);
|
||||
|
||||
const { tokens, requestedScope } = await completeEmaIdpAuthorizationAndMint(
|
||||
config,
|
||||
"auth-code",
|
||||
IDP_ISSUER,
|
||||
);
|
||||
|
||||
expect(tokens.access_token).toBe("resource-access-token");
|
||||
expect(requestedScope).toBe("mcp:read mcp:write");
|
||||
});
|
||||
|
||||
it("completeEmaIdpAuthorizationAndMint wraps leg 1 errors", async () => {
|
||||
const errorSpy = vi
|
||||
.spyOn(console, "error")
|
||||
|
|
|
|||
|
|
@ -798,6 +798,48 @@ describe("OAuthManager", () => {
|
|||
);
|
||||
});
|
||||
|
||||
it("records the request for a plain challenge redirect too, not just step-up", async () => {
|
||||
// A `token_expired` / `unauthorized` challenge can return an interactive
|
||||
// redirect whose callback lands in this same completeOAuthFlow. It used
|
||||
// to record nothing, so a silent token response left the old stored
|
||||
// scope standing — the same defect on a sibling entry point.
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
const params = createMockParams();
|
||||
storageOf(params).getScope.mockResolvedValue("stale:scope");
|
||||
storageOf(params).getClientInformation.mockResolvedValue({
|
||||
client_id: "cid",
|
||||
});
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({ scope: "catalog:scope" });
|
||||
const captureSpy = vi
|
||||
.spyOn(
|
||||
(await import("@inspector/core/auth/providers.js"))
|
||||
.BaseOAuthClientProvider.prototype,
|
||||
"getCapturedAuthUrl",
|
||||
)
|
||||
.mockReturnValue(capturedUrlWithScope("catalog:scope offline_access"));
|
||||
|
||||
const outcome = await manager.handleAuthChallenge({
|
||||
reason: "token_expired",
|
||||
});
|
||||
expect(outcome.kind).toBe("interactive");
|
||||
|
||||
mockedMcpAuth.mockResolvedValue("AUTHORIZED");
|
||||
storageOf(params).getTokens.mockResolvedValue({
|
||||
access_token: "access",
|
||||
token_type: "Bearer",
|
||||
});
|
||||
storageOf(params).saveScope.mockClear();
|
||||
|
||||
await manager.completeOAuthFlow("code");
|
||||
|
||||
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
"catalog:scope offline_access",
|
||||
);
|
||||
captureSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("persists nothing when neither the request nor the response names a scope", async () => {
|
||||
const params = await authorizeThenComplete(undefined, undefined);
|
||||
|
||||
|
|
@ -824,7 +866,7 @@ describe("OAuthManager", () => {
|
|||
|
||||
describe("completeOAuthFlow (EMA)", () => {
|
||||
it("mints resource tokens via the EMA path and dispatches complete", async () => {
|
||||
const tokens = { access_token: "EMA", token_type: "Bearer" };
|
||||
const tokens = { access_token: "EMA", token_type: "Bearer" as const };
|
||||
const params = createMockParams({
|
||||
enterpriseManagedAuth: {
|
||||
idp: {
|
||||
|
|
@ -839,7 +881,7 @@ describe("OAuthManager", () => {
|
|||
|
||||
const mintSpy = vi
|
||||
.spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
|
||||
.mockResolvedValue(tokens);
|
||||
.mockResolvedValue({ tokens });
|
||||
|
||||
await manager.completeOAuthFlow("ema-code");
|
||||
|
||||
|
|
@ -866,7 +908,10 @@ describe("OAuthManager", () => {
|
|||
|
||||
const mintSpy = vi
|
||||
.spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
|
||||
.mockResolvedValue({ access_token: "EMA", token_type: "Bearer" });
|
||||
.mockResolvedValue({
|
||||
tokens: { access_token: "EMA", token_type: "Bearer" },
|
||||
requestedScope: "mcp weather:read",
|
||||
});
|
||||
storageOf(params).saveScope.mockClear();
|
||||
|
||||
await manager.completeOAuthFlow("ema-code");
|
||||
|
|
@ -877,6 +922,42 @@ describe("OAuthManager", () => {
|
|||
);
|
||||
mintSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("persists a mint scope that came from resource metadata, not config (#2117)", async () => {
|
||||
// With neither a configured nor a stored scope, discoverEmaResourceContext
|
||||
// resolves the request from the protected resource metadata's
|
||||
// `scopes_supported`. That value never reaches the manager's own
|
||||
// `scopeForMint`, so only what the mint reports can be persisted.
|
||||
const params = createMockParams({
|
||||
enterpriseManagedAuth: {
|
||||
idp: {
|
||||
issuer: "https://idp.example.com",
|
||||
clientId: "app-client",
|
||||
clientSecret: "secret",
|
||||
},
|
||||
},
|
||||
});
|
||||
params.initialConfig.scope = undefined;
|
||||
storageOf(params).getScope.mockResolvedValue(undefined);
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({ enterpriseManaged: true });
|
||||
|
||||
const mintSpy = vi
|
||||
.spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
|
||||
.mockResolvedValue({
|
||||
tokens: { access_token: "EMA", token_type: "Bearer" },
|
||||
requestedScope: "mcp:read mcp:write",
|
||||
});
|
||||
storageOf(params).saveScope.mockClear();
|
||||
|
||||
await manager.completeOAuthFlow("ema-code");
|
||||
|
||||
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp:read mcp:write",
|
||||
);
|
||||
mintSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe("trySilentEnterpriseManagedAuth", () => {
|
||||
|
|
@ -1704,7 +1785,10 @@ describe("OAuthManager", () => {
|
|||
.mockResolvedValue(authUrl);
|
||||
const mintSpy = vi
|
||||
.spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
|
||||
.mockResolvedValue({ access_token: "tok", token_type: "Bearer" });
|
||||
.mockResolvedValue({
|
||||
tokens: { access_token: "tok", token_type: "Bearer" },
|
||||
requestedScope: "mcp tools:read weather:read",
|
||||
});
|
||||
const params = createMockParams({
|
||||
enterpriseManagedAuth: {
|
||||
idp: {
|
||||
|
|
|
|||
|
|
@ -124,11 +124,26 @@ export async function startEmaIdpAuthorization(
|
|||
return authorizationUrl;
|
||||
}
|
||||
|
||||
/**
|
||||
* The outcome of a completed EMA authorization.
|
||||
*
|
||||
* `requestedScope` is the scope the mint actually asked for, which is not
|
||||
* always `config.scope`: when neither the server config nor storage supplies
|
||||
* one, {@link discoverEmaResourceContext} resolves it from the protected
|
||||
* resource metadata's `scopes_supported`. Callers need that resolved value to
|
||||
* apply RFC 6749 §5.1 — a token response that omits `scope` granted exactly
|
||||
* what was requested — so it is returned rather than left inside the flow.
|
||||
*/
|
||||
export interface EmaAuthorizationResult {
|
||||
tokens: OAuthTokens;
|
||||
requestedScope?: string;
|
||||
}
|
||||
|
||||
export async function completeEmaIdpAuthorizationAndMint(
|
||||
config: EmaFlowConfig,
|
||||
authorizationCode: string,
|
||||
iss?: string,
|
||||
): Promise<OAuthTokens> {
|
||||
): Promise<EmaAuthorizationResult> {
|
||||
try {
|
||||
await completeIdpOidcAuthorization({
|
||||
idp: config.idp,
|
||||
|
|
@ -146,8 +161,17 @@ export async function completeEmaIdpAuthorizationAndMint(
|
|||
}
|
||||
|
||||
let tokens: OAuthTokens;
|
||||
// Resolve the resource context here rather than inside the mint, so the
|
||||
// scope it settles on is visible to the caller. It is passed straight
|
||||
// through, so this costs no extra discovery.
|
||||
let resourceContext: EmaResourceContext;
|
||||
try {
|
||||
tokens = await mintEmaResourceTokens(config);
|
||||
resourceContext = await discoverEmaResourceContext(
|
||||
config.serverUrl,
|
||||
config.scope,
|
||||
config.fetchFn,
|
||||
);
|
||||
tokens = await mintEmaResourceTokens(config, resourceContext);
|
||||
} catch (err) {
|
||||
throw wrapEmaMintError(err);
|
||||
}
|
||||
|
|
@ -155,7 +179,7 @@ export async function completeEmaIdpAuthorizationAndMint(
|
|||
await config.storage.saveTokens(config.serverUrl, tokens, {
|
||||
enterpriseManaged: true,
|
||||
});
|
||||
return tokens;
|
||||
return { tokens, requestedScope: resourceContext.scope };
|
||||
}
|
||||
|
||||
/** Re-run legs 2–3 on 401 when IdP session is still valid. */
|
||||
|
|
|
|||
|
|
@ -324,20 +324,7 @@ export class OAuthManager {
|
|||
throw new Error("Failed to capture authorization URL");
|
||||
}
|
||||
|
||||
// RFC 6749 §5.1: an omitted `scope` in the token response means the grant
|
||||
// is identical to what was requested. Carry the request across the
|
||||
// redirect so completeOAuthFlow has that fallback to persist (#2117) —
|
||||
// the step-up paths already do this, and without it an ordinary grant
|
||||
// whose AS echoes no scope leaves the previous stored scope standing as
|
||||
// if it were still what the current token carries.
|
||||
//
|
||||
// Read it off the authorize URL rather than from `provider.scope`: the
|
||||
// SDK augments the request with `offline_access` when the AS advertises
|
||||
// it and the client asks for a refresh token, so the provider's scope can
|
||||
// be a strict subset of what was actually asked for — and persisting the
|
||||
// subset would understate the grant in exactly the way this fixes.
|
||||
this.pendingAuthorizationScope =
|
||||
capturedUrl.searchParams.get("scope")?.trim() || requestedScope;
|
||||
this.recordAuthorizationRequestScope(capturedUrl, requestedScope);
|
||||
|
||||
const stateParam = capturedUrl.searchParams.get("state");
|
||||
if (stateParam && this.params.onBeforeOAuthRedirect) {
|
||||
|
|
@ -357,6 +344,27 @@ export class OAuthManager {
|
|||
return capturedUrl;
|
||||
}
|
||||
|
||||
/**
|
||||
* Remember what an interactive authorization asked for, so the callback can
|
||||
* apply RFC 6749 §5.1 — a token response that omits `scope` granted exactly
|
||||
* the requested scope (#2117). Without it, an ordinary grant whose AS echoes
|
||||
* no scope leaves the previous stored scope standing as though it were what
|
||||
* the current token carries.
|
||||
*
|
||||
* The authorize URL is the authority, not the scope handed to `auth()`: the
|
||||
* SDK augments the request with `offline_access` when the AS advertises it
|
||||
* and the client asks for a refresh token, so the input can be a strict
|
||||
* subset of what was actually requested. The fallback covers an authorize
|
||||
* URL that carries no `scope` parameter at all.
|
||||
*/
|
||||
private recordAuthorizationRequestScope(
|
||||
authorizationUrl: URL,
|
||||
requestedScope: string | undefined,
|
||||
): void {
|
||||
this.pendingAuthorizationScope =
|
||||
authorizationUrl.searchParams.get("scope")?.trim() || requestedScope;
|
||||
}
|
||||
|
||||
async completeOAuthFlow(
|
||||
authorizationCode: string,
|
||||
iss?: string,
|
||||
|
|
@ -368,18 +376,19 @@ export class OAuthManager {
|
|||
const config = scopeForMint
|
||||
? { ...emaConfig, scope: scopeForMint }
|
||||
: emaConfig;
|
||||
const tokens = await completeEmaIdpAuthorizationAndMint(
|
||||
config,
|
||||
authorizationCode,
|
||||
iss,
|
||||
);
|
||||
// `scopeForMint` — not `pendingAuthorizationScope` — is what the
|
||||
// mint actually asked for, so it is the RFC 6749 §5.1 fallback here
|
||||
// (the two differ on the ordinary leg, where only the config carries
|
||||
// a scope).
|
||||
const { tokens, requestedScope } =
|
||||
await completeEmaIdpAuthorizationAndMint(
|
||||
config,
|
||||
authorizationCode,
|
||||
iss,
|
||||
);
|
||||
// The mint reports what it actually asked for, which is neither
|
||||
// `pendingAuthorizationScope` (undefined on the ordinary leg) nor
|
||||
// always `scopeForMint` — with no configured or stored scope the
|
||||
// resource metadata's `scopes_supported` is what gets requested.
|
||||
const scopeToPersist = resolvePersistedScopeAfterGrant(
|
||||
tokens.scope,
|
||||
scopeForMint,
|
||||
requestedScope,
|
||||
);
|
||||
if (scopeToPersist) {
|
||||
await this.requireStorage().saveScope(
|
||||
|
|
@ -859,9 +868,12 @@ export class OAuthManager {
|
|||
};
|
||||
}
|
||||
|
||||
if (enriched.reason === "insufficient_scope" && scopeForAuth) {
|
||||
this.pendingAuthorizationScope = scopeForAuth;
|
||||
}
|
||||
// Every interactive redirect from here lands back in completeOAuthFlow,
|
||||
// so every one of them needs its request recorded — not just the step-up
|
||||
// (#2117). A plain `unauthorized` / `token_expired` challenge can request
|
||||
// a scope that differs from what storage holds, and used to persist
|
||||
// nothing when the AS answered without a `scope`.
|
||||
this.recordAuthorizationRequestScope(capturedUrl, scopeForAuth);
|
||||
|
||||
const clientInfo = await provider.clientInformation();
|
||||
await this.recordAuthorizationCodeFlowState(
|
||||
|
|
|
|||
Loading…
Reference in a new issue