fix: require configured trusted proxies before honoring X-Forwarded-For (L-2) (#74)

Author: jkyberneees

AGENTS.md

@@ -181,7 +181,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Memory add/replace pipe-to-shell filter** (`internal/memory/memory.go`) — `AddFact` and `ReplaceFact` now run `FactLooksUnsafe` in addition to the general guard scan, blocking agent-driven planting of download-and-execute facts.
 - **Skill learn-loop provenance propagation** (`internal/skills/learnloop.go`) — conversation-extracted suggestions and LLM-enhanced suggestions both retain the session's `SkillProvenance`, so tainted sessions cannot produce clean-looking auto-saved skills.
 - **Audit ingest recording for @-refs, `--ctx`, and Web-UI attachments** (`cmd/odek/refs.go`, `cmd/odek/serve.go`, `cmd/odek/audit.go`) — the per-session ingest recorder is attached before `@`-reference/`--ctx`/attachment resolution, `recordTurnAudit` scans `user` messages for untrusted wrappers in addition to `tool` messages, and the divergence heuristic compares agent actions against the original pre-enrichment prompt so attacker-injected resources are not treated as user-mentioned.
-- **Session ID entropy + session-scoped auth tokens** (`internal/session/session.go`, `cmd/odek/serve.go`) — session IDs now carry 128 bits of randomness (16 bytes / 32 hex chars); each session stores a 256-bit `AuthToken` required by `GET/DELETE/POST /api/sessions/<id>`, `POST /api/cancel`, and WebSocket session-resume messages via `X-Session-Token` header, `session_token` cookie, or `auth_token` WS field. Per-IP rate limiting (60/min) on session lookups adds a brute-force backstop.
+- **Session ID entropy + session-scoped auth tokens** (`internal/session/session.go`, `cmd/odek/serve.go`) — session IDs now carry 128 bits of randomness (16 bytes / 32 hex chars); each session stores a 256-bit `AuthToken` required by `GET/DELETE/POST /api/sessions/<id>`, `POST /api/cancel`, and WebSocket session-resume messages via `X-Session-Token` header, `session_token` cookie, or `auth_token` WS field. Per-IP rate limiting (60/min) on session lookups adds a brute-force backstop. `X-Forwarded-For` / `X-Real-Ip` headers are only honored when the direct remote address is in the configured `trusted_proxies` list, so spoofed headers cannot bypass the limiters.
 - **Skill/episode untrusted wrapper** (`internal/loop/loop.go` + `odek.go`) — skill context and retrieved session-episode context are passed through the caller-provided untrusted wrapper (the same nonce'd `<untrusted_content_*>` boundary used for tool output) before being injected into the model's system context. This prevents a compromised or tainted skill/episode from being treated as trusted system instructions.
 - **`env` / `printenv` environment-dump gate** (`internal/danger/classifier.go`) — bare `env` and `printenv` invocations are classified as `system_write` because they can leak process-environment secrets that the redaction scanner does not recognise. `env VAR=value <cmd>` still classifies `<cmd>` normally.
 - **Web UI attachment wrapping** (`cmd/odek/serve.go` + `cmd/odek/ui/app.js`) — files attached through the browser are sent separately from the user's text and wrapped with the nonce'd `<untrusted_content_*>` boundary (`source="attachment:<filename>"`) before injection into the prompt.

cmd/odek/next_security_vulnerabilities_test.go

@@ -304,6 +304,107 @@ func TestServe_API_AcceptsServeTokenCookie(t *testing.T) {
 	}
 }
 
+// ── 4c. rate-limiting clientIP must not trust X-Forwarded-From by default ─
+
+func TestClientIP_IgnoresForwardedHeadersByDefault(t *testing.T) {
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "127.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "1.2.3.4")
+	if got := clientIP(req, nil); got != "127.0.0.1" {
+		t.Fatalf("expected loopback address without trusted proxies, got %q", got)
+	}
+}
+
+func TestClientIP_HonorsForwardedHeadersFromTrustedProxy(t *testing.T) {
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "10.0.0.1:12345"
+	req.Header.Set("X-Forwarded-For", "1.2.3.4")
+	if got := clientIP(req, []string{"10.0.0.1"}); got != "1.2.3.4" {
+		t.Fatalf("expected forwarded IP from trusted proxy, got %q", got)
+	}
+}
+
+func TestClientIP_HonorsForwardedHeadersFromTrustedCIDR(t *testing.T) {
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "10.0.0.42:12345"
+	req.Header.Set("X-Forwarded-For", "1.2.3.4")
+	if got := clientIP(req, []string{"10.0.0.0/8"}); got != "1.2.3.4" {
+		t.Fatalf("expected forwarded IP from trusted CIDR, got %q", got)
+	}
+}
+
+func TestClientIP_RejectsForwardedHeadersFromUntrustedProxy(t *testing.T) {
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.RemoteAddr = "192.168.1.1:12345"
+	req.Header.Set("X-Forwarded-For", "1.2.3.4")
+	if got := clientIP(req, []string{"10.0.0.0/8"}); got != "192.168.1.1" {
+		t.Fatalf("expected direct remote address for untrusted proxy, got %q", got)
+	}
+}
+
+func TestIsTrustedProxy(t *testing.T) {
+	if !isTrustedProxy("127.0.0.1", []string{"127.0.0.1"}) {
+		t.Error("expected exact IP match")
+	}
+	if !isTrustedProxy("10.0.0.5", []string{"10.0.0.0/8"}) {
+		t.Error("expected CIDR match")
+	}
+	if isTrustedProxy("192.168.1.1", []string{"10.0.0.0/8"}) {
+		t.Error("expected no match for IP outside CIDR")
+	}
+	if isTrustedProxy("127.0.0.1", nil) {
+		t.Error("expected no match for empty trusted list")
+	}
+}
+
+func TestLoadConfig_TrustedProxiesFromEnv(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+	t.Setenv("ODEK_TRUSTED_PROXIES", "10.0.0.0/8, 127.0.0.1")
+
+	cfg := config.LoadConfig(config.CLIFlags{})
+	want := []string{"10.0.0.0/8", "127.0.0.1"}
+	if len(cfg.TrustedProxies) != len(want) {
+		t.Fatalf("TrustedProxies = %v, want %v", cfg.TrustedProxies, want)
+	}
+	for i := range want {
+		if cfg.TrustedProxies[i] != want[i] {
+			t.Errorf("TrustedProxies[%d] = %q, want %q", i, cfg.TrustedProxies[i], want[i])
+		}
+	}
+}
+
+func TestLoadConfig_TrustedProxiesFromCLI(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+
+	cfg := config.LoadConfig(config.CLIFlags{TrustedProxies: []string{"10.0.0.0/8", "127.0.0.1"}})
+	want := []string{"10.0.0.0/8", "127.0.0.1"}
+	if len(cfg.TrustedProxies) != len(want) {
+		t.Fatalf("TrustedProxies = %v, want %v", cfg.TrustedProxies, want)
+	}
+}
+
+func TestLoadConfig_TrustedProxiesFromFile(t *testing.T) {
+	dir := t.TempDir()
+	t.Setenv("HOME", dir)
+
+	cfgDir := filepath.Join(dir, ".odek")
+	os.MkdirAll(cfgDir, 0755)
+	cfgPath := filepath.Join(cfgDir, "config.json")
+	if err := os.WriteFile(cfgPath, []byte(`{
+		"trusted_proxies": ["10.0.0.0/8", "127.0.0.1"]
+	}`), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := config.LoadConfig(config.CLIFlags{})
+	want := []string{"10.0.0.0/8", "127.0.0.1"}
+	if len(cfg.TrustedProxies) != len(want) {
+		t.Fatalf("TrustedProxies = %v, want %v", cfg.TrustedProxies, want)
+	}
+}
+
 func TestServe_StaticSecurityHeaders(t *testing.T) {
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
 	rr := httptest.NewRecorder()

cmd/odek/serve.go

@@ -186,7 +186,7 @@ func serveCmd(args []string) error {
 	var sandboxReadonly *bool
 	var promptCaching *bool
 	var sandboxImage, sandboxNetwork, sandboxMemory, sandboxCPUs, sandboxUser string
-	var toolsEnabled, toolsDisabled []string
+	var toolsEnabled, toolsDisabled, trustedProxies []string
 
 	for i := 0; i < len(args); i++ {
 		switch args[i] {
@@ -246,6 +246,15 @@ func serveCmd(args []string) error {
 				return fmt.Errorf("--no-tool requires a value")
 			}
 			toolsDisabled = append(toolsDisabled, args[i])
+		case "--trusted-proxies":
+			i++
+			if i >= len(args) {
+				return fmt.Errorf("--trusted-proxies requires a value")
+			}
+			trustedProxies = strings.Split(args[i], ",")
+			for j := range trustedProxies {
+				trustedProxies[j] = strings.TrimSpace(trustedProxies[j])
+			}
 		default:
 			return fmt.Errorf("unknown flag %q for serve", args[i])
 		}
@@ -262,6 +271,7 @@ func serveCmd(args []string) error {
 		SandboxUser:     sandboxUser,
 		ToolsEnabled:    toolsEnabled,
 		ToolsDisabled:   toolsDisabled,
+		TrustedProxies:  trustedProxies,
 	})
 	if err := approveProjectSandbox(resolved, os.Stdin, os.Stdout); err != nil {
 		return err
@@ -305,7 +315,7 @@ func serveCmd(args []string) error {
 	mux.HandleFunc("/", handleStatic(wsToken))
 	mux.Handle("/ws", &golangws.Server{
 		Handshake: func(cfg *golangws.Config, req *http.Request) error {
-			return wsHandshakeWithLimits(cfg, req, wsToken)
+			return wsHandshakeWithLimits(cfg, req, wsToken, resolved.TrustedProxies)
 		},
 		Handler: func(conn *golangws.Conn) {
 			handleWS(store, resourceReg, resolved, systemMessage, conn)
@@ -319,7 +329,7 @@ func serveCmd(args []string) error {
 	}
 	mux.Handle("/api/resources", apiAuth(handleResourceSearch(resourceReg)))
 	mux.Handle("/api/sessions", apiAuth(handleSessionList(store)))
-	mux.Handle("/api/sessions/", apiAuth(handleSessionByID(store)))
+	mux.Handle("/api/sessions/", apiAuth(handleSessionByID(store, resolved.TrustedProxies)))
 	mux.Handle("/api/models", apiAuth(handleModelList(resolved.Model)))
 	mux.Handle("/api/cancel", apiAuth(handleCancel(store)))
 
@@ -370,6 +380,7 @@ Flags:
   --sandbox-user user      Container user (e.g. 1000:1000)
   --tool name              Enable a tool for the LLM (repeatable)
   --no-tool name           Disable a tool for the LLM (repeatable)
+  --trusted-proxies list   Comma-separated IPs/CIDRs whose X-Forwarded-For headers are trusted
   --help, -h               Show this help`)
 }
 
@@ -1311,14 +1322,14 @@ func validateServeToken(cfg *golangws.Config, req *http.Request, token string) e
 // applies a per-IP upgrade rate limit and acquires the global
 // concurrent-connection semaphore. The semaphore is acquired before the
 // WebSocket handshake completes and released when the handler exits.
-func wsHandshakeWithLimits(cfg *golangws.Config, req *http.Request, token string) error {
+func wsHandshakeWithLimits(cfg *golangws.Config, req *http.Request, token string, trustedProxies []string) error {
 	if err := validateServeToken(cfg, req, token); err != nil {
 		return err
 	}
 	if err := checkLocalOrigin(nil, req); err != nil {
 		return err
 	}
-	if !wsUpgradeLimiter.allow(clientIP(req)) {
+	if !wsUpgradeLimiter.allow(clientIP(req, trustedProxies)) {
 		return fmt.Errorf("WebSocket upgrade rate limit exceeded")
 	}
 	select {
@@ -1519,7 +1530,7 @@ func handleSessionList(store *session.Store) http.HandlerFunc {
 	}
 }
 
-func handleSessionByID(store *session.Store) http.HandlerFunc {
+func handleSessionByID(store *session.Store, trustedProxies []string) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		id := strings.TrimPrefix(r.URL.Path, "/api/sessions/")
 		if id == "" {
@@ -1531,7 +1542,7 @@ func handleSessionByID(store *session.Store) http.HandlerFunc {
 		case http.MethodGet:
 			// Rate-limit session detail lookups per IP to slow brute-force
 			// enumeration of the 128-bit ID space.
-			if !sessionLookupLimiter.allow(clientIP(r)) {
+			if !sessionLookupLimiter.allow(clientIP(r, trustedProxies)) {
 				http.Error(w, "rate limit exceeded", http.StatusTooManyRequests)
 				return
 			}
@@ -1599,16 +1610,16 @@ func handleSessionByID(store *session.Store) http.HandlerFunc {
 	}
 }
 
-// clientIP returns a best-effort client identifier for rate limiting. It prefers
-// X-Forwarded-For / X-Real-Ip only when the direct remote address is a loopback
-// proxy, otherwise uses RemoteAddr. This avoids trusting spoofed headers from
-// arbitrary clients while still working behind a local reverse proxy.
-func clientIP(r *http.Request) string {
+// clientIP returns a best-effort client identifier for rate limiting.
+// X-Forwarded-For / X-Real-Ip headers are only honored when the direct remote
+// address is in trustedProxies. An empty trustedProxies list means headers are
+// ignored even from loopback, closing the X-Forwarded-For spoofing bypass.
+func clientIP(r *http.Request, trustedProxies []string) string {
 	host, _, err := net.SplitHostPort(r.RemoteAddr)
 	if err != nil {
 		host = r.RemoteAddr
 	}
-	if host == "127.0.0.1" || host == "::1" || host == "localhost" {
+	if isTrustedProxy(host, trustedProxies) {
 		if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" {
 			if i := strings.Index(fwd, ","); i > 0 {
 				return strings.TrimSpace(fwd[:i])
@@ -1622,6 +1633,28 @@ func clientIP(r *http.Request) string {
 	return host
 }
 
+// isTrustedProxy reports whether host is in the trusted proxy list. Entries may
+// be exact IPs or CIDR ranges.
+func isTrustedProxy(host string, trusted []string) bool {
+	if host == "" {
+		return false
+	}
+	ip := net.ParseIP(host)
+	for _, entry := range trusted {
+		entry = strings.TrimSpace(entry)
+		if entry == "" {
+			continue
+		}
+		if entry == host {
+			return true
+		}
+		if _, cidr, err := net.ParseCIDR(entry); err == nil && ip != nil && cidr.Contains(ip) {
+			return true
+		}
+	}
+	return false
+}
+
 func handleModelList(configuredModel string) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != http.MethodGet {

cmd/odek/serve_api_test.go

@@ -70,7 +70,7 @@ func TestHandleSessionByID_GET_ReturnsSession(t *testing.T) {
 		t.Fatalf("Create session: %v", err)
 	}
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
 	req.Header.Set("X-Session-Token", sess.AuthToken)
 	w := httptest.NewRecorder()
@@ -106,7 +106,7 @@ func TestHandleSessionByID_GET_ReturnsSession(t *testing.T) {
 
 func TestHandleSessionByID_GET_NotFound(t *testing.T) {
 	store := newTestSessionStore(t)
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 
 	// Valid ID format but doesn't exist on disk.
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/20260101-aabbcc", nil)
@@ -120,7 +120,7 @@ func TestHandleSessionByID_GET_NotFound(t *testing.T) {
 
 func TestHandleSessionByID_GET_MissingID(t *testing.T) {
 	store := newTestSessionStore(t)
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/", nil)
 	w := httptest.NewRecorder()
@@ -145,7 +145,7 @@ func TestHandleSessionByID_GET_MessagesArePresent(t *testing.T) {
 		t.Fatalf("Create: %v", err)
 	}
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
 	req.Header.Set("X-Session-Token", sess.AuthToken)
 	w := httptest.NewRecorder()
@@ -161,7 +161,7 @@ func TestHandleSessionByID_GET_MessagesArePresent(t *testing.T) {
 
 func TestHandleSessionByID_MethodNotAllowed(t *testing.T) {
 	store := newTestSessionStore(t)
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 
 	for _, method := range []string{http.MethodPatch, http.MethodPut, http.MethodHead} {
 		req := httptest.NewRequest(method, "/api/sessions/20260101-aabbcc", nil)
@@ -182,7 +182,7 @@ func TestHandleSessionByID_DELETE_StillWorks(t *testing.T) {
 		t.Fatalf("Create: %v", err)
 	}
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodDelete, "/api/sessions/"+sess.ID, nil)
 	req.Header.Set("X-Session-Token", sess.AuthToken)
 	w := httptest.NewRecorder()
@@ -209,7 +209,7 @@ func TestHandleSessionByID_POST_RenameStillWorks(t *testing.T) {
 		t.Fatalf("Create: %v", err)
 	}
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	body := strings.NewReader(`{"name":"renamed task"}`)
 	req := httptest.NewRequest(http.MethodPost, "/api/sessions/"+sess.ID, body)
 	req.Header.Set("Content-Type", "application/json")
@@ -232,7 +232,7 @@ func TestHandleSessionByID_GET_InvalidToken(t *testing.T) {
 	store := newTestSessionStore(t)
 	sess, _ := store.Create([]llm.Message{{Role: "user", Content: "hi"}}, "m", "task")
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
 	req.Header.Set("X-Session-Token", "wrong-token")
 	w := httptest.NewRecorder()
@@ -247,7 +247,7 @@ func TestHandleSessionByID_GET_MissingToken(t *testing.T) {
 	store := newTestSessionStore(t)
 	sess, _ := store.Create([]llm.Message{{Role: "user", Content: "hi"}}, "m", "task")
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
 	w := httptest.NewRecorder()
 	handler(w, req)
@@ -268,7 +268,7 @@ func TestHandleSessionByID_GET_LazyTokenBootstrap(t *testing.T) {
 		t.Fatalf("Save: %v", err)
 	}
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
 	w := httptest.NewRecorder()
 	handler(w, req)
@@ -301,7 +301,7 @@ func TestHandleSessionByID_DELETE_RequiresToken(t *testing.T) {
 	store := newTestSessionStore(t)
 	sess, _ := store.Create([]llm.Message{{Role: "user", Content: "hi"}}, "m", "task")
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	req := httptest.NewRequest(http.MethodDelete, "/api/sessions/"+sess.ID, nil)
 	req.Header.Set("X-Session-Token", "wrong-token")
 	w := httptest.NewRecorder()
@@ -316,7 +316,7 @@ func TestHandleSessionByID_POST_RequiresToken(t *testing.T) {
 	store := newTestSessionStore(t)
 	sess, _ := store.Create([]llm.Message{{Role: "user", Content: "hi"}}, "m", "task")
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	body := strings.NewReader(`{"name":"renamed"}`)
 	req := httptest.NewRequest(http.MethodPost, "/api/sessions/"+sess.ID, body)
 	req.Header.Set("Content-Type", "application/json")
@@ -336,7 +336,7 @@ func TestHandleSessionByID_GET_RateLimit(t *testing.T) {
 	sessionLookupLimiter.reset()
 	defer sessionLookupLimiter.reset()
 
-	handler := handleSessionByID(store)
+	handler := handleSessionByID(store, nil)
 	// Exhaust the 60/min allowance.
 	for i := 0; i < 60; i++ {
 		req := httptest.NewRequest(http.MethodGet, "/api/sessions/"+sess.ID, nil)
@@ -741,7 +741,7 @@ func buildServeMuxWithSessionByID(t *testing.T, store *session.Store) (net.Liste
 	apiAuth := func(h http.Handler) http.Handler {
 		return requireServeToken(token)(requireLocalHost(requireLocalOrigin(h)))
 	}
-	mux.Handle("/api/sessions/", apiAuth(handleSessionByID(store)))
+	mux.Handle("/api/sessions/", apiAuth(handleSessionByID(store, nil)))
 	return ln, mux
 }
 

cmd/odek/serve_approval_test.go

@@ -56,7 +56,7 @@ func buildServeMuxPromptAll(t *testing.T, store *session.Store) (net.Listener, *
 	mux.HandleFunc("/", handleStatic(wsToken))
 	mux.Handle("/ws", &golangws.Server{
 		Handshake: func(cfg *golangws.Config, req *http.Request) error {
-			return wsHandshakeWithLimits(cfg, req, wsToken)
+			return wsHandshakeWithLimits(cfg, req, wsToken, nil)
 		},
 		Handler: func(conn *golangws.Conn) {
 			handleWS(store, resourceReg, resolved, systemMessage, conn)