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.

.gitea/workflows/ci.yml

@@ -0,0 +1,36 @@
+name: CI
+
+on:
+  push:
+  pull_request:
+
+jobs:
+  ci:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      - uses: actions/setup-go@v5
+        with:
+          go-version: '1.26'
+
+      - name: gofmt
+        run: test -z "$(gofmt -l .)"
+
+      - run: go build ./...
+
+      - run: go vet ./...
+
+      - run: go test ./...
+
+      - uses: actions/setup-node@v4
+        with:
+          node-version: '22'
+
+      - uses: pnpm/action-setup@v4
+        with:
+          version: '12'
+
+      - name: web check
+        working-directory: web
+        run: pnpm install --frozen-lockfile && pnpm check

cmd/vellum/main.go

@@ -88,6 +88,7 @@ func run(cfg core.Config, log *slog.Logger) error {
 	api := httpapi.New(db, log, version)
 
 	authenticator := auth.NewAuthenticator(db, cfg.CookieName, cfg.SessionTTL, cfg.CookieSecure, log)
+	authenticator.TrustedProxies = cfg.TrustedProxies
 	if !cfg.CookieSecure {
 		log.Warn("VELLUM_COOKIE_SECURE is false; session cookies will be sent over plain HTTP. " +
 			"Set it to true when serving over TLS.")

internal/auth/clientip_test.go

@@ -0,0 +1,118 @@
+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)
+			}
+		})
+	}
+}

internal/auth/handlers.go

@@ -83,7 +83,7 @@ func (h *Handlers) handleLogin(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	ip := ClientIP(r)
+	ip := h.auth.ClientIP(r)
 	if !h.limiter.allow(ip + "|" + req.Username) {
 		h.log.Warn("login rate limited", "ip", ip, "username", req.Username)
 		httpx.WriteError(w, http.StatusTooManyRequests, "too many login attempts, try again later")

internal/auth/middleware.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"net"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/mithral/vellum/internal/httpx"
@@ -33,6 +34,9 @@ type Authenticator struct {
 	CookieTTL  time.Duration
 	Secure     bool
 	Log        *slog.Logger
+	// TrustedProxies are the networks allowed to name the real client in
+	// X-Forwarded-For. Empty means trust nobody. Set from cfg.TrustedProxies.
+	TrustedProxies []*net.IPNet
 
 	// touchEvery is the minimum age of a session's last_seen before Middleware
 	// will write it again. A zero value means "touch on every request" (the
@@ -182,12 +186,49 @@ func (a *Authenticator) TokenFromRequest(r *http.Request) (string, error) {
 }
 
 // ClientIP extracts a best-effort client address for logging and rate limiting.
-// It deliberately does NOT trust X-Forwarded-For: behind an untrusted network
+//
-// that header is attacker-controlled, and this server is LAN-facing.
+// Behind a reverse proxy every request arrives from the proxy, so RemoteAddr
-func ClientIP(r *http.Request) string {
+// alone keys the login limiter identically for every client on the internet:
+// one attacker locks out a username for everyone, and sessions.ip records the
+// proxy. X-Forwarded-For fixes that, but only for a peer we already trust —
+// from an untrusted peer that header is attacker-controlled, so it is ignored
+// outright and the connection's own address is used, as before.
+//
+// With a trusted peer the chain is walked right-to-left and the first address
+// that is not itself a trusted proxy wins: everything to its right is a hop we
+// control, and everything to its left was appended by a client we do not. If
+// every hop is trusted the leftmost entry is the best available answer.
+func (a *Authenticator) ClientIP(r *http.Request) string {
 	host, _, err := net.SplitHostPort(r.RemoteAddr)
 	if err != nil {
-		return r.RemoteAddr
+		host = r.RemoteAddr
 	}
+	if !a.trusts(host) {
 		return host
+	}
+	chain := r.Header.Get("X-Forwarded-For")
+	if chain == "" {
+		return host
+	}
+	hops := strings.Split(chain, ",")
+	for i := len(hops) - 1; i >= 0; i-- {
+		if hop := strings.TrimSpace(hops[i]); !a.trusts(hop) {
+			return hop
+		}
+	}
+	return strings.TrimSpace(hops[0])
+}
+
+// trusts reports whether ip falls inside one of the configured proxy networks.
+func (a *Authenticator) trusts(ip string) bool {
+	parsed := net.ParseIP(ip)
+	if parsed == nil {
+		return false
+	}
+	for _, n := range a.TrustedProxies {
+		if n.Contains(parsed) {
+			return true
+		}
+	}
+	return false
 }

internal/core/config.go

@@ -2,6 +2,7 @@ package core
 
 import (
 	"fmt"
+	"net"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -44,6 +45,15 @@ type Config struct {
 	CookieName   string
 	CookieSecure bool
 
+	// PublicURL is the external origin Vellum is reached at, e.g.
+	// "https://media.example.com". Empty means "not published", which is the
+	// case for plain-HTTP LAN use. It is the signal that the deployment sits
+	// behind a TLS-terminating proxy, so it also drives the CookieSecure default.
+	PublicURL string
+	// TrustedProxies are the networks allowed to speak for a client in
+	// X-Forwarded-For. Empty means trust nobody: only RemoteAddr is used.
+	TrustedProxies []*net.IPNet
+
 	SessionTTL    time.Duration
 	ReadTimeout   time.Duration
 	WriteTimeout  time.Duration
@@ -54,6 +64,7 @@ type Config struct {
 // LoadConfig reads configuration from the environment and fills in defaults.
 // It does not create directories; call Validate for that.
 func LoadConfig() (Config, error) {
+	publicURL := env("VELLUM_PUBLIC_URL", "")
 	c := Config{
 		Listen:        env("VELLUM_LISTEN", ":7788"),
 		DataDir:       env("VELLUM_DATA_DIR", "./data"),
@@ -69,10 +80,11 @@ func LoadConfig() (Config, error) {
 		IGDBClientID:     env("VELLUM_IGDB_CLIENT_ID", ""),
 		IGDBClientSecret: env("VELLUM_IGDB_CLIENT_SECRET", ""),
 		CookieName:       env("VELLUM_COOKIE_NAME", "vellum_session"),
-		// Secure defaults to false so plain-HTTP development works. Production
+		// Secure defaults to true for an https:// PublicURL — a published origin
-		// behind TLS must set VELLUM_COOKIE_SECURE=true; boot logs a warning
+		// is TLS-terminated, so plain-HTTP dev still gets false with it unset.
-		// when it is off.
+		// VELLUM_COOKIE_SECURE overrides both ways; boot logs a warning when off.
-		CookieSecure:  envBool("VELLUM_COOKIE_SECURE", false),
+		CookieSecure:  envBool("VELLUM_COOKIE_SECURE", strings.HasPrefix(publicURL, "https://")),
+		PublicURL:     publicURL,
 		SessionTTL:    envDuration("VELLUM_SESSION_TTL", 30*24*time.Hour),
 		ReadTimeout:   envDuration("VELLUM_READ_TIMEOUT", 15*time.Second),
 		WriteTimeout:  envDuration("VELLUM_WRITE_TIMEOUT", 0), // 0 = no limit; streaming needs this
@@ -80,6 +92,12 @@ func LoadConfig() (Config, error) {
 		ShutdownGrace: envDuration("VELLUM_SHUTDOWN_GRACE", 15*time.Second),
 	}
 
+	proxies, err := envCIDRs("VELLUM_TRUSTED_PROXIES")
+	if err != nil {
+		return Config{}, err
+	}
+	c.TrustedProxies = proxies
+
 	abs, err := filepath.Abs(c.DataDir)
 	if err != nil {
 		return Config{}, fmt.Errorf("resolve data dir: %w", err)
@@ -149,6 +167,25 @@ func envList(key string, def []string) []string {
 	return out
 }
 
+// envCIDRs parses a comma-separated CIDR list, e.g.
+// "172.16.0.0/12,127.0.0.1/32". A malformed entry is an error rather than a
+// silent skip: dropping a proxy network quietly widens the trust boundary,
+// which is exactly the failure this setting exists to prevent.
+func envCIDRs(key string) ([]*net.IPNet, error) {
+	var out []*net.IPNet
+	for _, part := range strings.Split(env(key, ""), ",") {
+		if part = strings.TrimSpace(part); part == "" {
+			continue
+		}
+		_, n, err := net.ParseCIDR(part)
+		if err != nil {
+			return nil, fmt.Errorf("%s: %w", key, err)
+		}
+		out = append(out, n)
+	}
+	return out, nil
+}
+
 func envDuration(key string, def time.Duration) time.Duration {
 	v, ok := os.LookupEnv(key)
 	if !ok || v == "" {

internal/core/config_test.go

@@ -1,6 +1,7 @@
 package core
 
 import (
+	"net"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -50,6 +51,72 @@ func TestLoadConfigFromEnv(t *testing.T) {
 	}
 }
 
+func TestLoadConfigTrustedProxies(t *testing.T) {
+	t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, 127.0.0.1/32")
+
+	cfg, err := LoadConfig()
+	if err != nil {
+		t.Fatalf("LoadConfig: %v", err)
+	}
+	if len(cfg.TrustedProxies) != 2 {
+		t.Fatalf("TrustedProxies = %v, want 2 entries", cfg.TrustedProxies)
+	}
+	if !cfg.TrustedProxies[0].Contains(net.ParseIP("172.20.0.5")) {
+		t.Errorf("TrustedProxies[0] = %v, want it to contain 172.20.0.5", cfg.TrustedProxies[0])
+	}
+	if cfg.TrustedProxies[1].Contains(net.ParseIP("172.20.0.5")) {
+		t.Errorf("TrustedProxies[1] = %v, want it to contain only 127.0.0.1", cfg.TrustedProxies[1])
+	}
+}
+
+func TestLoadConfigRejectsBadTrustedProxy(t *testing.T) {
+	// A malformed entry must fail the boot: silently dropping it would quietly
+	// narrow the trust boundary and the operator would never know.
+	t.Setenv("VELLUM_TRUSTED_PROXIES", "172.16.0.0/12, not-a-cidr")
+
+	_, err := LoadConfig()
+	if err == nil {
+		t.Fatal("expected error for malformed VELLUM_TRUSTED_PROXIES")
+	}
+	if !strings.Contains(err.Error(), "not-a-cidr") {
+		t.Errorf("error %q should name the offending entry", err)
+	}
+}
+
+func TestCookieSecureDefaultsFromPublicURL(t *testing.T) {
+	tests := []struct {
+		name   string
+		public string
+		cookie string // empty means unset
+		want   bool
+	}{
+		{name: "https origin secures the cookie", public: "https://media.example.com", want: true},
+		{name: "http origin leaves it off", public: "http://media.example.com", want: false},
+		{name: "unset origin leaves it off", want: false},
+		{name: "explicit false overrides https", public: "https://media.example.com", cookie: "false", want: false},
+		{name: "explicit true overrides http", public: "http://media.example.com", cookie: "true", want: true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Setenv("VELLUM_PUBLIC_URL", tc.public)
+			// An empty value reads as unset to the env helpers, and t.Setenv
+			// restores the ambient value either way.
+			t.Setenv("VELLUM_COOKIE_SECURE", tc.cookie)
+
+			cfg, err := LoadConfig()
+			if err != nil {
+				t.Fatalf("LoadConfig: %v", err)
+			}
+			if cfg.PublicURL != tc.public {
+				t.Errorf("PublicURL = %q, want %q", cfg.PublicURL, tc.public)
+			}
+			if cfg.CookieSecure != tc.want {
+				t.Errorf("CookieSecure = %v, want %v", cfg.CookieSecure, tc.want)
+			}
+		})
+	}
+}
+
 func TestValidateRejectsMissingMediaDir(t *testing.T) {
 	cfg := Config{
 		DataDir:   t.TempDir(),

internal/core/migrate.go

@@ -31,6 +31,23 @@ func Migrate(ctx context.Context, db *sql.DB, fsys fs.FS, log *slog.Logger) erro
 	}
 	log.Debug("schema version", "current", current, "available", len(names))
 
+	highest := 0
+	for _, name := range names {
+		n, err := migrationNumber(name)
+		if err != nil {
+			return err
+		}
+		if n > highest {
+			highest = n
+		}
+	}
+	// A version above every embedded migration means this binary is older than
+	// the database. Without this, the loop below skips everything and starts up
+	// against a schema it cannot read.
+	if current > highest {
+		return fmt.Errorf("database schema is version %d but this binary only knows %d: this binary is older than the database — upgrade the binary or restore a backup", current, highest)
+	}
+
 	for _, name := range names {
 		n, err := migrationNumber(name)
 		if err != nil {

internal/core/migrate_test.go

@@ -3,6 +3,7 @@ package core
 import (
 	"context"
 	"database/sql"
+	"fmt"
 	"io"
 	"io/fs"
 	"log/slog"
@@ -104,6 +105,25 @@ func TestForeignKeyEnforcementIsOn(t *testing.T) {
 	}
 }
 
+func TestMigrateRefusesDatabaseNewerThanBinary(t *testing.T) {
+	ctx := context.Background()
+	db := openTempDB(t)
+
+	// A database migrated by a newer binary.
+	newer := highestMigrationNumber(t) + 1
+	if _, err := db.ExecContext(ctx, fmt.Sprintf("PRAGMA user_version = %d", newer)); err != nil {
+		t.Fatalf("set user_version: %v", err)
+	}
+
+	err := Migrate(ctx, db, migrations.FS, discardLogger())
+	if err == nil {
+		t.Fatal("expected error when user_version is above the highest embedded migration, got nil")
+	}
+	if !strings.Contains(err.Error(), fmt.Sprintf("version %d", newer)) {
+		t.Errorf("error should name the database version, got: %v", err)
+	}
+}
+
 func TestMigrationNumber(t *testing.T) {
 	cases := map[string]int{
 		"0001_core.sql":       1,