From b9aaad95cd1770deeb7a78d9d137fc807c12a365 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Sat, 14 Mar 2026 12:01:47 +0800 Subject: [PATCH 1/9] refactor(media): centralize temp media dir path --- pkg/channels/feishu/feishu_64.go | 2 +- pkg/channels/matrix/matrix.go | 4 +--- pkg/channels/matrix/matrix_test.go | 3 ++- pkg/media/tempdir.go | 13 +++++++++++++ pkg/utils/media.go | 3 ++- 5 files changed, 19 insertions(+), 6 deletions(-) create mode 100644 pkg/media/tempdir.go diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 5dbbcf0af..9c462e41e 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -618,7 +618,7 @@ func (c *FeishuChannel) downloadResource( } // Write to the shared picoclaw_media directory using a unique name to avoid collisions. - mediaDir := filepath.Join(os.TempDir(), "picoclaw_media") + mediaDir := media.TempDir() if mkdirErr := os.MkdirAll(mediaDir, 0o700); mkdirErr != nil { logger.ErrorCF("feishu", "Failed to create media directory", map[string]any{ "error": mkdirErr.Error(), diff --git a/pkg/channels/matrix/matrix.go b/pkg/channels/matrix/matrix.go index bec5dfdac..4cbe95c5c 100644 --- a/pkg/channels/matrix/matrix.go +++ b/pkg/channels/matrix/matrix.go @@ -35,8 +35,6 @@ const ( roomKindCacheTTL = 5 * time.Minute roomKindCacheCleanupPeriod = 1 * time.Minute roomKindCacheMaxEntries = 2048 - - matrixMediaTempDirName = "picoclaw_media" ) var matrixMentionHrefRegexp = regexp.MustCompile(`(?i)]+href=["']([^"']+)["']`) @@ -1105,7 +1103,7 @@ func (c *MatrixChannel) stripSelfMention(text string) string { } func matrixMediaTempDir() (string, error) { - mediaDir := filepath.Join(os.TempDir(), matrixMediaTempDirName) + mediaDir := media.TempDir() if err := os.MkdirAll(mediaDir, 0o700); err != nil { return "", err } diff --git a/pkg/channels/matrix/matrix_test.go b/pkg/channels/matrix/matrix_test.go index 07a35c021..7484c8d87 100644 --- a/pkg/channels/matrix/matrix_test.go +++ b/pkg/channels/matrix/matrix_test.go @@ -15,6 +15,7 @@ import ( "maunium.net/go/mautrix/id" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/media" ) func TestMatrixLocalpartMentionRegexp(t *testing.T) { @@ -165,7 +166,7 @@ func TestMatrixMediaTempDir(t *testing.T) { if err != nil { t.Fatalf("matrixMediaTempDir failed: %v", err) } - if filepath.Base(dir) != matrixMediaTempDirName { + if filepath.Base(dir) != media.TempDirName { t.Fatalf("unexpected media dir base: %q", filepath.Base(dir)) } diff --git a/pkg/media/tempdir.go b/pkg/media/tempdir.go new file mode 100644 index 000000000..45942b34f --- /dev/null +++ b/pkg/media/tempdir.go @@ -0,0 +1,13 @@ +package media + +import ( + "os" + "path/filepath" +) + +const TempDirName = "picoclaw_media" + +// TempDir returns the shared temporary directory used for downloaded media. +func TempDir() string { + return filepath.Join(os.TempDir(), TempDirName) +} diff --git a/pkg/utils/media.go b/pkg/utils/media.go index 3e1c5d88e..82e9f5f45 100644 --- a/pkg/utils/media.go +++ b/pkg/utils/media.go @@ -12,6 +12,7 @@ import ( "github.com/google/uuid" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/media" ) // IsAudioFile checks if a file is an audio file based on its filename extension and content type. @@ -67,7 +68,7 @@ func DownloadFile(urlStr, filename string, opts DownloadOptions) string { opts.LoggerPrefix = "utils" } - mediaDir := filepath.Join(os.TempDir(), "picoclaw_media") + mediaDir := media.TempDir() if err := os.MkdirAll(mediaDir, 0o700); err != nil { logger.ErrorCF(opts.LoggerPrefix, "Failed to create media directory", map[string]any{ "error": err.Error(), From 1bc05e83927ad0f914e96d6546a016f6fb9fbc6f Mon Sep 17 00:00:00 2001 From: Hoshina Date: Sat, 14 Mar 2026 12:02:06 +0800 Subject: [PATCH 2/9] fix(tools): allow sandbox access to temp media files --- pkg/agent/instance.go | 27 +++++++++++- pkg/agent/instance_test.go | 86 +++++++++++++++++++++++++++++++++++++ pkg/agent/loop.go | 3 ++ pkg/tools/filesystem.go | 48 ++++++++++++++++++--- pkg/tools/send_file.go | 17 +++++++- pkg/tools/send_file_test.go | 39 +++++++++++++++++ pkg/tools/shell.go | 44 +++++++++++++------ 7 files changed, 241 insertions(+), 23 deletions(-) diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index 0c7baa1ee..1c3635322 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/media" "github.com/sipeed/picoclaw/pkg/memory" "github.com/sipeed/picoclaw/pkg/providers" "github.com/sipeed/picoclaw/pkg/routing" @@ -66,7 +67,7 @@ func NewAgentInstance( readRestrict := restrict && !defaults.AllowReadOutsideWorkspace // Compile path whitelist patterns from config. - allowReadPaths := compilePatterns(cfg.Tools.AllowReadPaths) + allowReadPaths := buildAllowReadPatterns(cfg) allowWritePaths := compilePatterns(cfg.Tools.AllowWritePaths) toolsRegistry := tools.NewToolRegistry() @@ -82,7 +83,7 @@ func NewAgentInstance( toolsRegistry.Register(tools.NewListDirTool(workspace, readRestrict, allowReadPaths)) } if cfg.Tools.IsToolEnabled("exec") { - execTool, err := tools.NewExecToolWithConfig(workspace, restrict, cfg) + execTool, err := tools.NewExecToolWithConfig(workspace, restrict, cfg, allowReadPaths) if err != nil { log.Fatalf("Critical error: unable to initialize exec tool: %v", err) } @@ -282,6 +283,28 @@ func compilePatterns(patterns []string) []*regexp.Regexp { return compiled } +func buildAllowReadPatterns(cfg *config.Config) []*regexp.Regexp { + var configured []string + if cfg != nil { + configured = cfg.Tools.AllowReadPaths + } + + compiled := compilePatterns(configured) + mediaDirPattern := regexp.MustCompile(mediaTempDirPattern()) + for _, pattern := range compiled { + if pattern.String() == mediaDirPattern.String() { + return compiled + } + } + + return append(compiled, mediaDirPattern) +} + +func mediaTempDirPattern() string { + sep := regexp.QuoteMeta(string(os.PathSeparator)) + return "^" + regexp.QuoteMeta(filepath.Clean(media.TempDir())) + "(?:" + sep + "|$)" +} + // Close releases resources held by the agent's session store. func (a *AgentInstance) Close() error { if a.Sessions != nil { diff --git a/pkg/agent/instance_test.go b/pkg/agent/instance_test.go index 4f41ecd1c..f8057bb2f 100644 --- a/pkg/agent/instance_test.go +++ b/pkg/agent/instance_test.go @@ -1,10 +1,14 @@ package agent import ( + "context" "os" + "path/filepath" + "strings" "testing" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/media" ) func TestNewAgentInstance_UsesDefaultsTemperatureAndMaxTokens(t *testing.T) { @@ -160,3 +164,85 @@ func TestNewAgentInstance_ResolveCandidatesFromModelListAlias(t *testing.T) { }) } } + +func TestNewAgentInstance_AllowsMediaTempDirForReadListAndExec(t *testing.T) { + workspace := t.TempDir() + mediaDir := media.TempDir() + if err := os.MkdirAll(mediaDir, 0o700); err != nil { + t.Fatalf("MkdirAll(mediaDir) error = %v", err) + } + + mediaFile, err := os.CreateTemp(mediaDir, "instance-tool-*.txt") + if err != nil { + t.Fatalf("CreateTemp(mediaDir) error = %v", err) + } + mediaPath := mediaFile.Name() + if _, err := mediaFile.WriteString("attachment content"); err != nil { + mediaFile.Close() + t.Fatalf("WriteString(mediaFile) error = %v", err) + } + if err := mediaFile.Close(); err != nil { + t.Fatalf("Close(mediaFile) error = %v", err) + } + t.Cleanup(func() { _ = os.Remove(mediaPath) }) + + cfg := &config.Config{ + Agents: config.AgentsConfig{ + Defaults: config.AgentDefaults{ + Workspace: workspace, + Model: "test-model", + RestrictToWorkspace: true, + }, + }, + Tools: config.ToolsConfig{ + ReadFile: config.ReadFileToolConfig{Enabled: true}, + ListDir: config.ToolConfig{Enabled: true}, + Exec: config.ExecConfig{ + ToolConfig: config.ToolConfig{Enabled: true}, + EnableDenyPatterns: true, + AllowRemote: true, + }, + }, + } + + agent := NewAgentInstance(nil, &cfg.Agents.Defaults, cfg, &mockProvider{}) + + readTool, ok := agent.Tools.Get("read_file") + if !ok { + t.Fatal("read_file tool not registered") + } + readResult := readTool.Execute(context.Background(), map[string]any{"path": mediaPath}) + if readResult.IsError { + t.Fatalf("read_file should allow media temp dir, got: %s", readResult.ForLLM) + } + if !strings.Contains(readResult.ForLLM, "attachment content") { + t.Fatalf("read_file output missing media content: %s", readResult.ForLLM) + } + + listTool, ok := agent.Tools.Get("list_dir") + if !ok { + t.Fatal("list_dir tool not registered") + } + listResult := listTool.Execute(context.Background(), map[string]any{"path": mediaDir}) + if listResult.IsError { + t.Fatalf("list_dir should allow media temp dir, got: %s", listResult.ForLLM) + } + if !strings.Contains(listResult.ForLLM, filepath.Base(mediaPath)) { + t.Fatalf("list_dir output missing media file: %s", listResult.ForLLM) + } + + execTool, ok := agent.Tools.Get("exec") + if !ok { + t.Fatal("exec tool not registered") + } + execResult := execTool.Execute(context.Background(), map[string]any{ + "command": "cat " + filepath.Base(mediaPath), + "working_dir": mediaDir, + }) + if execResult.IsError { + t.Fatalf("exec should allow media temp dir, got: %s", execResult.ForLLM) + } + if !strings.Contains(execResult.ForLLM, "attachment content") { + t.Fatalf("exec output missing media content: %s", execResult.ForLLM) + } +} diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index dfa339dee..8a0303b50 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -117,6 +117,8 @@ func registerSharedTools( registry *AgentRegistry, provider providers.LLMProvider, ) { + allowReadPaths := buildAllowReadPatterns(cfg) + for _, agentID := range registry.ListAgentIDs() { agent, ok := registry.GetAgent(agentID) if !ok { @@ -195,6 +197,7 @@ func registerSharedTools( cfg.Agents.Defaults.RestrictToWorkspace, cfg.Agents.Defaults.GetMaxMediaSize(), nil, + allowReadPaths, ) agent.Tools.Register(sendFileTool) } diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 6b1cb1475..21385b01b 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -22,6 +22,10 @@ const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow // validatePath ensures the given path is within the workspace if restrict is true. func validatePath(path, workspace string, restrict bool) (string, error) { + return validatePathWithAllowPaths(path, workspace, restrict, nil) +} + +func validatePathWithAllowPaths(path, workspace string, restrict bool, patterns []*regexp.Regexp) (string, error) { if workspace == "" { return path, fmt.Errorf("workspace is not defined") } @@ -42,6 +46,10 @@ func validatePath(path, workspace string, restrict bool) (string, error) { } if restrict { + if isAllowedPath(absPath, patterns) { + return absPath, nil + } + if !isWithinWorkspace(absPath, absWorkspace) { return "", fmt.Errorf("access denied: path is outside the workspace") } @@ -73,6 +81,39 @@ func validatePath(path, workspace string, restrict bool) (string, error) { return absPath, nil } +func isAllowedPath(path string, patterns []*regexp.Regexp) bool { + if len(patterns) == 0 { + return false + } + + cleaned := filepath.Clean(path) + if !matchesAllowedPath(cleaned, patterns) { + return false + } + + resolved, err := filepath.EvalSymlinks(cleaned) + if err == nil { + return matchesAllowedPath(resolved, patterns) + } + if os.IsNotExist(err) { + parentResolved, parentErr := resolveExistingAncestor(filepath.Dir(cleaned)) + if parentErr == nil { + return matchesAllowedPath(parentResolved, patterns) + } + } + + return false +} + +func matchesAllowedPath(path string, patterns []*regexp.Regexp) bool { + for _, pattern := range patterns { + if pattern.MatchString(path) { + return true + } + } + return false +} + func resolveExistingAncestor(path string) (string, error) { for current := filepath.Clean(path); ; current = filepath.Dir(current) { if resolved, err := filepath.EvalSymlinks(current); err == nil { @@ -625,12 +666,7 @@ type whitelistFs struct { } func (w *whitelistFs) matches(path string) bool { - for _, p := range w.patterns { - if p.MatchString(path) { - return true - } - } - return false + return matchesAllowedPath(path, w.patterns) } func (w *whitelistFs) ReadFile(path string) ([]byte, error) { diff --git a/pkg/tools/send_file.go b/pkg/tools/send_file.go index 1a03e58ed..a67bd4210 100644 --- a/pkg/tools/send_file.go +++ b/pkg/tools/send_file.go @@ -6,6 +6,7 @@ import ( "mime" "os" "path/filepath" + "regexp" "strings" "github.com/h2non/filetype" @@ -21,20 +22,32 @@ type SendFileTool struct { restrict bool maxFileSize int mediaStore media.MediaStore + allowPaths []*regexp.Regexp defaultChannel string defaultChatID string } -func NewSendFileTool(workspace string, restrict bool, maxFileSize int, store media.MediaStore) *SendFileTool { +func NewSendFileTool( + workspace string, + restrict bool, + maxFileSize int, + store media.MediaStore, + allowPaths ...[]*regexp.Regexp, +) *SendFileTool { if maxFileSize <= 0 { maxFileSize = config.DefaultMaxMediaSize } + var patterns []*regexp.Regexp + if len(allowPaths) > 0 { + patterns = allowPaths[0] + } return &SendFileTool{ workspace: workspace, restrict: restrict, maxFileSize: maxFileSize, mediaStore: store, + allowPaths: patterns, } } @@ -92,7 +105,7 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe return ErrorResult("media store not configured") } - resolved, err := validatePath(path, t.workspace, t.restrict) + resolved, err := validatePathWithAllowPaths(path, t.workspace, t.restrict, t.allowPaths) if err != nil { return ErrorResult(fmt.Sprintf("invalid path: %v", err)) } diff --git a/pkg/tools/send_file_test.go b/pkg/tools/send_file_test.go index 08d129674..6daaab31c 100644 --- a/pkg/tools/send_file_test.go +++ b/pkg/tools/send_file_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "regexp" "strings" "testing" @@ -128,6 +129,44 @@ func TestSendFileTool_CustomFilename(t *testing.T) { } } +func TestSendFileTool_AllowsWhitelistedMediaTempPath(t *testing.T) { + workspace := t.TempDir() + mediaDir := media.TempDir() + if err := os.MkdirAll(mediaDir, 0o700); err != nil { + t.Fatalf("MkdirAll(mediaDir) error = %v", err) + } + + testFile, err := os.CreateTemp(mediaDir, "send-file-*.txt") + if err != nil { + t.Fatalf("CreateTemp(mediaDir) error = %v", err) + } + testPath := testFile.Name() + if _, err := testFile.WriteString("forward me"); err != nil { + testFile.Close() + t.Fatalf("WriteString(testFile) error = %v", err) + } + if err := testFile.Close(); err != nil { + t.Fatalf("Close(testFile) error = %v", err) + } + t.Cleanup(func() { _ = os.Remove(testPath) }) + + pattern := regexp.MustCompile( + "^" + regexp.QuoteMeta(filepath.Clean(mediaDir)) + "(?:" + regexp.QuoteMeta(string(os.PathSeparator)) + "|$)", + ) + + store := media.NewFileMediaStore() + tool := NewSendFileTool(workspace, true, 0, store, []*regexp.Regexp{pattern}) + tool.SetContext("feishu", "chat123") + + result := tool.Execute(context.Background(), map[string]any{"path": testPath}) + if result.IsError { + t.Fatalf("expected whitelisted temp media file to be sendable, got: %s", result.ForLLM) + } + if len(result.Media) != 1 { + t.Fatalf("expected 1 media ref, got %d", len(result.Media)) + } +} + func TestDetectMediaType_MagicBytes(t *testing.T) { dir := t.TempDir() diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 9ea05bb12..0dc85ae21 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -23,6 +23,7 @@ type ExecTool struct { denyPatterns []*regexp.Regexp allowPatterns []*regexp.Regexp customAllowPatterns []*regexp.Regexp + allowedPathPatterns []*regexp.Regexp restrictToWorkspace bool allowRemote bool } @@ -95,14 +96,23 @@ var ( } ) -func NewExecTool(workingDir string, restrict bool) (*ExecTool, error) { - return NewExecToolWithConfig(workingDir, restrict, nil) +func NewExecTool(workingDir string, restrict bool, allowPaths ...[]*regexp.Regexp) (*ExecTool, error) { + return NewExecToolWithConfig(workingDir, restrict, nil, allowPaths...) } -func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Config) (*ExecTool, error) { +func NewExecToolWithConfig( + workingDir string, + restrict bool, + config *config.Config, + allowPaths ...[]*regexp.Regexp, +) (*ExecTool, error) { denyPatterns := make([]*regexp.Regexp, 0) customAllowPatterns := make([]*regexp.Regexp, 0) + var allowedPathPatterns []*regexp.Regexp allowRemote := true + if len(allowPaths) > 0 { + allowedPathPatterns = allowPaths[0] + } if config != nil { execConfig := config.Tools.Exec @@ -146,6 +156,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf denyPatterns: denyPatterns, allowPatterns: nil, customAllowPatterns: customAllowPatterns, + allowedPathPatterns: allowedPathPatterns, restrictToWorkspace: restrict, allowRemote: allowRemote, }, nil @@ -198,7 +209,7 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult cwd := t.workingDir if wd, ok := args["working_dir"].(string); ok && wd != "" { if t.restrictToWorkspace && t.workingDir != "" { - resolvedWD, err := validatePath(wd, t.workingDir, true) + resolvedWD, err := validatePathWithAllowPaths(wd, t.workingDir, true, t.allowedPathPatterns) if err != nil { return ErrorResult("Command blocked by safety guard (" + err.Error() + ")") } @@ -226,16 +237,20 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult if err != nil { return ErrorResult(fmt.Sprintf("Command blocked by safety guard (path resolution failed: %v)", err)) } - absWorkspace, _ := filepath.Abs(t.workingDir) - wsResolved, _ := filepath.EvalSymlinks(absWorkspace) - if wsResolved == "" { - wsResolved = absWorkspace + if isAllowedPath(resolved, t.allowedPathPatterns) { + cwd = resolved + } else { + absWorkspace, _ := filepath.Abs(t.workingDir) + wsResolved, _ := filepath.EvalSymlinks(absWorkspace) + if wsResolved == "" { + wsResolved = absWorkspace + } + rel, err := filepath.Rel(wsResolved, resolved) + if err != nil || !filepath.IsLocal(rel) { + return ErrorResult("Command blocked by safety guard (working directory escaped workspace)") + } + cwd = resolved } - rel, err := filepath.Rel(wsResolved, resolved) - if err != nil || !filepath.IsLocal(rel) { - return ErrorResult("Command blocked by safety guard (working directory escaped workspace)") - } - cwd = resolved } // timeout == 0 means no timeout @@ -412,6 +427,9 @@ func (t *ExecTool) guardCommand(command, cwd string) string { if safePaths[p] { continue } + if isAllowedPath(p, t.allowedPathPatterns) { + continue + } rel, err := filepath.Rel(cwdPath, p) if err != nil { From 345452fba840d1839e82bab58f9e8894c4844abe Mon Sep 17 00:00:00 2001 From: Hoshina Date: Sat, 14 Mar 2026 12:08:11 +0800 Subject: [PATCH 3/9] refactor(tools): remove unused validatePath wrapper --- pkg/tools/filesystem.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index 21385b01b..d25ec1254 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -20,11 +20,6 @@ import ( const MaxReadFileSize = 64 * 1024 // 64KB limit to avoid context overflow -// validatePath ensures the given path is within the workspace if restrict is true. -func validatePath(path, workspace string, restrict bool) (string, error) { - return validatePathWithAllowPaths(path, workspace, restrict, nil) -} - func validatePathWithAllowPaths(path, workspace string, restrict bool, patterns []*regexp.Regexp) (string, error) { if workspace == "" { return path, fmt.Errorf("workspace is not defined") From bb1a4145274c78e1b8267af89a1816cd7b0107f5 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Sat, 14 Mar 2026 19:58:23 +0800 Subject: [PATCH 4/9] fix(tools): harden whitelist path resolution --- pkg/tools/filesystem.go | 42 +++++++++++++++++++++++-------- pkg/tools/filesystem_test.go | 49 ++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) diff --git a/pkg/tools/filesystem.go b/pkg/tools/filesystem.go index d25ec1254..92946ef98 100644 --- a/pkg/tools/filesystem.go +++ b/pkg/tools/filesystem.go @@ -82,22 +82,19 @@ func isAllowedPath(path string, patterns []*regexp.Regexp) bool { } cleaned := filepath.Clean(path) + if !filepath.IsAbs(cleaned) { + return false + } if !matchesAllowedPath(cleaned, patterns) { return false } - resolved, err := filepath.EvalSymlinks(cleaned) - if err == nil { - return matchesAllowedPath(resolved, patterns) - } - if os.IsNotExist(err) { - parentResolved, parentErr := resolveExistingAncestor(filepath.Dir(cleaned)) - if parentErr == nil { - return matchesAllowedPath(parentResolved, patterns) - } + resolved, err := resolvePathAgainstExistingAncestor(cleaned) + if err != nil { + return false } - return false + return matchesAllowedPath(resolved, patterns) } func matchesAllowedPath(path string, patterns []*regexp.Regexp) bool { @@ -122,6 +119,29 @@ func resolveExistingAncestor(path string) (string, error) { } } +func resolvePathAgainstExistingAncestor(path string) (string, error) { + cleaned := filepath.Clean(path) + for current := cleaned; ; current = filepath.Dir(current) { + resolved, err := filepath.EvalSymlinks(current) + if err == nil { + suffix, relErr := filepath.Rel(current, cleaned) + if relErr != nil { + return "", relErr + } + if suffix == "." { + return filepath.Clean(resolved), nil + } + return filepath.Clean(filepath.Join(resolved, suffix)), nil + } + if !os.IsNotExist(err) { + return "", err + } + if filepath.Dir(current) == current { + return "", os.ErrNotExist + } + } +} + func isWithinWorkspace(candidate, workspace string) bool { rel, err := filepath.Rel(filepath.Clean(workspace), filepath.Clean(candidate)) return err == nil && filepath.IsLocal(rel) @@ -661,7 +681,7 @@ type whitelistFs struct { } func (w *whitelistFs) matches(path string) bool { - return matchesAllowedPath(path, w.patterns) + return isAllowedPath(path, w.patterns) } func (w *whitelistFs) ReadFile(path string) ([]byte, error) { diff --git a/pkg/tools/filesystem_test.go b/pkg/tools/filesystem_test.go index 0bbf6caf0..78d69273f 100644 --- a/pkg/tools/filesystem_test.go +++ b/pkg/tools/filesystem_test.go @@ -521,6 +521,55 @@ func TestWhitelistFs_AllowsMatchingPaths(t *testing.T) { } } +func TestWhitelistFs_BlocksSymlinkEscapeInAllowedDir(t *testing.T) { + workspace := t.TempDir() + allowedDir := t.TempDir() + secretDir := t.TempDir() + secretFile := filepath.Join(secretDir, "secret.txt") + if err := os.WriteFile(secretFile, []byte("top secret"), 0o644); err != nil { + t.Fatalf("WriteFile(secretFile) error = %v", err) + } + + linkPath := filepath.Join(allowedDir, "link_out") + if err := os.Symlink(secretDir, linkPath); err != nil { + t.Skipf("symlink not supported in this environment: %v", err) + } + + patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(allowedDir))} + tool := NewReadFileTool(workspace, true, MaxReadFileSize, patterns) + + result := tool.Execute(context.Background(), map[string]any{"path": filepath.Join(linkPath, "secret.txt")}) + if !result.IsError { + t.Fatalf("expected symlink escape from allowed dir to be blocked, got: %s", result.ForLLM) + } +} + +func TestWhitelistFs_WriteAllowsNewFileUnderAllowedDir(t *testing.T) { + workspace := t.TempDir() + rootDir := t.TempDir() + allowedDir := filepath.Join(rootDir, "allowed") + targetFile := filepath.Join(allowedDir, "nested", "file.txt") + + patterns := []*regexp.Regexp{regexp.MustCompile(`^` + regexp.QuoteMeta(allowedDir))} + tool := NewWriteFileTool(workspace, true, patterns) + + result := tool.Execute(context.Background(), map[string]any{ + "path": targetFile, + "content": "outside write", + }) + if result.IsError { + t.Fatalf("expected whitelisted write to succeed, got: %s", result.ForLLM) + } + + data, err := os.ReadFile(targetFile) + if err != nil { + t.Fatalf("ReadFile(targetFile) error = %v", err) + } + if string(data) != "outside write" { + t.Fatalf("target file content = %q, want %q", string(data), "outside write") + } +} + // TestReadFileTool_ChunkedReading verifies the pagination logic of the tool // by reading a file in multiple chunks using 'offset' and 'length'. func TestReadFileTool_ChunkedReading(t *testing.T) { From f71eaaf7f8d8189319453314c157d3ab969798a4 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Sat, 14 Mar 2026 21:03:23 +0800 Subject: [PATCH 5/9] fix(cron): default scheduled jobs to agent execution --- pkg/tools/cron.go | 6 +++--- pkg/tools/cron_test.go | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 648cc3c6c..25608a54c 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -96,7 +96,7 @@ func (t *CronTool) Parameters() map[string]any { }, "deliver": map[string]any{ "type": "boolean", - "description": "If true, send message directly to channel. If false, let agent process message (for complex tasks). Default: true", + "description": "If true, send message directly to channel. If false, let agent process message (for complex tasks). Default: false", }, }, "required": []string{"action"}, @@ -174,8 +174,8 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult return ErrorResult("one of at_seconds, every_seconds, or cron_expr is required") } - // Read deliver parameter, default to true - deliver := true + // Read deliver parameter, default to false so scheduled tasks execute through the agent + deliver := false if d, ok := args["deliver"].(bool); ok { deliver = d } diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index 1776abc65..f1e857949 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -114,3 +114,25 @@ func TestCronTool_NonCommandJobAllowedFromRemoteChannel(t *testing.T) { t.Fatalf("expected non-command reminder to succeed from remote channel, got: %s", result.ForLLM) } } + +func TestCronTool_NonCommandJobDefaultsDeliverToFalse(t *testing.T) { + tool := newTestCronTool(t) + ctx := WithToolContext(context.Background(), "telegram", "chat-1") + result := tool.Execute(ctx, map[string]any{ + "action": "add", + "message": "send me a poem", + "at_seconds": float64(600), + }) + + if result.IsError { + t.Fatalf("expected non-command reminder to succeed, got: %s", result.ForLLM) + } + + jobs := tool.cronService.ListJobs(false) + if len(jobs) != 1 { + t.Fatalf("expected 1 job, got %d", len(jobs)) + } + if jobs[0].Payload.Deliver { + t.Fatal("expected deliver=false by default for non-command jobs") + } +} From 8fc36a4f9bdd4ea1c9784d2570102a5a1d16dd57 Mon Sep 17 00:00:00 2001 From: Dmitrii Balabanov Date: Fri, 13 Mar 2026 12:06:48 +0200 Subject: [PATCH 6/9] fix(logger): mask bot tokens in 3rd-party logger output --- pkg/logger/logger_3rd_party.go | 36 ++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/pkg/logger/logger_3rd_party.go b/pkg/logger/logger_3rd_party.go index da50d686a..3c311520c 100644 --- a/pkg/logger/logger_3rd_party.go +++ b/pkg/logger/logger_3rd_party.go @@ -2,7 +2,19 @@ package logger -import "fmt" +import ( + "fmt" + "regexp" +) + +// botTokenRe matches the secret part of a Telegram bot token embedded in a URL +// or log message: /bot:/ → /bot:****/ +var botTokenRe = regexp.MustCompile(`(bot\d+:)[A-Za-z0-9_-]{20,}`) + +// maskSecrets replaces any embedded bot tokens in s with a redacted placeholder. +func maskSecrets(s string) string { + return botTokenRe.ReplaceAllString(s, "${1}****") +} // Logger implements common Logger interface type Logger struct { @@ -12,52 +24,52 @@ type Logger struct { // Debug logs debug messages func (b *Logger) Debug(v ...any) { - logMessage(DEBUG, b.component, fmt.Sprint(v...), nil) + logMessage(DEBUG, b.component, maskSecrets(fmt.Sprint(v...)), nil) } // Info logs info messages func (b *Logger) Info(v ...any) { - logMessage(INFO, b.component, fmt.Sprint(v...), nil) + logMessage(INFO, b.component, maskSecrets(fmt.Sprint(v...)), nil) } // Warn logs warning messages func (b *Logger) Warn(v ...any) { - logMessage(WARN, b.component, fmt.Sprint(v...), nil) + logMessage(WARN, b.component, maskSecrets(fmt.Sprint(v...)), nil) } // Error logs error messages func (b *Logger) Error(v ...any) { - logMessage(ERROR, b.component, fmt.Sprint(v...), nil) + logMessage(ERROR, b.component, maskSecrets(fmt.Sprint(v...)), nil) } // Debugf logs formatted debug messages func (b *Logger) Debugf(format string, v ...any) { - logMessage(DEBUG, b.component, fmt.Sprintf(format, v...), nil) + logMessage(DEBUG, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Infof logs formatted info messages func (b *Logger) Infof(format string, v ...any) { - logMessage(INFO, b.component, fmt.Sprintf(format, v...), nil) + logMessage(INFO, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Warnf logs formatted warning messages func (b *Logger) Warnf(format string, v ...any) { - logMessage(WARN, b.component, fmt.Sprintf(format, v...), nil) + logMessage(WARN, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Warningf logs formatted warning messages func (b *Logger) Warningf(format string, v ...any) { - logMessage(WARN, b.component, fmt.Sprintf(format, v...), nil) + logMessage(WARN, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Errorf logs formatted error messages func (b *Logger) Errorf(format string, v ...any) { - logMessage(ERROR, b.component, fmt.Sprintf(format, v...), nil) + logMessage(ERROR, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Fatalf logs formatted fatal messages and exits func (b *Logger) Fatalf(format string, v ...any) { - logMessage(FATAL, b.component, fmt.Sprintf(format, v...), nil) + logMessage(FATAL, b.component, maskSecrets(fmt.Sprintf(format, v...)), nil) } // Log logs a message at a given level with caller information @@ -75,7 +87,7 @@ func (b *Logger) Log(msgL, caller int, format string, a ...any) { level = lvl } } - logMessage(level, b.component, fmt.Sprintf(format, a...), nil) + logMessage(level, b.component, maskSecrets(fmt.Sprintf(format, a...)), nil) } // Sync flushes log buffer (no-op for this implementation) From 64ceb5ab760703b0b27e933f1a022f7b64de64aa Mon Sep 17 00:00:00 2001 From: Dmitrii Balabanov Date: Fri, 13 Mar 2026 12:09:03 +0200 Subject: [PATCH 7/9] fix(logger): show first/last 4 chars of bot token for identification --- pkg/logger/logger_3rd_party.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pkg/logger/logger_3rd_party.go b/pkg/logger/logger_3rd_party.go index 3c311520c..d0cb178c5 100644 --- a/pkg/logger/logger_3rd_party.go +++ b/pkg/logger/logger_3rd_party.go @@ -7,13 +7,14 @@ import ( "regexp" ) -// botTokenRe matches the secret part of a Telegram bot token embedded in a URL -// or log message: /bot:/ → /bot:****/ -var botTokenRe = regexp.MustCompile(`(bot\d+:)[A-Za-z0-9_-]{20,}`) +// botTokenRe matches the bot ID prefix and the secret part of a Telegram bot token. +// Groups: 1 = "bot:", 2 = first 4 chars of secret, 3 = middle, 4 = last 4 chars. +var botTokenRe = regexp.MustCompile(`(bot\d+:)([A-Za-z0-9_-]{4})[A-Za-z0-9_-]{12,}([A-Za-z0-9_-]{4})`) -// maskSecrets replaces any embedded bot tokens in s with a redacted placeholder. +// maskSecrets replaces any embedded bot tokens in s with a redacted placeholder +// that keeps the first and last 4 characters of the secret for identification. func maskSecrets(s string) string { - return botTokenRe.ReplaceAllString(s, "${1}****") + return botTokenRe.ReplaceAllString(s, "${1}${2}****${3}") } // Logger implements common Logger interface From be4a33cc150c6ea0f41b3bd939b15dc526f84603 Mon Sep 17 00:00:00 2001 From: Cytown Date: Tue, 17 Mar 2026 09:35:52 +0800 Subject: [PATCH 8/9] refactor gateway/helpers and add server.pid to health (#1646) --- cmd/picoclaw/internal/gateway/helpers.go | 121 ++++++++++------------- pkg/health/server.go | 3 + 2 files changed, 55 insertions(+), 69 deletions(-) diff --git a/cmd/picoclaw/internal/gateway/helpers.go b/cmd/picoclaw/internal/gateway/helpers.go index 3562f03ef..85e93bcf9 100644 --- a/cmd/picoclaw/internal/gateway/helpers.go +++ b/cmd/picoclaw/internal/gateway/helpers.go @@ -7,6 +7,7 @@ import ( "os/signal" "path/filepath" "sync" + "syscall" "time" "github.com/sipeed/picoclaw/cmd/picoclaw/internal" @@ -43,7 +44,6 @@ import ( // Timeout constants for service operations const ( - serviceRestartTimeout = 30 * time.Second serviceShutdownTimeout = 30 * time.Second providerReloadTimeout = 30 * time.Second gracefulShutdownTimeout = 15 * time.Second @@ -121,7 +121,7 @@ func gatewayCmd(debug bool) error { defer stopWatch() sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, os.Interrupt) + signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM) // Main event loop - wait for signals or config changes for { @@ -150,7 +150,8 @@ func setupAndStartServices( // Setup cron tool and service execTimeout := time.Duration(cfg.Tools.Cron.ExecTimeoutMinutes) * time.Minute - services.CronService = setupCronTool( + var err error + services.CronService, err = setupCronTool( agentLoop, msgBus, cfg.WorkspacePath(), @@ -158,7 +159,10 @@ func setupAndStartServices( execTimeout, cfg, ) - if err := services.CronService.Start(); err != nil { + if err != nil { + return nil, fmt.Errorf("error setting up cron service: %w", err) + } + if err = services.CronService.Start(); err != nil { return nil, fmt.Errorf("error starting cron service: %w", err) } fmt.Println("✓ Cron service started") @@ -170,26 +174,8 @@ func setupAndStartServices( cfg.Heartbeat.Enabled, ) services.HeartbeatService.SetBus(msgBus) - services.HeartbeatService.SetHandler(func(prompt, channel, chatID string) *tools.ToolResult { - // Use cli:direct as fallback if no valid channel - if channel == "" || chatID == "" { - channel, chatID = "cli", "direct" - } - // Use ProcessHeartbeat - no session history, each heartbeat is independent - var response string - var err error - response, err = agentLoop.ProcessHeartbeat(context.Background(), prompt, channel, chatID) - if err != nil { - return tools.ErrorResult(fmt.Sprintf("Heartbeat error: %v", err)) - } - if response == "HEARTBEAT_OK" { - return tools.SilentResult("Heartbeat OK") - } - // For heartbeat, always return silent - the subagent result will be - // sent to user via processSystemMessage when the async task completes - return tools.SilentResult(response) - }) - if err := services.HeartbeatService.Start(); err != nil { + services.HeartbeatService.SetHandler(createHeartbeatHandler(agentLoop)) + if err = services.HeartbeatService.Start(); err != nil { return nil, fmt.Errorf("error starting heartbeat service: %w", err) } fmt.Println("✓ Heartbeat service started") @@ -206,7 +192,6 @@ func setupAndStartServices( } // Create channel manager - var err error services.ChannelManager, err = channels.NewManager(cfg, msgBus, services.MediaStore) if err != nil { // Stop the media store if it's a FileMediaStore with cleanup @@ -238,7 +223,7 @@ func setupAndStartServices( services.HealthServer = health.NewServer(cfg.Gateway.Host, cfg.Gateway.Port) services.ChannelManager.SetupHTTPServer(addr, services.HealthServer) - if err := services.ChannelManager.StartAll(context.Background()); err != nil { + if err = services.ChannelManager.StartAll(context.Background()); err != nil { return nil, fmt.Errorf("error starting channels: %w", err) } @@ -251,7 +236,7 @@ func setupAndStartServices( MonitorUSB: cfg.Devices.MonitorUSB, }, stateManager) services.DeviceService.SetBus(msgBus) - if err := services.DeviceService.Start(context.Background()); err != nil { + if err = services.DeviceService.Start(context.Background()); err != nil { logger.ErrorCF("device", "Error starting device service", map[string]any{"error": err.Error()}) } else if cfg.Devices.Enabled { fmt.Println("✓ Device event service started") @@ -386,17 +371,13 @@ func restartServices( services *gatewayServices, msgBus *bus.MessageBus, ) error { - // Create an independent context with timeout for service restart - // This prevents cancellation from the main loop context during reload - ctx, cancel := context.WithTimeout(context.Background(), serviceRestartTimeout) - defer cancel() - // Get current config from agent loop (which has been updated if this is a reload) cfg := al.GetConfig() // Re-create and start cron service with new config execTimeout := time.Duration(cfg.Tools.Cron.ExecTimeoutMinutes) * time.Minute - services.CronService = setupCronTool( + var err error + services.CronService, err = setupCronTool( al, msgBus, cfg.WorkspacePath(), @@ -404,7 +385,10 @@ func restartServices( execTimeout, cfg, ) - if err := services.CronService.Start(); err != nil { + if err != nil { + return fmt.Errorf("error restarting cron service: %w", err) + } + if err = services.CronService.Start(); err != nil { return fmt.Errorf("error restarting cron service: %w", err) } fmt.Println(" ✓ Cron service restarted") @@ -416,31 +400,12 @@ func restartServices( cfg.Heartbeat.Enabled, ) services.HeartbeatService.SetBus(msgBus) - services.HeartbeatService.SetHandler(func(prompt, channel, chatID string) *tools.ToolResult { - if channel == "" || chatID == "" { - channel, chatID = "cli", "direct" - } - var response string - var err error - response, err = al.ProcessHeartbeat(context.Background(), prompt, channel, chatID) - if err != nil { - return tools.ErrorResult(fmt.Sprintf("Heartbeat error: %v", err)) - } - if response == "HEARTBEAT_OK" { - return tools.SilentResult("Heartbeat OK") - } - return tools.SilentResult(response) - }) - if err := services.HeartbeatService.Start(); err != nil { + services.HeartbeatService.SetHandler(createHeartbeatHandler(al)) + if err = services.HeartbeatService.Start(); err != nil { return fmt.Errorf("error restarting heartbeat service: %w", err) } fmt.Println(" ✓ Heartbeat service restarted") - // Stop the old media store before creating a new one - if fms, ok := services.MediaStore.(*media.FileMediaStore); ok { - fms.Stop() - } - // Re-create media store with new config services.MediaStore = media.NewFileMediaStoreWithCleanup(media.MediaCleanerConfig{ Enabled: cfg.Tools.MediaCleanup.Enabled, @@ -454,13 +419,8 @@ func restartServices( al.SetMediaStore(services.MediaStore) // Re-create channel manager with new config - var err error services.ChannelManager, err = channels.NewManager(cfg, msgBus, services.MediaStore) if err != nil { - // Stop the media store if it's a FileMediaStore with cleanup - if fms, ok := services.MediaStore.(*media.FileMediaStore); ok { - fms.Stop() - } return fmt.Errorf("error recreating channel manager: %w", err) } al.SetChannelManager(services.ChannelManager) @@ -477,7 +437,8 @@ func restartServices( services.HealthServer = health.NewServer(cfg.Gateway.Host, cfg.Gateway.Port) services.ChannelManager.SetupHTTPServer(addr, services.HealthServer) - if err := services.ChannelManager.StartAll(ctx); err != nil { + // Use background context for lifecycle to ensure services persist after restartServices returns + if err = services.ChannelManager.StartAll(context.Background()); err != nil { return fmt.Errorf("error restarting channels: %w", err) } fmt.Printf( @@ -493,7 +454,7 @@ func restartServices( MonitorUSB: cfg.Devices.MonitorUSB, }, stateManager) services.DeviceService.SetBus(msgBus) - if err := services.DeviceService.Start(ctx); err != nil { + if err := services.DeviceService.Start(context.Background()); err != nil { logger.WarnCF("device", "Failed to restart device service", map[string]any{"error": err.Error()}) } else if cfg.Devices.Enabled { fmt.Println(" ✓ Device event service restarted") @@ -544,6 +505,10 @@ func setupConfigWatcherPolling(configPath string, debug bool) (chan *config.Conf // Debounce - wait a bit to ensure file write is complete time.Sleep(500 * time.Millisecond) + // Update last known state to prevent repeated reload attempts on failure + lastModTime = currentModTime + lastSize = currentSize + // Validate and load new config newCfg, err := config.LoadConfig(configPath) if err != nil { @@ -561,10 +526,6 @@ func setupConfigWatcherPolling(configPath string, debug bool) (chan *config.Conf logger.Info("✓ Config file validated and loaded") - // Update last known state - lastModTime = currentModTime - lastSize = currentSize - // Send new config to main loop (non-blocking) select { case configChan <- newCfg: @@ -613,7 +574,7 @@ func setupCronTool( restrict bool, execTimeout time.Duration, cfg *config.Config, -) *cron.CronService { +) (*cron.CronService, error) { cronStorePath := filepath.Join(workspace, "cron", "jobs.json") // Create cron service @@ -625,7 +586,7 @@ func setupCronTool( var err error cronTool, err = tools.NewCronTool(cronService, agentLoop, msgBus, workspace, restrict, execTimeout, cfg) if err != nil { - logger.Fatalf("Critical error during CronTool initialization: %v", err) + return nil, fmt.Errorf("critical error during CronTool initialization: %w", err) } agentLoop.RegisterTool(cronTool) @@ -639,5 +600,27 @@ func setupCronTool( }) } - return cronService + return cronService, nil +} + +func createHeartbeatHandler(agentLoop *agent.AgentLoop) func(prompt, channel, chatID string) *tools.ToolResult { + return func(prompt, channel, chatID string) *tools.ToolResult { + // Use cli:direct as fallback if no valid channel + if channel == "" || chatID == "" { + channel, chatID = "cli", "direct" + } + // Use ProcessHeartbeat - no session history, each heartbeat is independent + var response string + var err error + response, err = agentLoop.ProcessHeartbeat(context.Background(), prompt, channel, chatID) + if err != nil { + return tools.ErrorResult(fmt.Sprintf("Heartbeat error: %v", err)) + } + if response == "HEARTBEAT_OK" { + return tools.SilentResult("Heartbeat OK") + } + // For heartbeat, always return silent - the subagent result will be + // sent to user via processSystemMessage when the async task completes + return tools.SilentResult(response) + } } diff --git a/pkg/health/server.go b/pkg/health/server.go index 5609ebdf6..b9ee9f496 100644 --- a/pkg/health/server.go +++ b/pkg/health/server.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "net/http" + "os" "sync" "time" ) @@ -29,6 +30,7 @@ type StatusResponse struct { Status string `json:"status"` Uptime string `json:"uptime"` Checks map[string]Check `json:"checks,omitempty"` + Pid int `json:"pid"` } func NewServer(host string, port int) *Server { @@ -112,6 +114,7 @@ func (s *Server) healthHandler(w http.ResponseWriter, r *http.Request) { resp := StatusResponse{ Status: "ok", Uptime: uptime.String(), + Pid: os.Getpid(), } json.NewEncoder(w).Encode(resp) From fcb69860c4807bbe1da4ef9144fc38fbcaf49fd9 Mon Sep 17 00:00:00 2001 From: wenjie Date: Tue, 17 Mar 2026 09:44:32 +0800 Subject: [PATCH 9/9] feat(web): add configurable cron command execution settings (#1647) - add tools.cron.allow_command config with a default value of true - require command_confirm only when cron command execution is disabled - expose cron command permission and timeout settings in the config UI - add backend tests and update i18n strings --- pkg/config/config.go | 5 +- pkg/config/config_test.go | 23 +++++++ pkg/config/defaults.go | 1 + pkg/tools/cron.go | 34 +++++++---- pkg/tools/cron_test.go | 61 +++++++++++++++++-- .../src/components/config/config-page.tsx | 12 ++++ .../src/components/config/config-sections.tsx | 36 +++++++++++ .../src/components/config/form-model.ts | 13 ++++ web/frontend/src/i18n/locales/en.json | 5 ++ web/frontend/src/i18n/locales/zh.json | 5 ++ 10 files changed, 174 insertions(+), 21 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 2937c36e4..ad5618907 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -699,8 +699,9 @@ type WebToolsConfig struct { } type CronToolsConfig struct { - ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"` - ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout + ToolConfig ` envPrefix:"PICOCLAW_TOOLS_CRON_"` + ExecTimeoutMinutes int ` env:"PICOCLAW_TOOLS_CRON_EXEC_TIMEOUT_MINUTES" json:"exec_timeout_minutes"` // 0 means no timeout + AllowCommand bool ` env:"PICOCLAW_TOOLS_CRON_ALLOW_COMMAND" json:"allow_command"` } type ExecConfig struct { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 4c4dd9421..fc835f78f 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -405,6 +405,13 @@ func TestDefaultConfig_ExecAllowRemoteEnabled(t *testing.T) { } } +func TestDefaultConfig_CronAllowCommandEnabled(t *testing.T) { + cfg := DefaultConfig() + if !cfg.Tools.Cron.AllowCommand { + t.Fatal("DefaultConfig().Tools.Cron.AllowCommand should be true") + } +} + func TestLoadConfig_OpenAIWebSearchDefaultsTrueWhenUnset(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.json") @@ -437,6 +444,22 @@ func TestLoadConfig_ExecAllowRemoteDefaultsTrueWhenUnset(t *testing.T) { } } +func TestLoadConfig_CronAllowCommandDefaultsTrueWhenUnset(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + if err := os.WriteFile(configPath, []byte(`{"tools":{"cron":{"exec_timeout_minutes":5}}}`), 0o600); err != nil { + t.Fatalf("WriteFile() error: %v", err) + } + + cfg, err := LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error: %v", err) + } + if !cfg.Tools.Cron.AllowCommand { + t.Fatal("tools.cron.allow_command should remain true when unset in config file") + } +} + func TestLoadConfig_OpenAIWebSearchCanBeDisabled(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.json") diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index dc534d852..a029eeb59 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -452,6 +452,7 @@ func DefaultConfig() *Config { Enabled: true, }, ExecTimeoutMinutes: 5, + AllowCommand: true, }, Exec: ExecConfig{ ToolConfig: ToolConfig{ diff --git a/pkg/tools/cron.go b/pkg/tools/cron.go index 25608a54c..aa22f9aa6 100644 --- a/pkg/tools/cron.go +++ b/pkg/tools/cron.go @@ -20,10 +20,11 @@ type JobExecutor interface { // CronTool provides scheduling capabilities for the agent type CronTool struct { - cronService *cron.CronService - executor JobExecutor - msgBus *bus.MessageBus - execTool *ExecTool + cronService *cron.CronService + executor JobExecutor + msgBus *bus.MessageBus + execTool *ExecTool + allowCommand bool } // NewCronTool creates a new CronTool @@ -37,12 +38,18 @@ func NewCronTool( return nil, fmt.Errorf("unable to configure exec tool: %w", err) } + allowCommand := true + if config != nil { + allowCommand = config.Tools.Cron.AllowCommand + } + execTool.SetTimeout(execTimeout) return &CronTool{ - cronService: cronService, - executor: executor, - msgBus: msgBus, - execTool: execTool, + cronService: cronService, + executor: executor, + msgBus: msgBus, + execTool: execTool, + allowCommand: allowCommand, }, nil } @@ -76,7 +83,7 @@ func (t *CronTool) Parameters() map[string]any { }, "command_confirm": map[string]any{ "type": "boolean", - "description": "Required when using command=true. Must be true to explicitly confirm scheduling a shell command.", + "description": "Optional explicit confirmation flag for scheduling a shell command. Command execution must also be enabled via tools.cron.allow_command.", }, "at_seconds": map[string]any{ "type": "integer", @@ -180,16 +187,17 @@ func (t *CronTool) addJob(ctx context.Context, args map[string]any) *ToolResult deliver = d } - // GHSA-pv8c-p6jf-3fpp: command scheduling requires internal channel + explicit confirm. - // Non-command reminders (plain messages) remain open to all channels. + // GHSA-pv8c-p6jf-3fpp: command scheduling requires internal channel. When + // allow_command is disabled, explicit confirmation is required as an override. + // Non-command reminders remain open to all channels. command, _ := args["command"].(string) commandConfirm, _ := args["command_confirm"].(bool) if command != "" { if !constants.IsInternalChannel(channel) { return ErrorResult("scheduling command execution is restricted to internal channels") } - if !commandConfirm { - return ErrorResult("command_confirm=true is required to schedule command execution") + if !t.allowCommand && !commandConfirm { + return ErrorResult("command_confirm=true is required when allow_command is disabled") } deliver = false } diff --git a/pkg/tools/cron_test.go b/pkg/tools/cron_test.go index f1e857949..e46b13b13 100644 --- a/pkg/tools/cron_test.go +++ b/pkg/tools/cron_test.go @@ -11,12 +11,11 @@ import ( "github.com/sipeed/picoclaw/pkg/cron" ) -func newTestCronTool(t *testing.T) *CronTool { +func newTestCronToolWithConfig(t *testing.T, cfg *config.Config) *CronTool { t.Helper() storePath := filepath.Join(t.TempDir(), "cron.json") cronService := cron.NewCronService(storePath, nil) msgBus := bus.NewMessageBus() - cfg := config.DefaultConfig() tool, err := NewCronTool(cronService, nil, msgBus, t.TempDir(), true, 0, cfg) if err != nil { t.Fatalf("NewCronTool() error: %v", err) @@ -24,6 +23,11 @@ func newTestCronTool(t *testing.T) *CronTool { return tool } +func newTestCronTool(t *testing.T) *CronTool { + t.Helper() + return newTestCronToolWithConfig(t, config.DefaultConfig()) +} + // TestCronTool_CommandBlockedFromRemoteChannel verifies command scheduling is restricted to internal channels func TestCronTool_CommandBlockedFromRemoteChannel(t *testing.T) { tool := newTestCronTool(t) @@ -44,8 +48,7 @@ func TestCronTool_CommandBlockedFromRemoteChannel(t *testing.T) { } } -// TestCronTool_CommandRequiresConfirm verifies command_confirm=true is required -func TestCronTool_CommandRequiresConfirm(t *testing.T) { +func TestCronTool_CommandDoesNotRequireConfirmByDefault(t *testing.T) { tool := newTestCronTool(t) ctx := WithToolContext(context.Background(), "cli", "direct") result := tool.Execute(ctx, map[string]any{ @@ -55,11 +58,57 @@ func TestCronTool_CommandRequiresConfirm(t *testing.T) { "at_seconds": float64(60), }) + if result.IsError { + t.Fatalf("expected command scheduling without confirm to succeed by default, got: %s", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "Cron job added") { + t.Errorf("expected 'Cron job added', got: %s", result.ForLLM) + } +} + +func TestCronTool_CommandRequiresConfirmWhenAllowCommandDisabled(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Tools.Cron.AllowCommand = 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", + "at_seconds": float64(60), + }) + if !result.IsError { - t.Fatal("expected error when command_confirm is missing") + t.Fatal("expected command scheduling to require confirm when allow_command is disabled") } if !strings.Contains(result.ForLLM, "command_confirm=true") { - t.Errorf("expected 'command_confirm=true' message, got: %s", result.ForLLM) + t.Errorf("expected command_confirm requirement message, got: %s", result.ForLLM) + } +} + +func TestCronTool_CommandAllowedWithConfirmWhenAllowCommandDisabled(t *testing.T) { + cfg := config.DefaultConfig() + cfg.Tools.Cron.AllowCommand = 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.Fatalf( + "expected command scheduling with confirm to succeed when allow_command is disabled, got: %s", + result.ForLLM, + ) + } + if !strings.Contains(result.ForLLM, "Cron job added") { + t.Errorf("expected 'Cron job added', got: %s", result.ForLLM) } } diff --git a/web/frontend/src/components/config/config-page.tsx b/web/frontend/src/components/config/config-page.tsx index cbce7d27e..130498ba4 100644 --- a/web/frontend/src/components/config/config-page.tsx +++ b/web/frontend/src/components/config/config-page.tsx @@ -14,6 +14,7 @@ import { } from "@/api/system" import { AgentDefaultsSection, + CronSection, DevicesSection, LauncherSection, RuntimeSection, @@ -164,6 +165,11 @@ export function ConfigPage() { "Heartbeat interval", { min: 1 }, ) + const cronExecTimeoutMinutes = parseIntField( + form.cronExecTimeoutMinutes, + "Cron exec timeout", + { min: 0 }, + ) await patchAppConfig({ agents: { @@ -180,6 +186,10 @@ export function ConfigPage() { dm_scope: dmScope, }, tools: { + cron: { + allow_command: form.allowCommand, + exec_timeout_minutes: cronExecTimeoutMinutes, + }, exec: { allow_remote: form.allowRemote, }, @@ -279,6 +289,8 @@ export function ConfigPage() { + + + onFieldChange("allowCommand", checked)} + /> + + + + onFieldChange("cronExecTimeoutMinutes", e.target.value) + } + /> + + + ) +} + interface LauncherSectionProps { launcherForm: LauncherForm onFieldChange: UpdateLauncherField diff --git a/web/frontend/src/components/config/form-model.ts b/web/frontend/src/components/config/form-model.ts index d868c4bb4..8c850b2c4 100644 --- a/web/frontend/src/components/config/form-model.ts +++ b/web/frontend/src/components/config/form-model.ts @@ -4,6 +4,8 @@ export interface CoreConfigForm { workspace: string restrictToWorkspace: boolean allowRemote: boolean + allowCommand: boolean + cronExecTimeoutMinutes: string maxTokens: string maxToolIterations: string summarizeMessageThreshold: string @@ -56,6 +58,8 @@ export const EMPTY_FORM: CoreConfigForm = { workspace: "", restrictToWorkspace: true, allowRemote: true, + allowCommand: true, + cronExecTimeoutMinutes: "5", maxTokens: "32768", maxToolIterations: "50", summarizeMessageThreshold: "20", @@ -106,6 +110,7 @@ export function buildFormFromConfig(config: unknown): CoreConfigForm { const heartbeat = asRecord(root.heartbeat) const devices = asRecord(root.devices) const tools = asRecord(root.tools) + const cron = asRecord(tools.cron) const exec = asRecord(tools.exec) return { @@ -118,6 +123,14 @@ export function buildFormFromConfig(config: unknown): CoreConfigForm { exec.allow_remote === undefined ? EMPTY_FORM.allowRemote : asBool(exec.allow_remote), + allowCommand: + cron.allow_command === undefined + ? EMPTY_FORM.allowCommand + : asBool(cron.allow_command), + cronExecTimeoutMinutes: asNumberString( + cron.exec_timeout_minutes, + EMPTY_FORM.cronExecTimeoutMinutes, + ), maxTokens: asNumberString(defaults.max_tokens, EMPTY_FORM.maxTokens), maxToolIterations: asNumberString( defaults.max_tool_iterations, diff --git a/web/frontend/src/i18n/locales/en.json b/web/frontend/src/i18n/locales/en.json index b099dec13..2fa32ebb5 100644 --- a/web/frontend/src/i18n/locales/en.json +++ b/web/frontend/src/i18n/locales/en.json @@ -394,6 +394,10 @@ "restrict_workspace_hint": "Only allow file operations inside workspace.", "allow_remote": "Allow Remote Shell Execution", "allow_remote_hint": "When enabled, shell commands can also run for remote sessions or non-local contexts. When disabled, shell execution stays limited to local safe contexts.", + "allow_shell_execution": "Allow Shell Execution", + "allow_shell_execution_hint": "Enable scheduled shell commands for cron jobs by default. When disabled, users must pass command_confirm=true to schedule a cron command.", + "cron_exec_timeout": "Cron Command Timeout (minutes)", + "cron_exec_timeout_hint": "Maximum runtime for scheduled shell commands. Set to 0 to disable the timeout.", "max_tokens": "Max Tokens", "max_tokens_hint": "Upper token limit per model response.", "max_tool_iterations": "Max Tool Iterations", @@ -434,6 +438,7 @@ "sections": { "agent": "Agent", "runtime": "Runtime", + "cron": "Cron Tasks", "launcher": "Service", "devices": "Devices" }, diff --git a/web/frontend/src/i18n/locales/zh.json b/web/frontend/src/i18n/locales/zh.json index 78093e5c7..badf5bb3d 100644 --- a/web/frontend/src/i18n/locales/zh.json +++ b/web/frontend/src/i18n/locales/zh.json @@ -394,6 +394,10 @@ "restrict_workspace_hint": "仅允许在工作目录内执行文件操作。", "allow_remote": "允许远程执行 Shell 命令", "allow_remote_hint": "开启后，来自远程会话或非本地上下文的请求也可以执行 shell 命令；关闭后，仅允许本地安全上下文执行。", + "allow_shell_execution": "允许 Shell 执行", + "allow_shell_execution_hint": "开启后，cron 定时任务默认允许执行 shell 命令。关闭后，必须显式传入 command_confirm=true 才能创建 cron 命令任务。", + "cron_exec_timeout": "定时命令超时（分钟）", + "cron_exec_timeout_hint": "定时 shell 命令的最长执行时间。设置为 0 表示不限制超时。", "max_tokens": "最大 Token 数", "max_tokens_hint": "单次模型响应允许的最大 Token 数。", "max_tool_iterations": "最大工具迭代次数", @@ -434,6 +438,7 @@ "sections": { "agent": "智能体", "runtime": "运行时", + "cron": "定时任务", "launcher": "服务参数", "devices": "设备" },