Security hardening and code quality improvements#11
Security hardening and code quality improvements#11ISIL-AvishayRapp wants to merge 5 commits intoavifenesh:mainfrom
Conversation
- Shell command injection prevention in mcp.lua, mcp_manager.lua, terminal.lua - Path traversal prevention in MCP server (validateFilePath function) - Proper ENOENT error handling in TypeScript - Robust CLI argument parsing for quoted strings - Deprecated Neovim API migration (vim.bo/vim.wo) - Cross-platform path handling in session.lua - Removed dead code from bridge.lua
Summary of ChangesHello @ISIL-AvishayRapp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a comprehensive set of security hardening measures and significant code quality improvements. It addresses critical vulnerabilities and enhances the application's resilience by refining how it handles external inputs and system interactions. Additionally, the codebase has been modernized to align with current best practices and Neovim API standards, resulting in a more secure, stable, and maintainable system without introducing any breaking changes. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves security and code quality. The shell injection vulnerabilities have been correctly addressed by using array-based commands, and the path traversal vulnerability in the MCP server is fixed with proper path validation. The migration from deprecated Neovim APIs to the modern vim.bo/vim.wo syntax is also a great cleanup.
I have two suggestions for further improvement:
- A small bug in the path traversal validation logic that affects projects located in the root directory.
- A robustness improvement in
mcp_manager.luato ensure the original working directory is always restored, even if errors occur.
Overall, great work on hardening the plugin!
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
Pull request overview
This PR implements security hardening and code quality improvements for the claucode.nvim plugin, focusing on preventing shell injection vulnerabilities, path traversal attacks, and modernizing deprecated Neovim API usage.
Changes:
- Added path traversal validation to the MCP server to prevent file operations outside the project directory
- Replaced string concatenation for shell commands with array-based command execution to prevent shell injection
- Migrated from deprecated
nvim_buf_set_optionandnvim_win_set_optionAPIs to modernvim.boandvim.wosyntax - Improved error handling to distinguish ENOENT from other filesystem errors
- Enhanced CLI argument parsing to handle quoted strings
- Removed dead code and improved cross-platform path handling
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp-server/src/index.ts | Added path traversal validation, improved ENOENT error handling, and enhanced type safety |
| lua/claucode/mcp.lua | Replaced string-based shell commands with array form to prevent injection, updated to modern vim.bo/vim.wo APIs |
| lua/claucode/mcp_manager.lua | Implemented array-based command execution with directory change handling |
| lua/claucode/terminal.lua | Added robust CLI argument parser with quote handling, migrated to modern vim.bo/vim.wo APIs |
| lua/claucode/ui.lua | Migrated all buffer and window option setting to modern vim.bo/vim.wo APIs |
| lua/claucode/watcher.lua | Updated buffer option access to modern vim.bo API |
| lua/claucode/session.lua | Improved cross-platform path handling with better normalization |
| lua/claucode/bridge.lua | Removed unused escape_prompt function |
| .gitignore | Added .github/ directory to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Ensure the resolved path starts with the working directory | ||
| // This prevents escaping via ../ sequences | ||
| if (!resolved.startsWith(path.normalize(`${workDir}${path.sep}`)) && resolved !== workDir) { |
There was a problem hiding this comment.
The path validation logic has a potential issue. The condition resolved !== workDir allows the working directory itself to pass validation, but the main check !resolved.startsWith(path.normalize(\${workDir}${path.sep}`))` requires paths to start with workDir + separator. This means the working directory itself would fail the startsWith check (since it doesn't end with a separator), but is then allowed by the second part of the OR condition.
However, there's an edge case: if workDir ends with a path separator (which shouldn't happen with process.cwd() but could theoretically occur), the normalization could create unexpected behavior. Also, on Windows, path comparison can be case-insensitive, which this check doesn't account for.
A more robust approach would be to use path.relative() and check if the result starts with '..' or is an absolute path:
const relativePath = path.relative(workDir, resolved);
if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) {
return { valid: false, resolved, error: ... };
}This handles edge cases like case-sensitivity and trailing separators more reliably.
| // Ensure the resolved path starts with the working directory | |
| // This prevents escaping via ../ sequences | |
| if (!resolved.startsWith(path.normalize(`${workDir}${path.sep}`)) && resolved !== workDir) { | |
| // Compute the path relative to the working directory to robustly detect escapes | |
| const relativePath = path.relative(workDir, resolved); | |
| if (relativePath.startsWith("..") || path.isAbsolute(relativePath)) { |
| -- Use array form to prevent shell injection | ||
| -- vim.fn.system doesn't support cwd parameter, so we temporarily change directory | ||
| -- Use pcall to ensure working directory is always restored | ||
| local output | ||
| local prev_cwd = vim.fn.getcwd() | ||
| local ok, err = pcall(function() | ||
| vim.api.nvim_set_current_dir(project_dir) | ||
| output = vim.fn.system({claude_cmd, "mcp", "remove", server_name}) | ||
| end) | ||
| vim.api.nvim_set_current_dir(prev_cwd) | ||
| if not ok then | ||
| notify.error("Failed to execute command for MCP server removal: " .. tostring(err)) | ||
| return false | ||
| end | ||
| if vim.v.shell_error ~= 0 then |
There was a problem hiding this comment.
The implementation of directory change with pcall has a subtle issue. If nvim_set_current_dir() throws an error, the function catches it but the working directory might have been changed before the error occurred. The second nvim_set_current_dir(prev_cwd) outside the pcall will execute, but this could mask the original error if the directory was successfully changed and then the system() call failed.
Additionally, there's an inconsistency with _do_add_mcp_server() (lines 117-152) which uses vim.fn.jobstart() with the cwd option. Consider using jobstart() here as well for consistency and to avoid changing the global current directory, which could affect other concurrent operations:
vim.fn.jobstart({claude_cmd, "mcp", "remove", server_name}, {
cwd = project_dir,
on_exit = function(_, exit_code)
-- handle exit code
end
})This approach is safer and more consistent with the rest of the codebase.
| -- Use array form to prevent shell injection | |
| -- vim.fn.system doesn't support cwd parameter, so we temporarily change directory | |
| -- Use pcall to ensure working directory is always restored | |
| local output | |
| local prev_cwd = vim.fn.getcwd() | |
| local ok, err = pcall(function() | |
| vim.api.nvim_set_current_dir(project_dir) | |
| output = vim.fn.system({claude_cmd, "mcp", "remove", server_name}) | |
| end) | |
| vim.api.nvim_set_current_dir(prev_cwd) | |
| if not ok then | |
| notify.error("Failed to execute command for MCP server removal: " .. tostring(err)) | |
| return false | |
| end | |
| if vim.v.shell_error ~= 0 then | |
| -- Use array form to prevent shell injection and run in project_dir without | |
| -- changing Neovim's global working directory. | |
| local output = "" | |
| local job_id = vim.fn.jobstart({ claude_cmd, "mcp", "remove", server_name }, { | |
| cwd = project_dir, | |
| stdout_buffered = true, | |
| stderr_buffered = true, | |
| on_stdout = function(_, data) | |
| if data then | |
| output = output .. table.concat(data, "\n") | |
| end | |
| end, | |
| on_stderr = function(_, data) | |
| if data then | |
| output = output .. table.concat(data, "\n") | |
| end | |
| end, | |
| }) | |
| if job_id <= 0 then | |
| notify.error("Failed to start command for MCP server removal.") | |
| return false | |
| end | |
| -- Wait synchronously for the job to finish to preserve the original | |
| -- synchronous behavior of this function. | |
| local status = vim.fn.jobwait({ job_id }, -1)[1] | |
| -- jobwait returns: | |
| -- - exit code (0..255) on success | |
| -- - -1 on timeout, -2 on interrupted | |
| if status == -1 or status == -2 then | |
| notify.error("Timed out or interrupted while removing MCP server.") | |
| return false | |
| end | |
| local exit_code = status or 1 | |
| if exit_code ~= 0 then |
| -- Quoted argument: find matching closing quote | ||
| local quote = char | ||
| i = i + 1 | ||
| local start = i | ||
| while i <= len and cli_args:sub(i, i) ~= quote do | ||
| i = i + 1 | ||
| end | ||
| arg = cli_args:sub(start, i - 1) | ||
| i = i + 1 -- skip closing quote |
There was a problem hiding this comment.
The quoted string parser doesn't handle escape sequences. If a user wants to include a quote character within a quoted string (e.g., --message "He said \"Hello\""), the parser will terminate at the first escaped quote rather than treating it as part of the string.
While this might be acceptable for the current use case, consider documenting this limitation or adding support for escape sequences like \" within quoted strings. A simple approach would be to check for backslash-escaped quotes:
while i <= len do
if cli_args:sub(i, i) == '\\' and i < len and cli_args:sub(i+1, i+1) == quote then
arg = arg .. quote
i = i + 2
elseif cli_args:sub(i, i) == quote then
break
else
arg = arg .. cli_args:sub(i, i)
i = i + 1
end
endThis is a minor limitation but worth considering for robustness.
| -- Quoted argument: find matching closing quote | |
| local quote = char | |
| i = i + 1 | |
| local start = i | |
| while i <= len and cli_args:sub(i, i) ~= quote do | |
| i = i + 1 | |
| end | |
| arg = cli_args:sub(start, i - 1) | |
| i = i + 1 -- skip closing quote | |
| -- Quoted argument: handle backslash-escaped quotes | |
| local quote = char | |
| i = i + 1 | |
| while i <= len do | |
| local c = cli_args:sub(i, i) | |
| if c == '\\' and i < len and cli_args:sub(i + 1, i + 1) == quote then | |
| -- Backslash-escaped quote: add quote to arg and skip both characters | |
| arg = arg .. quote | |
| i = i + 2 | |
| elseif c == quote then | |
| -- Closing quote: consume it and stop | |
| i = i + 1 | |
| break | |
| else | |
| -- Regular character inside quoted argument | |
| arg = arg .. c | |
| i = i + 1 | |
| end | |
| end |
|
|
Summary
Testing
|
2 participants
Hey Avi,
This is the updated PR addressing the feedback from the previous review. The gemini-code-assist bot caught a few issues that have now been fixed:
vim.fn.system()not accepting acwdparameter (was passing an invalid third argument)--message "Hello World"The original security fixes remain (shell injection prevention, path traversal prevention, deprecated API migration), and all the review feedback has been incorporated.
All changes are backward-compatible and will not break anything for existing users.
Summary
mcp.lua,mcp_manager.lua,terminal.luamcp-server/src/index.tsmcp-server/src/index.tsterminal.luaSecurity Fixes
Shell Command Injection Prevention
Problem: String concatenation for shell commands is vulnerable if paths contain metacharacters.
Note:
vim.fn.system()does not support acwdparameter. When you need to run a command in a specific directory, save/restore the working directory:Path Traversal Prevention (MCP Server)
Problem: MCP server accepted file paths from Claude without validation. A malicious prompt could write to
../../../etc/passwd.Solution: Added
validateFilePath()that rejects any path escaping the project directory:CLI Argument Parsing
Problem: Simple
%S+tokenization breaks quoted arguments like--message "Hello World".Solution: Robust parser that handles single and double quoted strings. See terminal.lua for the full implementation.
Proper Error Handling in TypeScript
Problem: Empty
catchblocks swallow all errors, not just "file not found".Code Quality Improvements
Deprecated Neovim API Migration
Updated to Neovim 0.10+ patterns:
nvim_buf_set_option(buf, "opt", val)vim.bo[buf].opt = valnvim_win_set_option(win, "opt", val)vim.wo[win].opt = valOther Cleanups
escape_prompt()frombridge.luasession.luaFiles Changed
Testing
Breaking Changes
None. All public APIs remain unchanged.