fix(oauth): persist the scope the authorize URL carried
`provider.scope` is not what the authorization request asked for. The SDK augments it with `offline_access` when the AS advertises that scope and the client wants a refresh token, so persisting the provider's copy after a silent token response understates the RFC 6749 5.1 implied grant -- the same defect this change exists to fix, one notch smaller. Read the scope off the captured authorize URL instead, falling back to `provider.scope` when the URL carries none. 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
db23bc9b97
commit
8be48eb742
2 changed files with 38 additions and 9 deletions
|
|
@ -737,13 +737,18 @@ describe("OAuthManager", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("persisting the requested scope after an ordinary grant (#2117)", () => {
|
describe("persisting the requested scope after an ordinary grant (#2117)", () => {
|
||||||
const CAPTURED_URL = new URL(
|
function capturedUrlWithScope(scope?: string): URL {
|
||||||
"https://auth.example.com/authorize?state=abc",
|
const url = new URL("https://auth.example.com/authorize?state=abc");
|
||||||
);
|
if (scope !== undefined) {
|
||||||
|
url.searchParams.set("scope", scope);
|
||||||
|
}
|
||||||
|
return url;
|
||||||
|
}
|
||||||
|
|
||||||
async function authorizeThenComplete(
|
async function authorizeThenComplete(
|
||||||
storedScope: string | undefined,
|
storedScope: string | undefined,
|
||||||
tokenScope: string | undefined,
|
tokenScope: string | undefined,
|
||||||
|
authorizeUrlScope?: string,
|
||||||
) {
|
) {
|
||||||
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
mockedMcpAuth.mockResolvedValue("REDIRECT");
|
||||||
const params = createMockParams();
|
const params = createMockParams();
|
||||||
|
|
@ -758,7 +763,7 @@ describe("OAuthManager", () => {
|
||||||
.BaseOAuthClientProvider.prototype,
|
.BaseOAuthClientProvider.prototype,
|
||||||
"getCapturedAuthUrl",
|
"getCapturedAuthUrl",
|
||||||
)
|
)
|
||||||
.mockReturnValue(CAPTURED_URL);
|
.mockReturnValue(capturedUrlWithScope(authorizeUrlScope));
|
||||||
|
|
||||||
await manager.authenticate();
|
await manager.authenticate();
|
||||||
|
|
||||||
|
|
@ -798,6 +803,23 @@ describe("OAuthManager", () => {
|
||||||
|
|
||||||
expect(storageOf(params).saveScope).not.toHaveBeenCalled();
|
expect(storageOf(params).saveScope).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("persists the scope the authorize URL actually carried, not the provider's", async () => {
|
||||||
|
// The SDK augments the request with `offline_access` when the AS
|
||||||
|
// advertises it and the client wants a refresh token, so the authorize
|
||||||
|
// URL is a superset of `provider.scope`. Persisting the provider's copy
|
||||||
|
// would understate the grant the AS implied by staying silent.
|
||||||
|
const params = await authorizeThenComplete(
|
||||||
|
"mcp",
|
||||||
|
undefined,
|
||||||
|
"mcp offline_access",
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(storageOf(params).saveScope).toHaveBeenCalledWith(
|
||||||
|
SERVER_URL,
|
||||||
|
"mcp offline_access",
|
||||||
|
);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("completeOAuthFlow (EMA)", () => {
|
describe("completeOAuthFlow (EMA)", () => {
|
||||||
|
|
|
||||||
|
|
@ -324,13 +324,20 @@ export class OAuthManager {
|
||||||
throw new Error("Failed to capture authorization URL");
|
throw new Error("Failed to capture authorization URL");
|
||||||
}
|
}
|
||||||
|
|
||||||
// RFC 6749 5.1: an omitted `scope` in the token response means the grant
|
// 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
|
// is identical to what was requested. Carry the request across the
|
||||||
// redirect so completeOAuthFlow has that fallback to persist (#2117) --
|
// redirect so completeOAuthFlow has that fallback to persist (#2117) —
|
||||||
// the step-up paths already do this, and without it an ordinary grant
|
// 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
|
// whose AS echoes no scope leaves the previous stored scope standing as
|
||||||
// if it were still what the current token carries.
|
// if it were still what the current token carries.
|
||||||
this.pendingAuthorizationScope = requestedScope;
|
//
|
||||||
|
// 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;
|
||||||
|
|
||||||
const stateParam = capturedUrl.searchParams.get("state");
|
const stateParam = capturedUrl.searchParams.get("state");
|
||||||
if (stateParam && this.params.onBeforeOAuthRedirect) {
|
if (stateParam && this.params.onBeforeOAuthRedirect) {
|
||||||
|
|
@ -366,8 +373,8 @@ export class OAuthManager {
|
||||||
authorizationCode,
|
authorizationCode,
|
||||||
iss,
|
iss,
|
||||||
);
|
);
|
||||||
// `scopeForMint` -- not `pendingAuthorizationScope` -- is what the
|
// `scopeForMint` — not `pendingAuthorizationScope` — is what the
|
||||||
// mint actually asked for, so it is the RFC 6749 5.1 fallback here
|
// 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
|
// (the two differ on the ordinary leg, where only the config carries
|
||||||
// a scope).
|
// a scope).
|
||||||
const scopeToPersist = resolvePersistedScopeAfterGrant(
|
const scopeToPersist = resolvePersistedScopeAfterGrant(
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue