From 4b9776cb2f92774edda405d3fb3e52e37549f703 Mon Sep 17 00:00:00 2001 From: Thando Mini Date: Thu, 11 Jun 2026 12:52:34 +0200 Subject: [PATCH] fix(web): re-validate stored session name + exact-match tmux target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleKill validated the incoming agent_id with sanitize.ValidIdentifier but accepted the projection-read sessionName at face value before shelling out to `tmux kill-session -t `. ValidIdentifier allows `.` (story IDs use it for split-depth notation), and tmux's target syntax treats `session.0` as "pane 0 of that session" — killing the wrong thing if the projection ever contained a tampered or migrated row with a dot. - Add sanitize.ValidTmuxTarget: stricter than ValidIdentifier — excludes `.` (pane separator) and `:` (window separator). Documented why. - handleKill re-validates the read-back sessionName; refuses + logs on failure rather than running tmux. - Switch the tmux invocation to `tmux kill-session -t =`. The leading `=` is tmux's exact-match prefix — even substring-match cases that slipped past ValidTmuxTarget can no longer target adjacent sessions. 3 new tests: - TestValidTmuxTarget — table-driven, covers session.0 / session:1 / empty / shell metachars / happy paths. - TestHandleKill_RejectsUnsafeStoredSessionName — seeds an agent with a "nxd-session.0" SessionName (passes ValidIdentifier on insert, fails the new ValidTmuxTarget on read) and asserts handleKill refuses without running tmux. Surfaced by the 2026-06-11 security audit (SEC-H3). --- internal/sanitize/path.go | 27 ++++++++++++++++++++++++++- internal/sanitize/path_test.go | 27 +++++++++++++++++++++++++++ internal/web/handler_success_test.go | 25 +++++++++++++++++++++++++ internal/web/handlers.go | 15 ++++++++++++++- 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/internal/sanitize/path.go b/internal/sanitize/path.go index 37eb30d..01b68c1 100644 --- a/internal/sanitize/path.go +++ b/internal/sanitize/path.go @@ -30,7 +30,7 @@ func SafeJoin(root, rel string) (string, error) { return joined, nil } -// ValidIdentifier returns true if s contains only [a-zA-Z0-9_-]. Useful for +// ValidIdentifier returns true if s contains only [a-zA-Z0-9_.-]. Useful for // validating story IDs, agent IDs, and session names before using them in // filesystem paths or shell arguments. func ValidIdentifier(s string) bool { @@ -50,3 +50,28 @@ func ValidIdentifier(s string) bool { } return true } + +// ValidTmuxTarget returns true if s is safe to pass to tmux as a session +// target via -t. Stricter than ValidIdentifier: the `.` and `:` characters +// have window/pane semantics in tmux target specifiers ("session.0", +// "session:1") so we exclude both even though tmux session names CAN +// technically contain them. A session name like "mysession.0" would +// otherwise let an attacker (or a corrupted projection) target a specific +// pane in a different session via the kill command. +func ValidTmuxTarget(s string) bool { + if s == "" { + return false + } + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', + r >= 'A' && r <= 'Z', + r >= '0' && r <= '9', + r == '_', r == '-': + continue + default: + return false + } + } + return true +} diff --git a/internal/sanitize/path_test.go b/internal/sanitize/path_test.go index 7910ac9..5c96bb2 100644 --- a/internal/sanitize/path_test.go +++ b/internal/sanitize/path_test.go @@ -60,3 +60,30 @@ func TestValidIdentifier(t *testing.T) { } } } + +// TestValidTmuxTarget guards the stricter contract used by handleKill before +// passing a session name to `tmux kill-session -t`. Unlike ValidIdentifier, +// `.` and `:` are rejected — both are tmux target separators (`session.0` +// targets pane 0, `session:1` targets window 1) and would let a corrupted +// projection name kill the wrong tmux entity. +func TestValidTmuxTarget(t *testing.T) { + for _, tc := range []struct { + in string + want bool + }{ + {"nxd-req-junior-1", true}, + {"a_b", true}, + {"abc123", true}, + {"", false}, + {"a/b", false}, + {"a b", false}, + {"a;b", false}, + {"session.0", false}, // tmux pane separator + {"session:1", false}, // tmux window separator + {"../etc", false}, + } { + if got := ValidTmuxTarget(tc.in); got != tc.want { + t.Errorf("ValidTmuxTarget(%q) = %v, want %v", tc.in, got, tc.want) + } + } +} diff --git a/internal/web/handler_success_test.go b/internal/web/handler_success_test.go index da80bd3..698f5c6 100644 --- a/internal/web/handler_success_test.go +++ b/internal/web/handler_success_test.go @@ -81,6 +81,31 @@ func TestHandleKill_MalformedPayload(t *testing.T) { } } +// TestHandleKill_RejectsUnsafeStoredSessionName guards the +// defense-in-depth check on the projection-read sessionName: even though +// the spawn-time path validates names before insertion, a tampered or +// migrated projection could carry a value like "nxd-session.0" — tmux +// `.0` syntax then targets pane 0 of that session, killing the wrong +// thing. handleKill now re-validates via sanitize.ValidTmuxTarget and +// refuses the kill on failure. +func TestHandleKill_RejectsUnsafeStoredSessionName(t *testing.T) { + s := newTestServer(t) + // Insert an agent whose SessionName contains a tmux pane separator — + // passes ValidIdentifier (which permits '.') but must fail the + // stricter ValidTmuxTarget. + if err := s.projStore.InsertAgent("agent-bad-session", "dev", "claude", "tmux", "nxd-session.0"); err != nil { + t.Fatalf("InsertAgent: %v", err) + } + + resp := s.handleKill(mustMarshal(t, agentPayload{AgentID: "agent-bad-session"})) + if resp.Success { + t.Fatal("expected Success=false when session name has tmux pane separator") + } + if !strings.Contains(resp.Message, "failed validation") { + t.Errorf("expected 'failed validation' in message, got %q", resp.Message) + } +} + // TestHandleKill_AgentInListButNoSession covers the path where an // agent exists in the projection but its SessionName field is empty. // The handler refuses to issue a tmux kill in that case. diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 313ba93..3bbc026 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -262,7 +262,20 @@ func (s *Server) handleKill(payload json.RawMessage) WSResponse { return WSResponse{Type: "command_result", Action: action, Success: false, Message: "agent not found or no session"} } - cmd := exec.Command("tmux", "kill-session", "-t", sessionName) + // Defense in depth: the sessionName was written into the projection at + // agent spawn time. A corrupted or tampered store could carry a string + // like "real-session.0" — tmux's `.0` syntax targets pane 0 of that + // session, so we'd kill the wrong window. ValidTmuxTarget excludes + // `.` and `:` for exactly this case. + if !sanitize.ValidTmuxTarget(sessionName) { + log.Printf("[ws] kill_agent rejecting unsafe session name %q for agent %s", sessionName, p.AgentID) + return WSResponse{Type: "command_result", Action: action, Success: false, Message: "stored session name failed validation"} + } + + // The leading `=` is tmux's exact-match prefix: even if a future + // sessionName accidentally contained substring matches against other + // sessions, the kill targets only the literal name. Belt + braces. + cmd := exec.Command("tmux", "kill-session", "-t", "="+sessionName) if err := cmd.Run(); err != nil { log.Printf("[cmd] kill-session %s: %v", sessionName, err) // Don't fail — session may already be dead.