fix(mcp): normalize env names before secret-pattern matching (#106)

isSensitiveEnvVar substring-matched patterns like API_KEY against the raw
variable name, so MCP server env overrides named API-KEY, APIKEY, or
PRIVATE-KEY - all legal spellings in environment variable names - were
forwarded to potentially untrusted MCP subprocesses. Only the override
path was affected; the inherited-environment path is allowlist-based.

Names are now normalised (uppercased, "-" and "_" stripped) before
matching, and the patterns are stored in normalised form, so
API-KEY/APIKEY/Api-Key/PRIVATE-KEY/SECRET-KEY/AUTH-TOKEN and similar
spellings are all stripped.

Regression tests extend TestBuildEnv_OverridesCannotInjectSecrets with
non-underscore spellings and add TestIsSensitiveEnvVar_NormalizesSeparators
covering blocked spellings and benign names. Docs updated (SECURITY.md
section 19, AGENTS.md).

Author: jkyberneees

AGENTS.md

@@ -163,7 +163,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`, the `dangerous` section, `embedding`, `memory`, `sessions`, `skills.dirs`/`skills.embedding`, `telegram`, and `web_search` set there are ignored (with stderr warnings). Project-level sandbox knobs (`sandbox_env`, `sandbox_image`, `sandbox_network`, `sandbox_volumes`) are not silently ignored either; they are gated by the project-level sandbox approval flow before application. 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.
+- **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. Names are normalised (uppercased, `-`/`_` stripped) before matching, so non-underscore spellings such as `API-KEY` or `APIKEY` cannot slip a secret through the override path.
 - **MCP `tools/list` metadata hardening** (`internal/mcpclient/client.go`, `cmd/odek/mcp_approval.go`, `cmd/odek/main.go`) — tool names from MCP servers are validated (ASCII letters/digits/`-`/`_`, ≤ 64 chars) and descriptions are scanned for injection patterns. Tools whose raw names collide with odek built-ins are rejected. Each tool from a project-level server requires per-tool approval (interactive, `ODEK_APPROVE_MCP=1`, or persisted in `~/.odek/mcp_tool_approvals.json`); global servers from `~/.odek/config.json` are operator-trusted. Approval keys hash the server's `env` map (sorted key/value pairs), and the interactive prompt prints each env variable with its value, so a later `env` change cannot silently reuse an old approval.
 - **Read-only perf-tool file-size cap** (`cmd/odek/perf_tools.go`) — `count_lines`, `checksum`, `head_tail`, and `word_count` reject files larger than 10 MiB before scanning/hashing, consistent with other perf tools.
 - **Inline content size cap** (`cmd/odek/perf_tools.go`) — `base64` and `tr` reject inline `string`/`content` arguments larger than 10 MiB, preventing prompt-injected multi-hundred-megabyte payloads from OOMing the process.

docs/SECURITY.md

@@ -423,7 +423,7 @@ Go's `net.IP.IsPrivate()` covers RFC1918 and RFC4193 private ranges, but it does
 
 ### 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.
+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`. Matching normalises each name first (uppercased, `-` and `_` removed), so non-underscore spellings like `API-KEY` or `APIKEY` — both legal in environment variable names — cannot bypass the filter through the override path. 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.
 
 ### 20. Schedule file atomic-write hardening
 

internal/mcpclient/client.go

@@ -308,13 +308,18 @@ var allowedEnvVars = map[string]bool{
 // 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.
+//
+// The name is normalised (uppercased, "-" and "_" stripped) before matching so
+// non-underscore spellings — API-KEY, APIKEY, Private-Key — cannot slip a
+// secret past the filter; environment variable names legally contain hyphens
+// or no separator at all.
 func isSensitiveEnvVar(key string) bool {
-	upper := strings.ToUpper(key)
+	norm := strings.NewReplacer("-", "", "_", "").Replace(strings.ToUpper(key))
 	for _, pat := range []string{
-		"API_KEY", "TOKEN", "SECRET", "PASSWORD", "CREDENTIAL", "CREDS",
-		"PRIVATE_KEY", "ACCESS_KEY",
+		"APIKEY", "TOKEN", "SECRET", "PASSWORD", "CREDENTIAL", "CREDS",
+		"PRIVATEKEY", "ACCESSKEY",
 	} {
-		if strings.Contains(upper, pat) {
+		if strings.Contains(norm, pat) {
 			return true
 		}
 	}

internal/mcpclient/client_test.go

@@ -417,22 +417,50 @@ func TestBuildEnv_OverridesCannotInjectSecrets(t *testing.T) {
 	defer func() { osEnviron = orig }()
 
 	result := buildEnv(map[string]string{
-		"LEGIT_VAR":   "ok",
+		"LEGIT_VAR":    "ok",
 		"EVIL_API_KEY": "sk-stolen",
 		"BOT_TOKEN":    "tok-stolen",
+		// Non-underscore spellings: env var names legally contain hyphens or
+		// no separator, and must be caught by the normalised matching.
+		"API-KEY":     "sk-stolen",
+		"APIKEY":      "sk-stolen",
+		"PRIVATE-KEY": "pem-stolen",
+		"SECRET-KEY":  "shh",
 	})
 	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"} {
+	for _, k := range []string{"EVIL_API_KEY", "BOT_TOKEN", "API-KEY", "APIKEY", "PRIVATE-KEY", "SECRET-KEY"} {
 		if _, ok := m[k]; ok {
 			t.Errorf("sensitive override %q should be dropped", k)
 		}
 	}
 }
 
+func TestIsSensitiveEnvVar_NormalizesSeparators(t *testing.T) {
+	blocked := []string{
+		"MY_API_KEY", "API-KEY", "APIKEY", "Api-Key",
+		"PRIVATE-KEY", "PRIVATEKEY", "ACCESS-KEY", "ACCESSKEY",
+		"SECRET-KEY", "SECRETKEY", "AUTH-TOKEN", "AUTHTOKEN",
+		"DB-PASSWORD", "PASSWORD", "AWS-CREDS", "CREDENTIALS",
+	}
+	for _, k := range blocked {
+		if !isSensitiveEnvVar(k) {
+			t.Errorf("isSensitiveEnvVar(%q) = false, want true", k)
+		}
+	}
+	allowed := []string{
+		"PATH", "HOME", "EDITOR", "NODE_ENV", "MCP_SERVER_NAME", "KEYSTONE",
+	}
+	for _, k := range allowed {
+		if isSensitiveEnvVar(k) {
+			t.Errorf("isSensitiveEnvVar(%q) = true, want false", k)
+		}
+	}
+}
+
 func envToMap(env []string) map[string]string {
 	m := make(map[string]string, len(env))
 	for _, e := range env {