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.

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

URL unexpectedly removes single digit ports #50235

Closed
rlindner81 opened this issue Oct 18, 2023 · 11 comments · Fixed by ada-url/ada#540
Closed

URL unexpectedly removes single digit ports #50235

rlindner81 opened this issue Oct 18, 2023 · 11 comments · Fixed by ada-url/ada#540
Labels
confirmed-bug Issues with confirmed bugs. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@rlindner81
Copy link

Version

v18.18.2

Platform

Darwin M12VHYH70W 23.0.0 Darwin Kernel Version 23.0.0: Fri Sep 15 14:41:43 PDT 2023; root:xnu-10002.1.13~1/RELEASE_ARM64_T6000 arm64

Subsystem

url

What steps will reproduce the bug?

const { URL } = require("url");

let url;

url = new URL("http://test.com:5");
url.pathname = url.pathname.concat("path");
console.log(url.toString()); // works as expected without url params

url = new URL("http://test.com:5/?param=1");
url.pathname = url.pathname.concat("path");
console.log(url.toString()); // broken

url = new URL("http://test.com:15/?param=1");
url.pathname = url.pathname.concat("path");
console.log(url.toString()); // works as expected with double digit ports

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

consistently wrong

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

http://test.com:5/path
http://test.com:5/path?param=1
http://test.com:15/path?param=1

What do you see instead?

http://test.com:5/path
http://test.com/path?param=1
http://test.com:15/path?param=1

Additional information

Having the url parameter set makes urllib forget a single digit port.

@debadree25 debadree25 added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Oct 18, 2023
@debadree25
Copy link
Member

cc @nodejs/url

@richardlau richardlau added the v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. label Oct 18, 2023
@aduh95 aduh95 added confirmed-bug Issues with confirmed bugs. and removed v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Oct 18, 2023
@aduh95
Copy link
Contributor

aduh95 commented Oct 18, 2023

I can reproduce on the latest main.

@anonrig
Copy link
Member

anonrig commented Oct 18, 2023

cc @lemire

@MrJithil
Copy link
Member

Great. I can write a fix.

@lemire
Copy link
Member

lemire commented Oct 19, 2023

It is indeed a bug in ada-url/ada. I have a verification and a potential fix upstream.

Thank you for the clear report with reproducible cases.

@rlindner81
Copy link
Author

@lemire @anonrig Thanks for the very quick fix!

Could you give a rough estimate when this fix will reach v18 bugfix releases?

@lemire
Copy link
Member

lemire commented Oct 19, 2023

@anonrig Should be able to tell you.

@baymer
Copy link

baymer commented Nov 17, 2023

When will it appear in the 20th node?

@anonrig
Copy link
Member

anonrig commented Nov 17, 2023

The fix is included in Ada v2.7.3 (Ref commit : ada-url/ada@d4ab1a6). It is not yet merged into Node.js core.

@anonrig
Copy link
Member

anonrig commented Nov 17, 2023

I retriggered the update script: #50771

@anonrig
Copy link
Member

anonrig commented Nov 17, 2023

@nodejs/releasers I added lts-watch-v18 and lts-watch-v20 to the version update pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants