fix: clean up application images when an upload fails

A failed write left a partially written image file behind, and a failed
database update after saving the new image had already deleted the image
the application still referenced.

Write the uploaded image with a helper that removes the file if it can't
be written completely, and only delete the old image once the
application has been updated. If the update fails, remove the new image
instead.

Author: SulimanAbdulrazzaq

api/application.go

@@ -3,6 +3,8 @@ package api
 import (
 	"errors"
 	"fmt"
+	"io"
+	"mime/multipart"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -438,20 +440,25 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
 				return generateImageName() + ext
 			})
 
-			err = ctx.SaveUploadedFile(file, a.ImageDir+name)
+			err = saveImage(file, a.ImageDir+name)
 			if err != nil {
 				ctx.AbortWithError(500, err)
 				return
 			}
 
-			if app.Image != "" {
-				os.Remove(a.ImageDir + app.Image)
-			}
-
+			oldImage := app.Image
 			app.Image = name
-			if success := successOrAbort(ctx, 500, a.DB.UpdateApplication(app)); !success {
+			if err := a.DB.UpdateApplication(app); err != nil {
+				// The application still references the old image, so keep
+				// it and drop the new one that nothing references.
+				os.Remove(a.ImageDir + name)
+				ctx.AbortWithError(500, err)
 				return
 			}
+
+			if oldImage != "" {
+				os.Remove(a.ImageDir + oldImage)
+			}
 			ctx.JSON(200, withResolvedImage(app))
 		} else {
 			ctx.AbortWithError(404, fmt.Errorf("app with id %d doesn't exists", id))
@@ -533,6 +540,34 @@ func withResolvedImage(app *model.Application) *model.Application {
 	return app
 }
 
+// saveImage writes the uploaded file to dst. If it can't be written
+// completely, the partially written file is removed.
+func saveImage(file *multipart.FileHeader, dst string) error {
+	src, err := file.Open()
+	if err != nil {
+		return err
+	}
+	defer src.Close()
+	return writeImage(dst, src)
+}
+
+func writeImage(dst string, src io.Reader) (err error) {
+	out, err := os.Create(dst)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		if closeErr := out.Close(); err == nil {
+			err = closeErr
+		}
+		if err != nil {
+			os.Remove(dst)
+		}
+	}()
+	_, err = io.Copy(out, src)
+	return err
+}
+
 func exist(path string) bool {
 	if _, err := os.Stat(path); os.IsNotExist(err) {
 		return false

api/application_test.go

@@ -483,6 +483,43 @@ func (s *ApplicationSuite) Test_UploadAppImage_WithImageFile_DeleteExstingImage(
 	assert.Len(s.T(), listing, 1)
 }
 
+func (s *ApplicationSuite) Test_UploadAppImage_UpdateFails_KeepsExistingImage() {
+	existingImageName := "existing.png"
+	s.db.User(5)
+	s.db.CreateApplication(&model.Application{UserID: 5, ID: 1, Image: existingImageName})
+	fakeImage(s.T(), s.imageDir.Path(existingImageName))
+	s.a.DB = &failingUpdateAppDB{ApplicationDatabase: s.db}
+
+	cType, buffer, err := upload(map[string]*os.File{"file": mustOpen("../test/assets/image.png")})
+	require.NoError(s.T(), err)
+	s.ctx.Request = httptest.NewRequest("POST", "/irrelevant", &buffer)
+	s.ctx.Request.Header.Set("Content-Type", cType)
+	test.WithUser(s.ctx, 5)
+	s.ctx.Params = gin.Params{{Key: "id", Value: "1"}}
+
+	s.a.UploadApplicationImage(s.ctx)
+
+	assert.Equal(s.T(), 500, s.recorder.Code)
+	app, err := s.db.GetApplicationByID(1)
+	require.NoError(s.T(), err)
+	assert.Equal(s.T(), existingImageName, app.Image)
+	listing, err := os.ReadDir(s.imageDir.Path())
+	require.NoError(s.T(), err)
+	require.Len(s.T(), listing, 1)
+	assert.Equal(s.T(), existingImageName, listing[0].Name())
+}
+
+func (s *ApplicationSuite) Test_WriteImage_RemovesPartialFileOnError() {
+	dst := s.imageDir.Path("partial.png")
+	src := io.MultiReader(strings.NewReader("partial image data"), errReader{errors.New("connection reset")})
+
+	err := writeImage(dst, src)
+
+	assert.EqualError(s.T(), err, "connection reset")
+	_, err = os.Stat(dst)
+	assert.True(s.T(), os.IsNotExist(err), "the partially written file must be removed")
+}
+
 func (s *ApplicationSuite) Test_UploadAppImage_WithTextFile_expectBadRequest() {
 	s.db.User(5).App(1)
 
@@ -762,3 +799,16 @@ func fakeImage(t *testing.T, path string) {
 	err = os.WriteFile(path, data, 0o644)
 	assert.Nil(t, err)
 }
+
+// failingUpdateAppDB fails every UpdateApplication call.
+type failingUpdateAppDB struct {
+	ApplicationDatabase
+}
+
+func (d *failingUpdateAppDB) UpdateApplication(*model.Application) error {
+	return errors.New("update failed")
+}
+
+type errReader struct{ err error }
+
+func (r errReader) Read([]byte) (int, error) { return 0, r.err }