Skip to content

Commit dfd511f

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

35 files changed

+422
-109
lines changed

‎models/asymkey/gpg_key.go

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,9 +93,9 @@ func CountUserGPGKeys(userID int64) (int64, error) {
9393
}
9494

9595
// GetGPGKeyByID returns public key by given ID.
96-
func GetGPGKeyByID(keyID int64) (*GPGKey, error) {
96+
func GetGPGKeyForUserByID(ownerID, keyID int64) (*GPGKey, error) {
9797
key := new(GPGKey)
98-
has, err := db.GetEngine(db.DefaultContext).ID(keyID).Get(key)
98+
has, err := db.GetEngine(db.DefaultContext).Where("id=? AND owner_id=?", keyID, ownerID).Get(key)
9999
if err != nil {
100100
return nil, err
101101
} else if !has {
@@ -225,19 +225,14 @@ func deleteGPGKey(ctx context.Context, keyID string) (int64, error) {
225225

226226
// DeleteGPGKey deletes GPG key information in database.
227227
func DeleteGPGKey(doer *user_model.User, id int64) (err error) {
228-
key, err := GetGPGKeyByID(id)
228+
key, err := GetGPGKeyForUserByID(doer.ID, id)
229229
if err != nil {
230230
if IsErrGPGKeyNotExist(err) {
231231
return nil
232232
}
233233
return fmt.Errorf("GetPublicKeyByID: %w", err)
234234
}
235235

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-
241236
ctx, committer, err := db.TxContext(db.DefaultContext)
242237
if err != nil {
243238
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
@@ -1014,6 +1014,7 @@ type FindCommentsOptions struct {
10141014
Type CommentType
10151015
IssueIDs []int64
10161016
Invalidated util.OptionalBool
1017+
IsPull util.OptionalBool
10171018
}
10181019

10191020
// ToConds implements FindOptions interface
@@ -1048,14 +1049,17 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond {
10481049
if !opts.Invalidated.IsNone() {
10491050
cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()})
10501051
}
1052+
if opts.IsPull != util.OptionalBoolNone {
1053+
cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()})
1054+
}
10511055
return cond
10521056
}
10531057

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

‎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
@@ -306,6 +306,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) {
306306
return p, nil
307307
}
308308

309+
// GetProjectForRepoByID returns the projects in a repository
310+
func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) {
311+
p := new(Project)
312+
has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(p)
313+
if err != nil {
314+
return nil, err
315+
} else if !has {
316+
return nil, ErrProjectNotExist{ID: id}
317+
}
318+
return p, nil
319+
}
320+
309321
// UpdateProject updates project properties
310322
func UpdateProject(ctx context.Context, p *Project) error {
311323
if !IsCardTypeValid(p.CardType) {

‎models/repo/release.go

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

204+
// GetReleaseForRepoByID returns release with given ID.
205+
func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, error) {
206+
rel := new(Release)
207+
has, err := db.GetEngine(ctx).
208+
Where("id=? AND repo_id=?", id, repoID).
209+
Get(rel)
210+
if err != nil {
211+
return nil, err
212+
} else if !has {
213+
return nil, ErrReleaseNotExist{id, ""}
214+
}
215+
216+
return rel, nil
217+
}
218+
204219
// FindReleasesOptions describes the conditions to Find releases
205220
type FindReleasesOptions struct {
206221
db.ListOptions

‎models/webhook/webhook.go

Lines changed: 34 additions & 33 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.
415408
func GetWebhookByRepoID(repoID, id int64) (*Webhook, error) {
416-
return getWebhook(&Webhook{
417-
ID: id,
418-
RepoID: repoID,
419-
})
409+
webhook := new(Webhook)
410+
has, err := db.GetEngine(db.DefaultContext).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.
423420
func GetWebhookByOwnerID(ownerID, id int64) (*Webhook, error) {
424-
return getWebhook(&Webhook{
425-
ID: id,
426-
OwnerID: ownerID,
427-
})
421+
webhook := new(Webhook)
422+
has, err := db.GetEngine(db.DefaultContext).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,20 +483,20 @@ 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+
func DeleteWebhookByID(id int64) (err error) {
488489
ctx, committer, err := db.TxContext(db.DefaultContext)
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

@@ -504,16 +505,16 @@ func deleteWebhook(bean *Webhook) (err error) {
504505

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

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

‎routers/api/v1/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,8 +1148,8 @@ func Routes(ctx gocontext.Context) *web.Route {
11481148
m.Group("/{username}/{reponame}", func() {
11491149
m.Group("/issues", func() {
11501150
m.Combo("").Get(repo.ListIssues).
1151-
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue)
1152-
m.Get("/pinned", repo.ListPinnedIssues)
1151+
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue)
1152+
m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues)
11531153
m.Group("/comments", func() {
11541154
m.Get("", repo.ListRepoIssueComments)
11551155
m.Group("/{id}", func() {

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

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

454+
if isPull != util.OptionalBoolNone && !ctx.Repo.CanReadIssuesOrPulls(isPull.IsTrue()) {
455+
ctx.NotFound()
456+
return
457+
}
458+
459+
if isPull == util.OptionalBoolNone {
460+
canReadIssues := ctx.Repo.CanRead(unit.TypeIssues)
461+
canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests)
462+
if !canReadIssues && !canReadPulls {
463+
ctx.NotFound()
464+
return
465+
} else if !canReadIssues {
466+
isPull = util.OptionalBoolTrue
467+
} else if !canReadPulls {
468+
isPull = util.OptionalBoolFalse
469+
}
470+
}
471+
454472
// FIXME: we should be more efficient here
455473
createdByID := getUserIDForFilter(ctx, "created_by")
456474
if ctx.Written() {
@@ -561,6 +579,10 @@ func GetIssue(ctx *context.APIContext) {
561579
}
562580
return
563581
}
582+
if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) {
583+
ctx.NotFound()
584+
return
585+
}
564586
ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue))
565587
}
566588

0 commit comments

Comments
 (0)