mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 12:18:59 +00:00
fix: reject path-based MCP runtime commands
This commit is contained in:
1 parent
878c33e964
commit
f8875e46d6
3 files changed
+53
-9
No files matched your search
+1
-1
@@ -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.
|
||||
|
||||
|
||||
@@ -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") {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user