Skip to content

Commit e96ee3c

Browse files
gianfrancopianagianfrancopiana
committed
Handle download close errors
1 parent 4c8b42d commit e96ee3c

5 files changed

Lines changed: 32 additions & 35 deletions

File tree

‎internal/cmd/admin/products/download.go‎

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package products
22

33
import (
4+
"bytes"
45
"context"
56
"crypto/rand"
67
"encoding/json"
@@ -54,7 +55,7 @@ func newFilesDownloadCmd() *cobra.Command {
5455
path := cmdutil.JoinPath("products", productID, "files", fileID, "download_url")
5556
return admincmd.Run(fetchOpts, "Fetching download URL...", func(client *adminapi.Client) (json.RawMessage, error) {
5657
return client.Get(path, url.Values{})
57-
}, func(data json.RawMessage) error {
58+
}, func(data json.RawMessage) (err error) {
5859
resp, err := cmdutil.DecodeJSON[fileDownloadURLResponse](data)
5960
if err != nil {
6061
return err
@@ -96,7 +97,7 @@ func newFilesDownloadCmd() *cobra.Command {
9697
}
9798
installDest = filepath.Join(filepath.Dir(stagingDir), filepath.Base(dest))
9899
defer func() {
99-
stagedDownload.Close()
100+
err = errors.Join(err, closeDownloadFile(stagedDownload))
100101
os.Remove(stagedDownload.Name())
101102
stagingDirLock.Close()
102103
os.Remove(stagingDir)
@@ -106,17 +107,11 @@ func newFilesDownloadCmd() *cobra.Command {
106107
if err != nil {
107108
return err
108109
}
109-
if machineOutput != nil {
110-
defer func() {
111-
machineOutput.Close()
112-
os.Remove(machineOutput.Name())
113-
}()
114-
}
115110
if err := downloadToFile(opts, resp.SignedURL, stagedDownload, installDest, force); err != nil {
116111
return err
117112
}
118113
if machineOutput != nil {
119-
_, err := io.Copy(opts.Out(), machineOutput)
114+
_, err := io.Copy(opts.Out(), bytes.NewReader(machineOutput))
120115
return err
121116
}
122117
return renderDownloadSuccess(opts, file, dest)
@@ -185,7 +180,7 @@ func prepareDownloadStaging(dest string) (string, io.Closer, *os.File, error) {
185180
return "", nil, nil, err
186181
}
187182
if err := makeOpenPathPrivate(staged, 0o600); err != nil {
188-
staged.Close()
183+
err = errors.Join(err, closeDownloadFile(staged))
189184
os.Remove(staged.Name())
190185
lock.Close()
191186
os.Remove(dir)
@@ -241,6 +236,13 @@ var downloadStallTimeout, downloadHTTPClient = 2 * time.Minute, &http.Client{Che
241236
func refuseDownloadRedirects(_ *http.Request, _ []*http.Request) error {
242237
return http.ErrUseLastResponse
243238
}
239+
func closeDownloadFile(file *os.File) error {
240+
err := file.Close()
241+
if errors.Is(err, os.ErrClosed) {
242+
return nil
243+
}
244+
return err
245+
}
244246
func downloadToFile(opts cmdutil.Options, signedURL string, staged *os.File, dest string, force bool) error {
245247
downloadURL, err := url.Parse(signedURL)
246248
if err != nil || downloadURL.Scheme != "https" || downloadURL.Host == "" || downloadURL.User != nil {
@@ -325,7 +327,7 @@ func stallAwareDownloadError(ctx, parent context.Context, err error) error {
325327
}
326328
return fmt.Errorf("downloading the file failed: %w", err)
327329
}
328-
func stageDownloadOutput(opts cmdutil.Options, file json.RawMessage, stagingDir, dest string) (*os.File, error) {
330+
func stageDownloadOutput(opts cmdutil.Options, file json.RawMessage, stagingDir, dest string) (output []byte, err error) {
329331
if !opts.UsesJSONOutput() {
330332
return nil, nil
331333
}
@@ -341,24 +343,23 @@ func stageDownloadOutput(opts cmdutil.Options, file json.RawMessage, stagingDir,
341343
if err != nil {
342344
return nil, err
343345
}
344-
if err := makeOpenPathPrivate(staged, 0o600); err != nil {
345-
staged.Close()
346+
defer func() {
347+
err = errors.Join(err, closeDownloadFile(staged))
346348
os.Remove(staged.Name())
349+
}()
350+
if err = makeOpenPathPrivate(staged, 0o600); err != nil {
347351
return nil, err
348352
}
349353
stagedOpts := opts
350354
stagedOpts.Stdout = staged
351-
if err := cmdutil.PrintJSONResponse(stagedOpts, data); err != nil {
352-
staged.Close()
353-
os.Remove(staged.Name())
355+
if err = cmdutil.PrintJSONResponse(stagedOpts, data); err != nil {
354356
return nil, err
355357
}
356-
if _, err := staged.Seek(0, io.SeekStart); err != nil {
357-
staged.Close()
358-
os.Remove(staged.Name())
358+
if err = staged.Close(); err != nil {
359359
return nil, err
360360
}
361-
return staged, nil
361+
output, err = os.ReadFile(staged.Name())
362+
return output, err
362363
}
363364

364365
type stallTimeoutReader struct {

‎internal/cmd/admin/products/download_permissions_unix_common.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
)
1313

1414
func validateFileDownloadSupport(string) error { return nil }
15-
1615
func lockPrivateDirectory(path string, _ bool) (*os.File, string, error) {
1716
path, err := filepath.Abs(path)
1817
if err != nil {

‎internal/cmd/admin/products/download_test.go‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,7 @@ func downloadURLPayload(signedURL string, mutators ...func(map[string]any)) map[
2222
payload := map[string]any{
2323
"signed_url": signedURL,
2424
"external_link": false,
25-
"file": map[string]any{
26-
"id": "f_1", "display_name": "Big Guide", "file_name": "big-guide.pdf", "extension": "PDF", "filegroup": "document", "file_size": 11, "created_at": "2026-05-01T12:00:00Z", "deleted_at": nil,
27-
},
25+
"file": map[string]any{"id": "f_1", "display_name": "Big Guide", "file_name": "big-guide.pdf", "extension": "PDF", "filegroup": "document", "file_size": 11, "created_at": "2026-05-01T12:00:00Z", "deleted_at": nil},
2826
}
2927
for _, mutate := range mutators {
3028
mutate(payload)
@@ -97,14 +95,10 @@ func TestFilesDownloadWritesTrustedPrivateDestination(t *testing.T) {
9795
check(t, gotPath == "/internal/admin/products/abc123/files/f_1/download_url", "unexpected admin path: %s", gotPath)
9896
data, err := os.ReadFile(filepath.Join(reviewDirectoryName, "file-f_1.download"))
9997
check(t, err == nil && string(data) == "hello bytes", "unexpected downloaded file: %q, %v", data, err)
100-
downloaded, err := os.Open(implicitDownloadPath())
101-
must(t, err)
102-
must(t, verifyPrivateMode(downloaded, 0o600))
103-
downloaded.Close()
104-
reviewDir, err := os.Open(reviewDirectoryName)
105-
must(t, err)
106-
must(t, verifyPrivateMode(reviewDir, 0o700))
107-
reviewDir.Close()
98+
downloaded, err := os.Stat(implicitDownloadPath())
99+
check(t, err == nil && downloaded.Mode().Perm() == 0o600, "download mode was not private: %v", err)
100+
reviewDir, err := os.Stat(reviewDirectoryName)
101+
check(t, err == nil && reviewDir.Mode().Perm() == 0o700, "review directory mode was not private: %v", err)
108102
_, err = os.Stat("sitecustomize.py")
109103
check(t, err != nil, "seller filename was used as executable local path")
110104
}
@@ -160,9 +154,8 @@ func TestDownloadFileHelpers(t *testing.T) {
160154
must(t, renderDownloadSuccess(cmdutil.Options{Stdout: io.Discard}, productFile{ID: "id"}, "dest"))
161155
check(t, renderDownloadSuccess(cmdutil.Options{Stdout: closedWriter{}}, productFile{}, "dest") != nil, "expected output error")
162156
closed, err := os.Open(dest)
163-
must(t, err)
164-
check(t, verifyPrivateMode(closed, 0o700) != nil, "expected a private-mode mismatch")
165-
closed.Close()
157+
check(t, err == nil && verifyPrivateMode(closed, 0o700) != nil, "expected a private-mode mismatch: %v", err)
158+
must(t, closed.Close())
166159
check(t, installDownloadedFile(closed, filepath.Join(dir, "unused"), false) != nil, "expected closed-file error")
167160
check(t, verifyPrivateMode(closed, 0o600) != nil && makeOpenPathPrivate(closed, 0o600) != nil, "expected closed-file errors")
168161
check(t, preparePrivateDirectory(dest) != nil, "expected a non-directory review path error")

‎internal/cmd/root.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,9 @@ func rewriteAdminDownloadArgs(root *cobra.Command, args []string) []string {
362362
}
363363

364364
func downloadCommandFlag(command *cobra.Command, arg string) (bool, bool) {
365+
if !strings.HasPrefix(arg, "-") {
366+
return false, false
367+
}
365368
if strings.HasPrefix(arg, "--") {
366369
name, _, hasValue := strings.Cut(strings.TrimPrefix(arg, "--"), "=")
367370
flag := command.Flag(name)

‎internal/cmd/root_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1013,6 +1013,7 @@ func TestRewriteAdminDownloadArgs(t *testing.T) {
10131013
{"inherited flag between commands", []string{"admin", "--no-color", "products", "files", "download", "product", id, "--output", "download"}, []string{"admin", "products", "files", "download", "--no-color", "--output", "download", "--", "product", id}},
10141014
{"two dash IDs and assigned output", []string{"admin", "products", "files", "download", id, id, "--output=/tmp/x"}, []string{"admin", "products", "files", "download", "--output=/tmp/x", "--", id, id}},
10151015
{"attached output resembles ID", []string{"admin", "products", "files", "download", id, "file", "-oAAAAAAAAAAAAAAAAAAAA"}, []string{"admin", "products", "files", "download", "-oAAAAAAAAAAAAAAAAAAAA", "--", id, "file"}},
1016+
{"ordinary ID contains shorthand", []string{"admin", "products", "files", "download", "AoAAAAAAAAAAAAAAAAAAAA", "file", "-oAAAAAAAAAAAAAAAAAAAA"}, []string{"admin", "products", "files", "download", "-oAAAAAAAAAAAAAAAAAAAA", "--", "AoAAAAAAAAAAAAAAAAAAAA", "file"}},
10161017
{"multiple attached ambiguities", []string{"admin", "products", "files", "download", "-oAAAAAAAAAAAAAAAAAAAA", "-oBBBBBBBBBBBBBBBBBBBB", "file"}, []string{"admin", "products", "files", "download", "--", "-oAAAAAAAAAAAAAAAAAAAA", "-oBBBBBBBBBBBBBBBBBBBB", "file"}},
10171018
{"explicit empty output", []string{"admin", "products", "files", "download", "--output=", id, "file"}, []string{"admin", "products", "files", "download", "--output=", "--", id, "file"}},
10181019
}

0 commit comments

Comments
 (0)