hyperium / hyperium/http

`PathAndQuery == PathAndQuery` doesn't handle empty paths properly

Open
#534 1 comment 8 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
1.4k
Forks
378
Avg merge
1d 21h
Merged PRs (30d)
5

Description

PathAndQuery::from_static("") tries to pretend to be PathAndQuery::from_static("/"), even going so far as to have the same Debug printing, returning the same string from .as_str(), and comparing as the same with PartialOrd. But they compare differently with PartialEq. Comparing two Uris that differ in this way works as Uri compares the path() and query() separately, but PathAndQuery == PathAndQuery just compares the raw data.

let uri1 = dbg!(Uri::from_static("http://example.com"));
let uri2 = dbg!(Uri::from_static("http://example.com/"));
// Test URI behavior
assert_eq!(uri1.to_string(), uri2.to_string()); // same Display
assert_eq!(format!("{uri1:?}"), format!("{uri2:?}")); // same Debug
assert_eq!(uri1, uri2); // equal
// Test PathAndQuery behavior
let path1 = dbg!(uri1.path_and_query()).unwrap();
let path2 = dbg!(uri2.path_and_query()).unwrap();
assert_eq!(path1.to_string(), path2.to_string()); // same Display
assert_eq!(format!("{path1:?}"), format!("{path2:?}")); // same Debug
assert!(path1 <= path2 && path1 >= path2); // PartialOrd says they're equal
assert_eq!(path1, path2); // FAILURE

Similarly if I have two non-empty PathAndQuerys where one is missing the leading slash then both PartialEq and PartialOrd say they're unequal despite still displaying them the same from Debug/Display and despite Uri considering them the same when comparing at that level (which is because Uri compares path() and query() instead of the underlying path_and_query). This can be tested by appending ?foo to the two URLs in the above example:

let uri1 = dbg!(Uri::from_static("http://example.com?foo"));
let uri2 = dbg!(Uri::from_static("http://example.com/?foo"));
// …

Also


The fact that PathAndQuery tries so hard to pretend that an empty path is the same as a slash is somewhat confusing in general and just seems ripe for having bugs like this going forward. It's also completely undocumented, and existing documentation even implies differently (e.g. Uri::path() says that it may return an empty string, though in practice it only does so for authority-only relative URIs).

There's also other oddities like I can construct a Uri from parts using PathAndQuery::from_static("foo") and this actually works, but the returned Uri then leaves the path as-is and serializes like http://example.comfoo. This is actually rather surprising given how much Uri and PathAndQuery try to pretend that it has a leading slash when it doesn't. Even if it was changed to disallow "foo" this would still fail on "*" as PathAndQuery explicitly supports that. Given that asterisk-form comes from request-target and that this is what Uri is documented as being used for, support for * should probably be handled at the Uri level instead of PathAndQuery (especially as it doesn't represent a path at all). Doing that would simplify the handling of PathAndQuery slightly and fix issues like how PathAndQuery::from_static("foo") prints as /foo but PathAndQuery::from_static("*foo") prints as just *foo.

In any case, PartialEq and PartialOrd should be fixed in these cases, but some thought really should be given to structuring this in a way that avoids sprinkling all this logic everywhere about edge cases and hoping that everything matches up correctly.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the PathAndQuery and Uri comparison and formatting behavior described in the issue, using the provided empty-path, slash, query, missing-leading-slash, and asterisk examples as regression cases. Determine how PartialEq and PartialOrd should agree with the displayed and Uri-level representations, and consider the suggested separation of asterisk-form handling. Done means the comparison inconsistencies are fixed and the relevant edge cases are covered by tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.