fix(oauth): use discoverOAuthFromWWWAuthenticate for 401-triggered OAuth discovery#19626
Conversation
Summary of ChangesHello @yuvrajangadsingh, 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 in the OAuth discovery process that occurred when a 401 Unauthorized response was received. Previously, the system would incorrectly attempt to discover OAuth configuration from an already well-known resource metadata URI, resulting in malformed URLs and authentication failures. The fix re-architects the discovery mechanism to correctly interpret the 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
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the automatic OAuth discovery process by leveraging the discoverOAuthFromWWWAuthenticate utility to properly fetch and validate metadata as per RFC 9728. However, it introduces a Server-Side Request Forgery (SSRF) vulnerability in the automatic OAuth discovery flow. The client fetches the resource_metadata URI provided in the WWW-Authenticate header without verifying its origin, which could allow a malicious server to trigger requests to internal network resources. An origin check is recommended before fetching the metadata URI. The accompanying test updates are solid, improving robustness by correctly mocking the new code paths, and the changes generally enhance code clarity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request correctly implements the use of discoverOAuthFromWWWAuthenticate to handle OAuth discovery triggered by 401 responses, resolving issues with double-nested paths and improving compliance with RFC 9728. However, it introduces a potential SSRF vulnerability by fetching the resource_metadata URI directly from the untrusted WWW-Authenticate header without origin validation, violating the 'fail-closed' security principle. Additionally, a critical regression was introduced in the import section of mcp-client.ts, where value-level Zod schemas were merged into a type-only import block, which will cause runtime errors when those schemas are accessed as values.
…uth discovery The reactive OAuth flow passed the resource_metadata URI (a .well-known URL) to discoverOAuthConfig(), which expects a base server URL and constructs .well-known paths from it. This caused double-nested paths that 404, followed by a ResourceMismatchError when the root fallback compared against the wrong expected resource. Use the existing discoverOAuthFromWWWAuthenticate() method which correctly fetches the resource metadata directly and validates against the actual server URL per RFC 9728. Fixes google-gemini#18760
2b4a670 to
198b285
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly fixes a bug in the OAuth discovery flow for 401 responses. However, it introduces or exposes a Server-Side Request Forgery (SSRF) vulnerability by fetching URIs provided in the WWW-Authenticate header without proper validation. This could allow a malicious MCP server to probe the user's local network or interact with internal services. The issue is exacerbated by network requests occurring before user consent, which is against best practices for OAuth flows.
Validate that the resource_metadata URI from the WWW-Authenticate header has the same origin as the MCP server URL before fetching it. This prevents a malicious MCP server from directing the client to fetch internal network resources or cloud metadata endpoints.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves the issue of handleAutomaticOAuth generating invalid double-nested .well-known URIs by implementing discoverOAuthFromWWWAuthenticate to correctly fetch and validate OAuth metadata from the WWW-Authenticate header, adhering to RFC 9728. Although an origin check was introduced to prevent potential SSRF vulnerabilities, the current implementation remains susceptible to a bypass via HTTP redirects, as fetch calls follow redirects by default, which could allow probing of internal network resources. The changes are well-tested and maintain backward compatibility, generally enhancing the robustness of the OAuth discovery process.
|
/gemini review |
|
That's an excellent catch and a crucial security enhancement, @yuvrajangadsingh! Adding |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a bug in the OAuth discovery logic by correctly using discoverOAuthFromWWWAuthenticate and introduces important security enhancements to prevent Server-Side Request Forgery (SSRF) vulnerabilities, specifically by disabling redirects for resource_metadata fetches and enforcing a same-origin policy. However, the SSRF protection is incomplete as it does not cover the subsequent fetch of authorization server metadata or validate the authorization server URLs themselves. These gaps could still allow a malicious MCP server to probe internal network resources. It is recommended to apply the same redirect policy to all metadata fetches and implement IP/origin validation for authorization server URLs.
…ivate host authorization servers
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to address issues with double-nested .well-known URIs during OAuth discovery by switching to discoverOAuthFromWWWAuthenticate and introduces security controls like disabling redirects and validating against private hostnames. However, the implemented SSRF protection via isPrivateHost is incomplete, inconsistently applied, and easily bypassable, violating the 'fail-closed' principle for security checks and posing a significant security risk. The isPrivateHost implementation itself is fragile and susceptible to bypasses using various IP address representations, and is omitted in certain critical code paths like the discoverOAuthConfig fallback.
…s, broader IP coverage, protect discoverOAuthConfig path
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully addresses the OAuth discovery issue where double-nested .well-known paths were being constructed, leading to 404 errors. By switching to discoverOAuthFromWWWAuthenticate, the code now correctly handles the resource_metadata URI provided in the WWW-Authenticate header and validates it against the actual MCP server URL as per RFC 9728. Additionally, the PR introduces important security enhancements to mitigate SSRF risks, including disabling redirect following in metadata fetches and implementing a robust check to block private or internal hostnames. The changes are well-tested and maintain high code quality.
|
@nicolo-ribaudo @nichochar would love a maintainer review when you get a chance — fixes the OAuth discovery bug in #18760 where 401-triggered flows fail for path-based MCP servers, plus comprehensive SSRF hardening (origin checks, redirect protection, private host blocking). all tests passing and gemini-code-assist is clean. |
|
Closing — #19038 was merged to fix this issue. |
Fixes #18760
Problem
handleAutomaticOAuth()passes theresource_metadataURI from theWWW-Authenticateheader todiscoverOAuthConfig(), which expects a base server URL. Sinceresource_metadatais already a.well-knownURL,discoverOAuthConfigconstructs a double-nested path like:This 404s. The root-based fallback then fetches the root metadata but validation fails with
ResourceMismatchErrorbecause it compares the root resource against the.well-knownURI instead of the actual server URL.Fix
Replace
discoverOAuthConfig(resourceMetadataUri)withdiscoverOAuthFromWWWAuthenticate(wwwAuthenticate, serverUrl), which already exists inoauth-utils.tsand correctly:resource_metadataURIresourcefield against the actual MCP server URL (per RFC 9728 §7.3)The base-URL fallback path (
discoverOAuthConfig) is preserved for 401 responses that don't include aresource_metadatain theWWW-Authenticateheader.Testing
Updated existing tests in
mcp-client.test.tsto mockdiscoverOAuthFromWWWAuthenticateon the primary path anddiscoverOAuthConfigon the fallback path. All 60 tests pass.