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) {

tests/integration/attachment_test.go

@@ -34,6 +34,14 @@ func testGeneratePngBytes() []byte {
 }
 
 func testCreateIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, content []byte, expectedStatus int) string {
+	return testCreateAttachment(t, session, csrf, repoURL, "issues", filename, content, expectedStatus)
+}
+
+func testCreateReleaseAttachment(t *testing.T, session *TestSession, csrf, repoURL, filename string, content []byte, expectedStatus int) string {
+	return testCreateAttachment(t, session, csrf, repoURL, "releases", filename, content, expectedStatus)
+}
+
+func testCreateAttachment(t *testing.T, session *TestSession, csrf, repoURL, issueOrRelease, filename string, content []byte, expectedStatus int) string {
 	body := &bytes.Buffer{}
 
 	// Setup multi-part
@@ -45,7 +53,7 @@ func testCreateIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL
 	err = writer.Close()
 	assert.NoError(t, err)
 
-	req := NewRequestWithBody(t, "POST", repoURL+"/issues/attachments", body)
+	req := NewRequestWithBody(t, "POST", repoURL+"/"+issueOrRelease+"/attachments", body)
 	req.Header.Add("X-Csrf-Token", csrf)
 	req.Header.Add("Content-Type", writer.FormDataContentType())
 	resp := session.MakeRequest(t, req, expectedStatus)
@@ -58,12 +66,25 @@ func testCreateIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL
 	return obj["uuid"]
 }
 
+func testDeleteIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL, uuid string, expectedStatus int) {
+	req := NewRequestWithValues(t, "POST", repoURL+"/issues/attachments/remove", map[string]string{"file": uuid})
+	req.Header.Add("X-Csrf-Token", csrf)
+	session.MakeRequest(t, req, expectedStatus)
+}
+
+func testDeleteReleaseAttachment(t *testing.T, session *TestSession, csrf, repoURL, uuid string, expectedStatus int) {
+	req := NewRequestWithValues(t, "POST", repoURL+"/releases/attachments/remove", map[string]string{"file": uuid})
+	req.Header.Add("X-Csrf-Token", csrf)
+	session.MakeRequest(t, req, expectedStatus)
+}
+
 func TestAttachments(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	t.Run("CreateAnonymousAttachment", testCreateAnonymousAttachment)
 	t.Run("CreateUser2IssueAttachment", testCreateUser2IssueAttachment)
 	t.Run("UploadAttachmentDeleteTemp", testUploadAttachmentDeleteTemp)
 	t.Run("GetAttachment", testGetAttachment)
+	t.Run("DeleteAttachmentPermissions", testDeleteAttachmentPermissions)
 }
 
 func testUploadAttachmentDeleteTemp(t *testing.T) {
@@ -159,3 +180,31 @@ func testGetAttachment(t *testing.T) {
 		})
 	}
 }
+
+func testDeleteAttachmentPermissions(t *testing.T) {
+	const repoURL = "user2/repo1"
+
+	ownerSession := loginUser(t, "user2")
+	readonlySession := loginUser(t, "user5")
+
+	ownerCSRF := GetUserCSRFToken(t, ownerSession)
+	readonlyCSRF := GetUserCSRFToken(t, readonlySession)
+
+	issueFromOwner := testCreateIssueAttachment(t, ownerSession, ownerCSRF, repoURL, "owner-issue.png", testGeneratePngBytes(), http.StatusOK)
+	testDeleteIssueAttachment(t, readonlySession, readonlyCSRF, repoURL, issueFromOwner, http.StatusForbidden)
+
+	issueFromReader := testCreateIssueAttachment(t, readonlySession, readonlyCSRF, repoURL, "reader-issue.png", testGeneratePngBytes(), http.StatusOK)
+	testDeleteIssueAttachment(t, ownerSession, ownerCSRF, repoURL, issueFromReader, http.StatusOK)
+
+	testCreateReleaseAttachment(t, readonlySession, readonlyCSRF, repoURL, "reader-release.png", testGeneratePngBytes(), http.StatusNotFound)
+
+	crossRepoUUID := testCreateIssueAttachment(t, ownerSession, ownerCSRF, repoURL, "cross-repo.png", testGeneratePngBytes(), http.StatusOK)
+	testDeleteIssueAttachment(t, ownerSession, ownerCSRF, "user2/repo2", crossRepoUUID, http.StatusBadRequest)
+	testDeleteIssueAttachment(t, ownerSession, ownerCSRF, repoURL, crossRepoUUID, http.StatusOK)
+
+	releaseUUID := testCreateReleaseAttachment(t, ownerSession, ownerCSRF, repoURL, "reader-release.png", testGeneratePngBytes(), http.StatusOK)
+	testDeleteReleaseAttachment(t, ownerSession, ownerCSRF, repoURL, releaseUUID, http.StatusOK)
+
+	// test deleting release attachment from another repo
+	testDeleteReleaseAttachment(t, ownerSession, ownerCSRF, "user2/repo2", crossRepoUUID, http.StatusBadRequest)
+}