fix: use :authority header of http2 requests as host

[mgjm]

The now stable http2 api does not use the host header. The host can be found in the http2 :authority header. Therefore koa should use this header instead.

This fixes the host property and all properties that depend on it for http2 requests. These dependent properties include the hostname, the origin and the URL. This will also fix #1174.

[codecov]

Codecov Report

Merging #1262 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1262   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files           5       5           
  Lines         391     392    +1     
======================================
+ Hits          391     392    +1

Impacted Files	Coverage Δ
lib/request.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9146024...350f5e1. Read the comment docs.

[fengmk2]

I think this should be feature not bug fix?

[fengmk2]

cc @dead-horse

[fengmk2]

https://github.com/nodejs/node/blob/e7f710c1d4729890806610e5450d7a0edfb4ac11/lib/internal/http2/compat.js#L358

use this.req. authority will be better.

[mgjm]

This seems to be an undocumented property. Is it save to use it and should koa use undocumented props?

[fl0w]

It's unsafe either way, at least this.req.authority is a specific getter with a sane naming that conforms with the RFC headerfield.

[mgjm]

this.req.headers is the documented way to access http headers and this.get is using this way. And :authority is the official header name from RFC 7540. So this way should not be unsafe.

Whereas this.req.authority is not documented and therefore could change in a minor update and is considered an internal API (as described in the node COLLABORATOR_GUIDE).

But I agree that this.req.authority should be part of the public API and I just created an issue in the node repo (nodejs/node#23825).

[fengmk2]

2.6.0

[panva]

The requested change to keep x-forwarded values as host is not in?

[mgjm]

@panva The current order is...

1. If app.proxy the X-Forwarded-Host header
2. If HTTP/2 the :authority header
3. The Host header

(See lib/request.js)

[panva]

Yeah, as requested in the review by @dead-horse

If app.proxy was set, I'd like to keep the X-Forwarded-Host set by reverse proxy(nginx).

And in my practice, when client request nginx with http 1/2 and nginx proxy to node server, it always use X-Forwarded-Host, it is a de-facto standard for reverse proxy to set host when visit upstream.

But the fact is :authority will overwrite what's in the x-forwarded-host if proxy is true (looking at master)

It should be like this imho

if (this.req.httpVersionMajor >= 2) host = host || this.get(':authority');

[dead-horse]

1. If app.proxy the X-Forwarded-Host header
2. If HTTP/2 the :authority header
3. The Host header

I think this works for me, X-Forwarded-Host is set by reverse proxy, we just trust the proxy when app.proxy is true. Otherwise we detect HTTP 1 / HTTP 2 to choose which header we should use to get host.

[panva]

I agree, but the implementation takes :authority over x-forwarded-host.

[fengmk2]

@panva I will release a bugfix version soon.

[panva]

@fengmk2 perfect!

[fengmk2]

#1263

[mgjm]

@panva Good catch. @fengmk2 Thanks for the quick fix 👍