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: emit an error when custom lookup resolves a non-string address #57192

Merged
merged 2 commits into from
Mar 1, 2025
Merged

net: emit an error when custom lookup resolves a non-string address #57192

merged 2 commits into from
Mar 1, 2025
+45 −1

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Feb 24, 2025 •
edited
Loading

Fixes: #57112

This PR fixes the crash when the lookup function resolves to a non-string address. Instead, it will emit an ERR_INVALID_IP_ADDRESS error.

@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 Feb 24, 2025
@geeksilva97 geeksilva97 marked this pull request as ready for review February 24, 2025 03:57
@geeksilva97 geeksilva97 changed the title net: throw error when custom lookup resolves a non-string address net: throw error when custom lookup resolves to a non-string address Feb 24, 2025
@codecov Codecov
Copy link

codecov bot commented Feb 24, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.26%. Comparing base (c864dea) to head (94c760a).
Report is 27 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #57192      +/-   ##
==========================================
- Coverage   90.33%   90.26%   -0.07%     
==========================================
  Files         630      630              
  Lines      184537   184634      +97     
  Branches    36077    36125      +48     
==========================================
- Hits       166693   166653      -40     
- Misses      10950    11028      +78     
- Partials     6894     6953      +59
Files with missing lines Coverage Δ
lib/net.js 94.95% <100.00%> (+0.11%) ⬆️

... and 34 files with indirect coverage changes

@lpinca
Copy link
Member

lpinca commented Feb 24, 2025

The "throw" word is a bit misleading, no error is thrown.

@geeksilva97
Copy link
Contributor Author

The "throw" word is a bit misleading, no error is thrown.

Got it. Would you have a suggestion?

@lpinca
Copy link
Member

lpinca commented Feb 24, 2025

How about "handle" or "emit" an error?

@geeksilva97
Copy link
Contributor Author

How about "handle" or "emit" an error?

yeah, much better. Thank you

@lpinca
Copy link
Member

lpinca commented Feb 24, 2025

Would you mind fixing the first commit message as well?

@geeksilva97
Copy link
Contributor Author

Would you mind fixing the first commit message as well?

nice catch. will do

@geeksilva97 geeksilva97 force-pushed the fix-dns-lookup-on-net branch from 62885b9 to 831b908 Compare February 24, 2025 15:56
@geeksilva97 geeksilva97 changed the title net: throw error when custom lookup resolves to a non-string address net: emit an error when custom lookup resolves a non-string address Feb 24, 2025
@geeksilva97
Copy link
Contributor Author

Would you mind fixing the first commit message as well?

nice catch. will do

Done. I squashed into a single commit :)

@lpinca
Copy link
Member

lpinca commented Feb 24, 2025 •
edited
Loading

One final nit, feel free to ignore: test/parallel/test-net-connect-custom-lookup-function.mjs -> test/parallel/test-net-connect-custom-lookup-non-string-address.mjs

@geeksilva97
Copy link
Contributor Author

Could you add a request-ci, @lpinca ?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 24, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@geeksilva97 geeksilva97 force-pushed the fix-dns-lookup-on-net branch from a5ba0b1 to b6ff5b2 Compare February 25, 2025 00:19
@geeksilva97
Copy link
Contributor Author

The last push was just one more squash - and to run the CI now it's fixed.

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@nodejs-github-bot
Copy link
Collaborator

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

jasnell
jasnell reviewed Feb 25, 2025
View reviewed changes
targos
targos requested changes Feb 27, 2025
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I think we should not coerce to string nor use isStringObject.

@geeksilva97
Copy link
Contributor Author

I think we should not coerce to string nor use isStringObject.

Would you have a suggestion?

targos
targos reviewed Feb 27, 2025
View reviewed changes
@geeksilva97 geeksilva97 force-pushed the fix-dns-lookup-on-net branch from b6ff5b2 to 94c760a Compare February 27, 2025 13:33
@geeksilva97 geeksilva97 requested a review from targos February 27, 2025 13:35
@jazelly jazelly added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 27, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Feb 28, 2025
anonrig
anonrig reviewed Mar 1, 2025
View reviewed changes
lib/net.js
@@ -1416,7 +1416,7 @@ function lookupAndConnect(self, options) {
// calls net.Socket.connect() on it (that's us). There are no event
// listeners registered yet so defer the error event to the next tick.
process.nextTick(connectErrorNT, self, err);
} else if (!isIP(ip)) {
} else if ((typeof ip !== 'string') || !isIP(ip)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we handle this inside isIp() function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point. I think this is reasonable to have this inside of isIP. Let me check.

Copy link
Member

Choose a reason for hiding this comment

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

isIP is a public API. It would be a breaking change to do it.

@geeksilva97 geeksilva97 added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 1, 2025
@nodejs-github-bot nodejs-github-bot merged commit 71196c4 into nodejs:main Mar 1, 2025
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 71196c4

aduh95 pushed a commit that referenced this pull request Mar 9, 2025
@geeksilva97 @aduh95
net: emit an error when custom lookup resolves to a non-string address
a757f00 
PR-URL: #57192
Fixes: #57112
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@github-actions github-actions bot mentioned this pull request Mar 12, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@anonrig anonrig anonrig left review comments

@targos targos targos approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@pimterry pimterry pimterry approved these changes

@daeyeon daeyeon daeyeon approved these changes

@jazelly jazelly jazelly approved these changes

@mertcanaltin mertcanaltin mertcanaltin 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AssertionError: args[1]->IsString() when custom DNS lookup callback returns array instead of string