From 8ef8fc4f730670b96f31e85b3da3f1a9a3e397d3 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 18 May 2026 20:44:45 +0700 Subject: [PATCH] fix(skills): scope agent grant status joins --- internal/store/pg/skills_grants.go | 4 ++- internal/store/sqlitestore/skills_grants.go | 5 ++- internal/store/sqlitestore/skills_test.go | 37 ++++++++++++++++++++ tests/integration/v3_skills_store_test.go | 38 +++++++++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index cde3b66b..22f8947a 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -297,8 +297,10 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI return nil, err } tenantCond := "" + grantTenantCond := "" if tc != "" { tenantCond = fmt.Sprintf(" AND (s.is_system = true OR s.tenant_id = $%d)", 2) + grantTenantCond = fmt.Sprintf(" AND sag.tenant_id = $%d", 2) } rows, err := s.db.QueryContext(ctx, `SELECT s.id, s.name, s.slug, COALESCE(s.description, ''), s.visibility, s.version, @@ -307,7 +309,7 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI sag.pinned_version, s.is_system FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1 + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = $1`+grantTenantCond+` WHERE s.status = 'active'`+tenantCond+` ORDER BY s.name`, append([]any{agentID}, tcArgs...)...) if err != nil { diff --git a/internal/store/sqlitestore/skills_grants.go b/internal/store/sqlitestore/skills_grants.go index 088c8d48..d9dba9b6 100644 --- a/internal/store/sqlitestore/skills_grants.go +++ b/internal/store/sqlitestore/skills_grants.go @@ -310,14 +310,17 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid return nil, err } tenantCond := "" + grantTenantCond := "" if len(tArgs) > 0 { tenantCond = " AND (s.is_system = 1 OR s.tenant_id = ?)" + grantTenantCond = " AND sag.tenant_id = ?" } _ = tClause queryArgs := []any{agentID} if len(tArgs) > 0 { queryArgs = append(queryArgs, tArgs...) + queryArgs = append(queryArgs, tArgs...) } rows, err := s.db.QueryContext(ctx, @@ -327,7 +330,7 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid sag.pinned_version, s.is_system FROM skills s - LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ? + LEFT JOIN skill_agent_grants sag ON s.id = sag.skill_id AND sag.agent_id = ?`+grantTenantCond+` WHERE s.status = 'active'`+tenantCond+` ORDER BY s.name`, queryArgs...) if err != nil { diff --git a/internal/store/sqlitestore/skills_test.go b/internal/store/sqlitestore/skills_test.go index fc3d6847..3e640ca2 100644 --- a/internal/store/sqlitestore/skills_test.go +++ b/internal/store/sqlitestore/skills_test.go @@ -140,6 +140,43 @@ func TestSQLiteSkillStore_RevokeFromAgentDoesNotDemoteCrossTenantSkill(t *testin } } +func TestSQLiteSkillStore_ListWithGrantStatusIgnoresForeignTenantGrant(t *testing.T) { + _, skillStore, db := newTestSQLiteSkillStoreWithDB(t) + tenantA, _ := seedSQLiteTenantAgent(t, db) + tenantB, agentB := seedSQLiteTenantAgent(t, db) + ctxA := store.WithTenantID(context.Background(), tenantA) + + skillID := uuid.New() + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES (?, 'System Skill', ?, 'system', 'internal', 1, 'active', ?, 1, ?)`, + skillID.String(), "system-grant-status-"+skillID.String()[:8], filepath.Join(t.TempDir(), "system-skill", "1"), store.MasterTenantID.String(), + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + if _, err := db.Exec( + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, tenant_id) + VALUES (?, ?, ?, 1, 'tenant-b-admin', 1, ?)`, + uuid.New().String(), skillID.String(), agentB.String(), tenantB.String(), + ); err != nil { + t.Fatalf("insert foreign tenant grant: %v", err) + } + + skills, err := skillStore.ListWithGrantStatus(ctxA, agentB) + if err != nil { + t.Fatalf("ListWithGrantStatus error: %v", err) + } + for _, skill := range skills { + if skill.ID == skillID { + if skill.Granted || skill.CanManage { + t.Fatalf("foreign tenant grant leaked into tenant A status: granted=%v canManage=%v", skill.Granted, skill.CanManage) + } + return + } + } + t.Fatalf("system skill %s not returned for tenant A", skillID) +} + func newTestSQLiteSkillStore(t *testing.T) (context.Context, *SQLiteSkillStore) { ctx, skillStore, _ := newTestSQLiteSkillStoreWithDB(t) return ctx, skillStore diff --git a/tests/integration/v3_skills_store_test.go b/tests/integration/v3_skills_store_test.go index 882ac6dc..0ae48a34 100644 --- a/tests/integration/v3_skills_store_test.go +++ b/tests/integration/v3_skills_store_test.go @@ -403,6 +403,44 @@ func TestStoreSkill_RevokeFromAgentDoesNotDemoteCrossTenantSkill(t *testing.T) { } } +func TestStoreSkill_ListWithGrantStatusIgnoresForeignTenantGrant(t *testing.T) { + db := testDB(t) + tenantA, _ := seedTenantAgent(t, db) + tenantB, agentB := seedTenantAgent(t, db) + ctxA := tenantCtx(tenantA) + s := newSkillStore(t) + + skillID := uuid.New() + if _, err := db.Exec( + `INSERT INTO skills (id, name, slug, owner_id, visibility, version, status, file_path, is_system, tenant_id) + VALUES ($1, 'System Skill', $2, 'system', 'internal', 1, 'active', $3, true, $4)`, + skillID, "system-grant-status-"+skillID.String()[:8], "/tmp/skills/system-skill/1", store.MasterTenantID, + ); err != nil { + t.Fatalf("insert system skill: %v", err) + } + if _, err := db.Exec( + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, tenant_id) + VALUES ($1, $2, $3, 1, 'tenant-b-admin', true, $4)`, + uuid.New(), skillID, agentB, tenantB, + ); err != nil { + t.Fatalf("insert foreign tenant grant: %v", err) + } + + skills, err := s.ListWithGrantStatus(ctxA, agentB) + if err != nil { + t.Fatalf("ListWithGrantStatus error: %v", err) + } + for _, skill := range skills { + if skill.ID == skillID { + if skill.Granted || skill.CanManage { + t.Fatalf("foreign tenant grant leaked into tenant A status: granted=%v canManage=%v", skill.Granted, skill.CanManage) + } + return + } + } + t.Fatalf("system skill %s not returned for tenant A", skillID) +} + func TestStoreSkill_TenantIsolation(t *testing.T) { db := testDB(t) tenantA, _ := seedTenantAgent(t, db)