Include non-default ports, if extracting Host

[philippgl]

This fixes the Host extractor to include the port, if the request is made via http2. Host should always include the port, if it is not the default. See also
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host, which is referred to by the different forwarded headers.

Motivation

This fixes the different behavior between http and http2, if axum::extract::Host. This might also fix other code paths, that hit the last if in this extractor. (see also #1916)

Solution

The Host extractor now uses parts.uri.authority() to extract the host. (and to_string(), because to_owned is not defined on Authority)

⚠️ I just did some more digging and the authority part of an uri (according to rfc 3986 ) can contain userinfo (mostly "username:password@"), while the authority header in http2 must not include this. Even in http1 the userinfo is transmitted in the "Authorization" header.

In http1 the Host-header is mandatory and the last if (with the parts.uri.authority()) should not be hit and I tested it with http2 and the userinfo part was not there, so I think this fix should work as intended.

And I could not get retrieve the userinfo via the parts.uri even with http1 (and the use of username:password@ in the uri is deprecated according to the http crate)

[davidpdrsn]

We need to get some more clarity on whether this is a bug or just how h2 works. I don't know enough about the lower levels to know.

We'd also need tests 😊

[philippgl]

I do not understand, how h2 (assuming, that you talk about the crate) is involved here (besides as as implementation detail). An axum user will just look at https://docs.rs/axum/latest/axum/extract/struct.Host.html and most likely assume, that the behavior will not change, if the extractor is used with http2 instead of http1. The fact, that curl -v invents a new Host header, that is not there, just makes it more surprising, that the behavior changes with http2.

Regarding the tests, the extract::host::tests::uri_host just tests the Host header (I guess, this is inserted by reqwests), so I agree, that some tests are probably needed.

btw.: currently routing::tests::logging_rejections fails for me (also fails on main) unless I pass some -F tracing,headers to cargo test

PS: I really like the axum crate. This is just the second issue, I had with it. The other one is the now fixed slow .nest(), which we already re-introduced.

[ttys3]

1. as the extractor documented:

Extractor that resolves the hostname of the request

ref: https://docs.rs/axum/latest/axum/extract/struct.Host.html

hostname does not contains the port part.

host does.

even in js:

so, maybe we need rename the extractor to Hostname ?

2. the second is: do we have any other easy way (without consume the request)/ or extractors to get the host (hostname+port`) ?

[davidpdrsn]

btw.: currently routing::tests::logging_rejections fails for me (also fails on main) unless I pass some -F tracing,headers to cargo test

That's intentional. We require all features for tests. The additional maintenance of adding #[cfg(feature)] to all the tests isn't worth it.

1. as the extractor documented:

Extractor that resolves the hostname of the request

ref: https://docs.rs/axum/latest/axum/extract/struct.Host.html

hostname does not contains the port part.

host does.

That should probably just say "Extractor that resolves the host of the request".

I do not understand, how h2 (assuming, that you talk about the crate) is involved here (besides as as implementation detail).

You're saying the behavior changes wit h2, so clearly h2 is involved. I just don't know enough about h2 to know whether this is the correct fix 🤷

[philippgl]

You're saying the behavior changes wit h2, so clearly h2 is involved. I just don't know enough about h2 to know whether this is the correct fix 🤷

Ok, I think, I just misunderstood that. You are agreeing, that the behavior should be the same as with http1, but there might be a better implementation to achieve this? I can agree with that, because the last if, that is used as kind of fallback can only be hit with a broken http1 client (missing host header) or with http2 (and there the meaning or uri is different).

In the http1 the authority() in parts.uri is None (with my curl request), so that would not work with http1. According to MDN, an absolute uri in get is mostly used with proxies.

In the http2 case h2 assembles the uri (from the scheme, authority and path http2 pseudo headers), but in the http1 case it just contains the request target as send over the wire. On the other hand in the http2 case there is no request target, that could contain the uri as a whole (it only exists in the disassembled form).

I would say this implementation results in the intended behavior, but that is by accident: h2 cannot provide the original uri / request target, so it assembles it. On the other hand, I think using .authority() is an improvement over .host() (which should be .hostname(), if it does not contain the port).

(http2 == protocol, h2 == crate)

[jplatte]

Closing as superseded by #2242.