Fix delete attachment check (#36320)

routers/api/v1/repo/release_attachment.go

@@ -398,7 +398,6 @@ func DeleteReleaseAttachment(ctx *context.APIContext) {
 		ctx.APIErrorNotFound()
 		return
 	}
-	// FIXME Should prove the existence of the given repo, but results in unnecessary database requests
 
 	if err := repo_model.DeleteAttachment(ctx, attach, true); err != nil {
 		ctx.APIErrorInternal(err)

routers/web/repo/attachment.go

@@ -4,11 +4,12 @@
 package repo
 
 import (
-	"fmt"
 	"net/http"
 
+	issues_model "code.gitea.io/gitea/models/issues"
 	access_model "code.gitea.io/gitea/models/perm/access"
 	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/models/unit"
 	"code.gitea.io/gitea/modules/httpcache"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -40,7 +41,7 @@ func uploadAttachment(ctx *context.Context, repoID int64, allowedTypes string) {
 
 	file, header, err := ctx.Req.FormFile("file")
 	if err != nil {
-		ctx.HTTPError(http.StatusInternalServerError, fmt.Sprintf("FormFile: %v", err))
+		ctx.ServerError("FormFile", err)
 		return
 	}
 	defer file.Close()
@@ -56,7 +57,7 @@ func uploadAttachment(ctx *context.Context, repoID int64, allowedTypes string) {
 			ctx.HTTPError(http.StatusBadRequest, err.Error())
 			return
 		}
-		ctx.HTTPError(http.StatusInternalServerError, fmt.Sprintf("NewAttachment: %v", err))
+		ctx.ServerError("UploadAttachmentGeneralSizeLimit", err)
 		return
 	}
 
@@ -74,13 +75,44 @@ func DeleteAttachment(ctx *context.Context) {
 		ctx.HTTPError(http.StatusBadRequest, err.Error())
 		return
 	}
-	if !ctx.IsSigned || (ctx.Doer.ID != attach.UploaderID) {
+
+	if !ctx.IsSigned {
 		ctx.HTTPError(http.StatusForbidden)
 		return
 	}
+
+	if attach.RepoID != ctx.Repo.Repository.ID {
+		ctx.HTTPError(http.StatusBadRequest, "attachment does not belong to this repository")
+		return
+	}
+
+	if ctx.Doer.ID != attach.UploaderID {
+		if attach.IssueID > 0 {
+			issue, err := issues_model.GetIssueByID(ctx, attach.IssueID)
+			if err != nil {
+				ctx.ServerError("GetIssueByID", err)
+				return
+			}
+			if !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) {
+				ctx.HTTPError(http.StatusForbidden)
+				return
+			}
+		} else if attach.ReleaseID > 0 {
+			if !ctx.Repo.Permission.CanWrite(unit.TypeReleases) {
+				ctx.HTTPError(http.StatusForbidden)
+				return
+			}
+		} else {
+			if !ctx.Repo.Permission.IsAdmin() && !ctx.Repo.Permission.IsOwner() {
+				ctx.HTTPError(http.StatusForbidden)
+				return
+			}
+		}
+	}
+
 	err = repo_model.DeleteAttachment(ctx, attach, true)
 	if err != nil {
-		ctx.HTTPError(http.StatusInternalServerError, fmt.Sprintf("DeleteAttachment: %v", err))
+		ctx.ServerError("DeleteAttachment", err)
 		return
 	}
 	ctx.JSON(http.StatusOK, map[string]string{
@@ -114,7 +146,7 @@ func ServeAttachment(ctx *context.Context, uuid string) {
 	} else { // If we have the repository we check access
 		perm, err := access_model.GetUserRepoPermission(ctx, repository, ctx.Doer)
 		if err != nil {
-			ctx.HTTPError(http.StatusInternalServerError, "GetUserRepoPermission", err.Error())
+			ctx.ServerError("GetUserRepoPermission", err)
 			return
 		}
 		if !perm.CanRead(unitType) {