Skip to content

Commit d6151d9

Browse files
committed
fix(nuget): harden README fetch with body size limit + redirect host-pinning
validateReadme read the package README via io.ReadAll with no size cap, and the shared http.Client had no CheckRedirect. Bound the read with io.LimitReader (5 MiB) and pin every redirect hop to the originating request's host (and, for the real NuGet base, forbid scheme/port downgrades), so an oversized or redirected upstream response can't exhaust validator memory or be steered at an unexpected host. NuGet serves the service index, README, and package index directly without cross-host redirects, so this is behaviour-preserving for real packages. Mirrors the cargo hardening from #1330. Adds tests for README truncation and cross-host redirect refusal.
1 parent 535941a commit d6151d9

2 files changed

Lines changed: 188 additions & 3 deletions

File tree

internal/validators/registries/nuget.go

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,11 @@ var (
2222

2323
const userAgent = "MCP-Registry-Validator/1.0"
2424

25+
// maxNuGetReadmeBytes caps how much of a package README we buffer, so a hostile
26+
// or oversized response cannot exhaust validator memory. Mirrors the cargo
27+
// validator's maxCargoReadmeBytes.
28+
const maxNuGetReadmeBytes = 5 << 20 // 5 MiB
29+
2530
type cachedServiceIndex struct {
2631
index *serviceIndex
2732
expiresAt time.Time
@@ -76,7 +81,7 @@ func ValidateNuGet(ctx context.Context, pkg model.Package, serverName string) er
7681
return ErrMissingVersionForNuget
7782
}
7883

79-
client := &http.Client{Timeout: 10 * time.Second}
84+
client := newNuGetHTTPClient(pkg.RegistryBaseURL)
8085

8186
// Fetch the service serviceIndex
8287
serviceIndex, err := fetchAndCacheServiceIndex(ctx, client, pkg.RegistryBaseURL)
@@ -150,6 +155,56 @@ func validateAndNormalizeBaseURL(pkg *model.Package) error {
150155
return nil
151156
}
152157

158+
// nugetRedirectAllowed reports whether a redirect to target is permitted, given
159+
// the originating request. The target host must match the origin's host;
160+
// additionally, for the real NuGet base, the scheme must not change from the
161+
// origin's (which is https) and the port must stay default — so a redirect
162+
// cannot downgrade the scheme or steer the validator at a non-standard port on
163+
// an otherwise-trusted host. For test (httptest) bases only the host is checked,
164+
// so mocks on http/loopback keep working.
165+
//
166+
// This is the NuGet analogue of cargoURLAllowed. NuGet has no separate README
167+
// CDN host (the README URL comes from the trusted service-index template and is
168+
// served from the same host), so the allowed host is the origin's own host
169+
// rather than a static allow-list.
170+
func nugetRedirectAllowed(target, origin *url.URL, baseURL string) bool {
171+
if target.Hostname() != origin.Hostname() {
172+
return false
173+
}
174+
if baseURL == model.RegistryURLNuGet {
175+
if target.Scheme != origin.Scheme {
176+
return false
177+
}
178+
if p := target.Port(); p != "" && p != "443" {
179+
return false
180+
}
181+
}
182+
return true
183+
}
184+
185+
// newNuGetHTTPClient builds the client used for all NuGet calls (service index,
186+
// README, and package index). Its CheckRedirect pins every redirect hop to the
187+
// originating request's host (see nugetRedirectAllowed), so an upstream 3xx
188+
// cannot steer the validator at an internal or attacker-chosen host (SSRF).
189+
// NuGet serves these resources directly without cross-host redirects, so this is
190+
// behaviour-preserving for real packages.
191+
func newNuGetHTTPClient(baseURL string) *http.Client {
192+
return &http.Client{
193+
Timeout: 10 * time.Second,
194+
CheckRedirect: func(req *http.Request, via []*http.Request) error {
195+
if len(via) >= 10 {
196+
return errors.New("stopped after 10 redirects")
197+
}
198+
// via is non-empty whenever net/http invokes CheckRedirect, so
199+
// via[0] is the originating request.
200+
if !nugetRedirectAllowed(req.URL, via[0].URL, baseURL) {
201+
return fmt.Errorf("refusing redirect to unexpected URL %q", req.URL.Redacted())
202+
}
203+
return nil
204+
},
205+
}
206+
}
207+
153208
func fetchAndCacheServiceIndex(ctx context.Context, client *http.Client, baseURL string) (*serviceIndex, error) {
154209
cacheMu.RLock()
155210
if cached, exists := serviceIndexCache[baseURL]; exists {
@@ -234,8 +289,9 @@ func validateReadme(ctx context.Context, serverName, lowerID, lowerVersion strin
234289
defer resp.Body.Close()
235290

236291
if resp.StatusCode == http.StatusOK {
237-
// Check README content
238-
readmeBytes, err := io.ReadAll(resp.Body)
292+
// Check README content. Bound the read so a hostile or oversized
293+
// response cannot exhaust validator memory.
294+
readmeBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxNuGetReadmeBytes))
239295
if err != nil {
240296
return NoReadme, fmt.Errorf("failed to read NuGet README content: %w", err)
241297
}
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
package registries
2+
3+
import (
4+
"bytes"
5+
"context"
6+
"fmt"
7+
"net/http"
8+
"net/http/httptest"
9+
"net/url"
10+
"strings"
11+
"testing"
12+
13+
"github.com/modelcontextprotocol/registry/pkg/model"
14+
)
15+
16+
// TestNuGetRedirectAllowed covers the redirect SSRF policy directly, including
17+
// the real-base scheme/port branch that the httptest-driven tests below cannot
18+
// exercise (they use http/loopback bases). Mirrors cargo's TestCargoURLAllowed.
19+
func TestNuGetRedirectAllowed(t *testing.T) {
20+
const prod = model.RegistryURLNuGet // https://api.nuget.org/v3/index.json
21+
const prodOrigin = "https://api.nuget.org/v3-flatcontainer/x/1.0.0/readme"
22+
const mockBase = "http://127.0.0.1:54321"
23+
const mockOrigin = "http://127.0.0.1:54321/x/1.0.0/readme"
24+
25+
cases := []struct {
26+
desc string
27+
target string
28+
origin string
29+
baseURL string
30+
want bool
31+
}{
32+
{"prod: same-host https default", prodOrigin, prodOrigin, prod, true},
33+
{"prod: same-host explicit :443", "https://api.nuget.org:443/x", prodOrigin, prod, true},
34+
{"prod: http downgrade refused", "http://api.nuget.org/x", prodOrigin, prod, false},
35+
{"prod: non-default port refused", "https://api.nuget.org:8080/internal", prodOrigin, prod, false},
36+
{"prod: cross-host refused", "https://evil.example/x", prodOrigin, prod, false},
37+
{"prod: metadata IP refused", "https://169.254.169.254/latest/meta-data/", prodOrigin, prod, false},
38+
{"prod: userinfo trick refused", "https://api.nuget.org@evil.example/x", prodOrigin, prod, false},
39+
{"test base: same host any scheme/port ok", "http://127.0.0.1:54321/readme", mockOrigin, mockBase, true},
40+
{"test base: cross host refused", "http://127.0.0.2:54321/x", mockOrigin, mockBase, false},
41+
}
42+
43+
for _, tc := range cases {
44+
t.Run(tc.desc, func(t *testing.T) {
45+
target, err := url.Parse(tc.target)
46+
if err != nil {
47+
t.Fatalf("parse target %q: %v", tc.target, err)
48+
}
49+
origin, err := url.Parse(tc.origin)
50+
if err != nil {
51+
t.Fatalf("parse origin %q: %v", tc.origin, err)
52+
}
53+
if got := nugetRedirectAllowed(target, origin, tc.baseURL); got != tc.want {
54+
t.Fatalf("nugetRedirectAllowed(%q, origin=%q, base=%q) = %v, want %v", tc.target, tc.origin, tc.baseURL, got, tc.want)
55+
}
56+
})
57+
}
58+
}
59+
60+
// nugetTestIndex builds a minimal service index whose ReadmeUriTemplate points
61+
// at the given base, so validateReadme fetches the README from a mock server.
62+
func nugetTestIndex(base string) *serviceIndex {
63+
return &serviceIndex{Resources: []serviceIndexResource{
64+
{Type: "ReadmeUriTemplate/6.13.0", ID: base + "/{lower_id}/{lower_version}/readme"},
65+
}}
66+
}
67+
68+
// TestValidateReadme_TruncatesOversizedBody verifies the README read is bounded:
69+
// a token placed past maxNuGetReadmeBytes is truncated away, so it is not found.
70+
func TestValidateReadme_TruncatesOversizedBody(t *testing.T) {
71+
const serverName = "io.github.test/pkg"
72+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
73+
w.WriteHeader(http.StatusOK)
74+
// maxNuGetReadmeBytes of filler first, THEN the token — a correct
75+
// io.LimitReader stops before the token is reached.
76+
_, _ = w.Write(bytes.Repeat([]byte("a"), maxNuGetReadmeBytes))
77+
_, _ = fmt.Fprintf(w, "\nmcp-name: %s\n", serverName)
78+
}))
79+
defer srv.Close()
80+
81+
client := newNuGetHTTPClient(srv.URL)
82+
state, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL))
83+
if err != nil {
84+
t.Fatalf("unexpected error: %v", err)
85+
}
86+
if state != InvalidReadme {
87+
t.Fatalf("token past the %d-byte cap should be truncated -> InvalidReadme, got %v", maxNuGetReadmeBytes, state)
88+
}
89+
}
90+
91+
// TestValidateReadme_TokenWithinCap is the positive control: a token within the
92+
// cap is found normally.
93+
func TestValidateReadme_TokenWithinCap(t *testing.T) {
94+
const serverName = "io.github.test/pkg"
95+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
96+
_, _ = fmt.Fprintf(w, "intro text\nmcp-name: %s\n", serverName)
97+
}))
98+
defer srv.Close()
99+
100+
client := newNuGetHTTPClient(srv.URL)
101+
state, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL))
102+
if err != nil {
103+
t.Fatalf("unexpected error: %v", err)
104+
}
105+
if state != ValidReadme {
106+
t.Fatalf("expected ValidReadme, got %v", state)
107+
}
108+
}
109+
110+
// TestValidateReadme_RefusesCrossHostRedirect verifies the client refuses a
111+
// redirect that leaves the README host (SSRF guard). The redirect target is
112+
// never connected to, because CheckRedirect rejects it first.
113+
func TestValidateReadme_RefusesCrossHostRedirect(t *testing.T) {
114+
const serverName = "io.github.test/pkg"
115+
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
116+
// 302 to a different host; must NOT be followed.
117+
http.Redirect(w, r, "http://evil.example/readme", http.StatusFound)
118+
}))
119+
defer srv.Close()
120+
121+
client := newNuGetHTTPClient(srv.URL)
122+
_, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL))
123+
if err == nil {
124+
t.Fatal("expected an error: a cross-host redirect should be refused")
125+
}
126+
if !strings.Contains(err.Error(), "redirect") {
127+
t.Fatalf("expected a redirect-refusal error, got: %v", err)
128+
}
129+
}

0 commit comments

Comments
 (0)