Blocks 1.0: no trusted-proxy handling — every log line and login record shows the proxy's IP #94

New Issue

clawbot · 2026-08-09T07:03:11+02:00

clawbot commented

2026-08-09 07:03:11 +02:00

From the audit against the canonical REPO_POLICIES.md. Independently verified against main at 61f42e6 — grep for X-Forwarded-For|X-Real-IP|RealIP across internal/ and cmd/ returns zero matches.

The policy requires:

> True client IP detection when behind a reverse proxy (X-Forwarded-For, X-Real-IP). The application must accept forwarded headers only from a configured set of trusted proxy addresses — never trust X-Forwarded-For unconditionally.

and separately states that services "are always deployed behind a TLS-terminating reverse proxy" — so this is not a hypothetical deployment, it is the expected one.

Current behavior:

ipFromHostPort() (internal/middleware/middleware.go:47-57) parses only r.RemoteAddr, and middleware.go:99 logs "remoteIP", ipFromHostPort(r.RemoteAddr).
internal/handlers/auth.go:48 and :62 log "remote_addr", r.RemoteAddr for failed and successful logins.
chi's middleware.RealIP is not in the chain either (internal/server/routes.go:18-29 uses Recoverer, RequestID, SecurityHeaders, Logging, Metrics, CORS, Timeout).

So in the mandated deployment, every access log line and every login-attempt record shows the reverse proxy's address. Failed-login logs — the only signal there is today that someone is guessing the signing key — are useless for identifying a source or building a blocklist.

Note the policy's "never trust unconditionally" half matters as much as the detection half: naively enabling RealIP without a trusted-proxy allowlist lets any client spoof its own address by sending the header, which is worse than the status quo because the logs then look authoritative while being attacker-controlled.

Definition of done

A trusted_proxies config key taking a CIDR list. Strict parsing: an invalid entry aborts startup naming the key and value, per repo policy. Empty/absent means trust nothing and use RemoteAddr — the safe default.
Forwarded headers are honored only when the immediate peer is in that list; otherwise RemoteAddr wins. Handle X-Forwarded-For chains correctly (rightmost untrusted entry), not by blindly taking the leftmost value.
The resolved client IP is used consistently by the logging middleware, the auth logs, and anything else that records a client address.
Failing tests first: forwarded header honored from a trusted source; ignored from an untrusted source; a spoofed chain from an untrusted peer cannot influence the result.
Document the key in README.md and config.example.yml; it is a prerequisite for a correct reverse-proxy setup, so it also belongs in the deployment guide issue.
make check green.

Priority

Blocks 1.0.0, and it is a hard prerequisite for #66 (login rate limiting): without it, a per-IP limiter behind a proxy keys every request to the proxy's address and would lock out all users at once the moment one attacker trips it. Do this before #66.

From the audit against the canonical `REPO_POLICIES.md`. Independently verified against `main` at `61f42e6` — grep for `X-Forwarded-For|X-Real-IP|RealIP` across `internal/` and `cmd/` returns **zero matches**. The policy requires: > True client IP detection when behind a reverse proxy (`X-Forwarded-For`, `X-Real-IP`). The application must accept forwarded headers only from a configured set of trusted proxy addresses — never trust `X-Forwarded-For` unconditionally. and separately states that services "are always deployed behind a TLS-terminating reverse proxy" — so this is not a hypothetical deployment, it is the expected one. Current behavior: - `ipFromHostPort()` (`internal/middleware/middleware.go:47-57`) parses only `r.RemoteAddr`, and `middleware.go:99` logs `"remoteIP", ipFromHostPort(r.RemoteAddr)`. - `internal/handlers/auth.go:48` and `:62` log `"remote_addr", r.RemoteAddr` for failed and successful logins. - chi's `middleware.RealIP` is not in the chain either (`internal/server/routes.go:18-29` uses `Recoverer`, `RequestID`, `SecurityHeaders`, `Logging`, `Metrics`, `CORS`, `Timeout`). So in the mandated deployment, **every access log line and every login-attempt record shows the reverse proxy's address**. Failed-login logs — the only signal there is today that someone is guessing the signing key — are useless for identifying a source or building a blocklist. Note the policy's "never trust unconditionally" half matters as much as the detection half: naively enabling `RealIP` without a trusted-proxy allowlist lets any client spoof its own address by sending the header, which is worse than the status quo because the logs then look authoritative while being attacker-controlled. ## Definition of done 1. A `trusted_proxies` config key taking a CIDR list. Strict parsing: an invalid entry aborts startup naming the key and value, per repo policy. Empty/absent means trust nothing and use `RemoteAddr` — the safe default. 2. Forwarded headers are honored **only** when the immediate peer is in that list; otherwise `RemoteAddr` wins. Handle `X-Forwarded-For` chains correctly (rightmost untrusted entry), not by blindly taking the leftmost value. 3. The resolved client IP is used consistently by the logging middleware, the auth logs, and anything else that records a client address. 4. Failing tests first: forwarded header honored from a trusted source; ignored from an untrusted source; a spoofed chain from an untrusted peer cannot influence the result. 5. Document the key in `README.md` and `config.example.yml`; it is a prerequisite for a correct reverse-proxy setup, so it also belongs in the deployment guide issue. 6. `make check` green. ## Priority **Blocks 1.0.0**, and it is a hard prerequisite for #66 (login rate limiting): without it, a per-IP limiter behind a proxy keys every request to the proxy's address and would lock out all users at once the moment one attacker trips it. Do this **before** #66.

clawbot added this to the 1.0.0 milestone 2026-08-09 07:03:11 +02:00

clawbot referenced this issue

2026-08-09 16:10:39 +02:00

Tracking: 1.0.0 sequencing and critical path #103

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/pixa#94