From 970e4a877f308e56236533b3926a4ce8bee1d5d9 Mon Sep 17 00:00:00 2001 From: Ryan Fowler Date: Sun, 30 Aug 2026 21:02:36 +0000 Subject: [PATCH] fix(output): preserve permissions when clobbering --- docs/cli-reference.md | 3 +- docs/output-formatting.md | 3 + internal/fetch/output.go | 2 +- internal/fetch/output_test.go | 101 +++++++++++++++++++++++++++ internal/fileutil/replace_test.go | 28 ++++++++ internal/fileutil/replace_unix.go | 16 +++++ internal/fileutil/replace_windows.go | 16 +++++ 7 files changed, 167 insertions(+), 2 deletions(-) diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 673f1bd4..bc61a744 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -200,7 +200,8 @@ fetch -O -J example.com/download ### `--clobber` -Overwrite existing output file (default behavior is to fail if file exists). +Overwrite an existing output file while preserving its permission bits (default +behavior is to fail if the file exists). ```sh fetch -o output.json --clobber example.com/data diff --git a/docs/output-formatting.md b/docs/output-formatting.md index 9bb59483..1c19927b 100644 --- a/docs/output-formatting.md +++ b/docs/output-formatting.md @@ -375,6 +375,9 @@ Overwrite existing files: fetch -o output.json --clobber example.com/data ``` +When replacing an existing regular file, `--clobber` preserves its permission +bits. + ## Pager Use `--pager auto|on|off` to control paging. In `auto` mode, formatted text diff --git a/internal/fetch/output.go b/internal/fetch/output.go index 4cf44443..7d4bc03f 100644 --- a/internal/fetch/output.go +++ b/internal/fetch/output.go @@ -75,7 +75,7 @@ func writeOutputToFile(filename string, body io.Reader, size int64, p *core.Prin return err } if clobber { - err = fileutil.AtomicReplaceFileNoSymlink(tempName, name) + err = fileutil.AtomicReplaceFileNoSymlinkPreserveMode(tempName, name) } else { err = fileutil.AtomicWriteNewFile(tempName, name) } diff --git a/internal/fetch/output_test.go b/internal/fetch/output_test.go index 80876014..20bb4bf7 100644 --- a/internal/fetch/output_test.go +++ b/internal/fetch/output_test.go @@ -3,6 +3,7 @@ package fetch import ( "bytes" "errors" + "io" "net/http" "os" "path/filepath" @@ -204,6 +205,90 @@ func TestWriteOutputToFile_OverwritesExistingFileWithClobber(t *testing.T) { } } +func TestWriteOutputToFile_ClobberPreservesExistingPermissions(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "download.txt") + + if err := os.WriteFile(path, []byte("old"), 0751); err != nil { + t.Fatal(err) + } + before, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + err = writeOutputToFile(path, strings.NewReader("new"), 3, core.TestPrinter(false), core.VSilent, true) + if err != nil { + t.Fatalf("writeOutputToFile returned error: %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got, want := info.Mode().Perm(), before.Mode().Perm(); got != want { + t.Fatalf("output file permissions = %04o, want %04o", got, want) + } +} + +func TestWriteOutputToFile_ClobberUsesPermissionsAtCommit(t *testing.T) { + tests := []struct { + name string + setup func(*testing.T, string) + change func(string) error + }{ + { + name: "destination permissions change during download", + setup: func(t *testing.T, path string) { + if err := os.WriteFile(path, []byte("old"), 0751); err != nil { + t.Fatal(err) + } + }, + change: func(path string) error { return os.Chmod(path, 0600) }, + }, + { + name: "destination appears during download", + setup: func(*testing.T, string) {}, + change: func(path string) error { + return os.WriteFile(path, []byte("racing writer"), 0740) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "download.txt") + tt.setup(t, path) + var wantMode os.FileMode + body := &beforeReadReader{ + Reader: strings.NewReader("new"), + before: func() error { + if err := tt.change(path); err != nil { + return err + } + info, err := os.Stat(path) + if err == nil { + wantMode = info.Mode().Perm() + } + return err + }, + } + + err := writeOutputToFile(path, body, 3, core.TestPrinter(false), core.VSilent, true) + if err != nil { + t.Fatalf("writeOutputToFile returned error: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != wantMode { + t.Fatalf("output file permissions = %04o, want commit-time %04o", got, wantMode) + } + }) + } +} + func TestContentDispositionFilenamePrefersRFC5987(t *testing.T) { h := http.Header{} h.Set("Content-Disposition", `attachment; filename="plain.txt"; filename*=UTF-8''%E2%82%AC.txt`) @@ -282,6 +367,22 @@ type errorReader struct{} func (*errorReader) Read([]byte) (int, error) { return 0, errors.New("body failed") } +type beforeReadReader struct { + io.Reader + before func() error +} + +func (r *beforeReadReader) Read(p []byte) (int, error) { + if r.before != nil { + before := r.before + r.before = nil + if err := before(); err != nil { + return 0, err + } + } + return r.Reader.Read(p) +} + func TestWriteOutputToFile_DoesNotOverwriteExistingFileWithoutClobber(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "download.txt") diff --git a/internal/fileutil/replace_test.go b/internal/fileutil/replace_test.go index 06313109..7c3454c7 100644 --- a/internal/fileutil/replace_test.go +++ b/internal/fileutil/replace_test.go @@ -86,3 +86,31 @@ func TestAtomicWriteNewFile_DoesNotReplaceExistingFile(t *testing.T) { t.Fatalf("temp file should remain after failed install, stat err = %v", err) } } + +func TestAtomicReplaceFileNoSymlinkPreserveMode(t *testing.T) { + dir := t.TempDir() + targetPath := filepath.Join(dir, "target.txt") + tempPath := filepath.Join(dir, "temp.txt") + + if err := os.WriteFile(targetPath, []byte("old"), 0751); err != nil { + t.Fatal(err) + } + before, err := os.Stat(targetPath) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(tempPath, []byte("new"), 0600); err != nil { + t.Fatal(err) + } + + if err := AtomicReplaceFileNoSymlinkPreserveMode(tempPath, targetPath); err != nil { + t.Fatalf("AtomicReplaceFileNoSymlinkPreserveMode returned error: %v", err) + } + after, err := os.Stat(targetPath) + if err != nil { + t.Fatal(err) + } + if got, want := after.Mode().Perm(), before.Mode().Perm(); got != want { + t.Fatalf("target permissions = %04o, want %04o", got, want) + } +} diff --git a/internal/fileutil/replace_unix.go b/internal/fileutil/replace_unix.go index e774fbe9..2beecfa7 100644 --- a/internal/fileutil/replace_unix.go +++ b/internal/fileutil/replace_unix.go @@ -23,6 +23,17 @@ func AtomicReplaceFile(tempPath, targetPath string) error { // with rename: rename never follows the final target symlink, so a race cannot // redirect the staged bytes outside targetPath. func AtomicReplaceFileNoSymlink(tempPath, targetPath string) error { + return atomicReplaceFileNoSymlink(tempPath, targetPath, false) +} + +// AtomicReplaceFileNoSymlinkPreserveMode atomically replaces targetPath while +// inheriting the permission bits of an existing target. A target that does not +// exist at commit time retains tempPath's permissions. +func AtomicReplaceFileNoSymlinkPreserveMode(tempPath, targetPath string) error { + return atomicReplaceFileNoSymlink(tempPath, targetPath, true) +} + +func atomicReplaceFileNoSymlink(tempPath, targetPath string, preserveMode bool) error { // Serialize fileutil commits so concurrent fetches cannot invalidate each // other's identity check. The final rename never follows a symlink. noSymlinkCommitMu.Lock() @@ -58,6 +69,11 @@ func AtomicReplaceFileNoSymlink(tempPath, targetPath string) error { if !os.SameFile(info, latest) { return errTargetChanged } + if preserveMode { + if err := os.Chmod(tempPath, latest.Mode().Perm()); err != nil { + return err + } + } // os.Rename replaces the directory entry and never follows the final // target symlink. The identity check prevents a detected replacement from // being mistaken for the file we validated. diff --git a/internal/fileutil/replace_windows.go b/internal/fileutil/replace_windows.go index f8866f0a..584be9c8 100644 --- a/internal/fileutil/replace_windows.go +++ b/internal/fileutil/replace_windows.go @@ -41,6 +41,17 @@ func AtomicReplaceFile(tempPath, targetPath string) error { // AtomicReplaceFileNoSymlink atomically replaces targetPath but refuses a // symlink that was present when the commit was attempted. func AtomicReplaceFileNoSymlink(tempPath, targetPath string) error { + return atomicReplaceFileNoSymlink(tempPath, targetPath, false) +} + +// AtomicReplaceFileNoSymlinkPreserveMode atomically replaces targetPath while +// inheriting the permission bits of an existing target. A target that does not +// exist at commit time retains tempPath's permissions. +func AtomicReplaceFileNoSymlinkPreserveMode(tempPath, targetPath string) error { + return atomicReplaceFileNoSymlink(tempPath, targetPath, true) +} + +func atomicReplaceFileNoSymlink(tempPath, targetPath string, preserveMode bool) error { // Serialize fileutil commits so concurrent fetches cannot invalidate each // other's identity check. The final replacement does not follow a symlink. noSymlinkCommitMu.Lock() @@ -71,6 +82,11 @@ func AtomicReplaceFileNoSymlink(tempPath, targetPath string) error { if !os.SameFile(info, latest) { return errTargetChanged } + if preserveMode { + if err := os.Chmod(tempPath, latest.Mode().Perm()); err != nil { + return err + } + } return AtomicReplaceFile(tempPath, targetPath) }