Skip to content

[681] fix(config): validate AllowedHost fields at config load time #687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
decko wants to merge 1 commit into from
Closed

[681] fix(config): validate AllowedHost fields at config load time #687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions cmd/soda/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down
160 changes: 160 additions & 0 deletions cmd/soda/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
38 changes: 35 additions & 3 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down
Loading