fix: Non-existent path handling bug

2026-08-03 07:24:09 +02:00 · 2026-01-14 21:50:04 +05:30
parent 421133c3cc
commit dce5220241
2 changed files with 137 additions and 33 deletions
@@ -218,6 +218,13 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox
 		args = append(args, writeArgs...)
 	}

+	// Collect allowed write paths for deny rule processing
+	// This is used to determine if non-existent deny paths need to be blocked
+	allowedWritePaths := make([]string, 0, len(writeBoundPaths))
+	for path := range writeBoundPaths {
+		allowedWritePaths = append(allowedWritePaths, path)
+	}
+
 	// 3. Process deny_write rules (mount /dev/null to prevent creation)
 	allowGitConfig := utils.SafelyGetValue(policy.AllowGitConfig)
 	denyPatterns := append([]string{}, policy.Filesystem.DenyWrite...)
@@ -233,7 +240,7 @@ func (t *bubblewrapPolicyTranslator) translateFilesystem(policy *sandbox.Sandbox
 			continue
 		}

-		denyArgs, err := t.processDenyRule(expanded)
+		denyArgs, err := t.processDenyRule(expanded, allowedWritePaths)
 		if err != nil {
 			// Deny rules failing is not critical (file may not exist yet)
 			log.Debugf("Deny rule '%s' skipped: %v", expanded, err)
@@ -425,7 +432,8 @@ func (t *bubblewrapPolicyTranslator) processWriteRule(path string, boundPaths ma

 // processDenyRule handles deny rules by mounting /dev/null to prevent file creation.
 // This technique is borrowed from Anthropic's sandbox-runtime.
-func (t *bubblewrapPolicyTranslator) processDenyRule(path string) ([]string, error) {
+// allowedWritePaths is used to determine if non-existent paths need to be blocked.
+func (t *bubblewrapPolicyTranslator) processDenyRule(path string, allowedWritePaths []string) ([]string, error) {
 	args := []string{}

 	// For glob patterns, expand and deny each path
@@ -466,10 +474,18 @@ func (t *bubblewrapPolicyTranslator) processDenyRule(path string) ([]string, err
 				args = append(args, "--ro-bind", "/dev/null", path)
 			}
 		} else if os.IsNotExist(err) {
-			// File doesn't exist - skip blocking it
-			// Rationale: bwrap cannot create mount points in read-only parent directories.
-			// Non-existent files are already protected by deny-by-default (not in allow_write).
-			log.Debugf("Deny rule: skipping non-existent path '%s' (already protected by deny-by-default)", path)
+			// File doesn't exist - check if it's within an allowed write path
+			// If so, we need to block creation by mounting /dev/null at first non-existent component
+			if isWithinAllowedWritePath(path, allowedWritePaths) {
+				firstNonExistent := t.findFirstNonExistentPath(path)
+				if firstNonExistent != "" {
+					args = append(args, "--ro-bind", "/dev/null", firstNonExistent)
+					log.Debugf("Deny rule: blocking creation of '%s' by mounting /dev/null at '%s'", path, firstNonExistent)
+				}
+			} else {
+				// Not within allowed write paths - already protected by deny-by-default
+				log.Debugf("Deny rule: skipping '%s' - not within allowed write paths (protected by deny-by-default)", path)
+			}
 		}
 	}

@@ -499,6 +515,19 @@ func (t *bubblewrapPolicyTranslator) findFirstNonExistentPath(path string) strin
 	return ""
 }

+// isWithinAllowedWritePath checks if a target path is within any of the allowed write paths.
+// This is used to determine if a non-existent deny path needs to be blocked.
+func isWithinAllowedWritePath(targetPath string, allowedWritePaths []string) bool {
+	targetPath = filepath.Clean(targetPath)
+	for _, allowedPath := range allowedWritePaths {
+		allowedPath = filepath.Clean(allowedPath)
+		if targetPath == allowedPath || strings.HasPrefix(targetPath, allowedPath+"/") {
+			return true
+		}
+	}
+	return false
+}
+
 // expandGlobPattern expands a glob pattern to a list of concrete paths.
 // Implements depth limiting and path count limiting to prevent DoS.
 // Returns (paths, useFallback, error) where useFallback indicates if
@@ -480,40 +480,115 @@ func TestBubblewrapConfigEssentialDevices(t *testing.T) {
 }

 func TestBubblewrapTranslatorProcessDenyRule(t *testing.T) {
-	tmpDir := t.TempDir()
+	t.Run("existing file is blocked with /dev/null", func(t *testing.T) {
+		tmpDir := t.TempDir()

-	// Create a test file that exists
-	testFile := filepath.Join(tmpDir, "existing.txt")
-	require.NoError(t, os.WriteFile(testFile, []byte("test"), 0644))
+		// Create a test file that exists
+		testFile := filepath.Join(tmpDir, "existing.txt")
+		require.NoError(t, os.WriteFile(testFile, []byte("test"), 0644))

-	// Create a path that doesn't exist
-	nonExistentPath := filepath.Join(tmpDir, ".env")
-
-	policy := &sandbox.SandboxPolicy{
-		Filesystem: sandbox.FilesystemPolicy{
-			DenyWrite: []string{
-				testFile,        // Existing file
-				nonExistentPath, // Non-existent file
+		policy := &sandbox.SandboxPolicy{
+			Filesystem: sandbox.FilesystemPolicy{
+				DenyWrite: []string{testFile},
 			},
-		},
-	}
+		}

-	config := newDefaultBubblewrapConfig()
-	translator := newBubblewrapPolicyTranslator(config)
-	args, err := translator.translate(policy)
-	require.NoError(t, err)
+		config := newDefaultBubblewrapConfig()
+		translator := newBubblewrapPolicyTranslator(config)
+		args, err := translator.translate(policy)
+		require.NoError(t, err)

-	argsStr := argSliceToString(args)
+		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)
+		// 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)
+	})

-	// Non-existent file should NOT be in args (protected by deny-by-default)
-	// In bubblewrap, paths that are not explicitly mounted are inaccessible,
-	// so we don't need to add deny rules for non-existent files
-	assert.NotContains(t, argsStr, nonExistentPath)
+	t.Run("non-existent file outside allowed write paths is skipped", func(t *testing.T) {
+		// Use a path that is truly outside any allowed write path
+		// Note: /tmp is always in allowedWritePaths due to tmpdir support
+		// So we use a path under /nonexistent which doesn't exist
+		nonExistentPath := "/nonexistent/completely/fake/path/.env"
+
+		policy := &sandbox.SandboxPolicy{
+			Filesystem: sandbox.FilesystemPolicy{
+				DenyWrite: []string{nonExistentPath},
+				// No AllowWrite - path is not within any allowed write area
+			},
+		}
+
+		config := newDefaultBubblewrapConfig()
+		translator := newBubblewrapPolicyTranslator(config)
+		args, err := translator.translate(policy)
+		require.NoError(t, err)
+
+		argsStr := argSliceToString(args)
+
+		// Non-existent file should NOT be in args (protected by deny-by-default)
+		// Path is not within allowed write paths, so no need to block it
+		assert.NotContains(t, argsStr, nonExistentPath)
+	})
+
+	t.Run("non-existent file within allowed write path is blocked", func(t *testing.T) {
+		tmpDir := t.TempDir()
+
+		// Non-existent path WITHIN an allowed write path
+		nonExistentPath := filepath.Join(tmpDir, ".env")
+
+		policy := &sandbox.SandboxPolicy{
+			Filesystem: sandbox.FilesystemPolicy{
+				AllowWrite: []string{tmpDir}, // Allow writes to tmpDir
+				DenyWrite:  []string{nonExistentPath},
+			},
+		}
+
+		config := newDefaultBubblewrapConfig()
+		translator := newBubblewrapPolicyTranslator(config)
+		args, err := translator.translate(policy)
+		require.NoError(t, err)
+
+		argsStr := argSliceToString(args)
+
+		// Non-existent file within allowed write path should be blocked
+		// by mounting /dev/null at the first non-existent component
+		assert.Contains(t, argsStr, "--ro-bind")
+		assert.Contains(t, argsStr, "/dev/null")
+		assert.Contains(t, argsStr, nonExistentPath)
+	})
+
+	t.Run("deeply nested non-existent path is blocked at first missing component", func(t *testing.T) {
+		tmpDir := t.TempDir()
+
+		// Create a subdirectory that exists
+		existingDir := filepath.Join(tmpDir, "existing")
+		require.NoError(t, os.MkdirAll(existingDir, 0755))
+
+		// Deeply nested non-existent path: existing/nonexistent/deep/.env
+		// First non-existent component is "nonexistent"
+		nonExistentPath := filepath.Join(existingDir, "nonexistent", "deep", ".env")
+		firstNonExistent := filepath.Join(existingDir, "nonexistent")
+
+		policy := &sandbox.SandboxPolicy{
+			Filesystem: sandbox.FilesystemPolicy{
+				AllowWrite: []string{tmpDir}, // Allow writes to tmpDir
+				DenyWrite:  []string{nonExistentPath},
+			},
+		}
+
+		config := newDefaultBubblewrapConfig()
+		translator := newBubblewrapPolicyTranslator(config)
+		args, err := translator.translate(policy)
+		require.NoError(t, err)
+
+		argsStr := argSliceToString(args)
+
+		// Should mount /dev/null at the first non-existent component
+		assert.Contains(t, argsStr, "--ro-bind")
+		assert.Contains(t, argsStr, "/dev/null")
+		assert.Contains(t, argsStr, firstNonExistent)
+	})
 }

 func TestBubblewrapTranslatorTmpdirSupport(t *testing.T) {