From 2ba7f658c602a078dc92ceac8fda631b094d27d5 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Thu, 27 Aug 2026 05:18:34 +0000 Subject: [PATCH 1/5] fix(linux): skip write-deny binds already covered by a read-only denied directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When denyWithinAllow re-binds a directory read-only (--ro-bind ), every EXISTING deny path strictly beneath it — typically the mandatory-deny files of a write-protected checkout (.git/hooks, .git/config, .mcp.json, ...) — is already unwritable there, yet each still got its own --ro-bind

. The non-existent-path branch already skips its creation-blocking stub in exactly this situation; this is the existing-path twin. The skip uses the same evidence and the same vetoes: the covering directory must come from the order-independent pre-pass (so the caller's deny ordering does not matter) and pass coveringDirIsUnsafe (no allowed write path strictly beneath it, incomparable with every read-deny tmpfs), which also guarantees the covering bind survives the emission filter. Only STRICT descendants are skipped — a deny that equals an allowWrite root (cwd both allowed and denied) is the covering bind itself. A dest reached through a symlinked spelling keeps its bind, because the tmpfs/mask re-application passes key off emitted raw spellings that the covering directory's bind does not carry. Non-existent paths, the symlink checks and /dev handling are untouched. --- src/sandbox/linux-sandbox-utils.ts | 30 +++- test/sandbox/readonly-deny-dir-binds.test.ts | 141 +++++++++++++++++++ 2 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 test/sandbox/readonly-deny-dir-binds.test.ts diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index e31685cfe..270c4511c 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -927,7 +927,9 @@ async function generateFilesystemArgs( // path whose deepest existing ancestor lies within one of these is already // uncreatable, and must not get a /dev/null stub: bwrap would have to // creat() the mount point inside that read-only mount and abort ("Can't - // create file at : Read-only file system"). The spellings matter + // create file at : Read-only file system"). An EXISTING deny path + // strictly beneath one is likewise already unwritable and its own + // --ro-bind

is skipped as redundant. The spellings matter // because the emission filter and the denyRead re-application compare raw // spellings as well as the resolved dest, so the stub-skip guard tests a // covering directory in its canonical form AND every recorded spelling. @@ -1423,6 +1425,32 @@ async function generateFilesystemArgs( const isWithinAllowedPath = isWithinAnyAllowedWritePath(normalizedPath) if (isWithinAllowedPath) { + // The existing-path twin of the stub skip above: a path STRICTLY + // beneath a directory that another deny re-binds read-only is + // already unwritable there, so its own --ro-bind

is a + // redundant mount (one per mandatory-deny file under a write-denied + // checkout adds up). Same evidence, same vetoes: the covering + // directory comes from the order-independent pre-pass and must pass + // coveringDirIsUnsafe, which also guarantees its bind survives the + // emission filter. Equality is deliberately excluded — a deny that + // IS an allowWrite root (cwd both allowed and denied) is the covering + // bind itself. A dest reached through a symlinked spelling keeps its + // bind: the tmpfs/mask re-application passes below key off emitted + // raw spellings, and the covering directory's bind does not carry + // this one's. + const coveringReadOnlyDenyDirs = readOnlyDenyDirs.filter(denyDir => + normalizedPath.startsWith(denyDir + '/'), + ) + if ( + rawPath === normalizedPath && + coveringReadOnlyDenyDirs.length > 0 && + !coveringReadOnlyDenyDirs.some(coveringDirIsUnsafe) + ) { + logForDebugging( + `[Sandbox Linux] Skipping deny path already under read-only denied directory: ${normalizedPath}`, + ) + continue + } denyWriteArgs.push('--ro-bind', normalizedPath, normalizedPath) denyWriteRawDests.set(normalizedPath, rawPath) } else { diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts new file mode 100644 index 000000000..5631802db --- /dev/null +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -0,0 +1,141 @@ +import { describe, it, expect, beforeEach, afterEach } from 'bun:test' +import { + mkdirSync, + mkdtempSync, + realpathSync, + rmSync, + symlinkSync, + 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' + +/** + * Companion to readonly-deny-dir-stubs.test.ts for EXISTING deny targets. + * + * When denyWithinAllow re-binds a directory read-only (--ro-bind

+ * ), every existing deny path strictly beneath it — typically the + * mandatory-deny files of a write-protected checkout (.git/hooks, + * .git/config, .mcp.json, …) — is already unwritable, yet each used to get + * its own --ro-bind

. Those redundant mounts are now skipped under the + * same evidence and vetoes as the absent-path stub skip; a deny that equals + * an allowOnly root, or one with no covering read-only directory, is still + * bound. + */ +describe.if(isLinux)('Deny binds under a read-only denied directory', () => { + let BASE: string + let AREA: string // allowed write area + let PROJ: string // project dir inside AREA + let FILE: string // existing file under PROJ/sub + + const savedCwd = process.cwd() + + beforeEach(() => { + BASE = realpathSync(mkdtempSync(join(tmpdir(), 'ro-deny-bind-'))) + AREA = join(BASE, 'area') + PROJ = join(AREA, 'proj') + FILE = join(PROJ, 'sub', 'settings.json') + mkdirSync(join(PROJ, 'sub'), { recursive: true }) + writeFileSync(FILE, '{}\n') + // Keep cwd outside the allowlist so the mandatory-deny scan adds no + // binds of its own to reason about. + process.chdir(BASE) + }) + + afterEach(() => { + process.chdir(savedCwd) + cleanupBwrapMountPoints({ force: true }) + rmSync(BASE, { recursive: true, force: true }) + }) + + async function wrap( + denyPaths: string[], + allowPaths: string[], + readDenyPaths: string[] = [], + ): Promise { + return wrapCommandWithSandboxLinux({ + command: 'echo hello', + needsNetworkRestriction: false, + readConfig: { denyOnly: readDenyPaths }, + writeConfig: { allowOnly: allowPaths, denyWithinAllow: denyPaths }, + }) + } + + const countOccurrences = (haystack: string, needle: string): number => + haystack.split(needle).length - 1 + + it('binds the denied allow-root once and skips the existing file beneath it', async () => { + // allowOnly=[cwd], denyWithinAllow=[cwd, cwd/sub/file]: the directory + // deny equals the allow root and must still be emitted; the file is a + // strict descendant of that read-only bind and needs nothing. + const command = await wrap([PROJ, FILE], [PROJ]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('is independent of the order the denies are listed in', async () => { + const command = await wrap([FILE, PROJ], [PROJ]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('still binds the file when its directory is not itself denied', async () => { + const command = await wrap([FILE], [PROJ]) + + expect(command).toContain(`--bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('collapses a chain of nested directory denies to the outermost bind', async () => { + const sub = join(PROJ, 'sub') + const command = await wrap([PROJ, sub, FILE], [AREA]) + + expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) + expect(command).not.toContain(`--ro-bind ${sub} ${sub}`) + expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('keeps the descendant bind when an allowed write path sits strictly beneath the covering dir (veto)', async () => { + // Same veto as the stub skip: with an allowWrite under PROJ the denyRead + // re-application machinery could re-open part of the subtree, so the + // covering bind is not trusted and the explicit deny keeps its own. + const nestedAllow = join(PROJ, 'w') + mkdirSync(nestedAllow) + + const command = await wrap([PROJ, FILE], [AREA, nestedAllow]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('keeps the descendant bind when a denyRead tmpfs sits under the covering dir (veto)', async () => { + const readDenied = join(PROJ, 'secrets') + mkdirSync(readDenied) + + const command = await wrap([PROJ, FILE], [AREA], [readDenied]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) + + it('keeps the bind for a deny reached through a symlinked spelling', async () => { + // The re-application passes key off emitted raw spellings; a dest that + // was reached via a symlink keeps its bind so that breadcrumb survives. + const realSub = join(PROJ, 'sub') + const linkSub = join(PROJ, 'link') + symlinkSync(realSub, linkSub) + const viaLink = join(linkSub, 'settings.json') + + const command = await wrap([PROJ, viaLink], [AREA]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) + }) +}) From 355f0d11f06f256ad02e1dccc1b4c88687e87bcd Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Fri, 28 Aug 2026 21:55:21 -0400 Subject: [PATCH 2/5] refactor(linux): decide both write-deny skips with one strict predicate The absent-path stub skip and the new existing-path bind skip each spelled their own "covered by a safe read-only deny directory" check. One predicate now serves both: strictly under a recorded directory (a deny equal to one is that covering bind and must be emitted), stopping at the first vetoed directory, allocating nothing. The stub skip tests the absent path itself rather than its deepest existing ancestor; the two agree wherever a recorded directory exists, root included. isAtOrUnder moves to sandbox-utils.ts so both call sites share one spelling of segment-aware containment. The binds test takes its wrap() arguments in the same order as the stubs test beside it. --- src/sandbox/linux-sandbox-utils.ts | 49 ++++++++++---------- src/sandbox/sandbox-utils.ts | 8 ++++ test/sandbox/readonly-deny-dir-binds.test.ts | 34 ++++++-------- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 270c4511c..e318bedfe 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -17,6 +17,7 @@ import { isSymlinkOutsideBoundary, encodeSandboxedCommand, DANGEROUS_FILES, + isAtOrUnder, getDangerousDirectories, } from './sandbox-utils.js' import type { @@ -1256,6 +1257,20 @@ async function generateFilesystemArgs( // Materialized once: the pre-pass above fully populates the map and the // deny loop never mutates it. const readOnlyDenyDirs = [...readOnlyDenyDirSpellings.keys()] + // Is `candidate` already unwritable in the sandbox: strictly under a + // recorded read-only deny directory that survives every + // coveringDirIsUnsafe veto? Strictly, because a deny equal to a recorded + // directory IS that covering bind and must be emitted (an absent path + // never equals one). Stops at the first vetoed covering directory. + const coveredBySafeReadOnlyDenyDir = (candidate: string): boolean => { + let covered = false + for (const denyDir of readOnlyDenyDirs) { + if (candidate === denyDir || !isAtOrUnder(candidate, denyDir)) continue + if (coveringDirIsUnsafe(denyDir)) return false + covered = true + } + return covered + } for (const pathPattern of denyPaths) { const rawPath = normalizePathForSandbox(pathPattern) @@ -1373,13 +1388,11 @@ async function generateFilesystemArgs( // regardless of where it appears in denyPaths. A recorded covering // directory is evidence for skipping only if it survives the // coveringDirIsUnsafe vetoes (see the INVARIANT at its definition). - const coveringReadOnlyDenyDirs = readOnlyDenyDirs.filter( - denyDir => - ancestorPath === denyDir || ancestorPath.startsWith(denyDir + '/'), - ) + // (Tested on the absent path itself: a recorded directory that + // covers it is at-or-above its deepest existing ancestor, since + // recorded directories exist.) const ancestorIsWithinReadOnlyDeny = - coveringReadOnlyDenyDirs.length > 0 && - !coveringReadOnlyDenyDirs.some(coveringDirIsUnsafe) + coveredBySafeReadOnlyDenyDir(normalizedPath) if (ancestorIsWithinAllowedPath && !ancestorIsWithinReadOnlyDeny) { const firstNonExistent = findFirstNonExistentComponent(normalizedPath) @@ -1425,26 +1438,14 @@ async function generateFilesystemArgs( const isWithinAllowedPath = isWithinAnyAllowedWritePath(normalizedPath) if (isWithinAllowedPath) { - // The existing-path twin of the stub skip above: a path STRICTLY - // beneath a directory that another deny re-binds read-only is - // already unwritable there, so its own --ro-bind

is a - // redundant mount (one per mandatory-deny file under a write-denied - // checkout adds up). Same evidence, same vetoes: the covering - // directory comes from the order-independent pre-pass and must pass - // coveringDirIsUnsafe, which also guarantees its bind survives the - // emission filter. Equality is deliberately excluded — a deny that - // IS an allowWrite root (cwd both allowed and denied) is the covering - // bind itself. A dest reached through a symlinked spelling keeps its - // bind: the tmpfs/mask re-application passes below key off emitted - // raw spellings, and the covering directory's bind does not carry - // this one's. - const coveringReadOnlyDenyDirs = readOnlyDenyDirs.filter(denyDir => - normalizedPath.startsWith(denyDir + '/'), - ) + // Already unwritable under a read-only denied directory (the + // existing-path twin of the stub skip above). Veto (iii) keeps the + // covering bind through the emission filter; a symlinked spelling + // keeps its own bind because the re-application passes below key + // off emitted raw spellings. if ( rawPath === normalizedPath && - coveringReadOnlyDenyDirs.length > 0 && - !coveringReadOnlyDenyDirs.some(coveringDirIsUnsafe) + coveredBySafeReadOnlyDenyDir(normalizedPath) ) { logForDebugging( `[Sandbox Linux] Skipping deny path already under read-only denied directory: ${normalizedPath}`, diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 933585175..53ec8fb06 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -52,6 +52,14 @@ export function normalizeCaseForComparison(pathStr: string): string { return pathStr.toLowerCase() } +/** + * `p` is `dir` itself or lies beneath it, by path segment ('/x' is not under + * '/xy'); root-aware, since '/' + '/' is a prefix of nothing. + */ +export function isAtOrUnder(p: string, dir: string): boolean { + return p === dir || p.startsWith(dir === '/' ? '/' : dir + '/') +} + /** * Check if a path pattern contains glob characters */ diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts index 5631802db..085b511e5 100644 --- a/test/sandbox/readonly-deny-dir-binds.test.ts +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -16,16 +16,10 @@ import { import { isLinux } from '../helpers/platform.js' /** - * Companion to readonly-deny-dir-stubs.test.ts for EXISTING deny targets. - * - * When denyWithinAllow re-binds a directory read-only (--ro-bind

- * ), every existing deny path strictly beneath it — typically the - * mandatory-deny files of a write-protected checkout (.git/hooks, - * .git/config, .mcp.json, …) — is already unwritable, yet each used to get - * its own --ro-bind

. Those redundant mounts are now skipped under the - * same evidence and vetoes as the absent-path stub skip; a deny that equals - * an allowOnly root, or one with no covering read-only directory, is still - * bound. + * A deny path strictly beneath a directory that denyWithinAllow re-binds + * read-only gets no --ro-bind of its own, under the same evidence and vetoes + * as the absent-path stub skip (readonly-deny-dir-stubs.test.ts); a deny + * equal to an allowOnly root, or with no covering directory, is still bound. */ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { let BASE: string @@ -53,10 +47,12 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { rmSync(BASE, { recursive: true, force: true }) }) + // Same parameter order as readonly-deny-dir-stubs.test.ts, whose fixture + // and covering-directory predicate this suite shares. async function wrap( denyPaths: string[], - allowPaths: string[], readDenyPaths: string[] = [], + allowPaths: string[] = [AREA], ): Promise { return wrapCommandWithSandboxLinux({ command: 'echo hello', @@ -70,24 +66,24 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { haystack.split(needle).length - 1 it('binds the denied allow-root once and skips the existing file beneath it', async () => { - // allowOnly=[cwd], denyWithinAllow=[cwd, cwd/sub/file]: the directory + // allowOnly=[proj], denyWithinAllow=[proj, proj/sub/file]: the directory // deny equals the allow root and must still be emitted; the file is a // strict descendant of that read-only bind and needs nothing. - const command = await wrap([PROJ, FILE], [PROJ]) + const command = await wrap([PROJ, FILE], [], [PROJ]) expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) }) it('is independent of the order the denies are listed in', async () => { - const command = await wrap([FILE, PROJ], [PROJ]) + const command = await wrap([FILE, PROJ], [], [PROJ]) expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) }) it('still binds the file when its directory is not itself denied', async () => { - const command = await wrap([FILE], [PROJ]) + const command = await wrap([FILE], [], [PROJ]) expect(command).toContain(`--bind ${PROJ} ${PROJ}`) expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) @@ -95,7 +91,7 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { it('collapses a chain of nested directory denies to the outermost bind', async () => { const sub = join(PROJ, 'sub') - const command = await wrap([PROJ, sub, FILE], [AREA]) + const command = await wrap([PROJ, sub, FILE]) expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) expect(command).not.toContain(`--ro-bind ${sub} ${sub}`) @@ -109,7 +105,7 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { const nestedAllow = join(PROJ, 'w') mkdirSync(nestedAllow) - const command = await wrap([PROJ, FILE], [AREA, nestedAllow]) + const command = await wrap([PROJ, FILE], [], [AREA, nestedAllow]) expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) @@ -119,7 +115,7 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { const readDenied = join(PROJ, 'secrets') mkdirSync(readDenied) - const command = await wrap([PROJ, FILE], [AREA], [readDenied]) + const command = await wrap([PROJ, FILE], [readDenied]) expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) @@ -133,7 +129,7 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { symlinkSync(realSub, linkSub) const viaLink = join(linkSub, 'settings.json') - const command = await wrap([PROJ, viaLink], [AREA]) + const command = await wrap([PROJ, viaLink]) expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) From 00e00c4863568b970d0e96f3e6a8daf7c8aa80ec Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Sat, 29 Aug 2026 02:36:13 -0400 Subject: [PATCH 3/5] fix(linux): make the covering-directory vetoes and re-application root-aware A recorded '/' compared by string prefix was judged safe for every path: the existing-path skip dropped a project's own bind and the mask beneath it was never re-applied after --ro-bind / /. Containment now uses isAtOrUnder throughout, and a vetoed '/' neither covers a path nor disqualifies an inner recorded directory. Tests pin the root case, the re-application after a lone root bind, a string-prefix sibling, and the helper itself. --- src/sandbox/linux-sandbox-utils.ts | 62 +++++++---- test/sandbox/readonly-deny-dir-binds.test.ts | 109 ++++++++++++++++++- test/sandbox/symlink-boundary.test.ts | 12 ++ 3 files changed, 162 insertions(+), 21 deletions(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index e318bedfe..792ce987b 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -1008,10 +1008,10 @@ async function generateFilesystemArgs( allowedWritePaths.push(normalizedPath) } - // Inputs for the stub-skip guard's vetoes, computed at most once and only - // when an absent deny path actually has a covering read-only deny dir (an - // uncommon configuration) — ordinary commands skip the extra - // stat/realpath/readdir syscalls entirely. Lazy evaluation also means the + // Inputs for the covering-directory vetoes, computed at most once and + // only when a deny path (absent or existing) lies strictly beneath a + // recorded read-only deny dir — commands with no such covering directory + // skip the extra stat/realpath/readdir syscalls entirely. Lazy evaluation also means the // derivation runs from inside the deny loop, AFTER the (unbounded) // mandatory-deny ripgrep await below, keeping the snapshot as close as // possible to the denyRead loop that later acts on the real filesystem. @@ -1197,11 +1197,16 @@ async function generateFilesystemArgs( // Per-covering-dir veto verdict, computed once per recorded directory // (the inputs never change during the deny loop) instead of per absent // deny entry. - // INVARIANT: a stub is skipped only under a recorded covering deny - // directory that has no allowed write path strictly beneath it and is - // INCOMPARABLE with every read-deny tmpfs directory (neither - // at-or-beneath it nor containing it or any spelling it was reached - // through). Rationale: the only writable emissions that land after the + // INVARIANT: a stub, or an existing deny path's own bind, is skipped + // only under a recorded covering deny directory that has no allowed + // write path strictly beneath it and is INCOMPARABLE with every + // read-deny tmpfs directory (neither at-or-beneath it nor containing it + // or any spelling it was reached through). Containment is root-aware + // (isAtOrUnder): '/' is a recordable covering directory when allowOnly + // and denyWithinAllow both name it, and '/' + '/' is a prefix of + // nothing, so a string-prefix test would judge it safe for every path + // and drop the binds the re-application passes below key off. + // Rationale: the only writable emissions that land after the // buffered read-only binds are the denyRead re-applications // (pushReadDenyDirMounts), which mount a tmpfs and re-bind allowed write // paths beneath it WITHOUT re-emitting the binds it buries — so a @@ -1231,14 +1236,13 @@ async function generateFilesystemArgs( const unsafe = // (i) an allowed write path strictly beneath the dir: the // re-application's effect would re-bind it writable. - allowedWritePathsBothForms.some(writePath => - writePath.startsWith(denyDir + '/'), + allowedWritePathsBothForms.some( + writePath => writePath !== denyDir && isAtOrUnder(writePath, denyDir), ) || // (ii) a read-deny tmpfs at or beneath the dir: the re-application's // trigger. - prospectiveReadDenyTmpfsDirsBothForms.some( - tmpfsDir => - tmpfsDir === denyDir || tmpfsDir.startsWith(denyDir + '/'), + prospectiveReadDenyTmpfsDirsBothForms.some(tmpfsDir => + isAtOrUnder(tmpfsDir, denyDir), ) || // (iii) a read-deny tmpfs CONTAINING the dir or any raw spelling it // was reached through: the dir's own --ro-bind can be dropped as @@ -1247,8 +1251,7 @@ async function generateFilesystemArgs( // is not reliably read-only in the sandbox. prospectiveReadDenyTmpfsDirsBothForms.some(tmpfsDir => [denyDir, ...(readOnlyDenyDirSpellings.get(denyDir) ?? [])].some( - spelling => - spelling === tmpfsDir || spelling.startsWith(tmpfsDir + '/'), + spelling => isAtOrUnder(spelling, tmpfsDir), ), ) coveringDirUnsafeVerdicts.set(denyDir, unsafe) @@ -1266,7 +1269,16 @@ async function generateFilesystemArgs( let covered = false for (const denyDir of readOnlyDenyDirs) { if (candidate === denyDir || !isAtOrUnder(candidate, denyDir)) continue - if (coveringDirIsUnsafe(denyDir)) return false + if (coveringDirIsUnsafe(denyDir)) { + // A vetoed '/' neither covers a path nor disqualifies an inner + // recorded directory: everything lies beneath it, so it would + // veto every skip and stub each absent mandatory-deny path of a + // write-denied cwd after that cwd's own bind — the startup abort. + // Its descendants are decided by their own recorded directories, + // as on main, whose root-blind filter never recorded '/'. + if (denyDir === '/') continue + return false + } covered = true } return covered @@ -1623,9 +1635,15 @@ async function generateFilesystemArgs( // The inverse stacking problem: a denyWrite ro-bind whose dest strictly // contains a read-denied dir re-exposes that dir's real contents (the bind // landed after the tmpfs). Re-apply the tmpfs on top, with the same write - // and allowRead re-binds the denyRead loop emitted. + // and allowRead re-binds the denyRead loop emitted. A bind of '/' itself + // (allowOnly and denyWithinAllow both naming it) contains every one of + // them, so containment is root-aware. for (const tmpfsDir of tmpfsDirs) { - if (emittedDenyWriteDests.some(dest => tmpfsDir.startsWith(dest + '/'))) { + if ( + emittedDenyWriteDests.some( + dest => tmpfsDir !== dest && isAtOrUnder(tmpfsDir, dest), + ) + ) { logForDebugging( `[Sandbox Linux] Re-applying denyRead tmpfs re-exposed by denyWrite bind: ${tmpfsDir}`, ) @@ -1636,7 +1654,11 @@ async function generateFilesystemArgs( // ancestor bind, so the real file is back. Re-apply the mask with its // original source (/dev/null for read-deny, the fake for credential mask). for (const [maskedFile, source] of maskedFiles) { - if (emittedDenyWriteDests.some(dest => maskedFile.startsWith(dest + '/'))) { + if ( + emittedDenyWriteDests.some( + dest => maskedFile !== dest && isAtOrUnder(maskedFile, dest), + ) + ) { // maskedFiles holds both the symlink path and its resolved target so // the denyWrite skip-check above matches either. Re-emission must go // to the target only — bwrap rejects a symlink bind dest (see diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts index 085b511e5..6708d3bc6 100644 --- a/test/sandbox/readonly-deny-dir-binds.test.ts +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -2,11 +2,13 @@ import { describe, it, expect, beforeEach, afterEach } from 'bun:test' import { mkdirSync, mkdtempSync, + readFileSync, realpathSync, rmSync, symlinkSync, writeFileSync, } from 'node:fs' +import { spawnSync } from 'node:child_process' import { join } from 'node:path' import { tmpdir } from 'node:os' import { @@ -29,6 +31,26 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { const savedCwd = process.cwd() + // Runtime arm, as in readonly-deny-dir-stubs.test.ts: only where bwrap can + // run the namespace/proc surface the wrapped commands use. + const BWRAP_CAN_NAMESPACE = + spawnSync( + 'bwrap', + [ + '--unshare-pid', + '--unshare-user', + '--cap-drop', + 'ALL', + '--ro-bind', + '/', + '/', + '--proc', + '/proc', + 'true', + ], + { timeout: 5000 }, + ).status === 0 + beforeEach(() => { BASE = realpathSync(mkdtempSync(join(tmpdir(), 'ro-deny-bind-'))) AREA = join(BASE, 'area') @@ -53,9 +75,10 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { denyPaths: string[], readDenyPaths: string[] = [], allowPaths: string[] = [AREA], + command = 'echo hello', ): Promise { return wrapCommandWithSandboxLinux({ - command: 'echo hello', + command, needsNetworkRestriction: false, readConfig: { denyOnly: readDenyPaths }, writeConfig: { allowOnly: allowPaths, denyWithinAllow: denyPaths }, @@ -73,6 +96,28 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { expect(countOccurrences(command, `--ro-bind ${PROJ} ${PROJ}`)).toBe(1) expect(command).not.toContain(`--ro-bind ${FILE} ${FILE}`) + + // Where the host can run bwrap, prove the covering bind alone still + // holds: the file reads, and a write through it fails and changes + // nothing on the host. + if (BWRAP_CAN_NAMESPACE) { + const run = (wrapped: string) => + spawnSync(wrapped, { + shell: true, + encoding: 'utf8', + timeout: 15000, + cwd: BASE, + }) + const read = run(await wrap([PROJ, FILE], [], [PROJ], `cat ${FILE}`)) + expect(read.status).toBe(0) + expect(read.stdout).toContain('{}') + + const write = run( + await wrap([PROJ, FILE], [], [PROJ], `sh -c 'echo x >> ${FILE}'`), + ) + expect(write.status).not.toBe(0) + expect(readFileSync(FILE, 'utf8')).toBe('{}\n') + } }) it('is independent of the order the denies are listed in', async () => { @@ -121,6 +166,68 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { expect(command).toContain(`--ro-bind ${FILE} ${FILE}`) }) + it('does not trust a recorded "/" as a covering directory', async () => { + // allowOnly and denyWithinAllow both naming '/' records it as a + // read-only deny directory that every path lies beneath. A string-prefix + // veto ('/' + '/') could never fire, so PROJ's own bind would be dropped + // as covered; the recursive --ro-bind / / emitted later then shadows the + // FILE mask with no bind left to key its re-application off, and the + // read-denied file is readable. Root-aware containment vetoes '/' (AREA + // is an allowed write path beneath it), keeps PROJ's bind, and re-applies + // the mask after the root bind. + const command = await wrap(['/', PROJ], [FILE], ['/', AREA]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + const rootBind = command.lastIndexOf('--ro-bind / /') + const mask = command.lastIndexOf(`--ro-bind /dev/null ${FILE}`) + expect(rootBind).toBeGreaterThan(-1) + expect(mask).toBeGreaterThan(rootBind) + }) + + it('skips the stubs under a write-denied cwd even when a recorded "/" is vetoed', async () => { + // '/' recorded and vetoed (AREA is writable beneath it). A veto that + // disqualified every skip would stub each absent mandatory-deny dotfile + // of the write-denied cwd after the cwd's own bind — the startup abort + // readonly-deny-dir-stubs.test.ts documents. The cwd's recorded bind + // decides instead, as on main. + process.chdir(PROJ) + const command = await wrap(['/', PROJ], [], ['/', AREA]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).not.toContain(`/dev/null ${PROJ}/`) + expect(command).not.toMatch(/--ro-bind \S*claude-empty-\S+ \S*\/proj\//) + }) + + it('re-applies a read-deny mask and tmpfs shadowed by a bind of "/" alone', async () => { + // '/' is the only emitted deny bind. main compared the bind by string + // prefix ('/' + '/') and re-applied nothing after --ro-bind / /, so the + // recursive root bind left the file and the directory readable. + const secrets = join(PROJ, 'secrets') + mkdirSync(secrets) + const command = await wrap(['/'], [FILE, secrets], ['/']) + + const rootBind = command.lastIndexOf('--ro-bind / /') + expect(rootBind).toBeGreaterThan(-1) + expect(command.lastIndexOf(`--ro-bind /dev/null ${FILE}`)).toBeGreaterThan( + rootBind, + ) + expect(command.lastIndexOf(`--tmpfs ${secrets}`)).toBeGreaterThan(rootBind) + }) + + it('does not treat a string-prefix sibling as covered', async () => { + // AREA/proj2/x.txt shares a prefix with AREA/proj without lying beneath + // it: a plain startsWith would drop its bind and leave it writable. + const proj2 = join(AREA, 'proj2') + mkdirSync(proj2) + const sibling = join(proj2, 'x.txt') + writeFileSync(sibling, '') + + const command = await wrap([PROJ, sibling]) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + expect(command).toContain(`--ro-bind ${sibling} ${sibling}`) + }) + it('keeps the bind for a deny reached through a symlinked spelling', async () => { // The re-application passes key off emitted raw spellings; a dest that // was reached via a symlink keeps its bind so that breadcrumb survives. diff --git a/test/sandbox/symlink-boundary.test.ts b/test/sandbox/symlink-boundary.test.ts index 692926166..ed5b9ca3b 100644 --- a/test/sandbox/symlink-boundary.test.ts +++ b/test/sandbox/symlink-boundary.test.ts @@ -4,6 +4,7 @@ import { existsSync, mkdirSync, rmSync, unlinkSync, lstatSync } from 'node:fs' import { join } from 'node:path' import { wrapCommandWithSandboxMacOS } from '../../src/sandbox/macos-sandbox-utils.js' import { + isAtOrUnder, isSymlinkOutsideBoundary, normalizePathForSandbox, } from '../../src/sandbox/sandbox-utils.js' @@ -390,6 +391,17 @@ describe('isSymlinkOutsideBoundary Unit Tests', () => { }) }) +describe('isAtOrUnder', () => { + it('contains by path segment, root included', () => { + expect(isAtOrUnder('/a/b', '/a')).toBe(true) + expect(isAtOrUnder('/a', '/a')).toBe(true) + expect(isAtOrUnder('/ab', '/a')).toBe(false) + expect(isAtOrUnder('/x', '/')).toBe(true) + expect(isAtOrUnder('/', '/')).toBe(true) + expect(isAtOrUnder('/', '/a')).toBe(false) + }) +}) + /** * Tests for glob pattern symlink boundary validation */ From 12a079064b92696e5e2953ccef531a8c9e910959 Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Sat, 29 Aug 2026 15:32:14 -0400 Subject: [PATCH 4/5] test(linux): pin veto (i) without a read policy and no tmpfs re-application over a same-directory bind Two cases the existing suite left to the other veto or to an implicit string-prefix exclusion, plus a comment that described main's root handling inaccurately. --- src/sandbox/linux-sandbox-utils.ts | 3 ++- test/sandbox/readonly-deny-dir-binds.test.ts | 26 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 792ce987b..6332eb445 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -1275,7 +1275,8 @@ async function generateFilesystemArgs( // veto every skip and stub each absent mandatory-deny path of a // write-denied cwd after that cwd's own bind — the startup abort. // Its descendants are decided by their own recorded directories, - // as on main, whose root-blind filter never recorded '/'. + // as before, when the string-prefix filter matched '/' only for a + // path directly beneath it and the vetoes never fired for it. if (denyDir === '/') continue return false } diff --git a/test/sandbox/readonly-deny-dir-binds.test.ts b/test/sandbox/readonly-deny-dir-binds.test.ts index 6708d3bc6..bf1b4ee1d 100644 --- a/test/sandbox/readonly-deny-dir-binds.test.ts +++ b/test/sandbox/readonly-deny-dir-binds.test.ts @@ -228,6 +228,32 @@ describe.if(isLinux)('Deny binds under a read-only denied directory', () => { expect(command).toContain(`--ro-bind ${sibling} ${sibling}`) }) + it('vetoes "/" for an allowed write path beneath it even with no read policy', async () => { + // Veto (i) alone must be root-aware: with readConfig undefined there is + // no read-deny tmpfs for veto (ii) to catch '/' with. + const command = await wrapCommandWithSandboxLinux({ + command: 'echo hello', + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['/', AREA], denyWithinAllow: ['/', PROJ] }, + }) + + expect(command).toContain(`--ro-bind ${PROJ} ${PROJ}`) + }) + + it('does not re-apply a tmpfs over the bind that denies the same directory', async () => { + // X in allowOnly, denyWithinAllow and denyRead: the read-only bind of X + // is not "an ancestor that re-exposes X", so no --tmpfs X --bind X X may + // follow it and make X writable again. + const X = join(AREA, 'both') + mkdirSync(X) + const command = await wrap([X], [X], [AREA, X]) + + const lastRoBind = command.lastIndexOf(`--ro-bind ${X} ${X}`) + expect(lastRoBind).toBeGreaterThan(-1) + expect(command.indexOf(`--bind ${X} ${X}`, lastRoBind)).toBe(-1) + }) + it('keeps the bind for a deny reached through a symlinked spelling', async () => { // The re-application passes key off emitted raw spellings; a dest that // was reached via a symlink keeps its bind so that breadcrumb survives. From 6ea1501db14762ffcb22ef34185142e7cd5d5b4f Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Wed, 2 Sep 2026 08:37:24 +0100 Subject: [PATCH 5/5] refactor(linux): name the strict containment test the vetoes share Four sites spelled out "strictly beneath" as `x !== dir && isAtOrUnder(x, dir)`: veto (i), the covered-by-a-safe-read-only-deny walk, and the tmpfs and file-mask re-application passes. isStrictlyUnder in sandbox-utils says it once, next to isAtOrUnder. One comment paragraph re-flowed. --- src/sandbox/linux-sandbox-utils.ts | 28 +++++++++++----------------- src/sandbox/sandbox-utils.ts | 5 +++++ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 6332eb445..7523aa040 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -18,6 +18,7 @@ import { encodeSandboxedCommand, DANGEROUS_FILES, isAtOrUnder, + isStrictlyUnder, getDangerousDirectories, } from './sandbox-utils.js' import type { @@ -1011,10 +1012,11 @@ async function generateFilesystemArgs( // Inputs for the covering-directory vetoes, computed at most once and // only when a deny path (absent or existing) lies strictly beneath a // recorded read-only deny dir — commands with no such covering directory - // skip the extra stat/realpath/readdir syscalls entirely. Lazy evaluation also means the - // derivation runs from inside the deny loop, AFTER the (unbounded) - // mandatory-deny ripgrep await below, keeping the snapshot as close as - // possible to the denyRead loop that later acts on the real filesystem. + // skip the extra stat/realpath/readdir syscalls entirely. Lazy + // evaluation also means the derivation runs from inside the deny loop, + // AFTER the (unbounded) mandatory-deny ripgrep await below, keeping the + // snapshot as close as possible to the denyRead loop that later acts on + // the real filesystem. // // allowedWritePathsBothForms: allowWrite paths in their recorded and // realpath-canonical spellings. The canonical form is re-resolved HERE, @@ -1236,8 +1238,8 @@ async function generateFilesystemArgs( const unsafe = // (i) an allowed write path strictly beneath the dir: the // re-application's effect would re-bind it writable. - allowedWritePathsBothForms.some( - writePath => writePath !== denyDir && isAtOrUnder(writePath, denyDir), + allowedWritePathsBothForms.some(writePath => + isStrictlyUnder(writePath, denyDir), ) || // (ii) a read-deny tmpfs at or beneath the dir: the re-application's // trigger. @@ -1268,7 +1270,7 @@ async function generateFilesystemArgs( const coveredBySafeReadOnlyDenyDir = (candidate: string): boolean => { let covered = false for (const denyDir of readOnlyDenyDirs) { - if (candidate === denyDir || !isAtOrUnder(candidate, denyDir)) continue + if (!isStrictlyUnder(candidate, denyDir)) continue if (coveringDirIsUnsafe(denyDir)) { // A vetoed '/' neither covers a path nor disqualifies an inner // recorded directory: everything lies beneath it, so it would @@ -1640,11 +1642,7 @@ async function generateFilesystemArgs( // (allowOnly and denyWithinAllow both naming it) contains every one of // them, so containment is root-aware. for (const tmpfsDir of tmpfsDirs) { - if ( - emittedDenyWriteDests.some( - dest => tmpfsDir !== dest && isAtOrUnder(tmpfsDir, dest), - ) - ) { + if (emittedDenyWriteDests.some(dest => isStrictlyUnder(tmpfsDir, dest))) { logForDebugging( `[Sandbox Linux] Re-applying denyRead tmpfs re-exposed by denyWrite bind: ${tmpfsDir}`, ) @@ -1655,11 +1653,7 @@ async function generateFilesystemArgs( // ancestor bind, so the real file is back. Re-apply the mask with its // original source (/dev/null for read-deny, the fake for credential mask). for (const [maskedFile, source] of maskedFiles) { - if ( - emittedDenyWriteDests.some( - dest => maskedFile !== dest && isAtOrUnder(maskedFile, dest), - ) - ) { + if (emittedDenyWriteDests.some(dest => isStrictlyUnder(maskedFile, dest))) { // maskedFiles holds both the symlink path and its resolved target so // the denyWrite skip-check above matches either. Re-emission must go // to the target only — bwrap rejects a symlink bind dest (see diff --git a/src/sandbox/sandbox-utils.ts b/src/sandbox/sandbox-utils.ts index 53ec8fb06..f5d9bd8f1 100644 --- a/src/sandbox/sandbox-utils.ts +++ b/src/sandbox/sandbox-utils.ts @@ -60,6 +60,11 @@ export function isAtOrUnder(p: string, dir: string): boolean { return p === dir || p.startsWith(dir === '/' ? '/' : dir + '/') } +/** `p` lies strictly beneath `dir` (isAtOrUnder, excluding `dir` itself). */ +export function isStrictlyUnder(p: string, dir: string): boolean { + return p !== dir && isAtOrUnder(p, dir) +} + /** * Check if a path pattern contains glob characters */