src: support domains with empty labels in i18n by watilde · Pull Request #12707 · nodejs/node

watilde · 2017-04-27T21:17:40Z

Summary

The domainToUnicode and the domainToASCII could be used in the middle of parsing the origin, and it means the input values could be a non-final domain name label. Then the converter should ignore the UIDNA_ERROR_EMPTY_LABEL error. Since long domain name labels and long domain names are invalid already, it might not need to ignore UIDNA_ERROR_LABEL_TOO_LONG and UIDNA_ERROR_DOMAIN_NAME_TOO_LONG.

Refs:

http://icu-project.org/apiref/icu4c/uidna_8h.html#ab04a0655cd1e3bcac5e8f48c18df1a57

node/test/fixtures/url-idna.js

Lines 192 to 201 in 6c21397

    
           // long label 
        
           { 
        
             url: `${'a'.repeat(64)}.com`, 
        
             mode: 'ascii' 
        
           }, 
        
           // long URL 
        
           { 
        
             url: `${`${'a'.repeat(63)}.`.repeat(4)}com`, 
        
             mode: 'ascii' 
        
           },

domainToASCII and domainToUnicode in intl Intl#44

Updates

src: support domains with empty labels in i18n
test: synchronise WPT url test data

Checklist

make -j4 test
tests are included
commit message follows commit guidelines

Affected core subsystem(s)

src, test

watilde · 2017-04-28T08:27:01Z

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

watilde · 2017-04-29T16:51:54Z

cc @nodejs/url

TimothyGu

Since long domain name labels and long domain names are invalid already, it might not need to ignore UIDNA_ERROR_LABEL_TOO_LONG and UIDNA_ERROR_DOMAIN_NAME_TOO_LONG.

Can you elaborate on this? What do you mean by "invalid already"?

TimothyGu · 2017-04-29T18:32:54Z

src/node_i18n.cc

I'd leave off the if as the & will do nothing if the error is not present, and I'd add a reference to https://url.spec.whatwg.org/#concept-domain-to-ascii (especially the VerifyDnsLength bit) for why we are ignoring this error.

watilde · 2017-04-29T23:15:40Z

@TimothyGu Sorry for my unclear description. I meant we cannot ignore UIDNA_ERROR_LABEL_TOO_LONG and UIDNA_ERROR_DOMAIN_NAME_TOO_LONG that were mentioned in nodejs/Intl#44 since they are not allowed as is written in the test code.

--
PS
Oops, we should allow the long labels/domains as well in domainToASCII because of VerifyDnsLength set to false as you said. I'm going to update it.

watilde · 2017-04-30T01:09:38Z

new ci: https://ci.nodejs.org/job/node-test-pull-request/7754/

TimothyGu · 2017-04-30T03:57:58Z

src/node_i18n.cc

I'd add an explicit reference to the VerifyDnsLength option.

// The WHATWG URL "domain to ASCII" algorithm explicitly sets the // VerifyDnsLength flag to false, which disables the domain name length // verification step in ToASCII (as specified by UTS #46). Unfortunately, // ICU4C's IDNA module does not support disabling this flag through `options`, // so just filter out the errors that may be caused by the verification step // afterwards.

Thank you for the actual comment example! I put it into the commit :)

TimothyGu · 2017-04-30T04:32:07Z

src/node_i18n.cc

Hmm this is a bit tricky. A more detailed comment like the comment should be warranted:

// UTS #46's ToUnicode operation applies no validation of domain name length // (nor a flag requesting it to do so, like VerifyDnsLength for ToASCII). For // that reason, unlike ToASCII below, ICU4C correctly accepts long domain // names. However, ICU4C still sets the EMPTY_LABEL error in contrary to UTS // #46. Therefore, explicitly filters out that error here.

Follow the spec of domainToASCII/domainToUnicode in whatwg, and synchronise WPT url test data. Refs: web-platform-tests/wpt#5397

watilde · 2017-04-30T19:41:28Z

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

TimothyGu

Many thanks for upstreaming the WPT changes as well!

Follow the spec of domainToASCII/domainToUnicode in whatwg, and synchronise WPT url test data. Refs: web-platform-tests/wpt#5397 PR-URL: #12707 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

watilde · 2017-05-01T17:24:41Z

landed in 0f58d3c

Follow the spec of domainToASCII/domainToUnicode in whatwg, and synchronise WPT url test data. Refs: web-platform-tests/wpt#5397 PR-URL: nodejs#12707 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

TimothyGu · 2017-05-11T08:29:20Z

Looks like we should backport this to v7.x, no? /cc @watilde

watilde · 2017-05-11T11:46:20Z

Yes, we should backport it into v7. I'm not sure I should make a patch for it, will check it if it makes conflicts.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Apr 27, 2017

watilde added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Apr 27, 2017

jasnell approved these changes Apr 27, 2017

View reviewed changes

TimothyGu reviewed Apr 29, 2017

View reviewed changes

watilde force-pushed the feature/url-test branch 3 times, most recently from dd3b613 to f06dd84 Compare April 30, 2017 00:59

TimothyGu reviewed Apr 30, 2017

View reviewed changes

src: support domains with empty labels

d080abb

Follow the spec of domainToASCII/domainToUnicode in whatwg, and synchronise WPT url test data. Refs: web-platform-tests/wpt#5397

watilde force-pushed the feature/url-test branch from f06dd84 to d080abb Compare April 30, 2017 15:03

TimothyGu approved these changes May 1, 2017

View reviewed changes

watilde closed this May 1, 2017

watilde deleted the feature/url-test branch May 1, 2017 17:24

TimothyGu added dont-land-on-v4.x labels May 11, 2017

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

leonardoadame mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#427

Open

aliscco mentioned this pull request Dec 25, 2023

[Snyk] Security upgrade jest from 26.4.2 to 28.0.0 aliscco/alisco-node#1181

Open

leonardoadame mentioned this pull request Jan 3, 2024

[Snyk] Security upgrade jest from 26.6.3 to 28.0.0 leonardoadame/node#590

Open

leonardoadame mentioned this pull request May 13, 2024

[Snyk] Fix for 2 vulnerabilities leonardoadame/node#831

Open

This was referenced May 14, 2024

[Snyk] Security upgrade jest from 24.9.0 to 28.0.0 aliscco/alisco-node#1452

Open

[Snyk] Fix for 2 vulnerabilities aliscco/alisco-node#1499

Open

leonardoadame mentioned this pull request May 16, 2024

[Snyk] Security upgrade jest from 25.5.4 to 28.0.0 leonardoadame/node#969

Open

	// long label
	{
	url: `${'a'.repeat(64)}.com`,
	mode: 'ascii'
	},
	// long URL
	{
	url: `${`${'a'.repeat(63)}.`.repeat(4)}com`,
	mode: 'ascii'
	},

Uh oh!

Conversation

watilde commented Apr 27, 2017

Summary

Updates

Checklist

Affected core subsystem(s)

Uh oh!

watilde commented Apr 28, 2017

Uh oh!

watilde commented Apr 29, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 29, 2017

Choose a reason for hiding this comment

Uh oh!

watilde commented Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

watilde commented Apr 30, 2017

Uh oh!

TimothyGu Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

watilde Apr 30, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Apr 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde commented Apr 30, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

watilde commented May 1, 2017

Uh oh!

TimothyGu commented May 11, 2017

Uh oh!

watilde commented May 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

watilde commented Apr 29, 2017 •

edited

Loading

TimothyGu Apr 30, 2017 •

edited

Loading