fix(core): prevent OAuth IdP mix-up in MCP authentication (463963247) - #29117
fix(core): prevent OAuth IdP mix-up in MCP authentication (463963247)#29117jvargassanchez-dot wants to merge 4 commits into
Conversation
Implements RFC 9207 Authorization Server Issuer Identification validation in the OAuth callback handler to defend against Identity Provider (IdP) mix-up attacks and prevent unauthorized token leakage.
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 enhances the security of the OAuth authentication flow by implementing RFC 9207 validation. By verifying the issuer (iss) parameter returned during the callback, the system can now defend against Identity Provider (IdP) mix-up attacks. The changes include robust URL normalization, updated callback server logic, and comprehensive test coverage to ensure both modern and legacy authorization servers are handled correctly. Highlights
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 the 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 counterproductive. 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. Footnotes
|
|
📊 PR Size: size/L
|
![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 implements RFC 9207 Authorization Server Issuer Identification to defend against OAuth Mix-Up attacks. It introduces an areIssuersEqual utility to compare issuer URLs and updates startCallbackServer to validate the incoming iss parameter against the expected issuer, accompanied by comprehensive unit tests. However, a security vulnerability was identified in the areIssuersEqual normalization logic: discarding the userinfo component from the URL allows an attacker to bypass the issuer check using a crafted URL (e.g., containing an @ symbol). It is recommended to apply the provided suggestion to preserve the full URL structure while stripping only query parameters, fragments, and trailing slashes.
| export function areIssuersEqual(issuerA: string, issuerB: string): boolean { | ||
| if (issuerA === issuerB) return true; | ||
| const normalize = (iss: string): string => { | ||
| try { | ||
| const u = new URL(iss); | ||
| const normalizedPath = u.pathname.replace(/\/+$/, ''); | ||
| return `${u.protocol}//${u.host}${normalizedPath}`; | ||
| } catch { | ||
| return iss.replace(/\/+$/, ''); | ||
| } | ||
| }; | ||
| return normalize(issuerA) === normalize(issuerB); | ||
| } |
There was a problem hiding this comment.
The areIssuersEqual function's normalize helper is vulnerable to an OAuth Mix-Up attack (RFC 9207 bypass). The current implementation discards userinfo (username and password) from the URL, allowing an attacker to craft an issuer parameter like https://attacker.com@github.com/login/oauth which, after normalization, would incorrectly match the expected issuer. This could lead to the theft of authorization codes or access tokens.
To remediate this, the normalize function should preserve the full URL structure, including userinfo, while stripping query parameters, fragments, and trailing slashes. This ensures that the userinfo component is not inadvertently removed, preventing the described mix-up attack.
Note that the current normalization logic also discards query parameters (u.search) and fragments (u.hash). While RFC 8414 states that issuer URLs should not contain these, some custom or multi-tenant identity providers use them to distinguish between different tenants or environments. This could potentially lead to another form of IdP mix-up bypass if not carefully considered.
| export function areIssuersEqual(issuerA: string, issuerB: string): boolean { | |
| if (issuerA === issuerB) return true; | |
| const normalize = (iss: string): string => { | |
| try { | |
| const u = new URL(iss); | |
| const normalizedPath = u.pathname.replace(/\/+$/, ''); | |
| return `${u.protocol}//${u.host}${normalizedPath}`; | |
| } catch { | |
| return iss.replace(/\/+$/, ''); | |
| } | |
| }; | |
| return normalize(issuerA) === normalize(issuerB); | |
| } | |
| export function areIssuersEqual(issuerA: string, issuerB: string): boolean { | |
| if (issuerA === issuerB) return true; | |
| const normalize = (iss: string): string => { | |
| try { | |
| const u = new URL(iss); | |
| u.search = ''; | |
| u.hash = ''; | |
| return u.href.replace(~/+$/, ''); | |
| } catch { | |
| return iss.replace(~/+$/, ''); | |
| } | |
| }; | |
| return normalize(issuerA) === normalize(issuerB); | |
| } |
gemini-cli
Bot
added
the
status/need-issue
…63963247) - Reject issuer URLs containing userinfo (username/password) to prevent RFC 9207 mix-up bypasses - Preserve query parameters and fragments during normalization to avoid multi-tenant issuer collisions - Normalize trailing slashes on pathname rather than href - Add unit and callback server tests for userinfo spoofing and query parameter differentiation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements RFC 9207 Authorization Server Issuer Identification to defend against OAuth mix-up attacks. It introduces an areIssuersEqual utility to normalize and compare issuer URLs, updates startCallbackServer to validate the incoming iss parameter against an expected issuer, and adds corresponding unit tests. The review feedback highlights a security downgrade vulnerability where the iss parameter is allowed to be omitted even when an expected issuer is configured; per RFC 9207, the server should strictly reject the response in this scenario to maintain a fail-closed security posture.
| } else if (expectedIssuer && !iss) { | ||
| debugLogger.debug( | ||
| `OAuth callback omitted "iss" parameter; expected "${expectedIssuer}". Proceeding for backwards compatibility with legacy authorization servers.`, | ||
| ); | ||
| } else if (iss) { |
There was a problem hiding this comment.
Allowing the iss parameter to be omitted when an expectedIssuer is configured introduces a downgrade vulnerability that defeats the RFC 9207 mix-up defense. An attacker performing an IdP mix-up attack can simply strip the iss parameter from the authorization response to bypass this check entirely.
According to RFC 9207 Section 2.1:
If the client has been configured to expect a particular issuer identifier, and the authorization response does not contain the 'iss' parameter, the client MUST reject the response.
To maintain robust security, we should strictly reject the response by default when the expected issuer is missing, ensuring security checks are implemented in a 'fail-closed' manner.
} else if (expectedIssuer && !iss) {
debugLogger.error(
'OAuth callback omitted "iss" parameter; expected "' + expectedIssuer + '". Rejecting response per RFC 9207 Section 2.1.',
);
res.writeHead(400, { 'Content-Type': 'text/html' });
res.end(
'<html>' +
'<body>' +
'<h1>Authentication Failed</h1>' +
'<p>Error: Missing issuer parameter in response.</p>' +
'<p>You can close this window.</p>' +
'</body>' +
'</html>'
);
server.close();
reject(
new Error(
'Missing "iss" parameter in response; expected "' + expectedIssuer + '" per RFC 9207',
),
);
return;
}References
- Security checks should be implemented in a 'fail-closed' manner. If an item's validity cannot be verified (e.g., due to missing metadata or parameters), it should be rejected by default.
…e error output (b/463963247) - Reject callbacks when expectedIssuer is configured but iss is missing to prevent downgrade attacks - Sanitize error messages and debug logs by removing raw issuer URLs and sensitive configuration details - Add tests covering fail-closed missing iss behavior and information disclosure prevention
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements RFC 9207 Authorization Server Issuer Identification and Mix-Up defense in the OAuth flow. It introduces the areIssuersEqual utility function to safely compare issuer URLs by normalizing trailing slashes and origins, rejecting userinfo, and preserving query parameters and fragments. The startCallbackServer function is updated to accept an optional expectedIssuer parameter and enforce issuer validation when configured, rejecting authorization responses that lack a matching iss parameter. Comprehensive unit tests have been added to verify these security checks and normalization behaviors. There are no review comments, and I have no additional feedback to provide.
…th servers (b/463963247) - Add authorization server requirements and callback redirect examples for RFC 9207 - Add configuration example with explicit issuer in settings.json - Document issuer property under OAuth configuration properties
jvargassanchez-dot
changed the title
1 participant
Summary
Implements RFC 9207 Authorization Server Issuer Identification validation in the OAuth callback handler to defend against Identity Provider (IdP) mix-up attacks and prevent unauthorized token leakage.
Details
OAuthAuthorizationResponseto include the optionaliss?: stringparameter.areIssuersEqual(issuerA, issuerB)helper inoauth-flow.tsto normalize trailing slashes and origin paths per RFC 8414/9207 while rejectinguserinfoand preserving queries/fragments.startCallbackServerto acceptexpectedIssuer?: stringand enforce fail-closed RFC 9207 validation (rejecting missingissor mismatched issuers with HTTP 400).config.issuerfrom MCP discovery tostartCallbackServerinMCPOAuthProvider.authenticate.docs/tools/mcp-server.mdwith RFC 9207 authorization server requirements.Compatibility Note for MCP Developers
issparameter in the callback redirect (/oauth/callback?code=...&state=...&iss=https://<issuer>).isswill be rejected.Related Issues
How to Validate
npm test -w @google/gemini-cli-a2a-serverPre-Merge Checklist