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.