diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index aa22f9aa6..154ec75f0 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -25,6 +25,7 @@ type CronTool struct { msgBus *bus.MessageBus execTool *ExecTool allowCommand bool + execEnabled bool } // NewCronTool creates a new CronTool @@ -33,23 +34,32 @@ func NewCronTool( cronService *cron.CronService, executor JobExecutor, msgBus *bus.MessageBus, workspace string, restrict bool, execTimeout time.Duration, config *config.Config, ) (*CronTool, error) { - execTool, err := NewExecToolWithConfig(workspace, restrict, config) - if err != nil { - return nil, fmt.Errorf("unable to configure exec tool: %w", err) - } - allowCommand := true + execEnabled := true if config != nil { allowCommand = config.Tools.Cron.AllowCommand + execEnabled = config.Tools.Exec.Enabled } - execTool.SetTimeout(execTimeout) + var execTool *ExecTool + if execEnabled { + var err error + execTool, err = NewExecToolWithConfig(workspace, restrict, config) + if err != nil { + return nil, fmt.Errorf("unable to configure exec tool: %w", err) + } + } + + if execTool != nil { + execTool.SetTimeout(execTimeout) + } return &CronTool{ cronService: cronService, executor: executor, msgBus: msgBus, execTool: execTool, allowCommand: allowCommand, + execEnabled: execEnabled, }, nil } @@ -193,6 +203,9 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult command, _ := args["command"].(string) commandConfirm, _ := args["command_confirm"].(bool) if command != "" { + if !t.execEnabled { + return ErrorResult("command execution is disabled") + } if !constants.IsInternalChannel(channel) { return ErrorResult("scheduling command execution is restricted to internal channels") } @@ -298,6 +311,18 @@ func (t *CronTool) ExecuteJob(ctx context.Context, job *cron.CronJob) string { // Execute command if present if job.Payload.Command != "" { + if !t.execEnabled || t.execTool == nil { + output := "Error executing scheduled command: command execution is disabled" + pubCtx, pubCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer pubCancel() + t.msgBus.PublishOutbound(pubCtx, bus.OutboundMessage{ + Channel: channel, + ChatID: chatID, + Content: output, + }) + return "ok" + } + args := map[string]any{ "command": job.Payload.Command, "__channel": channel, diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index e46b13b13..09d29b6fa 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" @@ -112,6 +113,28 @@ func TestCronTool_CommandAllowedWithConfirmWhenAllowCommandDisabled(t *testing.T } } +func TestCronTool_CommandBlockedWhenExecDisabled(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Tools.Exec.Enabled = false + + tool := newTestCronToolWithConfig(t, cfg) + ctx := WithToolContext(context.Background(), "cli", "direct") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "check disk", + "command": "df -h", + "command_confirm": true, + "at_seconds": float64(60), + }) + + if !result.IsError { + t.Fatal("expected command scheduling to be blocked when exec is disabled") + } + if !strings.Contains(result.ForLLM, "command execution is disabled") { + t.Errorf("expected exec disabled message, got: %s", result.ForLLM) + } +} + // TestCronTool_CommandAllowedFromInternalChannel verifies command scheduling works from internal channels func TestCronTool_CommandAllowedFromInternalChannel(t *testing.T) { tool := newTestCronTool(t) @@ -185,3 +208,29 @@ func TestCronTool_NonCommandJobDefaultsDeliverToFalse(t *testing.T) { t.Fatal("expected deliver=false by default for non-command jobs") } } + +func TestCronTool_ExecuteJobPublishesErrorWhenExecDisabled(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Tools.Exec.Enabled = false + + tool := newTestCronToolWithConfig(t, cfg) + job := &cron.CronJob{} + job.Payload.Channel = "cli" + job.Payload.To = "direct" + job.Payload.Command = "df -h" + + if got := tool.ExecuteJob(context.Background(), job); got != "ok" { + t.Fatalf("ExecuteJob() = %q, want ok", got) + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + defer cancel() + + msg, ok := tool.msgBus.SubscribeOutbound(ctx) + if !ok { + t.Fatal("expected outbound message") + } + if !strings.Contains(msg.Content, "command execution is disabled") { + t.Fatalf("expected exec disabled message, got: %s", msg.Content) + } +} diff --git a/web/backend/api/config.go b/web/backend/api/config.go index 091e3fbae..a7d5b3c5d 100644 --- a/web/backend/api/config.go +++ b/web/backend/api/config.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "net/http" + "regexp" "github.com/sipeed/picoclaw/pkg/config" ) @@ -188,6 +189,27 @@ func validateConfig(cfg *config.Config) []string { errs = append(errs, "channels.discord.token is required when discord channel is enabled") } + if cfg.Tools.Exec.Enabled { + if cfg.Tools.Exec.EnableDenyPatterns { + errs = append( + errs, + validateRegexPatterns("tools.exec.custom_deny_patterns", cfg.Tools.Exec.CustomDenyPatterns)...) + } + errs = append( + errs, + validateRegexPatterns("tools.exec.custom_allow_patterns", cfg.Tools.Exec.CustomAllowPatterns)...) + } + + return errs +} + +func validateRegexPatterns(field string, patterns []string) []string { + var errs []string + for index, pattern := range patterns { + if _, err := regexp.Compile(pattern); err != nil { + errs = append(errs, fmt.Sprintf("%s[%d] is not a valid regular expression: %v", field, index, err)) + } + } return errs } diff --git a/web/backend/api/config_test.go b/web/backend/api/config_test.go index 29811e37e..54ec8e857 100644 --- a/web/backend/api/config_test.go +++ b/web/backend/api/config_test.go @@ -86,3 +86,82 @@ func TestHandleUpdateConfig_DoesNotInheritDefaultModelFields(t *testing.T) { t.Fatalf("model_list[0].api_base = %q, want empty string", got) } } + +func TestHandlePatchConfig_RejectsInvalidExecRegexPatterns(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{ + "tools": { + "exec": { + "custom_deny_patterns": ["("] + } + } + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusBadRequest, rec.Body.String()) + } + if !bytes.Contains(rec.Body.Bytes(), []byte("custom_deny_patterns")) { + t.Fatalf("expected validation error mentioning custom_deny_patterns, body=%s", rec.Body.String()) + } +} + +func TestHandlePatchConfig_AllowsInvalidExecRegexPatternsWhenExecDisabled(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{ + "tools": { + "exec": { + "enabled": false, + "custom_deny_patterns": ["("], + "custom_allow_patterns": ["("] + } + } + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } +} + +func TestHandlePatchConfig_AllowsInvalidDenyRegexPatternsWhenDenyPatternsDisabled(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{ + "tools": { + "exec": { + "enabled": true, + "enable_deny_patterns": false, + "custom_deny_patterns": ["("] + } + } + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } +} diff --git a/web/frontend/src/components/config/config-page.tsx b/web/frontend/src/components/config/config-page.tsx index 130498ba4..e533b956f 100644 --- a/web/frontend/src/components/config/config-page.tsx +++ b/web/frontend/src/components/config/config-page.tsx @@ -16,6 +16,7 @@ import { AgentDefaultsSection, CronSection, DevicesSection, + ExecSection, LauncherSection, RuntimeSection, } from "@/components/config/config-sections" @@ -27,6 +28,7 @@ import { buildFormFromConfig, parseCIDRText, parseIntField, + parseMultilineList, } from "@/components/config/form-model" import { PageHeader } from "@/components/page-header" import { Button } from "@/components/ui/button" @@ -170,6 +172,28 @@ export function ConfigPage() { "Cron exec timeout", { min: 0 }, ) + const execConfigPatch: Record = { + enabled: form.execEnabled, + } + + if (form.execEnabled) { + execConfigPatch.allow_remote = form.allowRemote + execConfigPatch.enable_deny_patterns = form.enableDenyPatterns + execConfigPatch.custom_allow_patterns = parseMultilineList( + form.customAllowPatternsText, + ) + execConfigPatch.timeout_seconds = parseIntField( + form.execTimeoutSeconds, + "Exec timeout", + { min: 0 }, + ) + + if (form.enableDenyPatterns) { + execConfigPatch.custom_deny_patterns = parseMultilineList( + form.customDenyPatternsText, + ) + } + } await patchAppConfig({ agents: { @@ -190,9 +214,7 @@ export function ConfigPage() { allow_command: form.allowCommand, exec_timeout_minutes: cronExecTimeoutMinutes, }, - exec: { - allow_remote: form.allowRemote, - }, + exec: execConfigPatch, }, heartbeat: { enabled: form.heartbeatEnabled, @@ -289,6 +311,8 @@ export function ConfigPage() { + + - onFieldChange("allowRemote", checked)} - /> - + onFieldChange("execEnabled", checked)} + /> + + {form.execEnabled && ( + <> + onFieldChange("allowRemote", checked)} + /> + + + onFieldChange("enableDenyPatterns", checked) + } + /> + + {form.enableDenyPatterns && ( + +