net: reject non-address SocketAddress.parse input

[araujogui]

SocketAddress.parse() no longer builds a http://${input} URL and reads the hostname and port back out of the Ada WHATWG URL parser. The input is now split in C++ into host, port and IPv6 zone id, and the host is validated by uv_inet_pton() — the same check the SocketAddress constructor already used.

The Ada URL parser is deliberately lenient about hosts, so parse() accepted much that is not a socket address: legacy IPv4 forms (0177.0.0.1, 0x7f.0.0.1, 2130706433, 127.1), URL syntax (user@1.2.3.4:80, 1.2.3.4:80/foo), and input it rewrites first, such as embedded tabs and non-ASCII digits that IDNA maps to ASCII. Those legacy forms are the ambiguity behind CVE-2021-29923 and CVE-2021-29922. It also disagreed with the constructor, which rejects 0177.0.0.1 with ERR_INVALID_ADDRESS.

Separately, SocketAddress.parse('1.2.3.4:80') returned port 0, because the URL parser drops a port equal to the scheme's default. It now returns 80, fixes #62906.

The accepted grammar is documented in doc/api/net.md. A zone id must be numeric, since libuv resolves one only as an interface name.

[nodejs-github-bot]

Review requested:

• @nodejs/net

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Renegade334]

This would make the port issue much simpler with this additional validation. If the string ends in /:(\d+)$/, then it can just be sliced off, and if the rest of the string gets parsed as a valid hostname by URLParse then the port can be obtained from the digit string with NumberParseInt.

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.07%. Comparing base (9f04fcd) to head (75ae3d0).
⚠️ Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sockaddr.cc	82.60%	0 Missing and 4 partials ⚠️
src/node_sockaddr_parser.cc	98.64%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #64832     +/-   ##
=========================================
  Coverage   90.07%   90.07%             
=========================================
  Files         751      755      +4     
  Lines      254916   256474   +1558     
  Branches    48133    48520    +387     
=========================================
+ Hits       229605   231016   +1411     
- Misses      16496    16569     +73     
- Partials     8815     8889     +74     

Files with missing lines	Coverage Δ
lib/internal/socketaddress.js	98.88% <100.00%> (-0.11%)	⬇️
src/node_sockaddr.h	51.28% <ø> (ø)
src/node_sockaddr_parser.cc	98.64% <98.64%> (ø)
src/node_sockaddr.cc	74.90% <82.60%> (+0.22%)	⬆️

... and 76 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Renegade334]

LGTM!

[nodejs-github-bot]

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

[araujogui]

@Renegade334 maybe we should include semver-major tag?

[nodejs-github-bot]

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

[jasnell]

I'm not yet convinced this is the best way to approach this (with the regex). Won't block for now but there's probably a more performant / reliable way.

[araujogui]

I'm not yet convinced this is the best way to approach this (with the regex). Won't block for now but there's probably a more performant / reliable way.

@jasnell I implemented a new strict socket address parser replacing Ada, it fixes #62906 and is faster. Benchmark results compared to main:

                                                           confidence improvement accuracy (*)   (**)  (***)
net/net-socketaddress-parse.js input='ipv4-port' n=1000000        ***     52.37 %       ±0.63% ±0.84% ±1.10%
net/net-socketaddress-parse.js input='ipv4' n=1000000             ***     43.38 %       ±0.64% ±0.86% ±1.12%
net/net-socketaddress-parse.js input='ipv6-port' n=1000000        ***     59.56 %       ±0.49% ±0.65% ±0.85%
net/net-socketaddress-parse.js input='ipv6' n=1000000             ***     47.35 %       ±0.50% ±0.66% ±0.86%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

[IlyasShabi]

Those inputs have valid addresses only, can we add others for rejection use case?

[github-actions]

Benchmark GHA (net / net-socketaddress-parse): https://github.com/nodejs/node/actions/runs/33605572788