From 78ca61e2cfda9b1e227b0339831ce7507921bc13 Mon Sep 17 00:00:00 2001 From: Denys Otrishko Date: Mon, 22 Jun 2020 19:53:38 +0300 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/33930 PR-URL: https://github.com/nodejs/node/pull/34022 Reviewed-By: Luigi Pinca Reviewed-By: James M Snell Reviewed-By: Zeyu Yang --- lib/net.js | 7 +++- test/parallel/test-net-connect-no-arg.js | 35 +++++++++++++++++++ .../parallel/test-net-connect-options-port.js | 3 +- test/parallel/test-net-normalize-args.js | 16 ++++----- ...test-tls-connect-allow-half-open-option.js | 4 +-- .../parallel/test-tls-connect-hints-option.js | 1 + .../test-tls-connect-timeout-option.js | 1 + 7 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 test/parallel/test-net-connect-no-arg.js diff --git a/lib/net.js b/lib/net.js index c040434f3ea050..9c5393bd28504b 100644 --- a/lib/net.js +++ b/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 --git a/test/parallel/test-net-connect-no-arg.js b/test/parallel/test-net-connect-no-arg.js new file mode 100644 index 00000000000000..c795ef7f6ebab6 --- /dev/null +++ b/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 --git a/test/parallel/test-net-connect-options-port.js b/test/parallel/test-net-connect-options-port.js index 91598f5f15af2f..30f106b0252d7f 100644 --- a/test/parallel/test-net-connect-options-port.js +++ b/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 --git a/test/parallel/test-net-normalize-args.js b/test/parallel/test-net-normalize-args.js index 347c18d638f5f6..65d569d5d5fd12 100644 --- a/test/parallel/test-net-normalize-args.js +++ b/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 --git a/test/parallel/test-tls-connect-allow-half-open-option.js b/test/parallel/test-tls-connect-allow-half-open-option.js index 46b4eeab898c83..e39eb509a81fa7 100644 --- a/test/parallel/test-tls-connect-allow-half-open-option.js +++ b/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 --git a/test/parallel/test-tls-connect-hints-option.js b/test/parallel/test-tls-connect-hints-option.js index 3f58266ac88263..4653e0a1418bbd 100644 --- a/test/parallel/test-tls-connect-hints-option.js +++ b/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 --git a/test/parallel/test-tls-connect-timeout-option.js b/test/parallel/test-tls-connect-timeout-option.js index 4e4d4325906a97..3c4328d94d917e 100644 --- a/test/parallel/test-tls-connect-timeout-option.js +++ b/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 });