diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index b91d2db0d..7106a6024 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -193,6 +193,7 @@ func registerSharedTools( ) { allowReadPaths := buildAllowReadPatterns(cfg) denyReadPaths := compilePatterns(cfg.Tools.DenyReadPaths) + denyWritePaths := compilePatterns(cfg.Tools.DenyWritePaths) var ttsProvider tts.TTSProvider if cfg.Tools.IsToolEnabled("send_tts") { ttsProvider = tts.DetectTTS(cfg) @@ -376,6 +377,7 @@ func registerSharedTools( agent.Workspace, cfg.Tools.Skills.Whitelist, cfg.Tools.Skills.WhitelistEnabled, + denyWritePaths, ), ) } diff --git a/pkg/channels/pico/pico.go b/pkg/channels/pico/pico.go index d5a71ba77..fcb4cad73 100644 --- a/pkg/channels/pico/pico.go +++ b/pkg/channels/pico/pico.go @@ -437,11 +437,11 @@ func (c *PicoChannel) authenticate(r *http.Request) bool { } logger.WarnCF("pico", "Authentication failed: No valid token provided in request", map[string]any{ - "path": r.URL.Path, - "remote_addr": r.RemoteAddr, - "has_auth_hdr": auth != "", - "has_token_q": r.URL.Query().Get("token") != "", - "has_subproto": r.Header.Get("Sec-WebSocket-Protocol") != "", + "path": r.URL.Path, + "remote_addr": r.RemoteAddr, + "has_auth_hdr": auth != "", + "has_token_q": r.URL.Query().Get("token") != "", + "has_subproto": r.Header.Get("Sec-WebSocket-Protocol") != "", }) return false } diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index 562809803..6bc9e5eca 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "sync" "time" @@ -20,23 +21,27 @@ type InstallSkillTool struct { workspace string whitelist []string whitelistEnabled bool + denyWritePaths []*regexp.Regexp mu sync.Mutex } // NewInstallSkillTool creates a new InstallSkillTool. // registryMgr is the shared registry manager (same instance as FindSkillsTool). // workspace is the root workspace directory; skills install to {workspace}/skills/{slug}/. +// denyWritePaths is a list of regex patterns to check before allowing installation. func NewInstallSkillTool( registryMgr *skills.RegistryManager, workspace string, whitelist []string, whitelistEnabled bool, + denyWritePaths []*regexp.Regexp, ) *InstallSkillTool { return &InstallSkillTool{ registryMgr: registryMgr, workspace: workspace, whitelist: whitelist, whitelistEnabled: whitelistEnabled, + denyWritePaths: denyWritePaths, mu: sync.Mutex{}, } } @@ -109,6 +114,18 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To version, _ := args["version"].(string) force, _ := args["force"].(bool) + // Check deny write paths before proceeding with installation. + // Patterns are expected to match relative paths (e.g., "skills", "skills/foo"), + // so we check against the relative path from workspace. + if len(t.denyWritePaths) > 0 { + relativePath := "skills" + for _, pattern := range t.denyWritePaths { + if pattern.MatchString(relativePath) { + return ErrorResult("access denied: cannot write to skills directory") + } + } + } + // Check if already installed. skillsDir := filepath.Join(t.workspace, "skills") targetDir := filepath.Join(skillsDir, slug) diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index 5c12f0029..e0dacc3ba 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "regexp" "testing" "github.com/stretchr/testify/assert" @@ -13,19 +14,19 @@ import ( ) func TestInstallSkillToolName(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) assert.Equal(t, "install_skill", tool.Name()) } func TestInstallSkillToolMissingSlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) result := tool.Execute(context.Background(), map[string]any{}) assert.True(t, result.IsError) assert.Contains(t, result.ForLLM, "identifier is required and must be a non-empty string") } func TestInstallSkillToolEmptySlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": " ", }) @@ -34,7 +35,7 @@ func TestInstallSkillToolEmptySlug(t *testing.T) { } func TestInstallSkillToolUnsafeSlug(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) cases := []string{ "../etc/passwd", @@ -56,7 +57,7 @@ func TestInstallSkillToolAlreadyExists(t *testing.T) { skillDir := filepath.Join(workspace, "skills", "existing-skill") require.NoError(t, os.MkdirAll(skillDir, 0o755)) - tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace, nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace, nil, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "existing-skill", "registry": "clawhub", @@ -67,7 +68,7 @@ func TestInstallSkillToolAlreadyExists(t *testing.T) { func TestInstallSkillToolRegistryNotFound(t *testing.T) { workspace := t.TempDir() - tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace, nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), workspace, nil, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "some-skill", "registry": "nonexistent", @@ -78,7 +79,7 @@ func TestInstallSkillToolRegistryNotFound(t *testing.T) { } func TestInstallSkillToolParameters(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) params := tool.Parameters() props, ok := params["properties"].(map[string]any) @@ -95,7 +96,7 @@ func TestInstallSkillToolParameters(t *testing.T) { } func TestInstallSkillToolMissingRegistry(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false) + tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir(), nil, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "some-skill", }) @@ -108,7 +109,7 @@ func TestInstallSkillToolWhitelist(t *testing.T) { rm := skills.NewRegistryManager() t.Run("blocked-by-whitelist", func(t *testing.T) { - tool := NewInstallSkillTool(rm, workspace, []string{"allowed-skill"}, true) + tool := NewInstallSkillTool(rm, workspace, []string{"allowed-skill"}, true, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "blocked-skill", "registry": "clawhub", @@ -119,7 +120,7 @@ func TestInstallSkillToolWhitelist(t *testing.T) { t.Run("allowed-by-whitelist", func(t *testing.T) { // This will still fail because registry is not found, but it should pass the whitelist check - tool := NewInstallSkillTool(rm, workspace, []string{"allowed-skill"}, true) + tool := NewInstallSkillTool(rm, workspace, []string{"allowed-skill"}, true, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "allowed-skill", "registry": "clawhub", @@ -129,7 +130,7 @@ func TestInstallSkillToolWhitelist(t *testing.T) { }) t.Run("empty-whitelist-allows-all", func(t *testing.T) { - tool := NewInstallSkillTool(rm, workspace, []string{}, false) + tool := NewInstallSkillTool(rm, workspace, []string{}, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "any-skill", "registry": "clawhub", @@ -139,7 +140,7 @@ func TestInstallSkillToolWhitelist(t *testing.T) { }) t.Run("nil-whitelist-allows-all", func(t *testing.T) { - tool := NewInstallSkillTool(rm, workspace, nil, false) + tool := NewInstallSkillTool(rm, workspace, nil, false, nil) result := tool.Execute(context.Background(), map[string]any{ "slug": "any-skill", "registry": "clawhub", @@ -148,3 +149,40 @@ func TestInstallSkillToolWhitelist(t *testing.T) { assert.NotContains(t, result.ForLLM, "not in whitelist") }) } + +func TestInstallSkillToolDenyWritePaths(t *testing.T) { + workspace := t.TempDir() + rm := skills.NewRegistryManager() + + t.Run("blocked-by-deny-write-paths", func(t *testing.T) { + denyPatterns := []*regexp.Regexp{regexp.MustCompile(`^skills(/.*)?$`)} + tool := NewInstallSkillTool(rm, workspace, nil, false, denyPatterns) + result := tool.Execute(context.Background(), map[string]any{ + "slug": "some-skill", + "registry": "clawhub", + }) + assert.True(t, result.IsError) + assert.Contains(t, result.ForLLM, "access denied") + }) + + t.Run("allowed-without-deny-paths", func(t *testing.T) { + tool := NewInstallSkillTool(rm, workspace, nil, false, nil) + result := tool.Execute(context.Background(), map[string]any{ + "slug": "some-skill", + "registry": "clawhub", + }) + assert.True(t, result.IsError) + assert.NotContains(t, result.ForLLM, "access denied") + }) + + t.Run("non-matching-deny-pattern-allows", func(t *testing.T) { + denyPatterns := []*regexp.Regexp{regexp.MustCompile(`^restricted(/.*)?$`)} + tool := NewInstallSkillTool(rm, workspace, nil, false, denyPatterns) + result := tool.Execute(context.Background(), map[string]any{ + "slug": "some-skill", + "registry": "clawhub", + }) + assert.True(t, result.IsError) + assert.NotContains(t, result.ForLLM, "access denied") + }) +}