diff --git a/cmd/soda/validate.go b/cmd/soda/validate.go index af2be68..83fc41e 100644 --- a/cmd/soda/validate.go +++ b/cmd/soda/validate.go @@ -634,16 +634,16 @@ func validateTranscript(w io.Writer, result *validationResult, cfg *config.Confi } // validateMCP checks that each declared MCP server's command binary exists in -// PATH and warns when a phase references an undeclared MCP server. -// The lookPath parameter is injected for testability, following the same -// pattern as validateRunner. +// PATH, validates AllowedHost fields, and warns when a phase references an +// undeclared MCP server. The lookPath parameter is injected for testability, +// following the same pattern as validateRunner. func validateMCP(w io.Writer, result *validationResult, cfg *config.Config, pl *pipeline.PhasePipeline, lookPath func(string) (string, error)) { if len(cfg.MCP.Servers) == 0 { fmt.Fprintln(w, "✓ mcp: no servers configured") return } - // Check each declared server's binary. + // Check each declared server's binary and AllowedHosts. for name, server := range cfg.MCP.Servers { _, err := lookPath(server.Command) if err != nil { @@ -652,6 +652,20 @@ func validateMCP(w io.Writer, result *validationResult, cfg *config.Config, pl * } else { fmt.Fprintf(w, " ✓ mcp server %s: %s\n", name, server.Command) } + + // Validate AllowedHost entries. These are hard errors because + // go-arapuca rejects port 0 at the FFI level and an empty/scheme- + // prefixed host may produce silently broken proxy rules. + for idx, ah := range server.AllowedHosts { + if ah.Host == "" { + result.addError("mcp: server %q: allowed_hosts[%d]: host must not be empty", name, idx) + } else if strings.Contains(ah.Host, "://") { + result.addError("mcp: server %q: allowed_hosts[%d]: host %q must not include a scheme prefix", name, idx, ah.Host) + } + if ah.Port < 1 || ah.Port > 65535 { + result.addError("mcp: server %q: allowed_hosts[%d]: port %d is out of range (must be 1-65535)", name, idx, ah.Port) + } + } } // Warn about phases referencing undeclared servers. diff --git a/cmd/soda/validate_test.go b/cmd/soda/validate_test.go index e8d6f6a..adc8889 100644 --- a/cmd/soda/validate_test.go +++ b/cmd/soda/validate_test.go @@ -1490,6 +1490,166 @@ func TestRunValidate_WithMCPServers(t *testing.T) { } } +// TestValidateMCP_AllowedHosts tests that validateMCP validates AllowedHost +// fields on declared MCP servers. +func TestValidateMCP_AllowedHosts_Valid(t *testing.T) { + cfg := &config.Config{ + MCP: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "jira": { + Command: "wtmcp", + AllowedHosts: []config.AllowedHost{ + {Host: "jira.example.com", Port: 443}, + }, + }, + }, + }, + } + result := &validationResult{} + var buf bytes.Buffer + lookPath := stubLookPath(map[string]string{"wtmcp": "/usr/bin/wtmcp"}) + validateMCP(&buf, result, cfg, nil, lookPath) + + if result.hasErrors() { + t.Errorf("expected no errors for valid AllowedHosts, got: %v", result.errors) + } + if len(result.warnings) != 0 { + t.Errorf("expected no warnings, got: %v", result.warnings) + } +} + +func TestValidateMCP_AllowedHosts_EmptyHost(t *testing.T) { + cfg := &config.Config{ + MCP: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "jira": { + Command: "wtmcp", + AllowedHosts: []config.AllowedHost{ + {Host: "", Port: 443}, + }, + }, + }, + }, + } + result := &validationResult{} + var buf bytes.Buffer + lookPath := stubLookPath(map[string]string{"wtmcp": "/usr/bin/wtmcp"}) + validateMCP(&buf, result, cfg, nil, lookPath) + + if !result.hasErrors() { + t.Error("expected errors for empty AllowedHost.Host") + } + found := false + for _, errMsg := range result.errors { + if strings.Contains(errMsg, "host must not be empty") { + found = true + break + } + } + if !found { + t.Errorf("expected error about empty host, got: %v", result.errors) + } +} + +func TestValidateMCP_AllowedHosts_SchemePrefix(t *testing.T) { + cfg := &config.Config{ + MCP: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "jira": { + Command: "wtmcp", + AllowedHosts: []config.AllowedHost{ + {Host: "https://jira.example.com", Port: 443}, + }, + }, + }, + }, + } + result := &validationResult{} + var buf bytes.Buffer + lookPath := stubLookPath(map[string]string{"wtmcp": "/usr/bin/wtmcp"}) + validateMCP(&buf, result, cfg, nil, lookPath) + + if !result.hasErrors() { + t.Error("expected errors for AllowedHost.Host with scheme prefix") + } + found := false + for _, errMsg := range result.errors { + if strings.Contains(errMsg, "scheme prefix") { + found = true + break + } + } + if !found { + t.Errorf("expected error about scheme prefix, got: %v", result.errors) + } +} + +func TestValidateMCP_AllowedHosts_PortZero(t *testing.T) { + cfg := &config.Config{ + MCP: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "jira": { + Command: "wtmcp", + AllowedHosts: []config.AllowedHost{ + {Host: "jira.example.com", Port: 0}, + }, + }, + }, + }, + } + result := &validationResult{} + var buf bytes.Buffer + lookPath := stubLookPath(map[string]string{"wtmcp": "/usr/bin/wtmcp"}) + validateMCP(&buf, result, cfg, nil, lookPath) + + if !result.hasErrors() { + t.Error("expected errors for AllowedHost.Port = 0") + } + found := false + for _, errMsg := range result.errors { + if strings.Contains(errMsg, "1-65535") { + found = true + break + } + } + if !found { + t.Errorf("expected error about port range, got: %v", result.errors) + } +} + +func TestValidateMCP_AllowedHosts_PortTooLarge(t *testing.T) { + cfg := &config.Config{ + MCP: config.MCPConfig{ + Servers: map[string]config.MCPServerConfig{ + "jira": { + Command: "wtmcp", + AllowedHosts: []config.AllowedHost{ + {Host: "jira.example.com", Port: 65536}, + }, + }, + }, + }, + } + result := &validationResult{} + var buf bytes.Buffer + lookPath := stubLookPath(map[string]string{"wtmcp": "/usr/bin/wtmcp"}) + validateMCP(&buf, result, cfg, nil, lookPath) + + if !result.hasErrors() { + t.Error("expected errors for AllowedHost.Port = 65536") + } + found := false + for _, errMsg := range result.errors { + if strings.Contains(errMsg, "1-65535") { + found = true + break + } + } + if !found { + t.Errorf("expected error about port range, got: %v", result.errors) + } +} + func TestValidateMCP_NilPipeline(t *testing.T) { cfg := &config.Config{ MCP: config.MCPConfig{ diff --git a/internal/config/config.go b/internal/config/config.go index d579cd7..40ade55 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -56,11 +56,26 @@ type OpencodeConfig struct { Model string `yaml:"model,omitempty"` // model override for Opencode; empty = use top-level Model } +// AllowedHost specifies an external host and port that an MCP server process +// is permitted to connect to. go-arapuca uses this information to set up +// network allow-list rules in the sandbox. An empty Host or a Port outside +// the 1-65535 range is rejected at config.Load time rather than at sandbox +// launch, where the error would surface deep in the FFI with no clear indication +// of the root cause. +type AllowedHost struct { + // Host is the hostname or IP address. Must not include a scheme prefix + // (e.g., "https://"). Must not be empty. + Host string `yaml:"host"` + // Port is the TCP port number. Must be in the range 1-65535. + Port int `yaml:"port"` +} + // MCPServerConfig holds the definition of a single MCP server process. type MCPServerConfig struct { - Command string `yaml:"command"` - Args []string `yaml:"args,omitempty"` - Env map[string]string `yaml:"env,omitempty"` + Command string `yaml:"command"` + Args []string `yaml:"args,omitempty"` + Env map[string]string `yaml:"env,omitempty"` + AllowedHosts []AllowedHost `yaml:"allowed_hosts,omitempty"` } // MCPConfig holds MCP server declarations available to pipeline phases. @@ -321,6 +336,23 @@ func Load(path string) (*Config, error) { maxConventionChecklistBytes, len(cfg.ConventionChecklist)) } + // Validate AllowedHost fields on each MCP server. go-arapuca rejects + // port 0 at the FFI level and an empty host may produce broken proxy rules, + // so we catch these early with a clear diagnostic. + for serverName, server := range cfg.MCP.Servers { + for idx, ah := range server.AllowedHosts { + if ah.Host == "" { + return nil, fmt.Errorf("config: mcp.servers[%s].allowed_hosts[%d]: host must not be empty", serverName, idx) + } + if strings.Contains(ah.Host, "://") { + return nil, fmt.Errorf("config: mcp.servers[%s].allowed_hosts[%d]: host %q must not include a scheme prefix (remove the scheme and keep only the hostname)", serverName, idx, ah.Host) + } + if ah.Port < 1 || ah.Port > 65535 { + return nil, fmt.Errorf("config: mcp.servers[%s].allowed_hosts[%d]: port %d is out of range (must be 1-65535)", serverName, idx, ah.Port) + } + } + } + return &cfg, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index b20bb59..3851ff5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "errors" + "fmt" "os" "path/filepath" "reflect" @@ -508,6 +509,174 @@ func TestLoad_ConventionChecklist_OverLimit(t *testing.T) { } } +// TestLoad_AllowedHosts validates that AllowedHost entries on MCP servers are +// checked at config.Load time. Invalid entries (empty host, scheme prefix, or +// out-of-range port) must cause Load to return an error. +func TestLoad_AllowedHosts_Valid(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := `mcp: + servers: + jira: + command: wtmcp + args: [jira] + allowed_hosts: + - host: jira.example.com + port: 443 + - host: 192.168.1.1 + port: 8080 +` + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(cfgFile) + if err != nil { + t.Fatalf("valid AllowedHosts should be accepted, got error: %v", err) + } + + jira, ok := cfg.MCP.Servers["jira"] + if !ok { + t.Fatal("MCP.Servers[jira] not found") + } + if len(jira.AllowedHosts) != 2 { + t.Fatalf("expected 2 AllowedHosts, got %d", len(jira.AllowedHosts)) + } + if jira.AllowedHosts[0].Host != "jira.example.com" { + t.Errorf("AllowedHosts[0].Host = %q, want %q", jira.AllowedHosts[0].Host, "jira.example.com") + } + if jira.AllowedHosts[0].Port != 443 { + t.Errorf("AllowedHosts[0].Port = %d, want 443", jira.AllowedHosts[0].Port) + } +} + +func TestLoad_AllowedHosts_EmptyHost(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := `mcp: + servers: + jira: + command: wtmcp + allowed_hosts: + - host: "" + port: 443 +` + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + _, err := Load(cfgFile) + if err == nil { + t.Fatal("empty host should be rejected") + } + if !strings.Contains(err.Error(), "host must not be empty") { + t.Errorf("error should mention empty host, got: %v", err) + } + if !strings.Contains(err.Error(), "jira") { + t.Errorf("error should mention server name 'jira', got: %v", err) + } +} + +func TestLoad_AllowedHosts_SchemePrefix(t *testing.T) { + tests := []struct { + name string + host string + }{ + {"https scheme", "https://jira.example.com"}, + {"http scheme", "http://api.example.com"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := "mcp:\n servers:\n jira:\n command: wtmcp\n allowed_hosts:\n - host: " + tt.host + "\n port: 443\n" + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + _, err := Load(cfgFile) + if err == nil { + t.Fatalf("host %q with scheme prefix should be rejected", tt.host) + } + if !strings.Contains(err.Error(), "scheme prefix") { + t.Errorf("error should mention scheme prefix, got: %v", err) + } + }) + } +} + +func TestLoad_AllowedHosts_InvalidPort(t *testing.T) { + tests := []struct { + name string + port string + want string + }{ + {"port zero", "0", "1-65535"}, + {"port too large", "65536", "1-65535"}, + {"negative port", "-1", "1-65535"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := "mcp:\n servers:\n jira:\n command: wtmcp\n allowed_hosts:\n - host: jira.example.com\n port: " + tt.port + "\n" + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + _, err := Load(cfgFile) + if err == nil { + t.Fatalf("port %s should be rejected", tt.port) + } + if !strings.Contains(err.Error(), tt.want) { + t.Errorf("error should mention %q, got: %v", tt.want, err) + } + }) + } +} + +func TestLoad_AllowedHosts_PortBoundaryValid(t *testing.T) { + for _, port := range []int{1, 65535} { + t.Run(fmt.Sprintf("port_%d", port), func(t *testing.T) { + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := fmt.Sprintf("mcp:\n servers:\n jira:\n command: wtmcp\n allowed_hosts:\n - host: jira.example.com\n port: %d\n", port) + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + _, err := Load(cfgFile) + if err != nil { + t.Fatalf("port %d should be accepted, got: %v", port, err) + } + }) + } +} + +func TestLoad_AllowedHosts_NoHostsConfigured(t *testing.T) { + // A server without AllowedHosts should be accepted. + dir := t.TempDir() + cfgFile := filepath.Join(dir, "soda.yaml") + content := `mcp: + servers: + jira: + command: wtmcp + args: [jira] +` + if err := os.WriteFile(cfgFile, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(cfgFile) + if err != nil { + t.Fatalf("server without AllowedHosts should be accepted, got: %v", err) + } + jira := cfg.MCP.Servers["jira"] + if len(jira.AllowedHosts) != 0 { + t.Errorf("expected no AllowedHosts, got %d", len(jira.AllowedHosts)) + } +} + func TestRepoConfigFieldParity(t *testing.T) { configType := reflect.TypeOf(RepoConfig{}) pipelineType := reflect.TypeOf(pipeline.RepoConfig{})