fix(app): publish downloads with umask permissions and add destination flags

Downloads were created like temporary files, readable by their owner only.
They now get the permissions the umask allows, as files a shell redirection
creates; API responses written with --output-file stay private. An
--output-file that is a directory is reported as such instead of "output
file already exists", and a download written to a terminal refuses binary
data and strips control sequences from text.

downloadFlags gives download commands the gh destination flags: -O/--output
(a file, a directory, or - for stdout; --output-file stays as a hidden alias),
-D/--dir, --clobber and --skip-existing, with the download's own name as the
default destination. Names that would escape the destination are refused.

Signed-off-by: bircni <bircni@icloud.com>

internal/app/download.go

@@ -4,17 +4,151 @@
 package app
 
 import (
+	"bufio"
 	"errors"
+	"fmt"
 	"io"
+	"math/rand/v2"
 	"os"
 	"path/filepath"
+	"strconv"
+	"strings"
+	"unicode/utf8"
 
 	"gitea.dev/tea/internal/clierr"
+	"gitea.dev/tea/internal/terminal"
 	"github.com/spf13/cobra"
+	"golang.org/x/term"
 )
 
+// downloadFlags are the destination flags of a download command.
+type downloadFlags struct {
+	// output is a file, an existing directory to download into, or - for standard output.
+	output string
+	// dir is the directory that receives the download under its own name.
+	dir string
+	// clobber replaces an existing file; skipExisting keeps it and skips the download.
+	clobber, skipExisting bool
+}
+
+// register adds -O/--output (and its older name --output-file), -D/--dir, --clobber and
+// --skip-existing to cmd.
+func (f *downloadFlags) register(cmd *cobra.Command) {
+	flags := cmd.Flags()
+	flags.StringVarP(&f.output, "output", "O", "", "File or directory to write to, or - for standard output (default: the download's name)")
+	flags.StringVar(&f.output, "output-file", "", "Alias of --output")
+	_ = flags.MarkHidden("output-file")
+	flags.StringVarP(&f.dir, "dir", "D", "", "Directory to download into")
+	flags.BoolVar(&f.clobber, "clobber", false, "Replace an existing file")
+	flags.BoolVar(&f.skipExisting, "skip-existing", false, "Keep an existing file and skip the download")
+}
+
+// destination returns the path a download named name is written to: --output (inside it when it
+// is a directory), else name inside --dir or the working directory, or "-" for standard
+// output. skip reports an existing file that --skip-existing keeps. It fails before anything
+// is downloaded when the file exists without --clobber or --skip-existing.
+func (f downloadFlags) destination(name string) (path string, skip bool, err error) {
+	switch {
+	case f.output != "" && f.dir != "":
+		return "", false, clierr.Wrap(clierr.Usage, errors.New("--output and --dir cannot be combined"))
+	case f.clobber && f.skipExisting:
+		return "", false, clierr.Wrap(clierr.Usage, errors.New("--clobber and --skip-existing cannot be combined"))
+	case f.output == "-":
+		return "-", false, nil
+	}
+	path = f.output
+	if path == "" || isDirectory(path) {
+		if name != filepath.Base(name) || name == "." || name == ".." || strings.ContainsAny(name, `/\`) {
+			return "", false, clierr.Wrap(clierr.Usage, fmt.Errorf("cannot use %q as a file name; pass --output", name))
+		}
+		path = filepath.Join(cmpOrDot(path, f.dir), name)
+	}
+	if _, err := os.Lstat(path); err == nil {
+		if f.skipExisting {
+			return path, true, nil
+		}
+		if !f.clobber {
+			return "", false, clierr.Wrap(clierr.Usage, fmt.Errorf("%s already exists; pass --clobber to replace it or --skip-existing to keep it", path))
+		}
+	} else if !errors.Is(err, os.ErrNotExist) {
+		return "", false, err
+	}
+	return path, false, nil
+}
+
+func cmpOrDot(values ...string) string {
+	for _, value := range values {
+		if value != "" {
+			return value
+		}
+	}
+	return "."
+}
+
+func isDirectory(path string) bool {
+	info, err := os.Stat(path)
+	return (err == nil && info.IsDir()) || strings.HasSuffix(path, string(filepath.Separator)) || strings.HasSuffix(path, "/")
+}
+
+// download writes body to path, a destination from downloadFlags.destination. A file is
+// published complete with the permissions the umask allows; standard output refuses binary
+// data on a terminal and strips terminal control sequences from text.
+func (a *application) download(cmd *cobra.Command, f downloadFlags, path string, body io.Reader) error {
+	if path == "-" {
+		return writeTerminalSafe(cmd.OutOrStdout(), a.deps.Streams.OutTTY, body)
+	}
+	return publish(cmd, path, false, f.clobber, func(destination io.Writer) error {
+		_, err := io.Copy(destination, body)
+		return err
+	})
+}
+
 func writeDownload(cmd *cobra.Command, body io.Reader, output string) error {
-	return writeOutput(cmd, output, func(destination io.Writer) error { _, err := io.Copy(destination, body); return err })
+	if output == "" || output == "-" {
+		return writeTerminalSafe(cmd.OutOrStdout(), isTerminal(cmd.OutOrStdout()), body)
+	}
+	return publish(cmd, output, false, false, func(destination io.Writer) error { _, err := io.Copy(destination, body); return err })
+}
+
+// isTerminal reports whether w is a terminal, for writers without injected stream capabilities.
+func isTerminal(w io.Writer) bool {
+	file, ok := w.(*os.File)
+	return ok && term.IsTerminal(int(file.Fd()))
+}
+
+// binarySniffLength is how much of a download decides whether it is text.
+const binarySniffLength = 8 << 10
+
+// writeTerminalSafe copies body to w. On a terminal it refuses binary data, which would garble
+// the terminal, and strips control sequences from text.
+func writeTerminalSafe(w io.Writer, tty bool, body io.Reader) error {
+	if !tty {
+		_, err := io.Copy(w, body)
+		return err
+	}
+	reader := bufio.NewReaderSize(body, binarySniffLength)
+	head, err := reader.Peek(binarySniffLength)
+	if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, bufio.ErrBufferFull) {
+		return err
+	}
+	if binary(head) {
+		return clierr.Wrap(clierr.Usage, errors.New("refusing to write binary data to the terminal; pass --output FILE or redirect standard output"))
+	}
+	_, err = io.Copy(terminal.SafeWriter(w, true, false), reader)
+	return err
+}
+
+// binary reports data that is not UTF-8 text: a NUL byte or an invalid sequence other than a
+// rune cut off at the end.
+func binary(data []byte) bool {
+	for len(data) > 0 {
+		r, size := utf8.DecodeRune(data)
+		if r == 0 || (r == utf8.RuneError && size == 1 && (len(data) >= utf8.UTFMax || utf8.FullRune(data))) {
+			return true
+		}
+		data = data[size:]
+	}
+	return false
 }
 
 // writeOutput writes to stdout when output is empty or -; otherwise it publishes a complete
@@ -23,7 +157,15 @@ func writeOutput(cmd *cobra.Command, output string, write func(io.Writer) error)
 	if output == "" || output == "-" {
 		return write(cmd.OutOrStdout())
 	}
-	file, err := os.CreateTemp(filepath.Dir(output), ".tea-download-*")
+	return publish(cmd, output, true, false, write)
+}
+
+// publish writes a temporary file next to output and then links it into place, so output only
+// ever holds a complete file. With replace it renames over an existing output; otherwise it
+// never replaces a file created while it wrote. A private file is readable by its owner only;
+// others get the permissions the umask allows, as files a shell redirection creates.
+func publish(cmd *cobra.Command, output string, private, replace bool, write func(io.Writer) error) error {
+	file, err := createTemp(filepath.Dir(output), private)
 	if err != nil {
 		return err
 	}
@@ -39,14 +181,33 @@ func writeOutput(cmd *cobra.Command, output string, write func(io.Writer) error)
 	if closeErr != nil {
 		return closeErr
 	}
+	if replace {
+		return os.Rename(file.Name(), output)
+	}
 	// Linking never replaces a file created while the download ran.
 	err = os.Link(file.Name(), output)
 	if errors.Is(err, os.ErrExist) {
-		return clierr.Wrap(clierr.Usage, errors.New("output file already exists"))
+		return clierr.Wrap(clierr.Usage, fmt.Errorf("%s already exists", output))
 	}
 	return err
 }
 
+// createTemp creates a new temporary file in dir. Unlike os.CreateTemp, which always creates
+// mode 0600, a file that is not private gets 0666 less the umask.
+func createTemp(dir string, private bool) (*os.File, error) {
+	if private {
+		return os.CreateTemp(dir, ".tea-download-*")
+	}
+	for range 100 {
+		name := filepath.Join(dir, ".tea-download-"+strconv.FormatUint(rand.Uint64(), 36))
+		file, err := os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o666)
+		if !errors.Is(err, os.ErrExist) {
+			return file, err
+		}
+	}
+	return nil, errors.New("cannot create a temporary file in " + dir)
+}
+
 func validateDownloadDestination(output string) error {
 	if output == "" {
 		return clierr.Wrap(clierr.Usage, errors.New("supply --output-file, or --output-file - for stdout"))
@@ -54,8 +215,11 @@ func validateDownloadDestination(output string) error {
 	if output == "-" {
 		return nil
 	}
+	if isDirectory(output) {
+		return clierr.Wrap(clierr.Usage, fmt.Errorf("%s is a directory; pass a file path inside it", output))
+	}
 	if _, err := os.Lstat(output); err == nil {
-		return clierr.Wrap(clierr.Usage, errors.New("output file already exists"))
+		return clierr.Wrap(clierr.Usage, fmt.Errorf("%s already exists", output))
 	} else if !errors.Is(err, os.ErrNotExist) {
 		return err
 	}

internal/app/download_test.go

@@ -10,8 +10,11 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
+	"gitea.dev/tea/internal/clierr"
+	"gitea.dev/tea/internal/terminal"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/require"
 )
@@ -20,7 +23,19 @@ type failingDownload struct{}
 
 func (failingDownload) Read([]byte) (int, error) { return 0, io.ErrUnexpectedEOF }
 
-func TestDownloadsPublishOnlyCompletePrivateFiles(t *testing.T) {
+// umaskMode returns the mode the umask leaves of 0666, as for files a shell creates.
+func umaskMode(t *testing.T) os.FileMode {
+	t.Helper()
+	probe := filepath.Join(t.TempDir(), "probe")
+	file, err := os.OpenFile(probe, os.O_CREATE|os.O_WRONLY, 0o666)
+	require.NoError(t, err)
+	require.NoError(t, file.Close())
+	info, err := os.Stat(probe)
+	require.NoError(t, err)
+	return info.Mode().Perm()
+}
+
+func TestDownloadsPublishOnlyCompleteFiles(t *testing.T) {
 	cmd := &cobra.Command{}
 	cmd.SetContext(context.Background())
 	var stdout bytes.Buffer
@@ -38,7 +53,7 @@ func TestDownloadsPublishOnlyCompletePrivateFiles(t *testing.T) {
 	require.NoError(t, writeDownload(cmd, bytes.NewBufferString("archive"), output))
 	info, err := os.Stat(output)
 	require.NoError(t, err)
-	require.Equal(t, os.FileMode(0o600), info.Mode().Perm())
+	require.Equal(t, umaskMode(t), info.Mode().Perm(), "downloads are not private files")
 	require.ErrorContains(t, writeDownload(cmd, bytes.NewBufferString("replacement"), output), "already exists")
 	data, err := os.ReadFile(output)
 	require.NoError(t, err)
@@ -46,4 +61,121 @@ func TestDownloadsPublishOnlyCompletePrivateFiles(t *testing.T) {
 	entries, err = os.ReadDir(directory)
 	require.NoError(t, err)
 	require.Len(t, entries, 1)
+
+	private := filepath.Join(directory, "response.json")
+	require.NoError(t, writeOutput(cmd, private, func(w io.Writer) error { _, err := io.WriteString(w, "{}"); return err }))
+	info, err = os.Stat(private)
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o600), info.Mode().Perm(), "API responses stay private")
+}
+
+func TestDownloadDestinations(t *testing.T) {
+	work := t.TempDir()
+	t.Chdir(work)
+	require.NoError(t, os.Mkdir("dl", 0o755))
+	require.NoError(t, os.WriteFile("existing.txt", []byte("old"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join("dl", "existing.txt"), []byte("old"), 0o644))
+	for _, tc := range []struct {
+		name  string
+		flags downloadFlags
+		file  string
+		want  string
+		skip  bool
+		err   string
+	}{
+		{name: "default name", file: "notes.txt", want: "notes.txt"},
+		{name: "--dir", flags: downloadFlags{dir: "dl"}, file: "notes.txt", want: filepath.Join("dl", "notes.txt")},
+		{name: "--output file", flags: downloadFlags{output: "renamed.txt"}, file: "notes.txt", want: "renamed.txt"},
+		{name: "--output directory", flags: downloadFlags{output: "dl"}, file: "notes.txt", want: filepath.Join("dl", "notes.txt")},
+		{name: "--output directory with slash", flags: downloadFlags{output: "dl/"}, file: "notes.txt", want: filepath.Join("dl", "notes.txt")},
+		{name: "stdout", flags: downloadFlags{output: "-"}, file: "notes.txt", want: "-"},
+		{name: "existing", file: "existing.txt", err: "existing.txt already exists; pass --clobber to replace it or --skip-existing to keep it"},
+		{name: "existing in --dir", flags: downloadFlags{dir: "dl"}, file: "existing.txt", err: filepath.Join("dl", "existing.txt") + " already exists"},
+		{name: "--clobber", flags: downloadFlags{clobber: true}, file: "existing.txt", want: "existing.txt"},
+		{name: "--skip-existing", flags: downloadFlags{skipExisting: true}, file: "existing.txt", want: "existing.txt", skip: true},
+		{name: "--skip-existing new file", flags: downloadFlags{skipExisting: true}, file: "notes.txt", want: "notes.txt"},
+		{name: "parent directory name", file: "../escape.txt", err: `cannot use "../escape.txt" as a file name; pass --output`},
+		{name: "nested name", file: "a/b.txt", err: "cannot use"},
+		{name: "dot dot", file: "..", err: "cannot use"},
+		{name: "--output and --dir", flags: downloadFlags{output: "x", dir: "dl"}, file: "notes.txt", err: "--output and --dir cannot be combined"},
+		{name: "--clobber and --skip-existing", flags: downloadFlags{clobber: true, skipExisting: true}, file: "notes.txt", err: "--clobber and --skip-existing cannot be combined"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			path, skip, err := tc.flags.destination(tc.file)
+			if tc.err != "" {
+				require.ErrorContains(t, err, tc.err)
+				require.Equal(t, clierr.Usage, clierr.ExitCode(err))
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.want, path)
+			require.Equal(t, tc.skip, skip)
+		})
+	}
+}
+
+func TestDownloadClobberReplacesTheFileCompletely(t *testing.T) {
+	t.Chdir(t.TempDir())
+	require.NoError(t, os.WriteFile("asset.bin", []byte("old"), 0o600))
+	deps, _, _ := testDeps(t)
+	a := &application{deps: deps}
+	cmd := &cobra.Command{}
+	cmd.SetContext(context.Background())
+	flags := downloadFlags{clobber: true}
+	path, _, err := flags.destination("asset.bin")
+	require.NoError(t, err)
+	require.NoError(t, a.download(cmd, flags, path, strings.NewReader("new")))
+	data, err := os.ReadFile("asset.bin")
+	require.NoError(t, err)
+	require.Equal(t, "new", string(data))
+	info, err := os.Stat("asset.bin")
+	require.NoError(t, err)
+	require.Equal(t, umaskMode(t), info.Mode().Perm())
+	entries, err := os.ReadDir(".")
+	require.NoError(t, err)
+	require.Len(t, entries, 1, "no temporary file is left behind")
+}
+
+func TestDownloadsToATerminalRefuseBinaryData(t *testing.T) {
+	for _, tc := range []struct {
+		name, body, want string
+		tty              bool
+		err              string
+	}{
+		{name: "piped binary stays byte exact", body: "PK\x03\x04\x00\x01", want: "PK\x03\x04\x00\x01"},
+		{name: "terminal text", body: "hello\nworld\n", want: "hello\nworld\n", tty: true},
+		{name: "terminal text loses escapes", body: "title\x1b]0;PWNED\x07\n", want: "title]0;PWNED\n", tty: true},
+		{name: "terminal binary", body: "PK\x03\x04\x00\x01", tty: true, err: "refusing to write binary data to the terminal"},
+		{name: "terminal invalid UTF-8", body: "abc\xff\xfedef", tty: true, err: "refusing to write binary data"},
+		{name: "terminal UTF-8 cut at the sniff boundary", body: strings.Repeat("a", binarySniffLength-1) + "é", want: strings.Repeat("a", binarySniffLength-1) + "é", tty: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			deps, out, _ := testDeps(t)
+			deps.Streams = terminal.Streams{Out: out, OutTTY: tc.tty}
+			a := &application{deps: deps}
+			cmd := &cobra.Command{}
+			cmd.SetContext(context.Background())
+			cmd.SetOut(out)
+			err := a.download(cmd, downloadFlags{output: "-"}, "-", strings.NewReader(tc.body))
+			if tc.err != "" {
+				require.ErrorContains(t, err, tc.err)
+				require.Empty(t, out.String())
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, tc.want, out.String())
+		})
+	}
+}
+
+func TestDownloadFlagsRegisterGhNames(t *testing.T) {
+	var flags downloadFlags
+	cmd := &cobra.Command{}
+	flags.register(cmd)
+	require.NoError(t, cmd.Flags().Parse([]string{"-O", "file", "--clobber"}))
+	require.Equal(t, downloadFlags{output: "file", clobber: true}, flags)
+	require.NoError(t, cmd.Flags().Parse([]string{"--output-file", "legacy", "-D", "dir", "--skip-existing"}))
+	require.Equal(t, "legacy", flags.output)
+	require.Equal(t, "dir", flags.dir)
+	require.True(t, cmd.Flags().Lookup("output-file").Hidden)
 }

internal/app/integration_helpers_test.go

@@ -197,7 +197,7 @@ func verifyIssueAttachmentLifecycle(t *testing.T, deps Dependencies, group, repo
 	require.Equal(t, "Disposable attachment content\n", string(content))
 	info, err := os.Stat(downloaded)
 	require.NoError(t, err)
-	require.Equal(t, os.FileMode(0o600), info.Mode().Perm())
+	require.Equal(t, umaskMode(t), info.Mode().Perm())
 	deleted := false
 	defer func() {
 		if !deleted {