fix(oauth): persist the requested scope when the grant omits it
RFC 6749 5.1 defines an omitted `scope` in a token response as a grant identical to what was requested, and `resolvePersistedScopeAfterGrant` already encodes that rule -- but the ordinary authorization leg never gave it a `requestedScope` to fall back to, so nothing was persisted and the previous stored scope kept being reported as what the current token carries. `authenticate()` now records `provider.scope` as the pending authorization scope once the authorization URL has been captured, so it survives the redirect and reaches `completeOAuthFlow`. The enterprise-managed leg had the same defect for the same reason and is fixed one level up: it now falls back to `scopeForMint`, which is what the mint actually asked for, rather than to the raw `pendingAuthorizationScope` that is undefined on this leg. A response that does carry `scope` still wins over the request, on both legs. 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
2b129087a3
commit
db23bc9b97
2 changed files with 108 additions and 3 deletions
|
|
@ -736,6 +736,70 @@ describe("OAuthManager", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("persisting the requested scope after an ordinary grant (#2117)", () => {
|
||||
const CAPTURED_URL = new URL(
|
||||
"https://auth.example.com/authorize?state=abc",
|
||||
);
|
||||
|
||||
async function authorizeThenComplete(
|
||||
storedScope: string | undefined,
|
||||
tokenScope: string | undefined,
|
||||
) {
|
||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||
const params = createMockParams();
|
||||
storageOf(params).getScope.mockResolvedValue(storedScope);
|
||||
storageOf(params).getClientInformation.mockResolvedValue({
|
||||
client_id: "cid",
|
||||
});
|
||||
const manager = new OAuthManager(params);
|
||||
const captureSpy = vi
|
||||
.spyOn(
|
||||
(await import("@inspector/core/auth/providers.js"))
|
||||
.BaseOAuthClientProvider.prototype,
|
||||
"getCapturedAuthUrl",
|
||||
)
|
||||
.mockReturnValue(CAPTURED_URL);
|
||||
|
||||
await manager.authenticate();
|
||||
|
||||
mockedMcpAuth.mockResolvedValue("AUTHORIZED");
|
||||
storageOf(params).getTokens.mockResolvedValue({
|
||||
access_token: "access",
|
||||
token_type: "Bearer",
|
||||
...(tokenScope === undefined ? {} : { scope: tokenScope }),
|
||||
});
|
||||
storageOf(params).saveScope.mockClear();
|
||||
|
||||
await manager.completeOAuthFlow("code");
|
||||
captureSpy.mockRestore();
|
||||
return params;
|
||||
}
|
||||
|
||||
it("persists the requested scope when the token response omits scope", async () => {
|
||||
const params = await authorizeThenComplete("mcp weather:read", undefined);
|
||||
|
||||
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp weather:read",
|
||||
);
|
||||
});
|
||||
|
||||
it("still prefers the granted scope when the AS echoes one", async () => {
|
||||
const params = await authorizeThenComplete("mcp weather:read", "mcp");
|
||||
|
||||
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp",
|
||||
);
|
||||
});
|
||||
|
||||
it("persists nothing when neither the request nor the response names a scope", async () => {
|
||||
const params = await authorizeThenComplete(undefined, undefined);
|
||||
|
||||
expect(storageOf(params).saveScope).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("completeOAuthFlow (EMA)", () => {
|
||||
it("mints resource tokens via the EMA path and dispatches complete", async () => {
|
||||
const tokens = { access_token: "EMA", token_type: "Bearer" };
|
||||
|
|
@ -762,6 +826,35 @@ describe("OAuthManager", () => {
|
|||
expect(manager.getOAuthFlowStep()).toBe("complete");
|
||||
mintSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("persists the minted scope request when the mint response omits scope (#2117)", async () => {
|
||||
const params = createMockParams({
|
||||
enterpriseManagedAuth: {
|
||||
idp: {
|
||||
issuer: "https://idp.example.com",
|
||||
clientId: "app-client",
|
||||
clientSecret: "secret",
|
||||
},
|
||||
},
|
||||
});
|
||||
params.initialConfig.scope = "mcp weather:read";
|
||||
storageOf(params).getScope.mockResolvedValue(undefined);
|
||||
const manager = new OAuthManager(params);
|
||||
manager.setOAuthConfig({ enterpriseManaged: true });
|
||||
|
||||
const mintSpy = vi
|
||||
.spyOn(emaFlow, "completeEmaIdpAuthorizationAndMint")
|
||||
.mockResolvedValue({ access_token: "EMA", token_type: "Bearer" });
|
||||
storageOf(params).saveScope.mockClear();
|
||||
|
||||
await manager.completeOAuthFlow("ema-code");
|
||||
|
||||
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||
SERVER_URL,
|
||||
"mcp weather:read",
|
||||
);
|
||||
mintSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe("trySilentEnterpriseManagedAuth", () => {
|
||||
|
|
|
|||
|
|
@ -305,9 +305,10 @@ export class OAuthManager {
|
|||
resourceMetadataUrl,
|
||||
});
|
||||
|
||||
const requestedScope = provider.scope;
|
||||
const result = await mcpAuth(provider, {
|
||||
serverUrl,
|
||||
scope: provider.scope,
|
||||
scope: requestedScope,
|
||||
fetchFn: this.params.effectiveAuthFetch,
|
||||
resourceMetadataUrl,
|
||||
});
|
||||
|
|
@ -323,6 +324,14 @@ 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.
|
||||
this.pendingAuthorizationScope = requestedScope;
|
||||
|
||||
const stateParam = capturedUrl.searchParams.get("state");
|
||||
if (stateParam && this.params.onBeforeOAuthRedirect) {
|
||||
const parsedState = parseOAuthState(stateParam);
|
||||
|
|
@ -357,10 +366,13 @@ export class OAuthManager {
|
|||
authorizationCode,
|
||||
iss,
|
||||
);
|
||||
const requestedScope = this.pendingAuthorizationScope;
|
||||
// `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 scopeToPersist = resolvePersistedScopeAfterGrant(
|
||||
tokens.scope,
|
||||
requestedScope,
|
||||
scopeForMint,
|
||||
);
|
||||
if (scopeToPersist) {
|
||||
await this.requireStorage().saveScope(
|
||||
|
|
|
|||
Loading…
Reference in a new issue