fix(core): preserve registrationUrl in OAuth config merges#20992
fix(core): preserve registrationUrl in OAuth config merges#20992struckoff wants to merge 6 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug affecting dynamic client registration for MCP servers. By ensuring the registrationUrl is properly retained during OAuth configuration processing, it restores the functionality of RFC 7591, which was previously failing due to the URL being dropped. The changes specifically target the points where OAuth configurations are constructed or merged, guaranteeing the complete propagation of necessary metadata. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
struckoff
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where the registrationUrl was being dropped during OAuth configuration merges. The changes add the missing property in three key places where OAuth configuration objects are constructed, ensuring that dynamic client registration can proceed as expected. The implementation is straightforward and effectively resolves the described issue.
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where the registrationUrl for OAuth was not being preserved during configuration merges. The changes add the registrationUrl property to OAuth configuration objects in oauth-provider.ts and mcp-client.ts to ensure it is available for dynamic client registration.
Note: Security Review did not run due to the size of the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug where the registrationUrl for OAuth was lost during configuration merging, which is critical for dynamic client registration. The changes across oauth-provider.ts and mcp-client.ts are direct and effectively address the issue.
However, the bug fix is not accompanied by corresponding tests. To prevent future regressions of this important functionality, I've added comments suggesting the addition of test cases that specifically cover the scenarios where registrationUrl is discovered and must be preserved.
Note: Security Review did not run due to the size of the PR.
struckoff
force-pushed
the
fix/preserve-registration-url-in-oauth-discovery
branch
2 times, most recently
from
b379f97 to
2d61dd9
Compare
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where the registrationUrl for OAuth dynamic client registration was being dropped during configuration merges, with changes made to oauth-provider.ts and mcp-client.ts and corresponding test updates in oauth-provider.test.ts and mcp-client.test.ts. While the bug is correctly addressed, the fix introduces a Server-Side Request Forgery (SSRF) vulnerability. By accepting and using discovered OAuth URLs without validation, a malicious MCP server could redirect the CLI to perform POST requests against internal network services. It is recommended to implement URL validation for all discovered OAuth endpoints to ensure they are secure and do not target sensitive internal resources.
|
/gemini help |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where the registrationUrl for OAuth dynamic client registration was being dropped during configuration merges, ensuring it is correctly preserved in oauth-provider.ts and mcp-client.ts. While resolving this issue, the introduction of dynamic client registration using discovered URLs increases the risk of Server-Side Request Forgery (SSRF). The application currently follows URLs from external MCP servers (via WWW-Authenticate headers or discovery metadata) without sufficient validation, which could allow malicious or compromised servers to trigger requests to internal services or sensitive local endpoints. It is recommended to implement strict URL validation and an internal IP blocklist for all discovered OAuth endpoints to mitigate this risk, adhering to a 'fail-closed' security principle.
There was a problem hiding this comment.
Code Review
This pull request aims to fix a bug where the registrationUrl for OAuth dynamic client registration was being dropped during configuration merges, ensuring it's correctly preserved across oauth-provider.ts and mcp-client.ts. However, preserving this registrationUrl without proper validation introduces a critical Server-Side Request Forgery (SSRF) vulnerability, as a malicious MCP server could direct the CLI to internal endpoints. Additionally, the review identified opportunities to improve maintainability by refactoring duplicated code blocks related to OAuth configuration, which would reduce redundancy and prevent future inconsistencies. It is highly recommended to implement strict URL validation for all discovered OAuth endpoints to mitigate the SSRF risk.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/mcp/oauth-provider.ts (788)
This section of code, which merges discovered OAuth configuration, is duplicated and also introduces a critical Server-Side Request Forgery (SSRF) vulnerability. The registrationUrl is used as a sink for a POST request, and without validation, a malicious MCP server could direct it to internal network resources. To improve maintainability and mitigate the SSRF risk, extract this logic into a helper function and implement strict validation for the registrationUrl to ensure it uses https: and does not resolve to private IP addresses.
References
- Security-sensitive settings, such as
registrationUrl, must not be merged in a way that allows less-trusted sources (like MCP server metadata) to introduce unvalidated or malicious values, as this can lead to critical vulnerabilities like SSRF.
packages/core/src/tools/mcp-client.ts (771)
This object construction, which includes the registrationUrl from MCP server metadata, is duplicated and also introduces a critical Server-Side Request Forgery (SSRF) vulnerability. The registrationUrl is used in the MCPOAuthProvider.authenticate flow for dynamic client registration via a POST request. Without proper validation, a malicious server could direct this to internal services. To improve maintainability and mitigate the SSRF risk, refactor this logic into a shared helper function and validate the registrationUrl to ensure it is a public HTTPS URL and not an internal or metadata endpoint.
References
- Security-sensitive settings, such as
registrationUrl, must not be merged in a way that allows less-trusted sources (like MCP server metadata) to introduce unvalidated or malicious values, as this can lead to critical vulnerabilities like SSRF.
packages/core/src/tools/mcp-client.ts (1869)
Server-Side Request Forgery (SSRF) in Dynamic Client Registration
Same as above, the registrationUrl is added to the OAuth configuration builder in the 401 handler without validation, which can lead to SSRF during the dynamic client registration process.
References
- Security-sensitive settings, such as
registrationUrl, must not be merged in a way that allows less-trusted sources (like MCP server metadata) to introduce unvalidated or malicious values, as this can lead to critical vulnerabilities like SSRF.
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the registrationUrl for OAuth dynamic client registration was being dropped, correctly propagating it through oauth-provider.ts and mcp-client.ts. It also introduces a security enhancement in oauth-utils.ts to prevent potential SSRF attacks by validating the registration_endpoint against the issuer's origin. However, this SSRF mitigation is incomplete as it fails to validate the issuer identifier against the discovery source origin, as required by RFC 8414, which could still allow malicious redirection to internal network resources. The fix should be extended to include validation of the issuer origin. The comprehensive new and updated tests, particularly an end-to-end test for dynamic registration, effectively validate the bug fix.
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the registrationUrl for OAuth was being dropped during configuration merges, which prevented dynamic client registration. The changes correctly preserve this URL in mcp-client.ts and oauth-provider.ts. A security validation was also added in oauth-utils.ts to prevent potential SSRF attacks, but this validation contains a critical flaw related to incorrect domain identification, which needs to be addressed to ensure a fail-closed security posture.
Note: Security Review did not run due to the size of the PR.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of registrationUrl being dropped during OAuth configuration merges, which is crucial for dynamic client registration. The changes correctly preserve and propagate this URL across oauth-provider.ts and mcp-client.ts.
More importantly, the introduction of robust URL validation in oauth-utils.ts is a significant security enhancement. By validating discovered endpoints against a trusted authServerUrl and using the Public Suffix List for domain comparison, you've effectively mitigated potential SSRF and open redirect vulnerabilities. The new tests are comprehensive and thoroughly cover these security-critical logic changes and the end-to-end dynamic registration flow. The code quality is excellent, and I found no issues.
Note: Security Review did not run due to the size of the PR.
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where the registrationUrl was being dropped during OAuth configuration merges, which prevented dynamic client registration. It also introduces important security validations for OAuth endpoints, ensuring they use HTTPS and match the authorization server's domain, which is a crucial defense against Server-Side Request Forgery (SSRF) attacks. However, a high-severity SSRF vulnerability remains in the discovery process: the trust anchor for endpoint validation is the discovered authorization server URL, which itself is not validated against the initial trusted MCP server URL. This could allow a malicious MCP server to trigger requests to internal services. A critical comment with a suggested fix has been provided to address this.
| const authorizationUrl = metadata.authorization_endpoint; | ||
| const tokenUrl = metadata.token_endpoint; | ||
| try { | ||
| const authzUrl = new URL(authorizationUrl); | ||
| const tokUrl = new URL(tokenUrl); | ||
|
|
||
| if ( | ||
| (authzUrl.protocol !== 'https:' && !OAuthUtils.isLoopback(authzUrl)) || | ||
| (tokUrl.protocol !== 'https:' && !OAuthUtils.isLoopback(tokUrl)) | ||
| ) { | ||
| debugLogger.debug( | ||
| 'authorization_endpoint and token_endpoint must use https: protocol.', | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| if (authServerUrl) { | ||
| const authUrl = new URL(authServerUrl); | ||
| // Only apply domain validation for public HTTPS discovery URLs. | ||
| // Loopback auth servers are used in local dev and are exempt. | ||
| if (authUrl.protocol === 'https:' && !OAuthUtils.isLoopback(authUrl)) { | ||
| const authDomain = OAuthUtils.extractRegistrableDomain( | ||
| authUrl.hostname, | ||
| ); | ||
| const authzDomain = OAuthUtils.extractRegistrableDomain( | ||
| authzUrl.hostname, | ||
| ); | ||
| const tokDomain = OAuthUtils.extractRegistrableDomain( | ||
| tokUrl.hostname, | ||
| ); | ||
|
|
||
| // When PSL returns null for one or both sides (e.g., IP addresses), | ||
| // fall back to exact hostname comparison instead of treating null as mismatch. | ||
| const mismatch = (eDomain: string | null, eHostname: string) => { | ||
| if (authDomain && eDomain) return eDomain !== authDomain; | ||
| return eHostname !== authUrl.hostname; | ||
| }; | ||
| if ( | ||
| mismatch(authzDomain, authzUrl.hostname) || | ||
| mismatch(tokDomain, tokUrl.hostname) | ||
| ) { | ||
| debugLogger.debug( | ||
| 'authorization_endpoint or token_endpoint domain does not match authServerUrl.', | ||
| ); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| debugLogger.debug( | ||
| `Failed to validate required OAuth endpoints: ${getErrorMessage(error)}`, | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| authorizationUrl: metadata.authorization_endpoint, | ||
| authorizationUrl, | ||
| issuer: metadata.issuer, | ||
| tokenUrl: metadata.token_endpoint, | ||
| tokenUrl, | ||
| scopes: metadata.scopes_supported || [], | ||
| registrationUrl: metadata.registration_endpoint, | ||
| registrationUrl, | ||
| }; |
There was a problem hiding this comment.
The metadata.issuer is taken from the authorization server metadata without validation. This value is later used in MCPOAuthProvider.discoverAuthServerMetadataForRegistration to make further network requests to discover the registration_endpoint. A malicious authorization server could provide a crafted issuer URL pointing to an internal service, leading to a Server-Side Request Forgery (SSRF) vulnerability.
The issuer URL should be validated against the authServerUrl (the trusted discovery anchor) in the same way that authorization_endpoint and token_endpoint are validated.
const authorizationUrl = metadata.authorization_endpoint;
const tokenUrl = metadata.token_endpoint;
const issuer = metadata.issuer;
try {
const authzUrl = new URL(authorizationUrl);
const tokUrl = new URL(tokenUrl);
const issuerUrl = new URL(issuer);
if (
(authzUrl.protocol !== 'https:' && !OAuthUtils.isLoopback(authzUrl)) ||
(tokUrl.protocol !== 'https:' && !OAuthUtils.isLoopback(tokUrl)) ||
(issuerUrl.protocol !== 'https:' && !OAuthUtils.isLoopback(issuerUrl))
) {
debugLogger.debug(
'authorization_endpoint, token_endpoint, and issuer must use https: protocol unless on a loopback address.',
);
return null;
}
if (authServerUrl) {
const authUrl = new URL(authServerUrl);
// Only apply domain validation for public HTTPS discovery URLs.
// Loopback auth servers are used in local dev and are exempt.
if (authUrl.protocol === 'https:' && !OAuthUtils.isLoopback(authUrl)) {
const authDomain = OAuthUtils.extractRegistrableDomain(
authUrl.hostname,
);
const authzDomain = OAuthUtils.extractRegistrableDomain(
authzUrl.hostname,
);
const tokDomain = OAuthUtils.extractRegistrableDomain(
tokUrl.hostname,
);
const issuerDomain = OAuthUtils.extractRegistrableDomain(
issuerUrl.hostname,
);
// When PSL returns null for one or both sides (e.g., IP addresses),
// fall back to exact hostname comparison instead of treating null as mismatch.
const mismatch = (eDomain: string | null, eHostname: string) => {
if (authDomain && eDomain) return eDomain !== authDomain;
return eHostname !== authUrl.hostname;
};
if (
mismatch(authzDomain, authzUrl.hostname) ||
mismatch(tokDomain, tokUrl.hostname) ||
mismatch(issuerDomain, issuerUrl.hostname)
) {
debugLogger.debug(
'authorization_endpoint, token_endpoint, or issuer domain does not match authServerUrl.',
);
return null;
}
}
}
} catch (error) {
debugLogger.debug(
`Failed to validate required OAuth endpoints: ${getErrorMessage(error)}`,
);
return null;
}
return {
authorizationUrl,
issuer,
tokenUrl,
scopes: metadata.scopes_supported || [],
registrationUrl,
};There was a problem hiding this comment.
Added validation for issuer alongside authorization_endpoint and token_endpoint: it must use HTTPS (with the same loopback exemption) and, when authServerUrl is a public HTTPS URL, must share its registrable domain. Per RFC 8414 §3, the issuer must match the authorization server's identity, so this check is consistent with the spec.
| if (authServerUrl) { | ||
| const authUrl = new URL(authServerUrl); | ||
| // RFC 8414 §3: the authorization server URL must also use HTTPS. | ||
| if (authUrl.protocol !== 'https:') { | ||
| debugLogger.debug( | ||
| 'authServerUrl must use https: protocol; discarding registration_endpoint.', | ||
| ); | ||
| registrationUrl = undefined; | ||
| } else { | ||
| // Anchor validation to the trusted discovery URL (RFC 8414 §3). | ||
| // The registration_endpoint must share the registrable domain with | ||
| // the authorization server the metadata was fetched from. | ||
| // Using registrable domain (rather than full origin) accommodates | ||
| // deployments where discovery and registration endpoints are on | ||
| // different subdomains of the same organization. | ||
| const authDomain = OAuthUtils.extractRegistrableDomain( | ||
| authUrl.hostname, | ||
| ); | ||
| if (regDomain && authDomain) { | ||
| if (regDomain !== authDomain) registrationUrl = undefined; | ||
| } else if (regUrl.hostname !== authUrl.hostname) { | ||
| // PSL returned null for one or both sides (e.g., IP addresses). | ||
| // Fall back to exact hostname comparison. | ||
| registrationUrl = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic for registrationUrl in metadataToOAuthConfig anchors the trust to the authServerUrl. However, authServerUrl is discovered from untrusted metadata provided by the MCP server (via the WWW-Authenticate header) in discoverOAuthFromWWWAuthenticate.
An attacker-controlled MCP server can provide a malicious resourceMetadataUri that points to an authServerUrl on an internal HTTPS service. Because the validation only ensures that registrationUrl shares the registrable domain with authServerUrl, the client will proceed to perform a POST request (dynamic client registration) to that internal service.
To remediate this, ensure that the authServerUrl (and the resourceMetadataUri) are validated against the trusted mcpServerUrl (e.g., by checking they share the same origin or registrable domain) before proceeding with discovery and registration.
There was a problem hiding this comment.
authServerUrl is derived from authorization_servers[0] in the resource metadata, which is validated against mcpServerUrl via the resource field check (RFC 9728 §7.3) before we use it. That validation is the trust anchor for the discovery chain. Protecting the underlying fetch calls from SSRF is tracked separately by the safeFetch migration TODO comments already in the codebase — it is out of scope for this PR.
struckoff
force-pushed
the
fix/preserve-registration-url-in-oauth-discovery
branch
from
f665f4e to
44f21c9
Compare
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the registrationUrl for OAuth dynamic client registration was being dropped during configuration merges. The fix correctly preserves this URL in oauth-provider.ts and mcp-client.ts. A significant and valuable part of this change is the introduction of robust security validation in OAuthUtils.metadataToOAuthConfig to prevent SSRF and other vulnerabilities by validating discovered OAuth endpoint URLs against a trusted anchor. The changes are well-tested with new, comprehensive tests covering both the bug fix and the new security validations. The code quality is high, and the fix appears correct and secure.
Per RFC 8414 §3, the issuer must identify the authorization server and must use HTTPS. Extend the existing validation block in metadataToOAuthConfig to parse metadata.issuer as a URL and apply the same checks already applied to authorization_endpoint and token_endpoint: - HTTPS protocol required (with the same loopback exemption for local dev) - When authServerUrl is a public HTTPS URL, issuer must share its registrable domain (prevents accepting crafted metadata with a foreign issuer)
struckoff
force-pushed
the
fix/preserve-registration-url-in-oauth-discovery
branch
from
f391e0e to
ab11c6f
Compare
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of registrationUrl being dropped during OAuth configuration merges, correctly preserving it in all three specified locations. This is a great fix that enables dynamic client registration as intended.
Beyond the bug fix, this PR introduces significant and crucial security enhancements to the OAuth discovery process. The new validation logic in metadataToOAuthConfig is well-implemented, using psl for flexible domain validation and enforcing HTTPS to mitigate risks like SSRF. The addition of comprehensive unit tests for this new security logic is also excellent.
One area for future improvement to enhance robustness would be to add timeouts to the discovery fetch calls in fetchProtectedResourceMetadata and fetchAuthorizationServerMetadata within oauth-utils.ts. This would prevent the CLI from hanging if a discovery endpoint is unresponsive. I'm mentioning this here as these functions are outside the scope of the current diff.
There was a problem hiding this comment.
Code Review
The pull request refactors OAuth endpoint validation, particularly for the registrationUrl, by introducing extractRegistrableDomain and isLoopback utility functions. The OAuthUtils.metadataToOAuthConfig function now performs comprehensive security checks, including domain matching and HTTPS enforcement, and can return null for invalid configurations. The MCPOAuthProvider and mcp-client components are updated to correctly pass and utilize the registrationUrl during dynamic client registration. However, a critical issue was identified where the registration_endpoint is incorrectly discarded if the authServerUrl uses HTTP, even for loopback addresses, which is permitted by RFC 8252 §8.3 and is essential for local development testing.
| let registrationUrl = metadata.registration_endpoint; | ||
| if (registrationUrl) { | ||
| try { | ||
| const regUrl = new URL(registrationUrl); | ||
| // RFC 7591 §3.1: the client registration endpoint MUST be an HTTPS URL. | ||
| if (regUrl.protocol !== 'https:') { | ||
| debugLogger.debug( | ||
| 'registration_endpoint must use https: protocol; discarding.', | ||
| ); | ||
| registrationUrl = undefined; | ||
| } else { | ||
| const regDomain = OAuthUtils.extractRegistrableDomain( | ||
| regUrl.hostname, | ||
| ); | ||
|
|
||
| if (authServerUrl) { | ||
| const authUrl = new URL(authServerUrl); | ||
| // RFC 8414 §3: the authorization server URL must also use HTTPS. | ||
| if (authUrl.protocol !== 'https:') { | ||
| debugLogger.debug( | ||
| 'authServerUrl must use https: protocol; discarding registration_endpoint.', | ||
| ); | ||
| registrationUrl = undefined; | ||
| } else { | ||
| // Anchor validation to the trusted discovery URL (RFC 8414 §3). | ||
| // The registration_endpoint must share the registrable domain with | ||
| // the authorization server the metadata was fetched from. | ||
| // Using registrable domain (rather than full origin) accommodates | ||
| // deployments where discovery and registration endpoints are on | ||
| // different subdomains of the same organization. | ||
| const authDomain = OAuthUtils.extractRegistrableDomain( | ||
| authUrl.hostname, | ||
| ); | ||
| if (regDomain && authDomain) { | ||
| if (regDomain !== authDomain) registrationUrl = undefined; | ||
| } else if (regUrl.hostname !== authUrl.hostname) { | ||
| // PSL returned null for one or both sides (e.g., IP addresses). | ||
| // Fall back to exact hostname comparison. | ||
| registrationUrl = undefined; | ||
| } | ||
| } | ||
| } else { | ||
| // When no discovery URL is available to anchor validation, we cannot | ||
| // safely trust the registration_endpoint. While we could fall back | ||
| // to comparing against the issuer, the issuer itself is part of the | ||
| // same untrusted metadata document. | ||
| debugLogger.debug( | ||
| 'No authServerUrl provided for validation; discarding registration_endpoint.', | ||
| ); | ||
| registrationUrl = undefined; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| debugLogger.debug( | ||
| `Failed to validate registration_endpoint URL: ${getErrorMessage(error)}`, | ||
| ); | ||
| registrationUrl = undefined; | ||
| } |
There was a problem hiding this comment.
The current implementation validates the registration_endpoint URL but discards it if authServerUrl uses HTTP. However, RFC 8252 §8.3 allows HTTP for loopback addresses. It's crucial to allow registrationUrl for loopback addresses to support local development scenarios where the auth server runs on localhost without TLS. This could be a critical issue because it prevents developers from testing dynamic client registration locally.
if (regUrl.protocol !== 'https:') {
if (!authServerUrl || !OAuthUtils.isLoopback(new URL(authServerUrl))) {
debugLogger.debug(
'registration_endpoint must use https: protocol; discarding.',
);
registrationUrl = undefined;
}There was a problem hiding this comment.
Added a loopback exemption to the registration_endpoint HTTPS check, consistent with the exemptions already applied to authorization_endpoint, token_endpoint, and issuer (RFC 8252 §8.3). Also restructured the authServerUrl guard to mirror the pattern used in the required-endpoints block: domain validation is only applied when authServerUrl is a public HTTPS URL, and is skipped for loopback or HTTP auth servers.
Per RFC 8252 §8.3, loopback addresses are treated as a secure context and are exempt from the HTTPS requirement. Apply the same exemption to registration_endpoint validation that is already applied to authorization_endpoint, token_endpoint, and issuer: - registration_endpoint: allow HTTP when the URL is a loopback address - authServerUrl guard: mirror the auth/token pattern — only apply domain validation when authServerUrl is a public HTTPS URL; skip for loopback or HTTP auth servers (consistent with existing required-endpoints logic)
There was a problem hiding this comment.
Code Review
This pull request effectively fixes a bug where the registrationUrl for OAuth dynamic client registration was being dropped. The changes correctly propagate this URL during configuration merges. A significant security enhancement has also been introduced in oauth-utils.ts, which now validates all discovered OAuth endpoint URLs against a trusted discovery anchor to prevent potential SSRF vulnerabilities. The implementation is robust, well-tested, and addresses the issue comprehensively.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of registrationUrl being silently dropped during OAuth configuration merges. The changes correctly propagate the registrationUrl in oauth-provider.ts and mcp-client.ts, ensuring dynamic client registration functions as expected. A significant improvement is the introduction of robust validation logic within OAuthUtils.metadataToOAuthConfig. This new logic critically checks for HTTPS protocol, applies loopback exemptions, and performs domain matching using the Mozilla Public Suffix List, which greatly enhances the security of the OAuth flow by mitigating potential Server-Side Request Forgery (SSRF) vulnerabilities from untrusted metadata. The comprehensive new test cases in oauth-utils.test.ts and oauth-provider.test.ts thoroughly verify this enhanced validation and the correct preservation of the registrationUrl across various scenarios. Overall, this is a well-implemented fix that improves both correctness and security.
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
2 participants
When discovering OAuth configuration via WWW-Authenticate header, the registrationUrl from authorization server metadata was silently dropped in three places where oauthAuthConfig objects are constructed. This caused dynamic client registration (RFC 7591) to fail for MCP servers that advertise registration_endpoint in their metadata.
The fix preserves registrationUrl in all three config merge/construction sites:
Summary
Details
Related Issues
Fixes #20990
How to Validate
Pre-Merge Checklist