From d6151d9fa4cc307116fb9abb78957bd33e1dce13 Mon Sep 17 00:00:00 2001 From: Saurabh Maydeo Date: Fri, 26 Jun 2026 04:19:33 -0500 Subject: [PATCH] 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. --- internal/validators/registries/nuget.go | 62 ++++++++- .../registries/nuget_internal_test.go | 129 ++++++++++++++++++ 2 files changed, 188 insertions(+), 3 deletions(-) create mode 100644 internal/validators/registries/nuget_internal_test.go diff --git a/internal/validators/registries/nuget.go b/internal/validators/registries/nuget.go index c229ba02c..9ffaccba6 100644 --- a/internal/validators/registries/nuget.go +++ b/internal/validators/registries/nuget.go @@ -22,6 +22,11 @@ var ( const userAgent = "MCP-Registry-Validator/1.0" +// maxNuGetReadmeBytes caps how much of a package README we buffer, so a hostile +// or oversized response cannot exhaust validator memory. Mirrors the cargo +// validator's maxCargoReadmeBytes. +const maxNuGetReadmeBytes = 5 << 20 // 5 MiB + type cachedServiceIndex struct { index *serviceIndex expiresAt time.Time @@ -76,7 +81,7 @@ func ValidateNuGet(ctx context.Context, pkg model.Package, serverName string) er return ErrMissingVersionForNuget } - client := &http.Client{Timeout: 10 * time.Second} + client := newNuGetHTTPClient(pkg.RegistryBaseURL) // Fetch the service serviceIndex serviceIndex, err := fetchAndCacheServiceIndex(ctx, client, pkg.RegistryBaseURL) @@ -150,6 +155,56 @@ func validateAndNormalizeBaseURL(pkg *model.Package) error { return nil } +// nugetRedirectAllowed reports whether a redirect to target is permitted, given +// the originating request. The target host must match the origin's host; +// additionally, for the real NuGet base, the scheme must not change from the +// origin's (which is https) and the port must stay default — so a redirect +// cannot downgrade the scheme or steer the validator at a non-standard port on +// an otherwise-trusted host. For test (httptest) bases only the host is checked, +// so mocks on http/loopback keep working. +// +// This is the NuGet analogue of cargoURLAllowed. NuGet has no separate README +// CDN host (the README URL comes from the trusted service-index template and is +// served from the same host), so the allowed host is the origin's own host +// rather than a static allow-list. +func nugetRedirectAllowed(target, origin *url.URL, baseURL string) bool { + if target.Hostname() != origin.Hostname() { + return false + } + if baseURL == model.RegistryURLNuGet { + if target.Scheme != origin.Scheme { + return false + } + if p := target.Port(); p != "" && p != "443" { + return false + } + } + return true +} + +// newNuGetHTTPClient builds the client used for all NuGet calls (service index, +// README, and package index). Its CheckRedirect pins every redirect hop to the +// originating request's host (see nugetRedirectAllowed), so an upstream 3xx +// cannot steer the validator at an internal or attacker-chosen host (SSRF). +// NuGet serves these resources directly without cross-host redirects, so this is +// behaviour-preserving for real packages. +func newNuGetHTTPClient(baseURL string) *http.Client { + return &http.Client{ + Timeout: 10 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + // via is non-empty whenever net/http invokes CheckRedirect, so + // via[0] is the originating request. + if !nugetRedirectAllowed(req.URL, via[0].URL, baseURL) { + return fmt.Errorf("refusing redirect to unexpected URL %q", req.URL.Redacted()) + } + return nil + }, + } +} + func fetchAndCacheServiceIndex(ctx context.Context, client *http.Client, baseURL string) (*serviceIndex, error) { cacheMu.RLock() if cached, exists := serviceIndexCache[baseURL]; exists { @@ -234,8 +289,9 @@ func validateReadme(ctx context.Context, serverName, lowerID, lowerVersion strin defer resp.Body.Close() if resp.StatusCode == http.StatusOK { - // Check README content - readmeBytes, err := io.ReadAll(resp.Body) + // Check README content. Bound the read so a hostile or oversized + // response cannot exhaust validator memory. + readmeBytes, err := io.ReadAll(io.LimitReader(resp.Body, maxNuGetReadmeBytes)) if err != nil { return NoReadme, fmt.Errorf("failed to read NuGet README content: %w", err) } diff --git a/internal/validators/registries/nuget_internal_test.go b/internal/validators/registries/nuget_internal_test.go new file mode 100644 index 000000000..7c5a90221 --- /dev/null +++ b/internal/validators/registries/nuget_internal_test.go @@ -0,0 +1,129 @@ +package registries + +import ( + "bytes" + "context" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/modelcontextprotocol/registry/pkg/model" +) + +// TestNuGetRedirectAllowed covers the redirect SSRF policy directly, including +// the real-base scheme/port branch that the httptest-driven tests below cannot +// exercise (they use http/loopback bases). Mirrors cargo's TestCargoURLAllowed. +func TestNuGetRedirectAllowed(t *testing.T) { + const prod = model.RegistryURLNuGet // https://api.nuget.org/v3/index.json + const prodOrigin = "https://api.nuget.org/v3-flatcontainer/x/1.0.0/readme" + const mockBase = "http://127.0.0.1:54321" + const mockOrigin = "http://127.0.0.1:54321/x/1.0.0/readme" + + cases := []struct { + desc string + target string + origin string + baseURL string + want bool + }{ + {"prod: same-host https default", prodOrigin, prodOrigin, prod, true}, + {"prod: same-host explicit :443", "https://api.nuget.org:443/x", prodOrigin, prod, true}, + {"prod: http downgrade refused", "http://api.nuget.org/x", prodOrigin, prod, false}, + {"prod: non-default port refused", "https://api.nuget.org:8080/internal", prodOrigin, prod, false}, + {"prod: cross-host refused", "https://evil.example/x", prodOrigin, prod, false}, + {"prod: metadata IP refused", "https://169.254.169.254/latest/meta-data/", prodOrigin, prod, false}, + {"prod: userinfo trick refused", "https://api.nuget.org@evil.example/x", prodOrigin, prod, false}, + {"test base: same host any scheme/port ok", "http://127.0.0.1:54321/readme", mockOrigin, mockBase, true}, + {"test base: cross host refused", "http://127.0.0.2:54321/x", mockOrigin, mockBase, false}, + } + + for _, tc := range cases { + t.Run(tc.desc, func(t *testing.T) { + target, err := url.Parse(tc.target) + if err != nil { + t.Fatalf("parse target %q: %v", tc.target, err) + } + origin, err := url.Parse(tc.origin) + if err != nil { + t.Fatalf("parse origin %q: %v", tc.origin, err) + } + if got := nugetRedirectAllowed(target, origin, tc.baseURL); got != tc.want { + t.Fatalf("nugetRedirectAllowed(%q, origin=%q, base=%q) = %v, want %v", tc.target, tc.origin, tc.baseURL, got, tc.want) + } + }) + } +} + +// nugetTestIndex builds a minimal service index whose ReadmeUriTemplate points +// at the given base, so validateReadme fetches the README from a mock server. +func nugetTestIndex(base string) *serviceIndex { + return &serviceIndex{Resources: []serviceIndexResource{ + {Type: "ReadmeUriTemplate/6.13.0", ID: base + "/{lower_id}/{lower_version}/readme"}, + }} +} + +// TestValidateReadme_TruncatesOversizedBody verifies the README read is bounded: +// a token placed past maxNuGetReadmeBytes is truncated away, so it is not found. +func TestValidateReadme_TruncatesOversizedBody(t *testing.T) { + const serverName = "io.github.test/pkg" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + // maxNuGetReadmeBytes of filler first, THEN the token — a correct + // io.LimitReader stops before the token is reached. + _, _ = w.Write(bytes.Repeat([]byte("a"), maxNuGetReadmeBytes)) + _, _ = fmt.Fprintf(w, "\nmcp-name: %s\n", serverName) + })) + defer srv.Close() + + client := newNuGetHTTPClient(srv.URL) + state, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if state != InvalidReadme { + t.Fatalf("token past the %d-byte cap should be truncated -> InvalidReadme, got %v", maxNuGetReadmeBytes, state) + } +} + +// TestValidateReadme_TokenWithinCap is the positive control: a token within the +// cap is found normally. +func TestValidateReadme_TokenWithinCap(t *testing.T) { + const serverName = "io.github.test/pkg" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, "intro text\nmcp-name: %s\n", serverName) + })) + defer srv.Close() + + client := newNuGetHTTPClient(srv.URL) + state, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if state != ValidReadme { + t.Fatalf("expected ValidReadme, got %v", state) + } +} + +// TestValidateReadme_RefusesCrossHostRedirect verifies the client refuses a +// redirect that leaves the README host (SSRF guard). The redirect target is +// never connected to, because CheckRedirect rejects it first. +func TestValidateReadme_RefusesCrossHostRedirect(t *testing.T) { + const serverName = "io.github.test/pkg" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 302 to a different host; must NOT be followed. + http.Redirect(w, r, "http://evil.example/readme", http.StatusFound) + })) + defer srv.Close() + + client := newNuGetHTTPClient(srv.URL) + _, err := validateReadme(context.Background(), serverName, "pkg", "1.0.0", client, nugetTestIndex(srv.URL)) + if err == nil { + t.Fatal("expected an error: a cross-host redirect should be refused") + } + if !strings.Contains(err.Error(), "redirect") { + t.Fatalf("expected a redirect-refusal error, got: %v", err) + } +}