net: type check createServer options object #2904

sam-github · 2015-09-16T15:23:48Z

net.createServer('aPipe') and net.createServer(8080) are mistakes, and
now throw a TypeError instead of silently being treated as an object.

net.createServer('aPipe') and net.createServer(8080) are mistakes, and now throw a TypeError instead of silently being treated as an object.

thefourtheye · 2015-09-16T15:25:21Z

test/parallel/test-net-server-options.js

@@ -0,0 +1,7 @@
+'use strict';
+var common = require('../common');
+var assert = require('assert');


cjihrig · 2015-09-17T01:54:12Z

lib/net.js

@@ -1080,12 +1080,14 @@ function Server(options, connectionListener) {
    connectionListener = options;
    options = {};
    self.on('connection', connectionListener);
-  } else {
+  } else if(options == null || typeof options === 'object') {


Space between if and (

Nit: typeof null returns 'object' so you can skip the check for options == null unless the intention is for the not-strict equality to allow undefined, in which case I imagine checking for undefined would be more clear. Meh, maybe not so much. == null is pretty idiomatic for "null or undefined".

cjihrig · 2015-09-17T01:54:22Z

LGTM

evanlucas · 2015-09-18T00:57:37Z

Is this semver-major with the throw or are we considering this a bugfix?

cjihrig · 2015-09-18T01:47:06Z

Seems like a bug fix to me. Currently, if you passed something like 5 as options, you'd hit options = options || {};, and then later crash on this.allowHalfOpen = options.allowHalfOpen || false;.

evanlucas · 2015-09-18T01:54:56Z

Ah that makes sense

evanlucas · 2015-09-18T01:55:57Z

So I guess the behavior would still be changing if false were passed?

cjihrig · 2015-09-18T02:00:58Z

Yea I guess so. Same for NaN, as undocumented as it may be :-)

evanlucas · 2015-09-18T02:03:03Z

LGTM minus the nit that @cjihrig pointed out

benjamingr · 2016-03-14T21:14:47Z

Anything blocking this?

cjihrig · 2016-03-14T22:25:24Z

@benjamingr just one style nit, and a CI run, as far as I can tell. If you want, you can address the style nit and land it, assuming CI is happy.

whitlockjc · 2016-03-14T22:29:04Z

We also might want to squash the commits no?

cjihrig · 2016-03-14T22:35:19Z

Yes

benjamingr · 2016-03-14T22:47:01Z

Sure, I'll shape it up and land it tomorrow

benjamingr · 2016-03-15T07:45:44Z

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

benjamingr · 2016-03-15T08:43:11Z

The linter is angry for missing a space between the if and a ( - otherwise tests pass - I'll fix this locally.

net.createServer('aPipe') and net.createServer(8080) are mistakes, and now throw a TypeError instead of silently being treated as an object. PR-URL: #2904 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

benjamingr · 2016-03-15T08:52:37Z

Landed in a78b334 . Thanks.

jasnell · 2016-03-15T14:58:09Z

Marked as a semver-major because it adds a new throw.

Fishrock123 · 2016-03-15T14:59:34Z

@evanlucas make sure you catch this for v5.8.1 ^

evanlucas · 2016-03-15T17:49:06Z

@Fishrock123 semver-major though?

benjamingr · 2016-03-15T17:54:33Z

See the discussion above @jasnell @Fishrock123 :

Seems like a bug fix to me. Currently, if you passed something like 5 as options, you'd hit options = options || {};, and then later crash on this.allowHalfOpen = options.allowHalfOpen || false;.

jasnell · 2016-03-15T18:20:36Z

hmm... not sure about that. If I passed 5 as options, options = options || {} would mean options = 5, which means this.allowHalfOpen = options.allowHalfOpen || false would not crash, it would simply resolve to this.allowHalfOpen = false.

benjamingr · 2016-03-15T18:55:40Z

That's right, I tend to agree.

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

net: type check createServer options object

213339b

net.createServer('aPipe') and net.createServer(8080) are mistakes, and now throw a TypeError instead of silently being treated as an object.

sam-github added the net Issues and PRs related to the net subsystem. label Sep 16, 2015

thefourtheye reviewed Sep 16, 2015
View reviewed changes

fixup! net: type check createServer options object

4c30369

cjihrig reviewed Sep 17, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

benjamingr closed this Mar 15, 2016

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 15, 2016

jasnell added the error-handling label Mar 15, 2016

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

sam-github deleted the net-create-server-arg-checking branch April 17, 2017 20:37

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Fix for 14 vulnerabilities nigelhanlon/node#1

Open

Uh oh!

net: type check createServer options object #2904

net: type check createServer options object #2904

Uh oh!

Conversation

sam-github commented Sep 16, 2015

Uh oh!

thefourtheye Sep 16, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig Sep 17, 2015

Choose a reason for hiding this comment

Uh oh!

Trott Sep 17, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Sep 17, 2015

Uh oh!

evanlucas commented Sep 18, 2015

Uh oh!

cjihrig commented Sep 18, 2015

Uh oh!

evanlucas commented Sep 18, 2015

Uh oh!

evanlucas commented Sep 18, 2015

Uh oh!

cjihrig commented Sep 18, 2015

Uh oh!

evanlucas commented Sep 18, 2015

Uh oh!

benjamingr commented Mar 14, 2016

Uh oh!

cjihrig commented Mar 14, 2016

Uh oh!

whitlockjc commented Mar 14, 2016

Uh oh!

cjihrig commented Mar 14, 2016

Uh oh!

benjamingr commented Mar 14, 2016

Uh oh!

benjamingr commented Mar 15, 2016

Uh oh!

benjamingr commented Mar 15, 2016

Uh oh!

benjamingr commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

Fishrock123 commented Mar 15, 2016

Uh oh!

evanlucas commented Mar 15, 2016

Uh oh!

benjamingr commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

benjamingr commented Mar 15, 2016

Uh oh!

Uh oh!