Skip to content

Commit

Permalink
net: check args in net.connect() and socket.connect() calls
Browse files Browse the repository at this point in the history
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: nodejs#33930
  • Loading branch information
lundibundi committed Jun 23, 2020
1 parent 12115eb commit 6711ed5
Show file tree
Hide file tree
Showing 7 changed files with 52 additions and 15 deletions.
7 changes: 6 additions & 1 deletion lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;

Expand Down
35 changes: 35 additions & 0 deletions test/parallel/test-net-connect-no-arg.js
Original file line number Diff line number Diff line change
@@ -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',
});
3 changes: 1 addition & 2 deletions test/parallel/test-net-connect-options-port.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down Expand Up @@ -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 */);
});
}

Expand Down
16 changes: 6 additions & 10 deletions test/parallel/test-net-normalize-args.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
}));
}

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-tls-connect-allow-half-open-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-connect-hints-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-tls-connect-timeout-option.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const assert = require('assert');
const tls = require('tls');

const socket = tls.connect({
port: 42,
lookup: () => {},
timeout: 1000
});
Expand Down

0 comments on commit 6711ed5

Please sign in to comment.