Skip to content

Commit 5341e9f

Browse files
committed
fix(net-http): parseUrl separates userinfo and rejects junk after an IPv6 bracket (#2072)
Two halves of one post-audit finding. USERINFO. RFC 3986 3.2 spells the authority `[ userinfo "@" ] host [ ":" port ]` and this parser had no userinfo rule at all, so http://user@host/p returned the host "user@host" -- a string that then went to getaddrinfo as a DNS name and into the Host: header. .NET keeps UserInfo as its own component and its Host never contains it. The LAST '@' is the delimiter, per RFC 3986 3.2.1: a '@' is legal inside the userinfo and the host production admits none. Splitting on the first would make "b@example.com" the host of http://a@b@example.com/, and a mutation that does exactly that is caught. The userinfo is discarded, deliberately, and that is recorded rather than glossed: ParsedUrl has no userinfo field, this handler has no authentication path to hand one to, and inventing one is new API. What matters is that it stops being part of the HOST. http://user:pass@host/ used to throw for the WRONG reason, and the ticket noticed: rfind(':') made "pass@host" the port text, so the failure was accidental and the message said "invalid port". It now succeeds with the host it always denoted. IPv6. http://[::1]x/p returned host "::1" and SILENTLY DISCARDED the 'x', so the URL the caller wrote and the URL the client connected to differed with no diagnostic. RFC 3986 3.2.2 allows only `":" port` after the closing bracket. Four mutations. Three caught. The fourth -- removing the empty-host check after the userinfo split -- was NOT caught, and that is reported rather than papered over: the check was redundant, because an empty authority already reaches the empty-host check at the end of the function. The dead code was deleted rather than defended, and a comment records that a mutation is how that was established. +5 tests. Gate: 17,256 run, 0 failed, 38 executables -- green. Downstream, measured: zero HttpClient sites in either consumer. Record: docs/Migration-HttpClientUrlUserInfo.md.
1 parent cd9fa04 commit 5341e9f

5 files changed

Lines changed: 169 additions & 2 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `parseUrl` separates userinfo and rejects junk after an IPv6 bracket (ticket #2072)
5+
6+
*2026-08-17.* `HttpClient::parseUrl` had no userinfo rule, so `http://user@host/p` returned the
7+
host `"user@host"` — a string that then went to `getaddrinfo` as a DNS name and into the `Host:`
8+
header. And `http://[::1]x/p` returned host `"::1"` with the `x` **silently discarded**.
9+
10+
Landed under `docs/StandingApprovals.md` SA-5. No public signature, layout, vtable or `noexcept`
11+
change.
12+
13+
---
14+
15+
## 1. What changed
16+
17+
| URL | Was | Is |
18+
|---|---|---|
19+
| `http://user@example.com/p` | host `"user@example.com"` | host `"example.com"` |
20+
| `http://user:pass@example.com/p` | `UriFormatException`**by accident** (see §3) | host `"example.com"` |
21+
| `http://user:pass@example.com:8080/p` | `UriFormatException` | host `"example.com"`, port 8080 |
22+
| `http://a@b@example.com/p` | host `"a@b@example.com"` | host `"example.com"` |
23+
| `http://user@/p` | host `"user@"` | `UriFormatException` (empty host) |
24+
| `http://[::1]x/p` | host `"::1"`, `x` **discarded** | `UriFormatException` |
25+
| `http://[::1]/p`, `http://[::1]:8080/p` || **unchanged** |
26+
| a URL with no `@` in its authority || **unchanged** |
27+
28+
## 2. Why
29+
30+
RFC 3986 §3.2 spells the authority `[ userinfo "@" ] host [ ":" port ]`, and §3.2.2 allows only
31+
`":" port` after an IPv6 literal's closing bracket. .NET keeps `UserInfo` as its own component
32+
and its `Host` never contains it.
33+
34+
Silently discarding text after the bracket is the more insidious of the two: the URL the caller
35+
wrote and the URL the client connected to differed, with no diagnostic.
36+
37+
**The last `@` is the delimiter.** RFC 3986 §3.2.1's `userinfo` production admits `@`, and the
38+
`host` production does not, so the final one splits. Using the first would make `b@example.com`
39+
the host of `http://a@b@example.com/`.
40+
41+
## 3. `http://user:pass@host/` used to throw for the wrong reason
42+
43+
It did fail before — but because `rfind(':')` made `"pass@example.com"` the **port** text, which
44+
then failed the port grammar. The failure was accidental, and the message said "invalid port".
45+
It now succeeds with the host `example.com`, which is what it always denoted.
46+
47+
## 4. The userinfo is discarded, deliberately
48+
49+
`ParsedUrl` has no userinfo field, this handler has no authentication path to hand one to, and
50+
inventing one is new API. What matters for this ticket is that the userinfo stops being part of
51+
the **host** — a name that reaches DNS and the `Host:` header.
52+
53+
If you were relying on userinfo reaching the server, it never did: it was going into the host
54+
name, where it made the lookup fail.
55+
56+
## 5. To migrate
57+
58+
Nothing, unless you pass URLs with credentials in them. Those used to fail (with a misleading
59+
message) or resolve wrongly; they now parse to the host they name, and the credentials are
60+
dropped rather than smuggled into a header.
61+
62+
## 6. Downstream, measured
63+
64+
Neither `cna` nor `mobile-eggbert` references `HttpClient` or `System::Net::Http`**zero sites
65+
in both**. Neither repository was modified.

modules/net-http/src/System/Net/Http/HttpClient.cpp

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,28 @@ HttpClient::ParsedUrl HttpClient::parseUrl(const std::string& url) {
149149
if (requestTarget.empty()) requestTarget = "/";
150150
result.path = requestTarget;
151151

152+
// Ticket #2072 (post-audit defect, deferred verification). RFC 3986 §3.2 spells the
153+
// authority `[ userinfo "@" ] host [ ":" port ]`, and this parser had no userinfo rule at
154+
// all -- measured, `http://user@host/p` returned host `"user@host"`, which then went to
155+
// getaddrinfo as a DNS name and into the `Host:` header. .NET separates it: `Uri.UserInfo`
156+
// is its own component and `Uri.Host` never contains it (`Uri.cs:3708-3760`, the
157+
// MayHaveUserInfo branch of ParseAuthority).
158+
//
159+
// The userinfo is DISCARDED rather than surfaced, and that is a deliberate scope decision
160+
// recorded rather than glossed: `ParsedUrl` has no userinfo field, this handler has no
161+
// authentication path to hand it to, and inventing one is new API. What matters here is
162+
// that it stops being part of the HOST -- a name that reaches DNS and the `Host:` header.
163+
//
164+
// Splitting on the LAST '@' is RFC 3986's rule: a '@' inside the userinfo is legal
165+
// (percent-encoded or not, per §3.2.1's `userinfo` production), while the host production
166+
// admits none, so the final one is the delimiter.
167+
const size_t userInfoEnd = authority.rfind('@');
168+
if (userInfoEnd != std::string::npos) authority = authority.substr(userInfoEnd + 1);
169+
// No empty-authority check here on purpose: `http://user@/p` leaves an empty authority,
170+
// which the empty-host check at the end of this function already rejects. A second check
171+
// would be dead code -- a mutation removing it changed nothing, which is how that was
172+
// established rather than assumed.
173+
152174
std::string portText;
153175
bool hasPort = false;
154176

@@ -161,7 +183,14 @@ HttpClient::ParsedUrl HttpClient::parseUrl(const std::string& url) {
161183
if (closeBracket == std::string::npos)
162184
throw System::UriFormatException("HttpClient: unterminated IPv6 literal in URL: " + url);
163185
result.host = authority.substr(1, closeBracket - 1);
164-
if (closeBracket + 1 < authority.size() && authority[closeBracket + 1] == ':') {
186+
if (closeBracket + 1 < authority.size()) {
187+
// #2072: anything after the closing bracket must be a port and nothing else.
188+
// Measured before this, `http://[::1]x/p` returned host `"::1"` and SILENTLY
189+
// DISCARDED the `x` -- so a URL the caller wrote and a URL the client connected to
190+
// differed, with no diagnostic. RFC 3986 §3.2.2 allows only `":" port` there.
191+
if (authority[closeBracket + 1] != ':')
192+
throw System::UriFormatException(
193+
"HttpClient: unexpected text after the IPv6 literal in URL: " + url);
165194
portText = authority.substr(closeBracket + 2);
166195
hasPort = true;
167196
}

modules/net-http/tests/System/Net/HttpClientTests.cpp

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2528,3 +2528,76 @@ TEST(HttpParserInteractionTests, AuthorityOnlyQueryReachesTheWireAsTheRequestTar
25282528
EXPECT_EQ(raw.find("key=secret\r\n"), std::string::npos)
25292529
<< "and must not appear as the tail of the Host header";
25302530
}
2531+
2532+
// ---------------------------------------------------------------------------
2533+
// Ticket #2072 (post-audit defect, deferred verification) -- userinfo and the
2534+
// text after an IPv6 bracket.
2535+
// ---------------------------------------------------------------------------
2536+
2537+
TEST(HttpClientParseUrlUserInfoTests, Fix2072_UserInfoIsNotPartOfTheHost) {
2538+
// RFC 3986 3.2 spells the authority `[ userinfo "@" ] host [ ":" port ]`, and this parser
2539+
// had no userinfo rule at all: `http://user@host/p` returned host "user@host", which then
2540+
// went to getaddrinfo as a DNS NAME and into the `Host:` header. .NET keeps UserInfo as its
2541+
// own component and its Host never contains it.
2542+
auto p = HttpClient::parseUrl("http://user@example.com/p");
2543+
EXPECT_EQ(p.host, "example.com");
2544+
EXPECT_EQ(p.port, 80);
2545+
EXPECT_EQ(p.path, "/p");
2546+
2547+
// ...with a password, which used to throw for an unrelated reason: rfind(':') made
2548+
// "pass@example.com" the PORT text, so the failure was accidental rather than a rule.
2549+
auto withPassword = HttpClient::parseUrl("http://user:pass@example.com/p");
2550+
EXPECT_EQ(withPassword.host, "example.com");
2551+
EXPECT_EQ(withPassword.port, 80);
2552+
2553+
// ...and with a port after it, which is the combination that proves the split happens in
2554+
// the right order.
2555+
auto withPort = HttpClient::parseUrl("http://user:pass@example.com:8080/p");
2556+
EXPECT_EQ(withPort.host, "example.com");
2557+
EXPECT_EQ(withPort.port, 8080);
2558+
}
2559+
2560+
TEST(HttpClientParseUrlUserInfoTests, Fix2072_TheLastAtSignIsTheDelimiter) {
2561+
// RFC 3986 3.2.1: a '@' is legal INSIDE the userinfo, and the host production admits none,
2562+
// so the FINAL one is the delimiter. Splitting on the first would make "b@example.com" the
2563+
// host of "http://a@b@example.com/".
2564+
auto p = HttpClient::parseUrl("http://a@b@example.com/p");
2565+
EXPECT_EQ(p.host, "example.com");
2566+
}
2567+
2568+
TEST(HttpClientParseUrlUserInfoTests, Fix2072_AnEmptyHostAfterUserInfoIsRejected) {
2569+
EXPECT_THROW((void)HttpClient::parseUrl("http://user@/p"), System::UriFormatException);
2570+
EXPECT_THROW((void)HttpClient::parseUrl("http://@/p"), System::UriFormatException);
2571+
}
2572+
2573+
TEST(HttpClientParseUrlUserInfoTests, Fix2072_TextAfterAnIPv6BracketIsRejectedNotDiscarded) {
2574+
// Measured before this, `http://[::1]x/p` returned host "::1" and SILENTLY DISCARDED the
2575+
// 'x' -- so the URL the caller wrote and the URL the client connected to differed, with no
2576+
// diagnostic. RFC 3986 3.2.2 allows only `":" port` after the closing bracket.
2577+
EXPECT_THROW((void)HttpClient::parseUrl("http://[::1]x/p"), System::UriFormatException);
2578+
EXPECT_THROW((void)HttpClient::parseUrl("http://[::1]8080/p"), System::UriFormatException);
2579+
2580+
// The two legal shapes still work.
2581+
auto bare = HttpClient::parseUrl("http://[::1]/p");
2582+
EXPECT_EQ(bare.host, "::1");
2583+
EXPECT_EQ(bare.port, 80);
2584+
auto withPort = HttpClient::parseUrl("http://[::1]:8080/p");
2585+
EXPECT_EQ(withPort.host, "::1");
2586+
EXPECT_EQ(withPort.port, 8080);
2587+
// ...including with userinfo in front of the literal.
2588+
auto withUser = HttpClient::parseUrl("http://user@[::1]:8080/p");
2589+
EXPECT_EQ(withUser.host, "::1");
2590+
EXPECT_EQ(withUser.port, 8080);
2591+
}
2592+
2593+
TEST(HttpClientParseUrlUserInfoTests, Fix2072_AUrlWithoutUserInfoIsUntouched) {
2594+
// The invariance row: the '@' rule must not disturb anything that has no '@'.
2595+
auto plain = HttpClient::parseUrl("http://example.com:8080/a/b?q=1");
2596+
EXPECT_EQ(plain.host, "example.com");
2597+
EXPECT_EQ(plain.port, 8080);
2598+
EXPECT_EQ(plain.path, "/a/b?q=1");
2599+
// An '@' in the PATH is not an authority delimiter, because the authority ended first.
2600+
auto atInPath = HttpClient::parseUrl("http://example.com/a@b");
2601+
EXPECT_EQ(atInPath.host, "example.com");
2602+
EXPECT_EQ(atInPath.path, "/a@b");
2603+
}

plan.sqlite3

4 KB
Binary file not shown.

0 commit comments

Comments
 (0)