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

net: exclude ipv6 loopback addresses from server.listen #54264

Merged
merged 10 commits into from
Aug 23, 2024
Merged

net: exclude ipv6 loopback addresses from server.listen #54264

merged 10 commits into from
Aug 23, 2024

Conversation

puskin94
Copy link
Contributor

@puskin94 puskin94 commented Aug 8, 2024

Fixes: #51732

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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot 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
@puskin94 puskin94 mentioned this pull request Aug 8, 2024
Closed
RedYetiDev
RedYetiDev reviewed Aug 8, 2024
View reviewed changes
test/sequential/test-net-server-listen-ipv6-loopback.js Outdated
Comment on lines 1 to 21
// 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

vbraun commented Aug 8, 2024 •
edited
Loading

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)

@puskin94
Copy link
Contributor Author

puskin94 commented Aug 8, 2024

@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 Codecov
Copy link

codecov bot commented Aug 9, 2024 •
edited
Loading

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

@puskin94
Copy link
Contributor Author

puskin94 commented Aug 9, 2024

@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 😄

pimterry
pimterry requested changes Aug 9, 2024
View reviewed changes
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Outdated Show resolved Hide resolved
lib/net.js Show resolved Hide resolved
@RedYetiDev RedYetiDev 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
@puskin94 puskin94 requested a review from pimterry August 10, 2024 17:23
ShogunPanda
ShogunPanda approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

mcollina
mcollina approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina 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
@mcollina
Copy link
Member

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.

@puskin94
Copy link
Contributor Author

Pushed again to fix the linting issue in cares_wrap.cc

ShogunPanda
ShogunPanda approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 20, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@pimterry 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
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@pimterry pimterry added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 23, 2024
@nodejs-github-bot 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 nodejs-github-bot merged commit 628469c into nodejs:main Aug 23, 2024
59 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 628469c

This was referenced Aug 25, 2024
Closed
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 25, 2024
@puskin94 @RafaelGSS
net: exclude ipv6 loopback addresses from server.listen
94eb81a 
Fixes: #51732
PR-URL: #54264
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 25, 2024
@RafaelGSS
2024-08-26, Version 22.8.0 (Current)
42e2b39 
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: TODO
@RafaelGSS RafaelGSS mentioned this pull request Aug 25, 2024
Merged
RafaelGSS added a commit that referenced this pull request Aug 25, 2024
@RafaelGSS
2024-08-26, Version 22.8.0 (Current)
04d4796 
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
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@puskin94 @RafaelGSS
net: exclude ipv6 loopback addresses from server.listen
1e01bdc 
Fixes: #51732
PR-URL: #54264
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
RafaelGSS added a commit that referenced this pull request Aug 30, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
e5b1a53 
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
RafaelGSS added a commit that referenced this pull request Aug 30, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
bce5d12 
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
RafaelGSS added a commit that referenced this pull request Aug 31, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
911a03d 
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
RafaelGSS added a commit that referenced this pull request Aug 31, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
0101f1f 
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
RafaelGSS added a commit that referenced this pull request Sep 1, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
7096d14 
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
RafaelGSS added a commit that referenced this pull request Sep 2, 2024
@RafaelGSS
2024-09-02, Version 22.8.0 (Current)
729abbe 
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
RafaelGSS added a commit that referenced this pull request Sep 3, 2024
@RafaelGSS
2024-09-03, Version 22.8.0 (Current)
78ee90e 
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
RafaelGSS added a commit that referenced this pull request Sep 3, 2024
@RafaelGSS
2024-09-03, Version 22.8.0 (Current)
9e5d2b7 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedYetiDev RedYetiDev RedYetiDev left review comments

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@pimterry pimterry pimterry approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DNS lookup should try to avoid IPv6 link-local addresses
7 participants
@puskin94 @nodejs-github-bot @vbraun @mcollina @ShogunPanda @pimterry @RedYetiDev