mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-14 08:18:19 +00:00
fix(skills): enforce tenant isolation in skill upload, versioning, and grants
- Inject WithTenantSlug into both HTTP (enrichContext) and WS (MethodRouter.Handle) context — previously WithTenantSlug was defined but never called, causing all non-master tenants to share the same filesystem path (data isolation breach) - Scope GetNextVersion, GetNextVersionLocked, and CreateSkillManaged version queries by tenant_id to prevent cross-tenant version collision - Make advisory lock tenant-aware (hash tenant_id + slug) so different tenants uploading the same slug don't contend on the same lock - Add agent tenant_id validation in GrantToAgent to prevent cross-tenant grant injection - Fix IsSystemSkill to filter by MasterTenantID only - Fix StoreMissingDeps to work for both system and custom skills
This commit is contained in:
1 parent
b178f843c0
commit
9168e4b4a7
11 files changed
+75
-36
No files matched your search
@@ -212,6 +212,9 @@ func (c *Client) RemoteAddr() string { return c.remoteAddr }
|
||||
// TenantID returns the resolved tenant UUID (uuid.Nil means cross-tenant).
|
||||
func (c *Client) TenantID() uuid.UUID { return c.tenantID }
|
||||
|
||||
// TenantSlug returns the resolved tenant URL slug (set during connect).
|
||||
func (c *Client) TenantSlug() string { return c.tenantSlug }
|
||||
|
||||
// IsOwner returns true if the client has the owner role (tenant management + full access).
|
||||
func (c *Client) IsOwner() bool { return c.role == permissions.RoleOwner }
|
||||
|
||||
|
||||
@@ -83,6 +83,9 @@ func (r *MethodRouter) Handle(ctx context.Context, client *Client, req *protocol
|
||||
if client.TenantID() != uuid.Nil {
|
||||
ctx = store.WithTenantID(ctx, client.TenantID())
|
||||
}
|
||||
if slug := client.TenantSlug(); slug != "" {
|
||||
ctx = store.WithTenantSlug(ctx, slug)
|
||||
}
|
||||
|
||||
slog.Debug("handling method", "method", req.Method, "client", client.id, "req_id", req.ID)
|
||||
handler(ctx, client, req)
|
||||
|
||||
@@ -159,6 +159,7 @@ type authResult struct {
|
||||
Authenticated bool
|
||||
KeyData *store.APIKeyData // non-nil when authenticated via API key
|
||||
TenantID uuid.UUID // resolved tenant; always concrete after resolution
|
||||
TenantSlug string // resolved tenant slug for filesystem paths
|
||||
}
|
||||
|
||||
// resolveAuth determines the caller's role from the request.
|
||||
@@ -187,9 +188,11 @@ func resolveAuthWithBearer(r *http.Request, bearer string) authResult {
|
||||
if tid, err := uuid.Parse(tenantVal); err == nil {
|
||||
if t, err := pkgTenantCache.GetTenant(r.Context(), tid); err == nil && t != nil {
|
||||
res.TenantID = t.ID
|
||||
res.TenantSlug = t.Slug
|
||||
}
|
||||
} else if t, err := pkgTenantCache.GetTenantBySlug(r.Context(), tenantVal); err == nil && t != nil {
|
||||
res.TenantID = t.ID
|
||||
res.TenantSlug = t.Slug
|
||||
}
|
||||
}
|
||||
if res.TenantID == uuid.Nil {
|
||||
@@ -206,6 +209,12 @@ func resolveAuthWithBearer(r *http.Request, bearer string) authResult {
|
||||
res.TenantID = store.MasterTenantID
|
||||
} else {
|
||||
res.TenantID = keyData.TenantID
|
||||
// Resolve tenant slug for filesystem path scoping.
|
||||
if pkgTenantCache != nil {
|
||||
if t, err := pkgTenantCache.GetTenant(r.Context(), keyData.TenantID); err == nil && t != nil {
|
||||
res.TenantSlug = t.Slug
|
||||
}
|
||||
}
|
||||
}
|
||||
return res
|
||||
}
|
||||
@@ -262,6 +271,9 @@ func enrichContext(ctx context.Context, r *http.Request, auth authResult) contex
|
||||
tenantID = store.MasterTenantID
|
||||
}
|
||||
ctx = store.WithTenantID(ctx, tenantID)
|
||||
if auth.TenantSlug != "" {
|
||||
ctx = store.WithTenantSlug(ctx, auth.TenantSlug)
|
||||
}
|
||||
slog.Debug("security.http_auth_resolved",
|
||||
"path", r.URL.Path,
|
||||
"role", string(auth.Role),
|
||||
|
||||
@@ -115,7 +115,7 @@ func (h *SkillsHandler) handleUpload(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
|
||||
// Determine version (always increment — includes archived skills so re-upload gets v2+)
|
||||
version := h.skills.GetNextVersion(slug)
|
||||
version := h.skills.GetNextVersion(r.Context(), slug)
|
||||
|
||||
// Extract to filesystem: tenant-scoped skills-store/slug/version/
|
||||
tenantSkillsBase := h.tenantSkillsDir(r)
|
||||
|
||||
@@ -19,7 +19,7 @@ import (
|
||||
// SystemSkillStore is the minimal interface needed by the seeder.
|
||||
type SystemSkillStore interface {
|
||||
UpsertSystemSkill(ctx context.Context, p pg.SkillCreateParams) (uuid.UUID, bool, string, error)
|
||||
GetNextVersion(slug string) int
|
||||
GetNextVersion(ctx context.Context, slug string) int
|
||||
BumpVersion()
|
||||
UpdateSkill(ctx context.Context, id uuid.UUID, updates map[string]interface{}) error
|
||||
StoreMissingDeps(ctx context.Context, id uuid.UUID, missing []string) error
|
||||
@@ -100,7 +100,7 @@ func (s *Seeder) Seed(ctx context.Context) (seeded int, skipped int, skills []se
|
||||
fmMap = parseSimpleYAML(fm)
|
||||
}
|
||||
|
||||
version := s.store.GetNextVersion(slug)
|
||||
version := s.store.GetNextVersion(ctx, slug)
|
||||
destDir := filepath.Join(s.managedDir, slug, fmt.Sprintf("%d", version))
|
||||
|
||||
desc := description
|
||||
|
||||
@@ -187,7 +187,8 @@ func (s *PGSkillStore) scanSkillInfoList(rows *sql.Rows) []store.SkillInfo {
|
||||
}
|
||||
|
||||
// StoreMissingDeps persists the missing_deps list for a skill into the deps JSONB column.
|
||||
// Only updates system skills unscoped; custom skills require tenant match.
|
||||
// Works for both system and custom skills. System skills bypass tenant filter;
|
||||
// custom skills require tenant_id match for cross-tenant safety.
|
||||
func (s *PGSkillStore) StoreMissingDeps(ctx context.Context, id uuid.UUID, missing []string) error {
|
||||
if missing == nil {
|
||||
missing = []string{}
|
||||
@@ -196,11 +197,10 @@ func (s *PGSkillStore) StoreMissingDeps(ctx context.Context, id uuid.UUID, missi
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
// System skills can be updated without tenant; custom skills need tenant scope.
|
||||
// Use is_system check to ensure cross-tenant safety for custom skills.
|
||||
tid := tenantIDForInsert(ctx)
|
||||
_, err = s.db.ExecContext(ctx,
|
||||
`UPDATE skills SET deps = $1, updated_at = NOW() WHERE id = $2 AND is_system = true`,
|
||||
encoded, id,
|
||||
`UPDATE skills SET deps = $1, updated_at = NOW() WHERE id = $2 AND (is_system = true OR tenant_id = $3)`,
|
||||
encoded, id, tid,
|
||||
)
|
||||
if err == nil {
|
||||
s.BumpVersion()
|
||||
|
||||
@@ -95,9 +95,12 @@ func (s *PGSkillStore) ListSystemSkillDirs(ctx context.Context) map[string]strin
|
||||
return dirs
|
||||
}
|
||||
|
||||
// IsSystemSkill checks if a skill slug belongs to a system skill.
|
||||
// IsSystemSkill checks if a skill slug belongs to a system skill (master tenant only).
|
||||
func (s *PGSkillStore) IsSystemSkill(slug string) bool {
|
||||
var isSystem bool
|
||||
err := s.db.QueryRow("SELECT is_system FROM skills WHERE slug = $1", slug).Scan(&isSystem)
|
||||
err := s.db.QueryRow(
|
||||
"SELECT is_system FROM skills WHERE slug = $1 AND tenant_id = $2 AND is_system = true",
|
||||
slug, store.MasterTenantID,
|
||||
).Scan(&isSystem)
|
||||
return err == nil && isSystem
|
||||
}
|
||||
@@ -111,10 +111,12 @@ func (s *PGSkillStore) DeleteSkill(ctx context.Context, id uuid.UUID) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// slugAdvisoryLock returns a stable int64 lock key derived from a slug string,
|
||||
// suitable for use with pg_advisory_xact_lock.
|
||||
func slugAdvisoryLock(slug string) int64 {
|
||||
// tenantSlugAdvisoryLock returns a stable int64 lock key derived from tenant ID + slug,
|
||||
// suitable for use with pg_advisory_xact_lock. Tenant-scoped so different tenants
|
||||
// uploading the same slug don't contend on the same lock.
|
||||
func tenantSlugAdvisoryLock(tenantID uuid.UUID, slug string) int64 {
|
||||
h := fnv.New64a()
|
||||
h.Write(tenantID[:])
|
||||
h.Write([]byte(slug))
|
||||
return int64(h.Sum64())
|
||||
}
|
||||
@@ -143,27 +145,27 @@ func (s *PGSkillStore) CreateSkillManaged(ctx context.Context, p SkillCreatePara
|
||||
}
|
||||
defer tx.Rollback()
|
||||
|
||||
// Acquire advisory lock scoped to this transaction so concurrent calls for
|
||||
// the same slug serialize version calculation and the upsert atomically.
|
||||
if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", slugAdvisoryLock(p.Slug)); err != nil {
|
||||
return uuid.Nil, fmt.Errorf("advisory lock: %w", err)
|
||||
}
|
||||
|
||||
// Compute next version atomically under the lock.
|
||||
var version int
|
||||
if err := tx.QueryRowContext(ctx,
|
||||
"SELECT COALESCE(MAX(version), 0) + 1 FROM skills WHERE slug = $1",
|
||||
p.Slug,
|
||||
).Scan(&version); err != nil {
|
||||
return uuid.Nil, fmt.Errorf("get next version: %w", err)
|
||||
}
|
||||
|
||||
// Resolve tenant_id: default to MasterTenantID if no tenant in context.
|
||||
tenantID := store.TenantIDFromContext(ctx)
|
||||
if tenantID == uuid.Nil {
|
||||
tenantID = store.MasterTenantID
|
||||
}
|
||||
|
||||
// Acquire advisory lock scoped to this transaction so concurrent calls for
|
||||
// the same (tenant, slug) pair serialize version calculation and the upsert atomically.
|
||||
if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", tenantSlugAdvisoryLock(tenantID, p.Slug)); err != nil {
|
||||
return uuid.Nil, fmt.Errorf("advisory lock: %w", err)
|
||||
}
|
||||
|
||||
// Compute next version atomically under the lock, scoped to tenant.
|
||||
var version int
|
||||
if err := tx.QueryRowContext(ctx,
|
||||
"SELECT COALESCE(MAX(version), 0) + 1 FROM skills WHERE slug = $1 AND tenant_id = $2",
|
||||
p.Slug, tenantID,
|
||||
).Scan(&version); err != nil {
|
||||
return uuid.Nil, fmt.Errorf("get next version: %w", err)
|
||||
}
|
||||
|
||||
id := store.GenNewID()
|
||||
var returnedID uuid.UUID
|
||||
err = tx.QueryRowContext(ctx,
|
||||
@@ -215,14 +217,15 @@ func (s *PGSkillStore) GetSkillFilePath(ctx context.Context, id uuid.UUID) (file
|
||||
return filePath, slug, version, isSystem, err == nil
|
||||
}
|
||||
|
||||
// GetNextVersion returns the next version number for a skill slug.
|
||||
// GetNextVersion returns the next version number for a skill slug, scoped to tenant.
|
||||
// NOTE: This function has an inherent race condition — two concurrent callers
|
||||
// for the same slug can receive the same version number. Use it only for
|
||||
// informational purposes (e.g. display). For write paths, use CreateSkillManaged
|
||||
// which computes the version atomically under a pg_advisory_xact_lock.
|
||||
func (s *PGSkillStore) GetNextVersion(slug string) int {
|
||||
func (s *PGSkillStore) GetNextVersion(ctx context.Context, slug string) int {
|
||||
tid := tenantIDForInsert(ctx)
|
||||
var maxVersion int
|
||||
s.db.QueryRow("SELECT COALESCE(MAX(version), 0) FROM skills WHERE slug = $1", slug).Scan(&maxVersion)
|
||||
s.db.QueryRowContext(ctx, "SELECT COALESCE(MAX(version), 0) FROM skills WHERE slug = $1 AND tenant_id = $2", slug, tid).Scan(&maxVersion)
|
||||
return maxVersion + 1
|
||||
}
|
||||
|
||||
@@ -230,17 +233,18 @@ func (s *PGSkillStore) GetNextVersion(slug string) int {
|
||||
// Safe for concurrent write paths (patch, create). Returns version and a cleanup func
|
||||
// that MUST be called to release the lock (commits the transaction).
|
||||
func (s *PGSkillStore) GetNextVersionLocked(ctx context.Context, slug string) (int, func() error, error) {
|
||||
tenantID := tenantIDForInsert(ctx)
|
||||
tx, err := s.db.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
return 0, nil, fmt.Errorf("begin tx: %w", err)
|
||||
}
|
||||
if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", slugAdvisoryLock(slug)); err != nil {
|
||||
if _, err := tx.ExecContext(ctx, "SELECT pg_advisory_xact_lock($1)", tenantSlugAdvisoryLock(tenantID, slug)); err != nil {
|
||||
tx.Rollback()
|
||||
return 0, nil, fmt.Errorf("advisory lock: %w", err)
|
||||
}
|
||||
var version int
|
||||
if err := tx.QueryRowContext(ctx,
|
||||
"SELECT COALESCE(MAX(version), 0) + 1 FROM skills WHERE slug = $1", slug,
|
||||
"SELECT COALESCE(MAX(version), 0) + 1 FROM skills WHERE slug = $1 AND tenant_id = $2", slug, tenantID,
|
||||
).Scan(&version); err != nil {
|
||||
tx.Rollback()
|
||||
return 0, nil, fmt.Errorf("get next version: %w", err)
|
||||
|
||||
@@ -14,15 +14,29 @@ import (
|
||||
// GrantToAgent grants a skill to an agent with version pinning.
|
||||
// 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 {
|
||||
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,
|
||||
"SELECT tenant_id FROM agents WHERE id = $1", agentID,
|
||||
).Scan(&agentTenantID); err != nil {
|
||||
return fmt.Errorf("agent not found")
|
||||
}
|
||||
if agentTenantID != tid {
|
||||
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(), tenantIDForInsert(ctx),
|
||||
store.GenNewID(), skillID, agentID, version, grantedBy, time.Now(), tid,
|
||||
)
|
||||
if err != nil {
|
||||
return err
|
||||
|
||||
@@ -119,7 +119,7 @@ func (t *PublishSkillTool) Execute(ctx context.Context, args map[string]any) *Re
|
||||
}
|
||||
|
||||
// Version + destination (tenant-scoped)
|
||||
version := t.skills.GetNextVersion(slug)
|
||||
version := t.skills.GetNextVersion(ctx, slug)
|
||||
destDir := filepath.Join(t.tenantSkillsDir(ctx), slug, fmt.Sprintf("%d", version))
|
||||
if err := os.MkdirAll(destDir, 0755); err != nil {
|
||||
return ErrorResult(fmt.Sprintf("failed to create destination: %v", err))
|
||||
|
||||
@@ -134,7 +134,7 @@ func (t *SkillManageTool) executeCreate(ctx context.Context, args map[string]any
|
||||
}
|
||||
|
||||
// Version + destination (tenant-scoped)
|
||||
version := t.skills.GetNextVersion(slug)
|
||||
version := t.skills.GetNextVersion(ctx, slug)
|
||||
destDir := filepath.Join(t.tenantSkillsDir(ctx), slug, fmt.Sprintf("%d", version))
|
||||
if err := os.MkdirAll(destDir, 0755); err != nil {
|
||||
return ErrorResult(fmt.Sprintf("failed to create skill directory: %v", err))
|
||||
|
||||
Reference in new issue
Block a user