fix(gate): validate every hop of a redirect, and release each response - #66
Merged
Conversation
Synced from the sibling libraries, where this was found and reviewed (bymaxone/nest-queue#65, nest-logger#50, nest-cache#44, nest-notification#32). redirect: 'follow' meant the guard checked only the URL written in the README: a public host answering 302 with Location: http://169.254.169.254/ would be followed, reaching the endpoint unroutableReason exists to stop, one hop later. The chain is followed by hand now, each hop validated before it is requested and the depth bounded at 5. Only the statuses Fetch redirects on are followed — a 300 or 304 is a response to return, not a hop. Every response body is cancelled once its status has been read, since undici holds the connection otherwise. A Location that does not parse fails the gate instead of degrading to a transport note. And the npmjs.com to registry.npmjs.org rewrite is validated too, because it changes what is actually requested.
There was a problem hiding this comment.
Pull request overview
Updates the repository’s release/docs “published surface” gate script to safely validate redirect chains and to proactively release undici connections, aligning behavior with the already-reviewed sibling repositories.
Changes:
- Replaces
redirect: 'follow'with manual redirect handling, validating every redirect hop (bounded to 5) before requesting it. - Cancels response bodies for intermediate hops and final responses to avoid holding undici sockets while only status/headers are needed.
- Re-validates the
npmjs.com → registry.npmjs.orgrewrite target so the first actually requested URL is also checked.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings this library's copy of the shared release gate in line with the four siblings, where these findings were raised and reviewed (bymaxone/nest-queue#65, bymaxone/nest-logger#50, bymaxone/nest-cache#44, bymaxone/nest-notification#32 — all merged).
The security fix
redirect: 'follow'meant the guard checked only the URL written in the README. A public host answering302withLocation: http://169.254.169.254/…would be followed, reaching the endpointunroutableReasonexists to stop — one hop later. Every IP-literal and private-hostname check was bypassable by a single redirect.The chain is followed by hand: each hop validated before it is requested, depth bounded at 5, and a refused hop fails rather than degrading to a note.
Refinements from the review
Locationthat does not parse fails the gate instead of falling through to the transport-error note.Verified
Depth measured against a local server issuing an unbounded 302 chain: 6 URLs requested — 1 initial plus 5 redirects — then refused. Refusals red-checked per class (metadata range, loopback, unique-local IPv6) with a public-target control so real redirects still follow.
lintandcheck:publishedgreen here. No release needed:scripts/is outsidepackage.json→files, sodist/and the published tarball are untouched.