From 4d69c8aef174ba79b6ed50456abb2435cec6791a Mon Sep 17 00:00:00 2001 From: ben Date: Sun, 21 Dec 2025 20:16:57 +0800 Subject: [PATCH] fix: allow claude backend to read env from setting.json while preventing recursion (#92) * fix: allow claude backend to read env from setting.json while preventing recursion Fixes #89 Problem: - --setting-sources "" prevents claude from reading ~/.claude/setting.json env - Removing it causes infinite recursion via skills/commands/agents loading Solution: - Keep --setting-sources "" to block all config sources - Add loadMinimalEnvSettings() to extract only env from setting.json - Pass env explicitly via --settings parameter - Update tests to validate dynamic --settings parameter Benefits: - Claude backend can access ANTHROPIC_API_KEY and other env vars - Skills/commands/agents remain blocked, preventing recursion - Graceful degradation if setting.json doesn't exist Generated with SWE-Agent.ai Co-Authored-By: SWE-Agent.ai * security: pass env via process environment instead of command line Critical security fix for issue #89: - Prevents ANTHROPIC_API_KEY leakage in process command line (ps) - Prevents sensitive values from being logged in wrapper logs Changes: 1. executor.go: - Add SetEnv() method to commandRunner interface - realCmd merges env with os.Environ() and sets to cmd.Env - All test mocks implement SetEnv() 2. backend.go: - Change loadMinimalEnvSettings() to return map[string]string - Use os.UserHomeDir() instead of os.Getenv("HOME") - Add 1MB file size limit check - Only accept string values in env (reject non-strings) - Remove --settings parameter (no longer in command line) 3. Tests: - Add loadMinimalEnvSettings() unit tests - Remove --settings validation (no longer in args) - All test mocks implement SetEnv() Security improvements: - No sensitive values in argv (safe from ps/logs) - Type-safe env parsing (string-only) - File size limit prevents memory issues - Graceful degradation if setting.json missing Tests: All pass (30.912s) Generated with SWE-Agent.ai Co-Authored-By: SWE-Agent.ai --------- Co-authored-by: SWE-Agent.ai --- codeagent-wrapper/backend.go | 51 +++++++++++++++ codeagent-wrapper/backend_test.go | 63 +++++++++++++++++++ codeagent-wrapper/executor.go | 53 ++++++++++++++++ codeagent-wrapper/executor_concurrent_test.go | 12 ++++ codeagent-wrapper/main_test.go | 40 ++++++++---- 5 files changed, 207 insertions(+), 12 deletions(-) diff --git a/codeagent-wrapper/backend.go b/codeagent-wrapper/backend.go index 2e6f42d..bdf4420 100644 --- a/codeagent-wrapper/backend.go +++ b/codeagent-wrapper/backend.go @@ -1,5 +1,11 @@ package main +import ( + "encoding/json" + "os" + "path/filepath" +) + // Backend defines the contract for invoking different AI CLI backends. // Each backend is responsible for supplying the executable command and // building the argument list based on the wrapper config. @@ -29,6 +35,51 @@ func (ClaudeBackend) BuildArgs(cfg *Config, targetArg string) []string { return buildClaudeArgs(cfg, targetArg) } +const maxClaudeSettingsBytes = 1 << 20 // 1MB + +// loadMinimalEnvSettings 从 ~/.claude/setting.json 只提取 env 配置。 +// 只接受字符串类型的值；文件缺失/解析失败/超限都返回空。 +func loadMinimalEnvSettings() map[string]string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return nil + } + + settingPath := filepath.Join(home, ".claude", "setting.json") + info, err := os.Stat(settingPath) + if err != nil || info.Size() > maxClaudeSettingsBytes { + return nil + } + + data, err := os.ReadFile(settingPath) + if err != nil { + return nil + } + + var cfg struct { + Env map[string]any `json:"env"` + } + if err := json.Unmarshal(data, &cfg); err != nil { + return nil + } + if len(cfg.Env) == 0 { + return nil + } + + env := make(map[string]string, len(cfg.Env)) + for k, v := range cfg.Env { + s, ok := v.(string) + if !ok { + continue + } + env[k] = s + } + if len(env) == 0 { + return nil + } + return env +} + func buildClaudeArgs(cfg *Config, targetArg string) []string { if cfg == nil { return nil diff --git a/codeagent-wrapper/backend_test.go b/codeagent-wrapper/backend_test.go index 9e894e9..402d23c 100644 --- a/codeagent-wrapper/backend_test.go +++ b/codeagent-wrapper/backend_test.go @@ -1,7 +1,9 @@ package main import ( + "bytes" "os" + "path/filepath" "reflect" "testing" ) @@ -148,3 +150,64 @@ func TestClaudeBuildArgs_BackendMetadata(t *testing.T) { } } } + +func TestLoadMinimalEnvSettings(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + + t.Run("missing file returns empty", func(t *testing.T) { + if got := loadMinimalEnvSettings(); len(got) != 0 { + t.Fatalf("got %v, want empty", got) + } + }) + + t.Run("valid env returns string map", func(t *testing.T) { + dir := filepath.Join(home, ".claude") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + path := filepath.Join(dir, "setting.json") + data := []byte(`{"env":{"ANTHROPIC_API_KEY":"secret","FOO":"bar"}}`) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + got := loadMinimalEnvSettings() + if got["ANTHROPIC_API_KEY"] != "secret" || got["FOO"] != "bar" { + t.Fatalf("got %v, want keys present", got) + } + }) + + t.Run("non-string values are ignored", func(t *testing.T) { + dir := filepath.Join(home, ".claude") + path := filepath.Join(dir, "setting.json") + data := []byte(`{"env":{"GOOD":"ok","BAD":123,"ALSO_BAD":true}}`) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + got := loadMinimalEnvSettings() + if got["GOOD"] != "ok" { + t.Fatalf("got %v, want GOOD=ok", got) + } + if _, ok := got["BAD"]; ok { + t.Fatalf("got %v, want BAD omitted", got) + } + if _, ok := got["ALSO_BAD"]; ok { + t.Fatalf("got %v, want ALSO_BAD omitted", got) + } + }) + + t.Run("oversized file returns empty", func(t *testing.T) { + dir := filepath.Join(home, ".claude") + path := filepath.Join(dir, "setting.json") + data := bytes.Repeat([]byte("a"), maxClaudeSettingsBytes+1) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if got := loadMinimalEnvSettings(); len(got) != 0 { + t.Fatalf("got %v, want empty", got) + } + }) +} diff --git a/codeagent-wrapper/executor.go b/codeagent-wrapper/executor.go index c515c04..3b48aa7 100644 --- a/codeagent-wrapper/executor.go +++ b/codeagent-wrapper/executor.go @@ -26,6 +26,7 @@ type commandRunner interface { StdinPipe() (io.WriteCloser, error) SetStderr(io.Writer) SetDir(string) + SetEnv(env map[string]string) Process() processHandle } @@ -81,6 +82,52 @@ func (r *realCmd) SetDir(dir string) { } } +func (r *realCmd) SetEnv(env map[string]string) { + if r == nil || r.cmd == nil || len(env) == 0 { + return + } + + merged := make(map[string]string, len(env)+len(os.Environ())) + for _, kv := range os.Environ() { + if kv == "" { + continue + } + idx := strings.IndexByte(kv, '=') + if idx <= 0 { + continue + } + merged[kv[:idx]] = kv[idx+1:] + } + for _, kv := range r.cmd.Env { + if kv == "" { + continue + } + idx := strings.IndexByte(kv, '=') + if idx <= 0 { + continue + } + merged[kv[:idx]] = kv[idx+1:] + } + for k, v := range env { + if strings.TrimSpace(k) == "" { + continue + } + merged[k] = v + } + + keys := make([]string, 0, len(merged)) + for k := range merged { + keys = append(keys, k) + } + sort.Strings(keys) + + out := make([]string, 0, len(keys)) + for _, k := range keys { + out = append(out, k+"="+merged[k]) + } + r.cmd.Env = out +} + func (r *realCmd) Process() processHandle { if r == nil || r.cmd == nil || r.cmd.Process == nil { return nil @@ -701,6 +748,12 @@ func runCodexTaskWithContext(parentCtx context.Context, taskSpec TaskSpec, backe cmd := newCommandRunner(ctx, commandName, codexArgs...) + if cfg.Backend == "claude" { + if env := loadMinimalEnvSettings(); len(env) > 0 { + cmd.SetEnv(env) + } + } + // For backends that don't support -C flag (claude, gemini), set working directory via cmd.Dir // Codex passes workdir via -C flag, so we skip setting Dir for it to avoid conflicts if cfg.Mode != "resume" && commandName != "codex" && cfg.WorkDir != "" { diff --git a/codeagent-wrapper/executor_concurrent_test.go b/codeagent-wrapper/executor_concurrent_test.go index d45d5ad..a77ae7d 100644 --- a/codeagent-wrapper/executor_concurrent_test.go +++ b/codeagent-wrapper/executor_concurrent_test.go @@ -87,6 +87,7 @@ type execFakeRunner struct { process processHandle stdin io.WriteCloser dir string + env map[string]string waitErr error waitDelay time.Duration startErr error @@ -129,6 +130,17 @@ func (f *execFakeRunner) StdinPipe() (io.WriteCloser, error) { } func (f *execFakeRunner) SetStderr(io.Writer) {} func (f *execFakeRunner) SetDir(dir string) { f.dir = dir } +func (f *execFakeRunner) SetEnv(env map[string]string) { + if len(env) == 0 { + return + } + if f.env == nil { + f.env = make(map[string]string, len(env)) + } + for k, v := range env { + f.env[k] = v + } +} func (f *execFakeRunner) Process() processHandle { if f.process != nil { return f.process diff --git a/codeagent-wrapper/main_test.go b/codeagent-wrapper/main_test.go index 1aa218b..b55a06d 100644 --- a/codeagent-wrapper/main_test.go +++ b/codeagent-wrapper/main_test.go @@ -255,6 +255,10 @@ func (d *drainBlockingCmd) SetDir(dir string) { d.inner.SetDir(dir) } +func (d *drainBlockingCmd) SetEnv(env map[string]string) { + d.inner.SetEnv(env) +} + func (d *drainBlockingCmd) Process() processHandle { return d.inner.Process() } @@ -387,6 +391,8 @@ type fakeCmd struct { stderr io.Writer + env map[string]string + waitDelay time.Duration waitErr error startErr error @@ -511,6 +517,20 @@ func (f *fakeCmd) SetStderr(w io.Writer) { func (f *fakeCmd) SetDir(string) {} +func (f *fakeCmd) SetEnv(env map[string]string) { + if len(env) == 0 { + return + } + f.mu.Lock() + defer f.mu.Unlock() + if f.env == nil { + f.env = make(map[string]string, len(env)) + } + for k, v := range env { + f.env[k] = v + } +} + func (f *fakeCmd) Process() processHandle { if f == nil { return nil @@ -1549,11 +1569,11 @@ func TestBackendBuildArgs_ClaudeBackend(t *testing.T) { got := backend.BuildArgs(cfg, "todo") want := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose", "todo"} if len(got) != len(want) { - t.Fatalf("length mismatch") + t.Fatalf("args length=%d, want %d: %v", len(got), len(want), got) } for i := range want { if got[i] != want[i] { - t.Fatalf("index %d got %s want %s", i, got[i], want[i]) + t.Fatalf("index %d got %q want %q (args=%v)", i, got[i], want[i], got) } } @@ -1568,19 +1588,15 @@ func TestClaudeBackendBuildArgs_OutputValidation(t *testing.T) { target := "ensure-flags" args := backend.BuildArgs(cfg, target) - expectedPrefix := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose"} - - if len(args) != len(expectedPrefix)+1 { - t.Fatalf("args length=%d, want %d", len(args), len(expectedPrefix)+1) + want := []string{"-p", "--setting-sources", "", "--output-format", "stream-json", "--verbose", target} + if len(args) != len(want) { + t.Fatalf("args length=%d, want %d: %v", len(args), len(want), args) } - for i, val := range expectedPrefix { - if args[i] != val { - t.Fatalf("args[%d]=%q, want %q", i, args[i], val) + for i := range want { + if args[i] != want[i] { + t.Fatalf("index %d got %q want %q (args=%v)", i, args[i], want[i], args) } } - if args[len(args)-1] != target { - t.Fatalf("last arg=%q, want target %q", args[len(args)-1], target) - } } func TestBackendBuildArgs_GeminiBackend(t *testing.T) {