diff --git a/.jules/sentinel.md b/.jules/sentinel.md index bc4152d..f598ede 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -7,3 +7,7 @@ **Vulnerability:** Multiple endpoints in `api/main.py` (`compile_fast`, `generate_skill_endpoint`, `generate_agent_endpoint`, `optimize_endpoint`) were returning raw exception details `str(e)` directly to the client via HTTP 500 errors. **Learning:** Returning raw exception details is a major security risk (Information Exposure) as it can leak sensitive internal implementation details, such as file paths, database schemas, third-party API keys, or stack traces. **Prevention:** Catch exceptions, log their details internally (e.g., via `print` or a logger for debugging), and raise an `HTTPException` with a generic `detail` message (like "An internal error occurred.") rather than returning `str(e)` directly to the user. +## 2024-05-24 - Command Injection via Unsafe Subprocess Execution with Environment Variables +**Vulnerability:** The application used `subprocess.run([editor, temp_path])` where `editor` could be derived from `os.environ.get("EDITOR")`. A malicious string with arguments or embedded shell commands (if shell=True was added later) could cause unexpected execution. +**Learning:** Directly passing unstructured environment variables like `EDITOR` into `subprocess.run()` without splitting them can lead to command execution bugs (e.g., `nano -w` failing as "nano -w" file not found) or security vulnerabilities if the path allows arbitrary strings. +**Prevention:** Always use `shlex.split()` to safely tokenize shell-like strings containing arguments before passing them as the first argument to `subprocess.run()`. diff --git a/app/quick_edit.py b/app/quick_edit.py index 88114d8..018238d 100644 --- a/app/quick_edit.py +++ b/app/quick_edit.py @@ -7,6 +7,7 @@ import tempfile import subprocess import os +import shlex from typing import Optional, Dict, Any, Literal from rich.console import Console @@ -106,7 +107,12 @@ def edit_text_in_editor(self, text: str) -> Optional[str]: editor = self.get_editor() # Open editor - result = subprocess.run([editor, temp_path]) + # Safely parse the editor string into arguments to handle cases like "nano -w" + # and prevent command injection if the EDITOR env var is maliciously crafted + editor_parts = shlex.split(editor) + editor_parts.append(temp_path) + + result = subprocess.run(editor_parts) if result.returncode != 0: console.print(f"[yellow]⚠️ Editor exited with code {result.returncode}[/yellow]") diff --git a/tests/test_quick_edit_security.py b/tests/test_quick_edit_security.py new file mode 100644 index 0000000..2411850 --- /dev/null +++ b/tests/test_quick_edit_security.py @@ -0,0 +1,23 @@ +import os +from unittest.mock import patch + +from app.quick_edit import QuickEditor + + +def test_editor_command_injection(): + editor = QuickEditor() + + # Mock subprocess.run + with patch("subprocess.run") as mock_run: + # Simulate an environment variable with arguments + with patch.dict(os.environ, {"EDITOR": "nano -w -K"}): + editor.edit_text_in_editor("test content") + + # subprocess.run should receive a list of arguments, correctly split + mock_run.assert_called_once() + args = mock_run.call_args[0][0] + assert args[0] == "nano" + assert args[1] == "-w" + assert args[2] == "-K" + # The last argument should be the temp file path + assert args[3].endswith(".txt")