From a762dc712d125039f1764914009a898b584a7d3f Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Sun, 19 Jul 2026 08:56:34 +0200 Subject: [PATCH] fix(api): keep per-client keys distinct under broad trusted_proxies (W2-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With trusted_proxies covering client networks (e.g. 10.0.0.0/8 over LAN clients), the right-to-left XFF walk skipped every entry, exhausted, and fell back to the proxy's RemoteAddr — collapsing all clients into one rate-limit/lockout bucket, so one user's failed logins locked out everyone. On exhaustion the walk now returns the leftmost valid entry (furthest-upstream hop), the best distinct per-client key such a config allows. An untrusted RemoteAddr still never gets its headers honoured. Also (W3-3): the CIDR list parses once per request instead of once per XFF candidate, config load warns about invalid CIDR entries at startup (a silently skipped entry silently un-trusts the proxy), and the sample config documents that trusted_proxies must list only proxy hops. Co-Authored-By: Claude Fable 5 --- Server/api/middleware.go | 51 +++++++++++++++++++++++++++++++++++++--- Server/config/config.go | 23 +++++++++++++++++- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/Server/api/middleware.go b/Server/api/middleware.go index 054d16d8..b270d4d5 100644 --- a/Server/api/middleware.go +++ b/Server/api/middleware.go @@ -205,8 +205,12 @@ func clientIPWithProxies(r *http.Request, trustedCIDRs []string) string { return remoteHost } - trusted, _ := isTrustedProxy(remoteHost, trustedCIDRs) - if !trusted { + // Parse the CIDR list once per request instead of once per XFF candidate. + // ponytail: parse at middleware construction if this ever shows in a + // profile — it would mean threading a parsed type through every caller. + nets := parseCIDRList(trustedCIDRs) + + if !ipInNets(remoteHost, nets) { return remoteHost } @@ -226,21 +230,62 @@ func clientIPWithProxies(r *http.Request, trustedCIDRs []string) string { // letting it forge per-IP rate-limit and lockout keys. if xff := r.Header.Get("X-Forwarded-For"); xff != "" { parts := strings.Split(xff, ",") + leftmostValid := "" for i := len(parts) - 1; i >= 0; i-- { candidate := strings.TrimSpace(parts[i]) if candidate == "" || net.ParseIP(candidate) == nil { continue } - if trusted, _ := isTrustedProxy(candidate, trustedCIDRs); trusted { + leftmostValid = candidate + if ipInNets(candidate, nets) { continue // our own proxy hop, keep walking left } return candidate } + // Every entry fell inside trustedCIDRs — a config that covers client + // networks too (e.g. trusted_proxies: 10.0.0.0/8 with LAN clients). + // Falling back to RemoteAddr here would collapse ALL clients behind + // the proxy into one rate-limit/lockout bucket, so one user's failed + // logins would lock out everyone. The leftmost valid entry is the + // furthest-upstream hop — the best distinct per-client key available + // under such a config. trusted_proxies must list only proxy hops; + // startup validation warns about entries that cannot be proxies. + if leftmostValid != "" { + return leftmostValid + } } return remoteHost } +// parseCIDRList parses CIDR strings, silently skipping invalid entries — a +// misconfigured entry must not crash request handling (config load warns +// about them at startup). +func parseCIDRList(cidrs []string) []*net.IPNet { + nets := make([]*net.IPNet, 0, len(cidrs)) + for _, c := range cidrs { + if _, n, err := net.ParseCIDR(c); err == nil { + nets = append(nets, n) + } + } + return nets +} + +// ipInNets reports whether ipStr (a plain IP, no port) falls inside any of +// the parsed networks. +func ipInNets(ipStr string, nets []*net.IPNet) bool { + ip := net.ParseIP(ipStr) + if ip == nil { + return false + } + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false +} + // isTrustedProxy reports whether remoteIP (a plain IP string, no port) falls // within any of the provided CIDR ranges. It returns an error if any CIDR is // malformed. diff --git a/Server/config/config.go b/Server/config/config.go index 69a32c37..2bd99919 100644 --- a/Server/config/config.go +++ b/Server/config/config.go @@ -6,6 +6,7 @@ import ( "encoding/hex" "fmt" "log/slog" + "net" "os" "strings" @@ -215,7 +216,10 @@ server: name: "OwnCord Server" data_dir: "data" # allowed_origins: [] # empty = deny cross-origin; set to ["*"] for dev or specific origins for prod - # trusted_proxies: [] # CIDRs of trusted reverse proxies, e.g. ["10.0.0.0/8"] + # trusted_proxies: [] # CIDRs of the reverse-proxy HOPS only (e.g. ["10.0.0.2/32"]). + # # Never list client networks here: a range that covers + # # clients degrades per-client rate limiting and lets + # # covered clients influence their own rate-limit key. # admin_allowed_cidrs: # CIDRs allowed to access /admin (default: private networks only) # - "127.0.0.0/8" # - "::1/128" @@ -349,9 +353,26 @@ func Load(cfgPath string) (*Config, error) { cfg.Voice.LiveKitAPISecret = "" } + // Invalid CIDR entries are skipped at request time (they must not crash + // handling), which silently un-trusts a misconfigured proxy — warn once + // at startup instead. Common mistake: a bare IP without the /32 mask. + warnInvalidCIDRs("server.trusted_proxies", cfg.Server.TrustedProxies) + warnInvalidCIDRs("server.admin_allowed_cidrs", cfg.Server.AdminAllowedCIDRs) + return &cfg, nil } +// warnInvalidCIDRs logs a startup warning for each list entry that is not +// valid CIDR notation. +func warnInvalidCIDRs(key string, cidrs []string) { + for _, c := range cidrs { + if _, _, err := net.ParseCIDR(c); err != nil { + slog.Warn("config: ignoring invalid CIDR entry (use address/prefix notation, e.g. 10.0.0.1/32)", + "key", key, "entry", c) + } + } +} + // defaultLiveKitAPIKey and defaultLiveKitAPISecret are the well-known dev // credentials that ship in the default config. They must never be used in // production — NewLiveKitClient rejects them.