Fix sandbox access to workdir secrets#366
Conversation
|
Warning Review limit reached
More reviews will be available in 22 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds sandbox-based path validation to tool services. BashToolService and FileToolService now validate sandboxed tool execution against per-chat directory allow-lists, blocking access outside permitted roots (app base directory, per-chat media/file directories under Env.WorkDir, and optionally Env.SandboxieGroupFilesRoot). Changes include helpers for path normalization and OS-aware containment checking, along with comprehensive tests and Sandboxie configuration alignment. ChangesSandbox Path Restrictions for Tool Services
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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 `@TelegramSearchBot.LLM/Service/Tools/BashToolService.cs`:
- Line 216: The sandbox validation uses ResolveFullPath(workingDirectory ??
AppContext.BaseDirectory) but execution actually uses Env.WorkDir, so make the
validation use the same default as execution: change the ResolveFullPath call
that sets workDir to use (workingDirectory ?? Env.WorkDir) instead of
AppContext.BaseDirectory; update any references in the BashToolService method
where workDir is validated to ensure it now validates the actual execution path
(symbols: ResolveFullPath, workingDirectory, AppContext.BaseDirectory,
Env.WorkDir, workDir).
In `@TelegramSearchBot.Test/Service/AI/LLM/SandboxieToolHostServiceTests.cs`:
- Around line 28-32: The assertions compare expected ClosedFilePath strings
without the trailing "\*" but BuildPortableBoxIni appends "\*" when a directory
exists, causing brittle failures; update the test to use the existing ClosedPath
helper (ClosedPath(Env.WorkDir, "Photos"), ClosedPath(Env.WorkDir, "Audios"),
ClosedPath(Env.WorkDir, "Videos"), ClosedPath(Env.WorkDir, "Files"), and
ClosedPath(Env.WorkDir, "logs")) for each Assert.Contains so the expected value
matches BuildPortableBoxIni's behavior regardless of whether the directory
exists; keep references to Env.WorkDir and ensure you call ClosedPath for each
asserted path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc260350-20cc-4811-aad7-0e35f4db8d34
📒 Files selected for processing (6)
TelegramSearchBot.LLM.Test/Service/Tools/BashToolServiceTests.csTelegramSearchBot.LLM.Test/Service/Tools/FileToolServiceTests.csTelegramSearchBot.LLM/Service/Tools/BashToolService.csTelegramSearchBot.LLM/Service/Tools/FileToolService.csTelegramSearchBot.Test/Service/AI/LLM/SandboxieToolHostServiceTests.csTelegramSearchBot/Service/AI/LLM/SandboxieToolHostService.cs
| return true; | ||
| } | ||
|
|
||
| var workDir = ResolveFullPath(workingDirectory ?? AppContext.BaseDirectory); |
There was a problem hiding this comment.
Critical: Default path mismatch causes sandbox bypass.
When workingDirectory is null, the sandbox validation uses AppContext.BaseDirectory as the default (Line 216), but actual command execution uses Env.WorkDir (Line 106). Since Env.WorkDir itself is not in the allowed roots (only chat-specific subdirectories are), a sandboxed command with no explicit workingDirectory will:
- Pass validation (because
AppContext.BaseDirectoryis allowed) - Execute in
Env.WorkDir(which is not allowed, exposing logs, config, sqlite)
This defeats the sandbox restriction this PR aims to implement.
🔒 Proposed fix
- var workDir = ResolveFullPath(workingDirectory ?? AppContext.BaseDirectory);
+ var workDir = ResolveFullPath(workingDirectory ?? Env.WorkDir);This makes the validation check the same path that will actually be used for execution. Since Env.WorkDir is not in the allowed list, sandboxed commands without an explicit allowed workingDirectory will be correctly rejected.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var workDir = ResolveFullPath(workingDirectory ?? AppContext.BaseDirectory); | |
| var workDir = ResolveFullPath(workingDirectory ?? Env.WorkDir); |
🤖 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 `@TelegramSearchBot.LLM/Service/Tools/BashToolService.cs` at line 216, The
sandbox validation uses ResolveFullPath(workingDirectory ??
AppContext.BaseDirectory) but execution actually uses Env.WorkDir, so make the
validation use the same default as execution: change the ResolveFullPath call
that sets workDir to use (workingDirectory ?? Env.WorkDir) instead of
AppContext.BaseDirectory; update any references in the BashToolService method
where workDir is validated to ensure it now validates the actual execution path
(symbols: ResolveFullPath, workingDirectory, AppContext.BaseDirectory,
Env.WorkDir, workDir).
PR Check ReportSummary
Test Results
Code Quality
Test Artifacts
LinksThis report is auto-generated by GitHub Actions |
1 participant
Summary
Validation
dotnet test TelegramSearchBot.LLM.Test/TelegramSearchBot.LLM.Test.csproj --filter "FullyQualifiedName~TelegramSearchBot.Test.Service.Tools.BashToolServiceTests|FullyQualifiedName~TelegramSearchBot.Test.Service.Tools.FileToolServiceTests"dotnet test TelegramSearchBot.Test/TelegramSearchBot.Test.csproj --filter "FullyQualifiedName~SandboxieToolHostServiceTests"