From e9155e0c5d4de5760e231a6117d613cf3cdd07ef Mon Sep 17 00:00:00 2001
From: Plateau Nguyen <nguyennlt.ncc@gmail.com>
Date: Tue, 7 Apr 2026 12:41:18 +0700
Subject: [PATCH] fix(permissions): use cron-specific permission check for cron
 tool (#725)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* 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>
---
 internal/store/config_permission_store.go | 41 +++++++++++++++++++++++
 internal/tools/cron.go                    |  6 ++--
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/internal/store/config_permission_store.go b/internal/store/config_permission_store.go
index aa0d82f3..d3ace7e6 100644
--- a/internal/store/config_permission_store.go
+++ b/internal/store/config_permission_store.go
@@ -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
+}
diff --git a/internal/tools/cron.go b/internal/tools/cron.go
index 7eaab3ad..f8a59b5e 100644
--- a/internal/tools/cron.go
+++ b/internal/tools/cron.go
@@ -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")
 		}
 	}