fix: preserve exact OAuth resource indicators (#2581)
Co-authored-by: Konstantin Konstantinov <konstantin@mach5technology.com>
This commit is contained in:
parent
6fa42279fe
commit
5119ee7fd7
2 changed files with 120 additions and 11 deletions
|
|
@ -1243,11 +1243,17 @@ async function authInternal(
|
|||
await provider.saveDiscoveryState?.(freshDiscoveryState);
|
||||
}
|
||||
|
||||
const resource: URL | undefined = await selectResourceURL(serverUrl, provider, resourceMetadata);
|
||||
// Send the metadata's resource indicator verbatim: `selectResourceURL` returns a parsed
|
||||
// `URL`, and `URL.href` appends "/" to a pathless indicator such as `https://example.com`,
|
||||
// which exact-match authorization servers reject (#1968). A URL returned by the
|
||||
// provider's own `validateResourceURL` is used as returned.
|
||||
const selectedResource = await selectResourceURL(serverUrl, provider, resourceMetadata);
|
||||
const resource: string | URL | undefined =
|
||||
selectedResource && resourceMetadata && !provider.validateResourceURL ? resourceMetadata.resource : selectedResource;
|
||||
|
||||
// Save resource URL for providers that need it (e.g., CrossAppAccessProvider)
|
||||
if (resource) {
|
||||
await provider.saveResourceUrl?.(String(resource));
|
||||
await provider.saveResourceUrl?.(resourceIndicatorToString(resource));
|
||||
}
|
||||
|
||||
// Scope selection used consistently for DCR and the authorization request.
|
||||
|
|
@ -1460,6 +1466,17 @@ export function isHttpsUrl(value?: string): boolean {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Selects the RFC 8707 resource indicator for an MCP server: the provider's
|
||||
* {@linkcode OAuthClientProvider.validateResourceURL | validateResourceURL} result when
|
||||
* implemented, otherwise the protected resource metadata's `resource` (checked against the
|
||||
* server URL with `checkResourceAllowed`), or `undefined` when there is no metadata.
|
||||
*
|
||||
* The result is a parsed `URL`, so a pathless indicator such as `https://example.com` has
|
||||
* the `href` `https://example.com/`. {@linkcode auth} therefore sends the metadata string
|
||||
* verbatim instead of this URL's `href` (#1968); callers that emit the `resource`
|
||||
* parameter themselves should do the same.
|
||||
*/
|
||||
export async function selectResourceURL(
|
||||
serverUrl: string | URL,
|
||||
provider: OAuthClientProvider,
|
||||
|
|
@ -2032,6 +2049,10 @@ export async function discoverOAuthServerInfo(
|
|||
};
|
||||
}
|
||||
|
||||
function resourceIndicatorToString(resource: string | URL): string {
|
||||
return typeof resource === 'string' ? resource : resource.href;
|
||||
}
|
||||
|
||||
/**
|
||||
* Begins the authorization flow with the given server, by generating a PKCE challenge and constructing the authorization URL.
|
||||
*/
|
||||
|
|
@ -2050,7 +2071,7 @@ export async function startAuthorization(
|
|||
redirectUrl: string | URL;
|
||||
scope?: string;
|
||||
state?: string;
|
||||
resource?: URL;
|
||||
resource?: string | URL;
|
||||
}
|
||||
): Promise<{ authorizationUrl: URL; codeVerifier: string }> {
|
||||
let authorizationUrl: URL;
|
||||
|
|
@ -2098,7 +2119,7 @@ export async function startAuthorization(
|
|||
}
|
||||
|
||||
if (resource) {
|
||||
authorizationUrl.searchParams.set('resource', resource.href);
|
||||
authorizationUrl.searchParams.set('resource', resourceIndicatorToString(resource));
|
||||
}
|
||||
|
||||
return { authorizationUrl, codeVerifier };
|
||||
|
|
@ -2147,7 +2168,7 @@ export async function executeTokenRequest(
|
|||
tokenRequestParams: URLSearchParams;
|
||||
clientInformation?: OAuthClientInformationMixed;
|
||||
addClientAuthentication?: OAuthClientProvider['addClientAuthentication'];
|
||||
resource?: URL;
|
||||
resource?: string | URL;
|
||||
/**
|
||||
* SEP-1932 / RFC 9449 §5: when set, signs a DPoP proof into the token request's `DPoP`
|
||||
* header — the prerequisite for obtaining a DPoP-bound access token. On a `400
|
||||
|
|
@ -2166,7 +2187,7 @@ export async function executeTokenRequest(
|
|||
});
|
||||
|
||||
if (resource) {
|
||||
tokenRequestParams.set('resource', resource.href);
|
||||
tokenRequestParams.set('resource', resourceIndicatorToString(resource));
|
||||
}
|
||||
|
||||
if (!addClientAuthentication && clientInformation) {
|
||||
|
|
@ -2269,7 +2290,7 @@ export async function exchangeAuthorization(
|
|||
iss?: string;
|
||||
codeVerifier: string;
|
||||
redirectUri: string | URL;
|
||||
resource?: URL;
|
||||
resource?: string | URL;
|
||||
addClientAuthentication?: OAuthClientProvider['addClientAuthentication'];
|
||||
/** SEP-1932 / RFC 9449: see {@linkcode executeTokenRequest}'s `dpop` option. */
|
||||
dpop?: DpopSession;
|
||||
|
|
@ -2321,7 +2342,7 @@ export async function refreshAuthorization(
|
|||
metadata?: AuthorizationServerMetadata;
|
||||
clientInformation: OAuthClientInformationMixed;
|
||||
refreshToken: string;
|
||||
resource?: URL;
|
||||
resource?: string | URL;
|
||||
addClientAuthentication?: OAuthClientProvider['addClientAuthentication'];
|
||||
/** SEP-1932 / RFC 9449: see {@linkcode executeTokenRequest}'s `dpop` option. */
|
||||
dpop?: DpopSession;
|
||||
|
|
@ -2386,7 +2407,7 @@ export async function fetchToken(
|
|||
fetchFn
|
||||
}: {
|
||||
metadata?: AuthorizationServerMetadata;
|
||||
resource?: URL;
|
||||
resource?: string | URL;
|
||||
/** Authorization code for the default `authorization_code` grant flow */
|
||||
authorizationCode?: string;
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -1496,7 +1496,8 @@ describe('OAuth Authorization', () => {
|
|||
|
||||
it('calls saveDiscoveryState after discovery when provider implements it', async () => {
|
||||
const saveDiscoveryState = vi.fn();
|
||||
const provider = createMockProvider({ saveDiscoveryState });
|
||||
const saveResourceUrl = vi.fn();
|
||||
const provider = createMockProvider({ saveDiscoveryState, saveResourceUrl });
|
||||
|
||||
mockFetch.mockImplementation(url => {
|
||||
const urlString = url.toString();
|
||||
|
|
@ -1529,6 +1530,9 @@ describe('OAuth Authorization', () => {
|
|||
authorizationServerMetadata: validAuthMetadata
|
||||
})
|
||||
);
|
||||
expect(saveResourceUrl).toHaveBeenCalledWith('https://resource.example.com');
|
||||
const authorizationUrl = vi.mocked(provider.redirectToAuthorization).mock.calls[0]![0];
|
||||
expect(authorizationUrl.searchParams.get('resource')).toBe('https://resource.example.com');
|
||||
});
|
||||
|
||||
it('restores full discovery state from cache including resource metadata', async () => {
|
||||
|
|
@ -1580,7 +1584,7 @@ describe('OAuth Authorization', () => {
|
|||
const tokenCall = mockFetch.mock.calls.find(call => call[0].toString().includes('/token'));
|
||||
expect(tokenCall).toBeDefined();
|
||||
const body = tokenCall![1].body as URLSearchParams;
|
||||
expect(body.get('resource')).toBe('https://resource.example.com/');
|
||||
expect(body.get('resource')).toBe('https://resource.example.com');
|
||||
});
|
||||
|
||||
it('re-saves enriched state when partial cache is supplemented with fetched metadata', async () => {
|
||||
|
|
@ -1787,6 +1791,16 @@ describe('OAuth Authorization', () => {
|
|||
expect(codeVerifier).toBe('test_verifier');
|
||||
});
|
||||
|
||||
it('preserves a string resource indicator without URL normalization', async () => {
|
||||
const { authorizationUrl } = await startAuthorization('https://auth.example.com', {
|
||||
clientInformation: validClientInfo,
|
||||
redirectUrl: 'http://localhost:3000/callback',
|
||||
resource: 'https://api.example.com'
|
||||
});
|
||||
|
||||
expect(authorizationUrl.searchParams.get('resource')).toBe('https://api.example.com');
|
||||
});
|
||||
|
||||
it('includes scope parameter when provided', async () => {
|
||||
const { authorizationUrl } = await startAuthorization('https://auth.example.com', {
|
||||
clientInformation: validClientInfo,
|
||||
|
|
@ -3353,6 +3367,75 @@ describe('OAuth Authorization', () => {
|
|||
expect(authUrl.searchParams.get('resource')).toBe('https://api.example.com/');
|
||||
});
|
||||
|
||||
it('sends a pathless PRM resource verbatim on the authorization and token requests (#1968)', async () => {
|
||||
// RFC 9728 publishes the resource identifier and RFC 8707 requires it to be
|
||||
// sent unchanged. `new URL('https://example.com').href` is 'https://example.com/',
|
||||
// and authorization servers that match the indicator exactly (Microsoft Entra
|
||||
// ID: AADSTS9010010) reject the extra slash.
|
||||
mockFetch.mockImplementation(url => {
|
||||
const urlString = url.toString();
|
||||
|
||||
if (urlString.includes('/.well-known/oauth-protected-resource')) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
resource: 'https://example.com',
|
||||
authorization_servers: ['https://auth.example.com'],
|
||||
scopes_supported: ['https://example.com/mcp:tools']
|
||||
})
|
||||
});
|
||||
} else if (urlString.includes('/.well-known/oauth-authorization-server')) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({
|
||||
issuer: 'https://auth.example.com',
|
||||
authorization_endpoint: 'https://auth.example.com/authorize',
|
||||
token_endpoint: 'https://auth.example.com/token',
|
||||
response_types_supported: ['code'],
|
||||
code_challenge_methods_supported: ['S256']
|
||||
})
|
||||
});
|
||||
} else if (urlString.includes('/token')) {
|
||||
return Promise.resolve({
|
||||
ok: true,
|
||||
status: 200,
|
||||
json: async () => ({ access_token: 'access123', token_type: 'bearer', expires_in: 3600 })
|
||||
});
|
||||
}
|
||||
|
||||
return Promise.resolve({ ok: false, status: 404 });
|
||||
});
|
||||
|
||||
(mockProvider.clientInformation as Mock).mockResolvedValue({
|
||||
client_id: 'test-client',
|
||||
client_secret: 'test-secret'
|
||||
});
|
||||
(mockProvider.tokens as Mock).mockResolvedValue(undefined);
|
||||
(mockProvider.saveCodeVerifier as Mock).mockResolvedValue(undefined);
|
||||
(mockProvider.redirectToAuthorization as Mock).mockResolvedValue(undefined);
|
||||
(mockProvider.codeVerifier as Mock).mockResolvedValue('verifier123');
|
||||
(mockProvider.saveTokens as Mock).mockResolvedValue(undefined);
|
||||
|
||||
// Authorization request: the redirect carries the metadata value byte for byte.
|
||||
const redirectResult = await auth(mockProvider, { serverUrl: 'https://example.com/mcp' });
|
||||
expect(redirectResult).toBe('REDIRECT');
|
||||
const authUrl: URL = (mockProvider.redirectToAuthorization as Mock).mock.calls[0]![0];
|
||||
expect(authUrl.searchParams.get('resource')).toBe('https://example.com');
|
||||
|
||||
// Token request: the authorization-code exchange sends the same value.
|
||||
const exchangeResult = await auth(mockProvider, {
|
||||
serverUrl: 'https://example.com/mcp',
|
||||
authorizationCode: 'code123'
|
||||
});
|
||||
expect(exchangeResult).toBe('AUTHORIZED');
|
||||
const tokenCall = mockFetch.mock.calls.find(call => call[0].toString().includes('/token'));
|
||||
expect(tokenCall).toBeDefined();
|
||||
const body = tokenCall![1].body as URLSearchParams;
|
||||
expect(body.get('resource')).toBe('https://example.com');
|
||||
});
|
||||
|
||||
it('excludes resource parameter when Protected Resource Metadata is not present', async () => {
|
||||
// Mock metadata discovery where protected resource metadata is not available (404)
|
||||
// but authorization server metadata is available
|
||||
|
|
|
|||
Loading…
Reference in a new issue