From cb8df094ceea809a7d433818f4131b51d5fdee2f Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 2 Oct 2026 20:26:48 +0000 Subject: [PATCH 1/2] linux: try a directory that cannot be listed once, and give each path once The patterns of one configuration share their listings, but only the ones that succeeded, so every pattern tried an unlistable directory again. And every `**` pattern over one base returns those directories to be denied whole, so the list the Linux wrapper looks at, entry by entry, held each of them once per pattern. A failed listing is now kept for as long as a successful one, and the resolved list holds each path once. The wrapped command is the same. --- src/sandbox/sandbox-manager.ts | 12 +++-- src/sandbox/sandbox-utils.ts | 21 +++++--- test/sandbox/glob-expand.test.ts | 82 ++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 11 deletions(-) diff --git a/src/sandbox/sandbox-manager.ts b/src/sandbox/sandbox-manager.ts index 7620fab31..d4438d9f6 100644 --- a/src/sandbox/sandbox-manager.ts +++ b/src/sandbox/sandbox-manager.ts @@ -1267,6 +1267,10 @@ function unionDenyReadPaths( * credential file that degraded to deny) — each names one file on disk, so * it is passed through whatever characters it contains. Expanded as a * pattern, a name holding `[` matches nothing and the deny is lost. + * + * Each path once, where it first came up: every `**` pattern over one base + * returns the directories it could not list, and the Linux wrapper looks at + * every entry it is given. */ function* resolveReadPathEntries( paths: readonly string[], @@ -1274,7 +1278,7 @@ function* resolveReadPathEntries( literalPaths: readonly string[] = [], ): Steps { const literal = new Set(literalPaths) - const resolved: string[] = [] + const resolved = new Set() for (const p of paths) { const stripped = removeTrailingGlobSuffix(p) if ( @@ -1282,12 +1286,12 @@ function* resolveReadPathEntries( !literal.has(p) && containsGlobChars(stripped) ) { - resolved.push(...(yield* expandGlob(p))) + for (const found of yield* expandGlob(p)) resolved.add(found) } else { - resolved.push(stripped) + resolved.add(stripped) } } - return resolved + return [...resolved] } /** diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index e17a72854..4c8bf4bf7 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1137,9 +1137,9 @@ 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 listing a real directory gave, its entries or the error: a second + * position, or a second pattern, reads the same answer. */ +export type GlobWalkListings = Map export type GlobWalkOptions = ExpandGlobOptions & { withDirectoryForm?: boolean @@ -1669,13 +1669,20 @@ export function* walkGlobPatternSteps( if (fresh.length === 0) continue yield for (const p of fresh) listed.add(p) - let entries = listings.get(real) + let entries: fs.Dirent[] try { - entries ??= onRealPath(real, frame.short, p => - fs.readdirSync(p, { withFileTypes: true }), - ) + const known = listings.get(real) + if (known instanceof Error) throw known + entries = + known ?? + onRealPath(real, frame.short, p => + fs.readdirSync(p, { withFileTypes: true }), + ) listings.set(real, entries) } catch (err) { + // Kept as well, or every pattern would try the directory again. One + // that would have cleared meanwhile hides more, never less. + listings.set(real, err instanceof Error ? err : new Error(String(err))) const errorCode = (err as NodeJS.ErrnoException | undefined)?.code logForDebugging( `[Sandbox] Error listing ${dir} for glob pattern ${globPath}: ${err}`, diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index c039b5a9a..a295721ff 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -639,6 +639,53 @@ describe.if(!isWindows)('walkGlobPattern', () => { } }) + it.if(process.getuid?.() !== 0)( + 'tries a directory it cannot list once for all the patterns handed the same listings', + () => { + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-failed-'))) + const locked = join(root, 'locked') + try { + mkdirSync(join(root, 'open')) + mkdirSync(locked) + chmodSync(locked, 0o000) + const patterns = ['**/.env', '**/*.pem', '**/*.key'].map(p => + join(root, p), + ) + const alone = patterns.map(p => walkGlobPattern(p).unlisted) + + const tried: string[] = [] + const readdirSync = fs.readdirSync + const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( + ...args: Parameters + ) => { + tried.push(String(args[0])) + // Gone since its parent was listed: absent, for every pattern. + if (String(args[0]) === join(root, 'open')) { + throw Object.assign(new Error('gone'), { code: 'ENOENT' }) + } + return readdirSync(...args) + }) as typeof fs.readdirSync) + let together + try { + const listings = new Map() + together = patterns.map( + p => walkGlobPattern(p, { listings }).unlisted, + ) + } finally { + readdirSpy.mockRestore() + } + + expect(tried.sort()).toEqual([root, locked, join(root, 'open')].sort()) + // Every pattern still has it to deny whole. + expect(together).toEqual(alone) + expect(together).toEqual(patterns.map(() => [locked])) + } finally { + chmodSync(locked, 0o755) + 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 @@ -1358,6 +1405,41 @@ describe.if(isLinux)('getFsReadConfig with glob patterns on Linux', () => { } }) + it.if(process.getuid?.() !== 0)( + 'gives each path once, however many patterns came to it', + async () => { + const { SandboxManager } = await import( + '../../src/sandbox/sandbox-manager.js' + ) + const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-once-'))) + const locked = join(root, 'locked') + mkdirSync(locked) + chmodSync(locked, 0o000) + writeFileSync(join(root, 'id.pem'), '') + + await SandboxManager.reset() + await SandboxManager.initialize({ + network: { allowedDomains: [], deniedDomains: [] }, + filesystem: { + // All three could not list `locked`, and two match the file. + denyRead: ['**/*.pem', '**/id.*', '**/.env'].map(p => join(root, p)), + allowWrite: [], + denyWrite: [], + }, + }) + try { + expect(SandboxManager.getFsReadConfig().denyOnly).toEqual([ + join(root, 'id.pem'), + locked, + ]) + } finally { + await SandboxManager.reset() + chmodSync(locked, 0o755) + rmSync(root, { recursive: true, force: true }) + } + }, + ) + 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( From e6fadd763e3be83052244e85a2cd907ec2080862 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 2 Oct 2026 20:33:01 +0000 Subject: [PATCH 2/2] linux: do not keep a listing that found the directory absent The commit before this one kept every failed listing for the patterns that follow. An absence is the one failure that denies nothing, so a pattern handed another's absence missed what it matches in a directory that is there when it would have asked. Only the failures that have a directory denied whole are kept. --- src/sandbox/sandbox-utils.ts | 12 +++++++----- test/sandbox/glob-expand.test.ts | 29 +++++++++++++++++++---------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 4c8bf4bf7..03663ca46 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -1137,8 +1137,9 @@ export interface ExpandGlobOptions { caseInsensitive?: boolean } -/** What listing a real directory gave, its entries or the error: a second - * position, or a second pattern, reads the same answer. */ +/** What listing a real directory gave, its entries or the error that has it + * denied whole: a second position, or a second pattern, reads the same + * answer. */ export type GlobWalkListings = Map export type GlobWalkOptions = ExpandGlobOptions & { @@ -1680,15 +1681,16 @@ export function* walkGlobPatternSteps( ) listings.set(real, entries) } catch (err) { - // Kept as well, or every pattern would try the directory again. One - // that would have cleared meanwhile hides more, never less. - listings.set(real, err instanceof Error ? err : new Error(String(err))) const errorCode = (err as NodeJS.ErrnoException | undefined)?.code logForDebugging( `[Sandbox] Error listing ${dir} for glob pattern ${globPath}: ${err}`, { level: errorCode === 'ENOENT' ? 'info' : 'warn' }, ) if (errorCode !== 'ENOENT') { + // Kept, or every pattern would try the directory again: one that + // would have cleared meanwhile hides more. An absence is not kept. It + // is the one answer that denies nothing, so each pattern asks. + listings.set(real, err instanceof Error ? err : new Error(String(err))) walk.unlisted.push(dir) if (real !== dir) walk.realOf.set(dir, real) } diff --git a/test/sandbox/glob-expand.test.ts b/test/sandbox/glob-expand.test.ts index a295721ff..166803200 100644 --- a/test/sandbox/glob-expand.test.ts +++ b/test/sandbox/glob-expand.test.ts @@ -644,8 +644,10 @@ describe.if(!isWindows)('walkGlobPattern', () => { () => { const root = realPath(mkdtempSync(join(tmpdir(), 'glob-walk-failed-'))) const locked = join(root, 'locked') + const open = join(root, 'open') try { - mkdirSync(join(root, 'open')) + mkdirSync(open) + writeFileSync(join(open, 'id.pem'), '') mkdirSync(locked) chmodSync(locked, 0o000) const patterns = ['**/.env', '**/*.pem', '**/*.key'].map(p => @@ -658,9 +660,11 @@ describe.if(!isWindows)('walkGlobPattern', () => { const readdirSpy = spyOn(fs, 'readdirSync').mockImplementation((( ...args: Parameters ) => { - tried.push(String(args[0])) - // Gone since its parent was listed: absent, for every pattern. - if (String(args[0]) === join(root, 'open')) { + const dir = String(args[0]) + const askedBefore = tried.includes(dir) + tried.push(dir) + // Absent when the first pattern asks, and there for the others. + if (dir === open && !askedBefore) { throw Object.assign(new Error('gone'), { code: 'ENOENT' }) } return readdirSync(...args) @@ -668,17 +672,22 @@ describe.if(!isWindows)('walkGlobPattern', () => { let together try { const listings = new Map() - together = patterns.map( - p => walkGlobPattern(p, { listings }).unlisted, - ) + together = patterns.map(p => walkGlobPattern(p, { listings })) } finally { readdirSpy.mockRestore() } - expect(tried.sort()).toEqual([root, locked, join(root, 'open')].sort()) + expect(tried.filter(dir => dir === locked)).toEqual([locked]) // Every pattern still has it to deny whole. - expect(together).toEqual(alone) - expect(together).toEqual(patterns.map(() => [locked])) + expect(together.map(walk => walk.unlisted)).toEqual(alone) + expect(alone).toEqual(patterns.map(() => [locked])) + // An absence denies nothing, so it is nobody's answer but the asker's. + expect(tried.filter(dir => dir === open)).toEqual([open, open]) + expect(together.map(walk => walk.matches)).toEqual([ + [], + [join(open, 'id.pem')], + [], + ]) } finally { chmodSync(locked, 0o755) rmSync(root, { recursive: true, force: true })