Implementation of sandbox "Write-Protected" Governance Files#23139
Implementation of sandbox "Write-Protected" Governance Files#23139DavidAPierce merged 15 commits intomainfrom
Conversation
|
Hi @DavidAPierce, 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 integrity and security of sandbox environments by implementing robust write-protection for critical governance files, such as 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
The pull request successfully implements write-protection for governance files (.gitignore, .geminiignore) within the sandbox environments for both Linux (Bubblewrap) and macOS (Seatbelt). The introduction of the GOVERNANCE_FILES constant centralizes the definition of these protected files, enhancing maintainability. The changes are well-tested with updated unit tests for both platforms, ensuring the protection flags and rules are correctly applied. The overall implementation is robust and directly addresses the stated objective of securing these critical files.
Note: Security Review did not run due to the size of the PR.
|
Size Change: +3.11 kB (+0.01%) Total Size: 26.2 MB
ℹ️ View Unchanged
|
gemini-cli
bot
added
the
status/need-issue
galz10
left a comment
galz10
left a comment
There was a problem hiding this comment.
Thanks for putting this together! The approach of centralizing governance files and enforcing them at the OS level (via bwrap and sandbox-exec) is the right way to handle this.
However, there are a few critical edge cases (specifically around non-existent files and symlinks) that need to be addressed before we can merge this safely, along with some minor improvements.
🔴 Critical Concerns (Action Required)
-
Linux
bwrapvulnerability with non-existent files:
InLinuxSandboxManager.ts, using--ro-bind-trypresents a security gap. If.gitignoreor.geminiignoredoes not exist on the host beforebwrapis invoked,bwrapwill silently skip mounting them. Because the parentworkspacedirectory is mounted read-write, a sandboxed process can then successfully create and write to these governance files, completely bypassing the protection.- Suggested fix: Ensure the files exist on the host before configuring the
bwrapmounts (e.g., "touch" the file without altering existing content). Once you guarantee they exist, you should change--ro-bind-tryto--ro-bindso the sandbox explicitly fails if the protection cannot be applied.
- Suggested fix: Ensure the files exist on the host before configuring the
-
Symlink Bypass (Linux & macOS):
If a repository intentionally symlinks.gitignoreto another file within the workspace (e.g.,.gitignore -> config/shared.gitignore), the sandbox will protect the symlink itself, but the AI could write directly toconfig/shared.gitignorebecause the workspace is mounted read-write.- Suggested fix: Resolve the real path of the governance file (if it exists) before passing it to
bwrapor Seatbelt (e.g., usingfs.realpathSync), and apply the read-only/deny protection to both the symlink path and the resolved target path.
- Suggested fix: Resolve the real path of the governance file (if it exists) before passing it to
-
macOS Seatbelt Validation:
As noted in the PR checklist, macOS Seatbelt validation was not performed. Please manually verify on macOS to ensure thedenyrule properly prevents writes as expected (especially given the comment regarding rule evaluation order mentioned in the nits below).
🟡 Suggestions & Nits (Low Priority)
-
Type Safety (
sandboxManager.ts):
Make theGOVERNANCE_FILESarray immutable using TypeScript'sas constto prevent accidental mutation at runtime:export const GOVERNANCE_FILES = ['.gitignore', '.geminiignore'] as const;
-
Windows Implementation Tracking:
Since this PR covers Linux and macOS, please add aTODOcomment in the code (e.g., insandboxManager.ts) or create a follow-up child bug/issue to implement equivalent protection for Windows (e.g., via Restricted Tokens or ACLs) to ensure consistent security across all platforms. -
Protecting the
.gitdirectory:
While this PR specifically addresses*ignorefiles, we should ideally protect the entire.git/directory from being modified by the AI (preventing forced commits, history rewrites, or altering.git/info/exclude). Consider adding.git(or.git/) to theGOVERNANCE_FILESlist. Both Seatbelt'ssubpathdeny and bwrap's directory bind mounts support protecting directories. -
Inaccurate Comment (
seatbeltArgsBuilder.ts):
The comment on line 64 states:
// These are added before the workspace allow rule to ensure they take precedence.
This is factually incorrect.profileis initialized withBASE_SEATBELT_PROFILE(which already contains the workspaceallowrule at the very end), and thedenyrules are appended to it. Therefore, they are added after the allow rule. If Seatbelt evaluatesdenyrules correctly regardless of order, simply remove or correct the comment. If the order does matter, the profile construction logic will need to be refactored to inject the deny rules prior to the base profile.
|
Updated PR with the following: SummaryThis PR enhances the sandbox protection for repository governance files ( Details
Related IssuesRelated to #21807 (Native Windows sandboxing was recently added, but this PR focuses on cross-platform governance protection). How to ValidateRun the sandbox unit tests: npm test -w @google/gemini-cli-core -- src/sandbox/Verify that the tests for
Pre-Merge Checklist
|
|
Now that the windows implementation of sandboxing is merged, this TODO is stale. I will update this pr to address it. |
galz10
left a comment
galz10
left a comment
There was a problem hiding this comment.
Code Review
Scope: Specific Commit: 8d652b7
This commit introduces logic to write-protect governance files (like .git, .gitignore, .geminiignore) in the Windows Sandbox environment. It achieves this by ensuring these files exist with Medium integrity on the host before executing a command in the Low integrity sandbox, effectively making them read-only to sandboxed processes. Overall, the approach is clever and aligns well with Windows security principles, but there is a critical flaw regarding how file paths are resolved.
Metadata Review
The PR title cleanly captures the intent of the change. The description provides an excellent, comprehensive breakdown of the implementation details across different operating systems (Linux, macOS, Windows) and includes clear validation steps. The context is great.
Concerns (Action Required)
packages/core/src/services/windowsSandboxManager.ts:WindowsSandboxManagerusesreq.cwdto construct the file path forGOVERNANCE_FILES(const filePath = path.join(req.cwd, file);). Becausereq.cwdis the working directory of the specific command being executed (which could be any nested subdirectory within the workspace), this implementation will incorrectly create dummy.git,.gitignore, and.geminiignorefiles in whatever directory the sandboxed command is run, rather than protecting the actual files at the workspace root.
Suggestion:- Update the
WindowsSandboxManagerconstructor to accept aworkspace: stringparameter (similar to howLinuxSandboxManagerandMacOsSandboxManagerare instantiated). - Update
createSandboxManagerinsandboxManagerFactory.tsto pass theworkspaceparameter when instantiatingWindowsSandboxManager. - Replace
req.cwdwiththis.workspacewhen joining the governance file paths.
- Update the
Nits (Optional Improvements)
-
packages/core/src/services/windowsSandboxManager.ts: The directory check (const isDirectory = file === '.git';) is hardcoded. While this works for the current contents ofGOVERNANCE_FILES, it's brittle if future directories are added to the governance list.
Suggestion: Consider checking the actual filesystem if the file already exists (similar to the fallback logic handled inseatbeltArgsBuilder.tsfor macOS), or update theGOVERNANCE_FILESdefinition to be more explicit about which entries are directories. -
packages/core/src/services/windowsSandboxManager.ts: In thetouch()implementation, if a governance file happens to be a broken symlink (points to a non-existent file in a non-existent directory),fs.existsSync(filePath)returnsfalse. The code then ensures the symlink's parent directory exists and callsfs.openSync(filePath, 'a'). This will follow the symlink and attempt to create the file at the target, which will throw anENOENTerror if the target's parent directory does not exist.
Suggestion: This is a rare edge case, but usingfs.lstatSyncinside atry/catchto check if it's a broken symlink before trying to create it might be safer to prevent unexpected sandbox crashes.
|
Merged with head and addressed previous comment. Here is a breakdown of what was done:
|
…gemini#23139) Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com>
4 participants
Summary
Added a governance files variable to the sandbox manager that should be used as a source of truth in operating system sandboxing to set read only status on gitignore and geminiignore files.
Details
workspace itself is mounted as read-write.
Related Issues
Fixes https://github.com/google-gemini/maintainers-gemini-cli/issues/1559
How to Validate
Pre-Merge Checklist