feature: allow absolute path for macos seatbelt profiles (sandbox)#25013
feature: allow absolute path for macos seatbelt profiles (sandbox)#25013flexponsive wants to merge 11 commits intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 flexibility of macOS Seatbelt configurations by allowing 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
|
![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 adds support for absolute paths in the SEATBELT_PROFILE environment variable within the macOS sandbox utility, along with a corresponding test case. The review feedback suggests enhancing security by trimming and sanitizing the profile path to prevent potential path traversal vulnerabilities. Additionally, it notes a violation of the repository's testing style guide, recommending the use of vi.stubEnv() for environment variable manipulation in tests to avoid state leakage.
| let profileFile; | ||
| if (path.isAbsolute(profile)) { | ||
| profileFile = profile; | ||
| } else { | ||
| profileFile = fileURLToPath( | ||
| new URL(`sandbox-macos-${profile}.sb`, import.meta.url), | ||
| ); | ||
| // if profile name is not recognized, then look for file under project settings directory | ||
| if (!BUILTIN_SEATBELT_PROFILES.includes(profile)) { | ||
| profileFile = path.join(GEMINI_DIR, `sandbox-macos-${profile}.sb`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The SEATBELT_PROFILE value should be trimmed to ensure that absolute paths with leading or trailing whitespace are correctly identified by path.isAbsolute(). Additionally, when a relative path or name is provided, it must be sanitized to prevent path traversal (e.g., by checking for .. or null bytes), as per the repository's security guidelines for handling untrusted input.
const trimmedProfile = profile.trim();
let profileFile;
if (path.isAbsolute(trimmedProfile)) {
profileFile = trimmedProfile;
} else {
if (trimmedProfile.includes('..') || trimmedProfile.includes('\\0')) {
throw new FatalSandboxError('Invalid seatbelt profile name: ' + trimmedProfile);
}
profileFile = fileURLToPath(
new URL('sandbox-macos-' + trimmedProfile + '.sb', import.meta.url),
);
if (!BUILTIN_SEATBELT_PROFILES.includes(trimmedProfile)) {
profileFile = path.join(GEMINI_DIR, 'sandbox-macos-' + trimmedProfile + '.sb');
}
}References
- Sanitize file paths extracted from untrusted sources, such as command output, to prevent path traversal (..), null byte injection (\0), and other vulnerabilities.
- When validating string parameters from tools, trim the string first and then check for emptiness to prevent whitespace-only values from being accepted.
|
|
||
| it('should handle absolute path for SEATBELT_PROFILE', async () => { | ||
| vi.mocked(os.platform).mockReturnValue('darwin'); | ||
| process.env['SEATBELT_PROFILE'] = '/absolute/path/to/profile.sb'; |
There was a problem hiding this comment.
Direct modification of process.env is discouraged in tests as it can lead to state leakage between test cases. Use vi.stubEnv() instead, as specified in the repository style guide. Note that vi.unstubAllEnvs() should also be called in afterEach to ensure proper cleanup.
| process.env['SEATBELT_PROFILE'] = '/absolute/path/to/profile.sb'; | |
| vi.stubEnv('SEATBELT_PROFILE', '/absolute/path/to/profile.sb'); |
References
- When testing code that depends on environment variables, use vi.stubEnv('NAME', 'value') in beforeEach and vi.unstubAllEnvs() in afterEach. Avoid modifying process.env directly as it can lead to test leakage and is less reliable. (link)
There was a problem hiding this comment.
Thank you for the feedback. I've noted that sandbox.test.ts currently uses direct process.env modification across 10 or so existing test cases. To avoid introducing inconsistent patterns within the same file, I followed the local convention for the new test.
Please advise on your preferred path:
- Holistic Refactor: Update the entire file to use
vi.stubEnv()andvi.unstubAllEnvswithin this PR. (risk of losing focus) - Surgical Update: Only update the newly added test case (accepting local inconsistency).
- Follow-up PR: Keep this PR focused on the feature and address the test debt in a dedicated cleanup task.
gemini-cli
Bot
added
the
area/core
flexponsive
force-pushed
the
fix-absolute-seatbelt-profile
branch
from
ac65a94 to
40499fb
Compare
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
flexponsive
force-pushed
the
fix-absolute-seatbelt-profile
branch
from
40499fb to
b256f1a
Compare
|
closed due to problem with email verification for CLA bot, created new clean PR |
|
Stop
…On Thu, Apr 9, 2026, 8:52 AM github-actions[bot] ***@***.***> wrote:
*github-actions[bot]* left a comment (google-gemini/gemini-cli#25013)
<#25013 (comment)>
🛑 Action Required: Evaluation Approval
Steering changes have been detected in this PR. To prevent regressions, a
maintainer must approve the evaluation run before this PR can be merged.
*Maintainers:*
1. Go to the *Workflow Run Summary*
<https://github.com/google-gemini/gemini-cli/actions/runs/24191095876>.
2. Click the yellow *'Review deployments'* button.
3. Select the *'eval-gate'* environment and click *'Approve'*.
Once approved, the evaluation results will be posted here automatically.
—
Reply to this email directly, view it on GitHub
<#25013 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A5M6X26PTIIF3VSG3H4DIOD4U6MIPAVCNFSM6AAAAACXSHPDW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DEMJUGM2TANBSGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
10 participants
Summary
Enhance macOS Seatbelt configuration flexibility by allowing the
SEATBELT_PROFILEenvironment variable to accept absolute paths. This enables developers to maintain centralized sandbox policies across multiple repositories without duplicating configuration into every project's.geminifolder.Details
Updated the sandbox utility to check if the
SEATBELT_PROFILEvalue is an absolute path..geminifolder).This change is non-breaking and preserves all existing workflows.
Related Issues
Closes #24991
How to Validate
GEMINI_SANDBOX=sandbox-exec SEATBELT_PROFILE=$(pwd)/bundle/sandbox-macos-permissive-open.sb node bundle/gemini.js -sPre-Merge Checklist