diff --git a/internal/cli/run.go b/internal/cli/run.go index a5ff8cd351..c5d10fb80a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -3,8 +3,10 @@ package cli import ( "context" "encoding/json" + "errors" "fmt" "io" + "io/fs" "net/http" "os" "os/exec" @@ -887,8 +889,12 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep // 9d. Extract target repo back to host. SafeDownload removes dangerous // symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape. - if clearErr := os.RemoveAll(hostRepositoryDir); clearErr != nil { - return fmt.Errorf("clearing local repo %s before extraction: %w", hostRepositoryDir, clearErr) + // Use forceRemoveAll to handle files created by sandboxed processes with + // restrictive permissions (#605). Warn on failure instead of aborting so + // the post-script still runs (review results are more valuable than a + // clean temp directory). + if clearErr := forceRemoveAll(hostRepositoryDir); clearErr != nil { + printer.StepWarn(fmt.Sprintf("Could not fully clear %s before extraction (continuing): %v", hostRepositoryDir, clearErr)) } repoExtractStart := time.Now() printer.StepStart("Extracting target repo") @@ -1296,6 +1302,25 @@ func escapeForDoubleQuotes(s string) string { return s } +// forceRemoveAll removes a directory tree. If the initial removal fails with a +// permission error (e.g. files created by a sandboxed process with a different +// UID), it makes all entries owner-writable and retries. See #605. +func forceRemoveAll(path string) error { + err := os.RemoveAll(path) + if err == nil || !errors.Is(err, fs.ErrPermission) { + return err + } + // Best-effort chmod: make every entry owner-rwx so the retry can unlink. + _ = filepath.WalkDir(path, func(p string, _ fs.DirEntry, walkErr error) error { + if walkErr != nil { + return nil // keep walking + } + _ = os.Chmod(p, 0o700) + return nil + }) + return os.RemoveAll(path) +} + // validationFailMessage returns a human-readable message for a validation // script failure. When the script produces output, that output is used; // otherwise it falls back to the exec error string (e.g. ENOENT / EACCES). diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 10fdb2a76c..6278629e66 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -1423,6 +1423,43 @@ func TestEffectiveMaxRuntimeFetches_MatchesFetchsvcDefault(t *testing.T) { } } +func TestForceRemoveAll(t *testing.T) { + t.Run("removes normal directory", func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + require.NoError(t, os.MkdirAll(filepath.Join(target, "sub"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(target, "sub", "file"), []byte("x"), 0o644)) + + require.NoError(t, forceRemoveAll(target)) + _, err := os.Stat(target) + assert.True(t, os.IsNotExist(err)) + }) + + t.Run("handles restrictive directory permissions", func(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("test requires non-root to simulate permission errors") + } + dir := t.TempDir() + target := filepath.Join(dir, "target") + sub := filepath.Join(target, "sub") + require.NoError(t, os.MkdirAll(sub, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(sub, "file"), []byte("x"), 0o644)) + // Remove write from the subdirectory so os.RemoveAll fails. + require.NoError(t, os.Chmod(sub, 0o500)) + + // Confirm plain RemoveAll fails. + require.Error(t, os.RemoveAll(target)) + + require.NoError(t, forceRemoveAll(target)) + _, err := os.Stat(target) + assert.True(t, os.IsNotExist(err)) + }) + + t.Run("nonexistent path is no-op", func(t *testing.T) { + require.NoError(t, forceRemoveAll(filepath.Join(t.TempDir(), "nope"))) + }) +} + type mockForgeClient struct { forge.Client }