diff --git a/api/application.go b/api/application.go index 90ced9239..7617da0de 100644 --- a/api/application.go +++ b/api/application.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "mime/multipart" "net/http" "os" "path/filepath" @@ -447,20 +448,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)) @@ -542,6 +548,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 diff --git a/api/application_test.go b/api/application_test.go index 623ed8b50..dfa8edf52 100644 --- a/api/application_test.go +++ b/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 }