fix: Handle bubblewrap deny write with read overlay (#251)

sandbox/platform/bubblewrap_translator_linux.go

@@ -229,7 +229,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox
 		args = append(args, writeArgs...)
 	}
 
-	// 3. Process deny_write rules (mount /dev/null to prevent access)
+	// 3. Process deny_write rules after allow_write so read-only binds override writable parents.
 	expandedAllowRead, err := expandAll(policy.Filesystem.AllowRead)
 	if err != nil {
 		log.Warnf("sandbox: failed to expand allow_read for mandatory deny suppression, all mandatory denies preserved: %v", err)
@@ -276,7 +276,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox
 			continue
 		}
 
-		denyArgs, err := t.processDenyRule(expanded)
+		denyArgs, err := t.processDenyWriteRule(expanded)
 		if err != nil {
 			log.Debugf("Deny rule '%s' skipped: %v", expanded, err)
 			continue
@@ -382,6 +382,37 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox
 	return args, nil
 }
 
+// processDenyWriteRule handles deny_write rules without masking reads. Files
+// and directories are mounted read-only over any earlier writable parent bind.
+func (t *bubblewrapPolicyTranslator) processDenyWriteRule(path string) ([]string, error) {
+	args := []string{}
+
+	if util.ContainsGlob(path) {
+		paths, _, err := t.expandGlobPattern(path, t.config.mandatoryDenyScanDepth, t.config.maxGlobPaths)
+		if err != nil {
+			return args, nil
+		}
+
+		for _, p := range paths {
+			if _, err := os.Stat(p); err == nil {
+				args = append(args, "--ro-bind-try", p, p)
+				log.Debugf("Deny write rule: mounted '%s' as read-only", p)
+			}
+		}
+
+		return args, nil
+	}
+
+	if _, err := os.Stat(path); err == nil {
+		args = append(args, "--ro-bind-try", path, path)
+		log.Debugf("Deny write rule: mounted '%s' as read-only", path)
+	} else if os.IsNotExist(err) {
+		log.Debugf("Deny write rule: skipping non-existent path '%s'", path)
+	}
+
+	return args, nil
+}
+
 // processReadRule handles a single allow_read rule, expanding globs and creating ro-bind mounts.
 func (t *bubblewrapPolicyTranslator) processReadRule(path string, boundPaths map[string]bool) ([]string, error) {
 	args := []string{}

sandbox/platform/bubblewrap_translator_linux_test.go

@@ -125,7 +125,7 @@ func TestBubblewrapTranslatorFilesystemRules(t *testing.T) {
 			},
 		},
 		{
-			name: "deny write with /dev/null mount",
+			name: "deny write with read-only bind",
 			policy: &sandbox.SandboxPolicy{
 				Filesystem: sandbox.FilesystemPolicy{
 					DenyWrite: []string{"/etc/passwd"},
@@ -133,14 +133,10 @@ func TestBubblewrapTranslatorFilesystemRules(t *testing.T) {
 			},
 			assert: func(t *testing.T, args []string, err error) {
 				require.NoError(t, err)
-				argsStr := argSliceToString(args)
 
-				// Should mount /dev/null over denied path if it exists
-				// Since /etc/passwd exists, it should be blocked
 				if _, err := os.Stat("/etc/passwd"); err == nil {
-					assert.Contains(t, argsStr, "--ro-bind")
-					assert.Contains(t, argsStr, "/dev/null")
-					assert.Contains(t, argsStr, "/etc/passwd")
+					assertNoDevNullMount(t, args, "/etc/passwd")
+					assertReadBind(t, args, "/etc/passwd")
 				}
 			},
 		},
@@ -480,8 +476,8 @@ func TestBubblewrapConfigEssentialDevices(t *testing.T) {
 	}
 }
 
-func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) {
-	t.Run("existing file is blocked with /dev/null", func(t *testing.T) {
+func TestBubblewrapTranslatorProcessDenyWriteRule(t *testing.T) {
+	t.Run("existing file is mounted read-only", func(t *testing.T) {
 		tmpDir := t.TempDir()
 
 		// Create a test file that exists
@@ -499,19 +495,15 @@ func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) {
 		args, err := translator.translate(policy)
 		require.NoError(t, err)
 
-		argsStr := argSliceToString(args)
-
-		// Existing file should be mounted with /dev/null
-		assert.Contains(t, argsStr, "--ro-bind")
-		assert.Contains(t, argsStr, "/dev/null")
-		assert.Contains(t, argsStr, testFile)
+		assertNoDevNullMount(t, args, testFile)
+		assertReadBind(t, args, testFile)
 	})
 
 	t.Run("non-existent file is skipped to avoid creating empty files", func(t *testing.T) {
 		tmpDir := t.TempDir()
 
-		// Non-existent file - should be skipped because using --ro-bind /dev/null
-		// on non-existent paths causes bwrap to create the file as a mount point
+		// Non-existent file - should be skipped because bwrap requires a real
+		// mount target for file-level read-only bind overrides.
 		nonExistentPath := filepath.Join(tmpDir, ".env")
 
 		policy := &sandbox.SandboxPolicy{
@@ -950,7 +942,7 @@ func TestBubblewrapMandatoryDenySuppression(t *testing.T) {
 		assertReadBind(t, args, envPath)
 	})
 
-	t.Run("user deny_write still wins for paths also in allow_read", func(t *testing.T) {
+	t.Run("user deny_write preserves read for paths also in allow_read", func(t *testing.T) {
 		dir := t.TempDir()
 		envPath := filepath.Join(dir, ".env")
 		require.NoError(t, os.WriteFile(envPath, []byte("X=1\n"), 0o600))
@@ -965,13 +957,15 @@ func TestBubblewrapMandatoryDenySuppression(t *testing.T) {
 		policy := &sandbox.SandboxPolicy{
 			Name: "test",
 			Filesystem: sandbox.FilesystemPolicy{
-				AllowRead: []string{envPath},
-				DenyWrite: []string{envPath},
+				AllowRead:  []string{envPath},
+				AllowWrite: []string{dir},
+				DenyWrite:  []string{envPath},
 			},
 		}
 		args := translateForTest(t, policy)
 
-		assertDevNullMount(t, args, envPath)
+		assertNoDevNullMount(t, args, envPath)
+		assertReadOnlyBindAfterWritableBind(t, args, envPath, dir)
 	})
 
 	t.Run("write-side opt-out skips both tmpfs and /dev/null for that path", func(t *testing.T) {
@@ -1013,16 +1007,6 @@ func assertNoTmpfsAt(t *testing.T, args []string, path string) {
 	}
 }
 
-func assertDevNullMount(t *testing.T, args []string, path string) {
-	t.Helper()
-	for i := 0; i+2 < len(args); i++ {
-		if (args[i] == "--ro-bind" || args[i] == "--bind") && args[i+1] == "/dev/null" && args[i+2] == path {
-			return
-		}
-	}
-	t.Fatalf("expected /dev/null mount at %q, not found in args: %v", path, args)
-}
-
 func assertNoDevNullMount(t *testing.T, args []string, path string) {
 	t.Helper()
 	for i := 0; i+2 < len(args); i++ {
@@ -1041,3 +1025,22 @@ func assertReadBind(t *testing.T, args []string, path string) {
 	}
 	t.Fatalf("expected --ro-bind %q %q, not found in args: %v", path, path, args)
 }
+
+func assertReadOnlyBindAfterWritableBind(t *testing.T, args []string, readOnlyPath string, writablePath string) {
+	t.Helper()
+
+	writableBindIndex := -1
+	readOnlyBindIndex := -1
+	for i := 0; i+2 < len(args); i++ {
+		if (args[i] == "--bind" || args[i] == "--bind-try") && args[i+1] == writablePath && args[i+2] == writablePath {
+			writableBindIndex = i
+		}
+		if (args[i] == "--ro-bind" || args[i] == "--ro-bind-try") && args[i+1] == readOnlyPath && args[i+2] == readOnlyPath {
+			readOnlyBindIndex = i
+		}
+	}
+
+	require.NotEqual(t, -1, writableBindIndex, "expected writable bind for %q in args: %v", writablePath, args)
+	require.NotEqual(t, -1, readOnlyBindIndex, "expected read-only bind for %q in args: %v", readOnlyPath, args)
+	assert.Greater(t, readOnlyBindIndex, writableBindIndex, "deny_write read-only bind must override earlier writable parent bind")
+}