From 85728351603ec6c7ff97211449e637bbceece934 Mon Sep 17 00:00:00 2001 From: Gusted Date: Thu, 12 Mar 2026 20:29:10 +0100 Subject: [PATCH] fix: don't trip deleting attachment with missing permission error (#11642) - Deleting attachments are also seen as updating attachments due to the frontend always sending a field to edit the name even if the name didn't change. This was not reflected in the unit tests. - Refactor the updating attachment logic to be more flexible if a attachment does not exist, because it was just deleted or because someone is being malicious. - Resolves forgejo/forgejo#11636 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11642 Reviewed-by: Mathieu Fenniak Co-authored-by: Gusted Co-committed-by: Gusted --- services/release/release.go | 20 +++++----------- services/release/release_test.go | 40 +++++++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/services/release/release.go b/services/release/release.go index 9536ebf6dd..c6492e00a0 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -267,7 +267,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo addAttachmentUUIDs := make(container.Set[string]) delAttachmentUUIDs := make(container.Set[string]) updateAttachmentUUIDs := make(container.Set[string]) - updateAttachments := make(container.Set[*AttachmentChange]) + updateAttachments := map[string]*AttachmentChange{} for _, attachmentChange := range attachmentChanges { switch attachmentChange.Action { @@ -305,7 +305,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo delAttachmentUUIDs.Add(attachmentChange.UUID) case "update": updateAttachmentUUIDs.Add(attachmentChange.UUID) - updateAttachments.Add(attachmentChange) + updateAttachments[attachmentChange.UUID] = attachmentChange default: if attachmentChange.Action == "" { return errors.New("missing attachment action") @@ -318,16 +318,12 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo return fmt.Errorf("AddReleaseAttachments: %w", err) } - deletedUUIDs := make(container.Set[string]) if len(delAttachmentUUIDs) > 0 { // Check delAttachments delAttachments, err := repo_model.FindRepoAttachmentsByUUID(ctx, rel.RepoID, delAttachmentUUIDs.Values(), repo_model.FindAttachmentOptions{ReleaseID: rel.ID}) if err != nil { return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", delAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - for _, attach := range delAttachments { - deletedUUIDs.Add(attach.UUID) - } if _, err := repo_model.DeleteAttachments(ctx, delAttachments, true); err != nil { return fmt.Errorf("DeleteAttachments [uuids: %v]: %w", delAttachmentUUIDs, err) @@ -341,16 +337,12 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo return fmt.Errorf("FindRepoAttachmentsByUUID[uuids=%q,repoID=%d,releaseID=%d]: %w", updateAttachmentUUIDs.Values(), rel.RepoID, rel.ID, err) } - if len(attachments) != len(updateAttachments) { - return util.SilentWrap{ - Message: "update attachment of release permission denied", - Err: util.ErrPermissionDenied, + for _, attachment := range attachments { + attachmentChange, ok := updateAttachments[attachment.UUID] + if !ok { + continue } - } - } - for attachmentChange := range updateAttachments { - if !deletedUUIDs.Contains(attachmentChange.UUID) { if err = repo_model.UpdateAttachmentByUUID(ctx, &repo_model.Attachment{ UUID: attachmentChange.UUID, Name: attachmentChange.Name, diff --git a/services/release/release_test.go b/services/release/release_test.go index e05efd9658..b29397b174 100644 --- a/services/release/release_test.go +++ b/services/release/release_test.go @@ -14,7 +14,6 @@ import ( "forgejo.org/modules/git" "forgejo.org/modules/gitrepo" "forgejo.org/modules/test" - "forgejo.org/modules/util" "forgejo.org/services/attachment" "github.com/stretchr/testify/assert" @@ -403,13 +402,48 @@ func TestRelease_Update(t *testing.T) { require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) assert.Empty(t, release.Attachments) - require.ErrorIs(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ + require.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ { Action: "update", Name: "test2.txt", UUID: "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a13", }, - }), util.ErrPermissionDenied) + })) + require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) + assert.Empty(t, release.Attachments) + }) + + t.Run("Delete and edit", func(t *testing.T) { + // Add a attachment. + attach, err := attachment.NewAttachment(t.Context(), &repo_model.Attachment{ + RepoID: repo.ID, + UploaderID: user.ID, + Name: "test.txt", + }, strings.NewReader("beep"), 4) + require.NoError(t, err) + + require.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ + { + Action: "add", + Type: "attachment", + UUID: attach.UUID, + }, + })) + require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) + assert.Len(t, release.Attachments, 1) + + // Delete and edit at the same time, this is identical how the browser would delete a attachment. + require.NoError(t, UpdateRelease(t.Context(), user, gitRepo, release, false, []*AttachmentChange{ + { + Action: "update", + Name: "test.txt", + UUID: attach.UUID, + }, + { + Action: "delete", + UUID: attach.UUID, + }, + })) require.NoError(t, repo_model.GetReleaseAttachments(t.Context(), release)) assert.Empty(t, release.Attachments) })