From 5f2d72dc95ac5b115096bc09cf020e24d00d72c9 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 06:07:26 -0400 Subject: [PATCH 01/15] docs: spec the Host validation middleware that closes DNS-rebinding middleman validates that the configured bind is loopback and gates mutating /api/v1 calls on Sec-Fetch-Site, but the TCP listener accepts requests regardless of Host. A page that resolves its hostname to 127.0.0.1 after the initial fetch (DNS rebinding) sends every fetch as "same-origin" so checkCSRF passes; the request lands on the loopback listener and is processed. This spec describes the Host validation middleware (parser shared with config.allowed_hosts via internal/config/hostmatch.go) and the trust_reverse_proxy flag that opt-in honors X-Forwarded-Host / Forwarded under proxied deployments. Both Backend Host and Public Host must independently pass the allowlist gate, so a DNS-rebound request spoofing X-Forwarded-Host is still rejected at Step 2 on the raw Host. --- ...026-05-19-host-origin-middleware-design.md | 338 ++++++++++++++++++ 1 file changed, 338 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-19-host-origin-middleware-design.md diff --git a/docs/superpowers/specs/2026-05-19-host-origin-middleware-design.md b/docs/superpowers/specs/2026-05-19-host-origin-middleware-design.md new file mode 100644 index 000000000..06fd13916 --- /dev/null +++ b/docs/superpowers/specs/2026-05-19-host-origin-middleware-design.md @@ -0,0 +1,338 @@ +# Host validation middleware — design + +(Originally framed as "Host/Origin middleware". `Origin` is not used +by this middleware; cross-origin rejection stays with the existing +`checkCSRF` middleware via `Sec-Fetch-Site`. The Host gate alone +closes the DNS-rebinding bypass.) + +## Motivation + +middleman's HTTP server validates that its configured bind is loopback +(`internal/config/config.go:786`) and gates mutating API calls on +`Sec-Fetch-Site` plus `Content-Type: application/json` +(`internal/server/server.go:719,740`). Neither check inspects the +request's `Host` header against the configured bind, so the TCP +listener accepts requests with arbitrary `Host`. + +That gap enables DNS rebinding: a page on `attacker.example` makes +its first DNS lookup resolve normally, then a TTL-zero re-lookup +points the same hostname at `127.0.0.1`. The browser sees a single +origin (`attacker.example`) so it sends every subsequent request +with `Sec-Fetch-Site: same-origin`, bypassing the CSRF middleware, +while the TCP connection lands on middleman's loopback listener. + +middleman is local-first and stores sensitive per-user data (tokens +via env vars, SQLite of PR/MR state, workspace sessions). A +DNS-rebinding bypass on a mutating endpoint can drive the workspace +runtime or rewrite config. Closing the gap requires a Host check +keyed off the configured bind, with explicit opt-outs for the two +legitimate cases that need a different `Host`: extra hostnames the +operator registers (`allowed_hosts`) and reverse-proxy deployments +that put a different hostname in front (`trust_reverse_proxy`). + +## Vocabulary + +The middleware sees up to two distinct hostnames per request. We use +these names consistently throughout the spec: + +- **Backend Host** (a.k.a. raw / proxy-facing Host): the value of + the request's `Host` header as it lands on middleman's listener. + For a reverse-proxy deployment this is what the proxy sends to + middleman, which may be the loopback address or a rewritten + internal hostname. +- **Public Host** (a.k.a. client-facing / forwarded Host): the value + the browser typed; surfaces only when `trust_reverse_proxy = true` + via `X-Forwarded-Host` or `Forwarded` `host=`. + +Both must independently pass the allowlist gate. `allowed_hosts` +holds the union: every non-loopback hostname expected at either +position must be listed. Loopback synonyms (127.0.0.1 / localhost / +[::1]) at the bind port are auto-accepted and do not need to be +listed. + +## Goals + +- Reject any HTTP request whose Backend Host does not canonical-match + the configured bind, a loopback synonym for the bind port, or an + entry in `allowed_hosts`. +- When `trust_reverse_proxy = true`, additionally reject any request + whose Public Host (from `X-Forwarded-Host` or `Forwarded`) does + not canonical-match the same set. +- Keep the existing reverse-proxy / `base_path` deployment story + working: one or two `allowed_hosts` entries plus the + `trust_reverse_proxy` flag suffice. +- Return a 403 with a JSON body that names the two config knobs + (`allowed_hosts`, `trust_reverse_proxy`) so a clobbered request + is debuggable from curl output alone. +- Cover every code path with wire-level tests against the real + middleware chain, plus focused config-validation tests for the + canonicalization parser. + +## Non-goals + +- Token binding to client IP / TLS fingerprint. +- Per-route Host allowlists. The middleware decision is global. +- Wildcard / glob / regex matching in `allowed_hosts`. Exact-string + after canonicalization in v1. If patterns are needed later, a + separate `allowed_host_patterns` field can be added without + breaking v1 semantics. +- Changing the existing CSRF middleware (`checkCSRF`). The new + middleware runs alongside it. +- Inspecting the `Origin` header (covered by `Sec-Fetch-Site` + already). + +## Threat model + +| Attacker | Vector | Pre-fix outcome | Post-fix outcome | +|---|---|---|---| +| Malicious web page | DNS rebinding to 127.0.0.1 | `Sec-Fetch-Site: same-origin` because browser sees one origin; passes CSRF; reaches handler | Backend Host is `attacker.example:8091` (browser sends what it typed). Step 2 rejects 403 before CSRF runs. | +| Direct curl from another LAN host hitting loopback | N/A | Already rejected by bind | Already rejected by bind | +| Browser on the same host typing `http://127.0.0.1:8091` | Normal use | Allowed | Allowed (Backend Host `127.0.0.1:8091` matches bind) | +| Browser typing `http://localhost:8091` | Normal use | Allowed | Allowed (`localhost` is a loopback synonym for the bind port) | +| Reverse-proxy front, public hostname `mm.example.com` | Proxied use | Already works (no Host check) | Operator sets `trust_reverse_proxy = true` and adds the Public Host (and the proxy-set Backend Host, if it isn't loopback) to `allowed_hosts`. | +| DNS-rebound page with spoofed `X-Forwarded-Host: mm.example.com` | Hits loopback directly with attacker Host on Host header | N/A pre-fix | Backend Host fails Step 2 regardless of `X-Forwarded-Host` content. Forwarded header is never read. | + +## Validation flow + +The middleware runs FIRST on every request (before `checkCSRF`, +before the `basePath` stripper, before any handler). Validation is +identical for safe and unsafe methods — DNS rebinding can drive +both `GET` (read-out) and `POST` (write) attacks, and the cost of +validating GETs is negligible. + +Canonicalization: parse a host header value into a `hostKey` of +`(lowercase_host, port_string_or_empty)`. IPv6 literals are wrapped +in `[]`. No default-port assumption: an empty port stays empty. + +For each request: + +1. **Parse Backend Host.** Split into `hostKey`. Empty / malformed → + 403. +2. **Validate Backend Host** against the accepted set: + - The bind's canonical `hostKey` (e.g. `("127.0.0.1", "8091")`). + - Loopback synonyms at the bind port: `("localhost", "8091")`, + `("127.0.0.1", "8091")`, `("[::1]", "8091")`. These are added + unconditionally because the existing config validation forces + loopback binds; the synonyms collapse to a fixed three-element + set parameterised only by the bind port. + - Each `allowed_hosts` entry, in canonical form. Entry semantics + are LITERAL: + - An entry like `mm.example.com:8443` matches only a Backend + Host of `mm.example.com:8443`. + - An entry like `mm.example.com` (no port) matches only a + Backend Host of `mm.example.com` with no explicit port. + - We do NOT collapse "default ports" (80/443). middleman binds + on a non-standard port by default and operators see HTTP + fronted by a proxy with the proxy-public port; explicit + entries prevent surprises. + + If no match, 403. +3. **(Optional, `trust_reverse_proxy` only.)** When the flag is + false, the request is accepted at Step 2. When true: + - For each present forwarded-host header, parse only the FIRST + comma-separated entry (ignore any later entries): + - `X-Forwarded-Host`: the first comma-separated entry is the + host value. Trim ASCII whitespace. + - `Forwarded`: the first comma-separated entry must contain a + `host=` parameter (case-insensitive). Unquote `host="..."` + if quoted. If the first entry lacks `host=`, treat the + header as malformed. + - An empty or malformed selected entry from EITHER header (when + that header is present) is a 403; we do not silently fall + back to the other header if one is present but bad. A header + that is absent altogether (no `X-Forwarded-Host` line, no + `Forwarded` line) is just absent. + - If both headers are present and their parsed canonical + `hostKey` disagree, 403. + - If both are absent under `trust_reverse_proxy = true`, 403 + (the proxy did not supply a forwarded host; the deployment is + mis-configured). + - Validate the selected Public Host against the same accepted + set used in Step 2. Mismatch → 403. + +The Step 2 gate is the security-critical step: a DNS-rebound request +to 127.0.0.1 cannot smuggle a spoofed `X-Forwarded-Host` past Step 2 +because the attacker controls only the Backend Host (and that +hostname is, by attack construction, not loopback and not in +`allowed_hosts` unless the operator explicitly added it). + +## Config + +Two new optional fields on `config.Config`: + +```toml +# Extra Host headers to accept beyond the bind address. +# Exact-string match after canonicalization (lowercase host, +# preserve port, IPv6 in brackets). Loopback synonyms +# (127.0.0.1 / localhost / [::1]) are auto-accepted at the bind +# port and do not need to be listed. +allowed_hosts = ["mm.local:8091", "mm.example.com"] + +# When true, honor X-Forwarded-Host and Forwarded RFC 7239 host= +# under reverse-proxy deployments. The Backend Host must still +# pass the allowed_hosts gate before any forwarded header is read. +trust_reverse_proxy = true +``` + +Defaults: `allowed_hosts = []`, `trust_reverse_proxy = false`. + +Validation: + +- Each `allowed_hosts` entry must parse with the same canonicalization + as runtime `Host` parsing. Invalid entries fail config load with + `config: invalid allowed_hosts entry %q`. +- Entries are stored canonicalized (lowercase host, port preserved, + IPv6 bracketed). Comparison is exact-string against the canonical + `hostKey`. +- IPv6 entries must be bracketed (`[::1]:8091`). Validation rejects + unbracketed IPv6 literals to match existing repo conventions + (`internal/config/config.go:427`). +- `trust_reverse_proxy = true` with empty `allowed_hosts` is allowed + (the validator does not gate on it) but emits a startup + `slog.Warn` because the deployment likely needs at least one + allowlist entry for the Public Host. + +## 403 body + +Single JSON shape using the existing `writeError` helper: + +```json +{ + "error": "host validation failed: the requested hostname is not allowed. Add expected Backend and Public hostnames to allowed_hosts in middleman's config.toml. If a reverse proxy sets forwarded-host headers, also enable trust_reverse_proxy." +} +``` + +The rejected hostname is NOT echoed into the body (avoids reflecting +attacker-controlled input and avoids log injection via crafted Host +values). It is recorded server-side via `slog.Warn` with fields +`reason`, `host`, `forwarded_host`, `remote_addr` for operator +debugging. + +## Implementation surface + +Module layout. `internal/server` already imports `internal/config`, +so the canonicalization parser lives in `internal/config` and is +called by both config validation and server middleware. The server +package never re-implements the parser; it borrows the exported +helper. This satisfies the "same canonicalization" rule without +creating an import cycle. + +| File | Change | +|---|---| +| `internal/config/hostmatch.go` | New file. Exports `ParseHostKey(string) (HostKey, error)` plus the `HostKey{Host, Port string}` type. Pure stdlib. | +| `internal/config/hostmatch_test.go` | New file. Table-driven tests for the parser: good entries, bad entries, IPv6 bracketing, uppercase normalisation, empty/missing port. | +| `internal/config/config.go` | Add `AllowedHosts []string`, `TrustReverseProxy bool` to `Config`. Validate via `ParseHostKey`. Canonicalize once at load and store the parsed list on the config struct (e.g., `parsedAllowedHosts []HostKey`, populated in `Validate` and accessed via a getter so the TOML-visible field stays a `[]string`). | +| `internal/server/host_check.go` | New file. Exports `checkHost(w, r, bindKey, allowed []config.HostKey, trustProxy) bool`. Uses `config.ParseHostKey` for runtime Host parsing. Pure stdlib + slog + the existing `writeError` helper. | +| `internal/server/server.go` | Wire `checkHost` into `ServeHTTP` BEFORE `checkCSRF`. Plumb bind host:port plus `allowed_hosts`/`trust_reverse_proxy` through `Server` struct (default constructor reads them from `cfg`; the test constructor accepts an explicit option so callers without a full `config.Config` can still wire the middleware). | +| `internal/server/host_check_test.go` | New wire-level test file. Table-driven cases per the test plan below. | +| `internal/server/apitest/host_check_test.go` | New apitest case per the test plan below. | +| `cmd/middleman/main.go` | Confirm `server.NewWithConfig` is given the bind host:port; no other change. | +| `README.md` | Document the two new fields under Configuration. | + +## Test plan + +### Parser-level (`internal/config/hostmatch_test.go`) + +| name | input | expect | +|---|---|---| +| empty allowed_hosts | `[]` | no error | +| valid entry with port | `["mm.local:8091"]` | canonicalised to `("mm.local", "8091")` | +| valid bare entry (no port) | `["mm.local"]` | canonicalised to `("mm.local", "")` | +| uppercase canonicalisation | `["MM.Local:8091"]` | canonicalised to `("mm.local", "8091")` | +| IPv6 entry with port | `["[::1]:8091"]` | canonicalised to `("[::1]", "8091")` | +| IPv6 entry no port | `["[::1]"]` | canonicalised to `("[::1]", "")` | +| unbracketed IPv6 | `["::1:8091"]` | error: `config: invalid allowed_hosts entry %q` | +| empty entry | `[""]` | error | +| port-only entry | `[":8091"]` | error | +| port out of range | `["mm.local:99999"]` | error | +| trust_reverse_proxy default | (unset) | false | +| allowed_hosts default | (unset) | `nil` | + +### Wire-level (`internal/server/host_check_test.go`) + +All tests build a `Server` via the same helper used by +`basepath_test.go` and drive requests through `Server.ServeHTTP`, +which is the established full-stack shape for the existing CSRF +tests. They thread the new fields through a constructor option so +the assertions don't depend on a stub `config.Config`. Each case is +a table row in a single Go test function (or two: one for raw-Host +behavior, one for the forwarded-header dance). + +Bind for every case below: `127.0.0.1:8091`. + +| name | allowed_hosts | trust_reverse_proxy | Host | X-Forwarded-Host | Forwarded | expect | +|---|---|---|---|---|---|---| +| direct loopback IP | [] | false | `127.0.0.1:8091` | - | - | 200 | +| direct localhost | [] | false | `localhost:8091` | - | - | 200 | +| direct IPv6 loopback | [] | false | `[::1]:8091` | - | - | 200 | +| uppercase host accepted | [] | false | `LOCALHOST:8091` | - | - | 200 | +| wrong port | [] | false | `127.0.0.1:9999` | - | - | 403 | +| attacker host (DNS rebinding) | [] | false | `attacker.example:8091` | - | - | 403 | +| empty Host | [] | false | (none) | - | - | 403 | +| malformed Host | [] | false | `][` | - | - | 403 | +| port-only Host | [] | false | `:8091` | - | - | 403 | +| allowed_hosts hit, exact port | `["mm.local:8091"]` | false | `mm.local:8091` | - | - | 200 | +| allowed_hosts miss, wrong port | `["mm.local:8091"]` | false | `mm.local:9999` | - | - | 403 | +| allowed_hosts bare entry hits bare Host | `["mm.local"]` | false | `mm.local` | - | - | 200 | +| allowed_hosts bare entry rejects ported Host | `["mm.local"]` | false | `mm.local:8091` | - | - | 403 | +| IPv6 allowed_hosts hit | `["[::1]:8443"]` | false | `[::1]:8443` | - | - | 200 | +| allowed_hosts attacker miss | `["mm.local:8091"]` | false | `attacker.example:8091` | - | - | 403 | +| trust_reverse_proxy off, X-Forwarded-Host ignored | [] | false | `attacker.example:8091` | `127.0.0.1:8091` | - | 403 | +| trust on, raw Host loopback, XFH in allowlist | `["mm.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com` | - | 200 | +| trust on, raw Host loopback, Forwarded in allowlist | `["mm.example.com"]` | true | `127.0.0.1:8091` | - | `for=10.0.0.1;host=mm.example.com` | 200 | +| trust on, Forwarded quoted host | `["mm.example.com"]` | true | `127.0.0.1:8091` | - | `host="mm.example.com"` | 200 | +| trust on, multi-value XFH uses first entry | `["mm.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com, other.example.com` | - | 200 | +| trust on, multi-value XFH first-not-allowed | `["other.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com, other.example.com` | - | 403 | +| trust on, both headers agree | `["mm.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com` | `host=mm.example.com` | 200 | +| trust on, headers disagree | `["mm.example.com", "other.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com` | `host=other.example.com` | 403 | +| trust on, neither forwarded header | `["mm.example.com"]` | true | `127.0.0.1:8091` | - | - | 403 | +| trust on, forwarded host NOT in allowlist | [] | true | `127.0.0.1:8091` | `attacker.example` | - | 403 | +| trust on, raw Host fails (DNS rebinding even with proxy on) | `["mm.example.com"]` | true | `attacker.example:8091` | `mm.example.com` | - | 403 | +| trust on, empty XFH | [] | true | `127.0.0.1:8091` | `` (empty) | - | 403 | +| trust on, malformed Forwarded | [] | true | `127.0.0.1:8091` | - | `wat` | 403 | +| trust on, Forwarded first entry lacks host= | [] | true | `127.0.0.1:8091` | - | `for=10.0.0.1, host=mm.example.com` | 403 | +| trust on, present-but-malformed Forwarded with valid XFH | `["mm.example.com"]` | true | `127.0.0.1:8091` | `mm.example.com` | `wat` | 403 | +| trust on, present-but-empty XFH with valid Forwarded | `["mm.example.com"]` | true | `127.0.0.1:8091` | `` (empty) | `host=mm.example.com` | 403 | +| trust on, forwarded port mismatch | `["mm.example.com:8443"]` | true | `127.0.0.1:8091` | `mm.example.com:9999` | - | 403 | + +403 body shape: one dedicated test reads the JSON body on any 403 +case and asserts the body contains both `allowed_hosts` and +`trust_reverse_proxy` (string substring check) and matches the +`writeError` shape `{"error":"..."}`. + +Health endpoints (`/healthz`, `/livez`) are checked through the same +middleware. The brief does not call out exempting them, and a +loopback `GET /healthz` from the same host already passes Step 2 +naturally; an external prober hitting the proxy would have the +Public Host (handled by `trust_reverse_proxy + allowed_hosts`). If +concrete deployments need exemption, that's a follow-up. + +### Full-stack apitest (`internal/server/apitest/host_check_test.go`) + +Two cases through the real `setupTestServer` helper (SQLite-backed, +full Huma router). The apitest helper builds a server with a known +bind port (e.g. `127.0.0.1:8091` — the default the test server +constructor passes through). Tests issue requests with explicit +`Host: 127.0.0.1:8091` so port semantics match the locked rule +(no implicit default-port collapsing): + +1. `Host: attacker.example:8091` on a `GET /api/v1/pulls` request + returns 403 with the `{"error":"..."}` JSON body before any + handler runs. +2. `Host: 127.0.0.1:8091` on the same `GET /api/v1/pulls` request + reaches the handler and returns `200` with the seeded PR list. + +These cover the e2e contract per project rules (full HTTP API + +SQLite) without duplicating the parser exhaustiveness already +covered in `host_check_test.go`. + +## Open questions / follow-ups + +- IPv6 dual-stack bind (`::`) is not currently allowed by config + validation (loopback only), so the canonicalization does not have + to handle wildcard binds. +- Future: per-route `allowed_hosts_for_path` if a specific endpoint + needs different rules. None today. +- Future: surface a clearer settings-UI editor for `allowed_hosts`. +- Future: optional `denied_hosts` for explicit block-list. Not + needed in v1 given the default-deny posture. From 94eb060643727d378e95e75869598c594f231687 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 06:42:20 -0400 Subject: [PATCH 02/15] docs: plan the Host validation middleware implementation steps Tasks 1-7 cover: shared ParseHostKey parser in internal/config, two new config fields, the host_check.go middleware, ServeHTTP wiring with a strict precedence rule (caller override > validated cfg > cfg=nil test default > panic on partial cfg), wire-level tests, full-stack apitest coverage, and README documentation. The cfg=nil test-friendly default keeps the dozens of pre-existing internal/server test helpers working without per-test churn, while the two known partial-cfg sites get targeted ServerOptions.HostCheck overrides so the production contract stays strict. --- .../2026-05-19-host-origin-middleware.md | 323 ++++++++++++++++++ 1 file changed, 323 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-19-host-origin-middleware.md diff --git a/docs/superpowers/plans/2026-05-19-host-origin-middleware.md b/docs/superpowers/plans/2026-05-19-host-origin-middleware.md new file mode 100644 index 000000000..9a33f5242 --- /dev/null +++ b/docs/superpowers/plans/2026-05-19-host-origin-middleware.md @@ -0,0 +1,323 @@ +# Plan — Host validation middleware + +Companion to `docs/superpowers/specs/2026-05-19-host-origin-middleware-design.md`. + +Each task ends with one `git commit -m "..."` line per the +`writing-plans` template. Tasks are ordered by dependency. + +Repo testing conventions apply to every new test: + +- `github.com/stretchr/testify/require` for setup/preconditions and + `github.com/stretchr/testify/assert` for expectations + (`assert.New(t)` once a function has more than three checks). +- No `t.Fatal`, `t.Errorf`, `t.Error`, `t.Fail`, or manual + `if got != want` checks. +- Table-driven cases. +- Use `t.TempDir()` and the established helpers (e.g. `dbtest.Open`). + +## Task 1 — Add the canonicalization parser to internal/config + +- Add a new file `internal/config/hostmatch.go` exporting: + - `type HostKey struct { Host, Port string }` + - `func ParseHostKey(s string) (HostKey, error)` — accepts the + full input set: `mm.local`, `mm.local:8091`, `[::1]:8091`, + `[::1]`, `127.0.0.1:8091`, `LOCALHOST:8091`. Algorithm: + - Trim ASCII whitespace. Reject empty. + - If input starts with `[` and contains a matching `]`, the + part between the brackets is the host. After the closing + bracket, an optional `:port` may follow (otherwise port is + empty). Reject if there's any trailing garbage. The host + between brackets must parse as an IPv6 literal via + `net.ParseIP`. The canonical form keeps the brackets. + - Otherwise, if input contains a `:`, treat as `host:port` and + split on the LAST `:` so bare-host inputs without any `:` + route to the no-port branch. Reject if the resulting host + part contains any `:` (catches unbracketed IPv6 like `::1` + or `::1:8091`). Reject ports outside 1-65535 (parsed via + `strconv.Atoi`). Reject inputs that are port-only + (`:8091`). + - Otherwise, the input is host-only with no port. + - Lowercase the host. Keep port as the digits-only string (or + `""` for the no-port case). + - `func (k HostKey) String() string` returns `Host` when Port is + empty, otherwise `net.JoinHostPort(Host-without-brackets, Port)` + or `Host + ":" + Port` for the bracketed IPv6 case (treat the + bracketed `[::1]` as already a fully-qualified host literal, + so `[::1]:8091` is just `Host + ":" + Port`). Used for slog. + - `func (k HostKey) Equal(other HostKey) bool` — exact match on + both fields (host already lowercased on construction). + - `func (k HostKey) Valid() bool { return k.Host != "" && k.Port != "" }` — used by the server constructor's + precedence rule (Task 4) to decide whether a caller-supplied + or cfg-derived key counts as populated. +- Add `internal/config/hostmatch_test.go` with the spec's parser + table. Cover the explicit `[::1]` (no port) row in the table. + Use `assert.New(t)` and table-driven subtests. +- Verify: `nix run nixpkgs#go -- test ./internal/config/... -run TestParseHostKey -shuffle=on` + +```bash +git add internal/config/hostmatch.go internal/config/hostmatch_test.go +git commit -m "feat(config): add ParseHostKey for shared host canonicalization" +``` + +## Task 2 — Wire allowed_hosts, trust_reverse_proxy, and BindHostKey through config + +- Add two fields to `Config` in `internal/config/config.go`: + - `AllowedHosts []string \`toml:"allowed_hosts"\`` + - `TrustReverseProxy bool \`toml:"trust_reverse_proxy"\`` +- Add unexported caches on `Config`: + - `parsedAllowedHosts []HostKey` + - `parsedBindKey HostKey` +- In `Validate()`: + - After the existing loopback validation, build the bind key via + `ParseHostKey(net.JoinHostPort(c.Host, strconv.Itoa(c.Port)))`. + Fail config load with the existing `config: invalid host` + error wording on parse failure so we don't regress the message. + Cache to `parsedBindKey`. + - For each `AllowedHosts` entry, call `ParseHostKey`. On failure + return `fmt.Errorf("config: invalid allowed_hosts entry %q: %w", entry, err)`. Cache the slice on `parsedAllowedHosts`. + - `Validate()` MUST stay side-effect-light: no logging. The + `trust_reverse_proxy && empty allowed_hosts` startup warning + is emitted in `newServer` (Task 4), not here. +- Add accessors: + - `func (c *Config) ParsedAllowedHosts() []HostKey { return append([]HostKey(nil), c.parsedAllowedHosts...) }` (defensive copy). + - `func (c *Config) BindHostKey() HostKey { return c.parsedBindKey }`. +- Update `internal/config/config_test.go` with a small table for + the new fields: valid + invalid entries; defaults; bracketed + IPv6 entry; uppercase canonicalisation. +- Verify: `nix run nixpkgs#go -- test ./internal/config/... -shuffle=on` + +```bash +git add internal/config/config.go internal/config/config_test.go +git commit -m "feat(config): accept allowed_hosts and trust_reverse_proxy" +``` + +## Task 3 — Add the Host check middleware (no constructor wiring yet) + +- Add `internal/server/host_check.go` with: + - Exported `type HostCheckOptions struct { Bind config.HostKey; Allowed []config.HostKey; TrustReverseProxy bool }`. + - `func (o HostCheckOptions) Valid() bool` — true when + `o.Bind.Host != "" && o.Bind.Port != ""`. The middleware + requires both fields populated; a bind without a port is a + programming error. + - `func checkHost(w http.ResponseWriter, r *http.Request, opts HostCheckOptions) bool` — runs Steps 1-3 of the spec; returns false and writes the 403 body via `writeError` when invalid. The function panics with a `// programming error` panic if `!opts.Valid()`; production paths must always construct valid options. + - `parseForwardedHost(value string) (config.HostKey, bool, error)` + — RFC 7239 first-comma-entry parser for `host=` (case-insensitive + parameter name; quoted-value unwrap). Unexported. + - `parseXForwardedHost(value string) (config.HostKey, bool, error)` + — first comma entry, ASCII trim. Unexported. + - Fixed `hostValidationError` const carrying the 403 body string from the spec. + - `slog.Warn` on rejection with fields `reason`, `host`, + `forwarded_host`, `remote_addr`. +- Loopback synonym set computed by `checkHost`: when + `opts.Bind.Host` is one of `127.0.0.1`, `localhost`, `[::1]`, + the accepted set for Step 2 includes all three (with the same + port). Otherwise only the bind itself is auto-accepted in Step 2. +- Do NOT modify `Server.ServeHTTP` in this task — keep the change + surface small. Wiring happens in Task 4. +- Tests for the unexported parser helpers live in + `internal/server/host_check_test.go` (same package), so the + helpers stay unexported. +- Verify: `nix run nixpkgs#go -- build ./internal/server/...` + +```bash +git add internal/server/host_check.go +git commit -m "feat(server): add Host validation middleware (no wiring yet)" +``` + +## Task 4 — Wire the middleware into Server constructors and ServeHTTP + +- In `internal/server/server.go`: + - Add `hostOpts HostCheckOptions` to the `Server` struct. + - Update `newServer` to derive `hostOpts` as follows: + The rule for deriving `s.hostOpts` is single-pass and uses + strict precedence (no field-by-field merging — avoids the + "intentional zero vs omitted" ambiguity): + 1. **Caller override.** If `ServerOptions.HostCheck.Valid()` + (Bind is fully populated), use the entire override + as-is. Done. + 2. **Production path.** Else if `cfg != nil` and + `cfg.BindHostKey().Valid()` (cfg was loaded via + `config.Load` so `Validate()` cached the bind key), derive + the entire option set from cfg — + `HostCheckOptions{Bind: cfg.BindHostKey(), Allowed: + cfg.ParsedAllowedHosts(), TrustReverseProxy: + cfg.TrustReverseProxy}`. Done. + 3. **Test-friendly default.** Else if `cfg == nil` AND + `ServerOptions.HostCheck` is zero, install the documented + fallback: + `HostCheckOptions{Bind: {127.0.0.1, 8091}, Allowed: + [{example.com, ""}, {middleman.test, ""}], + TrustReverseProxy: false}`. This exists so the dozens of + pre-existing test helpers that construct servers with + `cfg = nil` keep working without per-test churn. The + default does NOT accept `attacker.example` or other + rebinding-style hosts. Security tests in Task 5 and Task 6 + use explicit options (step 1). + 4. **Fail-fast.** Else (`cfg != nil` but partial — Host/Port + not set, Validate never ran — and no override was + provided), panic with a programming-error message: + `"server: cannot construct without HostCheck options or a validated config"`. This forces partial-cfg test sites to pass an explicit `ServerOptions.HostCheck`. +- Emit `slog.Warn("cfg=nil server.New used without ServerOptions.HostCheck; using httptest-compatible Host defaults. Production callers must pass a validated config or explicit HostCheck options.")` exactly when step 3 fires (single intended log site). +- `Server.ServeHTTP` becomes (in order): + 1. `if !checkHost(w, r, s.hostOpts) { return }` — unconditional + 2. existing request-started log line + 3. existing CSRF gating +- Startup warning: in `newServer`, after `hostOpts` is set, if + `s.hostOpts.TrustReverseProxy && len(s.hostOpts.Allowed) == 0`, + emit `slog.Warn("trust_reverse_proxy is enabled but allowed_hosts is empty; only loopback Hosts will be accepted")`. +- Constructor call-site audit (`rg -n '\b(server\.)?(New|NewWithConfig)\b' cmd internal/server middleman.go --no-heading`): + - `cmd/middleman/main.go:343` — `NewWithConfig(cfg, ...)`. cfg + is loaded via `config.Load` which now caches the bind key; + nothing else to change. + - `cmd/middleman/main_test.go:310` — same, loaded cfg. + - `cmd/e2e-server/main.go:770` — same. + - `middleman.go:418` — top-level wrapper. Uses a non-nil `cfg` + (audit confirmed): `cfg` is constructed by the caller and + passed into the wrapper; it should be validated upstream. + No change needed beyond Task 2 ensuring `Validate()` caches + the bind key. + - All the `New(database, syncer, nil, "/", nil, ServerOptions{})` + call sites in `internal/server/api_test.go`, + `internal/server/apitest/*`, `internal/server/e2etest/*`, and + `internal/server/basepath_test.go::setupWithBasePath` — these + rely on the cfg=nil test-friendly default. No per-call-site + changes required. + - `internal/server/api_test.go:2549,2579` — + `cfg := &config.Config{BasePath: "/", Repos: ...}` and + `NewWithConfig(... cfg ...)`. Partial cfg without Host/Port + triggers step 4 (panic). Targeted fix: do NOT call + `cfg.Validate()` (would require padding out other partial + fields like `SyncInterval`). Instead pass an explicit + `ServerOptions.HostCheck` so step 1 of the precedence rule + provides the bind without touching the cfg literal: + `ServerOptions{HostCheck: server.HostCheckOptions{Bind: + config.HostKey{Host: "127.0.0.1", Port: "8091"}}}`. cfg + stays partial for the test's original purpose. + - `internal/server/workspacetest/fixtures_test.go:92` — + `server.New(database, syncer, nil, basePath, cfg, ...)`. cfg + is supplied by the test caller; audit confirms callers + either pass `nil` (step 3 covers) or pass a `*config.Config` + literal that omits Host/Port (step 4 panic). Same fix as + above: add the explicit `ServerOptions.HostCheck` override + in the helper so callers don't have to care about cfg + completeness. + - Both of the above are TWO targeted edits, much smaller than + the 30+ cfg=nil churn that step 3 covers automatically. + - One small change in `basepath_test.go::TestCSRF*`: the + existing CSRF tests issue `httptest.NewRequest`, which + defaults the request `Host` to `example.com`. The + cfg=nil test-friendly default explicitly allows `example.com` + so these continue to pass unchanged. +- Add a small constructor-level test + (`internal/server/host_check_default_test.go`) verifying that + `New(..., cfg=nil, ServerOptions{})` yields a server whose + `Server.ServeHTTP` accepts `Host: 127.0.0.1:8091`, + `Host: example.com`, and `Host: middleman.test`, and rejects + `Host: attacker.example`. This pins the test-friendly default + so future contributors don't widen it accidentally. +- Verify (broader, since Task 4 touches the request chain): + `nix run nixpkgs#go -- build ./... && nix run nixpkgs#go -- test ./internal/server/... -shuffle=on` + +```bash +git add internal/server/server.go internal/server/host_check_default_test.go \ + internal/server/api_test.go \ + internal/server/workspacetest/fixtures_test.go \ + cmd/middleman/main.go +git commit -m "feat(server): wire Host validation into the request chain" +``` + +## Task 5 — Wire-level middleware tests + +- Add `internal/server/host_check_test.go` exercising every row of + the spec's wire-level table. A helper `setupHostCheckServer(t, + HostCheckOptions)` builds a `Server` via `New(..., nil, "/", + nil, ServerOptions{HostCheck: opts})` so each table row controls + bind, allowed_hosts, and trust_reverse_proxy precisely. +- Tests use `require.NoError` / `assert.Equal` / `assert.New(t)`. +- Each row issues `srv.ServeHTTP(rr, req)` with `req.Host` / + `req.Header.Set("X-Forwarded-Host", …)` / `req.Header.Set("Forwarded", …)` as the row dictates, then asserts: + - `rr.Code` + - On the dedicated 403-body row: response body parses as JSON + `{"error": "..."}` whose value contains the substrings + `allowed_hosts` and `trust_reverse_proxy`. +- A second table-driven `TestParseForwardedHost` exercises the + unexported parser helpers directly for zero-length-header and + malformed-but-present cases that don't round-trip cleanly via + `httptest.NewRequest`. +- Verify: `nix run nixpkgs#go -- test ./internal/server/... -run "TestHostCheck|TestParseForwardedHost|TestCSRF|TestBasePath" -shuffle=on` + +```bash +git add internal/server/host_check_test.go +git commit -m "test(server): cover Host validation middleware wire behavior" +``` + +## Task 6 — Full-stack apitest coverage + +- Add `internal/server/apitest/host_check_test.go` with two cases: + 1. `Host: attacker.example:8091` on `GET /api/v1/pulls` → 403 + with JSON body shape `{"error":"..."}` whose value contains + `allowed_hosts` and `trust_reverse_proxy`. Asserted BEFORE + any handler runs. + 2. `Host: 127.0.0.1:8091` on `GET /api/v1/pulls` → 200 with the + seeded PR list (using the existing `seedPR` helper). +- The apitest round-tripper in + `internal/server/apitest/fixtures_test.go` builds the test + `Request` via `httptest.NewRequest(req.Method, req.URL.String(), body)` + so `serverReq.Host` comes from `req.URL.Host`. The test client + base URL is `http://middleman.test` (so `req.URL.Host` is + `middleman.test`). That hostname is in Task 4's cfg=nil + test-friendly default allowlist, so existing apitest cases + continue to work unchanged. +- For Task 6, build a dedicated server with an EXPLICIT + `ServerOptions.HostCheck` so the production contract is tested + rather than the test-friendly fallback: + `HostCheckOptions{Bind: {127.0.0.1, 8091}, Allowed: nil}`. + This server only accepts loopback synonyms at port 8091. Use + the existing round-tripper but build the test request URL with + `http://127.0.0.1:8091` for the success case and + `http://attacker.example:8091` for the rejection case so + `req.URL.Host` (and therefore `serverReq.Host`) carries the + per-row value through. Mechanism: ONE — `req.URL.Host`. +- Use `require` and `assert.New(t)` per the convention. +- Verify: `make test-short` + +```bash +git add internal/server/apitest/host_check_test.go internal/server/apitest/fixtures_test.go +git commit -m "test(apitest): exercise Host validation through the full stack" +``` + +## Task 7 — README documentation + +- Add two rows to the configuration table in `README.md` under + `## Configuration`: + - `allowed_hosts` | `[]` | Extra Host headers to accept beyond the + bind address. + - `trust_reverse_proxy` | `false` | Honor X-Forwarded-Host and + Forwarded RFC 7239 host= under reverse-proxy deployments. +- Add a short paragraph below the table explaining the + DNS-rebinding rationale and how to configure for a reverse-proxy + deployment. +- Stage explicit files. Lint findings from earlier tasks fold back + into those tasks' commits during execution, not here. +- Verify: `make lint && make vet && make test` + +```bash +git add README.md +git commit -m "docs(readme): document allowed_hosts and trust_reverse_proxy" +``` + +## Verification checklist (final, before declaring done) + +- `make lint` clean +- `make vet` clean +- `make test` clean (`-shuffle=on` applied by Make) +- `nix run nixpkgs#go -- test ./internal/config/... ./internal/server/... -shuffle=on` + passes +- Acceptance behavior verified by tests: + - direct 127.0.0.1:8091 succeeds (Task 5 + Task 6 case 2) + - `Host: attacker.example` to 127.0.0.1:8091 → 403 (Task 5 + Task 6 case 1) + - `Host: localhost:8091` and `127.0.0.1:8091` canonicalised + equivalent against loopback bind (Task 5) + - `allowed_hosts = ["middleman.local:8091"]` allows that host + (Task 5) + - `trust_reverse_proxy` with forwarded headers succeeds (Task 5) From 6b613e66fc5aab2936759dc48394ed1a5462b40f Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 06:43:39 -0400 Subject: [PATCH 03/15] feat(config): add ParseHostKey for shared host canonicalization Introduces a canonical HostKey type (lowercased host, port preserved or empty, IPv6 bracketed) plus a ParseHostKey parser shared between config validation (allowed_hosts entries) and the upcoming Host validation middleware. Centralising the parser ensures the "canonicalisation is identical at config-load time and request time" invariant holds without duplication. Rejects empty input, port-only values, unbracketed IPv6 literals, and ports outside 1-65535. --- internal/config/hostmatch.go | 134 ++++++++++++++++++++++++++++++ internal/config/hostmatch_test.go | 89 ++++++++++++++++++++ 2 files changed, 223 insertions(+) create mode 100644 internal/config/hostmatch.go create mode 100644 internal/config/hostmatch_test.go diff --git a/internal/config/hostmatch.go b/internal/config/hostmatch.go new file mode 100644 index 000000000..4894b030a --- /dev/null +++ b/internal/config/hostmatch.go @@ -0,0 +1,134 @@ +package config + +import ( + "fmt" + "net" + "strconv" + "strings" +) + +// HostKey is the canonical representation of a host header value +// (or a configured allowed_hosts entry). Host is lowercased; for +// IPv6 literals it keeps the surrounding brackets so the textual +// form roundtrips. Port is the digits-only port string, or "" when +// the source value had no explicit port. ParseHostKey is the only +// constructor; downstream code should treat the struct as +// effectively immutable after parsing. +type HostKey struct { + Host string + Port string +} + +// ParseHostKey canonicalises a host header value or +// configuration entry. Accepted shapes: +// +// - bare host: "mm.local", "LOCALHOST" (case-folded) +// - host with port: "mm.local:8091", "127.0.0.1:8091" +// - bracketed IPv6: "[::1]", "[::1]:8091" +// +// Rejected: empty / whitespace-only input, port-only ("·:8091"), +// unbracketed IPv6 literals ("::1", "::1:8091"), ports outside +// 1-65535. +func ParseHostKey(s string) (HostKey, error) { + s = strings.TrimSpace(s) + if s == "" { + return HostKey{}, fmt.Errorf("host: empty") + } + + // Bracketed IPv6 path: [ipv6] or [ipv6]:port. + if strings.HasPrefix(s, "[") { + closing := strings.IndexByte(s, ']') + if closing < 0 { + return HostKey{}, fmt.Errorf("host: missing closing bracket") + } + host := s[1:closing] + // A bracketed value must parse as an IP literal and the + // textual form must contain a colon (IPv6 textual form + // always does; dotted-quad IPv4 never does). This rejects + // "[127.0.0.1]" while accepting IPv4-mapped IPv6 like + // "[::ffff:7f00:1]". + if ip := net.ParseIP(host); ip == nil || !strings.ContainsRune(host, ':') { + return HostKey{}, fmt.Errorf( + "host: bracketed value %q is not an IPv6 literal", host, + ) + } + rest := s[closing+1:] + var port string + if rest != "" { + if !strings.HasPrefix(rest, ":") { + return HostKey{}, fmt.Errorf( + "host: unexpected trailing data %q after bracketed host", rest, + ) + } + p, err := parsePort(rest[1:]) + if err != nil { + return HostKey{}, err + } + port = p + } + return HostKey{Host: "[" + strings.ToLower(host) + "]", Port: port}, nil + } + + // host:port (last colon) or bare host. Splitting on the last + // colon catches unbracketed IPv6 ("::1", "::1:8091") because + // the host part after the split still contains a `:`; we + // reject that explicitly below. + if idx := strings.LastIndexByte(s, ':'); idx >= 0 { + host := s[:idx] + portStr := s[idx+1:] + if host == "" { + return HostKey{}, fmt.Errorf("host: port-only input %q", s) + } + if strings.ContainsRune(host, ':') { + return HostKey{}, fmt.Errorf( + "host: unbracketed IPv6 literal %q (use [ipv6]:port instead)", s, + ) + } + port, err := parsePort(portStr) + if err != nil { + return HostKey{}, err + } + return HostKey{Host: strings.ToLower(host), Port: port}, nil + } + + // Bare host, no port. + return HostKey{Host: strings.ToLower(s), Port: ""}, nil +} + +func parsePort(p string) (string, error) { + if p == "" { + return "", fmt.Errorf("host: empty port") + } + n, err := strconv.Atoi(p) + if err != nil { + return "", fmt.Errorf("host: invalid port %q: %w", p, err) + } + if n < 1 || n > 65535 { + return "", fmt.Errorf("host: port %d out of range", n) + } + return p, nil +} + +// String renders the canonical form. Bracketed IPv6 hosts already +// carry their brackets in Host, so the renderer just joins. +func (k HostKey) String() string { + if k.Port == "" { + return k.Host + } + return k.Host + ":" + k.Port +} + +// Equal reports whether the keys match. Hosts are already +// lowercased by ParseHostKey, so this is an exact-string compare +// on both fields. +func (k HostKey) Equal(other HostKey) bool { + return k.Host == other.Host && k.Port == other.Port +} + +// Valid reports whether the key carries both host and port. The +// server constructor uses this to distinguish a populated bind +// key from the zero value when deciding between caller override, +// cfg-derived, and the cfg=nil test-friendly default. +func (k HostKey) Valid() bool { + return k.Host != "" && k.Port != "" +} diff --git a/internal/config/hostmatch_test.go b/internal/config/hostmatch_test.go new file mode 100644 index 000000000..c7a239acc --- /dev/null +++ b/internal/config/hostmatch_test.go @@ -0,0 +1,89 @@ +package config + +import ( + "testing" + + Assert "github.com/stretchr/testify/assert" +) + +func TestParseHostKey(t *testing.T) { + cases := []struct { + name string + in string + wantErr bool + want HostKey + }{ + {name: "bare host lowercases", in: "mm.local", want: HostKey{Host: "mm.local", Port: ""}}, + {name: "bare host uppercase folds", in: "LOCALHOST", want: HostKey{Host: "localhost", Port: ""}}, + {name: "host with port", in: "mm.local:8091", want: HostKey{Host: "mm.local", Port: "8091"}}, + {name: "ipv4 with port", in: "127.0.0.1:8091", want: HostKey{Host: "127.0.0.1", Port: "8091"}}, + {name: "mixed case with port", in: "MM.Local:8091", want: HostKey{Host: "mm.local", Port: "8091"}}, + {name: "bracketed ipv6 with port", in: "[::1]:8091", want: HostKey{Host: "[::1]", Port: "8091"}}, + {name: "bracketed ipv6 no port", in: "[::1]", want: HostKey{Host: "[::1]", Port: ""}}, + {name: "bracketed ipv6 uppercase folds", in: "[::FFFF:7F00:1]:8091", want: HostKey{Host: "[::ffff:7f00:1]", Port: "8091"}}, + {name: "whitespace trimmed", in: " mm.local:8091 ", want: HostKey{Host: "mm.local", Port: "8091"}}, + + {name: "empty string rejected", in: "", wantErr: true}, + {name: "whitespace only rejected", in: " ", wantErr: true}, + {name: "port-only rejected", in: ":8091", wantErr: true}, + {name: "unbracketed ipv6 with port rejected", in: "::1:8091", wantErr: true}, + {name: "unbracketed ipv6 no port rejected", in: "::1", wantErr: true}, + {name: "port zero rejected", in: "mm.local:0", wantErr: true}, + {name: "port too high rejected", in: "mm.local:99999", wantErr: true}, + {name: "port negative rejected", in: "mm.local:-1", wantErr: true}, + {name: "non-numeric port rejected", in: "mm.local:abc", wantErr: true}, + {name: "missing closing bracket rejected", in: "[::1:8091", wantErr: true}, + {name: "bracketed non-ipv6 rejected", in: "[127.0.0.1]:8091", wantErr: true}, + {name: "trailing garbage after bracketed host", in: "[::1]junk", wantErr: true}, + {name: "empty port after colon", in: "mm.local:", wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert := Assert.New(t) + got, err := ParseHostKey(tc.in) + if tc.wantErr { + assert.Error(err) + return + } + assert.NoError(err) + assert.Equal(tc.want, got) + }) + } +} + +func TestHostKeyString(t *testing.T) { + cases := []struct { + name string + key HostKey + want string + }{ + {name: "bare host", key: HostKey{Host: "mm.local"}, want: "mm.local"}, + {name: "with port", key: HostKey{Host: "mm.local", Port: "8091"}, want: "mm.local:8091"}, + {name: "ipv6 bracketed", key: HostKey{Host: "[::1]", Port: "8091"}, want: "[::1]:8091"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + Assert.Equal(t, tc.want, tc.key.String()) + }) + } +} + +func TestHostKeyEqual(t *testing.T) { + assert := Assert.New(t) + a := HostKey{Host: "mm.local", Port: "8091"} + b := HostKey{Host: "mm.local", Port: "8091"} + c := HostKey{Host: "mm.local", Port: ""} + d := HostKey{Host: "other.local", Port: "8091"} + assert.True(a.Equal(b)) + assert.False(a.Equal(c)) + assert.False(a.Equal(d)) +} + +func TestHostKeyValid(t *testing.T) { + assert := Assert.New(t) + assert.False(HostKey{}.Valid()) + assert.False(HostKey{Host: "mm.local"}.Valid()) + assert.False(HostKey{Port: "8091"}.Valid()) + assert.True(HostKey{Host: "mm.local", Port: "8091"}.Valid()) +} From 66118768c050edff77905089607cb1850ac4c476 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 06:45:12 -0400 Subject: [PATCH 04/15] feat(config): accept allowed_hosts and trust_reverse_proxy Adds two config fields to support the upcoming Host validation middleware. allowed_hosts is parsed via the shared ParseHostKey and cached on the Config struct so the server constructor can derive the runtime allowlist without re-parsing on each request setup. trust_reverse_proxy opt-in flag governs whether X-Forwarded-Host / Forwarded RFC 7239 host= are consulted. Validate() also caches the canonical bind HostKey so the server constructor can read it directly via BindHostKey(); the field is left zero for partial test configs that bypass Validate. Validate() itself stays side-effect-light; the trust_reverse_proxy warning and the cfg=nil test-friendly default warning are emitted at server construction time (next commit). --- internal/config/config.go | 85 +++++++++++++++++++++++++++------- internal/config/config_test.go | 74 +++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 18 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 7d18fb057..ad307e0f6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -540,24 +540,44 @@ type Shell struct { } type Config struct { - SyncInterval string `toml:"sync_interval"` - GitHubTokenEnv string `toml:"github_token_env"` - DefaultPlatformHost string `toml:"default_platform_host"` - Host string `toml:"host"` - Port int `toml:"port"` - BasePath string `toml:"base_path"` - DataDir string `toml:"data_dir"` - SyncBudgetPerHour int `toml:"sync_budget_per_hour"` - SSEBufferSize int `toml:"sse_buffer_size"` - IssueWorkspaceBranchStyle string `toml:"issue_workspace_branch_style"` - Repos []Repo `toml:"repos"` - Platforms []PlatformConfig `toml:"platforms"` - Activity Activity `toml:"activity"` - Terminal Terminal `toml:"terminal"` - Agents []Agent `toml:"agents"` - Roborev Roborev `toml:"roborev"` - Tmux Tmux `toml:"tmux"` - Shell Shell `toml:"shell"` + SyncInterval string `toml:"sync_interval"` + GitHubTokenEnv string `toml:"github_token_env"` + DefaultPlatformHost string `toml:"default_platform_host"` + Host string `toml:"host"` + Port int `toml:"port"` + BasePath string `toml:"base_path"` + DataDir string `toml:"data_dir"` + SyncBudgetPerHour int `toml:"sync_budget_per_hour"` + SSEBufferSize int `toml:"sse_buffer_size"` + IssueWorkspaceBranchStyle string `toml:"issue_workspace_branch_style"` + // AllowedHosts is an exact-match allowlist of Host header values + // beyond the bind address that the Host validation middleware + // should accept. Loopback synonyms (127.0.0.1 / localhost / + // [::1]) at the bind port are auto-accepted and do not need to + // be listed. + AllowedHosts []string `toml:"allowed_hosts"` + // TrustReverseProxy enables honoring X-Forwarded-Host and + // Forwarded RFC 7239 host= for the Public Host validation step. + // The raw Host header must still pass the allowed_hosts gate + // before any forwarded header is read. + TrustReverseProxy bool `toml:"trust_reverse_proxy"` + Repos []Repo `toml:"repos"` + Platforms []PlatformConfig `toml:"platforms"` + Activity Activity `toml:"activity"` + Terminal Terminal `toml:"terminal"` + Agents []Agent `toml:"agents"` + Roborev Roborev `toml:"roborev"` + Tmux Tmux `toml:"tmux"` + Shell Shell `toml:"shell"` + + // parsedAllowedHosts is the canonicalised form of AllowedHosts, + // populated by Validate so the server constructor does not have + // to re-parse on every request setup. Defensive copy via + // ParsedAllowedHosts. + parsedAllowedHosts []HostKey + // parsedBindKey is the canonical (Host, Port) key for the bind + // address, populated by Validate. + parsedBindKey HostKey } // SSEBufferSizeOrDefault returns the configured SSE replay ring size, @@ -861,6 +881,21 @@ func (c *Config) Validate() error { return fmt.Errorf("config: invalid port %d", c.Port) } + bindKey, err := ParseHostKey(net.JoinHostPort(c.Host, strconv.Itoa(c.Port))) + if err != nil { + return fmt.Errorf("config: invalid host %q: %w", c.Host, err) + } + c.parsedBindKey = bindKey + + c.parsedAllowedHosts = c.parsedAllowedHosts[:0] + for _, entry := range c.AllowedHosts { + key, err := ParseHostKey(entry) + if err != nil { + return fmt.Errorf("config: invalid allowed_hosts entry %q: %w", entry, err) + } + c.parsedAllowedHosts = append(c.parsedAllowedHosts, key) + } + if c.SyncBudgetPerHour != 0 && c.SyncBudgetPerHour < 50 { return fmt.Errorf( "config: sync_budget_per_hour must be >= 50 or omitted, got %d", @@ -1315,6 +1350,20 @@ func (c *Config) ListenAddr() string { return fmt.Sprintf("%s:%d", c.Host, c.Port) } +// BindHostKey returns the canonical (Host, Port) key for the bind +// address, populated by Validate. The zero HostKey is returned for +// configs that were not validated (e.g. test literals that omit +// Host/Port); callers should use HostKey.Valid() to gate behavior. +func (c *Config) BindHostKey() HostKey { + return c.parsedBindKey +} + +// ParsedAllowedHosts returns the canonicalised allowlist, populated +// by Validate. The returned slice is a defensive copy. +func (c *Config) ParsedAllowedHosts() []HostKey { + return append([]HostKey(nil), c.parsedAllowedHosts...) +} + func (c *Config) DBPath() string { return filepath.Join(c.DataDir, "middleman.db") } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5fc4e6136..484d079f5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2696,3 +2696,77 @@ func TestGitHubTokenInvokesGHWithGithubComHostname(t *testing.T) { require.Len(t, argv, 1) Assert.Equal(t, "auth token --hostname github.com", argv[0]) } + +func TestLoadAllowedHostsDefault(t *testing.T) { + assert := Assert.New(t) + cfg, err := Load(writeConfig(t, `host = "127.0.0.1" +port = 8091 +`)) + require.NoError(t, err) + assert.Empty(cfg.AllowedHosts) + assert.Empty(cfg.ParsedAllowedHosts()) + assert.False(cfg.TrustReverseProxy) + assert.Equal( + HostKey{Host: "127.0.0.1", Port: "8091"}, + cfg.BindHostKey(), + ) +} + +func TestLoadAllowedHostsParsesAndCanonicalises(t *testing.T) { + assert := Assert.New(t) + cfg, err := Load(writeConfig(t, `host = "127.0.0.1" +port = 8091 +allowed_hosts = ["mm.local:8091", "MM.Example.Com", "[::1]:8443"] +trust_reverse_proxy = true +`)) + require.NoError(t, err) + assert.Equal( + []HostKey{ + {Host: "mm.local", Port: "8091"}, + {Host: "mm.example.com", Port: ""}, + {Host: "[::1]", Port: "8443"}, + }, + cfg.ParsedAllowedHosts(), + ) + assert.True(cfg.TrustReverseProxy) +} + +func TestLoadAllowedHostsRejectsBadEntry(t *testing.T) { + cases := []struct { + name string + entry string + }{ + {name: "unbracketed ipv6", entry: "::1:8091"}, + {name: "port only", entry: ":8091"}, + {name: "empty", entry: ""}, + {name: "port out of range", entry: "mm.local:99999"}, + {name: "bracketed ipv4", entry: "[127.0.0.1]:8091"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := writeConfig(t, fmt.Sprintf(`host = "127.0.0.1" +port = 8091 +allowed_hosts = [%q] +`, tc.entry)) + _, err := Load(path) + require.Error(t, err) + Assert.Contains(t, err.Error(), "allowed_hosts") + }) + } +} + +func TestParsedAllowedHostsDefensiveCopy(t *testing.T) { + assert := Assert.New(t) + cfg, err := Load(writeConfig(t, `host = "127.0.0.1" +port = 8091 +allowed_hosts = ["mm.local:8091"] +`)) + require.NoError(t, err) + got := cfg.ParsedAllowedHosts() + got[0] = HostKey{Host: "tampered", Port: "1"} + again := cfg.ParsedAllowedHosts() + assert.Equal( + []HostKey{{Host: "mm.local", Port: "8091"}}, + again, + ) +} From 70075763213ebac04620d74ff0c40ff2712ad51f Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 06:46:03 -0400 Subject: [PATCH 05/15] feat(server): add Host validation middleware (no wiring yet) Introduces checkHost plus the supporting parsers for X-Forwarded-Host and Forwarded RFC 7239 host=, gated on a HostCheckOptions value. Three validation steps mirror the design spec: parse Backend Host, match Backend against bind + loopback synonyms + allowed_hosts, and (when trust_reverse_proxy is true) validate the Public Host from forwarded headers against the same accept-set with conflict detection across X-Forwarded-Host and Forwarded. Wiring into Server.ServeHTTP happens in the next commit so this diff can be reviewed in isolation. --- internal/server/host_check.go | 224 ++++++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) create mode 100644 internal/server/host_check.go diff --git a/internal/server/host_check.go b/internal/server/host_check.go new file mode 100644 index 000000000..092a9c9b3 --- /dev/null +++ b/internal/server/host_check.go @@ -0,0 +1,224 @@ +package server + +import ( + "log/slog" + "net/http" + "strings" + + "github.com/wesm/middleman/internal/config" +) + +// hostValidationError is the operator-facing 403 body for any host +// validation failure. The text deliberately does NOT echo the +// rejected hostname back into the response (avoids reflecting +// attacker-controlled input and avoids the slim risk of log +// injection via crafted Host values). Rejected hostnames go to +// slog.Warn on the server side for operator diagnosis. +const hostValidationError = "host validation failed: the requested hostname is not allowed. " + + "Add expected Backend and Public hostnames to allowed_hosts in middleman's config.toml. " + + "If a reverse proxy sets forwarded-host headers, also enable trust_reverse_proxy." + +// HostCheckOptions configures the Host validation middleware. +// +// Bind is the canonical (Host, Port) the listener serves on. The +// middleware always accepts Bind itself, and (when Bind.Host is a +// loopback synonym) the other two loopback synonyms at the same +// port. Allowed extends the accept-set with exact-match entries +// from config.allowed_hosts. TrustReverseProxy enables the Public +// Host (X-Forwarded-Host / Forwarded) validation step. +type HostCheckOptions struct { + Bind config.HostKey + Allowed []config.HostKey + TrustReverseProxy bool +} + +// Valid reports whether the options are populated enough to run +// the middleware (Bind has both host and port). The server +// constructor uses Valid to distinguish a populated override from +// a zero value when applying its precedence rule. +func (o HostCheckOptions) Valid() bool { + return o.Bind.Valid() +} + +// checkHost runs the Host validation steps from the design spec. +// Returns true when the request may proceed; returns false (and +// writes the 403) when it must be rejected. Panics when opts is +// not Valid — the server constructor enforces population. +func checkHost(w http.ResponseWriter, r *http.Request, opts HostCheckOptions) bool { + if !opts.Valid() { + panic("server: checkHost called with invalid options (programming error)") + } + + // Step 1+2: parse and validate the Backend (raw) Host. + rawHost := r.Host + backendKey, err := config.ParseHostKey(rawHost) + if err != nil { + rejectHost(w, r, "backend_host_malformed", rawHost, "") + return false + } + accepted := acceptedSet(opts.Bind, opts.Allowed) + if !matchAny(backendKey, accepted) { + rejectHost(w, r, "backend_host_not_allowed", rawHost, "") + return false + } + + // Step 3 only when trust_reverse_proxy is enabled. + if !opts.TrustReverseProxy { + return true + } + + xfh := r.Header.Values("X-Forwarded-Host") + fwd := r.Header.Values("Forwarded") + xfhPresent := len(xfh) > 0 + fwdPresent := len(fwd) > 0 + if !xfhPresent && !fwdPresent { + rejectHost( + w, r, + "trust_reverse_proxy_missing_forwarded_host", + rawHost, "", + ) + return false + } + + var xfhKey, fwdKey config.HostKey + if xfhPresent { + k, err := parseXForwardedHost(strings.Join(xfh, ",")) + if err != nil { + rejectHost(w, r, "x_forwarded_host_malformed", rawHost, "") + return false + } + xfhKey = k + } + if fwdPresent { + k, err := parseForwardedHost(strings.Join(fwd, ",")) + if err != nil { + rejectHost(w, r, "forwarded_malformed", rawHost, "") + return false + } + fwdKey = k + } + if xfhPresent && fwdPresent && !xfhKey.Equal(fwdKey) { + rejectHost(w, r, "forwarded_headers_disagree", rawHost, xfhKey.String()) + return false + } + publicKey := xfhKey + if !xfhPresent { + publicKey = fwdKey + } + if !matchAny(publicKey, accepted) { + rejectHost(w, r, "public_host_not_allowed", rawHost, publicKey.String()) + return false + } + return true +} + +// acceptedSet returns the union of the bind, the loopback +// synonyms-at-the-bind-port (when the bind is itself loopback), +// and the configured allowlist. +func acceptedSet(bind config.HostKey, allowed []config.HostKey) []config.HostKey { + out := make([]config.HostKey, 0, 3+len(allowed)) + out = append(out, bind) + if isLoopbackHost(bind.Host) { + for _, syn := range []string{"127.0.0.1", "localhost", "[::1]"} { + if syn == bind.Host { + continue + } + out = append(out, config.HostKey{Host: syn, Port: bind.Port}) + } + } + out = append(out, allowed...) + return out +} + +func isLoopbackHost(h string) bool { + switch h { + case "127.0.0.1", "localhost", "[::1]": + return true + } + return false +} + +func matchAny(k config.HostKey, set []config.HostKey) bool { + for _, e := range set { + if k.Equal(e) { + return true + } + } + return false +} + +func rejectHost(w http.ResponseWriter, r *http.Request, reason, host, forwarded string) { + slog.Warn( + "host validation failed", + "reason", reason, + "host", host, + "forwarded_host", forwarded, + "remote_addr", r.RemoteAddr, + "method", r.Method, + "path", r.URL.Path, + ) + writeError(w, http.StatusForbidden, hostValidationError) +} + +// parseXForwardedHost extracts and canonicalises the first +// comma-separated entry of the X-Forwarded-Host header value. +// Returns an error for empty or unparseable values. +func parseXForwardedHost(value string) (config.HostKey, error) { + first, _, _ := strings.Cut(value, ",") + first = strings.TrimSpace(first) + if first == "" { + return config.HostKey{}, errEmptyForwardedHost + } + return config.ParseHostKey(first) +} + +// parseForwardedHost extracts and canonicalises the host= parameter +// of the FIRST comma-separated entry of the RFC 7239 Forwarded +// header. The first entry must contain a host= parameter; if it +// lacks one or the parser cannot canonicalise the value, an error +// is returned. We deliberately do NOT scan later comma-separated +// entries because that would let a proxy emit +// "for=10.0.0.1, host=attacker.example" and have us pick up the +// attacker value. +func parseForwardedHost(value string) (config.HostKey, error) { + first, _, _ := strings.Cut(value, ",") + first = strings.TrimSpace(first) + if first == "" { + return config.HostKey{}, errEmptyForwardedHost + } + + // Walk the semicolon-separated key=value pairs of the first + // entry. Parameter names are case-insensitive per RFC 7239. + for part := range strings.SplitSeq(first, ";") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + key, val, ok := strings.Cut(part, "=") + if !ok { + continue + } + key = strings.TrimSpace(key) + if !strings.EqualFold(key, "host") { + continue + } + val = strings.TrimSpace(val) + if len(val) >= 2 && val[0] == '"' && val[len(val)-1] == '"' { + val = val[1 : len(val)-1] + } + if val == "" { + return config.HostKey{}, errEmptyForwardedHost + } + return config.ParseHostKey(val) + } + return config.HostKey{}, errMissingForwardedHostParam +} + +type hostCheckError string + +func (e hostCheckError) Error() string { return string(e) } + +const ( + errEmptyForwardedHost = hostCheckError("empty forwarded-host value") + errMissingForwardedHostParam = hostCheckError("Forwarded header lacks host= in first entry") +) From 2d011d83c58ad2fd3f0fb245e9997a30e0e0f129 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 12:47:04 -0400 Subject: [PATCH 06/15] feat(server): reject mismatched Host header to block DNS rebinding Wires the Host validation middleware into ServeHTTP before the existing CSRF gate. A DNS-rebound page that resolves attacker.example to 127.0.0.1 was previously seen as same-origin by the browser, slipping past Sec-Fetch-Site; this commit makes the server reject any request whose raw Host (and, under trust_reverse_proxy, forwarded host) does not match the bind, loopback synonyms at the bind port, or an allowed_hosts entry. HostCheckOptions gains an AllowLoopbackAnyPort flag that the test-friendly fallback (cfg=nil or partial cfg) and any go-test process turn on automatically. httptest.NewServer binds an ephemeral port that callers cannot pre-declare in allowed_hosts, so test binaries accept literal loopback IPs at any port; testing.Testing() is false in production so the strict spec semantics are preserved on the deployed binary. The doJSON helper sets req.Host so partial-cfg tests with a validated 127.0.0.1:8091 bind also pass cleanly. --- internal/config/hostmatch_test.go | 3 +- internal/server/host_check.go | 50 +++++++++--- internal/server/host_check_default_test.go | 66 ++++++++++++++++ internal/server/server.go | 92 ++++++++++++++++++++++ internal/server/settings_test.go | 9 +++ 5 files changed, 207 insertions(+), 13 deletions(-) create mode 100644 internal/server/host_check_default_test.go diff --git a/internal/config/hostmatch_test.go b/internal/config/hostmatch_test.go index c7a239acc..f68aa6771 100644 --- a/internal/config/hostmatch_test.go +++ b/internal/config/hostmatch_test.go @@ -4,6 +4,7 @@ import ( "testing" Assert "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseHostKey(t *testing.T) { @@ -46,7 +47,7 @@ func TestParseHostKey(t *testing.T) { assert.Error(err) return } - assert.NoError(err) + require.NoError(t, err) assert.Equal(tc.want, got) }) } diff --git a/internal/server/host_check.go b/internal/server/host_check.go index 092a9c9b3..85b1e73c2 100644 --- a/internal/server/host_check.go +++ b/internal/server/host_check.go @@ -3,9 +3,10 @@ package server import ( "log/slog" "net/http" + "slices" "strings" - "github.com/wesm/middleman/internal/config" + "go.kenn.io/middleman/internal/config" ) // hostValidationError is the operator-facing 403 body for any host @@ -26,10 +27,18 @@ const hostValidationError = "host validation failed: the requested hostname is n // port. Allowed extends the accept-set with exact-match entries // from config.allowed_hosts. TrustReverseProxy enables the Public // Host (X-Forwarded-Host / Forwarded) validation step. +// +// AllowLoopbackAnyPort relaxes the port match for loopback IPs +// (127.0.0.1, [::1]) so requests with any port pass Step 2. It +// exists solely so test helpers built on httptest.NewServer — which +// binds an ephemeral port that callers cannot know up front — keep +// working without per-test bookkeeping. Production callers must +// leave this false; the cfg-derived path always does. type HostCheckOptions struct { - Bind config.HostKey - Allowed []config.HostKey - TrustReverseProxy bool + Bind config.HostKey + Allowed []config.HostKey + TrustReverseProxy bool + AllowLoopbackAnyPort bool } // Valid reports whether the options are populated enough to run @@ -57,7 +66,7 @@ func checkHost(w http.ResponseWriter, r *http.Request, opts HostCheckOptions) bo return false } accepted := acceptedSet(opts.Bind, opts.Allowed) - if !matchAny(backendKey, accepted) { + if !matchHost(backendKey, accepted, opts.AllowLoopbackAnyPort) { rejectHost(w, r, "backend_host_not_allowed", rawHost, "") return false } @@ -105,13 +114,35 @@ func checkHost(w http.ResponseWriter, r *http.Request, opts HostCheckOptions) bo if !xfhPresent { publicKey = fwdKey } - if !matchAny(publicKey, accepted) { + if !matchHost(publicKey, accepted, opts.AllowLoopbackAnyPort) { rejectHost(w, r, "public_host_not_allowed", rawHost, publicKey.String()) return false } return true } +// matchHost reports whether k matches any allowlist entry. When +// allowLoopbackAnyPort is true and k is a literal loopback IP +// (127.0.0.1 or [::1]), the port is ignored — the request still +// has to come from the loopback listener, which the bind already +// guarantees, so accepting any source port matches the test +// fixtures (httptest.NewServer) without weakening production. +func matchHost(k config.HostKey, set []config.HostKey, allowLoopbackAnyPort bool) bool { + if allowLoopbackAnyPort && isLiteralLoopbackIP(k.Host) { + return true + } + return matchAny(k, set) +} + +// isLiteralLoopbackIP returns true for hosts that cannot be +// repointed by DNS (literal IPv4 / IPv6 loopback addresses). +// "localhost" deliberately does NOT qualify: an attacker +// /etc/hosts entry or a DNS resolver override could repoint it, +// however unlikely. +func isLiteralLoopbackIP(h string) bool { + return h == "127.0.0.1" || h == "[::1]" +} + // acceptedSet returns the union of the bind, the loopback // synonyms-at-the-bind-port (when the bind is itself loopback), // and the configured allowlist. @@ -139,12 +170,7 @@ func isLoopbackHost(h string) bool { } func matchAny(k config.HostKey, set []config.HostKey) bool { - for _, e := range set { - if k.Equal(e) { - return true - } - } - return false + return slices.ContainsFunc(set, k.Equal) } func rejectHost(w http.ResponseWriter, r *http.Request, reason, host, forwarded string) { diff --git a/internal/server/host_check_default_test.go b/internal/server/host_check_default_test.go new file mode 100644 index 000000000..d7fa6a66a --- /dev/null +++ b/internal/server/host_check_default_test.go @@ -0,0 +1,66 @@ +package server + +import ( + "io/fs" + "net/http" + "net/http/httptest" + "testing" + "testing/fstest" + "time" + + Assert "github.com/stretchr/testify/assert" + ghclient "go.kenn.io/middleman/internal/github" + "go.kenn.io/middleman/internal/testutil/dbtest" +) + +// TestNewCfgNilTestFriendlyDefault pins the test-friendly default +// installed by resolveHostCheckOptions for the cfg=nil server.New +// path. Future contributors must not widen this default +// accidentally (e.g., by adding 0.0.0.0 or attacker-style hosts). +// The default accepts loopback IPs at any port (httptest.NewServer +// uses ephemeral ports) plus the two named test hostnames; nothing +// else. +func TestNewCfgNilTestFriendlyDefault(t *testing.T) { + srv := newServerForDefaultTest(t) + + cases := []struct { + name string + host string + status int + }{ + {name: "127.0.0.1:8091 accepted", host: "127.0.0.1:8091", status: http.StatusOK}, + {name: "127.0.0.1 ephemeral port accepted (httptest.NewServer)", host: "127.0.0.1:44321", status: http.StatusOK}, + {name: "[::1] ephemeral port accepted", host: "[::1]:44321", status: http.StatusOK}, + {name: "example.com accepted (httptest default)", host: "example.com", status: http.StatusOK}, + {name: "middleman.test accepted (apitest default)", host: "middleman.test", status: http.StatusOK}, + {name: "attacker.example rejected", host: "attacker.example", status: http.StatusForbidden}, + {name: "localhost ephemeral port rejected (no any-port for non-literal)", host: "localhost:44321", status: http.StatusForbidden}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Host = tc.host + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + Assert.Equal(t, tc.status, rr.Code, rr.Body.String()) + }) + } +} + +func newServerForDefaultTest(t *testing.T) *Server { + t.Helper() + database := dbtest.Open(t) + syncer := ghclient.NewSyncer(nil, database, nil, nil, time.Minute, nil, nil) + t.Cleanup(syncer.Stop) + // cfg=nil, ServerOptions zero — exercise the test-friendly + // default branch of resolveHostCheckOptions. + return New(database, syncer, emptyFrontend(), "/", nil, ServerOptions{}) +} + +func emptyFrontend() fs.FS { + return fstest.MapFS{ + "index.html": &fstest.MapFile{ + Data: []byte("ok"), + }, + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 80fcc790d..ee4556db1 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -16,6 +16,7 @@ import ( "strconv" "strings" "sync" + "testing" "time" "github.com/danielgtaylor/huma/v2" @@ -76,6 +77,12 @@ type ServerOptions struct { PtyOwnerCommand []string PtyOwnerInProcess bool Telemetry telemetry.Client + // HostCheck overrides the Host validation middleware options. + // When Valid(), the override wins over any cfg-derived options. + // Used by wire-level tests that want to control the bind / + // allowed_hosts / trust_reverse_proxy independently of a full + // config.Config. + HostCheck HostCheckOptions } type shutdownDeadline struct { @@ -156,6 +163,7 @@ type Server struct { configWatcher *configwatch.Watcher basePath string options ServerOptions + hostOpts HostCheckOptions version string now func() time.Time handler http.Handler @@ -483,6 +491,78 @@ func NewWithConfig( ) } +// hostCheckTestFallbackBindHost / Port define the bind used when +// server.New is called with cfg=nil AND no explicit +// ServerOptions.HostCheck. These match the defaults that come out +// of config.Load, so existing same-package tests work without +// per-test churn. +const ( + hostCheckTestFallbackBindHost = "127.0.0.1" + hostCheckTestFallbackBindPort = "8091" +) + +// testFallbackAllowedHosts is the allowlist applied alongside the +// fallback bind. httptest.NewRequest defaults the Host to +// "example.com" and the apitest helpers use "middleman.test"; both +// must be accepted so the dozens of test helpers that pass +// cfg=nil work unchanged. +func testFallbackAllowedHosts() []config.HostKey { + return []config.HostKey{ + {Host: "example.com", Port: ""}, + {Host: "middleman.test", Port: ""}, + } +} + +// resolveHostCheckOptions applies the precedence rule: +// caller override > validated cfg > test-friendly fallback. The +// fallback fires when cfg is nil OR cfg lacks a validated bind key +// (partial config literal that bypassed Validate). In production +// cfg is always loaded via config.Load, which populates the bind +// key; the fallback exists solely for the many test helpers that +// construct servers from partial cfgs or pass cfg=nil entirely. +// +// Under `go test`, AllowLoopbackAnyPort is forced true on the +// resulting options regardless of which branch supplied them. +// httptest.NewServer binds an ephemeral port that callers cannot +// pre-declare in allowed_hosts, so test binaries need any-port +// acceptance for literal loopback IPs. Production code never has +// testing.Testing() return true, so the strict spec behavior is +// preserved on the deployed binary. +func resolveHostCheckOptions(cfg *config.Config, override HostCheckOptions) HostCheckOptions { + opts, fromOverride := pickHostCheckOptions(cfg, override) + if testing.Testing() && !fromOverride { + opts.AllowLoopbackAnyPort = true + } + return opts +} + +func pickHostCheckOptions(cfg *config.Config, override HostCheckOptions) (HostCheckOptions, bool) { + if override.Valid() { + return override, true + } + if cfg != nil { + if k := cfg.BindHostKey(); k.Valid() { + return HostCheckOptions{ + Bind: k, + Allowed: cfg.ParsedAllowedHosts(), + TrustReverseProxy: cfg.TrustReverseProxy, + }, false + } + } + slog.Warn( + "server.New used without a validated cfg or explicit ServerOptions.HostCheck; using httptest-compatible Host defaults. Production callers must pass a validated config or explicit HostCheck options.", + ) + return HostCheckOptions{ + Bind: config.HostKey{ + Host: hostCheckTestFallbackBindHost, + Port: hostCheckTestFallbackBindPort, + }, + Allowed: testFallbackAllowedHosts(), + TrustReverseProxy: false, + AllowLoopbackAnyPort: true, + }, false +} + func newServer( database *db.DB, syncer *ghclient.Syncer, @@ -497,6 +577,8 @@ func newServer( bgBaseCtx, bgCancel := context.WithCancel(context.Background()) bgDeadline := &shutdownDeadline{} + hostOpts := resolveHostCheckOptions(cfg, options.HostCheck) + s := &Server{ db: database, basePath: basePath, @@ -507,6 +589,7 @@ func newServer( cfgPath: cfgPath, bootCfgSnapshot: snapshotStartupConfig(cfg), options: options, + hostOpts: hostOpts, now: time.Now, hub: NewEventHubWithCapacity(cfg.SSEBufferSizeOrDefault()), tmuxActivity: newTmuxActivityTracker(nil), @@ -519,6 +602,12 @@ func newServer( bgDeadline: bgDeadline, } + if hostOpts.TrustReverseProxy && len(hostOpts.Allowed) == 0 { + slog.Warn( + "trust_reverse_proxy is enabled but allowed_hosts is empty; only loopback Hosts will be accepted", + ) + } + // (*Config).TmuxCommand handles a nil receiver and returns the // default ["tmux"]. Compute once so the workspace, runtime, and // terminal handler all share the same value and the nil-safety @@ -844,6 +933,9 @@ func scriptSafe(s string) string { // ServeHTTP implements http.Handler so Server can be used directly. func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if !checkHost(w, r, s.hostOpts) { + return + } start := time.Now() slog.Debug( "http request started", diff --git a/internal/server/settings_test.go b/internal/server/settings_test.go index 3d68316d6..d78f5d902 100644 --- a/internal/server/settings_test.go +++ b/internal/server/settings_test.go @@ -295,6 +295,13 @@ func doJSON( require.NoError(t, json.NewEncoder(&buf).Encode(body)) } req := httptest.NewRequest(method, path, &buf) + // httptest.NewRequest defaults the Host to "example.com" via the + // "/path" URL; tests built on doJSON use either the cfg=nil + // test-friendly default (loopback IP at any port is allowed) + // or a validated cfg whose bind is 127.0.0.1:8091. Sending the + // bind value here keeps both paths happy through the Host + // validation middleware without per-test churn. + req.Host = "127.0.0.1:8091" if method != http.MethodGet { req.Header.Set("Content-Type", "application/json") } @@ -1283,6 +1290,7 @@ name = "*" http.MethodPost, "/api/v1/repo/gh/roborev-dev/*/refresh", nil, ) + req.Host = "127.0.0.1:8091" req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() srv.ServeHTTP(rr, req) @@ -2110,6 +2118,7 @@ name = "widget" go func() { // Inline request avoids testify assertions inside this goroutine. req := httptest.NewRequest(http.MethodPost, "/api/v1/repos/bulk", bytes.NewReader(bulkBody.Bytes())) + req.Host = "127.0.0.1:8091" req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() srv.ServeHTTP(rr, req) From 67ce88d2a62bc2843af6d026663d977290bccc70 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 12:47:14 -0400 Subject: [PATCH 07/15] test(server): cover Host validation middleware wire behavior Adds table-driven wire-level tests that exercise every row of the spec's decision matrix: bind / loopback synonyms / allowed_hosts (Step 2), forwarded headers under trust_reverse_proxy (Step 3), and the 403 body shape. Each row constructs a Server with explicit HostCheckOptions so the test-friendly relaxation does not mask production behavior. Direct tests on the unexported parseForwardedHost and parseXForwardedHost helpers cover the malformed-but-present cases that the round-trip path cannot otherwise hand to the middleware. --- internal/server/host_check_test.go | 352 +++++++++++++++++++++++++++++ 1 file changed, 352 insertions(+) create mode 100644 internal/server/host_check_test.go diff --git a/internal/server/host_check_test.go b/internal/server/host_check_test.go new file mode 100644 index 000000000..920d1e41b --- /dev/null +++ b/internal/server/host_check_test.go @@ -0,0 +1,352 @@ +package server + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + Assert "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/wesm/middleman/internal/config" + ghclient "github.com/wesm/middleman/internal/github" + "github.com/wesm/middleman/internal/testutil/dbtest" +) + +// setupHostCheckServer builds a Server with the given +// HostCheckOptions so each row in the wire-level table controls +// bind, allowed_hosts, and trust_reverse_proxy precisely. The +// override is passed via ServerOptions, which takes precedence in +// resolveHostCheckOptions over both the cfg=nil fallback and the +// test-friendly AllowLoopbackAnyPort relaxation. +func setupHostCheckServer(t *testing.T, opts HostCheckOptions) *Server { + t.Helper() + database := dbtest.Open(t) + syncer := ghclient.NewSyncer(nil, database, nil, nil, time.Minute, nil, nil) + t.Cleanup(syncer.Stop) + return New(database, syncer, emptyFrontend(), "/", nil, ServerOptions{ + HostCheck: opts, + }) +} + +func bindLoopback8091() config.HostKey { + return config.HostKey{Host: "127.0.0.1", Port: "8091"} +} + +// TestHostCheckBackendHost exercises Step 1+2 of the spec: parse +// the request Host and validate against bind, loopback synonyms at +// the bind port, and any configured allowlist entries. +// +// Bind for every case is 127.0.0.1:8091. +func TestHostCheckBackendHost(t *testing.T) { + cases := []struct { + name string + allowed []config.HostKey + host string + status int + }{ + // Loopback synonyms at the bind port — always accepted. + {name: "direct loopback IP", allowed: nil, host: "127.0.0.1:8091", status: http.StatusOK}, + {name: "direct localhost", allowed: nil, host: "localhost:8091", status: http.StatusOK}, + {name: "direct IPv6 loopback", allowed: nil, host: "[::1]:8091", status: http.StatusOK}, + {name: "uppercase host accepted", allowed: nil, host: "LOCALHOST:8091", status: http.StatusOK}, + + // Bind-derived rejections. + {name: "wrong port", allowed: nil, host: "127.0.0.1:9999", status: http.StatusForbidden}, + {name: "attacker host (DNS rebinding)", allowed: nil, host: "attacker.example:8091", status: http.StatusForbidden}, + {name: "empty Host", allowed: nil, host: "", status: http.StatusForbidden}, + {name: "malformed Host", allowed: nil, host: "][", status: http.StatusForbidden}, + {name: "port-only Host", allowed: nil, host: ":8091", status: http.StatusForbidden}, + + // allowed_hosts entries. + {name: "allowed_hosts hit, exact port", + allowed: []config.HostKey{{Host: "mm.local", Port: "8091"}}, + host: "mm.local:8091", status: http.StatusOK}, + {name: "allowed_hosts miss, wrong port", + allowed: []config.HostKey{{Host: "mm.local", Port: "8091"}}, + host: "mm.local:9999", status: http.StatusForbidden}, + {name: "allowed_hosts bare entry hits bare Host", + allowed: []config.HostKey{{Host: "mm.local", Port: ""}}, + host: "mm.local", status: http.StatusOK}, + {name: "allowed_hosts bare entry rejects ported Host", + allowed: []config.HostKey{{Host: "mm.local", Port: ""}}, + host: "mm.local:8091", status: http.StatusForbidden}, + {name: "IPv6 allowed_hosts hit", + allowed: []config.HostKey{{Host: "[::1]", Port: "8443"}}, + host: "[::1]:8443", status: http.StatusOK}, + {name: "allowed_hosts attacker miss", + allowed: []config.HostKey{{Host: "mm.local", Port: "8091"}}, + host: "attacker.example:8091", status: http.StatusForbidden}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := setupHostCheckServer(t, HostCheckOptions{ + Bind: bindLoopback8091(), + Allowed: tc.allowed, + }) + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Host = tc.host + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + Assert.Equal(t, tc.status, rr.Code, rr.Body.String()) + }) + } +} + +// TestHostCheckForwardedHost exercises Step 3: when +// trust_reverse_proxy is true, validate the Public Host derived +// from X-Forwarded-Host / Forwarded against the same accept-set. +// Always also exercises that DNS-rebinding rejections from Step 2 +// run before any forwarded header is read. +func TestHostCheckForwardedHost(t *testing.T) { + cases := []struct { + name string + allowed []config.HostKey + trustReverseProxy bool + host string + xfh string + forwarded string + status int + }{ + {name: "trust_reverse_proxy off, X-Forwarded-Host ignored", + allowed: nil, trustReverseProxy: false, + host: "attacker.example:8091", xfh: "127.0.0.1:8091", + status: http.StatusForbidden}, + {name: "trust on, raw Host loopback, XFH in allowlist", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", xfh: "mm.example.com", + status: http.StatusOK}, + {name: "trust on, raw Host loopback, Forwarded in allowlist", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + forwarded: "for=10.0.0.1;host=mm.example.com", + status: http.StatusOK}, + {name: "trust on, Forwarded quoted host", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + forwarded: `host="mm.example.com"`, + status: http.StatusOK}, + {name: "trust on, multi-value XFH uses first entry", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com, other.example.com", + status: http.StatusOK}, + {name: "trust on, multi-value XFH first-not-allowed", + allowed: []config.HostKey{{Host: "other.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com, other.example.com", + status: http.StatusForbidden}, + {name: "trust on, both headers agree", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com", + forwarded: "host=mm.example.com", + status: http.StatusOK}, + {name: "trust on, headers disagree", + allowed: []config.HostKey{ + {Host: "mm.example.com", Port: ""}, + {Host: "other.example.com", Port: ""}, + }, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com", + forwarded: "host=other.example.com", + status: http.StatusForbidden}, + {name: "trust on, neither forwarded header", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + status: http.StatusForbidden}, + {name: "trust on, forwarded host NOT in allowlist", + allowed: nil, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "attacker.example", + status: http.StatusForbidden}, + {name: "trust on, raw Host fails (DNS rebinding even with proxy on)", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "attacker.example:8091", + xfh: "mm.example.com", + status: http.StatusForbidden}, + {name: "trust on, malformed Forwarded", + allowed: nil, + trustReverseProxy: true, + host: "127.0.0.1:8091", + forwarded: "wat", + status: http.StatusForbidden}, + {name: "trust on, Forwarded first entry lacks host=", + allowed: nil, + trustReverseProxy: true, + host: "127.0.0.1:8091", + forwarded: "for=10.0.0.1, host=mm.example.com", + status: http.StatusForbidden}, + {name: "trust on, present-but-malformed Forwarded with valid XFH", + allowed: []config.HostKey{{Host: "mm.example.com", Port: ""}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com", + forwarded: "wat", + status: http.StatusForbidden}, + {name: "trust on, forwarded port mismatch", + allowed: []config.HostKey{{Host: "mm.example.com", Port: "8443"}}, + trustReverseProxy: true, + host: "127.0.0.1:8091", + xfh: "mm.example.com:9999", + status: http.StatusForbidden}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := setupHostCheckServer(t, HostCheckOptions{ + Bind: bindLoopback8091(), + Allowed: tc.allowed, + TrustReverseProxy: tc.trustReverseProxy, + }) + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Host = tc.host + if tc.xfh != "" { + req.Header.Set("X-Forwarded-Host", tc.xfh) + } + if tc.forwarded != "" { + req.Header.Set("Forwarded", tc.forwarded) + } + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + Assert.Equal(t, tc.status, rr.Code, rr.Body.String()) + }) + } +} + +// TestHostCheck403BodyShape pins the 403 body shape used by the +// middleware. The body must be valid JSON of the form +// {"error":"..."} and the value must name both config knobs so an +// operator can debug a rejected request from curl output alone. +func TestHostCheck403BodyShape(t *testing.T) { + srv := setupHostCheckServer(t, HostCheckOptions{ + Bind: bindLoopback8091(), + }) + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Host = "attacker.example:8091" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + + require := require.New(t) + assert := Assert.New(t) + require.Equal(http.StatusForbidden, rr.Code) + body, err := io.ReadAll(rr.Body) + require.NoError(err) + var payload struct { + Error string `json:"error"` + } + require.NoError(json.Unmarshal(body, &payload)) + assert.Contains(payload.Error, "allowed_hosts") + assert.Contains(payload.Error, "trust_reverse_proxy") +} + +// TestParseForwardedHost exercises the unexported helpers +// directly, covering the zero-length-header and +// malformed-but-present cases that httptest.NewRequest cannot +// otherwise hand to the middleware (Go's http.Header normalises +// empty values away on set). +func TestParseForwardedHost(t *testing.T) { + t.Run("Forwarded", func(t *testing.T) { + cases := []struct { + name string + input string + wantOK bool + want config.HostKey + }{ + {name: "host param", + input: "host=mm.example.com", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "for and host", + input: "for=10.0.0.1;host=mm.example.com", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "quoted host", + input: `host="mm.example.com"`, + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "case-insensitive param", + input: "Host=mm.example.com", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "first comma entry only", + input: "host=mm.example.com, host=attacker.example", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "first entry lacks host=", + input: "for=10.0.0.1, host=mm.example.com", + wantOK: false}, + {name: "empty", + input: "", + wantOK: false}, + {name: "garbage", + input: "wat", + wantOK: false}, + {name: "empty quoted host", + input: `host=""`, + wantOK: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseForwardedHost(tc.input) + if tc.wantOK { + require.NoError(t, err) + Assert.Equal(t, tc.want, got) + } else { + Assert.Error(t, err) + } + }) + } + }) + + t.Run("X-Forwarded-Host", func(t *testing.T) { + cases := []struct { + name string + input string + wantOK bool + want config.HostKey + }{ + {name: "single host", + input: "mm.example.com", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "host with port", + input: "mm.example.com:8443", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: "8443"}}, + {name: "multi-value first wins", + input: "mm.example.com, attacker.example", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "leading whitespace trimmed", + input: " mm.example.com", + wantOK: true, + want: config.HostKey{Host: "mm.example.com", Port: ""}}, + {name: "empty", + input: "", + wantOK: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseXForwardedHost(tc.input) + if tc.wantOK { + require.NoError(t, err) + Assert.Equal(t, tc.want, got) + } else { + Assert.Error(t, err) + } + }) + } + }) +} From 3dda0b5b49b6f6fa62dcf415f9da1c9052354d75 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Tue, 19 May 2026 12:47:20 -0400 Subject: [PATCH 08/15] test(apitest): exercise Host validation through the full stack Adds a full-stack apitest case that exercises the production contract: an explicit HostCheckOptions override skips the test-friendly AllowLoopbackAnyPort relaxation, so the middleware behaves exactly as it would on a deployed binary. A request with Host attacker.example:8091 returns the documented JSON 403 body before any handler runs; a request whose Host matches the configured bind reaches the handler and returns 200 with the seeded PR list. --- internal/server/apitest/host_check_test.go | 121 +++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 internal/server/apitest/host_check_test.go diff --git a/internal/server/apitest/host_check_test.go b/internal/server/apitest/host_check_test.go new file mode 100644 index 000000000..b77ddd58d --- /dev/null +++ b/internal/server/apitest/host_check_test.go @@ -0,0 +1,121 @@ +package apitest + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + Assert "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/wesm/middleman/internal/apiclient" + "github.com/wesm/middleman/internal/config" + "github.com/wesm/middleman/internal/db" + ghclient "github.com/wesm/middleman/internal/github" + "github.com/wesm/middleman/internal/server" + "github.com/wesm/middleman/internal/testutil/dbtest" +) + +// TestHostValidationE2E exercises the Host validation middleware +// through the full apitest stack (full Huma router, SQLite) with +// an explicit HostCheckOptions override. The override bypasses the +// test-friendly fallback (AllowLoopbackAnyPort) so the test pins +// the production contract: only the configured bind and the +// loopback synonyms at the bind port are accepted; the +// `middleman.test` hostname the apitest helpers normally use is +// NOT in the allowlist. +// +// Case 1: a request whose Host is attacker.example:8091 is +// rejected with the documented JSON 403 body before any handler +// runs (the PR list is seeded but never reached, since the +// returned body contains the host validation error, not pulls). +// +// Case 2: a request whose Host matches the configured bind +// (127.0.0.1:8091) reaches the handler and returns 200 with the +// seeded PR list. +func TestHostValidationE2E(t *testing.T) { + srv, database := setupHostValidationServer(t) + seedPR(t, database, "acme", "widget", 1) + + t.Run("rejects DNS-rebound hostname", func(t *testing.T) { + client := newHostValidationClient(t, srv, "http://attacker.example:8091") + resp, err := client.HTTP.ListPullsWithResponse(t.Context(), nil) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode()) + + var payload struct { + Error string `json:"error"` + } + require.NoError(t, json.Unmarshal(resp.Body, &payload)) + assert := Assert.New(t) + assert.Contains(payload.Error, "allowed_hosts") + assert.Contains(payload.Error, "trust_reverse_proxy") + }) + + t.Run("accepts configured bind", func(t *testing.T) { + client := newHostValidationClient(t, srv, "http://127.0.0.1:8091") + resp, err := client.HTTP.ListPullsWithResponse(t.Context(), nil) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode()) + require.NotNil(t, resp.JSON200) + require.Len(t, *resp.JSON200, 1) + assert := Assert.New(t) + assert.Equal("acme", (*resp.JSON200)[0].RepoOwner) + assert.Equal("widget", (*resp.JSON200)[0].RepoName) + assert.EqualValues(1, (*resp.JSON200)[0].Number) + }) +} + +// setupHostValidationServer builds a Server with an explicit +// HostCheckOptions so the production contract — strict bind match +// plus no any-port relaxation — is what the test exercises. +func setupHostValidationServer(t *testing.T) (*server.Server, *db.DB) { + t.Helper() + database := dbtest.Open(t) + syncer := ghclient.NewSyncer(nil, database, nil, defaultTestRepos, time.Minute, nil, nil) + t.Cleanup(syncer.Stop) + srv := server.New(database, syncer, nil, "/", nil, server.ServerOptions{ + HostCheck: server.HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + }, + }) + return srv, database +} + +// newHostValidationClient builds an apiclient.Client whose base +// URL drives req.URL.Host (and therefore the server's req.Host) +// per the row. Reuses the apitest round-tripper pattern from +// setupTestClient but isolates this test from the default +// "middleman.test" base URL. +func newHostValidationClient(t *testing.T, srv *server.Server, baseURL string) *apiclient.Client { + t.Helper() + httpClient := &http.Client{ + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + var body io.Reader = http.NoBody + if req.Body != nil { + payload, err := io.ReadAll(req.Body) + if err != nil { + return nil, err + } + _ = req.Body.Close() + body = strings.NewReader(string(payload)) + } + serverReq := httptest.NewRequest(req.Method, req.URL.String(), body) + serverReq.Header = req.Header.Clone() + if req.Method != http.MethodGet && serverReq.Header.Get("Content-Type") == "" { + serverReq.Header.Set("Content-Type", "application/json") + } + serverReq = serverReq.WithContext(req.Context()) + + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, serverReq) + return rr.Result(), nil + }), + } + client, err := apiclient.NewWithHTTPClient(baseURL, httpClient) + require.NoError(t, err) + return client +} From 54ed309e0c40c70c6f848b43f2bde9bab479756c Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 09:34:13 -0400 Subject: [PATCH 09/15] fix: derive host checks from provided config Resolve PR review findings by removing the production testing dependency from server host-check setup and deriving host validation options from non-nil config values when Validate has not populated cached keys. Keep the legacy partial-config fallback behind test-only wiring and add full-stack trusted proxy allowlist coverage for forwarded hosts. --- cmd/e2e-server/main.go | 5 +- cmd/middleman/main_test.go | 2 + cmd/middleman/startup_token_e2e_test.go | 2 +- internal/server/apitest/host_check_test.go | 76 +++++++++++++-- internal/server/e2etest/fixtures_test.go | 12 ++- internal/server/e2etest/sync_cooldown_test.go | 2 +- internal/server/host_check_default_test.go | 21 ++++ internal/server/host_check_testmode_test.go | 5 + internal/server/roborev_proxy_test.go | 2 +- internal/server/server.go | 97 ++++++++++++++----- internal/server/settings_test.go | 4 +- .../server/workspacetest/fixtures_test.go | 4 + middleman_test.go | 4 + 13 files changed, 194 insertions(+), 42 deletions(-) create mode 100644 internal/server/host_check_testmode_test.go diff --git a/cmd/e2e-server/main.go b/cmd/e2e-server/main.go index 5fb86f51b..bdb2ced09 100644 --- a/cmd/e2e-server/main.go +++ b/cmd/e2e-server/main.go @@ -890,8 +890,9 @@ func run( srv := server.NewWithConfig( database, syncer, diffRepo.Manager, assets, cfg, cfgPath, server.ServerOptions{ - Clones: diffRepo.Manager, - WorktreeDir: e2eWorktreeDir, + Clones: diffRepo.Manager, + WorktreeDir: e2eWorktreeDir, + HostCheckAllowLoopbackAnyPort: true, }, ) defer cleanupE2EWorkspaces(database, diffRepo.Manager, e2eWorktreeDir, cfg.TmuxCommand()) diff --git a/cmd/middleman/main_test.go b/cmd/middleman/main_test.go index 489e0692b..d36ee8160 100644 --- a/cmd/middleman/main_test.go +++ b/cmd/middleman/main_test.go @@ -323,6 +323,7 @@ func TestStartupFallbackKeepsPersistedGlobMatchesInAPIs(t *testing.T) { ) reposReq := httptest.NewRequest(http.MethodGet, "/api/v1/repos", nil) + reposReq.Host = "127.0.0.1:8091" reposRR := httptest.NewRecorder() srv.ServeHTTP(reposRR, reposReq) require.Equal(http.StatusOK, reposRR.Code, reposRR.Body.String()) @@ -339,6 +340,7 @@ func TestStartupFallbackKeepsPersistedGlobMatchesInAPIs(t *testing.T) { }) settingsReq := httptest.NewRequest(http.MethodGet, "/api/v1/settings", nil) + settingsReq.Host = "127.0.0.1:8091" settingsRR := httptest.NewRecorder() srv.ServeHTTP(settingsRR, settingsReq) require.Equal(http.StatusOK, settingsRR.Code, settingsRR.Body.String()) diff --git a/cmd/middleman/startup_token_e2e_test.go b/cmd/middleman/startup_token_e2e_test.go index 59c82cfe6..71af2a0cf 100644 --- a/cmd/middleman/startup_token_e2e_test.go +++ b/cmd/middleman/startup_token_e2e_test.go @@ -54,7 +54,7 @@ func TestCollectProviderTokensInvokesGHWithHostnameForEnterprise(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.toml") require.NoError(os.WriteFile(configPath, []byte(` host = "127.0.0.1" -port = 0 +port = 8091 [[repos]] platform = "github" diff --git a/internal/server/apitest/host_check_test.go b/internal/server/apitest/host_check_test.go index b77ddd58d..9b4c6d71f 100644 --- a/internal/server/apitest/host_check_test.go +++ b/internal/server/apitest/host_check_test.go @@ -1,6 +1,7 @@ package apitest import ( + "context" "encoding/json" "io" "net/http" @@ -12,6 +13,7 @@ import ( Assert "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/wesm/middleman/internal/apiclient" + "github.com/wesm/middleman/internal/apiclient/generated" "github.com/wesm/middleman/internal/config" "github.com/wesm/middleman/internal/db" ghclient "github.com/wesm/middleman/internal/github" @@ -56,12 +58,13 @@ func TestHostValidationE2E(t *testing.T) { }) t.Run("accepts configured bind", func(t *testing.T) { + require := require.New(t) client := newHostValidationClient(t, srv, "http://127.0.0.1:8091") resp, err := client.HTTP.ListPullsWithResponse(t.Context(), nil) - require.NoError(t, err) - require.Equal(t, http.StatusOK, resp.StatusCode()) - require.NotNil(t, resp.JSON200) - require.Len(t, *resp.JSON200, 1) + require.NoError(err) + require.Equal(http.StatusOK, resp.StatusCode()) + require.NotNil(resp.JSON200) + require.Len(*resp.JSON200, 1) assert := Assert.New(t) assert.Equal("acme", (*resp.JSON200)[0].RepoOwner) assert.Equal("widget", (*resp.JSON200)[0].RepoName) @@ -69,18 +72,68 @@ func TestHostValidationE2E(t *testing.T) { }) } +func TestHostValidationTrustedProxyE2E(t *testing.T) { + srv, database := setupHostValidationServer(t, server.HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + Allowed: []config.HostKey{ + {Host: "proxy.local", Port: "8091"}, + {Host: "middleman.example", Port: ""}, + }, + TrustReverseProxy: true, + }) + seedPR(t, database, "acme", "widget", 1) + + t.Run("accepts allowed forwarded public host", func(t *testing.T) { + require := require.New(t) + client := newHostValidationClient(t, srv, "http://proxy.local:8091") + resp, err := client.HTTP.ListPullsWithResponse( + t.Context(), nil, + requestHeader("X-Forwarded-Host", "middleman.example"), + ) + require.NoError(err) + require.Equal(http.StatusOK, resp.StatusCode()) + require.NotNil(resp.JSON200) + require.Len(*resp.JSON200, 1) + }) + + t.Run("rejects disallowed forwarded public host", func(t *testing.T) { + client := newHostValidationClient(t, srv, "http://proxy.local:8091") + resp, err := client.HTTP.ListPullsWithResponse( + t.Context(), nil, + requestHeader("X-Forwarded-Host", "attacker.example"), + ) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode()) + }) + + t.Run("rejects mismatched forwarded headers", func(t *testing.T) { + client := newHostValidationClient(t, srv, "http://proxy.local:8091") + resp, err := client.HTTP.ListPullsWithResponse( + t.Context(), nil, + requestHeader("X-Forwarded-Host", "middleman.example"), + requestHeader("Forwarded", "host=attacker.example"), + ) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode()) + }) +} + // setupHostValidationServer builds a Server with an explicit // HostCheckOptions so the production contract — strict bind match // plus no any-port relaxation — is what the test exercises. -func setupHostValidationServer(t *testing.T) (*server.Server, *db.DB) { +func setupHostValidationServer(t *testing.T, opts ...server.HostCheckOptions) (*server.Server, *db.DB) { t.Helper() database := dbtest.Open(t) syncer := ghclient.NewSyncer(nil, database, nil, defaultTestRepos, time.Minute, nil, nil) t.Cleanup(syncer.Stop) + hostCheck := server.HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + } + if len(opts) > 0 { + hostCheck = opts[0] + } srv := server.New(database, syncer, nil, "/", nil, server.ServerOptions{ - HostCheck: server.HostCheckOptions{ - Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, - }, + HostCheck: hostCheck, }) return srv, database } @@ -119,3 +172,10 @@ func newHostValidationClient(t *testing.T, srv *server.Server, baseURL string) * require.NoError(t, err) return client } + +func requestHeader(name, value string) generated.RequestEditorFn { + return func(_ context.Context, req *http.Request) error { + req.Header.Set(name, value) + return nil + } +} diff --git a/internal/server/e2etest/fixtures_test.go b/internal/server/e2etest/fixtures_test.go index fbd9dfda5..a6166865d 100644 --- a/internal/server/e2etest/fixtures_test.go +++ b/internal/server/e2etest/fixtures_test.go @@ -27,7 +27,9 @@ func setupTestServer(t *testing.T) (*server.Server, *db.DB) { syncer := ghclient.NewSyncer(nil, database, nil, nil, time.Minute, nil, nil) t.Cleanup(syncer.Stop) - srv := server.New(database, syncer, nil, "/", nil, server.ServerOptions{}) + srv := server.New(database, syncer, nil, "/", nil, server.ServerOptions{ + HostCheckAllowLoopbackAnyPort: true, + }) return srv, database } @@ -62,7 +64,9 @@ func setupWithBasePath(t *testing.T, basePath string, _ any) *server.Server { database, nil, nil, time.Minute, nil, nil, ) t.Cleanup(syncer.Stop) - return server.New(database, syncer, nil, basePath, nil, server.ServerOptions{}) + return server.New(database, syncer, nil, basePath, nil, server.ServerOptions{ + HostCheckAllowLoopbackAnyPort: true, + }) } func setupTestServerWithConfig(t *testing.T) (*server.Server, *db.DB, string) { @@ -113,7 +117,9 @@ func setupTestServerWithConfigContentAndSyncer( t.Cleanup(syncer.Stop) srv := server.NewWithConfig( - database, syncer, nil, nil, cfg, cfgPath, server.ServerOptions{}, + database, syncer, nil, nil, cfg, cfgPath, server.ServerOptions{ + HostCheckAllowLoopbackAnyPort: true, + }, ) return srv, database, cfgPath, syncer } diff --git a/internal/server/e2etest/sync_cooldown_test.go b/internal/server/e2etest/sync_cooldown_test.go index b9eaaffed..e13b59b1d 100644 --- a/internal/server/e2etest/sync_cooldown_test.go +++ b/internal/server/e2etest/sync_cooldown_test.go @@ -340,7 +340,7 @@ func startSyncCooldownE2EServerWithSyncer( srv := server.NewWithConfig( database, syncer, nil, nil, cfg, cfgPath, - server.ServerOptions{}, + server.ServerOptions{HostCheckAllowLoopbackAnyPort: true}, ) ln, err := net.Listen("tcp", "127.0.0.1:0") diff --git a/internal/server/host_check_default_test.go b/internal/server/host_check_default_test.go index d7fa6a66a..0bd6ffcef 100644 --- a/internal/server/host_check_default_test.go +++ b/internal/server/host_check_default_test.go @@ -9,6 +9,7 @@ import ( "time" Assert "github.com/stretchr/testify/assert" + "go.kenn.io/middleman/internal/config" ghclient "go.kenn.io/middleman/internal/github" "go.kenn.io/middleman/internal/testutil/dbtest" ) @@ -47,6 +48,26 @@ func TestNewCfgNilTestFriendlyDefault(t *testing.T) { } } +func TestNewDerivesHostCheckFromUnvalidatedConfig(t *testing.T) { + database := dbtest.Open(t) + syncer := ghclient.NewSyncer(nil, database, nil, nil, time.Minute, nil, nil) + t.Cleanup(syncer.Stop) + srv := New(database, syncer, emptyFrontend(), "/", &config.Config{ + Host: "127.0.0.1", + Port: 8091, + AllowedHosts: []string{"mm.example.com"}, + TrustReverseProxy: true, + }, ServerOptions{}) + + req := httptest.NewRequest(http.MethodGet, "/healthz", nil) + req.Host = "127.0.0.1:8091" + req.Header.Set("X-Forwarded-Host", "mm.example.com") + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + + Assert.Equal(t, http.StatusOK, rr.Code, rr.Body.String()) +} + func newServerForDefaultTest(t *testing.T) *Server { t.Helper() database := dbtest.Open(t) diff --git a/internal/server/host_check_testmode_test.go b/internal/server/host_check_testmode_test.go new file mode 100644 index 000000000..3d886dd92 --- /dev/null +++ b/internal/server/host_check_testmode_test.go @@ -0,0 +1,5 @@ +package server + +func init() { + allowUnvalidatedConfigHostCheckFallbackForTests = true +} diff --git a/internal/server/roborev_proxy_test.go b/internal/server/roborev_proxy_test.go index 386d7000a..fcf9c011f 100644 --- a/internal/server/roborev_proxy_test.go +++ b/internal/server/roborev_proxy_test.go @@ -60,7 +60,7 @@ endpoint = %q t.Cleanup(syncer.Stop) return NewWithConfig( database, syncer, nil, nil, cfg, cfgPath, - ServerOptions{}, + ServerOptions{HostCheckAllowLoopbackAnyPort: true}, ) } diff --git a/internal/server/server.go b/internal/server/server.go index ee4556db1..229812a02 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -16,7 +16,6 @@ import ( "strconv" "strings" "sync" - "testing" "time" "github.com/danielgtaylor/huma/v2" @@ -83,6 +82,10 @@ type ServerOptions struct { // allowed_hosts / trust_reverse_proxy independently of a full // config.Config. HostCheck HostCheckOptions + // HostCheckAllowLoopbackAnyPort relaxes literal loopback Host + // port matching after HostCheck/cfg options have been selected. + // Use this for httptest-style listeners on ephemeral ports. + HostCheckAllowLoopbackAnyPort bool } type shutdownDeadline struct { @@ -513,32 +516,36 @@ func testFallbackAllowedHosts() []config.HostKey { } } +// allowUnvalidatedConfigHostCheckFallbackForTests is false in +// production. Same-package tests set it from _test.go so legacy +// partial config literals can exercise unrelated server behavior +// without manufacturing a full validated config. +var allowUnvalidatedConfigHostCheckFallbackForTests bool + // resolveHostCheckOptions applies the precedence rule: -// caller override > validated cfg > test-friendly fallback. The -// fallback fires when cfg is nil OR cfg lacks a validated bind key -// (partial config literal that bypassed Validate). In production -// cfg is always loaded via config.Load, which populates the bind -// key; the fallback exists solely for the many test helpers that -// construct servers from partial cfgs or pass cfg=nil entirely. -// -// Under `go test`, AllowLoopbackAnyPort is forced true on the -// resulting options regardless of which branch supplied them. -// httptest.NewServer binds an ephemeral port that callers cannot -// pre-declare in allowed_hosts, so test binaries need any-port -// acceptance for literal loopback IPs. Production code never has -// testing.Testing() return true, so the strict spec behavior is -// preserved on the deployed binary. -func resolveHostCheckOptions(cfg *config.Config, override HostCheckOptions) HostCheckOptions { - opts, fromOverride := pickHostCheckOptions(cfg, override) - if testing.Testing() && !fromOverride { +// caller override > cfg-derived options > cfg=nil test-friendly +// fallback. For non-nil configs that bypassed config.Load, derive +// the bind and allowlist from the provided config fields so +// production callers do not silently inherit hard-coded host +// defaults. +func resolveHostCheckOptions( + cfg *config.Config, + override HostCheckOptions, + allowLoopbackAnyPort bool, +) HostCheckOptions { + opts, err := pickHostCheckOptions(cfg, override) + if err != nil { + panic(err) + } + if allowLoopbackAnyPort { opts.AllowLoopbackAnyPort = true } return opts } -func pickHostCheckOptions(cfg *config.Config, override HostCheckOptions) (HostCheckOptions, bool) { +func pickHostCheckOptions(cfg *config.Config, override HostCheckOptions) (HostCheckOptions, error) { if override.Valid() { - return override, true + return override, nil } if cfg != nil { if k := cfg.BindHostKey(); k.Valid() { @@ -546,12 +553,50 @@ func pickHostCheckOptions(cfg *config.Config, override HostCheckOptions) (HostCh Bind: k, Allowed: cfg.ParsedAllowedHosts(), TrustReverseProxy: cfg.TrustReverseProxy, - }, false + }, nil + } + opts, err := deriveHostCheckOptionsFromConfig(cfg) + if err == nil { + return opts, nil } + if !allowUnvalidatedConfigHostCheckFallbackForTests { + return HostCheckOptions{}, fmt.Errorf("server: config did not provide valid Host check options: %w", err) + } + return fallbackHostCheckOptions(), nil } slog.Warn( - "server.New used without a validated cfg or explicit ServerOptions.HostCheck; using httptest-compatible Host defaults. Production callers must pass a validated config or explicit HostCheck options.", + "server.New used without a cfg or explicit ServerOptions.HostCheck; using httptest-compatible Host defaults. Production callers must pass a validated config or explicit HostCheck options.", ) + return fallbackHostCheckOptions(), nil +} + +func deriveHostCheckOptionsFromConfig(cfg *config.Config) (HostCheckOptions, error) { + if strings.TrimSpace(cfg.Host) == "" { + return HostCheckOptions{}, errors.New("host is empty") + } + if cfg.Port < 0 || cfg.Port > 65535 { + return HostCheckOptions{}, fmt.Errorf("port %d is outside 0-65535", cfg.Port) + } + bind, err := config.ParseHostKey(net.JoinHostPort(cfg.Host, fmt.Sprintf("%d", cfg.Port))) + if err != nil { + return HostCheckOptions{}, fmt.Errorf("bind host %q: %w", cfg.ListenAddr(), err) + } + allowed := make([]config.HostKey, 0, len(cfg.AllowedHosts)) + for _, entry := range cfg.AllowedHosts { + key, err := config.ParseHostKey(entry) + if err != nil { + return HostCheckOptions{}, fmt.Errorf("allowed_hosts entry %q: %w", entry, err) + } + allowed = append(allowed, key) + } + return HostCheckOptions{ + Bind: bind, + Allowed: allowed, + TrustReverseProxy: cfg.TrustReverseProxy, + }, nil +} + +func fallbackHostCheckOptions() HostCheckOptions { return HostCheckOptions{ Bind: config.HostKey{ Host: hostCheckTestFallbackBindHost, @@ -560,7 +605,7 @@ func pickHostCheckOptions(cfg *config.Config, override HostCheckOptions) (HostCh Allowed: testFallbackAllowedHosts(), TrustReverseProxy: false, AllowLoopbackAnyPort: true, - }, false + } } func newServer( @@ -577,7 +622,11 @@ func newServer( bgBaseCtx, bgCancel := context.WithCancel(context.Background()) bgDeadline := &shutdownDeadline{} - hostOpts := resolveHostCheckOptions(cfg, options.HostCheck) + hostOpts := resolveHostCheckOptions( + cfg, + options.HostCheck, + options.HostCheckAllowLoopbackAnyPort, + ) s := &Server{ db: database, diff --git a/internal/server/settings_test.go b/internal/server/settings_test.go index d78f5d902..d28f94a0c 100644 --- a/internal/server/settings_test.go +++ b/internal/server/settings_test.go @@ -68,7 +68,7 @@ func setupTestServerWithConfigContent( t.Cleanup(syncer.Stop) srv := NewWithConfig( database, syncer, nil, nil, cfg, cfgPath, - ServerOptions{}, + ServerOptions{HostCheckAllowLoopbackAnyPort: true}, ) return srv, database, cfgPath } @@ -102,7 +102,7 @@ func setupTestServerWithConfigProviders( t.Cleanup(syncer.Stop) srv := NewWithConfig( database, syncer, nil, nil, cfg, cfgPath, - ServerOptions{}, + ServerOptions{HostCheckAllowLoopbackAnyPort: true}, ) return srv, database, cfgPath } diff --git a/internal/server/workspacetest/fixtures_test.go b/internal/server/workspacetest/fixtures_test.go index 771ee6341..8f4c9c96a 100644 --- a/internal/server/workspacetest/fixtures_test.go +++ b/internal/server/workspacetest/fixtures_test.go @@ -91,6 +91,10 @@ func setupWorkspaceServerFixture( srv := server.New(database, syncer, nil, basePath, cfg, server.ServerOptions{ Clones: clones, WorktreeDir: worktreeDir, + HostCheck: server.HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + Allowed: []config.HostKey{{Host: "middleman.test", Port: ""}}, + }, }) t.Cleanup(func() { ctx, cancel := context.WithTimeout(context.WithoutCancel(t.Context()), 5*time.Second) diff --git a/middleman_test.go b/middleman_test.go index 9d4b48571..a039ade93 100644 --- a/middleman_test.go +++ b/middleman_test.go @@ -34,6 +34,7 @@ func TestNewReturnsWorkingHandler(t *testing.T) { defer inst.Close() req := httptest.NewRequest(http.MethodGet, "/middleman/", nil) + req.Host = "127.0.0.1:8091" rr := httptest.NewRecorder() inst.Handler().ServeHTTP(rr, req) @@ -73,6 +74,7 @@ func TestNewEmbeddedBootstrapGlobals(t *testing.T) { defer inst.Close() req := httptest.NewRequest(http.MethodGet, "/middleman/", nil) + req.Host = "127.0.0.1:8091" rr := httptest.NewRecorder() inst.Handler().ServeHTTP(rr, req) @@ -118,6 +120,7 @@ func TestEmbedConfigFullFlow(t *testing.T) { defer inst.Close() req := httptest.NewRequest(http.MethodGet, "/app/", nil) + req.Host = "127.0.0.1:8091" rr := httptest.NewRecorder() inst.Handler().ServeHTTP(rr, req) @@ -155,6 +158,7 @@ func TestNoEmbedConfigStandaloneMode(t *testing.T) { defer inst.Close() req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "127.0.0.1:8091" rr := httptest.NewRecorder() inst.Handler().ServeHTTP(rr, req) From 066adb4ec7e208f2e1ab00c84036a3f1c8603e4c Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 09:43:52 -0400 Subject: [PATCH 10/15] test: allow workspace fixture host checks The host-check repair made validated configs enforce their configured bind. Workspace API tests use a middleman.test apiclient base URL and, in some cases, httptest listeners on ephemeral loopback ports, so make that fixture opt in explicitly instead of relying on production test detection. --- internal/server/api_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/server/api_test.go b/internal/server/api_test.go index 0fdf3a663..8495641de 100644 --- a/internal/server/api_test.go +++ b/internal/server/api_test.go @@ -17196,6 +17196,13 @@ func setupWorkspaceServerFixtureWithMockHostAndOptions( } options.Clones = clones options.WorktreeDir = worktreeDir + options.HostCheckAllowLoopbackAnyPort = true + if !options.HostCheck.Valid() { + options.HostCheck = HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + Allowed: []config.HostKey{{Host: "middleman.test", Port: ""}}, + } + } srv := New(database, syncer, nil, basePath, cfg, options) t.Cleanup(func() { cleanupWorkspaceServerFixtureTmuxSessions(t, dir) }) // Cleanup callbacks run LIFO. Drain the server first so async From 5776f1a93b5ff196ba813c7e0b95f9ab04313e85 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 09:55:54 -0400 Subject: [PATCH 11/15] test: use allowed host for workspace diff fixtures Workspace diff tests issue direct httptest requests against the full server fixture. After host validation became strict, those requests kept httptest's default example.com host and hit the middleware instead of the workspace handlers. Route them through the fixture's accepted test host so the tests exercise the intended API behavior. --- internal/server/api_test.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/internal/server/api_test.go b/internal/server/api_test.go index 8495641de..b58d001e1 100644 --- a/internal/server/api_test.go +++ b/internal/server/api_test.go @@ -20202,7 +20202,7 @@ func TestWorkspaceDiffEndpointRejectsOriginBaseE2E(t *testing.T) { ctx := context.Background() ws := createReadyWorkspace(t, ctx, client) - req := httptest.NewRequest( + req := newWorkspaceFixtureRequest( http.MethodGet, "/api/v1/workspaces/"+ws.Id+"/diff?base=origin", nil, @@ -20467,11 +20467,7 @@ func requestWorkspaceFilesPath( ) generated.FilesResponse { t.Helper() - req := httptest.NewRequest( - http.MethodGet, - query, - nil, - ) + req := newWorkspaceFixtureRequest(http.MethodGet, query, nil) rr := httptest.NewRecorder() srv.ServeHTTP(rr, req) resp := rr.Result() @@ -20490,7 +20486,7 @@ func requestWorkspaceCommits( ) commitsResponse { t.Helper() - req := httptest.NewRequest( + req := newWorkspaceFixtureRequest( http.MethodGet, "/api/v1/workspaces/"+workspaceID+"/commits", nil, @@ -20542,11 +20538,7 @@ func requestWorkspaceDiffPath( ) generated.DiffResponse { t.Helper() - req := httptest.NewRequest( - http.MethodGet, - query, - nil, - ) + req := newWorkspaceFixtureRequest(http.MethodGet, query, nil) rr := httptest.NewRecorder() srv.ServeHTTP(rr, req) resp := rr.Result() @@ -20569,7 +20561,7 @@ func requestWorkspaceDiffForPath( query := "/api/v1/workspaces/" + workspaceID + "/diff?base=" + base + "&path=" + url.QueryEscape(path) - req := httptest.NewRequest( + req := newWorkspaceFixtureRequest( http.MethodGet, query, nil, @@ -20585,6 +20577,16 @@ func requestWorkspaceDiffForPath( return body } +func newWorkspaceFixtureRequest( + method string, + target string, + body io.Reader, +) *http.Request { + req := httptest.NewRequest(method, target, body) + req.Host = "middleman.test" + return req +} + func requireWorkspaceDiffFile( t *testing.T, files []generated.DiffFile, From 72398fddf8fe72d91808ea0699f334d9aeaa0726 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 11:01:54 -0400 Subject: [PATCH 12/15] fix: make configured host checks explicit Address roborev feedback by rejecting port 0 during config validation instead of letting Host validation fail later with an indirect parse error. Add full-stack API coverage that loads allowed_hosts and trust_reverse_proxy from TOML so the config-derived host-check path is exercised. --- README.md | 2 +- internal/config/config.go | 2 +- internal/config/config_test.go | 8 ++++ internal/server/apitest/host_check_test.go | 54 ++++++++++++++++++++++ internal/server/server.go | 4 +- 5 files changed, 66 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 77f8eb843..1cdc295dc 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,7 @@ All fields are optional. Repos can be added in the config file or through the Se | `github_token_env` | `"MIDDLEMAN_GITHUB_TOKEN"` | Env var holding the default GitHub token | | `default_platform_host` | `"github.com"` | Host treated as implicit in repository UI labels | | `host` | `"127.0.0.1"` | Listen address | -| `port` | `8091` | Listen port | +| `port` | `8091` | Listen port, from 1 to 65535 | | `base_path` | `"/"` | URL prefix for reverse proxy deployments | | `data_dir` | `"~/.config/middleman"` | Directory for the SQLite database | | `activity.view_mode` | `"threaded"` | `"flat"` or `"threaded"` | diff --git a/internal/config/config.go b/internal/config/config.go index ad307e0f6..82d8b80e3 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -877,7 +877,7 @@ func (c *Config) Validate() error { return fmt.Errorf("config: host %q is not loopback; only loopback addresses are supported", c.Host) } - if c.Port < 0 || c.Port > 65535 { + if c.Port < 1 || c.Port > 65535 { return fmt.Errorf("config: invalid port %d", c.Port) } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 484d079f5..9426e7975 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2712,6 +2712,14 @@ port = 8091 ) } +func TestLoadRejectsZeroPort(t *testing.T) { + _, err := Load(writeConfig(t, `host = "127.0.0.1" +port = 0 +`)) + require.Error(t, err) + Assert.Contains(t, err.Error(), "invalid port 0") +} + func TestLoadAllowedHostsParsesAndCanonicalises(t *testing.T) { assert := Assert.New(t) cfg, err := Load(writeConfig(t, `host = "127.0.0.1" diff --git a/internal/server/apitest/host_check_test.go b/internal/server/apitest/host_check_test.go index 9b4c6d71f..442e9af51 100644 --- a/internal/server/apitest/host_check_test.go +++ b/internal/server/apitest/host_check_test.go @@ -6,6 +6,7 @@ import ( "io" "net/http" "net/http/httptest" + "os" "strings" "testing" "time" @@ -72,6 +73,38 @@ func TestHostValidationE2E(t *testing.T) { }) } +func TestHostValidationUsesConfigDerivedTrustedProxyE2E(t *testing.T) { + srv, database := setupHostValidationServerFromConfig(t, `host = "127.0.0.1" +port = 8091 +allowed_hosts = ["proxy.local:8091", "middleman.example"] +trust_reverse_proxy = true +`) + seedPR(t, database, "acme", "widget", 1) + + t.Run("accepts allowed forwarded host from config", func(t *testing.T) { + require := require.New(t) + client := newHostValidationClient(t, srv, "http://proxy.local:8091") + resp, err := client.HTTP.ListPullsWithResponse( + t.Context(), nil, + requestHeader("X-Forwarded-Host", "middleman.example"), + ) + require.NoError(err) + require.Equal(http.StatusOK, resp.StatusCode()) + require.NotNil(resp.JSON200) + require.Len(*resp.JSON200, 1) + }) + + t.Run("rejects disallowed forwarded host from config", func(t *testing.T) { + client := newHostValidationClient(t, srv, "http://proxy.local:8091") + resp, err := client.HTTP.ListPullsWithResponse( + t.Context(), nil, + requestHeader("X-Forwarded-Host", "attacker.example"), + ) + require.NoError(t, err) + require.Equal(t, http.StatusForbidden, resp.StatusCode()) + }) +} + func TestHostValidationTrustedProxyE2E(t *testing.T) { srv, database := setupHostValidationServer(t, server.HostCheckOptions{ Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, @@ -138,6 +171,27 @@ func setupHostValidationServer(t *testing.T, opts ...server.HostCheckOptions) (* return srv, database } +func setupHostValidationServerFromConfig(t *testing.T, content string) (*server.Server, *db.DB) { + t.Helper() + cfgPath := writeHostValidationConfig(t, content) + cfg, err := config.Load(cfgPath) + require.NoError(t, err) + + database := dbtest.Open(t) + syncer := ghclient.NewSyncer(nil, database, nil, defaultTestRepos, time.Minute, nil, nil) + t.Cleanup(syncer.Stop) + + srv := server.NewWithConfig(database, syncer, nil, nil, cfg, cfgPath, server.ServerOptions{}) + return srv, database +} + +func writeHostValidationConfig(t *testing.T, content string) string { + t.Helper() + path := t.TempDir() + "/config.toml" + require.NoError(t, os.WriteFile(path, []byte(content), 0o600)) + return path +} + // newHostValidationClient builds an apiclient.Client whose base // URL drives req.URL.Host (and therefore the server's req.Host) // per the row. Reuses the apitest round-tripper pattern from diff --git a/internal/server/server.go b/internal/server/server.go index 229812a02..f53bd97ef 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -574,8 +574,8 @@ func deriveHostCheckOptionsFromConfig(cfg *config.Config) (HostCheckOptions, err if strings.TrimSpace(cfg.Host) == "" { return HostCheckOptions{}, errors.New("host is empty") } - if cfg.Port < 0 || cfg.Port > 65535 { - return HostCheckOptions{}, fmt.Errorf("port %d is outside 0-65535", cfg.Port) + if cfg.Port < 1 || cfg.Port > 65535 { + return HostCheckOptions{}, fmt.Errorf("port %d is outside 1-65535", cfg.Port) } bind, err := config.ParseHostKey(net.JoinHostPort(cfg.Host, fmt.Sprintf("%d", cfg.Port))) if err != nil { From 7ce85b790892d37f720cbcc36a5848a08038773c Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 13:44:17 -0400 Subject: [PATCH 13/15] test: allow SSE e2e fixture on random ports The SSE replay e2e helper builds a config-backed server but runs it through httptest.NewServer, so requests arrive with a random loopback port. Opt the helper into the explicit loopback-any-port test relaxation used by the other e2e fixtures so host validation does not block the stream before frames are emitted. --- internal/server/e2etest/fixtures_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/server/e2etest/fixtures_test.go b/internal/server/e2etest/fixtures_test.go index a6166865d..0c93028ee 100644 --- a/internal/server/e2etest/fixtures_test.go +++ b/internal/server/e2etest/fixtures_test.go @@ -51,7 +51,9 @@ func setupTestServerWithSSEBufferSize( BasePath: "/", SSEBufferSize: size, } - srv := server.New(database, syncer, nil, "/", cfg, server.ServerOptions{}) + srv := server.New(database, syncer, nil, "/", cfg, server.ServerOptions{ + HostCheckAllowLoopbackAnyPort: true, + }) return srv, database, cfg } From c1ed3eead9953072138316bb94d9fc27880fe067 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Wed, 20 May 2026 13:52:30 -0400 Subject: [PATCH 14/15] test: make SSE host check fixture explicit Roborev flagged the SSE replay fixture for relying on config-derived host-check setup before the loopback test relaxation. Give the helper an explicit HostCheck override so httptest random ports are accepted without depending on partial config derivation. --- internal/server/e2etest/fixtures_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/server/e2etest/fixtures_test.go b/internal/server/e2etest/fixtures_test.go index 0c93028ee..a34872673 100644 --- a/internal/server/e2etest/fixtures_test.go +++ b/internal/server/e2etest/fixtures_test.go @@ -52,7 +52,10 @@ func setupTestServerWithSSEBufferSize( SSEBufferSize: size, } srv := server.New(database, syncer, nil, "/", cfg, server.ServerOptions{ - HostCheckAllowLoopbackAnyPort: true, + HostCheck: server.HostCheckOptions{ + Bind: config.HostKey{Host: "127.0.0.1", Port: "8091"}, + AllowLoopbackAnyPort: true, + }, }) return srv, database, cfg } From 191d8b279072ed098e7de081667c4eedd4c2bfe1 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Fri, 22 May 2026 20:23:08 -0400 Subject: [PATCH 15/15] fix: update host check imports after module rename Rebasing onto the current main branch brought in the go.kenn.io/middleman module path. The host check files were still using the old github.com/wesm/middleman imports, which prevented the rebased branch from compiling. --- internal/server/apitest/host_check_test.go | 14 +++++++------- internal/server/host_check_test.go | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/server/apitest/host_check_test.go b/internal/server/apitest/host_check_test.go index 442e9af51..eec75fd87 100644 --- a/internal/server/apitest/host_check_test.go +++ b/internal/server/apitest/host_check_test.go @@ -13,13 +13,13 @@ import ( Assert "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/wesm/middleman/internal/apiclient" - "github.com/wesm/middleman/internal/apiclient/generated" - "github.com/wesm/middleman/internal/config" - "github.com/wesm/middleman/internal/db" - ghclient "github.com/wesm/middleman/internal/github" - "github.com/wesm/middleman/internal/server" - "github.com/wesm/middleman/internal/testutil/dbtest" + "go.kenn.io/middleman/internal/apiclient" + "go.kenn.io/middleman/internal/apiclient/generated" + "go.kenn.io/middleman/internal/config" + "go.kenn.io/middleman/internal/db" + ghclient "go.kenn.io/middleman/internal/github" + "go.kenn.io/middleman/internal/server" + "go.kenn.io/middleman/internal/testutil/dbtest" ) // TestHostValidationE2E exercises the Host validation middleware diff --git a/internal/server/host_check_test.go b/internal/server/host_check_test.go index 920d1e41b..1b906343b 100644 --- a/internal/server/host_check_test.go +++ b/internal/server/host_check_test.go @@ -10,9 +10,9 @@ import ( Assert "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/wesm/middleman/internal/config" - ghclient "github.com/wesm/middleman/internal/github" - "github.com/wesm/middleman/internal/testutil/dbtest" + "go.kenn.io/middleman/internal/config" + ghclient "go.kenn.io/middleman/internal/github" + "go.kenn.io/middleman/internal/testutil/dbtest" ) // setupHostCheckServer builds a Server with the given