Skip to content

fix(mcp): exact-match dangerous flags, stop -c substring false positives#1115

Open
nguyennguyenit wants to merge 1 commit into
devfrom
fix/mcp-arg-validation-substring-1027
Open

fix(mcp): exact-match dangerous flags, stop -c substring false positives#1115
nguyennguyenit wants to merge 1 commit into
devfrom
fix/mcp-arg-validation-substring-1027

Conversation

@nguyennguyenit

Copy link
Copy Markdown
Contributor

Summary

  • `ValidateArgs` now matches short/long flags (`-c`, `-e`, `-r`, `--eval`, `--require`, `--import`) by exact value or `--flag=value` prefix only.
  • Inline code-execution substrings (`exec(`, `eval(`, `import`, `child_process`, `subprocess`) keep their substring semantics.
  • Fixes false positive where `@nick.bester/clickup-cli` was rejected because it contained `-c` as a substring.

Closes #1027.

Test plan

  • `go test ./internal/mcp/ -run ValidateArgs` passes (existing cases + new `clickup-cli`, `some-experimental-pkg`, `some-runner-pkg`, `--eval=...` regression cases)
  • `go build ./...` and `go build -tags sqliteonly ./...` succeed

Previously ValidateArgs flagged any arg containing '-c', '-e', or '-r' as
substrings, blocking legitimate npm package names like 'clickup-cli'. Split
the check: short/long flags now require exact match (or '--flag=value' prefix),
while inline-code patterns ('exec(', '__import__', 'subprocess', etc.) keep
substring semantics.

Closes #1027.
@mrgoonie

Copy link
Copy Markdown
Contributor

Backlog review: merge-candidate. This cleanly fixes #1027 by separating exact dangerous flags from true code-execution substrings, includes regression tests for clickup-cli-style false positives and --flag=value rejection, and go/web checks pass. I closed #1152 as superseded by this cleaner dev-targeted PR.

@clark-cant clark-cant left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Verdict: REQUEST CHANGES

Summary

This PR fixes the real false positive from #1027: package names like @nick.bester/clickup-cli should not be rejected just because they contain -c as a substring.

Risk level: medium, because this is security validation for MCP stdio command arguments.

Findings

  1. [HIGH] internal/mcp/validation.go:103 — short dangerous flags can be bypassed when the option argument is attached to the flag.

The new logic only rejects short flags by exact argument match or flag= form. That fixes the substring false positive, but it also allows forms that many runtimes accept as code-execution flags with attached operands, for example:

  • python -cpass / python -c print style attached -c code
  • node -e throw style attached -e code
  • node -rmodule style attached require preload

Some payloads with parentheses will still be caught later by shellMetaChars, but not all valid code requires those characters. For example, an arg like -cpass has no shell metacharacters and currently passes validation.

Suggested fix: keep the PRs important substring fix, but treat short dangerous options as flags when they appear at the start of an argument too, not anywhere inside it. For example:

  • reject exact -c, -e, -r
  • reject attached forms beginning with -c, -e, -r when they are intended as short option operands
  • continue allowing package names or paths where -c, -e, -r appear later in the string, e.g. @nick.bester/clickup-cli

Please add regression tests for attached short-option bypasses, e.g. -cpass, -ethrow, and -rmodule.

Anti-AI-Slop Check

  • Intent clear: yes — fixes a concrete false positive and closes #1027.
  • Diff scope justified: yes — small and targeted.
  • Refactor justified: not applicable.
  • Author explanation needed: no.

Tests / CI

  • CI: go ✅, web ✅
  • Local targeted check: go test ./internal/mcp -run ValidateArgs ✅
  • Test gap: missing negative tests for attached short dangerous flags: -c code, -e code, -r module.

Final Notes

Direction is good, but this validator protects MCP stdio execution. Avoiding substring false positives is right; allowing attached short code-execution flags is too risky to merge as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: MCP arg validation false positive — substring match on '-c' blocks legitimate package names

3 participants