Conversation
The previous fix (PR nirholas#1) blocked hostnames by regex, which a maintainer review correctly flagged as insufficient: DNS rebinding, redirects, and alternate IP representations all bypass a string-based check. This adds safe-fetch.js, which resolves and validates the actual connection address (via an explicit public/private IP range table) for every request and every redirect hop, with a redirect limit, before ever dialing out. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Replacement for #1, addressing the review feedback in #1 (comment) and #1 (comment).
The original fix blocked requests by matching the hostname string against a regex blocklist. As the review correctly pointed out, that's not sufficient SSRF protection: DNS can resolve or rebind a "safe-looking" hostname to a private address, redirects can hop into private ranges after the initial check, and alternate IPv4/IPv6 representations can slip past a regex.
This PR replaces that approach with resolved-address validation in a new
registry/safe-fetch.jsmodule:net.BlockList, covering the IANA special-purpose ranges (loopback, RFC1918 private ranges, link-local/cloud-metadata169.254.0.0/16, CGNAT, multicast, reserved, IPv6 ULA/link-local/NAT64, etc).net.BlockListcompares canonical parsed addresses rather than strings, so alternate IPv4 representations (decimal, octal, etc.) can't bypass it — andnew URL()already canonicalizes those forms before the guard ever sees them.netmodule never calls a customlookupfor those), and hostnames are resolved viadns.lookupand validated before the socket ever pins to that address (via alookupoverride), closing the DNS-rebinding gap.registry/test/safe-fetch.test.js) covering: private IPv4/IPv6 ranges, alternate IPv4 representations, DNS resolving a hostname to a private address, redirects to private targets, and the redirect limit. Plus a regression test inregistry/test/health-checker.test.jsconfirmingcheckNodeHealthblocks a private-address target before opening a socket.Changes
registry/safe-fetch.js(new): SSRF-hardened GET helper, dependency-free (built onnode:http/node:https/node:dns/node:net).registry/health-checker.js:checkNodeHealthnow usessafeFetchinstead of the hostname-regexisPrivateHostcheck + rawfetch.registry/test/safe-fetch.test.js(new),registry/test/health-checker.test.js: regression tests.Test plan
npm testinregistry/— 57/57 passingnpm run lintinregistry/— cleansafeFetch('http://169.254.169.254/')rejects withERR_SSRF_BLOCKEDinstead of connecting🤖 Generated with Claude Code