diff --git a/workspace-server/internal/handlers/cross_tenant_isolation_test.go b/workspace-server/internal/handlers/cross_tenant_isolation_test.go index 10cd91ba9..8289ddf10 100644 --- a/workspace-server/internal/handlers/cross_tenant_isolation_test.go +++ b/workspace-server/internal/handlers/cross_tenant_isolation_test.go @@ -294,8 +294,9 @@ func TestProxyA2A_CrossTenant_RoutingDenied(t *testing.T) { // A URL exists for the target; the guard must deny BEFORE it is used. mr.Set(fmt.Sprintf("ws:%s:url", target), "http://localhost:1") - // CanCommunicate: both root-level (parent_id NULL) → its weak "root-level - // siblings" rule ALLOWS this. The org guard must catch it afterward. + // Post-#1955: CanCommunicate no longer has the root-sibling bypass. + // Both root-level (parent_id NULL) but unrelated org roots → hierarchy + // check DENIES with 403 BEFORE the org-scope guard or resolveAgentURL. mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id = "). WithArgs(caller). WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow(caller, nil)) @@ -303,15 +304,6 @@ func TestProxyA2A_CrossTenant_RoutingDenied(t *testing.T) { WithArgs(target). WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow(target, nil)) - // #1953 org-scope guard: caller resolves to org-a-root, target to org-b-root - // → different orgs → 403. (Each org root resolves to itself.) - mock.ExpectQuery("WITH RECURSIVE org_chain AS"). - WithArgs(caller). - WillReturnRows(sqlmock.NewRows([]string{"root_id"}).AddRow(caller)) - mock.ExpectQuery("WITH RECURSIVE org_chain AS"). - WithArgs(target). - WillReturnRows(sqlmock.NewRows([]string{"root_id"}).AddRow(target)) - w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Params = gin.Params{{Key: "id", Value: target}} @@ -329,8 +321,8 @@ func TestProxyA2A_CrossTenant_RoutingDenied(t *testing.T) { if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { t.Fatalf("body not JSON: %v", err) } - if msg, _ := resp["error"].(string); !strings.Contains(msg, "different org") { - t.Errorf("expected cross-org denial message, got %v", resp["error"]) + if msg, _ := resp["error"].(string); !strings.Contains(msg, "cannot communicate") { + t.Errorf("expected hierarchy denial message, got %v", resp["error"]) } if err := mock.ExpectationsWereMet(); err != nil { t.Errorf("unmet sqlmock expectations: %v", err) diff --git a/workspace-server/internal/handlers/delegation_executor_integration_test.go b/workspace-server/internal/handlers/delegation_executor_integration_test.go index 193ca385d..dd7d5cfa8 100644 --- a/workspace-server/internal/handlers/delegation_executor_integration_test.go +++ b/workspace-server/internal/handlers/delegation_executor_integration_test.go @@ -55,6 +55,7 @@ import ( const integrationTestDelegationID = "del-159-test-integration" const integrationTestSourceID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" const integrationTestTargetID = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" +const integrationTestParentID = "cccccccc-cccc-cccc-cccc-cccccccccccc" // rawHTTPServer starts a TCP listener, serves one HTTP response, and closes. // It runs in a background goroutine so the test can proceed immediately after diff --git a/workspace-server/internal/handlers/handlers_additional_test.go b/workspace-server/internal/handlers/handlers_additional_test.go index 9916f0469..64c2af2d4 100644 --- a/workspace-server/internal/handlers/handlers_additional_test.go +++ b/workspace-server/internal/handlers/handlers_additional_test.go @@ -576,13 +576,14 @@ func TestDiscover_TargetOffline(t *testing.T) { setupTestRedis(t) handler := NewDiscoveryHandler() - // Both root-level, access allowed + // Share a parent so communication is allowed under post-#1955 rules + sharedParent := "ws-parent" mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-caller"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-caller", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-caller", sharedParent)) mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-off"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-off", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-off", sharedParent)) // Name + runtime lookup (discovery now queries both) mock.ExpectQuery("SELECT COALESCE"). @@ -622,13 +623,14 @@ func TestCheckAccess_SiblingsAllowed(t *testing.T) { setupTestRedis(t) handler := NewDiscoveryHandler() - // Both root-level siblings → allowed + // Share a parent so communication is allowed under post-#1955 rules + sharedParent := "ws-parent" mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-a"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-a", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-a", sharedParent)) mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-b"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-b", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-b", sharedParent)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) diff --git a/workspace-server/internal/handlers/handlers_extended_test.go b/workspace-server/internal/handlers/handlers_extended_test.go index 59cd59e6e..0e0fecaa3 100644 --- a/workspace-server/internal/handlers/handlers_extended_test.go +++ b/workspace-server/internal/handlers/handlers_extended_test.go @@ -374,14 +374,14 @@ func TestExtended_DiscoverWithCallerID(t *testing.T) { handler := NewDiscoveryHandler() // CanCommunicate needs to look up both workspaces - // Caller: root-level (no parent) + // Share a parent so communication is allowed under post-#1955 rules + sharedParent := "ws-parent" mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-caller"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-caller", nil)) - // Target: also root-level (no parent) — root-level siblings are allowed + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-caller", sharedParent)) mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-target"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-target", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-target", sharedParent)) // Discover handler looks up workspace name + runtime mock.ExpectQuery("SELECT COALESCE"). @@ -515,13 +515,14 @@ func TestExtended_CheckAccess(t *testing.T) { handler := NewDiscoveryHandler() // CanCommunicate will look up both workspaces - // Both root-level — should be allowed + // Share a parent so communication is allowed under post-#1955 rules + sharedParent := "ws-parent" mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-a"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-a", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-a", sharedParent)) mock.ExpectQuery("SELECT id, parent_id FROM workspaces WHERE id ="). WithArgs("ws-b"). - WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-b", nil)) + WillReturnRows(sqlmock.NewRows([]string{"id", "parent_id"}).AddRow("ws-b", sharedParent)) w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) diff --git a/workspace-server/internal/registry/access.go b/workspace-server/internal/registry/access.go index 56a9c419e..8b07624b5 100644 --- a/workspace-server/internal/registry/access.go +++ b/workspace-server/internal/registry/access.go @@ -99,11 +99,6 @@ func CanCommunicate(callerID, targetID string) bool { *caller.ParentID == *target.ParentID { return true } - // Root-level siblings — both have no parent - if caller.ParentID == nil && target.ParentID == nil { - return true - } - // Direct parent → child (fast path; avoids the ancestor walk) if target.ParentID != nil && caller.ID == *target.ParentID { return true diff --git a/workspace-server/internal/registry/access_test.go b/workspace-server/internal/registry/access_test.go index b01a14691..ec20b28bd 100644 --- a/workspace-server/internal/registry/access_test.go +++ b/workspace-server/internal/registry/access_test.go @@ -63,14 +63,16 @@ func TestCanCommunicate_Siblings(t *testing.T) { } } -func TestCanCommunicate_RootSiblings(t *testing.T) { +func TestCanCommunicate_Denied_RootSiblings(t *testing.T) { mock := setupMockDB(t) - // Both at root level (no parent) + // Two unrelated org roots (both parent_id = NULL) must NOT communicate. + // This is the regression test for #1955: without an org_id column, two + // root workspaces have no shared ancestor, so they must be denied. expectLookup(mock, "ws-a", nil) expectLookup(mock, "ws-b", nil) - if !CanCommunicate("ws-a", "ws-b") { - t.Error("root-level siblings should communicate") + if CanCommunicate("ws-a", "ws-b") { + t.Error("unrelated root-level workspaces should NOT communicate") } }