Honour forwarded headers only from trusted proxies (closes #259)

A new ClientIP middleware sets request.RemoteAddr to the client's
address before the request is logged. X-Forwarded-For and X-Real-IP
count only when the peer is in TRUSTED_PROXIES, which defaults to the
RFC 1918 ranges. The login rate limit, the session's IP address and
hostname, and the request log all use that address.

Model: opus-5-5

README.md

@@ -1821,7 +1821,7 @@ over HTTPS. The proxy must:
       default does not trust them, so set `TRUSTED_PROXIES=127.0.0.1,::1`.
     - Through a published Docker port, that address is the Docker bridge
       gateway, such as `172.17.0.1`. The default trusts it.
-- Add the client's address to `X-Forwarded-For`, or set `X-Real-IP`.
+- Add the client's address to `X-Forwarded-For`.
 - Wait at least 60 seconds for a response. A long poll can take 30 seconds, and
   the server's own write timeout is 60 seconds.
 
@@ -1890,10 +1890,21 @@ The IRC listener has no TLS.
 
 The server logs with Go's `slog`, one line per event, to standard output. The
 lines are JSON, or plain text when standard output is a terminal. Every HTTP
-request is logged with its method, URL, protocol, status, latency, client
-address, user agent, referrer and request ID. The client address is the one
-found as described in [Reverse proxy](#reverse-proxy). `DEBUG=true` adds
-debug-level lines.
+request is logged with:
+
+- start time
+- method
+- URL
+- protocol
+- status
+- latency
+- client address
+- user agent
+- referrer
+- request ID
+
+The client address is the one found as described in
+[Reverse proxy](#reverse-proxy). `DEBUG=true` adds debug-level lines.
 
 ### The database
 

internal/config/config.go

@@ -43,6 +43,8 @@ var errOutOfRange = errors.New("setting out of range")
 
 var errNeverMatches = errors.New("entry can never match a connection")
 
+var errNotString = errors.New("not a string")
+
 // Params defines the dependencies for creating a Config.
 type Params struct {
 	fx.In
@@ -189,11 +191,19 @@ func parseSettings(cfg *Config) error {
 // counts an empty environment variable as unset, so the environment is
 // read directly as well.
 func parseTrustedProxies(value **netipx.IPSet) error {
-	raw := viper.GetString("TRUSTED_PROXIES")
+	setting := viper.Get("TRUSTED_PROXIES")
 
 	fromEnv, set := os.LookupEnv("TRUSTED_PROXIES")
 	if set {
-		raw = fromEnv
+		setting = fromEnv
+	}
+
+	// A YAML list or map in the config file is not a string, and
+	// viper.GetString would turn it into the empty string.
+	raw, isString := setting.(string)
+	if !isString {
+		return fmt.Errorf("cannot parse TRUSTED_PROXIES: %v is %w",
+			setting, errNotString)
 	}
 
 	var builder netipx.IPSetBuilder

internal/config/config_test.go

@@ -3,10 +3,12 @@ package config_test
 import (
 	"net/netip"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/spf13/viper"
 	"sneak.berlin/go/neoirc/internal/config"
 	"sneak.berlin/go/neoirc/internal/globals"
 	"sneak.berlin/go/neoirc/internal/logger"
@@ -141,8 +143,7 @@ func settingsOf(cfg *config.Config) settings {
 }
 
 func TestBadSettingStopsStartup(t *testing.T) {
-	// For each setting, the first value does not parse and the others
-	// parse but are out of range.
+	// Each value is invalid for its setting.
 	tests := []struct {
 		key    string
 		values []string
@@ -285,3 +286,50 @@ func TestTrustedProxies(t *testing.T) {
 		})
 	}
 }
+
+func TestTrustedProxiesNotAStringInYAMLStopsStartup(t *testing.T) {
+	// YAML reads the first value as a list and the second as a map.
+	values := []string{"[10.0.0.1]", "{proxy: 10.0.0.1}"}
+
+	for _, value := range values {
+		t.Run(value, func(t *testing.T) {
+			// t.Setenv puts back whatever value was there after the
+			// test, including when Unsetenv removes it.
+			t.Setenv("TRUSTED_PROXIES", "")
+
+			err := os.Unsetenv("TRUSTED_PROXIES")
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// loadConfig names the app neoirc-test, so config.New reads
+			// $HOME/.config/neoirc-test/neoirc-test.yaml.
+			home := t.TempDir()
+			t.Setenv("HOME", home)
+
+			dir := filepath.Join(home, ".config", "neoirc-test")
+
+			err = os.MkdirAll(dir, 0o700)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			err = os.WriteFile(
+				filepath.Join(dir, "neoirc-test.yaml"),
+				[]byte("TRUSTED_PROXIES: "+value+"\n"), 0o600,
+			)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// Viper keeps the values it read from this file after the
+			// file is gone. Reset clears them for later tests.
+			t.Cleanup(viper.Reset)
+
+			_, err = loadConfig(t)
+			if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") {
+				t.Errorf("error = %v, want one naming TRUSTED_PROXIES", err)
+			}
+		})
+	}
+}