From 6606d91203b97cc1cdb88b4a6db9bf6960b2fe33 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 2 Oct 2026 19:03:59 +0000 Subject: [PATCH 1/2] linux: a wrap that starts over reads the disk again Since #638 a wrap that starts over kept the directories it had listed and each pattern's finished walk. A host that wrote a file and then installed a rule denying it, while a command was being wrapped, got a command that could read the file: the rule was walked over a listing made before the file was there. A wrap that starts over now keeps nothing, so what it hands out was read after its configuration was installed. It still ends: after two restarts it walks without giving the thread back. And an attempt is left as soon as its configuration is seen to be replaced, not walked to its end. --- src/sandbox/read-deny-glob.ts | 9 +- src/sandbox/sandbox-manager.ts | 90 ++++++++------- src/sandbox/sandbox-utils.ts | 20 ---- test/sandbox/glob-expand.test.ts | 164 +++++++++++----------------- test/sandbox/read-deny-glob.test.ts | 36 +----- 5 files changed, 115 insertions(+), 204 deletions(-) diff --git a/src/sandbox/read-deny-glob.ts b/src/sandbox/read-deny-glob.ts index 0170de896..34c83bf08 100644 --- a/src/sandbox/read-deny-glob.ts +++ b/src/sandbox/read-deny-glob.ts @@ -1,7 +1,6 @@ import { logForDebugging } from '../utils/debug.js' import { type GlobWalkListings, - type GlobWalks, isAtOrUnder, normalizePathForSandbox, pathSpellings, @@ -92,23 +91,17 @@ export function expandReadDenyGlobLinux( ) } -/** - * {@link expandReadDenyGlobLinux}, in the walk's steps. With `walks`, a - * pattern walked before is not walked again; what follows the walk is done - * anew each time, since `reExposedPaths` may differ. - */ +/** {@link expandReadDenyGlobLinux}, in the walk's steps. */ export function* expandReadDenyGlobLinuxSteps( globPattern: string, reExposedPaths: readonly string[], unlistableDirs?: Set, listings?: GlobWalkListings, - walks?: GlobWalks, ): Steps { const walk = yield* walkGlobPatternSteps(globPattern, { withDirectoryForm: true, followSymlinkedDirectories: true, listings, - walks, }) // 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 438a04abb..e28c4935a 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -86,7 +86,6 @@ import { removeTrailingGlobSuffix, walkGlobPatternSteps, type GlobWalkListings, - type GlobWalks, type Steps, finish, finishInTurns, @@ -1308,11 +1307,8 @@ function stripWriteGlobs(paths: readonly string[]): string[] { }) } -function* expandAllowReadGlob( - pattern: string, - walks?: GlobWalks, -): Steps { - const expanded = (yield* walkGlobPatternSteps(pattern, { walks })).matches +function* expandAllowReadGlob(pattern: string): Steps { + const expanded = (yield* walkGlobPatternSteps(pattern)).matches logForDebugging( `[Sandbox] Expanded allowRead glob pattern "${pattern}" to ${expanded.length} paths on Linux`, ) @@ -1656,18 +1652,13 @@ export type WrapWithSandboxOptions = { commandText?: string } -/** What a wrap has found on disk so far, which it keeps when it starts over. */ -type WrapFindings = { - listings: GlobWalkListings - walks: GlobWalks - /** How many times the wrap has started over. */ - restarts: number -} - /** A wrap that has started over this many times walks without giving the * thread back. */ const RESTARTS_IN_TURNS = 2 +/** In place of what a walk finds: the configuration was replaced under it. */ +const REPLACED = Symbol('replaced') + async function wrapWithSandbox( command: string, binShell?: string, @@ -1675,8 +1666,8 @@ async function wrapWithSandbox( abortSignal?: AbortSignal, options?: WrapWithSandboxOptions, ): Promise { - return wrapWithSandboxKeeping( - { listings: new Map(), walks: new Map(), restarts: 0 }, + return wrapWithSandboxAgain( + 0, command, binShell, customConfig, @@ -1685,8 +1676,9 @@ async function wrapWithSandbox( ) } -async function wrapWithSandboxKeeping( - found: WrapFindings, +/** {@link wrapWithSandbox}, having started over `restarts` times. */ +async function wrapWithSandboxAgain( + restarts: number, command: string, binShell?: string, customConfig?: Partial, @@ -1697,9 +1689,30 @@ async function wrapWithSandboxKeeping( const commandId = options?.commandId registerCommandText(command, options) const startedWith = config - const walked = (steps: Steps): T | Promise => { - if (found.restarts < RESTARTS_IN_TURNS) { - return finishInTurns(steps, abortSignal) + const startOver = (): Promise => + wrapWithSandboxAgain( + restarts + 1, + command, + binShell, + customConfig, + abortSignal, + options, + ) + /** `steps`, left as soon as the configuration is seen to be replaced: + * what they would go on to find is for a wrap that starts over anyway. */ + function* whileCurrent(steps: Steps): Steps { + for (;;) { + if (config !== startedWith) return REPLACED + const step = steps.next() + if (step.done) return step.value + yield + } + } + const walked = ( + steps: Steps, + ): T | typeof REPLACED | Promise => { + if (restarts < RESTARTS_IN_TURNS) { + return finishInTurns(whileCurrent(steps), abortSignal) } abortSignal?.throwIfAborted() return finish(steps) @@ -1768,9 +1781,10 @@ async function wrapWithSandboxKeeping( customConfig?.filesystem?.allowRead ?? config?.filesystem.allowRead ?? [], - pattern => expandAllowReadGlob(pattern, found.walks), + expandAllowReadGlob, ), ) + if (expandedAllowRead === REPLACED) return startOver() // 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. @@ -1783,6 +1797,7 @@ async function wrapWithSandboxKeeping( } const reExposedPaths = [...expandedAllowRead, ...writeConfig.allowOnly] const unlistableDenyDirs = new Set() + const listings: GlobWalkListings = new Map() const expandedDenyRead = await walked( resolveReadPathEntries( unionDenyReadPaths( @@ -1796,12 +1811,12 @@ async function wrapWithSandboxKeeping( pattern, reExposedPaths, unlistableDenyDirs, - found.listings, - found.walks, + listings, ), credentialRestrictions.degradeToDenyPaths, ), ) + if (expandedDenyRead === REPLACED) return startOver() readConfig = { denyOnly: expandedDenyRead, allowWithinDeny: expandedAllowRead, @@ -1839,23 +1854,16 @@ async function wrapWithSandboxKeeping( // allowGitConfig of another is a policy nobody chose. So the wrap starts // over. Nothing from here to the platform's wrapper awaits. // - // INVARIANT: starting over costs only what is new, and ends. An embedder - // can replace the configuration many times during one walk, mostly with - // the same content. So the wrap keeps what it has listed and walked: an - // attempt with no pattern new to it takes no turn of the event loop, and - // after RESTARTS_IN_TURNS the new ones are walked without a turn as well. - // What is kept is as old as its walk, and misses a path made since, as one - // long walk misses a path made behind it. - if (config !== startedWith) { - return wrapWithSandboxKeeping( - { ...found, restarts: found.restarts + 1 }, - command, - binShell, - customConfig, - abortSignal, - options, - ) - } + // INVARIANT: what a wrap hands out was read from the disk after its + // configuration was installed. A host that writes a file and then installs + // a rule that denies it must find it denied. So a wrap that starts over + // keeps nothing of what it has listed or walked, the same rules included. + // + // INVARIANT: it ends. An embedder can replace the configuration many times + // during one walk. After RESTARTS_IN_TURNS the wrap walks without a turn of + // the event loop, so no timer or callback replaces it meanwhile. That holds + // the thread for one walk, as every wrap did before the walk took turns. + if (config !== startedWith) return startOver() // Check custom config to allow pseudo-terminal (can be applied dynamically) const allowPty = customConfig?.allowPty ?? config?.allowPty diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index fcd4a1121..e17a72854 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1141,19 +1141,12 @@ export interface ExpandGlobOptions { * pattern, reads the same entries. */ export type GlobWalkListings = Map -/** Finished walks, by their options and pattern. */ -export type GlobWalks = 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 - /** A walk found here is handed back as it is, and a finished one is put - * here: for a caller that starts over and should walk no pattern twice. - * What such a walk reports is as old as the walk. */ - walks?: GlobWalks } /** Work that can be left between two steps and taken up again. */ @@ -1550,18 +1543,6 @@ export function* walkGlobPatternSteps( globPath: string, opts: GlobWalkOptions = {}, ): Steps { - // The options that shape a walk, then its pattern. - const walkKey = - [ - opts.withDirectoryForm, - opts.followSymlinkedDirectories, - opts.caseInsensitive, - ] - .map(on => (on ? '1' : '0')) - .join('') + globPath - const walked = opts.walks?.get(walkKey) - if (walked !== undefined) return walked - const walk: GlobWalk = { baseLocation: '', matches: [], @@ -1804,6 +1785,5 @@ export function* walkGlobPatternSteps( } } - opts.walks?.set(walkKey, walk) return walk } diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index 3f244eba1..c039b5a9a 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -639,34 +639,6 @@ describe.if(!isWindows)('walkGlobPattern', () => { } }) - it('walks no pattern again that it is handed the walk of', () => { - const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-kept-'))) - try { - mkdirSync(join(root, 'a', 'b'), { recursive: true }) - writeFileSync(join(root, 'a', 'b', 'id.pem'), '') - const pattern = join(root, '**/*.pem') - const stepsOf = (steps: Steps): number => { - let taken = 0 - while (!steps.next().done) taken++ - return taken - } - const walks = new Map() - const first = walkGlobPattern(pattern, { walks }) - expect(first.matches).toEqual([join(root, 'a', 'b', 'id.pem')]) - expect(stepsOf(walkGlobPatternSteps(pattern, { walks }))).toBe(0) - expect(walkGlobPattern(pattern, { walks })).toBe(first) - // The same pattern with other options is another walk. - expect( - stepsOf( - walkGlobPatternSteps(pattern, { walks, withDirectoryForm: true }), - ), - ).toBeGreaterThan(0) - expect(walks.size).toBe(2) - } 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 @@ -1511,76 +1483,69 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { } }) - for (const list of ['denyRead', 'allowRead'] as const) { - it(`asks the disk nothing again for a ${list} pattern when the wrap starts over`, async () => { - const { SandboxManager } = await import( - '../../src/sandbox/sandbox-manager.js' - ) - const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-over-'))) - for (let i = 0; i < 6; i++) mkdirSync(join(root, `d${i}`)) - writeFileSync(join(root, 'd0', 'id.pem'), '') - const link = join(root, 'link') - symlinkSync(join(root, 'd0'), link) - const configured = (allowWrite: string[]) => ({ - 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; `duringSecond` runs in the - // second one. A denyRead walk resolves the link it meets, so a pattern - // walked again shows there even when its listings were kept. - const wrapAsking = async ( - duringSecond?: () => void, - ): Promise<[string, string[]]> => { - const asked: string[] = [] - let listed = 0 - const { readdirSync, realpathSync } = fs - const spies = [ - spyOn(fs, 'readdirSync').mockImplementation((( - ...args: Parameters - ) => { - if (String(args[0]).startsWith(root)) { - asked.push(`list ${String(args[0])}`) - if (++listed === 2) duringSecond?.() - const until = performance.now() + 15 - while (performance.now() < until); + for (const [what, before, after] of [ + [ + 'the rule that matches it was installed', + ['**/.env'], + ['**/.env', '**/*.pem'], + ], + ['the same rules were installed again', ['**/*.pem'], ['**/*.pem']], + ] as const) { + // The third listing is in the middle of the walk, the seventh is its + // last, after which only the check at the end of the wrap can tell. + it.each([3, 7])( + `denies a file written during the wrap, before ${what} (in listing %d)`, + async at => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-fresh-'))) + for (let i = 0; i < 6; i++) mkdirSync(join(root, `d${i}`)) + const configured = (patterns: readonly string[]) => ({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + denyRead: patterns.map(pattern => join(root, pattern)), + allowWrite: [], + denyWrite: [], + }, + }) + + await SandboxManager.reset() + await SandboxManager.initialize(configured(before)) + + // Each listing takes longer than a turn. In listing `at`, with `root` + // long listed, the host writes a file there and then installs rules. + const listed: string[] = [] + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + if (String(args[0]).startsWith(root)) { + if (listed.push(String(args[0])) === at) { + writeFileSync(join(root, 'key.pem'), '') + SandboxManager.updateConfig(configured(after)) } - return readdirSync(...args) - }) as typeof fs.readdirSync), - spyOn(fs, 'realpathSync').mockImplementation((( - ...args: Parameters - ) => { - if (String(args[0]) === link) asked.push('resolve link') - return realpathSync(...args) - }) as typeof fs.realpathSync), - ] + const until = performance.now() + 15 + while (performance.now() < until); + } + return readdirSync(...args) + }) as typeof fs.readdirSync) try { - return [await SandboxManager.wrapWithSandbox('true'), asked.sort()] + const disturbed = await SandboxManager.wrapWithSandbox('true') + readdirSpy.mockRestore() + + expect(listed[0]).toBe(root) + expect(disturbed).toContain(join(root, 'key.pem')) + expect(disturbed).toBe(await SandboxManager.wrapWithSandbox('true')) + // The first attempt is left at once; the second lists all seven. + expect(listed.length).toBe(at + 7) } finally { - for (const spy of spies) spy.mockRestore() + readdirSpy.mockRestore() + await SandboxManager.reset() + rmSync(root, { recursive: true, force: true }) } - } - - await SandboxManager.reset() - await SandboxManager.initialize(configured([])) - try { - const [disturbed, asked] = await wrapAsking(() => - SandboxManager.updateConfig(configured([root])), - ) - const [undisturbed, askedAlone] = await wrapAsking() - expect(asked).toEqual(askedAlone) - expect(asked).toContain(`list ${join(root, 'd5')}`) - expect(asked.includes('resolve link')).toBe(list === 'denyRead') - expect(disturbed).toBe(undisturbed) - } finally { - await SandboxManager.reset() - rmSync(root, { recursive: true, force: true }) - } - }) + }, + ) } it('walks without a turn once a wrap has started over twice', async () => { @@ -1606,9 +1571,9 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { await SandboxManager.initialize(configured(0)) // Each listing takes longer than a turn and leaves a new configuration - // for the next turn to bring. The first attempt walks t0 and the second - // t1, each with a turn between its two listings; the third walks t3 - // with none, so the configurations its listings leave come too late. + // for the next turn to bring. The first attempt is left after it has + // listed t0 and the second after t1; the third walks t2 without a turn, + // so the configurations its listings leave come too late. let replaced = 0 const listed: string[] = [] const readdirSync = fs.readdirSync @@ -1627,9 +1592,8 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { const wrapped = await SandboxManager.wrapWithSandbox('true') readdirSpy.mockRestore() - expect(listed).toEqual(['t0', 't0/d', 't1', 't1/d', 't3', 't3/d']) - expect(wrapped).toContain(join(root, 't3', 'd', 'id.pem')) - expect(wrapped).not.toContain(join(root, 't1', 'd', 'id.pem')) + expect(listed).toEqual(['t0', 't1', 't2', 't2/d']) + expect(wrapped).toContain(join(root, 't2', 'd', 'id.pem')) } finally { readdirSpy.mockRestore() await new Promise(resolve => setImmediate(resolve)) diff --git a/test/sandbox/read-deny-glob.test.ts b/test/sandbox/read-deny-glob.test.ts index 5a0d78000..1d36bf4c0 100644 --- a/test/sandbox/read-deny-glob.test.ts +++ b/test/sandbox/read-deny-glob.test.ts @@ -16,10 +16,7 @@ import { import { spawnSync } from 'node:child_process' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { - expandReadDenyGlobLinux, - expandReadDenyGlobLinuxSteps, -} from '../../src/sandbox/read-deny-glob.js' +import { expandReadDenyGlobLinux } from '../../src/sandbox/read-deny-glob.js' import { expandGlobPattern } from '../../src/sandbox/sandbox-utils.js' import { SandboxManager } from '../../src/sandbox/sandbox-manager.js' import { @@ -67,37 +64,6 @@ describe.if(!isWindows)('expandReadDenyGlobLinux (collapse)', () => { ]) }) - it('walks a pattern once when handed its walk, and collapses it anew', () => { - const build = join(ROOT, 'build') - const walks = new Map() - // No listings are handed over: only the kept walk can save the steps. - const expand = (reExposedPaths: string[]): [number, string[]] => { - const steps = expandReadDenyGlobLinuxSteps( - join(ROOT, 'build*/**'), - reExposedPaths, - undefined, - undefined, - walks, - ) - for (let taken = 0; ; taken++) { - const step = steps.next() - if (step.done) return [taken, step.value] - } - } - const [walked, mounts] = expand([]) - expect(walked).toBeGreaterThan(0) - expect(mounts).toEqual([build, join(ROOT, 'build-cache')]) - expect(expand([build])).toEqual([ - 0, - [ - build, - join(ROOT, 'build-cache'), - join(build, '1.out'), - join(build, 'sub'), - ], - ]) - }) - it('ignores re-exposers below the candidate or unrelated to it', () => { const mounts = expandReadDenyGlobLinux(join(ROOT, 'build*/**'), [ join(ROOT, 'build', 'sub', '2.out', 'deeper'), From 26469d81b027819a8b5e0c9e656430eca072d0e2 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 2 Oct 2026 19:23:12 +0000 Subject: [PATCH 2/2] linux: say no more of the restart than holds Comments only. The walk without a turn keeps timers and I/O callbacks out, not promise continuations, nor anything that runs while the network is still being initialized, so it is not a proof that the wrap ends. And a configuration installed after the last check is the next wrap's. --- src/sandbox/sandbox-manager.ts | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index e28c4935a..7620fab31 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -1856,13 +1856,18 @@ async function wrapWithSandboxAgain( // // INVARIANT: what a wrap hands out was read from the disk after its // configuration was installed. A host that writes a file and then installs - // a rule that denies it must find it denied. So a wrap that starts over - // keeps nothing of what it has listed or walked, the same rules included. + // a rule that denies it finds it denied by every wrap that goes by that + // rule. So a wrap that starts over keeps nothing of what it has listed or + // walked, the same rules included. A configuration installed after this + // check is the next wrap's. // - // INVARIANT: it ends. An embedder can replace the configuration many times - // during one walk. After RESTARTS_IN_TURNS the wrap walks without a turn of - // the event loop, so no timer or callback replaces it meanwhile. That holds - // the thread for one walk, as every wrap did before the walk took turns. + // An embedder can replace the configuration many times during one walk. + // After RESTARTS_IN_TURNS the wrap walks without a turn of the event loop, + // so no timer or I/O callback replaces it meanwhile. That holds the thread + // for one walk, as every wrap did before the walk took turns. It is not a + // proof that the wrap ends: a promise continuation, or anything at all + // while the network is still being initialized, can replace it between the + // walk and this check, and the wrap then starts over once more. if (config !== startedWith) return startOver() // Check custom config to allow pseudo-terminal (can be applied dynamically)