net: check args in net.connect() and socket.connect() calls

Previously Node.js would handle empty `net.connect()` and
`socket.connect()` call as if the user passed empty options object which
doesn't really make sense. This was due to the fact that it uses the
same `normalizeArgs` function as `.listen()` call where such call is
perfectly fine.

This will make it clear what is the problem with such call and how it
can be resolved. It now throws `ERR_MISSING_ARGS` if no arguments were
passed or neither `path` nor `port` is specified.

Fixes: #33930

PR-URL: #34022
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>

Author: lundibundi

Diff for: lib/net.js

@@ -95,7 +95,8 @@ const {
     ERR_INVALID_OPT_VALUE,
     ERR_SERVER_ALREADY_LISTEN,
     ERR_SERVER_NOT_RUNNING,
-    ERR_SOCKET_CLOSED
+    ERR_SOCKET_CLOSED,
+    ERR_MISSING_ARGS,
   },
   errnoException,
   exceptionWithHostPort,
@@ -921,6 +922,10 @@ Socket.prototype.connect = function(...args) {
   const options = normalized[0];
   const cb = normalized[1];
 
+  // options.port === null will be checked later.
+  if (options.port === undefined && options.path == null)
+    throw new ERR_MISSING_ARGS(['options', 'port', 'path']);
+
   if (this.write !== Socket.prototype.write)
     this.write = Socket.prototype.write;
 

Diff for: test/parallel/test-net-connect-no-arg.js

@@ -0,0 +1,35 @@
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const net = require('net');
+
+// Tests that net.connect() called without arguments throws ERR_MISSING_ARGS.
+
+assert.throws(() => {
+  net.connect();
+}, {
+  code: 'ERR_MISSING_ARGS',
+  message: 'The "options" or "port" or "path" argument must be specified',
+});
+
+assert.throws(() => {
+  new net.Socket().connect();
+}, {
+  code: 'ERR_MISSING_ARGS',
+  message: 'The "options" or "port" or "path" argument must be specified',
+});
+
+assert.throws(() => {
+  net.connect({});
+}, {
+  code: 'ERR_MISSING_ARGS',
+  message: 'The "options" or "port" or "path" argument must be specified',
+});
+
+assert.throws(() => {
+  new net.Socket().connect({});
+}, {
+  code: 'ERR_MISSING_ARGS',
+  message: 'The "options" or "port" or "path" argument must be specified',
+});

Diff for: test/parallel/test-net-connect-options-port.js

@@ -60,7 +60,7 @@ const net = require('net');
 {
   // connect({hint}, cb) and connect({hint})
   const hints = (dns.ADDRCONFIG | dns.V4MAPPED | dns.ALL) + 42;
-  const hintOptBlocks = doConnect([{ hints }],
+  const hintOptBlocks = doConnect([{ port: 42, hints }],
                                   () => common.mustNotCall());
   for (const fn of hintOptBlocks) {
     assert.throws(fn, {
@@ -95,7 +95,6 @@ const net = require('net');
   // Try connecting to random ports, but do so once the server is closed
   server.on('close', () => {
     asyncFailToConnect(0);
-    asyncFailToConnect(/* undefined */);
   });
 }
 

Diff for: test/parallel/test-net-normalize-args.js

@@ -4,7 +4,6 @@ const common = require('../common');
 const assert = require('assert');
 const net = require('net');
 const { normalizedArgsSymbol } = require('internal/net');
-const { getSystemErrorName } = require('util');
 
 function validateNormalizedArgs(input, output) {
   const args = net._normalizeArgs(input);
@@ -27,18 +26,15 @@ validateNormalizedArgs([{ port: 1234 }, assert.fail], res);
   const server = net.createServer(common.mustNotCall('should not connect'));
 
   server.listen(common.mustCall(() => {
-    const possibleErrors = ['ECONNREFUSED', 'EADDRNOTAVAIL'];
     const port = server.address().port;
     const socket = new net.Socket();
 
-    socket.on('error', common.mustCall((err) => {
-      assert(possibleErrors.includes(err.code));
-      assert(possibleErrors.includes(getSystemErrorName(err.errno)));
-      assert.strictEqual(err.syscall, 'connect');
-      server.close();
-    }));
-
-    socket.connect([{ port }, assert.fail]);
+    assert.throws(() => {
+      socket.connect([{ port }, assert.fail]);
+    }, {
+      code: 'ERR_MISSING_ARGS'
+    });
+    server.close();
   }));
 }
 

Diff for: test/parallel/test-tls-connect-allow-half-open-option.js

@@ -12,12 +12,12 @@ const fixtures = require('../common/fixtures');
 const tls = require('tls');
 
 {
-  const socket = tls.connect({ lookup() {} });
+  const socket = tls.connect({ port: 42, lookup() {} });
   assert.strictEqual(socket.allowHalfOpen, false);
 }
 
 {
-  const socket = tls.connect({ allowHalfOpen: false, lookup() {} });
+  const socket = tls.connect({ port: 42, allowHalfOpen: false, lookup() {} });
   assert.strictEqual(socket.allowHalfOpen, false);
 }
 

Diff for: test/parallel/test-tls-connect-hints-option.js

@@ -22,6 +22,7 @@ assert.notStrictEqual(hints, dns.V4MAPPED | dns.ALL);
 assert.notStrictEqual(hints, dns.ADDRCONFIG | dns.V4MAPPED | dns.ALL);
 
 tls.connect({
+  port: 42,
   lookup: common.mustCall((host, options) => {
     assert.strictEqual(host, 'localhost');
     assert.deepStrictEqual(options, { family: undefined, hints });

Diff for: test/parallel/test-tls-connect-timeout-option.js

@@ -12,6 +12,7 @@ const assert = require('assert');
 const tls = require('tls');
 
 const socket = tls.connect({
+  port: 42,
   lookup: () => {},
   timeout: 1000
 });