From 4d2907ba1e3d0c8b60dc81a4ca899dfe386dbc9a Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 13:10:06 +0000 Subject: [PATCH 1/6] linux: deny every dangerous name of a subdirectory, not only those three levels down Below the working directory the mandatory denies come from one ripgrep scan. ripgrep lists files, and `mandatoryDenySearchDepth` (3 by default) was handed to it as the depth of the file. So `sub/.bashrc` and `sub/.git/config` were denied while `sub/.git/hooks/pre-commit`, one level further down, was not: the hooks of a repository in a subdirectory could be written from inside the sandbox. The depth now counts the directory that holds a dangerous name. ripgrep looks two levels deeper, as far as a file in `.git/hooks` or `.claude/commands` lies below that directory, and a match counts only if the directory is within the depth. Every dangerous name of one directory is denied or none is, and no dangerous file is denied deeper than before. The loop that turns a match into a path had two more faults. It looked for one path segment equal to `.claude/commands`, which no segment can be, so only the file it had found was denied and a new one could be made beside it. And it searched the absolute path, so under a directory named `.idea` or `.vscode` every match was taken for that ancestor, and nothing below the working directory was denied. Names are now taken by component, on the path relative to the working directory. A repository is known by its HEAD, so its hooks are denied before there are any. --- README.md | 2 +- src/sandbox/linux-sandbox-utils.ts | 68 +++++++------ src/sandbox/sandbox-config.ts | 3 +- test/sandbox/mandatory-deny-paths.test.ts | 118 +++++++++++++++++++++- 4 files changed, 155 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index ad28cb770..4d69e1267 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). A dangerous directory other than a repository's hooks is only seen if it holds a file directly. You can configure this with `mandatoryDenySearchDepth`: ```json { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 4b22a9557..92a0218b3 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -344,8 +344,9 @@ async function linuxGetMandatoryDenyPaths( for (const dirName of dangerousDirectories) { iglobArgs.push('--iglob', `**/${dirName}/**`) } - // Git hooks always blocked in nested repos - iglobArgs.push('--iglob', '**/.git/hooks/**') + // Git hooks always blocked in nested repos. A repository is known by its + // HEAD, so that its hooks are denied before there are any. + iglobArgs.push('--iglob', '**/.git/hooks/**', '--iglob', '**/.git/HEAD') // Git config conditionally blocked in nested repos if (!allowGitConfig) { @@ -355,6 +356,10 @@ async function linuxGetMandatoryDenyPaths( // Single ripgrep call to find all dangerous paths in subdirectories // Limit depth for performance - deeply nested dangerous files are rare // and the security benefit doesn't justify the traversal cost + // + // ripgrep lists files, and its depth is the file's. `maxDepth` is that of a + // dangerous FILE, which lies in a directory one level up; the same + // directory's `.git/hooks/pre-commit` is two levels further down. let matches: string[] = [] try { matches = await ripGrep( @@ -362,7 +367,7 @@ async function linuxGetMandatoryDenyPaths( '--files', '--hidden', '--max-depth', - String(maxDepth), + String(maxDepth + 2), ...iglobArgs, '-g', '!**/node_modules/**', @@ -375,39 +380,38 @@ async function linuxGetMandatoryDenyPaths( logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`) } - // Process matches + // The names a match can lie under, by path component. + const directoryNames = [ + ...dangerousDirectories, + '.git/hooks', + '.git/config', + '.git/HEAD', + ].map(name => normalizeCaseForComparison(name).split('/')) for (const match of matches) { - const absolutePath = path.resolve(cwd, match) - - // File inside a dangerous directory -> add the directory path - let foundDir = false - for (const dirName of [...dangerousDirectories, '.git']) { - const normalizedDirName = normalizeCaseForComparison(dirName) - const segments = absolutePath.split(path.sep) - const dirIndex = segments.findIndex( - s => normalizeCaseForComparison(s) === normalizedDirName, - ) - if (dirIndex !== -1) { - // For .git, we want hooks/ or config, not the whole .git dir - if (dirName === '.git') { - const gitDir = segments.slice(0, dirIndex + 1).join(path.sep) - if (match.includes('.git/hooks')) { - denyPaths.push(path.join(gitDir, 'hooks')) - } else if (match.includes('.git/config')) { - denyPaths.push(path.join(gitDir, 'config')) - } - } else { - denyPaths.push(segments.slice(0, dirIndex + 1).join(path.sep)) + const segments = path + .relative(cwd, path.resolve(cwd, match)) + .split(path.sep) + const lower = segments.map(normalizeCaseForComparison) + // Where the dangerous name begins and how long it is: the first of + // `directoryNames` on the way down, else the file itself. + let at = segments.length - 1 + let length = 1 + search: for (let i = 0; i < lower.length; i++) { + for (const name of directoryNames) { + if (name.every((component, k) => lower[i + k] === component)) { + at = i + length = name.length + break search } - foundDir = true - break } } - - // Dangerous file match - if (!foundDir) { - denyPaths.push(absolutePath) - } + // INVARIANT: every dangerous name of one directory is denied or none is. + // `at` is how deep that directory lies. + if (at > maxDepth - 1) continue + const found = segments.slice(0, at + length) + if (lower[at] === '.git' && lower[at + 1] === 'head') + found[at + 1] = 'hooks' + denyPaths.push(path.join(cwd, ...found)) } return [...new Set(denyPaths)] diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index 8bd4cef6f..257958e91 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -1153,7 +1153,8 @@ export const SandboxRuntimeConfigSchema = z .max(10) .optional() .describe( - 'Maximum directory depth to search for dangerous files on Linux (default: 3). ' + + 'How far below the working directory dangerous names are looked for on Linux (default: 3): ' + + 'in it and in the directories up to one level less than this below it. ' + 'Higher values provide more protection but slower performance.', ), allowPty: z diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index 5b9ea24fb..0c421b3be 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -120,12 +120,17 @@ describe.if(isSupportedPlatform)( cleanupBwrapMountPoints({ force: true }) }) - async function runSandboxedWrite( + function runSandboxedWrite( filePath: string, content: string, + ): Promise<{ success: boolean; stderr: string }> { + return runSandboxed(`echo '${content}' > '${filePath}'`) + } + + async function runSandboxed( + command: string, ): Promise<{ success: boolean; stderr: string }> { const platform = getPlatform() - const command = `echo '${content}' > '${filePath}'` // Allow writes to current directory, but mandatory denies should still block dangerous files const writeConfig = { @@ -206,6 +211,115 @@ describe.if(isSupportedPlatform)( } }) + describe('The same names below the working directory', () => { + /** What holds files the host runs or trusts, and a file in each. */ + const heldIn = new Map([ + ['.git/hooks', 'pre-commit'], + ...getDangerousDirectories().map( + dir => [dir, DIRECTORY_PROBE_FILE] as const, + ), + ]) + const existing = [ + ...DANGEROUS_FILES, + '.git/config', + ...[...heldIn].map(([dir, file]) => `${dir}/${file}`), + ] + const populate = (container: string): void => { + for (const dir of heldIn.keys()) { + mkdirSync(join(TEST_DIR, container, dir), { recursive: true }) + } + for (const name of [...existing, 'safe-file.txt']) { + writeFileSync(join(TEST_DIR, container, name), ORIGINAL_CONTENT) + } + } + // The deepest directory the default depth reaches on Linux is `a/b`. + const covered = ['sub', 'a/b'] + const beyond = 'a/b/c' + beforeAll(() => [...covered, beyond].forEach(populate)) + + for (const container of covered) { + for (const name of existing) { + it(`blocks writes to ${container}/${name}`, async () => { + const target = `${container}/${name}` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect(result.success).toBe(false) + expect(readFileSync(target, 'utf8')).toBe(ORIGINAL_CONTENT) + }) + } + for (const dir of heldIn.keys()) { + it(`blocks a new file in ${container}/${dir}/`, async () => { + const target = `${container}/${dir}/new-file` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect(result.success).toBe(false) + expect(existsSync(target)).toBe(false) + }) + } + it(`allows writes to ${container}/safe-file.txt`, async () => { + const target = `${container}/safe-file.txt` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect(result.success).toBe(true) + expect(readFileSync(target, 'utf8').trim()).toBe(MODIFIED_CONTENT) + }) + } + + for (const state of ['empty', 'missing']) { + it(`blocks a new hook in a repository whose hooks directory is ${state}`, async () => { + const repository = `hooks-${state}` + const hooks = `${repository}/.git/hooks` + mkdirSync(join(TEST_DIR, repository, '.git'), { recursive: true }) + writeFileSync( + join(TEST_DIR, repository, '.git', 'HEAD'), + 'ref: refs/heads/main', + ) + if (state === 'empty') mkdirSync(join(TEST_DIR, hooks)) + + const result = await runSandboxed( + `mkdir -p '${hooks}' && echo x > '${hooks}/pre-commit'`, + ) + + expect(result.success).toBe(false) + expect(existsSync(`${hooks}/pre-commit`)).toBe(false) + // The rest of the repository is still the command's to write. + expect( + (await runSandboxedWrite(`${repository}/.git/HEAD`, 'x')).success, + ).toBe(true) + }) + } + + it.if(isLinux)( + 'takes no directory above the working directory for one of the names', + async () => { + const project = join(TEST_DIR, 'above', '.idea', 'project') + mkdirSync(join(project, 'sub'), { recursive: true }) + writeFileSync(join(project, 'sub', '.bashrc'), ORIGINAL_CONTENT) + process.chdir(project) + + expect((await runSandboxedWrite('sub/.bashrc', 'x')).success).toBe( + false, + ) + expect((await runSandboxedWrite('safe-file.txt', 'x')).success).toBe( + true, + ) + }, + ) + + it.if(isLinux)( + `looks no deeper than the depth says: none of the names in ${beyond}/`, + async () => { + for (const name of existing) { + const target = `${beyond}/${name}` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect([target, result.success]).toEqual([target, true]) + } + }, + 60_000, + ) + }) + describe('Safe files should still be writable', () => { it('allows writes to regular files', async () => { const result = await runSandboxedWrite( From eebc2a441b072fd5059c7978d6122e92c801cb0e Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 13:14:22 +0000 Subject: [PATCH 2/6] docs: say what is not seen below the working directory, and what stands in for missing hooks --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 4d69e1267..948564bd9 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). A dangerous directory other than a repository's hooks is only seen if it holds a file directly. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: ```json { From 8bf910895daa64bd14570ec7041b516bd4ac286a Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 13:53:18 +0000 Subject: [PATCH 3/6] linux: what the scan for dangerous names finds depends on the names alone Two things beside the names decided what the scan below the working directory found. How ripgrep ended. It exits 2 as soon as one directory cannot be read, and is killed after ten seconds, with everything it had found by then on its output. Any exit but 0 or 1 was taken for "nothing found", so one such directory anywhere within the depth left every name below the working directory undenied. Looking two levels deeper made that likelier. The helper's error now carries the whole lines ripgrep had listed, and the scan uses them. Ignore files and ripgrep's configuration file. Nothing under a directory named in .gitignore, .ignore, .rgignore or .git/info/exclude was listed, and a configuration file can add any flag. The scan now runs with --no-ignore and --no-config. On two large trees that made it faster. A scan that fails before it lists anything, or cannot be started, still denies nothing. --- README.md | 2 +- src/sandbox/linux-sandbox-utils.ts | 19 ++++++-- src/utils/ripgrep.ts | 20 +++++++- test/sandbox/mandatory-deny-paths.test.ts | 56 +++++++++++++++++++++++ test/utils/ripgrep.test.ts | 25 +++++++++- 5 files changed, 114 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 948564bd9..a27b4e7e6 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: ```json { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 92a0218b3..b5609b2bc 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -7,7 +7,7 @@ import { spawn, spawnSync } from 'node:child_process' import type { ChildProcess } from 'node:child_process' import { endianness, tmpdir } from 'node:os' import path, { join } from 'node:path' -import { ripGrep, type RipgrepConfig } from '../utils/ripgrep.js' +import { ripGrep, RipgrepError, type RipgrepConfig } from '../utils/ripgrep.js' import { buildJavaToolOptions } from './java-proxy-agent.js' import { generateProxyEnvVars, @@ -366,6 +366,11 @@ async function linuxGetMandatoryDenyPaths( [ '--files', '--hidden', + // INVARIANT: what is listed depends on the names alone. An ignore file + // that names a directory hides all beneath it, and a configuration + // file can add any flag; both are files in or above the tree. + '--no-ignore', + '--no-config', '--max-depth', String(maxDepth + 2), ...iglobArgs, @@ -377,7 +382,13 @@ async function linuxGetMandatoryDenyPaths( ripgrepConfig, ) } catch (error) { - logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`) + // INVARIANT: what ripgrep listed counts, however it ended. It exits 2 as + // soon as one directory cannot be read, and is killed after ten seconds, + // with all it had found by then on its output. + if (error instanceof RipgrepError) matches = error.listed + logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`, { + level: 'warn', + }) } // The names a match can lie under, by path component. @@ -405,8 +416,8 @@ async function linuxGetMandatoryDenyPaths( } } } - // INVARIANT: every dangerous name of one directory is denied or none is. - // `at` is how deep that directory lies. + // The depth is that of the directory holding the name, `at`, so that the + // names the scan saw in one directory are treated alike. if (at > maxDepth - 1) continue const found = segments.slice(0, at + length) if (lower[at] === '.git' && lower[at + 1] === 'head') diff --git a/src/utils/ripgrep.ts b/src/utils/ripgrep.ts index 153229327..af0b064dc 100644 --- a/src/utils/ripgrep.ts +++ b/src/utils/ripgrep.ts @@ -15,7 +15,7 @@ export interface RipgrepConfig { * @param abortSignal AbortSignal to cancel the operation * @param config Ripgrep configuration (command and optional args) * @returns Array of matching lines (one per line of output) - * @throws Error if ripgrep exits with non-zero status (except exit code 1 which means no matches) + * @throws RipgrepError if ripgrep exits with non-zero status (except exit code 1 which means no matches) */ export async function ripGrep( args: string[], @@ -48,5 +48,21 @@ export async function ripGrep( // Exit code 1 means "no matches found" - this is normal return [] } - throw new Error(`ripgrep failed with exit code ${code}: ${stderr}`) + // Whole lines only: killed, it can leave half of one. + throw new RipgrepError( + `ripgrep failed with exit code ${code}: ${stderr}`, + stdout.split('\n').slice(0, -1).filter(Boolean), + ) +} + +/** ripgrep ended otherwise than by finding something or nothing. */ +export class RipgrepError extends Error { + constructor( + message: string, + /** What it had listed by then. */ + readonly listed: string[], + ) { + super(message) + this.name = 'RipgrepError' + } } diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index 0c421b3be..c029702f4 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -10,6 +10,7 @@ import { import { spawn, spawnSync } from 'node:child_process' import { mkdirSync, + rmdirSync, rmSync, writeFileSync, readFileSync, @@ -289,6 +290,61 @@ describe.if(isSupportedPlatform)( }) } + for (const [file, line] of [ + ['.gitignore', 'sub/'], + ['.ignore', '*'], + ] as const) { + it(`blocks them all the same when ${file} holds ${line}`, async () => { + writeFileSync(join(TEST_DIR, file), `${line}\n`) + try { + for (const name of ['sub/.bashrc', 'sub/.git/hooks/pre-commit']) { + const result = await runSandboxedWrite(name, MODIFIED_CONTENT) + + expect([name, result.success]).toEqual([name, false]) + expect(readFileSync(name, 'utf8')).toBe(ORIGINAL_CONTENT) + } + } finally { + rmSync(join(TEST_DIR, file)) + } + }) + } + + it.if(isLinux)( + "blocks them all the same whatever ripgrep's configuration file says", + async () => { + const configuration = join(TEST_DIR, 'ripgrep-configuration') + writeFileSync(configuration, '--glob=!sub\n') + process.env.RIPGREP_CONFIG_PATH = configuration + try { + const result = await runSandboxedWrite( + 'sub/.bashrc', + MODIFIED_CONTENT, + ) + + expect(result.success).toBe(false) + expect(readFileSync('sub/.bashrc', 'utf8')).toBe(ORIGINAL_CONTENT) + } finally { + delete process.env.RIPGREP_CONFIG_PATH + rmSync(configuration) + } + }, + ) + + it('blocks them all the same beside a directory that cannot be read', async () => { + const unreadable = join(TEST_DIR, 'unreadable') + mkdirSync(unreadable, { mode: 0o000 }) + try { + for (const name of ['sub/.bashrc', 'sub/.git/hooks/pre-commit']) { + const result = await runSandboxedWrite(name, MODIFIED_CONTENT) + + expect([name, result.success]).toEqual([name, false]) + expect(readFileSync(name, 'utf8')).toBe(ORIGINAL_CONTENT) + } + } finally { + rmdirSync(unreadable) + } + }) + it.if(isLinux)( 'takes no directory above the working directory for one of the names', async () => { diff --git a/test/utils/ripgrep.test.ts b/test/utils/ripgrep.test.ts index a7943bbaf..bff94b390 100644 --- a/test/utils/ripgrep.test.ts +++ b/test/utils/ripgrep.test.ts @@ -2,7 +2,7 @@ import { describe, it, expect } from 'bun:test' import { writeFileSync, mkdtempSync, rmSync } from 'fs' import { tmpdir } from 'os' import { basename, join } from 'path' -import { ripGrep } from '../../src/utils/ripgrep.js' +import { ripGrep, RipgrepError } from '../../src/utils/ripgrep.js' describe('ripGrep', () => { it('finds matches with default config', async () => { @@ -74,6 +74,29 @@ describe('ripGrep', () => { } }) + it('says what had been listed when it rejects', async () => { + const dir = mkdtempSync(join(tmpdir(), 'rg-listed-')) + try { + const script = join(dir, 'lists-then-fails.cjs') + writeFileSync( + script, + "process.stdout.write('one\\ntwo\\nhalf a li'); process.exitCode = 2", + ) + + const error: unknown = await ripGrep( + [], + dir, + new AbortController().signal, + { command: process.execPath, args: [script] }, + ).catch((e: unknown) => e) + + expect(error).toBeInstanceOf(RipgrepError) + expect((error as RipgrepError).listed).toEqual(['one', 'two']) + } finally { + rmSync(dir, { recursive: true }) + } + }) + it('rejects on exit code > 1', () => { expect( ripGrep(['--invalid-flag-xyz'], '.', new AbortController().signal), From 9c6c6c42da5e9789e71f3695c5d15312623c3e8c Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 15:15:34 +0000 Subject: [PATCH 4/6] linux: look for dangerous directories one level down, not two Denying every dangerous name two levels down has a price that one level down does not. Each repository in reach costs four mounts, two denies and two that keep what holds them from being renamed, and bwrap's start grows quadratically with mounts. One level down three of the four are there already. Two levels down there were none: with 300 repositories a command went from 0.4 s to 3.1 s, and at about 750 the limit on bwrap's arguments is reached and no command runs. A mount also stops `a/b` from being removed or renamed from inside, and a checkout in the outer repository from replacing `apps/web/.vscode/settings.json`. So ripgrep looks one level deeper than the depth, not two. A dangerous file counts down to the depth, as it always has. A dangerous directory, and the hooks and config of a repository, count one level higher up, which is as far as `.vscode` and `.git/config` were seen before: what is new is that `.git/hooks`, `.claude/commands` and `.claude/agents` are treated like them. Over 119 paths at the default depth nothing that was refused before this branch is written, and 18 more are refused, all one level down. --- README.md | 2 +- src/sandbox/linux-sandbox-utils.ts | 17 ++++--- src/sandbox/sandbox-config.ts | 5 +- test/sandbox/mandatory-deny-paths.test.ts | 62 +++++++++++++---------- 4 files changed, 50 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index a27b4e7e6..2de0b4932 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and every other dangerous name of the same directories with it (`a/b/.git/hooks`, `a/b/.claude/commands`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and a dangerous directory, or the hooks and config of a repository, one level higher up (`a/.vscode`, `a/.claude/commands`, `a/.git/hooks`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: ```json { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index b5609b2bc..c3be2e1eb 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -357,9 +357,9 @@ async function linuxGetMandatoryDenyPaths( // Limit depth for performance - deeply nested dangerous files are rare // and the security benefit doesn't justify the traversal cost // - // ripgrep lists files, and its depth is the file's. `maxDepth` is that of a - // dangerous FILE, which lies in a directory one level up; the same - // directory's `.git/hooks/pre-commit` is two levels further down. + // ripgrep lists files, and its depth is the file's. `sub/.vscode/x` and + // `sub/.git/config` lie at `maxDepth`; `sub/.git/hooks/pre-commit`, of the + // same directory, one level further down. let matches: string[] = [] try { matches = await ripGrep( @@ -372,7 +372,7 @@ async function linuxGetMandatoryDenyPaths( '--no-ignore', '--no-config', '--max-depth', - String(maxDepth + 2), + String(maxDepth + 1), ...iglobArgs, '-g', '!**/node_modules/**', @@ -407,18 +407,21 @@ async function linuxGetMandatoryDenyPaths( // `directoryNames` on the way down, else the file itself. let at = segments.length - 1 let length = 1 + // How deep the directory holding the name, `at`, may lie. One level less + // for `directoryNames`, all alike: each costs mounts that keep what holds + // it from being renamed or removed, and bwrap's start grows with them. + let deepest = maxDepth - 1 search: for (let i = 0; i < lower.length; i++) { for (const name of directoryNames) { if (name.every((component, k) => lower[i + k] === component)) { at = i length = name.length + deepest = maxDepth - 2 break search } } } - // The depth is that of the directory holding the name, `at`, so that the - // names the scan saw in one directory are treated alike. - if (at > maxDepth - 1) continue + if (at > deepest) continue const found = segments.slice(0, at + length) if (lower[at] === '.git' && lower[at + 1] === 'head') found[at + 1] = 'hooks' diff --git a/src/sandbox/sandbox-config.ts b/src/sandbox/sandbox-config.ts index 257958e91..3cad0679c 100644 --- a/src/sandbox/sandbox-config.ts +++ b/src/sandbox/sandbox-config.ts @@ -1153,8 +1153,9 @@ export const SandboxRuntimeConfigSchema = z .max(10) .optional() .describe( - 'How far below the working directory dangerous names are looked for on Linux (default: 3): ' + - 'in it and in the directories up to one level less than this below it. ' + + 'How deep below the working directory dangerous names are looked for on Linux (default: 3): ' + + 'a dangerous file down to this depth, and a dangerous directory, or the hooks and ' + + 'config of a repository, one level higher up. ' + 'Higher values provide more protection but slower performance.', ), allowPty: z diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index c029702f4..53f7f4851 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -233,13 +233,17 @@ describe.if(isSupportedPlatform)( writeFileSync(join(TEST_DIR, container, name), ORIGINAL_CONTENT) } } - // The deepest directory the default depth reaches on Linux is `a/b`. - const covered = ['sub', 'a/b'] + // How far the default depth reaches on Linux: every name in `sub`, and + // the files alone in `a/b`. + const covered = [ + ['sub', existing], + ['a/b', DANGEROUS_FILES], + ] as const const beyond = 'a/b/c' - beforeAll(() => [...covered, beyond].forEach(populate)) + beforeAll(() => ['sub', 'a/b', beyond].forEach(populate)) - for (const container of covered) { - for (const name of existing) { + for (const [container, names] of covered) { + for (const name of names) { it(`blocks writes to ${container}/${name}`, async () => { const target = `${container}/${name}` const result = await runSandboxedWrite(target, MODIFIED_CONTENT) @@ -248,15 +252,6 @@ describe.if(isSupportedPlatform)( expect(readFileSync(target, 'utf8')).toBe(ORIGINAL_CONTENT) }) } - for (const dir of heldIn.keys()) { - it(`blocks a new file in ${container}/${dir}/`, async () => { - const target = `${container}/${dir}/new-file` - const result = await runSandboxedWrite(target, MODIFIED_CONTENT) - - expect(result.success).toBe(false) - expect(existsSync(target)).toBe(false) - }) - } it(`allows writes to ${container}/safe-file.txt`, async () => { const target = `${container}/safe-file.txt` const result = await runSandboxedWrite(target, MODIFIED_CONTENT) @@ -266,6 +261,16 @@ describe.if(isSupportedPlatform)( }) } + for (const dir of heldIn.keys()) { + it(`blocks a new file in sub/${dir}/`, async () => { + const target = `sub/${dir}/new-file` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect(result.success).toBe(false) + expect(existsSync(target)).toBe(false) + }) + } + for (const state of ['empty', 'missing']) { it(`blocks a new hook in a repository whose hooks directory is ${state}`, async () => { const repository = `hooks-${state}` @@ -362,18 +367,23 @@ describe.if(isSupportedPlatform)( }, ) - it.if(isLinux)( - `looks no deeper than the depth says: none of the names in ${beyond}/`, - async () => { - for (const name of existing) { - const target = `${beyond}/${name}` - const result = await runSandboxedWrite(target, MODIFIED_CONTENT) - - expect([target, result.success]).toEqual([target, true]) - } - }, - 60_000, - ) + for (const [container, names] of [ + ['a/b', existing.filter(name => name.includes('/'))], + [beyond, existing], + ] as const) { + it.if(isLinux)( + `looks no deeper than the depth says: ${names.length} names in ${container}/`, + async () => { + for (const name of names) { + const target = `${container}/${name}` + const result = await runSandboxedWrite(target, MODIFIED_CONTENT) + + expect([target, result.success]).toEqual([target, true]) + } + }, + 60_000, + ) + } }) describe('Safe files should still be writable', () => { From 4bb145e761c2d23341a29fbf0a0810b491399a0c Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 16:07:20 +0000 Subject: [PATCH 5/6] linux: deny whole a directory the scan could not read, and keep what a killed scan had listed Two things said two commits ago were not so. That what ripgrep listed counts when it is killed after its ten seconds. Into a pipe it writes by the block, and killed it drops the block: of 24 runs killed in mid scan, none left a line. The scan now asks for line buffering. What ripgrep had not come to is still not denied. That a directory which cannot be read no longer costs the names their denies. It no longer cost those of other directories theirs, but nothing in the directory itself was denied, and its mode is the user's own to change. Such a directory, down to the search depth, is now denied whole, which also keeps its mode from being changed from inside. ripgrep names the directory on its standard error, and that is taken for a hint only, because the paths in it are the tree's own text: a name can hold a newline and a whole message of its own, and a bind of a path that runs through a link would bring what the link leads to into the sandbox. So a directory counts only if it lies below the working directory with no link on the way to it and cannot be read from this process either. One that cannot be looked at, because what holds it cannot be searched, is stood in for by the nearest directory above it that can. --- README.md | 2 +- src/sandbox/linux-sandbox-utils.ts | 66 +++++++++++-- src/utils/ripgrep.ts | 3 + test/sandbox/mandatory-deny-paths.test.ts | 112 ++++++++++++++++++++++ test/utils/ripgrep.test.ts | 3 +- 5 files changed, 177 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2de0b4932..a11960faa 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and a dangerous directory, or the hooks and config of a repository, one level higher up (`a/.vscode`, `a/.claude/commands`, `a/.git/hooks`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and a dangerous directory, or the hooks and config of a repository, one level higher up (`a/.vscode`, `a/.claude/commands`, `a/.git/hooks`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it, and a directory it cannot read is denied whole. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: ```json { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index c3be2e1eb..4078c2602 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -317,6 +317,51 @@ export function linuxGetCwdMandatoryDenyPaths( return denyPaths } +/** + * The directories below `cwd`, down to `maxDepth`, that ripgrep said it could + * not read and that this process cannot read either. What is in one is not + * known, so the caller denies it whole. Exported for testing. + */ +export function unreadableDirectories( + stderr: string, + cwd: string, + maxDepth: number, +): string[] { + const found: string[] = [] + for (const line of stderr.split('\n')) { + const named = /^rg: (.+): .+ \(os error \d+\)$/.exec(line)?.[1] + if (named === undefined) continue + // What cannot be looked at, because what holds it cannot be searched, is + // stood in for by the nearest directory above it that can. + for ( + let dir = path.resolve(cwd, named); + dir.startsWith(cwd + path.sep); + dir = path.dirname(dir) + ) { + try { + // THREAT: the text is only a hint. A name can hold a newline and a + // whole line of its own, and a bind of `link/x` would bring what the + // link leads to INTO the sandbox. So: a directory, with no link on the + // way to it, that cannot be read from here. + if (!fs.lstatSync(dir).isDirectory() || fs.realpathSync(dir) !== dir) { + break + } + } catch { + continue + } + try { + fs.accessSync(dir, fs.constants.R_OK | fs.constants.X_OK) + } catch { + if (path.relative(cwd, dir).split(path.sep).length <= maxDepth) { + found.push(dir) + } + } + break + } + } + return found +} + /** * Get mandatory deny paths using ripgrep (Linux only). * Uses a SINGLE ripgrep call with multiple glob patterns for efficiency. @@ -366,11 +411,14 @@ async function linuxGetMandatoryDenyPaths( [ '--files', '--hidden', - // INVARIANT: what is listed depends on the names alone. An ignore file - // that names a directory hides all beneath it, and a configuration - // file can add any flag; both are files in or above the tree. + // INVARIANT: no file decides what is listed. An ignore file that names + // a directory hides all beneath it, and a configuration file can add + // any flag; both are files in or above the tree. '--no-ignore', '--no-config', + // Into a pipe ripgrep writes by the block, and killed it drops the + // block: it would have listed nothing. + '--line-buffered', '--max-depth', String(maxDepth + 1), ...iglobArgs, @@ -382,10 +430,14 @@ async function linuxGetMandatoryDenyPaths( ripgrepConfig, ) } catch (error) { - // INVARIANT: what ripgrep listed counts, however it ended. It exits 2 as - // soon as one directory cannot be read, and is killed after ten seconds, - // with all it had found by then on its output. - if (error instanceof RipgrepError) matches = error.listed + // INVARIANT: what ripgrep listed counts, however it ended. It exits 2 if + // there was a directory it could not read, having listed the rest, and is + // killed after ten seconds. What it had not come to by then is not denied. + if (error instanceof RipgrepError) { + matches = error.listed + // A read-only bind also keeps the command from giving the mode back. + denyPaths.push(...unreadableDirectories(error.stderr, cwd, maxDepth)) + } logForDebugging(`[Sandbox] ripgrep scan failed: ${error}`, { level: 'warn', }) diff --git a/src/utils/ripgrep.ts b/src/utils/ripgrep.ts index af0b064dc..739037ca7 100644 --- a/src/utils/ripgrep.ts +++ b/src/utils/ripgrep.ts @@ -52,6 +52,7 @@ export async function ripGrep( throw new RipgrepError( `ripgrep failed with exit code ${code}: ${stderr}`, stdout.split('\n').slice(0, -1).filter(Boolean), + stderr, ) } @@ -61,6 +62,8 @@ export class RipgrepError extends Error { message: string, /** What it had listed by then. */ readonly listed: string[], + /** What it said went wrong. The paths in it are the tree's own text. */ + readonly stderr: string, ) { super(message) this.name = 'RipgrepError' diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index 53f7f4851..d2807df6f 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -9,6 +9,7 @@ import { } from 'bun:test' import { spawn, spawnSync } from 'node:child_process' import { + chmodSync, mkdirSync, rmdirSync, rmSync, @@ -28,6 +29,7 @@ import { import { wrapCommandWithSandboxLinux, cleanupBwrapMountPoints, + unreadableDirectories, } from '../../src/sandbox/linux-sandbox-utils.js' import { DANGEROUS_FILES, @@ -350,6 +352,116 @@ describe.if(isSupportedPlatform)( } }) + for (const [directory, mode] of [ + ['locked', 0o000], + ['locked/.git', 0o000], + // As deep as one of the names can itself lie. + ['locked/.claude/commands', 0o000], + // Can be listed, and nothing in it looked at. + ['locked', 0o444], + // The other way about. + ['locked', 0o111], + ] as const) { + it(`blocks them all the same in locked/ when ${directory} has mode ${mode.toString(8)}`, async () => { + const names = [ + '.bashrc', + '.git/config', + '.git/hooks/pre-commit', + `.claude/commands/${DIRECTORY_PROBE_FILE}`, + ] + populate('locked') + try { + for (const name of names) { + // Each time: a command that is not held gives the mode back. + chmodSync(join(TEST_DIR, directory), mode) + const result = await runSandboxed( + `chmod 755 locked '${directory}'; echo x > 'locked/${name}'`, + ) + + expect([name, result.success]).toEqual([name, false]) + } + } finally { + chmodSync(join(TEST_DIR, directory), 0o755) + } + for (const name of names) { + expect(readFileSync(`locked/${name}`, 'utf8')).toBe( + ORIGINAL_CONTENT, + ) + } + }) + } + + // root reads a directory of any mode. + it.if(isLinux && process.getuid?.() !== 0)( + 'takes what ripgrep says it could not read for a hint only', + async () => { + const project = join(TEST_DIR, 'hints') + const outside = join(TEST_DIR, 'hints-outside') + for (const directory of [ + join(project, 'is-readable'), + join(project, 'really-locked'), + join(project, 'a/b/c/too-deep'), + join(outside, 'behind-a-link'), + ]) { + mkdirSync(directory, { recursive: true }) + } + symlinkSync(outside, join(project, 'link')) + const locked = [ + join(project, 'really-locked'), + join(project, 'a/b/c/too-deep'), + join(outside, 'behind-a-link'), + ] + // What a name in the tree could make ripgrep say. + const said = [ + 'is-readable', + 'really-locked', + 'a/b/c/too-deep', + 'link/behind-a-link', + '../hints-outside/behind-a-link', + 'not-there/at-all', + ].map( + name => `rg: ${project}/${name}: Permission denied (os error 13)`, + ) + const standIn = join(TEST_DIR, 'says-it-could-not-read') + const asked = join(TEST_DIR, 'was-asked') + writeFileSync( + standIn, + [ + '#!/bin/sh', + `printf '%s\\n' "$@" > '${asked}'`, + ...said.map(line => `echo '${line}' >&2`), + 'exit 2', + ].join('\n'), + { mode: 0o755 }, + ) + process.chdir(project) + locked.forEach(directory => chmodSync(directory, 0o000)) + try { + const wrapped = await wrapCommandWithSandboxLinux({ + command: 'true', + needsNetworkRestriction: false, + readConfig: undefined, + writeConfig: { allowOnly: ['.'], denyWithinAllow: [] }, + ripgrepConfig: { command: standIn }, + }) + + expect(unreadableDirectories(said.join('\n'), project, 3)).toEqual([ + join(project, 'really-locked'), + ]) + expect(wrapped).toContain('really-locked') + expect(readFileSync(asked, 'utf8').split('\n')).toEqual( + expect.arrayContaining([ + '--no-ignore', + '--no-config', + '--line-buffered', + ]), + ) + } finally { + locked.forEach(directory => chmodSync(directory, 0o755)) + } + }, + ) + it.if(isLinux)( 'takes no directory above the working directory for one of the names', async () => { diff --git a/test/utils/ripgrep.test.ts b/test/utils/ripgrep.test.ts index bff94b390..1f351bc84 100644 --- a/test/utils/ripgrep.test.ts +++ b/test/utils/ripgrep.test.ts @@ -80,7 +80,7 @@ describe('ripGrep', () => { const script = join(dir, 'lists-then-fails.cjs') writeFileSync( script, - "process.stdout.write('one\\ntwo\\nhalf a li'); process.exitCode = 2", + "process.stdout.write('one\\ntwo\\nhalf a li'); process.stderr.write('went wrong'); process.exitCode = 2", ) const error: unknown = await ripGrep( @@ -92,6 +92,7 @@ describe('ripGrep', () => { expect(error).toBeInstanceOf(RipgrepError) expect((error as RipgrepError).listed).toEqual(['one', 'two']) + expect((error as RipgrepError).stderr).toBe('went wrong') } finally { rmSync(dir, { recursive: true }) } From 03b4799f44a23027e651d98c84f6b2a30f3e909d Mon Sep 17 00:00:00 2001 From: Ron Leizrowice Date: Tue, 29 Sep 2026 16:48:14 +0000 Subject: [PATCH 6/6] linux: know a directory the scan could not read however ripgrep words it, if it is the user's own The last commit took the directory's path out of ripgrep's message with a pattern. ripgrep has two walkers, and they word the message differently: with one thread, as on one CPU, it says ": IO error for operation on : Permission denied", the pattern took something that is no path, and the directory was not denied. Versions before 14.1 also leave out the "rg: ". There is no pattern now. What stands before each ": " of a line is tried, and each goes through the same checks as before, so a wrong cut adds nothing. The directory above stands in only for what cannot be looked at, not for what is not there, and a directory is looked at once. One check is new: the directory must be the user's own. It is denied because its mode can be given back from inside, and only an owner can do that. Other users' directories are left as they were: below `/home` on a shared host they would be thousands of mounts, and past bwrap's limit on arguments no command runs. The cases with a directory that cannot be read now run twice, the second time with -j1, which words the message as one CPU does. --- README.md | 2 +- src/sandbox/linux-sandbox-utils.ts | 66 ++++++++++------ test/sandbox/mandatory-deny-paths.test.ts | 94 +++++++++++++++++------ 3 files changed, 112 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index a11960faa..388ad5ac9 100644 --- a/README.md +++ b/README.md @@ -787,7 +787,7 @@ With `allowWrite: ["/"]` the pins reach every ancestor, including any other allo A wrap that carries no write restrictions at all — `filesystem.disabled` with credential masks still in force, or a library caller passing no write config while a `denyRead` entry or a mask still seeds a pin — is the same shape: the whole tree is bound writable, so it gets the same pins and the same top-level covers, and the same `EXDEV` boundary applies there too. -**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and a dangerous directory, or the hooks and config of a repository, one level higher up (`a/.vscode`, `a/.claude/commands`, `a/.git/hooks`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it, and a directory it cannot read is denied whole. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: +**Linux search depth:** On Linux, the sandbox uses `ripgrep` to scan for dangerous files in subdirectories within allowed write paths. By default, it searches up to 3 levels deep for performance: a dangerous file is found down to `a/b/.bashrc`, and a dangerous directory, or the hooks and config of a repository, one level higher up (`a/.vscode`, `a/.claude/commands`, `a/.git/hooks`). Ignore files (`.gitignore`, `.ignore`) do not hide anything from it, and a directory of the user's own that it cannot read is denied whole. A dangerous directory other than a repository's hooks is only seen if it holds a file directly: below the working directory, one that is empty or does not exist yet can be filled. A repository that has no `hooks` directory has an empty file in its place while a command runs, which stops `git init` from being run again there and a hook from being installed. You can configure this with `mandatoryDenySearchDepth`: ```json { diff --git a/src/sandbox/linux-sandbox-utils.ts b/src/sandbox/linux-sandbox-utils.ts index 4078c2602..9010f8643 100644 --- a/src/sandbox/linux-sandbox-utils.ts +++ b/src/sandbox/linux-sandbox-utils.ts @@ -319,8 +319,9 @@ export function linuxGetCwdMandatoryDenyPaths( /** * The directories below `cwd`, down to `maxDepth`, that ripgrep said it could - * not read and that this process cannot read either. What is in one is not - * known, so the caller denies it whole. Exported for testing. + * not read, that this process cannot read either, and whose mode is this + * user's to change. What is in one is not known, so the caller denies it whole. + * Exported for testing. */ export function unreadableDirectories( stderr: string, @@ -328,35 +329,52 @@ export function unreadableDirectories( maxDepth: number, ): string[] { const found: string[] = [] + const tried = new Set() for (const line of stderr.split('\n')) { - const named = /^rg: (.+): .+ \(os error \d+\)$/.exec(line)?.[1] - if (named === undefined) continue - // What cannot be looked at, because what holds it cannot be searched, is - // stood in for by the nearest directory above it that can. + // How a message is worded depends on ripgrep's version and on how many + // threads it has, so what stands before each ": " is tried. + const said = line.startsWith('rg: ') ? line.slice(4) : line for ( - let dir = path.resolve(cwd, named); - dir.startsWith(cwd + path.sep); - dir = path.dirname(dir) + let cut = said.indexOf(': '); + cut !== -1; + cut = said.indexOf(': ', cut + 1) ) { - try { - // THREAT: the text is only a hint. A name can hold a newline and a - // whole line of its own, and a bind of `link/x` would bring what the - // link leads to INTO the sandbox. So: a directory, with no link on the - // way to it, that cannot be read from here. - if (!fs.lstatSync(dir).isDirectory() || fs.realpathSync(dir) !== dir) { + for ( + let dir = path.resolve(cwd, said.slice(0, cut)); + dir.startsWith(cwd + path.sep) && !tried.has(dir); + dir = path.dirname(dir) + ) { + tried.add(dir) + try { + // THREAT: the text is only a hint. A name can hold a newline and a + // whole message of its own, and a bind of `link/x` would bring what + // the link leads to INTO the sandbox. So: a directory, with no link + // on the way to it, that cannot be read from here. And this user's: + // nobody else's mode can be given back from inside, and there can + // be thousands of those (`/home`), each of them a mount. + const stat = fs.lstatSync(dir) + if ( + !stat.isDirectory() || + stat.uid !== process.getuid?.() || + fs.realpathSync(dir) !== dir + ) { + break + } + } catch (error) { + // What cannot be looked at, because what holds it cannot be + // searched, is stood in for by the directory above it. + if ((error as NodeJS.ErrnoException).code === 'EACCES') continue break } - } catch { - continue - } - try { - fs.accessSync(dir, fs.constants.R_OK | fs.constants.X_OK) - } catch { - if (path.relative(cwd, dir).split(path.sep).length <= maxDepth) { - found.push(dir) + try { + fs.accessSync(dir, fs.constants.R_OK | fs.constants.X_OK) + } catch { + if (path.relative(cwd, dir).split(path.sep).length <= maxDepth) { + found.push(dir) + } } + break } - break } } return found diff --git a/test/sandbox/mandatory-deny-paths.test.ts b/test/sandbox/mandatory-deny-paths.test.ts index d2807df6f..4431c274a 100644 --- a/test/sandbox/mandatory-deny-paths.test.ts +++ b/test/sandbox/mandatory-deny-paths.test.ts @@ -6,6 +6,7 @@ import { afterAll, beforeEach, afterEach, + spyOn, } from 'bun:test' import { spawn, spawnSync } from 'node:child_process' import { @@ -132,6 +133,7 @@ describe.if(isSupportedPlatform)( async function runSandboxed( command: string, + ripgrepArgs: string[] = [], ): Promise<{ success: boolean; stderr: string }> { const platform = getPlatform() @@ -155,6 +157,7 @@ describe.if(isSupportedPlatform)( needsNetworkRestriction: false, readConfig: undefined, writeConfig, + ripgrepConfig: { command: 'rg', args: ripgrepArgs }, }) } @@ -362,33 +365,37 @@ describe.if(isSupportedPlatform)( // The other way about. ['locked', 0o111], ] as const) { - it(`blocks them all the same in locked/ when ${directory} has mode ${mode.toString(8)}`, async () => { - const names = [ - '.bashrc', - '.git/config', - '.git/hooks/pre-commit', - `.claude/commands/${DIRECTORY_PROBE_FILE}`, - ] - populate('locked') - try { + // With one thread, as on one CPU, ripgrep words what it says otherwise. + for (const threads of [[], ['-j1']]) { + it(`blocks them all the same in locked/ when ${directory} has mode ${mode.toString(8)} (rg ${threads.join()})`, async () => { + const names = [ + '.bashrc', + '.git/config', + '.git/hooks/pre-commit', + `.claude/commands/${DIRECTORY_PROBE_FILE}`, + ] + populate('locked') + try { + for (const name of names) { + // Each time: a command that is not held gives the mode back. + chmodSync(join(TEST_DIR, directory), mode) + const result = await runSandboxed( + `chmod 755 locked '${directory}'; echo x > 'locked/${name}'`, + threads, + ) + + expect([name, result.success]).toEqual([name, false]) + } + } finally { + chmodSync(join(TEST_DIR, directory), 0o755) + } for (const name of names) { - // Each time: a command that is not held gives the mode back. - chmodSync(join(TEST_DIR, directory), mode) - const result = await runSandboxed( - `chmod 755 locked '${directory}'; echo x > 'locked/${name}'`, + expect(readFileSync(`locked/${name}`, 'utf8')).toBe( + ORIGINAL_CONTENT, ) - - expect([name, result.success]).toEqual([name, false]) } - } finally { - chmodSync(join(TEST_DIR, directory), 0o755) - } - for (const name of names) { - expect(readFileSync(`locked/${name}`, 'utf8')).toBe( - ORIGINAL_CONTENT, - ) - } - }) + }) + } } // root reads a directory of any mode. @@ -400,6 +407,11 @@ describe.if(isSupportedPlatform)( for (const directory of [ join(project, 'is-readable'), join(project, 'really-locked'), + join(project, 'locked: too'), + join(project, 'one-thread'), + join(project, 'no-prefix'), + join(project, 'listed-only/child'), + join(project, 'entered-only'), join(project, 'a/b/c/too-deep'), join(outside, 'behind-a-link'), ]) { @@ -408,6 +420,9 @@ describe.if(isSupportedPlatform)( symlinkSync(outside, join(project, 'link')) const locked = [ join(project, 'really-locked'), + join(project, 'locked: too'), + join(project, 'one-thread'), + join(project, 'no-prefix'), join(project, 'a/b/c/too-deep'), join(outside, 'behind-a-link'), ] @@ -419,9 +434,16 @@ describe.if(isSupportedPlatform)( 'link/behind-a-link', '../hints-outside/behind-a-link', 'not-there/at-all', + 'entered-only/not-there', + 'locked: too', + 'listed-only/child', ].map( name => `rg: ${project}/${name}: Permission denied (os error 13)`, ) + said.push( + `rg: ${project}/one-thread: IO error for operation on ${project}/one-thread: Permission denied (os error 13)`, + `${project}/no-prefix: Permission denied (os error 13)`, + ) const standIn = join(TEST_DIR, 'says-it-could-not-read') const asked = join(TEST_DIR, 'was-asked') writeFileSync( @@ -436,6 +458,8 @@ describe.if(isSupportedPlatform)( ) process.chdir(project) locked.forEach(directory => chmodSync(directory, 0o000)) + chmodSync(join(project, 'listed-only'), 0o444) + chmodSync(join(project, 'entered-only'), 0o111) try { const wrapped = await wrapCommandWithSandboxLinux({ command: 'true', @@ -447,7 +471,21 @@ describe.if(isSupportedPlatform)( expect(unreadableDirectories(said.join('\n'), project, 3)).toEqual([ join(project, 'really-locked'), + join(project, 'locked: too'), + join(project, 'listed-only'), + join(project, 'one-thread'), + join(project, 'no-prefix'), ]) + // Nobody else's mode can be given back from inside. + const uid = process.getuid!() + const getuid = spyOn(process, 'getuid').mockReturnValue(uid + 1) + try { + expect( + unreadableDirectories(said.join('\n'), project, 3), + ).toEqual([]) + } finally { + getuid.mockRestore() + } expect(wrapped).toContain('really-locked') expect(readFileSync(asked, 'utf8').split('\n')).toEqual( expect.arrayContaining([ @@ -457,7 +495,13 @@ describe.if(isSupportedPlatform)( ]), ) } finally { - locked.forEach(directory => chmodSync(directory, 0o755)) + for (const directory of [ + ...locked, + join(project, 'listed-only'), + join(project, 'entered-only'), + ]) { + chmodSync(directory, 0o755) + } } }, )