From 8c1b5d3a77eaac42148b6be604925ffbb58e003b Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 12:17:04 +0000 Subject: [PATCH 1/4] linux: make the read-deny glob walk about six times faster A denyRead pattern with `**` is expanded by listing everything under its base, at every command and on the caller's thread. Under a large directory that holds the caller up for seconds, once per pattern. For every entry the walk spelled two paths and converted one before it asked whether the entry matched. A pattern that splits into path components is matched by name, so a plain file it does not match, which is nearly every entry of a large tree and which nothing further on records, now costs no path at all. The walk already kept each directory's listing for a second position to read. The patterns of one read configuration now share that map, so those with a base in common list each directory once between them. The map lives for one expansion: the next command lists afresh. Six patterns over 400,000 files in 20,000 directories: one wrap 6.2 s before, 0.97 s after; resident memory 469 MB before, 286 MB after; the same mounts. --- src/sandbox/read-deny-glob.ts | 4 ++ src/sandbox/sandbox-manager.ts | 17 ++++++- src/sandbox/sandbox-utils.ts | 22 ++++++-- test/sandbox/glob-expand.test.ts | 87 ++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 5 deletions(-) diff --git a/src/sandbox/read-deny-glob.ts b/src/sandbox/read-deny-glob.ts index 342805143..0a1d7915c 100644 --- a/src/sandbox/read-deny-glob.ts +++ b/src/sandbox/read-deny-glob.ts @@ -1,5 +1,6 @@ import { logForDebugging } from '../utils/debug.js' import { + type GlobWalkListings, isAtOrUnder, normalizePathForSandbox, pathSpellings, @@ -70,15 +71,18 @@ function collapseReadDenyLocations({ * covering it. The Linux wrapper binds nothing back beneath one: what the * pattern matches under an allowed path in there was never found, and would * come back unmasked. + * @param listings - one map for all the patterns of a configuration. */ export function expandReadDenyGlobLinux( globPattern: string, reExposedPaths: readonly string[], unlistableDirs?: Set, + listings?: GlobWalkListings, ): string[] { const walk = walkGlobPattern(globPattern, { withDirectoryForm: true, followSymlinkedDirectories: true, + listings, }) // Where a path the walk reported really lives: the denyRead loop mounts an // entry there, whatever spelling named it. diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 91daa448a..b8e6c8611 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -85,6 +85,7 @@ import { normalizePathForSandbox, removeTrailingGlobSuffix, expandGlobPattern, + type GlobWalkListings, attributionKeyFor, decodeSandboxedCommand, encodeSandboxedCommand, @@ -1331,10 +1332,16 @@ function getFsReadConfig(): FsReadRestrictionConfig { ) const reExposedPaths = [...allowPaths, ...getFsWriteConfig().allowOnly] const unlistableDenyDirs = new Set() + const listings: GlobWalkListings = new Map() const denyPaths = resolveReadPathEntries( unionDenyReadPaths(config.filesystem.denyRead, credentialRestrictions), pattern => - expandReadDenyGlobLinux(pattern, reExposedPaths, unlistableDenyDirs), + expandReadDenyGlobLinux( + pattern, + reExposedPaths, + unlistableDenyDirs, + listings, + ), credentialRestrictions.degradeToDenyPaths, ) @@ -1717,13 +1724,19 @@ async function wrapWithSandbox( } const reExposedPaths = [...expandedAllowRead, ...writeConfig.allowOnly] const unlistableDenyDirs = new Set() + const listings: GlobWalkListings = new Map() const expandedDenyRead = resolveReadPathEntries( unionDenyReadPaths( customConfig?.filesystem?.denyRead ?? config?.filesystem.denyRead ?? [], credentialRestrictions, ), pattern => - expandReadDenyGlobLinux(pattern, reExposedPaths, unlistableDenyDirs), + expandReadDenyGlobLinux( + pattern, + reExposedPaths, + unlistableDenyDirs, + listings, + ), credentialRestrictions.degradeToDenyPaths, ) readConfig = { diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 23859d190..7fa101423 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1137,6 +1137,10 @@ export interface ExpandGlobOptions { caseInsensitive?: boolean } +/** Successful listings, by real directory: a second position, or a second + * pattern, reads the same entries. */ +export type GlobWalkListings = Map + /** What one recursive walk of a glob's base directory found; see {@link walkGlobPattern}. */ export interface GlobWalk { /** Where the walk started, with symlinks resolved (the spelling itself @@ -1488,6 +1492,9 @@ export function walkGlobPattern( opts: ExpandGlobOptions & { withDirectoryForm?: boolean followSymlinkedDirectories?: boolean + /** Handed to every walk of one configuration, so that patterns with a + * base in common list each directory once between them. */ + listings?: GlobWalkListings } = {}, ): GlobWalk { const walk: GlobWalk = { @@ -1550,9 +1557,7 @@ export function walkGlobPattern( } /** The positions each real directory has been listed for. */ const listedFor = new Map>() - /** Successful listings, by real directory: a second position reads the - * same entries. */ - const listings = new Map() + const listings: GlobWalkListings = opts.listings ?? new Map() const pending: Frame[] = [] /** A filesystem call on a real path, and on a shorter name for it when that * fails. The real path crosses no link, so a long chain of them cannot @@ -1636,6 +1641,17 @@ export function walkGlobPattern( continue } for (const entry of entries) { + // Nearly every entry of a large tree: a plain file the pattern does not + // match, which nothing below records. A pattern that splits is matched + // by name, so no path need be spelled to find that out. + if ( + positions.splits && + !entry.isDirectory() && + !entry.isSymbolicLink() && + !positions.matches(fresh, entry.name, '') + ) { + continue + } const fullPath = path.join(dir, entry.name) const realPath = path.join(real, entry.name) const candidate = toForwardSlashes(fullPath) diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index ec0625c9f..9f0d90ef8 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -595,6 +595,46 @@ describe.if(!isWindows)('walkGlobPattern', () => { } }) + it('lists a directory once for all the patterns handed the same listings', () => { + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-shared-'))) + try { + mkdirSync(join(root, 'a', 'b'), { recursive: true }) + writeFileSync(join(root, 'a', '.env'), '') + writeFileSync(join(root, 'a', 'b', 'id.pem'), '') + writeFileSync(join(root, 'a', 'b', 'notes.txt'), '') + const patterns = ['**/.env', '**/*.pem', '**/*.key'].map(p => + join(root, p), + ) + const alone = patterns.map(p => walkGlobPattern(p).matches) + + const listed: string[] = [] + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + listed.push(String(args[0])) + return readdirSync(...args) + }) as typeof fs.readdirSync) + let together + try { + const listings = new Map() + together = patterns.map(p => walkGlobPattern(p, { listings }).matches) + } finally { + readdirSpy.mockRestore() + } + + expect(listed.sort()).toEqual( + [root, join(root, 'a'), join(root, 'a', 'b')].sort(), + ) + expect(together).toEqual(alone) + expect(together.flat().sort()).toEqual( + [join(root, 'a', '.env'), join(root, 'a', 'b', 'id.pem')].sort(), + ) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) + it('lists a directory again under a name the pattern tells apart', () => { // vault is reached by its own name, which matches nothing, and through // config/secrets, the only spelling `**/secrets/*.pem` matches. However @@ -1185,6 +1225,53 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { await SandboxManager.reset() }) + it('lists a directory once for all the denyRead patterns of a configuration', async () => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { + allowedDomains: [], + deniedDomains: [], + }, + filesystem: { + denyRead: ['*.env', '*.txt', '*.pem'].map(p => join(RAW_TEST_DIR, p)), + allowWrite: ['/tmp'], + denyWrite: [], + }, + }) + + const realTestDir = realPath(RAW_TEST_DIR) + const timesListedBy = async (call: () => unknown): Promise => { + let times = 0 + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + if (String(args[0]) === realTestDir) times++ + return readdirSync(...args) + }) as typeof fs.readdirSync) + try { + await call() + } finally { + readdirSpy.mockRestore() + } + return times + } + try { + expect(await timesListedBy(() => SandboxManager.getFsReadConfig())).toBe( + 1, + ) + expect( + await timesListedBy(() => SandboxManager.wrapWithSandbox('true')), + ).toBe(1) + } finally { + await SandboxManager.reset() + } + }) + it('should pass non-glob paths through unchanged on Linux', async () => { const { SandboxManager } = await import( '../../src/sandbox/sandbox-manager.js' From 282bcbad6051b4018178f4c0ca92606d3b52e30b Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 12:33:12 +0000 Subject: [PATCH 2/4] linux: the wrap walks its patterns in turns, and stops when its signal is aborted Expanding a `denyRead` or `allowRead` pattern lists everything under its base. That ran from start to finish on the caller's thread: for as long as it took, the caller could neither draw nor hear that it should stop, and the AbortSignal that wrapWithSandbox() takes could not fire. The walk is now a generator, a step to a directory, and so are the two functions between it and the wrap: one implementation, run in one of two ways. getFsReadConfig(), which is synchronous by signature, runs it on the spot. wrapWithSandbox() lets the event loop have a turn every 10 ms and rejects with the signal's reason once that is aborted. The wrap reads every filesystem setting before the first turn, so a configuration replaced meanwhile does not reach it, as before. The CLI passed no signal, and its own handlers are installed once there is a child to pass an interrupt on to. Heard during the walk, an interrupt was therefore taken by the library's clean-up alone and the command ran all the same. It now stops the wrap: exit 1, at once, nothing run. Six patterns over 400,000 files: the event loop held for 950 ms at a stretch before, 13 to 18 ms after; one wrap 0.98 s before, 1.0 s after; the same mounts. --- src/cli.ts | 17 ++++- src/sandbox/read-deny-glob.ts | 23 +++++- src/sandbox/sandbox-manager.ts | 103 +++++++++++++++---------- src/sandbox/sandbox-utils.ts | 61 +++++++++++++-- test/cli.test.ts | 54 +++++++++++++- test/sandbox/glob-expand.test.ts | 124 +++++++++++++++++++++++++++++++ 6 files changed, 330 insertions(+), 52 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 2a099ad38..037b7d2ac 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -521,8 +521,21 @@ async function main(): Promise { env, }) } else { - const sandboxedCommand = - await SandboxManager.wrapWithSandbox(command) + // There is no child yet to pass an interrupt on to, so it stops + // the wrap, which can take seconds over a large tree. + const interrupted = new AbortController() + const interrupt = (signal: NodeJS.Signals): void => + interrupted.abort(new Error(`interrupted by ${signal}`)) + process.once('SIGINT', interrupt) + process.once('SIGTERM', interrupt) + const sandboxedCommand = await SandboxManager.wrapWithSandbox( + command, + undefined, + undefined, + interrupted.signal, + ) + process.off('SIGINT', interrupt) + process.off('SIGTERM', interrupt) child = spawn(sandboxedCommand, { shell: true, stdio: sandboxedStdio(controlFd), diff --git a/src/sandbox/read-deny-glob.ts b/src/sandbox/read-deny-glob.ts index 0a1d7915c..34c83bf08 100644 --- a/src/sandbox/read-deny-glob.ts +++ b/src/sandbox/read-deny-glob.ts @@ -5,7 +5,9 @@ import { normalizePathForSandbox, pathSpellings, properAncestors, - walkGlobPattern, + type Steps, + finish, + walkGlobPatternSteps, } from './sandbox-utils.js' /** @@ -79,7 +81,24 @@ export function expandReadDenyGlobLinux( unlistableDirs?: Set, listings?: GlobWalkListings, ): string[] { - const walk = walkGlobPattern(globPattern, { + return finish( + expandReadDenyGlobLinuxSteps( + globPattern, + reExposedPaths, + unlistableDirs, + listings, + ), + ) +} + +/** {@link expandReadDenyGlobLinux}, in the walk's steps. */ +export function* expandReadDenyGlobLinuxSteps( + globPattern: string, + reExposedPaths: readonly string[], + unlistableDirs?: Set, + listings?: GlobWalkListings, +): Steps { + const walk = yield* walkGlobPatternSteps(globPattern, { withDirectoryForm: true, followSymlinkedDirectories: true, listings, diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index b8e6c8611..3e135a840 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -49,7 +49,7 @@ import { cleanupBwrapMountPoints, linuxGetCwdMandatoryDenyPaths, } from './linux-sandbox-utils.js' -import { expandReadDenyGlobLinux } from './read-deny-glob.js' +import { expandReadDenyGlobLinuxSteps } from './read-deny-glob.js' import { wrapCommandWithSandboxMacOS, startMacOSSandboxLogMonitor, @@ -84,8 +84,11 @@ import { globPatternBaseDir, normalizePathForSandbox, removeTrailingGlobSuffix, - expandGlobPattern, + walkGlobPatternSteps, type GlobWalkListings, + type Steps, + finish, + finishInTurns, attributionKeyFor, decodeSandboxedCommand, encodeSandboxedCommand, @@ -1265,20 +1268,26 @@ function unionDenyReadPaths( * it is passed through whatever characters it contains. Expanded as a * pattern, a name holding `[` matches nothing and the deny is lost. */ -function resolveReadPathEntries( +function* resolveReadPathEntries( paths: readonly string[], - expandGlob: (pattern: string) => string[], + expandGlob: (pattern: string) => Steps, literalPaths: readonly string[] = [], -): string[] { +): Steps { const literal = new Set(literalPaths) - return paths.flatMap(p => { + const resolved: string[] = [] + for (const p of paths) { const stripped = removeTrailingGlobSuffix(p) - return getPlatform() === 'linux' && + if ( + getPlatform() === 'linux' && !literal.has(p) && containsGlobChars(stripped) - ? expandGlob(p) - : [stripped] - }) + ) { + resolved.push(...(yield* expandGlob(p))) + } else { + resolved.push(stripped) + } + } + return resolved } /** @@ -1298,8 +1307,8 @@ function stripWriteGlobs(paths: readonly string[]): string[] { }) } -function expandAllowReadGlob(pattern: string): string[] { - const expanded = expandGlobPattern(pattern) +function* expandAllowReadGlob(pattern: string): Steps { + const expanded = (yield* walkGlobPatternSteps(pattern)).matches logForDebugging( `[Sandbox] Expanded allowRead glob pattern "${pattern}" to ${expanded.length} paths on Linux`, ) @@ -1326,23 +1335,27 @@ function getFsReadConfig(): FsReadRestrictionConfig { ) // allowRead (re-allow within denied regions) is resolved first: the // denyRead glob expansion collapses against it. - const allowPaths = resolveReadPathEntries( - config.filesystem.allowRead ?? [], - expandAllowReadGlob, + const allowPaths = finish( + resolveReadPathEntries( + config.filesystem.allowRead ?? [], + expandAllowReadGlob, + ), ) const reExposedPaths = [...allowPaths, ...getFsWriteConfig().allowOnly] const unlistableDenyDirs = new Set() const listings: GlobWalkListings = new Map() - const denyPaths = resolveReadPathEntries( - unionDenyReadPaths(config.filesystem.denyRead, credentialRestrictions), - pattern => - expandReadDenyGlobLinux( - pattern, - reExposedPaths, - unlistableDenyDirs, - listings, - ), - credentialRestrictions.degradeToDenyPaths, + const denyPaths = finish( + resolveReadPathEntries( + unionDenyReadPaths(config.filesystem.denyRead, credentialRestrictions), + pattern => + expandReadDenyGlobLinuxSteps( + pattern, + reExposedPaths, + unlistableDenyDirs, + listings, + ), + credentialRestrictions.degradeToDenyPaths, + ), ) return { @@ -1708,9 +1721,14 @@ async function wrapWithSandbox( // allowRead is resolved first: on Linux a denyRead glob's expansion is // collapsed against the paths that re-expose contents under a denied // directory (allowRead + allowWrite), so both must be final here. - const expandedAllowRead = resolveReadPathEntries( - customConfig?.filesystem?.allowRead ?? config?.filesystem.allowRead ?? [], - expandAllowReadGlob, + const expandedAllowRead = await finishInTurns( + resolveReadPathEntries( + customConfig?.filesystem?.allowRead ?? + config?.filesystem.allowRead ?? + [], + expandAllowReadGlob, + ), + abortSignal, ) // The TLS-termination CA cert and the trust bundle the env vars point at // (NODE_EXTRA_CA_CERTS etc.) must be readable by the child, even if their @@ -1725,19 +1743,24 @@ async function wrapWithSandbox( const reExposedPaths = [...expandedAllowRead, ...writeConfig.allowOnly] const unlistableDenyDirs = new Set() const listings: GlobWalkListings = new Map() - const expandedDenyRead = resolveReadPathEntries( - unionDenyReadPaths( - customConfig?.filesystem?.denyRead ?? config?.filesystem.denyRead ?? [], - credentialRestrictions, - ), - pattern => - expandReadDenyGlobLinux( - pattern, - reExposedPaths, - unlistableDenyDirs, - listings, + const expandedDenyRead = await finishInTurns( + resolveReadPathEntries( + unionDenyReadPaths( + customConfig?.filesystem?.denyRead ?? + config?.filesystem.denyRead ?? + [], + credentialRestrictions, ), - credentialRestrictions.degradeToDenyPaths, + pattern => + expandReadDenyGlobLinuxSteps( + pattern, + reExposedPaths, + unlistableDenyDirs, + listings, + ), + credentialRestrictions.degradeToDenyPaths, + ), + abortSignal, ) readConfig = { denyOnly: expandedDenyRead, diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 7fa101423..0b3da1737 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1141,6 +1141,50 @@ export interface ExpandGlobOptions { * pattern, reads the same entries. */ export type GlobWalkListings = Map +export type GlobWalkOptions = ExpandGlobOptions & { + withDirectoryForm?: boolean + followSymlinkedDirectories?: boolean + /** Handed to every walk of one configuration, so that patterns with a + * base in common list each directory once between them. */ + listings?: GlobWalkListings +} + +/** Work that can be left between two steps and taken up again. */ +export type Steps = Generator + +/** Runs `steps` to the end, on the spot. */ +export function finish(steps: Steps): T { + for (;;) { + const step = steps.next() + if (step.done) return step.value + } +} + +/** How long {@link finishInTurns} works before it lets others have a turn. */ +const TURN_MS = 10 + +/** + * Runs `steps` to the end, letting the event loop have a turn every + * {@link TURN_MS}: a walk of a large tree takes seconds, and a caller whose + * thread is held that long can neither draw nor hear that it should stop. + * Rejects with `signal`'s reason once that is aborted. + */ +export async function finishInTurns( + steps: Steps, + signal?: AbortSignal, +): Promise { + signal?.throwIfAborted() + let since = performance.now() + for (;;) { + const step = steps.next() + if (step.done) return step.value + if (performance.now() - since < TURN_MS) continue + await new Promise(resolve => setImmediate(resolve)) + signal?.throwIfAborted() + since = performance.now() + } +} + /** What one recursive walk of a glob's base directory found; see {@link walkGlobPattern}. */ export interface GlobWalk { /** Where the walk started, with symlinks resolved (the spelling itself @@ -1489,14 +1533,16 @@ export function toForwardSlashes(s: string): string { */ export function walkGlobPattern( globPath: string, - opts: ExpandGlobOptions & { - withDirectoryForm?: boolean - followSymlinkedDirectories?: boolean - /** Handed to every walk of one configuration, so that patterns with a - * base in common list each directory once between them. */ - listings?: GlobWalkListings - } = {}, + opts: GlobWalkOptions = {}, ): GlobWalk { + return finish(walkGlobPatternSteps(globPath, opts)) +} + +/** {@link walkGlobPattern}, a step to a directory. */ +export function* walkGlobPatternSteps( + globPath: string, + opts: GlobWalkOptions = {}, +): Steps { const walk: GlobWalk = { baseLocation: '', matches: [], @@ -1621,6 +1667,7 @@ export function walkGlobPattern( // others it came with, so only the ones new to this directory are taken. const fresh = frame.positions.filter(p => !listed.has(p)) if (fresh.length === 0) continue + yield for (const p of fresh) listed.add(p) let entries = listings.get(real) try { diff --git a/test/cli.test.ts b/test/cli.test.ts index 654490796..4917ee47a 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -1,5 +1,5 @@ import { describe, test, expect } from 'bun:test' -import { spawnSync } from 'node:child_process' +import { spawn, spawnSync } from 'node:child_process' import { mkdirSync, mkdtempSync, @@ -9,6 +9,7 @@ import { } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { isLinux } from './helpers/platform.js' /** * Get the path to the CLI entry point @@ -306,4 +307,55 @@ describe('CLI', () => { expect(result.status).toBe(0) }) }) + + describe.if(isLinux)('an interrupt before the command has started', () => { + test('stops the wrap, and the command is not run', async () => { + // Enough directories for the walk of the pattern to outlast the signal. + const root = mkdtempSync(join(tmpdir(), 'cli-interrupt-')) + try { + for (let i = 0; i < 200; i++) { + for (let j = 0; j < 100; j++) { + mkdirSync(join(root, 'tree', `d${i}`, `e${j}`), { recursive: true }) + } + } + const settings = join(root, 'settings.json') + writeFileSync( + settings, + JSON.stringify({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: [join(root, 'tree', '**/*.pem')], + allowWrite: [], + denyWrite: [], + }, + }), + ) + const child = spawn( + 'bun', + ['run', getCliPath(), '-s', settings, 'echo', 'RAN'], + { env: { ...process.env, SRT_DEBUG: 'true' } }, + ) + let stdout = '' + let stderr = '' + let signalled = false + child.stdout.on('data', (chunk: Buffer) => (stdout += String(chunk))) + child.stderr.on('data', (chunk: Buffer) => { + stderr += String(chunk) + // The last thing the CLI says before it wraps the command. + if (!signalled && stderr.includes('Original command')) { + signalled = true + child.kill('SIGINT') + } + }) + const status = await new Promise(resolve => child.on('exit', resolve)) + + expect(signalled).toBe(true) + expect(stderr).toContain('interrupted by SIGINT') + expect(stdout).not.toContain('RAN') + expect(status).toBe(1) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }, 60_000) + }) }) diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index 9f0d90ef8..1b9845331 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -18,10 +18,14 @@ import { join } from 'node:path' import { expandGlobPattern, expandTilde, + finish, + finishInTurns, globPatternBaseDir, globToRegex, normalizePathForSandbox, + type Steps, walkGlobPattern, + walkGlobPatternSteps, } from '../../src/sandbox/sandbox-utils.js' import { containsGlobCharsWin, @@ -762,6 +766,73 @@ describe.if(!isWindows)('walkGlobPattern', () => { // expandTilde — `~\` form is Windows-only // ============================================================================ +describe('finishInTurns', () => { + /** `count` steps of about `ms` each; returns how many it took. */ + function* busy( + count: number, + ms: number, + taken = { steps: 0 }, + ): Steps { + for (let i = 0; i < count; i++) { + const until = performance.now() + ms + while (performance.now() < until); + taken.steps++ + yield + } + return taken.steps + } + + it('lets the event loop have a turn while the steps go on', async () => { + const taken = { steps: 0 } + let stepsWhenTheTimerFired = -1 + setTimeout(() => (stepsWhenTheTimerFired = taken.steps), 0) + expect(await finishInTurns(busy(30, 3, taken))).toBe(30) + expect(stepsWhenTheTimerFired).toBeGreaterThan(0) + expect(stepsWhenTheTimerFired).toBeLessThan(30) + }) + + it('takes no step for a signal already aborted', async () => { + const taken = { steps: 0 } + const reason = new Error('stopped') + expect( + await finishInTurns(busy(3, 0, taken), AbortSignal.abort(reason)).catch( + (e: unknown) => e, + ), + ).toBe(reason) + expect(taken.steps).toBe(0) + }) + + it("stops at the next turn with the signal's reason", async () => { + const taken = { steps: 0 } + const controller = new AbortController() + const reason = new Error('stopped') + setTimeout(() => controller.abort(reason), 0) + expect( + await finishInTurns(busy(30, 3, taken), controller.signal).catch( + (e: unknown) => e, + ), + ).toBe(reason) + expect(taken.steps).toBeLessThan(30) + }) + + it('finds what the walk finds on the spot', async () => { + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-turns-'))) + try { + mkdirSync(join(root, 'a', 'b'), { recursive: true }) + writeFileSync(join(root, 'a', 'b', 'id.pem'), '') + symlinkSync(join(root, 'a'), join(root, 'link')) + const opts = { followSymlinkedDirectories: true, withDirectoryForm: true } + const pattern = join(root, '**/*.pem') + const inTurns = await finishInTurns(walkGlobPatternSteps(pattern, opts)) + expect(inTurns).toEqual(walkGlobPattern(pattern, opts)) + expect(inTurns).toEqual(finish(walkGlobPatternSteps(pattern, opts))) + expect(inTurns.matches).toEqual([join(root, 'a', 'b', 'id.pem')]) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }) +}) + describe('expandTilde', () => { it.if(!isWindows)( 'should NOT expand `~\\` on POSIX (literal filename byte)', @@ -1272,6 +1343,59 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { } }) + for (const list of ['denyRead', 'allowRead'] as const) { + it(`gives up a wrap whose signal is aborted while a ${list} pattern is walked`, async () => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-abort-'))) + const dirs = Array.from({ length: 12 }, (_, i) => join(root, `d${i}`)) + for (const dir of dirs) mkdirSync(dir) + + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: list === 'denyRead' ? [join(root, '**/*.pem')] : [root], + allowRead: list === 'allowRead' ? [join(root, '**/*.pem')] : [], + allowWrite: [], + denyWrite: [], + }, + }) + + // Each listing takes longer than a turn, and the third one aborts. + const controller = new AbortController() + const reason = new Error('stopped') + let listed = 0 + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + if (String(args[0]).startsWith(root)) { + if (++listed === 3) controller.abort(reason) + const until = performance.now() + 15 + while (performance.now() < until); + } + return readdirSync(...args) + }) as typeof fs.readdirSync) + try { + expect( + await SandboxManager.wrapWithSandbox( + 'true', + undefined, + undefined, + controller.signal, + ).catch((e: unknown) => e), + ).toBe(reason) + expect(listed).toBe(3) + } finally { + readdirSpy.mockRestore() + await SandboxManager.reset() + rmSync(root, { recursive: true, force: true }) + } + }) + } + it('should pass non-glob paths through unchanged on Linux', async () => { const { SandboxManager } = await import( '../../src/sandbox/sandbox-manager.js' From e1926fd93e6ffa9a5494af43c0d419079967f3c0 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 12:41:16 +0000 Subject: [PATCH 3/4] linux: a wrap reads its configuration before its first turn, and an aborted one hands nothing out Two faults in the change before this one, and one it brought to light. With the allowRead patterns walked in turns too, the denyRead list and the network flags were read after the first turn. updateConfig() can run in a turn, so a wrap could get the writes of one configuration and the reads of another: a file the first denied was readable. Everything the configuration says about files and about restricting the network is again read in one stretch, ahead of the first turn, as before the walk was in turns. The scan for mandatory denies below the working directory takes the wrap's signal, and swallowed every failure of ripgrep as "nothing found", an abort included. An aborted wrap then resolved with a command that lacked those denies. It now rejects. The CLI has passed a signal since the last change, so an interrupt during the scan reached that path. finishInTurns() looks at the signal once more before it returns, and the CLI looks at its own after the wrap and before it spawns. --- src/cli.ts | 1 + src/sandbox/linux-sandbox-utils.ts | 3 + src/sandbox/sandbox-manager.ts | 81 +++++++++++++------------ src/sandbox/sandbox-utils.ts | 8 +-- test/sandbox/glob-expand.test.ts | 95 ++++++++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 45 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 037b7d2ac..3715f0a08 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -534,6 +534,7 @@ async function main(): Promise { undefined, interrupted.signal, ) + interrupted.signal.throwIfAborted() process.off('SIGINT', interrupt) process.off('SIGTERM', interrupt) child = spawn(sandboxedCommand, { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 4b22a9557..6a6a2ab12 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -372,6 +372,9 @@ async function linuxGetMandatoryDenyPaths( ripgrepConfig, ) } catch (error) { + // Stopped, it found nothing: the caller must not be handed a command + // without the denies it would have found. + signal.throwIfAborted() logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`) } diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 3e135a840..6ff1b68d4 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -1663,6 +1663,25 @@ async function wrapWithSandbox( const commandId = options?.commandId registerCommandText(command, options) + // Check if network config is specified - this determines if we need network restrictions + // Network restriction is needed when: + // 1. customConfig has network.allowedDomains defined (even if empty array = block all) + // 2. OR config has network.allowedDomains defined (even if empty array = block all) + // An empty allowedDomains array means "no domains allowed" = block all network access + const hasNetworkConfig = + customConfig?.network?.allowedDomains !== undefined || + config?.network?.allowedDomains !== undefined + + // Network RESTRICTION is needed whenever network config is specified + // This includes empty allowedDomains which means "block all network" + const needsNetworkRestriction = hasNetworkConfig + + // Network PROXY is needed whenever network config is specified + // Even with empty allowedDomains, we route through proxy so that: + // 1. updateConfig() can enable network access for already-running processes + // 2. The proxy blocks all requests when allowlist is empty + const needsNetworkProxy = hasNetworkConfig + // filesystem.disabled bypasses ALL filesystem rule generation. Both // platform wrappers treat readConfig/writeConfig === undefined as "no // filesystem restrictions" (seatbelt emits `(allow file-write*)`; bwrap @@ -1721,36 +1740,35 @@ async function wrapWithSandbox( // allowRead is resolved first: on Linux a denyRead glob's expansion is // collapsed against the paths that re-expose contents under a denied // directory (allowRead + allowWrite), so both must be final here. + // + // INVARIANT: what the configuration says is read above and here, ahead of + // the first turn below. updateConfig() can run in a turn, and this wrap + // must not get the writes of one configuration and the reads of another. + const allowRead = + customConfig?.filesystem?.allowRead ?? config?.filesystem.allowRead ?? [] + const denyRead = unionDenyReadPaths( + customConfig?.filesystem?.denyRead ?? config?.filesystem.denyRead ?? [], + credentialRestrictions, + ) + const ownReadPaths = [ + // The TLS-termination CA cert and the trust bundle the env vars point at + // (NODE_EXTRA_CA_CERTS etc.) must be readable by the child, even if + // their paths fall under a user-configured denyRead. + ...(mitmCA ? [mitmCA.certPath, mitmCA.trustBundlePath] : []), + // Likewise the JVM proxy agent jar JAVA_TOOL_OPTIONS points at. + ...(javaAgentJarPath ? [javaAgentJarPath] : []), + ] const expandedAllowRead = await finishInTurns( - resolveReadPathEntries( - customConfig?.filesystem?.allowRead ?? - config?.filesystem.allowRead ?? - [], - expandAllowReadGlob, - ), + resolveReadPathEntries(allowRead, expandAllowReadGlob), abortSignal, ) - // The TLS-termination CA cert and the trust bundle the env vars point at - // (NODE_EXTRA_CA_CERTS etc.) must be readable by the child, even if their - // paths fall under a user-configured denyRead. - if (mitmCA) { - expandedAllowRead.push(mitmCA.certPath, mitmCA.trustBundlePath) - } - // Likewise the JVM proxy agent jar JAVA_TOOL_OPTIONS points at. - if (javaAgentJarPath) { - expandedAllowRead.push(javaAgentJarPath) - } + expandedAllowRead.push(...ownReadPaths) const reExposedPaths = [...expandedAllowRead, ...writeConfig.allowOnly] const unlistableDenyDirs = new Set() const listings: GlobWalkListings = new Map() const expandedDenyRead = await finishInTurns( resolveReadPathEntries( - unionDenyReadPaths( - customConfig?.filesystem?.denyRead ?? - config?.filesystem.denyRead ?? - [], - credentialRestrictions, - ), + denyRead, pattern => expandReadDenyGlobLinuxSteps( pattern, @@ -1769,25 +1787,6 @@ async function wrapWithSandbox( } } - // Check if network config is specified - this determines if we need network restrictions - // Network restriction is needed when: - // 1. customConfig has network.allowedDomains defined (even if empty array = block all) - // 2. OR config has network.allowedDomains defined (even if empty array = block all) - // An empty allowedDomains array means "no domains allowed" = block all network access - const hasNetworkConfig = - customConfig?.network?.allowedDomains !== undefined || - config?.network?.allowedDomains !== undefined - - // Network RESTRICTION is needed whenever network config is specified - // This includes empty allowedDomains which means "block all network" - const needsNetworkRestriction = hasNetworkConfig - - // Network PROXY is needed whenever network config is specified - // Even with empty allowedDomains, we route through proxy so that: - // 1. updateConfig() can enable network access for already-running processes - // 2. The proxy blocks all requests when allowlist is empty - const needsNetworkProxy = hasNetworkConfig - // Wait for network initialization only if proxy is actually needed if (needsNetworkProxy) { await waitForNetworkInitialization() diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 0b3da1737..e17a72854 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1167,7 +1167,7 @@ const TURN_MS = 10 * Runs `steps` to the end, letting the event loop have a turn every * {@link TURN_MS}: a walk of a large tree takes seconds, and a caller whose * thread is held that long can neither draw nor hear that it should stop. - * Rejects with `signal`'s reason once that is aborted. + * Rejects with `signal`'s reason once that is aborted, however far it got. */ export async function finishInTurns( steps: Steps, @@ -1177,10 +1177,10 @@ export async function finishInTurns( let since = performance.now() for (;;) { const step = steps.next() - if (step.done) return step.value - if (performance.now() - since < TURN_MS) continue - await new Promise(resolve => setImmediate(resolve)) + if (!step.done && performance.now() - since < TURN_MS) continue + if (!step.done) await new Promise(resolve => setImmediate(resolve)) signal?.throwIfAborted() + if (step.done) return step.value since = performance.now() } } diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index 1b9845331..dfb285de8 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -815,6 +815,21 @@ describe('finishInTurns', () => { expect(taken.steps).toBeLessThan(30) }) + it('hands nothing out when the signal is aborted in the last step', async () => { + const controller = new AbortController() + const reason = new Error('stopped') + function* abortsAtTheEnd(): Steps { + yield + controller.abort(reason) + return 'done' + } + expect( + await finishInTurns(abortsAtTheEnd(), controller.signal).catch( + (e: unknown) => e, + ), + ).toBe(reason) + }) + it('finds what the walk finds on the spot', async () => { const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-turns-'))) try { @@ -1396,6 +1411,86 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { }) } + it('keeps to the configuration a wrap started with when that is replaced in a turn', async () => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-replaced-'))) + for (let i = 0; i < 6; i++) + mkdirSync(join(root, 'tree', `d${i}`), { recursive: true }) + for (const name of ['first', 'second']) writeFileSync(join(root, name), '') + const configDenying = (name: string) => ({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: [join(root, name)], + allowRead: [join(root, 'tree', '**/*.pem')], + allowWrite: [], + denyWrite: [], + }, + }) + + await SandboxManager.reset() + await SandboxManager.initialize(configDenying('first')) + + // Each listing takes longer than a turn; the configuration is replaced + // while the allowRead pattern, the first to be walked, is under way. + let listed = 0 + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + if (String(args[0]).startsWith(root)) { + if (++listed === 2) SandboxManager.updateConfig(configDenying('second')) + const until = performance.now() + 15 + while (performance.now() < until); + } + return readdirSync(...args) + }) as typeof fs.readdirSync) + try { + const command = await SandboxManager.wrapWithSandbox('true') + expect(listed).toBeGreaterThan(2) + expect(command).toContain(join(root, 'first')) + expect(command).not.toContain(join(root, 'second')) + } finally { + readdirSpy.mockRestore() + await SandboxManager.reset() + rmSync(root, { recursive: true, force: true }) + } + }) + + it('gives up a wrap whose signal is aborted while the mandatory denies are looked for', async () => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-scan-'))) + // Stands in for ripgrep, and outlasts the abort. + const slowScan = join(root, 'slow-scan') + writeFileSync(slowScan, '#!/bin/sh\nsleep 5\n', { mode: 0o755 }) + + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { denyRead: [], allowWrite: [root], denyWrite: [] }, + ripgrep: { command: slowScan }, + }) + const controller = new AbortController() + const reason = new Error('stopped') + setTimeout(() => controller.abort(reason), 200) + try { + expect( + await SandboxManager.wrapWithSandbox( + 'true', + undefined, + undefined, + controller.signal, + ).catch((e: unknown) => e), + ).toBe(reason) + } finally { + await SandboxManager.reset() + rmSync(root, { recursive: true, force: true }) + } + }) + it('should pass non-glob paths through unchanged on Linux', async () => { const { SandboxManager } = await import( '../../src/sandbox/sandbox-manager.js' From 2aa059183278f69a33d4f37760ed15ea1cbbf008 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 13:39:55 +0000 Subject: [PATCH 4/4] linux: one wrap, one configuration; and the CLI hears an interrupt from the start The last commit read the file lists and the network flags ahead of the first turn and said that the configuration was read there. It was not: some twenty settings are read after the last turn (allowGitConfig, the search depth, the unix sockets, the weaker nested sandbox). On main they follow an await that has already resolved, so nothing comes between. With the walk in turns updateConfig() can, and a command could get the write list of one configuration with the allowGitConfig of another: a policy nobody chose. The wrap now remembers the configuration it started with and, if that has been replaced by the time its last await is over, starts over. Nothing from there to the platform's wrapper awaits, so every setting is read from one configuration, and the command is the one an undisturbed wrap under the new configuration gives. The reads are back in their original order. The CLI installed its interrupt handlers just ahead of the wrap. initialize() installs the library's own, which only clean up, so an interrupt between the two was taken by those alone and the command was run all the same, as on main. The CLI's are now installed first. --- src/cli.ts | 27 +++++---- src/sandbox/sandbox-manager.ts | 97 +++++++++++++++++------------- test/cli.test.ts | 100 +++++++++++++++++-------------- test/sandbox/glob-expand.test.ts | 51 ++++++++++++---- 4 files changed, 166 insertions(+), 109 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 3715f0a08..8109b56d5 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -426,6 +426,20 @@ async function main(): Promise { controlStream.on('error', onControlError) } + // Until there is a child to pass an interrupt on to, it stops the + // run. The library's own handlers, which initialize() installs, only + // clean up: left to them the command would be run all the same. + const interrupted = new AbortController() + const interrupt = (signal: NodeJS.Signals): void => + interrupted.abort(new Error(`interrupted by ${signal}`)) + process.once('SIGINT', interrupt) + process.once('SIGTERM', interrupt) + const aboutToSpawn = (): void => { + interrupted.signal.throwIfAborted() + process.off('SIGINT', interrupt) + process.off('SIGTERM', interrupt) + } + // Initialize sandbox with config logForDebugging('Initializing sandbox...') await SandboxManager.initialize(runtimeConfig) @@ -515,28 +529,21 @@ async function main(): Promise { // entries to the child as CRT descriptors, so the control fd // is not among them (an inheritable HANDLE still reaches the // child, but unnamed — nothing there can find it). + aboutToSpawn() child = spawn(argv[0], argv.slice(1), { shell: false, stdio: 'inherit', env, }) } else { - // There is no child yet to pass an interrupt on to, so it stops - // the wrap, which can take seconds over a large tree. - const interrupted = new AbortController() - const interrupt = (signal: NodeJS.Signals): void => - interrupted.abort(new Error(`interrupted by ${signal}`)) - process.once('SIGINT', interrupt) - process.once('SIGTERM', interrupt) + // The wrap can take seconds over a large tree. const sandboxedCommand = await SandboxManager.wrapWithSandbox( command, undefined, undefined, interrupted.signal, ) - interrupted.signal.throwIfAborted() - process.off('SIGINT', interrupt) - process.off('SIGTERM', interrupt) + aboutToSpawn() child = spawn(sandboxedCommand, { shell: true, stdio: sandboxedStdio(controlFd), diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 6ff1b68d4..1c0d8a97b 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -1662,25 +1662,7 @@ async function wrapWithSandbox( const platform = getPlatform() const commandId = options?.commandId registerCommandText(command, options) - - // Check if network config is specified - this determines if we need network restrictions - // Network restriction is needed when: - // 1. customConfig has network.allowedDomains defined (even if empty array = block all) - // 2. OR config has network.allowedDomains defined (even if empty array = block all) - // An empty allowedDomains array means "no domains allowed" = block all network access - const hasNetworkConfig = - customConfig?.network?.allowedDomains !== undefined || - config?.network?.allowedDomains !== undefined - - // Network RESTRICTION is needed whenever network config is specified - // This includes empty allowedDomains which means "block all network" - const needsNetworkRestriction = hasNetworkConfig - - // Network PROXY is needed whenever network config is specified - // Even with empty allowedDomains, we route through proxy so that: - // 1. updateConfig() can enable network access for already-running processes - // 2. The proxy blocks all requests when allowlist is empty - const needsNetworkProxy = hasNetworkConfig + const startedWith = config // filesystem.disabled bypasses ALL filesystem rule generation. Both // platform wrappers treat readConfig/writeConfig === undefined as "no @@ -1740,35 +1722,36 @@ async function wrapWithSandbox( // allowRead is resolved first: on Linux a denyRead glob's expansion is // collapsed against the paths that re-expose contents under a denied // directory (allowRead + allowWrite), so both must be final here. - // - // INVARIANT: what the configuration says is read above and here, ahead of - // the first turn below. updateConfig() can run in a turn, and this wrap - // must not get the writes of one configuration and the reads of another. - const allowRead = - customConfig?.filesystem?.allowRead ?? config?.filesystem.allowRead ?? [] - const denyRead = unionDenyReadPaths( - customConfig?.filesystem?.denyRead ?? config?.filesystem.denyRead ?? [], - credentialRestrictions, - ) - const ownReadPaths = [ - // The TLS-termination CA cert and the trust bundle the env vars point at - // (NODE_EXTRA_CA_CERTS etc.) must be readable by the child, even if - // their paths fall under a user-configured denyRead. - ...(mitmCA ? [mitmCA.certPath, mitmCA.trustBundlePath] : []), - // Likewise the JVM proxy agent jar JAVA_TOOL_OPTIONS points at. - ...(javaAgentJarPath ? [javaAgentJarPath] : []), - ] const expandedAllowRead = await finishInTurns( - resolveReadPathEntries(allowRead, expandAllowReadGlob), + resolveReadPathEntries( + customConfig?.filesystem?.allowRead ?? + config?.filesystem.allowRead ?? + [], + expandAllowReadGlob, + ), abortSignal, ) - expandedAllowRead.push(...ownReadPaths) + // The TLS-termination CA cert and the trust bundle the env vars point at + // (NODE_EXTRA_CA_CERTS etc.) must be readable by the child, even if their + // paths fall under a user-configured denyRead. + if (mitmCA) { + expandedAllowRead.push(mitmCA.certPath, mitmCA.trustBundlePath) + } + // Likewise the JVM proxy agent jar JAVA_TOOL_OPTIONS points at. + if (javaAgentJarPath) { + expandedAllowRead.push(javaAgentJarPath) + } const reExposedPaths = [...expandedAllowRead, ...writeConfig.allowOnly] const unlistableDenyDirs = new Set() const listings: GlobWalkListings = new Map() const expandedDenyRead = await finishInTurns( resolveReadPathEntries( - denyRead, + unionDenyReadPaths( + customConfig?.filesystem?.denyRead ?? + config?.filesystem.denyRead ?? + [], + credentialRestrictions, + ), pattern => expandReadDenyGlobLinuxSteps( pattern, @@ -1787,11 +1770,45 @@ async function wrapWithSandbox( } } + // Check if network config is specified - this determines if we need network restrictions + // Network restriction is needed when: + // 1. customConfig has network.allowedDomains defined (even if empty array = block all) + // 2. OR config has network.allowedDomains defined (even if empty array = block all) + // An empty allowedDomains array means "no domains allowed" = block all network access + const hasNetworkConfig = + customConfig?.network?.allowedDomains !== undefined || + config?.network?.allowedDomains !== undefined + + // Network RESTRICTION is needed whenever network config is specified + // This includes empty allowedDomains which means "block all network" + const needsNetworkRestriction = hasNetworkConfig + + // Network PROXY is needed whenever network config is specified + // Even with empty allowedDomains, we route through proxy so that: + // 1. updateConfig() can enable network access for already-running processes + // 2. The proxy blocks all requests when allowlist is empty + const needsNetworkProxy = hasNetworkConfig + // Wait for network initialization only if proxy is actually needed if (needsNetworkProxy) { await waitForNetworkInitialization() } + // INVARIANT: one wrap, one configuration. updateConfig() can run in any of + // the turns above, and what was read before it must not be put together + // with what is read after it: the write list of one configuration with the + // allowGitConfig of another is a policy nobody chose. So the wrap starts + // over. Nothing from here to the platform's wrapper awaits. + if (config !== startedWith) { + return wrapWithSandbox( + command, + binShell, + customConfig, + abortSignal, + options, + ) + } + // Check custom config to allow pseudo-terminal (can be applied dynamically) const allowPty = customConfig?.allowPty ?? config?.allowPty diff --git a/test/cli.test.ts b/test/cli.test.ts index 4917ee47a..fc8b8c9bf 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -309,53 +309,61 @@ describe('CLI', () => { }) describe.if(isLinux)('an interrupt before the command has started', () => { - test('stops the wrap, and the command is not run', async () => { - // Enough directories for the walk of the pattern to outlast the signal. - const root = mkdtempSync(join(tmpdir(), 'cli-interrupt-')) - try { - for (let i = 0; i < 200; i++) { - for (let j = 0; j < 100; j++) { - mkdirSync(join(root, 'tree', `d${i}`, `e${j}`), { recursive: true }) - } - } - const settings = join(root, 'settings.json') - writeFileSync( - settings, - JSON.stringify({ - network: { allowedDomains: [], deniedDomains: [] }, - filesystem: { - denyRead: [join(root, 'tree', '**/*.pem')], - allowWrite: [], - denyWrite: [], - }, - }), - ) - const child = spawn( - 'bun', - ['run', getCliPath(), '-s', settings, 'echo', 'RAN'], - { env: { ...process.env, SRT_DEBUG: 'true' } }, - ) - let stdout = '' - let stderr = '' - let signalled = false - child.stdout.on('data', (chunk: Buffer) => (stdout += String(chunk))) - child.stderr.on('data', (chunk: Buffer) => { - stderr += String(chunk) - // The last thing the CLI says before it wraps the command. - if (!signalled && stderr.includes('Original command')) { - signalled = true - child.kill('SIGINT') + for (const [when, lastLine] of [ + // The library's handlers are in place, and there is more to wait for. + ['the sandbox is being initialized', 'Starting HTTP bridge'], + // The last thing the CLI says before it wraps the command. + ['the command is being wrapped', 'Original command'], + ] as const) { + test(`stops the run while ${when}, and the command is not run`, async () => { + // Enough directories for the walk of the pattern to outlast the signal. + const root = mkdtempSync(join(tmpdir(), 'cli-interrupt-')) + try { + for (let i = 0; i < 200; i++) { + for (let j = 0; j < 100; j++) { + mkdirSync(join(root, 'tree', `d${i}`, `e${j}`), { + recursive: true, + }) + } } - }) - const status = await new Promise(resolve => child.on('exit', resolve)) + const settings = join(root, 'settings.json') + writeFileSync( + settings, + JSON.stringify({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: [join(root, 'tree', '**/*.pem')], + allowWrite: [], + denyWrite: [], + }, + }), + ) + const child = spawn( + 'bun', + ['run', getCliPath(), '-s', settings, 'echo', 'RAN'], + { env: { ...process.env, SRT_DEBUG: 'true' } }, + ) + let stdout = '' + let stderr = '' + let signalled = false + child.stdout.on('data', (chunk: Buffer) => (stdout += String(chunk))) + child.stderr.on('data', (chunk: Buffer) => { + stderr += String(chunk) + if (!signalled && stderr.includes(lastLine)) { + signalled = true + child.kill('SIGINT') + } + }) + const status = await new Promise(resolve => child.on('exit', resolve)) - expect(signalled).toBe(true) - expect(stderr).toContain('interrupted by SIGINT') - expect(stdout).not.toContain('RAN') - expect(status).toBe(1) - } finally { - rmSync(root, { recursive: true, force: true }) - } - }, 60_000) + expect(signalled).toBe(true) + expect(stderr).toContain('interrupted by SIGINT') + expect(stdout).not.toContain('RAN') + expect(status).toBe(1) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }, 60_000) + } }) }) diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index dfb285de8..96ff621da 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -1411,26 +1411,47 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { }) } - it('keeps to the configuration a wrap started with when that is replaced in a turn', async () => { + it('wraps by one configuration when that is replaced in a turn', async () => { const { SandboxManager } = await import( '../../src/sandbox/sandbox-manager.js' ) const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-replaced-'))) - for (let i = 0; i < 6; i++) + for (let i = 0; i < 6; i++) { mkdirSync(join(root, 'tree', `d${i}`), { recursive: true }) - for (const name of ['first', 'second']) writeFileSync(join(root, name), '') - const configDenying = (name: string) => ({ - network: { allowedDomains: [], deniedDomains: [] }, + } + mkdirSync(join(root, '.git')) + for (const name of ['first', 'second', '.git/config']) { + writeFileSync(join(root, name), '') + } + // They differ in what the wrap reads before its first turn and after its + // last: put together, the first's write list and the second's + // allowGitConfig would leave .git/config writable, which neither does. + const network = { allowedDomains: [], deniedDomains: [] } + const allowRead = [join(root, 'tree', '**/*.pem')] + const first = { + network, filesystem: { - denyRead: [join(root, name)], - allowRead: [join(root, 'tree', '**/*.pem')], + denyRead: [join(root, 'first')], + allowRead, + allowWrite: [root], + denyWrite: [], + }, + } + const second = { + network, + filesystem: { + denyRead: [join(root, 'second')], + allowRead, allowWrite: [], denyWrite: [], + allowGitConfig: true, }, - }) + } + const cwd = process.cwd() + process.chdir(root) await SandboxManager.reset() - await SandboxManager.initialize(configDenying('first')) + await SandboxManager.initialize(first) // Each listing takes longer than a turn; the configuration is replaced // while the allowRead pattern, the first to be walked, is under way. @@ -1440,19 +1461,23 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { ...args: Parameters ) => { if (String(args[0]).startsWith(root)) { - if (++listed === 2) SandboxManager.updateConfig(configDenying('second')) + if (++listed === 2) SandboxManager.updateConfig(second) const until = performance.now() + 15 while (performance.now() < until); } return readdirSync(...args) }) as typeof fs.readdirSync) try { - const command = await SandboxManager.wrapWithSandbox('true') + const disturbed = await SandboxManager.wrapWithSandbox('true') + readdirSpy.mockRestore() + expect(listed).toBeGreaterThan(2) - expect(command).toContain(join(root, 'first')) - expect(command).not.toContain(join(root, 'second')) + expect(disturbed).toBe(await SandboxManager.wrapWithSandbox('true')) + expect(disturbed).toContain(join(root, 'second')) + expect(disturbed).not.toContain(join(root, 'first')) } finally { readdirSpy.mockRestore() + process.chdir(cwd) await SandboxManager.reset() rmSync(root, { recursive: true, force: true }) }