fix(shell): validate cwd parameter against workspace boundary#524
Merged
Hmbown merged 1 commit intoMay 4, 2026
Merged
Conversation
The shell tool's `cwd` / `working_dir` parameter was accepted raw without any workspace boundary check, unlike file tools which all go through `ToolContext::resolve_path()`. This allowed the AI model to execute shell commands from arbitrary directories outside the workspace. Reuse the existing `resolve_path()` validation so that: - Paths outside the workspace root are rejected with `PathEscape` - `trust_mode = true` still bypasses the check (consistent behavior) - `trusted_external_paths` entries are respected automatically - Default behavior (no cwd argument) remains unchanged Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces path validation for the working_dir (or cwd) parameter in the shell tool to ensure it respects workspace boundaries. The review feedback suggests a more idiomatic implementation using transpose() and identifies a limitation where the validated working directory is currently ignored during foreground execution, meaning the parameter only functions for background or interactive commands.
Comment on lines
+1563
to
+1574
| let working_dir = match input | ||
| .get("cwd") | ||
| .or_else(|| input.get("working_dir")) | ||
| .and_then(serde_json::Value::as_str) | ||
| .map(str::to_string); | ||
| { | ||
| Some(dir) => { | ||
| // Validate cwd against workspace boundary (same as file tools) | ||
| let resolved = context.resolve_path(dir)?; | ||
| Some(resolved.to_string_lossy().to_string()) | ||
| } | ||
| None => None, | ||
| }; |
Contributor
There was a problem hiding this comment.
This logic can be simplified using transpose() to handle the Result inside the Option more idiomatically.
Additionally, note that while working_dir is correctly validated here, it is currently ignored in the foreground execution path (lines 1602-1610 in the full file) because execute_foreground_via_background does not accept a working directory argument and hardcodes None when calling the manager. This means the cwd parameter currently only functions for background or interactive commands.
let working_dir = input
.get("cwd")
.or_else(|| input.get("working_dir"))
.and_then(serde_json::Value::as_str)
.map(|dir| context.resolve_path(dir))
.transpose()?
.map(|p| p.to_string_lossy().to_string());
Owner
|
great find - thank you so much |
Hmbown
added a commit
that referenced
this pull request
May 4, 2026
Picks up the v0.8.10 patch release contents: * Daemon API quartet for whalescale-desktop integration (#561-#564, PR #567). * Bug cluster: macOS seatbelt cargo registry (#558), MCP SIGTERM shutdown (#420), Linux PR_SET_PDEATHSIG (#421). * npm install on older glibc fix (#555/#560 via #556 + #565). * Shell cwd workspace-boundary validation (#524). * Memory help/docs polish (#497 via #569). * Onboarding language picker (#566). * Whale nicknames interleaved with Simplified Chinese. First-time contributors credited in CHANGELOG: @staryxchen, @shentoumengxin, @Vishnu1837, @20bytes. Workspace `Cargo.toml`, all 9 internal path-dep version pins, and `npm/deepseek-tui/package.json` all bumped to 0.8.10. `Cargo.lock` regenerated and committed alongside. Verified locally: * cargo fmt --all -- --check * cargo clippy --workspace --all-targets --all-features --locked -- -D warnings * cargo test --workspace --all-features --locked * bash scripts/release/check-versions.sh Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MMMarcinho
pushed a commit
to MMMarcinho/DeepSeek-TUI
that referenced
this pull request
May 6, 2026
…#524) The shell tool's `cwd` / `working_dir` parameter was accepted raw without any workspace boundary check, unlike file tools which all go through `ToolContext::resolve_path()`. This allowed the AI model to execute shell commands from arbitrary directories outside the workspace. Reuse the existing `resolve_path()` validation so that: - Paths outside the workspace root are rejected with `PathEscape` - `trust_mode = true` still bypasses the check (consistent behavior) - `trusted_external_paths` entries are respected automatically - Default behavior (no cwd argument) remains unchanged Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
MMMarcinho
pushed a commit
to MMMarcinho/DeepSeek-TUI
that referenced
this pull request
May 6, 2026
Picks up the v0.8.10 patch release contents: * Daemon API quartet for whalescale-desktop integration (Hmbown#561-Hmbown#564, PR Hmbown#567). * Bug cluster: macOS seatbelt cargo registry (Hmbown#558), MCP SIGTERM shutdown (Hmbown#420), Linux PR_SET_PDEATHSIG (Hmbown#421). * npm install on older glibc fix (Hmbown#555/Hmbown#560 via Hmbown#556 + Hmbown#565). * Shell cwd workspace-boundary validation (Hmbown#524). * Memory help/docs polish (Hmbown#497 via Hmbown#569). * Onboarding language picker (Hmbown#566). * Whale nicknames interleaved with Simplified Chinese. First-time contributors credited in CHANGELOG: @staryxchen, @shentoumengxin, @Vishnu1837, @20bytes. Workspace `Cargo.toml`, all 9 internal path-dep version pins, and `npm/deepseek-tui/package.json` all bumped to 0.8.10. `Cargo.lock` regenerated and committed alongside. Verified locally: * cargo fmt --all -- --check * cargo clippy --workspace --all-targets --all-features --locked -- -D warnings * cargo test --workspace --all-features --locked * bash scripts/release/check-versions.sh Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The shell tool's
cwd/working_dirJSON parameter was accepted as-iswithout workspace boundary validation. All file tools (
read_file,write_file,list_dir, etc.) already go throughToolContext::resolve_path()to enforce workspace boundaries, but
exec_shellskipped this check entirely.This meant the AI model could run shell commands from arbitrary directories
outside the user's workspace.
This PR adds a single
resolve_path()call on thecwdparameter, bringingthe shell tool in line with the existing file tool security model:
ToolError::PathEscapetrust_mode = truestill bypasses the check (consistent with file tools)trusted_external_pathsentries are respected automaticallycwdargument → use workspace root) is unchangedChanges
crates/tui/src/tools/shell.rs— Replace raw string extraction ofcwdwith a
context.resolve_path(dir)?validated version (+9 / -2 lines)