From 2ba51299f2b49328f5fda820fa31a2223505967d Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Fri, 30 Oct 2026 12:00:50 +0000 Subject: [PATCH 1/3] Harden nuget catalog fetches, URL parsing and pagination Only follow nuget catalog URLs on the registry origin, make scheme stripping linear with a length cap, and cap paginated version listings. --- internal/core/errors.go | 9 ++ internal/githubactions/githubactions.go | 3 + internal/githubactions/githubactions_test.go | 21 +++++ internal/golang/golang.go | 5 +- internal/golang/golang_test.go | 39 +++++++++ internal/nuget/nuget.go | 14 ++- internal/nuget/nuget_test.go | 34 ++++++++ internal/swift/swift.go | 3 + internal/urlparser/urlparser.go | 91 +++++++++----------- internal/urlparser/urlparser_test.go | 35 +++++++- registries.go | 2 + swift_test.go | 15 ++++ 12 files changed, 219 insertions(+), 52 deletions(-) diff --git a/internal/core/errors.go b/internal/core/errors.go index ad0b8b5..b475cc9 100644 --- a/internal/core/errors.go +++ b/internal/core/errors.go @@ -1,12 +1,21 @@ package core import ( + "errors" + "github.com/git-pkgs/registries/client" ) // ErrNotFound is returned when a package or version is not found. var ErrNotFound = client.ErrNotFound +// ErrTooManyPages is returned when a paginated listing runs past MaxPages. +var ErrTooManyPages = errors.New("too many pages") + +// MaxPages caps paginated listings so a registry that never reports a last +// page cannot keep the caller fetching. +const MaxPages = 1000 + // Type aliases for backward compatibility. type ( HTTPError = client.HTTPError diff --git a/internal/githubactions/githubactions.go b/internal/githubactions/githubactions.go index afc9cc9..711d3c6 100644 --- a/internal/githubactions/githubactions.go +++ b/internal/githubactions/githubactions.go @@ -183,6 +183,9 @@ func (r *Registry) FetchVersions(ctx context.Context, name string) ([]core.Versi var versions []core.Version for page := 1; ; page++ { + if page > core.MaxPages { + return nil, fmt.Errorf("%w: %s tags", core.ErrTooManyPages, name) + } endpoint := fmt.Sprintf("%s/tags?per_page=%d&page=%d", r.repoEndpoint(a), tagsPerPage, page) var tags []tagResponse if err := r.getJSON(ctx, endpoint, &tags); err != nil { diff --git a/internal/githubactions/githubactions_test.go b/internal/githubactions/githubactions_test.go index 7d833fc..a16f0de 100644 --- a/internal/githubactions/githubactions_test.go +++ b/internal/githubactions/githubactions_test.go @@ -337,3 +337,24 @@ func TestEcosystem(t *testing.T) { t.Errorf("Ecosystem() = %q", got) } } + +func TestFetchVersionsStopsAtPageCap(t *testing.T) { + var requests int + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + tags := make([]map[string]any, tagsPerPage) + for i := range tags { + tags[i] = map[string]any{"name": fmt.Sprintf("v%d.%d", requests, i)} + } + _ = json.NewEncoder(w).Encode(tags) + })) + defer server.Close() + + _, err := New(server.URL, core.DefaultClient()).FetchVersions(context.Background(), "actions/checkout") + if !errors.Is(err, core.ErrTooManyPages) { + t.Errorf("err = %v, want ErrTooManyPages", err) + } + if requests != core.MaxPages { + t.Errorf("made %d requests, want %d", requests, core.MaxPages) + } +} diff --git a/internal/golang/golang.go b/internal/golang/golang.go index 4a80ec3..7289846 100644 --- a/internal/golang/golang.go +++ b/internal/golang/golang.go @@ -214,7 +214,10 @@ func (r *Registry) fetchVersionsPkgsite(ctx context.Context, name string) ([]cor var versions []core.Version token := "" - for { + for pages := 0; ; pages++ { + if pages == core.MaxPages { + return nil, fmt.Errorf("%w: %s versions", core.ErrTooManyPages, name) + } q := url.Values{"limit": {fmt.Sprint(pkgsitePageLimit)}} if token != "" { q.Set("token", token) diff --git a/internal/golang/golang_test.go b/internal/golang/golang_test.go index d6b9f1c..f5057af 100644 --- a/internal/golang/golang_test.go +++ b/internal/golang/golang_test.go @@ -3,6 +3,7 @@ package golang import ( "context" "encoding/json" + "fmt" "net/http" "net/http/httptest" "testing" @@ -429,3 +430,41 @@ func TestDeriveRepoURL(t *testing.T) { }) } } + +func TestFetchVersionsPkgsiteStopsAtPageCap(t *testing.T) { + var requests int + pkgsite := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests++ + _ = json.NewEncoder(w).Encode(pkgsiteVersions{ + Items: []pkgsiteVersion{{Version: fmt.Sprintf("v0.0.%d", requests)}}, + NextPageToken: fmt.Sprint(requests), + }) + })) + defer pkgsite.Close() + + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/example.com/mod/@v/list": + _, _ = w.Write([]byte("v1.0.0\n")) + case "/example.com/mod/@v/v1.0.0.info": + _ = json.NewEncoder(w).Encode(versionInfo{Version: "v1.0.0"}) + default: + w.WriteHeader(404) + } + })) + defer proxy.Close() + + reg := New(proxy.URL, core.DefaultClient()) + reg.pkgsiteURL = pkgsite.URL + + versions, err := reg.FetchVersions(context.Background(), "example.com/mod") + if err != nil { + t.Fatalf("FetchVersions failed: %v", err) + } + if requests != core.MaxPages { + t.Errorf("made %d pkgsite requests, want %d", requests, core.MaxPages) + } + if len(versions) != 1 || versions[0].Number != "v1.0.0" { + t.Errorf("expected proxy fallback v1.0.0, got %+v", versions) + } +} diff --git a/internal/nuget/nuget.go b/internal/nuget/nuget.go index 998baed..a64a186 100644 --- a/internal/nuget/nuget.go +++ b/internal/nuget/nuget.go @@ -4,6 +4,7 @@ package nuget import ( "context" "fmt" + "net/url" "strings" "time" @@ -213,8 +214,10 @@ func (r *Registry) FetchVersions(ctx context.Context, name string) ([]core.Versi // fetchIntegrity fetches the catalog leaf to extract the package hash. // The registration index does not include packageHash, only the full // catalog leaf does. Errors are swallowed since integrity is best-effort. +// The leaf URL comes from the registry response, so only follow it on the +// registry's own origin. func (r *Registry) fetchIntegrity(ctx context.Context, catalogURL string) string { - if catalogURL == "" { + if catalogURL == "" || !sameOrigin(r.baseURL, catalogURL) { return "" } var leaf catalogLeaf @@ -231,6 +234,15 @@ func (r *Registry) fetchIntegrity(ctx context.Context, catalogURL string) string return algo + "-" + leaf.PackageHash } +func sameOrigin(a, b string) bool { + first, err := url.Parse(a) + if err != nil { + return false + } + second, err := url.Parse(b) + return err == nil && first.Scheme == second.Scheme && strings.EqualFold(first.Host, second.Host) +} + func (r *Registry) FetchDependencies(ctx context.Context, name, version string) ([]core.Dependency, error) { lowerName := strings.ToLower(name) url := fmt.Sprintf("%s/registration5-semver1/%s/index.json", r.baseURL, lowerName) diff --git a/internal/nuget/nuget_test.go b/internal/nuget/nuget_test.go index 47d4105..e730d62 100644 --- a/internal/nuget/nuget_test.go +++ b/internal/nuget/nuget_test.go @@ -313,3 +313,37 @@ func TestEcosystem(t *testing.T) { t.Errorf("expected ecosystem 'nuget', got %q", reg.Ecosystem()) } } + +func TestFetchVersionsSkipsCatalogURLOnOtherOrigin(t *testing.T) { + var otherHits int + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + otherHits++ + _ = json.NewEncoder(w).Encode(catalogLeaf{PackageHash: "leaked", PackageHashAlgorithm: "SHA512"}) + })) + defer other.Close() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _ = json.NewEncoder(w).Encode(registrationResponse{ + Items: []registrationPage{{Items: []registrationLeaf{{ + CatalogEntry: catalogEntry{ + CatalogURL: other.URL + "/latest/meta-data/", + ID: "xunit", + Version: "2.6.0", + Listed: true, + }, + }}}}, + }) + })) + defer server.Close() + + versions, err := New(server.URL, core.DefaultClient()).FetchVersions(context.Background(), "xunit") + if err != nil { + t.Fatalf("FetchVersions failed: %v", err) + } + if otherHits != 0 { + t.Errorf("fetched catalog URL on another origin %d times", otherHits) + } + if len(versions) != 1 || versions[0].Integrity != "" { + t.Errorf("versions = %+v, want one version with empty integrity", versions) + } +} diff --git a/internal/swift/swift.go b/internal/swift/swift.go index 5029e42..2dbe826 100644 --- a/internal/swift/swift.go +++ b/internal/swift/swift.go @@ -70,6 +70,9 @@ func (r *Registry) list(ctx context.Context, name string) (*listing, error) { if seen[page] { return nil, fmt.Errorf("swift registry pagination loop at %s", page) } + if len(seen) == core.MaxPages { + return nil, fmt.Errorf("%w: swift registry listing for %s", core.ErrTooManyPages, name) + } seen[page] = true var response struct { Releases map[string]listedRelease `json:"releases"` diff --git a/internal/urlparser/urlparser.go b/internal/urlparser/urlparser.go index bbd661a..dae1ccc 100644 --- a/internal/urlparser/urlparser.go +++ b/internal/urlparser/urlparser.go @@ -51,11 +51,16 @@ func init() { } } +// maxURLLength bounds input from registry metadata; real repository URLs +// are far shorter. +const maxURLLength = 4096 + // Clean removes common noise from git URLs: schemes, auth, brackets, anchors, etc. // Returns just the host/path portion ready for further processing. +// URLs longer than maxURLLength return "". func Clean(rawURL string) string { s := strings.TrimSpace(rawURL) - if s == "" { + if s == "" || len(s) > maxURLLength { return "" } @@ -133,62 +138,49 @@ func trimGitSuffix(s string) string { return s } -// removeSchemes strips all scheme prefixes from a URL -func removeSchemes(s string) string { - for { - prev := s - - // Remove scm:git:, scm:svn:, scm:hg: prefixes - sLower := strings.ToLower(s) - switch { - case strings.HasPrefix(sLower, "scm:git:"): - s = s[len("scm:git:"):] - case strings.HasPrefix(sLower, "scm:svn:"): - s = s[len("scm:svn:"):] - case strings.HasPrefix(sLower, "scm:hg:"): - s = s[len("scm:hg:"):] - } +var scmPrefixes = []string{"scm:git:", "scm:svn:", "scm:hg:"} + +var schemes = []string{ + "git+https://", "git+https:", + "git+ssh://", "git+ssh:", + "https://", "https:", + "http://", "http:", + "git://", "git:", + "ssh://", "ssh:", + "svn://", "svn:", + "hg://", "hg:", + "scm://", "scm:", +} - // Remove standard schemes - s = removeScheme(s) +var gitSlashPrefix = []string{"git//"} - // Remove git// pattern - if strings.HasPrefix(strings.ToLower(s), "git//") { - s = s[5:] +// removeSchemes strips all scheme prefixes from a URL. It advances an offset +// rather than re-slicing and lowercasing, so repeated prefixes stay linear. +func removeSchemes(s string) string { + i := 0 + for { + start := i + i += prefixLen(s[i:], scmPrefixes) + i += prefixLen(s[i:], schemes) + i += prefixLen(s[i:], gitSlashPrefix) + for i < len(s) && s[i] == '/' { + i++ } - - // Remove leading slashes - s = strings.TrimLeft(s, "/") - - if s == prev { - break + if i == start { + return s[i:] } } - return s } -// removeScheme removes a single URL scheme prefix -func removeScheme(s string) string { - sLower := strings.ToLower(s) - - schemes := []string{ - "git+https://", "git+https:", - "git+ssh://", "git+ssh:", - "https://", "https:", - "http://", "http:", - "git://", "git:", - "ssh://", "ssh:", - "svn://", "svn:", - "hg://", "hg:", - "scm://", "scm:", - } - - for _, scheme := range schemes { - if strings.HasPrefix(sLower, scheme) { - return s[len(scheme):] +// prefixLen returns the length of the first prefix that s starts with, +// ignoring case, or 0. +func prefixLen(s string, prefixes []string) int { + for _, p := range prefixes { + if len(s) >= len(p) && strings.EqualFold(s[:len(p)], p) { + return len(p) } } - return s + return 0 } // removeAuth removes user:pass@ or user@ from URLs @@ -420,9 +412,10 @@ func IsKnownHost(rawURL string) bool { } // Normalize cleans a git URL and ensures it has an https scheme. +// URLs longer than maxURLLength return "". func Normalize(rawURL string) string { s := strings.TrimSpace(rawURL) - if s == "" { + if s == "" || len(s) > maxURLLength { return "" } diff --git a/internal/urlparser/urlparser_test.go b/internal/urlparser/urlparser_test.go index dbea38f..018adfb 100644 --- a/internal/urlparser/urlparser_test.go +++ b/internal/urlparser/urlparser_test.go @@ -1,6 +1,10 @@ package urlparser -import "testing" +import ( + "strings" + "testing" + "time" +) func TestParse(t *testing.T) { tests := []struct { @@ -360,3 +364,32 @@ func TestClean(t *testing.T) { }) } } + +func TestRemoveSchemesRepeatedPrefixIsLinear(t *testing.T) { + input := strings.Repeat("GIT:", 25_000) + "github.com/foo/bar" + start := time.Now() + got := removeSchemes(input) + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("removeSchemes took %v on %d bytes", elapsed, len(input)) + } + if got != "github.com/foo/bar" { + t.Errorf("removeSchemes = %q", got) + } +} + +func TestOverlongURLRejected(t *testing.T) { + input := strings.Repeat("GIT:", 100_000) + "github.com/foo/bar" + start := time.Now() + if got := Parse(input); got != "" { + t.Errorf("Parse = %q, want empty", got) + } + if got := CanonicalURL(input); got != "" { + t.Errorf("CanonicalURL = %q, want empty", got) + } + if got := Normalize(input); got != "" { + t.Errorf("Normalize = %q, want empty", got) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("took %v on %d bytes", elapsed, len(input)) + } +} diff --git a/registries.go b/registries.go index 3311dd7..ccb6e20 100644 --- a/registries.go +++ b/registries.go @@ -99,6 +99,8 @@ var ( ErrNotFound = client.ErrNotFound // ErrResponseTooLarge is returned when a response body exceeds the client limit. ErrResponseTooLarge = client.ErrResponseTooLarge + // ErrTooManyPages is returned when a paginated version listing does not end. + ErrTooManyPages = core.ErrTooManyPages ) // Error types diff --git a/swift_test.go b/swift_test.go index b8477c7..6ca3503 100644 --- a/swift_test.go +++ b/swift_test.go @@ -364,3 +364,18 @@ func TestSwiftReleaseURLOnOtherOrigin(t *testing.T) { t.Error("cross-origin release URL accepted") } } + +func TestSwiftPaginationStopsAtPageCap(t *testing.T) { + var requests int + reg := newSwiftRegistry(t, func(w http.ResponseWriter, _ *http.Request) { + requests++ + w.Header().Set("Link", fmt.Sprintf(`; rel="next"`, requests+1)) + _, _ = fmt.Fprint(w, `{"releases":{}}`) + }) + if _, err := reg.FetchVersions(context.Background(), "mona.linked-list"); !errors.Is(err, registries.ErrTooManyPages) { + t.Errorf("err = %v, want ErrTooManyPages", err) + } + if requests != 1000 { + t.Errorf("made %d requests, want 1000", requests) + } +} From a2e38b0b2facc84529d1355780328a4e85bf7733 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Fri, 30 Oct 2026 14:04:11 +0000 Subject: [PATCH 2/3] Restrict NuGet catalog redirects to the registry origin --- internal/nuget/nuget.go | 25 +++++++++-- nuget_test.go | 95 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 nuget_test.go diff --git a/internal/nuget/nuget.go b/internal/nuget/nuget.go index a64a186..fae1b98 100644 --- a/internal/nuget/nuget.go +++ b/internal/nuget/nuget.go @@ -4,6 +4,7 @@ package nuget import ( "context" "fmt" + "net/http" "net/url" "strings" "time" @@ -13,8 +14,9 @@ import ( ) const ( - DefaultURL = "https://api.nuget.org/v3" - ecosystem = "nuget" + DefaultURL = "https://api.nuget.org/v3" + ecosystem = "nuget" + maxCatalogRedirects = 10 ) func init() { @@ -220,8 +222,25 @@ func (r *Registry) fetchIntegrity(ctx context.Context, catalogURL string) string if catalogURL == "" || !sameOrigin(r.baseURL, catalogURL) { return "" } + // Scope the redirect restriction to this fetch without changing the shared client. + catalogClient := *r.client + httpClient := *r.client.HTTPClient + checkRedirect := httpClient.CheckRedirect + httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if !sameOrigin(r.baseURL, req.URL.String()) { + return fmt.Errorf("nuget catalog redirect leaves registry origin") + } + if len(via) >= maxCatalogRedirects { + return fmt.Errorf("nuget catalog stopped after %d redirects", maxCatalogRedirects) + } + if checkRedirect != nil { + return checkRedirect(req, via) + } + return nil + } + catalogClient.HTTPClient = &httpClient var leaf catalogLeaf - if err := r.client.GetJSON(ctx, catalogURL, &leaf); err != nil { + if err := catalogClient.GetJSON(ctx, catalogURL, &leaf); err != nil { return "" } if leaf.PackageHash == "" { diff --git a/nuget_test.go b/nuget_test.go new file mode 100644 index 0000000..bf1c6c2 --- /dev/null +++ b/nuget_test.go @@ -0,0 +1,95 @@ +package registries_test + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/git-pkgs/registries" +) + +func TestNugetCatalogRedirects(t *testing.T) { + for _, tt := range []struct { + name string + target string + denyRedirect bool + wantIntegrity string + }{ + {name: "same origin", target: "/catalog/leaf", wantIntegrity: "sha512-local"}, + {name: "other origin", target: "other"}, + {name: "later other origin", target: "/catalog/next"}, + {name: "caller policy", target: "/catalog/leaf", denyRedirect: true}, + {name: "redirect loop", target: "/catalog/start"}, + } { + t.Run(tt.name, func(t *testing.T) { + testNugetCatalogRedirect(t, tt.target, tt.denyRedirect, tt.wantIntegrity) + }) + } +} + +func testNugetCatalogRedirect(t *testing.T, target string, denyRedirect bool, wantIntegrity string) { + t.Helper() + otherHits := 0 + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + otherHits++ + _, _ = fmt.Fprint(w, `{"packageHash":"leaked","packageHashAlgorithm":"SHA512"}`) + })) + defer other.Close() + if target == "other" { + target = other.URL + } + var baseURL string + redirectHits := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/registration5-semver1/xunit/index.json": + _, _ = fmt.Fprintf(w, `{"items":[{"items":[{"catalogEntry":{"@id":%q,"id":"xunit","version":"2.6.0","listed":true}}]}]}`, baseURL+"/catalog/start") + case "/catalog/start": + redirectHits++ + http.Redirect(w, r, target, http.StatusFound) + case "/catalog/next": + http.Redirect(w, r, other.URL, http.StatusFound) + case "/catalog/leaf": + _, _ = fmt.Fprint(w, `{"packageHash":"local","packageHashAlgorithm":"SHA512"}`) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + baseURL = server.URL + + c := registries.NewClient(registries.WithMaxRetries(0)) + policyCalls := 0 + if denyRedirect { + c.HTTPClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + policyCalls++ + return errors.New("redirect denied by caller") + } + } + reg, err := registries.New("nuget", baseURL, c) + if err != nil { + t.Fatal(err) + } + versions, err := reg.FetchVersions(context.Background(), "xunit") + if err != nil { + t.Fatal(err) + } + if len(versions) != 1 || versions[0].Integrity != wantIntegrity { + t.Errorf("versions = %+v, want one version with integrity %q", versions, wantIntegrity) + } + if otherHits != 0 { + t.Errorf("catalog fetched another origin %d times", otherHits) + } + if denyRedirect && policyCalls != 1 { + t.Errorf("caller redirect policy called %d times, want 1", policyCalls) + } + if target == "/catalog/start" && redirectHits != 10 { + t.Errorf("redirect loop made %d requests, want 10", redirectHits) + } + if !denyRedirect && c.HTTPClient.CheckRedirect != nil { + t.Error("changed the shared client's redirect policy") + } +} From 51a546b8e3b0485b81452f51922c04591676816c Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Fri, 30 Oct 2026 16:06:40 +0000 Subject: [PATCH 3/3] Avoid retries for best-effort NuGet catalog fetches --- internal/nuget/nuget.go | 2 ++ nuget_test.go | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/nuget/nuget.go b/internal/nuget/nuget.go index fae1b98..584b712 100644 --- a/internal/nuget/nuget.go +++ b/internal/nuget/nuget.go @@ -224,6 +224,8 @@ func (r *Registry) fetchIntegrity(ctx context.Context, catalogURL string) string } // Scope the redirect restriction to this fetch without changing the shared client. catalogClient := *r.client + // Integrity is best-effort; avoid backoff for rejected catalog redirects. + catalogClient.MaxRetries = 0 httpClient := *r.client.HTTPClient checkRedirect := httpClient.CheckRedirect httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { diff --git a/nuget_test.go b/nuget_test.go index bf1c6c2..efb7702 100644 --- a/nuget_test.go +++ b/nuget_test.go @@ -61,7 +61,8 @@ func testNugetCatalogRedirect(t *testing.T, target string, denyRedirect bool, wa defer server.Close() baseURL = server.URL - c := registries.NewClient(registries.WithMaxRetries(0)) + c := registries.DefaultClient() + maxRetries := c.MaxRetries policyCalls := 0 if denyRedirect { c.HTTPClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { @@ -89,6 +90,12 @@ func testNugetCatalogRedirect(t *testing.T, target string, denyRedirect bool, wa if target == "/catalog/start" && redirectHits != 10 { t.Errorf("redirect loop made %d requests, want 10", redirectHits) } + if target != "/catalog/start" && redirectHits != 1 { + t.Errorf("catalog made %d requests, want 1", redirectHits) + } + if c.MaxRetries != maxRetries { + t.Error("changed the shared client's retry policy") + } if !denyRedirect && c.HTTPClient.CheckRedirect != nil { t.Error("changed the shared client's redirect policy") }