diff --git a/cmd/agent/workspace/build.go b/cmd/agent/workspace/build.go index d828a5487..05bc52825 100644 --- a/cmd/agent/workspace/build.go +++ b/cmd/agent/workspace/build.go @@ -120,6 +120,8 @@ func deleteWorkspace( log.Errorf("Removing container: %v", err) } - _ = os.RemoveAll(workspaceInfo.Origin) + if err := forceRemoveAll(workspaceInfo.Origin); err != nil { + log.Errorf("remove workspace folder: %v", err) + } return nil } diff --git a/cmd/agent/workspace/delete.go b/cmd/agent/workspace/delete.go index 38897acc0..4d4f7846f 100644 --- a/cmd/agent/workspace/delete.go +++ b/cmd/agent/workspace/delete.go @@ -3,7 +3,9 @@ package workspace import ( "context" "fmt" + "io/fs" "os" + "path/filepath" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent" @@ -79,7 +81,9 @@ func (cmd *DeleteCmd) Run(ctx context.Context) error { } // delete workspace folder - _ = os.RemoveAll(workspaceInfo.Origin) + if err := forceRemoveAll(workspaceInfo.Origin); err != nil { + log.Errorf("remove workspace folder: %v", err) + } return nil } @@ -109,6 +113,39 @@ func removeContainer( return nil } +// forceRemoveAll attempts os.RemoveAll and, on failure, makes all directories +// writable before retrying. Container runtimes (e.g. crun with Podman) can +// leave directories without write permission, causing a standard RemoveAll to +// fail with "permission denied". +func forceRemoveAll(path string) error { + err := os.RemoveAll(path) + if err == nil || os.IsNotExist(err) { + return nil + } + + // Walk the tree and add owner-write+execute to every directory so + // entries inside them can be unlinked on the retry. + _ = filepath.WalkDir(path, func(p string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return nil + } + if !d.IsDir() { + return nil + } + info, infoErr := d.Info() + if infoErr != nil { + return nil + } + _ = os.Chmod( + p, + info.Mode()|0o700, + ) // #nosec G122 -- intentional: fixing perms for deletion, path is not user-controlled + return nil + }) + + return os.RemoveAll(path) +} + func removeDaemon(workspaceInfo *provider2.AgentWorkspaceInfo) error { if len(workspaceInfo.Agent.Exec.Shutdown) == 0 { return nil diff --git a/cmd/agent/workspace/delete_test.go b/cmd/agent/workspace/delete_test.go new file mode 100644 index 000000000..be0fec0b3 --- /dev/null +++ b/cmd/agent/workspace/delete_test.go @@ -0,0 +1,122 @@ +package workspace + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +func skipIfPermissionsNotEnforced(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("permission test not applicable on Windows") + } + if os.Getuid() == 0 { + t.Skip("test not meaningful when running as root") + } +} + +func chmodReadOnly(t *testing.T, path string) { + t.Helper() + err := os.Chmod(path, 0o500) // #nosec G302 -- intentional: testing restrictive perms + if err != nil { + t.Fatal(err) + } +} + +func assertRemoved(t *testing.T, path string) { + t.Helper() + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("expected directory to be removed, got stat err: %v", err) + } +} + +func TestForceRemoveAll_RegularDirectory(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "workspace") + if err := os.MkdirAll(filepath.Join(target, "content", "src"), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile( + filepath.Join(target, "content", "src", "main.go"), + []byte("package main"), + 0o600, + ); err != nil { + t.Fatal(err) + } + + if err := forceRemoveAll(target); err != nil { + t.Fatalf("forceRemoveAll failed: %v", err) + } + assertRemoved(t, target) +} + +func TestForceRemoveAll_NonExistentPath(t *testing.T) { + if err := forceRemoveAll("/tmp/nonexistent-path-that-does-not-exist-12345"); err != nil { + t.Fatalf("forceRemoveAll on nonexistent path should not error: %v", err) + } +} + +func TestForceRemoveAll_ReadOnlyDirectory(t *testing.T) { + skipIfPermissionsNotEnforced(t) + + dir := t.TempDir() + target := filepath.Join(dir, "workspace") + content := filepath.Join(target, "content") + if err := os.MkdirAll(content, 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(content, "file.txt"), []byte("data"), 0o600); err != nil { + t.Fatal(err) + } + + // Remove write+execute from content dir — simulates what crun does. + chmodReadOnly(t, content) + + // Standard RemoveAll should fail. + if err := os.RemoveAll(target); err == nil { + t.Skip("os.RemoveAll succeeded unexpectedly — filesystem may not enforce permissions") + } + + // forceRemoveAll should fix permissions and succeed. + if err := forceRemoveAll(target); err != nil { + t.Fatalf("forceRemoveAll failed: %v", err) + } + assertRemoved(t, target) +} + +func TestForceRemoveAll_NestedReadOnlyDirectories(t *testing.T) { + skipIfPermissionsNotEnforced(t) + + dir := t.TempDir() + target := filepath.Join(dir, "workspace") + deep := filepath.Join(target, "content", "a", "b", "c") + if err := os.MkdirAll(deep, 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(deep, "file.txt"), []byte("data"), 0o600); err != nil { + t.Fatal(err) + } + + // Make several levels read-only (bottom-up to avoid locking ourselves out). + for _, d := range []string{ + deep, + filepath.Join(target, "content", "a", "b"), + filepath.Join(target, "content", "a"), + filepath.Join(target, "content"), + } { + chmodReadOnly(t, d) + } + + if err := forceRemoveAll(target); err != nil { + t.Fatalf("forceRemoveAll failed: %v", err) + } + assertRemoved(t, target) +} + +func TestForceRemoveAll_EmptyString(t *testing.T) { + if err := forceRemoveAll(""); err != nil { + t.Fatalf("forceRemoveAll with empty string should not error: %v", err) + } +} diff --git a/e2e/tests/down/down.go b/e2e/tests/down/down.go index 250432cdf..f0df2cdd4 100644 --- a/e2e/tests/down/down.go +++ b/e2e/tests/down/down.go @@ -65,6 +65,42 @@ var _ = ginkgo.Describe("testing down command", ginkgo.Label("down"), func() { gomega.Expect(err).To(gomega.HaveOccurred(), "workspace should not be in list after down") }, ginkgo.SpecTimeout(framework.TimeoutModerate())) + ginkgo.It( + "down deletes workspace with restrictive folder permissions", + func(ctx context.Context) { + f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") + framework.ExpectNoError(err) + + name := "vscode-remote-try-python" + ginkgo.DeferCleanup(f.DevsyWorkspaceDelete, name) + + err = f.DevsyUp(ctx, "https://github.com/microsoft/vscode-remote-try-python.git") + framework.ExpectNoError(err) + + // Get workspace to find local folder path + workspace, err := f.FindWorkspace(ctx, name) + framework.ExpectNoError(err) + + // Set restrictive permissions on the workspace source folder to simulate the bug + // where container runtime leaves folders with 0500 permissions + if workspace.Source.LocalFolder != "" { + folder := workspace.Source.LocalFolder + err = os.Chmod(folder, 0o500) //nolint:gosec + framework.ExpectNoError(err) + } + + // down should succeed even with restrictive permissions + err = f.DevsyDown(ctx, name) + framework.ExpectNoError(err) + + // Verify workspace no longer appears in list + _, err = f.FindWorkspace(ctx, name) + gomega.Expect(err). + To(gomega.HaveOccurred(), "workspace should not be in list after down") + }, + ginkgo.SpecTimeout(framework.TimeoutModerate()), + ) + ginkgo.It("stop only stops and does not delete workspace", func(ctx context.Context) { f, err := framework.SetupDockerProvider(initialDir+"/bin", "docker") framework.ExpectNoError(err)