From 5b77ced09ad346921f49d0e1c00e580e0dfcf4f3 Mon Sep 17 00:00:00 2001 From: Amit Vijapur Date: Tue, 21 Jul 2026 23:11:33 +0800 Subject: [PATCH] fix(validators): reject loopback/private/link-local hosts in IsValidRemoteURL IsValidRemoteURL only compared the hostname against the literal strings "localhost", "127.0.0.1", and "*.localhost", so it missed every other notation for an internal address: IPv6 loopback ([::1]), the rest of 127.0.0.0/8, unspecified addresses (0.0.0.0, [::]), IPv4-mapped IPv6 loopback ([::ffff:127.0.0.1]), and RFC1918/link-local ranges including the 169.254.169.254 cloud metadata endpoint. Parse the hostname with net.ParseIP and reject it if IsLoopback, IsUnspecified, IsPrivate, or IsLinkLocalUnicast is true, so every IP notation for these ranges is caught consistently. DNS names (which can't be classified without a network round trip) still fall back to the existing "localhost" / "*.localhost" string checks. Fixes #1465. --- internal/validators/utils.go | 28 ++++++++++++++-- internal/validators/utils_test.go | 56 +++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 internal/validators/utils_test.go diff --git a/internal/validators/utils.go b/internal/validators/utils.go index 035132dbb..27e0a425f 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 000000000..529ad1185 --- /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) + }) + } +}