Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

URL.parse fails on URLs with comma separated hosts #48850

Closed
evansnicholas opened this issue Jul 20, 2023 · 9 comments
Closed

URL.parse fails on URLs with comma separated hosts #48850

evansnicholas opened this issue Jul 20, 2023 · 9 comments
Labels
url Issues and PRs related to the legacy built-in url module. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.

Comments

@evansnicholas
Copy link

Version

v18.17.0

Platform

Linux XXXXX 5.4.0-137-generic #154-Ubuntu SMP Thu Jan 5 17:03:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Parsing URL's with comma separated hosts throws a TypeError [ERR_INVALID_URL]: Invalid URL error. This URL parsed correctly in in node v18.16.1.

const URL = require('url');
URL.parse("mongodb://127.0.0.1,127.0.0.2:27017")

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

Always.

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

URL parsed rather than exception thrown.

What do you see instead?

An exception is thrown.

Additional information

This is a serious problem because such URL's are used reguarly when configuring eg DB connections which now fail. This is the case with the MongoDB driver for instance.
@evansnicholas evansnicholas changed the title URL.parse fails on URLs with comma separated domains URL.parse fails on URLs with comma separated hosts Jul 20, 2023
@VoltrexKeyva VoltrexKeyva added the url Issues and PRs related to the legacy built-in url module. label Jul 20, 2023
@targos targos added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Jul 20, 2023
@3XC1T3D
Copy link

3XC1T3D commented Jul 20, 2023 •
edited
Loading

With node v18.17.0, Ada 2.5.0 is delivered and we have the same issue.

We noticed that in node v20.4.0 , Ada 2.5.1 is delivered and there we dont have the issue.

Maybe it is possible to update also to Ada 2.5.1 in node v18.17.1 or something?

@anonrig
Copy link
Member

anonrig commented Jul 20, 2023

cc @nodejs/url

@FrederickEngelhardt FrederickEngelhardt mentioned this issue Jul 21, 2023
Closed
@aduh95 aduh95 mentioned this issue Jul 21, 2023
Closed
@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

I don't think it is related to Ada? AFAIK url.parse() is the legacy URL parser.

@anonrig
Copy link
Member

anonrig commented Jul 21, 2023

url.parse calls toASCII method which is implemented under ada::idna::to_ascii. to_ascii returns an empty string if the value is invalid. In this particular case, we are receiving an empty string, which may be caused by this. Investigating it under: #48855

@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

Ah ok.

@3XC1T3D
Copy link

3XC1T3D commented Jul 21, 2023

strange that it doesnt cause problems under ada 2.5.1 🙈

This was referenced Jul 21, 2023
Closed
Closed
Closed
nodejs-github-bot pushed a commit that referenced this issue Jul 22, 2023
@anonrig @nodejs-github-bot
test: validate host with commas on url.parse
f4863fa 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
nodejs-github-bot pushed a commit that referenced this issue Jul 22, 2023
@anonrig @nodejs-github-bot
lib: remove invalid parameter to toASCII
9dd574c 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
anonrig added a commit to anonrig/node that referenced this issue Jul 22, 2023
@anonrig
test: validate host with commas on url.parse
a0edb33 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
anonrig added a commit to anonrig/node that referenced this issue Jul 22, 2023
@anonrig
lib: remove invalid parameter to toASCII
60f1bb5 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Jul 27, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
932d92e 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Jul 27, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
9923dd9 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this issue Aug 6, 2023
@anonrig
test: validate host with commas on url.parse
f1d25bc 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this issue Aug 6, 2023
@anonrig
lib: remove invalid parameter to toASCII
c40e40a 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this issue Aug 7, 2023
@anonrig
test: validate host with commas on url.parse
24e9fb4 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
pluris pushed a commit to pluris/node that referenced this issue Aug 7, 2023
@anonrig
lib: remove invalid parameter to toASCII
161ff90 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
ba96a46 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
c433de3 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
@anonrig @Ceres6
test: validate host with commas on url.parse
6c0c565 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Aug 14, 2023
@anonrig @Ceres6
lib: remove invalid parameter to toASCII
bb7ffa8 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this issue Aug 14, 2023
@anonrig @UlisesGascon
test: validate host with commas on url.parse
17a1be6 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this issue Aug 14, 2023
@anonrig @UlisesGascon
lib: remove invalid parameter to toASCII
5affce3 
PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
@anonrig @RafaelGSS
test: validate host with commas on url.parse
3680a66 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 15, 2023
@anonrig @RafaelGSS
lib: remove invalid parameter to toASCII
420b85c 
PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 16, 2023
@anonrig @ruyadorno
test: validate host with commas on url.parse
2de9868 
PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 16, 2023
@anonrig @ruyadorno
lib: remove invalid parameter to toASCII
69aaf8b 
PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@exiadbq
Copy link

exiadbq commented Aug 28, 2023

With node v18.17.0, Ada 2.5.0 is delivered and we have the same issue.

We noticed that in node v20.4.0 , Ada 2.5.1 is delivered and there we dont have the issue.

Maybe it is possible to update also to Ada 2.5.1 in node v18.17.1 or something?

Still happening in v18.17.1, any plan to fix it?

@anonrig
Copy link
Member

anonrig commented Aug 28, 2023

It is fixed at #48873 and will be released in the next Node 18 release.

@anonrig
Copy link
Member

anonrig commented Sep 21, 2023

Resolved with latest v18

@anonrig anonrig closed this as completed Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
url Issues and PRs related to the legacy built-in url module. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[v18.x] backport: src: replace idna functions with ada::idna anonrig/node
7 participants
@lpinca @anonrig @3XC1T3D @targos @evansnicholas @exiadbq @VoltrexKeyva