From e87b881e98092c4469aa68a65aea16ca81fbc0e8 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 14 May 2026 19:27:20 -0500 Subject: [PATCH 1/2] fix(agent): force-remove workspace folder with restrictive permissions Container runtimes like crun (used by Podman) can leave directories inside the workspace content folder without write permission. The standard os.RemoveAll fails on these and the error was silently discarded, leaving the workspace directory behind after delete. Add forceRemoveAll that retries removal after making all directories writable. Also log the error if removal still fails instead of silently ignoring it. Fixes: #754 --- cmd/agent/workspace/build.go | 4 +- cmd/agent/workspace/delete.go | 39 ++++++++- cmd/agent/workspace/delete_test.go | 122 +++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 cmd/agent/workspace/delete_test.go 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) + } +} From 36a23a900f1e3f2564e690815945a328d4c318d0 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 14 May 2026 19:36:28 -0500 Subject: [PATCH 2/2] test(e2e): add delete flow test with restrictive folder permissions --- e2e/tests/down/down.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) 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)