Skip to content

dns: idna encode hostnames before resolution - #25559

Closed
santigimeno wants to merge 4 commits into
nodejs:masterfrom
santigimeno:fix_idna
Closed

dns: idna encode hostnames before resolution#25559
santigimeno wants to merge 4 commits into
nodejs:masterfrom
santigimeno:fix_idna

Conversation

@santigimeno

@santigimenosantigimeno commented Jan 18, 2019

Copy link
Copy Markdown
Member

Fixes: #25558

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Jan 18, 2019

Copy link
Copy Markdown
Collaborator

@santigimeno build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2281/pipeline

@nodejs-github-botnodejs-github-bot added the dns Issues and PRs related to the dns subsystem. label Jan 18, 2019
@santigimenosantigimeno mentioned this pull request Jan 18, 2019
Closed
targos
targos reviewed Jan 18, 2019
View reviewed changes
Comment threadlib/dns.js Outdated
throw new ERR_INVALID_CALLBACK();
}

name = toASCII(name, true);

@targostargosJan 18, 2019

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 function can theoretically throw in

node/lib/punycode.js

Line 335 in 2c0a751

error('overflow');
. Do we need to handle it?

@santigimenosantigimenoJan 18, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

idk, if so, shouldn't we also handle it here to be lenient whether it uses icu or punycode?

@targostargosJan 18, 2019

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.

url.parse is synchronous so it's less of an issue. Here the error could only lead to an uncaught exception.

@santigimenosantigimenoJan 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Updated to ignore exceptions. Thanks

@targos

targos commented Jan 18, 2019

Copy link
Copy Markdown
Member

/cc @saghul@cjihrig@bnoordhuis

saghul
saghul reviewed Jan 18, 2019
View reviewed changes

@saghulsaghul 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.

Left a couple of comments. I think the C++ code which does the utf-8 decoding also needs to go, it should assume ASCII, and throw on failure.

Comment threadlib/dns.js

const cares = internalBinding('cares_wrap');
const { toASCII } = internalBinding('config').hasIntl ?
internalBinding('icu') : require('punycode');

@saghulsaghulJan 18, 2019

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.

What's the policy here, considering punycode is deprecated?

@santigimenosantigimenoJan 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't know 🤷‍♂️

@cjihrigcjihrigJan 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW, we do the same thing in lib/url.js, so it's probably fine.

@jasnelljasnellJan 18, 2019

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.

Using punycode is the fallback if Node.js is compiled without ICU.

@silverwindsilverwindJan 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's only deprecated for userland.

Comment threadlib/dns.js
throw new ERR_INVALID_CALLBACK();
}

try {

@saghulsaghulJan 18, 2019

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.

Would this be needed if we were only using the ICU based toASCII?

@santigimenosantigimenoJan 18, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

My guess is no

@saghul

saghul commented Jan 18, 2019

Copy link
Copy Markdown
Member

Thanks for picking this up @santigimeno ❤️

@santigimeno

santigimeno commented Jan 18, 2019

Copy link
Copy Markdown
MemberAuthor

I think the C++ code which does the utf-8 decoding also needs to go, it should assume ASCII, and throw on failure

I'm not sure you can remove the node::Utf8Value name(env->isolate(), string); code as it's needed to convert a Local<String> into a const char*

@saghul

saghul commented Jan 18, 2019

Copy link
Copy Markdown
Member

I'm not sure you can remove the node::Utf8Value name(env->isolate(), string); code as it's needed to convert a Local<String> into a const char*

Isn't there an equivalent that does the job in ASCII instead?

@santigimeno

santigimeno commented Jan 18, 2019

Copy link
Copy Markdown
MemberAuthor

Isn't there an equivalent that does the job in ASCII instead?

I don't know, but what's the issue being ASCII a subset of UTF-8?

silverwind
silverwind reviewed Jan 18, 2019
View reviewed changes
Comment threadtest/common/internet.js
// An accessible IPv4 DNS server
DNS6_SERVER: '2001:4860:4860::8888'
DNS6_SERVER: '2001:4860:4860::8888',
IDNA_HOST: 'españa.icom.museum'

@silverwindsilverwindJan 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

domaintest.みんな might be a better testing domain name, info here.

@saghulsaghulJan 22, 2019

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.

👍 also, I'd say a comment is missing.

@bnoordhuis

bnoordhuis commented Jan 21, 2019

Copy link
Copy Markdown
Member

@santigimeno Do you have access to node-private? Because this PR is a spiritual duplicate of https://github.com/nodejs-private/node-private/pull/147 :)

@santigimeno

santigimeno commented Jan 21, 2019

Copy link
Copy Markdown
MemberAuthor

Do you have access to node-private?

Not really, first time I hear about that repo. Feel free to close this if it's already handled there.

@bnoordhuisbnoordhuis mentioned this pull request Jan 24, 2019
Closed
@bnoordhuis

bnoordhuis commented Jan 24, 2019

Copy link
Copy Markdown
Member

@santigimeno Sorry for the delay, Santi. I've moved it to #25679.

@santigimeno

santigimeno commented Jan 24, 2019

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #25679. Thanks for the heads up Ben.

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

Reviewers

@targostargostargos left review comments
@cjihrigcjihrigcjihrig left review comments
@jasnelljasnelljasnell approved these changes
+2 more reviewers
@silverwindsilverwindsilverwind left review comments
@saghulsaghulsaghul left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

dnsIssues and PRs related to the dns subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DNS resolution fails for internationalized domain names

8 participants

@santigimeno@nodejs-github-bot@targos@saghul@bnoordhuis@silverwind@jasnell@cjihrig