net: validate non-string host for socket.connect

[daeyeon]

This fixes another issue discovered while reviewing #57112.

Internally, socket.connect checks the host string, implicitly coercing an array to a string in the process. That leads to the error below.

importnetfrom'node:net';net.createConnection({host: ['192.168.0.1'],port: 8080,});

# node[2568546]: static void node::TCPWrap::Connect(const v8::FunctionCallbackInfo<v8::Value>&, std::function<int(const char*, T*)>) [with T = sockaddr_in] at ../src/tcp_wrap.cc:325# Assertion failed: args[1]->IsString() ----- Native stack trace ----- 1: 0x102d7b7 node::Assert(node::AssertionInfo const&) [node] 2: 0x11aa8c2 void node::TCPWrap::Connect<sockaddr_in>(v8::FunctionCallbackInfo<v8::Value> const&, std::function<int (char const*, sockaddr_in*)>) [node] 3: 0x11a9a73 node::TCPWrap::Connect(v8::FunctionCallbackInfo<v8::Value> const&) [node] 4: 0x7f5b17e0f186 ----- JavaScript stack trace ----- 1: internalConnect (node:net:1096:26) 2: defaultTriggerAsyncIdScope (node:internal/async_hooks:464:18) 3: node:net:1353:9 4: processTicksAndRejections (node:internal/process/task_queues:85:11)

Signed-off-by: Daeyeon Jeong [email protected]

[nodejs-github-bot]

Review requested:

• @nodejs/net

[anonrig]

Why don't we just validate this on C++? We could just replace the IsString() assertion.

[daeyeon]

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.

[anonrig]

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

[daeyeon]

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.

[anonrig]

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.

[daeyeon]

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.

[daeyeon]

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");

[daeyeon]

@anonrig, could you provide feedback ?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.31%. Comparing base (85c0f7a) to head (7a7ae44).
Report is 116 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #57198 +/- ## ========================================== - Coverage 90.33% 90.31% -0.02%  ========================================== Files 630 630 Lines 184513 184515 +2 Branches 36076 36067 -9 ========================================== - Hits 166674 166650 -24 - Misses 10953 10961 +8 - Partials 6886 6904 +18 

Files with missing lines	Coverage Δ
lib/net.js	94.84% <100.00%> (+<0.01%)	⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[anonrig]

I see that this has an approval now. Blocking it until my comment is addressed.

[nodejs-github-bot]

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

I believe this patch aligns with our general style and is sufficient for now. Since there has been no response for over a week, I will dismiss the objection.

[nodejs-github-bot]

Landed in c3ed292