http: fix `request` when `setHost` is `true` #19502

XadillaX · 2018-03-21T04:13:51Z

Fixes: #19457

Checklist

make -j4 test (UNIX) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Fixes: nodejs#19457

lpinca · 2018-03-21T08:35:38Z

test/parallel/test-https-host-headers.js

+      setHost: 0,
+      port: this.address().port,
+      rejectUnauthorized: false
+    }, cb).on('error', thrower).end();


Nit: .end() is automatically called when using https.get().

benjamingr · 2018-03-21T09:31:36Z

lib/_http_client.js

@@ -115,7 +115,7 @@ function ClientRequest(options, cb) {
  var host = options.host = validateHost(options.hostname, 'hostname') ||
                            validateHost(options.host, 'host') || 'localhost';

-  var setHost = (options.setHost === undefined);
+  var setHost = (options.setHost === undefined || options.setHost);


Maybe just Boolean(options.setHost)?

Also this is technically a breaking (semver) change.

How about !!options.setHost?

I prefer Boolean since it's more explicit (we are converting this to a boolean) rather than "we are negating this, which is coercing it to a boolean, and then negating it again to get the boolean value".

Using only Boolean(options.setHost) is not ok as undefined should set setHost to true.

Doesn't it a bugfix but breaking?

Technically yes but Imho it's semver-patch as I doubt anyone is relying on the current behavior. The option is currently not documented.

lpinca · 2018-03-21T14:36:03Z

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

trivikr · 2018-03-22T14:14:20Z

We need to document options.setHost at https://nodejs.org/api/http.html#http_http_request_options_callback

XadillaX · 2018-03-24T14:00:55Z

lib/_http_client.js

@@ -115,7 +115,7 @@ function ClientRequest(options, cb) {
  var host = options.host = validateHost(options.hostname, 'hostname') ||
                            validateHost(options.host, 'host') || 'localhost';

-  var setHost = (options.setHost === undefined);
+  var setHost = (options.setHost === undefined || Boolean(options.setHost));


I'll add the document.

XadillaX · 2018-03-24T14:23:47Z

I've updated the document.

lpinca · 2018-03-24T14:25:21Z

doc/api/http.md

@@ -1858,6 +1858,8 @@ changes:
    details. Any [`Duplex`][] stream is a valid return value.
  * `timeout` {number}: A number specifying the socket timeout in milliseconds.
    This will set the timeout before the socket is connected.
+  * `setHost` {boolean}: When values `true`, force set request header `host` to


Nit: "Specifies whether or not to automatically add the Host header. Defaults to true".

tniessen · 2018-03-25T10:26:12Z

lib/_http_client.js

@@ -115,7 +115,7 @@ function ClientRequest(options, cb) {
  var host = options.host = validateHost(options.hostname, 'hostname') ||
                            validateHost(options.host, 'host') || 'localhost';

-  var setHost = (options.setHost === undefined);
+  var setHost = (options.setHost === undefined || Boolean(options.setHost));


Tiny nit: It would be great if you could change var → let / const while you are at this line.

I think we should open another PR to fix all vars in this file but not in this PR.

trivikr · 2018-03-28T23:32:33Z

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

XadillaX · 2018-03-30T11:56:22Z

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

trivikr · 2018-04-03T22:45:03Z

Landed in b06f686

Fixes: #19457 PR-URL: #19502 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Trott · 2018-04-04T01:27:40Z

Looks like this breaks CI because it violates the lint rule that landed after the last CI run. Will open a fast-track fix if no one else has done it yet...

Ref: nodejs#19502 (comment)

Trott · 2018-04-04T01:33:10Z

(#19784)

Ref: nodejs#19502 (comment) PR-URL: nodejs#19784 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

trivikr · 2018-04-04T03:09:19Z

Thanks @Trott for the quick fix

Fixes: #19457 PR-URL: #19502 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Ref: #19502 (comment) PR-URL: #19784 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

MylesBorins · 2018-04-13T04:37:20Z

Should this land on 6.x or 8.x?

http: fix request when setHost is true

867fee8

Fixes: nodejs#19457

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Mar 21, 2018

lpinca approved these changes Mar 21, 2018

View reviewed changes

benjamingr reviewed Mar 21, 2018

View reviewed changes

f

a308e56

benjamingr approved these changes Mar 21, 2018

View reviewed changes

lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 21, 2018

jasnell approved these changes Mar 23, 2018

View reviewed changes

hiroppy approved these changes Mar 23, 2018

View reviewed changes

XadillaX commented Mar 24, 2018

View reviewed changes

doc

02c474f

lpinca reviewed Mar 24, 2018

View reviewed changes

XadillaX added 2 commits March 24, 2018 22:27

update

63dff00

update

e9035ae

tniessen reviewed Mar 25, 2018

View reviewed changes

trivikr approved these changes Mar 28, 2018

View reviewed changes

trivikr closed this Apr 3, 2018

Trott added a commit to Trott/io.js that referenced this pull request Apr 4, 2018

test: update test to comply with lint rule

b979dab

Ref: nodejs#19502 (comment)

Trott mentioned this pull request Apr 4, 2018

test: update test to comply with lint rule #19784

Closed

2 tasks

Trott added a commit to Trott/io.js that referenced this pull request Apr 4, 2018

test: update test to comply with lint rule

8891072

Ref: nodejs#19502 (comment) PR-URL: nodejs#19784 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

targos pushed a commit that referenced this pull request Apr 4, 2018

test: update test to comply with lint rule

4df3377

Ref: #19502 (comment) PR-URL: #19784 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matheus Marchini <matheus@sthima.com>

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins added lts-watch-v6.x labels Apr 13, 2018

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix `request` when `setHost` is `true` #19502

http: fix `request` when `setHost` is `true` #19502

XadillaX commented Mar 21, 2018 •

edited

Loading

lpinca Mar 21, 2018

benjamingr Mar 21, 2018

XadillaX Mar 21, 2018

benjamingr Mar 21, 2018

lpinca Mar 21, 2018 •

edited

Loading

XadillaX Mar 24, 2018

lpinca Mar 24, 2018

lpinca commented Mar 21, 2018

trivikr commented Mar 22, 2018

XadillaX Mar 24, 2018

XadillaX commented Mar 24, 2018

lpinca Mar 24, 2018

tniessen Mar 25, 2018

XadillaX Mar 26, 2018 •

edited

Loading

trivikr commented Mar 28, 2018

XadillaX commented Mar 30, 2018

trivikr commented Apr 3, 2018

Trott commented Apr 4, 2018

Trott commented Apr 4, 2018

trivikr commented Apr 4, 2018

MylesBorins commented Apr 13, 2018

http: fix request when setHost is true #19502

http: fix request when setHost is true #19502

Conversation

XadillaX commented Mar 21, 2018 • edited Loading

Checklist

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

lpinca Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca commented Mar 21, 2018

trivikr commented Mar 22, 2018

Choose a reason for hiding this comment

XadillaX commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

trivikr commented Mar 28, 2018

XadillaX commented Mar 30, 2018

trivikr commented Apr 3, 2018

Trott commented Apr 4, 2018

Trott commented Apr 4, 2018

trivikr commented Apr 4, 2018

MylesBorins commented Apr 13, 2018

http: fix `request` when `setHost` is `true` #19502

http: fix `request` when `setHost` is `true` #19502

XadillaX commented Mar 21, 2018 •

edited

Loading

lpinca Mar 21, 2018 •

edited

Loading

XadillaX Mar 26, 2018 •

edited

Loading