fix(output): preserve permissions when clobbering

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

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

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)
 	}

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")

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)
+	}
+}

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.

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)
 }