diff --git a/docs/changes/unreleased/1484-provider-keys-in-shell.md b/docs/changes/unreleased/1484-provider-keys-in-shell.md new file mode 100644 index 0000000000..21098b2aa0 --- /dev/null +++ b/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. +--- diff --git a/internal/config/settings.go b/internal/config/settings.go index 8f53d85d88..d03bf2e56c 100644 --- a/internal/config/settings.go +++ b/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. diff --git a/internal/exec/bare/shell_env_key_test.go b/internal/exec/bare/shell_env_key_test.go new file mode 100644 index 0000000000..2bb0be1e86 --- /dev/null +++ b/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") + } + } +} diff --git a/internal/exec/bare/streaming.go b/internal/exec/bare/streaming.go index 658adcaf14..ce3c7e0952 100644 --- a/internal/exec/bare/streaming.go +++ b/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 diff --git a/internal/exec/providerkeyenv_test.go b/internal/exec/providerkeyenv_test.go new file mode 100644 index 0000000000..0dad212ef4 --- /dev/null +++ b/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 +} diff --git a/internal/exec/tools.go b/internal/exec/tools.go index 35fff9418d..71829ceefd 100644 --- a/internal/exec/tools.go +++ b/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 diff --git a/internal/manual/chat/running-from-the-terminal.md b/internal/manual/chat/running-from-the-terminal.md index 27c60a2479..9ad4506d99 100644 --- a/internal/manual/chat/running-from-the-terminal.md +++ b/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.