From 224a2e242d956f30b20fdeb370c8b9958e321f0c Mon Sep 17 00:00:00 2001 From: Olivier Chafik Date: Tue, 17 Jun 2025 00:18:33 +0100 Subject: [PATCH] update resource to be a url --- src/client/auth.ts | 18 ++-- .../server/demoInMemoryOAuthProvider.ts | 2 +- src/server/auth/handlers/authorize.test.ts | 6 +- src/server/auth/handlers/authorize.ts | 2 +- src/server/auth/provider.ts | 4 +- .../auth/providers/proxyProvider.test.ts | 2 +- src/server/auth/providers/proxyProvider.ts | 6 +- src/shared/auth-utils.test.ts | 85 +++---------------- 8 files changed, 33 insertions(+), 92 deletions(-) diff --git a/src/client/auth.ts b/src/client/auth.ts index 28188b7c..e465ea3b 100644 --- a/src/client/auth.ts +++ b/src/client/auth.ts @@ -100,12 +100,12 @@ export async function auth( authorizationCode?: string; scope?: string; resourceMetadataUrl?: URL; - resource?: string }): Promise { + resource?: URL }): Promise { // Remove fragment from resource parameter if provided - let canonicalResource: string | undefined; + let canonicalResource: URL | undefined; if (resource) { - canonicalResource = resourceUrlFromServerUrl(resource); + canonicalResource = resourceUrlFromServerUrl(new URL(resource)); } let authorizationServerUrl = serverUrl; @@ -329,7 +329,7 @@ export async function startAuthorization( redirectUrl: string | URL; scope?: string; state?: string; - resource?: string; + resource?: URL; }, ): Promise<{ authorizationUrl: URL; codeVerifier: string }> { const responseType = "code"; @@ -380,7 +380,7 @@ export async function startAuthorization( } if (resource) { - authorizationUrl.searchParams.set("resource", resource); + authorizationUrl.searchParams.set("resource", resource.href); } return { authorizationUrl, codeVerifier }; @@ -404,7 +404,7 @@ export async function exchangeAuthorization( authorizationCode: string; codeVerifier: string; redirectUri: string | URL; - resource?: string; + resource?: URL; }, ): Promise { const grantType = "authorization_code"; @@ -439,7 +439,7 @@ export async function exchangeAuthorization( } if (resource) { - params.set("resource", resource); + params.set("resource", resource.href); } const response = await fetch(tokenUrl, { @@ -471,7 +471,7 @@ export async function refreshAuthorization( metadata?: OAuthMetadata; clientInformation: OAuthClientInformation; refreshToken: string; - resource?: string; + resource?: URL; }, ): Promise { const grantType = "refresh_token"; @@ -504,7 +504,7 @@ export async function refreshAuthorization( } if (resource) { - params.set("resource", resource); + params.set("resource", resource.href); } const response = await fetch(tokenUrl, { diff --git a/src/examples/server/demoInMemoryOAuthProvider.ts b/src/examples/server/demoInMemoryOAuthProvider.ts index 9fcb2517..316d1f8a 100644 --- a/src/examples/server/demoInMemoryOAuthProvider.ts +++ b/src/examples/server/demoInMemoryOAuthProvider.ts @@ -122,7 +122,7 @@ export class DemoInMemoryAuthProvider implements OAuthServerProvider { client: OAuthClientInformationFull, _refreshToken: string, _scopes?: string[], - resource?: string + resource?: URL ): Promise { throw new Error('Refresh tokens not implemented for example demo'); } diff --git a/src/server/auth/handlers/authorize.test.ts b/src/server/auth/handlers/authorize.test.ts index 20a2af89..2742d1e5 100644 --- a/src/server/auth/handlers/authorize.test.ts +++ b/src/server/auth/handlers/authorize.test.ts @@ -295,7 +295,7 @@ describe('Authorization Handler', () => { expect(mockProviderWithResource).toHaveBeenCalledWith( validClient, expect.objectContaining({ - resource: 'https://api.example.com/resource', + resource: new URL('https://api.example.com/resource'), redirectUri: 'https://example.com/callback', codeChallenge: 'challenge123' }), @@ -365,7 +365,7 @@ describe('Authorization Handler', () => { expect(mockProviderWithResources).toHaveBeenCalledWith( validClient, expect.objectContaining({ - resource: 'https://api1.example.com/resource', + resource: new URL('https://api1.example.com/resource'), state: 'test-state' }), expect.any(Object) @@ -391,7 +391,7 @@ describe('Authorization Handler', () => { expect(mockProviderPost).toHaveBeenCalledWith( validClient, expect.objectContaining({ - resource: 'https://api.example.com/resource' + resource: new URL('https://api.example.com/resource') }), expect.any(Object) ); diff --git a/src/server/auth/handlers/authorize.ts b/src/server/auth/handlers/authorize.ts index f6c862ac..17c88b45 100644 --- a/src/server/auth/handlers/authorize.ts +++ b/src/server/auth/handlers/authorize.ts @@ -142,7 +142,7 @@ export function authorizationHandler({ provider, rateLimit: rateLimitConfig }: A scopes: requestedScopes, redirectUri: redirect_uri, codeChallenge: code_challenge, - resource, + resource: resource ? new URL(resource) : undefined, }, res); } catch (error) { // Post-redirect errors - redirect with error parameters diff --git a/src/server/auth/provider.ts b/src/server/auth/provider.ts index 93a56a09..18beb216 100644 --- a/src/server/auth/provider.ts +++ b/src/server/auth/provider.ts @@ -42,13 +42,13 @@ export interface OAuthServerProvider { authorizationCode: string, codeVerifier?: string, redirectUri?: string, - resource?: string + resource?: URL ): Promise; /** * Exchanges a refresh token for an access token. */ - exchangeRefreshToken(client: OAuthClientInformationFull, refreshToken: string, scopes?: string[], resource?: string): Promise; + exchangeRefreshToken(client: OAuthClientInformationFull, refreshToken: string, scopes?: string[], resource?: URL): Promise; /** * Verifies an access token and returns information about it. diff --git a/src/server/auth/providers/proxyProvider.test.ts b/src/server/auth/providers/proxyProvider.test.ts index b652390b..75dc1a15 100644 --- a/src/server/auth/providers/proxyProvider.test.ts +++ b/src/server/auth/providers/proxyProvider.test.ts @@ -112,7 +112,7 @@ describe("Proxy OAuth Server Provider", () => { codeChallenge: 'test-challenge', state: 'test-state', scopes: ['read', 'write'], - resource: 'https://api.example.com/resource' + resource: new URL('https://api.example.com/resource') }, mockResponse ); diff --git a/src/server/auth/providers/proxyProvider.ts b/src/server/auth/providers/proxyProvider.ts index 7f8b8d3d..4c807444 100644 --- a/src/server/auth/providers/proxyProvider.ts +++ b/src/server/auth/providers/proxyProvider.ts @@ -134,7 +134,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { // Add optional standard OAuth parameters if (params.state) searchParams.set("state", params.state); if (params.scopes?.length) searchParams.set("scope", params.scopes.join(" ")); - if (params.resource) searchParams.set("resource", params.resource); + if (params.resource) searchParams.set("resource", params.resource.href); targetUrl.search = searchParams.toString(); res.redirect(targetUrl.toString()); @@ -154,7 +154,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { authorizationCode: string, codeVerifier?: string, redirectUri?: string, - resource?: string + resource?: URL ): Promise { const params = new URLSearchParams({ grant_type: "authorization_code", @@ -199,7 +199,7 @@ export class ProxyOAuthServerProvider implements OAuthServerProvider { client: OAuthClientInformationFull, refreshToken: string, scopes?: string[], - resource?: string + resource?: URL ): Promise { const params = new URLSearchParams({ diff --git a/src/shared/auth-utils.test.ts b/src/shared/auth-utils.test.ts index b9571408..c35bb122 100644 --- a/src/shared/auth-utils.test.ts +++ b/src/shared/auth-utils.test.ts @@ -1,89 +1,30 @@ -import { validateResourceUri, extractResourceUri, resourceUrlFromServerUrl } from './auth-utils.js'; +import { resourceUrlFromServerUrl } from './auth-utils.js'; describe('auth-utils', () => { describe('resourceUrlFromServerUrl', () => { it('should remove fragments', () => { - expect(resourceUrlFromServerUrl('https://example.com/path#fragment')).toBe('https://example.com/path'); - expect(resourceUrlFromServerUrl('https://example.com#fragment')).toBe('https://example.com'); - expect(resourceUrlFromServerUrl('https://example.com/path?query=1#fragment')).toBe('https://example.com/path?query=1'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/path#fragment')).href).toBe('https://example.com/path'); + expect(resourceUrlFromServerUrl(new URL('https://example.com#fragment')).href).toBe('https://example.com/'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/path?query=1#fragment')).href).toBe('https://example.com/path?query=1'); }); it('should return URL unchanged if no fragment', () => { - expect(resourceUrlFromServerUrl('https://example.com')).toBe('https://example.com'); - expect(resourceUrlFromServerUrl('https://example.com/path')).toBe('https://example.com/path'); - expect(resourceUrlFromServerUrl('https://example.com/path?query=1')).toBe('https://example.com/path?query=1'); + expect(resourceUrlFromServerUrl(new URL('https://example.com')).href).toBe('https://example.com/'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/path')).href).toBe('https://example.com/path'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/path?query=1')).href).toBe('https://example.com/path?query=1'); }); it('should keep everything else unchanged', () => { // Case sensitivity preserved - expect(resourceUrlFromServerUrl('HTTPS://EXAMPLE.COM/PATH')).toBe('HTTPS://EXAMPLE.COM/PATH'); + expect(resourceUrlFromServerUrl(new URL('https://EXAMPLE.COM/PATH')).href).toBe('https://example.com/PATH'); // Ports preserved - expect(resourceUrlFromServerUrl('https://example.com:443/path')).toBe('https://example.com:443/path'); - expect(resourceUrlFromServerUrl('https://example.com:8080/path')).toBe('https://example.com:8080/path'); + expect(resourceUrlFromServerUrl(new URL('https://example.com:443/path')).href).toBe('https://example.com/path'); + expect(resourceUrlFromServerUrl(new URL('https://example.com:8080/path')).href).toBe('https://example.com:8080/path'); // Query parameters preserved - expect(resourceUrlFromServerUrl('https://example.com?foo=bar&baz=qux')).toBe('https://example.com?foo=bar&baz=qux'); + expect(resourceUrlFromServerUrl(new URL('https://example.com?foo=bar&baz=qux')).href).toBe('https://example.com/?foo=bar&baz=qux'); // Trailing slashes preserved - expect(resourceUrlFromServerUrl('https://example.com/')).toBe('https://example.com/'); - expect(resourceUrlFromServerUrl('https://example.com/path/')).toBe('https://example.com/path/'); - }); - }); - - - describe('validateResourceUri', () => { - it('should accept valid resource URIs without fragments', () => { - expect(() => validateResourceUri('https://example.com')).not.toThrow(); - expect(() => validateResourceUri('https://example.com/path')).not.toThrow(); - expect(() => validateResourceUri('http://example.com:8080')).not.toThrow(); - expect(() => validateResourceUri('https://example.com?query=1')).not.toThrow(); - expect(() => validateResourceUri('ftp://example.com')).not.toThrow(); // Only fragment check now - }); - - it('should reject URIs with fragments', () => { - expect(() => validateResourceUri('https://example.com#fragment')).toThrow('must not contain a fragment'); - expect(() => validateResourceUri('https://example.com/path#section')).toThrow('must not contain a fragment'); - expect(() => validateResourceUri('https://example.com?query=1#anchor')).toThrow('must not contain a fragment'); - }); - - it('should accept any URI without fragment', () => { - // These are all valid now since we only check for fragments - expect(() => validateResourceUri('//example.com')).not.toThrow(); - expect(() => validateResourceUri('https://user:pass@example.com')).not.toThrow(); - expect(() => validateResourceUri('/path')).not.toThrow(); - expect(() => validateResourceUri('path')).not.toThrow(); - }); - }); - - describe('extractResourceUri', () => { - it('should remove fragments from URLs', () => { - expect(extractResourceUri('https://example.com/path#fragment')).toBe('https://example.com/path'); - expect(extractResourceUri('https://example.com/path?query=1#fragment')).toBe('https://example.com/path?query=1'); - }); - - it('should handle URL object', () => { - const url = new URL('https://example.com:8443/path?query=1#fragment'); - expect(extractResourceUri(url)).toBe('https://example.com:8443/path?query=1'); - }); - - it('should keep everything else unchanged', () => { - // Preserves case - expect(extractResourceUri('HTTPS://EXAMPLE.COM/path')).toBe('HTTPS://EXAMPLE.COM/path'); - // Preserves all ports - expect(extractResourceUri('https://example.com:443/path')).toBe('https://example.com:443/path'); - expect(extractResourceUri('http://example.com:80/path')).toBe('http://example.com:80/path'); - // Preserves query parameters - expect(extractResourceUri('https://example.com/path?query=1')).toBe('https://example.com/path?query=1'); - // Preserves trailing slashes - expect(extractResourceUri('https://example.com/')).toBe('https://example.com/'); - expect(extractResourceUri('https://example.com/app1/')).toBe('https://example.com/app1/'); - }); - - it('should distinguish between different paths on same domain', () => { - // This is the key test for the security concern mentioned - const app1 = extractResourceUri('https://api.example.com/mcp-server-1'); - const app2 = extractResourceUri('https://api.example.com/mcp-server-2'); - expect(app1).not.toBe(app2); - expect(app1).toBe('https://api.example.com/mcp-server-1'); - expect(app2).toBe('https://api.example.com/mcp-server-2'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/')).href).toBe('https://example.com/'); + expect(resourceUrlFromServerUrl(new URL('https://example.com/path/')).href).toBe('https://example.com/path/'); }); }); }); \ No newline at end of file