Strip provider API keys from a model's shell environment

docs/changes/unreleased/1484-provider-keys-in-shell.md

@@ -0,0 +1,8 @@
+---
+kind: fixed
+title: Provider keys no longer reach a model's shell commands
+pr: 1484
+surface: [engine]
+invalidates:
+  - A bash command the model ran, foreground or background, inherited codeaf's whole process environment; a provider credential such as `OPENROUTER_API_KEY` was readable and printable from inside it. `JobShellEnv` now strips every provider key codeaf knows about — the OpenRouter/OpenAI variables, each vendored service's key variable, and any custom key variable a person named for a connected service — unless `CODEAF_ALLOW_PROVIDER_KEYS_IN_SHELL` is set in codeaf's own environment.
+---

internal/config/settings.go

@@ -1362,6 +1362,15 @@ var OperatorEnvPins = []string{
 	"CODEAF_MODEL_POOL_SUBMIT_URL",
 	"CODEAF_MODEL_POOL_MIRROR_URL",
 	"CODEAF_MODEL_POOL_TTL",
+	// CODEAF_ALLOW_PROVIDER_KEYS_IN_SHELL is the opt-in that keeps a provider
+	// credential (OPENROUTER_API_KEY and its siblings) in the environment of a
+	// bash command a model runs (internal/exec's JobShellEnv, issue #1484); unset,
+	// every such key is stripped before the shell ever starts. It is plumbing and
+	// not a row for the reason CODEAF_CALL_LOG_BODIES is: keeping a secret
+	// readable by the model's own commands is not a preference a sheet should
+	// make convenient, it is a decision somebody makes on purpose, in a shell,
+	// for one run that genuinely needs it.
+	"CODEAF_ALLOW_PROVIDER_KEYS_IN_SHELL",
 }
 
 // Defaults the registry owns beyond the ones config.go already declares.

internal/exec/bare/shell_env_key_test.go

@@ -0,0 +1,16 @@
+package bare
+
+import (
+	"strings"
+	"testing"
+)
+
+// A provider key in codeaf's own environment must not reach a model's shell.
+func TestAProviderKeyDoesNotReachAModelsShell(t *testing.T) {
+	t.Setenv("OPENROUTER_API_KEY", "probe-not-a-key")
+	for _, entry := range StreamingEnv() {
+		if strings.HasPrefix(entry, "OPENROUTER_API_KEY=") {
+			t.Fatal("OPENROUTER_API_KEY reaches the environment of the model's shell")
+		}
+	}
+}

internal/exec/bare/streaming.go

@@ -104,13 +104,15 @@ func StreamingShell(command string) (string, []string) {
 // StreamingEnv is the environment such a command runs in: the person's own,
 // plus the one variable that stops Python holding its output back.
 //
-// IT ALSO CARRIES THE TMUX FLOOR. A command the model runs must not reach the
-// tmux server hosting codeaf, so the environment is passed through
-// internal/exec's [exec.JobShellEnv], which strips TMUX and TMUX_PANE and points
-// TMUX_TMPDIR at a directory codeaf owns (see tools.go there for why both halves
-// are the floor). This is the bare path's one seam for a model's shell — the
-// foreground bash tool and the session's job registry both reach it through
-// here — so the strip lives here rather than at each caller.
+// IT ALSO CARRIES THE TMUX FLOOR AND STRIPS PROVIDER KEYS. A command the model
+// runs must not reach the tmux server hosting codeaf, and must not read back a
+// provider credential codeaf itself holds, so the environment is passed
+// through internal/exec's [exec.JobShellEnv], which strips TMUX, TMUX_PANE and
+// every provider key codeaf knows about, and points TMUX_TMPDIR at a directory
+// codeaf owns (see tools.go for why both tmux halves are the floor, and for the
+// opt-in a task can use to keep a key). This is the bare path's one seam for a
+// model's shell — the foreground bash tool and the session's job registry both
+// reach it through here — so the strip lives here rather than at each caller.
 //
 // THE PERSON'S OWN SETTING WINS. Somebody who exported PYTHONUNBUFFERED
 // themselves — to any value, including an empty one — meant it, and a harness

internal/exec/providerkeyenv_test.go

@@ -0,0 +1,86 @@
+package exec
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/Agent-Field/codeaf/internal/config"
+	"github.com/Agent-Field/codeaf/internal/env"
+)
+
+// unsetOwned clears both spellings of an owned variable, so a test asserting
+// the "off" state cannot pass by accident of whatever the AFORGE_ legacy name
+// happens to hold in the runner's own environment.
+func unsetOwned(t *testing.T, name string) {
+	t.Helper()
+	t.Setenv(name, "")
+	t.Setenv(env.Legacy(name), "")
+}
+
+// A MODEL'S SHELL MUST NOT INHERIT A PROVIDER CREDENTIAL.
+//
+// codeaf exports OPENROUTER_API_KEY (or OPENAI_API_KEY, or a vendored
+// service's own KeyEnv) into its own process to talk to a provider; a bash
+// call the model runs used to inherit that value and could print it straight
+// back (issue #1484).
+func TestJobShellEnvStripsProviderKeys(t *testing.T) {
+	t.Setenv("CODEAF_PROFILE_DIR", t.TempDir())
+	unsetOwned(t, AllowProviderKeysInShell)
+	environment := JobShellEnv([]string{
+		"OPENROUTER_API_KEY=sk-or-v1-secret",
+		"OPENAI_API_KEY=sk-secret",
+		"DEEPSEEK_API_KEY=secret",
+		"PATH=/usr/bin",
+	})
+	for _, name := range []string{"OPENROUTER_API_KEY", "OPENAI_API_KEY", "DEEPSEEK_API_KEY"} {
+		if _, ok := envValue(environment, name); ok {
+			t.Errorf("%s reached the job shell environment", name)
+		}
+	}
+	if value, ok := envValue(environment, "PATH"); !ok || value != "/usr/bin" {
+		t.Errorf("PATH = %q (present %v), want /usr/bin", value, ok)
+	}
+}
+
+// A CUSTOM KEY VARIABLE IS STILL A PROVIDER CREDENTIAL. A person can name
+// their own environment variable for a connected service's key
+// (config.PersistedSource.KeyEnv); it must be stripped exactly like a
+// conventional one.
+func TestJobShellEnvStripsACustomNamedProviderKey(t *testing.T) {
+	profile := t.TempDir()
+	t.Setenv("CODEAF_PROFILE_DIR", profile)
+	unsetOwned(t, AllowProviderKeysInShell)
+	if err := config.WriteSources(profile, []config.PersistedSource{
+		{ID: "z-ai", Written: "z-ai", KeyEnv: "MY_ZAI_KEY"},
+	}); err != nil {
+		t.Fatalf("WriteSources: %v", err)
+	}
+
+	environment := JobShellEnv([]string{"MY_ZAI_KEY=secret", "PATH=/usr/bin"})
+	if _, ok := envValue(environment, "MY_ZAI_KEY"); ok {
+		t.Error("MY_ZAI_KEY reached the job shell environment")
+	}
+}
+
+// THE OPT-IN IS CODEAF'S OWN ENVIRONMENT, NOT THE MODEL'S COMMAND. Whoever
+// started codeaf can ask for a task that genuinely needs the running key by
+// exporting AllowProviderKeysInShell before codeaf starts; the model cannot
+// grant this to itself.
+func TestJobShellEnvAllowProviderKeysInShellOptsIn(t *testing.T) {
+	t.Setenv("CODEAF_PROFILE_DIR", t.TempDir())
+	t.Setenv(AllowProviderKeysInShell, "1")
+	environment := JobShellEnv([]string{"OPENROUTER_API_KEY=sk-or-v1-secret"})
+	if value, ok := envValue(environment, "OPENROUTER_API_KEY"); !ok || value != "sk-or-v1-secret" {
+		t.Errorf("OPENROUTER_API_KEY = %q (present %v), want it kept under the opt-in", value, ok)
+	}
+}
+
+func envValue(env []string, name string) (string, bool) {
+	prefix := name + "="
+	for _, entry := range env {
+		if strings.HasPrefix(entry, prefix) {
+			return strings.TrimPrefix(entry, prefix), true
+		}
+	}
+	return "", false
+}

internal/exec/tools.go

@@ -23,6 +23,7 @@ import (
 	"github.com/Agent-Field/codeaf/internal/ctxbudget"
 	"github.com/Agent-Field/codeaf/internal/env"
 	"github.com/Agent-Field/codeaf/internal/guard"
+	"github.com/Agent-Field/codeaf/internal/modelsource"
 	"github.com/Agent-Field/codeaf/internal/processgroup"
 	"github.com/Agent-Field/codeaf/internal/rtk"
 	"github.com/Agent-Field/codeaf/internal/store"
@@ -1913,9 +1914,19 @@ func replaceEnv(environment []string, key, value string) []string {
 	return append(replaced, prefix+value)
 }
 
+// AllowProviderKeysInShell is the opt-in that keeps a provider credential in a
+// model's shell. It is read from codeaf's own environment through env.Get, so
+// it carries the same one-release AFORGE_ fallback every owned variable does,
+// and it is never read from a model's command, so only whoever started codeaf
+// can grant it — a task that genuinely needs the running key (a script that
+// calls the provider's API itself, say) gets it by exporting this before
+// codeaf starts, not by asking the model to.
+const AllowProviderKeysInShell = "CODEAF_ALLOW_PROVIDER_KEYS_IN_SHELL"
+
 // JobShellEnv is the environment every shell a model's command runs in must
-// receive: with TMUX and TMUX_PANE removed, and TMUX_TMPDIR pointed at a socket
-// directory codeaf owns.
+// receive: with TMUX and TMUX_PANE removed, TMUX_TMPDIR pointed at a socket
+// directory codeaf owns, and every provider credential codeaf itself knows
+// about removed unless [AllowProviderKeysInShell] says otherwise.
 //
 // A bash call the model runs inherits this process's environment, TMUX and
 // TMUX_PANE included, so a bare `tmux` it runs targets the very server hosting
@@ -1931,16 +1942,50 @@ func replaceEnv(environment []string, key, value string) []string {
 // `tmux` reaches, and nothing outside it. A test that needs its own tmux still
 // works: it gets that private TMUX_TMPDIR rather than a stripped-to-broken env.
 //
+// A PROVIDER KEY EXPORTED FOR CODEAF IS NOT A KEY HANDED TO THE MODEL. A
+// person who exported OPENROUTER_API_KEY so codeaf could talk to a provider
+// did not thereby mean every shell command the model runs should be able to
+// read it back and print it (issue #1484). A key that lives only in the
+// profile file never reaches os.Environ() in the first place — config.APIKeyAt
+// reads it without exporting it — so stripping the environment is the whole
+// fix; there is nothing there to leave behind.
+//
 // A NIL SLICE IS THE PARENT'S ENVIRONMENT. runShell and the background-job
 // registry leave cmd.Env unset on the benchmarked bare path, which inherits
-// everything; the caller must now hand a real, TMUX-stripped environment, so a
-// nil here is read as os.Environ() and stripped the same way.
+// everything; the caller must now hand a real, stripped environment, so a nil
+// here is read as os.Environ() and stripped the same way.
 func JobShellEnv(environment []string) []string {
 	if environment == nil {
 		environment = os.Environ()
 	}
 	environment = withoutEnv(environment, "TMUX", "TMUX_PANE")
-	return replaceEnv(environment, "TMUX_TMPDIR", jobTmuxDir())
+	environment = replaceEnv(environment, "TMUX_TMPDIR", jobTmuxDir())
+	if env.Get(AllowProviderKeysInShell) == "" {
+		environment = withoutEnv(environment, providerKeyEnvNames()...)
+	}
+	return environment
+}
+
+// providerKeyEnvNames is every environment variable a provider credential can
+// live in: the two [config.APIKeyAt] reads directly, one per vendored service
+// (modelsource.Vendored's KeyEnv), and one per custom variable a person named
+// for a connected service in this profile (config.PersistedSource.KeyEnv,
+// config.sourceKeyFromRow's third rung) — a service added later, or a person's
+// own MY_ZAI_KEY, is covered without anyone remembering this list. It is read
+// fresh rather than cached for the same reason.
+func providerKeyEnvNames() []string {
+	names := []string{config.APIKeyEnv, "OPENAI_API_KEY"}
+	for _, source := range modelsource.Vendored() {
+		if source.KeyEnv != "" {
+			names = append(names, source.KeyEnv)
+		}
+	}
+	for _, row := range config.PersistedSources(config.ProfileDir()) {
+		if row.KeyEnv != "" {
+			names = append(names, row.KeyEnv)
+		}
+	}
+	return names
 }
 
 // withoutEnv drops the named variables from an environment slice. It is

internal/manual/chat/running-from-the-terminal.md

@@ -864,6 +864,13 @@ Each directly connected service may instead name its own environment variable, w
 stored with that service. `codeaf doctor`'s first row still reports only which default-service key answered — `key set · OPENROUTER_API_KEY`, or
 `key set · /home/you/.codeaf/config.json`, or `key none ·` and the two lines above.
 
+**None of these keys reach a shell command the model runs.** `OPENROUTER_API_KEY`,
+`OPENAI_API_KEY` and every connected service's own key variable are stripped before a
+bash call or background job starts, the same way `TMUX` is, so a command cannot read one
+back or print it. A task that genuinely needs the running key — a script that calls the
+provider's API itself, say — gets it by exporting `CODEAF_ALLOW_PROVIDER_KEYS_IN_SHELL=1`
+in the shell that starts codeaf, not by asking the model to.
+
 **These change state without model spending**: `connect`, `disconnect`, `cache clean`,
 `rebuild`, `notebook retract|restore`, `services stop` and `devices revoke`. A browser
 connection may make authentication and model-list network requests, but sends no prompt.