From 2477e5023031f6f8795983f0fc355b369d7ef538 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 01:29:29 -0400 Subject: [PATCH] fix(opencode): address gadfly's dogfood review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gadfly's own swarm reviewed PR #26 and reached consensus (3/3 models) on a real bug, plus flagged security/maintainability items. Fixes: - Pass-through auth (BLOCKING, 3/3 agreement): openCodeEnv() stripped every provider key except OLLAMA_API_KEY, so the documented opencode// escape hatch (e.g. opencode/anthropic/...) had no way to authenticate — the reusable workflow forwards ANTHROPIC_API_KEY/OPENAI_API_KEY into the container and the allowlist discarded them. Now forward ANTHROPIC_*/OPENAI_*/GOOGLE_*/ GEMINI_* so OpenCode's built-in providers can authenticate, while still withholding gadfly's own secrets (Gitea/findings tokens, claude-code OAuth). - Read-only hardening (security lens): the generated config denied only edit/bash. Using OpenCode's documented permission schema, also deny webfetch/websearch/ external_directory — the network + out-of-sandbox tools — closing the exfiltration surface a prompt-injected review could otherwise reach. Permission is now a map so the deny set is extensible. - Dedup (maintainability lens, 3/3): extract shared filterEnv() and killGroupOnCancel() helpers in engine.go, used by both shell-out engines' runPass/env builders instead of the copy-pasted blocks. - Cosmetic: split the const block so defaultOpenCodeBaseURL's doc comment no longer visually misattaches to the agent-name const. README updated: the read-only note and the reduced-env note now reflect the broader deny set and the forwarded provider keys. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 17 ++++--- cmd/gadfly/engine.go | 48 +++++++++++-------- cmd/gadfly/opencode.go | 92 ++++++++++++++++++++----------------- cmd/gadfly/opencode_test.go | 28 ++++++----- 4 files changed, 106 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index 07d97b9..a7dc0ef 100644 --- a/README.md +++ b/README.md @@ -176,13 +176,16 @@ secret in config). Tuning knobs (all optional): | `GADFLY_OPENCODE_BIN` | `opencode` | CLI binary path | > **Read-only is enforced through config, not a flag.** OpenCode has no `--append-system-prompt`, so -> Gadfly generates a per-lens config — the lens system prompt as a `gadfly` agent's prompt, with -> `edit`/`bash` denied at both the global and agent level — and injects it via `OPENCODE_CONFIG_CONTENT`. -> That env var is the highest-precedence config source in the container, so it **outranks any -> `opencode.json` a reviewed repo ships** — a repo can't re-enable edits on the reviewer. The -> subprocess runs with a **minimal environment** (`OLLAMA_API_KEY` + `PATH`/`HOME`/locale/`OPENCODE_*`/ -> `GADFLY_OPENCODE_*`), not the runner's full env; the Gitea token, Anthropic/Claude keys, and findings -> token aren't handed to the CLI. +> Gadfly generates a per-lens config — the lens system prompt as a `gadfly` agent's prompt, with the +> mutating and network tools (`edit`/`bash`/`webfetch`/`websearch`/`external_directory`) denied at both +> the global and agent level — and injects it via `OPENCODE_CONFIG_CONTENT`. That env var is the +> highest-precedence config source in the container, so it **outranks any `opencode.json` a reviewed +> repo ships** — a repo can't re-enable edits on the reviewer. The subprocess runs with a **reduced +> environment**: the provider keys OpenCode needs to authenticate (`OLLAMA_API_KEY` for the primary +> path, plus `ANTHROPIC_*`/`OPENAI_*`/`GOOGLE_*`/`GEMINI_*` for the `opencode//` +> pass-through) alongside `PATH`/`HOME`/locale/`OPENCODE_*`/`GADFLY_OPENCODE_*` — but **not** gadfly's +> own secrets (the Gitea token, the findings token, or the claude-code subscription token), which the +> CLI has no use for. > **Newly wired, lightly tested.** Like the claude-code engine, `auto` specialist selection and the > `delegate_investigation` worker are majordomo-only and are skipped here (OpenCode does its own diff --git a/cmd/gadfly/engine.go b/cmd/gadfly/engine.go index 0ed5fca..853240f 100644 --- a/cmd/gadfly/engine.go +++ b/cmd/gadfly/engine.go @@ -160,16 +160,7 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i // Force an extended-thinking budget for this run (a "...:max" spec). cmd.Env = append(cmd.Env, "MAX_THINKING_TOKENS="+strconv.Itoa(e.thinkingTokens)) } - // Put the CLI and the Node children it spawns in their own process group and - // kill the WHOLE group on context cancel, so a timed-out lens can't leave - // orphaned claude/node processes behind in the container. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - cmd.Cancel = func() error { - if cmd.Process != nil { - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - } - return nil - } + killGroupOnCancel(cmd) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -220,13 +211,39 @@ func (e *claudeCodeEngine) runPass(ctx context.Context, system, task string, _ i return "", fmt.Errorf("claude -p produced no parseable output") } +// killGroupOnCancel puts cmd in its own process group and, on context cancel, +// SIGKILLs the whole group — so a timed-out shell-out CLI (claude/opencode) can't +// leave orphaned Node children behind in the container. Call before cmd.Run. +func killGroupOnCancel(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { + if cmd.Process != nil { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } + return nil + } +} + +// filterEnv returns the current process environment reduced to the variables for +// which keep returns true. Shared by the shell-out engines' minimal-env builders +// (claudeEnv, openCodeEnv), which differ only in their keep predicate. +func filterEnv(keep func(string) bool) []string { + var env []string + for _, kv := range os.Environ() { + if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { + env = append(env, kv) + } + } + return env +} + // claudeEnv builds a minimal environment for the `claude` subprocess: only what // the CLI needs (PATH/HOME, its auth tokens, locale, Node/XDG/GADFLY_CLAUDE_* // knobs), deliberately dropping the rest of the runner's secrets — GITEA_TOKEN, // GADFLY_FINDINGS_TOKEN, provider keys — so they never reach the third-party // CLI. Defense in depth: the parent already holds them, but the CLI has no need. func claudeEnv() []string { - keep := func(k string) bool { + return filterEnv(func(k string) bool { switch k { case "PATH", "HOME", "USER", "LOGNAME", "TMPDIR", "LANG", "TERM", "SHELL", "MAX_THINKING_TOKENS": return true @@ -237,14 +254,7 @@ func claudeEnv() []string { strings.HasPrefix(k, "GADFLY_CLAUDE_") || strings.HasPrefix(k, "NODE_") || strings.HasPrefix(k, "XDG_") - } - var env []string - for _, kv := range os.Environ() { - if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { - env = append(env, kv) - } - } - return env + }) } // truncateForErr caps CLI error detail so a stderr dump can't bloat the comment, diff --git a/cmd/gadfly/opencode.go b/cmd/gadfly/opencode.go index 785ebeb..c1f9594 100644 --- a/cmd/gadfly/opencode.go +++ b/cmd/gadfly/opencode.go @@ -8,7 +8,6 @@ import ( "os" "os/exec" "strings" - "syscall" ) // openCodeEngine reviews by shelling out to the `opencode` CLI (opencode.ai) in @@ -35,16 +34,17 @@ type openCodeEngine struct { extraArgs []string // appended verbatim (GADFLY_OPENCODE_EXTRA_ARGS) } +// openCodeProviderName / openCodeAgentName are the internal names of the provider +// and agent gadfly generates in the injected config. "gadfly" won't collide with +// OpenCode's models.dev provider registry. const ( - // openCodeProviderName / openCodeAgentName are the internal names of the - // provider and agent gadfly generates in the injected config. "gadfly" won't - // collide with OpenCode's models.dev provider registry. openCodeProviderName = "gadfly" openCodeAgentName = "gadfly" - // defaultOpenCodeBaseURL is ollama-cloud's OpenAI-compatible endpoint. - defaultOpenCodeBaseURL = "https://ollama.com/v1" ) +// defaultOpenCodeBaseURL is ollama-cloud's OpenAI-compatible endpoint. +const defaultOpenCodeBaseURL = "https://ollama.com/v1" + // isOpenCodeSpec reports whether a GADFLY_MODEL spec selects the opencode engine: // the bare id "opencode"/"open-code" or an "opencode/" form (both // spellings accepted; "opencode" is canonical). @@ -122,9 +122,24 @@ type openCodeConfig struct { Agent map[string]openCodeAgent `json:"agent"` } -type openCodePermission struct { - Edit string `json:"edit"` - Bash string `json:"bash"` +// openCodePermission is OpenCode's permission map: tool key → "allow"|"ask"|"deny". +type openCodePermission map[string]string + +// denyMutations denies every OpenCode permission that could change the repo, run +// commands, or reach the network / the filesystem outside the checked-out tree — +// keeping the reviewer strictly read-only. The read/search tools OpenCode gates +// separately (read/glob/grep/list/lsp) stay at their default so the agent can +// still verify findings against the code. OpenCode's permission keys are +// enumerated at https://opencode.ai/docs/agents; a key it doesn't recognize is +// simply ignored, so listing extras is safe. +func denyMutations() openCodePermission { + return openCodePermission{ + "edit": "deny", + "bash": "deny", + "webfetch": "deny", + "websearch": "deny", + "external_directory": "deny", + } } type openCodeProvider struct { @@ -147,13 +162,13 @@ type openCodeAgent struct { } // config builds the OpenCode config JSON for one pass. The system prompt becomes -// the "gadfly" agent's prompt; edit/bash are denied at BOTH the global and agent -// level (defense in depth — OpenCode's read-only tools stay available). For a -// bare or pass-through spec the generated ollama-cloud provider block is omitted. -// The API key is always the "{env:OLLAMA_API_KEY}" reference, never a literal -// secret baked into the config. +// the "gadfly" agent's prompt; the mutating/network tools are denied at BOTH the +// global and agent level (defense in depth — OpenCode's read tools stay +// available). For a bare or pass-through spec the generated ollama-cloud provider +// block is omitted. The API key is always the "{env:OLLAMA_API_KEY}" reference, +// never a literal secret baked into the config. func (e *openCodeEngine) config(system string) ([]byte, error) { - deny := openCodePermission{Edit: "deny", Bash: "deny"} + deny := denyMutations() cfg := openCodeConfig{ Schema: "https://opencode.ai/config.json", Permission: deny, @@ -194,16 +209,7 @@ func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int // outranks any opencode.json the reviewed repo itself ships. NO_COLOR keeps the // captured stdout free of ANSI decoration. cmd.Env = append(openCodeEnv(), "OPENCODE_CONFIG_CONTENT="+string(cfg), "NO_COLOR=1") - // Put the CLI and the Node children it spawns in their own process group and - // kill the WHOLE group on context cancel, so a timed-out lens can't leave - // orphaned opencode/node processes behind in the container. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - cmd.Cancel = func() error { - if cmd.Process != nil { - _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) - } - return nil - } + killGroupOnCancel(cmd) // don't orphan the CLI's Node children on a timed-out lens var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -233,15 +239,20 @@ func (e *openCodeEngine) runPass(ctx context.Context, system, task string, _ int return "", fmt.Errorf("opencode run returned no output") } -// openCodeEnv builds a minimal environment for the `opencode` subprocess: only -// what the CLI needs (PATH/HOME, locale, Node/XDG/OPENCODE_*/GADFLY_OPENCODE_* -// knobs) plus OLLAMA_API_KEY — which the generated provider references and which -// claudeEnv deliberately DROPS. The runner's other secrets (GITEA_TOKEN, -// GADFLY_FINDINGS_TOKEN, Anthropic/Claude keys) are withheld — the CLI has no need -// for them. OPENCODE_CONFIG_CONTENT is never inherited: runPass sets it, and a -// duplicate key would be ambiguous (getenv returns the first occurrence). +// openCodeEnv builds a minimal environment for the `opencode` subprocess. It +// forwards what the CLI needs to reach a model provider: OLLAMA_API_KEY for the +// generated ollama-cloud provider (the primary opencode/ path), PLUS the +// standard provider keys — ANTHROPIC_*, OPENAI_*, GOOGLE_*, GEMINI_* — so the +// opencode// pass-through form can authenticate against +// OpenCode's own built-in providers (those keys are otherwise stripped, which +// broke the documented escape hatch). It still withholds gadfly's OWN secrets — +// GITEA_TOKEN, GADFLY_API_KEY, GADFLY_FINDINGS_TOKEN, and the claude-code +// subscription token (CLAUDE_CODE_OAUTH_TOKEN, which OpenCode can't use anyway) — +// so they never reach the third-party CLI. OPENCODE_CONFIG_CONTENT is never +// inherited: runPass sets it, and a duplicate key would be ambiguous (getenv +// returns the first occurrence). func openCodeEnv() []string { - keep := func(k string) bool { + return filterEnv(func(k string) bool { if k == "OPENCODE_CONFIG_CONTENT" { return false // set explicitly by runPass; never inherit a competing value } @@ -253,13 +264,10 @@ func openCodeEnv() []string { strings.HasPrefix(k, "OPENCODE_") || strings.HasPrefix(k, "GADFLY_OPENCODE_") || strings.HasPrefix(k, "NODE_") || - strings.HasPrefix(k, "XDG_") - } - var env []string - for _, kv := range os.Environ() { - if k, _, ok := strings.Cut(kv, "="); ok && keep(k) { - env = append(env, kv) - } - } - return env + strings.HasPrefix(k, "XDG_") || + strings.HasPrefix(k, "ANTHROPIC_") || + strings.HasPrefix(k, "OPENAI_") || + strings.HasPrefix(k, "GOOGLE_") || + strings.HasPrefix(k, "GEMINI_") + }) } diff --git a/cmd/gadfly/opencode_test.go b/cmd/gadfly/opencode_test.go index b8e90ac..1a6b8af 100644 --- a/cmd/gadfly/opencode_test.go +++ b/cmd/gadfly/opencode_test.go @@ -138,12 +138,15 @@ func TestOpenCodeConfig(t *testing.T) { if ag.Prompt != sys { t.Errorf("agent prompt = %q, want it to round-trip the system prompt", ag.Prompt) } - if ag.Permission.Edit != "deny" || ag.Permission.Bash != "deny" { - t.Errorf("agent permission = %+v, want edit/bash deny", ag.Permission) - } - // Global permission also denies (defense in depth). - if cfg.Permission.Edit != "deny" || cfg.Permission.Bash != "deny" { - t.Errorf("global permission = %+v, want edit/bash deny", cfg.Permission) + // Mutating/network tools are denied at BOTH the agent and global level (defense + // in depth); the read/search tools stay at OpenCode's default. + for _, k := range []string{"edit", "bash", "webfetch", "websearch", "external_directory"} { + if ag.Permission[k] != "deny" { + t.Errorf("agent permission[%q] = %q, want deny", k, ag.Permission[k]) + } + if cfg.Permission[k] != "deny" { + t.Errorf("global permission[%q] = %q, want deny", k, cfg.Permission[k]) + } } // Provider block: correct npm, default baseURL, env-ref apiKey, model in map. @@ -199,7 +202,8 @@ func TestOpenCodeEnvFilters(t *testing.T) { t.Setenv("OLLAMA_API_KEY", "keep-ollama") t.Setenv("GADFLY_API_KEY", "secret-gadfly") t.Setenv("GADFLY_FINDINGS_TOKEN", "secret-findings") - t.Setenv("ANTHROPIC_API_KEY", "secret-anthropic") + t.Setenv("ANTHROPIC_API_KEY", "keep-anthropic") // pass-through provider auth + t.Setenv("OPENAI_API_KEY", "keep-openai") // pass-through provider auth t.Setenv("CLAUDE_CODE_OAUTH_TOKEN", "secret-claude") t.Setenv("GADFLY_OPENCODE_MODEL", "keep-knob") t.Setenv("OPENCODE_CONFIG_CONTENT", "should-not-inherit") @@ -213,14 +217,16 @@ func TestOpenCodeEnvFilters(t *testing.T) { } return false } - // kept: the ollama key the provider references + opencode knobs + PATH - for _, k := range []string{"OLLAMA_API_KEY", "GADFLY_OPENCODE_MODEL", "PATH"} { + // kept: the ollama key + the standard provider keys the opencode// + // pass-through form needs + opencode knobs + PATH + for _, k := range []string{"OLLAMA_API_KEY", "ANTHROPIC_API_KEY", "OPENAI_API_KEY", "GADFLY_OPENCODE_MODEL", "PATH"} { if !has(k) { t.Errorf("openCodeEnv dropped %s, but it should be kept", k) } } - // dropped: the runner's other secrets + the claude engine's auth - for _, k := range []string{"GITEA_TOKEN", "GADFLY_API_KEY", "GADFLY_FINDINGS_TOKEN", "ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"} { + // dropped: gadfly's own secrets + the claude engine's subscription token + // (OpenCode's anthropic provider uses ANTHROPIC_API_KEY, not this OAuth token). + for _, k := range []string{"GITEA_TOKEN", "GADFLY_API_KEY", "GADFLY_FINDINGS_TOKEN", "CLAUDE_CODE_OAUTH_TOKEN"} { if has(k) { t.Errorf("openCodeEnv leaked %s into the subprocess env", k) }