Conversation
Add URL validation to fetchJSON() to block requests to private/internal IP ranges and require HTTPS. A malicious external MCP server could previously craft WWW-Authenticate headers pointing to internal network addresses (e.g., cloud metadata endpoints at 169.254.169.254), causing the Gram server to make arbitrary requests from its trusted network position.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 SSRF bypass via HTTP redirects: validateFetchURL only checks the initial URL, not redirect targets
The SSRF protection in validateFetchURL validates the initial URL's scheme and resolved IP, but the HTTP client created at oauthdiscovery.go:312 (retryablehttp.NewClient().StandardClient()) follows redirects by default (Go's net/http.Client follows up to 10 redirects when CheckRedirect is nil). A malicious external MCP server could host a public HTTPS endpoint that returns a 302 redirect to an internal address like http://169.254.169.254/latest/meta-data/ (cloud metadata) or any other private IP. The validateFetchURL check passes because the initial hostname resolves to a public IP, but the HTTP client then follows the redirect to the internal target, completely bypassing the SSRF protection.
Attack scenario
- Attacker registers an MCP server at
https://evil.com https://evil.com/.well-known/oauth-authorization-serverreturns302 Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/validateFetchURL("https://evil.com/...")passes —evil.comresolves to a public IP- Go HTTP client follows the redirect to the internal metadata endpoint
- Response is parsed as JSON and returned through the OAuth metadata fields
(Refers to line 312)
Prompt for agents
In server/internal/externalmcp/oauthdiscovery.go, the HTTP client at line 312 follows redirects by default, bypassing the SSRF validation. Fix by setting CheckRedirect on the standard client returned by retryablehttp to validate each redirect URL. After line 312 (client := retryablehttp.NewClient().StandardClient()), add:
client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
if err := validateFetchURL(req.URL.String()); err != nil {
return fmt.Errorf("redirect blocked: %w", err)
}
if len(via) >= 10 {
return fmt.Errorf("too many redirects")
}
return nil
}
This ensures every redirect target is also validated against the same SSRF checks (HTTPS scheme and non-private IP).
Was this helpful? React with 👍 or 👎 to provide feedback.
| if err := validateFetchURL(fetchURL); err != nil { | ||
| return nil, fmt.Errorf("URL validation failed: %w", err) | ||
| } |
There was a problem hiding this comment.
🔴 TOCTOU DNS rebinding allows SSRF bypass between validateFetchURL and actual HTTP request
The validateFetchURL function at oauthdiscovery.go:278 performs a DNS lookup via net.LookupHost and checks the resolved IPs against private ranges. However, the actual HTTP request at oauthdiscovery.go:313 performs its own independent DNS resolution. An attacker with a DNS server configured with a short TTL can serve a public IP during the validateFetchURL check, then switch to a private/internal IP (e.g., 169.254.169.254) before the HTTP client resolves the name — a classic DNS rebinding attack. While OS-level DNS caching can mitigate this in many cases, it is not guaranteed (especially with TTL=0 records). The robust fix is to pin the resolved IP and use it for the actual connection via a custom DialContext on the transport.
Prompt for agents
In server/internal/externalmcp/oauthdiscovery.go, the DNS resolution in validateFetchURL (line 278) is separate from the DNS resolution done by the HTTP client (line 312-313). To prevent DNS rebinding, refactor so that the resolved IP from validation is pinned and used for the actual connection. One approach:
1. Have validateFetchURL return the validated IP addresses alongside the nil error.
2. Create a custom http.Transport with a DialContext that dials the pre-resolved IP instead of re-resolving DNS:
transport := &http.Transport{
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
// Replace the hostname with the pre-validated IP
_, port, _ := net.SplitHostPort(addr)
return (&net.Dialer{}).DialContext(ctx, network, net.JoinHostPort(validatedIP, port))
},
}
3. Set this transport on the HTTP client used in fetchJSON.
This ensures the same IP that was validated is the one actually connected to.
Was this helpful? React with 👍 or 👎 to provide feedback.
| func TestValidateFetchURL(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| url string | ||
| wantErr bool | ||
| }{ | ||
| {"http scheme rejected", "http://example.com/foo", true}, | ||
| {"private IP via localhost", "https://localhost/foo", true}, | ||
| {"private IP via 127.0.0.1", "https://127.0.0.1/foo", true}, | ||
| {"metadata endpoint", "https://169.254.169.254/latest/meta-data/", true}, | ||
| {"no scheme", "example.com/foo", true}, | ||
| {"valid public https", "https://accounts.google.com/.well-known/openid-configuration", false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := validateFetchURL(tt.url) | ||
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("validateFetchURL(%q) error = %v, wantErr %v", tt.url, err, tt.wantErr) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 Test coverage for validateFetchURL depends on live DNS resolution
The TestValidateFetchURL tests at oauthdiscovery_test.go:39-61 make live DNS queries (e.g., resolving localhost, 127.0.0.1, accounts.google.com). These tests will fail in air-gapped CI environments or when DNS is unavailable. The localhost test also depends on the system's /etc/hosts configuration resolving to 127.0.0.1. Consider adding a note or using a test helper that mocks net.LookupHost for more reliable CI behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
fetchJSON()used by OAuth metadata discovery for external MCP serversWWW-Authenticateheaders with URLs pointing to internal networks (e.g., AWS metadata at169.254.169.254), causing the Gram server to make requests from its trusted network positionvalidateFetchURL()that enforces HTTPS and blocks private/internal IP ranges (loopback, RFC 1918, link-local, cloud metadata) by resolving DNS before connectingTest plan
isPrivateIPcovering all private ranges and public IPsvalidateFetchURLcovering HTTP rejection, localhost, metadata endpoint, missing scheme, and valid HTTPS