From 4b11ef32fe1c501baafc615da77c63b952339684 Mon Sep 17 00:00:00 2001 From: Keith Patrick Date: Mon, 9 Mar 2026 04:42:42 +0000 Subject: [PATCH] Refactor env handling: extract MapToEnvSlice, clean up unused code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unused BuildSanitizedEnv function - Extract MapToEnvSlice helper from MergeEnvVars - MergeEnvVars now returns map, caller converts to []string - Fix lint issues (gci, gofumpt, golines, whitespace) 💘 Generated with Crush --- pkg/config/config.go | 6 +- pkg/tools/shell.go | 2 +- pkg/tools/shell/env.go | 169 +++++++++++------------------------------ 3 files changed, 50 insertions(+), 127 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index a4e1bf51d..b683eadbe 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -640,13 +640,13 @@ type CronToolsConfig struct { } type ExecConfig struct { - ToolConfig ` envPrefix:"PICOCLAW_TOOLS_EXEC_"` + ToolConfig ` envPrefix:"PICOCLAW_TOOLS_EXEC_"` EnableDenyPatterns bool ` env:"PICOCLAW_TOOLS_EXEC_ENABLE_DENY_PATTERNS" json:"enable_deny_patterns"` CustomDenyPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_DENY_PATTERNS" json:"custom_deny_patterns"` CustomAllowPatterns []string ` env:"PICOCLAW_TOOLS_EXEC_CUSTOM_ALLOW_PATTERNS" json:"custom_allow_patterns"` TimeoutSeconds int ` env:"PICOCLAW_TOOLS_EXEC_TIMEOUT_SECONDS" json:"timeout_seconds"` // 0 means use default (60s) - EnvSet map[string]string ` json:"env_set"` // env vars to set for all exec commands - EnvAllowlist []string ` json:"env_allowlist"` // additional env vars to allow (extends default) - use explicit names, not wildcards + EnvSet map[string]string ` json:"env_set"` // env vars to set for all exec commands + EnvAllowlist []string ` json:"env_allowlist"` // additional env vars to allow (extends default) - use explicit names, not wildcards } type SkillsToolsConfig struct { diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index 62cdf1c9a..134d1df37 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -263,7 +263,7 @@ func (t *ExecTool) Execute(ctx context.Context, args map[string]any) *ToolResult // Use sanitized environment - merge cached env with exec time vars and LLM extra env // Note: cachedEnv is NOT re-filtered - PICOCLAW_* vars are preserved - cmd.Env = shell.MergeEnvVars(t.cachedEnv, execTimeEnv, extraEnv) + cmd.Env = shell.MapToEnvSlice(shell.MergeEnvVars(t.cachedEnv, execTimeEnv, extraEnv)) if cwd != "" { cmd.Dir = cwd diff --git a/pkg/tools/shell/env.go b/pkg/tools/shell/env.go index 41c4f4dc3..49739f418 100644 --- a/pkg/tools/shell/env.go +++ b/pkg/tools/shell/env.go @@ -17,45 +17,39 @@ import ( // 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, + "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, // Locale - "LC_ALL": true, - "LC_CTYPE": true, - "LC_MESSAGES": true, - "LC_MONETARY": true, - "LC_NUMERIC": true, - "LC_TIME": true, - "LC_PAPER": true, - "LC_NAME": true, - "LC_ADDRESS": true, - "LC_TELEPHONE": true, - "LC_MEASUREMENT": true, + "LC_ALL": true, + "LC_CTYPE": true, + "LC_MESSAGES": true, + "LC_MONETARY": true, + "LC_NUMERIC": true, + "LC_TIME": true, + "LC_PAPER": true, + "LC_NAME": true, + "LC_ADDRESS": true, + "LC_TELEPHONE": true, + "LC_MEASUREMENT": true, "LC_IDENTIFICATION": true, - "LC_COLLATE": true, -} - -// defaultEnvAllowPrefixes are env var prefixes that are always allowed. -// Currently empty - all allowed vars are explicit in DefaultEnvAllowlist. -var defaultEnvAllowPrefixes = []string{ - // Currently empty - all allowed vars are explicit + "LC_COLLATE": true, } // LLMBlocklist is the set of environment variable names that the LLM @@ -65,21 +59,21 @@ 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 + "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 + "LD_AUDIT": true, // Could inject code + "LD_DEBUG": true, // Could leak info // PICOCLAW_* vars - controlled by the agent, not LLM - "PICOCLAW_HOME": true, - "PICOCLAW_CONFIG": true, - "PICOCLAW_AGENT_WORKSPACE": true, - "PICOCLAW_EXE": true, - "PICOCLAW_SERVICE_NAME": true, - "PICOCLAW_EXEC_TIME": true, - "PICOCLAW_EXEC_TIMEOUT": true, + "PICOCLAW_HOME": true, + "PICOCLAW_CONFIG": true, + "PICOCLAW_AGENT_WORKSPACE": true, + "PICOCLAW_EXE": true, + "PICOCLAW_SERVICE_NAME": true, + "PICOCLAW_EXEC_TIME": true, + "PICOCLAW_EXEC_TIMEOUT": true, } // windowsEnvAllowlist contains additional variables needed on Windows. @@ -154,11 +148,11 @@ func isBlocked(key string) bool { return false } -// MergeEnvVars merges multiple env sources into a final []string for exec.Cmd.Env. +// MergeEnvVars merges multiple env sources into a map. // baseEnv is the cached map from AllowedEnv. // envSet provides explicit key=value pairs (config, not filtered). // extraEnv provides additional key=value pairs from LLM (filtered by blocklist). -func MergeEnvVars(baseEnv map[string]string, envSet, extraEnv map[string]string) []string { +func MergeEnvVars(baseEnv map[string]string, envSet, extraEnv map[string]string) map[string]string { vars := make(map[string]string, len(baseEnv)+len(envSet)+len(extraEnv)) // Start with base env (already filtered) @@ -183,74 +177,12 @@ func MergeEnvVars(baseEnv map[string]string, envSet, extraEnv map[string]string) } } - // 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 + return vars } -// 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 - } - } - - if envSet != nil { - 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 - if extraEnv != nil { - for k, v := range extraEnv { - if LLMBlocklist[envKey(k)] { - continue // Skip blocked vars - } - vars[envKey(k)] = v - } - } - - // Convert to []string for exec.Cmd.Env +// MapToEnvSlice converts a map of environment variables to a []string +// in the format "KEY=value" suitable for exec.Cmd.Env. +func MapToEnvSlice(vars map[string]string) []string { result := make([]string, 0, len(vars)) for k, v := range vars { result = append(result, k+"="+v) @@ -268,15 +200,6 @@ func envKey(k string) string { return k } -func isAllowedPrefix(name string) bool { - for _, prefix := range defaultEnvAllowPrefixes { - if strings.HasPrefix(name, prefix) { - return true - } - } - return false -} - // WithPicoclawEnvVars ensures PICOCLAW_* vars are set in envSet. // These are needed for child processes to locate config, workspace, etc. func WithPicoclawEnvVars(envSet map[string]string, workspace string) map[string]string {