feat(security): add environment sanitization for shell execution

- Add BuildSanitizedEnv() with default allowlist (PATH, HOME, USER, etc)
- Add PICOCLAW_* vars to allowlist for config propagation
- Add LLMBlocklist for vars LLM cannot override
- Cache sanitized env at ExecTool init, pass to exec.Cmd
- Add tests and docs

Part of PR1: Environment Sanitization for External Execution

docs/tools_configuration.md

@@ -52,10 +52,35 @@ The exec tool is used to execute shell commands.
 | `enable_deny_patterns` | bool  | true    | Enable default dangerous command blocking  |
 | `custom_deny_patterns` | array | []      | Custom deny patterns (regular expressions) |
 
-### Functionality
+### Environment Sanitization
 
-- **`enable_deny_patterns`**: Set to `false` to completely disable the default dangerous command blocking patterns
+The exec tool sanitizes the environment passed to child processes:
-- **`custom_deny_patterns`**: Add custom deny regex patterns; commands matching these will be blocked
+
+1. **Default allowlist** — Only these variables are inherited from the parent process:
+   - `PATH`, `HOME`, `USER`, `LANG`, `SHELL`, `TERM`, `PWD`, `OLDPWD`, `HOSTNAME`, `LOGNAME`, `TZ`, `DISPLAY`, `TMPDIR`, `EDITOR`, `PAGER`
+   - Plus: `PICOCLAW_CONFIG`, `PICOCLAW_HOME`, `PICOCLAW_SERVICE_NAME`, `PICOCLAW_EXE`, `PICOCLAW_WORKSPACE`
+   - Plus: `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY`
+   - Plus any variable starting with `LC_`
+
+2. **Config env_set** — Variables from config are merged (can override inherited values)
+
+3. **LLM env injection** — The LLM can inject additional variables per-call via the `env` parameter:
+
+   ```json
+   {
+     "name": "exec",
+     "arguments": {
+       "command": "echo $DEBUG_MODE",
+       "env": {
+         "DEBUG_MODE": "true"
+       }
+     }
+   }
+   ```
+
+   **Blocked variables** — The LLM cannot override these sensitive variables:
+   - `PATH`, `HOME`, `USER`, `LOGNAME`, `SHELL`
+   - `LD_PRELOAD`, `LD_LIBRARY_PATH`, `LD_AUDIT`, `LD_DEBUG`
 
 ### Default Blocked Command Patterns
 

pkg/tools/shell.go

@@ -14,6 +14,7 @@ import (
 	"time"
 
 	"github.com/sipeed/picoclaw/pkg/config"
+	"github.com/sipeed/picoclaw/pkg/tools/shell"
 )
 
 type ExecTool struct {
@@ -23,6 +24,7 @@ type ExecTool struct {
 	allowPatterns       []*regexp.Regexp
 	customAllowPatterns []*regexp.Regexp
 	restrictToWorkspace bool
+	cachedEnv           []string // cached sanitized env from os.Environ() at init
 }
 
 var (
@@ -143,6 +145,7 @@ func NewExecToolWithConfig(workingDir string, restrict bool, config *config.Conf
 		allowPatterns:       nil,
 		customAllowPatterns: customAllowPatterns,
 		restrictToWorkspace: restrict,
+		cachedEnv:           shell.BuildSanitizedEnv(os.Environ(), nil, nil, nil),
 	}, nil
 }
 
@@ -217,6 +220,10 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult
 	} else {
 		cmd = exec.CommandContext(cmdCtx, "sh", "-c", command)
 	}
+
+	// Use sanitized environment - strips secrets, prevents env-based attacks
+	cmd.Env = t.cachedEnv
+
 	if cwd != "" {
 		cmd.Dir = cwd
 	}

pkg/tools/shell/env.go

@@ -0,0 +1,156 @@
+package shell
+
+import (
+	"os"
+	"runtime"
+	"strings"
+)
+
+// DefaultEnvAllowlist is the set of environment variable names that are safe
+// to propagate to child processes. Everything else is stripped to prevent
+// accidental credential leakage.
+//
+// To add a new variable:
+// 1. Add to this map if it's safe to pass through
+// 2. Or add a prefix to defaultEnvAllowPrefixes for pattern matching
+// Note: Do NOT add wildcard patterns like "*_API_KEY" here - use explicit names
+// to avoid accidentally leaking secrets.
+var DefaultEnvAllowlist = map[string]bool{
+	"PATH":            true,
+	"HOME":            true,
+	"USER":            true,
+	"LANG":            true,
+	"SHELL":           true,
+	"TERM":            true,
+	"PWD":             true,
+	"OLDPWD":          true,
+	"HOSTNAME":        true,
+	"LOGNAME":         true,
+	"TZ":              true,
+	"DISPLAY":         true,
+	"TMPDIR":       true,
+	"EDITOR":       true,
+	"PAGER":        true,
+	"HTTP_PROXY":   true,
+	"HTTPS_PROXY":  true,
+	"NO_PROXY":     true,
+	// PICOCLAW_* vars - inherited from parent process if set
+	"PICOCLAW_HOME":        true,
+	"PICOCLAW_CONFIG":      true,
+	"PICOCLAW_SERVICE_NAME": true,
+	"PICOCLAW_EXE":        true,
+	"PICOCLAW_WORKSPACE":   true,
+}
+
+// defaultEnvAllowPrefixes are env var prefixes that are always allowed.
+var defaultEnvAllowPrefixes = []string{
+	"LC_",
+}
+
+// LLMBlocklist is the set of environment variable names that the LLM
+// cannot override, even if passed via the env parameter. These vars
+// control fundamental process behavior and could be exploited.
+var LLMBlocklist = map[string]bool{
+	"PATH":            true, // Could hijack command resolution
+	"HOME":            true, // Could redirect file access
+	"USER":            true, // Could impersonate user
+	"LOGNAME":        true, // Could impersonate user
+	"SHELL":           true, // Could change shell behavior
+	"LD_PRELOAD":      true, // Could inject code
+	"LD_LIBRARY_PATH": true, // Could hijack library resolution
+	"LD_AUDIT":       true, // Could inject code
+	"LD_DEBUG":       true, // Could leak info
+}
+
+// windowsEnvAllowlist contains additional variables needed on Windows.
+var windowsEnvAllowlist = map[string]bool{
+	"PATHEXT":     true,
+	"SYSTEMROOT":  true,
+	"SYSTEMDRIVE": true,
+	"COMSPEC":     true,
+	"APPDATA":     true,
+	"USERPROFILE": true,
+	"HOMEDRIVE":   true,
+	"HOMEPATH":    true,
+}
+
+// BuildSanitizedEnv constructs a sanitized environment []string suitable for
+// exec.Cmd.Env. It filters the inherited environment to only allowlisted variables.
+//
+// baseEnv is the inherited environment (e.g., from os.Environ() or cached).
+// If nil, os.Environ() will be used for backwards compatibility.
+// extraAllowlist adds additional variable names to the default allowlist.
+// envSet provides explicit key=value pairs from config (override inherited).
+// extraEnv provides additional key=value pairs from tool call (merged with envSet).
+func BuildSanitizedEnv(baseEnv []string, extraAllowlist []string, envSet, extraEnv map[string]string) []string {
+	// Use provided env or fall back to os.Environ
+	inherited := baseEnv
+	if inherited == nil {
+		inherited = os.Environ()
+	}
+
+	allowed := make(map[string]bool, len(DefaultEnvAllowlist)+len(extraAllowlist)+len(windowsEnvAllowlist))
+	for k := range DefaultEnvAllowlist {
+		allowed[envKey(k)] = true
+	}
+	if runtime.GOOS == "windows" {
+		for k := range windowsEnvAllowlist {
+			allowed[envKey(k)] = true
+		}
+	}
+	for _, k := range extraAllowlist {
+		allowed[envKey(k)] = true
+	}
+
+	vars := make(map[string]string, len(allowed)+len(envSet)+len(extraEnv))
+
+	for _, entry := range inherited {
+		k, v, ok := strings.Cut(entry, "=")
+		if !ok {
+			continue
+		}
+		norm := envKey(k)
+		if allowed[norm] || isAllowedPrefix(norm) {
+			vars[norm] = v
+		}
+	}
+
+	for k, v := range envSet {
+		vars[envKey(k)] = v
+	}
+
+	// Merge extraEnv (tool call) - highest priority
+	// Filter against LLM blocklist to prevent override of sensitive vars
+	for k, v := range extraEnv {
+		if LLMBlocklist[envKey(k)] {
+			continue // Skip blocked vars
+		}
+		vars[envKey(k)] = v
+	}
+
+	// Convert to []string for exec.Cmd.Env
+	result := make([]string, 0, len(vars))
+	for k, v := range vars {
+		result = append(result, k+"="+v)
+	}
+	return result
+}
+
+// envKey normalizes an environment variable name. On Windows, where env
+// vars are case-insensitive, it uppercases the key so that "Path" and
+// "PATH" map to the same entry. On other platforms it's a no-op.
+func envKey(k string) string {
+	if runtime.GOOS == "windows" {
+		return strings.ToUpper(k)
+	}
+	return k
+}
+
+func isAllowedPrefix(name string) bool {
+	for _, prefix := range defaultEnvAllowPrefixes {
+		if strings.HasPrefix(name, prefix) {
+			return true
+		}
+	}
+	return false
+}

pkg/tools/shell/env_test.go

@@ -0,0 +1,180 @@
+package shell
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestBuildSanitizedEnv_FiltersSecrets(t *testing.T) {
+	// Set some secret env vars.
+	t.Setenv("OPENAI_API_KEY", "sk-secret")
+	t.Setenv("ANTHROPIC_API_KEY", "anthro-secret")
+	t.Setenv("AWS_SECRET_ACCESS_KEY", "aws-secret")
+	t.Setenv("DATABASE_URL", "postgres://secret")
+	// Set something that should pass.
+	t.Setenv("PATH", "/usr/bin")
+	t.Setenv("HOME", "/home/test")
+	t.Setenv("LC_ALL", "en_US.UTF-8")
+
+	env := BuildSanitizedEnv(nil, nil, nil, nil)
+
+	assertEnvPresent(t, env, "PATH")
+	assertEnvPresent(t, env, "HOME")
+	assertEnvPresent(t, env, "LC_ALL")
+
+	assertEnvAbsent(t, env, "OPENAI_API_KEY")
+	assertEnvAbsent(t, env, "ANTHROPIC_API_KEY")
+	assertEnvAbsent(t, env, "AWS_SECRET_ACCESS_KEY")
+	assertEnvAbsent(t, env, "DATABASE_URL")
+}
+
+func TestBuildSanitizedEnv_ExtraAllowlist(t *testing.T) {
+	t.Setenv("MY_CUSTOM_VAR", "hello")
+
+	env := BuildSanitizedEnv(nil, []string{"MY_CUSTOM_VAR"}, nil, nil)
+	assertEnvPresent(t, env, "MY_CUSTOM_VAR")
+}
+
+func TestBuildSanitizedEnv_EnvSet(t *testing.T) {
+	env := BuildSanitizedEnv(nil, nil, map[string]string{
+		"INJECTED": "value123",
+	}, nil)
+
+	v := getEnvValue(env, "INJECTED")
+	if v == "" {
+		t.Fatal("expected INJECTED to be present")
+	}
+	if v != "value123" {
+		t.Errorf("INJECTED = %q, want %q", v, "value123")
+	}
+}
+
+func TestBuildSanitizedEnv_EnvSetOverridesInherited(t *testing.T) {
+	t.Setenv("PATH", "/original")
+
+	env := BuildSanitizedEnv(nil, nil, map[string]string{
+		"PATH": "/overridden",
+	}, nil)
+
+	v := getEnvValue(env, "PATH")
+	if v != "/overridden" {
+		t.Errorf("PATH = %q, want %q", v, "/overridden")
+	}
+}
+
+func TestBuildSanitizedEnv_DefaultAllowlist(t *testing.T) {
+	for name := range DefaultEnvAllowlist {
+		t.Setenv(name, "test-"+name)
+	}
+
+	env := BuildSanitizedEnv(nil, nil, nil, nil)
+
+	for name := range DefaultEnvAllowlist {
+		v := getEnvValue(env, name)
+		if v == "" {
+			t.Errorf("expected %s to be in sanitized env", name)
+		}
+	}
+}
+
+func TestBuildSanitizedEnv_ReturnsSlice(t *testing.T) {
+	env := BuildSanitizedEnv(nil, nil, map[string]string{
+		"TEST_A": "a",
+		"TEST_B": "b",
+	}, nil)
+
+	if env == nil {
+		t.Fatal("env should not be nil")
+	}
+
+	// Should be a slice
+	if len(env) == 0 {
+		t.Error("expected non-empty env slice")
+	}
+
+	// Check format
+	found := make(map[string]string)
+	for _, entry := range env {
+		k, v, ok := strings.Cut(entry, "=")
+		if !ok {
+			t.Errorf("invalid env entry: %q", entry)
+			continue
+		}
+		found[k] = v
+	}
+
+	if found["TEST_A"] != "a" {
+		t.Errorf("TEST_A = %q, want %q", found["TEST_A"], "a")
+	}
+	if found["TEST_B"] != "b" {
+		t.Errorf("TEST_B = %q, want %q", found["TEST_B"], "b")
+	}
+}
+
+func TestLLMBlocklist_BlocksSensitiveVars(t *testing.T) {
+	// Set up inherited env
+	t.Setenv("PATH", "/original/path")
+	t.Setenv("HOME", "/original/home")
+
+	// LLM tries to override these via extraEnv
+	extraEnv := map[string]string{
+		"PATH":            "/malicious/path",
+		"HOME":            "/etc",
+		"LD_PRELOAD":      "/evil.so",
+		"MY_CUSTOM_VAR":   "allowed", // Not blocked
+	}
+
+	env := BuildSanitizedEnv(nil, nil, nil, extraEnv)
+
+	// Blocked vars should keep their inherited value, not the LLM override
+	v := getEnvValue(env, "PATH")
+	if v != "/original/path" {
+		t.Errorf("PATH should be /original/path, got %q - LLM override was not blocked", v)
+	}
+
+	v = getEnvValue(env, "HOME")
+	if v != "/original/home" {
+		t.Errorf("HOME should be /original/home, got %q - LLM override was not blocked", v)
+	}
+
+	// LD_PRELOAD was not in inherited, so should still be absent (blocked)
+	v = getEnvValue(env, "LD_PRELOAD")
+	if v != "" {
+		t.Error("LD_PRELOAD should be blocked by LLMBlocklist")
+	}
+
+	// Non-blocked var should be present
+	v = getEnvValue(env, "MY_CUSTOM_VAR")
+	if v == "" {
+		t.Error("MY_CUSTOM_VAR should be allowed")
+	}
+	if v != "allowed" {
+		t.Errorf("MY_CUSTOM_VAR = %q, want %q", v, "allowed")
+	}
+}
+
+// Helper to get value from env slice
+func getEnvValue(env []string, name string) string {
+	prefix := name + "="
+	for _, entry := range env {
+		if strings.HasPrefix(entry, prefix) {
+			return strings.TrimPrefix(entry, prefix)
+		}
+	}
+	return ""
+}
+
+func assertEnvPresent(t *testing.T, env []string, name string) {
+	t.Helper()
+	if getEnvValue(env, name) == "" {
+		t.Errorf("expected %s to be present in sanitized env", name)
+	}
+}
+
+func assertEnvAbsent(t *testing.T, env []string, name string) {
+	t.Helper()
+	v := getEnvValue(env, name)
+	if v != "" {
+		t.Errorf("expected %s to be absent from sanitized env, got %q", name, v)
+	}
+}

pkg/tools/shell/env_windows_test.go

@@ -0,0 +1,68 @@
+package shell
+
+import (
+	"runtime"
+	"testing"
+)
+
+func TestBuildSanitizedEnv_WindowsCaseInsensitive(t *testing.T) {
+	// Skip on non-Windows
+	if runtime.GOOS != "windows" {
+		t.Skip("Windows-specific test")
+	}
+
+	// On Windows the OS typically stores "Path" not "PATH".
+	// Verify that mixed-case inherited vars still pass the allowlist.
+	t.Setenv("Path", `C:\Windows\system32`)
+
+	env := BuildSanitizedEnv(nil, nil, nil, nil)
+
+	v := getEnvValue(env, "PATH")
+	if v == "" {
+		t.Fatal("expected PATH to be present when OS provides 'Path'")
+	}
+	if v != `C:\Windows\system32` {
+		t.Errorf("PATH = %q, want %q", v, `C:\Windows\system32`)
+	}
+
+	// Also verify lookup with original casing works.
+	v2 := getEnvValue(env, "Path")
+	if v2 == "" {
+		t.Fatal("expected Get('Path') to resolve via case-insensitive lookup")
+	}
+}
+
+func TestBuildSanitizedEnv_WindowsEnvSetCaseInsensitive(t *testing.T) {
+	// Skip on non-Windows
+	if runtime.GOOS != "windows" {
+		t.Skip("Windows-specific test")
+	}
+
+	env := BuildSanitizedEnv(nil, nil, map[string]string{
+		"path": `C:\custom\bin`,
+	}, nil)
+
+	v := getEnvValue(env, "PATH")
+	if v == "" {
+		t.Fatal("expected PATH to be set via lowercase 'path' envSet key")
+	}
+	if v != `C:\custom\bin` {
+		t.Errorf("PATH = %q, want %q", v, `C:\custom\bin`)
+	}
+}
+
+func TestBuildSanitizedEnv_WindowsExtraAllowlistCaseInsensitive(t *testing.T) {
+	// Skip on non-Windows
+	if runtime.GOOS != "windows" {
+		t.Skip("Windows-specific test")
+	}
+
+	t.Setenv("my_custom_var", "hello")
+
+	env := BuildSanitizedEnv(nil, []string{"MY_CUSTOM_VAR"}, nil, nil)
+
+	v := getEnvValue(env, "my_custom_var")
+	if v == "" {
+		t.Fatal("expected my_custom_var to be found via case-insensitive allowlist")
+	}
+}