Skip to content

Commit 14e8c9b

Browse files
lunnylunny
authored
Release attachments must belong to the intended repo (#36347)
1 parent 7b5de59 commit 14e8c9b

9 files changed

Lines changed: 121 additions & 31 deletions

File tree

‎models/repo/attachment.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,11 @@ func GetAttachmentByReleaseIDFileName(ctx context.Context, releaseID int64, file
166166
return attach, nil
167167
}
168168

169+
func GetUnlinkedAttachmentsByUserID(ctx context.Context, userID int64) ([]*Attachment, error) {
170+
attachments := make([]*Attachment, 0, 10)
171+
return attachments, db.GetEngine(ctx).Where("uploader_id = ? AND issue_id = 0 AND release_id = 0 AND comment_id = 0", userID).Find(&attachments)
172+
}
173+
169174
// DeleteAttachment deletes the given attachment and optionally the associated file.
170175
func DeleteAttachment(ctx context.Context, a *Attachment, remove bool) error {
171176
_, err := DeleteAttachments(ctx, []*Attachment{a}, remove)

‎models/repo/attachment_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,19 @@ func TestGetAttachmentsByUUIDs(t *testing.T) {
101101
assert.Equal(t, int64(1), attachList[0].IssueID)
102102
assert.Equal(t, int64(5), attachList[1].IssueID)
103103
}
104+
105+
func TestGetUnlinkedAttachmentsByUserID(t *testing.T) {
106+
assert.NoError(t, unittest.PrepareTestDatabase())
107+
108+
attachments, err := repo_model.GetUnlinkedAttachmentsByUserID(t.Context(), 8)
109+
assert.NoError(t, err)
110+
assert.Len(t, attachments, 1)
111+
assert.Equal(t, int64(10), attachments[0].ID)
112+
assert.Zero(t, attachments[0].IssueID)
113+
assert.Zero(t, attachments[0].ReleaseID)
114+
assert.Zero(t, attachments[0].CommentID)
115+
116+
attachments, err = repo_model.GetUnlinkedAttachmentsByUserID(t.Context(), 1)
117+
assert.NoError(t, err)
118+
assert.Empty(t, attachments)
119+
}

‎models/repo/release.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,13 +174,22 @@ func UpdateReleaseNumCommits(ctx context.Context, rel *Release) error {
174174

175175
// AddReleaseAttachments adds a release attachments
176176
func AddReleaseAttachments(ctx context.Context, releaseID int64, attachmentUUIDs []string) (err error) {
177+
rel, err := GetReleaseByID(ctx, releaseID)
178+
if err != nil {
179+
return err
180+
}
181+
177182
// Check attachments
178183
attachments, err := GetAttachmentsByUUIDs(ctx, attachmentUUIDs)
179184
if err != nil {
180185
return fmt.Errorf("GetAttachmentsByUUIDs [uuids: %v]: %w", attachmentUUIDs, err)
181186
}
182187

183188
for i := range attachments {
189+
if attachments[i].RepoID != rel.RepoID {
190+
return util.NewPermissionDeniedErrorf("attachment belongs to different repository")
191+
}
192+
184193
if attachments[i].ReleaseID != 0 {
185194
return util.NewPermissionDeniedErrorf("release permission denied")
186195
}

‎models/repo/release_test.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"testing"
88

99
"code.gitea.io/gitea/models/unittest"
10+
"code.gitea.io/gitea/modules/util"
1011

1112
"github.com/stretchr/testify/assert"
1213
)
@@ -37,3 +38,16 @@ func Test_FindTagsByCommitIDs(t *testing.T) {
3738
assert.Equal(t, "delete-tag", rels[1].TagName)
3839
assert.Equal(t, "v1.0", rels[2].TagName)
3940
}
41+
42+
func TestAddReleaseAttachmentsRejectsDifferentRepo(t *testing.T) {
43+
assert.NoError(t, unittest.PrepareTestDatabase())
44+
45+
uuid := "a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a12" // attachment 2 belongs to repo 2
46+
err := AddReleaseAttachments(t.Context(), 1, []string{uuid})
47+
assert.Error(t, err)
48+
assert.ErrorIs(t, err, util.ErrPermissionDenied)
49+
50+
attach, err := GetAttachmentByUUID(t.Context(), uuid)
51+
assert.NoError(t, err)
52+
assert.Zero(t, attach.ReleaseID, "attachment should not be linked to release on failure")
53+
}

‎routers/web/repo/attachment.go‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -132,23 +132,40 @@ func ServeAttachment(ctx *context.Context, uuid string) {
132132
return
133133
}
134134

135-
repository, unitType, err := repo_service.LinkedRepository(ctx, attach)
135+
// prevent visiting attachment from other repository directly
136+
if ctx.Repo.Repository != nil && ctx.Repo.Repository.ID != attach.RepoID {
137+
ctx.HTTPError(http.StatusNotFound)
138+
return
139+
}
140+
141+
unitType, err := repo_service.GetAttachmentLinkedType(ctx, attach)
136142
if err != nil {
137-
ctx.ServerError("LinkedRepository", err)
143+
ctx.ServerError("GetAttachmentLinkedType", err)
138144
return
139145
}
140146

141-
if repository == nil { // If not linked
147+
if unitType == unit.TypeInvalid { // unlinked attachment can only be accessed by the uploader
142148
if !(ctx.IsSigned && attach.UploaderID == ctx.Doer.ID) { // We block if not the uploader
143149
ctx.HTTPError(http.StatusNotFound)
144150
return
145151
}
146-
} else { // If we have the repository we check access
147-
perm, err := access_model.GetUserRepoPermission(ctx, repository, ctx.Doer)
148-
if err != nil {
149-
ctx.ServerError("GetUserRepoPermission", err)
150-
return
152+
} else { // If we have the linked type, we need to check access
153+
var perm access_model.Permission
154+
if ctx.Repo.Repository == nil {
155+
repo, err := repo_model.GetRepositoryByID(ctx, attach.RepoID)
156+
if err != nil {
157+
ctx.ServerError("GetRepositoryByID", err)
158+
return
159+
}
160+
perm, err = access_model.GetUserRepoPermission(ctx, repo, ctx.Doer)
161+
if err != nil {
162+
ctx.ServerError("GetUserRepoPermission", err)
163+
return
164+
}
165+
} else {
166+
perm = ctx.Repo.Permission
151167
}
168+
152169
if !perm.CanRead(unitType) {
153170
ctx.HTTPError(http.StatusNotFound)
154171
return

‎services/repository/repository.go‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -221,28 +221,25 @@ func MakeRepoPrivate(ctx context.Context, repo *repo_model.Repository) (err erro
221221
})
222222
}
223223

224-
// LinkedRepository returns the linked repo if any
225-
func LinkedRepository(ctx context.Context, a *repo_model.Attachment) (*repo_model.Repository, unit.Type, error) {
224+
// GetAttachmentLinkedType returns the linked type of attachment if any
225+
func GetAttachmentLinkedType(ctx context.Context, a *repo_model.Attachment) (unit.Type, error) {
226226
if a.IssueID != 0 {
227227
iss, err := issues_model.GetIssueByID(ctx, a.IssueID)
228228
if err != nil {
229-
return nil, unit.TypeIssues, err
229+
return unit.TypeIssues, err
230230
}
231-
repo, err := repo_model.GetRepositoryByID(ctx, iss.RepoID)
232231
unitType := unit.TypeIssues
233232
if iss.IsPull {
234233
unitType = unit.TypePullRequests
235234
}
236-
return repo, unitType, err
237-
} else if a.ReleaseID != 0 {
238-
rel, err := repo_model.GetReleaseByID(ctx, a.ReleaseID)
239-
if err != nil {
240-
return nil, unit.TypeReleases, err
241-
}
242-
repo, err := repo_model.GetRepositoryByID(ctx, rel.RepoID)
243-
return repo, unit.TypeReleases, err
235+
return unitType, nil
236+
}
237+
238+
if a.ReleaseID != 0 {
239+
_, err := repo_model.GetReleaseByID(ctx, a.ReleaseID)
240+
return unit.TypeReleases, err
244241
}
245-
return nil, -1, nil
242+
return unit.TypeInvalid, nil
246243
}
247244

248245
// CheckDaemonExportOK creates/removes git-daemon-export-ok for git-daemon...

‎services/repository/repository_test.go‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,28 +16,24 @@ import (
1616
"github.com/stretchr/testify/require"
1717
)
1818

19-
func TestLinkedRepository(t *testing.T) {
19+
func TestAttachLinkedType(t *testing.T) {
2020
assert.NoError(t, unittest.PrepareTestDatabase())
2121
testCases := []struct {
2222
name string
2323
attachID int64
24-
expectedRepo *repo_model.Repository
2524
expectedUnitType unit.Type
2625
}{
27-
{"LinkedIssue", 1, &repo_model.Repository{ID: 1}, unit.TypeIssues},
28-
{"LinkedComment", 3, &repo_model.Repository{ID: 1}, unit.TypePullRequests},
29-
{"LinkedRelease", 9, &repo_model.Repository{ID: 1}, unit.TypeReleases},
30-
{"Notlinked", 10, nil, -1},
26+
{"LinkedIssue", 1, unit.TypeIssues},
27+
{"LinkedComment", 3, unit.TypePullRequests},
28+
{"LinkedRelease", 9, unit.TypeReleases},
29+
{"Notlinked", 10, unit.TypeInvalid},
3130
}
3231
for _, tc := range testCases {
3332
t.Run(tc.name, func(t *testing.T) {
3433
attach, err := repo_model.GetAttachmentByID(t.Context(), tc.attachID)
3534
assert.NoError(t, err)
36-
repo, unitType, err := LinkedRepository(t.Context(), attach)
35+
unitType, err := GetAttachmentLinkedType(t.Context(), attach)
3736
assert.NoError(t, err)
38-
if tc.expectedRepo != nil {
39-
assert.Equal(t, tc.expectedRepo.ID, repo.ID)
40-
}
4137
assert.Equal(t, tc.expectedUnitType, unitType)
4238
})
4339
}

‎services/user/user.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,11 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
239239
if err := deleteUser(ctx, u, purge); err != nil {
240240
return fmt.Errorf("DeleteUser: %w", err)
241241
}
242+
243+
// Finally delete any unlinked attachments, this will also delete the attached files
244+
if err := deleteUserUnlinkedAttachments(ctx, u); err != nil {
245+
return fmt.Errorf("deleteUserUnlinkedAttachments: %w", err)
246+
}
242247
return nil
243248
}); err != nil {
244249
return err
@@ -269,6 +274,19 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error {
269274
return nil
270275
}
271276

277+
func deleteUserUnlinkedAttachments(ctx context.Context, u *user_model.User) error {
278+
attachments, err := repo_model.GetUnlinkedAttachmentsByUserID(ctx, u.ID)
279+
if err != nil {
280+
return fmt.Errorf("GetUnlinkedAttachmentsByUserID: %w", err)
281+
}
282+
for _, attach := range attachments {
283+
if err := repo_model.DeleteAttachment(ctx, attach, true); err != nil {
284+
return fmt.Errorf("DeleteAttachment ID[%d]: %w", attach.ID, err)
285+
}
286+
}
287+
return nil
288+
}
289+
272290
// DeleteInactiveUsers deletes all inactive users and their email addresses.
273291
func DeleteInactiveUsers(ctx context.Context, olderThan time.Duration) error {
274292
inactiveUsers, err := user_model.GetInactiveUsers(ctx, olderThan)

‎services/user/user_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,24 @@ func TestDeleteUser(t *testing.T) {
6363
assert.Error(t, DeleteUser(t.Context(), org, false))
6464
}
6565

66+
func TestDeleteUserUnlinkedAttachments(t *testing.T) {
67+
t.Run("DeleteExisting", func(t *testing.T) {
68+
assert.NoError(t, unittest.PrepareTestDatabase())
69+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 8})
70+
unittest.AssertExistsAndLoadBean(t, &repo_model.Attachment{ID: 10})
71+
72+
assert.NoError(t, deleteUserUnlinkedAttachments(t.Context(), user))
73+
unittest.AssertNotExistsBean(t, &repo_model.Attachment{ID: 10})
74+
})
75+
76+
t.Run("NoUnlinkedAttachments", func(t *testing.T) {
77+
assert.NoError(t, unittest.PrepareTestDatabase())
78+
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
79+
80+
assert.NoError(t, deleteUserUnlinkedAttachments(t.Context(), user))
81+
})
82+
}
83+
6684
func TestPurgeUser(t *testing.T) {
6785
test := func(userID int64) {
6886
assert.NoError(t, unittest.PrepareTestDatabase())

0 commit comments

Comments
 (0)