fix: sanitise MCP server subprocess environment

MCP server children no longer inherit the full odek process environment.
They receive a minimal allowlist of safe variables plus explicit env
overrides, and keys matching secret patterns are always stripped.

- Replace inherited os.Environ() with an allowlist (PATH, HOME, LANG, etc.)
  in internal/mcpclient/client.go buildEnv.
- Block *_API_KEY, *_TOKEN, *_SECRET, *_PASSWORD, *_CREDENTIAL,
  *_PRIVATE_KEY, and *_ACCESS_KEY from both inherited env and overrides.
- Always set cmd.Env so servers with no overrides still get sanitised env.
- Add regression tests for allowlist/blocklist behaviour.
- Update docs/MCP.md, docs/SECURITY.md, and AGENTS.md.

Author: jkyberneees

AGENTS.md

@@ -133,6 +133,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Sandbox volume confinement** (`internal/sandbox/sandbox.go`) — extra `--sandbox-volume` host paths must resolve to a location under the working directory, cannot contain `..` or symlink escapes, and cannot match sensitive prefixes such as `/etc`, `/proc`, `/sys`, `/dev`, `/root`, `/home`, `/var`, `/run`, or `/var/run/docker.sock`.
 - **Sandbox read-only enforcement** (`cmd/odek/sandbox_file.go` + `cmd/odek/file_tool.go` + `cmd/odek/perf_tools.go`) — when a sandbox container is active, `write_file`, `patch`, and `batch_patch` translate host paths to `/workspace/...` and copy data into the container with `docker cp`, so a read-only workspace mount (`--sandbox-readonly`) is enforced for the agent's own file tools.
 - **Project config sensitive-field rejection** (`internal/config/loader.go`) — `./odek.json` is untrusted, so `base_url`, `api_key`, `system`, and the `dangerous` section set there are ignored (with stderr warnings). These can only be configured from operator-controlled sources: `~/.odek/config.json`, `ODEK_*` env vars, or CLI flags.
+- **MCP subprocess environment sanitisation** (`internal/mcpclient/client.go`) — MCP server children receive only a minimal allowlist of safe environment variables plus explicit `env` overrides. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, etc.) are stripped, preventing a compromised or malicious MCP server from reading parent secrets.
 - **Telegram photo caption wrapping** (`cmd/odek/telegram.go`) — photo captions cross the Telegram trust boundary, so they are wrapped as untrusted content both when passed to the local vision model and when injected into the main agent's user message.
 - **Secret redaction** (`internal/redact/redact.go`) — 20+ patterns: OpenAI, Anthropic, GitHub PAT, AWS, PEM, JWT, Vault, Google OAuth, SendGrid, Discord, DB URLs, etc.
 

docs/MCP.md

@@ -108,11 +108,11 @@ Add `mcp_servers` to `~/.odek/config.json` (global, operator-trusted) or `odek.j
       "command": "uvx",
       "args": ["mcp-server-fetch"]
     },
-    "github": {
-      "command": "node",
-      "args": ["/path/to/github-mcp-server/index.js"],
+    "fetch": {
+      "command": "uvx",
+      "args": ["mcp-server-fetch"],
       "env": {
-        "GITHUB_TOKEN": "${GITHUB_TOKEN}"
+        "LOG_LEVEL": "debug"
       }
     }
   }
@@ -124,6 +124,14 @@ Each server is defined by:
 - `args` — optional command-line arguments
 - `env` — optional environment variable overrides (empty string removes the variable)
 
+> **Environment sanitisation.** MCP server children receive only a minimal
+> allowlist of safe variables (e.g. `PATH`, `HOME`, `LANG`) plus the overrides
+> from `env`. Keys matching secret patterns (`*_API_KEY`, `*_TOKEN`,
+> `*_SECRET`, `*_PASSWORD`, etc.) are stripped even when listed in `env`, so a
+> compromised server cannot exfiltrate parent secrets. Pass authentication
+> material via server-specific config files or command-line arguments instead
+> of environment variables.
+
 The format matches Claude Code's `mcpServers` config — any MCP server you use
 with Claude Code can be added to odek's config.
 

docs/SECURITY.md

@@ -308,6 +308,10 @@ exceed the cap are rejected before they are written.
 
 These fields can only be set from operator-controlled sources: `~/.odek/config.json`, `ODEK_*` environment variables, or CLI flags.
 
+### 19. MCP server environment sanitisation
+
+MCP server subprocesses no longer inherit the full odek process environment. They receive only a minimal allowlist of safe variables (e.g. `PATH`, `HOME`, `LANG`, `TMPDIR`) plus any explicit `env` overrides from the server config. Keys matching secret patterns — `*_API_KEY`, `*_TOKEN`, `*_SECRET`, `*_PASSWORD`, `*_CREDENTIAL`, `*_PRIVATE_KEY`, etc. — are stripped even when listed in `env`. This prevents a compromised or malicious MCP server from reading secrets loaded from `~/.odek/secrets.env` or other provider keys that were present in the parent environment.
+
 ### YOLO mode
 
 ```json

internal/mcpclient/client.go

@@ -167,10 +167,9 @@ type Client struct {
 func New(name string, cfg ServerConfig) (*Client, error) {
 	cmd := exec.Command(cfg.Command, cfg.Args...)
 
-	// Apply env overrides
-	if len(cfg.Env) > 0 {
-		cmd.Env = buildEnv(cfg.Env)
-	}
+	// Apply env overrides. Always build a sanitized environment so MCP children
+	// do not inherit the full parent environment (API keys, tokens, secrets).
+	cmd.Env = buildEnv(cfg.Env)
 
 	stdin, err := cmd.StdinPipe()
 	if err != nil {
@@ -213,25 +212,79 @@ func New(name string, cfg ServerConfig) (*Client, error) {
 	return c, nil
 }
 
+// allowedEnvVars is the allowlist of parent environment variables that may be
+// forwarded to MCP server subprocesses. It contains only non-sensitive,
+// commonly-required variables (e.g. PATH so the server can find binaries).
+var allowedEnvVars = map[string]bool{
+	"PATH": true,
+	"HOME": true,
+	"USER": true,
+	"LOGNAME": true,
+	"SHELL": true,
+	"TMPDIR": true,
+	"LANG": true,
+	"LC_ALL": true,
+	"LC_CTYPE": true,
+	"LC_MESSAGES": true,
+	"LC_NUMERIC": true,
+	"LC_TIME": true,
+	"LC_COLLATE": true,
+	"LC_MONETARY": true,
+	"LC_PAPER": true,
+	"LC_NAME": true,
+	"LC_ADDRESS": true,
+	"LC_TELEPHONE": true,
+	"LC_MEASUREMENT": true,
+	"LC_IDENTIFICATION": true,
+	"TZ": true,
+	"TERM": true,
+}
+
+// isSensitiveEnvVar reports whether a key looks like a secret. These patterns
+// are blocked from being forwarded to MCP children even if they are present in
+// the parent environment or explicitly supplied as overrides.
+func isSensitiveEnvVar(key string) bool {
+	upper := strings.ToUpper(key)
+	for _, pat := range []string{
+		"API_KEY", "TOKEN", "SECRET", "PASSWORD", "CREDENTIAL", "CREDS",
+		"PRIVATE_KEY", "ACCESS_KEY",
+	} {
+		if strings.Contains(upper, pat) {
+			return true
+		}
+	}
+	return false
+}
+
 // buildEnv constructs the environment for the subprocess.
-// Overrides ODEK_ prefixed env vars cannot be removed.
+//
+// Only a small allowlist of parent environment variables is forwarded, plus any
+// overrides from the MCP server config. Keys that look like secrets (e.g.
+// *_API_KEY, *_TOKEN, *_SECRET) are always stripped, even when provided as
+// overrides, so a compromised or malicious MCP server cannot exfiltrate tokens.
 func buildEnv(overrides map[string]string) []string {
 	// Start with current env
 	env := osEnviron()
 	if env == nil {
 		env = environ() // fallback for testing
 	}
 
-	// Build a map for efficient update
-	envMap := make(map[string]string, len(env))
+	// Build a map from the allowlist only.
+	envMap := make(map[string]string)
 	for _, e := range env {
 		if k, v, ok := strings.Cut(e, "="); ok {
-			envMap[k] = v
+			if allowedEnvVars[k] && !isSensitiveEnvVar(k) {
+				envMap[k] = v
+			}
 		}
 	}
 
-	// Apply overrides
+	// Apply overrides. Sensitive overrides are dropped; empty values remove the
+	// variable.
 	for k, v := range overrides {
+		if isSensitiveEnvVar(k) {
+			continue
+		}
 		if v == "" {
 			delete(envMap, k)
 		} else {

internal/mcpclient/client_test.go

@@ -282,16 +282,78 @@ func TestBuildEnv_Overrides(t *testing.T) {
 
 func TestBuildEnv_RemovesEmptyValue(t *testing.T) {
 	result := buildEnv(map[string]string{
-		"REMOVE_ME": "", // empty = remove from env
+		"PATH": "", // empty = remove from env
 	})
 
 	for _, e := range result {
-		if len(e) > 8 && e[:9] == "REMOVE_ME" {
-			t.Errorf("expected REMOVE_ME to be removed, but found: %s", e)
+		if strings.HasPrefix(e, "PATH=") {
+			t.Errorf("expected PATH to be removed, but found: %s", e)
 		}
 	}
 }
 
+func TestBuildEnv_AllowlistBlocksSecrets(t *testing.T) {
+	orig := osEnviron
+	osEnviron = func() []string {
+		return []string{
+			"PATH=/usr/bin",
+			"HOME=/home/user",
+			"ODEK_API_KEY=sk-odek",
+			"GITHUB_TOKEN=ghp-secret",
+			"SOME_SECRET=shh",
+			"MY_PASSWORD=hunter2",
+		}
+	}
+	defer func() { osEnviron = orig }()
+
+	result := buildEnv(nil)
+	m := envToMap(result)
+
+	if m["PATH"] != "/usr/bin" {
+		t.Errorf("PATH = %q, want /usr/bin", m["PATH"])
+	}
+	if m["HOME"] != "/home/user" {
+		t.Errorf("HOME = %q, want /home/user", m["HOME"])
+	}
+	for _, k := range []string{"ODEK_API_KEY", "GITHUB_TOKEN", "SOME_SECRET", "MY_PASSWORD"} {
+		if _, ok := m[k]; ok {
+			t.Errorf("sensitive key %q should not be forwarded", k)
+		}
+	}
+}
+
+func TestBuildEnv_OverridesCannotInjectSecrets(t *testing.T) {
+	orig := osEnviron
+	osEnviron = func() []string { return []string{"PATH=/usr/bin"} }
+	defer func() { osEnviron = orig }()
+
+	result := buildEnv(map[string]string{
+		"LEGIT_VAR":   "ok",
+		"EVIL_API_KEY": "sk-stolen",
+		"BOT_TOKEN":    "tok-stolen",
+	})
+	m := envToMap(result)
+
+	if m["LEGIT_VAR"] != "ok" {
+		t.Errorf("LEGIT_VAR = %q, want ok", m["LEGIT_VAR"])
+	}
+	for _, k := range []string{"EVIL_API_KEY", "BOT_TOKEN"} {
+		if _, ok := m[k]; ok {
+			t.Errorf("sensitive override %q should be dropped", k)
+		}
+	}
+}
+
+func envToMap(env []string) map[string]string {
+	m := make(map[string]string, len(env))
+	for _, e := range env {
+		if k, v, ok := strings.Cut(e, "="); ok {
+			m[k] = v
+		}
+	}
+	return m
+}
+
 func TestDiscover_FailsOnDeadProcess(t *testing.T) {
 	client, err := New("dead", ServerConfig{
 		Command: fakeServerPath(t),