Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 39 additions & 6 deletions packages/client/src/client/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,38 @@
}
}

/**
* Selects scopes per the MCP spec and augment for refresh token support.
*/
export function determineScope(options: {
requestedScope?: string;
resourceMetadata?: OAuthProtectedResourceMetadata;
authServerMetadata?: AuthorizationServerMetadata;
clientMetadata: OAuthClientMetadata;
}): string | undefined {
const { requestedScope, resourceMetadata, authServerMetadata, clientMetadata } = options;

// Scope selection priority (MCP spec):
// 1. WWW-Authenticate header scope
// 2. PRM scopes_supported
// 3. clientMetadata.scope (SDK fallback)
// 4. Omit scope parameter
let effectiveScope = requestedScope || resourceMetadata?.scopes_supported?.join(' ') || clientMetadata.scope;

// SEP-2207: Append offline_access when the AS advertises it
// and the client supports the refresh_token grant.
if (
effectiveScope &&
authServerMetadata?.scopes_supported?.includes('offline_access') &&
!effectiveScope.split(' ').includes('offline_access') &&

Check notice on line 493 in packages/client/src/client/auth.ts

View check run for this annotation

Claude / Claude Code Review

Inconsistent offline_access check: substring vs exact token match

Nit (pre-existing): `startAuthorization()` at line 1259 uses `scope?.includes('offline_access')` (substring match on the string), while the new `determineScope()` correctly uses `.split(' ').includes('offline_access')` (exact token match). A hypothetical scope like `"no_offline_access"` would incorrectly trigger `prompt=consent`. Consider updating line 1259 to `scope?.split(' ').includes('offline_access')` for consistency.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Nit (pre-existing): startAuthorization() at line 1259 uses scope?.includes('offline_access') (substring match on the string), while the new determineScope() correctly uses .split(' ').includes('offline_access') (exact token match). A hypothetical scope like "no_offline_access" would incorrectly trigger prompt=consent. Consider updating line 1259 to scope?.split(' ').includes('offline_access') for consistency.

Extended reasoning...

Bug Analysis

The new determineScope() function (line 493) correctly checks whether offline_access is present in a space-delimited scope string using exact token matching:

\!effectiveScope.split(' ').includes('offline_access')

However, the consumer of this scope — startAuthorization() at line 1259 — checks for the same scope token using String.prototype.includes(), which is a substring match:

if (scope?.includes('offline_access')) {
    authorizationUrl.searchParams.append('prompt', 'consent');
}

How it manifests

Consider a scope string like "mcp:read no_offline_access". The determineScope() function would correctly determine that offline_access is not present as a discrete scope token. But when this scope string reaches startAuthorization(), the substring check "mcp:read no_offline_access".includes("offline_access") returns true, causing prompt=consent to be incorrectly appended to the authorization URL.

Step-by-step proof

  1. determineScope() returns "mcp:read no_offline_access" (no augmentation, since offline_access is not a discrete token).
  2. authInternal() passes this as scope: resolvedScope to startAuthorization().
  3. Inside startAuthorization(), line 1259 evaluates: "mcp:read no_offline_access"?.includes("offline_access")true.
  4. prompt=consent is appended to the authorization URL — incorrectly, since offline_access was never actually requested.

Why existing code does not prevent it

The determineScope() function uses the correct exact-token approach, but it cannot control how downstream consumers check the scope string. The startAuthorization() function predates this PR and was not updated.

Impact

This is a pre-existing issue — the substring check in startAuthorization() was there before this PR. However, this PR makes offline_access appear in scope strings more frequently via the SEP-2207 augmentation logic, which means the code path at line 1259 gets exercised more often. In practice, a scope token containing "offline_access" as a substring is extremely unlikely in real OAuth deployments, so the practical impact is negligible.

Fix

Replace the substring check on line 1259 with an exact token match:

if (scope?.split(' ').includes('offline_access')) {

This is a one-line change that aligns startAuthorization() with the correct pattern already used in determineScope().

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixweinberger I'm happy to update this as well if you like, but we don't need to increase the scope here unnecessarily.

clientMetadata.grant_types?.includes('refresh_token')
) {
effectiveScope = `${effectiveScope} offline_access`;
}

return effectiveScope;
}

async function authInternal(
provider: OAuthClientProvider,
{
Expand Down Expand Up @@ -555,15 +587,16 @@
await provider.saveResourceUrl?.(String(resource));
}

// Apply scope selection strategy (SEP-835):
// 1. WWW-Authenticate scope (passed via `scope` param)
// 2. PRM scopes_supported
// 3. Client metadata scope (user-configured fallback)
// The resolved scope is used consistently for both DCR and the authorization request.
const resolvedScope = scope || resourceMetadata?.scopes_supported?.join(' ') || provider.clientMetadata.scope;
// Scope selection used consistently for DCR and the authorization request.
const resolvedScope = determineScope({
requestedScope: scope,
resourceMetadata,
authServerMetadata: metadata,
clientMetadata: provider.clientMetadata
});

// Handle client registration if needed
let clientInformation = await Promise.resolve(provider.clientInformation());

Check warning on line 599 in packages/client/src/client/auth.ts

View check run for this annotation

Claude / Claude Code Review

fetchToken bypasses determineScope for non-interactive flows

Nit: `fetchToken()` (line 650) receives the raw `scope` parameter instead of `resolvedScope`, so non-interactive flows using `prepareTokenRequest()` miss the PRM `scopes_supported` fallback and the `offline_access` augmentation from `determineScope()`. In practice the impact is minimal — authorization_code flows don't send scope in the token request at all, and `client_credentials` + refresh tokens is not a valid combination per RFC 6749 §4.4.3 — but passing `resolvedScope` here would be more co
Comment on lines +590 to 599
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: fetchToken() (line 650) receives the raw scope parameter instead of resolvedScope, so non-interactive flows using prepareTokenRequest() miss the PRM scopes_supported fallback and the offline_access augmentation from determineScope(). In practice the impact is minimal — authorization_code flows don't send scope in the token request at all, and client_credentials + refresh tokens is not a valid combination per RFC 6749 §4.4.3 — but passing resolvedScope here would be more consistent with the stated goal of centralizing scope selection.

Extended reasoning...

What the bug is

In authInternal(), determineScope() computes resolvedScope (line 590-596) with a three-level fallback (WWW-Authenticate scope → PRM scopes_supportedclientMetadata.scope) plus SEP-2207 offline_access augmentation. This resolvedScope is correctly passed to registerClient() (line 631) and startAuthorization() (line 688). However, at line 650, fetchToken() receives the raw scope parameter — the original value from auth() options — not resolvedScope.

How fetchToken handles scope differently

Inside fetchToken() (line 1503), scope resolution follows a completely different chain: const effectiveScope = scope ?? provider.clientMetadata.scope. This bypasses both the PRM scopes_supported fallback (priority 2 in the MCP spec) and the offline_access augmentation added by this PR. The effectiveScope is then passed to provider.prepareTokenRequest(effectiveScope) for non-interactive flows.

Step-by-step proof for non-interactive flow

Consider a client_credentials provider where: (1) no explicit scope is passed to auth(), (2) PRM metadata has scopes_supported: ["mcp:read", "mcp:write"], (3) AS metadata has scopes_supported including offline_access, and (4) clientMetadata.grant_types includes refresh_token but clientMetadata.scope is undefined.

  • determineScope() returns "mcp:read mcp:write offline_access" (PRM fallback + augmentation)
  • DCR registration uses this resolved scope ✓
  • fetchToken() receives scope = undefined, computes effectiveScope = undefined ?? undefined = undefined
  • prepareTokenRequest(undefined) gets no scope at all, vs the intended "mcp:read mcp:write offline_access"

Why the practical impact is minimal

For authorization_code flows (the primary target of this PR), prepareAuthorizationCodeRequest() only sends grant_type, code, code_verifier, and redirect_uri — scope is not included in the token exchange request because it was already baked into the authorization URL via startAuthorization(resolvedScope). So this bug has zero impact on auth code flows.

For client_credentials flows, RFC 6749 §4.4.3 states the AS "MUST NOT issue a refresh token," making offline_access semantically meaningless. The PRM fallback gap is more relevant here, but providers implementing prepareTokenRequest() have full control over what scope they send.

Suggested fix

Pass resolvedScope instead of scope at line 650 for consistency with the PR's goal of centralizing scope selection via determineScope(). This is a one-line change: scoperesolvedScope in the fetchToken() call.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixweinberger happy to update this if you want. Was trying to be surgical, but the review is probably accurate. lmk

if (!clientInformation) {
if (authorizationCode !== undefined) {
throw new Error('Existing OAuth client information is required when exchanging an authorization code');
Expand Down
224 changes: 224 additions & 0 deletions packages/client/test/client/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { OAuthClientProvider } from '../../src/client/auth.js';
import {
auth,
buildDiscoveryUrls,
determineScope,
discoverAuthorizationServerMetadata,
discoverOAuthMetadata,
discoverOAuthProtectedResourceMetadata,
Expand Down Expand Up @@ -3668,4 +3669,227 @@ describe('OAuth Authorization', () => {
});
});
});

describe('determineScope', () => {
const baseClientMetadata = {
redirect_uris: ['http://localhost:3000/callback'],
client_name: 'Test Client'
};

describe('MCP Scope Selection Strategy', () => {
it('returns explicit requestedScope as-is (priority 1)', () => {
const result = determineScope({
requestedScope: 'files:read',
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
clientMetadata: {
...baseClientMetadata,
scope: 'fallback:scope'
}
});

expect(result).toBe('files:read');
});

it('uses PRM scopes_supported when no explicit scope (priority 2)', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write', 'mcp:admin']
},
clientMetadata: {
...baseClientMetadata,
scope: 'fallback:scope'
}
});

expect(result).toBe('mcp:read mcp:write mcp:admin');
});

it('falls back to clientMetadata.scope when no PRM scopes (priority 3)', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/'
},
clientMetadata: {
...baseClientMetadata,
scope: 'client:default'
}
});

expect(result).toBe('client:default');
});

it('returns undefined when no scope source available (priority 4)', () => {
const result = determineScope({
clientMetadata: baseClientMetadata
});

expect(result).toBeUndefined();
});

it('returns undefined when PRM has no scopes_supported and clientMetadata has no scope', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/'
},
clientMetadata: baseClientMetadata
});

expect(result).toBeUndefined();
});
});

describe('SEP-2207: offline_access scope augmentation', () => {
const asMetadataWithOfflineAccess = {
issuer: 'https://auth.example.com',
authorization_endpoint: 'https://auth.example.com/authorize',
token_endpoint: 'https://auth.example.com/token',
response_types_supported: ['code'] as string[],
scopes_supported: ['openid', 'profile', 'offline_access']
};

const asMetadataWithoutOfflineAccess = {
issuer: 'https://auth.example.com',
authorization_endpoint: 'https://auth.example.com/authorize',
token_endpoint: 'https://auth.example.com/token',
response_types_supported: ['code'] as string[],
scopes_supported: ['openid', 'profile']
};

const clientMetadataWithRefreshToken = {
...baseClientMetadata,
grant_types: ['authorization_code', 'refresh_token']
};

it('augments explicit scope with offline_access', () => {
const result = determineScope({
requestedScope: 'mcp:read mcp:write',
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBe('mcp:read mcp:write offline_access');
});

it('adds offline_access when AS supports it and client grant_types includes refresh_token', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBe('mcp:read mcp:write offline_access');
});

it('adds offline_access when using clientMetadata.scope fallback', () => {
const result = determineScope({
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: {
...clientMetadataWithRefreshToken,
scope: 'mcp:tools'
}
});

expect(result).toBe('mcp:tools offline_access');
});

it('does NOT augment when no other scopes are present', () => {
const result = determineScope({
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBeUndefined();
});

it('does NOT augment when AS metadata lacks offline_access', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
authServerMetadata: asMetadataWithoutOfflineAccess,
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBe('mcp:read mcp:write');
});

it('does NOT augment when AS metadata is undefined', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBe('mcp:read mcp:write');
});

it('does NOT augment when offline_access already in clientMetadata.scope', () => {
const result = determineScope({
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: {
...clientMetadataWithRefreshToken,
scope: 'mcp:tools offline_access'
}
});

expect(result).toBe('mcp:tools offline_access');
});

it('does NOT augment when non-compliant PRM already includes offline_access', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'offline_access', 'mcp:write']
},
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: clientMetadataWithRefreshToken
});

expect(result).toBe('mcp:read offline_access mcp:write');
});

it('does NOT augment when grant_types omits refresh_token', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: {
...baseClientMetadata,
grant_types: ['authorization_code']
}
});

expect(result).toBe('mcp:read mcp:write');
});

it('does NOT augment when grant_types is undefined (respects OAuth defaults)', () => {
const result = determineScope({
resourceMetadata: {
resource: 'https://api.example.com/',
scopes_supported: ['mcp:read', 'mcp:write']
},
authServerMetadata: asMetadataWithOfflineAccess,
clientMetadata: baseClientMetadata
});

expect(result).toBe('mcp:read mcp:write');
});
});
});
});
Loading