Skip to content

fix: prevent SSRF in OAuth metadata discovery #2035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mfbx9da4 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: prevent SSRF in OAuth metadata discovery #2035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 65 additions & 2 deletions server/internal/externalmcp/oauthdiscovery.go
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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
  1. Attacker registers an MCP server at https://evil.com
  2. https://evil.com/.well-known/oauth-authorization-server returns 302 Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/
  3. validateFetchURL("https://evil.com/...") passes — evil.com resolves to a public IP
  4. Go HTTP client follows the redirect to the internal metadata endpoint
  5. 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).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"log/slog"
"net"
"net/http"
"net/url"
"regexp"
Expand Down Expand Up @@ -235,12 +236,74 @@ func buildWellKnownURL(baseURL string) string {
return baseURL + "/.well-known/oauth-authorization-server"
}

// isPrivateIP returns true if the given IP is in a private, loopback, or link-local range.
func isPrivateIP(ip net.IP) bool {
privateRanges := []string{
"127.0.0.0/8", // IPv4 loopback
"10.0.0.0/8", // RFC 1918
"172.16.0.0/12", // RFC 1918
"192.168.0.0/16", // RFC 1918
"169.254.0.0/16", // Link-local / cloud metadata
"0.0.0.0/8", // Current network
"::1/128", // IPv6 loopback
"fc00::/7", // IPv6 unique local
"fe80::/10", // IPv6 link-local
}
for _, cidr := range privateRanges {
_, network, err := net.ParseCIDR(cidr)
if err != nil {
continue
}
if network.Contains(ip) {
return true
}
}
return false
}

// validateFetchURL checks that a URL is safe to fetch: it must use HTTPS and must not
// resolve to a private/internal IP address. This prevents SSRF attacks where a malicious
// external MCP server could trick the Gram server into making requests to internal services.
func validateFetchURL(rawURL string) error {
u, err := url.Parse(rawURL)
if err != nil {
return fmt.Errorf("invalid URL: %w", err)
}

if u.Scheme != "https" {
return fmt.Errorf("URL scheme must be https, got %q", u.Scheme)
}

hostname := u.Hostname()
ips, err := net.LookupHost(hostname)
if err != nil {
return fmt.Errorf("DNS lookup failed for %q: %w", hostname, err)
}

for _, ipStr := range ips {
ip := net.ParseIP(ipStr)
if ip == nil {
return fmt.Errorf("could not parse resolved IP %q", ipStr)
}
if isPrivateIP(ip) {
return fmt.Errorf("URL %q resolves to private/internal IP %s", rawURL, ipStr)
}
}

return nil
}

// fetchJSON fetches JSON from a URL and decodes it into the target.
func fetchJSON[T any](ctx context.Context, logger *slog.Logger, url string) (*T, error) {
// The URL is validated before fetching to prevent SSRF attacks.
func fetchJSON[T any](ctx context.Context, logger *slog.Logger, fetchURL string) (*T, error) {
if err := validateFetchURL(fetchURL); err != nil {
return nil, fmt.Errorf("URL validation failed: %w", err)
}
Comment on lines +299 to +301
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
defer cancel()

req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, fetchURL, nil)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
Expand Down
61 changes: 61 additions & 0 deletions server/internal/externalmcp/oauthdiscovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package externalmcp

import (
"net"
"testing"
)

func TestIsPrivateIP(t *testing.T) {
tests := []struct {
ip string
private bool
}{
{"127.0.0.1", true},
{"10.0.0.1", true},
{"172.16.0.1", true},
{"192.168.1.1", true},
{"169.254.169.254", true},
{"0.0.0.0", true},
{"::1", true},
{"8.8.8.8", false},
{"1.1.1.1", false},
{"93.184.216.34", false},
}

for _, tt := range tests {
t.Run(tt.ip, func(t *testing.T) {
ip := net.ParseIP(tt.ip)
if ip == nil {
t.Fatalf("failed to parse IP %s", tt.ip)
}
got := isPrivateIP(ip)
if got != tt.private {
t.Errorf("isPrivateIP(%s) = %v, want %v", tt.ip, got, tt.private)
}
})
}
}

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)
}
})
}
}
Comment on lines +39 to +61
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Loading