Skip to content

Commit 8aa5d2f

Browse files
TylerLeonhardtTylerLeonhardt
authored
Important fix for resource validation in MCP (microsoft#259089)
In this example: MCP url: https://foo.com/mcp WWW-Auth resource_metadata: https://foo.com/laugh/in/the/face/of/conventions.json PRM: https://foo.com/laugh/in/the/face/of/conventions.json PRM resource property: https://foo.com/mcp I use to do: a URL derived from "WWW-Auth resource_metadata" (which is actually just PRM) is compared to "PRM resource property" but instead it should be: "MCP url" compared to "PRM resource property" Which is what this PR addresses
1 parent 3eaa650 commit 8aa5d2f

File tree

3 files changed

+7
-141
lines changed

3 files changed

+7
-141
lines changed

‎src/vs/base/common/oauth.ts‎

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -819,35 +819,3 @@ export function getClaimsFromJWT(token: string): IAuthorizationJWTClaims {
819819
throw new Error('Failed to parse JWT token');
820820
}
821821
}
822-
823-
/**
824-
* Extracts the resource server base URL from an OAuth protected resource metadata discovery endpoint URL.
825-
*
826-
* @param discoveryUrl The full URL to the OAuth protected resource metadata discovery endpoint
827-
* @returns The base URL of the resource server
828-
*
829-
* @example
830-
* ```typescript
831-
* getResourceServerBaseUrlFromDiscoveryUrl('https://mcp.example.com/.well-known/oauth-protected-resource')
832-
* // Returns: 'https://mcp.example.com/'
833-
*
834-
* getResourceServerBaseUrlFromDiscoveryUrl('https://mcp.example.com/.well-known/oauth-protected-resource/mcp')
835-
* // Returns: 'https://mcp.example.com/mcp'
836-
* ```
837-
*/
838-
export function getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl: string): string {
839-
const url = new URL(discoveryUrl);
840-
841-
// Remove the well-known discovery path only if it appears at the beginning
842-
if (!url.pathname.startsWith(AUTH_PROTECTED_RESOURCE_METADATA_DISCOVERY_PATH)) {
843-
throw new Error(`Invalid discovery URL: expected path to start with ${AUTH_PROTECTED_RESOURCE_METADATA_DISCOVERY_PATH}`);
844-
}
845-
846-
const pathWithoutDiscovery = url.pathname.substring(AUTH_PROTECTED_RESOURCE_METADATA_DISCOVERY_PATH.length);
847-
848-
// Construct the base URL
849-
const baseUrl = new URL(url.origin);
850-
baseUrl.pathname = pathWithoutDiscovery || '/';
851-
852-
return baseUrl.toString();
853-
}

‎src/vs/base/test/common/oauth.test.ts‎

Lines changed: 0 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import * as sinon from 'sinon';
88
import {
99
getClaimsFromJWT,
1010
getDefaultMetadataForUrl,
11-
getResourceServerBaseUrlFromDiscoveryUrl,
1211
isAuthorizationAuthorizeResponse,
1312
isAuthorizationDeviceResponse,
1413
isAuthorizationErrorResponse,
@@ -625,106 +624,6 @@ suite('OAuth', () => {
625624
});
626625
});
627626

628-
suite('getResourceServerBaseUrlFromDiscoveryUrl', () => {
629-
test('should extract base URL from discovery URL at root', () => {
630-
const discoveryUrl = 'https://mcp.example.com/.well-known/oauth-protected-resource';
631-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
632-
assert.strictEqual(result, 'https://mcp.example.com/');
633-
});
634-
635-
test('should extract base URL from discovery URL with subpath', () => {
636-
const discoveryUrl = 'https://mcp.example.com/.well-known/oauth-protected-resource/mcp';
637-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
638-
assert.strictEqual(result, 'https://mcp.example.com/mcp');
639-
});
640-
641-
test('should extract base URL from discovery URL with nested subpath', () => {
642-
const discoveryUrl = 'https://api.example.com/.well-known/oauth-protected-resource/v1/services/mcp';
643-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
644-
assert.strictEqual(result, 'https://api.example.com/v1/services/mcp');
645-
});
646-
647-
test('should handle discovery URL with port number', () => {
648-
const discoveryUrl = 'https://localhost:8443/.well-known/oauth-protected-resource/api';
649-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
650-
assert.strictEqual(result, 'https://localhost:8443/api');
651-
});
652-
653-
test('should handle discovery URL with query parameters', () => {
654-
const discoveryUrl = 'https://example.com/.well-known/oauth-protected-resource/api?version=1';
655-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
656-
assert.strictEqual(result, 'https://example.com/api');
657-
});
658-
659-
test('should handle discovery URL with fragment', () => {
660-
const discoveryUrl = 'https://example.com/.well-known/oauth-protected-resource/api#section';
661-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
662-
assert.strictEqual(result, 'https://example.com/api');
663-
});
664-
665-
test('should handle discovery URL ending with trailing slash', () => {
666-
const discoveryUrl = 'https://example.com/.well-known/oauth-protected-resource/api/';
667-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
668-
assert.strictEqual(result, 'https://example.com/api/');
669-
});
670-
671-
test('should handle HTTP URLs', () => {
672-
const discoveryUrl = 'http://localhost:3000/.well-known/oauth-protected-resource/dev';
673-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
674-
assert.strictEqual(result, 'http://localhost:3000/dev');
675-
});
676-
677-
test('should throw error for URL without discovery path', () => {
678-
const discoveryUrl = 'https://example.com/some/other/path';
679-
assert.throws(
680-
() => getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl),
681-
/Invalid discovery URL: expected path to start with \/\.well-known\/oauth-protected-resource/
682-
);
683-
});
684-
685-
test('should throw error for URL with partial discovery path', () => {
686-
const discoveryUrl = 'https://example.com/.well-known/oauth';
687-
assert.throws(
688-
() => getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl),
689-
/Invalid discovery URL: expected path to start with \/\.well-known\/oauth-protected-resource/
690-
);
691-
});
692-
693-
test('should throw error for URL with discovery path not at beginning', () => {
694-
const discoveryUrl = 'https://example.com/api/.well-known/oauth-protected-resource';
695-
assert.throws(
696-
() => getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl),
697-
/Invalid discovery URL: expected path to start with \/\.well-known\/oauth-protected-resource/
698-
);
699-
});
700-
701-
test('should throw error for invalid URL format', () => {
702-
const discoveryUrl = 'not-a-valid-url';
703-
assert.throws(
704-
() => getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl),
705-
TypeError
706-
);
707-
});
708-
709-
test('should handle empty path after discovery path', () => {
710-
const discoveryUrl = 'https://example.com/.well-known/oauth-protected-resource';
711-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
712-
assert.strictEqual(result, 'https://example.com/');
713-
});
714-
715-
test('should preserve URL encoding in subpath', () => {
716-
const discoveryUrl = 'https://example.com/.well-known/oauth-protected-resource/api%20v1';
717-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
718-
assert.strictEqual(result, 'https://example.com/api%20v1');
719-
});
720-
721-
test('should normalize hostname case consistently', () => {
722-
const discoveryUrl = 'https://MCP.EXAMPLE.COM/.well-known/oauth-protected-resource';
723-
const result = getResourceServerBaseUrlFromDiscoveryUrl(discoveryUrl);
724-
assert.strictEqual(result, 'https://mcp.example.com/');
725-
});
726-
});
727-
728627
suite('Client ID Fallback Scenarios', () => {
729628
let sandbox: sinon.SinonSandbox;
730629
let fetchStub: sinon.SinonStub;

‎src/vs/workbench/api/common/extHostMcp.ts‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { extensionPrefixedIdentifier, McpCollectionDefinition, McpConnectionStat
1616
import { ExtHostMcpShape, MainContext, MainThreadMcpShape } from './extHost.protocol.js';
1717
import { IExtHostRpcService } from './extHostRpcService.js';
1818
import * as Convert from './extHostTypeConverters.js';
19-
import { AUTH_SERVER_METADATA_DISCOVERY_PATH, OPENID_CONNECT_DISCOVERY_PATH, getDefaultMetadataForUrl, getResourceServerBaseUrlFromDiscoveryUrl, IAuthorizationProtectedResourceMetadata, IAuthorizationServerMetadata, isAuthorizationProtectedResourceMetadata, isAuthorizationServerMetadata, parseWWWAuthenticateHeader } from '../../../base/common/oauth.js';
19+
import { AUTH_SERVER_METADATA_DISCOVERY_PATH, OPENID_CONNECT_DISCOVERY_PATH, getDefaultMetadataForUrl, IAuthorizationProtectedResourceMetadata, IAuthorizationServerMetadata, isAuthorizationProtectedResourceMetadata, isAuthorizationServerMetadata, parseWWWAuthenticateHeader } from '../../../base/common/oauth.js';
2020
import { URI } from '../../../base/common/uri.js';
2121
import { MCP } from '../../contrib/mcp/common/modelContextProtocol.js';
2222
import { CancellationError } from '../../../base/common/errors.js';
@@ -314,7 +314,7 @@ class McpHTTPHandle extends Disposable {
314314
}
315315
}
316316

317-
private async _populateAuthMetadata(originalResponse: Response): Promise<void> {
317+
private async _populateAuthMetadata(url: string, originalResponse: Response): Promise<void> {
318318
// If there is a resource_metadata challenge, use that to get the oauth server. This is done in 2 steps.
319319
// First, extract the resource_metada challenge from the WWW-Authenticate header (if available)
320320
let resourceMetadataChallenge: string | undefined;
@@ -331,6 +331,10 @@ class McpHTTPHandle extends Disposable {
331331
let resource: IAuthorizationProtectedResourceMetadata | undefined;
332332
if (resourceMetadataChallenge) {
333333
const resourceMetadata = await this._getResourceMetadata(resourceMetadataChallenge);
334+
// Use URL constructor for normalization - it handles hostname case and trailing slashes
335+
if (new URL(resourceMetadata.resource).toString() !== new URL(url).toString()) {
336+
throw new Error(`Protected Resource Metadata resource "${resourceMetadata.resource}" does not match MCP server resolved resource "${url}". The MCP server must follow OAuth spec https://datatracker.ietf.org/doc/html/rfc9728#PRConfigurationValidation`);
337+
}
334338
// TODO:@TylerLeonhardt support multiple authorization servers
335339
// Consider using one that has an auth provider first, over the dynamic flow
336340
serverMetadataUrl = resourceMetadata.authorization_servers?.[0];
@@ -395,11 +399,6 @@ class McpHTTPHandle extends Disposable {
395399
}
396400
const body = await resourceMetadataResponse.json();
397401
if (isAuthorizationProtectedResourceMetadata(body)) {
398-
const resolvedResource = getResourceServerBaseUrlFromDiscoveryUrl(resourceMetadata);
399-
// Use URL constructor for normalization - it handles hostname case and trailing slashes
400-
if (new URL(body.resource).toString() !== new URL(resolvedResource).toString()) {
401-
throw new Error(`Protected Resource Metadata resource "${body.resource}" does not match MCP server resolved resource "${resolvedResource}". The MCP server must follow OAuth spec https://datatracker.ietf.org/doc/html/rfc9728#PRConfigurationValidation`);
402-
}
403402
return body;
404403
} else {
405404
throw new Error(`Invalid resource metadata: ${JSON.stringify(body)}`);
@@ -701,7 +700,7 @@ class McpHTTPHandle extends Disposable {
701700
let res = await doFetch();
702701
if (res.status === 401) {
703702
if (!this._authMetadata) {
704-
await this._populateAuthMetadata(res);
703+
await this._populateAuthMetadata(url, res);
705704
await this._addAuthHeader(headers);
706705
if (headers['Authorization']) {
707706
// Update the headers in the init object

0 commit comments

Comments
 (0)