Skip to content

fix(uri): don't glue * onto the authority in Display - #884

Open
patrick-immometrica wants to merge 1 commit into
hyperium:masterfrom
patrick-immometrica:fix-star-uri-display
Open

patrick-immometrica wants to merge 1 commit into
hyperium:masterfrom
patrick-immometrica:fix-star-uri-display

Conversation

@patrick-immometrica

Copy link
Copy Markdown
Contributor

Since #843, an asterisk-form path can be combined with a scheme and authority, which h2 needs for OPTIONS *. Display then writes the path straight after the authority:

let uri = Uri::builder()
    .scheme("https")
    .authority("example.com")
    .path_and_query("*")
    .build()
    .unwrap();
assert_eq!(uri.to_string(), "https://example.com*");

example.com* is a different authority. hyper's h1 client formats the request target with Display, so an OPTIONS * sent through an HTTP proxy in absolute form ends up with the wrong host on the wire.

RFC 9112 3.2.4 says the absolute form of an asterisk-form request has an empty path (OPTIONS http://example.com HTTP/1.1), so this skips writing * when there is an authority. path() still returns "*", and only Display changes.

I extended the existing build_star_for_http2 test. It fails on master:

  left: "https://example.com*"
 right: "https://example.com"

Note that the output no longer round-trips to an equal Uri, since it reparses with path /. I think that's fine because the old output didn't round-trip either, and it parsed to the wrong host.

A Uri with scheme, authority and path `*` (allowed since hyperium#843 for HTTP/2
OPTIONS requests) was formatted as `https://example.com*`, which parses
back with an authority of `example.com*`. hyper writes the request
target with Display when talking to a proxy, so this also went out on
the wire.

Per RFC 9112 section 3.2.4 the absolute-form of `OPTIONS *` has an
empty path, so leave the `*` out when there is an authority.
@patrick-immometrica
patrick-immometrica marked this pull request as ready for review November 5, 2026 11:08

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant