fix(net): harden the loopback transport guard across all HTTP clients

[benw5483]

Stacked on #803. This PR now targets feat/mint-token-jwt-bearer rather than main, so the diff above is only this PR's own change. #803 adds a headless jwt-bearer client that calls the same build_http_client, and stacking means that client inherits the hardened guard directly instead of the two PRs racing to redefine it. GitHub retargets this PR to main automatically once #803 merges.

Summary

Several HTTP clients in this crate allow clear-text http for a local dev / mock server by disabling reqwest's https_only, and they all decided "is this loopback?" with a raw string prefix check (starts_with("http://localhost") / 127.0.0.1 / [::1]). That prefix match is satisfied by attacker-controlled hosts:

• http://localhost.evil.com / http://127.0.0.1.evil.com — lookalike hosts that merely start with the loopback literal;
• http://127.0.0.1@evil.com — a userinfo prefix whose real connection host is evil.com.

Any of these made a client treat a non-loopback host as loopback, disabling https_only and letting credentials be sent in clear text when the caller controls the base URL (--api-url / ACTUAL_API_URL).

The same bypassable check was duplicated across ~6 constructors: the auth client (auth/oauth.rs), the platform API client (api/client.rs, which attaches the login-session bearer to every authenticated call), the model-cache probes (model_cache.rs), and the OpenAI / Anthropic runner clients.

This extracts one hardened helper — net::is_loopback_http_url — and routes every constructor through it. The helper parses the URL, requires the http scheme, rejects any userinfo component, and matches the host exactly against localhost / 127.0.0.1 / ::1. Genuine loopback dev endpoints keep working; every other URL is forced through HTTPS. No divergent copies of the check remain.

Test plan

• New unit tests on the helper assert the bypass URLs are not loopback (the guard stays on) and that real loopback / https endpoints behave correctly; the auth client's integration tests cover its use of the helper.
• cargo test green; cargo fmt --check and cargo clippy -- -D warnings clean.
• A grep confirms no starts_with loopback guard remains — every client routes through the shared helper.
• Reviewer: confirm the accepted loopback set (localhost / 127.0.0.1 / ::1) matches the intended dev endpoints for all of these clients.

---

Generated by the operator's software factory.
• On behalf of: @benw5483

[davidmiuraactualai]

Actual Adversarial Review

Key findings

---

No actionable defects survived validation. The shared predicate parses the URL, rejects userinfo, and accepts only clear-text loopback identities. All six affected constructors call it; their non-loopback paths retain reqwest HTTPS-only transport. This matches ClientBuilder::https_only semantics for the repository-pinned reqwest 0.12 client.

The implementation also meets docs/adr/fd1181e0-c600-46af-94d8-829c8e8b2b12-adopt-comprehensive-unit-testing-strategy-for-ci-cd-pipeline-components.md: src/net.rs has isolated positive loopback and negative lookalike, userinfo, non-HTTP, and malformed-URL tests, while the OAuth client retains integration-level guard tests.

Architecture intent

No architecture-intent decision is needed for this change.

---

Review metadata

Important

All tracked findings are addressed at head b26c7b8854bfd5386e83d6d2ac56fcce7c36e31f. This is the canonical remediation record for the PR.

Compared: upstream main at cfcae4cd84645417e7d8042517a69cbd55d22404 … PR head b26c7b8854bfd5386e83d6d2ac56fcce7c36e31f
CI: Build, Test, Lint, Coverage Enforcement, and CodeQL all succeeded for the exact head; Live E2E was skipped. Review verification: immutable-head static trace confirmed all affected client constructors call the shared guard and preserve HTTPS-only behavior for non-loopback URLs.

[wattswolf]

Clean security fix: swaps the bypassable starts_with prefix checks for a real URL parse that matches the actual host, rejects userinfo smuggling, and only relaxes https_only for genuine loopback. All six call sites converted consistently, well covered by unit tests, CI green. Good to merge.

[austinborn]

Reviewed the full diff at head b26c7b88. This is the one with a real bug behind it, and the fix is correct.

No blocking findings. Approving.

The bug this closes

The old guard decided "is this loopback?" with str::starts_with on the raw URL, which is a prefix test standing in for a host test. http://localhost.evil.com satisfies starts_with("http://localhost"). So does http://127.0.0.1.evil.com for the IPv4 form. Matching means https_only gets switched off, and a caller-controlled base URL then carries credentials over clear-text HTTP to a host the attacker owns. is_loopback_http_url parses the URL and compares the real host instead, which is the right shape: the userinfo rejection also kills http://127.0.0.1@evil.com, where the connection's actual destination is the part after the @.

Comparing against Ipv4Addr::LOCALHOST and Ipv6Addr::LOCALHOST rather than the wider 127.0.0.0/8 range is stricter than it strictly needs to be, and the doc comment owns that choice, so I read it as deliberate.

Verifying the "across all HTTP clients" claim

The title makes a coverage claim, so I checked it against the tree at head rather than the file list:

• All six production sites that gate https_only on a loopback decision now route through is_loopback_http_url: api/client.rs:65, auth/oauth.rs:155, both model_cache.rs sites (160, 251), runner/anthropic_api.rs:87, and the https_only(false) branch at runner/openai_api.rs:156.
• No raw prefix-match loopback guard survives in production code. The three remaining starts_with("http://…") hits are two test assertions and the doc comment in net.rs describing the old pattern.
• Every Client::builder() without an https_only call sits inside a #[cfg(test)] module, past the single #[cfg(test)] marker in each file.
• No danger_accept_invalid_certs or other TLS escape anywhere in the crate.

The claim holds. The test table covering localhost.evil.com, 127.0.0.1.evil.com, 127.0.0.1@evil.com, and the https/no-host/garbage cases is the right set, and pinning url (already an indirect dependency via reqwest) is the cheap way to get an actual parser.

Merge order with #801

#801 touches build_http_client too, only to widen it to pub(crate), and its hunk overlaps this one. Whichever lands second will conflict in src/auth/oauth.rs. This one should land first, and when #801 is rebased the resolution needs to keep is_loopback_http_url rather than restore the starts_with form. Worth saying out loud, because a conflict resolved by taking the incoming side would silently undo this fix while leaving the PR looking merged.

---

Posted by the operator's software factory.
• City: factory-main · Agent: local-core.builder-1
• On behalf of: @austinborn