Merge pull request #1536 from alexhoshina/fix/allow-picoclaw-media-tempdir

Fix: allow picoclaw media tempdir

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 {

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)
+	}
+}

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)
 		}

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(),

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)<a[^>]+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
 	}

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))
 	}
 

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)
+}

pkg/tools/filesystem.go

@@ -20,8 +20,7 @@ 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) {
+func validatePathWithAllowPaths(path, workspace string, restrict bool, patterns []*regexp.Regexp) (string, error) {
 	if workspace == "" {
 		return path, fmt.Errorf("workspace is not defined")
 	}
@@ -42,6 +41,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 +76,36 @@ 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 !filepath.IsAbs(cleaned) {
+		return false
+	}
+	if !matchesAllowedPath(cleaned, patterns) {
+		return false
+	}
+
+	resolved, err := resolvePathAgainstExistingAncestor(cleaned)
+	if err != nil {
+		return false
+	}
+
+	return matchesAllowedPath(resolved, patterns)
+}
+
+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 {
@@ -86,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)
@@ -625,12 +681,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 isAllowedPath(path, w.patterns)
 }
 
 func (w *whitelistFs) ReadFile(path string) ([]byte, error) {

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) {

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))
 	}

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()
 

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 {

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(),