fix: harden MCP approval env hashing and skill tool provenance gates (H6, H7)

AGENTS.md

@@ -109,7 +109,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **Untrusted-content wrapper** (`cmd/odek/untrusted.go`) — every tool whose output sources from outside the trust boundary (`browser`, `read_file`, `shell`, `search_files`, `multi_grep`, `transcribe`, `head_tail`, `diff`, `tr`, `sort`, `json_query`, `batch_patch`, `glob`, `file_info`, `tree`, `base64` file mode, `session_search`, `@-resources`, `--ctx` files, any MCP tool) wraps results in `<untrusted_content_<nonce> source="...">…</untrusted_content_<nonce>>`. Browser page title and interactive-element text are wrapped in addition to the main content. Per-call nonce defeats wrapper-escape via literal close tag.
 - **Audit log** (`cmd/odek/audit.go` + `internal/session/audit.go`) — every `wrapUntrusted` call records source + content-hash + turn into `<sessions>/audit/<id>.json`. After each turn a divergence heuristic flags `suspicious_divergence=true` when the agent ingested untrusted content AND its actions or final response reference resources that either did not appear in the user's message or were introduced by the untrusted content itself (closing response-only exfiltration and reused-resource injection bypasses). Inspect with `odek audit <session-id>` / `odek audit --list`.
 - **Memory taint** (`internal/memory/provenance.go`) — `EpisodeProvenance` tracks Untrusted/Sources/UserApproved. Tainted episodes are stored but `Search()` filters them out, so a one-shot injection cannot persist via the episode pipeline. User must explicitly promote.
-- **Skill provenance gate** (`internal/skills/loader.go` + `cache.go`) — `Skill.Provenance{Untrusted, Sources, NeedsReview}`. NeedsReview skills pin to Lazy regardless of `auto_load`. The auto-save path declines tainted suggestions by default, and `odek skill promote <name> --force` clears the flag after explicit user review.
+- **Skill provenance gate** (`internal/skills/loader.go` + `cache.go` + `tools.go`) — `Skill.Provenance{Untrusted, Sources, NeedsReview}`. NeedsReview skills pin to Lazy regardless of `auto_load`. The auto-save path declines tainted suggestions by default. Agent-created skills via `skill_save` and patched skills via `skill_patch` are forced to `Untrusted` + `NeedsReview`, and `skill_patch` refuses edits that touch the YAML frontmatter, blocking an injected agent from flipping `auto_load` or clearing `needs_review`. `odek skill promote <name> --force` clears the flag after explicit user review.
 - **Sub-agent damage cap** (`cmd/odek/subagent.go::applySubagentTrust`) — `delegate_tasks` carries `trust_level` + `max_risk`. Untrusted ⇒ NonInteractive=deny, Destructive/CodeExec/Install/SystemWrite/NetworkEgress all forced to Deny. `max_risk` ⇒ everything above cap forced to Deny.
 - **FD-based API key handoff** (`cmd/odek/subagent_key.go`) — parent writes key to a 0600 tempfile, immediately `unlink()`s, passes the FD via `cmd.ExtraFiles`. Sub-agent reads from `$ODEK_API_KEY_FD` and closes. Key never in `/proc/<pid>/environ`.
 - **Approver friction** (`internal/danger/approver.go`, `cmd/odek/wsapprover.go`) — both TTYApprover and WSApprover engage friction mode after 3 approvals of the same class in 60s: require typing literal `approve`, 1.5s pause. Trust-class shortcut disabled for `destructive` + `blocked` regardless.
@@ -152,7 +152,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
 - **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). 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 `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.
+- **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.
 - **Schedule atomic-write hardening** (`internal/schedule/store.go` + `internal/fsatomic`) — schedule file writes now use `fsatomic.WriteFile`, which creates a random temp file with `O_EXCL`, fsyncs data and directory, and renames over the target. A swapped-in symlink is replaced rather than followed, closing the symlink-override attack on `schedules.json` / `schedule-state.json`.

cmd/odek/mcp_approval.go

@@ -6,6 +6,7 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
+	"hash"
 	"io"
 	"os"
 	"path/filepath"
@@ -33,10 +34,10 @@ func mcpApprovalEnv() bool {
 // approval.
 //
 // Approval can be granted in three ways:
-//   1. Set ODEK_APPROVE_MCP=1 (useful for CI/non-interactive use).
-//   2. Answer the interactive y/N prompt when running on a TTY.
-//   3. A prior approval for the same project/server/command/args fingerprint is
-//      persisted in ~/.odek/mcp_approvals.json.
+//  1. Set ODEK_APPROVE_MCP=1 (useful for CI/non-interactive use).
+//  2. Answer the interactive y/N prompt when running on a TTY.
+//  3. A prior approval for the same project/server/command/args fingerprint is
+//     persisted in ~/.odek/mcp_approvals.json.
 //
 // If approval is required and cannot be obtained, approveMCPServers returns an
 // error and the command should abort before spawning any MCP subprocess.
@@ -97,12 +98,11 @@ func approveMCPServersWithTTY(resolved config.ResolvedConfig, stdin io.Reader, s
 			fmt.Fprintf(stdout, "  args:    %s\n", strings.Join(cfg.Args, " "))
 		}
 		if len(cfg.Env) > 0 {
-			envKeys := make([]string, 0, len(cfg.Env))
-			for k := range cfg.Env {
-				envKeys = append(envKeys, k)
+			envKeys := sortedEnvKeys(cfg.Env)
+			fmt.Fprintf(stdout, "  env:\n")
+			for _, k := range envKeys {
+				fmt.Fprintf(stdout, "    %s=%s\n", k, cfg.Env[k])
 			}
-			sort.Strings(envKeys)
-			fmt.Fprintf(stdout, "  env:     %s\n", strings.Join(envKeys, ", "))
 		}
 		fmt.Fprintf(stdout, "Approve? [y/N] ")
 
@@ -125,14 +125,15 @@ func approveMCPServersWithTTY(resolved config.ResolvedConfig, stdin io.Reader, s
 }
 
 // mcpApprovalKey returns a stable key for the persisted approval store. It
-// includes the project directory, server name, command, and arguments so a
-// change to any of those invalidates the prior approval.
+// includes the project directory, server name, command, arguments, and env
+// overrides so a change to any of those invalidates the prior approval.
 func mcpApprovalKey(projectDir, name string, cfg mcpclient.ServerConfig) string {
 	h := sha256.New()
 	fmt.Fprintf(h, "%s\x00%s\x00%s", projectDir, name, cfg.Command)
 	for _, a := range cfg.Args {
 		fmt.Fprintf(h, "\x00%s", a)
 	}
+	hashEnv(h, cfg.Env)
 	return hex.EncodeToString(h.Sum(nil))
 }
 
@@ -252,6 +253,7 @@ func mcpToolApprovalKey(projectDir, serverName, toolName string, cfg mcpclient.S
 	for _, a := range cfg.Args {
 		fmt.Fprintf(h, "\x00%s", a)
 	}
+	hashEnv(h, cfg.Env)
 	return hex.EncodeToString(h.Sum(nil))
 }
 
@@ -290,6 +292,26 @@ func saveMCPToolApprovals(approvals map[string]bool) error {
 	return os.WriteFile(path, data, 0600)
 }
 
+// sortedEnvKeys returns the keys of an env map in deterministic order.
+func sortedEnvKeys(env map[string]string) []string {
+	keys := make([]string, 0, len(env))
+	for k := range env {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys
+}
+
+// hashEnv writes a canonical representation of the env map into h.
+// The order is deterministic and key/value pairs are separated by NUL bytes
+// so that distinct key/value boundaries cannot collide.
+func hashEnv(h hash.Hash, env map[string]string) {
+	keys := sortedEnvKeys(env)
+	for _, k := range keys {
+		fmt.Fprintf(h, "\x00env\x00%s\x00%s", k, env[k])
+	}
+}
+
 // truncateDescription limits a tool description for the approval prompt.
 func truncateDescription(desc string, max int) string {
 	if len(desc) <= max {

cmd/odek/mcp_approval_test.go

@@ -163,3 +163,58 @@ func TestMCPApprovalKey_Stability(t *testing.T) {
 		t.Fatal("approval key did not change when args changed")
 	}
 }
+
+func TestMCPApprovalKey_IncludesEnv(t *testing.T) {
+	cfg := mcpclient.ServerConfig{Command: "node", Args: []string{"a.js"}, Env: map[string]string{"X": "1"}}
+	k1 := mcpApprovalKey("/proj", "srv", cfg)
+
+	cfg.Env["X"] = "2"
+	k2 := mcpApprovalKey("/proj", "srv", cfg)
+	if k1 == k2 {
+		t.Fatal("approval key did not change when env value changed")
+	}
+
+	delete(cfg.Env, "X")
+	k3 := mcpApprovalKey("/proj", "srv", cfg)
+	if k1 == k3 || k2 == k3 {
+		t.Fatal("approval key did not change when env key removed")
+	}
+}
+
+func TestApproveMCPServers_PromptShowsEnvValues(t *testing.T) {
+	setupTestHome(t)
+	resolved := config.ResolvedConfig{
+		MCPServers: map[string]mcpclient.ServerConfig{
+			"project": {
+				Command: "node",
+				Args:    []string{"server.js"},
+				Env:     map[string]string{"NODE_OPTIONS": "--require ./pwn.js", "DEBUG": "1"},
+			},
+		},
+		ProjectMCPServerNames: []string{"project"},
+	}
+
+	var out bytes.Buffer
+	err := approveMCPServersWithTTY(resolved, strings.NewReader("yes\n"), &out, true)
+	if err != nil {
+		t.Fatalf("expected approval, got: %v", err)
+	}
+	prompt := out.String()
+	if !strings.Contains(prompt, "NODE_OPTIONS=--require ./pwn.js") {
+		t.Errorf("prompt did not show env value: %q", prompt)
+	}
+	if !strings.Contains(prompt, "DEBUG=1") {
+		t.Errorf("prompt did not show env value: %q", prompt)
+	}
+}
+
+func TestMCPToolApprovalKey_IncludesEnv(t *testing.T) {
+	cfg := mcpclient.ServerConfig{Command: "node", Args: []string{"a.js"}, Env: map[string]string{"X": "1"}}
+	k1 := mcpToolApprovalKey("/proj", "srv", "fetch", cfg)
+
+	cfg.Env["X"] = "2"
+	k2 := mcpToolApprovalKey("/proj", "srv", "fetch", cfg)
+	if k1 == k2 {
+		t.Fatal("tool approval key did not change when env value changed")
+	}
+}

docs/SECURITY.md

@@ -185,6 +185,8 @@ Promotion is **CLI-only and human-gated** — it is deliberately *not* exposed a
 
 `internal/skills` carries the same provenance model and shares the exact taint decision (`memory.ToolCallTaints`). Skills auto-saved from sessions that crossed the trust boundary — `browser` / `http_batch` / `transcribe` / `vision` / any MCP tool, or a `read_file` / `search_files` / `multi_grep` of a **sensitive** path — are tagged with `Provenance.Untrusted=true` and `NeedsReview=true`. The skill loader pins those skills to the Lazy set regardless of their `auto_load` flag.
 
+Skills created or edited through the agent-facing `skill_save` and `skill_patch` tools are also marked `Untrusted` with `NeedsReview=true`, and `skill_patch` refuses to edit the YAML frontmatter. This prevents an injected agent from silently creating an auto-loading skill or from patching `auto_load` / `needs_review` flags to bypass the promotion gate.
+
 The non-interactive auto-save path (`RunAutoSaveLoop`) now **declines to persist tainted suggestions by default**, so a prompt-injected turn cannot silently leave a poisoned skill on disk. Tainted suggestions are still surfaced in the interactive TUI and can be saved explicitly by the user after review.
 
 After reviewing the skill body, promote it with `--force`:
@@ -460,6 +462,8 @@ MCP servers supply both the names and descriptions of the tools they expose via
 
 4. **Description scanning** — tool descriptions are already scanned with `ScanInjection` at registration and withheld if injection patterns are found.
 
+5. **Env-aware approval fingerprint** — both server and tool approval keys hash the server's `env` map as sorted key/value pairs, so adding, removing, or changing an environment variable invalidates any previously persisted approval. The interactive approval prompt also prints each env variable with its value, so the operator can see code-execution vectors such as `NODE_OPTIONS=--require ./pwn.js` before approving.
+
 ### 29. Read-only perf-tool file-size cap
 
 The read-only perf tools `count_lines`, `checksum`, `head_tail`, and `word_count` previously opened a file and scanned or hashed the entire contents without checking its size. They now `Stat` the file and reject anything larger than `maxFileReadBytes` (10 MiB) with an error, matching the cap already enforced by `diff`, `base64`, `tr`, `sort`, `json_query`, and `batch_patch`. This prevents a prompt-injected call from pointing these tools at multi-gigabyte logs or core dumps and stalling or OOMing the turn.

internal/skills/tools.go

@@ -528,6 +528,14 @@ func (t *SkillSaveTool) Call(args string) (string, error) {
 		},
 		Body:     input.Body,
 		BodyHash: HashBody(input.Body),
+		// Agent-created skills always require explicit review before they can
+		// be auto-loaded. This closes the skill_save → skill_patch → auto_load
+		// persistence bypass documented in the security roadmap.
+		Provenance: SkillProvenance{
+			Untrusted:   true,
+			NeedsReview: true,
+			Sources:     []string{"skill_save"},
+		},
 	}
 
 	// Check for duplicate
@@ -642,23 +650,36 @@ func (t *SkillPatchTool) Call(args string) (string, error) {
 		return "", fmt.Errorf("skill_patch: text %q not found in skill %q", input.OldText, input.Name)
 	}
 
+	// Refuse any patch whose matched text lives inside the YAML frontmatter.
+	// Editing frontmatter would let an injected agent flip auto_load or
+	// needs_review without human approval.
+	if bodyOffset := skillBodyStart(body); bodyOffset > 0 && idx < bodyOffset {
+		return "", fmt.Errorf("skill_patch: old_text matches the YAML frontmatter / odek metadata section; editing frontmatter is not permitted")
+	}
+
 	newBody := strings.Replace(body, input.OldText, input.NewText, 1) // n=1: unique match enforced above
 	if err := os.WriteFile(skill.Source.Path, []byte(newBody), 0644); err != nil {
 		return "", fmt.Errorf("skill_patch: write: %w", err)
 	}
 
-	// Scan the patched body. If the guard flags it, pin the skill to manual
-	// review by rewriting its provenance.
+	// Re-parse and force the patched skill into a reviewed state. Even a
+	// body-only patch can be used to smuggle instructions, so agent-patched
+	// skills must never auto-load until explicitly promoted.
 	flagged := false
-	if patched := parseSkillContent(newBody, skill.Source.Path); patched != nil {
-		if t.Manager.scanSkill(context.Background(), patched) {
-			flagged = true
-			patched.Source = skill.Source
-			content := MarshalSkill(*patched)
-			if err := os.WriteFile(skill.Source.Path, []byte(content), 0644); err != nil {
-				return "", fmt.Errorf("skill_patch: rewrite flagged provenance: %w", err)
-			}
-		}
+	patched := parseSkillContent(newBody, skill.Source.Path)
+	if patched == nil {
+		return "", fmt.Errorf("skill_patch: patched file is no longer a valid SKILL.md")
+	}
+	patched.Source = skill.Source
+	patched.Provenance.Untrusted = true
+	patched.Provenance.NeedsReview = true
+	patched.Provenance.Sources = append(patched.Provenance.Sources, "skill_patch")
+	if t.Manager.scanSkill(context.Background(), patched) {
+		flagged = true
+	}
+	marshaled := MarshalSkill(*patched)
+	if err := os.WriteFile(skill.Source.Path, []byte(marshaled), 0644); err != nil {
+		return "", fmt.Errorf("skill_patch: rewrite provenance: %w", err)
 	}
 
 	t.Manager.MarkDirty()
@@ -746,6 +767,21 @@ func (t *SkillDeleteTool) Call(args string) (string, error) {
 	return fmt.Sprintf("✓ Deleted skill %q", input.Name), nil
 }
 
+// skillBodyStart returns the byte offset in a SKILL.md where the body begins
+// (immediately after the closing `---` frontmatter delimiter). If the file has
+// no recognizable frontmatter it returns -1.
+func skillBodyStart(content string) int {
+	content = strings.TrimSpace(content)
+	if !strings.HasPrefix(content, "---") {
+		return -1
+	}
+	closing := strings.Index(content, "\n---")
+	if closing < 0 {
+		return -1
+	}
+	return closing + len("\n---")
+}
+
 // findAnySkill searches for a skill in both auto-load and lazy lists.
 func findAnySkill(sm *SkillManager, name string) (*Skill, error) {
 	for i := range sm.Result.AutoLoad {