sneak/pixa
1
0
Fork 0
Code Issues 13 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 07:03:11 +02:00 by clawbot · 4 comments
clawbot commented 2026-08-09 07:03:11 +02:00
Collaborator
Copy Link
Copy Source

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.

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 commented 2026-09-21 09:25:39 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan.

  • New trusted_proxies config key: a list of CIDR ranges (YAML list or comma-separated string). Strict parsing via netip.ParsePrefix; an invalid entry aborts startup naming the key and the offending value. Absent or empty means trust nothing, which keeps RemoteAddr as the client address.
  • New internal/clientip package with a pure resolver. Forwarded headers are honored only when the immediate peer (RemoteAddr) falls inside a trusted range; otherwise the peer address wins and X-Forwarded-For is ignored entirely, so an untrusted client cannot spoof its address. When the peer is trusted, the chain is walked right-to-left and the resolver returns the rightmost entry that is not itself a trusted proxy.
  • The resolved address replaces the raw remote address in the request-logging middleware and in both login-attempt logs.
  • Tests first: honored from a trusted peer; ignored from an untrusted peer; a spoofed chain from an untrusted peer cannot influence the result; rightmost-untrusted selection across a mixed chain; plus config parsing (valid CIDRs, invalid entry aborts, absent means trust nothing). Docs in README.md and config.example.yml.
  • Scope: the config.go change is limited to the new key; #91 and #110 touch middleware.go and config.go in parallel.

model: claude-opus-4-8

Plan. - New `trusted_proxies` config key: a list of CIDR ranges (YAML list or comma-separated string). Strict parsing via `netip.ParsePrefix`; an invalid entry aborts startup naming the key and the offending value. Absent or empty means trust nothing, which keeps `RemoteAddr` as the client address. - New `internal/clientip` package with a pure resolver. Forwarded headers are honored **only** when the immediate peer (`RemoteAddr`) falls inside a trusted range; otherwise the peer address wins and `X-Forwarded-For` is ignored entirely, so an untrusted client cannot spoof its address. When the peer is trusted, the chain is walked right-to-left and the resolver returns the rightmost entry that is not itself a trusted proxy. - The resolved address replaces the raw remote address in the request-logging middleware and in both login-attempt logs. - Tests first: honored from a trusted peer; ignored from an untrusted peer; a spoofed chain from an untrusted peer cannot influence the result; rightmost-untrusted selection across a mixed chain; plus config parsing (valid CIDRs, invalid entry aborts, absent means trust nothing). Docs in `README.md` and `config.example.yml`. - Scope: the `config.go` change is limited to the new key; https://git.eeqj.de/sneak/pixa/issues/91 and https://git.eeqj.de/sneak/pixa/issues/110 touch `middleware.go` and `config.go` in parallel. model: claude-opus-4-8
clawbot commented 2026-09-22 01:29:58 +02:00
Author
Collaborator
Copy Link
Copy Source

Implemented in #127 (base next).

A trusted_proxies CIDR list (parsed by the same parser as blocked_networks) gates a new internal/clientip resolver: X-Forwarded-For is honored only when the direct peer is a trusted proxy, and the chain is walked right-to-left to the rightmost entry that is not itself a trusted proxy, so a directly connecting client cannot spoof its address. Omitted or empty trusts no one. The request-logging middleware and both login-attempt logs now record the resolved address. Documented in README.md and config.example.yml.

One reading taken: the resolver uses X-Forwarded-For only, not X-Real-IP, since that header carries the verifiable chain the definition of done describes.

Model: opus-4-8

Implemented in https://git.eeqj.de/sneak/pixa/pulls/127 (base `next`). A `trusted_proxies` CIDR list (parsed by the same parser as `blocked_networks`) gates a new `internal/clientip` resolver: `X-Forwarded-For` is honored only when the direct peer is a trusted proxy, and the chain is walked right-to-left to the rightmost entry that is not itself a trusted proxy, so a directly connecting client cannot spoof its address. Omitted or empty trusts no one. The request-logging middleware and both login-attempt logs now record the resolved address. Documented in `README.md` and `config.example.yml`. One reading taken: the resolver uses `X-Forwarded-For` only, not `X-Real-IP`, since that header carries the verifiable chain the definition of done describes. Model: opus-4-8
sneak commented 2026-09-22 08:40:27 +02:00
Owner
Copy Link
Copy Source

default trusted proxies should be all rfc1918 addresses

default trusted proxies should be all rfc1918 addresses
clawbot commented 2026-09-22 09:02:44 +02:00
Author
Collaborator
Copy Link
Copy Source

The default was changed per the ruling in this thread: when trusted_proxies is omitted, the trusted set is the RFC 1918 private ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) rather than trusting no one. An explicitly empty list still trusts nothing, and an explicit list replaces the default. Loopback is not included (not an RFC 1918 range, and no test expects it). Updated in #127.

Model: opus-4-8

The default was changed per the ruling in this thread: when `trusted_proxies` is omitted, the trusted set is the RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`) rather than trusting no one. An explicitly empty list still trusts nothing, and an explicit list replaces the default. Loopback is not included (not an RFC 1918 range, and no test expects it). Updated in https://git.eeqj.de/sneak/pixa/pulls/127. Model: opus-4-8
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/pixa#94