Repository navigation
fix(ext/node): keep dns.lookup permission token internal - #35224
Merged
Merged
Conversation
node:dns.lookup() exposed an internal permission token to its callback as a 4th argument, where Node only passes (err, address, family). The token exists solely so node:net.connect() can check --allow-net against the original hostname for an address Deno resolved, so it should never be visible to user code or reusable through a custom net.connect() lookup. Route the token through a module-private symbol defined in the internal cares_wrap binding. dns.lookup() only hands it to a callback bearing that symbol, and net.connect() tags its internal callback only when it uses the built-in lookup. User dns.lookup() callbacks now match Node's signature, and a custom net.connect() lookup has its addresses checked against the literal IP with no hostname substitution. TCPWrap also keeps the token's resolved-IP set and only substitutes the hostname for an address the token actually resolved, mirroring op_net_connect_tcp.
bartlomieju
marked this pull request as ready for review
June 15, 2026 12:11
Add NetPermToken::check_host() and use it from both op_net_connect_tcp_inner and TCPWrap, removing the duplicated resolved-IP/hostname substitution logic. TCPWrap now stores an owned NetPermToken instead of a bespoke NetPermTokenInfo struct.
`usingDefaultLookup` keyed off `options.lookup === undefined`, but the resolver selection one line below is `options.lookup || dnsLookup`. The two could disagree: explicitly passing the built-in `dns.lookup` as `options.lookup` runs the trusted resolver yet left the token uninstalled, so `--allow-net=<host>` was checked against the literal resolved IP and denied a connection it should have allowed. Derive `usingDefaultLookup` from the selected `lookup` instead, and key `getLookupDnsOpts` off the same flag so the resolver, its options, and token honoring stay in sync.
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.
node:dns.lookup() was handing an internal Deno permission token to the
lookup callback as a fourth argument. Node's lookup callback only receives
(err, address, family), so this was both a compatibility wart and an
internal implementation detail leaking into user space.
The token exists only so that node:net.connect() can check --allow-net
against the hostname that was resolved, rather than against the resolved
IP, when Deno itself performed the DNS resolution. It has no meaning to
user code, and it should not be reusable through a custom net.connect()
lookupfunction, where the returned addresses are arbitrary.This routes the token through a module-private symbol defined in the
internal cares_wrap binding. dns.lookup() only attaches the token when the
receiving callback carries that marker, and net.connect() tags its
internal callback only on the built-in lookup path. As a result, user
dns.lookup() callbacks now see exactly Node's (err, address, family)
signature, and a custom net.connect()
lookuphas its addresses checkedagainst the literal IP with no hostname substitution.
As an additional safeguard, TCPWrap now retains the resolved-IP set from
the token and only substitutes the original hostname for an address that
the token actually resolved, matching the behavior already implemented in
the native op_net_connect_tcp path.
A spec test under tests/specs/permission covers three cases: that
dns.lookup() exposes no token, that a custom net.connect() lookup cannot
authorize an unrelated IP, and that the built-in lookup still lets
--allow-net= reach the IPs that host resolves to.