http: add type checking for hostname #12494

jasnell · 2017-04-18T20:38:08Z

Add type checking for options.hostname / options.host Maintains the use of undefined and null for default localhost, but other falsy values (like false, 0 and NaN) are rejected.

Refs: #12488

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

http

mscdex · 2017-04-18T20:58:26Z

lib/_http_client.js

@@ -65,6 +65,14 @@ function isInvalidPath(s) {
  return false;
 }

+function validateHost(host) {
+  if (host !== undefined && host !== null && typeof host !== 'string') {


The first two conditionals can be shortened to just host != null or host != undefined.

Yeah... that's fine. I'm not a fan of != for that but shorter is better.

It could be more efficient too, I have not checked how V8 compiles the two different methods of checking.

mscdex · 2017-04-18T20:58:57Z

lib/_http_client.js

+function validateHost(host) {
+  if (host !== undefined && host !== null && typeof host !== 'string') {
+    throw new TypeError(
+      'hostname must either be a string, undefined or null');


Perhaps we might use: "hostname"/"host" must either be a string, undefined, or null to keep with the theme of quoting property names?

Oh, right... good catch :-)

jasnell · 2017-04-18T21:15:23Z

@mscdex Updated

cjihrig

LGTM with a question.

cjihrig · 2017-04-18T21:18:39Z

test/parallel/test-http-hostname-typechecking.js

+
+['', undefined, null].forEach((v) => {
+  assert.doesNotThrow(() => {
+    http.request({hostname: v}).on('error', common.noop);


Why the common.noop here? I'd think we'd either want mustCall() or mustNotCall().

The main part that this is testing is that the call to http.request() does not throw synchronously. The fact that it will emit the error event is non-sequential to this particular test. This is set simply to avoid the unhandled error exception.

mscdex · 2017-04-18T21:27:24Z

lib/_http_client.js

    throw new TypeError(
-      'hostname must either be a string, undefined or null');
+      '"hostname" must either be a string, undefined or null');


Should we include "host" as well, since this same function is used for both?

I was stewing over that. Not sure. I'm fine with doing so just not sure if we should

lpinca · 2017-04-19T07:53:02Z

lib/_http_client.js

@@ -65,6 +65,15 @@ function isInvalidPath(s) {
  return false;
 }

+function validateHost(host) {
+  // eslint-disable-next-line eqeqeq


If I'm not wrong this can be removed if host != null is used instead of host != undefined.

jasnell · 2017-04-19T22:39:15Z

@mscdex @lpinca ... updated!

jasnell · 2017-04-20T23:10:11Z

@nodejs/ctc ... PTAL

mscdex · 2017-04-20T23:36:18Z

test/parallel/test-http-hostname-typechecking.js

+
+// All of these values should cause http.request() to throw synchronously
+// when passed as the value of either options.hostname or options.host
+const vals = [{}, [], NaN, Infinity, -Infinity, true, false, 1, new Date()];


Maybe include 0 as well, since it would have been treated as a false-y value?

thefourtheye · 2017-04-21T15:04:52Z

test/parallel/test-http-hostname-typechecking.js

+// when passed as the value of either options.hostname or options.host
+const vals = [{}, [], NaN, Infinity, -Infinity, true, false, 1, new Date()];
+
+function errCheck(name) {


This function is not doing any checks actually

thefourtheye · 2017-04-21T15:06:01Z

test/parallel/test-http-hostname-typechecking.js

+// These values are OK and should not throw synchronously
+['', undefined, null].forEach((v) => {
+  assert.doesNotThrow(() => {
+    http.request({hostname: v}).on('error', common.noop);


Why not common.mustNotCall() here?

@jasnell's reply on a comment above for same question

The main part that this is testing is that the call to http.request() does not throw synchronously. The fact that it will emit the error event is non-sequential to this particular test. This is set simply to avoid the unhandled error exception.

Mmmm, makes sense. Cool then.

Add type checking for options.hostname / options.host Maintains the use of `undefined` and `null` for default `localhost`, but other falsy values (like `false`, `0` and `NaN`) are rejected.

jasnell · 2017-04-24T16:28:56Z

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

jasnell · 2017-04-24T17:48:40Z

Linting error was from an unrelated commit

Add type checking for options.hostname / options.host Maintains the use of `undefined` and `null` for default `localhost`, but other falsy values (like `false`, `0` and `NaN`) are rejected. PR-URL: #12494 Ref: #12488 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>

jasnell · 2017-04-24T17:51:25Z

Landed in 85a4e25

This test would currently create HTTP requests to localhost:80 and would time out on machines that actually had an server listening there. To address that, `end()` the requests that are generated. PR-URL: nodejs#12627 Ref: nodejs#12494

This test would currently create HTTP requests to localhost:80 and would time out on machines that actually had an server listening there. To address that, `end()` the requests that are generated. PR-URL: #12627 Ref: #12494 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell added http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 18, 2017

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 18, 2017

jasnell requested review from bnoordhuis, mscdex and indutny April 18, 2017 20:38

mscdex reviewed Apr 18, 2017

View reviewed changes

cjihrig approved these changes Apr 18, 2017

View reviewed changes

mscdex reviewed Apr 18, 2017

View reviewed changes

lpinca reviewed Apr 19, 2017

View reviewed changes

lpinca approved these changes Apr 19, 2017

View reviewed changes

jasnell force-pushed the http-hostname-typechecking branch from 97f9dc6 to 46c75f1 Compare April 19, 2017 22:38

mscdex reviewed Apr 20, 2017

View reviewed changes

ofrobots approved these changes Apr 21, 2017

View reviewed changes

thefourtheye reviewed Apr 21, 2017

View reviewed changes

jasnell force-pushed the http-hostname-typechecking branch from 46c75f1 to 0b5dc3f Compare April 21, 2017 17:15

http: add type checking for hostname

9ec4133

Add type checking for options.hostname / options.host Maintains the use of `undefined` and `null` for default `localhost`, but other falsy values (like `false`, `0` and `NaN`) are rejected.

jasnell force-pushed the http-hostname-typechecking branch from 0b5dc3f to 9ec4133 Compare April 21, 2017 17:32

jasnell closed this Apr 24, 2017

addaleax mentioned this pull request Apr 25, 2017

test: fixup test-http-hostname-typechecking #12627

Closed

3 tasks

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: add type checking for hostname #12494

http: add type checking for hostname #12494

jasnell commented Apr 18, 2017

mscdex Apr 18, 2017

jasnell Apr 18, 2017

mscdex Apr 18, 2017 •

edited

Loading

mscdex Apr 18, 2017 •

edited

Loading

jasnell Apr 18, 2017

jasnell commented Apr 18, 2017

cjihrig left a comment

cjihrig Apr 18, 2017

jasnell Apr 18, 2017

mscdex Apr 18, 2017

jasnell Apr 18, 2017

lpinca Apr 19, 2017

jasnell commented Apr 19, 2017

jasnell commented Apr 20, 2017

mscdex Apr 20, 2017

thefourtheye Apr 21, 2017

thefourtheye Apr 21, 2017

lpinca Apr 21, 2017 •

edited

Loading

thefourtheye Apr 21, 2017

jasnell commented Apr 24, 2017

jasnell commented Apr 24, 2017

jasnell commented Apr 24, 2017

http: add type checking for hostname #12494

http: add type checking for hostname #12494

Conversation

jasnell commented Apr 18, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 18, 2017

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 19, 2017

jasnell commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Apr 24, 2017

jasnell commented Apr 24, 2017

jasnell commented Apr 24, 2017

mscdex Apr 18, 2017 •

edited

Loading

mscdex Apr 18, 2017 •

edited

Loading

lpinca Apr 21, 2017 •

edited

Loading