Skip to content
Closed
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
119 changes: 119 additions & 0 deletions packages/core/src/mcp/oauth-provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,125 @@ describe('MCPOAuthProvider', () => {
);
});

it('should preserve registrationUrl from WWW-Authenticate discovery and use it for dynamic client registration', async () => {
const mcpServerUrl = 'https://mcp.example.com/mcp';
const registrationUrl = 'https://auth.example.com/register';

const configWithoutClient: MCPOAuthConfig = {
enabled: true,
scopes: ['read'],
};

const mockResourceMetadata: OAuthProtectedResourceMetadata = {
resource: mcpServerUrl,
authorization_servers: ['https://auth.example.com'],
};

const mockAuthServerMetadata: OAuthAuthorizationServerMetadata = {
issuer: 'https://auth.example.com',
authorization_endpoint: 'https://auth.example.com/authorize',
token_endpoint: 'https://auth.example.com/token',
registration_endpoint: registrationUrl,
code_challenge_methods_supported: ['S256'],
};

const mockRegistrationResponse: OAuthClientRegistrationResponse = {
client_id: 'dynamic_client_id',
redirect_uris: ['http://localhost:7777/oauth/callback'],
grant_types: ['authorization_code'],
response_types: ['code'],
token_endpoint_auth_method: 'none',
};

mockFetch
// 1. HEAD mcpServerUrl → 401 + WWW-Authenticate
.mockResolvedValueOnce({
ok: false,
status: 401,
headers: {
get: (name: string) =>
name.toLowerCase() === 'www-authenticate'
? `Bearer resource_metadata="https://mcp.example.com/.well-known/oauth-protected-resource/mcp"`
: null,
},
})
// 2. GET resource metadata
.mockResolvedValueOnce(
createMockResponse({
ok: true,
contentType: 'application/json',
text: JSON.stringify(mockResourceMetadata),
json: mockResourceMetadata,
}),
)
// 3. GET auth server metadata (includes registration_endpoint)
.mockResolvedValueOnce(
createMockResponse({
ok: true,
contentType: 'application/json',
text: JSON.stringify(mockAuthServerMetadata),
json: mockAuthServerMetadata,
}),
)
// 4. POST registration endpoint
.mockResolvedValueOnce(
createMockResponse({
ok: true,
contentType: 'application/json',
text: JSON.stringify(mockRegistrationResponse),
json: mockRegistrationResponse,
}),
);

// Setup callback server to simulate browser redirect with auth code
let callbackHandler: unknown;
vi.mocked(http.createServer).mockImplementation((handler) => {
callbackHandler = handler;
return mockHttpServer as unknown as http.Server;
});

mockHttpServer.listen.mockImplementation((port, callback) => {
callback?.();
setTimeout(() => {
const mockReq = {
url: '/oauth/callback?code=auth_code_123&state=bW9ja19zdGF0ZV8xNl9ieXRlcw',
};
const mockRes = { writeHead: vi.fn(), end: vi.fn() };
(callbackHandler as (req: unknown, res: unknown) => void)(
mockReq,
mockRes,
);
}, 10);
});

// 5. Token exchange
mockFetch.mockResolvedValueOnce(
createMockResponse({
ok: true,
contentType: 'application/json',
text: JSON.stringify(mockTokenResponse),
json: mockTokenResponse,
}),
);

const authProvider = new MCPOAuthProvider();
const result = await authProvider.authenticate(
'test-server',
configWithoutClient,
mcpServerUrl,
);

expect(result).toBeDefined();
// registerClient must have been called with the registrationUrl from WWW-Authenticate discovery
expect(mockFetch).toHaveBeenCalledWith(
registrationUrl,
expect.objectContaining({
method: 'POST',
headers: { 'Content-Type': 'application/json' },
}),
);
});

it('should throw error when issuer is missing and dynamic registration is needed', async () => {
const configWithoutIssuer: MCPOAuthConfig = {
enabled: mockConfig.enabled,
Expand Down
10 changes: 8 additions & 2 deletions packages/core/src/mcp/oauth-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ export class MCPOAuthProvider {
authorizationUrl: discoveredConfig.authorizationUrl,
issuer: discoveredConfig.issuer,
tokenUrl: discoveredConfig.tokenUrl,
registrationUrl: discoveredConfig.registrationUrl,
Comment thread
struckoff marked this conversation as resolved.
Comment thread
struckoff marked this conversation as resolved.
Comment thread
struckoff marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-high high

While this line correctly preserves the validated registrationUrl, the authenticate method contains a fallback discovery path (lines 389-400 in the full file) that is triggered if registrationUrl is missing (e.g., if it was stripped during validation). This fallback path extracts the registration_endpoint from metadata without any domain or origin validation and uses it in a subsequent fetch call. This allows an attacker to bypass the security checks added in oauth-utils.ts by providing a metadata response that fails initial validation but is then re-accepted in the fallback path, leading to SSRF.

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.

Addressed in the latest commit: the fallback path in authenticate() now routes through OAuthUtils.metadataToOAuthConfig(authServerMetadata, issuerUrl) so the same PSL-based registrable-domain validation is applied before using the re-discovered registration_endpoint. The issuerUrl returned by discoverAuthServerMetadataForRegistration is the URL that was actually used for discovery (the trusted anchor), so this properly mirrors how the primary flow works.

scopes: config.scopes || discoveredConfig.scopes || [],
// Preserve existing client credentials
clientId: config.clientId,
Expand Down Expand Up @@ -392,9 +393,14 @@ export class MCPOAuthProvider {
}

debugLogger.debug('→ Attempting dynamic client registration...');
const { metadata: authServerMetadata } =
const { metadata: authServerMetadata, issuerUrl } =
await this.discoverAuthServerMetadataForRegistration(config.issuer);
registrationUrl = authServerMetadata.registration_endpoint;
if (authServerMetadata) {
registrationUrl = OAuthUtils.metadataToOAuthConfig(
authServerMetadata,
issuerUrl,
)?.registrationUrl;
}
}

// Register client if registration endpoint is available
Expand Down
Loading