nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
105.68k stars 28.67k forks source link

dns.resolveMx returns invalid result in Node <22 #53708

Open analogic opened 3 weeks ago

analogic commented 3 weeks ago

Version

v20.15.0

Platform

linux & docker

Subsystem

dns

What steps will reproduce the bug?

I have this code:

const dns = require('dns');
const ign = dns.resolveMx("blek.cz", (err, addresses) => console.log([err, addresses]));

And results:

$ docker run -ti node:16 node
[ null, [ { exchange: 'blekmx1.blek.cz', priority: 100 } ] ]

$ docker run -ti node:18 node
[ null, [ { exchange: 'blek.cz', priority: 100 } ] ]

$ docker run -ti node:20 node
[ null, [ { exchange: 'blek.cz', priority: 100 } ] ]

$ docker run -ti node:20-alpine node
[ null, [ { exchange: 'blek.cz', priority: 100 } ] ]

$ docker run -ti node:22 node
[ null, [ { exchange: 'blekmx1.blek.cz', priority: 100 } ] ]

How often does it reproduce? Is there a required condition?

Might be system/resolver dependent. Also there should be something unusual about this particular domain

What is the expected behavior? Why is that the expected behavior?

$ dig +short mx blek.cz
100 blekmx1.blek.cz.

What do you see instead?

Bare "blek.cz" host even don't have SMTP port open so it makes emails undeliverable

Additional information

No response

JQuags commented 3 weeks ago

I am able to reproduce this on freebsd (13.3) with blek.cz and visa.com but not gmail.com and other domains however unlike the above test it also is an issue in v22.3.0 installed from ports.

targos commented 3 weeks ago

I verified that it was fixed by https://github.com/nodejs/node/pull/53155 in Node.js 22.3.0.

Probably due to this (from c-ares changelog):

Due to a logic flaw dns name compression writing was not properly implemented which would result in the name prefix not being written for a partial match. This could cause issues in various record types such as MX records when using the deprecated API. Regression introduced in 1.28.0. Issue #757

RedYetiDev commented 3 weeks ago

Does that make this is a known limitation?

targos commented 3 weeks ago

No

RedYetiDev commented 3 weeks ago

So, IIUC the solution would be to backport https://github.com/c-ares/c-ares/commit/d7c14a168f05c889672569674d807135fa3a2f82?

targos commented 3 weeks ago

No, the solution is to cherry-pick or backport https://github.com/nodejs/node/pull/53155. That's why I added lts-watch- labels to it.

RedYetiDev commented 3 weeks ago

Oh, thanks, I don't know that much about backporting, thanks for the info!