net: reject non-address SocketAddress.parse input

[araujogui]

No description provided.

[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.10%. Comparing base (58d58a4) to head (9d21a93).
⚠️ Report is 1 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   91.85%   90.10%   -1.75%     
==========================================
  Files         400      753     +353     
  Lines      179110   252400   +73290     
  Branches    27346    47481   +20135     
==========================================
+ Hits       164516   227418   +62902     
- Misses      14264    16271    +2007     
- Partials      330     8711    +8381     

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%> (ø)

... and 482 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 (***)