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.

Sign up for GitHub

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: handle array in host prop #29568

Closed
wants to merge 1 commit into from
Closed

http: handle array in host prop #29568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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_agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@ const net = require('net');
const EventEmitter = require('events');
const debug = require('internal/util/debuglog').debuglog('http');
const { async_id_symbol } = require('internal/async_hooks').symbols;

const {
codes: {
ERR_INVALID_ARG_TYPE,
},
} = require('internal/errors');
// New Agent code.

// The largest departure from the previous implementation is that
Expand Down Expand Up @@ -240,6 +244,11 @@ function calculateServerName(options, req) {
let servername = options.host;
const hostHeader = req.getHeader('host');
if (hostHeader) {
if (typeof hostHeader !== 'string') {
throw new ERR_INVALID_ARG_TYPE('options.headers.host',
'String', hostHeader);
}

// abc => abc
// abc:123 => abc
// [::1] => ::1
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-http-client-headers-host-array.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

require('../common');

const assert = require('assert');
const http = require('http');

{
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this block is not needed?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer we keep individual tests in block scopes, even if there's only one test case in the test file. It's not necessary but it keeps everything consistent with test files where they are useful, and it makes the test file more easily-extensible and friendly-for-beginners. Need to add a new test case? Hey, put it in some brackets and look at the brackets elsewhere in the test file.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion it only hinders readability when there is only one test case. It's one more unneeded level of indentation. Anyway I don't mind, it is a nit.


const options = {
port: '80',
path: '/',
headers: {
host: []
}
};

assert.throws(() => {
http.request(options);
}, {
code: /ERR_INVALID_ARG_TYPE/
}, 'http request should throw when passing array as header host');
}