From bc087665ab9ac8f1e458594487df731ee6ce330d Mon Sep 17 00:00:00 2001 From: tiwillia-ai-bot Date: Wed, 1 Apr 2026 19:28:03 +0000 Subject: [PATCH 1/2] Add spawn_agent space enforcement (SEC-007) Problem: The spawn_agent tool allowed agents to create other agents in arbitrary spaces, not just their own assigned space. This caused issues where agents created in wrong spaces (e.g., jsell-agent-boss instead of odis-devel). Solution: Add mechanical enforcement to prevent cross-space spawning: 1. When a parent/spawner agent is specified, look up which space that parent exists in 2. Enforce that the target space parameter matches the parent's space 3. Reject the spawn attempt if spaces don't match Changes: - mcp_tools.go: Added space validation in spawn_agent MCP tool handler - handlers_agent.go: Added same validation in HTTP POST /spaces/{space}/agents - server_test.go: Added comprehensive test coverage for space enforcement The validation is case-insensitive and only applies when a parent is specified, maintaining backward compatibility for spawns without parents. Fixes issue where agents could spawn agents in arbitrary spaces. Co-Authored-By: Claude Sonnet 4.5 --- internal/coordinator/handlers_agent.go | 31 ++++++ internal/coordinator/mcp_tools.go | 29 ++++++ internal/coordinator/server_test.go | 129 +++++++++++++++++++++++++ 3 files changed, 189 insertions(+) diff --git a/internal/coordinator/handlers_agent.go b/internal/coordinator/handlers_agent.go index 8b37298..74144a8 100644 --- a/internal/coordinator/handlers_agent.go +++ b/internal/coordinator/handlers_agent.go @@ -1543,6 +1543,37 @@ func (s *Server) handleCreateAgents(w http.ResponseWriter, r *http.Request, spac return } + // Space enforcement (SEC-007): When a parent is specified, enforce that the + // new agent is created in the same space as the parent. This prevents agents + // from being spawned in arbitrary spaces via the HTTP API. + if req.Parent != "" { + s.mu.RLock() + parentSpace := "" + parentFound := false + // Search all spaces to find which space contains the parent agent. + for spName, ks := range s.spaces { + parentCanonical := resolveAgentName(ks, req.Parent) + if _, exists := ks.Agents[parentCanonical]; exists { + parentSpace = spName + parentFound = true + break + } + } + s.mu.RUnlock() + + if !parentFound { + writeJSONError(w, fmt.Sprintf("parent agent %q not found in any space", req.Parent), http.StatusBadRequest) + return + } + if !strings.EqualFold(parentSpace, spaceName) { + writeJSONError(w, fmt.Sprintf( + "space enforcement: agent %q in space %q cannot spawn agents in space %q", + req.Parent, parentSpace, spaceName, + ), http.StatusForbidden) + return + } + } + var createOpts SessionCreateOpts if backend.Name() == "ambient" { sessionName := tmuxDefaultSession(spaceName, req.Name) diff --git a/internal/coordinator/mcp_tools.go b/internal/coordinator/mcp_tools.go index 476cd8e..33d2758 100644 --- a/internal/coordinator/mcp_tools.go +++ b/internal/coordinator/mcp_tools.go @@ -1088,6 +1088,35 @@ func (s *Server) addToolSpawnAgent(srv *mcp.Server) { workDir := strArg(args, "work_dir") spawnerName := strArg(args, "parent") + // Space enforcement (SEC-007): When a parent/spawner is specified, enforce + // that the new agent is created in the same space as the parent. This prevents + // agents from spawning other agents in arbitrary spaces. + if spawnerName != "" { + s.mu.RLock() + spawnerSpace := "" + spawnerFound := false + // Search all spaces to find which space contains the spawner agent. + for spName, ks := range s.spaces { + spawnerCanonical := resolveAgentName(ks, spawnerName) + if _, exists := ks.Agents[spawnerCanonical]; exists { + spawnerSpace = spName + spawnerFound = true + break + } + } + s.mu.RUnlock() + + if !spawnerFound { + return toolError(fmt.Sprintf("parent agent %q not found in any space", spawnerName)), nil + } + if !strings.EqualFold(spawnerSpace, spaceName) { + return toolError(fmt.Sprintf( + "space enforcement: agent %q in space %q cannot spawn agents in space %q", + spawnerName, spawnerSpace, spaceName, + )), nil + } + } + if workDir != "" { // Ensure an AgentConfig entry exists so spawnAgentService picks up the workDir. s.mu.Lock() diff --git a/internal/coordinator/server_test.go b/internal/coordinator/server_test.go index 0246cc3..24e4093 100644 --- a/internal/coordinator/server_test.go +++ b/internal/coordinator/server_test.go @@ -4850,3 +4850,132 @@ func TestOperatorExcludedFromHierarchy(t *testing.T) { t.Errorf("operator should not appear in hierarchy nodes") } } +// TestSpawnAgentSpaceEnforcement verifies SEC-007: agents can only spawn other +// agents in their own space, not in arbitrary spaces. This prevents cross-space +// spawning attacks where an agent in space-a creates agents in space-b. +func TestSpawnAgentSpaceEnforcement(t *testing.T) { + srv, cleanup := mustStartServer(t) + defer cleanup() + base := serverBaseURL(srv) + + // Setup: Create a parent agent in space-a + spaceA := "space-a" + parentAgent := "parent-agent" + + // Register parent agent in space-a via direct API call + payload := map[string]any{ + "status": "active", + "summary": parentAgent + ": parent in space-a", + } + resp := postJSON(t, base+"/spaces/"+spaceA+"/agent/"+parentAgent, payload) + resp.Body.Close() + if resp.StatusCode != http.StatusAccepted { + t.Fatalf("setup: failed to create parent agent: status %d", resp.StatusCode) + } + + // Test 1: MCP tool - Attempt to spawn child in space-b with parent from space-a (should fail) + t.Run("MCP_CrossSpaceSpawnRejected", func(t *testing.T) { + result := callMCPTool(t, base, "spawn_agent", map[string]any{ + "space": "space-b", // different space + "name": "child-agent", + "parent": parentAgent, // parent is in space-a + }) + + // Should contain error about space enforcement + if !strings.Contains(result, "space enforcement") { + t.Errorf("Expected space enforcement error, got: %s", result) + } + if !strings.Contains(result, "space-a") || !strings.Contains(result, "space-b") { + t.Errorf("Error should mention both spaces, got: %s", result) + } + }) + + // Test 2: MCP tool - Spawn child in same space as parent (should succeed) + t.Run("MCP_SameSpaceSpawnAllowed", func(t *testing.T) { + result := callMCPTool(t, base, "spawn_agent", map[string]any{ + "space": spaceA, // same space as parent + "name": "child-agent-ok", + "parent": parentAgent, + }) + + // Should succeed (no error) + if strings.Contains(strings.ToLower(result), "error") { + t.Errorf("Expected success, got error: %s", result) + } + // Verify response contains success indicators + var response map[string]any + if err := json.Unmarshal([]byte(result), &response); err == nil { + if ok, _ := response["ok"].(bool); !ok { + t.Errorf("Expected ok:true in response, got: %s", result) + } + } + }) + + // Test 3: MCP tool - Non-existent parent should be rejected + t.Run("MCP_NonExistentParentRejected", func(t *testing.T) { + result := callMCPTool(t, base, "spawn_agent", map[string]any{ + "space": spaceA, + "name": "orphan-agent", + "parent": "non-existent-parent", + }) + + if !strings.Contains(result, "not found") { + t.Errorf("Expected 'not found' error for non-existent parent, got: %s", result) + } + }) + + // Test 4: HTTP endpoint - Attempt cross-space spawn via POST /spaces/{space}/agents + t.Run("HTTP_CrossSpaceSpawnRejected", func(t *testing.T) { + createReq := map[string]any{ + "name": "http-child", + "parent": parentAgent, // parent is in space-a + "backend": "tmux", + } + resp := postJSON(t, base+"/spaces/space-b/agents", createReq) + defer resp.Body.Close() + + if resp.StatusCode != http.StatusForbidden { + t.Errorf("Expected 403 Forbidden for cross-space spawn, got %d", resp.StatusCode) + } + + // Read error message + bodyBytes, _ := io.ReadAll(resp.Body) + bodyStr := string(bodyBytes) + if !strings.Contains(bodyStr, "space enforcement") { + t.Errorf("Expected space enforcement error message, got: %s", bodyStr) + } + }) + + // Test 5: HTTP endpoint - Same space spawn should succeed (or at least not fail on space check) + t.Run("HTTP_SameSpaceSpawnAllowed", func(t *testing.T) { + createReq := map[string]any{ + "name": "http-child-ok", + "parent": parentAgent, // parent is in space-a + "backend": "tmux", + } + resp := postJSON(t, base+"/spaces/"+spaceA+"/agents", createReq) + defer resp.Body.Close() + + // Should not be rejected for space enforcement (may fail for other reasons like no tmux) + if resp.StatusCode == http.StatusForbidden { + bodyBytes, _ := io.ReadAll(resp.Body) + if strings.Contains(string(bodyBytes), "space enforcement") { + t.Errorf("Same-space spawn should not be rejected by space enforcement") + } + } + }) + + // Test 6: No parent specified - should be allowed (backward compatibility) + t.Run("MCP_NoParentAllowed", func(t *testing.T) { + result := callMCPTool(t, base, "spawn_agent", map[string]any{ + "space": "space-c", // any space + "name": "independent-agent", + // no parent specified + }) + + // Should not fail on space enforcement (may fail for other reasons) + if strings.Contains(result, "space enforcement") { + t.Errorf("Spawn without parent should not trigger space enforcement, got: %s", result) + } + }) +} From bb0fe7e411066ae9663936bcff6f27e199c067a2 Mon Sep 17 00:00:00 2001 From: tiwillia-ai-bot Date: Wed, 8 Apr 2026 19:42:59 +0000 Subject: [PATCH 2/2] Address PR #10 review feedback Fixes: 1. Replace strings.EqualFold with exact string comparison for space enforcement (security fix - case-insensitive comparison could allow bypass attacks) 2. Extract duplicate parent lookup logic to shared findAgentSpace helper function in helpers.go to eliminate code duplication Note: callMCPTool function exists at line 3457 of server_test.go and tests compile successfully. Co-Authored-By: Claude Sonnet 4.5 --- internal/coordinator/handlers_agent.go | 14 ++------------ internal/coordinator/helpers.go | 13 +++++++++++++ internal/coordinator/mcp_tools.go | 14 ++------------ 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/internal/coordinator/handlers_agent.go b/internal/coordinator/handlers_agent.go index 74144a8..682b513 100644 --- a/internal/coordinator/handlers_agent.go +++ b/internal/coordinator/handlers_agent.go @@ -1548,24 +1548,14 @@ func (s *Server) handleCreateAgents(w http.ResponseWriter, r *http.Request, spac // from being spawned in arbitrary spaces via the HTTP API. if req.Parent != "" { s.mu.RLock() - parentSpace := "" - parentFound := false - // Search all spaces to find which space contains the parent agent. - for spName, ks := range s.spaces { - parentCanonical := resolveAgentName(ks, req.Parent) - if _, exists := ks.Agents[parentCanonical]; exists { - parentSpace = spName - parentFound = true - break - } - } + parentSpace, parentFound := s.findAgentSpace(req.Parent) s.mu.RUnlock() if !parentFound { writeJSONError(w, fmt.Sprintf("parent agent %q not found in any space", req.Parent), http.StatusBadRequest) return } - if !strings.EqualFold(parentSpace, spaceName) { + if parentSpace != spaceName { writeJSONError(w, fmt.Sprintf( "space enforcement: agent %q in space %q cannot spawn agents in space %q", req.Parent, parentSpace, spaceName, diff --git a/internal/coordinator/helpers.go b/internal/coordinator/helpers.go index b51f9c5..b4f5a92 100644 --- a/internal/coordinator/helpers.go +++ b/internal/coordinator/helpers.go @@ -187,3 +187,16 @@ func pruneNotifications(ag *AgentUpdate) { } ag.Notifications = append(unread, read...) } + +// findAgentSpace searches all spaces to find which space contains the specified agent. +// Returns the space name and true if found, empty string and false otherwise. +// Must be called with s.mu.RLock held. +func (s *Server) findAgentSpace(agentName string) (string, bool) { + for spaceName, ks := range s.spaces { + canonical := resolveAgentName(ks, agentName) + if _, exists := ks.Agents[canonical]; exists { + return spaceName, true + } + } + return "", false +} diff --git a/internal/coordinator/mcp_tools.go b/internal/coordinator/mcp_tools.go index 33d2758..ca84241 100644 --- a/internal/coordinator/mcp_tools.go +++ b/internal/coordinator/mcp_tools.go @@ -1093,23 +1093,13 @@ func (s *Server) addToolSpawnAgent(srv *mcp.Server) { // agents from spawning other agents in arbitrary spaces. if spawnerName != "" { s.mu.RLock() - spawnerSpace := "" - spawnerFound := false - // Search all spaces to find which space contains the spawner agent. - for spName, ks := range s.spaces { - spawnerCanonical := resolveAgentName(ks, spawnerName) - if _, exists := ks.Agents[spawnerCanonical]; exists { - spawnerSpace = spName - spawnerFound = true - break - } - } + spawnerSpace, spawnerFound := s.findAgentSpace(spawnerName) s.mu.RUnlock() if !spawnerFound { return toolError(fmt.Sprintf("parent agent %q not found in any space", spawnerName)), nil } - if !strings.EqualFold(spawnerSpace, spaceName) { + if spawnerSpace != spaceName { return toolError(fmt.Sprintf( "space enforcement: agent %q in space %q cannot spawn agents in space %q", spawnerName, spawnerSpace, spaceName,