Skip to content

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

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

Merged
nodejs-github-bot merged 2 commits into from
Mar 1, 2025
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a02a6f0
net: emit an error when custom lookup resolves to a non-string address
geeksilva97 Feb 25, 2025
94c760a
fixup: remove isStringObject check
geeksilva97 Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
fixup: remove isStringObject check
  • Loading branch information
@geeksilva97
geeksilva97 committed Feb 27, 2025
commit 94c760ade60bcd73ce0aaa4b3c7079307fea4d5d
4 changes: 2 additions & 2 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ const {
},
genericNodeError,
} = require('internal/errors');
const { isStringObject, isUint8Array } = require('internal/util/types');
const { isUint8Array } = require('internal/util/types');
const { queueMicrotask } = require('internal/process/task_queues');
const {
guessHandleType,
Expand Down Expand Up @@ -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 ((typeof ip !== 'string' && !isStringObject(ip)) || !isIP(ip)) {
} else if ((typeof ip !== 'string') || !isIP(ip)) {
Copy link
Copy Markdown
Member

@anonrig anonrig Mar 1, 2025

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
Copy Markdown
Contributor Author

@geeksilva97 geeksilva97 Mar 1, 2025

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
Copy Markdown
Member

@targos targos Mar 1, 2025

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.

err = new ERR_INVALID_IP_ADDRESS(ip);
process.nextTick(connectErrorNT, self, err);
} else if (addressType !== 4 && addressType !== 6) {
Expand Down