bvandeusen
381e9cedb7
Fixes the defect the operator spotted in #370 immediately after it shipped: auth.ClientIP ignored X-Forwarded-For whenever RemoteAddr was public, so a proxy on a public address — a separate host, or a CDN, i.e. anyone running this publicly, since public means TLS means a proxy — recorded the PROXY for every session. created_ip and last_ip were then always equal and the "Address changed" signal could never fire. The feature looked like it worked and reported nothing. Replaced with the standard trusted-hop model (Rails, Caddy, Traefik, nginx). XFF grows left-to-right as each proxy appends the peer it received from, so for client -> CDN -> own-proxy -> app the app sees [client, CDN] with RemoteAddr = own-proxy, and the client sits at XFF[len - hops]: 0 RemoteAddr, XFF ignored — no proxy 1 the address your own proxy observed 2 through a CDN in front of your proxy Default 1, per the operator: publicly reachable means a TLS terminator in front. The cost is real and stated rather than hidden. hops >= 1 DECLARES that a proxy exists; set it with no proxy, or deeper than the actual chain, and the index reaches attacker-supplied entries, letting a visitor choose which address their own session shows — defeating exactly the detection #370 is for. That's inherent to the model, which is why 0 is a first-class value and the admin card says "count your proxies, don't guess high" instead of just exposing a number. Both mis-set shapes are pinned by tests so they stay known consequences rather than surprises. Migration 0053 + internal/netsettings, cached under an RWMutex. That's not an optimisation: ClientIP runs in RequireUser for every authenticated request, so a per-request query would put the database on the critical path of the whole API. New() always returns a usable service so a boot-time DB hiccup degrades to the default instead of breaking that path (rule #131), and Hops() is nil-safe because test routers construct middleware without it. RequireUser now takes a func() int rather than an int — the value is operator-editable at runtime while the middleware is built once at boot, and reading it per request is what makes a save take effect with no restart (rule #25). The admin card is verifiable, not just configurable: it reports the address the CURRENT setting resolves THIS request to, the raw forwarded chain, and the socket peer — so you set the number, save, and confirm the address matches the machine you're on. It also counts the arriving chain and says how many proxies that implies. GET/PUT both return that payload, PUT recomputed under the new value, so the effect is visible without a reload. Also fixes styling in the #370 card that CI could not catch: text-destructive and bg-destructive don't exist in this Tailwind config — the palette is colors.action.destructive — so the "Address changed" warning and the sign-out-others button were rendering unstyled. Both now use text-action-destructive / bg-action-destructive / text-action-fg. Not done here: requestlog.go still logs raw RemoteAddr and will disagree with the sessions UI about who connected. Left for its own change.
56 lines
1.8 KiB
Go
56 lines
1.8 KiB
Go
package netsettings
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"testing"
|
|
)
|
|
|
|
// A nil service reaches middleware in test routers and anywhere the settings
|
|
// aren't wired. It must read as "trust nothing" rather than panic — the
|
|
// alternative is a nil dereference inside RequireUser, on every request.
|
|
func TestHops_NilServiceTrustsNothing(t *testing.T) {
|
|
var s *Service
|
|
if got := s.Hops(); got != 0 {
|
|
t.Errorf("(*Service)(nil).Hops() = %d, want 0", got)
|
|
}
|
|
}
|
|
|
|
func TestNew_NilPoolYieldsDefault(t *testing.T) {
|
|
s, err := New(context.Background(), nil, nil)
|
|
if err != nil {
|
|
t.Fatalf("New with nil pool: %v", err)
|
|
}
|
|
if s == nil {
|
|
t.Fatal("New returned nil service")
|
|
}
|
|
if got := s.Hops(); got != DefaultTrustedProxyHops {
|
|
t.Errorf("Hops() = %d, want %d", got, DefaultTrustedProxyHops)
|
|
}
|
|
}
|
|
|
|
// Range is rejected before the query so the API answers 400 rather than
|
|
// surfacing a CHECK violation as a 500.
|
|
func TestSetHops_RejectsOutOfRange(t *testing.T) {
|
|
s, _ := New(context.Background(), nil, nil)
|
|
for _, hops := range []int{-1, MaxTrustedProxyHops + 1, 999} {
|
|
if err := s.SetHops(context.Background(), hops); !errors.Is(err, ErrHopsOutOfRange) {
|
|
t.Errorf("SetHops(%d) error = %v, want ErrHopsOutOfRange", hops, err)
|
|
}
|
|
}
|
|
}
|
|
|
|
// In-range values with no pool must still fail, and must not mutate the
|
|
// cache — a write that didn't persist reporting success would leave the
|
|
// running process disagreeing with the database.
|
|
func TestSetHops_NoPoolFailsWithoutMutatingCache(t *testing.T) {
|
|
s, _ := New(context.Background(), nil, nil)
|
|
before := s.Hops()
|
|
if err := s.SetHops(context.Background(), 2); err == nil {
|
|
t.Error("SetHops with nil pool returned nil error")
|
|
}
|
|
if after := s.Hops(); after != before {
|
|
t.Errorf("cache changed from %d to %d despite a failed write", before, after)
|
|
}
|
|
}
|