net: exclude ipv6 loopback addresses from server.listen by puskin · Pull Request #54264 · nodejs/node

puskin · 2024-08-08T10:18:59Z

This is not a direct take at the dns module. In this PR we discussed how it is better to approach the issue editing the net module instead, filtering out IPV6 loopback addresses when possible.

Fixes: nodejs#51732

nodejs-github-bot · 2024-08-08T10:19:04Z

Review requested:

@nodejs/net

avivkeller · 2024-08-08T12:09:35Z

+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+


Suggested change
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
If you're adding a file, I don't think it needs the copyright notice. I have no legal experience, so I might be wrong.

vbraun · 2024-08-08T12:58:39Z

Its technically possible to bind to a link-local address with scope fe80::1%eth0, just not to an unscoped link-local address fe80::1. Though

Thats a very weird use case
I've never tried whether that actually currently works with nodejs/net
Web browser clients don't support that, so it would have to be a non-web service
DNS lookup doesn't return the scope, because scope is different for each client. So the problem is sort of special to DNS

IMHO the problem of randomly picking nonsense ipv6 is much worse than the potential regression (if there actually is one)

puskin · 2024-08-08T13:30:20Z

@vbraun very good point. Trying the new test I wrote with something like fe80::dc30:4dff:fede:ce33%awdl0 the call to server.listen does not throw the EADDRNOTAVAIL error but starts listening even tho, as you rightfully mentioned, it is a weird case.
If we want to be super picky I could "loosen" the check and see if the address has a % in the ip, otherwise we can keep it as it

codecov · 2024-08-09T04:21:25Z

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 87.32%. Comparing base (88bac52) to head (f04cc46).
Report is 135 commits behind head on main.

Files	Patch %	Lines
src/cares_wrap.cc	66.66%	3 Missing and 2 partials ⚠️
lib/net.js	97.22%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #54264 +/- ##
==========================================
+ Coverage 87.11% 87.32% +0.21% 
==========================================
Files 647 648 +1 Lines 181754 182431 +677 Branches 34885 35001 +116 ==========================================
+ Hits 158332 159309 +977 + Misses 16738 16387 -351 - Partials 6684 6735 +51

Files	Coverage Δ
lib/net.js	`92.81% <97.22%> (+0.17%)`	⬆️
src/cares_wrap.cc	`65.44% <66.66%> (+0.01%)`	⬆️

... and 95 files with indirect coverage changes

puskin · 2024-08-09T11:15:46Z

@ShogunPanda fixed a broken test, it looks like the call to server.listen fails with a different error code on a mac compared to linux. Glad to see you here too 😄

Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>

ShogunPanda

LGTM!

mcollina

lgtm

mcollina · 2024-08-20T08:18:53Z

I fear it might be a breaking change, but I can't point to the problem. I've added a backing-for-lts label and semver-minor.

nodejs-github-bot · 2024-08-20T10:24:41Z

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

nodejs-github-bot · 2024-08-20T15:19:32Z

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

nodejs-github-bot · 2024-08-22T14:54:53Z

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

nodejs-github-bot · 2024-08-23T09:11:45Z

Landed in 628469c

Fixes: #51732 PR-URL: #54264 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 PR-URL: #54560

Fixes: #51732 PR-URL: #54264 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: net: * (SEMVER-MINOR) exclude ipv6 loopback addresses from server.listen (Giovanni Bucci) #54264 src: * (SEMVER-MINOR) add JS APIs for compile cache and NODE_DISABLE_COMPILE_CACHE (Joyee Cheung) #54501 src,lib: * (SEMVER-MINOR) add performance.uvMetricsInfo (Rafael Gonzaga) #54413 test_runner: * (SEMVER-MINOR) add support for coverage thresholds (Aviv Keller) #54429 * (SEMVER-MINOR) support running tests in process (Colin Ihrig) #53927 * (SEMVER-MINOR) defer inheriting hooks until run() (Colin Ihrig) #53927 vm: * (SEMVER-MINOR) introduce vanilla contexts via vm.constants.DONT_CONTEXTIFY (Joyee Cheung) #54394 PR-URL: #54560

yonran · 2025-08-07T01:05:25Z

The PR title and commit message are incorrect. This function filterOnlyValidAddress excludes link-local addresses (fe80::/10, or fe80:: to febf:ffff:ffff:ffff:ffff:ffff:ffff:ffff), not the loopback address (::1).

net: exclude ipv6 loopback addresses from server.listen
9e65b33
Fixes: nodejs#51732

nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Aug 8, 2024

puskin mentioned this pull request Aug 8, 2024
dns: exclude loopback link from the single lookup resolution #54249
Closed

net: changed the replacing of the dns module with a proper mock
804842d

avivkeller reviewed Aug 8, 2024
View reviewed changes

Giovanni added 2 commits August 9, 2024 08:13

dns: removed copyright from new test and fixed broken one
5dec579

dns: added automated test to check the error emission in lookupAndListen
bbf4e0b

puskin requested a review from avivkeller August 9, 2024 06:18

ShogunPanda approved these changes Aug 9, 2024
View reviewed changes

dns: fixed the broken test. On mac it is failing with a different code
6a5f988

pimterry requested changes Aug 9, 2024
View reviewed changes

Comment threadlib/net.js Outdated
Comment threadlib/net.js Outdated
Comment threadlib/net.js

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Aug 9, 2024

net: checking the whole range of ipv6 link-back addresses
1896a8e

puskin requested a review from pimterry August 10, 2024 17:23

net: bridging native uv_inet_pton to js to parse ip to binary buffer
97d8fc3

avivkeller reviewed Aug 19, 2024
View reviewed changes

Comment threadtest/parallel/test-net-server-close-before-calling-lookup-callback.js Outdated
Comment threadtest/sequential/test-net-server-listen-ipv6-link-local.js Outdated

Giovanni Bucciand others added 2 commits August 19, 2024 19:50

test: update case of comment in a test
195b631
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>

test: updated the message when ipv6 is not enabled for link-local test
0f8eb48
Co-authored-by: Aviv Keller <38299977+RedYetiDev@users.noreply.github.com>

ShogunPanda approved these changes Aug 20, 2024
View reviewed changes

mcollina approved these changes Aug 20, 2024
View reviewed changes

mcollina added baking-for-lts PRs that need to wait before landing in a LTS release. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 20, 2024

net: formatted cares_wrap.cc to fix linting issue
f04cc46

pimterry approved these changes Aug 20, 2024
View reviewed changes

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024

pimterry added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2024

pimterry added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024

nodejs-github-bot merged commit 628469c into nodejs:mainAug 23, 2024

This was referenced Aug 25, 2024
Revert "net: validate host name for server listen" #54554
Closed
test: refactor test_runner tests to change default reporter #54547
Merged

RafaelGSS mentioned this pull request Aug 25, 2024
v22.8.0 proposal #54560
Merged

aduh95 removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Feb 3, 2025

Uh oh!

Uh oh!

Conversation

puskin commented Aug 8, 2024

Uh oh!

nodejs-github-bot commented Aug 8, 2024

Uh oh!

avivkellerAug 8, 2024

Choose a reason for hiding this comment

Uh oh!

vbraun commented Aug 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puskin commented Aug 8, 2024

Uh oh!

codecovBot commented Aug 9, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

puskin commented Aug 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ShogunPanda left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 20, 2024

Uh oh!

nodejs-github-bot commented Aug 20, 2024

Uh oh!

nodejs-github-bot commented Aug 20, 2024

Uh oh!

nodejs-github-bot commented Aug 22, 2024

Uh oh!

nodejs-github-bot commented Aug 23, 2024

Uh oh!

yonran commented Aug 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

vbraun commented Aug 8, 2024•
edited
Loading

codecovBot commented Aug 9, 2024•
edited
Loading