Skip to content

Commit d6895c6

Browse files
committed
security: require explicit approval for project-level MCP servers
- Track project-level MCP server names in ResolvedConfig.ProjectMCPServerNames. - Add approveMCPServers helper with interactive y/N prompt, ODEK_APPROVE_MCP=1 bypass, and persisted approvals in ~/.odek/mcp_approvals.json (0600). - Gate loadMCPTools on approval before spawning any MCP subprocess. - Update all call sites (run, repl, serve, subagent, mcp, schedule). - Add unit tests for approval logic and persistence keying. - Document ODEK_APPROVE_MCP and the approval flow in docs/MCP.md. Full suite: go test ./... -count=1 passes.
1 parent f5691cc commit d6895c6

11 files changed

Lines changed: 343 additions & 15 deletions

File tree

cmd/odek/main.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,7 @@ func run(args []string) error {
867867
// MCP server tools
868868
var mcpCleanup func()
869869
if len(resolved.MCPServers) > 0 {
870-
cl, err := loadMCPTools(resolved.MCPServers, &tools)
870+
cl, err := loadMCPTools(resolved, &tools)
871871
if err != nil {
872872
return fmt.Errorf("mcp: %w", err)
873873
}
@@ -1214,9 +1214,18 @@ func builtinTools(dc danger.DangerousConfig, sm *skills.SkillManager, approver d
12141214
// loadMCPTools connects to configured MCP servers and appends their tools
12151215
// to the tool slice. Returns a cleanup function that closes all connections.
12161216
// The passed-in tool slice pointer is extended with ToolAdapters.
1217-
func loadMCPTools(servers map[string]mcpclient.ServerConfig, tools *[]odek.Tool) (func(), error) {
1217+
//
1218+
// Before spawning any server that was defined in the project-level ./odek.json,
1219+
// loadMCPTools calls approveMCPServers, which requires explicit user approval
1220+
// (interactive prompt or ODEK_APPROVE_MCP=1) and persists approvals in
1221+
// ~/.odek/mcp_approvals.json.
1222+
func loadMCPTools(resolved config.ResolvedConfig, tools *[]odek.Tool) (func(), error) {
1223+
if err := approveMCPServers(resolved, os.Stdin, os.Stdout); err != nil {
1224+
return nil, err
1225+
}
1226+
12181227
var cleaners []func()
1219-
for name, cfg := range servers {
1228+
for name, cfg := range resolved.MCPServers {
12201229
client, err := mcpclient.New(name, cfg)
12211230
if err != nil {
12221231
// Clean up any servers we already started
@@ -1710,7 +1719,7 @@ func continueCmd(args []string) error {
17101719
// MCP server tools
17111720
var mcpCleanup func()
17121721
if len(resolved.MCPServers) > 0 {
1713-
cl, err := loadMCPTools(resolved.MCPServers, &tools)
1722+
cl, err := loadMCPTools(resolved, &tools)
17141723
if err != nil {
17151724
return fmt.Errorf("mcp: %w", err)
17161725
}

cmd/odek/main_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1914,7 +1914,7 @@ func TestBuildSandboxArgs_RejectsHostNetwork(t *testing.T) {
19141914

19151915
func TestLoadMCPTools_EmptyServers(t *testing.T) {
19161916
tools := make([]odek.Tool, 0)
1917-
cleanup, err := loadMCPTools(nil, &tools)
1917+
cleanup, err := loadMCPTools(config.ResolvedConfig{}, &tools)
19181918
if err != nil {
19191919
t.Fatalf("loadMCPTools(nil) error: %v", err)
19201920
}
@@ -1925,7 +1925,7 @@ func TestLoadMCPTools_EmptyServers(t *testing.T) {
19251925
cleanup()
19261926

19271927
// Also test with empty map
1928-
cleanup2, err := loadMCPTools(map[string]mcpclient.ServerConfig{}, &tools)
1928+
cleanup2, err := loadMCPTools(config.ResolvedConfig{MCPServers: map[string]mcpclient.ServerConfig{}}, &tools)
19291929
if err != nil {
19301930
t.Fatalf("loadMCPTools(empty map) error: %v", err)
19311931
}

cmd/odek/mcp.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ Flags:
7979
// MCP server tools — connect and discover before sandbox
8080
var mcpCleanup func()
8181
if len(resolved.MCPServers) > 0 {
82-
cl, err := loadMCPTools(resolved.MCPServers, &toolSet)
82+
cl, err := loadMCPTools(resolved, &toolSet)
8383
if err != nil {
8484
return fmt.Errorf("mcp: %w", err)
8585
}

cmd/odek/mcp_approval.go

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
package main
2+
3+
import (
4+
"bufio"
5+
"crypto/sha256"
6+
"encoding/hex"
7+
"encoding/json"
8+
"fmt"
9+
"io"
10+
"os"
11+
"path/filepath"
12+
"sort"
13+
"strings"
14+
15+
"github.com/BackendStack21/odek/internal/config"
16+
"github.com/BackendStack21/odek/internal/mcpclient"
17+
"golang.org/x/term"
18+
)
19+
20+
// mcpApprovalsFile is the persistent store for user-approved project-level MCP
21+
// servers. It lives next to config.json under ~/.odek and is created 0600.
22+
const mcpApprovalsFile = "mcp_approvals.json"
23+
24+
// mcpApprovalEnv returns true if the user has opted in globally via the
25+
// ODEK_APPROVE_MCP environment variable.
26+
func mcpApprovalEnv() bool {
27+
return os.Getenv("ODEK_APPROVE_MCP") == "1"
28+
}
29+
30+
// approveMCPServers requires explicit user approval for any MCP servers that
31+
// were introduced by the project-level ./odek.json config. Global servers from
32+
// ~/.odek/config.json are considered operator-trusted and do not require
33+
// approval.
34+
//
35+
// Approval can be granted in three ways:
36+
// 1. Set ODEK_APPROVE_MCP=1 (useful for CI/non-interactive use).
37+
// 2. Answer the interactive y/N prompt when running on a TTY.
38+
// 3. A prior approval for the same project/server/command/args fingerprint is
39+
// persisted in ~/.odek/mcp_approvals.json.
40+
//
41+
// If approval is required and cannot be obtained, approveMCPServers returns an
42+
// error and the command should abort before spawning any MCP subprocess.
43+
func approveMCPServers(resolved config.ResolvedConfig, stdin io.Reader, stdout io.Writer) error {
44+
isTTY := stdin == os.Stdin && term.IsTerminal(int(os.Stdin.Fd()))
45+
return approveMCPServersWithTTY(resolved, stdin, stdout, isTTY)
46+
}
47+
48+
// approveMCPServersWithTTY is the testable core of approveMCPServers. The tty
49+
// argument tells the function whether it may prompt interactively.
50+
func approveMCPServersWithTTY(resolved config.ResolvedConfig, stdin io.Reader, stdout io.Writer, tty bool) error {
51+
if len(resolved.ProjectMCPServerNames) == 0 {
52+
return nil
53+
}
54+
55+
if mcpApprovalEnv() {
56+
return nil
57+
}
58+
59+
projectDir, err := os.Getwd()
60+
if err != nil {
61+
return fmt.Errorf("mcp approval: get working directory: %w", err)
62+
}
63+
projectDir, err = filepath.Abs(projectDir)
64+
if err != nil {
65+
return fmt.Errorf("mcp approval: abs working directory: %w", err)
66+
}
67+
68+
approved, err := loadMCPApprovals()
69+
if err != nil {
70+
return fmt.Errorf("mcp approval: load approvals: %w", err)
71+
}
72+
73+
reader := bufio.NewReader(stdin)
74+
75+
for _, name := range resolved.ProjectMCPServerNames {
76+
cfg, ok := resolved.MCPServers[name]
77+
if !ok {
78+
continue
79+
}
80+
81+
key := mcpApprovalKey(projectDir, name, cfg)
82+
if approved[key] {
83+
continue
84+
}
85+
86+
if !tty {
87+
return fmt.Errorf(
88+
"project-level MCP server %q (%s %q) requires explicit approval\n"+
89+
"set ODEK_APPROVE_MCP=1 to approve all project MCP servers, or run interactively",
90+
name, cfg.Command, strings.Join(cfg.Args, " "),
91+
)
92+
}
93+
94+
fmt.Fprintf(stdout, "\nProject-level MCP server %q wants to run:\n", name)
95+
fmt.Fprintf(stdout, " command: %s\n", cfg.Command)
96+
if len(cfg.Args) > 0 {
97+
fmt.Fprintf(stdout, " args: %s\n", strings.Join(cfg.Args, " "))
98+
}
99+
if len(cfg.Env) > 0 {
100+
envKeys := make([]string, 0, len(cfg.Env))
101+
for k := range cfg.Env {
102+
envKeys = append(envKeys, k)
103+
}
104+
sort.Strings(envKeys)
105+
fmt.Fprintf(stdout, " env: %s\n", strings.Join(envKeys, ", "))
106+
}
107+
fmt.Fprintf(stdout, "Approve? [y/N] ")
108+
109+
line, err := reader.ReadString('\n')
110+
if err != nil {
111+
return fmt.Errorf("mcp approval: read prompt: %w", err)
112+
}
113+
line = strings.ToLower(strings.TrimSpace(line))
114+
if line != "y" && line != "yes" {
115+
return fmt.Errorf("mcp approval: server %q was not approved", name)
116+
}
117+
118+
approved[key] = true
119+
if err := saveMCPApprovals(approved); err != nil {
120+
return fmt.Errorf("mcp approval: save approvals: %w", err)
121+
}
122+
}
123+
124+
return nil
125+
}
126+
127+
// mcpApprovalKey returns a stable key for the persisted approval store. It
128+
// includes the project directory, server name, command, and arguments so a
129+
// change to any of those invalidates the prior approval.
130+
func mcpApprovalKey(projectDir, name string, cfg mcpclient.ServerConfig) string {
131+
h := sha256.New()
132+
fmt.Fprintf(h, "%s\x00%s\x00%s", projectDir, name, cfg.Command)
133+
for _, a := range cfg.Args {
134+
fmt.Fprintf(h, "\x00%s", a)
135+
}
136+
return hex.EncodeToString(h.Sum(nil))
137+
}
138+
139+
// loadMCPApprovals reads the persisted approval map. A missing file is treated
140+
// as an empty approval set.
141+
func loadMCPApprovals() (map[string]bool, error) {
142+
path := filepath.Join(expandHome("~/.odek"), mcpApprovalsFile)
143+
data, err := os.ReadFile(path)
144+
if err != nil {
145+
if os.IsNotExist(err) {
146+
return make(map[string]bool), nil
147+
}
148+
return nil, err
149+
}
150+
151+
var approvals map[string]bool
152+
if err := json.Unmarshal(data, &approvals); err != nil {
153+
return nil, fmt.Errorf("parse %s: %w", path, err)
154+
}
155+
if approvals == nil {
156+
approvals = make(map[string]bool)
157+
}
158+
return approvals, nil
159+
}
160+
161+
// saveMCPApprovals writes the approval map to disk with 0600 permissions.
162+
func saveMCPApprovals(approvals map[string]bool) error {
163+
dir := expandHome("~/.odek")
164+
if err := os.MkdirAll(dir, 0700); err != nil {
165+
return err
166+
}
167+
path := filepath.Join(dir, mcpApprovalsFile)
168+
data, err := json.MarshalIndent(approvals, "", " ")
169+
if err != nil {
170+
return err
171+
}
172+
return os.WriteFile(path, data, 0600)
173+
}

cmd/odek/mcp_approval_test.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
package main
2+
3+
import (
4+
"bytes"
5+
"os"
6+
"strings"
7+
"testing"
8+
9+
"github.com/BackendStack21/odek/internal/config"
10+
"github.com/BackendStack21/odek/internal/mcpclient"
11+
)
12+
13+
func TestApproveMCPServers_NoProjectServers(t *testing.T) {
14+
resolved := config.ResolvedConfig{
15+
MCPServers: map[string]mcpclient.ServerConfig{
16+
"global": {Command: "node", Args: []string{"global.js"}},
17+
},
18+
}
19+
if err := approveMCPServersWithTTY(resolved, strings.NewReader(""), &bytes.Buffer{}, false); err != nil {
20+
t.Fatalf("expected no approval needed for global servers, got: %v", err)
21+
}
22+
}
23+
24+
func TestApproveMCPServers_ProjectServerRequiresApproval(t *testing.T) {
25+
resolved := config.ResolvedConfig{
26+
MCPServers: map[string]mcpclient.ServerConfig{
27+
"project": {Command: "sh", Args: []string{"-c", "echo pwned"}},
28+
},
29+
ProjectMCPServerNames: []string{"project"},
30+
}
31+
32+
var out bytes.Buffer
33+
err := approveMCPServersWithTTY(resolved, strings.NewReader("\n"), &out, true)
34+
if err == nil {
35+
t.Fatal("expected error when user denies approval, got nil")
36+
}
37+
if !strings.Contains(err.Error(), "was not approved") {
38+
t.Errorf("error = %q, want 'was not approved'", err)
39+
}
40+
if !strings.Contains(out.String(), "Project-level MCP server") {
41+
t.Errorf("prompt = %q, want project-level prompt", out.String())
42+
}
43+
}
44+
45+
func TestApproveMCPServers_ApprovalViaTTY(t *testing.T) {
46+
resolved := config.ResolvedConfig{
47+
MCPServers: map[string]mcpclient.ServerConfig{
48+
"project": {Command: "node", Args: []string{"server.js"}},
49+
},
50+
ProjectMCPServerNames: []string{"project"},
51+
}
52+
53+
var out bytes.Buffer
54+
err := approveMCPServersWithTTY(resolved, strings.NewReader("yes\n"), &out, true)
55+
if err != nil {
56+
t.Fatalf("expected approval, got: %v", err)
57+
}
58+
}
59+
60+
func TestApproveMCPServers_ApprovalViaEnv(t *testing.T) {
61+
resolved := config.ResolvedConfig{
62+
MCPServers: map[string]mcpclient.ServerConfig{
63+
"project": {Command: "sh", Args: []string{"-c", "echo pwned"}},
64+
},
65+
ProjectMCPServerNames: []string{"project"},
66+
}
67+
68+
t.Setenv("ODEK_APPROVE_MCP", "1")
69+
if err := approveMCPServersWithTTY(resolved, strings.NewReader(""), &bytes.Buffer{}, false); err != nil {
70+
t.Fatalf("expected env approval, got: %v", err)
71+
}
72+
}
73+
74+
func TestApproveMCPServers_NonTTYRequiresEnv(t *testing.T) {
75+
resolved := config.ResolvedConfig{
76+
MCPServers: map[string]mcpclient.ServerConfig{
77+
"project": {Command: "sh", Args: []string{"-c", "echo pwned"}},
78+
},
79+
ProjectMCPServerNames: []string{"project"},
80+
}
81+
82+
// Ensure env is not set.
83+
os.Unsetenv("ODEK_APPROVE_MCP")
84+
err := approveMCPServersWithTTY(resolved, strings.NewReader(""), &bytes.Buffer{}, false)
85+
if err == nil {
86+
t.Fatal("expected error for non-interactive unapproved project server")
87+
}
88+
if !strings.Contains(err.Error(), "ODEK_APPROVE_MCP") {
89+
t.Errorf("error = %q, want ODEK_APPROVE_MCP hint", err)
90+
}
91+
}
92+
93+
func TestMCPApprovalKey_Stability(t *testing.T) {
94+
cfg := mcpclient.ServerConfig{Command: "node", Args: []string{"a.js", "b.js"}, Env: map[string]string{"X": "1"}}
95+
k1 := mcpApprovalKey("/proj", "srv", cfg)
96+
k2 := mcpApprovalKey("/proj", "srv", cfg)
97+
if k1 != k2 {
98+
t.Fatalf("approval key not stable: %q vs %q", k1, k2)
99+
}
100+
101+
cfg2 := mcpclient.ServerConfig{Command: "node", Args: []string{"a.js", "c.js"}, Env: map[string]string{"X": "1"}}
102+
k3 := mcpApprovalKey("/proj", "srv", cfg2)
103+
if k1 == k3 {
104+
t.Fatal("approval key did not change when args changed")
105+
}
106+
}

cmd/odek/repl.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ func replCmd(args []string) error {
8484
// MCP server tools
8585
var mcpCleanup func()
8686
if len(resolved.MCPServers) > 0 {
87-
cl, err := loadMCPTools(resolved.MCPServers, &tools)
87+
cl, err := loadMCPTools(resolved, &tools)
8888
if err != nil {
8989
return fmt.Errorf("mcp: %w", err)
9090
}

cmd/odek/schedule.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,7 @@ func buildScheduledMCPTools(resolved config.ResolvedConfig) ([]odek.Tool, func()
708708
return nil, func() {}, nil
709709
}
710710
var tools []odek.Tool
711-
cleanup, err := loadMCPTools(resolved.MCPServers, &tools)
711+
cleanup, err := loadMCPTools(resolved, &tools)
712712
if err != nil {
713713
return nil, func() {}, fmt.Errorf("mcp: %w", err)
714714
}

cmd/odek/serve.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ func newServeAgent(resolved config.ResolvedConfig, system string, sendFn func(v
307307
// MCP server tools
308308
var mcpCleanup func()
309309
if len(resolved.MCPServers) > 0 {
310-
cl, err := loadMCPTools(resolved.MCPServers, &tools)
310+
cl, err := loadMCPTools(resolved, &tools)
311311
if err != nil {
312312
return nil, nil, nil, nil, fmt.Errorf("mcp: %w", err)
313313
}

cmd/odek/subagent.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ func subagentCmd(args []string) error {
305305
// MCP server tools
306306
var mcpCleanup func()
307307
if len(resolved.MCPServers) > 0 {
308-
cl, err := loadMCPTools(resolved.MCPServers, &tools)
308+
cl, err := loadMCPTools(resolved, &tools)
309309
if err != nil {
310310
return fmt.Errorf("mcp: %w", err)
311311
}

0 commit comments

Comments
 (0)