Skip to content

Harden nuget catalog fetches, URL parsing and pagination - #127

Merged
andrew merged 3 commits into
mainfrom
harden-ssrf-urlparser-pagination
Oct 31, 2026
Merged

andrew merged 3 commits into
mainfrom
harden-ssrf-urlparser-pagination

Conversation

@andrew

@andrew andrew commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

nuget FetchVersions followed each catalog entry's @id to whatever host it named, so a hostile registry response could point it at loopback or cloud metadata addresses and get the body back as Integrity. It now only fetches catalog leaves on the same scheme and host as the registry base URL. nuget.org serves both from api.nuget.org, so integrity is unaffected there.

urlparser.removeSchemes stripped one prefix per iteration and re-lowercased the remaining string each time, so a repository field of GIT: repeated was quadratic (100 KB took about 12s). It now walks an offset with strings.EqualFold, and Clean and Normalize return "" for input over 4096 bytes.

The githubactions tag listing, golang pkgsite listing and swift registry listing kept paging for as long as the server said there was more. They now stop after core.MaxPages (1000) pages and return ErrTooManyPages, re-exported from the root package. For golang that falls back to the module proxy like any other pkgsite error.

Only follow nuget catalog URLs on the registry origin, make scheme
stripping linear with a length cap, and cap paginated version listings.
Copilot AI balanced review requested due to automatic review settings October 30, 2026 12:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@andrew
andrew force-pushed the harden-ssrf-urlparser-pagination branch from bc926fa to 2ba5129 Compare October 30, 2026 12:18
@andrew
andrew requested a review from simonchrz October 30, 2026 14:05
@simonchrz

Copy link
Copy Markdown
Contributor

Looks good to me. The removeSchemes rewrite gives the same output as before: a table of 26 inputs (mixed case, chained scm:git:, repeated schemes, non-ASCII case folds, invalid UTF-8) matched, and a 60s fuzz comparing the old and new implementation found no difference.

One thing in the nuget change: when a catalog @id redirects to another host, the rejected redirect comes back from HTTPClient.Do as a plain *url.Error, which client.open treats as retryable. So each version is tried MaxRetries times with backoff. With core.DefaultClient() and every @id redirecting to a CDN, 5 versions took about 8s and 30 requests, and integrity still ends up empty. Before, it was one followed redirect per version. Making the refusal non-retryable, or using no retries on catalogClient since integrity is best effort, would avoid that.

Smaller: sameOrigin compares Host as written, so a base URL with :443, or an http @id under an https base behind a TLS proxy, drops integrity without a log. nuget.org isn't affected.

@andrew

andrew commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

@simonchrz Fixed the retry issue in 51a546b. The catalog client copy now uses MaxRetries = 0, since integrity is best effort. The shared client's retry settings are unchanged.

The redirect tests now use the default client and assert one attempt for rejected redirects and ten requests for a redirect loop. The public FetchVersions probe went from six catalog requests to one, with no request reaching the other origin.

@simonchrz simonchrz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, retry fix confirmed.

@andrew
andrew merged commit c2802de into main Oct 31, 2026
6 checks passed
@andrew
andrew deleted the harden-ssrf-urlparser-pagination branch October 31, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants