Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: add type checking for hostname #12494

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,14 @@ function isInvalidPath(s) {
return false;
}

function validateHost(host, name) {
if (host != null && typeof host !== 'string') {
throw new TypeError(
`"options.${name}" must either be a string, undefined or null`);
}
return host;
}

function ClientRequest(options, cb) {
OutgoingMessage.call(this);

Expand Down Expand Up @@ -123,7 +131,8 @@ function ClientRequest(options, cb) {
this.agent && this.agent.defaultPort;

var port = options.port = options.port || defaultPort || 80;
var host = options.host = options.hostname || options.host || 'localhost';
var host = options.host = validateHost(options.hostname, 'hostname') ||
validateHost(options.host, 'host') || 'localhost';

var setHost = (options.setHost === undefined);

Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-http-hostname-typechecking.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const http = require('http');

// 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, 0, new Date()];

function errCheck(name) {
return new RegExp(`^TypeError: "options\\.${name}" must either be a ` +
'string, undefined or null$');
}

vals.forEach((v) => {
assert.throws(() => http.request({hostname: v}), errCheck('hostname'));
assert.throws(() => http.request({host: v}), errCheck('host'));
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not common.mustNotCall() here?

Copy link
Member

@lpinca lpinca Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm, makes sense. Cool then.

http.request({host: v}).on('error', common.noop);
});
});