From ca9d074b8736116a97e873fe1990348fd927c6f1 Mon Sep 17 00:00:00 2001 From: Distronode <254259913+distronode-com@users.noreply.github.com> Date: Sun, 6 Sep 2026 00:14:20 -0400 Subject: [PATCH] server: let an operator name the proxies whose client IP is real Rate limits key on the TCP peer, and that is correct for a directly reachable instance and useless behind a fronting CDN, where every visitor arrives from the same few addresses and shares one bucket. TRUSTED_PROXY_CIDRS opts in per network: for a peer inside one of them the client IP comes from X-Forwarded-For, else the peer. The default does not move, and cannot be moved by a header. A peer that is not in the list has its headers ignored before they are read, so an instance that sets nothing behaves exactly as it did. Four details are the difference between this and a spoofable version. The X-Forwarded-For walk goes RIGHT TO LEFT past trusted hops and returns the first untrusted address, not the leftmost entry. The left of that header is whatever the original client sent, and every well-behaved proxy prepends to it and preserves it, so the leftmost value is chosen by the very party being rate limited. The rightmost non-trusted hop is the last address one of our own proxies actually observed. A hop that does not parse ends the walk and falls back to the peer rather than being skipped. Skipping it would let a client inject one malformed entry to push the walk past the real hop and onto a value it chose. X-Forwarded-For is read with Header.Values and joined, not Header.Get. Get returns only the FIRST field line, and a header may legitimately arrive as several: a client that sends its own line, in front of a proxy that adds a second rather than appending to the first, leaves the walk looking at a chain containing no trusted hop at all, so it returns the client's chosen address on its first step. RFC 9110 makes repeated field lines equivalent to one comma-joined value in the order received. Single-value vendor headers (CF-Connecting-IP, X-Real-IP, True-Client-IP) are not consulted at all, from any peer. Trusting one means trusting it from every network in the list, and the list names networks rather than CDNs: an ordinary reverse proxy in it forwards whatever headers the client sent, and the header carries nothing that says which hop wrote it. Nothing is lost by leaving them out, because a CDN sets X-Forwarded-For as well and its own ranges belong in the list anyway, so the walk steps over its edge address and lands on the same visitor the vendor header would have named. The audit claim about rate limits never reading proxy headers is updated rather than left to rot: it now states the condition under which they are read, and names the test that holds each half. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 17 +++ DEPLOY.md | 1 + audit/claims.yaml | 33 +++-- docs/ARCHITECTURE.md | 29 +++- internal/config/config.go | 9 ++ internal/server/middleware.go | 164 +++++++++++++++++++-- internal/server/server.go | 14 +- internal/server/trustedproxy_test.go | 207 +++++++++++++++++++++++++++ 8 files changed, 451 insertions(+), 23 deletions(-) create mode 100644 internal/server/trustedproxy_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index b4c8bb2..ce5d0d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,23 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad copied price is how a paid meeting quietly starts selling for nothing. Bookings are not copied. +- **`TRUSTED_PROXY_CIDRS`: per-IP rate limits that work behind a CDN.** Rate limits key + on the TCP peer, which is right for a directly-reachable instance and useless behind a + fronting CDN, where every visitor arrives from the same handful of addresses and shares + one bucket. List the networks you control, a fronting CDN's own ranges included, and + the client IP is taken from `X-Forwarded-For` walked right to left past those hops. + + Nothing changes if you do not set it: a header from a peer you have not listed is still + not read at all, because it is a value the client chose. Within the header the *leftmost* + entry is likewise client-chosen, so the walk stops at the rightmost address one of your + proxies actually observed, and a malformed hop ends the walk on the peer rather than + being stepped over. Repeated `X-Forwarded-For` field lines are joined in order rather + than only the first being read, so a client's own line in front of a proxy that adds a + second one cannot hide the hop that matters. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`, + `True-Client-IP`) are never read, from any peer: the setting names networks rather than + CDNs, and a plain reverse proxy in the list forwards whatever the client sent. + + ## [0.8.0] - 2026-09-03 ### Added diff --git a/DEPLOY.md b/DEPLOY.md index 81d15b1..3ad7cbb 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -35,6 +35,7 @@ This guide covers a generic Docker deploy and a step-by-step **Railway** deploy | `GOOGLE_CLIENT_ID` / `GOOGLE_CLIENT_SECRET` | no | — | Google sign-in + calendar. Can also be set in Settings → Google OAuth. | | `LITESTREAM_REPLICA_URL` | recommended | — | Enables continuous SQLite backup (see §6). | | `COOKIE_SECURE` | no | https→true | Override cookie Secure flag; defaults from `BASE_URL` scheme. | +| `TRUSTED_PROXY_CIDRS` | no | — | Comma-separated CIDRs (a bare address = one host) whose `X-Forwarded-For` is believed when keying per-IP rate limits, e.g. `10.0.0.0/8`. Include a fronting CDN's own ranges so the walk steps over its edge and lands on the visitor. Unset ⇒ the header is ignored and the limit keys on the TCP peer, so behind a CDN every visitor shares one bucket. **Only list networks you control**: anything in the list can name any client IP it likes. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`) are never read, from any peer. | | `LOG_LEVEL` | no | `info` | `debug`/`info`/`warn`/`error`. | ¹ Email is optional to boot, but bookings won't send confirmations until SMTP is configured (env **or** the admin UI). Precedence is **env var > DB setting > default**. diff --git a/audit/claims.yaml b/audit/claims.yaml index 28bca98..79aea67 100644 --- a/audit/claims.yaml +++ b/audit/claims.yaml @@ -276,25 +276,38 @@ claims: - id: rate-limit-keys-on-tcp-source-address claim: > - Per-IP rate limiting (internal/server/middleware.go's RateLimit) keys - strictly on the TCP-level remote address of the connection, never on client- - supplied X-Forwarded-For/X-Real-IP headers — a client cannot spoof those - headers to evade or split its rate-limit bucket. + Per-IP rate limiting (internal/server/middleware.go's RateLimit) keys on the + TCP-level remote address of the connection, and never on a client-supplied + X-Forwarded-For unless the peer that sent it is inside an operator-configured + TRUSTED_PROXY_CIDRS range — so a client cannot spoof headers to evade or split + its rate-limit bucket. Single-value vendor headers (X-Real-IP, + CF-Connecting-IP, True-Client-IP) are never read, from any peer. verify: - "internal/server/middleware.go's remoteIP — net.SplitHostPort(r.RemoteAddr) - only; the proxy headers are never read." + via peerIP, unless TrustClientIP has resolved a client IP into the request + context, which it only does for a peer matching a trusted CIDR." + - "internal/server/middleware.go's resolveClientIP — returns the peer outright + for an untrusted peer, before any header is read." - "internal/server/ratelimit_test.go's TestRemoteIP_* — assert X-Forwarded-For and X-Real-IP are ignored even when RemoteAddr is loopback." + - "internal/server/trustedproxy_test.go — asserts an untrusted peer's spoofed + headers are ignored, that the X-Forwarded-For walk goes right-to-left past + trusted hops (never the client-seeded leftmost entry), that a malformed + header falls back to the peer, and that a vendor header is ignored even from + a TRUSTED peer." status: verified caveat: > Recorded here because a prior Layer 2 audit pass flagged this as trusting spoofable proxy headers — it doesn't; the flagged behavior traced back to a stale doc comment describing the opposite of what the code does (fixed - alongside this entry). Correct behavior does require the deployment's - reverse proxy to connect to Calnode directly (or over a trusted private - network) — see the deployment docs for reverse-proxy requirements (forward - the original Host header, connect over a trusted path, strip client-supplied - proxy headers at the edge). + alongside this entry). TRUSTED_PROXY_CIDRS is empty by default, so an + unconfigured instance behaves exactly as this claim originally described. + Anything an operator does list can name any client IP it likes — that is what + trusting a proxy means — so the list must hold only networks they control. + Correct behavior otherwise requires the deployment's reverse proxy to connect + to Calnode directly (or over a trusted private network) — see the deployment + docs for reverse-proxy requirements (forward the original Host header, connect + over a trusted path, strip client-supplied proxy headers at the edge). - id: caldav-connect-self-service-no-admin-gate claim: > diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e81e86c..177b7bd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -694,10 +694,31 @@ as the desired state: **original `Host` header**. The CSRF same-origin check (§6) compares the request's `Origin`/`Referer` against `Host`, so a proxy that rewrites Host would *false-block admin writes* (403). Fly and Railway preserve Host by default; a hand-rolled nginx - needs `proxy_set_header Host $host;`. Related: per-IP rate limits (§8) key on the - **TCP remote address** (proxy headers like `X-Forwarded-For` are intentionally - ignored as forgeable), so behind a shared proxy the limit keys on the proxy's - connection — fine for per-instance Fly/Railway, worth knowing for a fronting proxy. + needs `proxy_set_header Host $host;`. +- **Per-IP rate limits (§8) key on the TCP remote address by default**, and + `X-Forwarded-For` / `X-Real-IP` / `CF-Connecting-IP` are not read at all. That is not + an oversight: those headers are client-chosen values, so believing them unconditionally + would let anyone split their own rate-limit bucket by sending a different one each + request. Behind a shared proxy the limit therefore keys on the proxy's connection — + fine for a per-instance Fly/Railway deploy, worth knowing for a fronting CDN. + **`TRUSTED_PROXY_CIDRS`** (comma-separated CIDRs, a bare address meaning one host) + opts in per network: for a peer inside one of those ranges, `TrustClientIP` + (`internal/server/middleware.go`) resolves the client IP by walking `X-Forwarded-For` + **right to left past trusted hops** and taking the first untrusted address, else the + peer. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`, `True-Client-IP`) + are never consulted, even from a trusted peer: the list names *networks*, not CDNs, so + an ordinary reverse proxy in it forwards whatever the client sent, and the header + itself carries nothing that says which hop wrote it. A CDN sets `X-Forwarded-For` too + and its own ranges belong in the list, so the walk reaches the same visitor without + believing a header for a reason the code cannot check. ⛔ Not the leftmost entry: the left + of that header is whatever the original client sent, and every well-behaved proxy + preserves it. A hop that does not parse ends the walk and falls back to the peer rather + than being skipped, so one malformed entry cannot push the walk onto a value the client + chose. `X-Forwarded-For` is read as **every** field line joined in order, not just the + first, because a client's own line in front of a proxy that adds a second one would + otherwise be the only thing the walk saw. Headers from an **untrusted** peer are never read, which is what keeps the + default un-weakenable by a header. Resolution happens once, in the outermost + middleware, and is carried in the request context. --- diff --git a/internal/config/config.go b/internal/config/config.go index ac96fa0..8e19f05 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -59,6 +59,14 @@ type Config struct { // the public endpoints are rate-limited regardless. Comma-separated. EmbedAllowedOrigins []string + // TrustedProxyCIDRs lists the networks whose forwarded headers are believed when + // resolving the client IP for per-IP rate limiting. Empty (the default) ⇒ the limit + // keys on the TCP peer and X-Forwarded-For is ignored entirely, because a header + // from an unvetted peer is a client-chosen value. Comma-separated CIDRs; a bare + // address is taken as a single host. A fronting CDN's own ranges belong here: the + // walk steps over its edge address and lands on the visitor. + TrustedProxyCIDRs []string + // DemoMode turns this instance into a public, self-resetting demo: seeds sample // data on every boot (there's no persistent volume, so every boot is a fresh DB), // disables calendar/Zoom connect, serves a disallow-all robots.txt, and exposes @@ -97,6 +105,7 @@ func Load() *Config { EmbedAllowedOrigins: splitCSV(getEnv("EMBED_ALLOWED_ORIGINS", "")), DataDir: getEnv("DATA_DIR", "data"), + TrustedProxyCIDRs: splitCSV(getEnv("TRUSTED_PROXY_CIDRS", "")), } cfg.EncryptionKey = os.Getenv("CALNODE_ENCRYPTION_KEY") diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 363b10e..0f5c412 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -18,6 +18,10 @@ type contextKey string const requestIDKey contextKey = "request_id" +// clientIPKey carries the client IP resolved by TrustClientIP. Absent unless +// TRUSTED_PROXY_CIDRS is configured, in which case remoteIP falls back to the peer. +const clientIPKey contextKey = "client_ip" + func RequestID(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { id := r.Header.Get("X-Request-Id") @@ -236,18 +240,162 @@ func (rl *rateLimiter) cleanup() { } } -// remoteIP returns the TCP-level remote address, stripped of its port. -// X-Real-IP and X-Forwarded-For are intentionally ignored: without a -// configured trusted-proxy allowlist, those headers can be forged by any -// client and would bypass the rate limit entirely. Operators behind a reverse -// proxy should strip proxy headers at the proxy level and rely on the TCP -// address the proxy connects with. See audit/claims.yaml's -// rate-limit-keys-on-tcp-source-address claim, recorded specifically because a -// prior Layer 2 audit pass mistook this deliberate behavior for a spoofable gap. +// remoteIP returns the IP a per-IP limit keys on. +// +// By default that is the TCP-level remote address, stripped of its port, and the +// forwarded headers are ignored: without a configured trusted-proxy allowlist those +// headers can be forged by any client and would bypass the rate limit entirely. +// Operators behind a reverse proxy should strip proxy headers at the proxy and rely on +// the TCP address the proxy connects with. See audit/claims.yaml's +// rate-limit-keys-on-tcp-source-address claim, recorded specifically because a prior +// Layer 2 audit pass mistook this deliberate behavior for a spoofable gap. +// +// When TRUSTED_PROXY_CIDRS is set, TrustClientIP has already resolved the client IP for +// this request and left it in the context — see resolveClientIP for what that means and +// what it deliberately does not do. This function reads that value when it is there, so +// the untrusted-peer path stays byte-for-byte the old behaviour. func remoteIP(r *http.Request) string { + if ip, ok := r.Context().Value(clientIPKey).(string); ok && ip != "" { + return ip + } + return peerIP(r) +} + +// peerIP returns the TCP-level remote address, stripped of its port. This is the only +// value in a request that a client cannot choose. +func peerIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { return r.RemoteAddr } return host } + +// ParseTrustedProxies parses TRUSTED_PROXY_CIDRS entries. A bare address is accepted and +// treated as a single-host range (/32 or /128), because "10.0.0.7" is what an operator +// naming one proxy will write. +// +// Every parseable entry is returned even when others fail, and the error names the ones +// that did not: one typo should cost that hop's trust, not the whole list's. The caller +// is expected to log the error — an unparsed entry means that proxy's headers are NOT +// believed, which degrades to per-proxy rate limiting rather than to trusting a forgery. +func ParseTrustedProxies(entries []string) ([]*net.IPNet, error) { + var nets []*net.IPNet + var bad []string + for _, e := range entries { + e = strings.TrimSpace(e) + if e == "" { + continue + } + if _, n, err := net.ParseCIDR(e); err == nil { + nets = append(nets, n) + continue + } + if ip := net.ParseIP(e); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + nets = append(nets, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + continue + } + bad = append(bad, e) + } + if len(bad) > 0 { + return nets, fmt.Errorf("not a CIDR or IP address: %s", strings.Join(bad, ", ")) + } + return nets, nil +} + +// TrustClientIP returns middleware that resolves the client IP once per request and +// stores it in the request context for remoteIP to read. With no trusted proxies it is a +// pass-through, so an instance that has not configured any is unchanged. +// +// It is middleware rather than a package-level setting because the trust list belongs to +// one server instance: a mutable global would leak between instances in a test binary +// and would make "which requests are affected" unanswerable from the wiring. +func TrustClientIP(trusted []*net.IPNet) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + if len(trusted) == 0 { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := context.WithValue(r.Context(), clientIPKey, resolveClientIP(r, trusted)) + next.ServeHTTP(w, r.WithContext(ctx)) + }) + } +} + +// resolveClientIP applies the forwarded headers, but only for a peer inside a trusted +// CIDR. A header from an untrusted peer is never read at all — that is the whole point, +// and it is why the default (no trusted proxies) cannot be weakened by a header. +// +// For a trusted peer the answer comes from X-Forwarded-For, walked RIGHT TO LEFT past +// trusted hops, returning the first untrusted address. ⛔ Not the leftmost entry: the +// left of that header is whatever the original client sent, so a client that pre-seeds +// "X-Forwarded-For: 1.2.3.4" gets it prepended and preserved by every well-behaved +// proxy. The rightmost non-trusted hop is the last address a trusted proxy actually +// observed, which is the only one in the header that anything vouched for. The peer is +// the answer whenever the header is absent or unusable. +// +// ⛔ Single-value vendor headers (CF-Connecting-IP, X-Real-IP, True-Client-IP) are NOT +// consulted, and that is deliberate rather than an omission. Trusting one means +// trusting it from every peer in the list, and the list is a list of *networks* rather +// than a list of CDNs: an ordinary reverse proxy inside it forwards whatever headers +// the client sent, so a client could name its own rate-limit bucket by sending one. The +// header carries nothing that says which hop set it, so there is no way to tell the +// value a CDN wrote from the value a visitor typed. +// +// Nothing is lost by leaving them out. A CDN sets X-Forwarded-For as well, and its own +// ranges belong in TRUSTED_PROXY_CIDRS anyway, so the walk steps over its edge address +// and lands on the visitor. That is the same answer the vendor header would have given, +// reached without having to believe a header for a reason the code cannot check. +// +// A hop that does not parse ends the walk and falls back to the peer rather than being +// skipped. Skipping it would let a client inject one malformed entry to push the walk +// past the real hop and onto a value it chose. Note this also means a proxy that appends +// "ip:port" (rather than a bare address) reads as malformed and lands on the peer, which +// is the safe direction to be wrong in. +func resolveClientIP(r *http.Request, trusted []*net.IPNet) string { + peer := peerIP(r) + if !ipInAny(peer, trusted) { + return peer + } + + // ⛔ Values, not Get. A header may arrive as several field lines, and Get returns + // only the FIRST. A client that sends its own "X-Forwarded-For: 1.2.3.4" followed by + // a proxy that ADDS a line rather than appending to the existing one leaves two + // lines, and Get would hand the walk the client's line alone — no trusted hop in it, + // so the walk returns the client's chosen address on its first step. RFC 9110 says + // repeated field lines are equivalent to one comma-joined value in the order + // received, so joining them is both correct and what the walk already assumes. + if xff := strings.Join(r.Header.Values("X-Forwarded-For"), ","); xff != "" { + hops := strings.Split(xff, ",") + for i := len(hops) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(hops[i])) + if ip == nil { + break + } + if ipInAny(ip.String(), trusted) { + continue // a trusted hop of our own; keep walking left + } + return ip.String() + } + } + + return peer +} + +// ipInAny reports whether ip (a textual address) falls inside any of nets. +func ipInAny(ip string, nets []*net.IPNet) bool { + parsed := net.ParseIP(ip) + if parsed == nil { + return false + } + for _, n := range nets { + if n.Contains(parsed) { + return true + } + } + return false +} diff --git a/internal/server/server.go b/internal/server/server.go index b25f7ab..460b46f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -544,7 +544,19 @@ func New(ctx context.Context, cfg *config.Config, db *sql.DB, logger *slog.Logge // permanently if a marketing landing page is ever added here. mux.Handle("GET /{$}", http.RedirectHandler("/admin/", http.StatusFound)) - return RequestID(Logging(logger, SameOriginCheck(mux))), drain + // Trusted-proxy resolution wraps everything, so the per-IP limiters and anything else + // asking for the client IP see one answer computed once. A bad CIDR is logged and + // dropped rather than fatal: the consequence is that that hop's headers are not + // believed, which costs shared rate-limit buckets, never a trusted forgery. + trustedProxies, err := ParseTrustedProxies(cfg.TrustedProxyCIDRs) + if err != nil { + logger.Error("TRUSTED_PROXY_CIDRS: ignoring unparseable entries", "error", err) + } + if len(trustedProxies) > 0 { + logger.Info("trusting forwarded headers from proxies", "cidrs", cfg.TrustedProxyCIDRs) + } + + return TrustClientIP(trustedProxies)(RequestID(Logging(logger, SameOriginCheck(mux)))), drain } // seedSMTPToDB writes env-var SMTP settings into the DB on first boot so they diff --git a/internal/server/trustedproxy_test.go b/internal/server/trustedproxy_test.go new file mode 100644 index 0000000..8792d8f --- /dev/null +++ b/internal/server/trustedproxy_test.go @@ -0,0 +1,207 @@ +package server + +import ( + "net" + "net/http" + "net/http/httptest" + "testing" +) + +// serveThroughTrust runs one request through TrustClientIP and reports what remoteIP +// resolved to inside the handler — the value a rate limiter would key on. +func serveThroughTrust(t *testing.T, cidrs []string, peer string, headers map[string]string) string { + t.Helper() + trusted, err := ParseTrustedProxies(cidrs) + if err != nil { + t.Fatalf("ParseTrustedProxies(%v): %v", cidrs, err) + } + var got string + h := TrustClientIP(trusted)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = remoteIP(r) + })) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = peer + for k, v := range headers { + req.Header.Set(k, v) + } + h.ServeHTTP(httptest.NewRecorder(), req) + return got +} + +// The default. An untrusted peer's forwarded headers are not read at all, so the spoof +// keys on the connection the spoofer actually made. +func TestTrustClientIP_untrustedPeerIgnoresSpoofedHeaders(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "203.0.113.9:1234", map[string]string{ + "X-Forwarded-For": "198.51.100.1", + "CF-Connecting-IP": "198.51.100.2", + "X-Real-IP": "198.51.100.3", + }) + if got != "203.0.113.9" { + t.Errorf("client IP = %q; want the peer 203.0.113.9", got) + } +} + +// With no trusted proxies configured at all, TrustClientIP is a pass-through and the +// behaviour is exactly what it was before the setting existed. +func TestTrustClientIP_noTrustedProxiesIsUnchanged(t *testing.T) { + got := serveThroughTrust(t, nil, "10.0.0.5:1234", map[string]string{ + "X-Forwarded-For": "198.51.100.1", + }) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +// ⛔ Single-value vendor headers are ignored even from a TRUSTED peer, and this is the +// case that says why. Trusting one means trusting it from every network in the list, +// and an ordinary reverse proxy in that list forwards whatever headers the client sent. +// If CF-Connecting-IP were preferred, the client below would have chosen its own +// rate-limit key by sending one header, which is exactly the spoof the right-to-left +// walk exists to prevent. +// +// The chain is what is believed: 10.0.0.9 is one of ours, so the walk steps over it and +// stops at 198.51.100.7, the address our outermost proxy actually observed. +func TestTrustClientIP_ignoresVendorHeadersFromATrustedPeer(t *testing.T) { + for _, header := range []string{"CF-Connecting-IP", "X-Real-IP", "True-Client-IP"} { + t.Run(header, func(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", map[string]string{ + header: "1.2.3.4", + "X-Forwarded-For": "198.51.100.7, 10.0.0.9", + }) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 from the chain, never %s", got, header) + } + }) + } +} + +// And with no X-Forwarded-For to fall back on, a vendor header still buys the client +// nothing: the answer is the peer, not the address the client named. +func TestTrustClientIP_vendorHeaderAloneFallsBackToThePeer(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"CF-Connecting-IP": "1.2.3.4"}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +// ⛔ X-Forwarded-For can arrive as SEVERAL field lines, and Header.Get returns only the +// first one. +// +// A client that sends its own X-Forwarded-For, in front of a proxy that ADDS a line +// rather than appending to the existing one, produces exactly this: line 1 is the +// client's, line 2 is the proxy's. Reading only line 1 hands the walk a chain with no +// trusted hop in it, so it returns the client's chosen address on the first step and the +// client has named its own rate-limit bucket. RFC 9110 makes repeated field lines +// equivalent to one comma-joined value in order, which is what the walk needs. +// +// This uses Add rather than the shared helper on purpose: the helper calls Set, which +// collapses everything to one line and cannot express the case. +func TestTrustClientIP_joinsRepeatedForwardedForLines(t *testing.T) { + trusted, err := ParseTrustedProxies([]string{"10.0.0.0/8"}) + if err != nil { + t.Fatalf("ParseTrustedProxies: %v", err) + } + var got string + h := TrustClientIP(trusted)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = remoteIP(r) + })) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.5:1234" + req.Header.Add("X-Forwarded-For", "1.2.3.4") // the client's own line + req.Header.Add("X-Forwarded-For", "198.51.100.7, 10.0.0.9") // what our proxies observed + h.ServeHTTP(httptest.NewRecorder(), req) + + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 — with only the first field line read, "+ + "the client's own 1.2.3.4 becomes the rate-limit key", got) + } +} + +// Two of our own hops appended themselves; the walk goes right to left past both and +// stops at the address the outermost trusted proxy actually saw. +func TestTrustClientIP_walksTrustedChainRightToLeft(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8", "192.168.0.0/16"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "198.51.100.7, 192.168.1.1, 10.0.0.9"}) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7", got) + } +} + +// The leftmost entry is client-supplied. A client that pre-seeds the header must not be +// able to choose its own key by putting a value to the left of the real one. +func TestTrustClientIP_ignoresEntriesLeftOfTheRealHop(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "1.2.3.4, 198.51.100.7, 10.0.0.9"}) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 (not the client-seeded 1.2.3.4)", got) + } +} + +func TestTrustClientIP_malformedHeaderFallsBackToPeer(t *testing.T) { + cases := map[string]string{ + "garbage": "not-an-ip", + "empty entries": " , ,", + "address with port": "198.51.100.7:443", + "malformed left hop": "not-an-ip, 10.0.0.9", + } + for name, xff := range cases { + t.Run(name, func(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": xff}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } + }) + } +} + +// A chain of nothing but our own proxies has no client address in it. The peer is the +// only honest answer left. +func TestTrustClientIP_allHopsTrustedFallsBackToPeer(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "10.0.0.7, 10.0.0.9"}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +func TestTrustClientIP_ipv6(t *testing.T) { + // The proxies sit in 2001:db8:0::/48; the client is one prefix over, so the walk + // skips the trusted hop and stops on it. + got := serveThroughTrust(t, []string{"2001:db8:0::/48"}, "[2001:db8::1]:1234", + map[string]string{"X-Forwarded-For": "2001:db8:1::5, 2001:db8::9"}) + if got != "2001:db8:1::5" { + t.Errorf("client IP = %q; want 2001:db8:1::5", got) + } +} + +func TestParseTrustedProxies(t *testing.T) { + // A bare address is what an operator naming one proxy writes. + nets, err := ParseTrustedProxies([]string{"10.0.0.7", "192.168.0.0/16", "2001:db8::1"}) + if err != nil { + t.Fatalf("err = %v; want nil", err) + } + if len(nets) != 3 { + t.Fatalf("parsed %d entries; want 3", len(nets)) + } + if !nets[0].Contains(net.ParseIP("10.0.0.7")) || nets[0].Contains(net.ParseIP("10.0.0.8")) { + t.Errorf("bare IPv4 %v should be exactly one host", nets[0]) + } + if !nets[2].Contains(net.ParseIP("2001:db8::1")) || nets[2].Contains(net.ParseIP("2001:db8::2")) { + t.Errorf("bare IPv6 %v should be exactly one host", nets[2]) + } +} + +// One typo costs that hop's trust, not the whole list's — and it is reported rather than +// swallowed, because an operator who thinks a proxy is trusted and is wrong gets shared +// rate-limit buckets with no explanation. +func TestParseTrustedProxies_reportsBadEntriesAndKeepsTheRest(t *testing.T) { + nets, err := ParseTrustedProxies([]string{"10.0.0.0/8", "10.0.0.0/99", "nonsense"}) + if err == nil { + t.Fatal("err = nil; want the bad entries named") + } + if len(nets) != 1 { + t.Errorf("parsed %d entries; want the 1 good one kept", len(nets)) + } +}