Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion cmd/agent/workspace/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
39 changes: 38 additions & 1 deletion cmd/agent/workspace/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down
122 changes: 122 additions & 0 deletions cmd/agent/workspace/delete_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
36 changes: 36 additions & 0 deletions e2e/tests/down/down.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading