mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
fix(permissions): use cron-specific permission check for cron tool (#725)
* fix(security): harden exec path exemption matching (#721) - Add absolute path exemption for dataDir/skills-store/ (fixes skill scripts using absolute paths like /app/data/skills-store/ being denied) - Strip surrounding quotes before prefix matching (LLMs often quote paths) - Reject path traversal ("..") in exempt fields to prevent escape - Switch from "any field exempt → skip" to per-field matching: only exempt if ALL fields that match the deny pattern are individually exempt - Closes pipe/comment bypass vectors where an exempt path in one argument would exempt the entire command including non-exempt paths Includes 27 test cases covering: legitimate access, quoted paths, path traversal, unicode bypass, pipe/comment bypass, mixed args. * fix(permissions): use cron-specific permission check for cron tool Cron tool was hardcoded to check `file_writer` configType via CheckFileWriterPermission(), ignoring the `cron` configType that the UI actually saves when granting cron permissions. This caused agents in group chats to be denied cron access even with correct permission configured. Add ConfigTypeCron constant and CheckCronPermission() that checks `cron` configType first, falling back to `file_writer`. --------- Co-authored-by: Viet Tran <viettranx@gmail.com>
This commit is contained in:
1 parent
e22a870cba
commit
e9155e0c5d
2 files changed
+44
-3
No files matched your search
@@ -14,6 +14,7 @@ import (
|
||||
const (
|
||||
ConfigTypeFileWriter = "file_writer" // Group file write access
|
||||
ConfigTypeHeartbeat = "heartbeat" // Heartbeat config access
|
||||
ConfigTypeCron = "cron" // Cron job management access
|
||||
)
|
||||
|
||||
// ConfigPermission represents an allow/deny rule for agent configuration.
|
||||
@@ -74,3 +75,43 @@ func CheckFileWriterPermission(ctx context.Context, permStore ConfigPermissionSt
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// CheckCronPermission returns an error if the caller is in a group context
|
||||
// and does not have cron or file_writer permission. Returns nil if allowed.
|
||||
// Fail-open: returns nil on DB errors or missing context (cron, subagent).
|
||||
func CheckCronPermission(ctx context.Context, permStore ConfigPermissionStore) error {
|
||||
if permStore == nil {
|
||||
return nil
|
||||
}
|
||||
userID := UserIDFromContext(ctx)
|
||||
if !strings.HasPrefix(userID, "group:") && !strings.HasPrefix(userID, "guild:") {
|
||||
return nil // not a group context
|
||||
}
|
||||
agentID := AgentIDFromContext(ctx)
|
||||
if agentID == uuid.Nil {
|
||||
return nil // no agent context
|
||||
}
|
||||
senderID := SenderIDFromContext(ctx)
|
||||
if senderID == "" {
|
||||
return nil // system context (cron, subagent)
|
||||
}
|
||||
numericID := strings.SplitN(senderID, "|", 2)[0]
|
||||
|
||||
// Check cron-specific permission first.
|
||||
allowed, err := permStore.CheckPermission(ctx, agentID, userID, ConfigTypeCron, numericID)
|
||||
if err != nil {
|
||||
return nil // fail-open
|
||||
}
|
||||
if allowed {
|
||||
return nil
|
||||
}
|
||||
// Fall back to file_writer (implies full mutation access).
|
||||
allowed, err = permStore.CheckPermission(ctx, agentID, userID, ConfigTypeFileWriter, numericID)
|
||||
if err != nil {
|
||||
return nil // fail-open
|
||||
}
|
||||
if !allowed {
|
||||
return fmt.Errorf("permission denied: only users with cron or file_writer permission can manage cron jobs in group chats")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -142,10 +142,10 @@ func (t *CronTool) Execute(ctx context.Context, args map[string]any) *Result {
|
||||
return ErrorResult("action parameter is required")
|
||||
}
|
||||
|
||||
// Group write permission check for mutation actions
|
||||
// Group cron permission check for mutation actions
|
||||
if t.permStore != nil && (action == "add" || action == "update" || action == "remove") {
|
||||
if err := store.CheckFileWriterPermission(ctx, t.permStore); err != nil {
|
||||
return ErrorResult("permission denied: only file writers can manage cron jobs in group chats")
|
||||
if err := store.CheckCronPermission(ctx, t.permStore); err != nil {
|
||||
return ErrorResult("permission denied: only users with cron or file_writer permission can manage cron jobs in group chats")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user