Skip to content

net: validate non-string host for socket.connect #57198

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 11, 2025
Merged

net: validate non-string host for socket.connect #57198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1311,6 +1311,8 @@ function lookupAndConnect(self, options) {
const host = options.host || 'localhost';
let { port, autoSelectFamilyAttemptTimeout, autoSelectFamily } = options;

validateString(host, 'options.host');
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 just validate this on C++? We could just replace the IsString() assertion.

Copy link
Member Author

@daeyeon daeyeon Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, but since the net module has JS layer, validating it in JS seems better for performance. Plus, since other validations are handled in this function, keeping this pattern feels more cohesive.

Copy link
Member

Choose a reason for hiding this comment

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

How come validating it on JS is better? I'm not following.

Copy link
Member Author

Choose a reason for hiding this comment

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

For simple string validations, JS is typically faster since it avoids crossing the JS/C++ boundary. For complex cases, C++ might be more efficient. Let me know if I'm missing anything.

Copy link
Member

Choose a reason for hiding this comment

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

Right now you are adding a branch to both happy and bad path in the benefit of improving bad path (invalid input).

In terms of performance, validating at the C++ and removing that assertion should be the most optimum solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I get it now. The function where the assertion occurs is also used internally in places other than socket.connect. In those cases, IsString() assertion seems more appropriate than throwing an exception with option.host. I'll look into it further.

Copy link
Member Author

Choose a reason for hiding this comment

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

static void node::TCPWrap::Connect(const v8::FunctionCallbackInfov8::Value&, std::function<int(const char*, T*)>) [with T = sockaddr_in] at ../src/tcp_wrap.cc:325
Assertion failed: args[1]->IsString()

@anonrig

args[1] in TCPWrap::Connect may not be the options.host passed in from users.

So, it may not be appropriate to change it like below. It seems better to leave the validation in JS like others.

template <typename T>
void TCPWrap::Connect(const FunctionCallbackInfo<Value>& args,
    std::function<int(const char* ip_address, T* addr)> uv_ip_addr) {
  Environment* env = Environment::GetCurrent(args);

  TCPWrap* wrap;
  ASSIGN_OR_RETURN_UNWRAP(
      &wrap, args.This(), args.GetReturnValue().Set(UV_EBADF));

  CHECK(args[0]->IsObject());
- CHECK(args[1]->IsString());
+ THROW_AND_RETURN_IF_NOT_STRING(env, args[1], "options.host");

Copy link
Member Author

Choose a reason for hiding this comment

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

@anonrig, could you provide feedback ?


if (localAddress && !isIP(localAddress)) {
throw new ERR_INVALID_IP_ADDRESS(localAddress);
}
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-net-connect-options-invalid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,15 @@ const net = require('net');
});
});
}

{
assert.throws(() => {
net.createConnection({
host: ['192.168.0.1'],
port: 8080,
});
}, {
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError',
});
}
Loading