Skip to content

use issuer instead of authorization_endpoint for oauth discovery #17332

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
scidomino merged 2 commits into from
Feb 18, 2026
Merged

use issuer instead of authorization_endpoint for oauth discovery #17332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
069689a
use issuer instead of authorization_endpoint for dynamic client regis…
garrettsparks Jan 22, 2026
a62c99d
Merge branch 'main' into garrett.sparks/fix-mcp-oauth-authorization-s…
scidomino Feb 18, 2026
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
22 changes: 22 additions & 0 deletions packages/core/src/mcp/oauth-provider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ describe('MCPOAuthProvider', () => {
clientId: 'test-client-id',
clientSecret: 'test-client-secret',
authorizationUrl: 'https://auth.example.com/authorize',
issuer: 'https://auth.example.com',
tokenUrl: 'https://auth.example.com/token',
scopes: ['read', 'write'],
redirectUri: 'http://localhost:7777/oauth/callback',
Expand Down Expand Up @@ -622,6 +623,27 @@ describe('MCPOAuthProvider', () => {
);
});

it('should throw error when issuer is missing and dynamic registration is needed', async () => {
const configWithoutIssuer: MCPOAuthConfig = {
enabled: mockConfig.enabled,
authorizationUrl: mockConfig.authorizationUrl,
tokenUrl: mockConfig.tokenUrl,
scopes: mockConfig.scopes,
redirectUri: mockConfig.redirectUri,
audiences: mockConfig.audiences,
};

mockHttpServer.listen.mockImplementation((port, callback) => {
callback?.();
});

const authProvider = new MCPOAuthProvider();

await expect(
authProvider.authenticate('test-server', configWithoutIssuer),
).rejects.toThrow('Cannot perform dynamic registration without issuer');
});

it('should handle OAuth callback errors', async () => {
let callbackHandler: unknown;
vi.mocked(http.createServer).mockImplementation((handler) => {
Expand Down
19 changes: 9 additions & 10 deletions packages/core/src/mcp/oauth-provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export interface MCPOAuthConfig {
clientId?: string;
clientSecret?: string;
authorizationUrl?: string;
issuer?: string;
tokenUrl?: string;
scopes?: string[];
audiences?: string[];
Expand Down Expand Up @@ -161,14 +162,14 @@ export class MCPOAuthProvider {
}

private async discoverAuthServerMetadataForRegistration(
authorizationUrl: string,
issuer: string,
Comment thread
scidomino marked this conversation as resolved.
): Promise<{
issuerUrl: string;
metadata: NonNullable<
Awaited<ReturnType<typeof OAuthUtils.discoverAuthorizationServerMetadata>>
>;
}> {
const authUrl = new URL(authorizationUrl);
const authUrl = new URL(issuer);

// Preserve path components for issuers with path-based discovery (e.g., Keycloak)
// Extract issuer by removing the OIDC protocol-specific path suffix
Expand Down Expand Up @@ -784,6 +785,7 @@ export class MCPOAuthProvider {
config = {
...config,
authorizationUrl: discoveredConfig.authorizationUrl,
issuer: discoveredConfig.issuer,
tokenUrl: discoveredConfig.tokenUrl,
scopes: config.scopes || discoveredConfig.scopes || [],
// Preserve existing client credentials
Expand Down Expand Up @@ -814,6 +816,7 @@ export class MCPOAuthProvider {
...config,
authorizationUrl: discoveredConfig.authorizationUrl,
tokenUrl: discoveredConfig.tokenUrl,
issuer: discoveredConfig.issuer,
scopes: config.scopes || discoveredConfig.scopes || [],
registrationUrl: discoveredConfig.registrationUrl,
// Preserve existing client credentials
Expand Down Expand Up @@ -852,18 +855,14 @@ export class MCPOAuthProvider {

// If no registration URL was previously discovered, try to discover it
if (!registrationUrl) {
// Extract server URL from authorization URL
if (!config.authorizationUrl) {
throw new Error(
'Cannot perform dynamic registration without authorization URL',
);
// Use the issuer to discover registration endpoint
if (!config.issuer) {
throw new Error('Cannot perform dynamic registration without issuer');
}

debugLogger.debug('→ Attempting dynamic client registration...');
const { metadata: authServerMetadata } =
await this.discoverAuthServerMetadataForRegistration(
config.authorizationUrl,
);
await this.discoverAuthServerMetadataForRegistration(config.issuer);

@gemini-code-assist gemini-code-assist Bot Jan 22, 2026

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

The application performs OAuth discovery and dynamic client registration using URLs provided by external MCP servers without proper validation. Specifically, the issuer URL (and subsequent URLs derived from it or provided in metadata) is used in fetch calls. A malicious MCP server can provide URLs pointing to internal services (e.g., http://localhost, http://169.254.169.254) or other sensitive endpoints, leading to Server-Side Request Forgery (SSRF). This can be used to probe internal networks or access cloud metadata services from the user's machine.

Remediation: Implement strict validation for all URLs obtained from external metadata before using them in fetch calls. This should include:

  1. Blocking internal IP ranges (e.g., 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16).
  2. Blocking cloud metadata service IPs (e.g., 169.254.169.254).
  3. Ensuring that the issuer in the metadata matches the expected issuer URL.
  4. Consider using an allow-list of trusted domains if possible.

@garrettsparks garrettsparks Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This behavior is already present in the codebase and is not modified as a result of this change: authorization_endpoint (currently in use) is unvalidated.

registrationUrl = authServerMetadata.registration_endpoint;
}

Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/mcp/oauth-utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ describe('OAuthUtils', () => {

expect(config).toEqual({
authorizationUrl: 'https://auth.example.com/authorize',
issuer: 'https://auth.example.com',
tokenUrl: 'https://auth.example.com/token',
scopes: ['read', 'write'],
});
Expand Down Expand Up @@ -286,6 +287,7 @@ describe('OAuthUtils', () => {

expect(config).toEqual({
authorizationUrl: 'https://auth.example.com/authorize',
issuer: 'https://auth.example.com',
tokenUrl: 'https://auth.example.com/token',
scopes: ['read', 'write'],
});
Expand All @@ -302,6 +304,19 @@ describe('OAuthUtils', () => {

expect(config.scopes).toEqual([]);
});

it('should use issuer from metadata', () => {
const metadata: OAuthAuthorizationServerMetadata = {
issuer: 'https://auth.example.com',
authorization_endpoint: 'https://auth.example.com/oauth/authorize',
token_endpoint: 'https://auth.example.com/token',
scopes_supported: ['read', 'write'],
};

const config = OAuthUtils.metadataToOAuthConfig(metadata);

expect(config.issuer).toBe('https://auth.example.com');
});
});

describe('parseWWWAuthenticateHeader', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/mcp/oauth-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export class OAuthUtils {
): MCPOAuthConfig {
return {
authorizationUrl: metadata.authorization_endpoint,
issuer: metadata.issuer,
tokenUrl: metadata.token_endpoint,
scopes: metadata.scopes_supported || [],
registrationUrl: metadata.registration_endpoint,
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/tools/mcp-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ async function handleAutomaticOAuth(
const oauthAuthConfig = {
enabled: true,
authorizationUrl: oauthConfig.authorizationUrl,
issuer: oauthConfig.issuer,
tokenUrl: oauthConfig.tokenUrl,
scopes: oauthConfig.scopes || [],
};
Expand Down Expand Up @@ -1783,6 +1784,7 @@ export async function connectToMcpServer(
const oauthAuthConfig = {
enabled: true,
authorizationUrl: oauthConfig.authorizationUrl,
issuer: oauthConfig.issuer,
tokenUrl: oauthConfig.tokenUrl,
scopes: oauthConfig.scopes || [],
};
Expand Down
Loading