Skip to content

Commit 882e502

Browse files
lunnylunny
authored
Fix comment permissions (#28213)
This PR will fix some missed checks for private repositories' data on web routes and API routes.
1 parent 80217ca commit 882e502

34 files changed

+417
-105
lines changed

‎models/asymkey/gpg_key.go

+3-9
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

+9
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

+1-1
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

+5-1
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,7 @@ type FindCommentsOptions struct {
10241024
Type CommentType
10251025
IssueIDs []int64
10261026
Invalidated util.OptionalBool
1027+
IsPull util.OptionalBool
10271028
}
10281029

10291030
// ToConds implements FindOptions interface
@@ -1058,14 +1059,17 @@ func (opts FindCommentsOptions) ToConds() builder.Cond {
10581059
if !opts.Invalidated.IsNone() {
10591060
cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()})
10601061
}
1062+
if opts.IsPull != util.OptionalBoolNone {
1063+
cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()})
1064+
}
10611065
return cond
10621066
}
10631067

10641068
// FindComments returns all comments according options
10651069
func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) {
10661070
comments := make([]*Comment, 0, 10)
10671071
sess := db.GetEngine(ctx).Where(opts.ToConds())
1068-
if opts.RepoID > 0 {
1072+
if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone {
10691073
sess.Join("INNER", "issue", "issue.id = comment.issue_id")
10701074
}
10711075

‎models/issues/content_history.go

+2-2
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

+2-2
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

+12
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) {
294294
return p, nil
295295
}
296296

297+
// GetProjectForRepoByID returns the projects in a repository
298+
func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) {
299+
p := new(Project)
300+
has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(p)
301+
if err != nil {
302+
return nil, err
303+
} else if !has {
304+
return nil, ErrProjectNotExist{ID: id}
305+
}
306+
return p, nil
307+
}
308+
297309
// UpdateProject updates project properties
298310
func UpdateProject(ctx context.Context, p *Project) error {
299311
if !IsCardTypeValid(p.CardType) {

‎models/repo/release.go

+15
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

+34-33
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(ctx context.Context, bean *Webhook) (*Webhook, error) {
398-
has, err := db.GetEngine(ctx).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(ctx context.Context, id int64) (*Webhook, error) {
409-
return getWebhook(ctx, &Webhook{
410-
ID: id,
411-
})
412-
}
413-
414407
// GetWebhookByRepoID returns webhook of repository by given ID.
415408
func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) {
416-
return getWebhook(ctx, &Webhook{
417-
ID: id,
418-
RepoID: repoID,
419-
})
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.
423420
func GetWebhookByOwnerID(ctx context.Context, ownerID, id int64) (*Webhook, error) {
424-
return getWebhook(ctx, &Webhook{
425-
ID: id,
426-
OwnerID: ownerID,
427-
})
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
@@ -461,20 +462,20 @@ func UpdateWebhookLastStatus(ctx context.Context, w *Webhook) error {
461462
return err
462463
}
463464

464-
// deleteWebhook uses argument bean as query condition,
465+
// DeleteWebhookByID uses argument bean as query condition,
465466
// ID must be specified and do not assign unnecessary fields.
466-
func deleteWebhook(ctx context.Context, bean *Webhook) (err error) {
467+
func DeleteWebhookByID(ctx context.Context, id int64) (err error) {
467468
ctx, committer, err := db.TxContext(ctx)
468469
if err != nil {
469470
return err
470471
}
471472
defer committer.Close()
472473

473-
if count, err := db.DeleteByBean(ctx, bean); err != nil {
474+
if count, err := db.DeleteByID(ctx, id, new(Webhook)); err != nil {
474475
return err
475476
} else if count == 0 {
476-
return ErrWebhookNotExist{ID: bean.ID}
477-
} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: bean.ID}); err != nil {
477+
return ErrWebhookNotExist{ID: id}
478+
} else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: id}); err != nil {
478479
return err
479480
}
480481

@@ -483,16 +484,16 @@ func deleteWebhook(ctx context.Context, bean *Webhook) (err error) {
483484

484485
// DeleteWebhookByRepoID deletes webhook of repository by given ID.
485486
func DeleteWebhookByRepoID(ctx context.Context, repoID, id int64) error {
486-
return deleteWebhook(ctx, &Webhook{
487-
ID: id,
488-
RepoID: repoID,
489-
})
487+
if _, err := GetWebhookByRepoID(ctx, repoID, id); err != nil {
488+
return err
489+
}
490+
return DeleteWebhookByID(ctx, id)
490491
}
491492

492493
// DeleteWebhookByOwnerID deletes webhook of a user or organization by given ID.
493494
func DeleteWebhookByOwnerID(ctx context.Context, ownerID, id int64) error {
494-
return deleteWebhook(ctx, &Webhook{
495-
ID: id,
496-
OwnerID: ownerID,
497-
})
495+
if _, err := GetWebhookByOwnerID(ctx, ownerID, id); err != nil {
496+
return err
497+
}
498+
return DeleteWebhookByID(ctx, id)
498499
}

‎routers/api/v1/api.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -1259,8 +1259,8 @@ func Routes() *web.Route {
12591259
m.Group("/{username}/{reponame}", func() {
12601260
m.Group("/issues", func() {
12611261
m.Combo("").Get(repo.ListIssues).
1262-
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue)
1263-
m.Get("/pinned", repo.ListPinnedIssues)
1262+
Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue)
1263+
m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues)
12641264
m.Group("/comments", func() {
12651265
m.Get("", repo.ListRepoIssueComments)
12661266
m.Group("/{id}", func() {

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

+22
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)