fix(security): make the client IP trustworthy and scope credential lockout to it

.env.example

@@ -9,6 +9,17 @@ PORT=8083
 # COMPANY_LOGO_URL="/images/logo.svg"
 # AGENT_DESK_SERVER_CORS_ALLOWEDORIGINS="http://localhost:3000,http://127.0.0.1:8083"
 
+# Client IP resolution.
+# Gin defaults to trusting every proxy, which makes X-Forwarded-For authoritative:
+# any caller could then choose the IP recorded in the login credential log and in
+# user.last_login_ip, and any IP-keyed rate limit or lockout would be bypassable.
+# Unset, trustedProxies falls back to the loopback / RFC1918 / IPv6 unique-local /
+# link-local ranges, which already covers a cloudflared sidecar on a compose
+# network. Set trustedPlatform when an edge overwrites the header instead of
+# appending to it - Cloudflare does, and CF-Connecting-IP is not client-forgeable.
+# TRUSTED_PROXIES="10.0.0.0/8,172.16.0.0/12"
+TRUSTED_PLATFORM=cloudflare
+
 # Database Configuration
 # Driver options: sqlite, mysql, postgres
 # Supabase PostgreSQL (DOS):

config/config.example.yaml

@@ -5,6 +5,18 @@ server:
   # Custom platform / instance branding (optional)
   companyName: ""
   companyLogoUrl: ""
+  # Reverse proxies in front of the application, as CIDR blocks. Gin's own default
+  # trusts every peer, which makes X-Forwarded-For authoritative and lets any
+  # caller choose the client address the server records in the login credential
+  # log and in user.last_login_ip. Left empty this falls back to the loopback,
+  # RFC1918, IPv6 unique-local and link-local ranges, which covers a sidecar
+  # tunnel, a compose network and a local nginx. Set it explicitly when the proxy
+  # sits on a public address, and never set it to 0.0.0.0/0.
+  trustedProxies: []
+  # Set to "cloudflare" (also accepted: "fly.io", "google-app-engine", or a literal
+  # header name) when an edge overwrites rather than appends the real client
+  # address. When set it takes precedence over X-Forwarded-For entirely.
+  trustedPlatform: ""
   cors:
     # Browser CORS allowlist. In production, replace this with the actual frontend or embedded-site domains, such as https://support.example.com.
     # Leave it empty to reject cross-origin browser requests. Same-origin and non-browser calls are still supported.

internal/bootstrap/server.go

@@ -1,6 +1,7 @@
 package bootstrap
 
 import (
+	"fmt"
 	"log/slog"
 	"net/http"
 	"path"
@@ -35,6 +36,18 @@ func NewServer() (*gin.Engine, error) {
 	printBanner()
 
 	app := gin.New()
+
+	// Gin defaults to trusting every proxy, which makes ClientIP() return the
+	// leftmost X-Forwarded-For value - a header any caller can set. Everything
+	// keyed on a client address depends on this being settled first: the login
+	// credential log, the user's last login IP, and any abuse control.
+	if platform := cfg.Server.TrustedPlatformHeader(); platform != "" {
+		app.TrustedPlatform = platform
+	}
+	if err := app.SetTrustedProxies(cfg.Server.TrustedProxiesOrDefault()); err != nil {
+		return nil, fmt.Errorf("invalid server.trustedProxies: %w", err)
+	}
+
 	app.Use(requestIDMiddleware())
 	app.Use(corsMiddleware())
 	app.Use(gin.Recovery())

internal/bootstrap/server_clientip_test.go

@@ -0,0 +1,148 @@
+package bootstrap
+
+import (
+	"bytes"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"agent-desk/internal/pkg/config"
+
+	"github.com/gin-gonic/gin"
+)
+
+// captureRequestLog swaps the slog default for one writing into buf, because
+// requestLogMiddleware records the address Gin resolved. That is the only place
+// the server exposes its ClientIP() decision, and it is the value that ends up in
+// t_login_credential_log and t_user.last_login_ip.
+func captureRequestLog(t *testing.T) *bytes.Buffer {
+	t.Helper()
+	var buf bytes.Buffer
+	previous := slog.Default()
+	slog.SetDefault(slog.New(slog.NewTextHandler(&buf, nil)))
+	t.Cleanup(func() { slog.SetDefault(previous) })
+	return &buf
+}
+
+func setTestServerConfig(server config.ServerConfig) {
+	config.SetCurrent(&config.Config{
+		Server:  server,
+		Storage: config.StorageConfig{Local: config.LocalStorageConfig{Root: "storage", BaseURL: "/storage"}},
+	})
+}
+
+// TestGinDefaultTrustsEveryProxy pins the vulnerability the configuration above
+// exists to close. A bare Gin engine - which is what NewServer used to build -
+// trusts 0.0.0.0/0 and ::/0, so validateHeader walks X-Forwarded-For right to
+// left, finds no untrusted proxy to stop at, and returns the leftmost value the
+// caller chose. If this assertion ever starts failing, Gin's default has changed
+// and the trusted-proxy configuration should be revisited rather than assumed
+// necessary.
+func TestGinDefaultTrustsEveryProxy(t *testing.T) {
+	app := gin.New()
+	var resolved string
+	app.GET("/probe", func(ctx *gin.Context) { resolved = ctx.ClientIP() })
+
+	req := httptest.NewRequest(http.MethodGet, "/probe", nil)
+	req.RemoteAddr = "203.0.113.7:52000"
+	req.Header.Set("X-Forwarded-For", "198.51.100.9")
+	app.ServeHTTP(httptest.NewRecorder(), req)
+
+	if resolved != "198.51.100.9" {
+		t.Fatalf("a bare engine resolved ClientIP to %q, expected the forged 198.51.100.9", resolved)
+	}
+}
+
+func TestNewServerIgnoresForgedForwardedForFromAnUntrustedPeer(t *testing.T) {
+	buf := captureRequestLog(t)
+	setTestServerConfig(config.ServerConfig{})
+
+	app, err := NewServer()
+	if err != nil {
+		t.Fatalf("NewServer() error = %v", err)
+	}
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/api/health", nil)
+	// A public peer is outside every default trusted range, so nothing it claims
+	// about the originating address may be believed.
+	req.RemoteAddr = "203.0.113.7:52000"
+	req.Header.Set("X-Forwarded-For", "198.51.100.9")
+	req.Header.Set("X-Real-IP", "198.51.100.9")
+	app.ServeHTTP(rec, req)
+
+	logged := buf.String()
+	if !strings.Contains(logged, "clientIp=203.0.113.7") {
+		t.Fatalf("expected the real peer address to be logged, got: %s", logged)
+	}
+	if strings.Contains(logged, "198.51.100.9") {
+		t.Fatalf("a forged forwarding header was trusted: %s", logged)
+	}
+}
+
+func TestNewServerReadsForwardedForFromATrustedProxy(t *testing.T) {
+	buf := captureRequestLog(t)
+	setTestServerConfig(config.ServerConfig{TrustedProxies: []string{"10.0.0.0/8"}})
+
+	app, err := NewServer()
+	if err != nil {
+		t.Fatalf("NewServer() error = %v", err)
+	}
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/api/health", nil)
+	req.RemoteAddr = "10.0.0.5:52000"
+	// A hostile client prepends a value it chose; the proxy appends the address it
+	// actually saw. Gin walks the list right to left and stops at the first
+	// address that is not itself a trusted proxy, which is the appended one.
+	req.Header.Set("X-Forwarded-For", "198.51.100.9, 203.0.113.7")
+	app.ServeHTTP(rec, req)
+
+	logged := buf.String()
+	if !strings.Contains(logged, "clientIp=203.0.113.7") {
+		t.Fatalf("expected the proxy-appended address, got: %s", logged)
+	}
+	if strings.Contains(logged, "198.51.100.9") {
+		t.Fatalf("the client-prepended address was trusted: %s", logged)
+	}
+}
+
+func TestNewServerPrefersTheConfiguredTrustedPlatformHeader(t *testing.T) {
+	buf := captureRequestLog(t)
+	setTestServerConfig(config.ServerConfig{TrustedPlatform: "cloudflare"})
+
+	app, err := NewServer()
+	if err != nil {
+		t.Fatalf("NewServer() error = %v", err)
+	}
+
+	rec := httptest.NewRecorder()
+	req := httptest.NewRequest(http.MethodGet, "/api/health", nil)
+	req.RemoteAddr = "10.0.0.5:52000"
+	req.Header.Set("X-Forwarded-For", "198.51.100.9")
+	// The edge overwrites this header rather than appending to it, which is what
+	// makes it usable as an identity at all.
+	req.Header.Set("CF-Connecting-IP", "203.0.113.7")
+	app.ServeHTTP(rec, req)
+
+	logged := buf.String()
+	if !strings.Contains(logged, "clientIp=203.0.113.7") {
+		t.Fatalf("expected the platform header to win, got: %s", logged)
+	}
+	if strings.Contains(logged, "198.51.100.9") {
+		t.Fatalf("X-Forwarded-For was consulted despite a trusted platform: %s", logged)
+	}
+}
+
+func TestNewServerRejectsAnInvalidTrustedProxyCIDR(t *testing.T) {
+	captureRequestLog(t)
+	setTestServerConfig(config.ServerConfig{TrustedProxies: []string{"10.0.0.0/8", "not-a-cidr"}})
+
+	if _, err := NewServer(); err == nil {
+		t.Fatal("NewServer() accepted an unparseable CIDR; a typo here would silently disable the trust boundary")
+	} else if !strings.Contains(err.Error(), "server.trustedProxies") {
+		t.Fatalf("NewServer() error = %v, expected it to name server.trustedProxies", err)
+	}
+}

internal/pkg/config/config.go

@@ -61,6 +61,65 @@ type ServerConfig struct {
 	CompanyLogoURL    string     `yaml:"companyLogoUrl"`
 	CompanyFaviconURL string     `yaml:"companyFaviconUrl"`
 	CORS              CORSConfig `yaml:"cors"`
+	// TrustedProxies are the CIDR blocks of the reverse proxies that sit in front
+	// of the application. Gin's own default is 0.0.0.0/0 and ::/0, which trusts
+	// every peer and makes ClientIP() return the leftmost X-Forwarded-For value -
+	// a header any caller can set.
+	TrustedProxies []string `yaml:"trustedProxies"`
+	// TrustedPlatform names an edge that overwrites rather than appends the real
+	// client address, for example "cloudflare". When set it takes precedence over
+	// X-Forwarded-For entirely.
+	TrustedPlatform string `yaml:"trustedPlatform"`
+}
+
+// defaultTrustedProxies covers loopback, RFC1918, IPv6 unique-local and
+// link-local ranges. That is the shape of almost every real deployment - a
+// sidecar tunnel, a compose network, a local nginx - and a client on the public
+// internet cannot present one of these addresses as its direct peer, so
+// X-Forwarded-For stays honest. When the app is exposed directly the peer is a
+// public address, is not trusted, and Gin falls back to it.
+var defaultTrustedProxies = []string{
+	"127.0.0.0/8",
+	"10.0.0.0/8",
+	"172.16.0.0/12",
+	"192.168.0.0/16",
+	"::1/128",
+	"fc00::/7",
+	"fe80::/10",
+}
+
+func (s ServerConfig) TrustedProxiesOrDefault() []string {
+	proxies := make([]string, 0, len(s.TrustedProxies))
+	for _, proxy := range s.TrustedProxies {
+		if proxy = strings.TrimSpace(proxy); proxy != "" {
+			proxies = append(proxies, proxy)
+		}
+	}
+	if len(proxies) == 0 {
+		return defaultTrustedProxies
+	}
+	return proxies
+}
+
+// TrustedPlatformHeader resolves the configured platform name to the header Gin
+// should read the client address from. Recognised names map to Gin's own
+// constants; any other non-empty value is passed through as a literal header
+// name, which is what Gin's TrustedPlatform field expects.
+func (s ServerConfig) TrustedPlatformHeader() string {
+	platform := strings.TrimSpace(s.TrustedPlatform)
+	if platform == "" {
+		return ""
+	}
+	switch strings.ToLower(platform) {
+	case "cloudflare", "cf":
+		return "CF-Connecting-IP"
+	case "fly.io", "flyio", "fly-io":
+		return "Fly-Client-IP"
+	case "google-app-engine", "appengine", "gae":
+		return "X-Appengine-Remote-Addr"
+	default:
+		return platform
+	}
 }
 
 func (s ServerConfig) Address() string {
@@ -107,7 +166,25 @@ type AuthConfig struct {
 	PasswordLoginEnabled *bool `yaml:"passwordLoginEnabled"`
 	TokenTTLHours        int   `yaml:"tokenTTLHours"`
 	MaxFailedAttempts    int   `yaml:"maxFailedAttempts"`
-	CredentialLockMinute int   `yaml:"credentialLockMinute"`
+	// MaxFailedAttemptsPerIP bounds failures from one client address across every
+	// username, which is what credential stuffing looks like. Zero or unset
+	// derives four times MaxFailedAttempts; it is disabled when MaxFailedAttempts
+	// is disabled.
+	MaxFailedAttemptsPerIP int `yaml:"maxFailedAttemptsPerIP"`
+	CredentialLockMinute   int `yaml:"credentialLockMinute"`
+}
+
+// MaxFailedAttemptsPerIPOrDefault derives the per-address threshold from the
+// per-account one so that a deployment which only tunes MaxFailedAttempts still
+// gets a coherent pair of limits.
+func (a AuthConfig) MaxFailedAttemptsPerIPOrDefault() int {
+	if a.MaxFailedAttemptsPerIP > 0 {
+		return a.MaxFailedAttemptsPerIP
+	}
+	if a.MaxFailedAttempts <= 0 {
+		return 0
+	}
+	return a.MaxFailedAttempts * 4
 }
 
 func (a AuthConfig) IsPasswordLoginEnabled() bool {
@@ -431,6 +508,8 @@ func bindConfigDefaults(v *viper.Viper) {
 	v.SetDefault("server.companyLogoUrl", "")
 	v.SetDefault("server.companyFaviconUrl", "")
 	v.SetDefault("server.cors.allowedOrigins", []string{})
+	v.SetDefault("server.trustedProxies", []string{})
+	v.SetDefault("server.trustedPlatform", "")
 	v.SetDefault("db.type", "sqlite")
 	v.SetDefault("db.dsn", "file:./data/app.db?_busy_timeout=5000")
 	v.SetDefault("db.maxIdleConns", 5)
@@ -442,6 +521,7 @@ func bindConfigDefaults(v *viper.Viper) {
 	v.SetDefault("logger.addSource", false)
 	v.SetDefault("auth.tokenTTLHours", 12)
 	v.SetDefault("auth.maxFailedAttempts", 5)
+	v.SetDefault("auth.maxFailedAttemptsPerIP", 0)
 	v.SetDefault("auth.credentialLockMinute", 15)
 	v.SetDefault("customerSession.ttlMinutes", 120)
 	v.SetDefault("customerSession.refreshThresholdMinutes", 30)
@@ -499,6 +579,8 @@ func bindEnvironmentAliases(v *viper.Viper) {
 	_ = v.BindEnv("server.companyName", "AGENT_DESK_SERVER_COMPANYNAME", "COMPANY_NAME", "NEXT_PUBLIC_COMPANY_NAME", "BRAND_NAME", "BRAND_COMPANY_NAME")
 	_ = v.BindEnv("server.companyLogoUrl", "AGENT_DESK_SERVER_COMPANYLOGOURL", "COMPANY_LOGO_URL", "NEXT_PUBLIC_COMPANY_LOGO_URL", "BRAND_LOGO_URL")
 	_ = v.BindEnv("server.companyFaviconUrl", "AGENT_DESK_SERVER_COMPANYFAVICONURL", "COMPANY_FAVICON_URL", "NEXT_PUBLIC_COMPANY_FAVICON_URL", "BRAND_FAVICON_URL", "FAVICON_URL")
+	_ = v.BindEnv("server.trustedProxies", "AGENT_DESK_SERVER_TRUSTEDPROXIES", "TRUSTED_PROXIES")
+	_ = v.BindEnv("server.trustedPlatform", "AGENT_DESK_SERVER_TRUSTEDPLATFORM", "TRUSTED_PLATFORM")
 	_ = v.BindEnv("db.type", "AGENT_DESK_DB_TYPE", "DATABASE_TYPE", "DB_TYPE")
 	_ = v.BindEnv("db.dsn", "AGENT_DESK_DB_DSN", "DATABASE_URL", "DB_DSN")
 	_ = v.BindEnv("auth.passwordLoginEnabled", "AGENT_DESK_AUTH_PASSWORDLOGINENABLED", "PASSWORD_LOGIN_ENABLED")