From ad5c370f663b08500e130b9cce43a4bfbbc42518 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Mon, 28 Sep 2026 18:11:23 +0000 Subject: [PATCH] linux: bind a deny path that another sandbox makes while the wrap is looking An absent deny path is blocked by a mount point that bubblewrap makes on the host. The wrap looks whether the path exists and then whether a component of it is a file, since nothing can be created beneath a file (a worktree's `.git`). That second look included the path itself. Two processes wrapping in one project deny the same absent paths, so one's bubblewrap can make its empty file there between the two looks; the path then counted as lying beneath a file and got no bind at all, for the whole command. The second look now asks about what lies above the path. --- src/sandbox/linux-sandbox-utils.ts | 11 ++- test/sandbox/deny-path-made-meanwhile.test.ts | 87 +++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 test/sandbox/deny-path-made-meanwhile.test.ts diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index b09532eaa..4b22a9557 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -2414,11 +2414,14 @@ async function generateFilesystemArgs( // We track them in bwrapMountPoints so cleanupBwrapMountPoints() can // remove them after the command exits. if (!fs.existsSync(normalizedPath)) { - // Fix 1 (worktree): If any existing component in the deny path is a - // file (not a directory), skip the deny entirely. You can't mkdir + // Fix 1 (worktree): If any existing component above the deny path is + // a file (not a directory), skip the deny entirely. You can't mkdir // under a file, so the deny path can never be created. This handles - // git worktrees where .git is a file. - if (hasFileAncestor(normalizedPath)) { + // git worktrees where .git is a file. Asked of what lies above the + // path, not of the path: another sandbox's bubblewrap may have made a + // mount point at it since the look above, and a file there is no + // reason to leave it unbound. + if (hasFileAncestor(path.dirname(normalizedPath))) { logForDebugging( `[Sandbox Linux] Skipping deny path with file ancestor (cannot create paths under a file): ${normalizedPath}`, ) diff --git a/test/sandbox/deny-path-made-meanwhile.test.ts b/test/sandbox/deny-path-made-meanwhile.test.ts new file mode 100644 index 000000000..30eb6693a --- /dev/null +++ b/test/sandbox/deny-path-made-meanwhile.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, beforeEach, afterEach, spyOn } from 'bun:test' +import * as fs from 'fs' +import { + chmodSync, + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { + wrapCommandWithSandboxLinux, + cleanupBwrapMountPoints, +} from '../../src/sandbox/linux-sandbox-utils.js' +import { isLinux } from '../helpers/platform.js' +import { countMounts } from '../helpers/bwrap-argv.js' + +/** + * An absent deny path is blocked by a mount point bubblewrap makes on the + * host. Two processes wrapping in one project deny the same absent paths, so + * one's bubblewrap can make that file between two looks of the other's wrap. + */ +describe.if(isLinux)('A deny path made while the wrap is looking', () => { + let base: string + let area: string + const savedCwd = process.cwd() + + beforeEach(() => { + cleanupBwrapMountPoints({ force: true }) + base = realpathSync(mkdtempSync(join(tmpdir(), 'deny-meanwhile-'))) + area = join(base, 'area') + mkdirSync(area) + process.chdir(area) + }) + + afterEach(() => { + process.chdir(savedCwd) + cleanupBwrapMountPoints({ force: true }) + rmSync(base, { recursive: true, force: true }) + }) + + const wrap = (deny: string): Promise => + wrapCommandWithSandboxLinux({ + command: 'true', + needsNetworkRestriction: false, + readConfig: { denyOnly: [] }, + writeConfig: { allowOnly: [area], denyWithinAllow: [deny] }, + }) + + it('still binds over a path that was absent when asked and is a file a moment later', async () => { + const denied = join(area, 'denied.lock') + const exists = fs.existsSync + let made = false + // What another sandbox's bubblewrap does: an empty read-only file. + const spy = spyOn(fs, 'existsSync').mockImplementation((( + p: fs.PathLike, + ) => { + const was = exists(p) + if (p === denied && !made) { + made = true + writeFileSync(denied, '') + chmodSync(denied, 0o444) + } + return was + }) as typeof fs.existsSync) + let wrapped: string + try { + wrapped = await wrap(denied) + } finally { + spy.mockRestore() + } + + expect(made).toBe(true) + expect(countMounts(wrapped, '--ro-bind', '/dev/null', denied)).toBe(1) + }) + + it('still leaves alone a path beneath a file, which can never be created', async () => { + writeFileSync(join(area, '.git'), 'gitdir: elsewhere\n') + const denied = join(area, '.git', 'hooks') + + const wrapped = await wrap(denied) + + expect(wrapped).not.toContain(denied) + }) +})