fix(mcp): add SSRF protection to OAuth metadata discovery#28112
fix(mcp): add SSRF protection to OAuth metadata discovery#28112herdiyana256 wants to merge 3 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 critical SSRF vulnerabilities in the OAuth discovery flow and the WebFetch tool. By consistently applying loopback and private IP validation, the changes prevent the application from making unauthorized requests to internal cloud metadata services or local network endpoints. The implementation leverages existing utility functions to ensure a robust and uniform security posture across all URL-fetching operations. 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
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
![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 introduces SSRF protection by resolving and validating DNS hostnames to prevent access to private or loopback IP addresses across OAuth providers, utilities, and web fetch tools. However, the current implementation of pinning the IP address by replacing the hostname in the URL and setting the Host header will cause HTTPS requests to fail TLS certificate verification. To securely prevent DNS rebinding attacks without breaking HTTPS/TLS, it is recommended to keep the original URL intact and use a custom undici Agent with a custom lookup function passed as a dispatcher option to fetch.
| const resolvedAddrs = await resolveAndValidateDns(resourceMetadataUrl); | ||
| if (resolvedAddrs.length === 0) { | ||
| debugLogger.debug( | ||
| `Blocked OAuth metadata fetch to private/reserved IP: ${resourceMetadataUrl}`, | ||
| ); | ||
| return null; | ||
| } | ||
| const response = await fetch(resourceMetadataUrl); |
There was a problem hiding this comment.
The fetchProtectedResourceMetadata method is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) / DNS Rebinding attack. To securely pin the IP address without breaking HTTPS/TLS, keep the original URL intact and use a custom undici Agent with a custom lookup function passed as a dispatcher option to fetch, maintaining consistency with the reference implementation in web-fetch.ts.
const resolvedAddrs = await resolveAndValidateDns(resourceMetadataUrl);
if (resolvedAddrs.length === 0) {
debugLogger.debug(
"Blocked OAuth metadata fetch to private/reserved IP: " + resourceMetadataUrl,
);
return null;
}
const pinnedIp = resolvedAddrs[0];
const dispatcher = new Agent({
connect: {
lookup: (hostname, options, callback) => {
callback(null, [{ address: pinnedIp, family: pinnedIp.includes(':') ? 6 : 4 }]);
},
},
});
const response = await fetch(resourceMetadataUrl, {
dispatcher,
});References
- When a change's primary purpose is to align a component with a reference implementation (e.g., a 'main agent'), prioritize consistency with the reference, even if it means not addressing potential issues in the aligned code.
There was a problem hiding this comment.
Fixed in 04ef846. Now uses undici Agent with a custom lookup callback to pin the resolved IP at the connection level, preventing DNS rebinding between check and use. The original URL is preserved so TLS SNI verification remains intact.
| const resolvedAddrs = await resolveAndValidateDns(authServerMetadataUrl); | ||
| if (resolvedAddrs.length === 0) { | ||
| debugLogger.debug( | ||
| `Blocked OAuth metadata fetch to private/reserved IP: ${authServerMetadataUrl}`, | ||
| ); | ||
| return null; | ||
| } | ||
| const response = await fetch(authServerMetadataUrl); |
There was a problem hiding this comment.
The fetchAuthorizationServerMetadata method is vulnerable to a Time-of-Check to Time-of-Use (TOCTOU) / DNS Rebinding attack. To securely pin the IP address without breaking HTTPS/TLS, keep the original URL intact and use a custom undici Agent with a custom lookup function passed as a dispatcher option to fetch, maintaining consistency with the reference implementation in web-fetch.ts.
const resolvedAddrs = await resolveAndValidateDns(authServerMetadataUrl);
if (resolvedAddrs.length === 0) {
debugLogger.debug(
"Blocked OAuth metadata fetch to private/reserved IP: " + authServerMetadataUrl,
);
return null;
}
const pinnedIp = resolvedAddrs[0];
const dispatcher = new Agent({
connect: {
lookup: (hostname, options, callback) => {
callback(null, [{ address: pinnedIp, family: pinnedIp.includes(':') ? 6 : 4 }]);
},
},
});
const response = await fetch(authServerMetadataUrl, {
dispatcher,
});References
- When a change's primary purpose is to align a component with a reference implementation (e.g., a 'main agent'), prioritize consistency with the reference, even if it means not addressing potential issues in the aligned code.
There was a problem hiding this comment.
Fixed in 04ef846. Same undici Agent dispatcher pattern applied , IP pinned inside the Agent.connect.lookup callback, original URL kept for correct TLS SNI.
| import { | ||
| fetchWithTimeout, | ||
| isLoopbackHost, | ||
| resolveAndValidateDns, | ||
| } from '../utils/fetch.js'; |
There was a problem hiding this comment.
Import Agent from undici to allow creating a custom dispatcher for secure DNS resolution.
| import { | |
| fetchWithTimeout, | |
| isLoopbackHost, | |
| resolveAndValidateDns, | |
| } from '../utils/fetch.js'; | |
| import { | |
| fetchWithTimeout, | |
| isLoopbackHost, | |
| resolveAndValidateDns, | |
| } from '../utils/fetch.js'; | |
| import { Agent } from 'undici'; |
There was a problem hiding this comment.
Fixed in 04ef846. Now imports Agent and Dispatcher from undici.
| const urlWithIp = new URL(url); | ||
| const originalHostname = urlWithIp.hostname; | ||
| urlWithIp.hostname = pinnedIp; | ||
|
|
||
| const response = await retryWithBackoff( | ||
| async () => { | ||
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | ||
| signal, | ||
| headers: { | ||
| 'User-Agent': USER_AGENT, | ||
| const res = await fetchWithTimeout( | ||
| urlWithIp.toString(), | ||
| URL_FETCH_TIMEOUT_MS, | ||
| { | ||
| signal, | ||
| headers: { | ||
| 'User-Agent': USER_AGENT, | ||
| Host: originalHostname, | ||
| }, | ||
| }, | ||
| }); | ||
| ); |
There was a problem hiding this comment.
Replacing the hostname with the resolved IP address in the URL (e.g., https://8.8.8.8/) and setting the Host header will cause HTTPS requests to fail TLS certificate verification in Node.js/undici. This is because the TLS handshake will use the IP address as the SNI servername, resulting in an ERR_TLS_CERT_ALTNAME_INVALID error.
To securely pin the IP address without breaking HTTPS/TLS, keep the original URL intact and use a custom undici Agent with a custom lookup function passed as a dispatcher option to fetch.
| const urlWithIp = new URL(url); | |
| const originalHostname = urlWithIp.hostname; | |
| urlWithIp.hostname = pinnedIp; | |
| const response = await retryWithBackoff( | |
| async () => { | |
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | |
| signal, | |
| headers: { | |
| 'User-Agent': USER_AGENT, | |
| const res = await fetchWithTimeout( | |
| urlWithIp.toString(), | |
| URL_FETCH_TIMEOUT_MS, | |
| { | |
| signal, | |
| headers: { | |
| 'User-Agent': USER_AGENT, | |
| Host: originalHostname, | |
| }, | |
| }, | |
| }); | |
| ); | |
| const dispatcher = new Agent({ | |
| connect: { | |
| lookup: (hostname, options, callback) => { | |
| callback(null, [{ address: pinnedIp, family: pinnedIp.includes(':') ? 6 : 4 }]); | |
| }, | |
| }, | |
| }); | |
| const response = await retryWithBackoff( | |
| async () => { | |
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | |
| signal, | |
| dispatcher, | |
| headers: { | |
| 'User-Agent': USER_AGENT, | |
| }, | |
| }); |
There was a problem hiding this comment.
Fixed in 04ef846. Removed hostname substitution entirely. Now creates an undici Agent with a custom lookup function that returns the pre-resolved IP, then passes it as dispatcher to fetchWithTimeout. The original URL is never modified, so TLS certificate verification works correctly.
| const urlWithIp = new URL(url); | ||
| const originalHostname = urlWithIp.hostname; | ||
| urlWithIp.hostname = pinnedIp; | ||
|
|
||
| const response = await retryWithBackoff( | ||
| async () => { | ||
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | ||
| signal, | ||
| headers: { | ||
| Accept: | ||
| 'text/markdown, text/plain;q=0.9, application/json;q=0.9, text/html;q=0.8, application/pdf;q=0.7, video/*;q=0.7, */*;q=0.5', | ||
| 'User-Agent': USER_AGENT, | ||
| const res = await fetchWithTimeout( | ||
| urlWithIp.toString(), | ||
| URL_FETCH_TIMEOUT_MS, | ||
| { | ||
| signal, | ||
| headers: { | ||
| Accept: | ||
| 'text/markdown, text/plain;q=0.9, application/json;q=0.9, text/html;q=0.8, application/pdf;q=0.7, video/*;q=0.7, */*;q=0.5', | ||
| 'User-Agent': USER_AGENT, | ||
| Host: originalHostname, | ||
| }, | ||
| }, | ||
| }); | ||
| ); | ||
| return res; | ||
| }, | ||
| { |
There was a problem hiding this comment.
Apply the same custom undici Agent dispatcher pattern here to prevent TLS certificate verification failures on HTTPS URLs while securely pinning the resolved IP address.
| const urlWithIp = new URL(url); | |
| const originalHostname = urlWithIp.hostname; | |
| urlWithIp.hostname = pinnedIp; | |
| const response = await retryWithBackoff( | |
| async () => { | |
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | |
| signal, | |
| headers: { | |
| Accept: | |
| 'text/markdown, text/plain;q=0.9, application/json;q=0.9, text/html;q=0.8, application/pdf;q=0.7, video/*;q=0.7, */*;q=0.5', | |
| 'User-Agent': USER_AGENT, | |
| const res = await fetchWithTimeout( | |
| urlWithIp.toString(), | |
| URL_FETCH_TIMEOUT_MS, | |
| { | |
| signal, | |
| headers: { | |
| Accept: | |
| 'text/markdown, text/plain;q=0.9, application/json;q=0.9, text/html;q=0.8, application/pdf;q=0.7, video/*;q=0.7, */*;q=0.5', | |
| 'User-Agent': USER_AGENT, | |
| Host: originalHostname, | |
| }, | |
| }, | |
| }); | |
| ); | |
| return res; | |
| }, | |
| { | |
| const dispatcher = new Agent({ | |
| connect: { | |
| lookup: (hostname, options, callback) => { | |
| callback(null, [{ address: pinnedIp, family: pinnedIp.includes(':') ? 6 : 4 }]); | |
| }, | |
| }, | |
| }); | |
| const response = await retryWithBackoff( | |
| async () => { | |
| const res = await fetchWithTimeout(url, URL_FETCH_TIMEOUT_MS, { | |
| signal, | |
| dispatcher, | |
| headers: { | |
| Accept: | |
| 'text/markdown, text/plain;q=0.9, application/json;q=0.9, text/html;q=0.8, application/pdf;q=0.7, video/*;q=0.7, */*;q=0.5', | |
| 'User-Agent': USER_AGENT, | |
| }, | |
| }); |
There was a problem hiding this comment.
Fixed in 04ef846. Same approach , undici Agent dispatcher with custom lookup, original URL preserved.
| import { coreEvents } from '../utils/events.js'; | ||
| import { debugLogger } from '../utils/debugLogger.js'; | ||
| import { getConsentForOauth } from '../utils/authConsent.js'; | ||
| import { isLoopbackHost, resolveAndValidateDns } from '../utils/fetch.js'; |
There was a problem hiding this comment.
Import Agent from undici to support secure IP pinning via a custom dispatcher.
import { isLoopbackHost, resolveAndValidateDns } from '../utils/fetch.js';
import { Agent } from 'undici';There was a problem hiding this comment.
Fixed in 04ef846. Imports Agent and Dispatcher from undici, used in registerClient.
| import { | ||
| isLoopbackHost, | ||
| resolveAndValidateDns, | ||
| } from '../utils/fetch.js'; |
There was a problem hiding this comment.
Import Agent from undici to support secure IP pinning via a custom dispatcher.
| import { | |
| isLoopbackHost, | |
| resolveAndValidateDns, | |
| } from '../utils/fetch.js'; | |
| import { | |
| isLoopbackHost, | |
| resolveAndValidateDns, | |
| } from '../utils/fetch.js'; | |
| import { Agent } from 'undici'; |
There was a problem hiding this comment.
Fixed in 04ef846. Imports Agent and Dispatcher from undici.
| expect(fetchUtils.fetchWithTimeout).toHaveBeenCalledWith( | ||
| 'https://example.com/', | ||
| 'https://8.8.8.8/', | ||
| expect.any(Number), | ||
| expect.objectContaining({ | ||
| headers: expect.objectContaining({ |
There was a problem hiding this comment.
Update the test assertion to match the new custom dispatcher pattern, where the original URL is passed to fetchWithTimeout instead of the IP-replaced URL.
| expect(fetchUtils.fetchWithTimeout).toHaveBeenCalledWith( | |
| 'https://example.com/', | |
| 'https://8.8.8.8/', | |
| expect.any(Number), | |
| expect.objectContaining({ | |
| headers: expect.objectContaining({ | |
| expect(fetchUtils.fetchWithTimeout).toHaveBeenCalledWith( | |
| 'https://example.com/', | |
| expect.any(Number), | |
| expect.objectContaining({ | |
| dispatcher: expect.any(Object), | |
| headers: expect.objectContaining({ |
There was a problem hiding this comment.
Fixed in 04ef846. Updated assertion to expect the original URL ('https://example.com/') and objectContaining({ dispatcher: expect.any(Object) }), consistent with the new undici Agent pattern. Also simplified the mockFetch helper to no longer construct the pinned IP URL since that substitution no longer happens.
herdiyana256
force-pushed
the
fix/ssrf-dns-hostname-private-ip-bypass
branch
from
a37e073 to
6b2f24c
Compare
gemini-cli
Bot
added
the
status/need-issue
herdiyana256
force-pushed
the
fix/ssrf-dns-hostname-private-ip-bypass
branch
from
6b2f24c to
f6fe405
Compare
The OAuth discovery flow in oauth-utils.ts and oauth-provider.ts fetches URLs received directly from external MCP server responses without any SSRF validation, unlike web-fetch.ts which already uses isLoopbackHost() and resolveAndValidateDns() from utils/fetch.ts. A malicious MCP server can return private/internal IPs in: - WWW-Authenticate: Bearer resource_metadata="http://169.254.169.254/..." - /.well-known/oauth-protected-resource: {authorization_servers: ["http://169.254.169.254"]} - /.well-known/oauth-authorization-server: {registration_endpoint: "http://10.x.x.x/admin"} This causes gemini-cli to make GET and POST requests to cloud IMDS endpoints (AWS 169.254.169.254, GCP metadata.google.internal, Azure 169.254.169.254) or internal services on the developer's network. Fix: apply the same isLoopbackHost() + resolveAndValidateDns() guards already used in web-fetch.ts to: - OAuthUtils.fetchProtectedResourceMetadata() - OAuthUtils.fetchAuthorizationServerMetadata() - MCPOAuthProvider.registerClient()
herdiyana256
force-pushed
the
fix/ssrf-dns-hostname-private-ip-bypass
branch
from
f6fe405 to
04ef846
Compare
1 participant
Summary
The OAuth discovery flow in
oauth-utils.tsandoauth-provider.tsfetches URLsreceived directly from MCP server responses without SSRF validation. This is a
coverage gap relative to
web-fetch.ts, which already usesisLoopbackHost()and
resolveAndValidateDns()fromutils/fetch.ts.Vulnerable paths
Three functions fetch attacker-controlled URLs without any IP validation:
OAuthUtils.fetchProtectedResourceMetadata(resourceMetadataUrl)— URL extracted from the server'sWWW-Authenticate: Bearer resource_metadata="..."headerOAuthUtils.fetchAuthorizationServerMetadata(authServerMetadataUrl)— URL fromauthorization_servers[0]in the server's protected resource metadata responseMCPOAuthProvider.registerClient(registrationUrl)URL fromregistration_endpointin the authorization server metadata responseA malicious MCP server can point any of these at
169.254.169.254,metadata.google.internal, a private RFC1918 address, orlocalhost— causing gemini-cli to make GET and POST requests to cloud IMDS endpoints or internal services on the developer's machine.Fix
Applies the same
isLoopbackHost()+resolveAndValidateDns()guards fromutils/fetch.tsto all three fetch sites. No new logic introduced , reuses the existing SSRF protection utilities already used inweb-fetch.ts.