feat(core): refactor SandboxManager to a stateless architecture and introduce explicit Deny interface#23141
feat(core): refactor SandboxManager to a stateless architecture and introduce explicit Deny interface#23141
Conversation
|
Hi @ehedlund, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and maintainability of the sandboxing mechanism by transitioning the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
ehedlund
force-pushed
the
feature/sandbox-request
branch
from
1258ef4 to
910bb9c
Compare
|
Size Change: +1.19 kB (0%) Total Size: 26.1 MB
ℹ️ View Unchanged
|
![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 refactors sandbox policy management by introducing a new ExecutionPolicy interface and moving policy-specific configurations (like allowedPaths, networkAccess, and sanitizationConfig) from global manager options or a generic config object directly into the SandboxRequest's policy field. This change affects LinuxSandboxManager and MacOsSandboxManager, making sandbox policies dynamic per execution request rather than static per manager instance. The MacOsSandboxManager also internalizes the logic for building Seatbelt arguments, removing a separate seatbeltArgsBuilder module, and includes robust path normalization with symlink resolution. Additionally, a new forbiddenResourceService is introduced to identify resources that should be excluded from the sandbox based on .gitignore and .geminiignore files, complete with new unit tests. However, the current implementation has two high-severity security vulnerabilities: the MacOsSandboxManager ignores the forbiddenPaths policy, potentially allowing access to restricted subpaths within allowed directories, and the ShellExecutionService fails to pass forbiddenPaths to the sandbox manager, preventing their enforcement.
gemini-cli
bot
added
the
status/need-issue
galz10
left a comment
galz10
left a comment
There was a problem hiding this comment.
Code Review
Scope: Pull Request #23141
This PR successfully refactors the SandboxManager implementations (MacOsSandboxManager and LinuxSandboxManager) to a stateless architecture. By migrating configurations like allowedPaths and networkAccess from the constructor to a per-request ExecutionPolicy, the lifecycle of the managers is simplified and the foundation is laid for dynamic security boundaries. The consolidation of macOS Seatbelt logic into the manager class itself is also a nice cleanup.
There are a few areas that need minor adjustments before merging, primarily regarding the missing OS-specific implementations for the newly introduced forbiddenPaths interface and some edge cases in the path handling.
Metadata Review
The PR title and description clearly explain the motivation and the technical changes made. The Conventional Commits prefix (feat(core):) is used correctly. However, the title and body claim to have introduced "support for path-level Deny rules", but the underlying OS manager implementations for forbiddenPaths are currently missing.
Concerns (Action Required)
-
packages/core/src/sandbox/macos/MacOsSandboxManager.ts&packages/core/src/sandbox/linux/LinuxSandboxManager.ts:
Missing OS-level implementation forforbiddenPaths. The PR explicitly mentions adding support for explicit "Deny" rules (forbiddenPaths). WhileforbiddenPathshas been added to theExecutionPolicyinterface, neither the macOS nor the Linux sandbox managers process this array.Since the interface is in place, if the OS-specific implementation is out of scope for this current PR:
- Please add
TODOcomments in both managers whereforbiddenPathswill be processed. - Please create child bugs/issues for adding the OS-specific implementations (e.g., adding
(deny file-read* file-write* (subpath (param "FORBIDDEN_PATH_X")))to the Seatbelt profile for macOS, and masking directories/files for Linux bwrap) and link them in the PR description or code comments. - You may also want to update the PR title/description slightly to clarify that only the interface/infrastructure for
forbiddenPathswas added in this step.
- Please add
Nits (Minor Suggestions / Enhancements)
-
packages/core/src/sandbox/linux/LinuxSandboxManager.ts:
Use--bind-tryfor dynamically allowed paths. Currently, the code uses--bindforallowedPaths. If a path inallowedPathsdoes not exist on the host filesystem,bwrapwill immediately fail to start. Using--bind-trytells Bubblewrap to silently ignore the bind if the source path doesn't exist. This aligns its behavior more closely with macOS Seatbelt (which handles non-existent paths gracefully) and prevents execution crashes for dynamically evaluated paths. -
packages/core/src/sandbox/linux/LinuxSandboxManager.ts:
Robust Workspace Path Comparison. The checkif (path !== this.options.workspace)is a strict string comparison. Ifoptions.workspaceis/workspacebut an allowed path is passed as/workspace/(with a trailing slash), the condition will evaluate to true and it will attempt to bind the workspace twice. Consider usingpath.normalize()orpath.resolve()on both strings before comparing them to ensure accurate matching. -
packages/core/src/sandbox/macos/MacOsSandboxManager.ts&packages/core/src/sandbox/linux/LinuxSandboxManager.ts:
DeduplicateallowedPaths. Currently, ifreq.policy?.allowedPathscontains duplicate entries, both managers will process them redundantly (macOS will generate duplicate Seatbelt parameters/rules, and Linux will push duplicate--bindarguments). Consider deduplicating the paths (e.g.,Array.from(new Set(req.policy?.allowedPaths || []))) before processing them. -
packages/core/src/services/sandboxManager.ts:
ExecutionPolicyDocumentation. The comment forforbiddenPathsmentions "Absolute paths". However, the current macOS implementation (tryRealpath) natively converts relative paths to absolute ones viafs.realpathSync. It might be worth documenting whetherallowedPathsandforbiddenPathsare strictly required to be absolute, or if the managers will handle resolving them if relative paths are passed. -
packages/core/src/sandbox/macos/MacOsSandboxManager.ts:
Profile String Construction. The Seatbelt profile string is built via string concatenation (profile += ...). While functionally correct and safe here since you are properly using parameterized bindings (-D ALLOWED_PATH_X=...), using a string array andjoin('\n')might be slightly cleaner for readability if this profile generation logic grows more complex in the future.
ehedlund
force-pushed
the
feature/sandbox-request
branch
from
29b4d45 to
be84ca7
Compare
ehedlund
force-pushed
the
feature/sandbox-request
branch
from
be84ca7 to
936abb0
Compare
ehedlund
force-pushed
the
feature/sandbox-request
branch
from
936abb0 to
930206c
Compare
ehedlund
changed the title
2 participants
Summary
This PR refactors the
SandboxManagerand its OS-specific implementations into a stateless architecture and introduces the infrastructure for explicit "Deny" rules (forbiddenPaths). By standardizing the configuration flow across macOS, Linux, and Windows, we've created a more robust foundation for dynamic security policies.Details
1. Stateless Architecture
MacOsSandboxManager,LinuxSandboxManager, andwindowsSandboxManagerno longer hold per-request state. They are initialized with a staticGlobalSandboxOptions(containing the workspace root) and receive a dynamicExecutionPolicyfor every command.GlobalSandboxOptions(startup-time) andExecutionPolicy(execution-time) interfaces to clarify when specific security boundaries are applied.2. Standardized Path Processing
sanitizePathsutility that deduplicates paths and enforces absolute-path requirements. It correctly handles case-insensitivity on Windows while preserving original casing.--bind-tryfor dynamic paths, preventing sandbox initialization crashes if an allowed path is missing on the host.3. OS Integration & Security
MacOsSandboxManager, removing the separate builder file.getSecureSanitizationConfighelper, ensuring it inherits the same strict secret-filtering and redaction policies as other platforms.forbiddenPathsfield to the execution policy across all platforms (enforcement logic remains a TODO for a subsequent PR).4. Test Suite Refactoring
sandboxManager.test.ts.Related Issues
Resolves https://github.com/google-gemini/maintainers-gemini-cli/issues/1556
How to Validate
npm test -w @google/gemini-cli-corenpm test -w @google/gemini-cli-core -- src/services/sandboxManager.test.tsPre-Merge Checklist