From 0d6c5bbb7cafeb49d9e605f8519e1e50ff0613c7 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Sun, 17 May 2026 23:15:52 +0700 Subject: [PATCH 1/5] fix(skills): add agent manage grants Add explicit per-agent manage grants for skills so granted agents can patch/delete skills when ownership identity drifts. Expose skill owner and manage-grant controls in the web skills UI, and add PostgreSQL/SQLite migrations plus coverage for preserve/revoke behavior. --- docs/project-changelog.md | 18 ++ internal/gateway/methods/skills.go | 10 +- internal/http/skills.go | 3 +- internal/http/skills_grants.go | 34 +++- internal/http/skills_upload_test.go | 8 +- internal/store/pg/skills_grants.go | 70 ++++++- internal/store/skill_store.go | 13 +- internal/store/sqlitestore/schema.go | 7 +- internal/store/sqlitestore/schema.sql | 1 + internal/store/sqlitestore/skills_grants.go | 81 +++++++- internal/tools/publish_skill.go | 2 +- internal/tools/skill_manage.go | 29 ++- internal/upgrade/version.go | 2 +- .../000066_skill_agent_manage_grants.down.sql | 2 + .../000066_skill_agent_manage_grants.up.sql | 2 + tests/integration/v3_skills_store_test.go | 34 ++++ ui/web/src/i18n/locales/en/skills.json | 19 ++ ui/web/src/i18n/locales/vi/skills.json | 21 +- ui/web/src/i18n/locales/zh/skills.json | 21 +- ui/web/src/pages/skills/hooks/use-skills.ts | 31 ++- .../skills/skill-agent-grants-dialog.tsx | 184 ++++++++++++++++++ ui/web/src/pages/skills/skill-table-row.tsx | 17 +- ui/web/src/pages/skills/skills-page.tsx | 15 ++ ui/web/src/types/skill.ts | 9 + 24 files changed, 595 insertions(+), 38 deletions(-) create mode 100644 migrations/000066_skill_agent_manage_grants.down.sql create mode 100644 migrations/000066_skill_agent_manage_grants.up.sql create mode 100644 ui/web/src/pages/skills/skill-agent-grants-dialog.tsx diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 596fc922..e5fcd321 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,24 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-17 +### Skills: agent manage grants + +**Fixes** + +- Added explicit per-agent skill manage grants so agents can edit/delete skills they were authorized to maintain even when `owner_id` no longer matches their current actor identity. +- Auto-granted manage permission to the creating/publishing agent for new managed skills. + +**UI** + +- Show custom skill owner IDs in the Skills table. +- Added Skills page controls to grant agent skill access and edit/delete permission. + +**Tests** + +- Added PG/SQLite grant coverage and verified Go builds plus Web UI build. + +--- + ### Agents: provider switch save fix **Fixes** diff --git a/internal/gateway/methods/skills.go b/internal/gateway/methods/skills.go index ac5349d1..3d2caf1f 100644 --- a/internal/gateway/methods/skills.go +++ b/internal/gateway/methods/skills.go @@ -17,7 +17,7 @@ import ( // skillOwnerGetter is an optional interface for stores that can return a skill's owner ID. type skillOwnerGetter interface { - GetSkillOwnerID(id uuid.UUID) (string, bool) + GetSkillOwnerID(ctx context.Context, id uuid.UUID) (string, bool) } // SkillsMethods handles skills.list, skills.get, skills.update. @@ -56,6 +56,9 @@ func (m *SkillsMethods) handleList(ctx context.Context, client *gateway.Client, "is_system": s.IsSystem, "enabled": s.Enabled, } + if s.OwnerID != "" { + entry["owner_id"] = s.OwnerID + } if s.ID != "" { entry["id"] = s.ID } @@ -146,6 +149,9 @@ func (m *SkillsMethods) handleGet(ctx context.Context, client *gateway.Client, r if info.Visibility != "" { resp["visibility"] = info.Visibility } + if info.OwnerID != "" { + resp["owner_id"] = info.OwnerID + } if len(info.Tags) > 0 { resp["tags"] = info.Tags } @@ -219,7 +225,7 @@ func (m *SkillsMethods) handleUpdate(ctx context.Context, client *gateway.Client client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "skills.update"))) return } - if ownerID, found := ownerGetter.GetSkillOwnerID(skillID); found && ownerID != client.UserID() { + if ownerID, found := ownerGetter.GetSkillOwnerID(ctx, skillID); found && ownerID != client.UserID() { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrUnauthorized, i18n.T(locale, i18n.MsgPermissionDenied, "skills.update"))) return } diff --git a/internal/http/skills.go b/internal/http/skills.go index 52078ca3..ee1e17dc 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -37,7 +37,7 @@ type SkillsHandler struct { msgBus *bus.MessageBus tenantCfgStore store.SkillTenantConfigStore tenantStore store.TenantStore - db *sql.DB // for export/import direct queries + db *sql.DB // for export/import direct queries uploadLocks sync.Map // per-slug mutex; bounded by validated slug set, entries are tiny (*sync.Mutex) } @@ -92,6 +92,7 @@ func (h *SkillsHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("PUT /v1/skills/{id}", h.adminMiddleware(h.handleUpdate)) mux.HandleFunc("DELETE /v1/skills/{id}", h.adminMiddleware(h.handleDelete)) // Skill grants (admin+) + mux.HandleFunc("GET /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleListAgentGrants)) mux.HandleFunc("POST /v1/skills/{id}/grants/agent", h.adminMiddleware(h.handleGrantAgent)) mux.HandleFunc("DELETE /v1/skills/{id}/grants/agent/{agentID}", h.adminMiddleware(h.handleRevokeAgent)) mux.HandleFunc("POST /v1/skills/{id}/grants/user", h.adminMiddleware(h.handleGrantUser)) diff --git a/internal/http/skills_grants.go b/internal/http/skills_grants.go index d359b94b..eccebcad 100644 --- a/internal/http/skills_grants.go +++ b/internal/http/skills_grants.go @@ -33,6 +33,25 @@ func (h *SkillsHandler) handleListAgentSkills(w http.ResponseWriter, r *http.Req writeJSON(w, http.StatusOK, map[string]any{"skills": skills}) } +func (h *SkillsHandler) handleListAgentGrants(w http.ResponseWriter, r *http.Request) { + locale := store.LocaleFromContext(r.Context()) + idStr := r.PathValue("id") + skillID, err := uuid.Parse(idStr) + if err != nil { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidID, "skill")}) + return + } + + grants, err := h.skills.ListAgentGrantsForSkill(r.Context(), skillID) + if err != nil { + slog.Error("failed to list skill agent grants", "skill_id", skillID, "error", err) + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgFailedToList, "skill grants")}) + return + } + + writeJSON(w, http.StatusOK, map[string]any{"grants": grants}) +} + func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) { locale := store.LocaleFromContext(r.Context()) userID := store.UserIDFromContext(r.Context()) @@ -53,8 +72,9 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) } var req struct { - AgentID string `json:"agent_id"` - Version int `json:"version"` + AgentID string `json:"agent_id"` + Version int `json:"version"` + CanManage *bool `json:"can_manage"` } if !bindJSON(w, r, locale, &req) { return @@ -70,8 +90,14 @@ func (h *SkillsHandler) handleGrantAgent(w http.ResponseWriter, r *http.Request) req.Version = 1 } - if err := h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID); err != nil { - writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) + var grantErr error + if req.CanManage == nil { + grantErr = h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID) + } else { + grantErr = h.skills.GrantToAgent(r.Context(), skillID, agentID, req.Version, userID, *req.CanManage) + } + if grantErr != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": grantErr.Error()}) return } diff --git a/internal/http/skills_upload_test.go b/internal/http/skills_upload_test.go index f8dca60a..f8ae2935 100644 --- a/internal/http/skills_upload_test.go +++ b/internal/http/skills_upload_test.go @@ -528,7 +528,7 @@ func (s *skillManageStoreStub) StoreMissingDeps(_ context.Context, id uuid.UUID, s.skills[id] = skill return nil } -func (s *skillManageStoreStub) GrantToAgent(context.Context, uuid.UUID, uuid.UUID, int, string) error { +func (s *skillManageStoreStub) GrantToAgent(context.Context, uuid.UUID, uuid.UUID, int, string, ...bool) error { return nil } func (s *skillManageStoreStub) RevokeFromAgent(context.Context, uuid.UUID, uuid.UUID) error { @@ -541,6 +541,12 @@ func (s *skillManageStoreStub) RevokeFromUser(context.Context, uuid.UUID, string func (s *skillManageStoreStub) ListWithGrantStatus(context.Context, uuid.UUID) ([]store.SkillWithGrantStatus, error) { return nil, nil } +func (s *skillManageStoreStub) ListAgentGrantsForSkill(context.Context, uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + return nil, nil +} +func (s *skillManageStoreStub) AgentCanManageSkill(context.Context, uuid.UUID, uuid.UUID) (bool, error) { + return false, nil +} func (s *skillManageStoreStub) GetSkillFilePath(context.Context, uuid.UUID) (string, string, int, bool, bool) { return "", "", 0, false, false } diff --git a/internal/store/pg/skills_grants.go b/internal/store/pg/skills_grants.go index 5a42f698..d2c2990c 100644 --- a/internal/store/pg/skills_grants.go +++ b/internal/store/pg/skills_grants.go @@ -2,6 +2,7 @@ package pg import ( "context" + "database/sql" "fmt" "log/slog" "time" @@ -15,12 +16,11 @@ import ( // Auto-promotes visibility from 'private' to 'internal' so the skill // becomes accessible via ListAccessible for granted agents. // Validates the agent belongs to the requesting tenant (prevents cross-tenant grant injection). -func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error { +func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error { if err := store.ValidateUserID(grantedBy); err != nil { return err } tid := tenantIDForInsert(ctx) - // Verify agent belongs to the requesting tenant. var agentTenantID uuid.UUID if err := s.db.QueryRowContext(ctx, @@ -32,12 +32,28 @@ func (s *PGSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.U return fmt.Errorf("agent not found") } - _, err := s.db.ExecContext(ctx, - `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) - VALUES ($1, $2, $3, $4, $5, $6, $7) - ON CONFLICT (skill_id, agent_id) DO UPDATE SET pinned_version = EXCLUDED.pinned_version`, - store.GenNewID(), skillID, agentID, version, grantedBy, time.Now(), tid, - ) + now := time.Now() + var err error + if len(canManage) > 0 { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, created_at, tenant_id) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = EXCLUDED.pinned_version, + granted_by = EXCLUDED.granted_by, + can_manage = EXCLUDED.can_manage`, + store.GenNewID(), skillID, agentID, version, grantedBy, canManage[0], now, tid, + ) + } else { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) + VALUES ($1, $2, $3, $4, $5, $6, $7) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = EXCLUDED.pinned_version, + granted_by = EXCLUDED.granted_by`, + store.GenNewID(), skillID, agentID, version, grantedBy, now, tid, + ) + } if err != nil { return err } @@ -101,6 +117,41 @@ func (s *PGSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUID) ( return result, nil } +// ListAgentGrantsForSkill returns all agent grants for one skill. +func (s *PGSkillStore) ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + tClause, tArgs, _, err := scopeClause(ctx, 2) + if err != nil { + return nil, err + } + var result []store.SkillAgentGrantInfo + err = pkgSqlxDB.SelectContext(ctx, &result, + "SELECT agent_id, pinned_version, granted_by, can_manage FROM skill_agent_grants WHERE skill_id = $1"+tClause+" ORDER BY created_at DESC", + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + return result, nil +} + +// AgentCanManageSkill reports whether an agent has explicit edit/delete rights for a skill. +func (s *PGSkillStore) AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) { + tClause, tArgs, _, err := scopeClause(ctx, 3) + if err != nil { + return false, err + } + var canManage bool + err = s.db.QueryRowContext(ctx, + "SELECT can_manage FROM skill_agent_grants WHERE skill_id = $1 AND agent_id = $2"+tClause, + append([]any{skillID, agentID}, tArgs...)...).Scan(&canManage) + if err == sql.ErrNoRows { + return false, nil + } + if err != nil { + return false, err + } + return canManage, nil +} + // GrantToUser grants a skill to a user (for internal visibility skills). func (s *PGSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error { if err := store.ValidateUserID(userID); err != nil { @@ -216,6 +267,7 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI rows, err := s.db.QueryContext(ctx, `SELECT s.id, s.name, s.slug, COALESCE(s.description, ''), s.visibility, s.version, (sag.id IS NOT NULL) AS granted, + COALESCE(sag.can_manage, false) AS can_manage, sag.pinned_version, s.is_system FROM skills s @@ -230,7 +282,7 @@ func (s *PGSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid.UUI var result []store.SkillWithGrantStatus for rows.Next() { var r store.SkillWithGrantStatus - if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, &r.Version, &r.Granted, &r.PinnedVer, &r.IsSystem); err != nil { + if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, &r.Version, &r.Granted, &r.CanManage, &r.PinnedVer, &r.IsSystem); err != nil { slog.Warn("skill_grants: scan error in ListWithGrantStatus", "error", err) continue } diff --git a/internal/store/skill_store.go b/internal/store/skill_store.go index 478725ee..4a976acf 100644 --- a/internal/store/skill_store.go +++ b/internal/store/skill_store.go @@ -89,10 +89,19 @@ type SkillWithGrantStatus struct { Visibility string `json:"visibility" db:"visibility"` Version int `json:"version" db:"version"` Granted bool `json:"granted" db:"granted"` + CanManage bool `json:"can_manage" db:"can_manage"` PinnedVer *int `json:"pinned_version,omitempty" db:"pinned_version"` IsSystem bool `json:"is_system" db:"is_system"` } +// SkillAgentGrantInfo is a grant row for one skill across agents. +type SkillAgentGrantInfo struct { + AgentID uuid.UUID `json:"agent_id" db:"agent_id"` + PinnedVersion int `json:"pinned_version" db:"pinned_version"` + GrantedBy string `json:"granted_by" db:"granted_by"` + CanManage bool `json:"can_manage" db:"can_manage"` +} + // SkillManageStore extends SkillStore with CRUD, ownership, and grant operations // needed by HTTP upload handlers and agent tools (skill_manage, publish_skill). // Implemented by both PGSkillStore and SQLiteSkillStore. @@ -119,11 +128,13 @@ type SkillManageStore interface { ListSystemSkillDirs(ctx context.Context) map[string]string StoreMissingDeps(ctx context.Context, id uuid.UUID, missing []string) error // Grants - GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error + GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error RevokeFromAgent(ctx context.Context, skillID, agentID uuid.UUID) error GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error RevokeFromUser(ctx context.Context, skillID uuid.UUID, userID string) error ListWithGrantStatus(ctx context.Context, agentID uuid.UUID) ([]SkillWithGrantStatus, error) + ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]SkillAgentGrantInfo, error) + AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) // Files GetSkillFilePath(ctx context.Context, id uuid.UUID) (filePath string, slug string, version int, isSystem bool, ok bool) } diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index b06ba9b3..208bae97 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 34 +const SchemaVersion = 35 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -596,6 +596,9 @@ CREATE INDEX IF NOT EXISTS idx_ws_activity_retention ON workstation_activity(c // Version 33 → 34: per-agent ordered provider/model fallback config. 33: `ALTER TABLE agents ADD COLUMN model_fallback TEXT NOT NULL DEFAULT '{}';`, + // Version 34 → 35: agent skill grants can optionally allow skill management. + 34: `ALTER TABLE skill_agent_grants ADD COLUMN can_manage INTEGER NOT NULL DEFAULT 0;`, + // Version 23 → 24: vault_documents scope/ownership consistency triggers. // Mirrors PG migration 000055 CHECK constraint; SQLite cannot add CHECK via // ALTER TABLE so we use BEFORE INSERT + BEFORE UPDATE triggers instead. @@ -975,6 +978,8 @@ func idempotentColumnMigration(version int) (string, string, bool) { return "webhooks", "encrypted_secret", true case 33: return "agents", "model_fallback", true + case 34: + return "skill_agent_grants", "can_manage", true default: return "", "", false } diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index b3d749c5..553e71ee 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -408,6 +408,7 @@ CREATE TABLE IF NOT EXISTS skill_agent_grants ( agent_id TEXT NOT NULL REFERENCES agents(id) ON DELETE CASCADE, pinned_version INT NOT NULL, granted_by VARCHAR(255) NOT NULL, + can_manage INTEGER NOT NULL DEFAULT 0, tenant_id TEXT NOT NULL REFERENCES tenants(id), created_at TEXT DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), UNIQUE(skill_id, agent_id) diff --git a/internal/store/sqlitestore/skills_grants.go b/internal/store/sqlitestore/skills_grants.go index 344189c8..bf2bba9d 100644 --- a/internal/store/sqlitestore/skills_grants.go +++ b/internal/store/sqlitestore/skills_grants.go @@ -4,6 +4,7 @@ package sqlitestore import ( "context" + "database/sql" "log/slog" "time" @@ -20,19 +21,35 @@ type SkillGrantInfo struct { } // GrantToAgent grants a skill to an agent with version pinning. -func (s *SQLiteSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string) error { +func (s *SQLiteSkillStore) GrantToAgent(ctx context.Context, skillID, agentID uuid.UUID, version int, grantedBy string, canManage ...bool) error { if err := store.ValidateUserID(grantedBy); err != nil { return err } - // Upsert grant. id := store.GenNewID() - _, err := s.db.ExecContext(ctx, - `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) - VALUES (?, ?, ?, ?, ?, ?, ?) - ON CONFLICT (skill_id, agent_id) DO UPDATE SET pinned_version = excluded.pinned_version`, - id, skillID, agentID, version, grantedBy, time.Now().UTC(), tenantIDForInsert(ctx), - ) + now := time.Now().UTC() + tid := tenantIDForInsert(ctx) + var err error + if len(canManage) > 0 { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, can_manage, created_at, tenant_id) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = excluded.pinned_version, + granted_by = excluded.granted_by, + can_manage = excluded.can_manage`, + id, skillID, agentID, version, grantedBy, canManage[0], now, tid, + ) + } else { + _, err = s.db.ExecContext(ctx, + `INSERT INTO skill_agent_grants (id, skill_id, agent_id, pinned_version, granted_by, created_at, tenant_id) + VALUES (?, ?, ?, ?, ?, ?, ?) + ON CONFLICT (skill_id, agent_id) DO UPDATE SET + pinned_version = excluded.pinned_version, + granted_by = excluded.granted_by`, + id, skillID, agentID, version, grantedBy, now, tid, + ) + } if err != nil { return err } @@ -102,6 +119,51 @@ func (s *SQLiteSkillStore) ListAgentGrants(ctx context.Context, agentID uuid.UUI return result, rows.Err() } +// ListAgentGrantsForSkill returns all agent grants for one skill. +func (s *SQLiteSkillStore) ListAgentGrantsForSkill(ctx context.Context, skillID uuid.UUID) ([]store.SkillAgentGrantInfo, error) { + tClause, tArgs, err := scopeClause(ctx) + if err != nil { + return nil, err + } + rows, err := s.db.QueryContext(ctx, + "SELECT agent_id, pinned_version, granted_by, can_manage FROM skill_agent_grants WHERE skill_id = ?"+tClause+" ORDER BY created_at DESC", + append([]any{skillID}, tArgs...)...) + if err != nil { + return nil, err + } + defer rows.Close() + + var result []store.SkillAgentGrantInfo + for rows.Next() { + var g store.SkillAgentGrantInfo + if err := rows.Scan(&g.AgentID, &g.PinnedVersion, &g.GrantedBy, &g.CanManage); err != nil { + slog.Warn("skill_grants: scan error in ListAgentGrantsForSkill", "error", err) + continue + } + result = append(result, g) + } + return result, rows.Err() +} + +// AgentCanManageSkill reports whether an agent has explicit edit/delete rights for a skill. +func (s *SQLiteSkillStore) AgentCanManageSkill(ctx context.Context, skillID, agentID uuid.UUID) (bool, error) { + tClause, tArgs, err := scopeClause(ctx) + if err != nil { + return false, err + } + var canManage bool + err = s.db.QueryRowContext(ctx, + "SELECT can_manage FROM skill_agent_grants WHERE skill_id = ? AND agent_id = ?"+tClause, + append([]any{skillID, agentID}, tArgs...)...).Scan(&canManage) + if err == sql.ErrNoRows { + return false, nil + } + if err != nil { + return false, err + } + return canManage, nil +} + // GrantToUser grants a skill to a user. func (s *SQLiteSkillStore) GrantToUser(ctx context.Context, skillID uuid.UUID, userID, grantedBy string) error { if err := store.ValidateUserID(userID); err != nil { @@ -214,6 +276,7 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid rows, err := s.db.QueryContext(ctx, `SELECT s.id, s.name, s.slug, COALESCE(s.description, ''), s.visibility, s.version, (sag.id IS NOT NULL) AS granted, + COALESCE(sag.can_manage, 0) AS can_manage, sag.pinned_version, s.is_system FROM skills s @@ -229,7 +292,7 @@ func (s *SQLiteSkillStore) ListWithGrantStatus(ctx context.Context, agentID uuid for rows.Next() { var r store.SkillWithGrantStatus if err := rows.Scan(&r.ID, &r.Name, &r.Slug, &r.Description, &r.Visibility, - &r.Version, &r.Granted, &r.PinnedVer, &r.IsSystem); err != nil { + &r.Version, &r.Granted, &r.CanManage, &r.PinnedVer, &r.IsSystem); err != nil { slog.Warn("skill_grants: scan error in ListWithGrantStatus", "error", err) continue } diff --git a/internal/tools/publish_skill.go b/internal/tools/publish_skill.go index dd3f50a7..227033ce 100644 --- a/internal/tools/publish_skill.go +++ b/internal/tools/publish_skill.go @@ -170,7 +170,7 @@ func (t *PublishSkillTool) Execute(ctx context.Context, args map[string]any) *Re // Auto-grant to calling agent (granted-by = owner, same as CreateSkillManaged) agentID := store.AgentIDFromContext(ctx) if agentID != uuid.Nil { - if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID); err != nil { + if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID, true); err != nil { slog.Warn("publish_skill: auto-grant failed", "error", err) } } diff --git a/internal/tools/skill_manage.go b/internal/tools/skill_manage.go index 16b70545..aabcd6b1 100644 --- a/internal/tools/skill_manage.go +++ b/internal/tools/skill_manage.go @@ -51,6 +51,29 @@ func isOwnerOfSkill(ctx context.Context, skills store.SkillManageStore, slug str return ownerID == actorID || ownerID == userID || ownerID == senderID } +func canManageSkill(ctx context.Context, skills store.SkillManageStore, info *store.SkillInfo) bool { + if isOwnerOfSkill(ctx, skills, info.Slug) { + return true + } + if info.ID == "" { + return false + } + skillID, err := uuid.Parse(info.ID) + if err != nil { + return false + } + agentID := store.AgentIDFromContext(ctx) + if agentID == uuid.Nil { + return false + } + ok, err := skills.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + slog.Warn("skill_manage: manage grant check failed", "skill", info.Slug, "agent_id", agentID, "error", err) + return false + } + return ok +} + // tenantSkillsDir returns the skills-store directory scoped to the calling agent's tenant. func (t *SkillManageTool) tenantSkillsDir(ctx context.Context) string { tid := store.TenantIDFromContext(ctx) @@ -211,7 +234,7 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any granted := false agentID := store.AgentIDFromContext(ctx) if agentID != uuid.Nil { - if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID); err != nil { + if err := t.skills.GrantToAgent(ctx, id, agentID, version, ownerID, true); err != nil { slog.Warn("skill_manage: auto-grant failed", "error", err) } else { granted = true @@ -278,7 +301,7 @@ func (t *SkillManageTool) executePatch(ctx context.Context, args map[string]any) // where DM owners got the raw channel sender) // A DM user merged to "viettx" with Telegram ID "386246614" matches all // three of their skills regardless of when they were created. - if !isOwnerOfSkill(ctx, t.skills, slug) { + if !canManageSkill(ctx, t.skills, info) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } @@ -392,7 +415,7 @@ func (t *SkillManageTool) executeDelete(ctx context.Context, args map[string]any // Ownership check: only the skill owner can delete. // Same three-identity match as the patch flow above (#915). - if !isOwnerOfSkill(ctx, t.skills, slug) { + if !canManageSkill(ctx, t.skills, info) { return ErrorResult(fmt.Sprintf("cannot manage skill %q: you are not the owner", slug)) } diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index ab3b06d7..4a4ce17b 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 65 +const RequiredSchemaVersion uint = 66 diff --git a/migrations/000066_skill_agent_manage_grants.down.sql b/migrations/000066_skill_agent_manage_grants.down.sql new file mode 100644 index 00000000..6ff8efba --- /dev/null +++ b/migrations/000066_skill_agent_manage_grants.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE skill_agent_grants + DROP COLUMN IF EXISTS can_manage; diff --git a/migrations/000066_skill_agent_manage_grants.up.sql b/migrations/000066_skill_agent_manage_grants.up.sql new file mode 100644 index 00000000..8cf33e67 --- /dev/null +++ b/migrations/000066_skill_agent_manage_grants.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE skill_agent_grants + ADD COLUMN IF NOT EXISTS can_manage BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/tests/integration/v3_skills_store_test.go b/tests/integration/v3_skills_store_test.go index 5d9cd130..0def8c97 100644 --- a/tests/integration/v3_skills_store_test.go +++ b/tests/integration/v3_skills_store_test.go @@ -271,6 +271,9 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { if !g.Granted { t.Error("expected Granted=true for granted skill") } + if g.CanManage { + t.Error("expected CanManage=false by default") + } found = true break } @@ -295,6 +298,37 @@ func TestStoreSkill_GrantToAgent(t *testing.T) { t.Error("granted skill not found in ListAccessible") } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner", true); err != nil { + t.Fatalf("GrantToAgent can_manage: %v", err) + } + canManage, err := s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill: %v", err) + } + if !canManage { + t.Error("expected AgentCanManageSkill=true after manage grant") + } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner"); err != nil { + t.Fatalf("GrantToAgent preserve can_manage: %v", err) + } + canManage, err = s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill after preserve grant: %v", err) + } + if !canManage { + t.Error("expected omitted can_manage grant update to preserve existing manage permission") + } + if err := s.GrantToAgent(ctx, skillID, agentID, 1, "test-owner", false); err != nil { + t.Fatalf("GrantToAgent can_manage false: %v", err) + } + canManage, err = s.AgentCanManageSkill(ctx, skillID, agentID) + if err != nil { + t.Fatalf("AgentCanManageSkill after false grant: %v", err) + } + if canManage { + t.Error("expected explicit can_manage=false to revoke manage permission") + } + // Revoke if err := s.RevokeFromAgent(ctx, skillID, agentID); err != nil { t.Fatalf("RevokeFromAgent: %v", err) diff --git a/ui/web/src/i18n/locales/en/skills.json b/ui/web/src/i18n/locales/en/skills.json index a5f1dce3..c18380f8 100644 --- a/ui/web/src/i18n/locales/en/skills.json +++ b/ui/web/src/i18n/locales/en/skills.json @@ -11,11 +11,30 @@ "description": "Description", "source": "Source", "author": "Author", + "owner": "Owner", "visibility": "Visibility", "status": "Status", "actions": "Actions" }, "noDescription": "No description", + "owner": "Owner", + "unknownOwner": "Unknown", + "grants": { + "title": "Agent access for {{name}}", + "manage": "Manage agent access", + "current": "Current agent grants", + "none": "No agent grants yet.", + "add": "Add grant", + "update": "Update grant", + "grant": "Grant", + "save": "Save", + "selectAgent": "Select agent", + "allowManage": "Allow this agent to edit or delete the skill", + "canManage": "Can edit", + "loadFailed": "Failed to load grants", + "saveFailed": "Failed to save grant", + "revokeFailed": "Failed to revoke grant" + }, "visibility": { "clickToCycle": "Click to change visibility" }, diff --git a/ui/web/src/i18n/locales/vi/skills.json b/ui/web/src/i18n/locales/vi/skills.json index 9f35cecb..c99e9813 100644 --- a/ui/web/src/i18n/locales/vi/skills.json +++ b/ui/web/src/i18n/locales/vi/skills.json @@ -13,7 +13,8 @@ "author": "Tác giả", "visibility": "Hiển thị", "status": "Trạng thái", - "actions": "Thao tác" + "actions": "Thao tác", + "owner": "Chủ sở hữu" }, "noDescription": "Không có mô tả", "visibility": { @@ -125,5 +126,23 @@ "rescanUpdated": "Đã cập nhật {{count}} skill", "rescanNoChanges": "Tất cả skill đã cập nhật", "rescanFailed": "Không thể quét lại dependencies" + }, + "owner": "Chủ sở hữu", + "unknownOwner": "Không rõ", + "grants": { + "title": "Quyền agent cho {{name}}", + "manage": "Quản lý quyền agent", + "current": "Grant agent hiện tại", + "none": "Chưa có grant agent.", + "add": "Thêm grant", + "update": "Cập nhật grant", + "grant": "Grant", + "save": "Lưu", + "selectAgent": "Chọn agent", + "allowManage": "Cho phép agent này sửa hoặc xóa skill", + "canManage": "Được sửa", + "loadFailed": "Không thể tải grant", + "saveFailed": "Không thể lưu grant", + "revokeFailed": "Không thể thu hồi grant" } } diff --git a/ui/web/src/i18n/locales/zh/skills.json b/ui/web/src/i18n/locales/zh/skills.json index 1ec658e9..d74788d5 100644 --- a/ui/web/src/i18n/locales/zh/skills.json +++ b/ui/web/src/i18n/locales/zh/skills.json @@ -13,7 +13,8 @@ "author": "作者", "visibility": "可见性", "status": "状态", - "actions": "操作" + "actions": "操作", + "owner": "所有者" }, "noDescription": "暂无描述", "visibility": { @@ -125,5 +126,23 @@ "rescanUpdated": "已更新 {{count}} 个技能", "rescanNoChanges": "所有技能已是最新", "rescanFailed": "重新扫描依赖失败" + }, + "owner": "所有者", + "unknownOwner": "未知", + "grants": { + "title": "{{name}} 的 Agent 权限", + "manage": "管理 Agent 权限", + "current": "当前 Agent 授权", + "none": "暂无 Agent 授权。", + "add": "添加授权", + "update": "更新授权", + "grant": "授权", + "save": "保存", + "selectAgent": "选择 Agent", + "allowManage": "允许此 Agent 编辑或删除 Skill", + "canManage": "可编辑", + "loadFailed": "无法加载授权", + "saveFailed": "无法保存授权", + "revokeFailed": "无法撤销授权" } } diff --git a/ui/web/src/pages/skills/hooks/use-skills.ts b/ui/web/src/pages/skills/hooks/use-skills.ts index a842c67e..8dc3ac9a 100644 --- a/ui/web/src/pages/skills/hooks/use-skills.ts +++ b/ui/web/src/pages/skills/hooks/use-skills.ts @@ -7,7 +7,7 @@ import { queryKeys } from "@/lib/query-keys"; import { toast } from "@/stores/use-toast-store"; import i18next from "i18next"; import { userFriendlyError } from "@/lib/error-utils"; -import type { SkillInfo, SkillFile, SkillVersions } from "@/types/skill"; +import type { SkillInfo, SkillFile, SkillVersions, SkillAgentGrant } from "@/types/skill"; export type { SkillInfo, SkillFile, SkillVersions }; @@ -105,6 +105,34 @@ export function useSkills() { [http, invalidate], ); + const listAgentGrants = useCallback( + async (id: string) => { + const res = await http.get<{ grants: SkillAgentGrant[] }>(`/v1/skills/${id}/grants/agent`); + return res.grants ?? []; + }, + [http], + ); + + const grantSkillToAgent = useCallback( + async (id: string, agentId: string, version: number, canManage: boolean) => { + await http.post<{ ok: string }>(`/v1/skills/${id}/grants/agent`, { + agent_id: agentId, + version, + can_manage: canManage, + }); + await invalidate(); + }, + [http, invalidate], + ); + + const revokeSkillFromAgent = useCallback( + async (id: string, agentId: string) => { + await http.delete<{ ok: string }>(`/v1/skills/${id}/grants/agent/${agentId}`); + await invalidate(); + }, + [http, invalidate], + ); + const getSkillVersions = useCallback( async (id: string) => { return http.get(`/v1/skills/${id}/versions`); @@ -220,6 +248,7 @@ export function useSkills() { return { skills, loading, refresh: invalidate, getSkill, uploadSkill, updateSkill, deleteSkill, + listAgentGrants, grantSkillToAgent, revokeSkillFromAgent, getSkillVersions, getSkillFiles, getSkillFileContent, rescanDeps, installDeps, installSingleDep, toggleSkill, setTenantConfig, deleteTenantConfig, }; diff --git a/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx b/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx new file mode 100644 index 00000000..f9ecb9d1 --- /dev/null +++ b/ui/web/src/pages/skills/skill-agent-grants-dialog.tsx @@ -0,0 +1,184 @@ +import { useEffect, useMemo, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { Plus, Trash2, ShieldCheck } from "lucide-react"; +import { + Dialog, + DialogContent, + DialogHeader, + DialogTitle, +} from "@/components/ui/dialog"; +import { Button } from "@/components/ui/button"; +import { Badge } from "@/components/ui/badge"; +import { Label } from "@/components/ui/label"; +import { Switch } from "@/components/ui/switch"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@/components/ui/select"; +import { useAgents } from "@/pages/agents/hooks/use-agents"; +import type { SkillAgentGrant, SkillInfo } from "@/types/skill"; + +interface SkillAgentGrantsDialogProps { + skill: SkillInfo; + onClose: () => void; + onLoad: (skillId: string) => Promise; + onGrant: (skillId: string, agentId: string, version: number, canManage: boolean) => Promise; + onRevoke: (skillId: string, agentId: string) => Promise; +} + +export function SkillAgentGrantsDialog({ + skill, + onClose, + onLoad, + onGrant, + onRevoke, +}: SkillAgentGrantsDialogProps) { + const { t } = useTranslation("skills"); + const { agents } = useAgents(); + const [grants, setGrants] = useState([]); + const [agentId, setAgentId] = useState(""); + const [canManage, setCanManage] = useState(false); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(""); + + useEffect(() => { + if (!skill.id) return; + setLoading(true); + setError(""); + onLoad(skill.id) + .then(setGrants) + .catch((err) => setError(err instanceof Error ? err.message : t("grants.loadFailed"))) + .finally(() => setLoading(false)); + }, [skill.id, onLoad, t]); + + const agentNames = useMemo(() => { + const map = new Map(); + for (const agent of agents) map.set(agent.id, agent.display_name || agent.agent_key); + return map; + }, [agents]); + + const selectedGrant = grants.find((grant) => grant.agent_id === agentId); + + useEffect(() => { + setCanManage(selectedGrant?.can_manage ?? false); + }, [selectedGrant]); + + const handleGrant = async () => { + if (!skill.id || !agentId) return; + setLoading(true); + setError(""); + try { + await onGrant(skill.id, agentId, skill.version ?? 1, canManage); + setGrants((current) => { + const next: SkillAgentGrant = { + agent_id: agentId, + pinned_version: skill.version ?? 1, + granted_by: "", + can_manage: canManage, + }; + if (current.some((grant) => grant.agent_id === agentId)) { + return current.map((grant) => (grant.agent_id === agentId ? next : grant)); + } + return [...current, next]; + }); + setAgentId(""); + setCanManage(false); + } catch (err) { + setError(err instanceof Error ? err.message : t("grants.saveFailed")); + } finally { + setLoading(false); + } + }; + + const handleRevoke = async (grant: SkillAgentGrant) => { + if (!skill.id) return; + setLoading(true); + setError(""); + try { + await onRevoke(skill.id, grant.agent_id); + setGrants((current) => current.filter((item) => item.agent_id !== grant.agent_id)); + if (agentId === grant.agent_id) setAgentId(""); + } catch (err) { + setError(err instanceof Error ? err.message : t("grants.revokeFailed")); + } finally { + setLoading(false); + } + }; + + return ( + !open && onClose()}> + + + {t("grants.title", { name: skill.name })} + + +
+
+ {t("owner")}:{" "} + {skill.owner_id || t("unknownOwner")} +
+ +
+ + {grants.length === 0 ? ( +

{t("grants.none")}

+ ) : ( +
+ {grants.map((grant) => ( +
+
+

{agentNames.get(grant.agent_id) || grant.agent_id}

+
+ v{grant.pinned_version} + {grant.can_manage && ( + + {t("grants.canManage")} + + )} +
+
+ +
+ ))} +
+ )} +
+ +
+ + + + +
+ + {error &&

{error}

} +
+ + + ); +} diff --git a/ui/web/src/pages/skills/skill-table-row.tsx b/ui/web/src/pages/skills/skill-table-row.tsx index 71a16a46..078fff42 100644 --- a/ui/web/src/pages/skills/skill-table-row.tsx +++ b/ui/web/src/pages/skills/skill-table-row.tsx @@ -1,5 +1,5 @@ import { useTranslation } from "react-i18next"; -import { Zap, Pencil, Trash2 } from "lucide-react"; +import { Zap, Pencil, Trash2, Users } from "lucide-react"; import { Button } from "@/components/ui/button"; import { Badge } from "@/components/ui/badge"; import { Switch } from "@/components/ui/switch"; @@ -20,6 +20,7 @@ interface SkillTableRowProps { toggling: string | null; onView: (name: string) => void; onEdit: (skill: SkillInfo) => void; + onManageGrants: (skill: SkillInfo) => void; onDelete: (skill: SkillInfo) => void; onToggle: (skill: SkillInfo, enabled: boolean) => void; onCycleVisibility: (skill: SkillInfo) => void; @@ -30,7 +31,7 @@ interface SkillTableRowProps { /** Single row in the skills table with inline status, visibility, and action controls. */ export function SkillTableRow({ skill, tab, hasTenantScope, toggling, - onView, onEdit, onDelete, onToggle, onCycleVisibility, + onView, onEdit, onManageGrants, onDelete, onToggle, onCycleVisibility, onSetTenantConfig, onDeleteTenantConfig, }: SkillTableRowProps) { const { t } = useTranslation("skills"); @@ -64,6 +65,13 @@ export function SkillTableRow({ {tab === "custom" && ( {skill.author || "—"} )} + {tab === "custom" && ( + + + {skill.owner_id || t("unknownOwner")} + + + )}
onEdit(skill)} className="gap-1"> + {!skill.is_system && ( + + )} {!skill.is_system && (
diff --git a/ui/web/src/pages/skills/skill-table-row.tsx b/ui/web/src/pages/skills/skill-table-row.tsx index 078fff42..97bc6bbb 100644 --- a/ui/web/src/pages/skills/skill-table-row.tsx +++ b/ui/web/src/pages/skills/skill-table-row.tsx @@ -65,13 +65,6 @@ export function SkillTableRow({ {tab === "custom" && ( {skill.author || "—"} )} - {tab === "custom" && ( - - - {skill.owner_id || t("unknownOwner")} - - - )}
{t("columns.name")} {t("columns.description")} {tab === "custom" && {t("columns.author")}} - {tab === "custom" && {t("columns.owner")}} {t("columns.status")} {tab === "custom" && {t("columns.visibility")}} {t("columns.actions")} diff --git a/ui/web/src/types/skill.ts b/ui/web/src/types/skill.ts index f4edfbf6..ade8342c 100644 --- a/ui/web/src/types/skill.ts +++ b/ui/web/src/types/skill.ts @@ -12,7 +12,6 @@ export interface SkillInfo { enabled?: boolean; tenant_enabled?: boolean | null; author?: string; - owner_id?: string; missing_deps?: string[]; } From 2686a6c9896f2e480871c93bcab82b271635f3a9 Mon Sep 17 00:00:00 2001 From: Goon Date: Mon, 18 May 2026 17:43:25 +0700 Subject: [PATCH 3/5] docs: add skill grants beta ship journal --- ...518-1741-beta-skill-grants-tenant-scope.md | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 docs/journals/260518-1741-beta-skill-grants-tenant-scope.md diff --git a/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md b/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md new file mode 100644 index 00000000..29489bcc --- /dev/null +++ b/docs/journals/260518-1741-beta-skill-grants-tenant-scope.md @@ -0,0 +1,43 @@ +# Beta Skill Grants Tenant-Scope Hardening + +**Date**: 2026-05-18 17:41 +**Severity**: High +**Component**: Skills grants, tenant isolation, skill management UI/API +**Status**: Resolved + +## What Happened + +This beta ship started from PR #14, which added per-agent skill manage grants so an authorized agent can edit/delete skills even when `owner_id` no longer matches its current actor identity. The review caught the ugly part: the grant path could mutate skill visibility or grant records across tenant boundaries, and the UI/API exposed `owner_id` in skill responses where clients did not need it. Issue digitopvn/goclaw#15 tracks the hardening fallout. + +## The Brutal Truth + +This was a permission feature shipped close to a tenant-isolation fault line. That is always where mistakes hurt most. The exhausting part is that the baseline feature was useful, but the first version trusted grant inputs too much. A beta is exactly where this should be caught, but it still feels bad because cross-tenant mutation risk is not a cosmetic bug. + +## Technical Details + +- PG now validates grant scope before insert/update in `internal/store/pg/skills_grants.go:19` and `verifySkillGrantScope` rejects mismatched agent tenants as `agent not found`. +- SQLite mirrors the same check in `internal/store/sqlitestore/skills_grants.go:25`. +- Cleanup migration `migrations/000067_skill_agent_grants_scope_cleanup.up.sql:1` deletes `skill_agent_grants` rows where `sag.tenant_id <> a.tenant_id` or non-system skill tenant differs. +- `internal/store/skill_store.go:19` hides `OwnerID` with `json:"-"`, stopping owner IDs from leaking through skill responses/UI. +- Changelog updated under `CHANGELOG.md`. + +## What We Tried + +- Kept PR #14's per-agent `can_manage` model because it directly solves agent-owned skill maintenance without pretending `owner_id` is stable agent identity. +- Rejected UI-only filtering because it would not protect HTTP, WebSocket, import, or future callers. +- Added store-layer verification in both PG and SQLite because that is the point every grant mutation must cross. + +## Root Cause Analysis + +We shipped the first feature around ownership semantics without enforcing the tenant relationship at the same boundary as the write. The fundamental mistake was treating authorization and scope as already resolved by callers. In a multi-tenant system, that assumption is how one tenant ends up mutating another tenant's visibility state. + +## Lessons Learned + +Grant writes must prove both sides of the relationship: the skill and the agent. Do not rely on caller discipline. Also, response structs should not expose security-sensitive ownership fields just because the database row has them. + +## Next Steps + +- Owner: maintainers. Monitor digitopvn/goclaw#15 through beta. +- Owner: release lead. Verify beta users receive migration `000067` and no stale cross-tenant rows survive upgrade. +- Owner: reviewers. Treat future grant/import/export changes as tenant-scope sensitive by default. +- Tests/build are passing for this ship; keep PG, SQLite, and Web UI build checks required before promoting beyond beta. From 8ef8fc4f730670b96f31e85b3da3f1a9a3e397d3 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 18 May 2026 20:44:45 +0700 Subject: [PATCH 4/5] 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) From c174279e016d5dd39a39efde916f0fb7f7c57443 Mon Sep 17 00:00:00 2001 From: Duy Nguyen Date: Mon, 18 May 2026 21:43:03 +0700 Subject: [PATCH 5/5] fix(packages): use runtime dir for GitHub binaries --- cmd/gateway_github_installer.go | 2 +- docs/14-skills-runtime.md | 2 +- docs/packages-github.md | 6 +++--- docs/project-changelog.md | 13 +++++++++++++ internal/skills/apk_helper_call_test.go | 4 ++++ internal/skills/github_installer.go | 4 ++-- internal/skills/github_installer_test.go | 20 +++++++++++++++----- internal/skills/runtime_check.go | 3 ++- 8 files changed, 41 insertions(+), 13 deletions(-) diff --git a/cmd/gateway_github_installer.go b/cmd/gateway_github_installer.go index 2be97534..41b2fc14 100644 --- a/cmd/gateway_github_installer.go +++ b/cmd/gateway_github_installer.go @@ -18,7 +18,7 @@ import ( // GOCLAW_PACKAGES_GITHUB_TOKEN optional PAT (boosts rate limit, enables private repos) // GOCLAW_PACKAGES_MAX_ASSET_SIZE_MB default 200 // GOCLAW_PACKAGES_GITHUB_ALLOWED_ORGS comma-separated allowlist (empty = all allowed) -// GOCLAW_PACKAGES_GITHUB_BIN_DIR default /app/data/.runtime/bin +// GOCLAW_PACKAGES_GITHUB_BIN_DIR default {runtimeDir}/bin // GOCLAW_PACKAGES_GITHUB_MANIFEST default {BIN_DIR}/../github-packages.json func initGitHubInstaller() { cfg := &skills.GitHubPackagesConfig{ diff --git a/docs/14-skills-runtime.md b/docs/14-skills-runtime.md index c41f4c44..ac8fe740 100644 --- a/docs/14-skills-runtime.md +++ b/docs/14-skills-runtime.md @@ -217,7 +217,7 @@ github:owner/repo[@tag] ``` Admin-only, SHA256-verified, ELF-validated, with a release-picker UI. Binaries -land in `/app/data/.runtime/bin/` (on `$PATH`). See +land in `{runtimeDir}/bin/` (on `$PATH`). See [`docs/packages-github.md`](./packages-github.md) for syntax, configuration, security posture, and troubleshooting (especially musl/glibc compatibility). diff --git a/docs/packages-github.md b/docs/packages-github.md index d8e6fbb7..9749fd67 100644 --- a/docs/packages-github.md +++ b/docs/packages-github.md @@ -24,7 +24,7 @@ Examples: 4. Verifies SHA256 if the publisher ships `checksums.txt` / `SHA256SUMS` 5. Validates ELF magic bytes + 64-bit class + machine matches runtime arch 6. Extracts archive safely (tar.gz / zip / raw binary) with path-traversal + zip-bomb guards -7. Installs to `/app/data/.runtime/bin/` (prepended to `$PATH`) +7. Installs to `{runtimeDir}/bin/` (prepended to `$PATH`) 8. Persists a manifest for later listing + uninstall ## Usage @@ -69,7 +69,7 @@ token in `config.json`. | `GOCLAW_PACKAGES_GITHUB_TOKEN` | `""` | Optional PAT: rate 60/hr → 5000/hr + private repo access | | `GOCLAW_PACKAGES_MAX_ASSET_SIZE_MB` | `200` | Applies to both download cap and 2× uncompressed cap | | `GOCLAW_PACKAGES_GITHUB_ALLOWED_ORGS` | `""` | Comma-separated allowlist (empty = all orgs allowed) | -| `GOCLAW_PACKAGES_GITHUB_BIN_DIR` | `/app/data/.runtime/bin` | Where extracted binaries land | +| `GOCLAW_PACKAGES_GITHUB_BIN_DIR` | `{runtimeDir}/bin` | Where extracted binaries land | | `GOCLAW_PACKAGES_GITHUB_MANIFEST` | `{bin_dir}/../github-packages.json` | Manifest path | Token scopes: @@ -133,7 +133,7 @@ the release. Do not force-install; report upstream. ## Limitations (Phase 1) - Linux-only (Lite/Desktop editions not yet supported) -- Docker edition only (runtime dir `/app/data/.runtime/bin`) +- Docker and bare-metal gateway editions (default runtime dir resolves to `/app/data/.runtime/bin` in Docker or `/var/lib/goclaw/data/.runtime/bin` on bare-metal Linux) - Installs all top-level executables in an archive (no interactive picker if archive contains multiple binaries) - No version history / rollback — re-installing replaces in place diff --git a/docs/project-changelog.md b/docs/project-changelog.md index 4a307bcb..73b65f85 100644 --- a/docs/project-changelog.md +++ b/docs/project-changelog.md @@ -6,6 +6,19 @@ Significant changes, features, and fixes in reverse chronological order. ## 2026-05-18 +### Packages: GitHub installer runtime path + +**Fixes** + +- Fixed GitHub Releases package installs on bare-metal gateways by defaulting the GitHub binary directory to `{runtimeDir}/bin` instead of Docker-only `/app/data/.runtime/bin`. +- The fix covers installs such as `github:nextlevelbuilder/goclaw-cli@v0.4.1` on the VPS, where `/app` is not writable or present. + +**Tests** + +- Added default-path regression coverage and made Unix-socket apk helper tests skip cleanly on Windows environments that cannot bind Unix sockets. + +--- + ### Providers: ChatGPT OAuth GPT-5.5 default **Changed** diff --git a/internal/skills/apk_helper_call_test.go b/internal/skills/apk_helper_call_test.go index 382bb40e..67da4eab 100644 --- a/internal/skills/apk_helper_call_test.go +++ b/internal/skills/apk_helper_call_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net" + "runtime" "strings" "sync/atomic" "testing" @@ -42,6 +43,9 @@ func servePkgHelper(t *testing.T, sockPath, respJSON string) func() { ln, err := net.Listen("unix", sockPath) if err != nil { + if runtime.GOOS == "windows" { + t.Skipf("unix sockets are not available in this Windows test environment: %v", err) + } t.Fatalf("servePkgHelper: listen %q: %v", sockPath, err) } diff --git a/internal/skills/github_installer.go b/internal/skills/github_installer.go index 504f92e2..f44bdad3 100644 --- a/internal/skills/github_installer.go +++ b/internal/skills/github_installer.go @@ -57,7 +57,7 @@ func ParseGitHubSpec(s string) (*GitHubSpec, error) { // Token is sourced from env var only (never config.json plaintext). type GitHubPackagesConfig struct { Token string // optional GitHub personal access token - BinDir string // where to install binaries (default /app/data/.runtime/bin) + BinDir string // where to install binaries (default {runtimeDir}/bin) ManifestPath string // manifest file path (default {BinDir}/../github-packages.json) AllowedOrgs []string // lowercase list; empty = all allowed MaxAssetSizeMB int // default 200 @@ -66,7 +66,7 @@ type GitHubPackagesConfig struct { // Defaults fills in zero-valued fields. func (c *GitHubPackagesConfig) Defaults() { if c.BinDir == "" { - c.BinDir = "/app/data/.runtime/bin" + c.BinDir = filepath.Join(packageRuntimeDir(), "bin") } if c.ManifestPath == "" { c.ManifestPath = filepath.Join(filepath.Dir(c.BinDir), "github-packages.json") diff --git a/internal/skills/github_installer_test.go b/internal/skills/github_installer_test.go index 3d226fce..aec3874b 100644 --- a/internal/skills/github_installer_test.go +++ b/internal/skills/github_installer_test.go @@ -2,17 +2,18 @@ package skills import ( "errors" + "path/filepath" "strings" "testing" ) func TestParseGitHubSpec(t *testing.T) { cases := []struct { - in string - ok bool - owner string - repo string - tag string + in string + ok bool + owner string + repo string + tag string }{ {"github:cli/cli@v2.45.0", true, "cli", "cli", "v2.45.0"}, {"github:cli/cli", true, "cli", "cli", ""}, @@ -123,11 +124,20 @@ func TestAllowedOrg(t *testing.T) { } func TestConfigDefaults(t *testing.T) { + runtimeDir := t.TempDir() + t.Setenv("RUNTIME_DIR", runtimeDir) + c := &GitHubPackagesConfig{} c.Defaults() if c.BinDir == "" || c.ManifestPath == "" || c.MaxAssetSizeMB != 200 { t.Errorf("unexpected defaults: %+v", c) } + if want := filepath.Join(runtimeDir, "bin"); c.BinDir != want { + t.Errorf("BinDir = %q, want %q", c.BinDir, want) + } + if want := filepath.Join(runtimeDir, "github-packages.json"); c.ManifestPath != want { + t.Errorf("ManifestPath = %q, want %q", c.ManifestPath, want) + } if c.MaxAssetBytes() != 200*1024*1024 { t.Errorf("MaxAssetBytes wrong: %d", c.MaxAssetBytes()) } diff --git a/internal/skills/runtime_check.go b/internal/skills/runtime_check.go index b39298ca..56b5e70f 100644 --- a/internal/skills/runtime_check.go +++ b/internal/skills/runtime_check.go @@ -4,6 +4,7 @@ import ( "context" "os" "os/exec" + "path/filepath" "strings" "time" ) @@ -65,7 +66,7 @@ func CheckRuntimes() *RuntimeStatus { // Check github-bin runtime directory (where GitHub-installed binaries live). ghInfo := RuntimeInfo{Name: "github-bin"} - binDir := "/app/data/.runtime/bin" + binDir := filepath.Join(packageRuntimeDir(), "bin") if gh := DefaultGitHubInstaller(); gh != nil && gh.Config != nil && gh.Config.BinDir != "" { binDir = gh.Config.BinDir }