Conversation
…debase - Replaces manual `env_clear()` and iterative environment whitelist logic with standardized `crate::utils::env::apply_isolated_env` or `apply_isolated_env_async` helpers. - Updates `src-tauri/src/utils/platform.rs`, `src-tauri/src/mcp/builtin/bootstrap/platform.rs`, `src-tauri/src/mcp/server/lifecycle.rs`, `src-tauri/src/mcp/session_isolation/stdio_manager/lifecycle.rs`, and `src-tauri/src/mcp/builtin/workspace/persistent_shell/session.rs`. - Updates corresponding test assertions in `tests.rs` to check for `apply_isolated_env_async`. - Appends new security learning to `.jules/sentinel.md`. Co-authored-by: fritzprix <917899+fritzprix@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Centralizes subprocess environment isolation across the Rust backend by replacing scattered env_clear() + whitelist loops with the standardized crate::utils::env::apply_isolated_env / apply_isolated_env_async helpers, reducing the risk of future divergence that could leak host secrets to child processes.
Changes:
- Replaced manual env scrubbing/whitelisting with
apply_isolated_env/apply_isolated_env_asyncin multiple process-spawning paths. - Updated the stdio manager “design verification” test to assert usage of the centralized helper.
- Documented the new enforcement rule in
.jules/sentinel.md.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src-tauri/src/utils/platform.rs |
Uses centralized env isolation when probing command existence. |
src-tauri/src/mcp/session_isolation/stdio_manager/tests.rs |
Updates test expectation to look for apply_isolated_env_async. |
src-tauri/src/mcp/session_isolation/stdio_manager/lifecycle.rs |
Uses centralized async env isolation before applying per-server env. |
src-tauri/src/mcp/server/lifecycle.rs |
Uses centralized async env isolation in stdio server startup. |
src-tauri/src/mcp/builtin/workspace/persistent_shell/session.rs |
Uses centralized async env isolation for persistent shell spawn. |
src-tauri/src/mcp/builtin/bootstrap/platform.rs |
Uses centralized env isolation for tool/version/path detection commands. |
.jules/sentinel.md |
Adds a Sentinel entry documenting the centralized isolation enforcement rule. |
| // Verify that we use apply_isolated_env_async | ||
| assert!( | ||
| source.contains("cmd.env_clear()"), | ||
| "stdio_manager MUST call env_clear() to isolate process environment" | ||
| ); | ||
|
|
||
| // Verify that we are whitelisting PATH | ||
| assert!( | ||
| source.contains("\"PATH\""), | ||
| "stdio_manager must whitelist PATH" | ||
| source.contains("apply_isolated_env_async"), | ||
| "stdio_manager MUST call apply_isolated_env_async() to isolate process environment" | ||
| ); |
There was a problem hiding this comment.
The test is now only checking source.contains("apply_isolated_env_async"), which can produce false positives (e.g., if the string appears in a comment or unrelated code). Since this test is meant to prevent security regressions, make the assertion more specific (e.g., require crate::utils::env::apply_isolated_env_async and/or the exact call on cmd) and update the test name/message to match the new expectation (it no longer verifies env_clear).
Make the stdio manager design-verification test assert the exact centralized helper call and rename the test to match the behavior it verifies.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🛡️ Sentinel: Enforce centralized process environment isolation
🚨 Severity: HIGH
💡 Vulnerability: Scattered manual environment clearing and looping over whitelist variables could lead to bypasses of the central isolation logic, exposing host secrets (e.g.
OPENAI_API_KEY) to child processes.🎯 Impact: Untrusted code or tools could access environment variables if the manual isolation logic diverges from the centralized rules.
🔧 Fix: Replaced all scattered implementations with the standardized
crate::utils::env::apply_isolated_envandapply_isolated_env_asynchelpers.✅ Verification: Ran
cargo checkandcargo test --teststo verify successful compilation and that process behavior remains functionally correct while adhering to the new code requirement.PR created automatically by Jules for task 125451657414619863 started by @fritzprix