diff --git a/docs/09-security.md b/docs/09-security.md index e9bfb4a4..1e7d65d0 100644 --- a/docs/09-security.md +++ b/docs/09-security.md @@ -77,7 +77,7 @@ 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 allowlisted runtime basenames, 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. +**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. diff --git a/internal/mcp/validation.go b/internal/mcp/validation.go index 050c64c7..6d8194b8 100644 --- a/internal/mcp/validation.go +++ b/internal/mcp/validation.go @@ -5,6 +5,7 @@ import ( "net/url" "os" "regexp" + "sort" "strings" "github.com/nextlevelbuilder/goclaw/internal/security" @@ -74,17 +75,15 @@ func ValidateCommand(cmd string) error { basename := commandBasename(cmd) - // Allow absolute paths to known commands - if strings.HasPrefix(cmd, "/") || strings.ContainsAny(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[basename] { - return fmt.Errorf("command %q not in allowlist (allowed: node, npx, python, python3, ruby, go, java, uvx, uv, pipx, deno, bun)", basename) + return fmt.Errorf("command %q not in allowlist (allowed: %s)", basename, allowedCommandNames()) } return nil } @@ -145,6 +144,15 @@ func commandBasename(command string) string { 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") { diff --git a/internal/mcp/validation_test.go b/internal/mcp/validation_test.go index 7ab6244a..c756f059 100644 --- a/internal/mcp/validation_test.go +++ b/internal/mcp/validation_test.go @@ -30,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 { @@ -52,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