[Security] Harden CLI dev command against injection on Windows#1904
[Security] Harden CLI dev command against injection on Windows#1904RinZ27 wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
f7b9f72 to
a63bde1
Compare
a63bde1 to
4f4dba9
Compare
|
@Kludex I can't apply it universally because on POSIX I'm using Also, I realized |
ed1565d to
4fd818c
Compare
Switched from shlex.quote to subprocess.list2cmdline for proper argument escaping on Windows when shell=True is required for npx. This ensures security and reliability when file paths contain special characters.
95b080c to
93d26b6
Compare
Kludex
left a comment
There was a problem hiding this comment.
This is handled internally in subprocess.run().
|
Also, please don't create PRs with "SECURITY". Security vulnerabilities are supposed to be disclosed via proper means. |
|
@Kludex Thanks for the clarification and the CPython reference. I see now that subprocess.run already handles the argument quoting internally through list2cmdline, making this manual check redundant. Apologies for the 'SECURITY' tag in the title as well; I'll stick to the official disclosure channels for any future security-related findings. Closing this PR now. |
I was looking through the CLI code and spotted a potential security risk when using the
mcp devcommand on Windows. Becauseshell=Trueis required fornpx, passing raw arguments can lead to command injection if a user provides a file path containing shell metacharacters.I decided to use
shlex.quoteto sanitize these arguments before they are joined into the final command string. This way, I ensure that any special characters are safely escaped, keeping the execution restricted to the intended command. I've verified the fix and it correctly handles paths with spaces and other characters.