diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index 43e97beb27..5f5423fafe 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/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) diff --git a/routers/web/repo/attachment.go b/routers/web/repo/attachment.go index 54200d8de8..bff91b51a7 100644 --- a/routers/web/repo/attachment.go +++ b/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) { diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index 18efde7214..18c14314b0 100644 --- a/tests/integration/attachment_test.go +++ b/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) +}