fix(security): tool boundary checks - #4869
Conversation
📝 WalkthroughWalkthroughPatches three CVEs in sandbox isolation and shell approval gates. ChangesDangling Symlink Sandbox Escape Fix
Shell Risk Classifier Security Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
github-actions
Bot
added
scope: tool/builtin
think-in-universe
changed the title
![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 pull request enhances path validation to reject dangling symlinks and significantly improves shell command risk classification by introducing robust tokenization and unwrapping of transparent command wrappers (like env, time, and shells). Feedback on these changes highlights several critical security and compatibility improvements: ensuring the shell script argument parser excludes long options starting with -- to prevent bypasses, handling wrapper command options that take separate arguments (e.g., env -u), and supporting Windows path separators (\\) in both command_basename and is_env_assignment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/builtin/path_utils.rs`:
- Around line 100-107: Add a `// silent-ok:` annotation to justify the silent
error handling on the symlink_metadata call. This annotation should be placed
before or on the line containing `.unwrap_or(false)` and should explain why
silently converting errors to false is acceptable in this context (e.g., that
NotFound errors are expected for new files and other IO errors will surface at
write time). This satisfies the "Fail loud" invariant by making the intentional
error suppression explicit and documented.
In `@src/tools/builtin/shell.rs`:
- Around line 402-450: The delegated_env_command function currently skips the
argument following -S or --split-string without inspecting its content for
dangerous shell metacharacters or injection patterns. This allows malicious
payloads to hide in that argument. Modify the function to extract and inspect
the argument at idx + 1 when encountering -S or --split-string, checking for
shell metacharacters and dangerous patterns before deciding whether to skip or
process it further. Either return the dangerous content so it gets properly
classified by the risk classifier, or integrate an inline risk check to detect
shell injection attempts within the -S argument itself rather than blindly
skipping it with idx += 2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51395c0b-8da0-4c0e-9a50-ec4dd7b3948f
📒 Files selected for processing (4)
src/tools/builtin/file.rssrc/tools/builtin/path_utils.rssrc/tools/builtin/shell.rstests/shell_risk_regression.rs
think-in-universe
left a comment
think-in-universe
left a comment
There was a problem hiding this comment.
Code review skill pass for current head 2963f61250d6eecc0bd6a006507a2c1978ea1754.
Findings: 4 total, including 2 high-severity approval-classification regressions. GitHub would not allow this account to request changes on this PR, so I am posting as review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/builtin/path_utils.rs (1)
266-278: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider asserting the specific error variant.
The test confirms rejection but doesn't verify it's
NotAuthorizedwith the "dangling symlink" message. A more specific assertion would catch accidental regressions where the path is rejected for a different reason.Suggested tightening
let result = validate_path("jump", Some(sandbox.path())); - assert!(result.is_err()); + let err = result.unwrap_err(); + assert!( + matches!(&err, ToolError::NotAuthorized(msg) if msg.contains("dangling symlink")), + "expected NotAuthorized(dangling symlink), got {err:?}" + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/builtin/path_utils.rs` around lines 266 - 278, The test `test_validate_path_rejects_dangling_final_symlink` uses a generic assertion that only checks if the result is an error without verifying the specific error type or message. Replace the `assert!(result.is_err());` statement with a more specific assertion that verifies the error is the `NotAuthorized` variant and that the error message contains the text "dangling symlink" to ensure the path is being rejected for the correct reason and catch regressions where rejection occurs for a different cause.src/tools/builtin/shell.rs (1)
421-454:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
env -S<adjacent>form bypasses detection; destructive payload is classified Low.
delegated_env_commandchecks exact"-S"/"--split-string"matches (line 425) and the--split-string=prefix (line 428), but not the POSIX-style-S<value>(no space). When the attacker writes:env -S'rm -rf /tmp/marker'…the token
-S'rm -rf /tmp/marker'falls through to line 443 (generic-*skip),delegated_env_commandreturnsNone, and the whole command is classified againstenvinLOW_RISK_PATTERNS→Low.Add handling analogous to
--split-string=:if token == "-S" || token == "--split-string" { return tokens.get(idx + 1).map(|script| shell_tokens(script)); } if let Some(script) = token.strip_prefix("--split-string=") { return Some(shell_tokens(script)); } +if let Some(script) = token.strip_prefix("-S") { + if !script.is_empty() { + return Some(shell_tokens(script)); + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/builtin/shell.rs` around lines 421 - 454, The delegated_env_command function handles the `-S` flag only when it has a space before its argument, and the `--split-string=` prefix form, but it does not handle the POSIX-style `-S<value>` form where the value is adjacent to the flag with no space (e.g., `-S'rm -rf /tmp/marker'`). Add a check using token.strip_prefix("-S") after the existing `--split-string=` check to detect this form, extract the script value, and return Some(shell_tokens(script)), treating it the same way as the spaced and equals-sign variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/shell_risk_regression.rs`:
- Around line 248-260: Add a regression test case for the `-S<adjacent>` form
(where the flag is directly adjacent to the argument with no space) to the
env_split_string_payloads_are_classified test function. Insert the string "env
-S'rm -rf /tmp/env-adjacent-marker'" into the cmds array alongside the existing
test cases. This ensures that once the delegated_env_command bypass is fixed,
future refactors cannot reintroduce the gap by missing this command form.
---
Outside diff comments:
In `@src/tools/builtin/path_utils.rs`:
- Around line 266-278: The test
`test_validate_path_rejects_dangling_final_symlink` uses a generic assertion
that only checks if the result is an error without verifying the specific error
type or message. Replace the `assert!(result.is_err());` statement with a more
specific assertion that verifies the error is the `NotAuthorized` variant and
that the error message contains the text "dangling symlink" to ensure the path
is being rejected for the correct reason and catch regressions where rejection
occurs for a different cause.
In `@src/tools/builtin/shell.rs`:
- Around line 421-454: The delegated_env_command function handles the `-S` flag
only when it has a space before its argument, and the `--split-string=` prefix
form, but it does not handle the POSIX-style `-S<value>` form where the value is
adjacent to the flag with no space (e.g., `-S'rm -rf /tmp/marker'`). Add a check
using token.strip_prefix("-S") after the existing `--split-string=` check to
detect this form, extract the script value, and return
Some(shell_tokens(script)), treating it the same way as the spaced and
equals-sign variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14dc5a89-6a43-4bd7-bb05-03d94939b37e
📒 Files selected for processing (3)
src/tools/builtin/path_utils.rssrc/tools/builtin/shell.rstests/shell_risk_regression.rs
|
Human final review guidance: focus on the security-sensitive tool boundary changes. Please verify shell risk classification still unwraps delegated commands through shell/env/time/sort wrappers, CRLF and single-& separators raise approval correctly, and dangling symlink metadata failures fail closed without weakening normal new-file writes. CI is green at head |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/builtin/shell.rs (1)
438-451:⚠️ Potential issue | 🔴 CriticalThese three signal options do not consume a separate token; remove them from the
idx += 2branch.GNU
envaccepts--block-signal,--default-signal, and--ignore-signalwith optional signal values attached via=(e.g.,--block-signal=TERM). The signal is not a separate token, soenv --block-signal TERM rm -rf /will fail becauseTERMis misinterpreted as an environment variable assignment, not a signal argument.The code currently skips these three flags with
idx += 2, which is incorrect. They should either be removed from this match arm (soidx += 1applies) or checked for--flag=valueprefix variants separately. Move them to the generictoken.starts_with('-')fallback to skip only the current token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/builtin/shell.rs` around lines 438 - 451, The three signal options `--block-signal`, `--default-signal`, and `--ignore-signal` are incorrectly grouped in the match arm that increments idx by 2, but these flags accept their signal value as an optional `=value` suffix (not a separate token), so they should only skip one token like other flag prefixes. Remove these three options from the current match branch and allow them to fall through to the generic `token.starts_with('-')` fallback that applies `idx += 1` instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tools/builtin/shell.rs`:
- Around line 438-451: The three signal options `--block-signal`,
`--default-signal`, and `--ignore-signal` are incorrectly grouped in the match
arm that increments idx by 2, but these flags accept their signal value as an
optional `=value` suffix (not a separate token), so they should only skip one
token like other flag prefixes. Remove these three options from the current
match branch and allow them to fall through to the generic
`token.starts_with('-')` fallback that applies `idx += 1` instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1fa05b44-3d7c-4c06-b9ed-8cacf2905922
📒 Files selected for processing (2)
src/tools/builtin/shell.rstests/shell_risk_regression.rs
|
I found one still-actionable issue in the current head I prepared and locally validated a fix in detached commit
Local verification: I could not push the fix because GitHub rejected updates to Please dequeue this PR or apply the same patch before merge; otherwise this review comment remains unresolved. |
* Fix security tool boundary checks * Address shell wrapper review feedback * Fix shell clippy lifetime lint * Fix shell wrapper risk regressions * test: cover adjacent env split-string payloads * fix: harden env wrapper risk parsing --------- Co-authored-by: Codex <codex@openai.com>
Summary
Fixes the validated security issues around built-in filesystem and shell tool boundaries:
write_filecannot follow them outsidebase_direnv ... sh -c, direct shell-c, andtime ...before reusing session-level shell auto-approvalsort --compress-programthrough the high-risk approval floorRoot Cause
write_filevalidated non-existent paths by checking the nearest existing ancestor while the final write sink followed symlinks. For shell execution, risk classification only inspected shallow command segments, so dangerous payloads hidden behind newlines or wrappers could be downgraded toUnlessAutoApprovedand inherit prior session approval.Validation
cargo fmtcargo test --test shell_risk_regressioncargo test --lib tools::builtin::path_utils::testscargo test --lib tools::builtin::file::tests::test_write_file_rejects_dangling_final_symlinkFixes #4797.
Fixes #4861.
Fixes #4862.
Fixes #4863.
Fixes #4864.
Fixes #4865.
Summary by CodeRabbit
envandsort --compress-program).