security: consolidate the HTTP clients through one hardened builder (the config client leaks a credential on redirect)

hexajon commented

2026-10-03 21:15:59 +00:00

Owner

What happens

madtea builds three HTTP clients independently, and only one carries the redirect guard.

internal/gitea/client.go builds through buildHTTPClient, which wires a CheckRedirect that strips the Authorization header on any cross-origin or scheme-downgrade redirect hop, and stops a method-changing redirect. Hardened.
internal/config/config.go:61 builds a bare &http.Client{Timeout: ...} with no CheckRedirect. It carries a credential on three call sites:
- internal/config/request.go:36 - the token header on whoami and the pre-trust probes.
- internal/config/oauth_mint.go:76-79 - the OAuth token via basic-auth.
- internal/config/oauth.go:223-230 - the OAuth code-exchange POST (code + verifier).
internal/cmd/setup/prompt.go:168 also builds a bare client, but its one request is a token-less HEAD, so it carries no credential.

net/http re-attaches Authorization across a redirect to the same host and ignores a scheme downgrade, so a same-host https -> http redirect from the configured forge sends the token (or the OAuth token) in cleartext. The OAuth POST also resends its body on a 307/308. The gitea client is immune; the config client is not.

Scope (full sweep)

Exactly three production http.Client constructions exist (gitea client.go, config config.go:61, setup prompt.go:168). No http.DefaultClient, no custom transport elsewhere. Three clients, one policy, is the whole surface. (The sweep also found unrelated items filed separately: a missing ref check on git tag <name> at tag.go:173, a raw path-concat in client_repo_flags.go, and the OAuth client-id legacy-key fallback.)

Root cause

There is no shared HTTP module. internal/config is the base layer - the API client is built from the credentials it resolves - so it cannot import internal/gitea, and it hand-rolled a bare client. The redirect guard was written into the gitea client alone.

Decision grounding

This finishes the logic of two accepted records; it does not fork them. ADR 0041 ruled that the token-security check belongs at one chokepoint, no opt-out, but scoped it to internal/config's request function and never reckoned with the separate gitea attach path or the redirect dimension. ADR 0025 section 5: a credential attaches only to its authorized origin - the redirect strip is the per-hop half of that. Credential resolution stays in internal/config (ADR 0025 / 0027); the new module owns transport only. The transport-module decision is filed as a proposed ADR for second-contributor assent (ADR 0047), not landed as accepted by one author.

Fix

A new base package internal/httpx owns the hardened builder (redirect credential strip, method-preserving redirect stop, 64 MiB body cap, same-origin check) and imports no other madtea package. internal/gitea, internal/config, and internal/cmd/setup all build through httpx.Build; the config client's three call sites inherit the strip with no change to how they attach the token.

Acceptance criteria

Every production http.Client is built through internal/httpx.
A test proves the config client strips the credential on a cross-origin and on an https -> http same-host redirect, and stops a POST redirect.
A guard test fails if a raw http.Client, http.Get, or http.DefaultClient is used outside internal/httpx.
The gitea client's existing redirect, token-pin, and body-cap tests pass unchanged.
./scripts/gate.sh is green.
The transport-module ADR is filed as proposed.

Part of v0.20.0 (#675). The only behavior change for a legitimate user is a credential stripped on a malicious downgrade redirect - intended.

## What happens madtea builds three HTTP clients independently, and only one carries the redirect guard. - `internal/gitea/client.go` builds through `buildHTTPClient`, which wires a `CheckRedirect` that strips the `Authorization` header on any cross-origin or scheme-downgrade redirect hop, and stops a method-changing redirect. Hardened. - `internal/config/config.go:61` builds a bare `&http.Client{Timeout: ...}` with no `CheckRedirect`. It carries a credential on three call sites: - `internal/config/request.go:36` - the `token ` header on `whoami` and the pre-trust probes. - `internal/config/oauth_mint.go:76-79` - the OAuth token via basic-auth. - `internal/config/oauth.go:223-230` - the OAuth code-exchange POST (code + verifier). - `internal/cmd/setup/prompt.go:168` also builds a bare client, but its one request is a token-less `HEAD`, so it carries no credential. net/http re-attaches `Authorization` across a redirect to the same host and ignores a scheme downgrade, so a same-host `https -> http` redirect from the configured forge sends the token (or the OAuth token) in cleartext. The OAuth POST also resends its body on a 307/308. The gitea client is immune; the config client is not. ## Scope (full sweep) Exactly three production `http.Client` constructions exist (gitea `client.go`, config `config.go:61`, setup `prompt.go:168`). No `http.DefaultClient`, no custom transport elsewhere. Three clients, one policy, is the whole surface. (The sweep also found unrelated items filed separately: a missing ref check on `git tag <name>` at `tag.go:173`, a raw path-concat in `client_repo_flags.go`, and the OAuth client-id legacy-key fallback.) ## Root cause There is no shared HTTP module. `internal/config` is the base layer - the API client is built from the credentials it resolves - so it cannot import `internal/gitea`, and it hand-rolled a bare client. The redirect guard was written into the gitea client alone. ## Decision grounding This finishes the logic of two accepted records; it does not fork them. ADR 0041 ruled that the token-security check belongs at one chokepoint, no opt-out, but scoped it to `internal/config`'s request function and never reckoned with the separate gitea attach path or the redirect dimension. ADR 0025 section 5: a credential attaches only to its authorized origin - the redirect strip is the per-hop half of that. Credential resolution stays in `internal/config` (ADR 0025 / 0027); the new module owns transport only. The transport-module decision is filed as a `proposed` ADR for second-contributor assent (ADR 0047), not landed as `accepted` by one author. ## Fix A new base package `internal/httpx` owns the hardened builder (redirect credential strip, method-preserving redirect stop, 64 MiB body cap, same-origin check) and imports no other madtea package. `internal/gitea`, `internal/config`, and `internal/cmd/setup` all build through `httpx.Build`; the config client's three call sites inherit the strip with no change to how they attach the token. ## Acceptance criteria - [ ] Every production `http.Client` is built through `internal/httpx`. - [ ] A test proves the config client strips the credential on a cross-origin and on an `https -> http` same-host redirect, and stops a POST redirect. - [ ] A guard test fails if a raw `http.Client`, `http.Get`, or `http.DefaultClient` is used outside `internal/httpx`. - [ ] The gitea client's existing redirect, token-pin, and body-cap tests pass unchanged. - [ ] `./scripts/gate.sh` is green. - [ ] The transport-module ADR is filed as `proposed`. Part of v0.20.0 (#675). The only behavior change for a legitimate user is a credential stripped on a malicious downgrade redirect - intended.

hexajon added the

severity/medium

security

labels

2026-10-03 21:15:59 +00:00

hexajon referenced this issue from a pull request that will close it,

2026-10-04 18:02:51 +00:00

security: route every HTTP client through one hardened builder (closes the config redirect leak) #709

hexajon referenced this issue from a commit

2026-10-04 18:02:52 +00:00

feat(httpx): add the hardened HTTP client builder

hexajon referenced this issue from a commit