From 880f058cc69bc7e0904c21f7aadee75a2612c9a2 Mon Sep 17 00:00:00 2001 From: arya rizky Date: Tue, 19 May 2026 12:12:03 +0700 Subject: [PATCH] fix(mcp): use standalone flag matching to prevent false positives Previously ValidateArgs used strings.Contains for all dangerous patterns, causing legitimate package names like 'clickup-cli' to be rejected because they contain '-c' as a substring of '-cli'. Split dangerous patterns into: - dangerousFlagPatterns: matched as standalone flags only (exact or flag=value) - dangerousCodePatterns: matched by substring for code injection patterns Closes #1027 --- internal/mcp/validation.go | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/mcp/validation.go b/internal/mcp/validation.go index d5f5feec69..f17ab2b928 100644 --- a/internal/mcp/validation.go +++ b/internal/mcp/validation.go @@ -24,10 +24,15 @@ var allowedCommands = map[string]bool{ var shellMetaChars = regexp.MustCompile(`[;|&$` + "`" + `(){}[\]<>]`) // Dangerous arg flags that enable code execution. -var dangerousArgPatterns = []string{ +// These are matched as standalone flags only (exact match or flag=value form). +var dangerousFlagPatterns = []string{ "--eval", "-e", "-c", // Code execution flags "--require", "-r", // Module injection "--import", // ES module injection +} + +// Dangerous code patterns that should be caught anywhere in an argument. +var dangerousCodePatterns = []string{ "exec(", "eval(", // Inline code "__import__", // Python import injection "child_process", // Node.js process spawning @@ -96,7 +101,12 @@ func ValidateCommand(cmd string) error { func ValidateArgs(args []string) error { for i, arg := range args { argLower := strings.ToLower(arg) - for _, pattern := range dangerousArgPatterns { + for _, pattern := range dangerousFlagPatterns { + if isStandaloneFlag(argLower, pattern) { + return fmt.Errorf("arg[%d] contains dangerous pattern %q", i, pattern) + } + } + for _, pattern := range dangerousCodePatterns { if strings.Contains(argLower, pattern) { return fmt.Errorf("arg[%d] contains dangerous pattern %q", i, pattern) } @@ -109,6 +119,26 @@ func ValidateArgs(args []string) error { return nil } +// isStandaloneFlag checks if arg matches a flag pattern as a standalone flag, +// avoiding false positives from package names containing the flag as a substring. +// Examples: "-c" matches, "-c=value" matches, "clickup-cli" does NOT match. +func isStandaloneFlag(arg, flag string) bool { + if arg == flag { + return true + } + if after, ok := strings.CutPrefix(arg, flag); ok { + // Must be followed by a non-alphabetic character or end-of-string + if len(after) == 0 || !isAlpha(rune(after[0])) { + return true + } + } + return false +} + +func isAlpha(r rune) bool { + return (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') +} + // ValidateURL checks URL for SSRF vulnerabilities using the existing security package. // This provides DNS rebinding protection via IP pinning. func ValidateURL(rawURL string) error {