From 88efebb940c66493259f417c69f7b2ac00da8b14 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Mon, 21 Sep 2026 10:16:37 +0200 Subject: [PATCH 1/4] fix(tools): use atomic temp-and-replace writes for write_file and edit_file (fixes #921) Unify compare-and-replace preimage identity checks with atomic temp-and-replace publication via fsutil.WriteFileAtomic in commitFileContents. Route write_file and edit_file through isolated staging format-on-write prior to publication, eliminating destination truncation and dead RestoreFailed error paths. Preserve ACP structured diff previews and Windows/Darwin DACL/ACL inheritance. --- internal/fsutil/getattrlist_darwin.go | 41 ++ internal/fsutil/getattrlist_darwin.s | 18 + internal/fsutil/private_temp.go | 56 ++ internal/fsutil/private_temp_darwin.go | 61 ++ internal/fsutil/private_temp_other.go | 15 + internal/fsutil/private_temp_unix.go | 12 + internal/fsutil/private_temp_windows.go | 56 ++ internal/fsutil/rename.go | 163 ++++ internal/fsutil/rename_acl_darwin.go | 78 ++ internal/fsutil/rename_acl_darwin_test.go | 181 +++++ internal/fsutil/rename_acl_linux_test.go | 223 ++++++ internal/fsutil/rename_acl_other.go | 15 + internal/fsutil/rename_owner_unix.go | 25 + internal/fsutil/rename_owner_unix_test.go | 41 ++ internal/fsutil/rename_owner_windows.go | 13 + internal/fsutil/rename_special_unix_test.go | 42 ++ internal/fsutil/rename_staging_other.go | 14 + internal/fsutil/rename_staging_windows.go | 96 +++ .../fsutil/rename_staging_windows_test.go | 180 +++++ internal/fsutil/rename_test.go | 186 ++++- internal/fsutil/rename_umask_unix_test.go | 57 ++ internal/fsutil/rename_xattr_notfound_bsd.go | 13 + .../fsutil/rename_xattr_notfound_freebsd.go | 13 + .../fsutil/rename_xattr_notfound_linux.go | 13 + internal/fsutil/rename_xattr_stub.go | 9 + internal/fsutil/rename_xattr_unix.go | 115 +++ internal/fsutil/rename_xattr_unix_test.go | 35 + internal/tools/edit_file.go | 30 +- internal/tools/file_commit.go | 101 +-- internal/tools/file_commit_atomic_test.go | 178 +++++ internal/tools/format_on_write.go | 202 +++-- internal/tools/format_on_write_test.go | 696 +++++++++--------- .../tools/format_on_write_timeout_test.go | 95 ++- .../tools/format_on_write_windows_test.go | 125 ++++ internal/tools/write_file.go | 41 +- 35 files changed, 2662 insertions(+), 577 deletions(-) create mode 100644 internal/fsutil/getattrlist_darwin.go create mode 100644 internal/fsutil/getattrlist_darwin.s create mode 100644 internal/fsutil/private_temp.go create mode 100644 internal/fsutil/private_temp_darwin.go create mode 100644 internal/fsutil/private_temp_other.go create mode 100644 internal/fsutil/private_temp_unix.go create mode 100644 internal/fsutil/private_temp_windows.go create mode 100644 internal/fsutil/rename_acl_darwin.go create mode 100644 internal/fsutil/rename_acl_darwin_test.go create mode 100644 internal/fsutil/rename_acl_linux_test.go create mode 100644 internal/fsutil/rename_acl_other.go create mode 100644 internal/fsutil/rename_owner_unix.go create mode 100644 internal/fsutil/rename_owner_unix_test.go create mode 100644 internal/fsutil/rename_owner_windows.go create mode 100644 internal/fsutil/rename_special_unix_test.go create mode 100644 internal/fsutil/rename_staging_other.go create mode 100644 internal/fsutil/rename_staging_windows.go create mode 100644 internal/fsutil/rename_staging_windows_test.go create mode 100644 internal/fsutil/rename_umask_unix_test.go create mode 100644 internal/fsutil/rename_xattr_notfound_bsd.go create mode 100644 internal/fsutil/rename_xattr_notfound_freebsd.go create mode 100644 internal/fsutil/rename_xattr_notfound_linux.go create mode 100644 internal/fsutil/rename_xattr_stub.go create mode 100644 internal/fsutil/rename_xattr_unix.go create mode 100644 internal/fsutil/rename_xattr_unix_test.go create mode 100644 internal/tools/file_commit_atomic_test.go create mode 100644 internal/tools/format_on_write_windows_test.go diff --git a/internal/fsutil/getattrlist_darwin.go b/internal/fsutil/getattrlist_darwin.go new file mode 100644 index 000000000..d5a13ff79 --- /dev/null +++ b/internal/fsutil/getattrlist_darwin.go @@ -0,0 +1,41 @@ +//go:build darwin + +package fsutil + +import ( + "syscall" + "unsafe" + + "golang.org/x/sys/unix" +) + +var getattrlistZero byte + +var libc_getattrlist_trampoline_addr uintptr + +//go:linkname syscall_syscall6 syscall.syscall6 +func syscall_syscall6(fn, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err syscall.Errno) + +//go:cgo_import_dynamic libc_getattrlist getattrlist "/usr/lib/libSystem.B.dylib" + +func getattrlist(path string, attrList *unix.Attrlist, attrBuf []byte, options uint32) error { + p, err := unix.BytePtrFromString(path) + if err != nil { + return err + } + bufPtr := unsafe.Pointer(&getattrlistZero) + if len(attrBuf) > 0 { + bufPtr = unsafe.Pointer(&attrBuf[0]) + } + _, _, e := syscall_syscall6(libc_getattrlist_trampoline_addr, + uintptr(unsafe.Pointer(p)), + uintptr(unsafe.Pointer(attrList)), + uintptr(bufPtr), + uintptr(len(attrBuf)), + uintptr(options), + 0) + if e != 0 { + return e + } + return nil +} diff --git a/internal/fsutil/getattrlist_darwin.s b/internal/fsutil/getattrlist_darwin.s new file mode 100644 index 000000000..eae4d261a --- /dev/null +++ b/internal/fsutil/getattrlist_darwin.s @@ -0,0 +1,18 @@ +//go:build darwin + +#include "textflag.h" + +TEXT libc_getattrlist_trampoline<>(SB),NOSPLIT,$0-0 + JMP libc_getattrlist(SB) +GLOBL ·libc_getattrlist_trampoline_addr(SB), RODATA, $8 +DATA ·libc_getattrlist_trampoline_addr(SB)/8, $libc_getattrlist_trampoline<>(SB) + +TEXT libc_open_extended_trampoline<>(SB),NOSPLIT,$0-0 + JMP libc_open_extended(SB) +GLOBL ·libc_open_extended_trampoline_addr(SB), RODATA, $8 +DATA ·libc_open_extended_trampoline_addr(SB)/8, $libc_open_extended_trampoline<>(SB) + +TEXT libc_mkdir_extended_trampoline<>(SB),NOSPLIT,$0-0 + JMP libc_mkdir_extended(SB) +GLOBL ·libc_mkdir_extended_trampoline_addr(SB), RODATA, $8 +DATA ·libc_mkdir_extended_trampoline_addr(SB)/8, $libc_mkdir_extended_trampoline<>(SB) diff --git a/internal/fsutil/private_temp.go b/internal/fsutil/private_temp.go new file mode 100644 index 000000000..191e73c61 --- /dev/null +++ b/internal/fsutil/private_temp.go @@ -0,0 +1,56 @@ +package fsutil + +import ( + "crypto/rand" + "errors" + "os" + "path/filepath" + "strings" +) + +// privateCreationObserver observes the object immediately after the creation +// syscall, before any metadata changes or content writes. Tests are serial. +var privateCreationObserver func(string) + +// CreatePrivateTemp creates an owner-only staging file, suppressing effective +// inherited grants in the creation syscall. The caller must close and remove it. +func CreatePrivateTemp(dir, pattern string) (*os.File, error) { + var file *os.File + _, err := createPrivateTemp(dir, pattern, func(path string) error { + var err error + file, err = createPrivateFile(path) + return err + }) + return file, err +} + +// CreatePrivateTempDir isolates formatter rewrites and auxiliary files as well +// as the initial copy. The caller must remove the directory after the child exits. +func CreatePrivateTempDir(dir, pattern string) (string, error) { + return createPrivateTemp(dir, pattern, createPrivateDir) +} + +func createPrivateTemp(dir, pattern string, create func(string) error) (string, error) { + if strings.ContainsAny(pattern, `/\`) { + return "", errors.New("fsutil: invalid temporary pattern") + } + prefix, suffix := pattern, "" + if index := strings.LastIndexByte(pattern, '*'); index >= 0 { + prefix, suffix = pattern[:index], pattern[index+1:] + } + for range 10000 { + name := filepath.Join(dir, prefix+rand.Text()+suffix) + err := create(name) + if errors.Is(err, os.ErrExist) { + continue + } + if err != nil { + return "", err + } + if privateCreationObserver != nil { + privateCreationObserver(name) + } + return name, nil + } + return "", errors.New("fsutil: temporary name collisions") +} diff --git a/internal/fsutil/private_temp_darwin.go b/internal/fsutil/private_temp_darwin.go new file mode 100644 index 000000000..f2e796e2f --- /dev/null +++ b/internal/fsutil/private_temp_darwin.go @@ -0,0 +1,61 @@ +//go:build darwin + +package fsutil + +import ( + "encoding/binary" + "os" + "runtime" + "unsafe" + + "golang.org/x/sys/unix" +) + +var libc_open_extended_trampoline_addr uintptr +var libc_mkdir_extended_trampoline_addr uintptr + +// These are the libSystem primitives underlying openx_np/mkdirx_np since 10.4. +//go:cgo_import_dynamic libc_open_extended __open_extended "/usr/lib/libSystem.B.dylib" +//go:cgo_import_dynamic libc_mkdir_extended __mkdir_extended "/usr/lib/libSystem.B.dylib" + +// kauth_filesec: magic, owner/group GUIDs, ACL entry count and ACL flags. +// An empty ACL with NO_INHERIT prevents the parent from granting access at birth. +func privateFilesec() []byte { + blob := make([]byte, 44) + binary.LittleEndian.PutUint32(blob, 0x012cc16d) + binary.LittleEndian.PutUint32(blob[40:], 1<<17) + return blob +} +func createPrivateFile(path string) (*os.File, error) { + name, err := unix.BytePtrFromString(path) + if err != nil { + return nil, err + } + security := privateFilesec() + const noID = uintptr(0xffffff9b) // KAUTH_UID_NONE / KAUTH_GID_NONE + fd, _, errno := syscall_syscall6(libc_open_extended_trampoline_addr, + uintptr(unsafe.Pointer(name)), unix.O_RDWR|unix.O_CREAT|unix.O_EXCL|unix.O_CLOEXEC, + noID, noID, 0o600, uintptr(unsafe.Pointer(&security[0]))) + runtime.KeepAlive(name) + runtime.KeepAlive(security) + if errno != 0 { + return nil, &os.PathError{Op: "create private staging", Path: path, Err: errno} + } + return os.NewFile(fd, path), nil +} +func createPrivateDir(path string) error { + name, err := unix.BytePtrFromString(path) + if err != nil { + return err + } + security := privateFilesec() + const noID = uintptr(0xffffff9b) + _, _, errno := syscall_syscall6(libc_mkdir_extended_trampoline_addr, + uintptr(unsafe.Pointer(name)), noID, noID, 0o700, uintptr(unsafe.Pointer(&security[0])), 0) + runtime.KeepAlive(name) + runtime.KeepAlive(security) + if errno != 0 { + return &os.PathError{Op: "mkdir private staging", Path: path, Err: errno} + } + return nil +} diff --git a/internal/fsutil/private_temp_other.go b/internal/fsutil/private_temp_other.go new file mode 100644 index 000000000..348a7bd23 --- /dev/null +++ b/internal/fsutil/private_temp_other.go @@ -0,0 +1,15 @@ +//go:build !linux && !darwin && !windows + +package fsutil + +import ( + "errors" + "os" +) + +func createPrivateFile(string) (*os.File, error) { + return nil, errors.New("fsutil: private staging creation is unsupported on this platform") +} +func createPrivateDir(string) error { + return errors.New("fsutil: private staging creation is unsupported on this platform") +} diff --git a/internal/fsutil/private_temp_unix.go b/internal/fsutil/private_temp_unix.go new file mode 100644 index 000000000..19ec74b61 --- /dev/null +++ b/internal/fsutil/private_temp_unix.go @@ -0,0 +1,12 @@ +//go:build linux + +package fsutil + +import "os" + +// POSIX ACL inheritance intersects the ACL mask with the requested group bits. +// 0600/0700 therefore disable every inherited named-user and group grant. +func createPrivateFile(path string) (*os.File, error) { + return os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) +} +func createPrivateDir(path string) error { return os.Mkdir(path, 0o700) } diff --git a/internal/fsutil/private_temp_windows.go b/internal/fsutil/private_temp_windows.go new file mode 100644 index 000000000..04a2d146f --- /dev/null +++ b/internal/fsutil/private_temp_windows.go @@ -0,0 +1,56 @@ +//go:build windows + +package fsutil + +import ( + "fmt" + "os" + "unsafe" + + "golang.org/x/sys/windows" +) + +// An explicit protected current-user DACL is installed by CreateFile/CreateDirectory, +// not by a second call after an inheritable object has become visible. +func privateSecurityAttributes() (*windows.SecurityAttributes, error) { + token := windows.GetCurrentProcessToken() + user, err := token.GetTokenUser() + if err != nil { + return nil, err + } + descriptor, err := windows.SecurityDescriptorFromString(fmt.Sprintf("D:P(A;OICI;FA;;;%s)", user.User.Sid.String())) + if err != nil { + return nil, err + } + return &windows.SecurityAttributes{Length: uint32(unsafe.Sizeof(windows.SecurityAttributes{})), SecurityDescriptor: descriptor}, nil +} +func createPrivateFile(path string) (*os.File, error) { + name, err := windows.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + security, err := privateSecurityAttributes() + if err != nil { + return nil, err + } + handle, err := windows.CreateFile(name, windows.GENERIC_READ|windows.GENERIC_WRITE|windows.READ_CONTROL|windows.WRITE_DAC, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, security, windows.CREATE_NEW, windows.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + return nil, &os.PathError{Op: "create private staging", Path: path, Err: err} + } + return os.NewFile(uintptr(handle), path), nil +} +func createPrivateDir(path string) error { + name, err := windows.UTF16PtrFromString(path) + if err != nil { + return err + } + security, err := privateSecurityAttributes() + if err != nil { + return err + } + if err := windows.CreateDirectory(name, security); err != nil { + return &os.PathError{Op: "mkdir private staging", Path: path, Err: err} + } + return nil +} diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 4f74e8544..f1a7e7aea 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -2,14 +2,177 @@ package fsutil import ( + "crypto/rand" "errors" "fmt" "os" + "path/filepath" "runtime" "syscall" "time" ) +// ErrNonRegularDestination is returned when WriteFileAtomic would replace a +// FIFO, device, socket, directory, or other special file with a regular file. +var ErrNonRegularDestination = errors.New("fsutil: destination exists and is not a regular file") + +// stagingModeObserver, when non-nil, receives the mode of the freshly created +// staging file before any metadata is copied onto it. Tests use it to assert +// that a replacement is staged no broader than its destination. +var stagingModeObserver func(os.FileMode) + +// stagingProtectionObserver, when non-nil, receives the path of the staging +// file after protectStaging has copied the destination's authorization metadata +// onto the completed content, before publication. Tests use it to +// assert that a replacement is staged no broader than its destination on +// platforms (Windows) where the mode bits do not carry that answer. +var stagingProtectionObserver func(stagingPath string) + +// WriteFileAtomic writes data to a temporary file in the same directory as filename, +// flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. +// For new files, it honors the process umask by creating the temporary file with +// os.OpenFile(..., perm). For existing regular files, it requires write access to +// the current destination and copies that destination's authorization metadata onto +// the replacement (mode bits including setuid/setgid/sticky, owner, and xattrs such +// as POSIX ACLs and capabilities). If that metadata cannot be preserved, the call +// fails and leaves the destination unchanged. Non-regular, non-symlink destinations +// are refused before staging so a FIFO, device, or socket is not replaced. +// +// Platform differences on symlink destinations: On Unix (Linux/macOS), replacing a +// symlink destination replaces the symlink itself with the new regular file. On Windows, +// ReplaceFileW refuses symlink destinations outright and returns an error. +// Hard links to destination files are broken by design (temp-and-rename publishes a new inode). +func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { + return writeFileAtomic(filename, data, perm, nil) +} + +// The replacement dependency is local to the call, so failure tests need no global hook. +func writeFileAtomic(filename string, data []byte, perm os.FileMode, replace func(string, string) error) error { + dir := filepath.Dir(filename) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + + var existingMode *os.FileMode + info, err := os.Lstat(filename) + switch { + case err == nil: + mode := info.Mode() + switch { + case mode.IsRegular(): + if err := ensureWritable(filename); err != nil { + return err + } + existingMode = &mode + case mode&os.ModeSymlink != 0: + // Documented platform-specific replacement of the symlink itself. + default: + return fmt.Errorf("%w: %s", ErrNonRegularDestination, filename) + } + case !os.IsNotExist(err): + return err + } + + var tmpFile *os.File + if existingMode != nil { + tmpFile, err = CreatePrivateTemp(dir, ".zero-tmp-*") + } else { + tmpFile, err = createTempFile(dir, perm) + } + if err != nil { + return err + } + if stagingModeObserver != nil { + if info, statErr := tmpFile.Stat(); statErr == nil { + stagingModeObserver(info.Mode()) + } + } + tmpName := tmpFile.Name() + closed := false + defer func() { + if !closed { + _ = tmpFile.Close() + } + _ = os.Remove(tmpName) + }() + + if _, err := tmpFile.Write(data); err != nil { + return err + } + if existingMode != nil { + if err := preserveOwner(tmpFile, info); err != nil { + return err + } + // Remove/copy inherited authorization before broadening mode bits. + if err := preserveNativeACL(tmpFile, filename); err != nil { + return err + } + if err := preserveXattrs(tmpFile, filename); err != nil { + return err + } + if err := protectStaging(tmpFile, filename); err != nil { + return err + } + if err := tmpFile.Chmod(*existingMode); err != nil { + return err + } + if stagingProtectionObserver != nil { + stagingProtectionObserver(tmpName) + } + } + if err := tmpFile.Sync(); err != nil { + return err + } + closed = true + if err := tmpFile.Close(); err != nil { + return err + } + + replaceErr := ReplaceWithRetry(tmpName, filename, replace) + if replaceErr == nil || isCommittedReplacement(replaceErr) { + syncDir(dir) + } + return replaceErr +} + +func ensureWritable(path string) error { + f, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + return err + } + return f.Close() +} + +func createTempFile(dir string, perm os.FileMode) (*os.File, error) { + for i := 0; i < 10000; i++ { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return nil, err + } + name := filepath.Join(dir, fmt.Sprintf(".zero-tmp-%x", b)) + f, err := os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm) + if errors.Is(err, os.ErrExist) { + continue + } + return f, err + } + return nil, errors.New("fsutil: failed to create temporary file after repeated attempts") +} + +func isCommittedReplacement(err error) bool { + var committed *CommittedReplacementCleanupError + return errors.As(err, &committed) +} + +func syncDir(dir string) { + d, err := os.Open(dir) + if err != nil { + return + } + defer d.Close() + _ = d.Sync() +} + // CommittedReplacementCleanupError reports that a replacement was committed, // but the old destination retained at BackupPath could not be removed. Callers // must treat the replacement itself as successful and surface the cleanup diff --git a/internal/fsutil/rename_acl_darwin.go b/internal/fsutil/rename_acl_darwin.go new file mode 100644 index 000000000..ac1e2d414 --- /dev/null +++ b/internal/fsutil/rename_acl_darwin.go @@ -0,0 +1,78 @@ +//go:build darwin + +package fsutil + +import ( + "encoding/binary" + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +// preserveNativeACL copies the Darwin kauth/FILESEC ACL using getattrlist and +// setattrlist ATTR_CMN_EXTENDED_SECURITY. Listxattr/Getxattr/Fsetxattr cannot +// see com.apple.system.Security (protected namespace); that path is not used. +func preserveNativeACL(f *os.File, srcPath string) error { + acl, err := readNativeACL(srcPath) + if err != nil { + return err + } + if err := applyNativeACL(f.Name(), acl); err != nil { + return fmt.Errorf("fsutil: preserving native ACL on replacement for %s: %w", srcPath, err) + } + return nil +} + +func extendedSecurityAttrlist() unix.Attrlist { + return unix.Attrlist{ + Bitmapcount: unix.ATTR_BIT_MAP_COUNT, + Commonattr: unix.ATTR_CMN_EXTENDED_SECURITY, + } +} + +func readNativeACL(path string) ([]byte, error) { + al := extendedSecurityAttrlist() + buf := make([]byte, 4096) + if err := getattrlist(path, &al, buf, 0); err != nil { + if isXattrNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("fsutil: reading native ACL of %s: %w", path, err) + } + total := binary.LittleEndian.Uint32(buf[:4]) + if total < 12 { + return nil, fmt.Errorf("fsutil: invalid native ACL response for %s", path) + } + off := int32(binary.LittleEndian.Uint32(buf[4:8])) + length := binary.LittleEndian.Uint32(buf[8:12]) + if length == 0 { + return nil, nil + } + start := 4 + int(off) + end := start + int(length) + if start < 12 || end > int(total) || end > len(buf) { + return nil, fmt.Errorf("fsutil: truncated native ACL on %s", path) + } + out := make([]byte, length) + copy(out, buf[start:end]) + return out, nil +} + +func applyNativeACL(path string, blob []byte) error { + // A nil ACL is a verified absence, not permission to retain inherited ACEs. + if blob == nil { + blob = make([]byte, 44) + binary.LittleEndian.PutUint32(blob, 0x012cc16d) // KAUTH_FILESEC_MAGIC + binary.LittleEndian.PutUint32(blob[36:], 0xffffffff) // KAUTH_FILESEC_NOACL + } + al := extendedSecurityAttrlist() + buf := make([]byte, 8+len(blob)) + binary.LittleEndian.PutUint32(buf[0:4], 8) + binary.LittleEndian.PutUint32(buf[4:8], uint32(len(blob))) + copy(buf[8:], blob) + if err := unix.Setattrlist(path, &al, buf, 0); err != nil { + return err + } + return nil +} diff --git a/internal/fsutil/rename_acl_darwin_test.go b/internal/fsutil/rename_acl_darwin_test.go new file mode 100644 index 000000000..c4f191670 --- /dev/null +++ b/internal/fsutil/rename_acl_darwin_test.go @@ -0,0 +1,181 @@ +//go:build darwin + +package fsutil + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestWriteFileAtomicPreservesOrRefusesNativeACL pins the fail-closed contract +// for a Darwin native (kauth/FILESEC) ACL: replacing the destination either +// carries the ACL over to the new inode, or the replacement is refused and the +// original file is left byte-for-byte intact. What must never happen is a +// successful replacement that silently drops the ACL. +func TestWriteFileAtomicPreservesOrRefusesNativeACL(t *testing.T) { + if _, err := exec.LookPath("chmod"); err != nil { + t.Skip("chmod not on PATH") + } + if _, err := exec.LookPath("ls"); err != nil { + t.Skip("ls not on PATH") + } + + dir := t.TempDir() + target := filepath.Join(dir, "restricted.txt") + original := "old" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + set := exec.Command("chmod", "+a", "user:nobody deny read", target) + if out, err := set.CombinedOutput(); err != nil { + t.Skipf("chmod +a failed (ACLs unavailable on this filesystem): %v\n%s", err, out) + } + + acl, err := readNativeACL(target) + if err != nil { + t.Skipf("cannot read the native ACL that chmod +a just set: %v", err) + } + if len(acl) == 0 { + t.Skip("chmod +a reported success but no native ACL is present") + } + before, err := exec.Command("ls", "-le", target).CombinedOutput() + if err != nil { + t.Fatalf("ls -le before: %v\n%s", err, before) + } + if !strings.Contains(string(before), "deny") { + t.Skipf("named deny entry did not stick; ls -le:\n%s", before) + } + + replaceErr := WriteFileAtomic(target, []byte("new"), 0o600) + if replaceErr != nil { + // Refusal path: the error is acceptable only if the destination was + // not mutated on the way out. + got, rerr := os.ReadFile(target) + if rerr != nil { + t.Fatalf("WriteFileAtomic failed (%v) and the original is unreadable: %v", replaceErr, rerr) + } + if string(got) != original { + t.Fatalf("WriteFileAtomic failed (%v) but mutated the destination to %q", replaceErr, got) + } + return + } + + // Preservation path: the replacement succeeded, so the deny entry must + // still be attached to the new inode. + after, err := readNativeACL(target) + if err != nil { + t.Fatalf("WriteFileAtomic succeeded but the native ACL became unreadable: %v", err) + } + if len(after) == 0 { + t.Fatalf("WriteFileAtomic replaced the destination and lost the native ACL") + } + afterListing, err := exec.Command("ls", "-le", target).CombinedOutput() + if err != nil { + t.Fatalf("ls -le after: %v\n%s", err, afterListing) + } + if !strings.Contains(string(afterListing), "deny") { + t.Fatalf("native ACL deny entry was lost after replacement\nbefore:\n%s\nafter:\n%s", before, afterListing) + } +} + +func TestPreserveNativeACLRemovesInheritedACLWhenSourceHasNone(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "restricted") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("chmod", "-N", target).CombinedOutput(); err != nil { + t.Fatalf("clear ACL: %s: %v", out, err) + } + if out, err := exec.Command("chmod", "+a", "user:nobody allow read,file_inherit,directory_inherit", dir).CombinedOutput(); err != nil { + t.Fatalf("parent ACL: %s: %v", out, err) + } + // Exercise preservation directly so private creation cannot mask a nil/no-op bug. + staging, err := os.CreateTemp(dir, "inherited-*") + if err != nil { + t.Fatal(err) + } + defer staging.Close() + before, err := exec.Command("ls", "-le", staging.Name()).CombinedOutput() + if err != nil || !strings.Contains(string(before), "nobody allow read") { + t.Fatalf("inherited grant missing: %s, %v", before, err) + } + if err := preserveNativeACL(staging, target); err != nil { + t.Fatal(err) + } + after, err := exec.Command("ls", "-le", staging.Name()).CombinedOutput() + if err != nil || strings.Contains(string(after), "nobody") { + t.Fatalf("absence of source ACL was not preserved: %s, %v", after, err) + } + if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { + t.Fatal(err) + } + after, err = exec.Command("ls", "-le", target).CombinedOutput() + if err != nil || strings.Contains(string(after), "nobody") { + t.Fatalf("replacement inherited a grant: %s, %v", after, err) + } + fresh := filepath.Join(dir, "fresh") + if err := WriteFileAtomic(fresh, []byte("public"), 0o644); err != nil { + t.Fatal(err) + } + after, err = exec.Command("ls", "-le", fresh).CombinedOutput() + if err != nil || !strings.Contains(string(after), "nobody allow read") { + t.Fatalf("new file lost inheritance: %s, %v", after, err) + } +} + +func TestPrivateStagingDarwinCreationSuppressesInheritance(t *testing.T) { + dir := t.TempDir() + if out, err := exec.Command("chmod", "+a", "user:nobody allow read,file_inherit,directory_inherit", dir).CombinedOutput(); err != nil { + t.Fatalf("parent ACL: %s: %v", out, err) + } + count := 0 + previous := privateCreationObserver + privateCreationObserver = func(path string) { + count++ + listing, err := exec.Command("ls", "-lde", path).CombinedOutput() + if err != nil || strings.Contains(string(listing), "nobody") { + t.Fatalf("initial staging inherited access: %s, %v", listing, err) + } + info, err := os.Stat(path) + if err != nil || info.Mode().Perm()&0o077 != 0 { + t.Fatalf("initial staging mode: %v, %v", info, err) + } + } + defer func() { privateCreationObserver = previous }() + file, err := CreatePrivateTemp(dir, ".zero-tmp-*") + if err != nil { + t.Fatal(err) + } + if err := file.Close(); err != nil { + t.Fatal(err) + } + if err := os.Remove(file.Name()); err != nil { + t.Fatal(err) + } + stagingDir, err := CreatePrivateTempDir(dir, ".zero-fmt-*") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(stagingDir); err != nil { + t.Fatal(err) + } + if count != 2 { + t.Fatalf("creation observations: %d", count) + } +} + +func TestPreserveNativeACLReadFailureDoesNotMeanAbsence(t *testing.T) { + file, err := os.CreateTemp(t.TempDir(), "staging") + if err != nil { + t.Fatal(err) + } + defer file.Close() + if err := preserveNativeACL(file, filepath.Join(t.TempDir(), "missing")); err == nil { + t.Fatal("unreadable source ACL treated as absent") + } +} diff --git a/internal/fsutil/rename_acl_linux_test.go b/internal/fsutil/rename_acl_linux_test.go new file mode 100644 index 000000000..00f8e7dba --- /dev/null +++ b/internal/fsutil/rename_acl_linux_test.go @@ -0,0 +1,223 @@ +//go:build linux + +package fsutil + +import ( + "bytes" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestWriteFileAtomicPreservesRestrictivePOSIXACL(t *testing.T) { + if _, err := exec.LookPath("setfacl"); err != nil { + t.Skip("setfacl not on PATH; POSIX ACL preservation is not exercised on this host") + } + if _, err := exec.LookPath("getfacl"); err != nil { + t.Skip("getfacl not on PATH; POSIX ACL preservation is not exercised on this host") + } + + dir := t.TempDir() + target := filepath.Join(dir, "restricted.txt") + if err := os.WriteFile(target, []byte("old"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + set := exec.Command("setfacl", "-m", "u:65534:---", target) + if out, err := set.CombinedOutput(); err != nil { + t.Skipf("setfacl failed (filesystem may lack ACL support): %v\n%s", err, out) + } + + before, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl before: %v\n%s", err, before) + } + if !namedUserACLDenied(before) { + t.Skipf("named-user ACL did not stick; getfacl:\n%s", before) + } + + if err := WriteFileAtomic(target, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } + + after, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl after: %v\n%s", err, after) + } + if !namedUserACLDenied(after) { + t.Fatalf("restrictive named-user ACL was lost after replacement\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +func namedUserACLDenied(listing []byte) bool { + return bytes.Contains(listing, []byte("user:65534:---")) || + bytes.Contains(listing, []byte("user:nobody:---")) || + bytes.Contains(listing, []byte("user:nfsnobody:---")) +} + +func namedUserACLPresent(listing []byte) bool { + return bytes.Contains(listing, []byte("user:65534:")) || + bytes.Contains(listing, []byte("user:nobody:")) || + bytes.Contains(listing, []byte("user:nfsnobody:")) +} + +func TestWriteFileAtomicDropsInheritedAccessACL(t *testing.T) { + if _, err := exec.LookPath("setfacl"); err != nil { + t.Skip("setfacl not on PATH; POSIX ACL inheritance is not exercised on this host") + } + if _, err := exec.LookPath("getfacl"); err != nil { + t.Skip("getfacl not on PATH; POSIX ACL inheritance is not exercised on this host") + } + + base := t.TempDir() + dir := filepath.Join(base, "shared") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("setfacl", "-d", "-m", "u:65534:r--", dir).CombinedOutput(); err != nil { + t.Skipf("setfacl default ACL failed (filesystem may lack ACL support): %v\n%s", err, out) + } + + target := filepath.Join(dir, "plain.txt") + if err := os.WriteFile(target, []byte("old"), 0o640); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("setfacl", "-b", target).CombinedOutput(); err != nil { + t.Skipf("setfacl -b failed: %v\n%s", err, out) + } + before, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl before: %v\n%s", err, before) + } + if namedUserACLPresent(before) { + t.Skipf("source still has a named-user ACL; cannot assert inheritance removal\ngetfacl:\n%s", before) + } + + if err := WriteFileAtomic(target, []byte("new"), 0o640); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + after, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl after: %v\n%s", err, after) + } + if namedUserACLPresent(after) { + t.Fatalf("access ACL inherited from the directory default survived the replacement\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +// Observe the creation boundary, not only the metadata-ready or publication state. +func TestPrivateStagingCreationMasksInheritedPOSIXGrants(t *testing.T) { + for _, commandName := range []string{"setfacl", "getfacl"} { + if _, err := exec.LookPath(commandName); err != nil { + t.Skip(commandName + " not installed") + } + } + for _, denied := range []bool{false, true} { + t.Run(map[bool]string{false: "source-without-acl", true: "source-named-deny"}[denied], func(t *testing.T) { + dir := t.TempDir() + if out, err := exec.Command("setfacl", "-d", "-m", "u:65534:r-x", dir).CombinedOutput(); err != nil { + t.Fatalf("default ACL: %s: %v", out, err) + } + target := filepath.Join(dir, "target") + mode := os.FileMode(0o640) + if denied { + mode = 0o644 + } + if err := os.WriteFile(target, []byte("old"), mode); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("setfacl", "-b", target).CombinedOutput(); err != nil { + t.Fatalf("clear ACL: %s: %v", out, err) + } + if err := os.Chmod(target, mode); err != nil { + t.Fatal(err) + } + if denied { + if out, err := exec.Command("setfacl", "-m", "u:65534:---", target).CombinedOutput(); err != nil { + t.Fatalf("deny ACL: %s: %v", out, err) + } + } + info, err := os.Stat(target) + if err != nil || info.Mode().Perm() != mode { + t.Fatalf("fixture destination mode: %v, %v, want %04o", info, err, mode) + } + count := 0 + previous := privateCreationObserver + privateCreationObserver = func(path string) { + count++ + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if info.Mode().Perm()&0o077 != 0 { + t.Fatalf("initial staging exposed inherited access: mode %04o", info.Mode().Perm()) + } + acl, err := exec.Command("getfacl", "-cpn", path).CombinedOutput() + if err != nil { + t.Fatalf("creation ACL: %s: %v", acl, err) + } + if !strings.Contains(string(acl), "mask::---") { + t.Fatalf("initial inherited named grant is not masked: %s", acl) + } + if !info.IsDir() && info.Size() != 0 { + t.Fatal("creation observer ran after content write") + } + } + t.Cleanup(func() { privateCreationObserver = previous }) + if err := WriteFileAtomic(target, []byte("new"), mode); err != nil { + t.Fatal(err) + } + failure := errors.New("replace failed") + err = writeFileAtomic(target, []byte("must not land"), mode, func(string, string) error { return failure }) + if !errors.Is(err, failure) { + t.Fatal(err) + } + got, err := os.ReadFile(target) + if err != nil || string(got) != "new" { + t.Fatalf("destination = %q, %v", got, err) + } + acl, err := exec.Command("getfacl", "-cpn", target).CombinedOutput() + if err != nil { + t.Fatal(err) + } + if denied && !namedUserACLDenied(acl) || !denied && namedUserACLPresent(acl) { + t.Fatalf("destination ACL changed: %s", acl) + } + fmtDir, err := CreatePrivateTempDir(dir, ".zero-fmt-*") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(fmtDir); err != nil { + t.Fatal(err) + } + if count != 3 { + t.Fatalf("creation observations = %d, want 3", count) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-*")) + if err != nil || len(leftovers) != 0 { + t.Fatalf("staging leftovers: %v, %v", leftovers, err) + } + // Genuinely new publications still receive the ordinary inherited ACL. + fresh := filepath.Join(dir, "fresh") + if err := WriteFileAtomic(fresh, []byte("public"), 0o644); err != nil { + t.Fatal(err) + } + acl, err = exec.Command("getfacl", "-cpn", fresh).CombinedOutput() + if err != nil || !namedUserACLPresent(acl) || strings.Contains(string(acl), "#effective:---") { + t.Fatalf("new file lost normal inheritance: %s, %v", acl, err) + } + }) + } +} diff --git a/internal/fsutil/rename_acl_other.go b/internal/fsutil/rename_acl_other.go new file mode 100644 index 000000000..050b9271a --- /dev/null +++ b/internal/fsutil/rename_acl_other.go @@ -0,0 +1,15 @@ +//go:build !darwin + +package fsutil + +import "os" + +// preserveNativeACL is a no-op on platforms without the Darwin native ACL +// representation. Linux POSIX ACLs travel through preserveXattrs as +// system.posix_acl_access; on Windows the destination DACL is applied to the +// private staging file by protectStaging after writing its contents and before +// atomic replacement, then carried across publication by the DACL-preserving +// replace primitive. +func preserveNativeACL(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_owner_unix.go b/internal/fsutil/rename_owner_unix.go new file mode 100644 index 000000000..42ce0716d --- /dev/null +++ b/internal/fsutil/rename_owner_unix.go @@ -0,0 +1,25 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "syscall" +) + +var posixChown = func(f *os.File, uid, gid int) error { + return f.Chown(uid, gid) +} + +func preserveOwner(f *os.File, info os.FileInfo) error { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + if err := posixChown(f, int(stat.Uid), int(stat.Gid)); err != nil { + if int(stat.Uid) != os.Getuid() || int(stat.Gid) != os.Getgid() { + return err + } + } + return nil +} diff --git a/internal/fsutil/rename_owner_unix_test.go b/internal/fsutil/rename_owner_unix_test.go new file mode 100644 index 000000000..ae813cc82 --- /dev/null +++ b/internal/fsutil/rename_owner_unix_test.go @@ -0,0 +1,41 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicCallsChownWithDestOwner(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "dest") + if err := os.WriteFile(path, []byte("old"), 0o644); err != nil { + t.Fatal(err) + } + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + stat := info.Sys().(*syscall.Stat_t) + var gotUID, gotGID int + called := false + orig := posixChown + t.Cleanup(func() { posixChown = orig }) + posixChown = func(f *os.File, uid, gid int) error { + called = true + gotUID, gotGID = uid, gid + return orig(f, uid, gid) + } + if err := WriteFileAtomic(path, []byte("new"), 0o600); err != nil { + t.Fatal(err) + } + if !called { + t.Fatal("posixChown was not called; owner would be the writer inode") + } + if gotUID != int(stat.Uid) || gotGID != int(stat.Gid) { + t.Fatalf("chown uid/gid = %d/%d, want %d/%d", gotUID, gotGID, stat.Uid, stat.Gid) + } +} diff --git a/internal/fsutil/rename_owner_windows.go b/internal/fsutil/rename_owner_windows.go new file mode 100644 index 000000000..6bafcc4ad --- /dev/null +++ b/internal/fsutil/rename_owner_windows.go @@ -0,0 +1,13 @@ +//go:build windows + +package fsutil + +import "os" + +func preserveOwner(*os.File, os.FileInfo) error { + return nil +} + +func preserveXattrs(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_special_unix_test.go b/internal/fsutil/rename_special_unix_test.go new file mode 100644 index 000000000..dfeeebb3a --- /dev/null +++ b/internal/fsutil/rename_special_unix_test.go @@ -0,0 +1,42 @@ +//go:build !windows + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicRefusesNamedPipe(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "endpoint.fifo") + if err := syscall.Mkfifo(target, 0o644); err != nil { + t.Fatalf("mkfifo: %v", err) + } + + err := WriteFileAtomic(target, []byte("should-not-replace-fifo"), 0o644) + if err == nil { + t.Fatal("expected WriteFileAtomic to refuse a FIFO destination") + } + if !errors.Is(err, ErrNonRegularDestination) { + t.Fatalf("error = %v, want ErrNonRegularDestination", err) + } + + info, err := os.Lstat(target) + if err != nil { + t.Fatalf("Lstat: %v", err) + } + if info.Mode()&os.ModeNamedPipe == 0 { + t.Fatalf("FIFO was replaced; mode = %s", info.Mode()) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil { + t.Fatalf("Glob: %v", err) + } + if len(leftovers) != 0 { + t.Fatalf("temporary files left behind: %v", leftovers) + } +} diff --git a/internal/fsutil/rename_staging_other.go b/internal/fsutil/rename_staging_other.go new file mode 100644 index 000000000..3222183ae --- /dev/null +++ b/internal/fsutil/rename_staging_other.go @@ -0,0 +1,14 @@ +//go:build !windows + +package fsutil + +import "os" + +// protectStaging is a no-op on platforms whose authorization metadata is +// already copied onto the staging file by WriteFileAtomic: Unix mode bits, +// owner, and extended attributes (including POSIX ACLs) travel through +// Chmod, preserveOwner, preserveXattrs, and preserveNativeACL. Only Windows +// needs an explicit DACL transfer before publication. +func protectStaging(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_staging_windows.go b/internal/fsutil/rename_staging_windows.go new file mode 100644 index 000000000..857b7de67 --- /dev/null +++ b/internal/fsutil/rename_staging_windows.go @@ -0,0 +1,96 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +// protectStaging applies the destination DACL after the complete content was +// written under the creation-time owner-only descriptor. Keep inheritance +// disabled on staging even when the source DACL is unprotected: its effective +// ACEs are copied explicitly, and parent grants must not be re-added here. +// ReplaceFileW preserves the destination security descriptor on publication. +func protectStaging(f *os.File, destPath string) error { + descriptor, err := windows.GetNamedSecurityInfo(destPath, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return fmt.Errorf("fsutil: reading DACL of %s for staging: %w", destPath, err) + } + if descriptor == nil { + return fmt.Errorf("fsutil: destination %s has no security descriptor to protect the staging file", destPath) + } + dacl, _, err := descriptor.DACL() + if err != nil { + // A destination with no DACL at all is fully permissive, so an inherited + // staging DACL cannot expose anything the destination does not already. + if errors.Is(err, windows.ERROR_OBJECT_NOT_FOUND) { + return nil + } + return fmt.Errorf("fsutil: extracting DACL of %s for staging: %w", destPath, err) + } + if dacl == nil { + // A present but NULL DACL grants everyone full control: the destination + // is not restricted, so there is no narrower descriptor to carry over. + // Setting a NULL DACL here would only widen the staging file. + return nil + } + info := windows.SECURITY_INFORMATION(windows.DACL_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION) + handle, err := openStagingForDACL(f) + if err != nil { + return err + } + defer func() { _ = windows.CloseHandle(handle) }() + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, info, nil, nil, dacl, nil); err != nil { + return fmt.Errorf("fsutil: applying DACL of %s to staging file: %w", destPath, err) + } + return nil +} + +// openStagingForDACL opens the staging file f names with the READ_CONTROL and +// WRITE_DAC rights SetSecurityInfo requires, which the handle inside f does not +// carry. The new handle is verified to reference the same volume and file index +// as f, and to not be a reparse point, so a directory entry swapped in after +// creation cannot receive the descriptor instead of the object this process +// created. The caller owns the returned handle. +func openStagingForDACL(f *os.File) (windows.Handle, error) { + name, err := windows.UTF16PtrFromString(f.Name()) + if err != nil { + return 0, err + } + handle, err := windows.CreateFile( + name, + windows.READ_CONTROL|windows.WRITE_DAC, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return 0, fmt.Errorf("fsutil: opening staging file %s to apply its DACL: %w", f.Name(), err) + } + var created, opened windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(windows.Handle(f.Fd()), &created); err != nil { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: querying created staging file %s: %w", f.Name(), err) + } + if err := windows.GetFileInformationByHandle(handle, &opened); err != nil { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: querying reopened staging file %s: %w", f.Name(), err) + } + if opened.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: staging path %s is unexpectedly a reparse point", f.Name()) + } + if created.VolumeSerialNumber != opened.VolumeSerialNumber || + created.FileIndexHigh != opened.FileIndexHigh || + created.FileIndexLow != opened.FileIndexLow { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: staging path %s no longer names the created file", f.Name()) + } + return handle, nil +} diff --git a/internal/fsutil/rename_staging_windows_test.go b/internal/fsutil/rename_staging_windows_test.go new file mode 100644 index 000000000..42cb4db58 --- /dev/null +++ b/internal/fsutil/rename_staging_windows_test.go @@ -0,0 +1,180 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +// The creation observer checks the initial descriptor; the publication observer +// checks the destination DACL transfer. Both boundaries must exclude Everyone. +func TestProtectStagingCopiesRestrictiveDACL(t *testing.T) { + user, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatal(err) + } + sid := user.User.Sid.String() + destinationACE := "(A;;FA;;;" + sid + ")" + dir := t.TempDir() + + // Make the parent more permissive than the destination so an inherited + // staging DACL is observably broader than the destination's. + if err := grantEveryoneInheritableDACL(dir); err != nil { + t.Skipf("cannot widen the parent directory DACL on this filesystem: %v", err) + } + + target := filepath.Join(dir, "restricted.txt") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + restricted, err := windows.SecurityDescriptorFromString("D:P" + destinationACE) + if err != nil { + t.Skipf("cannot build the restrictive test descriptor: %v", err) + } + ownerOnly, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the restrictive test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + target, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, ownerOnly, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + if got, err := readDACLString(target); err != nil { + t.Skipf("cannot read back the restrictive destination DACL: %v", err) + } else if !strings.Contains(got, destinationACE) { + t.Skipf("the restrictive DACL did not take effect on this filesystem: %q", got) + } + + observedCreation := 0 + previousCreation := privateCreationObserver + privateCreationObserver = func(path string) { + observedCreation++ + acl, err := readDACLString(path) + if err != nil || !strings.Contains(acl, ";;;"+sid+")") || strings.Contains(acl, ";;;WD)") || !strings.Contains(acl, "D:P") { + t.Fatalf("initial staging DACL = %q, %v", acl, err) + } + info, err := os.Stat(path) + if err != nil || !info.IsDir() && info.Size() != 0 { + t.Fatalf("observer did not run at creation: %v, %v", info, err) + } + } + defer func() { privateCreationObserver = previousCreation }() + var ( + stagingDACL string + captureErr error + ) + previous := stagingProtectionObserver + stagingProtectionObserver = func(stagingPath string) { + stagingDACL, captureErr = readDACLString(stagingPath) + } + defer func() { stagingProtectionObserver = previous }() + + if err := WriteFileAtomic(target, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + if captureErr != nil { + t.Fatalf("reading the staging DACL before publication: %v", captureErr) + } + if stagingDACL == "" { + t.Fatal("staging DACL was not observed: the protection hook did not run before publication") + } + if !strings.Contains(stagingDACL, destinationACE) { + t.Fatalf("staging DACL = %q, want the current-user-only destination DACL", stagingDACL) + } + if strings.Contains(stagingDACL, ";;;WD)") { + t.Fatalf("staging DACL = %q is broader than the destination (inherited Everyone)", stagingDACL) + } + + failure := errors.New("replace failed") + if err := writeFileAtomic(target, []byte("must not land"), 0o600, func(string, string) error { return failure }); !errors.Is(err, failure) { + t.Fatal(err) + } + got, err := os.ReadFile(target) + if err != nil || string(got) != "new" { + t.Fatalf("failure changed destination: %q, %v", got, err) + } + stagingDir, err := CreatePrivateTempDir(dir, ".zero-fmt-*") + if err != nil { + t.Fatal(err) + } + if err := os.Remove(stagingDir); err != nil { + t.Fatal(err) + } + if observedCreation != 3 { + t.Fatalf("initial creation observations: %d", observedCreation) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-*")) + if err != nil || len(leftovers) != 0 { + t.Fatalf("staging leftovers: %v, %v", leftovers, err) + } + fresh := filepath.Join(dir, "fresh") + if err := WriteFileAtomic(fresh, []byte("public"), 0o644); err != nil { + t.Fatal(err) + } + inherited, err := readDACLString(fresh) + if err != nil || !strings.Contains(inherited, ";;;WD)") { + t.Fatalf("new file lost inheritance: %q, %v", inherited, err) + } + after, err := readDACLString(target) + if err != nil { + t.Fatalf("reading the destination DACL after replacement: %v", err) + } + if !strings.Contains(after, destinationACE) || strings.Contains(after, ";;;WD)") { + t.Fatalf("destination DACL after replacement = %q, want the original current-user-only DACL", after) + } +} + +// grantEveryoneInheritableDACL replaces path's DACL with a single inheritable +// grant of full control to Everyone (S-1-1-0), making it broader than any +// current-user-only destination inside it. +func grantEveryoneInheritableDACL(path string) error { + everyone, err := windows.StringToSid("S-1-1-0") + if err != nil { + return err + } + dacl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(everyone), + }, + }}, nil) + if err != nil { + return err + } + return windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ) +} + +// readDACLString returns the SDDL DACL portion of path's security descriptor. +func readDACLString(path string) (string, error) { + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return "", err + } + if descriptor == nil { + return "", nil + } + text := descriptor.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:], nil + } + return text, nil +} diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index 8a64852d2..8d03325b6 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -2,6 +2,8 @@ package fsutil import ( "errors" + "os" + "path/filepath" "runtime" "syscall" "testing" @@ -31,17 +33,191 @@ func TestRenameWithRetryRetriesOnWindows(t *testing.T) { } } +func TestWriteFileAtomic(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "sub", "test.txt") + content := []byte("hello atomic world") + + if err := WriteFileAtomic(target, content, 0o644); err != nil { + t.Fatalf("WriteFileAtomic failed: %v", err) + } + + read, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile failed: %v", err) + } + if string(read) != string(content) { + t.Fatalf("read content mismatch: got %q, want %q", string(read), string(content)) + } + + // Overwrite test + newContent := []byte("overwritten atomic content") + if err := WriteFileAtomic(target, newContent, 0o644); err != nil { + t.Fatalf("WriteFileAtomic overwrite failed: %v", err) + } + read2, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile after overwrite failed: %v", err) + } + if string(read2) != string(newContent) { + t.Fatalf("read content mismatch: got %q, want %q", string(read2), string(newContent)) + } +} + +func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { + dir := t.TempDir() + cases := []os.FileMode{0o600, 0o755} + for _, want := range cases { + target := filepath.Join(dir, "mode-"+want.String()+".txt") + if err := os.WriteFile(target, []byte("old"), want); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Chmod(target, want); err != nil { + t.Fatalf("Chmod: %v", err) + } + initialInfo, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat initial: %v", err) + } + expectedMode := initialInfo.Mode().Perm() + if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := info.Mode().Perm(); got != expectedMode { + t.Fatalf("mode = %04o, want %04o", got, expectedMode) + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } + } +} + func TestRenameWithRetryNonRetryableError(t *testing.T) { - sentinel := errors.New("disk on fire") var attempts int + expectedErr := os.ErrInvalid err := RenameWithRetry("src", "dst", func(src, dst string) error { attempts++ - return sentinel + return expectedErr }) - if !errors.Is(err, sentinel) { - t.Fatalf("expected sentinel error, got: %v", err) + if !errors.Is(err, expectedErr) { + t.Fatalf("got err %v, want %v", err, expectedErr) } if attempts != 1 { - t.Errorf("expected only 1 attempt for non-retryable error, got %d", attempts) + t.Fatalf("attempts = %d, want 1", attempts) + } +} + +func TestWriteFileAtomicRefusesDirectoryBeforeStaging(t *testing.T) { + previous := privateCreationObserver + privateCreationObserver = func(string) { t.Fatal("directory validation reached staging creation") } + defer func() { privateCreationObserver = previous }() + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } + marker := filepath.Join(target, "keep.txt") + if err := os.WriteFile(marker, []byte("keep"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if err := WriteFileAtomic(target, []byte("should-not-land"), 0o644); !errors.Is(err, ErrNonRegularDestination) { + t.Fatalf("expected directory validation failure, got %v", err) + } + + got, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("original destination was disturbed: %v", err) + } + if string(got) != "keep" { + t.Fatalf("marker = %q, want keep", got) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil { + t.Fatalf("Glob: %v", err) + } + if len(leftovers) != 0 { + t.Fatalf("temporary files left behind: %v", leftovers) + } +} + +func TestWriteFileAtomicRefusesNonWritableTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "readonly.txt") + if err := os.WriteFile(target, []byte("original"), 0o444); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Chmod(target, 0o444); err != nil { + t.Fatalf("Chmod: %v", err) + } + probe, err := os.OpenFile(target, os.O_WRONLY, 0) + if err == nil { + _ = probe.Close() + t.Skip("this host allows writing a mode-0444 file (for example when running as root)") + } + + if err := WriteFileAtomic(target, []byte("replaced"), 0o644); err == nil { + t.Fatal("expected WriteFileAtomic to refuse a non-writable destination") + } + + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "original" { + t.Fatalf("content = %q, want %q", got, "original") + } + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := info.Mode().Perm(); got&0o222 != 0 { + t.Fatalf("destination became writable: perm=%04o", got) + } +} + +func TestWriteFileAtomicLeavesDestinationOnReplaceFailure(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.WriteFile(target, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + injected := errors.New("injected replacement failure") + var owned string + err := writeFileAtomic(target, []byte("complete replacement"), 0o644, func(src, dst string) error { + owned = src + if dst != target { + t.Fatalf("destination = %q", dst) + } + got, err := os.ReadFile(src) + if err != nil || string(got) != "complete replacement" { + t.Fatalf("replacement did not receive complete staging: %q, %v", got, err) + } + return injected + }) + if !errors.Is(err, injected) { + t.Fatalf("error = %v, want replacement failure", err) + } + if owned == "" { + t.Fatal("replacement was never reached") + } + got, err := os.ReadFile(target) + if err != nil || string(got) != "original" { + t.Fatalf("original destination changed: %q, %v", got, err) + } + if _, err := os.Lstat(owned); !os.IsNotExist(err) { + t.Fatalf("owned staging was not cleaned: %s: %v", owned, err) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil || len(leftovers) != 0 { + t.Fatalf("staging leftovers: %v, %v", leftovers, err) } } diff --git a/internal/fsutil/rename_umask_unix_test.go b/internal/fsutil/rename_umask_unix_test.go new file mode 100644 index 000000000..4b9fd2d5b --- /dev/null +++ b/internal/fsutil/rename_umask_unix_test.go @@ -0,0 +1,57 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicRespectsProcessUmask(t *testing.T) { + // Temporarily set umask to 0o077 + oldMask := syscall.Umask(0o077) + defer syscall.Umask(oldMask) + + dir := t.TempDir() + target := filepath.Join(dir, "umask_test.txt") + + // Write new file with 0o644 requested perm + if err := WriteFileAtomic(target, []byte("umask test"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + + // Under umask 0o077, 0o644 & ~0o077 = 0o600 + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("created file perm = %04o, want %04o (honoring umask 0o077)", got, 0o600) + } +} + +func TestWriteFileAtomicStagesReplacementWithDestinationMode(t *testing.T) { + oldMask := syscall.Umask(0) + defer syscall.Umask(oldMask) + + dir := t.TempDir() + target := filepath.Join(dir, "secret.txt") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + + orig := stagingModeObserver + t.Cleanup(func() { stagingModeObserver = orig }) + var observed os.FileMode + stagingModeObserver = func(mode os.FileMode) { observed = mode } + + if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + if got := observed.Perm(); got != 0o600 { + t.Fatalf("staging mode = %04o, want the destination mode %04o", got, 0o600) + } +} diff --git a/internal/fsutil/rename_xattr_notfound_bsd.go b/internal/fsutil/rename_xattr_notfound_bsd.go new file mode 100644 index 000000000..3d6344465 --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_bsd.go @@ -0,0 +1,13 @@ +//go:build darwin || netbsd + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENOATTR) || errors.Is(err, unix.ENODATA) +} diff --git a/internal/fsutil/rename_xattr_notfound_freebsd.go b/internal/fsutil/rename_xattr_notfound_freebsd.go new file mode 100644 index 000000000..23ebc6a0d --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_freebsd.go @@ -0,0 +1,13 @@ +//go:build freebsd + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENOATTR) +} diff --git a/internal/fsutil/rename_xattr_notfound_linux.go b/internal/fsutil/rename_xattr_notfound_linux.go new file mode 100644 index 000000000..d24539038 --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_linux.go @@ -0,0 +1,13 @@ +//go:build linux + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENODATA) +} diff --git a/internal/fsutil/rename_xattr_stub.go b/internal/fsutil/rename_xattr_stub.go new file mode 100644 index 000000000..79a39002f --- /dev/null +++ b/internal/fsutil/rename_xattr_stub.go @@ -0,0 +1,9 @@ +//go:build !windows && !linux && !darwin && !freebsd && !netbsd + +package fsutil + +import "os" + +func preserveXattrs(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_xattr_unix.go b/internal/fsutil/rename_xattr_unix.go new file mode 100644 index 000000000..eac309734 --- /dev/null +++ b/internal/fsutil/rename_xattr_unix.go @@ -0,0 +1,115 @@ +//go:build linux || darwin || freebsd || netbsd + +package fsutil + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +const posixACLAccessXattr = "system.posix_acl_access" + +func preserveXattrs(f *os.File, srcPath string) error { + names, err := listXattrs(srcPath) + if err != nil { + if isXattrUnsupported(err) { + return nil + } + return fmt.Errorf("fsutil: listing xattrs of %s: %w", srcPath, err) + } + hasAccessACL := false + for _, name := range names { + if name == posixACLAccessXattr { + hasAccessACL = true + } + data, err := getXattr(srcPath, name) + if err != nil { + if isXattrUnsupported(err) { + continue + } + return fmt.Errorf("fsutil: reading xattr %s from %s: %w", name, srcPath, err) + } + if err := unix.Fsetxattr(int(f.Fd()), name, data, 0); err != nil { + if name == "security.selinux" && isSELinuxPolicyDenial(err) { + continue + } + return fmt.Errorf("fsutil: preserving xattr %s: %w", name, err) + } + } + if !hasAccessACL { + if err := unix.Fremovexattr(int(f.Fd()), posixACLAccessXattr); err != nil { + if !isXattrNotFound(err) && !isXattrUnsupported(err) { + return fmt.Errorf("fsutil: removing inherited ACL from replacement: %w", err) + } + } + } + return nil +} + +func isSELinuxPolicyDenial(err error) bool { + return errors.Is(err, unix.EACCES) || + errors.Is(err, unix.EPERM) || + errors.Is(err, unix.ENOTSUP) || + errors.Is(err, unix.EOPNOTSUPP) +} + +func listXattrs(path string) ([]string, error) { + dest := []byte(nil) + for { + size, err := unix.Listxattr(path, dest) + if err != nil { + return nil, err + } + if size == 0 { + return nil, nil + } + if len(dest) < size { + dest = make([]byte, size) + continue + } + return splitXattrNames(dest[:size]), nil + } +} + +func getXattr(path, name string) ([]byte, error) { + dest := []byte(nil) + for { + size, err := unix.Getxattr(path, name, dest) + if err != nil { + return nil, err + } + if size == 0 { + return []byte{}, nil + } + if len(dest) < size { + dest = make([]byte, size) + continue + } + return dest[:size], nil + } +} + +func splitXattrNames(buf []byte) []string { + names := make([]string, 0) + start := 0 + for i, b := range buf { + if b != 0 { + continue + } + if i > start { + names = append(names, string(buf[start:i])) + } + start = i + 1 + } + if start < len(buf) { + names = append(names, string(buf[start:])) + } + return names +} + +func isXattrUnsupported(err error) bool { + return errors.Is(err, unix.ENOTSUP) || errors.Is(err, unix.EOPNOTSUPP) +} diff --git a/internal/fsutil/rename_xattr_unix_test.go b/internal/fsutil/rename_xattr_unix_test.go new file mode 100644 index 000000000..abe717819 --- /dev/null +++ b/internal/fsutil/rename_xattr_unix_test.go @@ -0,0 +1,35 @@ +//go:build linux || darwin || freebsd || netbsd + +package fsutil + +import ( + "fmt" + "testing" + + "golang.org/x/sys/unix" +) + +func TestSELinuxPolicyDenialClassification(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"eacces", unix.EACCES, true}, + {"eperm", unix.EPERM, true}, + {"enotsup", unix.ENOTSUP, true}, + {"eopnotsupp", unix.EOPNOTSUPP, true}, + {"eio", unix.EIO, false}, + {"enospc", unix.ENOSPC, false}, + {"wrapped eacces", fmt.Errorf("setxattr: %w", unix.EACCES), true}, + {"wrapped eio", fmt.Errorf("setxattr: %w", unix.EIO), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSELinuxPolicyDenial(tc.err); got != tc.want { + t.Fatalf("isSELinuxPolicyDenial(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index 34f55781e..d5408a166 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -157,25 +157,22 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } - if err := commitFileContents(absolutePath, priorInfo, &content, updated); err != nil { - return errorResult("Error writing " + relativePath + ": " + err.Error()) - } modelKnownContent := updated - // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the - // FileTracker re-baseline: recording pre-format content would make the very - // next edit look like an external modification and trip the conflict guard. - formatting := maybeFormatWrittenFileScoped(ctx, tool.workspaceRoot, tool.scope, absolutePath, updated, priorInfo.Mode().Perm()) + formatting := maybeFormatWrittenFile(ctx, absolutePath, updated) updated = formatting.Content - finalContentKnown := formatting.ContentKnown + finalContentKnown := true + + // Bind the atomic publication to the pre-edit object and bytes: a path or + // inode swap since the read fails closed instead of clobbering a newer file. + expectedContent := &content + cleanupWarning, err := commitFileContents(absolutePath, priorInfo, expectedContent, updated) + if err != nil { + return errorResult("Error writing " + relativePath + ": " + err.Error()) + } // Re-baseline to the content we just wrote so subsequent edits in this session // compare against the current on-disk state, not the pre-edit version. - newInfo := formatting.Info - if newInfo == nil { - newInfo, _ = os.Stat(absolutePath) - } - if !finalContentKnown { - options.FileTracker.Forget(absolutePath) - } else if updated == modelKnownContent { + newInfo, _ := os.Stat(absolutePath) + if updated == modelKnownContent { // OUR edit, so we know precisely which lines moved: RecordEdit carries // across the reads this edit did not disturb instead of dropping them. // @@ -205,6 +202,9 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } summary := fmt.Sprintf("Successfully edited %s (replaced %d occurrence%s).", relativePath, replacedCount, suffix) summary += formatting.notice(relativePath) + if cleanupWarning != "" { + summary += " " + cleanupWarning + } if finalContentKnown { summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) } diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index 924e056dd..36533eb82 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -2,9 +2,10 @@ package tools import ( "errors" - "fmt" "io" "os" + + "github.com/Gitlawb/zero/internal/fsutil" ) var errFileChangedDuringWrite = errors.New("file changed on disk before the write committed") @@ -20,89 +21,89 @@ var fileWriteBeforeCommit func(path string) var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat() } // commitFileContents binds an overwrite to the file identity and bytes that -// the caller observed. A create uses exclusive creation. An overwrite opens the -// observed object without truncation, verifies identity/content through that -// handle, then truncates and writes the same handle. A path replacement before -// or during commit therefore fails instead of publishing stale rich evidence. +// the caller observed, then publishes the new content through a private +// same-directory temporary file and an atomic replacement (fsutil.WriteFileAtomic). +// +// The identity checks run against the object opened at commit time, before any +// mutation: a path replacement between observation and commit therefore fails +// instead of publishing stale content, and no reader ever observes a truncated +// destination (invariant #921). The exclusive-create branch refuses a path that +// appeared after the caller observed it missing. // -// expectedInfo nil means the caller observed a missing path. expectedContent -// may be nil for an existing but unreadable file; that path may still be -// overwritten, but callers must omit rich before/after evidence. -func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent *string, content string) error { +// The returned warning string is non-empty only when the replacement already +// committed but its backup cleanup failed; the caller reports success and +// surfaces the warning. A non-nil error means nothing was published. +func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent *string, content string) (string, error) { if fileWriteBeforeCommit != nil { fileWriteBeforeCommit(path) } if expectedInfo == nil { - file, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err != nil { - return err + if _, err := os.Lstat(path); err == nil { + return "", errFileChangedDuringWrite + } else if !os.IsNotExist(err) { + return "", err } - openedInfo, err := fileWriteStat(file) - if err != nil { - _ = file.Close() - return err - } - return writeAndVerifyFileIdentity(path, file, openedInfo, content, false) + return publishFileContents(path, content) } - flags := os.O_WRONLY - if expectedContent != nil { - flags = os.O_RDWR - } - file, err := os.OpenFile(path, flags, 0) + file, err := os.OpenFile(path, fileCommitOpenFlags(expectedContent), 0) if err != nil { - return err + return "", err } openedInfo, err := fileWriteStat(file) if err != nil { _ = file.Close() - return err + return "", err } if !os.SameFile(expectedInfo, openedInfo) { _ = file.Close() - return errFileChangedDuringWrite + return "", errFileChangedDuringWrite } pathInfo, err := os.Stat(path) if err != nil || !os.SameFile(openedInfo, pathInfo) { _ = file.Close() - return errFileChangedDuringWrite + return "", errFileChangedDuringWrite } if expectedContent != nil { current, readErr := io.ReadAll(file) if readErr != nil { _ = file.Close() - return readErr + return "", readErr } if string(current) != *expectedContent { _ = file.Close() - return errFileChangedDuringWrite + return "", errFileChangedDuringWrite } } - return writeAndVerifyFileIdentity(path, file, openedInfo, content, true) + if err := file.Close(); err != nil { + return "", err + } + return publishFileContents(path, content) } -func writeAndVerifyFileIdentity(path string, file *os.File, openedInfo os.FileInfo, content string, truncate bool) error { - if truncate { - if err := file.Truncate(0); err != nil { - _ = file.Close() - return err - } - if _, err := file.Seek(0, io.SeekStart); err != nil { - _ = file.Close() - return err - } - } - if _, err := io.WriteString(file, content); err != nil { - _ = file.Close() - return err +// fileCommitOpenFlags selects the descriptor used to bind an overwrite to the +// observed object. When the caller has preimage bytes to compare, the descriptor +// must be readable (and writable, to prove the same authorization an in-place +// write would have required); when it does not, write access alone is enough. +func fileCommitOpenFlags(expectedContent *string) int { + if expectedContent != nil { + return os.O_RDWR } - if err := file.Close(); err != nil { - return err + return os.O_WRONLY +} + +// publishFileContents performs the atomic replacement and treats a committed +// replacement whose backup cleanup failed as a successful write, returning the +// warning instead of flipping the tool status to error. +func publishFileContents(path, content string) (string, error) { + err := fsutil.WriteFileAtomic(path, []byte(content), 0o644) + if err == nil { + return "", nil } - pathInfo, err := os.Stat(path) - if err != nil || !os.SameFile(openedInfo, pathInfo) { - return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + var committed *fsutil.CommittedReplacementCleanupError + if errors.As(err, &committed) { + return "replacement committed, but backup cleanup failed", nil } - return nil + return "", err } diff --git a/internal/tools/file_commit_atomic_test.go b/internal/tools/file_commit_atomic_test.go new file mode 100644 index 000000000..4c5007411 --- /dev/null +++ b/internal/tools/file_commit_atomic_test.go @@ -0,0 +1,178 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +func strPtr(value string) *string { return &value } + +// installCommitRace replaces the destination after the caller observed it but +// before commitFileContents opens the object it will publish. It is the +// deterministic half of the check-to-use window. +func installCommitRace(t *testing.T, mutate func(string)) { + t.Helper() + prior := fileWriteBeforeCommit + fileWriteBeforeCommit = mutate + t.Cleanup(func() { fileWriteBeforeCommit = prior }) +} + +func TestCommitFileContentsRefusesCreateRace(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "created.txt") + installCommitRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + + if _, err := commitFileContents(target, nil, nil, "zero\n"); err == nil { + t.Fatal("a path that appeared after the observation must not be overwritten") + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced create content = %q, err=%v", got, err) + } +} + +func TestCommitFileContentsRefusesOverwriteInodeSwap(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + observed, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + installCommitRace(t, func(path string) { + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + + if _, err := commitFileContents(target, observed, strPtr("observed\n"), "zero\n"); err == nil { + t.Fatal("a destination replaced since the observation must not be overwritten") + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced overwrite content = %q, err=%v", got, err) + } +} + +func TestCommitFileContentsRefusesOverwriteContentChange(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + observed, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + installCommitRace(t, func(path string) { + // Same inode, different bytes: only the preimage comparison can catch it. + if err := os.WriteFile(path, []byte("rewritten in place\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + + if _, err := commitFileContents(target, observed, strPtr("observed\n"), "zero\n"); err == nil { + t.Fatal("a destination whose bytes changed since the observation must not be overwritten") + } + if got, err := os.ReadFile(target); err != nil || string(got) != "rewritten in place\n" { + t.Fatalf("raced preimage content = %q, err=%v", got, err) + } +} + +// The publication must be a same-directory temp-and-replace, not an in-place +// truncation: that is what makes invariant #921 (no partial destination after a +// crash or cancellation) hold. The inode changing across a successful overwrite +// is the observable consequence. +func TestCommitFileContentsPublishesByReplacingTheInode(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + observed, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + + warning, err := commitFileContents(target, observed, strPtr("observed\n"), "replacement\n") + if err != nil { + t.Fatalf("commitFileContents: %v", err) + } + if warning != "" { + t.Fatalf("unexpected cleanup warning: %q", warning) + } + after, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if os.SameFile(observed, after) { + t.Fatal("destination inode survived the write: this is an in-place truncation, not an atomic replacement") + } + if got, err := os.ReadFile(target); err != nil || string(got) != "replacement\n" { + t.Fatalf("published content = %q, err=%v", got, err) + } + // No staging leftovers next to the destination. + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".zero-tmp-") { + t.Fatalf("staging file %q was left behind", entry.Name()) + } + } +} + +func TestWriteFileRefusesRaceBeforeAtomicPublish(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installCommitRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("raced overwrite = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced overwrite content = %q, err=%v", got, err) + } +} + +func TestEditFileRefusesRaceBeforeAtomicPublish(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installCommitRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "old_string": "observed", "new_string": "zero", + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("raced edit = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("raced edit content = %q, err=%v", got, err) + } +} diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index a7cdafd50..fe821d7d3 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -1,30 +1,29 @@ package tools import ( + "bytes" "context" "errors" - "fmt" - "io" "os" "os/exec" "path/filepath" "strings" "time" - "github.com/Gitlawb/zero/internal/sandbox" + "github.com/Gitlawb/zero/internal/fsutil" ) // Format-on-write for the mutating file tools. When enabled, a successful -// edit_file/write_file runs the language's standard formatter on the file it -// just wrote, so the model's output always lands in project-canonical style +// edit_file/write_file formats staged content before the single atomic +// publication, so the model's output always lands in project-canonical style // and never fails a CI format check it cannot see. Off by default (set // ZERO_FORMAT_ON_WRITE=1): auto-reformatting changes bytes the model did not // write, which strict workflows may not want. // -// Ordering matters: formatting runs BEFORE the FileTracker re-baseline, and -// the caller records the POST-format content. Formatting after the baseline -// would make the very next edit look like an external modification and trip -// the conflict guard. +// Ordering matters: formatting runs on an isolated temporary copy BEFORE +// publication and BEFORE the FileTracker re-baseline. The caller records the +// POST-format content that was actually published. Formatting the destination +// in place after publication would reintroduce partial-file writes. // formatOnWriteTimeout bounds one formatter run; a wedged formatter must never // hang a tool call. On timeout the unformatted write stands, and the caller @@ -48,33 +47,16 @@ var formatOnWriteTimeout = 10 * time.Second // and finds out from a CI format check it cannot see, which is the thing this // feature exists to prevent. type formatOnWriteResult struct { - Content string - ContentKnown bool - Info os.FileInfo + Content string // Formatter is the binary that was run, named in the notice so the user can // tell a slow gofmt from a slow prettier. Formatter string TimedOut bool - // RestoreFailed means the file on disk is not known to hold Content. - // - // These formatters edit in place, so one that is killed or fails partway - // can leave the target truncated or half-rewritten: what a dead - // `prettier --write` leaves behind is not the input and not the output. - // Returning the written bytes while disk holds something else would put the - // tracker baseline, the diff preview and the file itself into three - // different states, so the failure paths write the bytes back. When even - // that fails the user has to hear about it: it is their file. - RestoreFailed bool } // notice is the line appended to the tool summary when formatting was expected // and did not happen, and empty in every other case. func (result formatOnWriteResult) notice(relativePath string) string { - if result.RestoreFailed { - return "\n\nWARNING: " + relativePath + " may not hold what was written. " + - result.Formatter + " was interrupted while rewriting it in place and the " + - "content could not be written back. Re-read the file before trusting it." - } if !result.TimedOut { return "" } @@ -84,8 +66,11 @@ func (result formatOnWriteResult) notice(relativePath string) string { } // formatterCommands maps a file extension to the formatter argv; the file path -// is appended as the final argument. Only in-place, config-respecting, -// community-standard formatters — a missing binary silently skips formatting. +// is appended for physical-file adapters. Ruff and Prettier instead receive +// the logical destination with stdin. Physical adapters keep the original +// basename inside an owner-only sibling directory (including auxiliary files), +// and retain the destination directory as their working directory. A missing +// binary silently skips formatting. var formatterCommands = map[string][]string{ ".go": {"gofmt", "-w"}, ".rs": {"rustfmt"}, @@ -123,36 +108,20 @@ func formatOnWriteEnabled() bool { return value != "" && value != "0" && !strings.EqualFold(value, "false") } -var runFormatOnWriteCommand = func(ctx context.Context, binaryPath string, arguments []string, directory string) error { - formatter := exec.CommandContext(ctx, binaryPath, arguments...) - formatter.Dir = directory - formatter.Stdin = strings.NewReader("") - return formatter.Run() -} - -var readFormattedFile = readRootedFile - -// maybeFormatWrittenFile is the unscoped test-facing wrapper. Production -// callers use maybeFormatWrittenFileScoped so a formatter cannot redirect the -// final read or recovery write outside the configured write roots. +// maybeFormatWrittenFile runs the configured formatter on an isolated copy of +// writtenContent (when enabled and on PATH) and returns the bytes to publish. +// The destination path is never opened or rewritten here. Best-effort +// throughout: any failure — no formatter, formatter error, timeout, unreadable +// result — returns writtenContent so the caller's state matches the last write +// it performed itself. Only the timeout is reported back, for the reason on +// formatOnWriteResult. func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) formatOnWriteResult { - restoreMode := os.FileMode(0o644) - if info, err := os.Stat(absolutePath); err == nil { - restoreMode = info.Mode().Perm() - } - return maybeFormatWrittenFileScoped(ctx, filepath.Dir(absolutePath), nil, absolutePath, writtenContent, restoreMode) -} - -// maybeFormatWrittenFileScoped runs the configured formatter and returns only -// content verified through a descriptor-bound root. Formatter failures restore -// writtenContent through that same root; if restoration or the final read -// fails, ContentKnown is false and callers omit exact diff evidence. -func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, scope PathScope, absolutePath string, writtenContent string, restoreMode os.FileMode) formatOnWriteResult { - unformatted := formatOnWriteResult{Content: writtenContent, ContentKnown: true} + unformatted := formatOnWriteResult{Content: writtenContent} if !formatOnWriteEnabled() { return unformatted } - command, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))] + ext := strings.ToLower(filepath.Ext(absolutePath)) + command, ok := formatterCommands[ext] if !ok { return unformatted } @@ -160,90 +129,85 @@ func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, sco if err != nil { return unformatted } - root, relativePath, err := openFormattedFileRoot(workspaceRoot, scope, absolutePath) + dir := filepath.Dir(absolutePath) + if command[0] == "prettier" { + return formatWithPrettier(ctx, command[0], binaryPath, command[1:], absolutePath, writtenContent) + } + if command[0] == "ruff" { + return formatWithStdin(ctx, command[0], binaryPath, append(append([]string(nil), command[1:]...), "--stdin-filename", absolutePath, "-"), absolutePath, writtenContent) + } + stagingDir, err := fsutil.CreatePrivateTempDir(dir, ".zero-fmt-*") + if err != nil { + return unformatted + } + defer func() { _ = os.RemoveAll(stagingDir) }() + staging, err := os.OpenFile(filepath.Join(stagingDir, filepath.Base(absolutePath)), os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) if err != nil { - unformatted.ContentKnown = false return unformatted } - defer root.Close() - + stagingName := staging.Name() + defer func() { _ = os.Remove(stagingName) }() + if _, err := staging.WriteString(writtenContent); err != nil { + _ = staging.Close() + return unformatted + } + if err := staging.Close(); err != nil { + return unformatted + } formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() - arguments := append(append([]string(nil), command[1:]...), absolutePath) - if err := runFormatOnWriteCommand(formatCtx, binaryPath, arguments, filepath.Dir(absolutePath)); err != nil { + arguments := append(append([]string(nil), command[1:]...), stagingName) + formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) + formatter.Dir = dir + formatter.Stdin = strings.NewReader("") + if err := formatter.Run(); err != nil { unformatted.Formatter = command[0] - if restoreErr := restoreFormattedFile(root, relativePath, writtenContent, restoreMode); restoreErr != nil { - unformatted.RestoreFailed = true - unformatted.ContentKnown = false - } else if restored, info, readErr := readFormattedFile(root, relativePath); readErr != nil { - unformatted.ContentKnown = false - } else { - unformatted.Content = string(restored) - unformatted.Info = info - } if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { unformatted.TimedOut = true } return unformatted } - - formatted, info, err := readFormattedFile(root, relativePath) + formatted, err := os.ReadFile(stagingName) if err != nil { - unformatted.ContentKnown = false return unformatted } - return formatOnWriteResult{Content: string(formatted), ContentKnown: true, Info: info, Formatter: command[0]} + return formatOnWriteResult{Content: string(formatted), Formatter: command[0]} } -func restoreFormattedFile(root *os.Root, relativePath string, content string, mode os.FileMode) error { - file, err := root.OpenFile(relativePath, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, mode) - if err != nil { - return err - } - if _, err := io.WriteString(file, content); err != nil { - _ = file.Close() - return err +// formatWithPrettier preserves config overrides and ignore rules by transmitting +// the logical destination alongside stdin, without physical destination writes. +func formatWithPrettier(ctx context.Context, formatterName, binaryPath string, formatterArgs []string, absolutePath, writtenContent string) formatOnWriteResult { + arguments := make([]string, 0, len(formatterArgs)+2) + for _, arg := range formatterArgs { + if arg == "--write" { + continue + } + arguments = append(arguments, arg) } - return file.Close() + arguments = append(arguments, "--stdin-filepath", absolutePath) + return formatWithStdin(ctx, formatterName, binaryPath, arguments, absolutePath, writtenContent) } -// openFormattedFileRoot opens the write root before the formatter runs and -// computes the target relative to that descriptor-bound root. Atomic in-root -// replacement remains valid; a formatter that swaps the target to an escaping -// symlink is rejected when readFormattedFile opens it through the root. -func openFormattedFileRoot(workspaceRoot string, scope PathScope, absolutePath string) (*os.Root, string, error) { - roots, err := scopedRoots(workspaceRoot, scope) - if err != nil { - return nil, "", err - } - var firstErr error - for _, configuredRoot := range roots { - resolvedRoot, err := filepath.Abs(configuredRoot) - if err == nil { - resolvedRoot, err = filepath.EvalSymlinks(resolvedRoot) - } - if err != nil { - if firstErr == nil { - firstErr = err - } - continue - } - candidate := sandbox.NormalizePrefixForRoot(absolutePath, resolvedRoot) - relativePath, err := filepath.Rel(resolvedRoot, candidate) - if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { - continue - } - root, err := os.OpenRoot(resolvedRoot) - if err != nil { - if firstErr == nil { - firstErr = err - } - continue +func formatWithStdin(ctx context.Context, formatterName, binaryPath string, arguments []string, absolutePath, writtenContent string) formatOnWriteResult { + unformatted := formatOnWriteResult{Content: writtenContent} + formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) + defer cancel() + formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) + formatter.Dir = filepath.Dir(absolutePath) + formatter.Stdin = strings.NewReader(writtenContent) + var stdout bytes.Buffer + formatter.Stdout = &stdout + if err := formatter.Run(); err != nil { + unformatted.Formatter = formatterName + if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { + unformatted.TimedOut = true } - return root, relativePath, nil + return unformatted } - if firstErr != nil { - return nil, "", firstErr + if stdout.Len() == 0 && writtenContent != "" { + // A formatter that produced nothing for non-empty input (an ignored + // path on some CLI versions) must not publish an empty file. + return unformatted } - return nil, "", fmt.Errorf("%s must stay inside the configured write roots", absolutePath) + return formatOnWriteResult{Content: stdout.String(), Formatter: formatterName} } diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 2750b037a..9bcda8fc6 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -8,6 +8,7 @@ import ( "runtime" "strings" "testing" + "time" ) // gofmt ships with the Go toolchain, so it is the one formatter guaranteed to @@ -99,7 +100,7 @@ func TestFormatOnWriteFormatsAndKeepsTrackerConsistent(t *testing.T) { func TestFormatOnWriteSkipsUnknownExtensions(t *testing.T) { t.Setenv("ZERO_FORMAT_ON_WRITE", "1") formatting := maybeFormatWrittenFile(context.Background(), filepath.Join(t.TempDir(), "notes.xyz"), "raw text") - if formatting.Content != "raw text" || !formatting.ContentKnown { + if formatting.Content != "raw text" { t.Fatalf("unknown extension must pass through: %q", formatting.Content) } if notice := formatting.notice("notes.xyz"); notice != "" { @@ -116,7 +117,7 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { t.Fatal(err) } formatting := maybeFormatWrittenFile(context.Background(), targetPath, uglyContent) - if formatting.Content != uglyContent || !formatting.ContentKnown { + if formatting.Content != uglyContent { t.Fatalf("missing formatter must return written content, got %q", formatting.Content) } if notice := formatting.notice("a.go"); notice != "" { @@ -124,410 +125,425 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { } } -func TestFormatOnWriteReadsMutatedFileAfterFormatterFailure(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - targetPath := filepath.Join(t.TempDir(), "a.go") - if err := os.WriteFile(targetPath, []byte("requested"), 0o644); err != nil { - t.Fatal(err) - } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { - if err := os.WriteFile(targetPath, []byte("formatter-mutated"), 0o644); err != nil { - t.Fatal(err) - } - return exec.ErrNotFound - } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - - formatting := maybeFormatWrittenFile(context.Background(), targetPath, "requested") - if !formatting.ContentKnown || formatting.Content != "requested" { - t.Fatalf("formatter failure content = %q, known=%t", formatting.Content, formatting.ContentKnown) +// requirePrettier skips when the Node-based formatter is not installed; unlike +// gofmt it is not guaranteed by the Go toolchain. +func requirePrettier(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("prettier"); err != nil { + t.Skip("prettier not on PATH") } } -func TestFormatOnWriteReportsWhenFormatterFailureCannotBeRestored(t *testing.T) { - requireGofmt(t) +func TestFormatOnWritePrettierUsesDestinationFileName(t *testing.T) { + requirePrettier(t) t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - targetPath := filepath.Join(t.TempDir(), "a.go") - if err := os.WriteFile(targetPath, []byte("requested"), 0o644); err != nil { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".prettierrc"), []byte(`{"overrides":[{"files":"special.js","options":{"singleQuote":true}}]}`), 0o644); err != nil { t.Fatal(err) } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { - if err := os.Remove(targetPath); err != nil { - t.Fatal(err) - } - if err := os.Mkdir(targetPath, 0o755); err != nil { - t.Fatal(err) - } - return exec.ErrNotFound + if err := os.WriteFile(filepath.Join(dir, ".prettierignore"), []byte("ignored.js\n"), 0o644); err != nil { + t.Fatal(err) } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - formatting := maybeFormatWrittenFile(context.Background(), targetPath, "requested") - if !formatting.RestoreFailed { - t.Fatal("failed recovery was not reported") - } - if formatting.ContentKnown { - t.Fatalf("failed recovery reported known content %q", formatting.Content) - } - if notice := formatting.notice("a.go"); !strings.Contains(notice, "WARNING: a.go may not hold what was written") { - t.Fatalf("failed recovery notice = %q", notice) + // The override matches "special.js" only. If Prettier saw the staging name + // ".zero-fmt-*.js" instead of the destination, it would use the default + // double quotes and this assertion would fail. + specialPath := filepath.Join(dir, "special.js") + special := maybeFormatWrittenFile(context.Background(), specialPath, "const x = \"a\";\n") + if !strings.Contains(special.Content, "const x = 'a';") { + t.Fatalf("prettier must resolve .prettierrc against the destination name, got %q", special.Content) } -} - -func TestWriteFileRecreatesFormatterDeletedTargetWithOriginalMode(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - for _, tc := range []struct { - name string - existing bool - wantMode os.FileMode - overwrite bool - }{ - {name: "create", wantMode: 0o644}, - {name: "overwrite", existing: true, wantMode: 0o751, overwrite: true}, - } { - t.Run(tc.name, func(t *testing.T) { - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - if tc.existing { - if err := os.WriteFile(targetPath, []byte("before\n"), tc.wantMode); err != nil { - t.Fatal(err) - } - } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { - if err := os.Remove(targetPath); err != nil { - t.Fatal(err) - } - return exec.ErrNotFound - } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ - "path": "a.go", "content": "requested\n", "overwrite": tc.overwrite, - }) - if result.Status != StatusOK { - t.Fatalf("write status = %s: %s", result.Status, result.Output) - } - content, err := os.ReadFile(targetPath) - if err != nil || string(content) != "requested\n" { - t.Fatalf("recreated content = %q, err=%v", content, err) - } - if runtime.GOOS != "windows" { - info, err := os.Stat(targetPath) - if err != nil { - t.Fatal(err) - } - if got := info.Mode().Perm(); got != tc.wantMode { - t.Fatalf("recreated mode = %o, want %o", got, tc.wantMode) - } - } - }) + // Default Prettier would reformat this to `const x = "a";`. Because the + // destination name is ignored, Prettier echoes the input and the fallback + // keeps the written bytes intact. Against the staging name it would not be + // ignored and the reformatted bytes would win. + ignoredPath := filepath.Join(dir, "ignored.js") + ignoredInput := "const x=\"a\";\n" + ignored := maybeFormatWrittenFile(context.Background(), ignoredPath, ignoredInput) + if ignored.Content != ignoredInput { + t.Fatalf("prettier must honour .prettierignore for the destination name, got %q", ignored.Content) } } -func TestFormatOnWriteMarksUnreadableFinalStateUnknown(t *testing.T) { - requireGofmt(t) +// TestFormatOnWritePrettierUsesDestinationFilename exercises the same +// destination-name resolution end-to-end through write_file: the bytes actually +// published to disk, not just the formatter helper's return value, must reflect +// the .prettierrc override and the .prettierignore rule. Resolving either +// against the ".zero-fmt-*.js" staging name would flip both assertions. +func TestFormatOnWritePrettierUsesDestinationFilename(t *testing.T) { + requirePrettier(t) t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - targetPath := filepath.Join(t.TempDir(), "a.go") - if err := os.WriteFile(targetPath, []byte("requested"), 0o644); err != nil { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".prettierrc"), []byte(`{"overrides":[{"files":"special.js","options":{"singleQuote":true}}]}`), 0o644); err != nil { t.Fatal(err) } - priorRunner := runFormatOnWriteCommand - priorReader := readFormattedFile - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } - readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } - t.Cleanup(func() { - runFormatOnWriteCommand = priorRunner - readFormattedFile = priorReader - }) - - formatting := maybeFormatWrittenFile(context.Background(), targetPath, "requested") - if formatting.ContentKnown || formatting.Content != "requested" { - t.Fatalf("unreadable formatter result = %q, known=%t", formatting.Content, formatting.ContentKnown) - } -} - -func TestWriteFileUsesVerifiedBytesAfterFormatterFailure(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { - if err := os.WriteFile(targetPath, []byte("formatter-mutated\n"), 0o644); err != nil { - t.Fatal(err) - } - return exec.ErrNotFound + if err := os.WriteFile(filepath.Join(dir, ".prettierignore"), []byte("ignored.js\n"), 0o644); err != nil { + t.Fatal(err) } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ - "path": "a.go", "content": "requested\n", - }) - if result.Status != StatusOK { - t.Fatalf("write status = %s: %s", result.Status, result.Output) + specialPath := filepath.Join(dir, "special.js") + if err := os.WriteFile(specialPath, []byte("const x = \"a\";\n"), 0o644); err != nil { + t.Fatal(err) } - if got := result.FileDiffs; len(got) != 1 || got[0].NewText != "requested\n" { - t.Fatalf("formatter-failure FileDiff = %#v", got) + write := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "special.js", + "content": "const x = \"a\";\n", + "overwrite": true, + }, RunOptions{}) + if write.Status != StatusOK { + t.Fatalf("write_file failed: %q", write.Output) } -} - -func TestEditFileUsesVerifiedBytesAfterFormatterFailure(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - if err := os.WriteFile(targetPath, []byte("before\n"), 0o644); err != nil { + onDisk, err := os.ReadFile(specialPath) + if err != nil { t.Fatal(err) } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(_ context.Context, _ string, _ []string, _ string) error { - if err := os.WriteFile(targetPath, []byte("formatter-mutated\n"), 0o644); err != nil { - t.Fatal(err) - } - return exec.ErrNotFound - } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - - result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ - "path": "a.go", "old_string": "before", "new_string": "requested", - }) - if result.Status != StatusOK { - t.Fatalf("edit status = %s: %s", result.Status, result.Output) + if !strings.Contains(string(onDisk), "const x = 'a';") { + t.Fatalf("write_file must resolve .prettierrc against the destination filename, got %q", onDisk) } - if got := result.FileDiffs; len(got) != 1 || got[0].OldText != "before\n" || got[0].NewText != "requested\n" { - t.Fatalf("formatter-failure edit FileDiff = %#v", got) + if strings.Contains(string(onDisk), "\"a\"") { + t.Fatalf("write_file published the unformatted double-quoted content %q", onDisk) } -} -func TestWriteFileOmitsRichDiffWhenFormatterFinalReadFails(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - priorRunner := runFormatOnWriteCommand - priorReader := readFormattedFile - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } - readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } - t.Cleanup(func() { - runFormatOnWriteCommand = priorRunner - readFormattedFile = priorReader - }) - - result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ - "path": "a.go", "content": "requested\n", - }) - if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 { - t.Fatalf("unverified formatter result = status=%s changed=%#v diffs=%#v", result.Status, result.ChangedFiles, result.FileDiffs) - } - if result.Display.Preview != "" { - t.Fatalf("unverified formatter result exposed stale preview: %q", result.Display.Preview) + // The input has no spaces around "=", so default Prettier would rewrite it + // to `const x = "a";`. Honouring .prettierignore means those bytes are + // published unchanged; against the staging name they would not be ignored. + ignoredInput := "const x=\"a\";\n" + ignored := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "ignored.js", + "content": ignoredInput, + }, RunOptions{}) + if ignored.Status != StatusOK { + t.Fatalf("write_file ignored.js failed: %q", ignored.Output) } -} - -func TestEditFileOmitsPreviewWhenFormatterFinalReadFails(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - if err := os.WriteFile(targetPath, []byte("before\n"), 0o644); err != nil { + ignoredOnDisk, err := os.ReadFile(filepath.Join(dir, "ignored.js")) + if err != nil { t.Fatal(err) } - priorRunner := runFormatOnWriteCommand - priorReader := readFormattedFile - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { return nil } - readFormattedFile = func(*os.Root, string) ([]byte, os.FileInfo, error) { return nil, nil, os.ErrPermission } - t.Cleanup(func() { - runFormatOnWriteCommand = priorRunner - readFormattedFile = priorReader - }) - - result := NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ - "path": "a.go", "old_string": "before", "new_string": "requested", - }) - if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 { - t.Fatalf("unverified formatter result = status=%s changed=%#v diffs=%#v", result.Status, result.ChangedFiles, result.FileDiffs) - } - if result.Display.Preview != "" { - t.Fatalf("unverified formatter result exposed stale preview: %q", result.Display.Preview) + if string(ignoredOnDisk) != ignoredInput { + t.Fatalf("write_file must honour .prettierignore for the destination filename, got %q", ignoredOnDisk) } } -func TestWriteFileOmitsRichEvidenceWhenFormatterReplacesTargetWithOutOfRootSymlink(t *testing.T) { +const uglyGoSource = "package a\n\nfunc A( ) { }\n" + +func TestFormatOnWritePublishesFormattedBytesForWriteAndEdit(t *testing.T) { requireGofmt(t) t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - resolvedRoot, err := filepath.EvalSymlinks(root) - if err != nil { - t.Fatal(err) - } - trackedPath := filepath.Join(resolvedRoot, "a.go") - outsidePath := filepath.Join(t.TempDir(), "outside.go") - outsideContent := "package external\n\nconst Secret = \"outside\"\n" - if err := os.WriteFile(outsidePath, []byte(outsideContent), 0o644); err != nil { - t.Fatal(err) - } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { - if err := os.Remove(targetPath); err != nil { - t.Fatal(err) - } - if err := os.Symlink(outsidePath, targetPath); err != nil { - t.Skipf("symlinks unavailable: %v", err) - } - return nil - } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) + dir := t.TempDir() tracker := NewFileTracker() - diagnosticsCalled := false - result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", "content": "package requested\n", - }, RunOptions{FileTracker: tracker, Diagnostics: func(context.Context, string) string { - diagnosticsCalled = true - return "must not run" - }}) - if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 || result.Display.Preview != "" { - t.Fatalf("out-of-root formatter result = status=%s changed=%#v diffs=%#v preview=%q output=%q", result.Status, result.ChangedFiles, result.FileDiffs, result.Display.Preview, result.Output) + write := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + }, RunOptions{FileTracker: tracker}) + if write.Status != StatusOK { + t.Fatalf("write_file failed: %q", write.Output) } - if diagnosticsCalled { - t.Fatal("diagnostics must not inspect an unverified formatter target") + assertFormattedOnDiskAndTracked(t, tracker, filepath.Join(dir, "a.go"), write.Display.Preview) + + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) } - if _, tracked := tracker.Version(trackedPath); tracked { - t.Fatal("out-of-root formatter target must not be recorded in the tracker") + edit := NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func A() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + if edit.Status != StatusOK { + t.Fatalf("edit_file failed: %q", edit.Output) } -} - -func TestEditFileOmitsRichEvidenceWhenFormatterReplacesTargetWithOutOfRootSymlink(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - resolvedRoot, err := filepath.EvalSymlinks(root) + onDisk, err := os.ReadFile(filepath.Join(dir, "a.go")) if err != nil { t.Fatal(err) } - trackedPath := filepath.Join(resolvedRoot, "a.go") - if err := os.WriteFile(targetPath, []byte("package before\n"), 0o644); err != nil { - t.Fatal(err) - } - outsidePath := filepath.Join(t.TempDir(), "outside.go") - outsideContent := "package external\n\nconst Secret = \"outside\"\n" - if err := os.WriteFile(outsidePath, []byte(outsideContent), 0o644); err != nil { - t.Fatal(err) + if !strings.Contains(string(onDisk), "func B() {") { + t.Fatalf("edit_file must publish formatted bytes, got %q", onDisk) } - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { - if err := os.Remove(targetPath); err != nil { - t.Fatal(err) - } - if err := os.Symlink(outsidePath, targetPath); err != nil { - t.Skipf("symlinks unavailable: %v", err) - } - return nil + assertTrackerMatchesDisk(t, tracker, filepath.Join(dir, "a.go")) + if !strings.Contains(edit.Display.Preview, "func B() {") && !strings.Contains(edit.Display.Preview, "func B()") { + t.Fatalf("edit_file preview must reflect formatted bytes, got %q", edit.Display.Preview) } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - tracker := NewFileTracker() - read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", - }, RunOptions{FileTracker: tracker}) - if read.Status != StatusOK { - t.Fatalf("read before edit failed: %s", read.Output) +} + +// A FORMATTER FAILURE ON THE STAGING COPY MUST NOT REACH THE DESTINATION. +// +// The physical formatter is handed a private staging path, so even a run that +// scribbles "PARTIAL" and exits non-zero cannot rewrite the destination. The +// tool path then publishes the fallback (unformatted) staged bytes, never the +// formatter's partial output and never through a direct destination write. +func TestFormatOnWriteFailureLeavesDestinationIntactUntilPublish(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - diagnosticsCalled := false - result := NewScopedEditFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", "old_string": "before", "new_string": "requested", - }, RunOptions{FileTracker: tracker, Diagnostics: func(context.Context, string) string { - diagnosticsCalled = true - return "must not run" - }}) - if result.Status != StatusOK || len(result.ChangedFiles) != 1 || len(result.FileDiffs) != 0 || result.Display.Preview != "" { - t.Fatalf("out-of-root formatter result = status=%s changed=%#v diffs=%#v preview=%q output=%q", result.Status, result.ChangedFiles, result.FileDiffs, result.Display.Preview, result.Output) + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "a.go") + original := "package a\n\nfunc Original() {}\n" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + probeSaw := filepath.Join(dir, "probe-saw") + installFakeGofmt(t, `#!/bin/sh +if [ -n "$ZERO_FORMAT_PROBE" ]; then + cp "$ZERO_FORMAT_PROBE" "$ZERO_FORMAT_PROBE_SAW" || true +fi +path= +for a in "$@"; do path="$a"; done +printf 'PARTIAL' > "$path" +exit 1 +`) + t.Setenv("ZERO_FORMAT_PROBE", target) + t.Setenv("ZERO_FORMAT_PROBE_SAW", probeSaw) + + tracker := NewFileTracker() + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) + } + + var result Result + wantPublished := "" + switch toolName { + case "write_file": + wantPublished = uglyGoSource + result = NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + "overwrite": true, + }, RunOptions{FileTracker: tracker}) + default: + wantPublished = "package a\n\nfunc B( ) { }\n" + result = NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func Original() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + } + if result.Status != StatusOK { + t.Fatalf("%s failed: %q", toolName, result.Output) + } + + saw, err := os.ReadFile(probeSaw) + if err != nil { + t.Fatalf("formatter never observed the destination: %v", err) + } + if string(saw) != original { + t.Fatalf("formatter observed %q, want the previous destination %q", saw, original) + } + onDisk, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(onDisk) == "PARTIAL" { + t.Fatalf("%s left formatter-partial bytes on the destination", toolName) + } + if strings.Contains(string(onDisk), "PARTIAL") { + t.Fatalf("%s published formatter-partial bytes: %q", toolName, onDisk) + } + // The failed formatter must not have scribbled on the destination: + // the fallback staged bytes are what gets published. + if string(onDisk) != wantPublished { + t.Fatalf("%s destination = %q, want the fallback staged bytes %q", toolName, onDisk, wantPublished) + } + assertTrackerMatchesDisk(t, tracker, target) + }) } - if diagnosticsCalled { - t.Fatal("diagnostics must not inspect an unverified formatter target") +} + +func TestFormatOnWriteRefusesWhenDestinationChangesDuringFormat(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") } - if _, tracked := tracker.Version(trackedPath); tracked { - t.Fatal("out-of-root formatter target must not be recorded in the tracker") + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "a.go") + original := "package a\n\nfunc Original() {}\n" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + ready := filepath.Join(dir, "fmt-ready") + release := filepath.Join(dir, "fmt-release") + installFakeGofmt(t, `#!/bin/sh +: > "$ZERO_FORMAT_READY" +while [ ! -f "$ZERO_FORMAT_RELEASE" ]; do sleep 0.01; done +exit 0 +`) + t.Setenv("ZERO_FORMAT_READY", ready) + t.Setenv("ZERO_FORMAT_RELEASE", release) + + tracker := NewFileTracker() + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) + } + + resultCh := make(chan Result, 1) + go func() { + switch toolName { + case "write_file": + resultCh <- NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + "overwrite": true, + }, RunOptions{FileTracker: tracker}) + default: + resultCh <- NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func Original() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + } + }() + + waitForFile(t, ready) + external := "package a\n\nfunc External() {}\n" + if err := os.WriteFile(target, []byte(external), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(release, nil, 0o644); err != nil { + t.Fatal(err) + } + + var result Result + select { + case result = <-resultCh: + case <-time.After(15 * time.Second): + t.Fatal("tool did not return after the formatter was released") + } + if result.Status != StatusError || !strings.Contains(result.Output, "changed on disk") { + t.Fatalf("%s must refuse when the destination changed during formatting, got %q", toolName, result.Output) + } + onDisk, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(onDisk) != external { + t.Fatalf("external bytes were clobbered: got %q, want %q", onDisk, external) + } + }) } } -func TestWriteFileAcceptsInRootAtomicFormatterReplacement(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - formatted := "package formatted\n" - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { - tempPath := filepath.Join(root, "formatter.tmp") - if err := os.WriteFile(tempPath, []byte(formatted), 0o644); err != nil { - t.Fatal(err) +func waitForFile(t *testing.T, path string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(path); err == nil { + return + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for %s", path) } - return os.Rename(tempPath, targetPath) + time.Sleep(10 * time.Millisecond) } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - tracker := NewFileTracker() +} - result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", "content": "package requested\n", - }, RunOptions{FileTracker: tracker}) - if result.Status != StatusOK || len(result.FileDiffs) != 1 || result.FileDiffs[0].NewText != formatted { - t.Fatalf("atomic formatter write = status=%s diffs=%#v output=%q", result.Status, result.FileDiffs, result.Output) - } - resolvedRoot, err := filepath.EvalSymlinks(root) +func assertFormattedOnDiskAndTracked(t *testing.T, tracker *FileTracker, path, preview string) { + t.Helper() + onDisk, err := os.ReadFile(path) if err != nil { t.Fatal(err) } - version, tracked := tracker.Version(filepath.Join(resolvedRoot, "a.go")) - if !tracked || version.Hash != HashContent([]byte(formatted)) { - t.Fatalf("atomic formatter tracker = %#v, tracked=%t", version, tracked) + if !strings.Contains(string(onDisk), "func A() {") { + t.Fatalf("expected gofmt-formatted content, got %q", onDisk) + } + assertTrackerMatchesDisk(t, tracker, path) + if preview != "" && !strings.Contains(preview, "func A() {") && !strings.Contains(preview, "func A()") { + t.Fatalf("preview must reflect formatted bytes, got %q", preview) } } -func TestEditFileAcceptsInRootAtomicFormatterReplacement(t *testing.T) { - requireGofmt(t) - t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - root := t.TempDir() - targetPath := filepath.Join(root, "a.go") - if err := os.WriteFile(targetPath, []byte("package before\n"), 0o644); err != nil { +func assertTrackerMatchesDisk(t *testing.T, tracker *FileTracker, path string) { + t.Helper() + onDisk, err := os.ReadFile(path) + if err != nil { t.Fatal(err) } - formatted := "package formatted\n" - priorRunner := runFormatOnWriteCommand - runFormatOnWriteCommand = func(context.Context, string, []string, string) error { - tempPath := filepath.Join(root, "formatter.tmp") - if err := os.WriteFile(tempPath, []byte(formatted), 0o644); err != nil { - t.Fatal(err) + trackedPath := path + if resolved, err := filepath.EvalSymlinks(path); err == nil { + if _, ok := tracker.Version(resolved); ok { + trackedPath = resolved } - return os.Rename(tempPath, targetPath) } - t.Cleanup(func() { runFormatOnWriteCommand = priorRunner }) - tracker := NewFileTracker() - read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", - }, RunOptions{FileTracker: tracker}) - if read.Status != StatusOK { - t.Fatalf("read before edit failed: %s", read.Output) + version, ok := tracker.Version(trackedPath) + if !ok { + t.Fatalf("tracker has no version for %s", trackedPath) } - - result := NewScopedEditFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ - "path": "a.go", "old_string": "before", "new_string": "requested", - }, RunOptions{FileTracker: tracker}) - if result.Status != StatusOK || len(result.FileDiffs) != 1 || result.FileDiffs[0].NewText != formatted { - t.Fatalf("atomic formatter edit = status=%s diffs=%#v output=%q", result.Status, result.FileDiffs, result.Output) + if got, want := version.Hash, HashContent(onDisk); got != want { + t.Fatalf("tracker hash %s does not match on-disk bytes (hash %s)", got, want) } - resolvedRoot, err := filepath.EvalSymlinks(root) - if err != nil { +} + +func installFakeGofmt(t *testing.T, script string) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "gofmt") + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { t.Fatal(err) } - version, tracked := tracker.Version(filepath.Join(resolvedRoot, "a.go")) - if !tracked || version.Hash != HashContent([]byte(formatted)) { - t.Fatalf("atomic formatter tracker = %#v, tracked=%t", version, tracked) + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func TestFormatOnWriteRuffUsesLogicalDestinationForWriteAndEdit(t *testing.T) { + if _, err := exec.LookPath("ruff"); err != nil { + t.Skip("ruff not installed") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "ruff.toml"), []byte("force-exclude = true\n[format]\nexclude = [\"special.py\"]\n"), 0o600); err != nil { + t.Fatal(err) + } + for _, name := range []string{"special.py", "adjacent.py"} { + t.Run(name, func(t *testing.T) { + original := "x= [1,2]\n" + target := filepath.Join(dir, name) + if err := os.WriteFile(target, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + tracker := NewFileTracker() + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{"path": name}, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file: %s", read.Output) + } + input := original + want := "x = [1, 2]\n" + var result Result + if toolName == "write_file" { + result = NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{"path": name, "content": input, "overwrite": true}, RunOptions{FileTracker: tracker}) + } else { + input = "y= [3,4]\n" + want = "y = [3, 4]\n" + result = NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{"path": name, "old_string": original, "new_string": input}, RunOptions{FileTracker: tracker}) + } + if result.Status != StatusOK { + t.Fatalf("%s: %s", toolName, result.Output) + } + if name == "special.py" { + want = input + } + got, err := os.ReadFile(target) + if err != nil || string(got) != want { + t.Fatalf("%s logical filename lost: got %q, want %q, err %v", toolName, got, want, err) + } + assertTrackerMatchesDisk(t, tracker, target) + }) + } + }) } } diff --git a/internal/tools/format_on_write_timeout_test.go b/internal/tools/format_on_write_timeout_test.go index b66fbba70..fe15d5caa 100644 --- a/internal/tools/format_on_write_timeout_test.go +++ b/internal/tools/format_on_write_timeout_test.go @@ -177,67 +177,104 @@ func shortenFormatOnWriteTimeout(t *testing.T, timeout time.Duration) { t.Cleanup(func() { formatOnWriteTimeout = previous }) } -// A FAILED FORMATTER MUST NOT LEAVE THE FILE HALF-REWRITTEN. +// A FORMATTER RUNS ON A PRIVATE STAGING COPY, NEVER ON THE DESTINATION. // -// These commands edit in place, so one killed by the deadline, killed by the -// caller, or exiting partway through its own rewrite can leave the target -// truncated: neither the input nor the output. Returning the written bytes on -// top of that would leave the tracker baseline and the diff preview describing -// a file that is not on disk, and the next edit would compare against content -// the file does not have. -func TestFormatOnWriteRestoresTheFileWhenTheFormatterFails(t *testing.T) { - installFakeFormatter(t, ".clobberfmt", "clobberfmt", clobberingFormatterScript()) +// Physical formatters are handed a path inside an owner-only sibling directory +// (fsutil.CreatePrivateTempDir), so a killed, cancelled, or clobbering run can +// neither truncate nor half-rewrite the destination. A failure on that staging +// copy therefore keeps the fallback bytes the caller staged, and the +// destination stays byte-for-byte (and inode-for-inode) untouched until the +// single atomic publication in commitFileContents. +func TestFormatOnWriteRunsTheFormatterOnAPrivateCopy(t *testing.T) { + argRecord := filepath.Join(t.TempDir(), "formatter-arg") + installFakeFormatter(t, ".clobberfmt", "clobberfmt", recordingClobberingFormatterScript()) t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + t.Setenv("ZERO_FORMAT_ARG_RECORD", argRecord) + requireFormatterClobbers(t, "the bytes the caller wrote\n") - target := filepath.Join(t.TempDir(), "subject.clobberfmt") + dir := t.TempDir() + target := filepath.Join(dir, "subject.clobberfmt") const written = "the bytes the caller wrote\n" if err := os.WriteFile(target, []byte(written), 0o644); err != nil { t.Fatal(err) } - requireFormatterClobbers(t, written) + before, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } formatting := maybeFormatWrittenFile(context.Background(), target, written) + recorded, err := os.ReadFile(argRecord) + if err != nil { + t.Fatalf("formatter never recorded the path it was handed: %v", err) + } + handed := strings.TrimSpace(string(recorded)) + if handed == target { + t.Fatalf("formatter was handed the destination path %q", handed) + } + if base := filepath.Base(filepath.Dir(handed)); !strings.HasPrefix(base, ".zero-fmt-") { + t.Fatalf("formatter path %q is not inside a private staging directory", handed) + } + + after, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if !os.SameFile(before, after) { + t.Fatal("destination inode changed while the formatter ran on the staging copy") + } onDisk, err := os.ReadFile(target) if err != nil { t.Fatal(err) } if string(onDisk) != written { - t.Errorf("file on disk = %q, want the bytes that were written back", onDisk) + t.Errorf("destination = %q, want the untouched bytes %q", onDisk, written) } if formatting.Content != written { - t.Errorf("content = %q, want the bytes that were written", formatting.Content) - } - if formatting.RestoreFailed { - t.Error("restoration was reported as failed on a writable file") + t.Errorf("content = %q, want the fallback bytes that were written", formatting.Content) } } -// And the same for a run cut off by the deadline, which is the case the notice -// already covers: the disclosure and the file have to agree. -func TestFormatOnWriteRestoresTheFileOnTimeout(t *testing.T) { +// The same isolation must hold on the deadline path, which is the one the +// timeout notice already covers: the disclosure and the file have to agree. +func TestFormatOnWriteKeepsFallbackAndDestinationOnTimeout(t *testing.T) { installFakeFormatter(t, ".clobberfmt", "clobberfmt", clobberingFormatterScript()) shortenFormatOnWriteTimeout(t, time.Nanosecond) t.Setenv("ZERO_FORMAT_ON_WRITE", "1") - target := filepath.Join(t.TempDir(), "subject.clobberfmt") + dir := t.TempDir() + target := filepath.Join(dir, "subject.clobberfmt") const written = "the bytes the caller wrote\n" if err := os.WriteFile(target, []byte(written), 0o644); err != nil { t.Fatal(err) } - requireFormatterClobbers(t, written) + before, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } formatting := maybeFormatWrittenFile(context.Background(), target, written) + after, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if !os.SameFile(before, after) { + t.Fatal("a timed-out formatter replaced the destination inode") + } onDisk, err := os.ReadFile(target) if err != nil { t.Fatal(err) } if string(onDisk) != written { - t.Errorf("file on disk = %q, want the bytes that were written back", onDisk) + t.Errorf("destination = %q, want the untouched bytes %q", onDisk, written) + } + if formatting.Content != written { + t.Errorf("content = %q, want the fallback bytes that were written", formatting.Content) } if !formatting.TimedOut { - t.Error("the deadline path stopped being reported once restoration was added") + t.Error("the deadline path stopped being reported once formatting moved off the destination") } if notice := formatting.notice("subject.clobberfmt"); !strings.Contains(notice, "not formatted") { t.Errorf("notice = %q, want the timeout note", notice) @@ -245,7 +282,7 @@ func TestFormatOnWriteRestoresTheFileOnTimeout(t *testing.T) { } // clobberingFormatterScript truncates the file it is handed and then fails, the -// way an interrupted in-place formatter leaves a partial rewrite. +// way an interrupted physical formatter would if it were handed the file. func clobberingFormatterScript() string { if runtime.GOOS == "windows" { return "@echo off\r\necho CLOBBERED> %1\r\nexit /b 3\r\n" @@ -253,6 +290,16 @@ func clobberingFormatterScript() string { return "#!/bin/sh\necho CLOBBERED > \"$1\"\nexit 3\n" } +// recordingClobberingFormatterScript is clobberingFormatterScript plus a record +// of the path it was handed, so a test can prove that path is the private +// staging copy and not the destination. +func recordingClobberingFormatterScript() string { + if runtime.GOOS == "windows" { + return "@echo off\r\nif defined ZERO_FORMAT_ARG_RECORD echo %1> \"%ZERO_FORMAT_ARG_RECORD%\"\r\necho CLOBBERED> %1\r\nexit /b 3\r\n" + } + return "#!/bin/sh\nprintf '%s' \"$1\" > \"$ZERO_FORMAT_ARG_RECORD\"\necho CLOBBERED > \"$1\"\nexit 3\n" +} + // requireFormatterClobbers proves the fixture really does damage the file it is // handed, on a throwaway copy. // diff --git a/internal/tools/format_on_write_windows_test.go b/internal/tools/format_on_write_windows_test.go new file mode 100644 index 000000000..99a5dadab --- /dev/null +++ b/internal/tools/format_on_write_windows_test.go @@ -0,0 +1,125 @@ +//go:build windows + +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +func setFormatterTestDACL(t *testing.T, path, sddl string) { + t.Helper() + descriptor, err := windows.SecurityDescriptorFromString(sddl) + if err != nil { + t.Fatal(err) + } + acl, _, err := descriptor.DACL() + if err != nil { + t.Fatal(err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, nil, nil, acl, nil); err != nil { + t.Fatal(err) + } +} + +func requirePrivateFormatterDACL(t *testing.T, path string) { + t.Helper() + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatal(err) + } + user, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatal(err) + } + sddl := descriptor.String() + if strings.Contains(sddl, ";;;WD)") || !strings.Contains(sddl, ";;;"+user.User.Sid.String()+")") { + t.Fatalf("formatter staging DACL is not current-user-only: %s: %s", path, sddl) + } +} + +// The real subprocess verifies the copy and an auxiliary file created by the +// formatter, so a restrictive initial file alone cannot make the test pass. +func TestPrivateFormatterDACLHelper(t *testing.T) { + if os.Getenv("ZERO_PRIVATE_FORMAT_HELPER") != "1" { + return + } + path := os.Args[len(os.Args)-1] + requirePrivateFormatterDACL(t, path) + requirePrivateFormatterDACL(t, filepath.Dir(path)) + auxiliary := filepath.Join(filepath.Dir(path), "formatter-auxiliary") + if err := os.WriteFile(auxiliary, []byte("sensitive formatter scratch"), 0o666); err != nil { + t.Fatal(err) + } + requirePrivateFormatterDACL(t, auxiliary) + if err := os.WriteFile(os.Getenv("ZERO_PRIVATE_FORMAT_PROOF"), []byte("checked"), 0o600); err != nil { + t.Fatal(err) + } + if os.Getenv("ZERO_PRIVATE_FORMAT_FAIL") == "1" { + t.Fatal("injected formatter failure") + } + if err := os.WriteFile(path, []byte("formatted"), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestFormatOnWriteProtectsWindowsStagingThroughoutFormatter(t *testing.T) { + user, err := windows.GetCurrentProcessToken().GetTokenUser() + if err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + const extension = ".privateprobe" + previous, present := formatterCommands[extension] + formatterCommands[extension] = []string{executable, "-test.run=^TestPrivateFormatterDACLHelper$", "--"} + defer func() { + if present { + formatterCommands[extension] = previous + } else { + delete(formatterCommands, extension) + } + }() + for _, fail := range []bool{false, true} { + t.Run(map[bool]string{false: "success", true: "failure"}[fail], func(t *testing.T) { + dir := t.TempDir() + setFormatterTestDACL(t, dir, "D:P(A;OICI;FA;;;WD)") + target := filepath.Join(dir, "restricted"+extension) + if err := os.WriteFile(target, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + setFormatterTestDACL(t, target, "D:P(A;;FA;;;"+user.User.Sid.String()+")") + proof := filepath.Join(t.TempDir(), "proof") + t.Setenv("ZERO_PRIVATE_FORMAT_HELPER", "1") + t.Setenv("ZERO_PRIVATE_FORMAT_PROOF", proof) + t.Setenv("ZERO_PRIVATE_FORMAT_FAIL", map[bool]string{false: "0", true: "1"}[fail]) + result := maybeFormatWrittenFile(context.Background(), target, "secret") + want := "formatted" + if fail { + want = "secret" + } + if result.Content != want { + t.Fatalf("formatter result = %q, want %q", result.Content, want) + } + if got, err := os.ReadFile(proof); err != nil || string(got) != "checked" { + t.Fatalf("formatter protection was not exercised: %q, %v", got, err) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "original" { + t.Fatalf("destination changed: %q, %v", got, err) + } + requirePrivateFormatterDACL(t, target) + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-fmt-*")) + if err != nil || len(leftovers) != 0 { + t.Fatalf("formatter staging leaked: %v, %v", leftovers, err) + } + }) + } +} diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 937d0c02a..d9a441542 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -113,36 +113,28 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } + modelKnownContent := content + // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Format staged bytes, then + // publish once. Recording pre-format content would make the next edit look + // like an external modification and trip the conflict guard; formatting the + // destination in place after publication would reintroduce partial writes. + formatting := maybeFormatWrittenFile(ctx, absolutePath, content) + content = formatting.Content + finalContentKnown := true + var expectedContent *string if priorContentKnown { expectedContent = &priorContent } - if err := commitFileContents(absolutePath, priorInfo, expectedContent, content); err != nil { + cleanupWarning, err := commitFileContents(absolutePath, priorInfo, expectedContent, content) + if err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } - modelKnownContent := content - // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the - // FileTracker baseline: recording pre-format content would make the very - // next edit look like an external modification and trip the conflict guard. - restoreMode := os.FileMode(0o644) - if priorInfo != nil { - restoreMode = priorInfo.Mode().Perm() - } - formatting := maybeFormatWrittenFileScoped(ctx, tool.workspaceRoot, tool.scope, absolutePath, content, restoreMode) - content = formatting.Content - finalContentKnown := formatting.ContentKnown // Baseline the freshly written content so a later edit/overwrite in this // session compares against what is now on disk. - newInfo := formatting.Info - if newInfo == nil { - newInfo, _ = os.Stat(absolutePath) - } - if finalContentKnown { - options.FileTracker.Record(absolutePath, []byte(content), newInfo) - } else { - options.FileTracker.Forget(absolutePath) - } - if finalContentKnown && content == modelKnownContent { + newInfo, _ := os.Stat(absolutePath) + options.FileTracker.Record(absolutePath, []byte(content), newInfo) + if content == modelKnownContent { options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content)) } if !existed { @@ -161,9 +153,10 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } summary := fmt.Sprintf("%s %s (%d lines).", verb, relativePath, lines) summary += formatting.notice(relativePath) - if finalContentKnown { - summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) + if cleanupWarning != "" { + summary += " " + cleanupWarning } + summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} // Do not pretend an unreadable overwrite was a creation. The write may be From 44b763e1450e3ab06995b9cc0643b466c54cc233 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Mon, 21 Sep 2026 12:27:45 +0200 Subject: [PATCH 2/4] fix(tools): address review feedback on atomic writes, formatter lifetime, and SID validation Bureau: desk-pr-941 --- internal/fsutil/rename.go | 7 + internal/fsutil/rename_acl_darwin_test.go | 14 +- internal/tools/file_commit.go | 5 + internal/tools/format_on_write.go | 170 ++++++++++-------- internal/tools/format_on_write_test.go | 92 ++++++++++ .../tools/format_on_write_timeout_test.go | 60 ++++++- .../tools/format_on_write_windows_test.go | 31 +++- 7 files changed, 299 insertions(+), 80 deletions(-) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index f1a7e7aea..67053def5 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -42,6 +42,13 @@ var stagingProtectionObserver func(stagingPath string) // symlink destination replaces the symlink itself with the new regular file. On Windows, // ReplaceFileW refuses symlink destinations outright and returns an error. // Hard links to destination files are broken by design (temp-and-rename publishes a new inode). +// +// On Windows, replacing a destination that another process holds open without +// delete sharing fails with a sharing violation ("being used by another +// process"). ReplaceWithRetry retries briefly for a holder that is only passing +// through, but a long-lived handle (Go's os.Open, Python's open) makes the +// replacement fail closed: the old bytes stay and no temporary file is left +// behind. func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return writeFileAtomic(filename, data, perm, nil) } diff --git a/internal/fsutil/rename_acl_darwin_test.go b/internal/fsutil/rename_acl_darwin_test.go index c4f191670..264f6636a 100644 --- a/internal/fsutil/rename_acl_darwin_test.go +++ b/internal/fsutil/rename_acl_darwin_test.go @@ -92,7 +92,7 @@ func TestPreserveNativeACLRemovesInheritedACLWhenSourceHasNone(t *testing.T) { t.Fatalf("clear ACL: %s: %v", out, err) } if out, err := exec.Command("chmod", "+a", "user:nobody allow read,file_inherit,directory_inherit", dir).CombinedOutput(); err != nil { - t.Fatalf("parent ACL: %s: %v", out, err) + t.Skipf("parent ACL unavailable on this filesystem: %s: %v", out, err) } // Exercise preservation directly so private creation cannot mask a nil/no-op bug. staging, err := os.CreateTemp(dir, "inherited-*") @@ -101,8 +101,14 @@ func TestPreserveNativeACLRemovesInheritedACLWhenSourceHasNone(t *testing.T) { } defer staging.Close() before, err := exec.Command("ls", "-le", staging.Name()).CombinedOutput() - if err != nil || !strings.Contains(string(before), "nobody allow read") { - t.Fatalf("inherited grant missing: %s, %v", before, err) + if err != nil { + t.Fatalf("ls -le: %v\n%s", err, before) + } + if !strings.Contains(string(before), "nobody allow read") { + // The precondition is filesystem inheritance, not the helper under + // test: a host whose filesystem does not apply file_inherit entries to + // new files cannot exercise the absence case at all. + t.Skipf("filesystem did not inherit the directory ACL onto a new file; ls -le:\n%s", before) } if err := preserveNativeACL(staging, target); err != nil { t.Fatal(err) @@ -131,7 +137,7 @@ func TestPreserveNativeACLRemovesInheritedACLWhenSourceHasNone(t *testing.T) { func TestPrivateStagingDarwinCreationSuppressesInheritance(t *testing.T) { dir := t.TempDir() if out, err := exec.Command("chmod", "+a", "user:nobody allow read,file_inherit,directory_inherit", dir).CombinedOutput(); err != nil { - t.Fatalf("parent ACL: %s: %v", out, err) + t.Skipf("parent ACL unavailable on this filesystem: %s: %v", out, err) } count := 0 previous := privateCreationObserver diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index 36533eb82..e7ac42e71 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -30,6 +30,11 @@ var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat( // destination (invariant #921). The exclusive-create branch refuses a path that // appeared after the caller observed it missing. // +// The binding covers observation through the pre-publication check only. This +// handle is closed before publishFileContents replaces the path, so a swap +// between the check and the replace is overwritten rather than refused; that +// window is inherent to temp-and-replace. +// // The returned warning string is non-empty only when the replacement already // committed but its backup cleanup failed; the caller reports success and // surfaces the warning. A non-nil error means nothing was published. diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index fe821d7d3..33655e2a1 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -65,77 +65,105 @@ func (result formatOnWriteResult) notice(relativePath string) string { ". The file holds exactly what was written, so a project format check may still flag it." } -// formatterCommands maps a file extension to the formatter argv; the file path -// is appended for physical-file adapters. Ruff and Prettier instead receive -// the logical destination with stdin. Physical adapters keep the original -// basename inside an owner-only sibling directory (including auxiliary files), -// and retain the destination directory as their working directory. A missing -// binary silently skips formatting. -var formatterCommands = map[string][]string{ - ".go": {"gofmt", "-w"}, - ".rs": {"rustfmt"}, - ".py": {"ruff", "format", "--quiet"}, - ".ts": {"prettier", "--log-level", "silent", "--write"}, - ".tsx": {"prettier", "--log-level", "silent", "--write"}, - ".js": {"prettier", "--log-level", "silent", "--write"}, - ".jsx": {"prettier", "--log-level", "silent", "--write"}, - ".json": {"prettier", "--log-level", "silent", "--write"}, - ".css": {"prettier", "--log-level", "silent", "--write"}, - ".scss": {"prettier", "--log-level", "silent", "--write"}, - ".html": {"prettier", "--log-level", "silent", "--write"}, - ".md": {"prettier", "--log-level", "silent", "--write"}, - ".yaml": {"prettier", "--log-level", "silent", "--write"}, - ".yml": {"prettier", "--log-level", "silent", "--write"}, - ".zig": {"zig", "fmt"}, - ".dart": {"dart", "format"}, - ".tf": {"terraform", "fmt"}, - ".gleam": {"gleam", "format"}, - ".sh": {"shfmt", "-w"}, - ".bash": {"shfmt", "-w"}, - ".c": {"clang-format", "-i"}, - ".h": {"clang-format", "-i"}, - ".cpp": {"clang-format", "-i"}, - ".hpp": {"clang-format", "-i"}, - ".cc": {"clang-format", "-i"}, - ".kt": {"ktlint", "-F"}, - ".swift": {"swiftformat"}, - ".lua": {"stylua"}, +// formatterAdapter describes how one formatter is run. +// +// STDIN ADAPTERS READ THE WRITTEN BYTES ON STDIN AND PRINT THE FORMATTED RESULT +// TO STDOUT. That route lets filenameFlag carry the logical destination path, +// so any rule written against the file's own path (.clang-format-ignore, an +// .editorconfig section, rustfmt's ignore) still resolves against the file the +// caller named instead of a random staging name. Physical adapters have no +// such option: they are handed a private staging copy inside an owner-only +// sibling directory (including auxiliary files) and keep the destination +// directory as their working directory. +type formatterAdapter struct { + argv []string + // stdin selects the stdin/stdout route. When false the private staging path + // is appended to argv. + stdin bool + // filenameFlag is the option that supplies the logical destination to a + // stdin adapter. It is emitted as "--flag=" and empty when the + // formatter resolves its configuration from the working directory alone. + filenameFlag string +} + +// formatterCommands maps a file extension to its formatter. A missing binary +// silently skips formatting. +var formatterCommands = map[string]formatterAdapter{ + ".go": {argv: []string{"gofmt"}, stdin: true}, + ".rs": {argv: []string{"rustfmt"}, stdin: true}, + ".py": {argv: []string{"ruff", "format", "--quiet"}, stdin: true, filenameFlag: "--stdin-filename"}, + ".ts": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".tsx": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".js": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".jsx": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".json": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".css": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".scss": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".html": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".md": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".yaml": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".yml": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, + ".zig": {argv: []string{"zig", "fmt", "--stdin"}, stdin: true}, + ".dart": {argv: []string{"dart", "format"}, stdin: true, filenameFlag: "--stdin-name"}, + ".tf": {argv: []string{"terraform", "fmt", "-"}, stdin: true}, + ".gleam": {argv: []string{"gleam", "format", "--stdin"}, stdin: true}, + ".sh": {argv: []string{"shfmt"}, stdin: true, filenameFlag: "--filename"}, + ".bash": {argv: []string{"shfmt"}, stdin: true, filenameFlag: "--filename"}, + ".c": {argv: []string{"clang-format"}, stdin: true, filenameFlag: "--assume-filename"}, + ".h": {argv: []string{"clang-format"}, stdin: true, filenameFlag: "--assume-filename"}, + ".cpp": {argv: []string{"clang-format"}, stdin: true, filenameFlag: "--assume-filename"}, + ".hpp": {argv: []string{"clang-format"}, stdin: true, filenameFlag: "--assume-filename"}, + ".cc": {argv: []string{"clang-format"}, stdin: true, filenameFlag: "--assume-filename"}, + ".kt": {argv: []string{"ktlint", "-F", "--stdin"}, stdin: true, filenameFlag: "--stdin-path"}, + ".swift": {argv: []string{"swiftformat"}, stdin: true, filenameFlag: "--stdinpath"}, + ".lua": {argv: []string{"stylua", "-"}, stdin: true, filenameFlag: "--stdin-filepath"}, } +// formatterCommandObserver, when non-nil, receives each formatter command after +// its lifetime bounds are applied. Tests use it to assert that a wedged +// formatter can be killed and cannot block Wait past the deadline. +var formatterCommandObserver func(*exec.Cmd) + // formatOnWriteEnabled reports whether the opt-in env toggle is set. func formatOnWriteEnabled() bool { value := strings.TrimSpace(os.Getenv("ZERO_FORMAT_ON_WRITE")) return value != "" && value != "0" && !strings.EqualFold(value, "false") } -// maybeFormatWrittenFile runs the configured formatter on an isolated copy of -// writtenContent (when enabled and on PATH) and returns the bytes to publish. -// The destination path is never opened or rewritten here. Best-effort -// throughout: any failure — no formatter, formatter error, timeout, unreadable -// result — returns writtenContent so the caller's state matches the last write -// it performed itself. Only the timeout is reported back, for the reason on +// maybeFormatWrittenFile runs the configured formatter over the written bytes +// (when enabled and on PATH) and returns the bytes to publish. Stdin adapters +// receive the bytes on stdin; physical adapters get a private staging copy. The +// destination path is never opened or rewritten here. Best-effort throughout: +// any failure — no formatter, formatter error, timeout, unreadable result — +// returns writtenContent so the caller's state matches the last write it +// performed itself. Only the timeout is reported back, for the reason on // formatOnWriteResult. func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} if !formatOnWriteEnabled() { return unformatted } - ext := strings.ToLower(filepath.Ext(absolutePath)) - command, ok := formatterCommands[ext] + adapter, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))] if !ok { return unformatted } - binaryPath, err := exec.LookPath(command[0]) + binaryPath, err := exec.LookPath(adapter.argv[0]) if err != nil { return unformatted } - dir := filepath.Dir(absolutePath) - if command[0] == "prettier" { - return formatWithPrettier(ctx, command[0], binaryPath, command[1:], absolutePath, writtenContent) - } - if command[0] == "ruff" { - return formatWithStdin(ctx, command[0], binaryPath, append(append([]string(nil), command[1:]...), "--stdin-filename", absolutePath, "-"), absolutePath, writtenContent) + if adapter.stdin { + return formatWithStdin(ctx, adapter, binaryPath, absolutePath, writtenContent) } + return formatWithStaging(ctx, adapter, binaryPath, absolutePath, writtenContent) +} + +// formatWithStaging runs a physical formatter on a private copy of the written +// bytes, never on the destination. The copy keeps the destination's basename +// so filename-derived formatter behaviour still applies, and any scribble from +// a killed or failing run lands there, not in the user's file. +func formatWithStaging(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent string) formatOnWriteResult { + unformatted := formatOnWriteResult{Content: writtenContent} + dir := filepath.Dir(absolutePath) stagingDir, err := fsutil.CreatePrivateTempDir(dir, ".zero-fmt-*") if err != nil { return unformatted @@ -156,12 +184,16 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon } formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() - arguments := append(append([]string(nil), command[1:]...), stagingName) + arguments := append(append([]string(nil), adapter.argv[1:]...), stagingName) formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) formatter.Dir = dir formatter.Stdin = strings.NewReader("") + hardenProcessLifetime(formatter) + if formatterCommandObserver != nil { + formatterCommandObserver(formatter) + } if err := formatter.Run(); err != nil { - unformatted.Formatter = command[0] + unformatted.Formatter = adapter.argv[0] if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { unformatted.TimedOut = true } @@ -171,34 +203,32 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon if err != nil { return unformatted } - return formatOnWriteResult{Content: string(formatted), Formatter: command[0]} -} - -// formatWithPrettier preserves config overrides and ignore rules by transmitting -// the logical destination alongside stdin, without physical destination writes. -func formatWithPrettier(ctx context.Context, formatterName, binaryPath string, formatterArgs []string, absolutePath, writtenContent string) formatOnWriteResult { - arguments := make([]string, 0, len(formatterArgs)+2) - for _, arg := range formatterArgs { - if arg == "--write" { - continue - } - arguments = append(arguments, arg) - } - arguments = append(arguments, "--stdin-filepath", absolutePath) - return formatWithStdin(ctx, formatterName, binaryPath, arguments, absolutePath, writtenContent) + return formatOnWriteResult{Content: string(formatted), Formatter: adapter.argv[0]} } -func formatWithStdin(ctx context.Context, formatterName, binaryPath string, arguments []string, absolutePath, writtenContent string) formatOnWriteResult { +// formatWithStdin runs a formatter over stdin, supplies the logical +// destination through the adapter's filename flag when it has one, and reads +// the formatted bytes from stdout. The empty-output guard keeps a formatter +// that ignores the path (and so prints nothing) from publishing an empty file. +func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() + arguments := append([]string(nil), adapter.argv[1:]...) + if adapter.filenameFlag != "" { + arguments = append(arguments, adapter.filenameFlag+"="+absolutePath) + } formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) formatter.Dir = filepath.Dir(absolutePath) formatter.Stdin = strings.NewReader(writtenContent) + hardenProcessLifetime(formatter) + if formatterCommandObserver != nil { + formatterCommandObserver(formatter) + } var stdout bytes.Buffer formatter.Stdout = &stdout if err := formatter.Run(); err != nil { - unformatted.Formatter = formatterName + unformatted.Formatter = adapter.argv[0] if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { unformatted.TimedOut = true } @@ -209,5 +239,5 @@ func formatWithStdin(ctx context.Context, formatterName, binaryPath string, argu // path on some CLI versions) must not publish an empty file. return unformatted } - return formatOnWriteResult{Content: stdout.String(), Formatter: formatterName} + return formatOnWriteResult{Content: stdout.String(), Formatter: adapter.argv[0]} } diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 9bcda8fc6..9a5d386bc 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -225,6 +225,98 @@ func TestFormatOnWritePrettierUsesDestinationFilename(t *testing.T) { } } +// PATH-KEYED RULES MUST RESOLVE AGAINST THE FILE THE CALLER NAMED. +// +// A stdin adapter receives the logical destination through its filename flag, +// so a .clang-format-ignore pattern, an .editorconfig section, or rustfmt's +// ignore still matches "vendor/lib.hintfmt" rather than a random staging name. +func TestFormatOnWritePassesLogicalDestinationToStdinFormatter(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + record := filepath.Join(t.TempDir(), "formatter-args") + installFakeStdinFormatter(t, ".hintfmt", "hintfmt", "--assume-filename", record) + + dir := t.TempDir() + target := filepath.Join(dir, "vendor", "lib.hintfmt") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(target, []byte("original\n"), 0o644); err != nil { + t.Fatal(err) + } + + formatting := maybeFormatWrittenFile(context.Background(), target, "written\n") + if formatting.Content != "written\n" { + t.Fatalf("stdin adapter echoed %q, want the written bytes", formatting.Content) + } + recorded, err := os.ReadFile(record) + if err != nil { + t.Fatalf("formatter never recorded its arguments: %v", err) + } + args := string(recorded) + want := "--assume-filename=" + target + if !strings.Contains(args, want) { + t.Fatalf("formatter args %q do not carry the logical destination %q", args, want) + } + if strings.Contains(args, ".zero-fmt-") { + t.Fatalf("formatter args %q leaked a staging path instead of the destination", args) + } +} + +// THE PRODUCTION TABLE MUST CARRY THE HINTS, NOT ONLY THE TEST SEAM. +func TestFormatterFilenameHints(t *testing.T) { + want := map[string]string{ + ".c": "--assume-filename", + ".h": "--assume-filename", + ".cpp": "--assume-filename", + ".hpp": "--assume-filename", + ".cc": "--assume-filename", + ".sh": "--filename", + ".bash": "--filename", + ".lua": "--stdin-filepath", + ".swift": "--stdinpath", + ".kt": "--stdin-path", + ".dart": "--stdin-name", + } + for ext, flag := range want { + adapter, ok := formatterCommands[ext] + if !ok { + t.Fatalf("no formatter adapter for %s", ext) + } + if !adapter.stdin { + t.Errorf("%s adapter is not on the stdin route, so its filename hint cannot apply", ext) + } + if adapter.filenameFlag != flag { + t.Errorf("%s filenameFlag = %q, want %q", ext, adapter.filenameFlag, flag) + } + } +} + +// installFakeStdinFormatter puts a stdin formatter on PATH and registers it for +// the test's duration. The script records its argv and echoes stdin, so a test +// can prove which logical path the adapter was handed. +func installFakeStdinFormatter(t *testing.T, extension, name, filenameFlag, record string) { + t.Helper() + directory := t.TempDir() + script := "#!/bin/sh\nprintf '%s' \"$*\" > \"$ZERO_FORMAT_ARG_RECORD\"\ncat\n" + if err := os.WriteFile(filepath.Join(directory, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("ZERO_FORMAT_ARG_RECORD", record) + previous, existed := formatterCommands[extension] + formatterCommands[extension] = formatterAdapter{argv: []string{name}, stdin: true, filenameFlag: filenameFlag} + t.Cleanup(func() { + if existed { + formatterCommands[extension] = previous + return + } + delete(formatterCommands, extension) + }) +} + const uglyGoSource = "package a\n\nfunc A( ) { }\n" func TestFormatOnWritePublishesFormattedBytesForWriteAndEdit(t *testing.T) { diff --git a/internal/tools/format_on_write_timeout_test.go b/internal/tools/format_on_write_timeout_test.go index fe15d5caa..4ae92f8fa 100644 --- a/internal/tools/format_on_write_timeout_test.go +++ b/internal/tools/format_on_write_timeout_test.go @@ -125,6 +125,64 @@ func TestFormatOnWriteStaysQuietWhenTheFormatterFails(t *testing.T) { } } +// A WEDGED FORMATTER MUST BE KILLABLE AND MUST NOT BLOCK PAST THE DEADLINE. +// +// formatWithStdin captures stdout, so Run waits for every holder of that pipe, +// not only the process the deadline kills. On Windows an npm-installed prettier +// is a .cmd hosting node.exe, and a grandchild that keeps the pipe open would +// push the return past the timeout. hardenProcessLifetime installs the tree +// kill and the WaitDelay backstop on both routes; without it the command has +// neither. +func TestFormatOnWriteHardensFormatterLifetime(t *testing.T) { + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + + for _, stdin := range []bool{true, false} { + name := map[bool]string{true: "stdin", false: "staging"}[stdin] + t.Run(name, func(t *testing.T) { + target := filepath.Join(t.TempDir(), "subject.padfmt") + installFakeAdapter(t, ".padfmt", "padfmt", stdin) + + var seen *exec.Cmd + previous := formatterCommandObserver + formatterCommandObserver = func(command *exec.Cmd) { seen = command } + defer func() { formatterCommandObserver = previous }() + + _ = maybeFormatWrittenFile(context.Background(), target, "written\n") + + if seen == nil { + t.Fatal("no formatter command was observed") + } + if seen.WaitDelay == 0 { + t.Error("formatter command has no WaitDelay, so a leaked grandchild can block Wait past the deadline") + } + if seen.Cancel == nil { + t.Error("formatter command has no Cancel, so the deadline cannot kill the formatter tree") + } + }) + } +} + +// installFakeAdapter registers a formatter for the test's duration. The stdin +// flag chooses the stdin/stdout route over the private staging copy. +func installFakeAdapter(t *testing.T, extension, name string, stdin bool) { + t.Helper() + binaryName := name + formatterScriptExtension() + directory := t.TempDir() + if err := os.WriteFile(filepath.Join(directory, binaryName), []byte(succeedingFormatterScript()), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + previous, existed := formatterCommands[extension] + formatterCommands[extension] = formatterAdapter{argv: []string{binaryName}, stdin: stdin} + t.Cleanup(func() { + if existed { + formatterCommands[extension] = previous + return + } + delete(formatterCommands, extension) + }) +} + // installFakeFormatter puts a formatter on PATH and registers it in the command // table for the test's duration, returning the binary name the notice carries. func installFakeFormatter(t *testing.T, extension, name, script string) string { @@ -137,7 +195,7 @@ func installFakeFormatter(t *testing.T, extension, name, script string) string { t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) previous, existed := formatterCommands[extension] - formatterCommands[extension] = []string{binaryName} + formatterCommands[extension] = formatterAdapter{argv: []string{binaryName}} t.Cleanup(func() { if existed { formatterCommands[extension] = previous diff --git a/internal/tools/format_on_write_windows_test.go b/internal/tools/format_on_write_windows_test.go index 99a5dadab..fad4c7d50 100644 --- a/internal/tools/format_on_write_windows_test.go +++ b/internal/tools/format_on_write_windows_test.go @@ -6,8 +6,8 @@ import ( "context" "os" "path/filepath" - "strings" "testing" + "unsafe" "golang.org/x/sys/windows" ) @@ -33,13 +33,34 @@ func requirePrivateFormatterDACL(t *testing.T, path string) { if err != nil { t.Fatal(err) } + if descriptor == nil { + t.Fatalf("formatter staging has no security descriptor: %s", path) + } user, err := windows.GetCurrentProcessToken().GetTokenUser() if err != nil { t.Fatal(err) } - sddl := descriptor.String() - if strings.Contains(sddl, ";;;WD)") || !strings.Contains(sddl, ";;;"+user.User.Sid.String()+")") { - t.Fatalf("formatter staging DACL is not current-user-only: %s: %s", path, sddl) + acl, _, err := descriptor.DACL() + if err != nil { + t.Fatal(err) + } + if acl == nil || acl.AceCount == 0 { + t.Fatalf("formatter staging DACL has no entries: %s", path) + } + // Compare each ACE's SID with the process token's SID. SDDL prints + // well-known accounts as two-letter aliases (the hosted runner's built-in + // administrator is "LA"), so a textual match against descriptor.String() + // is not portable; EqualSid resolves the alias. + for i := uint32(0); i < uint32(acl.AceCount); i++ { + var ace *windows.ACCESS_ALLOWED_ACE + if err := windows.GetAce(acl, i, &ace); err != nil { + t.Fatal(err) + } + sid := (*windows.SID)(unsafe.Pointer(&ace.SidStart)) + if windows.EqualSid(sid, user.User.Sid) { + continue + } + t.Fatalf("formatter staging DACL grants access to a SID other than the current user: %s", path) } } @@ -80,7 +101,7 @@ func TestFormatOnWriteProtectsWindowsStagingThroughoutFormatter(t *testing.T) { } const extension = ".privateprobe" previous, present := formatterCommands[extension] - formatterCommands[extension] = []string{executable, "-test.run=^TestPrivateFormatterDACLHelper$", "--"} + formatterCommands[extension] = formatterAdapter{argv: []string{executable, "-test.run=^TestPrivateFormatterDACLHelper$", "--"}} defer func() { if present { formatterCommands[extension] = previous From e93871d871899f5012605478b046785d19ca03d6 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Tue, 22 Sep 2026 10:08:26 +0200 Subject: [PATCH 3/4] fix(tools): address review findings on exclusive-create, symlinks, SELinux, and format scope - Close exclusive-create race via WriteFileAtomicExclusive using link(2) on Unix and MoveFileEx without replace on Windows - Enforce explicit symlink target refusal in write_file, edit_file, and commitFileContents to prevent link destruction - Fail closed on unrecoverable SELinux and xattr preservation failures in preserveXattrs - Restore maybeFormatWrittenFileScoped and confine formatter execution directory to authorized write roots - Add regression coverage for all four review findings across fsutil and tools packages --- internal/fsutil/rename.go | 57 ++++++++++++++ internal/fsutil/rename_test.go | 45 +++++++++++ internal/fsutil/rename_xattr_unix.go | 39 +++++---- internal/fsutil/rename_xattr_unix_test.go | 96 +++++++++++++++++------ internal/fsutil/replace_other.go | 8 ++ internal/fsutil/replace_windows.go | 36 +++++++++ internal/tools/edit_file.go | 11 ++- internal/tools/file_commit.go | 53 +++++++++++-- internal/tools/file_commit_test.go | 63 +++++++++++++++ internal/tools/format_on_write.go | 83 ++++++++++++++++---- internal/tools/format_on_write_test.go | 86 ++++++++++++++++++++ internal/tools/write_file.go | 7 +- internal/tools/write_tools_test.go | 42 ++++++++++ 13 files changed, 564 insertions(+), 62 deletions(-) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 67053def5..87a80d076 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -53,6 +53,63 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return writeFileAtomic(filename, data, perm, nil) } +// WriteFileAtomicExclusive creates filename with data and fails if the path +// already exists or appears concurrently. Unlike WriteFileAtomic it never +// replaces a pre-existing object (regular file, symlink, or special file): the +// destination is staged and synced like WriteFileAtomic, then published with an +// atomic no-replace primitive, so a racing creator is refused with an error +// wrapping os.ErrExist rather than overwritten. Readers never observe a partial +// destination. This is the fail-closed equivalent of O_CREATE|O_EXCL for a +// caller that must not clobber a file it did not observe. +func WriteFileAtomicExclusive(filename string, data []byte, perm os.FileMode) error { + return writeFileAtomicExclusive(filename, data, perm) +} + +// exclusiveBeforePublish, when non-nil, runs after the staging file has been +// synced and closed but before the no-replace publication. Tests use it to +// prove that a file appearing in that window is refused, not overwritten. +var exclusiveBeforePublish func(filename string) + +func writeFileAtomicExclusive(filename string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(filename) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + + tmpFile, err := createTempFile(dir, perm) + if err != nil { + return err + } + tmpName := tmpFile.Name() + closed := false + defer func() { + if !closed { + _ = tmpFile.Close() + } + _ = os.Remove(tmpName) + }() + + if _, err := tmpFile.Write(data); err != nil { + return err + } + if err := tmpFile.Sync(); err != nil { + return err + } + closed = true + if err := tmpFile.Close(); err != nil { + return err + } + + if exclusiveBeforePublish != nil { + exclusiveBeforePublish(filename) + } + if err := publishExclusive(tmpName, filename); err != nil { + return err + } + syncDir(dir) + return nil +} + // The replacement dependency is local to the call, so failure tests need no global hook. func writeFileAtomic(filename string, data []byte, perm os.FileMode, replace func(string, string) error) error { dir := filepath.Dir(filename) diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index 8d03325b6..c4e18af65 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -221,3 +221,48 @@ func TestWriteFileAtomicLeavesDestinationOnReplaceFailure(t *testing.T) { t.Fatalf("staging leftovers: %v, %v", leftovers, err) } } + +func TestWriteFileAtomicExclusiveCreatesWithoutReplacing(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "created.txt") + + if err := WriteFileAtomicExclusive(target, []byte("first"), 0o644); err != nil { + t.Fatalf("exclusive create: %v", err) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "first" { + t.Fatalf("created content = %q, err=%v", got, err) + } + + if err := WriteFileAtomicExclusive(target, []byte("second"), 0o644); !errors.Is(err, os.ErrExist) { + t.Fatalf("existing-destination error = %v, want os.ErrExist", err) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "first" { + t.Fatalf("exclusive create overwrote the existing file: %q, err=%v", got, err) + } +} + +// The no-replace decision must be atomic: a file appearing after the staging +// copy is synced and closed is refused by the publication primitive rather than +// overwritten. This is the window the create branch previously lost. +func TestWriteFileAtomicExclusiveRefusesConcurrentAppearance(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "raced.txt") + previous := exclusiveBeforePublish + exclusiveBeforePublish = func(filename string) { + if err := os.WriteFile(filename, []byte("other writer"), 0o644); err != nil { + t.Fatal(err) + } + } + defer func() { exclusiveBeforePublish = previous }() + + if err := WriteFileAtomicExclusive(target, []byte("zero"), 0o644); !errors.Is(err, os.ErrExist) { + t.Fatalf("raced create error = %v, want os.ErrExist", err) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer" { + t.Fatalf("raced create overwrote the competitor: %q, err=%v", got, err) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil || len(leftovers) != 0 { + t.Fatalf("staging leftovers: %v, %v", leftovers, err) + } +} diff --git a/internal/fsutil/rename_xattr_unix.go b/internal/fsutil/rename_xattr_unix.go index eac309734..6e1ce3d3e 100644 --- a/internal/fsutil/rename_xattr_unix.go +++ b/internal/fsutil/rename_xattr_unix.go @@ -12,8 +12,29 @@ import ( const posixACLAccessXattr = "system.posix_acl_access" +// xattrListFunc, xattrGetFunc, xattrSetFunc and xattrRemoveFunc are seams for +// the four permitter syscalls so a test can exercise the fail-closed contract +// without a host that actually denies the operation. Production uses the real +// syscalls. +var ( + xattrListFunc = listXattrs + xattrGetFunc = getXattr + xattrSetFunc = func(fd int, name string, data []byte, flags int) error { + return unix.Fsetxattr(fd, name, data, flags) + } + xattrRemoveFunc = func(fd int, name string) error { + return unix.Fremovexattr(fd, name) + } +) + +// preserveXattrs copies every extended attribute of srcPath onto f. The copy is +// fail-closed: any attribute that cannot be listed, read, or set aborts the +// replacement and leaves the destination unchanged. There is deliberately no +// best-effort exception, not even for security.selinux: WriteFileAtomic's +// contract is that the destination's authorization metadata is either +// preserved in full or the call fails. func preserveXattrs(f *os.File, srcPath string) error { - names, err := listXattrs(srcPath) + names, err := xattrListFunc(srcPath) if err != nil { if isXattrUnsupported(err) { return nil @@ -25,22 +46,19 @@ func preserveXattrs(f *os.File, srcPath string) error { if name == posixACLAccessXattr { hasAccessACL = true } - data, err := getXattr(srcPath, name) + data, err := xattrGetFunc(srcPath, name) if err != nil { if isXattrUnsupported(err) { continue } return fmt.Errorf("fsutil: reading xattr %s from %s: %w", name, srcPath, err) } - if err := unix.Fsetxattr(int(f.Fd()), name, data, 0); err != nil { - if name == "security.selinux" && isSELinuxPolicyDenial(err) { - continue - } + if err := xattrSetFunc(int(f.Fd()), name, data, 0); err != nil { return fmt.Errorf("fsutil: preserving xattr %s: %w", name, err) } } if !hasAccessACL { - if err := unix.Fremovexattr(int(f.Fd()), posixACLAccessXattr); err != nil { + if err := xattrRemoveFunc(int(f.Fd()), posixACLAccessXattr); err != nil { if !isXattrNotFound(err) && !isXattrUnsupported(err) { return fmt.Errorf("fsutil: removing inherited ACL from replacement: %w", err) } @@ -49,13 +67,6 @@ func preserveXattrs(f *os.File, srcPath string) error { return nil } -func isSELinuxPolicyDenial(err error) bool { - return errors.Is(err, unix.EACCES) || - errors.Is(err, unix.EPERM) || - errors.Is(err, unix.ENOTSUP) || - errors.Is(err, unix.EOPNOTSUPP) -} - func listXattrs(path string) ([]string, error) { dest := []byte(nil) for { diff --git a/internal/fsutil/rename_xattr_unix_test.go b/internal/fsutil/rename_xattr_unix_test.go index abe717819..6ef92b977 100644 --- a/internal/fsutil/rename_xattr_unix_test.go +++ b/internal/fsutil/rename_xattr_unix_test.go @@ -3,33 +3,83 @@ package fsutil import ( - "fmt" + "errors" + "os" + "path/filepath" + "strings" "testing" "golang.org/x/sys/unix" ) -func TestSELinuxPolicyDenialClassification(t *testing.T) { - cases := []struct { - name string - err error - want bool - }{ - {"nil", nil, false}, - {"eacces", unix.EACCES, true}, - {"eperm", unix.EPERM, true}, - {"enotsup", unix.ENOTSUP, true}, - {"eopnotsupp", unix.EOPNOTSUPP, true}, - {"eio", unix.EIO, false}, - {"enospc", unix.ENOSPC, false}, - {"wrapped eacces", fmt.Errorf("setxattr: %w", unix.EACCES), true}, - {"wrapped eio", fmt.Errorf("setxattr: %w", unix.EIO), false}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - if got := isSELinuxPolicyDenial(tc.err); got != tc.want { - t.Fatalf("isSELinuxPolicyDenial(%v) = %v, want %v", tc.err, got, tc.want) - } - }) +// swapXattrSeams replaces the xattr syscall seams for the duration of a test +// and restores them on cleanup. +func swapXattrSeams(t *testing.T) { + t.Helper() + priorList, priorGet, priorSet, priorRemove := xattrListFunc, xattrGetFunc, xattrSetFunc, xattrRemoveFunc + t.Cleanup(func() { + xattrListFunc, xattrGetFunc, xattrSetFunc, xattrRemoveFunc = priorList, priorGet, priorSet, priorRemove + }) +} + +// A denied xattr set is fatal even for security.selinux. The previous code +// swallowed EACCES/EPERM for that name, which let WriteFileAtomic publish a +// replacement whose SELinux label had been silently dropped. +func TestPreserveXattrsFailsClosedOnSELinuxDenial(t *testing.T) { + swapXattrSeams(t) + xattrListFunc = func(string) ([]string, error) { return []string{"security.selinux"}, nil } + xattrGetFunc = func(string, string) ([]byte, error) { + return []byte("system_u:object_r:etc_t:s0"), nil + } + xattrSetFunc = func(int, string, []byte, int) error { return unix.EACCES } + + file, err := os.CreateTemp(t.TempDir(), "xattr-*") + if err != nil { + t.Fatal(err) + } + defer file.Close() + + err = preserveXattrs(file, "source") + if err == nil { + t.Fatal("a denied security.selinux set must fail closed") + } + if !errors.Is(err, unix.EACCES) { + t.Fatalf("error %v does not wrap EACCES", err) + } +} + +// A set failure on any attribute aborts the atomic replacement, removes the +// staging file, and leaves the destination bytes intact. +func TestWriteFileAtomicFailsClosedWhenXattrPreservationFails(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.WriteFile(target, []byte("original"), 0o600); err != nil { + t.Fatal(err) + } + if err := unix.Setxattr(target, "user.zero_test", []byte("value"), 0); err != nil { + t.Skipf("user xattrs unavailable: %v", err) + } + + swapXattrSeams(t) + xattrSetFunc = func(int, string, []byte, int) error { return unix.EACCES } + + err := WriteFileAtomic(target, []byte("replacement"), 0o600) + if err == nil { + t.Fatal("WriteFileAtomic must fail when authorization metadata cannot be preserved") + } + if !errors.Is(err, unix.EACCES) { + t.Fatalf("error %v does not wrap EACCES", err) + } + if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "original" { + t.Fatalf("destination mutated to %q, readErr=%v", got, readErr) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasPrefix(entry.Name(), ".zero-tmp-") { + t.Fatalf("staging file %q was left behind", entry.Name()) + } } } diff --git a/internal/fsutil/replace_other.go b/internal/fsutil/replace_other.go index 35a985518..77690009a 100644 --- a/internal/fsutil/replace_other.go +++ b/internal/fsutil/replace_other.go @@ -10,3 +10,11 @@ import "os" func replaceExisting(src, dst string) error { return os.Rename(src, dst) } + +// publishExclusive publishes src as a new dst without replacing anything. +// link(2) is atomic and fails with EEXIST when dst exists, so a destination +// that appeared concurrently is refused rather than overwritten. The caller +// removes the now-duplicated temporary afterwards. +func publishExclusive(src, dst string) error { + return os.Link(src, dst) +} diff --git a/internal/fsutil/replace_windows.go b/internal/fsutil/replace_windows.go index 7cf6be44c..a5c54b98c 100644 --- a/internal/fsutil/replace_windows.go +++ b/internal/fsutil/replace_windows.go @@ -47,8 +47,41 @@ const ( var ( replaceKernel32 = syscall.NewLazyDLL("kernel32.dll") replaceProcReplaceFil = replaceKernel32.NewProc("ReplaceFileW") + moveFileExProc = replaceKernel32.NewProc("MoveFileExW") ) +// publishExclusive publishes src as a new dst without replacing anything. With +// no MOVEFILE_REPLACE_EXISTING flag, MoveFileExW fails when dst exists (with +// ERROR_ALREADY_EXISTS or ERROR_FILE_EXISTS, which syscall.Errno maps to +// os.ErrExist), so a destination that appeared concurrently is refused rather +// than overwritten. MoveFileExW removes src on success, so the caller's +// deferred temporary cleanup becomes a no-op. +func publishExclusive(src, dst string) error { + source, err := syscall.UTF16PtrFromString(src) + if err != nil { + return err + } + target, err := syscall.UTF16PtrFromString(dst) + if err != nil { + return err + } + result, _, callErr := moveFileExProc.Call( + uintptr(unsafe.Pointer(source)), + uintptr(unsafe.Pointer(target)), + 0, + ) + if result != 0 { + return nil + } + if callErr == nil || errors.Is(callErr, syscall.Errno(0)) { + return fmt.Errorf("create %s: MoveFileExW failed", dst) + } + if errors.Is(callErr, os.ErrExist) { + return &os.PathError{Op: "create", Path: dst, Err: os.ErrExist} + } + return callErr +} + // replaceExisting publishes src over dst with ReplaceFileW rather than // MoveFileEx (what os.Rename uses) to preserve destination metadata. The // replacement is a freshly created temporary file, so it carries the directory's @@ -87,6 +120,9 @@ func replaceExistingWithCleanup(src, dst string, replace func(string, string, st if info.Mode()&os.ModeSymlink != 0 { return fmt.Errorf("refusing to replace symlink destination: %s", dst) } + if !info.Mode().IsRegular() { + return fmt.Errorf("%w: %s", ErrNonRegularDestination, dst) + } backup, err := prepareReplaceBackup(dst) if err != nil { diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index d5408a166..59bcff096 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -64,6 +64,13 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err != nil { return errorResult("Error reading " + requestedPath + ": " + err.Error()) } + if info, lerr := os.Lstat(absolutePath); lerr == nil { + if info.Mode()&os.ModeSymlink != 0 { + return errorResult("Error: " + relativePath + " is a symbolic link. Editing through a symlink is not allowed; target the real file instead.") + } + } else if !os.IsNotExist(lerr) { + return errorResult("Error reading " + relativePath + ": " + lerr.Error()) + } contentBytes, err := os.ReadFile(absolutePath) if err != nil { return errorResult("Error reading " + relativePath + ": " + err.Error()) @@ -80,7 +87,7 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } } content := string(contentBytes) - priorInfo, err := os.Stat(absolutePath) + priorInfo, err := os.Lstat(absolutePath) if err != nil { return errorResult("Error reading " + relativePath + ": " + err.Error()) } @@ -158,7 +165,7 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any return errorResult("Error writing " + relativePath + ": " + err.Error()) } modelKnownContent := updated - formatting := maybeFormatWrittenFile(ctx, absolutePath, updated) + formatting := maybeFormatWrittenFileScoped(ctx, tool.workspaceRoot, tool.scope, absolutePath, updated) updated = formatting.Content finalContentKnown := true diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index e7ac42e71..6e79af39e 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -10,11 +10,24 @@ import ( var errFileChangedDuringWrite = errors.New("file changed on disk before the write committed") +// errSymlinkDestination refuses any write whose destination is, or has become, +// a symbolic link. Observation and validation inspect the link (Lstat) and the +// publication must replace the very object it validated: replacing the symlink +// itself would strand the pointed-to file with stale bytes while destroying the +// link. Failing closed is the only consistent contract. +var errSymlinkDestination = errors.New("refusing to write through a symbolic link") + // fileWriteBeforeCommit is a deterministic test hook. Production leaves it // nil; tests use it to replace a path after observation but before opening the // object that will actually be mutated. var fileWriteBeforeCommit func(path string) +// fileCreateBeforeExclusivePublish is a deterministic test hook. It runs after +// commitFileContents has observed the path missing but before the exclusive +// publication, so a test can prove a file appearing in that window is refused +// rather than overwritten. +var fileCreateBeforeExclusivePublish func(path string) + // fileWriteStat is a deterministic test seam for proving that the opened-file // identity is captured before the final preimage comparison. Production uses // the file descriptor directly. @@ -28,7 +41,9 @@ var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat( // mutation: a path replacement between observation and commit therefore fails // instead of publishing stale content, and no reader ever observes a truncated // destination (invariant #921). The exclusive-create branch refuses a path that -// appeared after the caller observed it missing. +// appeared after the caller observed it missing, and publishes with an atomic +// no-replace primitive so a racing creator is refused rather than overwritten. +// A symlink destination is refused in both branches. // // The binding covers observation through the pre-publication check only. This // handle is closed before publishFileContents replaces the path, so a swap @@ -43,13 +58,23 @@ func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent * fileWriteBeforeCommit(path) } + pathInfo, statErr := os.Lstat(path) + if statErr == nil { + if pathInfo.Mode()&os.ModeSymlink != 0 { + return "", errSymlinkDestination + } + } else if !os.IsNotExist(statErr) { + return "", statErr + } + if expectedInfo == nil { - if _, err := os.Lstat(path); err == nil { + if statErr == nil { return "", errFileChangedDuringWrite - } else if !os.IsNotExist(err) { - return "", err } - return publishFileContents(path, content) + if fileCreateBeforeExclusivePublish != nil { + fileCreateBeforeExclusivePublish(path) + } + return publishFileContentsExclusive(path, content) } file, err := os.OpenFile(path, fileCommitOpenFlags(expectedContent), 0) @@ -65,8 +90,8 @@ func commitFileContents(path string, expectedInfo os.FileInfo, expectedContent * _ = file.Close() return "", errFileChangedDuringWrite } - pathInfo, err := os.Stat(path) - if err != nil || !os.SameFile(openedInfo, pathInfo) { + pathInfo, err = os.Lstat(path) + if err != nil || pathInfo.Mode()&os.ModeSymlink != 0 || !os.SameFile(openedInfo, pathInfo) { _ = file.Close() return "", errFileChangedDuringWrite } @@ -112,3 +137,17 @@ func publishFileContents(path, content string) (string, error) { } return "", err } + +// publishFileContentsExclusive creates the destination with an atomic no-replace +// publication. A destination that already exists, or that appears concurrently, +// is reported as errFileChangedDuringWrite instead of being overwritten. +func publishFileContentsExclusive(path, content string) (string, error) { + err := fsutil.WriteFileAtomicExclusive(path, []byte(content), 0o644) + if err == nil { + return "", nil + } + if errors.Is(err, os.ErrExist) { + return "", errFileChangedDuringWrite + } + return "", err +} diff --git a/internal/tools/file_commit_test.go b/internal/tools/file_commit_test.go index 8347412fc..f73f6b4b1 100644 --- a/internal/tools/file_commit_test.go +++ b/internal/tools/file_commit_test.go @@ -2,6 +2,7 @@ package tools import ( "context" + "errors" "os" "path/filepath" "strings" @@ -15,6 +16,68 @@ func installFileWriteRace(t *testing.T, mutate func(string)) { t.Cleanup(func() { fileWriteBeforeCommit = prior }) } +func installFileCreateRace(t *testing.T, mutate func(string)) { + t.Helper() + prior := fileCreateBeforeExclusivePublish + fileCreateBeforeExclusivePublish = mutate + t.Cleanup(func() { fileCreateBeforeExclusivePublish = prior }) +} + +// The window named by the review: the path was observed missing by +// commitFileContents's own Lstat, then a file appeared before the exclusive +// publication. The creation must be refused, not silently converted into an +// overwrite. +func TestCommitFileContentsRefusesCreateRaceAfterObservationLstat(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "created.txt") + installFileCreateRace(t, func(path string) { + if err := os.WriteFile(path, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + + warning, err := commitFileContents(target, nil, nil, "zero\n") + if err == nil || !errors.Is(err, errFileChangedDuringWrite) { + t.Fatalf("raced create = warning %q, error %v; want errFileChangedDuringWrite", warning, err) + } + if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "other writer\n" { + t.Fatalf("raced create content = %q, err=%v", got, readErr) + } +} + +// commitFileContents must bind its publication to the object it validated. A +// symlink observed as its target (what the old Stat-based observation produced) +// must be refused, leaving both the link and the pointed-to file untouched. +func TestCommitFileContentsRefusesSymlinkDestination(t *testing.T) { + dir := t.TempDir() + real := filepath.Join(dir, "real.txt") + if err := os.WriteFile(real, []byte("target bytes\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + observed, err := os.Stat(link) + if err != nil { + t.Fatal(err) + } + + if _, err := commitFileContents(link, observed, strPtr("target bytes\n"), "new bytes\n"); err == nil { + t.Fatal("commit must refuse a symlink destination") + } + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatal("symlink destination was replaced by a regular file") + } + if got, err := os.ReadFile(real); err != nil || string(got) != "target bytes\n" { + t.Fatalf("symlink target mutated: %q, err=%v", got, err) + } +} + func installFileWriteStat(t *testing.T, stat func(*os.File) (os.FileInfo, error)) { t.Helper() prior := fileWriteStat diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index 33655e2a1..1d62ad0e5 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -11,6 +12,7 @@ import ( "time" "github.com/Gitlawb/zero/internal/fsutil" + "github.com/Gitlawb/zero/internal/sandbox" ) // Format-on-write for the mutating file tools. When enabled, a successful @@ -130,15 +132,22 @@ func formatOnWriteEnabled() bool { return value != "" && value != "0" && !strings.EqualFold(value, "false") } -// maybeFormatWrittenFile runs the configured formatter over the written bytes -// (when enabled and on PATH) and returns the bytes to publish. Stdin adapters -// receive the bytes on stdin; physical adapters get a private staging copy. The -// destination path is never opened or rewritten here. Best-effort throughout: -// any failure — no formatter, formatter error, timeout, unreadable result — -// returns writtenContent so the caller's state matches the last write it -// performed itself. Only the timeout is reported back, for the reason on -// formatOnWriteResult. +// maybeFormatWrittenFile is the unscoped test-facing wrapper. Production +// callers use maybeFormatWrittenFileScoped so the formatter cannot operate +// outside the configured write roots. func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenContent string) formatOnWriteResult { + return maybeFormatWrittenFileScoped(ctx, filepath.Dir(absolutePath), nil, absolutePath, writtenContent) +} + +// maybeFormatWrittenFileScoped runs the configured formatter over a transient +// copy of the written bytes (stdin, or a private staging file) and returns the +// bytes to publish. The destination is never opened or rewritten here. If +// absolutePath does not resolve inside one of the scope's write roots, the +// formatting is refused and the written bytes pass through unchanged: a +// formatter must not be able to read or write outside the same roots the tool +// itself is confined to. The formatter's working directory is pinned inside the +// matched root for the same reason. +func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, scope PathScope, absolutePath string, writtenContent string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} if !formatOnWriteEnabled() { return unformatted @@ -151,19 +160,65 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon if err != nil { return unformatted } + root, relativePath, err := openFormattedFileRoot(workspaceRoot, scope, absolutePath) + if err != nil { + return unformatted + } + defer root.Close() + workDir := filepath.Join(root.Name(), filepath.Dir(relativePath)) if adapter.stdin { - return formatWithStdin(ctx, adapter, binaryPath, absolutePath, writtenContent) + return formatWithStdin(ctx, adapter, binaryPath, absolutePath, writtenContent, workDir) + } + return formatWithStaging(ctx, adapter, binaryPath, absolutePath, writtenContent, workDir) +} + +// openFormattedFileRoot resolves absolutePath against the scope's write roots +// and opens the first root that contains it. It returns the descriptor-bound +// root and the path relative to it, or an error when the path lies outside +// every allowed root. +func openFormattedFileRoot(workspaceRoot string, scope PathScope, absolutePath string) (*os.Root, string, error) { + roots, err := scopedRoots(workspaceRoot, scope) + if err != nil { + return nil, "", err + } + var firstErr error + for _, configuredRoot := range roots { + resolvedRoot, err := filepath.Abs(configuredRoot) + if err == nil { + resolvedRoot, err = filepath.EvalSymlinks(resolvedRoot) + } + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + candidate := sandbox.NormalizePrefixForRoot(absolutePath, resolvedRoot) + relativePath, err := filepath.Rel(resolvedRoot, candidate) + if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { + continue + } + root, err := os.OpenRoot(resolvedRoot) + if err != nil { + if firstErr == nil { + firstErr = err + } + continue + } + return root, relativePath, nil } - return formatWithStaging(ctx, adapter, binaryPath, absolutePath, writtenContent) + if firstErr != nil { + return nil, "", firstErr + } + return nil, "", fmt.Errorf("%s must stay inside the configured write roots", absolutePath) } // formatWithStaging runs a physical formatter on a private copy of the written // bytes, never on the destination. The copy keeps the destination's basename // so filename-derived formatter behaviour still applies, and any scribble from // a killed or failing run lands there, not in the user's file. -func formatWithStaging(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent string) formatOnWriteResult { +func formatWithStaging(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent, dir string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} - dir := filepath.Dir(absolutePath) stagingDir, err := fsutil.CreatePrivateTempDir(dir, ".zero-fmt-*") if err != nil { return unformatted @@ -210,7 +265,7 @@ func formatWithStaging(ctx context.Context, adapter formatterAdapter, binaryPath // destination through the adapter's filename flag when it has one, and reads // the formatted bytes from stdout. The empty-output guard keeps a formatter // that ignores the path (and so prints nothing) from publishing an empty file. -func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent string) formatOnWriteResult { +func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath, absolutePath, writtenContent, dir string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() @@ -219,7 +274,7 @@ func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath, arguments = append(arguments, adapter.filenameFlag+"="+absolutePath) } formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) - formatter.Dir = filepath.Dir(absolutePath) + formatter.Dir = dir formatter.Stdin = strings.NewReader(writtenContent) hardenProcessLifetime(formatter) if formatterCommandObserver != nil { diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 9a5d386bc..eb8b1cb0e 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -590,6 +590,92 @@ func installFakeGofmt(t *testing.T, script string) { t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) } +// Formatting must not run for a path outside the configured write roots. The +// unscoped wrapper cannot express that; the scoped entry point refuses and +// returns the written bytes unchanged, while the same call inside the root +// still formats. +func TestFormatOnWriteScopedConfinement(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + outside := filepath.Join(t.TempDir(), "a.go") + if err := os.WriteFile(outside, []byte(uglyGoSource), 0o644); err != nil { + t.Fatal(err) + } + refused := maybeFormatWrittenFileScoped(context.Background(), root, nil, outside, uglyGoSource) + if refused.Content != uglyGoSource { + t.Fatalf("formatting outside the write root must be refused, got %q", refused.Content) + } + + inside := filepath.Join(root, "a.go") + if err := os.WriteFile(inside, []byte(uglyGoSource), 0o644); err != nil { + t.Fatal(err) + } + formatted := maybeFormatWrittenFileScoped(context.Background(), root, nil, inside, uglyGoSource) + if !strings.Contains(formatted.Content, "func A() {") { + t.Fatalf("formatting inside the write root must apply, got %q", formatted.Content) + } +} + +// A physical formatter is handed a staging copy; its working directory must +// still be pinned inside the allowed write root, not at the caller's cwd. +func TestFormatOnWriteScopedStagingRunsInsideWriteRoot(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + record := filepath.Join(t.TempDir(), "formatter-pwd") + installFakePhysicalFormatter(t, ".physfmt", "physfmt", record) + + root := t.TempDir() + target := filepath.Join(root, "sub", "a.physfmt") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(target, []byte("written\n"), 0o644); err != nil { + t.Fatal(err) + } + formatting := maybeFormatWrittenFileScoped(context.Background(), root, nil, target, "written\n") + if formatting.Content != "written\n" { + t.Fatalf("physical formatter output = %q, want the written bytes", formatting.Content) + } + recorded, err := os.ReadFile(record) + if err != nil { + t.Fatalf("formatter never recorded its working directory: %v", err) + } + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(string(recorded), resolvedRoot) { + t.Fatalf("formatter cwd %q escaped the write root %q", recorded, resolvedRoot) + } +} + +// installFakePhysicalFormatter puts a non-stdin formatter on PATH and registers +// it for the test's duration. The script records its working directory and +// leaves the staging file untouched, so the published bytes are the written +// bytes. +func installFakePhysicalFormatter(t *testing.T, extension, name, record string) { + t.Helper() + directory := t.TempDir() + script := "#!/bin/sh\nprintf '%s' \"$PWD\" > \"$ZERO_FORMAT_PWD_RECORD\"\nexit 0\n" + if err := os.WriteFile(filepath.Join(directory, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("ZERO_FORMAT_PWD_RECORD", record) + previous, existed := formatterCommands[extension] + formatterCommands[extension] = formatterAdapter{argv: []string{name}} + t.Cleanup(func() { + if existed { + formatterCommands[extension] = previous + return + } + delete(formatterCommands, extension) + }) +} + func TestFormatOnWriteRuffUsesLogicalDestinationForWriteAndEdit(t *testing.T) { if _, err := exec.LookPath("ruff"); err != nil { t.Skip("ruff not installed") diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index d9a441542..ed1443b18 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -64,7 +64,10 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an existed := false var priorInfo os.FileInfo - if info, err := os.Stat(absolutePath); err == nil { + if info, err := os.Lstat(absolutePath); err == nil { + if info.Mode()&os.ModeSymlink != 0 { + return errorResult("Error: " + relativePath + " is a symbolic link. Writing through a symlink is not allowed; target the real file instead.") + } existed = true priorInfo = info if !overwrite { @@ -118,7 +121,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // publish once. Recording pre-format content would make the next edit look // like an external modification and trip the conflict guard; formatting the // destination in place after publication would reintroduce partial writes. - formatting := maybeFormatWrittenFile(ctx, absolutePath, content) + formatting := maybeFormatWrittenFileScoped(ctx, tool.workspaceRoot, tool.scope, absolutePath, content) content = formatting.Content finalContentKnown := true diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 29afd57b2..222de04a9 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -1358,6 +1358,48 @@ func TestApplyPatchReportsWorkspaceRelativeChangedFilesUnderCwd(t *testing.T) { } } +// Writing or editing through a symbolic link must fail closed: the link is not +// replaced by a regular file and the bytes it points at are not altered. +func TestWriteAndEditToolsRefuseSymlinkDestinations(t *testing.T) { + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + root := t.TempDir() + real := filepath.Join(root, "real.txt") + if err := os.WriteFile(real, []byte("original bytes\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "link.txt") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + var result Result + if toolName == "write_file" { + result = NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "link.txt", "content": "clobbered\n", "overwrite": true, + }) + } else { + result = NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "link.txt", "old_string": "original", "new_string": "edited", + }) + } + if result.Status != StatusError { + t.Fatalf("%s on a symlink = %s (%q), want error", toolName, result.Status, result.Output) + } + info, err := os.Lstat(link) + if err != nil { + t.Fatal(err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("%s replaced the symlink with a regular file", toolName) + } + if got, err := os.ReadFile(real); err != nil || string(got) != "original bytes\n" { + t.Fatalf("%s altered the symlink target: %q, err=%v", toolName, got, err) + } + }) + } +} + func TestWriteFileReportsChangedFileAndDisplay(t *testing.T) { root := t.TempDir() res := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{"path": "notes.txt", "content": "hello"}) From c00d83b8976f05b2cf7a472eda245cd8a5e77061 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sat, 26 Sep 2026 09:54:26 +0200 Subject: [PATCH 4/4] fix(tools): pass terminal stdin marker to ruff and clarify path exclusions --- internal/tools/format_on_write.go | 59 +++++++--- internal/tools/format_on_write_test.go | 145 ++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 17 deletions(-) diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index 1d62ad0e5..4581a903f 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -71,12 +71,13 @@ func (result formatOnWriteResult) notice(relativePath string) string { // // STDIN ADAPTERS READ THE WRITTEN BYTES ON STDIN AND PRINT THE FORMATTED RESULT // TO STDOUT. That route lets filenameFlag carry the logical destination path, -// so any rule written against the file's own path (.clang-format-ignore, an -// .editorconfig section, rustfmt's ignore) still resolves against the file the -// caller named instead of a random staging name. Physical adapters have no -// such option: they are handed a private staging copy inside an owner-only -// sibling directory (including auxiliary files) and keep the destination -// directory as their working directory. +// so a rule written against the file's own path is aimed at the name the +// caller used instead of a random staging name. Aiming the rule at that name +// does not make a path exclusion apply to a file that does not exist yet; see +// formatterCommands. Physical adapters have no such option: they are handed a +// private staging copy inside an owner-only sibling directory (including +// auxiliary files) and keep the destination directory as their working +// directory. type formatterAdapter struct { argv []string // stdin selects the stdin/stdout route. When false the private staging path @@ -86,14 +87,28 @@ type formatterAdapter struct { // stdin adapter. It is emitted as "--flag=" and empty when the // formatter resolves its configuration from the working directory alone. filenameFlag string + // stdinArg is appended after filenameFlag, and must not also be stored in + // argv: a "-" kept in argv would be emitted before the filename flag. + // For Ruff, "-" serves as an explicit, defensive marker ensuring input is read + // from standard input when invoked as `ruff format --stdin-filename= -`. + // Empty for formatters that read stdin when given no path. + stdinArg string } // formatterCommands maps a file extension to its formatter. A missing binary // silently skips formatting. +// +// PATH EXCLUSIONS DO NOT APPLY ON THE FIRST WRITE OF A NEW FILE. The stdin +// adapters pass the logical destination, but a formatter that consults its +// ignore file only when that path already exists (clang-format and +// .clang-format-ignore) formats a not-yet-created file even when the path +// matches an exclusion. An overwrite still finds the previous file at that +// path, so the same rule can apply. TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles +// pins this split; it is the contract, not an oversight. var formatterCommands = map[string]formatterAdapter{ ".go": {argv: []string{"gofmt"}, stdin: true}, ".rs": {argv: []string{"rustfmt"}, stdin: true}, - ".py": {argv: []string{"ruff", "format", "--quiet"}, stdin: true, filenameFlag: "--stdin-filename"}, + ".py": {argv: []string{"ruff", "format", "--quiet"}, stdin: true, filenameFlag: "--stdin-filename", stdinArg: "-"}, ".ts": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, ".tsx": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, ".js": {argv: []string{"prettier", "--log-level", "silent"}, stdin: true, filenameFlag: "--stdin-filepath"}, @@ -142,11 +157,11 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon // maybeFormatWrittenFileScoped runs the configured formatter over a transient // copy of the written bytes (stdin, or a private staging file) and returns the // bytes to publish. The destination is never opened or rewritten here. If -// absolutePath does not resolve inside one of the scope's write roots, the -// formatting is refused and the written bytes pass through unchanged: a -// formatter must not be able to read or write outside the same roots the tool -// itself is confined to. The formatter's working directory is pinned inside the -// matched root for the same reason. +// absolutePath does not resolve inside one of the scope's write roots, +// formatting is refused and the written bytes pass through unchanged. That +// refusal is the pre-launch path check in openFormattedFileRoot. The +// formatter's working directory is a path string derived from that check, not +// a directory handle the subprocess is bound to. func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, scope PathScope, absolutePath string, writtenContent string) formatOnWriteResult { unformatted := formatOnWriteResult{Content: writtenContent} if !formatOnWriteEnabled() { @@ -172,10 +187,19 @@ func maybeFormatWrittenFileScoped(ctx context.Context, workspaceRoot string, sco return formatWithStaging(ctx, adapter, binaryPath, absolutePath, writtenContent, workDir) } -// openFormattedFileRoot resolves absolutePath against the scope's write roots -// and opens the first root that contains it. It returns the descriptor-bound -// root and the path relative to it, or an error when the path lies outside -// every allowed root. +// openFormattedFileRoot checks, before a formatter subprocess is started, that +// absolutePath resolves inside one of the scope's write roots. It resolves +// each configured root, applies a lexical prefix check to the candidate path +// (previously validated by resolveScopedTargetPath), and opens the first root +// containing it. It returns that root and the path relative to it, or an error +// when the path lies outside every allowed root. +// +// The caller does not use the root as a handle for formatter I/O. It joins +// root.Name() with the relative directory and passes that string as +// exec.Cmd.Dir. The check is therefore a pre-launch lexical path check, not +// descriptor-level confinement of the subprocess: a symlink or Windows junction +// swapped onto a component of that directory between the check and process start +// is not excluded, and there is no portable way to hand Cmd.Dir a directory handle. func openFormattedFileRoot(workspaceRoot string, scope PathScope, absolutePath string) (*os.Root, string, error) { roots, err := scopedRoots(workspaceRoot, scope) if err != nil { @@ -273,6 +297,9 @@ func formatWithStdin(ctx context.Context, adapter formatterAdapter, binaryPath, if adapter.filenameFlag != "" { arguments = append(arguments, adapter.filenameFlag+"="+absolutePath) } + if adapter.stdinArg != "" { + arguments = append(arguments, adapter.stdinArg) + } formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) formatter.Dir = dir formatter.Stdin = strings.NewReader(writtenContent) diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index eb8b1cb0e..5db03e491 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -229,7 +229,9 @@ func TestFormatOnWritePrettierUsesDestinationFilename(t *testing.T) { // // A stdin adapter receives the logical destination through its filename flag, // so a .clang-format-ignore pattern, an .editorconfig section, or rustfmt's -// ignore still matches "vendor/lib.hintfmt" rather than a random staging name. +// ignore is aimed at "vendor/lib.hintfmt" rather than a random staging name. +// Whether the formatter consults that rule for a path that does not exist yet +// is a separate contract; see TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles. func TestFormatOnWritePassesLogicalDestinationToStdinFormatter(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("fake formatter shim is a POSIX script") @@ -265,6 +267,85 @@ func TestFormatOnWritePassesLogicalDestinationToStdinFormatter(t *testing.T) { } } +// PATH EXCLUSIONS DO NOT APPLY TO A FILE THAT DOES NOT EXIST YET. +// +// clang-format consults .clang-format-ignore only when --assume-filename names +// a path that is already on disk. Formatting runs before publication, so the +// first write of a new file is formatted even when that path would match an +// exclusion. An overwrite still finds the previous file: the formatter prints +// nothing, and the empty-output guard keeps the written bytes. Depositing the +// bytes at the destination before the formatter runs would make the subsequent +// exclusive create fail, so this split is the contract. +func TestFormatOnWritePathIgnoreDoesNotApplyToNewFiles(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + installExistenceGatedFormatter(t, ".ignfmt", "ignfmt", "--assume-filename") + + dir := t.TempDir() + target := filepath.Join(dir, "vendor", "lib.ignfmt") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + + const written = "unformatted\n" + created := maybeFormatWrittenFile(context.Background(), target, written) + if created.Content != "REFORMATTED\n" { + t.Fatalf("a new file must be formatted even when its path would be ignored, got %q", created.Content) + } + if _, err := os.Stat(target); !os.IsNotExist(err) { + t.Fatalf("formatting a new file must not create it ahead of publication: %v", err) + } + + if err := os.WriteFile(target, []byte("previous\n"), 0o644); err != nil { + t.Fatal(err) + } + overwritten := maybeFormatWrittenFile(context.Background(), target, written) + if overwritten.Content != written { + t.Fatalf("an existing ignored path must keep the written bytes, got %q", overwritten.Content) + } + onDisk, err := os.ReadFile(target) + if err != nil || string(onDisk) != "previous\n" { + t.Fatalf("formatter must not rewrite the destination, got %q, %v", onDisk, err) + } +} + +// installExistenceGatedFormatter registers a stdin formatter that honours a +// path exclusion only when the assumed path already exists, matching +// clang-format's .clang-format-ignore behaviour (where existing ignored files +// have their input returned unchanged). A missing path is reformatted. +func installExistenceGatedFormatter(t *testing.T, extension, name, filenameFlag string) { + t.Helper() + directory := t.TempDir() + script := "#!/bin/sh\n" + + "path=\n" + + "for arg in \"$@\"; do\n" + + " case \"$arg\" in\n" + + " " + filenameFlag + "=*) path=${arg#" + filenameFlag + "=} ;;\n" + + " esac\n" + + "done\n" + + "if [ -n \"$path\" ] && [ -e \"$path\" ]; then\n" + + " cat\n" + + " exit 0\n" + + "fi\n" + + "cat >/dev/null\n" + + "printf 'REFORMATTED\\n'\n" + if err := os.WriteFile(filepath.Join(directory, name), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + previous, existed := formatterCommands[extension] + formatterCommands[extension] = formatterAdapter{argv: []string{name}, stdin: true, filenameFlag: filenameFlag} + t.Cleanup(func() { + if existed { + formatterCommands[extension] = previous + return + } + delete(formatterCommands, extension) + }) +} + // THE PRODUCTION TABLE MUST CARRY THE HINTS, NOT ONLY THE TEST SEAM. func TestFormatterFilenameHints(t *testing.T) { want := map[string]string{ @@ -676,6 +757,68 @@ func installFakePhysicalFormatter(t *testing.T, extension, name, record string) }) } +// RUFF MUST READ STDIN WITH AN EXPLICIT TERMINAL MARKER. +// +// The stdin route passes an explicit "-" marker after --stdin-filename, +// ensuring Ruff strictly reads input from stdin as documented. +func TestFormatOnWriteRuffPassesTerminalStdinMarker(t *testing.T) { + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + adapter := formatterCommands[".py"] + if !adapter.stdin || adapter.filenameFlag != "--stdin-filename" || adapter.stdinArg != "-" { + t.Fatalf("ruff adapter = %+v, want stdin with --stdin-filename and stdinArg %q", adapter, "-") + } + + directory := t.TempDir() + binaryName := "ruff" + formatterScriptExtension() + script := "#!/bin/sh\ncat\n" + if runtime.GOOS == "windows" { + script = "@echo off\r\nexit /b 0\r\n" + } + if err := os.WriteFile(filepath.Join(directory, binaryName), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", directory+string(os.PathListSeparator)+os.Getenv("PATH")) + + var seen *exec.Cmd + previous := formatterCommandObserver + formatterCommandObserver = func(command *exec.Cmd) { seen = command } + t.Cleanup(func() { formatterCommandObserver = previous }) + + dir := t.TempDir() + target := filepath.Join(dir, "pkg", "new_file.py") + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + t.Fatal(err) + } + _ = maybeFormatWrittenFile(context.Background(), target, "x=1\n") + if seen == nil { + t.Fatal("ruff was not executed") + } + args := seen.Args + if len(args) == 0 || args[len(args)-1] != "-" { + t.Fatalf("ruff execution args %q must end with the stdin marker \"-\"", args) + } + dashes := 0 + flagAt := -1 + wantFlag := "--stdin-filename=" + target + for i, arg := range args { + if arg == "-" { + dashes++ + } + if arg == wantFlag { + flagAt = i + } + if arg == target { + t.Fatalf("ruff execution args %q pass the path as a file operand; the stdin route must not", args) + } + } + if dashes != 1 { + t.Fatalf("ruff execution args %q must contain exactly one stdin marker", args) + } + if flagAt < 0 || flagAt >= len(args)-1 { + t.Fatalf("ruff execution args %q must carry %q before the terminal \"-\"", args, wantFlag) + } +} + func TestFormatOnWriteRuffUsesLogicalDestinationForWriteAndEdit(t *testing.T) { if _, err := exec.LookPath("ruff"); err != nil { t.Skip("ruff not installed")