oauthex: oauth discovery checks (#1220)

add extra checks for the default oauth discovery client: * disallow redirects which downgrade to http * disallow redirects to localhost or private ip * disallow the initial call to a private ip * proxy configuration opts out of dial time checking
2026-09-02 21:07:19 +02:00 · 2026-09-02 21:07:19 +02:00 · 12cbafe54d
commit 12cbafe54d
parent 3632967404
6 changed files with 376 additions and 31 deletions
--- a/docs/protocol.md
+++ b/docs/protocol.md
@ -639,24 +639,24 @@ provided to

 ### Server-Side Request Forgery

-The [mitigations](https://modelcontextprotocol.io/docs/tutorials/security/security_best_practices#mitigation-3) are as follows:
+The OAuth discovery helpers apply these [mitigations](https://modelcontextprotocol.io/docs/tutorials/security/security_best_practices#mitigation-3) by default:

- _Enforce HTTPS_. The OAuth helpers provided by the SDK reject the `http://` URLs
-except loopback addresses (`localhost`, `127.0.0.1`, `::1`).
+- _Enforce HTTPS_. Reject `http://` URLs (except loopback: `localhost`, `127.0.0.1`, `::1`)
+and redirects that downgrade `https` to a non-`https` scheme.

- _Block Private IP Ranges_. The OAuth helpers provided by the SDK allow passing
-a custom `http.Client`. Developers are advised to customize the client it with
-appropriate network protections, including IP range blocking. The SDK does not provide
-this capability out of the box.
+- _Block Private IP Ranges_. Reject targets that resolve to a non-public address
+(private, link-local including the `169.254.169.254` metadata endpoint, CGNAT, multicast,
+unspecified). The check runs on the initial URL and at dial time, guarding against DNS rebinding.

- _Validate Redirect Targets_. Similarly to previous point, customized `http.Client`
-can be used to validate network hops. The SDK does not provide this capability out
-of the box.
+- _Validate Redirect Targets_. Reject redirects to non-public addresses and cap the redirect count.

- _Use Egress Proxies_. This is out of scope for the SDK and can be configured separately.
+- _Use Egress Proxies_. Out of scope for the SDK; configure separately.

- _DNS Resolution Considerations_. The SDK has DNS rebinding protection on the server side which is enabled by default. For the client side, consider providing
-a custom `http.Client` that would implement DNS pinning.
+**Opting out.** Passing a custom `http.Client` can bypass these checks, making SSRF
+protection your responsibility. Specifically: a custom `Transport.DialContext`/`DialTLSContext`,
+a non-`*http.Transport`, or a configured `Proxy` disables the dial-time IP check; setting
+`CheckRedirect` replaces all redirect validation. In these cases, implement your own IP
+blocking, redirect validation, and DNS pinning.

 ### Session Hijacking

--- a/internal/docs/protocol.src.md
+++ b/internal/docs/protocol.src.md
@ -534,24 +534,24 @@ provided to

 ### Server-Side Request Forgery

-The [mitigations](https://modelcontextprotocol.io/docs/tutorials/security/security_best_practices#mitigation-3) are as follows:
+The OAuth discovery helpers apply these [mitigations](https://modelcontextprotocol.io/docs/tutorials/security/security_best_practices#mitigation-3) by default:

- _Enforce HTTPS_. The OAuth helpers provided by the SDK reject the `http://` URLs
-except loopback addresses (`localhost`, `127.0.0.1`, `::1`).
+- _Enforce HTTPS_. Reject `http://` URLs (except loopback: `localhost`, `127.0.0.1`, `::1`)
+and redirects that downgrade `https` to a non-`https` scheme.

- _Block Private IP Ranges_. The OAuth helpers provided by the SDK allow passing
-a custom `http.Client`. Developers are advised to customize the client it with
-appropriate network protections, including IP range blocking. The SDK does not provide
-this capability out of the box.
+- _Block Private IP Ranges_. Reject targets that resolve to a non-public address
+(private, link-local including the `169.254.169.254` metadata endpoint, CGNAT, multicast,
+unspecified). The check runs on the initial URL and at dial time, guarding against DNS rebinding.

- _Validate Redirect Targets_. Similarly to previous point, customized `http.Client`
-can be used to validate network hops. The SDK does not provide this capability out
-of the box.
+- _Validate Redirect Targets_. Reject redirects to non-public addresses and cap the redirect count.

- _Use Egress Proxies_. This is out of scope for the SDK and can be configured separately.
+- _Use Egress Proxies_. Out of scope for the SDK; configure separately.

- _DNS Resolution Considerations_. The SDK has DNS rebinding protection on the server side which is enabled by default. For the client side, consider providing
-a custom `http.Client` that would implement DNS pinning.
+**Opting out.** Passing a custom `http.Client` can bypass these checks, making SSRF
+protection your responsibility. Specifically: a custom `Transport.DialContext`/`DialTLSContext`,
+a non-`*http.Transport`, or a configured `Proxy` disables the dial-time IP check; setting
+`CheckRedirect` replaces all redirect validation. In these cases, implement your own IP
+blocking, redirect validation, and DNS pinning.

 ### Session Hijacking

--- a/internal/util/net.go
+++ b/internal/util/net.go
@ -24,3 +24,26 @@ func IsLoopback(addr string) bool {
 	}
 	return ip.IsLoopback()
 }
+
+// cgnat is the RFC 6598 carrier-grade NAT range (100.64.0.0/10). netip's
+// IsPrivate does not report it, but it is shared ISP space that is not publicly
+// routable and could point at another tenant behind the same NAT.
+var cgnat = netip.MustParsePrefix("100.64.0.0/10")
+
+// IsPrivateOrReserved reports whether ip belongs to a range that must not be
+// reachable through a server-controlled URL, as a defense against SSRF: private
+// (RFC 1918 / RFC 4193 ULA), link-local (including the 169.254.169.254 cloud
+// metadata endpoint), carrier-grade NAT, multicast, and the unspecified address.
+func IsPrivateOrReserved(ip netip.Addr) bool {
+	ip = ip.Unmap()
+	if !ip.IsValid() {
+		return true
+	}
+	if ip.IsPrivate() ||
+		ip.IsLinkLocalUnicast() ||
+		ip.IsMulticast() ||
+		ip.IsUnspecified() {
+		return true
+	}
+	return cgnat.Contains(ip)
+}
--- a/internal/util/net_test.go
+++ b/internal/util/net_test.go
@ -3,7 +3,10 @@
 // that can be found in the LICENSE file.
 package util

-import "testing"
+import (
+	"net/netip"
+	"testing"
+)

 // TestIsLoopback tests the IsLoopback helper function.
 func TestIsLoopback(t *testing.T) {
@ -33,3 +36,52 @@ func TestIsLoopback(t *testing.T) {
 		})
 	}
 }
+
+func TestIsPrivateOrReserved(t *testing.T) {
+	tests := []struct {
+		ip   string
+		want bool
+	}{
+		// Private (RFC 1918) and IPv6 ULA (RFC 4193).
+		{"10.0.0.1", true},
+		{"172.16.5.4", true},
+		{"192.168.1.1", true},
+		{"fd00::1", true},
+		// Link-local, including the cloud metadata endpoint.
+		{"169.254.169.254", true},
+		{"169.254.0.1", true},
+		{"fe80::1", true},
+		// Carrier-grade NAT (RFC 6598).
+		{"100.64.0.1", true},
+		{"100.127.255.255", true},
+		// Multicast and unspecified.
+		{"224.0.0.1", true},
+		{"ff02::1", true},
+		{"0.0.0.0", true},
+		{"::", true},
+		// IPv4-mapped IPv6 of a private address must also be caught.
+		{"::ffff:10.0.0.1", true},
+		{"::ffff:192.168.0.1", true},
+		// Loopback is intentionally NOT reserved here (policy handled elsewhere).
+		{"127.0.0.1", false},
+		{"::1", false},
+		// Public addresses are allowed.
+		{"8.8.8.8", false},
+		{"1.1.1.1", false},
+		{"2606:4700:4700::1111", false},
+		// Just outside CGNAT.
+		{"100.63.255.255", false},
+		{"100.128.0.0", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.ip, func(t *testing.T) {
+			ip, err := netip.ParseAddr(tt.ip)
+			if err != nil {
+				t.Fatalf("ParseAddr(%q): %v", tt.ip, err)
+			}
+			if got := IsPrivateOrReserved(ip); got != tt.want {
+				t.Errorf("IsPrivateOrReserved(%q) = %v, want %v", tt.ip, got, tt.want)
+			}
+		})
+	}
+}
--- a/oauthex/discovery_test.go
+++ b/oauthex/discovery_test.go
@ -0,0 +1,158 @@
+// Copyright 2025 The Go MCP SDK Authors. All rights reserved.
+// Use of this source code is governed by an MIT-style
+// license that can be found in the LICENSE file.
+
+package oauthex
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+)
+
+func TestCheckHTTPSOrLoopback(t *testing.T) {
+	tests := []struct {
+		name    string
+		addr    string
+		wantErr bool
+	}{
+		{name: "empty", addr: "", wantErr: false},
+		{name: "https dns allowed", addr: "https://example.com/prm", wantErr: false},
+		{name: "cleartext dns disallowed", addr: "http://example.com/prm", wantErr: true},
+		{name: "https loopback ip allowed", addr: "http://127.0.0.1:8080/prm", wantErr: false},
+		{name: "http localhost allowed", addr: "http://localhost/prm", wantErr: false},
+		{name: "https private ip disallowed", addr: "https://10.0.0.1/prm", wantErr: true},
+		{name: "link local metadata disallowed", addr: "https://169.254.169.254/", wantErr: true},
+		{name: "mapped private address disallowed", addr: "https://[::ffff:10.0.0.1]/prm", wantErr: true},
+		{name: "https public ip allowed", addr: "https://8.8.8.8/prm", wantErr: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := checkHTTPSOrLoopback(tt.addr)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("checkHTTPSOrLoopback(%q) error = %v, wantErr %v", tt.addr, err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestDiscoveryClientRedirect(t *testing.T) {
+	newRequestTo := func(u string) *http.Request {
+		t.Helper()
+		r, err := http.NewRequest(http.MethodGet, u, nil)
+		if err != nil {
+			t.Fatalf("NewRequest(%q): %v", u, err)
+		}
+		return r
+	}
+	testCases := []struct {
+		name        string
+		custom      *http.Client
+		req         *http.Request
+		via         []*http.Request
+		wantBlocked bool
+	}{
+		{
+			name:        "redirect to public allowed",
+			req:         newRequestTo("https://8.8.8.8/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: false,
+		},
+		{
+			name:        "downgrade blocked",
+			req:         newRequestTo("http://93.184.216.34/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: true,
+		},
+		{
+			name:        "redirect to loopback blocked",
+			req:         newRequestTo("https://127.0.0.1/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: true,
+		},
+		{
+			name:        "redirect to hostname resolving to loopback blocked",
+			req:         newRequestTo("https://localhost/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: true,
+		},
+		{
+			name:        "custom client with no custom check",
+			custom:      &http.Client{},
+			req:         newRequestTo("https://127.0.0.1/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: true,
+		},
+		{
+			name: "custom client check disable",
+			custom: &http.Client{
+				CheckRedirect: func(req *http.Request, via []*http.Request) error {
+					return nil
+				},
+			},
+			req:         newRequestTo("https://127.0.0.1/"),
+			via:         []*http.Request{newRequestTo("https://93.184.216.34/")},
+			wantBlocked: false,
+		},
+		{
+			name: "too many redirects",
+			req:  newRequestTo("https://8.8.8.8/"),
+			via: func() []*http.Request {
+				via := make([]*http.Request, maxDiscoveryRedirects)
+				for i := range via {
+					via[i] = newRequestTo("https://8.8.8.8/")
+				}
+				return via
+			}(),
+			wantBlocked: true,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			client := newDiscoveryClient(tc.custom)
+			gotErr := client.CheckRedirect(tc.req, tc.via)
+			if gotErr != nil && !tc.wantBlocked {
+				t.Fatalf("client.CheckRedirect() error = %v", gotErr)
+			}
+			if gotErr == nil && tc.wantBlocked {
+				t.Fatal("client.CheckRedirect() error = nil, want blocked")
+			}
+		})
+	}
+}
+
+func TestDiscoveryBlocksInternalTarget(t *testing.T) {
+	ctx := t.Context()
+	for _, target := range []string{
+		"https://169.254.169.254/latest/meta-data/",
+		"https://10.0.0.1/.well-known/oauth-protected-resource",
+	} {
+		t.Run(target, func(t *testing.T) {
+			_, err := GetProtectedResourceMetadata(ctx, target, target, nil)
+			if err == nil {
+				t.Fatalf("GetProtectedResourceMetadata(%q) succeeded, want SSRF rejection", target)
+			}
+			if !strings.Contains(err.Error(), "non-public") {
+				t.Errorf("error = %v, want it to mention a non-public address", err)
+			}
+		})
+	}
+}
+
+func TestDiscoveryAllowsLoopbackInitial(t *testing.T) {
+	ctx := t.Context()
+	h := &fakeResourceHandler{}
+	server := httptest.NewTLSServer(h)
+	defer server.Close()
+	h.installHandlers(server.URL)
+
+	metadataURL := server.URL + "/.well-known/oauth-protected-resource"
+	prm, err := GetProtectedResourceMetadata(ctx, metadataURL, server.URL, server.Client())
+	if err != nil {
+		t.Fatalf("GetProtectedResourceMetadata on loopback failed: %v", err)
+	}
+	if prm == nil {
+		t.Fatal("nil prm")
+	}
+}
--- a/oauthex/oauth2.go
+++ b/oauthex/oauth2.go
@ -11,14 +11,22 @@ import (
 	"fmt"
 	"io"
 	"mime"
+	"net"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"strings"
+	"syscall"
+	"time"

 	"github.com/modelcontextprotocol/go-sdk/internal/json"
 	"github.com/modelcontextprotocol/go-sdk/internal/util"
 )

+const maxDiscoveryRedirects = 10
+
+var defaultDiscoveryTransport = newDiscoveryTransport(http.DefaultTransport)
+
 type httpStatusError struct {
 	StatusCode int
 }
@ -35,10 +43,7 @@ func getJSON[T any](ctx context.Context, c *http.Client, url string, limit int64
 	if err != nil {
 		return nil, err
 	}
-	if c == nil {
-		c = http.DefaultClient
-	}
-	res, err := c.Do(req)
+	res, err := newDiscoveryClient(c).Do(req)
 	if err != nil {
 		return nil, err
 	}
@ -91,5 +96,112 @@ func checkHTTPSOrLoopback(addr string) error {
 	if !util.IsLoopback(u.Host) && u.Scheme != "https" {
 		return fmt.Errorf("URL %q does not use HTTPS or is not a loopback address", addr)
 	}
+	if ip, err := netip.ParseAddr(u.Hostname()); err == nil && util.IsPrivateOrReserved(ip) {
+		return fmt.Errorf("URL %q refers to a non-public address", addr)
+	}
+	return nil
+}
+
+func newDiscoveryClient(c *http.Client) *http.Client {
+	transport := defaultDiscoveryTransport
+	if c != nil && c.Transport != nil {
+		// a caller that has taken over dialing or TLS opts out of the checks
+		if t, ok := c.Transport.(*http.Transport); ok && t.DialContext == nil && t.DialTLSContext == nil {
+			transport = newDiscoveryTransport(t)
+		} else {
+			transport = c.Transport
+		}
+	}
+	discoveryClient := &http.Client{
+		Transport: transport,
+
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			if c != nil && c.CheckRedirect != nil {
+				return c.CheckRedirect(req, via)
+			}
+			if len(via) >= maxDiscoveryRedirects {
+				return fmt.Errorf("max redirects exceeded (%d)", maxDiscoveryRedirects)
+			}
+			if prev := via[len(via)-1]; prev.URL.Scheme == "https" && req.URL.Scheme != "https" {
+				return fmt.Errorf("redirect to non-HTTPS scheme %q is a security downgrade", req.URL.Scheme)
+			}
+			return checkLoopbackRedirect(req.Context(), req.URL.Hostname())
+		},
+	}
+	if c != nil {
+		discoveryClient.Timeout = c.Timeout
+		discoveryClient.Jar = c.Jar
+	}
+	return discoveryClient
+}
+
+// newDiscoveryTransport creates an http.RoundTripper with extra protections enabled,
+// unless the provided RoundTripper is not *http.Transport.
+func newDiscoveryTransport(rt http.RoundTripper) http.RoundTripper {
+	base, ok := rt.(*http.Transport)
+	if !ok {
+		return rt
+	}
+	// If a proxy is configured Control checks should be disabled, because they will
+	// basically be running for proxy dials.
+	if proxyConfigured(base) {
+		return rt
+	}
+	result := base.Clone()
+	result.DialContext = (&net.Dialer{
+		Timeout:   30 * time.Second,
+		KeepAlive: 30 * time.Second,
+		Control: func(_, address string, _ syscall.RawConn) error {
+			host, _, err := net.SplitHostPort(address)
+			if err != nil {
+				host = address
+			}
+			ip, err := netip.ParseAddr(host)
+			if err != nil {
+				return fmt.Errorf("non-ip address: %w", err)
+			}
+			if util.IsPrivateOrReserved(ip) {
+				return fmt.Errorf("non-public ip address: %q", ip)
+			}
+			return nil
+		},
+	}).DialContext
+	return result
+}
+
+func proxyConfigured(t *http.Transport) bool {
+	if t.Proxy == nil {
+		return false
+	}
+	req, err := http.NewRequest(http.MethodGet, "https://check-proxy-existence.com", nil)
+	if err != nil {
+		return false
+	}
+	u, err := t.Proxy(req)
+	return err == nil && u != nil
+}
+
+func checkLoopbackRedirect(ctx context.Context, host string) error {
+	doCheck := func(ip netip.Addr) error {
+		if ip.Unmap().IsLoopback() {
+			return fmt.Errorf("redirect into loopback address %q", host)
+		}
+		if util.IsPrivateOrReserved(ip) {
+			return fmt.Errorf("redirect into non-public address %q", host)
+		}
+		return nil
+	}
+	if ip, err := netip.ParseAddr(host); err == nil {
+		return doCheck(ip)
+	}
+	ips, err := net.DefaultResolver.LookupNetIP(ctx, "ip", host)
+	if err != nil {
+		return fmt.Errorf("cannot resolve redirect host %q: %w", host, err)
+	}
+	for _, ip := range ips {
+		if err := doCheck(ip); err != nil {
+			return err
+		}
+	}
 	return nil
 }