fix(net): harden the loopback transport guard across all HTTP clients#804
Open
benw5483 wants to merge 2 commits into
Open
fix(net): harden the loopback transport guard across all HTTP clients#804benw5483 wants to merge 2 commits into
benw5483 wants to merge 2 commits into
Conversation
`build_http_client` decided whether to relax the HTTPS-only transport
guard with `str::starts_with("http://localhost")` / `127.0.0.1` / `[::1]`
on the raw URL. That prefix match is satisfied by attacker-controlled
hosts — `http://localhost.evil.com`, `http://127.0.0.1.evil.com`, and the
userinfo form `http://127.0.0.1@evil.com` (the connection's real host is
`evil.com`) — so a caller who controls the base URL could disable
`https_only` and send credentials to a non-loopback host in clear text.
Replace the prefix check with a parsed-URL match (`is_loopback_http_url`):
require the `http` scheme, reject any userinfo component, and match the
host exactly against `localhost` / `127.0.0.1` / `::1`. Genuine loopback
dev endpoints still work; every other URL is forced through HTTPS.
Add negative tests for the three bypass URLs (the guard now refuses them)
and positive tests for real loopback and https endpoints.
Generated by the operator's software factory.
City: factory-main · Agent: local-core.builder-2
On behalf of: @benw5483
Co-Authored-By: Actual Factory Bot <factory-bot@actual-software.invalid>
benw5483
changed the title
benw5483
force-pushed
the
oauth-exact-loopback-guard
branch
from
1bbaa61 to
0acc438
Compare
…uard The bypassable loopback check fixed in the auth client was duplicated, with the same `str::starts_with` shape, in every other HTTP-client constructor in the crate: the platform API client (which attaches the login-session bearer to every authenticated call), the model-cache probes, and the OpenAI / Anthropic runner clients. Each could be tricked by `http://localhost.evil.com` or `http://127.0.0.1@evil.com` into disabling `https_only` for a non-loopback host. Extract the hardened check into a single `crate::net::is_loopback_http_url` (parse with `url::Url`, require the `http` scheme, reject userinfo, and match the host exactly against `localhost` / `127.0.0.1` / `::1`) and route every one of these constructors through it, so there is one guard and no divergent copies. Behavior is preserved for genuine loopback dev endpoints; every other URL is forced through HTTPS. The predicate's unit tests move to the new module alongside it. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-2 On behalf of: @benw5483 Co-Authored-By: Actual Factory Bot <factory-bot@actual-software.invalid>
benw5483
force-pushed
the
oauth-exact-loopback-guard
branch
from
0acc438 to
b26c7b8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Several HTTP clients in this crate allow clear-text
httpfor a local dev / mock server by disabling reqwest'shttps_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 isevil.com.Any of these made a client treat a non-loopback host as loopback, disabling
https_onlyand 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 thehttpscheme, rejects any userinfo component, and matches the host exactly againstlocalhost/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
httpsendpoints behave correctly; the auth client's integration tests cover its use of the helper.cargo testgreen;cargo fmt --checkandcargo clippy -- -D warningsclean.grepconfirms nostarts_withloopback guard remains — every client routes through the shared helper.localhost/127.0.0.1/::1) matches the intended dev endpoints for all of these clients.