Skip to content

fix: replace SSRF hostname blocklist with resolved-address validation - #2

Open
anupamme wants to merge 1 commit into
nirholas:mainfrom
anupamme:fix-ssrf-health-checker-resolved-address
Open

anupamme wants to merge 1 commit into
nirholas:mainfrom
anupamme:fix-ssrf-health-checker-resolved-address

Conversation

@anupamme

Copy link
Copy Markdown

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.js module:

  • Explicit public/private IP allow rules for IPv4 and IPv6 via net.BlockList, covering the IANA special-purpose ranges (loopback, RFC1918 private ranges, link-local/cloud-metadata 169.254.0.0/16, CGNAT, multicast, reserved, IPv6 ULA/link-local/NAT64, etc). net.BlockList compares canonical parsed addresses rather than strings, so alternate IPv4 representations (decimal, octal, etc.) can't bypass it — and new URL() already canonicalizes those forms before the guard ever sees them.
  • Resolved-address validation on every connection, not just the hostname: IP literals are checked directly (Node's net module never calls a custom lookup for those), and hostnames are resolved via dns.lookup and validated before the socket ever pins to that address (via a lookup override), closing the DNS-rebinding gap.
  • Explicit redirect handling with a limit: redirects are followed manually (default max 5 hops), and each redirect target is independently re-resolved and re-validated before it's followed — a redirect into a private range is rejected, not followed.
  • Regression tests (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 in registry/test/health-checker.test.js confirming checkNodeHealth blocks a private-address target before opening a socket.

Changes

  • registry/safe-fetch.js (new): SSRF-hardened GET helper, dependency-free (built on node:http/node:https/node:dns/node:net).
  • registry/health-checker.js: checkNodeHealth now uses safeFetch instead of the hostname-regex isPrivateHost check + raw fetch.
  • registry/test/safe-fetch.test.js (new), registry/test/health-checker.test.js: regression tests.

Test plan

  • npm test in registry/ — 57/57 passing
  • npm run lint in registry/ — clean
  • Manual check: safeFetch('http://169.254.169.254/') rejects with ERR_SSRF_BLOCKED instead of connecting

🤖 Generated with Claude Code

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>
@anupamme
anupamme requested a review from nirholas as a code owner September 15, 2026 02:18
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.

1 participant