From 872c5d663cdcc7fd7e4a086d2fd971ff3f8ce319 Mon Sep 17 00:00:00 2001 From: Sahil Bansal Date: Sun, 7 Jun 2026 10:03:23 +0530 Subject: [PATCH] fix(sandbox): bind parent dir for globstar allow_write on bwrap (#321) * fix(sandbox): bind parent dir for globstar allow_write on bwrap Fine-grained per-path mounts under read-only project binds broke pip install into in-project .venv directories. Always mount the parent tree for ** write rules instead. Fixes #315 * test(sandbox): tighten globstar bind assertions and ensure ~/.npm exists for e2e Strengthen TestBubblewrapAllowWriteGlobstarBindsParentOnly to verify the parent dir is writably bound and the child path is read-only bound, not just substring presence. Pre-create ~/.npm in the e2e harness so bubblewrap --bind-try does not skip the npm cache dir on fresh runners. * switch pnpm to /tmp in sandbox e2e * test(sandbox): update glob ** test for parent-bind semantics Globstar allow_write now binds the parent dir only (e2e740d), so the test should assert the parent is writably bound and child subdirs are not individually bound, instead of substring-matching subdir names. * fix(sandbox): bind correct base dir for in-pattern globstar allow_write Globstar allow_write previously used extractGlobParentDir, which walks past the first ** and yields the wrong root for patterns like /a/b/**/d/**/e. Introduce extractGlobstarWriteBaseDir, which takes the prefix before the first /**, and use it in processWriteRule. Also dedup the coarse-fallback parent-bind loop to mirror the read-rule fallback. --- .../platform/bubblewrap_translator_linux.go | 18 ++- .../bubblewrap_translator_linux_test.go | 116 ++++++++++++++++-- sandbox/platform/glob_linux.go | 16 +++ test/pm-e2e.js | 28 ++++- 4 files changed, 165 insertions(+), 13 deletions(-) diff --git a/sandbox/platform/bubblewrap_translator_linux.go b/sandbox/platform/bubblewrap_translator_linux.go index 28d073b..4c65b92 100644 --- a/sandbox/platform/bubblewrap_translator_linux.go +++ b/sandbox/platform/bubblewrap_translator_linux.go @@ -529,8 +529,21 @@ func (t *bubblewrapPolicyTranslator) processWriteRule(path string, boundPaths ma // Check if path contains glob pattern if util.ContainsGlob(path) { - // Check if the base directory is already bound (e.g., /tmp already bound, skip /tmp/**) baseDir := t.extractParentDir(path) + + // Globstar write rules always bind the parent directory read-write. Per-path + // binds interact badly with earlier read-only parent mounts (e.g. ${CWD}/**) + // and miss files beyond maxGlobDepth — see https://github.com/safedep/pmg/issues/315. + // Base dir is the path prefix before the first "/**" (see extractGlobstarWriteBaseDir). + if strings.Contains(path, "**") { + baseDir = extractGlobstarWriteBaseDir(path) + args = append(args, "--bind-try", baseDir, baseDir) + boundPaths[baseDir] = true + log.Debugf("Globstar allow_write: bound parent directory '%s' (read-write)", baseDir) + return args, nil + } + + // Check if the base directory is already bound (e.g., /tmp already bound, skip /tmp/**) if boundPaths[baseDir] { log.Debugf("Skipping pattern '%s' - base directory '%s' already bound", path, baseDir) return args, nil @@ -550,8 +563,7 @@ func (t *bubblewrapPolicyTranslator) processWriteRule(path string, boundPaths ma boundPaths[parentDir] = true log.Debugf("Coarse-grained fallback: bound parent directory '%s' (read-write)", parentDir) } else { - // Path already bound, skip (likely already bound as read-only from essential paths) - log.Debugf("Parent directory '%s' already bound, skipping duplicate bind", parentDir) + log.Debugf("Parent directory '%s' already bound for write, skipping duplicate bind", parentDir) } } } else { diff --git a/sandbox/platform/bubblewrap_translator_linux_test.go b/sandbox/platform/bubblewrap_translator_linux_test.go index 34ddc21..75461fe 100644 --- a/sandbox/platform/bubblewrap_translator_linux_test.go +++ b/sandbox/platform/bubblewrap_translator_linux_test.go @@ -274,13 +274,9 @@ func TestBubblewrapTranslatorGlobPatterns(t *testing.T) { }, assert: func(t *testing.T, args []string, err error) { require.NoError(t, err) - argsStr := argSliceToString(args) - - // Should include the base directory - assert.Contains(t, argsStr, tmpDir) - // Should include subdirectories - assert.Contains(t, argsStr, "subdir1") - assert.Contains(t, argsStr, "subdir2") + assertWriteBind(t, args, tmpDir) + assertNoWriteBind(t, args, filepath.Join(tmpDir, "subdir1")) + assertNoWriteBind(t, args, filepath.Join(tmpDir, "subdir2")) }, }, { @@ -775,6 +771,42 @@ func TestGlobFallbackThresholdGlobstar(t *testing.T) { assert.Less(t, len(args), 300, "Coarse-grained fallback should prevent argument explosion") } +// TestBubblewrapAllowWriteGlobstarBindsParentOnly verifies globstar allow_write rules +// bind the parent tree (e.g. .venv) instead of per-file mounts that break pip in-project venvs. +// See https://github.com/safedep/pmg/issues/315 +func TestBubblewrapAllowWriteGlobstarBindsParentOnly(t *testing.T) { + tmpDir := t.TempDir() + venvDir := filepath.Join(tmpDir, ".venv") + binDir := filepath.Join(venvDir, "bin") + require.NoError(t, os.MkdirAll(binDir, 0755)) + pythonPath := filepath.Join(binDir, "python") + require.NoError(t, os.Symlink("/usr/bin/python3", pythonPath)) + + // Populate enough shallow files that old logic would fine-grain bind without hitting fallback. + for i := 0; i < 30; i++ { + path := filepath.Join(venvDir, fmt.Sprintf("file%d.txt", i)) + require.NoError(t, os.WriteFile(path, []byte("x"), 0644)) + } + + config := newDefaultBubblewrapConfig() + translator := newBubblewrapPolicyTranslator(config) + + policy := &sandbox.SandboxPolicy{ + Name: "test-venv-write", + Filesystem: sandbox.FilesystemPolicy{ + AllowRead: []string{tmpDir + "/**"}, + AllowWrite: []string{venvDir + "/**"}, + }, + } + + args, err := translator.translate(policy) + require.NoError(t, err) + + assertWriteBind(t, args, venvDir) + assertNoWriteBind(t, args, pythonPath) + assertReadBind(t, args, pythonPath) +} + // TestGlobNoFallbackSmallPattern tests that small patterns don't trigger fallback func TestGlobNoFallbackSmallPattern(t *testing.T) { tmpDir := t.TempDir() @@ -905,6 +937,57 @@ func TestExtractParentDir(t *testing.T) { } } +func TestExtractGlobstarWriteBaseDir(t *testing.T) { + cases := []struct { + name string + pattern string + expected string + }{ + { + name: "suffix globstar (profiles)", + pattern: "/home/user/.venv/**", + expected: "/home/user/.venv", + }, + { + name: "in-pattern globstars", + pattern: "/a/b/**/d/**/e", + expected: "/a/b", + }, + { + name: "middle globstar with file suffix", + pattern: "/usr/lib/**/*.so", + expected: "/usr/lib", + }, + } + + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, extractGlobstarWriteBaseDir(tt.pattern)) + }) + } +} + +func TestBubblewrapGlobstarWriteMultiSegmentBind(t *testing.T) { + tmpDir := t.TempDir() + baseDir := filepath.Join(tmpDir, "a", "b") + require.NoError(t, os.MkdirAll(baseDir, 0755)) + + pattern := filepath.Join(tmpDir, "a", "b", "**", "d", "**", "e") + config := newDefaultBubblewrapConfig() + translator := newBubblewrapPolicyTranslator(config) + + policy := &sandbox.SandboxPolicy{ + Name: "test-multi-globstar-write", + Filesystem: sandbox.FilesystemPolicy{ + AllowWrite: []string{pattern}, + }, + } + + args, err := translator.translate(policy) + require.NoError(t, err) + assertWriteBind(t, args, baseDir) +} + // Helper function to convert arg slice to string for easier assertion func argSliceToString(args []string) string { result := "" @@ -1054,6 +1137,25 @@ 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 assertWriteBind(t *testing.T, args []string, path string) { + t.Helper() + for i := 0; i+2 < len(args); i++ { + if (args[i] == "--bind" || args[i] == "--bind-try") && args[i+1] == path && args[i+2] == path { + return + } + } + t.Fatalf("expected --bind-try %q %q, not found in args: %v", path, path, args) +} + +func assertNoWriteBind(t *testing.T, args []string, path string) { + t.Helper() + for i := 0; i+2 < len(args); i++ { + if (args[i] == "--bind" || args[i] == "--bind-try") && args[i+1] == path && args[i+2] == path { + t.Fatalf("unexpected writable bind at %q in args: %v", path, args) + } + } +} + func assertReadOnlyBindAfterWritableBind(t *testing.T, args []string, readOnlyPath string, writablePath string) { t.Helper() diff --git a/sandbox/platform/glob_linux.go b/sandbox/platform/glob_linux.go index 9688386..4888db7 100644 --- a/sandbox/platform/glob_linux.go +++ b/sandbox/platform/glob_linux.go @@ -108,3 +108,19 @@ func extractGlobParentDir(pattern string) string { } return pattern } + +// extractGlobstarWriteBaseDir returns the directory to bind read-write for an +// allow_write globstar pattern. It uses the path before the first "/**" so +// suffix-only profiles (${CWD}/.venv/**) and in-pattern globstars (/a/b/**/d/**/e) +// both bind the intended tree root (/a/b for the latter). Falls back to +// extractGlobParentDir when the pattern has ** but no "/**" segment. +func extractGlobstarWriteBaseDir(pattern string) string { + if i := strings.Index(pattern, "/**"); i >= 0 { + base := strings.TrimSuffix(pattern[:i], string(filepath.Separator)) + if base == "" { + return string(filepath.Separator) + } + return base + } + return extractGlobParentDir(pattern) +} diff --git a/test/pm-e2e.js b/test/pm-e2e.js index 2ba7511..d5c131b 100644 --- a/test/pm-e2e.js +++ b/test/pm-e2e.js @@ -47,15 +47,37 @@ function cleanup(dir) { } } +// npm-restrictive allow_write includes /tmp/**. Use cache/store paths under /tmp so +// bubblewrap can bind /tmp read-write without pre-creating ~/.npm or ~/.cache/pnpm. +function pmEnv(pm) { + const base = path.join(os.tmpdir(), 'pmg-e2e'); + fs.mkdirSync(base, { recursive: true }); + + const env = { ...process.env }; + env.npm_config_cache = path.join(base, 'npm-cache'); + fs.mkdirSync(env.npm_config_cache, { recursive: true }); + + if (pm === 'pnpm') { + env.PNPM_HOME = path.join(base, 'pnpm-home'); + env.npm_config_store_dir = path.join(base, 'pnpm-store'); + fs.mkdirSync(env.PNPM_HOME, { recursive: true }); + fs.mkdirSync(env.npm_config_store_dir, { recursive: true }); + env.PATH = `${env.PNPM_HOME}${path.delimiter}${env.PATH}`; + } + + return env; +} + function testPackageManager(pm) { const testDir = createTempDir(`pmg-e2e-${pm}-`); + const env = pmEnv(pm); console.log(`\n Test directory: ${testDir}`); try { // Initialize project test(`${pm}: Initialize project`, () => { const initCmd = pm === 'npm' ? 'npm init -y' : 'pnpm init'; - const result = exec(initCmd, { cwd: testDir }); + const result = exec(initCmd, { cwd: testDir, env }); if (!result.success) { console.log(` ❌ FAIL: ${result.error}`); return false; @@ -77,7 +99,7 @@ function testPackageManager(pm) { const addCmd = pm === 'npm' ? `npm install ${depsList}` : `pnpm add ${depsList}`; - const result = exec(addCmd, { cwd: testDir }); + const result = exec(addCmd, { cwd: testDir, env }); if (!result.success) { console.log(` ❌ FAIL: ${result.error}`); return false; @@ -122,7 +144,7 @@ function testPackageManager(pm) { // Reinstall const installCmd = pm === 'npm' ? 'npm install' : 'pnpm install'; - const result = exec(installCmd, { cwd: testDir }); + const result = exec(installCmd, { cwd: testDir, env }); if (!result.success) { console.log(` ❌ FAIL: ${result.error}`); return false;