Experiment with limiting terminal on mac to certain folders #74
Conversation
WalkthroughThis update introduces a platform-aware sandboxed command execution feature, primarily targeting macOS. The configuration system is extended to support a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalManager
participant ConfigManager
participant Sandbox (macOS)
participant Shell
User->>TerminalManager: executeCommand(command)
TerminalManager->>ConfigManager: getConfig()
ConfigManager-->>TerminalManager: config (with useSandbox, allowedDirectories)
alt Sandbox enabled and available
TerminalManager->>Sandbox: executeSandboxedCommand(command, allowedDirectories)
alt Success (PID != -1)
Sandbox-->>TerminalManager: result (output, exitCode, pid)
TerminalManager-->>User: result
else Failure or not supported
Sandbox-->>TerminalManager: error result (pid -1)
TerminalManager->>Shell: spawn(command)
Shell-->>TerminalManager: result
TerminalManager-->>User: result
end
else Sandbox not enabled/available
TerminalManager->>Shell: spawn(command)
Shell-->>TerminalManager: result
TerminalManager-->>User: result
end
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/sandbox/index.ts (1)
9-55: Well-implemented platform-aware command execution.The
executeSandboxedCommandfunction:
- Retrieves allowed directories from configuration with a sensible fallback
- Implements platform detection for routing to the correct implementation
- Properly handles errors and provides meaningful error messages
- Returns a consistent result structure regardless of platform support
I would suggest adding a comment explaining the implementation status of other platforms for future maintainers.
- // Add cases for other platforms when implemented - // case 'linux': - // case 'win32': + // Add cases for other platforms when implemented + // TODO: Implement sandbox support for other platforms + // case 'linux': // Planned for future implementation + // case 'win32': // Planned for future implementationsrc/terminal-manager.ts (3)
25-28: Log the error when config retrieval fails.
Currently, the catch block quietly assigns a fallback without logging the underlying error. Logging helps debugging.try { config = await configManager.getConfig(); } catch (error) { + console.warn('Failed to retrieve config:', error); config = { allowedDirectories: [os.homedir()] }; }
30-34: Consider masking command if it may contain secrets.
Logging the command in plain text could expose credentials or sensitive data.
56-57: Consider capturing error details for analytics.
Instead of only logging the error, you could also reuse yourcapturefunction for consistency with line 180.} catch (error) { - console.error('Error in sandbox execution:', error); + capture('sandbox_error', { error, message: 'Error in sandbox execution' }); console.warn('Falling back to regular execution due to error'); }test/sandbox-test.js (2)
8-22: Use caution with direct shell exec calls.
For test code, this is fine, but relying on shell-based commands can pose injection risks if user input is involved.
38-52: Improve test clarity with assertions.
Logging works, but actual assertions would be clearer in a formal test environment. For instance, check file existence with a testing framework'sassert.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)package.json(1 hunks)src/config-manager.ts(2 hunks)src/sandbox/index.ts(1 hunks)src/sandbox/mac-sandbox.ts(1 hunks)src/terminal-manager.ts(2 hunks)test/sandbox-test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/sandbox/index.ts (3)
src/sandbox/mac-sandbox.ts (1)
executeSandboxedCommand(101-196)src/types.ts (1)
CommandExecutionResult(18-22)src/config-manager.ts (1)
configManager(187-187)
🪛 Biome (1.9.4)
src/sandbox/index.ts
[error] 59-60: Expected a catch clause but instead found 'export'.
Expected a catch clause here.
(parse)
[error] 65-65: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (18)
package.json (1)
35-35: Good addition of a dedicated sandbox test script.The new
test:sandboxscript provides a convenient way to verify the sandbox behavior, which is important for this security-focused feature.src/config-manager.ts (2)
11-11: Well-documented configuration option.Clear comment explaining the purpose of the
useSandboxoption.
123-125: Good platform-aware default configuration.Enabling sandbox by default on macOS (the only supported platform) is a sensible security-first approach, while maintaining compatibility with other platforms by disabling it where not supported.
README.md (1)
190-205: Excellent security documentation.The new Security Features section clearly explains the two main security mechanisms:
- Path Restricted File Access - Informs users about default directory restrictions and configuration options
- Blocked Commands - Lists categories of potentially dangerous commands that are blocked by default
This documentation helps users understand the security model and how to configure it to their needs.
src/sandbox/index.ts (2)
1-5: Good imports and module organization.The file correctly imports the platform-specific implementation and necessary dependencies. The organization with separate modules for platform-specific code is a good architectural decision.
57-65: Simple and effective platform detection function.The
isSandboxAvailablefunction correctly identifies if sandboxing is available on the current platform, which will be useful for conditional execution paths.🧰 Tools
🪛 Biome (1.9.4)
[error] 59-60: Expected a catch clause but instead found 'export'.
Expected a catch clause here.
(parse)
[error] 65-65: expected
}but instead the file endsthe file ends here
(parse)
src/terminal-manager.ts (4)
5-7: No issues with these new imports.
They cleanly introduce required dependencies for sandboxing and error capturing.
22-23: Trivial variable initialization.
This looks fine and straightforward.
48-51: Successful sandbox fallback.
This logic correctly distinguishes success from error states (PID == -1).
36-46:❓ Verification inconclusive
Potential mismatch with sandbox function parameters.
Your call toexecuteSandboxedCommand(command, timeoutMs, shell)may not align with the signature inmac-sandbox.ts, which expects(command, allowedDirectories, timeoutMs). Verify that the wrapper function in./sandbox/index.jsproperly forwards arguments.
🏁 Script executed:
#!/bin/bash # Search for the definition and usage of executeSandboxedCommand across the codebase rg -A 5 "function executeSandboxedCommand" rg -A 5 "export function executeSandboxedCommand"Length of output: 771
Action Required: Verify Parameter Forwarding for
executeSandboxedCommand
It appears that the call insrc/terminal-manager.ts—executeSandboxedCommand(command, timeoutMs, shell)—matches the parameter order of the wrapper function exported fromsrc/sandbox/index.ts(i.e.(command, timeoutMs, shell?)). However, the underlying implementation insrc/sandbox/mac-sandbox.tsexpects parameters in the order(command, allowedDirectories, timeoutMs). Please confirm that the wrapper insrc/sandbox/index.tsis correctly transforming or inserting the missingallowedDirectories(possibly retrieved from configuration) when forwarding the call to the mac-specific implementation.
- Review in
src/sandbox/index.ts: Ensure that allowed directories are obtained (e.g., fromconfig.allowedDirectories || [os.homedir()]) and passed in place of the second parameter.- Double-check parameter order: The current call from
src/terminal-manager.tsmust ultimately align with the signature expected bymac-sandbox.ts.test/sandbox-test.js (6)
24-28: Good setup of test directories.
You properly prepare separate allowed and restricted directories before tests.
54-70: Clear negative test approach.
This successfully demonstrates restricted access.
72-80: Cross-platform note.
Usingls -lais acceptable on macOS, but note that it might behave differently on other platforms if extended for cross-platform tests.
81-89: Another negative test for restricted directory.
Well-structured scenario confirming the sandbox denies access.
90-99: Cleanup logic is comprehensive.
Removing the test directory ensures minimal clutter after test completion.
103-106: Wrap test steps in promise chain.
The code flows logically, printing final results upon completion. Looks good.src/sandbox/mac-sandbox.ts (2)
48-67: Ensure essential system paths coverage.
Allowing/usr,/bin, etc. is often necessary, but confirm no unneeded directories are excessively whitelisted. Overly broad rules can undermine sandbox restrictions.
107-185: Cleanup of temporary files.
You remove the sandbox profile and wrapper script regardless of success. This is a good practice. However, if concurrency is expected, also handle partial cleanup or naming collisions more robustly.
| let shellToUse: string | boolean | undefined = shell; | ||
| if (!shellToUse) { | ||
| shellToUse = config.defaultShell || true; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate shell existence.
If config.defaultShell is invalid or missing from the user's system, this might fail. Consider verifying or gracefully handling shell errors.
🏁 Script executed:
#!/bin/bash
# Confirm references to defaultShell and check if there's logic handling invalid shells
rg -A 5 "defaultShell"Length of output: 2683
Review Comment: Validate Shell Existence in Terminal Manager
At src/terminal-manager.ts (lines 63–67), the current approach assigns the shell via:
let shellToUse: string | boolean | undefined = shell;
if (!shellToUse) {
shellToUse = config.defaultShell || true;
}Currently, it directly uses config.defaultShell (or falls back to true, which relies on Node’s default behavior) without validating that the specified shell actually exists on the user's system. If config.defaultShell is invalid or missing, the subsequent process execution may fail unexpectedly.
Recommendations:
- Validate the Shell: Incorporate a check (e.g., using
fs.accessor a similar method) to verify thatconfig.defaultShellexists and is executable before assigning it toshellToUse. - Graceful Fallback: If the validation fails, consider falling back to a safe default shell or handling the error gracefully (e.g., logging a warning and using Node’s default shell).
| // Final test of the updated sandbox implementation | ||
| import { executeSandboxedCommand } from '../dist/sandbox/index.js'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| import fs from 'fs/promises'; | ||
| import { exec } from 'child_process'; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check build output path for correctness.
Referencing ../dist/sandbox/index.js might cause issues if the dist folder is absent or if the script runs before a build. Ensure the build step precedes these tests.
🏁 Script executed:
#!/bin/bash
# Check whether ../dist/sandbox/index.js exists or is generated by the build script.
[ -f "../dist/sandbox/index.js" ] && echo "dist file found" || echo "dist file missing"Length of output: 82
Build Output File Missing: Action Required
The build output file ../dist/sandbox/index.js was not found, indicating that the build step either wasn’t executed or is misconfigured. Please ensure that:
- The build process runs before the tests.
- The output path is correctly set up in your build configuration.
- Test order is adjusted such that these tests run only after the build step completes successfully.
| export async function executeSandboxedCommand( | ||
| command: string, | ||
| allowedDirectories: string[], | ||
| timeoutMs: number = 30000 | ||
| ): Promise<{ output: string; exitCode: number | null; isBlocked: boolean; pid: number }> { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check for sandbox-exec deprecation.
sandbox-exec is rumored to be deprecated in newer macOS versions. Confirm continued support.
🌐 Web query:
Is sandbox-exec deprecated in the latest macOS versions?
💡 Result:
Yes, sandbox-exec is deprecated in the latest versions of macOS. The deprecation is explicitly indicated in the official documentation and man pages, which state: "The sandbox-exec command is DEPRECATED. Developers who wish to sandbox an app should instead adopt the App Sandbox feature" as outlined in Apple's App Sandbox Design Guide[5][3]. The command has been marked as deprecated since at least macOS 10.12, and the lower-level sandbox.h APIs are noted as "No longer supported"[1][2][7].
While sandbox-exec may still function on some versions of macOS, it is no longer maintained, and there is no supported command-line alternative provided for custom sandbox profiles in recent releases. Key points include:
- The sandbox profile language and related APIs are private and not supported for third-party development[9].
- Apple recommends using the App Sandbox for application sandboxing, primarily for apps distributed through the Mac App Store or with proper entitlements[5].
- There is no documented replacement for
sandbox-execfor general-purpose or ad hoc sandboxing from the command line, although advanced users sometimes rely on virtualization or third-party tools as a workaround[1][3].
In summary, sandbox-exec is deprecated and should not be used for new development or relied upon for future compatibility with macOS[5][1][3].
Citations:
- 1: https://developer.apple.com/forums/thread/661939
- 2: https://forums.swift.org/t/sandbox-exec-is-deprecated/10201
- 3: https://mybyways.com/blog/run-code-in-a-macos-sandbox
- 4: Sandbox doesn't work on macOS Big Sur when the sandbox directory is in a firm linked directory bazelbuild/bazel#13766
- 5: https://manp.gs/mac/1/sandbox-exec
- 6: https://jhftss.github.io/A-New-Era-of-macOS-Sandbox-Escapes/
- 7: https://jmmv.dev/2019/11/macos-sandbox-exec.html
- 8: https://github.com/orgs/Homebrew/discussions/59
- 9: https://bdash.net.nz/posts/sandboxing-on-macos/
- 10: https://www.karltarvas.com/macos-app-sandboxing-via-sandbox-exec.html
Action: Update sandboxing approach due to deprecation
The web query confirms that sandbox-exec has been deprecated since macOS 10.12 and is no longer supported for future macOS releases. As the current implementation in src/sandbox/mac-sandbox.ts may rely on this deprecated functionality, please consider revising the approach.
- In src/sandbox/mac-sandbox.ts (around lines 101–105), review whether the implementation still depends on
sandbox-exec. - If so, evaluate alternative sandboxing strategies (e.g., adopting Apple's App Sandbox or integrating a supported virtualization/containment mechanism).
- If continued support for legacy macOS versions is required, add clear documentation and fallback mechanisms, but plan for deprecation mitigation in future releases.
| const tempDir = path.join(os.tmpdir(), 'claude-server-sandbox'); | ||
|
|
||
| // Ensure temp directory exists | ||
| try { | ||
| // Check if directory exists first | ||
| try { | ||
| await fs.access(tempDir); | ||
| console.log(`Temp directory exists: ${tempDir}`); | ||
| } catch { | ||
| // Directory doesn't exist, create it | ||
| console.log(`Creating temp directory: ${tempDir}`); | ||
| await fs.mkdir(tempDir, { recursive: true }); | ||
| console.log(`Temp directory created: ${tempDir}`); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error creating temp directory for sandbox:', error); | ||
| throw new Error(`Failed to create sandbox temp directory: ${error}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Concurrent usage might cause collisions.
Using a fixed temp path (sandbox-profile.sb) in one directory could overwrite profiles if multiple commands run simultaneously. Consider generating a unique file name with a random suffix.
-const profilePath = path.join(tempDir, 'sandbox-profile.sb');
+const profilePath = path.join(
+ tempDir,
+ `sandbox-profile-${Date.now()}-${Math.random().toString(36).slice(2)}.sb`
+);📝 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.
| const tempDir = path.join(os.tmpdir(), 'claude-server-sandbox'); | |
| // Ensure temp directory exists | |
| try { | |
| // Check if directory exists first | |
| try { | |
| await fs.access(tempDir); | |
| console.log(`Temp directory exists: ${tempDir}`); | |
| } catch { | |
| // Directory doesn't exist, create it | |
| console.log(`Creating temp directory: ${tempDir}`); | |
| await fs.mkdir(tempDir, { recursive: true }); | |
| console.log(`Temp directory created: ${tempDir}`); | |
| } | |
| } catch (error) { | |
| console.error('Error creating temp directory for sandbox:', error); | |
| throw new Error(`Failed to create sandbox temp directory: ${error}`); | |
| } | |
| const tempDir = path.join(os.tmpdir(), 'claude-server-sandbox'); | |
| const profilePath = path.join( | |
| tempDir, | |
| `sandbox-profile-${Date.now()}-${Math.random().toString(36).slice(2)}.sb` | |
| ); | |
| // Ensure temp directory exists | |
| try { | |
| // Check if directory exists first | |
| try { | |
| await fs.access(tempDir); | |
| console.log(`Temp directory exists: ${tempDir}`); | |
| } catch { | |
| // Directory doesn't exist, create it | |
| console.log(`Creating temp directory: ${tempDir}`); | |
| await fs.mkdir(tempDir, { recursive: true }); | |
| console.log(`Temp directory created: ${tempDir}`); | |
| } | |
| } catch (error) { | |
| console.error('Error creating temp directory for sandbox:', error); | |
| throw new Error(`Failed to create sandbox temp directory: ${error}`); | |
| } |
Summary by CodeRabbit
New Features
Documentation
Tests
Chores