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

Host extractor loses port number on HTTP/2 #2955

Closed
1 task done
tonywu6 opened this issue Oct 3, 2024 · 3 comments
Closed
1 task done

Host extractor loses port number on HTTP/2 #2955

tonywu6 opened this issue Oct 3, 2024 · 3 comments
Labels
C-bug Category: This is a bug.
Milestone

Comments

@tonywu6
Copy link

tonywu6 commented Oct 3, 2024

  • I have looked for existing issues (including closed) about this

Bug Report

Version

> cargo tree | grep axum
├── axum v0.7.7
│   ├── axum-core v0.4.5
│   ├── axum-macros v0.4.2 (proc-macro)
├── axum v0.7.7 (*)
├── axum v0.7.7 (*)

Platform

Darwin *** 24.0.0 Darwin Kernel Version 24.0.0: Mon Aug 12 20:51:54 PDT 2024; root:xnu-11215.1.10~2/RELEASE_ARM64_T6000 arm64

Crates

axum = { version = "0.7.7", features = ["http2"] }

Description

On HTTP/2, with a non-conventional port, the result of axum::extract::Host doesn't include a port number:

use axum::{
    extract::{Host, Request},
    routing::get,
    BoxError, Router,
};

async fn handler(Host(host): Host, request: Request) {
    if let Some(authority) = request.uri().authority() {
        assert_eq!(host, authority.to_string())
        // assertion `left == right` failed
        // left: "127.0.0.1"
        // right: "127.0.0.1:8000"
    }
}

#[tokio::main]
async fn main() -> Result<(), BoxError> {
    let router = Router::new().route("/", get(handler));
    let listener = tokio::net::TcpListener::bind("127.0.0.1:8000").await?;
    Ok(axum::serve(listener, router).await?)
}

// curl -v http://127.0.0.1:8000 --http2-prior-knowledge

In the extractor:

if let Some(host) = parts.uri.host() {
return Ok(Host(host.to_owned()));
}

Is this supposed to be parts.uri.authority() instead?

Moreover, should parts.uri.authority() take priority over the Host header? Host is almost always used in HTTP/1.1 but is supplanted by the :authority pseudo-header in HTTP/2. It seems with pseudo-headers, hyper is then able to provide the complete Uri. In RFC 9113 it says

A server SHOULD treat a request as malformed if it contains a Host header field that identifies an entity that differs from the entity in the ":authority" pseudo-header field.

@mladedav
Copy link
Collaborator

mladedav commented Oct 3, 2024

I agree using authority might be better to match the Host header semantics.

As for the ordering, it shouldn't matter as long as the linked RFC is followed since either both value will be the same or one will be missing. When looking at http 2 it seems that :authority is optional and if it is missing, the old host header should be used. So that might be a case for swapping them around but we should first double check we don't set authority in the request URI anywhere for any reason.

We can also check whether hyper refuses requests with different host and authority, but if not, I think it should be raised and fixed there and we still have to arbitrarily pick one or the other in the meantime (or fail extraction which seems a bit harsh).

@mladedav
Copy link
Collaborator

mladedav commented Oct 3, 2024

Oh and looks like someone already had a go at changing it here but then probably fell through the cracks.

@jplatte jplatte added this to the 0.8 milestone Oct 5, 2024
@jplatte jplatte added the C-bug Category: This is a bug. label Oct 5, 2024
@yanns
Copy link
Collaborator

yanns commented Nov 14, 2024

fixed by #2242

@yanns yanns closed this as completed Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

4 participants