Harden tool execution: secure subprocess runner, improved tool checks, and safer installs#36
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change implements a comprehensive security-hardening overhaul for external tool execution in NeuroRift, introducing restricted environments, subprocess limits, output sanitization, retry logic, and enhanced error handling across tool installation, verification, and generation workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
Sequence DiagramThis PR centralizes external command execution into a secure runner and routes both tool capability checks and installation through it. The flow improves safety by using restricted execution settings, validated output, and consolidated status reporting for missing or failed tools. sequenceDiagram
participant User
participant NeuroRift
participant SecureRunner
participant OS
User->>NeuroRift: Start tool check or install
NeuroRift->>NeuroRift: Resolve executable path and permissions
NeuroRift->>SecureRunner: Verify tool capability with help command
SecureRunner->>OS: Run command with restricted environment and limits
OS-->>SecureRunner: Return exit code and output
SecureRunner-->>NeuroRift: Return sanitized validated result
NeuroRift->>SecureRunner: Run package update and missing tool installs
SecureRunner-->>NeuroRift: Return per tool outcomes with retries
NeuroRift-->>User: Show consolidated success and warnings
Generated by CodeAnt AI |
 |
|
Overall Grade Focus Area: Hygiene |
Security Reliability Complexity Hygiene  |
Feedback
- Stale, copy-pasted fragments
- Multiple unused imports, redefinitions, and dead variables indicate bits were copied or left over as the file evolved; extracting small, focused functions and removing unused symbols will stop churn from accumulating.
- Weak encapsulation and boundary-crossing
- Direct calls into a client's protected internals and instance methods that don't use self show blurred module/class boundaries; define clear public APIs, convert stray instance methods to module functions, and avoid touching _-prefixed internals.
- Brittle path/temp handling
- The ad-hoc startswith path checks and insecure temp-dir usage reveal inconsistent filesystem assumptions; use pathlib comparisons (isrelativeto/resolve) and secure tempfile APIs (mkstemp/TemporaryDirectory) to validate and create safe paths.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Mar 14, 2026 11:39a.m. | Review ↗ |
Nitpicks 🔍
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc75b322e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not stdout.strip() and not stderr.strip(): | ||
| validation_passed = False | ||
| error_type = error_type or "empty_output" |
There was a problem hiding this comment.
This validation rejects any command that exits with code 0 but produces no stdout/stderr, because empty_output flips validation_passed to false and _run_command_secure requires that flag for success. That makes silent-but-successful commands (including common go install runs when nothing is printed) look like failures, so install_missing_tools can report failed installs even when the tool was actually installed.
Useful? React with 👍 / 👎.
| """Create a minimal execution environment for subprocesses.""" | ||
| return { | ||
| "PATH": os.environ.get("PATH", "/usr/bin:/bin"), | ||
| "HOME": str(self.base_dir), |
There was a problem hiding this comment.
The restricted environment forces HOME to the repo directory and drops Go-related env vars, so go install no longer honors the user’s configured GOBIN/GOPATH paths. In installs triggered from install_missing_tools, binaries are written under ${HOME}/go/bin relative to the project (not the user’s normal tool path), which commonly leaves the installed tools undiscoverable by later shutil.which checks.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| validation_passed = True | ||
| error_type = None | ||
|
|
There was a problem hiding this comment.
Suggestion: Treating empty stdout and stderr as a validation failure breaks successful commands that intentionally produce no output (for example go install when cached), so installs/checks are marked failed even with exit code 0. Only enforce non-empty output when JSON output is explicitly expected. [logic error]
Severity Level: Major ⚠️
- ⚠️ `--install` reports false failures for Go tools.
- ❌ Setup automation may misclassify successful installs.
- ⚠️ Users retry installs unnecessarily, wasting time.| validation_passed = True | |
| error_type = None | |
| if expect_json and not stdout.strip(): | |
| validation_passed = False | |
| error_type = error_type or "empty_output" |
Steps of Reproduction ✅
1. Run CLI install flow via `python neurorift_main.py --install`; `_async_main()` calls
`vf.install_missing_tools()` at `neurorift_main.py:1174-1175`.
2. In `install_missing_tools()` (`neurorift_main.py:536-546`), missing Go tools trigger
`_run_command_secure(["go", "install", package], ...)`.
3. If `go install` exits `0` with no output, `_validate_tool_output()`
(`neurorift_main.py:247-249`) marks it invalid (`empty_output`), and
`_run_command_secure()` computes `success=False` at `neurorift_main.py:360-362`.
4. The install path logs failure and sets `all_successful=False`
(`neurorift_main.py:549-558`) even though the command returned success.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift_main.py
**Line:** 247:249
**Comment:**
*Logic Error: Treating empty `stdout` and `stderr` as a validation failure breaks successful commands that intentionally produce no output (for example `go install` when cached), so installs/checks are marked failed even with exit code 0. Only enforce non-empty output when JSON output is explicitly expected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| "tool_capability_check_failed", | ||
| extra={ | ||
| "tool": tool, | ||
| "error_type": result.get("error_type"), |
There was a problem hiding this comment.
Suggestion: Requiring sudo unconditionally causes installation to fail on root-only/minimal environments where apt is available but sudo is not installed. Build the apt command dynamically so root runs apt directly and only require sudo for non-root execution. [possible bug]
Severity Level: Major ⚠️
- ❌ Root containers fail automated tool installation path.
- ⚠️ `--check` remediation cannot proceed without sudo binary.
- ⚠️ Extra manual setup needed in minimal environments.| "tool_capability_check_failed", | |
| extra={ | |
| "tool": tool, | |
| "error_type": result.get("error_type"), | |
| if not self._validate_dependency("apt"): | |
| return False | |
| apt_cmd = ( | |
| ["apt", "update"] | |
| if hasattr(os, "geteuid") and os.geteuid() == 0 | |
| else ["sudo", "apt", "update"] | |
| ) | |
| if apt_cmd[0] == "sudo" and not self._validate_dependency("sudo"): | |
| return False | |
| update_result = self._run_command_secure(apt_cmd, timeout=180, retries=1) |
Steps of Reproduction ✅
1. Trigger install path using `python neurorift_main.py --install` (or `--check` then
approve install); call site is `_async_main()` at `neurorift_main.py:1169-1175`.
2. `install_missing_tools()` immediately requires both `sudo` and `apt` at
`neurorift_main.py:516-517`, before checking execution context.
3. In root/minimal images where `apt` exists but `sudo` is absent,
`_validate_dependency("sudo")` fails through `is_tool_installed()`
(`neurorift_main.py:487-490`), returning `False`.
4. Function exits early (`neurorift_main.py:517`); `apt update` at `neurorift_main.py:519`
never runs, though root could execute `apt` directly.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift_main.py
**Line:** 516:519
**Comment:**
*Possible Bug: Requiring `sudo` unconditionally causes installation to fail on root-only/minimal environments where `apt` is available but `sudo` is not installed. Build the apt command dynamically so root runs `apt` directly and only require `sudo` for non-root execution.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| list_tools_parser.add_argument('--verbose', action='store_true', help='Show detailed tool information') | ||
| list_tools_parser = subparsers.add_parser( | ||
| "list-tools", help="List all custom tools" | ||
| ) |
There was a problem hiding this comment.
Suggestion: The orchestrated mode defaults to defensive whenever --mode is omitted, which conflicts with the earlier default session mode (offensive) and can block offensive tools unexpectedly. Use the same default mode resolution in both places. [logic error]
Severity Level: Critical 🚨
- ❌ Orchestrated scans block offensive recon tools unexpectedly.
- ❌ ExecutionManager permission checks fail planned offensive steps.
- ⚠️ Session mode and runtime mode become inconsistent.| ) | |
| tool_mode = ( | |
| ToolMode.OFFENSIVE | |
| if (args.mode or "offensive") == "offensive" | |
| else ToolMode.DEFENSIVE | |
| ) |
Steps of Reproduction ✅
1. Start orchestrated workflow without explicit mode: `python neurorift_main.py
--orchestrated --target example.com`; control enters `_async_main()` orchestrated branch
at `neurorift_main.py:1011`.
2. Earlier in the same function, session creation defaults mode to offensive
(`mode=args.mode or "offensive"`) at `neurorift_main.py:1005-1008`.
3. Orchestrated context then sets `tool_mode` using `args.mode == "offensive"` at
`neurorift_main.py:1029`; with omitted `--mode`, this resolves to `ToolMode.DEFENSIVE`.
4. During execution, `NROperator.execute_plan()` calls `ExecutionManager.execute_tool()`
(`modules/ai/agents.py:61-69`), and the mode gate rejects offensive tools
(`modules/orchestration/execution_manager.py:42-50`), including wrappers marked offensive
like `modules/tools/wrappers/nmap.py:12`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** neurorift_main.py
**Line:** 1029:1029
**Comment:**
*Logic Error: The orchestrated mode defaults to defensive whenever `--mode` is omitted, which conflicts with the earlier default session mode (`offensive`) and can block offensive tools unexpectedly. Use the same default mode resolution in both places.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
1 participant
User description
Motivation
Description
_run_command_securewith restricted environment, retries, resource limits, output sanitization, and optional Linux sandboxing (firejail)._get_restricted_env,_is_safe_argument,_is_safe_workdir,_build_preexec_limits,_sanitize_output,_validate_tool_output,_should_retry, and_verify_tool_capability.is_tool_installedto acceptverify_executionand by usingshutil.whichand executable checks; updatedcheck_toolsto use this verification and improved logging/console messages.install_missing_toolsto validate package manager dependencies (sudo,apt,go), performapt updatevia the secure runner, install Go-based tools with_run_command_secure, surface failures without aborting when possible, and return consolidated success state.shutil,List) and added an interactivedev_mode_shellhelper for development-time commands.Testing
Codex Task
CodeAnt-AI Description
Secure and validated tool execution, safer installs, and clearer tool checks
What Changed
Impact
✅ Fewer unsafe tool executions✅ Clearer install failure messages✅ Safer automatic tool installation💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Improvements