Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

🐛 Bug Report — Incorrect data returned from resolveTxt in node:dns #3325

Closed
TheIndra55 opened this issue Jan 12, 2025 · 8 comments · Fixed by #3327 or #3330
Closed

🐛 Bug Report — Incorrect data returned from resolveTxt in node:dns #3325

TheIndra55 opened this issue Jan 12, 2025 · 8 comments · Fixed by #3327 or #3330
Assignees
Labels
bug Something isn't working nodejs compat

Comments

@TheIndra55
Copy link

The current code in the node:dns implementation strips the leading and trailing quotation marks, however this results in an incorrect result for large TXT records with multiple quoted strings.

export function normalizeTxt({ data }: Answer): string[] {
// Each entry has quotation marks as a prefix and suffix.
// Node.js APIs doesn't have them.
if (data.startsWith('"') && data.endsWith('"')) {
return [data.slice(1, -1)];
}
return [data];
}

As an example you can use resolve s1024._domainkey.yahoo.com, in Node.js this returns

[
  [
    'k=rsa;  p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDrEee0Ri4Juz+QfiWYui/E9UGSXau/2P8LjnTD8V4Unn+2FAZVGE3kL23bzeoULYv4PeleB3gfmJiDJOKU3Ns5L4KJAUUHjFwDebt0NP+sBK0VKeTATL2Yr/S3bT/xhy+1xtj4RkdV7fVxTn56Lb4udUnwuxK4V5b5PdOKj/+XcwIDAQAB; n=A 1024 bit key;'
  ]
]

But in workerd this returns the following (note the quotation marks in the middle of the string)

[
  [
    'k=rsa;  p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDrEee0Ri4Juz+QfiWYui/E9UGSXau/2P8LjnTD8V4Unn+2FAZVGE3kL23bzeoULYv4PeleB3gfm""JiDJOKU3Ns5L4KJAUUHjFwDebt0NP+sBK0VKeTATL2Yr/S3bT/xhy+1xtj4RkdV7fVxTn56Lb4udUnwuxK4V5b5PdOKj/+XcwIDAQAB; n=A 1024 bit key;'
  ]
]
@jasnell
Copy link
Member

jasnell commented Jan 12, 2025

@anonrig

@anonrig anonrig self-assigned this Jan 12, 2025
@anonrig anonrig added bug Something isn't working nodejs compat labels Jan 12, 2025
@anonrig
Copy link
Member

anonrig commented Jan 12, 2025

Thank you. I'll take a look at this.

@TheIndra55
Copy link
Author

The current fix fixes multiple character strings, however completely breaks TXT records containing quotation marks.

For example the following DNS record

theindra.nl.            300     IN      TXT     "test" "test" "test with \" quote"

Will be returned as

[
  [ 'testtesttest with  quote' ]
]

@anonrig
Copy link
Member

anonrig commented Jan 13, 2025

The current fix fixes multiple character strings, however completely breaks TXT records containing quotation marks.

For example the following DNS record

theindra.nl.            300     IN      TXT     "test" "test" "test with \" quote"

Will be returned as

[
  [ 'testtesttest with  quote' ]
]

What should be the correct response in here? I'm not following.

@anonrig anonrig reopened this Jan 13, 2025
@TheIndra55
Copy link
Author

TheIndra55 commented Jan 13, 2025

The correct response should contain the quotation mark, since it's part of the character string.

For example in Node.js the response is

[
  [ 'testtesttest with " quote' ]
]

For some context RFC 1035 specifies that a TXT record can contain one or more character strings, often they are split at the max lenght but a record with "hello " "world" is completely valid.

@TheIndra55
Copy link
Author

TheIndra55 commented Jan 13, 2025

Noting that it does seem weird the response by Cloudflare doesn't correctly escape the " in the response, but taking how there's no real standard beyond "following Google's schema" can't say whether it's wrong. If it tries to follow the master files format then it would be incorrect.

5.1
<character-string> is expressed in one or two ways: as a contiguous set
of characters without interior spaces, or as a string beginning with a "
and ending with a ". Inside a " delimited string any character can
occur, except for a " itself, which must be quoted using \ (back slash).

Comparing it to other DNS servers, in the response of Quad9 it is correctly escpaped, and Google seems to omit the quotes.

// Cloudflare DNS
{
    "name": "theindra.nl",
    "type": 16,
    "TTL": 300,
    "data": "\"test\"\"test\"\"test with \" quote\""
},

// Quad9
{
    "name": "theindra.nl.",
    "type": 16,
    "TTL": 300,
    "Expires": "Mon, 13 Jan 2025 20:06:19 UTC",
    "data": "\"test\" \"test\" \"test with \\\" quote\""
},

// Google DNS
{
    "name": "theindra.nl.",
    "type": 16,
    "TTL": 300,
    "data": "testtesttest with \" quote"
},

@anonrig
Copy link
Member

anonrig commented Jan 13, 2025

Here's Node.js behavior

Welcome to Node.js v22.13.0.
Type ".help" for more information.
> const dnsPromises = require('node:dns/promises')
undefined
> await dnsPromises.resolveTxt('theindra.nl')
[
  [ 'test', 'test', 'test with " quote' ],
  [ 'v=spf1 -all' ],
  [
    'google-site-verification=n07ELiamzbmMp_Hei_jd8U58Hbc4LThIVZl7r_D6UHA'
  ]
]

@TheIndra55
Copy link
Author

Seems like the behavior was changed between Node.js 20 and Node.js 22, both are correct however.

Welcome to Node.js v20.13.1.
Type ".help" for more information.
> const dnsPromises = require('node:dns/promises')
undefined
> await dnsPromises.resolveTxt('theindra.nl')
[
  [ 'testtesttest with " quote' ],
Welcome to Node.js v22.13.0.
Type ".help" for more information.
> const dnsPromises = require('node:dns/promises')
undefined
> await dnsPromises.resolveTxt('theindra.nl')
[
  [ 'test', 'test', 'test with " quote' ],

The current fix just seems very complex, likely due to the format returned by Cloudflare DNS not being very parseable since it fails to escape the quotes correctly.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working nodejs compat
Projects
None yet
3 participants