fix: serialize application image changes

Concurrent uploads for the same application could both read the same
old image, both delete it, and leave the image written by the request
whose database update was overwritten on disk without any reference.

Serialize image uploads, image removal and application deletion, and
read the application again under the lock before replacing its image.

Fixes #1047

Author: SulimanAbdulrazzaq

api/application.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/gin-gonic/gin"
 	"github.com/gotify/server/v3/auth"
@@ -29,6 +30,11 @@ type ApplicationDatabase interface {
 type ApplicationAPI struct {
 	DB       ApplicationDatabase
 	ImageDir string
+
+	// imageMu serializes changes to application images, so that concurrent
+	// requests neither delete an image that is still referenced nor leave
+	// an unreferenced image behind.
+	imageMu sync.Mutex
 }
 
 // Application Params Model
@@ -188,6 +194,9 @@ func (a *ApplicationAPI) GetApplications(ctx *gin.Context) {
 //	        $ref: "#/definitions/Error"
 func (a *ApplicationAPI) DeleteApplication(ctx *gin.Context) {
 	withID(ctx, "id", func(id uint) {
+		a.imageMu.Lock()
+		defer a.imageMu.Unlock()
+
 		app, err := a.DB.GetApplicationByID(id)
 		if success := successOrAbort(ctx, 500, err); !success {
 			return
@@ -434,6 +443,20 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
 				return
 			}
 
+			a.imageMu.Lock()
+			defer a.imageMu.Unlock()
+
+			// Read the application again, a concurrent request may have
+			// changed or deleted its image in the meantime.
+			app, err = a.DB.GetApplicationByID(id)
+			if success := successOrAbort(ctx, 500, err); !success {
+				return
+			}
+			if app == nil || app.UserID != auth.GetUserID(ctx) {
+				ctx.AbortWithError(404, fmt.Errorf("app with id %d doesn't exists", id))
+				return
+			}
+
 			name := generateNonExistingImageName(a.ImageDir, func() string {
 				return generateImageName() + ext
 			})
@@ -500,6 +523,9 @@ func (a *ApplicationAPI) UploadApplicationImage(ctx *gin.Context) {
 //	        $ref: "#/definitions/Error"
 func (a *ApplicationAPI) RemoveApplicationImage(ctx *gin.Context) {
 	withID(ctx, "id", func(id uint) {
+		a.imageMu.Lock()
+		defer a.imageMu.Unlock()
+
 		app, err := a.DB.GetApplicationByID(id)
 		if success := successOrAbort(ctx, 500, err); !success {
 			return

api/application_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/gin-gonic/gin"
@@ -483,6 +484,49 @@ func (s *ApplicationSuite) Test_UploadAppImage_WithImageFile_DeleteExstingImage(
 	assert.Len(s.T(), listing, 1)
 }
 
+func (s *ApplicationSuite) Test_UploadAppImage_Concurrent_KeepsOnlyReferencedImage() {
+	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))
+
+	// Both uploads read the application before either of them changes it.
+	db := &concurrentReadsAppDB{ApplicationDatabase: s.db, pending: 2}
+	db.wg.Add(2)
+	s.a.DB = db
+
+	var wg sync.WaitGroup
+	for range 2 {
+		cType, buffer, err := upload(map[string]*os.File{"file": mustOpen("../test/assets/image.png")})
+		require.NoError(s.T(), err)
+		recorder := httptest.NewRecorder()
+		ctx, _ := gin.CreateTestContext(recorder)
+		withURL(ctx, "http", "example.com")
+		ctx.Request = httptest.NewRequest("POST", "/irrelevant", &buffer)
+		ctx.Request.Header.Set("Content-Type", cType)
+		test.WithUser(ctx, 5)
+		ctx.Params = gin.Params{{Key: "id", Value: "1"}}
+
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			s.a.UploadApplicationImage(ctx)
+			assert.Equal(s.T(), 200, recorder.Code)
+		}()
+	}
+	wg.Wait()
+
+	app, err := s.db.GetApplicationByID(1)
+	require.NoError(s.T(), err)
+	listing, err := os.ReadDir(s.imageDir.Path())
+	require.NoError(s.T(), err)
+	names := []string{}
+	for _, entry := range listing {
+		names = append(names, entry.Name())
+	}
+	assert.Equal(s.T(), []string{app.Image}, names)
+}
+
 func (s *ApplicationSuite) Test_UploadAppImage_WithTextFile_expectBadRequest() {
 	s.db.User(5).App(1)
 
@@ -762,3 +806,27 @@ func fakeImage(t *testing.T, path string) {
 	err = os.WriteFile(path, data, 0o644)
 	assert.Nil(t, err)
 }
+
+// concurrentReadsAppDB lets the first pending calls to GetApplicationByID
+// return only after all of them have been made.
+type concurrentReadsAppDB struct {
+	ApplicationDatabase
+	mu      sync.Mutex
+	pending int
+	wg      sync.WaitGroup
+}
+
+func (d *concurrentReadsAppDB) GetApplicationByID(id uint) (*model.Application, error) {
+	app, err := d.ApplicationDatabase.GetApplicationByID(id)
+	d.mu.Lock()
+	wait := d.pending > 0
+	if wait {
+		d.pending--
+	}
+	d.mu.Unlock()
+	if wait {
+		d.wg.Done()
+		d.wg.Wait()
+	}
+	return app, err
+}