Vellum/internal/auth/clientip_test.go
mithral 87c17d4854
Some checks failed
CI / ci (push) Has been cancelled
Make the login limiter correct behind a reverse proxy
ClientIP used r.RemoteAddr and ignored X-Forwarded-For. That was right
about not trusting the header from an untrusted peer, but it is only half
the design: behind a proxy every request arrives from the proxy, so the
limiter key in handlers.go was "<proxy-ip>|<username>" for every client on
the internet. One attacker locked out a username for everyone, an attacker
rotating usernames never hit a limit, and sessions.ip recorded the proxy
so the device list was useless.

- VELLUM_TRUSTED_PROXIES is a CIDR list. X-Forwarded-For is honoured only
  when RemoteAddr is inside it, and the chain is walked right-to-left so
  the first address that is not itself a trusted proxy wins. Everything to
  its left was appended by a client we do not control, so a spoofed
  leftmost entry is discarded.
- VELLUM_PUBLIC_URL is the external origin. CookieSecure now defaults to
  true when it is https://, so a TLS deployment that forgets
  VELLUM_COOKIE_SECURE no longer ships a plaintext session cookie.
- A malformed CIDR fails the boot rather than being skipped: silently
  dropping a proxy network widens the trust boundary, which is the exact
  failure this setting exists to prevent.

ClientIP became a method on Authenticator so it can see the trusted set,
which is why handlers.go and main.go changed as well.

Verified red-green: neutering the trusted-peer path fails five ClientIP
tests, including the spoofed-leftmost case.

Known gap, pre-existing and not fixed here: this limiter still covers only
/api/v1/auth/login. Jellyfin AuthenticateByName and Subsonic auth call
auth.Authenticate with no limiting, so the same brute force is available
through those two surfaces.
2026-10-12 00:45:27 -04:00

118 lines
3.3 KiB
Go

package auth
import (
"net"
"net/http"
"net/http/httptest"
"testing"
)
// clientIPFor builds an Authenticator trusting the given CIDRs and returns the
// address it derives from a request arriving from peer with header XFF.
func clientIPFor(t *testing.T, peer, xff string, trusted ...string) string {
t.Helper()
a := &Authenticator{}
for _, c := range trusted {
_, n, err := net.ParseCIDR(c)
if err != nil {
t.Fatalf("parse test CIDR %q: %v", c, err)
}
a.TrustedProxies = append(a.TrustedProxies, n)
}
r := httptest.NewRequest(http.MethodGet, "/", nil)
r.RemoteAddr = peer
if xff != "" {
r.Header.Set("X-Forwarded-For", xff)
}
return a.ClientIP(r)
}
func TestClientIPUntrustedPeerIgnoresForwardedFor(t *testing.T) {
// The security case: an untrusted peer sending X-Forwarded-For must not be
// believed, or anyone could evade (or poison) the login rate limiter.
got := clientIPFor(t, "203.0.113.9:44321", "1.2.3.4", "172.16.0.0/12")
if got != "203.0.113.9" {
t.Errorf("ClientIP = %q, want the peer 203.0.113.9", got)
}
}
func TestClientIPTrustedPeerUsesForwardedFor(t *testing.T) {
// The reverse-proxy case: every request arrives from the proxy, so without
// the header every client shares one limiter key.
got := clientIPFor(t, "172.20.0.5:51000", "1.2.3.4", "172.16.0.0/12")
if got != "1.2.3.4" {
t.Errorf("ClientIP = %q, want the client 1.2.3.4 from the header", got)
}
}
func TestClientIPChain(t *testing.T) {
tests := []struct {
name string
peer string
xff string
trusted []string
want string
}{
{
name: "no trusted proxies at all",
peer: "172.20.0.5:51000", xff: "1.2.3.4",
want: "172.20.0.5",
},
{
name: "trusted peer without header",
peer: "172.20.0.5:51000",
trusted: []string{"172.16.0.0/12"},
want: "172.20.0.5",
},
{
name: "spans proxies on the right of the client",
peer: "172.20.0.5:51000", xff: "1.2.3.4, 172.18.0.1",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
// A client-supplied leftmost entry is attacker-controlled, so the
// walk must stop at the first untrusted hop counting from the right.
name: "ignores a spoofed leftmost entry",
peer: "172.20.0.5:51000", xff: "9.9.9.9, 1.2.3.4, 172.18.0.1",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
name: "every hop trusted falls back to the leftmost",
peer: "172.20.0.5:51000", xff: "172.19.0.1, 172.18.0.1, 172.17.0.1",
trusted: []string{"172.16.0.0/12"},
want: "172.19.0.1",
},
{
name: "multiple trusted networks",
peer: "127.0.0.1:51000", xff: "1.2.3.4",
trusted: []string{"172.16.0.0/12", "127.0.0.1/32"},
want: "1.2.3.4",
},
{
name: "unparseable hop is not trusted",
peer: "172.20.0.5:51000", xff: "not-an-ip",
trusted: []string{"172.16.0.0/12"},
want: "not-an-ip",
},
{
name: "remote address without a port",
peer: "172.20.0.5", xff: "1.2.3.4",
trusted: []string{"172.16.0.0/12"},
want: "1.2.3.4",
},
{
name: "untrusted peer without a port",
peer: "203.0.113.9", xff: "1.2.3.4",
want: "203.0.113.9",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if got := clientIPFor(t, tc.peer, tc.xff, tc.trusted...); got != tc.want {
t.Errorf("ClientIP = %q, want %q", got, tc.want)
}
})
}
}