From 7fd3062cd816fb434cbc3d6769994f3a7e555f25 Mon Sep 17 00:00:00 2001 From: argonui <92067588+argonui@users.noreply.github.com> Date: Sun, 2 Aug 2026 19:58:27 -0500 Subject: [PATCH] file: guard Clear with an ownership marker and a path guard Reverse mode calls DirOps.Clear(), which os.RemoveAll's the objects directory. A mistargeted --moddir (e.g. $HOME) could destroy unrelated data, and the extension allowlist was only a weak proxy for ownership. Clear now drops a hidden .ttsmm-managed sentinel after recreating the directory and refuses to delete unless the directory is absent, empty, already carries the marker, or (for backward compatibility) passes the existing extension allowlist. A pathGuard additionally refuses the filesystem root, the home directory, any ancestor of home, and suspiciously shallow paths. The marker is filtered out of ListFilesAndFolders so it stays invisible to the rest of the pipeline. Fixes #96 Co-Authored-By: Claude Opus 5 --- file/AGENTS.md | 33 +++++++++--- file/dirops.go | 126 ++++++++++++++++++++++++++++++++++++++++++- file/dirops_test.go | 129 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 278 insertions(+), 10 deletions(-) diff --git a/file/AGENTS.md b/file/AGENTS.md index b932e03..1416bc9 100644 --- a/file/AGENTS.md +++ b/file/AGENTS.md @@ -39,13 +39,32 @@ way POSIX tools expect, without that newline leaking into the mod content and ap spurious character in TTS. The two halves are a matched pair — changing one without the other adds or eats a newline on every round trip. -## `Clear` and its safety check - -`DirOps.Clear()` deletes and recreates a directory. Because it is destructive, `preClearCheck` -first walks the tree and refuses if it finds a file whose extension is not in `allowedExtensions` -(`.json`, `.gmnotes`, `.luascriptstate`, `.ttslua`, `.xml`). The intent is that the objects -directory should only ever contain files this tool generated, so anything unrecognized means the -path is wrong and deleting would destroy someone's work. +## `Clear` and its safety checks + +`DirOps.Clear()` deletes and recreates a directory. Because it is destructive it runs two guards +before touching anything. + +**Ownership marker.** After a successful clear, `Clear` drops a hidden sentinel file +(`.ttsmm-managed`, the `managedMarker` constant) into the directory. On the next run that marker is +proof the tool created the directory, so deleting it is safe. `Clear` refuses unless one of: + +- the directory does not exist, +- the directory is empty, +- the directory already contains the marker, or +- the directory has no marker but every file passes the legacy extension allowlist + (`allowedExtensions`: `.json`, `.gmnotes`, `.luascriptstate`, `.ttslua`, `.xml`) — backward + compatibility for trees written before the marker existed; they clear once and gain a marker + going forward. + +A non-empty directory with no marker and unrecognized content (a mistargeted `--moddir` pointed at, +say, `$HOME`) is refused with an error explaining how to proceed. This also resolves the older +`.DS_Store`/`.gitkeep` false-positive, since a tool-created directory carries a marker and the +extension check no longer gates it. + +**Path guard.** Independent of contents, `pathGuard` resolves the target to an absolute, cleaned +path and refuses the filesystem root, the user's home directory, any ancestor of home, and +suspiciously shallow single-segment paths — targets that are almost always a typo rather than a mod +tree. ## `conversions.go` diff --git a/file/dirops.go b/file/dirops.go index 455bef2..902bc04 100644 --- a/file/dirops.go +++ b/file/dirops.go @@ -6,6 +6,7 @@ import ( "os" "path" "path/filepath" + "strings" "time" ) @@ -18,6 +19,12 @@ var allowedExtensions = map[string]struct{}{ ".xml": {}, } +// managedMarker is the hidden ownership marker (sentinel) this tool drops into any +// directory it manages. Its presence is proof that the directory was created by +// TTSModManager, which is what makes deleting the directory's contents safe. See the +// "Clear and its safety check" section of AGENTS.md. +const managedMarker = ".ttsmm-managed" + // DirCreator abstracts folder creation type DirCreator interface { CreateDir(relpath string, suggestion string) (string, error) @@ -89,12 +96,119 @@ func (d *DirOps) preClearCheck() error { return walkErr } +// pathGuard refuses to operate on paths that are almost certainly a mistargeted +// --moddir rather than a real objects directory: the filesystem root, the user's home +// directory (or any ancestor of it), or any suspiciously shallow path. Deleting any of +// these would destroy unrelated user data. +func pathGuard(target string) error { + abs, err := filepath.Abs(target) + if err != nil { + return fmt.Errorf("could not resolve absolute path of %s: %w", target, err) + } + abs = filepath.Clean(abs) + + // Refuse the filesystem root. + if abs == filepath.Clean(string(filepath.Separator)) { + return fmt.Errorf("refusing to clear filesystem root %q", abs) + } + + // Refuse the home directory and any ancestor of it. + if home, herr := os.UserHomeDir(); herr == nil && home != "" { + home = filepath.Clean(home) + if abs == home { + return fmt.Errorf("refusing to clear home directory %q", abs) + } + // An ancestor of home (e.g. /home, / on some systems) is even more dangerous. + // abs is a strict ancestor when the path from abs down to home never climbs out. + if rel, rerr := filepath.Rel(abs, home); rerr == nil && rel != "." && !startsWithParent(rel) { + return fmt.Errorf("refusing to clear %q, an ancestor of your home directory %q", abs, home) + } + } + + // Refuse suspiciously shallow paths: a single segment below the root such as + // "/objects" is far more likely a typo than a real mod tree. + trimmed := strings.Trim(abs, string(filepath.Separator)) + if trimmed == "" { + return fmt.Errorf("refusing to clear filesystem root %q", abs) + } + if !strings.ContainsRune(trimmed, filepath.Separator) { + return fmt.Errorf("refusing to clear suspiciously shallow path %q; pass a --moddir at least two levels deep", abs) + } + + return nil +} + +// startsWithParent reports whether a filepath.Rel result indicates the target is an +// ancestor of home (the relative path from target to home does not need to climb out). +func startsWithParent(rel string) bool { + return rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + +// isClearable decides whether it is safe to delete the contents of d.base. Deletion is +// allowed when the directory: +// - does not exist, or +// - is empty, or +// - already contains our ownership marker (proof this tool created it), or +// - passes the legacy extension allowlist (backward compatibility for trees written +// by versions predating the marker). +// +// Anything else - a non-empty directory of unrecognized files with no marker, such as a +// mistargeted $HOME - is refused. +func (d *DirOps) isClearable() error { + entries, err := os.ReadDir(d.base) + if err != nil { + if os.IsNotExist(err) { + return nil // absent: nothing to destroy + } + return fmt.Errorf("reading directory %s: %w", d.base, err) + } + + if len(entries) == 0 { + return nil // empty: nothing to destroy + } + + // Marker present: this tool created the directory, so clearing is safe. + for _, e := range entries { + if !e.IsDir() && e.Name() == managedMarker { + return nil + } + } + + // Backward compatibility: a directory that predates the marker but contains only + // recognized content is still ours to clear (and gains a marker going forward). + if err := d.preClearCheck(); err != nil { + return fmt.Errorf( + "%s is not empty, has no %s ownership marker, and contains unrecognized content: %w; "+ + "refusing to delete it in case --moddir is pointed at the wrong place. "+ + "If this really is a mod objects directory, remove the offending file or the whole directory by hand and re-run", + d.base, managedMarker, err) + } + return nil +} + +// writeMarker drops the ownership marker into d.base so future Clear calls recognize the +// directory as one this tool manages. +func (d *DirOps) writeMarker() error { + marker := filepath.Join(d.base, managedMarker) + content := []byte("This directory is managed by TTSModManager. It may be deleted and recreated by reverse mode.\n") + if err := os.WriteFile(marker, content, 0644); err != nil { + return fmt.Errorf("error writing ownership marker %s: %w", marker, err) + } + return nil +} + // Clear removes all contents from the base directory and recreates it. func (d *DirOps) Clear() error { log.Println("Performing safety check...") startTime := time.Now() - if err := d.preClearCheck(); err != nil { + // Path guard: refuse obviously dangerous targets regardless of contents. + if err := pathGuard(d.base); err != nil { + return fmt.Errorf("pre-clear safety check failed, operation aborted: %w", err) + } + + // Ownership guard: refuse to delete a directory this tool does not appear to own. + if err := d.isClearable(); err != nil { return fmt.Errorf("pre-clear safety check failed, operation aborted: %w", err) } @@ -111,6 +225,11 @@ func (d *DirOps) Clear() error { return fmt.Errorf("error recreating directory %s: %w", d.base, err) } + // Drop the ownership marker so this directory is recognized as ours next time. + if err := d.writeMarker(); err != nil { + return err + } + log.Printf("Cleared and recreated directory: %s", d.base) return nil } @@ -128,6 +247,11 @@ func (d *DirOps) ListFilesAndFolders(relpath string) ([]string, []string, error) if entry.IsDir() { folnames = append(folnames, filepath.Join(relpath, entry.Name())) } else { + // Hide the ownership marker: it is an internal file/ bookkeeping detail and + // must not be seen as mod content by the rest of the pipeline. + if entry.Name() == managedMarker { + continue + } fnames = append(fnames, filepath.Join(relpath, entry.Name())) } } diff --git a/file/dirops_test.go b/file/dirops_test.go index 3ae9b80..40a8481 100644 --- a/file/dirops_test.go +++ b/file/dirops_test.go @@ -114,8 +114,12 @@ func TestClearRemovesAndRecreates(t *testing.T) { if err != nil { t.Fatalf("ReadDir() after Clear: %v", err) } - if len(entries) != 0 { - t.Errorf("expected empty directory after Clear, found %d entries", len(entries)) + // Clear now drops the ownership marker into the recreated dir (#96), + // so the only permitted entry is that marker. + for _, e := range entries { + if e.Name() != managedMarker { + t.Errorf("expected only the ownership marker after Clear, found %q", e.Name()) + } } } @@ -162,3 +166,124 @@ func TestListFilesAndFolders(t *testing.T) { t.Errorf("folders want != got:\n%v\n", diff) } } + +// --- ownership-marker + path-guard tests (#96) --- + +func writeFile(t *testing.T, p, content string) { + t.Helper() + if err := os.WriteFile(p, []byte(content), 0644); err != nil { + t.Fatalf("could not write %s: %v", p, err) + } +} + +func markerExists(dir string) bool { + _, err := os.Stat(filepath.Join(dir, managedMarker)) + return err == nil +} + +// (a) An unmarked, non-empty directory containing a disallowed file is refused, and its +// contents are left untouched. +func TestClearRefusesUnmarkedForeignContent(t *testing.T) { + dir := t.TempDir() + victim := filepath.Join(dir, "important.txt") + writeFile(t, victim, "the user's real data") + + d := NewDirOps(dir) + if err := d.Clear(); err == nil { + t.Fatalf("expected Clear to refuse an unmarked directory with foreign content, got nil") + } + + if _, err := os.Stat(victim); err != nil { + t.Fatalf("Clear deleted the user's file despite refusing: %v", err) + } +} + +// (b) A directory that already carries the ownership marker clears successfully, and the +// marker is present afterwards. +func TestClearAllowsMarkedDirectory(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, managedMarker), "managed") + // Even foreign content is fine once the marker proves we own the directory. + writeFile(t, filepath.Join(dir, "leftover.bin"), "stale") + + d := NewDirOps(dir) + if err := d.Clear(); err != nil { + t.Fatalf("expected Clear to succeed on a marked directory, got %v", err) + } + + if _, err := os.Stat(filepath.Join(dir, "leftover.bin")); !os.IsNotExist(err) { + t.Fatalf("expected old content to be removed, stat err = %v", err) + } + if !markerExists(dir) { + t.Fatalf("expected marker to be re-written after Clear") + } +} + +// (c) An empty directory, and an absent directory, both clear and gain a marker. +func TestClearEmptyAndAbsentGetMarker(t *testing.T) { + t.Run("empty", func(t *testing.T) { + dir := t.TempDir() + d := NewDirOps(dir) + if err := d.Clear(); err != nil { + t.Fatalf("expected Clear to succeed on empty directory, got %v", err) + } + if !markerExists(dir) { + t.Fatalf("expected marker after clearing empty directory") + } + }) + + t.Run("absent", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "does-not-exist-yet") + d := NewDirOps(dir) + if err := d.Clear(); err != nil { + t.Fatalf("expected Clear to succeed on absent directory, got %v", err) + } + if !markerExists(dir) { + t.Fatalf("expected marker after creating absent directory") + } + }) +} + +// Backward compatibility: an unmarked directory whose files all pass the legacy +// extension allowlist still clears (and gains a marker going forward). +func TestClearAllowsLegacyRecognizedContent(t *testing.T) { + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "obj.json"), "{}") + writeFile(t, filepath.Join(dir, "script.ttslua"), "-- lua") + + d := NewDirOps(dir) + if err := d.Clear(); err != nil { + t.Fatalf("expected Clear to succeed on legacy recognized content, got %v", err) + } + if !markerExists(dir) { + t.Fatalf("expected marker after clearing legacy directory") + } +} + +// (d) The path guard refuses the filesystem root and the home directory outright. +func TestPathGuardRefusesDangerousTargets(t *testing.T) { + if err := pathGuard(string(filepath.Separator)); err == nil { + t.Fatalf("expected path guard to refuse filesystem root") + } + + home, err := os.UserHomeDir() + if err != nil || home == "" { + t.Skip("no home directory available on this platform") + } + if err := pathGuard(home); err == nil { + t.Fatalf("expected path guard to refuse home directory %q", home) + } + + // Clear must also refuse the home directory, not just the raw guard. + d := NewDirOps(home) + if err := d.Clear(); err == nil { + t.Fatalf("expected Clear to refuse home directory %q", home) + } +} + +// The path guard refuses suspiciously shallow single-segment paths. +func TestPathGuardRefusesShallowPaths(t *testing.T) { + if err := pathGuard(string(filepath.Separator) + "objects"); err == nil { + t.Fatalf("expected path guard to refuse a single-segment path") + } +}