From aa3151e25d4e59a8ebdebf15ac075a8819479e10 Mon Sep 17 00:00:00 2001 From: jnhu Date: Wed, 17 Jun 2026 07:05:11 +0800 Subject: [PATCH] fix(trash): fix two regressions from PR #230 path hardening - List() now uses trashLogicalName() to produce slash-separated logical names, preventing Windows backslash paths that validateTrashName rejects. - ensureUnderBase/ensureStrictlyUnderBase now use filepath.Abs+filepath.Rel instead of string prefix, fixing false escapes when base is '.'. - Add regression tests for Restore(entry, '.'), nested List->Restore roundtrip, sibling prefix rejection, and validateTrashName backslash enforcement. --- internal/trash/trash.go | 69 ++++++++++---- internal/trash/trash_security_test.go | 127 ++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 19 deletions(-) diff --git a/internal/trash/trash.go b/internal/trash/trash.go index 8d4c30f1..e576b845 100644 --- a/internal/trash/trash.go +++ b/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 { diff --git a/internal/trash/trash_security_test.go b/internal/trash/trash_security_test.go index e9d57947..6052d6c8 100644 --- a/internal/trash/trash_security_test.go +++ b/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") + } +}