Conversation
|
Size Change: +5.29 kB (+0.02%) Total Size: 26.3 MB
ℹ️ View Unchanged
|
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 Windows sandbox capabilities by introducing dynamic permission management. It allows the system to intelligently grant network access and filesystem write permissions at a low integrity level, adapting to specific command requirements and predefined policies. This improves both security and flexibility by enabling fine-grained control over sandbox behavior, preventing unauthorized operations while facilitating legitimate ones. 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
|
![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 the Windows sandbox manager by introducing new configuration options for sandbox mode and policy management. It implements logic for command approval and merges various permission sources for network and file system access. New tests cover these functionalities, including network access, plan mode restrictions, and additional write paths. However, there are critical security concerns regarding the additionalPermissions allowing arbitrary write access outside the workspace and potential UNC path vulnerabilities. Additionally, there are high-severity logical flaws in the isStrictlyApproved method that can lead to incorrect command rejections, and a bug in the network permission evaluation where the nullish coalescing operator ?? incorrectly short-circuits, preventing proper consideration of all permission sources.
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 #23691
This PR introduces dynamic sandbox expansion for Windows (Phase 1 and 2.1) by updating WindowsSandboxManager to support explicit and persistent overrides for network access and filesystem write permissions. The changes align well with the overall architecture, but there are several critical bugs affecting network access resolution, path handling, and shell parsing that need to be addressed before merging.
Metadata Review
The PR Title (feat(core): implement Windows sandbox dynamic expansion Phase 1 and 2.1) perfectly follows the Conventional Commits format. The PR Body is detailed, provides clear context, references the related issue, and has a completed pre-merge checklist for the relevant platforms.
Concerns (Action Required)
-
packages/core/src/sandbox/windows/WindowsSandboxManager.ts(Network Access Resolution): The resolution ofnetworkAccesscompletely ignores overrides if the default mode explicitly disables network access.const networkAccess = this.options.modeConfig?.network ?? req.policy?.networkAccess ?? mergedAdditional.network;
Because it uses the nullish coalescing operator (
??), ifthis.options.modeConfig?.networkisfalse(e.g., in a restrictive mode whereallowOverridesistrue), the expression evaluates tofalse. This completely ignoresmergedAdditional.network, breaking the ability to dynamically grant network access on Windows.
Suggestion: Update the logic to use a boolean OR to allow the override to take precedence over afalsedefault, similar to the MacOS sandbox implementation:const defaultNetwork = this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; const networkAccess = defaultNetwork || mergedAdditional.network;
-
packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts(Test Coverage Gap): The test caseshould handle persistent permissions from policyManagercreatesmodeConfig: { allowOverrides: true }without explicitly settingnetwork: false. Becausenetworkis leftundefined, the nullish coalescing bug mentioned above is not triggered, allowing the test to incorrectly pass.
Suggestion: Update the test'smodeConfigto explicitly includenetwork: false. This will ensure it properly asserts that an override can bypass a restrictive default. -
packages/core/src/sandbox/windows/WindowsSandboxManager.ts(UNC Path Rejection Bug): The attempt to prevent SMB credential theft and SSRF by blocking UNC paths is overly broad:if (resolvedPath.startsWith('\\\\')) { debugLogger.log('WindowsSandboxManager: Rejecting UNC path...'); return; }
While this successfully blocks
\\attacker\share\malicious, it also blocks legitimate Windows extended-length paths (\\?\C:\long\path\to\workspace) and local device paths (\\.\PhysicalDrive0). If a user's project is deeply nested and Node falls back to the\\?\prefix to bypassMAX_PATHlimitations, the sandbox will silently fail to grant Low Integrity write access, breaking builds entirely.
Suggestion: Refine the check to explicitly exclude the local extended-length and device path prefixes:if (resolvedPath.startsWith('\\\\') && !resolvedPath.startsWith('\\\\?\\') && !resolvedPath.startsWith('\\\\.\\')) { debugLogger.log('WindowsSandboxManager: Rejecting UNC path...'); return; }
-
packages/core/src/sandbox/windows/WindowsSandboxManager.ts(Incorrect Casting of Shell Parsing Objects): InsideisStrictlyApproved, the pipeline command parsing usesshellParsefollowed by a direct map toString:const parsedArgs = shellParse(cmdString).map(String); if (!isKnownSafeCommand(parsedArgs)) { ... }
The
shellParsemethod fromshell-quotereturns an array of entries, which can be standard strings but can also be objects for globs or operators (e.g., parsingls *.jsreturns['ls', { op: 'glob', pattern: '*' }]). Using.map(String)on these objects coerces them into the literal string"[object Object]". While this currently just safely fails theisKnownSafeCommandcheck, it's brittle and obfuscates what the command actually was.
Suggestion: Update the map function to properly extract the underlying string representation from operator/glob objects. (Note: SinceisStrictlyApprovedseems to be copied directly fromMacOSSandboxManager.ts, this bug exists there as well and should ideally be fixed in both places or moved to a shared utility).const parsedArgs = shellParse(cmdString).map(entry => typeof entry === 'string' ? entry : String(entry.pattern || entry.op || '') );
Nits (Low Priority/Suggestions)
packages/core/src/sandbox/windows/WindowsSandboxManager.ts: TheisStrictlyApprovedmethod appears to be a direct duplicate of the method found inMacOSSandboxManager.ts. Consider extracting this logic into a shared utility function (e.g., insandbox/utils/) to prevent code duplication.packages/core/src/sandbox/windows/WindowsSandboxManager.ts: ImportingisKnownSafeCommanddirectly from../macos/commandSafety.jsinside the Windows sandbox manager is an architectural smell. It would be better to movecommandSafety.tsto a shared location since it is now used by multiple platforms.
scidomino
force-pushed
the
tomm_win
branch
4 times, most recently
from
1045ea2 to
885d4a2
Compare
|
Addressed all comments. |
| return false; | ||
| } | ||
|
|
||
| isDangerousCommand(_args: string[]): boolean { |
This commit introduces the WindowsSandboxOptions interface to WindowsSandboxManager, passing down modeConfig and policyManager from the sandbox manager factory (Phase 1). It also evaluates and merges additionalPermissions with persistent policyManager approvals and global mode configurations, allowing for dynamic network access and Low Integrity file writes on Windows (Phase 2.1). Fixes #1601.
2 participants
Summary
This PR implements Phase 1 and 2.1 of dynamic sandbox expansion for the Windows platform. It allows the CLI to dynamically grant network access and Low Integrity filesystem write permissions based on global settings, persistent approvals, and just-in-time requests.
Details
WindowsSandboxOptionsinterface, updating the constructor to acceptmodeConfigandpolicyManager.sandboxManagerFactory.tsto pass these new configurations to theWindowsSandboxManager.prepareCommandto mergereq.policy.additionalPermissionswith persistent permissions frompolicyManagerandmodeConfig.commandSafety.tsto decouple the implementation from macOS-specific utilities.Related Issues
For google-gemini/maintainers-gemini-cli#1601 (Phase 1 & Phase 2.1)
How to Validate
Tests have been added to verify that the configurations merge correctly and trigger the expected
icaclscalls for Low Integrity access.Pre-Merge Checklist