diff --git a/docs/internal/fileops.md b/docs/internal/fileops.md index 0c8b61d..dae5363 100644 --- a/docs/internal/fileops.md +++ b/docs/internal/fileops.md @@ -21,11 +21,12 @@ no-replace primitive reject promotion before writing a temporary tree. ## Cleanup Contract -`RemoveAllUnderRoot` accepts an explicit root and a proper descendant. It opens -the root and each target ancestor without following symlinks, then removes the -tree through those directory handles. It rejects root deletion and any symlink -encountered in the target path or tree; repeated removal of a missing target is -successful. Command and post-publish policy remains owned by `internal/app`. +`RemoveAllUnderRoot` accepts an explicit root and a proper descendant, opens +that root with `os.OpenRoot`, and removes the normalized root-relative target +with `Root.RemoveAll`. It rejects root, traversal, and out-of-root targets; a +missing target is successful. `RemoveFileUnderRoot` additionally requires an +existing leaf to be regular. Command and post-publish policy remains owned by +`internal/app`. ## Bounded Reads diff --git a/docs/internal/workspace.md b/docs/internal/workspace.md index b309eb3..5872f69 100644 --- a/docs/internal/workspace.md +++ b/docs/internal/workspace.md @@ -14,8 +14,8 @@ path model, and layout creation goes through `EnsureLayoutFor`. Callers should consume those helpers instead of rebuilding relative paths. `internal/pathsafe` validates relative destinations. `internal/fileops` opens -cleanup roots and their descendants through no-follow directory handles before -removing them. +explicit cleanup roots and removes only normalized proper descendants through +the resulting root handle. `internal/fileops` owns the ordinary workspace mode contract. On POSIX, `WorkspaceDirectoryMode` is setgid `02775` and `WorkspaceFileMode` is `0664`. @@ -60,8 +60,7 @@ Automatic post-publish cleanup: key in publish metadata; - consumes the resolved cleanup policy described in [Configuration](../config.md); -- refuses unsafe deletes (root delete, out-of-root delete, and symlinked - ancestors or entries); +- refuses unsafe deletes (root delete, traversal, and out-of-root targets); - retries any recorded incomplete target on later invocations even when no publish work is selected. Missing targets are a successful, idempotent cleanup result only after the completion evidence is saved. diff --git a/docs/operations.md b/docs/operations.md index 9c26f19..e5a90e6 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -555,7 +555,7 @@ Rules: - `clean` deletes work/spool session state; - cache is preserved unless `--clear-cache` is set; - each deletion is confined beneath its configured workspace, spool, or cache - root and refuses symlinked paths; + root; root, traversal, and out-of-root targets are refused; - automatic post-publish cleanup is gated by successful publish commit plus: - `pipeline.spool.delete_audio_after_publish=true` - `pipeline.workspace.cleanup_after_publish=true` diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index bb3572a..13346a6 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -369,7 +369,7 @@ object model. ## Stage 4 — Replace Custom Cleanup Traversal With `os.Root` -**Status: Pending** +**Status: Completed** ### Goal diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 2e55d84..43d3ac5 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -362,8 +362,8 @@ Likely causes: Safe fix: - allow the automatic rerun after verifying the current configuration. Treat - an unsafe path or symlink error as filesystem corruption or tampering and - investigate it rather than replacing files manually. + an unsafe path or bundle symlink error as filesystem corruption or tampering + and investigate it rather than replacing files manually. ## Notarius transitive configuration changed diff --git a/internal/fileops/cleanup.go b/internal/fileops/cleanup.go index 069c9f1..f91de90 100644 --- a/internal/fileops/cleanup.go +++ b/internal/fileops/cleanup.go @@ -5,28 +5,28 @@ import ( "fmt" "os" "path/filepath" - "sort" "strings" ) -// RemoveAllUnderRoot removes target and its contents only after opening root and -// every target ancestor without following symbolic links. target must name a -// proper descendant of root. Symlinks are rejected rather than followed or -// removed so a failed cleanup never silently changes its scope. +// RemoveAllUnderRoot removes target and its contents beneath root. target must +// name a proper descendant of root; removal is confined by the opened root. func RemoveAllUnderRoot(rootPath, target string) error { - root, targetName, err := openConfinedCleanupTarget(rootPath, target) + root, targetName, err := openCleanupRoot(rootPath, target) if err != nil { return err } defer func() { _ = root.Close() }() - return removeConfinedEntry(root, targetName) + if err := root.RemoveAll(targetName); err != nil { + return fmt.Errorf("remove cleanup target %q: %w", targetName, err) + } + return nil } // RemoveFileUnderRoot removes an exact regular-file target below root without -// following symlinked ancestors or the leaf. A missing target is successful; -// directories, symlinks, and other non-regular entries are rejected. +// following the leaf. A missing target is successful; directories, symlinks, +// and other non-regular entries are rejected. func RemoveFileUnderRoot(rootPath, target string) error { - root, targetName, err := openConfinedCleanupTarget(rootPath, target) + root, targetName, err := openCleanupRoot(rootPath, target) if err != nil { return err } @@ -48,7 +48,7 @@ func RemoveFileUnderRoot(rootPath, target string) error { return nil } -func openConfinedCleanupTarget(rootPath, target string) (*os.Root, string, error) { +func openCleanupRoot(rootPath, target string) (*os.Root, string, error) { if strings.TrimSpace(rootPath) == "" { return nil, "", fmt.Errorf("cleanup root is required") } @@ -70,122 +70,13 @@ func openConfinedCleanupTarget(rootPath, target string) (*os.Root, string, error if relative == "." || relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { return nil, "", fmt.Errorf("cleanup target %q must be below root %q", targetAbs, rootAbs) } - parts, err := relativePathParts(relative) + name := filepath.Clean(relative) + if filepath.IsAbs(name) || name == "." || name == ".." || strings.HasPrefix(name, ".."+string(filepath.Separator)) { + return nil, "", fmt.Errorf("cleanup target %q must be below root %q", targetAbs, rootAbs) + } + root, err := os.OpenRoot(rootAbs) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("open cleanup root %q: %w", rootAbs, err) } - root, err := openConfinedDirectory(rootAbs) - if err != nil { - return nil, "", err - } - for _, part := range parts[:len(parts)-1] { - child, err := openConfinedChild(root, part, false, 0) - if err != nil { - _ = root.Close() - return nil, "", err - } - _ = root.Close() - root = child - } - return root, parts[len(parts)-1], nil -} - -func openConfinedDirectory(path string) (*os.Root, error) { - abs, err := filepath.Abs(path) - if err != nil { - return nil, fmt.Errorf("resolve directory: %w", err) - } - parts, err := absolutePathParts(abs) - if err != nil { - return nil, err - } - rootPath := filesystemRoot(abs) - root, err := os.OpenRoot(rootPath) - if err != nil { - return nil, fmt.Errorf("open filesystem root %q: %w", rootPath, err) - } - for _, part := range parts { - child, err := openConfinedChild(root, part, false, 0) - if err != nil { - _ = root.Close() - return nil, err - } - _ = root.Close() - root = child - } - return root, nil -} - -func relativePathParts(path string) ([]string, error) { - parts := strings.FieldsFunc(path, func(r rune) bool { return r == filepath.Separator || r == '/' || r == '\\' }) - if len(parts) == 0 { - return nil, fmt.Errorf("cleanup target is required") - } - for _, part := range parts { - if part == "" || part == "." || part == ".." { - return nil, fmt.Errorf("unsafe cleanup path component %q", part) - } - } - return parts, nil -} - -func removeConfinedEntry(parent *os.Root, name string) error { - info, err := parent.Lstat(name) - if errors.Is(err, os.ErrNotExist) { - return nil - } - if err != nil { - return fmt.Errorf("inspect cleanup entry %q: %w", name, err) - } - if info.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("refusing to delete symlink path %q", name) - } - if !info.IsDir() { - if err := parent.Remove(name); err != nil { - return fmt.Errorf("remove cleanup entry %q: %w", name, err) - } - return nil - } - - child, err := openConfinedChild(parent, name, false, 0) - if err != nil { - return err - } - err = removeConfinedChildren(child) - closeErr := child.Close() - if err != nil { - if closeErr != nil { - return errors.Join(err, closeErr) - } - return err - } - if closeErr != nil { - return fmt.Errorf("close cleanup directory %q: %w", name, closeErr) - } - if err := parent.Remove(name); err != nil { - return fmt.Errorf("remove cleanup directory %q: %w", name, err) - } - return nil -} - -func removeConfinedChildren(root *os.Root) error { - directory, err := root.Open(".") - if err != nil { - return fmt.Errorf("open cleanup directory: %w", err) - } - entries, readErr := directory.ReadDir(-1) - closeErr := directory.Close() - if readErr != nil { - return fmt.Errorf("read cleanup directory: %w", readErr) - } - if closeErr != nil { - return fmt.Errorf("close cleanup directory: %w", closeErr) - } - sort.Slice(entries, func(i, j int) bool { return entries[i].Name() < entries[j].Name() }) - for _, entry := range entries { - if err := removeConfinedEntry(root, entry.Name()); err != nil { - return err - } - } - return nil + return root, name, nil } diff --git a/internal/fileops/cleanup_test.go b/internal/fileops/cleanup_test.go index 4b43b3c..44740a0 100644 --- a/internal/fileops/cleanup_test.go +++ b/internal/fileops/cleanup_test.go @@ -38,6 +38,9 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) { if err := RemoveAllUnderRoot(root, filepath.Join(outside, "target")); err == nil { t.Fatal("RemoveAllUnderRoot(outside) error = nil, want outside-root rejection") } + if err := RemoveAllUnderRoot(root, filepath.Join(root, "runs", "..")); err == nil { + t.Fatal("RemoveAllUnderRoot(root relative traversal) error = nil, want root rejection") + } if err := os.Symlink(outside, filepath.Join(root, "runs")); err == nil { t.Fatal("Symlink() error = nil, want collision because runs already exists") } @@ -48,7 +51,7 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) { t.Fatalf("Symlink() error = %v", err) } if err := RemoveAllUnderRoot(root, target); err == nil { - t.Fatal("RemoveAllUnderRoot(symlinked ancestor) error = nil, want rejection") + t.Fatal("RemoveAllUnderRoot(symlinked ancestor) error = nil, want root confinement error") } data, err := os.ReadFile(sentinel) if err != nil { @@ -59,7 +62,7 @@ func TestRemoveAllUnderRootRejectsUnsafeTargets(t *testing.T) { } } -func TestRemoveAllUnderRootRejectsSymlinkInTree(t *testing.T) { +func TestRemoveAllUnderRootRemovesRootContainedSymlink(t *testing.T) { root := t.TempDir() outside := t.TempDir() target := filepath.Join(root, "runs", "run-1") @@ -72,12 +75,15 @@ func TestRemoveAllUnderRootRejectsSymlinkInTree(t *testing.T) { t.Fatalf("Symlink() error = %v", err) } - if err := RemoveAllUnderRoot(root, target); err == nil { - t.Fatal("RemoveAllUnderRoot() error = nil, want symlink rejection") + if err := RemoveAllUnderRoot(root, target); err != nil { + t.Fatalf("RemoveAllUnderRoot() error = %v", err) } if _, err := os.Stat(sentinel); err != nil { t.Fatalf("outside sentinel was changed: %v", err) } + if _, err := os.Lstat(target); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("Lstat(%q) error = %v, want not exist", target, err) + } } func TestRemoveFileUnderRootRemovesOnlyRegularFile(t *testing.T) { diff --git a/internal/fileops/directory.go b/internal/fileops/directory.go index 5747f63..3bb02e1 100644 --- a/internal/fileops/directory.go +++ b/internal/fileops/directory.go @@ -66,7 +66,7 @@ func promoteDirectoryConfined(src, dst string, parent *os.Root, destinationName closeErr = temporary.Close() } if removeTemporary { - removeErr := removeConfinedEntry(parent, temporaryName) + removeErr := parent.RemoveAll(temporaryName) if removeErr != nil { resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary destination directory: %w", removeErr)) }