Reject userinfo and malformed ports in Url::parse

[DanGould]

Url::parse had no notion of RFC 3986 userinfo. The host ended at the first : and any error from the port parser was discarded, so https://trusted.com:1234@evil.com/pj parsed with host trusted.com and the tail :1234@evil.com stored as the path and re-serialised verbatim. domain() and host_str() then disagreed with as_str(). A standards-compliant client such as reqwest reads that authority as userinfo trusted.com:1234 and host evil.com.

The v1 endpoint check consults domain() to allow plain http for .onion hosts, so a BIP 21 pj= value of http://x.onion:1@evil.com/pj passed as an onion endpoint, bypassing the check, and a v1 sender posted the Original PSBT in cleartext to evil.com. The https form redirects a v1 sender to an attacker host as well. The v2 sender is not affected because the encapsulated request authority is rebuilt from host_str() and the relay URL is sender-configured.

Payjoin endpoints never carry credentials, so an @ anywhere in the authority is now rejected with a new ParseError::UserinfoNotSupported (the enum is non_exhaustive, so this is additive & non-breaking). Port parse errors now propagate, which makes InvalidPort reachable and rejects inputs such as https://host:abc/ that were previously accepted with the junk stored in the path. https://host:/ still parses as no port, and @ remains valid in the path, query, and fragment.

Tests cover the host-confusion inputs, userinfo rejection with and without a port and for IPv6 literals, @ outside the authority, malformed ports, the .onion userinfo form through Uri::try_from and PjParam::parse, and the extended into_url_rejects_userinfo cases. The Arbitrary impl for Url builds the struct directly and the fuzz targets never reparse the mutated value, so they need no change.

Affects every release that ships the in-tree URL parser, 1.0.0-rc.3 through 1.0.0.

This was caught by loupe. It isn't really breaking anything about the threat model but does make the v1 reference actually spec compliant. For that reason, I'd like to get it through. Tagging @benalleng becaus he authored most of the Url module.

Disclosure: co-authored by Claude Code

[coveralls]

Coverage Report for CI Build 35750962934

Coverage increased (+0.04%) to 86.739%

Details

• Coverage increased (+0.04%) from the base build.
• Patch coverage: 1 uncovered change across 1 file (62 of 63 lines covered, 98.41%).
• 2 coverage regressions across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin/src/core/url.rs	50	49	98.0%
Total (3 files)	63	62	98.41%

Coverage Regressions

2 previously-covered lines in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin/src/core/url.rs	2	95.12%

---

Coverage Stats

Relevant Lines:	16764
Covered Lines:	14541
Line Coverage:	86.74%
Coverage Strength:	337.86 hits per line

---

💛 - Coveralls

[DanGould]

making one push to make use of PartialEq here instead

[benalleng]

CACK seems simple enough, I remember in the past there was some discussion early on about how much we want to limit our URL parse to accept some things like this for directories, but this seems like the right approach to be opinionated here.

Waiting for some test cleanup on a merge ACK

[benalleng]

TACK 4606377

Fuzzed for ~2 cpu/hrs @ 100000 exec/s on url_decode_url