Skip to content

dns: do not consult hosts file in reverse lookups#64268

Open
Archkon wants to merge 1 commit into
nodejs:mainfrom
Archkon:fix
Open

dns: do not consult hosts file in reverse lookups#64268
Archkon wants to merge 1 commit into
nodejs:mainfrom
Archkon:fix

Conversation

@Archkon

@Archkon Archkon commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Fixes: #64257

@nodejs-github-bot

nodejs-github-bot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Jul 3, 2026
@Renegade334 Renegade334 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 3, 2026
@Renegade334

Renegade334 commented Jul 3, 2026

Copy link
Copy Markdown
Member

Marking as semver-major (happy to be overruled). Although we do state in the small print that .reverse() shouldn't interrogate the hosts file, it feels like there's a decent chance of something out there relying on this behaviour.

@Archkon

Archkon commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@Renegade334 How about using baking-for-lts label and ship to assess the impact? like this pr: #64114

@Renegade334 Renegade334 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 5, 2026
@nodejs-github-bot

nodejs-github-bot commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74556/

addaleax
addaleax requested changes Jul 5, 2026
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should come with a changes: entry in the documentation as well, even if it only aligns behavior with what is already documented. It's a significant change, and semver-major seems appropriate.

Additionally, I would strongly recommend giving this a proper commit message. Commit messages should explain:

  • What happened
  • How it happened
  • Why it happened this way

Right now, this PR's commit message explains very little, but most importantly, it's not explaining why this change in behavior is better than e.g. a change of the documentation to just match current reality.

@addaleax addaleax mentioned this pull request Jul 5, 2026
Open
@Archkon
dns: do not consult hosts file in reverse lookups
ba63e11 
dns.reverse() is documented with dns.resolve*() as always performing DNS
queries without using hosts file configuration. However, c-ares
ares_gethostbyaddr() honors the channel lookup order, which can include
the files source and cause reverse lookups to return entries from
/etc/hosts.

Configure c-ares resolver channels with ARES_OPT_LOOKUPS set to "b" so
getHostByAddr uses DNS/PTR lookups only. This keeps dns.reverse()
aligned with dns.resolve*() and preserves the distinction from
system-resolution APIs such as dns.lookup() and dns.lookupService().

This is preferable to documenting the current behavior because hosts-
file reverse lookup results are platform-dependent and expose hostent
alias semantics rather than DNS PTR record semantics.

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon Archkon requested a review from addaleax July 6, 2026 05:01
@Archkon

Archkon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

This should come with a changes: entry in the documentation as well, even if it only aligns behavior with what is already documented. It's a significant change, and semver-major seems appropriate.

Additionally, I would strongly recommend giving this a proper commit message. Commit messages should explain:

  • What happened
  • How it happened
  • Why it happened this way

Right now, this PR's commit message explains very little, but most importantly, it's not explaining why this change in behavior is better than e.g. a change of the documentation to just match current reality.

Done, thanks for the helpful reminder. Could you please trigger a fresh CI run by applying request-ci? Thanks !

addaleax
addaleax approved these changes Jul 6, 2026
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll approve so CI can be kicked off but one thing that stands out a bit is that this, just based on the code changes, seems to also affect regular/non-reverse lookups? That seems like a pretty significant unintentional change

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 6, 2026
@nodejs-github-bot

nodejs-github-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74586/

@Archkon

Archkon commented Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'll approve so CI can be kicked off but one thing that stands out a bit is that this, just based on the code changes, seems to also affect regular/non-reverse lookups? That seems like a pretty significant unintentional change

Not really, the normal dns.lookup would use libuv api to support this feature which is not the low-level api that dns.reverse use

And I can't access jenkins ci so could you review what happend for these failed tests when available? Thanks!

@nodejs-github-bot

nodejs-github-bot commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/74593/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dns.reverse uses /etc/hosts, but not the first entry

5 participants

@Archkon @nodejs-github-bot @Renegade334 @jasnell @addaleax