Skip to content

Commit 1201cbd

Browse files
rdimitrovclaude
rdimitrov
and
claude
authored
security: fix open redirect and add small hardening (#1227)
## What this does Fixes an open redirect plus a handful of small, mostly-invisible hardening changes around it. ## What's in it - **The fix**: trailing-slash redirects no longer let `//evil.com/` redirect off-host. - **SSRF block** on the DNS/HTTP namespace verification — refuses to dial loopback, RFC1918, link-local, multicast, CGNAT, or IP-literal/single-label \"domains\". - **Slow-client guard**: `ReadTimeout` and `IdleTimeout` on the HTTP server (no `WriteTimeout` — would risk cutting off legitimate multi-package publishes). - **Defence-in-depth headers** on the UI (`/`) and 404 responses. - **5xx responses** stop leaking raw DB error text to clients (logged server-side instead). - **Search filter** stops treating `%` and `_` as wildcards. - **Validators**: PyPI/NuGet identifiers URL-escaped before fetching; MCPB no longer follows redirects. - **Misc**: `Version` field bounded to 255 chars; logs quote user input; `BlockedNamespaces` denylist now catches subdomain claimants. Each change is its own commit so they can be reviewed (or reverted) independently. ## Test plan - [ ] CI green - [ ] Synthetic publish in staging for each package type (npm, pypi, nuget, oci, mcpb) — confirms validator changes don't regress - [ ] Quick browser check of `/` after deploy to confirm UI still loads with the new CSP - [ ] `curl https://<host>//evil.com/` — confirm the open redirect is closed 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f7663fe commit 1201cbd

22 files changed

Lines changed: 330 additions & 48 deletions

File tree

‎docs/reference/api/openapi.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -688,6 +688,7 @@ components:
688688
description: "Package version. Must be a specific version. Version ranges are rejected (e.g., '^1.2.3', '~1.2.3', '>=1.2.3', '1.x', '1.*')."
689689
example: "1.0.2"
690690
minLength: 1
691+
maxLength: 255
691692
not:
692693
const: "latest"
693694
fileSha256:

‎docs/reference/server-json/draft/server.schema.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,7 @@
282282
"version": {
283283
"description": "Package version. Must be a specific version. Version ranges are rejected (e.g., '^1.2.3', '~1.2.3', '\u003e=1.2.3', '1.x', '1.*').",
284284
"example": "1.0.2",
285+
"maxLength": 255,
285286
"minLength": 1,
286287
"not": {
287288
"const": "latest"

‎internal/api/handlers/v0/auth/common.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"errors"
1212
"fmt"
1313
"math/big"
14+
"net"
1415
"regexp"
1516
"strings"
1617
"time"
@@ -376,6 +377,18 @@ func IsValidDomain(domain string) bool {
376377
return false
377378
}
378379

380+
// Reject IP literals — this auth method proves domain ownership, not IP
381+
// ownership, and IP literals are an SSRF vector into internal networks.
382+
if net.ParseIP(domain) != nil {
383+
return false
384+
}
385+
386+
// Require at least one dot — rejects single-label names like "localhost"
387+
// or "kubernetes" that resolve only inside private networks.
388+
if !strings.Contains(domain, ".") {
389+
return false
390+
}
391+
379392
// Check for valid characters and structure
380393
domainPattern := regexp.MustCompile(`^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)*$`)
381394
return domainPattern.MatchString(domain)

‎internal/api/handlers/v0/auth/common_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package auth_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/modelcontextprotocol/registry/internal/api/handlers/v0/auth"
7+
)
8+
9+
func TestIsValidDomain(t *testing.T) {
10+
tests := []struct {
11+
domain string
12+
want bool
13+
}{
14+
// Valid
15+
{"example.com", true},
16+
{"sub.example.com", true},
17+
{"a.b.c.d.example.com", true},
18+
{"foo-bar.example.com", true},
19+
{"123.example.com", true},
20+
21+
// Invalid — empty / oversize
22+
{"", false},
23+
24+
// Invalid — IP literals (SSRF vector)
25+
{"127.0.0.1", false},
26+
{"10.0.0.1", false},
27+
{"169.254.169.254", false},
28+
{"::1", false},
29+
{"fe80::1", false},
30+
31+
// Invalid — single-label internal names (SSRF vector)
32+
{"localhost", false},
33+
{"kubernetes", false},
34+
{"internal", false},
35+
36+
// Invalid — bad characters / structure
37+
{"-example.com", false},
38+
{"example.com-", false},
39+
{"exa mple.com", false},
40+
{"example..com", false},
41+
}
42+
for _, tc := range tests {
43+
t.Run(tc.domain, func(t *testing.T) {
44+
if got := auth.IsValidDomain(tc.domain); got != tc.want {
45+
t.Errorf("IsValidDomain(%q) = %v, want %v", tc.domain, got, tc.want)
46+
}
47+
})
48+
}
49+
}

‎internal/api/handlers/v0/auth/dns_test.go‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -280,17 +280,6 @@ func TestDNSAuthHandler_Permissions(t *testing.T) {
280280
"v1.api.example.com/*", // should be reversed
281281
},
282282
},
283-
{
284-
name: "single part domain",
285-
domain: "localhost",
286-
expectedPatterns: []string{
287-
"localhost/*", // exact pattern (no reversal needed)
288-
"localhost.*", // subdomain pattern
289-
},
290-
unexpectedPatterns: []string{
291-
"*.localhost", // wrong wildcard position
292-
},
293-
},
294283
{
295284
name: "hyphenated domain",
296285
domain: "my-app.example-site.com",

‎internal/api/handlers/v0/auth/http.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"net"
78
"net/http"
89
"strings"
910
"time"
@@ -34,6 +35,9 @@ type DefaultHTTPKeyFetcher struct {
3435

3536
// NewDefaultHTTPKeyFetcher creates a new HTTP key fetcher with timeout
3637
func NewDefaultHTTPKeyFetcher() *DefaultHTTPKeyFetcher {
38+
transport := http.DefaultTransport.(*http.Transport).Clone()
39+
transport.DialContext = safeDialContext
40+
3741
return &DefaultHTTPKeyFetcher{
3842
client: &http.Client{
3943
Timeout: 10 * time.Second,
@@ -42,10 +46,92 @@ func NewDefaultHTTPKeyFetcher() *DefaultHTTPKeyFetcher {
4246
CheckRedirect: func(_ *http.Request, _ []*http.Request) error {
4347
return http.ErrUseLastResponse
4448
},
49+
Transport: transport,
4550
},
4651
}
4752
}
4853

54+
// safeDialContext resolves the target hostname and refuses to dial loopback,
55+
// private (RFC1918, ULA), link-local, or unspecified addresses. Combined with
56+
// IsValidDomain rejecting IP literals, this neutralises SSRF abuse of the
57+
// well-known fetcher: an attacker cannot reach internal HTTPS services
58+
// (Kubernetes API server, internal admin panels, internal DNS-resolved hosts)
59+
// even if they control DNS for an attacker domain.
60+
//
61+
// The hostname is resolved once here; we then dial the resolved IP directly,
62+
// which pins the connection against DNS rebinding (a TOCTOU where the resolver
63+
// returns a public IP to a pre-flight check and an internal IP to the actual
64+
// dial). TLS SNI and the Host header continue to use the original hostname
65+
// since they are set by http.Transport from the request URL, not the dial
66+
// address.
67+
func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error) {
68+
host, port, err := net.SplitHostPort(addr)
69+
if err != nil {
70+
return nil, err
71+
}
72+
73+
var resolver net.Resolver
74+
ips, err := resolver.LookupIPAddr(ctx, host)
75+
if err != nil {
76+
return nil, err
77+
}
78+
79+
// Try each non-blocked address in order, falling through on dial failure.
80+
// Without this, a stale public AAAA record that no longer routes (or any
81+
// individually-unreachable IP) breaks auth where the default transport
82+
// would have recovered by trying the next answer.
83+
//
84+
// Each attempt is bounded by perIPDialTimeout so that a single hanging
85+
// address can't consume the whole http.Client budget. This is a
86+
// simpler substitute for Happy Eyeballs (parallel A/AAAA racing) — we
87+
// fail fast and try the next answer instead of racing them.
88+
const perIPDialTimeout = 3 * time.Second
89+
90+
var lastErr error
91+
allBlocked := true
92+
for _, ip := range ips {
93+
if isBlockedIP(ip.IP) {
94+
continue
95+
}
96+
allBlocked = false
97+
dialCtx, cancel := context.WithTimeout(ctx, perIPDialTimeout)
98+
var d net.Dialer
99+
conn, dialErr := d.DialContext(dialCtx, network, net.JoinHostPort(ip.IP.String(), port))
100+
cancel()
101+
if dialErr == nil {
102+
return conn, nil
103+
}
104+
lastErr = dialErr
105+
}
106+
if allBlocked {
107+
return nil, fmt.Errorf("dial %s: refusing to connect to private or loopback address", host)
108+
}
109+
return nil, fmt.Errorf("dial %s: all resolved public addresses failed: %w", host, lastErr)
110+
}
111+
112+
// cgnatRange covers RFC 6598 Carrier-Grade NAT (100.64.0.0/10), which the
113+
// stdlib does not classify via any Is* helper but is reachable on some
114+
// cloud / mobile networks where it shadows internal infrastructure.
115+
var cgnatRange = func() *net.IPNet {
116+
_, n, _ := net.ParseCIDR("100.64.0.0/10")
117+
return n
118+
}()
119+
120+
// isBlockedIP reports whether an IP must not be dialled by the namespace
121+
// verification fetcher. Covers loopback (127/8, ::1), RFC1918 + ULA via
122+
// IsPrivate, link-local (169.254/16, fe80::/10 — includes cloud metadata
123+
// 169.254.169.254), unspecified (0.0.0.0, ::), all multicast (admin-scoped
124+
// 239/8 and ff00::/8 in addition to link-local-multicast), and CGNAT.
125+
func isBlockedIP(ip net.IP) bool {
126+
if ip == nil {
127+
return true
128+
}
129+
return ip.IsLoopback() || ip.IsPrivate() ||
130+
ip.IsLinkLocalUnicast() || ip.IsMulticast() ||
131+
ip.IsUnspecified() ||
132+
cgnatRange.Contains(ip)
133+
}
134+
49135
// NewDefaultHTTPKeyFetcherWithClient creates a new HTTP key fetcher with a custom HTTP client.
50136
// This is primarily useful in tests to inject transports or TLS settings.
51137
func NewDefaultHTTPKeyFetcherWithClient(client *http.Client) *DefaultHTTPKeyFetcher {

‎internal/api/handlers/v0/auth/http_internal_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package auth
2+
3+
import (
4+
"net"
5+
"testing"
6+
)
7+
8+
func TestIsBlockedIP(t *testing.T) {
9+
tests := []struct {
10+
ip string
11+
blocked bool
12+
}{
13+
// Blocked — loopback
14+
{"127.0.0.1", true},
15+
{"::1", true},
16+
// Blocked — RFC1918 / ULA (IsPrivate)
17+
{"10.0.0.1", true},
18+
{"172.16.0.1", true},
19+
{"192.168.1.1", true},
20+
{"fc00::1", true},
21+
// Blocked — link-local (includes cloud metadata 169.254.169.254)
22+
{"169.254.169.254", true},
23+
{"fe80::1", true},
24+
// Blocked — unspecified
25+
{"0.0.0.0", true},
26+
{"::", true},
27+
// Blocked — admin-scoped and broader multicast
28+
{"239.0.0.1", true},
29+
{"ff00::1", true},
30+
// Blocked — Carrier-Grade NAT (RFC 6598)
31+
{"100.64.0.1", true},
32+
{"100.127.255.254", true},
33+
// Allowed — public
34+
{"1.1.1.1", false},
35+
{"8.8.8.8", false},
36+
{"2606:4700:4700::1111", false},
37+
// Allowed — outside CGNAT range
38+
{"100.63.255.255", false},
39+
{"100.128.0.1", false},
40+
}
41+
for _, tc := range tests {
42+
t.Run(tc.ip, func(t *testing.T) {
43+
ip := net.ParseIP(tc.ip)
44+
if ip == nil {
45+
t.Fatalf("ParseIP(%q) returned nil", tc.ip)
46+
}
47+
if got := isBlockedIP(ip); got != tc.blocked {
48+
t.Errorf("isBlockedIP(%q) = %v, want %v", tc.ip, got, tc.blocked)
49+
}
50+
})
51+
}
52+
}

‎internal/api/handlers/v0/auth/http_test.go‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -472,17 +472,6 @@ func TestHTTPAuthHandler_Permissions(t *testing.T) {
472472
"v1.api.example.com/*", // should be reversed
473473
},
474474
},
475-
{
476-
name: "single part domain",
477-
domain: "localhost",
478-
expectedPatterns: []string{
479-
"localhost/*", // exact pattern only (no reversal needed)
480-
},
481-
unexpectedPatterns: []string{
482-
"localhost.*", // HTTP should not grant subdomain permissions
483-
"*.localhost", // wrong wildcard position
484-
},
485-
},
486475
{
487476
name: "hyphenated domain",
488477
domain: "my-app.example-site.com",

‎internal/api/handlers/v0/edit.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package v0
33
import (
44
"context"
55
"errors"
6+
"log"
67
"net/http"
78
"net/url"
89
"strings"
@@ -74,7 +75,8 @@ func RegisterEditEndpoints(api huma.API, pathPrefix string, registry service.Reg
7475
if errors.Is(err, database.ErrNotFound) {
7576
return nil, huma.Error404NotFound("Server not found")
7677
}
77-
return nil, huma.Error500InternalServerError("Failed to get current server", err)
78+
log.Printf("edit: get current server (%q/%q) failed: %v", serverName, version, err)
79+
return nil, huma.Error500InternalServerError("Failed to get current server")
7880
}
7981

8082
// Verify edit permissions for this server using the existing server name

‎internal/api/handlers/v0/servers.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package v0
33
import (
44
"context"
55
"errors"
6+
"log"
67
"net/http"
78
"net/url"
89
"reflect"
@@ -131,7 +132,8 @@ func RegisterServersEndpoints(api huma.API, pathPrefix string, registry service.
131132
// Get paginated results with filtering
132133
servers, nextCursor, err := registry.ListServers(ctx, filter, input.Cursor, input.Limit)
133134
if err != nil {
134-
return nil, huma.Error500InternalServerError("Failed to get registry list", err)
135+
log.Printf("list servers failed: %v", err)
136+
return nil, huma.Error500InternalServerError("Failed to get registry list")
135137
}
136138

137139
// Convert []*ServerResponse to []ServerResponse
@@ -184,7 +186,8 @@ func RegisterServersEndpoints(api huma.API, pathPrefix string, registry service.
184186
if err.Error() == errRecordNotFound || errors.Is(err, database.ErrNotFound) {
185187
return nil, huma.Error404NotFound("Server not found")
186188
}
187-
return nil, huma.Error500InternalServerError("Failed to get server details", err)
189+
log.Printf("get server details (%q/%q) failed: %v", serverName, version, err)
190+
return nil, huma.Error500InternalServerError("Failed to get server details")
188191
}
189192

190193
return &Response[apiv0.ServerResponse]{
@@ -213,7 +216,8 @@ func RegisterServersEndpoints(api huma.API, pathPrefix string, registry service.
213216
if err.Error() == errRecordNotFound || errors.Is(err, database.ErrNotFound) {
214217
return nil, huma.Error404NotFound("Server not found")
215218
}
216-
return nil, huma.Error500InternalServerError("Failed to get server versions", err)
219+
log.Printf("get server versions (%q) failed: %v", serverName, err)
220+
return nil, huma.Error500InternalServerError("Failed to get server versions")
217221
}
218222

219223
// Convert []*ServerResponse to []ServerResponse

0 commit comments

Comments
 (0)