diff --git a/internal/validators/utils.go b/internal/validators/utils.go index 035132db..27e0a425 100644 --- a/internal/validators/utils.go +++ b/internal/validators/utils.go @@ -1,6 +1,7 @@ package validators import ( + "net" "net/url" "regexp" "strings" @@ -147,9 +148,10 @@ func IsValidRemoteURL(rawURL string) bool { return false } - // Reject localhost URLs for remotes (security/production concerns) - hostname := u.Hostname() - if hostname == "localhost" || hostname == "127.0.0.1" || strings.HasSuffix(hostname, ".localhost") { + // Reject localhost/loopback/unspecified/private/link-local hosts for remotes + // (security/production concerns - a remote must point at a real, publicly + // reachable, non-internal endpoint). + if isDisallowedRemoteHost(u.Hostname()) { return false } @@ -160,6 +162,26 @@ func IsValidRemoteURL(rawURL string) bool { return true } +// isDisallowedRemoteHost reports whether hostname is not allowed as a remote +// URL host because it is loopback, unspecified, private, or link-local. +// +// hostname may be an IP literal - including bracketed IPv6 forms already +// stripped of their brackets by url.URL.Hostname (e.g. "::1"), and +// IPv4-mapped IPv6 forms (e.g. "::ffff:127.0.0.1") - or a DNS name. IP +// literals are checked with net.ParseIP and net.IP's classification methods +// so every notation for loopback/private/link-local addresses is caught, not +// just the literal strings "127.0.0.1" and "::1" the previous check used. +// DNS names fall back to the pre-existing "localhost" / "*.localhost" string +// checks, since resolving them here would require a network round trip +// during validation. +func isDisallowedRemoteHost(hostname string) bool { + if ip := net.ParseIP(hostname); ip != nil { + return ip.IsLoopback() || ip.IsUnspecified() || ip.IsPrivate() || ip.IsLinkLocalUnicast() + } + + return hostname == "localhost" || strings.HasSuffix(hostname, ".localhost") +} + // IsValidTemplatedURL validates a URL with template variables against available variables // For packages: validates that template variables reference package arguments or environment variables // For remotes: validates that template variables reference the transport's variables map diff --git a/internal/validators/utils_test.go b/internal/validators/utils_test.go new file mode 100644 index 00000000..529ad118 --- /dev/null +++ b/internal/validators/utils_test.go @@ -0,0 +1,56 @@ +package validators_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/modelcontextprotocol/registry/internal/validators" +) + +// TestIsValidRemoteURL_LoopbackAndPrivateAddresses exercises IsValidRemoteURL +// directly against every bypass listed in the GitHub issue: the previous +// implementation only rejected the literal strings "localhost", "127.0.0.1", +// and "*.localhost", so IPv6 loopback, the rest of 127.0.0.0/8, unspecified +// addresses, IPv4-mapped loopback, and RFC1918/link-local addresses all +// passed validation despite the "no localhost allowed" contract. +func TestIsValidRemoteURL_LoopbackAndPrivateAddresses(t *testing.T) { + tests := []struct { + name string + url string + valid bool + }{ + // Previously-caught cases - must keep working + {name: "literal localhost", url: "https://localhost/", valid: false}, + {name: "literal localhost with port", url: "https://localhost:8443/mcp", valid: false}, + {name: "localhost subdomain", url: "https://foo.localhost/", valid: false}, + {name: "literal 127.0.0.1", url: "https://127.0.0.1/", valid: false}, + + // Previously-missed bypasses called out in the issue + {name: "IPv6 loopback", url: "https://[::1]/", valid: false}, + {name: "rest of 127.0.0.0/8", url: "https://127.0.0.2/", valid: false}, + {name: "127.0.0.0/8 upper range", url: "https://127.255.255.254/", valid: false}, + {name: "IPv4 unspecified", url: "https://0.0.0.0/", valid: false}, + {name: "IPv6 unspecified", url: "https://[::]/", valid: false}, + {name: "IPv4-mapped IPv6 loopback", url: "https://[::ffff:127.0.0.1]/", valid: false}, + {name: "RFC1918 10.0.0.0/8", url: "https://10.0.0.1/", valid: false}, + {name: "RFC1918 172.16.0.0/12", url: "https://172.16.0.1/", valid: false}, + {name: "RFC1918 192.168.0.0/16", url: "https://192.168.1.1/", valid: false}, + {name: "IPv4 link-local (cloud metadata endpoint)", url: "https://169.254.169.254/", valid: false}, + {name: "IPv6 unique local address", url: "https://[fc00::1]/", valid: false}, + {name: "IPv6 link-local unicast", url: "https://[fe80::1]/", valid: false}, + + // Sanity checks - must not overblock legitimate remotes + {name: "public hostname", url: "https://example.com/mcp", valid: true}, + {name: "public IPv4 address", url: "https://8.8.8.8/mcp", valid: true}, + {name: "public IPv6 address", url: "https://[2001:4860:4860::8888]/mcp", valid: true}, + {name: "http scheme still rejected regardless of host", url: "http://example.com/mcp", valid: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := validators.IsValidRemoteURL(tt.url) + assert.Equal(t, tt.valid, got, "IsValidRemoteURL(%q)", tt.url) + }) + } +}