Skip to content

Fix node:dns lookup CNAME filtering - #7129

Open
xgame92 wants to merge 3 commits into
cloudflare:mainfrom
xgame92:fix/dns-lookup-cname-filter
Open

Fix node:dns lookup CNAME filtering#7129
xgame92 wants to merge 3 commits into
cloudflare:mainfrom
xgame92:fix/dns-lookup-cname-filter

Conversation

@xgame92

@xgame92 xgame92 commented Aug 26, 2026

Copy link
Copy Markdown

Summary

  • filter DNS-over-HTTPS answers by A/AAAA record type before returning node:dns lookup results
  • preserve ENOTFOUND behavior when no matching address records remain
  • add regression coverage for CNAME chains across combined and explicit family lookups

Fixes #6886

Testing

  • bazel test //src/workerd/api/node/tests:dns-nodejs-test@ --test_output=errors --nocache_test_results
  • bazel build //src/node:node@tsproject
  • pnpm exec eslint --config src/node/eslint.config.mjs src/node/internal/internal_dns.ts src/workerd/api/node/tests/dns-nodejs-test.js
  • pnpm exec prettier --check src/node/internal/internal_dns.ts src/workerd/api/node/tests/dns-nodejs-test.js

@xgame92
xgame92 requested review from a team as code owners August 26, 2026 14:58
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@xgame92

xgame92 commented Aug 26, 2026

Copy link
Copy Markdown
Author

Could an assigned reviewer please run the internal build workflow for this external contribution? The local native DNS target and TypeScript checks are included in the PR description. Thank you.

@xgame92

xgame92 commented Aug 26, 2026

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

@xgame92

xgame92 commented Aug 26, 2026

Copy link
Copy Markdown
Author

recheck

@xgame92

xgame92 commented Aug 26, 2026

Copy link
Copy Markdown
Author

recheck

github-actions Bot added a commit that referenced this pull request Aug 26, 2026
@jasnell

jasnell commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Overall, this is good but I think it needs to go a bit further. I think all the resolve* methods likely have this issue. Worth a check before this proceeds.

Will leave other comments inline directly

// A host with CNAME records registered
CNAME_HOST: 'blog.nodejs.org',
// A host whose A and AAAA responses begin with a CNAME record
CNAME_CHAIN_HOST: 'writings.hongminhee.org',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Needs to use a generic name, something in the example.org family, perhaps?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated this to www.iana.org. The example.* hosts currently return direct A/AAAA answers, so they do not exercise the CNAME-chain case.

Comment thread src/node/internal/internal_dns.ts Outdated
type DnsOrder = 'verbatim' | 'ipv4first' | 'ipv6first';

const DNS_RECORD_TYPE_A = 1;
const DNS_RECORD_TYPE_AAAA = 28;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: these are likely better defined in internal_dns_client

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Moved the RR type mapping into internal_dns_client and now filter Answer there using the requested type. That makes the filtering apply to lookup, every implemented resolve* method, and reverse.

@xgame92

xgame92 commented Aug 27, 2026

Copy link
Copy Markdown
Author

I checked the full resolver surface and moved the filtering into sendDnsRequest(), so every caller now receives only the requested RR type. The regression coverage now uses www.iana.org and exercises lookup(), resolve4(), and resolve6(). The full dns-nodejs-test@ target, including the existing tests for each implemented resolve* method, passes locally.

@xgame92

xgame92 commented Aug 28, 2026

Copy link
Copy Markdown
Author

@jasnell Thanks for the review and approval. Could you please run the internal build workflow for this external contribution when convenient? The automated check cannot run for non-org forks.

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.

node:dns lookup() returns CNAME records as IPv4/IPv6 LookupAddress entries

2 participants