From 370c45de67bb5e465005d5e32cd9342e7d388ac9 Mon Sep 17 00:00:00 2001 From: Emily Hedlund Date: Fri, 3 Apr 2026 13:54:48 -0700 Subject: [PATCH] fix(core): improve windows sandbox reliability and fix integration tests (#24480) --- .../core/src/sandbox/windows/GeminiSandbox.cs | 87 ++++++++---- .../windows/WindowsSandboxManager.test.ts | 126 ++++++++++++------ .../sandbox/windows/WindowsSandboxManager.ts | 78 ++++++----- .../sandboxManager.integration.test.ts | 85 +++++++----- 4 files changed, 242 insertions(+), 134 deletions(-) diff --git a/packages/core/src/sandbox/windows/GeminiSandbox.cs b/packages/core/src/sandbox/windows/GeminiSandbox.cs index 6275b701c4..eef08b250b 100644 --- a/packages/core/src/sandbox/windows/GeminiSandbox.cs +++ b/packages/core/src/sandbox/windows/GeminiSandbox.cs @@ -21,6 +21,8 @@ using System.Text; */ public class GeminiSandbox { // P/Invoke constants and structures + private const int JobObjectExtendedLimitInformation = 9; + private const int JobObjectNetRateControlInformation = 32; private const uint JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE = 0x00002000; private const uint JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION = 0x00000400; private const uint JOB_OBJECT_LIMIT_ACTIVE_PROCESS = 0x00000008; @@ -74,6 +76,9 @@ public class GeminiSandbox { [DllImport("kernel32.dll", SetLastError = true)] static extern bool AssignProcessToJobObject(IntPtr hJob, IntPtr hProcess); + [DllImport("kernel32.dll", SetLastError = true)] + static extern uint ResumeThread(IntPtr hThread); + [DllImport("advapi32.dll", SetLastError = true)] static extern bool OpenProcessToken(IntPtr ProcessHandle, uint DesiredAccess, out IntPtr TokenHandle); @@ -191,7 +196,8 @@ public class GeminiSandbox { IntPtr hToken = IntPtr.Zero; IntPtr hRestrictedToken = IntPtr.Zero; - IntPtr lowIntegritySid = IntPtr.Zero; + IntPtr hJob = IntPtr.Zero; + PROCESS_INFORMATION pi = new PROCESS_INFORMATION(); try { // 1. Duplicate Primary Token @@ -208,6 +214,7 @@ public class GeminiSandbox { // 2. Lower Integrity Level to Low // S-1-16-4096 is the SID for "Low Mandatory Level" + IntPtr lowIntegritySid = IntPtr.Zero; if (ConvertStringSidToSid("S-1-16-4096", out lowIntegritySid)) { TOKEN_MANDATORY_LABEL tml = new TOKEN_MANDATORY_LABEL(); tml.Label.Sid = lowIntegritySid; @@ -226,25 +233,42 @@ public class GeminiSandbox { } // 3. Setup Job Object for cleanup - IntPtr hJob = CreateJobObject(IntPtr.Zero, null); + hJob = CreateJobObject(IntPtr.Zero, null); + if (hJob == IntPtr.Zero) { + Console.Error.WriteLine("Error: CreateJobObject failed (" + Marshal.GetLastWin32Error() + ")"); + return 1; + } + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimits = new JOBOBJECT_EXTENDED_LIMIT_INFORMATION(); jobLimits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION; - + IntPtr lpJobLimits = Marshal.AllocHGlobal(Marshal.SizeOf(jobLimits)); - Marshal.StructureToPtr(jobLimits, lpJobLimits, false); - SetInformationJobObject(hJob, 9 /* JobObjectExtendedLimitInformation */, lpJobLimits, (uint)Marshal.SizeOf(jobLimits)); - Marshal.FreeHGlobal(lpJobLimits); + try { + Marshal.StructureToPtr(jobLimits, lpJobLimits, false); + if (!SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, lpJobLimits, (uint)Marshal.SizeOf(jobLimits))) { + Console.Error.WriteLine("Error: SetInformationJobObject(Limits) failed (" + Marshal.GetLastWin32Error() + ")"); + return 1; + } + } finally { + Marshal.FreeHGlobal(lpJobLimits); + } if (!networkAccess) { JOBOBJECT_NET_RATE_CONTROL_INFORMATION netLimits = new JOBOBJECT_NET_RATE_CONTROL_INFORMATION(); netLimits.MaxBandwidth = 1; netLimits.ControlFlags = 0x1 | 0x2; // ENABLE | MAX_BANDWIDTH netLimits.DscpTag = 0; - + IntPtr lpNetLimits = Marshal.AllocHGlobal(Marshal.SizeOf(netLimits)); - Marshal.StructureToPtr(netLimits, lpNetLimits, false); - SetInformationJobObject(hJob, 32 /* JobObjectNetRateControlInformation */, lpNetLimits, (uint)Marshal.SizeOf(netLimits)); - Marshal.FreeHGlobal(lpNetLimits); + try { + Marshal.StructureToPtr(netLimits, lpNetLimits, false); + if (!SetInformationJobObject(hJob, JobObjectNetRateControlInformation, lpNetLimits, (uint)Marshal.SizeOf(netLimits))) { + // Some versions of Windows might not support network rate control, but we should know if it fails. + Console.Error.WriteLine("Warning: SetInformationJobObject(NetRate) failed (" + Marshal.GetLastWin32Error() + "). Network might not be throttled."); + } + } finally { + Marshal.FreeHGlobal(lpNetLimits); + } } // 4. Handle Internal Commands or External Process @@ -310,32 +334,49 @@ public class GeminiSandbox { commandLine += QuoteArgument(args[i]); } - PROCESS_INFORMATION pi = new PROCESS_INFORMATION(); - // Creation Flags: 0x04000000 (CREATE_BREAKAWAY_FROM_JOB) to allow job assignment if parent is in job - uint creationFlags = 0; + // Creation Flags: 0x01000000 (CREATE_BREAKAWAY_FROM_JOB) to allow job assignment if parent is in job + // 0x00000004 (CREATE_SUSPENDED) to prevent the process from executing before being placed in the job + uint creationFlags = 0x01000000 | 0x00000004; if (!CreateProcessAsUser(hRestrictedToken, null, commandLine, IntPtr.Zero, IntPtr.Zero, true, creationFlags, IntPtr.Zero, cwd, ref si, out pi)) { - Console.WriteLine("Error: CreateProcessAsUser failed (" + Marshal.GetLastWin32Error() + ") Command: " + commandLine); + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: CreateProcessAsUser failed (" + err + ") Command: " + commandLine); return 1; } - AssignProcessToJobObject(hJob, pi.hProcess); - - // Wait for exit - uint waitResult = WaitForSingleObject(pi.hProcess, 0xFFFFFFFF); - uint exitCode = 0; - GetExitCodeProcess(pi.hProcess, out exitCode); + if (!AssignProcessToJobObject(hJob, pi.hProcess)) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: AssignProcessToJobObject failed (" + err + ") Command: " + commandLine); + TerminateProcess(pi.hProcess, 1); + return 1; + } - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - CloseHandle(hJob); + ResumeThread(pi.hThread); + + if (WaitForSingleObject(pi.hProcess, 0xFFFFFFFF) == 0xFFFFFFFF) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: WaitForSingleObject failed (" + err + ")"); + } + + uint exitCode = 0; + if (!GetExitCodeProcess(pi.hProcess, out exitCode)) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: GetExitCodeProcess failed (" + err + ")"); + return 1; + } return (int)exitCode; } finally { if (hToken != IntPtr.Zero) CloseHandle(hToken); if (hRestrictedToken != IntPtr.Zero) CloseHandle(hRestrictedToken); + if (hJob != IntPtr.Zero) CloseHandle(hJob); + if (pi.hProcess != IntPtr.Zero) CloseHandle(pi.hProcess); + if (pi.hThread != IntPtr.Zero) CloseHandle(pi.hThread); } } + [DllImport("kernel32.dll", SetLastError = true)] + static extern bool TerminateProcess(IntPtr hProcess, uint uExitCode); + [DllImport("kernel32.dll", SetLastError = true)] static extern uint WaitForSingleObject(IntPtr hHandle, uint dwMilliseconds); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 8b38179177..c814f740f7 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -25,17 +25,40 @@ vi.mock('../../utils/shell-utils.js', async (importOriginal) => { }; }); -// TODO: reenable once test is fixed -describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { +describe('WindowsSandboxManager', () => { let manager: WindowsSandboxManager; let testCwd: string; + /** + * Creates a temporary directory and returns its canonical real path. + */ + function createTempDir(name: string, parent = os.tmpdir()): string { + const rawPath = fs.mkdtempSync(path.join(parent, `gemini-test-${name}-`)); + return fs.realpathSync(rawPath); + } + + const helperExePath = path.resolve( + __dirname, + WindowsSandboxManager.HELPER_EXE, + ); + beforeEach(() => { vi.spyOn(os, 'platform').mockReturnValue('win32'); vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p.toString(), ); - testCwd = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-cli-test-')); + + // Mock existsSync to skip the csc.exe auto-compilation of helper during unit tests. + const originalExistsSync = fs.existsSync; + vi.spyOn(fs, 'existsSync').mockImplementation((p) => { + if (typeof p === 'string' && path.resolve(p) === helperExePath) { + return true; + } + return originalExistsSync(p); + }); + + testCwd = createTempDir('cwd'); + manager = new WindowsSandboxManager({ workspace: testCwd, modeConfig: { readonly: false, allowOverrides: true }, @@ -45,7 +68,9 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { afterEach(() => { vi.restoreAllMocks(); - fs.rmSync(testCwd, { recursive: true, force: true }); + if (testCwd && fs.existsSync(testCwd)) { + fs.rmSync(testCwd, { recursive: true, force: true }); + } }); it('should prepare a GeminiSandbox.exe command', async () => { @@ -155,8 +180,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should handle persistent permissions from policyManager', async () => { - const persistentPath = path.join(testCwd, 'persistent_path'); - fs.mkdirSync(persistentPath, { recursive: true }); + const persistentPath = createTempDir('persistent', testCwd); const mockPolicyManager = { getCommandPermissions: vi.fn().mockReturnValue({ @@ -189,6 +213,8 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(icaclsArgs).toContainEqual([ persistentPath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -234,10 +260,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should grant Low Integrity access to the workspace and allowed paths', async () => { - const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); - if (!fs.existsSync(allowedPath)) { - fs.mkdirSync(allowedPath); - } + const allowedPath = createTempDir('allowed'); try { const req: SandboxRequest = { command: 'test', @@ -257,13 +280,17 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - path.resolve(testCwd), + testCwd, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); expect(icaclsArgs).toContainEqual([ - path.resolve(allowedPath), + allowedPath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -273,13 +300,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should grant Low Integrity access to additional write paths', async () => { - const extraWritePath = path.join( - os.tmpdir(), - 'gemini-cli-test-extra-write', - ); - if (!fs.existsSync(extraWritePath)) { - fs.mkdirSync(extraWritePath); - } + const extraWritePath = createTempDir('extra-write'); try { const req: SandboxRequest = { command: 'test', @@ -303,7 +324,9 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - path.resolve(extraWritePath), + extraWritePath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -330,26 +353,26 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }, }; - await manager.prepareCommand(req); + // Rejected because it's an unreachable/invalid UNC path or it doesn't exist + await expect(manager.prepareCommand(req)).rejects.toThrow(); const icaclsArgs = vi .mocked(spawnAsync) .mock.calls.filter((c) => c[0] === 'icacls') .map((c) => c[1]); - expect(icaclsArgs).not.toContainEqual([ - uncPath, - '/setintegritylevel', - '(OI)(CI)Low', - ]); + expect(icaclsArgs).not.toContainEqual(expect.arrayContaining([uncPath])); }, ); it.runIf(process.platform === 'win32')( 'should allow extended-length and local device paths', async () => { - const longPath = '\\\\?\\C:\\very\\long\\path'; - const devicePath = '\\\\.\\PhysicalDrive0'; + // Create actual files for inheritance/existence checks + const longPath = path.join(testCwd, 'very_long_path.txt'); + const devicePath = path.join(testCwd, 'device_path.txt'); + fs.writeFileSync(longPath, ''); + fs.writeFileSync(devicePath, ''); const req: SandboxRequest = { command: 'test', @@ -373,12 +396,16 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - longPath, + path.resolve(longPath), + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); expect(icaclsArgs).toContainEqual([ - devicePath, + path.resolve(devicePath), + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -420,10 +447,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should deny Low Integrity access to forbidden paths', async () => { - const forbiddenPath = path.join(os.tmpdir(), 'gemini-cli-test-forbidden'); - if (!fs.existsSync(forbiddenPath)) { - fs.mkdirSync(forbiddenPath); - } + const forbiddenPath = createTempDir('forbidden'); try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, @@ -440,7 +464,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { await managerWithForbidden.prepareCommand(req); expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(forbiddenPath), + forbiddenPath, '/deny', '*S-1-16-4096:(OI)(CI)(F)', ]); @@ -450,10 +474,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should override allowed paths if a path is also in forbidden paths', async () => { - const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); - if (!fs.existsSync(conflictPath)) { - fs.mkdirSync(conflictPath); - } + const conflictPath = createTempDir('conflict'); try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, @@ -478,14 +499,14 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { call[1] && call[1].includes('/setintegritylevel') && call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), + call[1][0] === conflictPath, ); const denyCallIndex = spawnMock.mock.calls.findIndex( (call) => call[1] && call[1].includes('/deny') && call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), + call[1][0] === conflictPath, ); // Conflict should have been filtered out of allow calls @@ -513,8 +534,8 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(result.args[5]).toBe(filePath); }); - it('should safely handle special characters in __write path', async () => { - const maliciousPath = path.join(testCwd, 'foo"; echo bar; ".txt'); + it('should safely handle special characters in __write path using environment variables', async () => { + const maliciousPath = path.join(testCwd, 'foo & echo bar; ! .txt'); fs.writeFileSync(maliciousPath, ''); const req: SandboxRequest = { command: '__write', @@ -545,4 +566,23 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(result.args[4]).toBe('__read'); expect(result.args[5]).toBe(filePath); }); + + it('should return a cleanup function that deletes the temporary manifest', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + }; + + const result = await manager.prepareCommand(req); + const manifestPath = result.args[3]; + + expect(fs.existsSync(manifestPath)).toBe(true); + expect(result.cleanup).toBeDefined(); + + result.cleanup?.(); + expect(fs.existsSync(manifestPath)).toBe(false); + expect(fs.existsSync(path.dirname(manifestPath))).toBe(false); + }); }); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 3328c2b918..3cfb85b36a 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -16,7 +16,6 @@ import { findSecretFiles, type GlobalSandboxOptions, sanitizePaths, - tryRealpath, type SandboxPermissions, type ParsedSandboxDenial, resolveSandboxPaths, @@ -36,23 +35,28 @@ import { } from './commandSafety.js'; import { verifySandboxOverrides } from '../utils/commandUtils.js'; import { parseWindowsSandboxDenials } from './windowsSandboxDenialUtils.js'; +import { isSubpath, resolveToRealPath } from '../../utils/paths.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); +// S-1-16-4096 is the SID for "Low Mandatory Level" (Low Integrity) +const LOW_INTEGRITY_SID = '*S-1-16-4096'; + /** * A SandboxManager implementation for Windows that uses Restricted Tokens, * Job Objects, and Low Integrity levels for process isolation. * Uses a native C# helper to bypass PowerShell restrictions. */ export class WindowsSandboxManager implements SandboxManager { + static readonly HELPER_EXE = 'GeminiSandbox.exe'; private readonly helperPath: string; private initialized = false; private readonly allowedCache = new Set(); private readonly deniedCache = new Set(); constructor(private readonly options: GlobalSandboxOptions) { - this.helperPath = path.resolve(__dirname, 'GeminiSandbox.exe'); + this.helperPath = path.resolve(__dirname, WindowsSandboxManager.HELPER_EXE); } isKnownSafeCommand(args: string[]): boolean { @@ -259,9 +263,14 @@ export class WindowsSandboxManager implements SandboxManager { this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; const networkAccess = defaultNetwork || mergedAdditional.network; - // 1. Handle filesystem permissions for Low Integrity - // Grant "Low Mandatory Level" write access to the workspace. - // If not in readonly mode OR it's a strictly approved pipeline, allow workspace writes + const { allowed: allowedPaths, forbidden: forbiddenPaths } = + await resolveSandboxPaths(this.options, req); + + // Track all roots where Low Integrity write access has been granted. + // New files created within these roots will inherit the Low label. + const writableRoots: string[] = []; + + // 1. Workspace access const isApproved = allowOverrides ? await isStrictlyApproved( command, @@ -272,20 +281,19 @@ export class WindowsSandboxManager implements SandboxManager { if (!isReadonlyMode || isApproved) { await this.grantLowIntegrityAccess(this.options.workspace); + writableRoots.push(this.options.workspace); } - const { allowed: allowedPaths, forbidden: forbiddenPaths } = - await resolveSandboxPaths(this.options, req); - - // Grant "Low Mandatory Level" access to includeDirectories. + // 2. Globally included directories const includeDirs = sanitizePaths(this.options.includeDirectories); for (const includeDir of includeDirs) { await this.grantLowIntegrityAccess(includeDir); + writableRoots.push(includeDir); } - // Grant "Low Mandatory Level" read/write access to allowedPaths. + // 3. Explicitly allowed paths from the request policy for (const allowedPath of allowedPaths) { - const resolved = await tryRealpath(allowedPath); + const resolved = resolveToRealPath(allowedPath); try { await fs.promises.access(resolved, fs.constants.F_OK); } catch { @@ -295,23 +303,32 @@ export class WindowsSandboxManager implements SandboxManager { ); } await this.grantLowIntegrityAccess(resolved); + writableRoots.push(resolved); } - // Grant "Low Mandatory Level" write access to additional permissions write paths. + // 4. Additional write paths (e.g. from internal __write command) const additionalWritePaths = sanitizePaths( mergedAdditional.fileSystem?.write, ); for (const writePath of additionalWritePaths) { - const resolved = await tryRealpath(writePath); + const resolved = resolveToRealPath(writePath); try { await fs.promises.access(resolved, fs.constants.F_OK); + await this.grantLowIntegrityAccess(resolved); + continue; } catch { - throw new Error( - `Sandbox request rejected: Additional write path does not exist: ${resolved}. ` + - 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', + // If the file doesn't exist, it's only allowed if it resides within a granted root. + const isInherited = writableRoots.some((root) => + isSubpath(root, resolved), ); + + if (!isInherited) { + throw new Error( + `Sandbox request rejected: Additional write path does not exist and its parent directory is not allowed: ${resolved}. ` + + 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', + ); + } } - await this.grantLowIntegrityAccess(resolved); } // 2. Collect secret files and apply protective ACLs @@ -382,15 +399,6 @@ export class WindowsSandboxManager implements SandboxManager { const manifestPath = path.join(tempDir, 'manifest.txt'); fs.writeFileSync(manifestPath, allForbidden.join('\n')); - // Cleanup on exit - process.on('exit', () => { - try { - fs.rmSync(tempDir, { recursive: true, force: true }); - } catch { - // Ignore errors - } - }); - // 5. Construct the helper command // GeminiSandbox.exe --forbidden-manifest [args...] const program = this.helperPath; @@ -411,6 +419,13 @@ export class WindowsSandboxManager implements SandboxManager { args: finalArgs, env: finalEnv, cwd: req.cwd, + cleanup: () => { + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch { + // Ignore errors + } + }, }; } @@ -422,7 +437,7 @@ export class WindowsSandboxManager implements SandboxManager { return; } - const resolvedPath = await tryRealpath(targetPath); + const resolvedPath = resolveToRealPath(targetPath); if (this.allowedCache.has(resolvedPath)) { return; } @@ -446,8 +461,12 @@ export class WindowsSandboxManager implements SandboxManager { } try { + // 1. Grant explicit Modify access to the Low Integrity SID + // 2. Set the Mandatory Label to Low to allow "Write Up" from Low processes await spawnAsync('icacls', [ resolvedPath, + '/grant', + `${LOW_INTEGRITY_SID}:(OI)(CI)(M)`, '/setintegritylevel', '(OI)(CI)Low', ]); @@ -469,7 +488,7 @@ export class WindowsSandboxManager implements SandboxManager { return; } - const resolvedPath = await tryRealpath(targetPath); + const resolvedPath = resolveToRealPath(targetPath); if (this.deniedCache.has(resolvedPath)) { return; } @@ -479,9 +498,6 @@ export class WindowsSandboxManager implements SandboxManager { return; } - // S-1-16-4096 is the SID for "Low Mandatory Level" (Low Integrity) - const LOW_INTEGRITY_SID = '*S-1-16-4096'; - // icacls flags: (OI) Object Inherit, (CI) Container Inherit, (F) Full Access Deny. // Omit /T (recursive) for performance; (OI)(CI) ensures inheritance for new items. // Windows dynamically evaluates existing items, though deep explicit Allow ACEs diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index 524726cdd4..1cd1e77269 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -28,7 +28,14 @@ const Platform = { /** Returns a command to create an empty file. */ touch(filePath: string) { return this.isWindows - ? { command: 'cmd.exe', args: ['/c', `type nul > "${filePath}"`] } + ? { + command: 'powershell.exe', + args: [ + '-NoProfile', + '-Command', + `New-Item -Path "${filePath}" -ItemType File -Force`, + ], + } : { command: 'touch', args: [filePath] }; }, @@ -48,18 +55,13 @@ const Platform = { /** Returns a command to perform a network request. */ curl(url: string) { - return this.isWindows - ? { - command: 'powershell.exe', - args: ['-Command', `Invoke-WebRequest -Uri ${url} -TimeoutSec 1`], - } - : { command: 'curl', args: ['-s', '--connect-timeout', '1', url] }; + return { command: 'curl', args: ['-s', '--connect-timeout', '1', url] }; }, /** Returns a command that checks if the current terminal is interactive. */ isPty() { return this.isWindows - ? 'cmd.exe /c echo True' + ? 'powershell.exe -NoProfile -Command "echo True"' : 'bash -c "if [ -t 1 ]; then echo True; else echo False; fi"'; }, @@ -103,8 +105,7 @@ function ensureSandboxAvailable(): boolean { if (platform === 'win32') { // Windows sandboxing relies on icacls, which is a core system utility and // always available. - // TODO: reenable once test is fixed - return false; + return true; } if (platform === 'darwin') { @@ -167,23 +168,28 @@ describe('SandboxManager Integration', () => { expect(result.stdout.trim()).toBe('sandbox test'); }); - it('supports interactive pseudo-terminals (node-pty)', async () => { - const handle = await ShellExecutionService.execute( - Platform.isPty(), - workspace, - () => {}, - new AbortController().signal, - true, - { - sanitizationConfig: getSecureSanitizationConfig(), - sandboxManager: manager, - }, - ); + // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes + // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. + it.skipIf(Platform.isWindows)( + 'supports interactive pseudo-terminals (node-pty)', + async () => { + const handle = await ShellExecutionService.execute( + Platform.isPty(), + workspace, + () => {}, + new AbortController().signal, + true, + { + sanitizationConfig: getSecureSanitizationConfig(), + sandboxManager: manager, + }, + ); - const result = await handle.result; - expect(result.exitCode).toBe(0); - expect(result.output).toContain('True'); - }); + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(result.output).toContain('True'); + }, + ); }); describe('File System Access', () => { @@ -511,18 +517,23 @@ describe('SandboxManager Integration', () => { if (server) await new Promise((res) => server.close(() => res())); }); - it('blocks network access by default', async () => { - const { command, args } = Platform.curl(url); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); + // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, + // so this test cannot verify loopback blocking on Windows. + it.skipIf(Platform.isWindows)( + 'blocks network access by default', + async () => { + const { command, args } = Platform.curl(url); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); - }); + const result = await runCommand(sandboxed); + expect(result.status).not.toBe(0); + }, + ); it('grants network access when explicitly allowed', async () => { const { command, args } = Platform.curl(url);