Merge pull request #195 from anthropic-experimental/atp/cc-1468-denywrite-unmasks-denyread-regression

fix: denyWrite unmasks denyRead when paths overlap (#190 regression)

Author: poteat

src/sandbox/linux-sandbox-utils.ts

@@ -868,6 +868,9 @@ async function generateFilesystemArgs(
   const readAllowPaths = (readConfig?.allowWithinDeny || []).map(p =>
     normalizePathForSandbox(p),
   )
+  // Files masked by --ro-bind /dev/null below. Used to filter denyWriteArgs so
+  // that --ro-bind <host> <host> doesn't undo the mask.
+  const maskedFiles = new Set<string>()
 
   // --tmpfs / would wipe all prior mounts (ro-bind /, write binds, deny binds).
   // Expand a root deny into its direct children so the existing per-dir tmpfs
@@ -892,8 +895,14 @@ async function generateFilesystemArgs(
     readDenyPaths.push('/etc/ssh/ssh_config.d')
   }
 
-  for (const pathPattern of readDenyPaths) {
-    const normalizedPath = normalizePathForSandbox(pathPattern)
+  // Normalize then sort shallow-first so tmpfs over ancestor dirs lands before
+  // /dev/null masks on descendant files. Otherwise a file-deny listed before
+  // a dir-deny in denyRead gets wiped when the ancestor tmpfs is applied.
+  const normalizedDenyPaths = readDenyPaths
+    .map(p => normalizePathForSandbox(p))
+    .sort((a, b) => a.split('/').length - b.split('/').length)
+
+  for (const normalizedPath of normalizedDenyPaths) {
     if (!fs.existsSync(normalizedPath)) {
       logForDebugging(
         `[Sandbox Linux] Skipping non-existent read deny path: ${normalizedPath}`,
@@ -948,27 +957,32 @@ async function generateFilesystemArgs(
         }
       }
     } else {
-      // For files, check if this specific file is re-allowed
-      const isReAllowed = readAllowPaths.some(
-        allowPath =>
-          normalizedPath === allowPath ||
-          normalizedPath.startsWith(allowPath + '/'),
-      )
-      if (isReAllowed) {
+      // For files, only an exact allowRead match overrides the deny. A
+      // directory allowRead does not un-deny a file specifically listed in
+      // denyRead — otherwise denyRead: ['.env'] + allowRead: ['.'] silently
+      // drops the .env deny.
+      if (readAllowPaths.includes(normalizedPath)) {
         logForDebugging(
           `[Sandbox Linux] Skipping read deny for re-allowed path: ${normalizedPath}`,
         )
         continue
       }
       // For files, bind /dev/null instead of tmpfs
       args.push('--ro-bind', '/dev/null', normalizedPath)
+      maskedFiles.add(normalizedPath)
     }
   }
 
   // Emitting denyWrite last means these ro-binds layer on top of any write
   // paths the denyRead loop just re-bound. Before this ordering, tmpfs over
-  // an ancestor of cwd would wipe the .git/hooks protection.
-  args.push(...denyWriteArgs)
+  // an ancestor of cwd would wipe the .git/hooks protection. But skip any
+  // dest already masked by denyRead — --ro-bind <host> <host> for denyWrite
+  // would undo --ro-bind /dev/null <host> from denyRead, which landed first.
+  for (let i = 0; i < denyWriteArgs.length; i += 3) {
+    const dest = denyWriteArgs[i + 2]!
+    if (maskedFiles.has(dest)) continue
+    args.push(denyWriteArgs[i]!, denyWriteArgs[i + 1]!, dest)
+  }
 
   return args
 }

test/sandbox/wrap-with-sandbox.test.ts

@@ -755,4 +755,114 @@ describe('allowWrite glob suffix handling', () => {
       rmSync(parentDir, { recursive: true, force: true })
     }
   })
+
+  // Regression: #190 reordered denyWrite after denyRead so .git/hooks ro-binds
+  // survive a tmpfs over an ancestor. But denyWrite's --ro-bind <host> <host>
+  // now lands after denyRead's --ro-bind /dev/null <host>, undoing the mask
+  // when the same file is in both lists.
+  it('does not let denyWrite unmask a denyRead /dev/null bind (Linux)', async () => {
+    if (getPlatform() !== 'linux') {
+      return
+    }
+
+    const parentDir = join(tmpdir(), `srt-test-unmask-${Date.now()}`)
+    const secret = join(parentDir, 'secret.txt')
+    mkdirSync(parentDir, { recursive: true })
+    writeFileSync(secret, '')
+
+    try {
+      await SandboxManager.reset()
+      await SandboxManager.initialize({
+        network: { allowedDomains: [], deniedDomains: [] },
+        filesystem: {
+          denyRead: [secret],
+          allowWrite: [parentDir],
+          denyWrite: [secret],
+        },
+      })
+
+      const result = await SandboxManager.wrapWithSandbox(command)
+
+      // The /dev/null mask is what we want; the host-file bind is what we don't.
+      expect(result).toContain(`--ro-bind /dev/null ${secret}`)
+      expect(result).not.toContain(`--ro-bind ${secret} ${secret}`)
+    } finally {
+      await SandboxManager.reset()
+      rmSync(parentDir, { recursive: true, force: true })
+    }
+  })
+
+  // A file listed in denyRead should stay denied even if allowRead covers its
+  // parent directory. Before this change, startsWith(allowPath + '/') matched
+  // and the file-deny was silently skipped.
+  it('file-level denyRead survives a parent-directory allowRead (Linux)', async () => {
+    if (getPlatform() !== 'linux') {
+      return
+    }
+
+    const parentDir = join(tmpdir(), `srt-test-file-deny-${Date.now()}`)
+    const secret = join(parentDir, '.env')
+    mkdirSync(parentDir, { recursive: true })
+    writeFileSync(secret, '')
+
+    try {
+      await SandboxManager.reset()
+      await SandboxManager.initialize({
+        network: { allowedDomains: [], deniedDomains: [] },
+        filesystem: {
+          denyRead: [secret],
+          allowRead: [parentDir],
+          allowWrite: [parentDir],
+          denyWrite: [],
+        },
+      })
+
+      const result = await SandboxManager.wrapWithSandbox(command)
+
+      expect(result).toContain(`--ro-bind /dev/null ${secret}`)
+    } finally {
+      await SandboxManager.reset()
+      rmSync(parentDir, { recursive: true, force: true })
+    }
+  })
+
+  // denyRead entries are sorted shallow-first before mounting, so a file-deny
+  // listed before its ancestor dir-deny still lands on top of the ancestor's
+  // tmpfs + re-allow binds.
+  it('file-deny survives ancestor dir-deny listed after it in denyRead (Linux)', async () => {
+    if (getPlatform() !== 'linux') {
+      return
+    }
+
+    const parentDir = join(tmpdir(), `srt-test-order-${Date.now()}`)
+    const projectDir = join(parentDir, 'project')
+    const envFile = join(projectDir, '.env')
+    mkdirSync(projectDir, { recursive: true })
+    writeFileSync(envFile, '')
+
+    try {
+      await SandboxManager.reset()
+      await SandboxManager.initialize({
+        network: { allowedDomains: [], deniedDomains: [] },
+        filesystem: {
+          // File deliberately listed before the dir that contains it
+          denyRead: [envFile, parentDir],
+          allowRead: [projectDir],
+          allowWrite: [projectDir],
+          denyWrite: [],
+        },
+      })
+
+      const result = await SandboxManager.wrapWithSandbox(command)
+
+      // The /dev/null mask must come after the tmpfs + ro-bind in arg order.
+      const tmpfsAt = result.indexOf(`--tmpfs ${parentDir}`)
+      const maskAt = result.indexOf(`--ro-bind /dev/null ${envFile}`)
+      expect(tmpfsAt).toBeGreaterThan(-1)
+      expect(maskAt).toBeGreaterThan(tmpfsAt)
+    } finally {
+      await SandboxManager.reset()
+      rmSync(parentDir, { recursive: true, force: true })
+    }
+  })
 })