Fix trash restore regressions from path hardening

internal/trash/trash.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"io/fs"
 	"os"
+	"path"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -47,33 +48,66 @@ func validateTrashName(name string) error {
 	return nil
 }
 
+// trashLogicalName builds a slash-separated logical name for a trash entry.
+// It normalizes OS path separators to '/' so that List() returns names
+// compatible with validateTrashName() (which rejects backslashes).
+func trashLogicalName(parentRel, name string) string {
+	if parentRel == "." {
+		return name
+	}
+
+	// filepath.ToSlash converts the current OS path separator to slash.
+	// Do not translate literal backslashes on Unix; they are filename
+	// characters, not path separators, and should remain invalid input.
+	parentRel = filepath.ToSlash(parentRel)
+	return path.Join(parentRel, name)
+}
+
 // ensureUnderBase asserts that candidate resolves to a path under base.
-// Both are cleaned before comparison.  candidate == base is allowed (used by
-// RestoreAgent where targetDir may equal destDir).
+// Both are resolved to absolute paths before comparison.
+// candidate == base is allowed (used by RestoreAgent where targetDir may
+// equal destDir).
 func ensureUnderBase(base, candidate string) error {
-	base = filepath.Clean(base)
-	candidate = filepath.Clean(candidate)
-	if candidate == base {
-		return nil
-	}
-	if !strings.HasPrefix(candidate, base+string(filepath.Separator)) {
-		return fmt.Errorf("path %q escapes base %q", candidate, base)
-	}
-	return nil
+	return ensureUnderBaseRel(base, candidate, true)
 }
 
 // ensureStrictlyUnderBase is like ensureUnderBase but rejects candidate == base.
 // Used by destructive sinks (e.g. Cleanup) that must never target the base
 // directory itself.
 func ensureStrictlyUnderBase(base, candidate string) error {
-	base = filepath.Clean(base)
-	candidate = filepath.Clean(candidate)
-	if candidate == base {
+	return ensureUnderBaseRel(base, candidate, false)
+}
+
+func ensureUnderBaseRel(base, candidate string, allowEqual bool) error {
+	baseAbs, err := filepath.Abs(base)
+	if err != nil {
+		return fmt.Errorf("resolve base %q: %w", base, err)
+	}
+
+	candidateAbs, err := filepath.Abs(candidate)
+	if err != nil {
+		return fmt.Errorf("resolve candidate %q: %w", candidate, err)
+	}
+
+	baseAbs = filepath.Clean(baseAbs)
+	candidateAbs = filepath.Clean(candidateAbs)
+
+	rel, err := filepath.Rel(baseAbs, candidateAbs)
+	if err != nil {
+		return fmt.Errorf("path %q escapes base %q: %w", candidate, base, err)
+	}
+
+	if rel == "." {
+		if allowEqual {
+			return nil
+		}
 		return fmt.Errorf("path %q equals base %q — refusing to operate on base", candidate, base)
 	}
-	if !strings.HasPrefix(candidate, base+string(filepath.Separator)) {
+
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) {
 		return fmt.Errorf("path %q escapes base %q", candidate, base)
 	}
+
 	return nil
 }
 
@@ -210,10 +244,7 @@ func List(trashBase string) []TrashEntry {
 
 		// Compute full nested name from relative path
 		parentRel, _ := filepath.Rel(trashBase, filepath.Dir(path))
-		fullName := name
-		if parentRel != "." {
-			fullName = filepath.Join(parentRel, name)
-		}
+		fullName := trashLogicalName(parentRel, name)
 
 		date, parseErr := time.Parse("2006-01-02_15-04-05", ts)
 		if parseErr != nil {

internal/trash/trash_security_test.go

@@ -3,6 +3,7 @@ package trash
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -597,3 +598,129 @@ func TestReadDir_NameNeverContainsSeparator(t *testing.T) {
 		}
 	}
 }
+
+// --- Regression tests for PR #230 compatibility fixes ---
+
+func TestRestoreAllowsCurrentDirectoryDestination(t *testing.T) {
+	origDir, _ := os.Getwd()
+	tmpDir := t.TempDir()
+	if err := os.Chdir(tmpDir); err != nil {
+		t.Fatal(err)
+	}
+	defer os.Chdir(origDir)
+
+	trashBase := filepath.Join(tmpDir, ".trash", "skills")
+	trashDir := filepath.Join(trashBase, "demo_2006-01-02_15-04-05")
+	os.MkdirAll(trashDir, 0755)
+	os.WriteFile(filepath.Join(trashDir, "SKILL.md"), []byte("demo content"), 0644)
+
+	entry := &TrashEntry{
+		Name:      "demo",
+		Path:      trashDir,
+		Timestamp: "2006-01-02_15-04-05",
+		Date:      time.Now(),
+	}
+
+	if err := Restore(entry, "."); err != nil {
+		t.Fatalf("Restore to current directory failed: %v", err)
+	}
+
+	restored := filepath.Join(tmpDir, "demo", "SKILL.md")
+	if _, err := os.Stat(restored); err != nil {
+		t.Errorf("restored file not found: %v", err)
+	}
+
+	if _, err := os.Stat(trashDir); !os.IsNotExist(err) {
+		t.Error("trash entry should be removed after restore")
+	}
+}
+
+func TestEnsureUnderBaseAllowsChildOfDotButRejectsEscapes(t *testing.T) {
+	if err := ensureUnderBase(".", "demo"); err != nil {
+		t.Fatalf("ensureUnderBase(\".\", \"demo\") should succeed: %v", err)
+	}
+	if err := ensureUnderBase(".", "sub/dir"); err != nil {
+		t.Fatalf("ensureUnderBase(\".\", \"sub/dir\") should succeed: %v", err)
+	}
+	if err := ensureUnderBase(".", "../outside"); err == nil {
+		t.Error("ensureUnderBase(\".\", \"../outside\") should fail")
+	}
+	if err := ensureUnderBase(".", "../../etc/passwd"); err == nil {
+		t.Error("ensureUnderBase(\".\", \"../../etc/passwd\") should fail")
+	}
+}
+
+func TestRestoreNestedEntryReturnedByList(t *testing.T) {
+	tmpDir := t.TempDir()
+	trashBase := filepath.Join(tmpDir, "trash")
+	destDir := filepath.Join(tmpDir, "dest")
+	os.MkdirAll(destDir, 0755)
+
+	// Create nested trash entry: trash/org/demo_2006-01-02_15-04-05/SKILL.md
+	trashDir := filepath.Join(trashBase, "org", "demo_2006-01-02_15-04-05")
+	os.MkdirAll(trashDir, 0755)
+	os.WriteFile(filepath.Join(trashDir, "SKILL.md"), []byte("nested content"), 0644)
+
+	items := List(trashBase)
+	if len(items) != 1 {
+		t.Fatalf("expected 1 item, got %d", len(items))
+	}
+	if items[0].Name != "org/demo" {
+		t.Errorf("expected Name 'org/demo', got %q", items[0].Name)
+	}
+
+	if err := Restore(&items[0], destDir); err != nil {
+		t.Fatalf("Restore failed: %v", err)
+	}
+
+	restored := filepath.Join(destDir, "org", "demo", "SKILL.md")
+	if _, err := os.Stat(restored); err != nil {
+		t.Errorf("restored file not found: %v", err)
+	}
+}
+
+func TestTrashLogicalNameNormalizesOSNativeSeparators(t *testing.T) {
+	parentRel := filepath.Join("org", "team")
+
+	got := trashLogicalName(parentRel, "demo")
+	want := "org/team/demo"
+
+	if got != want {
+		t.Fatalf("trashLogicalName(%q, %q) = %q, want %q", parentRel, "demo", got, want)
+	}
+}
+
+func TestTrashLogicalNameDoesNotTranslateLiteralUnixBackslashes(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("literal backslash is an OS separator on Windows")
+	}
+
+	got := trashLogicalName(`..\outside`, "demo")
+	if got == "../outside/demo" {
+		t.Fatalf("literal Unix backslash should not be translated into path separator: got %q", got)
+	}
+	if !strings.Contains(got, `\`) {
+		t.Fatalf("literal Unix backslash should be preserved, got %q", got)
+	}
+}
+
+func TestEnsureUnderBaseRejectsSiblingPrefix(t *testing.T) {
+	root := t.TempDir()
+
+	base := filepath.Join(root, "skills")
+	sibling := filepath.Join(root, "skills-evil", "demo")
+
+	if err := os.MkdirAll(filepath.Dir(sibling), 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := ensureUnderBase(base, sibling); err == nil {
+		t.Fatal("sibling path with shared prefix should be rejected")
+	}
+}
+
+func TestValidateTrashNameRejectsBackslash(t *testing.T) {
+	if err := validateTrashName(`org\demo`); err == nil {
+		t.Fatal("trash name with backslash should be rejected")
+	}
+}