Skip to content

Commit bc3d8bf

Browse files
lunnylunny
authored
Fix comment permissions (#28213) (#28216)
backport #28213 This PR will fix some missed checks for private repositories' data on web routes and API routes.
1 parent 7f81110 commit bc3d8bf

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+441
-129
lines changed

‎models/asymkey/gpg_key.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,9 @@ func CountUserGPGKeys(ctx context.Context, userID int64) (int64, error) {
9292
return db.GetEngine(ctx).Where("owner_id=? AND primary_key_id=''", userID).Count(&GPGKey{})
9393
}
9494

95-
// GetGPGKeyByID returns public key by given ID.
96-
func GetGPGKeyByID(ctx context.Context, keyID int64) (*GPGKey, error) {
95+
func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, error) {
9796
key := new(GPGKey)
98-
has, err := db.GetEngine(ctx).ID(keyID).Get(key)
97+
has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", keyID, ownerID).Get(key)
9998
if err != nil {
10099
return nil, err
101100
} else if !has {
@@ -225,19 +224,14 @@ func deleteGPGKey(ctx context.Context, keyID string) (int64, error) {
225224

226225
// DeleteGPGKey deletes GPG key information in database.
227226
func DeleteGPGKey(ctx context.Context, doer *user_model.User, id int64) (err error) {
228-
key, err := GetGPGKeyByID(ctx, id)
227+
key, err := GetGPGKeyForUserByID(ctx, doer.ID, id)
229228
if err != nil {
230229
if IsErrGPGKeyNotExist(err) {
231230
return nil
232231
}
233232
return fmt.Errorf("GetPublicKeyByID: %w", err)
234233
}
235234

236-
// Check if user has access to delete this key.
237-
if !doer.IsAdmin && doer.ID != key.OwnerID {
238-
return ErrGPGKeyAccessDenied{doer.ID, key.ID}
239-
}
240-
241235
ctx, committer, err := db.TxContext(ctx)
242236
if err != nil {
243237
return err

‎models/fixtures/comment.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,12 @@
6666
tree_path: "README.md"
6767
created_unix: 946684812
6868
invalidated: true
69+
70+
-
71+
id: 8
72+
type: 0 # comment
73+
poster_id: 2
74+
issue_id: 4 # in repo_id 2
75+
content: "comment in private pository"
76+
created_unix: 946684811
77+
updated_unix: 946684811

‎models/fixtures/issue.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
priority: 0
6262
is_closed: true
6363
is_pull: false
64-
num_comments: 0
64+
num_comments: 1
6565
created_unix: 946684830
6666
updated_unix: 978307200
6767
is_locked: false

‎models/issues/comment.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1016,6 +1016,7 @@ type FindCommentsOptions struct {
10161016
Type CommentType
10171017
IssueIDs []int64
10181018
Invalidated util.OptionalBool
1019+
IsPull util.OptionalBool
10191020
}
10201021

10211022
// ToConds implements FindOptions interface
@@ -1050,14 +1051,17 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond {
10501051
if !opts.Invalidated.IsNone() {
10511052
cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()})
10521053
}
1054+
if opts.IsPull != util.OptionalBoolNone {
1055+
cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()})
1056+
}
10531057
return cond
10541058
}
10551059

10561060
// FindComments returns all comments according options
10571061
func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) {
10581062
comments := make([]*Comment, 0, 10)
10591063
sess := db.GetEngine(ctx).Where(opts.ToConds())
1060-
if opts.RepoID > 0 {
1064+
if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone {
10611065
sess.Join("INNER", "issue", "issue.id = comment.issue_id")
10621066
}
10631067

‎models/issues/content_history.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,9 +218,9 @@ func GetIssueContentHistoryByID(dbCtx context.Context, id int64) (*ContentHistor
218218
}
219219

220220
// GetIssueContentHistoryAndPrev get a history and the previous non-deleted history (to compare)
221-
func GetIssueContentHistoryAndPrev(dbCtx context.Context, id int64) (history, prevHistory *ContentHistory, err error) {
221+
func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (history, prevHistory *ContentHistory, err error) {
222222
history = &ContentHistory{}
223-
has, err := db.GetEngine(dbCtx).ID(id).Get(history)
223+
has, err := db.GetEngine(dbCtx).Where("id=? AND issue_id=?", id, issueID).Get(history)
224224
if err != nil {
225225
log.Error("failed to get issue content history %v. err=%v", id, err)
226226
return nil, nil, err

‎models/issues/content_history_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@ func TestContentHistory(t *testing.T) {
5858
hasHistory2, _ := issues_model.HasIssueContentHistory(dbCtx, 10, 1)
5959
assert.False(t, hasHistory2)
6060

61-
h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6)
61+
h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6)
6262
assert.EqualValues(t, 6, h6.ID)
6363
assert.EqualValues(t, 5, h6Prev.ID)
6464

6565
// soft-delete
6666
_ = issues_model.SoftDeleteIssueContentHistory(dbCtx, 5)
67-
h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6)
67+
h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6)
6868
assert.EqualValues(t, 6, h6.ID)
6969
assert.EqualValues(t, 4, h6Prev.ID)
7070

‎models/project/project.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) {
311311
return p, nil
312312
}
313313

314+
// GetProjectForRepoByID returns the projects in a repository
315+
func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) {
316+
p := new(Project)
317+
has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(p)
318+
if err != nil {
319+
return nil, err
320+
} else if !has {
321+
return nil, ErrProjectNotExist{ID: id}
322+
}
323+
return p, nil
324+
}
325+
314326
// UpdateProject updates project properties
315327
func UpdateProject(ctx context.Context, p *Project) error {
316328
if !IsCardTypeValid(p.CardType) {

‎models/repo/release.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,21 @@ func GetReleaseByID(ctx context.Context, id int64) (*Release, error) {
207207
return rel, nil
208208
}
209209

210+
// GetReleaseForRepoByID returns release with given ID.
211+
func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, error) {
212+
rel := new(Release)
213+
has, err := db.GetEngine(ctx).
214+
Where("id=? AND repo_id=?", id, repoID).
215+
Get(rel)
216+
if err != nil {
217+
return nil, err
218+
} else if !has {
219+
return nil, ErrReleaseNotExist{id, ""}
220+
}
221+
222+
return rel, nil
223+
}
224+
210225
// FindReleasesOptions describes the conditions to Find releases
211226
type FindReleasesOptions struct {
212227
db.ListOptions

‎models/webhook/webhook.go

Lines changed: 39 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -392,39 +392,40 @@ func CreateWebhooks(ctx context.Context, ws []*Webhook) error {
392392
return db.Insert(ctx, ws)
393393
}
394394

395-
// getWebhook uses argument bean as query condition,
396-
// ID must be specified and do not assign unnecessary fields.
397-
func getWebhook(bean *Webhook) (*Webhook, error) {
398-
has, err := db.GetEngine(db.DefaultContext).Get(bean)
395+
// GetWebhookByID returns webhook of repository by given ID.
396+
func GetWebhookByID(ctx context.Context, id int64) (*Webhook, error) {
397+
bean := new(Webhook)
398+
has, err := db.GetEngine(ctx).ID(id).Get(bean)
399399
if err != nil {
400400
return nil, err
401401
} else if !has {
402-
return nil, ErrWebhookNotExist{ID: bean.ID}
402+
return nil, ErrWebhookNotExist{ID: id}
403403
}
404404
return bean, nil
405405
}
406406

407-
// GetWebhookByID returns webhook of repository by given ID.
408-
func GetWebhookByID(id int64) (*Webhook, error) {
409-
return getWebhook(&Webhook{
410-
ID: id,
411-
})
412-
}
413-
414407
// GetWebhookByRepoID returns webhook of repository by given ID.
415-
func GetWebhookByRepoID(repoID, id int64) (*Webhook, error) {
416-
return getWebhook(&Webhook{
417-
ID: id,
418-
RepoID: repoID,
419-
})
408+
func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) {
409+
webhook := new(Webhook)
410+
has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(webhook)
411+
if err != nil {
412+
return nil, err
413+
} else if !has {
414+
return nil, ErrWebhookNotExist{ID: id}
415+
}
416+
return webhook, nil
420417
}
421418

422419
// GetWebhookByOwnerID returns webhook of a user or organization by given ID.
423-
func GetWebhookByOwnerID(ownerID, id int64) (*Webhook, error) {
424-
return getWebhook(&Webhook{
425-
ID: id,
426-
OwnerID: ownerID,
427-
})
420+
func GetWebhookByOwnerID(ctx context.Context, ownerID, id int64) (*Webhook, error) {
421+
webhook := new(Webhook)
422+
has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", id, ownerID).Get(webhook)
423+
if err != nil {
424+
return nil, err
425+
} else if !has {
426+
return nil, ErrWebhookNotExist{ID: id}
427+
}
428+
return webhook, nil
428429
}
429430

430431
// ListWebhookOptions are options to filter webhooks on ListWebhooksByOpts
@@ -482,38 +483,38 @@ func UpdateWebhookLastStatus(w *Webhook) error {
482483
return err
483484
}
484485

485-
// deleteWebhook uses argument bean as query condition,
486+
// DeleteWebhookByID uses argument bean as query condition,
486487
// ID must be specified and do not assign unnecessary fields.
487-
func deleteWebhook(bean *Webhook) (err error) {
488-
ctx, committer, err := db.TxContext(db.DefaultContext)
488+
func DeleteWebhookByID(ctx context.Context, id int64) (err error) {
489+
ctx, committer, err := db.TxContext(ctx)
489490
if err != nil {
490491
return err
491492
}
492493
defer committer.Close()
493494

494-
if count, err := db.DeleteByBean(ctx, bean); err != nil {
495+
if count, err := db.DeleteByID(ctx, id, new(Webhook)); err != nil {
495496
return err
496497
} else if count == 0 {
497-
return ErrWebhookNotExist{ID: bean.ID}
498-
} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: bean.ID}); err != nil {
498+
return ErrWebhookNotExist{ID: id}
499+
} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: id}); err != nil {
499500
return err
500501
}
501502

502503
return committer.Commit()
503504
}
504505

505506
// DeleteWebhookByRepoID deletes webhook of repository by given ID.
506-
func DeleteWebhookByRepoID(repoID, id int64) error {
507-
return deleteWebhook(&Webhook{
508-
ID: id,
509-
RepoID: repoID,
510-
})
507+
func DeleteWebhookByRepoID(ctx context.Context, repoID, id int64) error {
508+
if _, err := GetWebhookByRepoID(ctx, repoID, id); err != nil {
509+
return err
510+
}
511+
return DeleteWebhookByID(ctx, id)
511512
}
512513

513514
// DeleteWebhookByOwnerID deletes webhook of a user or organization by given ID.
514-
func DeleteWebhookByOwnerID(ownerID, id int64) error {
515-
return deleteWebhook(&Webhook{
516-
ID: id,
517-
OwnerID: ownerID,
518-
})
515+
func DeleteWebhookByOwnerID(ctx context.Context, ownerID, id int64) error {
516+
if _, err := GetWebhookByOwnerID(ctx, ownerID, id); err != nil {
517+
return err
518+
}
519+
return DeleteWebhookByID(ctx, id)
519520
}

‎models/webhook/webhook_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -101,22 +101,22 @@ func TestCreateWebhook(t *testing.T) {
101101

102102
func TestGetWebhookByRepoID(t *testing.T) {
103103
assert.NoError(t, unittest.PrepareTestDatabase())
104-
hook, err := GetWebhookByRepoID(1, 1)
104+
hook, err := GetWebhookByRepoID(db.DefaultContext, 1, 1)
105105
assert.NoError(t, err)
106106
assert.Equal(t, int64(1), hook.ID)
107107

108-
_, err = GetWebhookByRepoID(unittest.NonexistentID, unittest.NonexistentID)
108+
_, err = GetWebhookByRepoID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
109109
assert.Error(t, err)
110110
assert.True(t, IsErrWebhookNotExist(err))
111111
}
112112

113113
func TestGetWebhookByOwnerID(t *testing.T) {
114114
assert.NoError(t, unittest.PrepareTestDatabase())
115-
hook, err := GetWebhookByOwnerID(3, 3)
115+
hook, err := GetWebhookByOwnerID(db.DefaultContext, 3, 3)
116116
assert.NoError(t, err)
117117
assert.Equal(t, int64(3), hook.ID)
118118

119-
_, err = GetWebhookByOwnerID(unittest.NonexistentID, unittest.NonexistentID)
119+
_, err = GetWebhookByOwnerID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
120120
assert.Error(t, err)
121121
assert.True(t, IsErrWebhookNotExist(err))
122122
}
@@ -174,21 +174,21 @@ func TestUpdateWebhook(t *testing.T) {
174174
func TestDeleteWebhookByRepoID(t *testing.T) {
175175
assert.NoError(t, unittest.PrepareTestDatabase())
176176
unittest.AssertExistsAndLoadBean(t, &Webhook{ID: 2, RepoID: 1})
177-
assert.NoError(t, DeleteWebhookByRepoID(1, 2))
177+
assert.NoError(t, DeleteWebhookByRepoID(db.DefaultContext, 1, 2))
178178
unittest.AssertNotExistsBean(t, &Webhook{ID: 2, RepoID: 1})
179179

180-
err := DeleteWebhookByRepoID(unittest.NonexistentID, unittest.NonexistentID)
180+
err := DeleteWebhookByRepoID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
181181
assert.Error(t, err)
182182
assert.True(t, IsErrWebhookNotExist(err))
183183
}
184184

185185
func TestDeleteWebhookByOwnerID(t *testing.T) {
186186
assert.NoError(t, unittest.PrepareTestDatabase())
187187
unittest.AssertExistsAndLoadBean(t, &Webhook{ID: 3, OwnerID: 3})
188-
assert.NoError(t, DeleteWebhookByOwnerID(3, 3))
188+
assert.NoError(t, DeleteWebhookByOwnerID(db.DefaultContext, 3, 3))
189189
unittest.AssertNotExistsBean(t, &Webhook{ID: 3, OwnerID: 3})
190190

191-
err := DeleteWebhookByOwnerID(unittest.NonexistentID, unittest.NonexistentID)
191+
err := DeleteWebhookByOwnerID(db.DefaultContext, unittest.NonexistentID, unittest.NonexistentID)
192192
assert.Error(t, err)
193193
assert.True(t, IsErrWebhookNotExist(err))
194194
}

‎routers/api/v1/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1258,8 +1258,8 @@ func Routes() *web.Route {
12581258
m.Group("/{username}/{reponame}", func() {
12591259
m.Group("/issues", func() {
12601260
m.Combo("").Get(repo.ListIssues).
1261-
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue)
1262-
m.Get("/pinned", repo.ListPinnedIssues)
1261+
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue)
1262+
m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues)
12631263
m.Group("/comments", func() {
12641264
m.Get("", repo.ListRepoIssueComments)
12651265
m.Group("/{id}", func() {

‎routers/api/v1/repo/hook.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ func DeleteHook(ctx *context.APIContext) {
301301
// "$ref": "#/responses/empty"
302302
// "404":
303303
// "$ref": "#/responses/notFound"
304-
if err := webhook.DeleteWebhookByRepoID(ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")); err != nil {
304+
if err := webhook.DeleteWebhookByRepoID(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")); err != nil {
305305
if webhook.IsErrWebhookNotExist(err) {
306306
ctx.NotFound()
307307
} else {

‎routers/api/v1/repo/issue.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,24 @@ func ListIssues(ctx *context.APIContext) {
462462
isPull = util.OptionalBoolNone
463463
}
464464

465+
if isPull != util.OptionalBoolNone && !ctx.Repo.CanReadIssuesOrPulls(isPull.IsTrue()) {
466+
ctx.NotFound()
467+
return
468+
}
469+
470+
if isPull == util.OptionalBoolNone {
471+
canReadIssues := ctx.Repo.CanRead(unit.TypeIssues)
472+
canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests)
473+
if !canReadIssues && !canReadPulls {
474+
ctx.NotFound()
475+
return
476+
} else if !canReadIssues {
477+
isPull = util.OptionalBoolTrue
478+
} else if !canReadPulls {
479+
isPull = util.OptionalBoolFalse
480+
}
481+
}
482+
465483
// FIXME: we should be more efficient here
466484
createdByID := getUserIDForFilter(ctx, "created_by")
467485
if ctx.Written() {
@@ -593,6 +611,10 @@ func GetIssue(ctx *context.APIContext) {
593611
}
594612
return
595613
}
614+
if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
615+
ctx.NotFound()
616+
return
617+
}
596618
ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue))
597619
}
598620

0 commit comments

Comments
 (0)