peezy-tech/jojo
2
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-05-12 22:10:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

[v11.0/forgejo] fix: don't trip deleting attachment with missing permission error (#11679)

Browse source 
**Backport of forgejo/forgejo#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 <mfenniak@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>
(cherry picked from commit 8572835160)

<!--start release-notes-assistant-->

## Release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Bug fixes
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/11679): <!--number 11679 --><!--line 0 --><!--description Zml4OiBkb24ndCB0cmlwIGRlbGV0aW5nIGF0dGFjaG1lbnQgd2l0aCBtaXNzaW5nIHBlcm1pc3Npb24gZXJyb3I=-->fix: don't trip deleting attachment with missing permission error<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11679
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
This commit is contained in:
Gusted 2026-03-14 19:11:33 +01:00 • committed by Gusted
parent 865b590bb5
commit 91e3c36c69
2 changed files with 43 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

20
services/release/release.go
View file

@ -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 fmt.Errorf("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,

40
services/release/release_test.go
View file

@ -14,7 +14,6 @@ import (
user_model "forgejo.org/models/user"
"forgejo.org/modules/git"
"forgejo.org/modules/gitrepo"
"forgejo.org/modules/util"
"forgejo.org/services/attachment"
_ "forgejo.org/models/actions"
@ -406,13 +405,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)
})