Skip to content

fix(probe-endpoint): resolve hostname and check for private addresses - #19

Closed
ygd58 wants to merge 1 commit into
ProjectOpenSea:mainfrom
ygd58:fix/probe-endpoint-dns-resolve-check
Closed

fix(probe-endpoint): resolve hostname and check for private addresses#19
ygd58 wants to merge 1 commit into
ProjectOpenSea:mainfrom
ygd58:fix/probe-endpoint-dns-resolve-check

Conversation

@ygd58

@ygd58 ygd58 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #13.

Root cause

isPrivateHostname() is a lexical guard: it only inspects the hostname string as written in the URL. A hostname that looks public but resolves to a private/internal address (misconfiguration, or an attacker pointing a DNS record inward, e.g. at 169.254.169.254) passes the check, and probeEndpoint() then fetch()es whatever the resolver actually returns.

Fix

Adds isPrivateResolvedAddress(), which resolves the hostname via node:dns/promises (lookup(..., { all: true })) and checks every returned A/AAAA record against the same private-range logic already used for IP literals. probeEndpoint() now applies both the lexical guard and the resolve-and-check before ever issuing a request. DNS resolution failures are not treated as private — they fall through so fetch() surfaces the real network error.

Known residual

A narrow TOCTOU window remains between this check and the actual fetch() connect, since fetch re-resolves independently (classic DNS rebinding). Fully closing it needs a connect-time-pinned request (resolve once, dial the validated IP directly) — left as further hardening, consistent with the comment already in this file before this change.

Testing

6 new regression tests: single/multi-address private resolution, a cloud-metadata address mixed in with a public one, resolution-failure fallthrough, and direct isPrivateResolvedAddress() coverage. probe-endpoint.test.ts + verify-probe.test.ts: 50/50 passing.

Note: this local environment runs Node 18.20.8, under which several unrelated test files (x402-scheme.test.ts, eip3009-auth.test.ts, etc.) fail with crypto is not defined — global WebCrypto isn't exposed until Node 20+. Confirmed this is pre-existing and unrelated to this change (same failures occur on a clean checkout of main without these commits, in files this PR does not touch).

Fixes ProjectOpenSea#13. isPrivateHostname() is a lexical guard — it only inspects
the hostname string itself. A hostname that looks public but resolves
to a private/internal address (misconfiguration, or an attacker
pointing a DNS record at 127.0.0.1 / 169.254.169.254 / an internal
service) sailed straight through the check, and the subsequent fetch()
connected to whatever the resolver actually returned.

Adds isPrivateResolvedAddress(), which resolves the hostname via
node:dns/promises and checks every returned A/AAAA record against the
same private-range logic already used for literal IPs. probeEndpoint()
now applies both checks — the lexical guard and the resolve-and-check
— before ever issuing a request. DNS resolution failures are not
treated as private; they fall through so fetch() can surface the real
network error.

A residual TOCTOU window remains between this check and the actual
fetch() connect (classic DNS-rebinding), since fetch re-resolves
independently. Closing that fully needs a connect-time-pinned request
(resolve once, dial the validated IP directly) — left as further
hardening, consistent with the comment already in this file.

Six regression tests added: single and multi-address private
resolution, cloud-metadata address among multiple A/AAAA records,
resolution-failure fallthrough, and direct isPrivateResolvedAddress()
coverage.
@ryanio

ryanio commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks. Recreated in our internal monorepo as PR 639 (private repo, so that number will not resolve from here) and merged, with an extension.

isPrivateResolvedAddress reusing isPrivateHostname as the predicate over resolved records is the right structure, since it means the range logic has one definition rather than two that drift. Checking every returned A/AAAA record instead of the first is the part that matters, because a multi-homed name only needs one inward record, and your test with a metadata address sitting behind a public one covers exactly that.

One thing the patch missed. You wired the check into probeEndpoint only. inspect.ts imports isPrivateHostname and runs its own lexical check with its own fetch, and that is the path #13 actually demonstrated: metadataURI comes from a permissionlessly writable registry, and the localtest.me row that reached 127.0.0.1 went through inspect, not through probeEndpoint. We wired the resolve-and-check into both.

The two paths ended up differing in one respect, from a later review round. inspect now refuses an unresolvable host rather than proceeding. fetch resolves independently of your lookup, so a nameserver that answers SERVFAIL to the check and then hands undici a private address would walk straight past a fall-through. probeEndpoint still falls through there, because that endpoint is the operator's own and surfacing the real network error is the point of the command.

The rebinding window in your "Known residual" section is still open, and we are not claiming otherwise. A resolve-and-check does not close it. It still needs the connect-time-pinned dial you and #13 both described.

The change syncs out to this mirror on the next release.

@ryanio ryanio closed this Aug 23, 2026
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.

inspect: the private-address guard is lexical, so a DNS name that resolves inward is fetched (the residual noted at probe-endpoint.ts:13-16)

2 participants