From af3ccb4dc8f554ab5f787fac7cba5954ca71c926 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 25 Oct 2023 22:55:59 +0800 Subject: [PATCH 01/18] Fix comments permission check --- models/issues/comment.go | 6 +- routers/api/v1/api.go | 6 +- routers/api/v1/repo/issue.go | 9 +++ routers/api/v1/repo/issue_comment.go | 56 ++++++++++++++++++- .../api/v1/repo/issue_comment_attachment.go | 4 ++ routers/api/v1/repo/issue_reaction.go | 20 ++++++- routers/web/repo/issue.go | 25 +++++++++ 7 files changed, 118 insertions(+), 8 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 8e06838f73a32..1fd187a56ac96 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1016,6 +1016,7 @@ type FindCommentsOptions struct { Type CommentType IssueIDs []int64 Invalidated util.OptionalBool + IsPull util.OptionalBool } // ToConds implements FindOptions interface @@ -1050,6 +1051,9 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond { if !opts.Invalidated.IsNone() { cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()}) } + if opts.IsPull != util.OptionalBoolNone { + cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()}) + } return cond } @@ -1057,7 +1061,7 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond { func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) { comments := make([]*Comment, 0, 10) sess := db.GetEngine(ctx).Where(opts.ToConds()) - if opts.RepoID > 0 { + if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone { sess.Join("INNER", "issue", "issue.id = comment.issue_id") } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 53ae8c4d016a0..deee1afbddd5a 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1295,10 +1295,10 @@ func Routes() *web.Route { m.Group("/{username}/{reponame}", func() { m.Group("/issues", func() { m.Combo("").Get(repo.ListIssues). - Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue) - m.Get("/pinned", repo.ListPinnedIssues) + Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) + m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues) m.Group("/comments", func() { - m.Get("", repo.ListRepoIssueComments) + m.Get("", reqRepoReader(unit.TypeIssues), repo.ListRepoIssueComments) m.Group("/{id}", func() { m.Combo(""). Get(repo.GetIssueComment). diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 74e6361f6c5d1..4504275f71731 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -462,6 +462,11 @@ func ListIssues(ctx *context.APIContext) { isPull = util.OptionalBoolNone } + if isPull != util.OptionalBoolNone && !ctx.Repo.CanWriteIssuesOrPulls(isPull.IsTrue()) { + ctx.NotFound() + return + } + // FIXME: we should be more efficient here createdByID := getUserIDForFilter(ctx, "created_by") if ctx.Written() { @@ -593,6 +598,10 @@ func GetIssue(ctx *context.APIContext) { } return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue)) } diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index c718424f7ebc2..4db2c68a79800 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -12,9 +12,11 @@ import ( issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/context" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/convert" @@ -71,6 +73,11 @@ func ListIssueComments(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "GetRawIssueByIndex", err) return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } + issue.Repo = ctx.Repo.Repository opts := &issues_model.FindCommentsOptions{ @@ -271,12 +278,27 @@ func ListRepoIssueComments(ctx *context.APIContext) { return } + var isPull util.OptionalBool + canReadIssue := ctx.Repo.CanRead(unit.TypeIssues) + canReadPull := ctx.Repo.CanRead(unit.TypePullRequests) + if canReadIssue && canReadPull { + isPull = util.OptionalBoolNone + } else if canReadIssue { + isPull = util.OptionalBoolFalse + } else if canReadPull { + isPull = util.OptionalBoolTrue + } else { + ctx.NotFound() + return + } + opts := &issues_model.FindCommentsOptions{ ListOptions: utils.GetListOptions(ctx), RepoID: ctx.Repo.Repository.ID, Type: issues_model.CommentTypeComment, Since: since, Before: before, + IsPull: isPull, } comments, err := issues_model.FindComments(ctx, opts) @@ -367,6 +389,11 @@ func CreateIssueComment(ctx *context.APIContext) { return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin { ctx.Error(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked"))) return @@ -436,6 +463,11 @@ func GetIssueComment(ctx *context.APIContext) { return } + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound() + return + } + if comment.Type != issues_model.CommentTypeComment { ctx.Status(http.StatusNoContent) return @@ -555,7 +587,17 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) return } - if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.Status(http.StatusNotFound) + return + } + + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) return } @@ -658,7 +700,17 @@ func deleteIssueComment(ctx *context.APIContext) { return } - if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.Status(http.StatusNotFound) + return + } + + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) return } else if comment.Type != issues_model.CommentTypeComment { diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index c327c54d101c0..21e2f4dabd6b8 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -329,6 +329,10 @@ func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment { return nil } + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + return nil + } + comment.Issue.Repo = ctx.Repo.Repository return comment diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index 29c99184e7649..c886bd71b7687 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/routers/api/v1/repo/issue_reaction.go @@ -61,6 +61,12 @@ func GetIssueCommentReactions(ctx *context.APIContext) { if err := comment.LoadIssue(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return } if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { @@ -190,9 +196,19 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp return } - err = comment.LoadIssue(ctx) - if err != nil { + if err = comment.LoadIssue(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return + } + + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound() + return } if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 96fce48878219..5a615e7b2a4b4 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3104,6 +3104,11 @@ func UpdateCommentContent(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(http.StatusForbidden) return @@ -3170,6 +3175,11 @@ func DeleteComment(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(http.StatusForbidden) return @@ -3296,6 +3306,11 @@ func ChangeCommentReaction(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull)) { if log.IsTrace() { if ctx.IsSigned { @@ -3439,6 +3454,16 @@ func GetCommentAttachments(ctx *context.Context) { return } + if err := comment.LoadIssue(ctx); err != nil { + ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !comment.Type.HasAttachmentSupport() { ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type)) return From f3dea3b82bec4510e97141fb1867dbf590666971 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 25 Oct 2023 23:07:58 +0800 Subject: [PATCH 02/18] Some improvements --- routers/api/v1/api.go | 2 +- routers/api/v1/repo/issue.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index deee1afbddd5a..f65bb5e8889f4 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1298,7 +1298,7 @@ func Routes() *web.Route { Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues) m.Group("/comments", func() { - m.Get("", reqRepoReader(unit.TypeIssues), repo.ListRepoIssueComments) + m.Get("", repo.ListRepoIssueComments) m.Group("/{id}", func() { m.Combo(""). Get(repo.GetIssueComment). diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 4504275f71731..2d55d15904d94 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -467,6 +467,20 @@ func ListIssues(ctx *context.APIContext) { return } + canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) + canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) + + if isPull == util.OptionalBoolNone { + if !canReadIssues && !canReadPulls { + ctx.NotFound() + return + } else if !canReadIssues { + isPull = util.OptionalBoolTrue + } else if !canReadPulls { + isPull = util.OptionalBoolFalse + } + } + // FIXME: we should be more efficient here createdByID := getUserIDForFilter(ctx, "created_by") if ctx.Written() { From 2d1deaac774b5cb28ef032abe0d8e6bb1801cd75 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 26 Oct 2023 20:27:46 +0800 Subject: [PATCH 03/18] Follow @delvh suggestion --- routers/web/repo/issue.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 5a615e7b2a4b4..44a69ca142674 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3105,7 +3105,7 @@ func UpdateCommentContent(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3176,7 +3176,7 @@ func DeleteComment(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3307,7 +3307,7 @@ func ChangeCommentReaction(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3460,7 +3460,7 @@ func GetCommentAttachments(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } From cadbe0be18a188ceb2528ef859ca4e6128f643f9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 26 Oct 2023 23:32:03 +0800 Subject: [PATCH 04/18] Follow @delvh suggestion --- routers/api/v1/repo/issue.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 2d55d15904d94..5a61f069c7fab 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -467,10 +467,9 @@ func ListIssues(ctx *context.APIContext) { return } - canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) - canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) - if isPull == util.OptionalBoolNone { + canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) + canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) if !canReadIssues && !canReadPulls { ctx.NotFound() return From ead2fa33677f8082311c884d8523649e17900552 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 25 Oct 2023 22:55:59 +0800 Subject: [PATCH 05/18] Fix comments permission check --- models/issues/comment.go | 6 +- routers/api/v1/api.go | 6 +- routers/api/v1/repo/issue.go | 9 +++ routers/api/v1/repo/issue_comment.go | 56 ++++++++++++++++++- .../api/v1/repo/issue_comment_attachment.go | 4 ++ routers/api/v1/repo/issue_reaction.go | 20 ++++++- routers/web/repo/issue.go | 25 +++++++++ 7 files changed, 118 insertions(+), 8 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index 8e06838f73a32..1fd187a56ac96 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -1016,6 +1016,7 @@ type FindCommentsOptions struct { Type CommentType IssueIDs []int64 Invalidated util.OptionalBool + IsPull util.OptionalBool } // ToConds implements FindOptions interface @@ -1050,6 +1051,9 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond { if !opts.Invalidated.IsNone() { cond = cond.And(builder.Eq{"comment.invalidated": opts.Invalidated.IsTrue()}) } + if opts.IsPull != util.OptionalBoolNone { + cond = cond.And(builder.Eq{"issue.is_pull": opts.IsPull.IsTrue()}) + } return cond } @@ -1057,7 +1061,7 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond { func FindComments(ctx context.Context, opts *FindCommentsOptions) (CommentList, error) { comments := make([]*Comment, 0, 10) sess := db.GetEngine(ctx).Where(opts.ToConds()) - if opts.RepoID > 0 { + if opts.RepoID > 0 || opts.IsPull != util.OptionalBoolNone { sess.Join("INNER", "issue", "issue.id = comment.issue_id") } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 61658d213b365..793642a0f16e1 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1295,10 +1295,10 @@ func Routes() *web.Route { m.Group("/{username}/{reponame}", func() { m.Group("/issues", func() { m.Combo("").Get(repo.ListIssues). - Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), repo.CreateIssue) - m.Get("/pinned", repo.ListPinnedIssues) + Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) + m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues) m.Group("/comments", func() { - m.Get("", repo.ListRepoIssueComments) + m.Get("", reqRepoReader(unit.TypeIssues), repo.ListRepoIssueComments) m.Group("/{id}", func() { m.Combo(""). Get(repo.GetIssueComment). diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 74e6361f6c5d1..4504275f71731 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -462,6 +462,11 @@ func ListIssues(ctx *context.APIContext) { isPull = util.OptionalBoolNone } + if isPull != util.OptionalBoolNone && !ctx.Repo.CanWriteIssuesOrPulls(isPull.IsTrue()) { + ctx.NotFound() + return + } + // FIXME: we should be more efficient here createdByID := getUserIDForFilter(ctx, "created_by") if ctx.Written() { @@ -593,6 +598,10 @@ func GetIssue(ctx *context.APIContext) { } return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } ctx.JSON(http.StatusOK, convert.ToAPIIssue(ctx, issue)) } diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index c718424f7ebc2..4db2c68a79800 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -12,9 +12,11 @@ import ( issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/context" api "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/convert" @@ -71,6 +73,11 @@ func ListIssueComments(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "GetRawIssueByIndex", err) return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } + issue.Repo = ctx.Repo.Repository opts := &issues_model.FindCommentsOptions{ @@ -271,12 +278,27 @@ func ListRepoIssueComments(ctx *context.APIContext) { return } + var isPull util.OptionalBool + canReadIssue := ctx.Repo.CanRead(unit.TypeIssues) + canReadPull := ctx.Repo.CanRead(unit.TypePullRequests) + if canReadIssue && canReadPull { + isPull = util.OptionalBoolNone + } else if canReadIssue { + isPull = util.OptionalBoolFalse + } else if canReadPull { + isPull = util.OptionalBoolTrue + } else { + ctx.NotFound() + return + } + opts := &issues_model.FindCommentsOptions{ ListOptions: utils.GetListOptions(ctx), RepoID: ctx.Repo.Repository.ID, Type: issues_model.CommentTypeComment, Since: since, Before: before, + IsPull: isPull, } comments, err := issues_model.FindComments(ctx, opts) @@ -367,6 +389,11 @@ func CreateIssueComment(ctx *context.APIContext) { return } + if !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull) { + ctx.NotFound() + return + } + if issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin { ctx.Error(http.StatusForbidden, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked"))) return @@ -436,6 +463,11 @@ func GetIssueComment(ctx *context.APIContext) { return } + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound() + return + } + if comment.Type != issues_model.CommentTypeComment { ctx.Status(http.StatusNoContent) return @@ -555,7 +587,17 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) return } - if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.Status(http.StatusNotFound) + return + } + + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) return } @@ -658,7 +700,17 @@ func deleteIssueComment(ctx *context.APIContext) { return } - if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.IsAdmin()) { + if err := comment.LoadIssue(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.Status(http.StatusNotFound) + return + } + + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) return } else if comment.Type != issues_model.CommentTypeComment { diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index c327c54d101c0..21e2f4dabd6b8 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -329,6 +329,10 @@ func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment { return nil } + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + return nil + } + comment.Issue.Repo = ctx.Repo.Repository return comment diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index 29c99184e7649..c886bd71b7687 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/routers/api/v1/repo/issue_reaction.go @@ -61,6 +61,12 @@ func GetIssueCommentReactions(ctx *context.APIContext) { if err := comment.LoadIssue(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return } if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { @@ -190,9 +196,19 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp return } - err = comment.LoadIssue(ctx) - if err != nil { + if err = comment.LoadIssue(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "comment.LoadIssue() failed", err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return + } + + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound() + return } if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 94300da868330..cb397d1fc2785 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3106,6 +3106,11 @@ func UpdateCommentContent(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(http.StatusForbidden) return @@ -3172,6 +3177,11 @@ func DeleteComment(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Error(http.StatusForbidden) return @@ -3298,6 +3308,11 @@ func ChangeCommentReaction(ctx *context.Context) { return } + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull)) { if log.IsTrace() { if ctx.IsSigned { @@ -3441,6 +3456,16 @@ func GetCommentAttachments(ctx *context.Context) { return } + if err := comment.LoadIssue(ctx); err != nil { + ctx.NotFoundOrServerError("LoadIssue", issues_model.IsErrIssueNotExist, err) + return + } + + if comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + return + } + if !comment.Type.HasAttachmentSupport() { ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type)) return From b8c4bf0db45878fee35cbd670bcb81f3ac5abf82 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 25 Oct 2023 23:07:58 +0800 Subject: [PATCH 06/18] Some improvements --- routers/api/v1/api.go | 2 +- routers/api/v1/repo/issue.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 793642a0f16e1..a03dfef0cdb6e 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1298,7 +1298,7 @@ func Routes() *web.Route { Post(reqToken(), mustNotBeArchived, bind(api.CreateIssueOption{}), reqRepoReader(unit.TypeIssues), repo.CreateIssue) m.Get("/pinned", reqRepoReader(unit.TypeIssues), repo.ListPinnedIssues) m.Group("/comments", func() { - m.Get("", reqRepoReader(unit.TypeIssues), repo.ListRepoIssueComments) + m.Get("", repo.ListRepoIssueComments) m.Group("/{id}", func() { m.Combo(""). Get(repo.GetIssueComment). diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 4504275f71731..2d55d15904d94 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -467,6 +467,20 @@ func ListIssues(ctx *context.APIContext) { return } + canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) + canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) + + if isPull == util.OptionalBoolNone { + if !canReadIssues && !canReadPulls { + ctx.NotFound() + return + } else if !canReadIssues { + isPull = util.OptionalBoolTrue + } else if !canReadPulls { + isPull = util.OptionalBoolFalse + } + } + // FIXME: we should be more efficient here createdByID := getUserIDForFilter(ctx, "created_by") if ctx.Written() { From b111de59710a48cfb4f709e528feb5db98b7a06c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 26 Oct 2023 20:27:46 +0800 Subject: [PATCH 07/18] Follow @delvh suggestion --- routers/web/repo/issue.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index cb397d1fc2785..47a03ad0dff59 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3107,7 +3107,7 @@ func UpdateCommentContent(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3178,7 +3178,7 @@ func DeleteComment(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3309,7 +3309,7 @@ func ChangeCommentReaction(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } @@ -3462,7 +3462,7 @@ func GetCommentAttachments(ctx *context.Context) { } if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.NotFoundOrServerError("CompareRepoID", issues_model.IsErrCommentNotExist, err) + ctx.NotFound("CompareRepoID", issues_model.ErrCommentNotExist{}) return } From 02d54a3d9123fa95cb1fba01d704c375ae25f4b0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 26 Oct 2023 23:32:03 +0800 Subject: [PATCH 08/18] Follow @delvh suggestion --- routers/api/v1/repo/issue.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 2d55d15904d94..5a61f069c7fab 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -467,10 +467,9 @@ func ListIssues(ctx *context.APIContext) { return } - canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) - canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) - if isPull == util.OptionalBoolNone { + canReadIssues := ctx.Repo.CanRead(unit.TypeIssues) + canReadPulls := ctx.Repo.CanRead(unit.TypePullRequests) if !canReadIssues && !canReadPulls { ctx.NotFound() return From 7ebfef049b9f96ab9c766ff0479ca29fb662930e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Nov 2023 22:29:46 +0800 Subject: [PATCH 09/18] Add tests --- models/fixtures/comment.yml | 9 ++++ routers/web/repo/issue.go | 5 ++ .../api_comment_attachment_test.go | 8 +++ tests/integration/api_comment_test.go | 27 +++++++++- tests/integration/api_issue_reaction_test.go | 21 ++++++++ tests/integration/issue_test.go | 50 +++++++++++++++++++ 6 files changed, 118 insertions(+), 2 deletions(-) diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index bd64680c8c787..17586caa2191f 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -66,3 +66,12 @@ tree_path: "README.md" created_unix: 946684812 invalidated: true + +- + id: 8 + type: 0 # comment + poster_id: 2 + issue_id: 4 # in repo_id 2 + content: "comment in private pository" + created_unix: 946684811 + updated_unix: 946684811 diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 47a03ad0dff59..ca5d13784b821 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3466,6 +3466,11 @@ func GetCommentAttachments(ctx *context.Context) { return } + if !ctx.Repo.Permission.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound("CanReadIssuesOrPulls", issues_model.ErrCommentNotExist{}) + return + } + if !comment.Type.HasAttachmentSupport() { ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type)) return diff --git a/tests/integration/api_comment_attachment_test.go b/tests/integration/api_comment_attachment_test.go index e211376c3c888..95a7a81eb4040 100644 --- a/tests/integration/api_comment_attachment_test.go +++ b/tests/integration/api_comment_attachment_test.go @@ -35,6 +35,14 @@ func TestAPIGetCommentAttachment(t *testing.T) { repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: comment.Issue.RepoID}) repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + t.Run("UnrelatedCommentID", func(t *testing.T) { + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/comments/%d/assets/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, attachment.ID, token) + MakeRequest(t, req, http.StatusNotFound) + }) + session := loginUser(t, repoOwner.Name) token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadIssue) req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/issues/comments/%d/assets/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, attachment.ID, token) diff --git a/tests/integration/api_comment_test.go b/tests/integration/api_comment_test.go index ee648210e5e4e..1074493da4fbd 100644 --- a/tests/integration/api_comment_test.go +++ b/tests/integration/api_comment_test.go @@ -140,12 +140,25 @@ func TestAPIEditComment(t *testing.T) { defer tests.PrepareTestEnv(t)() const newCommentBody = "This is the new comment body" - comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{}, + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 8}, unittest.Cond("type = ?", issues_model.CommentTypeComment)) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + t.Run("UnrelatedCommentID", func(t *testing.T) { + // Using the ID of a comment that does not belong to the repository must fail + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d?token=%s", + repoOwner.Name, repo.Name, comment.ID, token) + req := NewRequestWithValues(t, "PATCH", urlStr, map[string]string{ + "body": newCommentBody, + }) + MakeRequest(t, req, http.StatusNotFound) + }) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, token) @@ -164,12 +177,22 @@ func TestAPIEditComment(t *testing.T) { func TestAPIDeleteComment(t *testing.T) { defer tests.PrepareTestEnv(t)() - comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{}, + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 8}, unittest.Cond("type = ?", issues_model.CommentTypeComment)) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: comment.IssueID}) repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + t.Run("UnrelatedCommentID", func(t *testing.T) { + // Using the ID of a comment that does not belong to the repository must fail + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) + req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/issues/comments/%d?token=%s", + repoOwner.Name, repo.Name, comment.ID, token) + MakeRequest(t, req, http.StatusNotFound) + }) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/issues/comments/%d?token=%s", repoOwner.Name, repo.Name, comment.ID, token) diff --git a/tests/integration/api_issue_reaction_test.go b/tests/integration/api_issue_reaction_test.go index 7d3ee2d15432f..124d729353ca7 100644 --- a/tests/integration/api_issue_reaction_test.go +++ b/tests/integration/api_issue_reaction_test.go @@ -12,6 +12,7 @@ import ( auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" api "code.gitea.io/gitea/modules/structs" @@ -107,6 +108,26 @@ func TestAPICommentReactions(t *testing.T) { }) MakeRequest(t, req, http.StatusOK) + t.Run("UnrelatedCommentID", func(t *testing.T) { + // Using the ID of a comment that does not belong to the repository must fail + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4}) + repoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + token := getUserToken(t, repoOwner.Name, auth_model.AccessTokenScopeWriteIssue) + urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/issues/comments/%d/reactions?token=%s", + repoOwner.Name, repo.Name, comment.ID, token) + req = NewRequestWithJSON(t, "POST", urlStr, &api.EditReactionOption{ + Reaction: "+1", + }) + MakeRequest(t, req, http.StatusNotFound) + req = NewRequestWithJSON(t, "DELETE", urlStr, &api.EditReactionOption{ + Reaction: "+1", + }) + MakeRequest(t, req, http.StatusNotFound) + + req = NewRequestf(t, "GET", urlStr) + MakeRequest(t, req, http.StatusNotFound) + }) + // Add allowed reaction req = NewRequestWithJSON(t, "POST", urlStr, &api.EditReactionOption{ Reaction: "+1", diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index ac06b487db25f..b1080c998a361 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -206,6 +206,56 @@ func TestIssueCommentClose(t *testing.T) { assert.Equal(t, "Description", val) } +func TestIssueCommentDelete(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user2") + issueURL := testNewIssue(t, session, "user2", "repo1", "Title", "Description") + comment1 := "Test comment 1" + commentID := testIssueAddComment(t, session, issueURL, comment1, "") + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: commentID}) + assert.Equal(t, comment1, comment.Content) + + // Using the ID of a comment that does not belong to the repository must fail + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d/delete", "user5", "repo4", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + }) + session.MakeRequest(t, req, http.StatusNotFound) + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d/delete", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + }) + session.MakeRequest(t, req, http.StatusOK) + unittest.AssertNotExistsBean(t, &issues_model.Comment{ID: commentID}) +} + +func TestIssueCommentUpdate(t *testing.T) { + defer tests.PrepareTestEnv(t)() + session := loginUser(t, "user2") + issueURL := testNewIssue(t, session, "user2", "repo1", "Title", "Description") + comment1 := "Test comment 1" + commentID := testIssueAddComment(t, session, issueURL, comment1, "") + + comment := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: commentID}) + assert.Equal(t, comment1, comment.Content) + + modifiedContent := comment.Content + "MODIFIED" + + // Using the ID of a comment that does not belong to the repository must fail + req := NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user5", "repo4", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": modifiedContent, + }) + session.MakeRequest(t, req, http.StatusNotFound) + + req = NewRequestWithValues(t, "POST", fmt.Sprintf("/%s/%s/comments/%d", "user2", "repo1", commentID), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "content": modifiedContent, + }) + session.MakeRequest(t, req, http.StatusOK) + + comment = unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: commentID}) + assert.Equal(t, modifiedContent, comment.Content) +} + func TestIssueReaction(t *testing.T) { defer tests.PrepareTestEnv(t)() session := loginUser(t, "user2") From d5c916495355608f3242db0c3c8bc17536476579 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Nov 2023 21:25:27 +0800 Subject: [PATCH 10/18] Add more fix and check --- routers/api/v1/repo/key.go | 6 +++++ routers/api/v1/repo/release.go | 2 +- routers/api/v1/repo/release_tags.go | 2 +- routers/api/v1/repo/tag.go | 2 +- routers/web/repo/issue_content_history.go | 5 ++++ routers/web/repo/projects.go | 5 ++++ routers/web/repo/release.go | 30 +++++++++++++++++------ services/release/release.go | 18 +++----------- tests/integration/api_keys_test.go | 11 +++++++++ tests/integration/mirror_pull_test.go | 2 +- 10 files changed, 58 insertions(+), 25 deletions(-) diff --git a/routers/api/v1/repo/key.go b/routers/api/v1/repo/key.go index 3dc5a60d1c654..af48c40885df5 100644 --- a/routers/api/v1/repo/key.go +++ b/routers/api/v1/repo/key.go @@ -153,6 +153,12 @@ func GetDeployKey(ctx *context.APIContext) { return } + // this check make it more consistent + if key.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return + } + if err = key.GetContent(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "GetContent", err) return diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index 61e5bdd67943c..c40b72929711c 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -403,7 +403,7 @@ func DeleteRelease(ctx *context.APIContext) { ctx.NotFound() return } - if err := release_service.DeleteReleaseByID(ctx, id, ctx.Doer, false); err != nil { + if err := release_service.DeleteReleaseByID(ctx, ctx.Repo.Repository, rel, ctx.Doer, false); err != nil { if models.IsErrProtectedTagName(err) { ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") return diff --git a/routers/api/v1/repo/release_tags.go b/routers/api/v1/repo/release_tags.go index 926a713c9477c..9f2098df06642 100644 --- a/routers/api/v1/repo/release_tags.go +++ b/routers/api/v1/repo/release_tags.go @@ -112,7 +112,7 @@ func DeleteReleaseByTag(ctx *context.APIContext) { return } - if err = releaseservice.DeleteReleaseByID(ctx, release.ID, ctx.Doer, false); err != nil { + if err = releaseservice.DeleteReleaseByID(ctx, ctx.Repo.Repository, release, ctx.Doer, false); err != nil { if models.IsErrProtectedTagName(err) { ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") return diff --git a/routers/api/v1/repo/tag.go b/routers/api/v1/repo/tag.go index dbc8df0ef81b1..2f19f95e66dec 100644 --- a/routers/api/v1/repo/tag.go +++ b/routers/api/v1/repo/tag.go @@ -272,7 +272,7 @@ func DeleteTag(ctx *context.APIContext) { return } - if err = releaseservice.DeleteReleaseByID(ctx, tag.ID, ctx.Doer, true); err != nil { + if err = releaseservice.DeleteReleaseByID(ctx, ctx.Repo.Repository, tag, ctx.Doer, true); err != nil { if models.IsErrProtectedTagName(err) { ctx.Error(http.StatusMethodNotAllowed, "delTag", "user not allowed to delete protected tag") return diff --git a/routers/web/repo/issue_content_history.go b/routers/web/repo/issue_content_history.go index 5c378fe9d79df..9967df024abbe 100644 --- a/routers/web/repo/issue_content_history.go +++ b/routers/web/repo/issue_content_history.go @@ -130,6 +130,11 @@ func GetContentHistoryDetail(ctx *context.Context) { return } + if history.IssueID != issue.ID { + ctx.NotFound("GetContentHistoryDetail", nil) + return + } + // get the related comment if this history revision is for a comment, otherwise the history revision is for an issue. var comment *issues_model.Comment if history.CommentID != 0 { diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 199a06524546d..4c69c77435bba 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -478,6 +478,11 @@ func AddBoardToProjectPost(ctx *context.Context) { return } + if project.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound("", nil) + return + } + if err := project_model.NewBoard(ctx, &project_model.Board{ ProjectID: project.ID, Title: form.Title, diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 5cbd6b3d51c3b..e6fffaf30f2db 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -616,7 +616,28 @@ func DeleteTag(ctx *context.Context) { } func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) { - if err := releaseservice.DeleteReleaseByID(ctx, ctx.FormInt64("id"), ctx.Doer, isDelTag); err != nil { + redirect := func() { + if isDelTag { + ctx.JSONRedirect(ctx.Repo.RepoLink + "/tags") + return + } + + ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases") + } + + rel, err := repo_model.GetReleaseByID(ctx, ctx.FormInt64("id")) + if err != nil { + ctx.Flash.Error("DeleteReleaseByID: " + err.Error()) + redirect() + return + } + + if rel.RepoID != ctx.Repo.Repository.ID { + redirect() + return + } + + if err := releaseservice.DeleteReleaseByID(ctx, ctx.Repo.Repository, rel, ctx.Doer, isDelTag); err != nil { if models.IsErrProtectedTagName(err) { ctx.Flash.Error(ctx.Tr("repo.release.tag_name_protected")) } else { @@ -630,10 +651,5 @@ func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) { } } - if isDelTag { - ctx.JSONRedirect(ctx.Repo.RepoLink + "/tags") - return - } - - ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases") + redirect() } diff --git a/services/release/release.go b/services/release/release.go index e0035d42fc2a7..3ba2a3f611786 100644 --- a/services/release/release.go +++ b/services/release/release.go @@ -301,17 +301,7 @@ func UpdateRelease(ctx context.Context, doer *user_model.User, gitRepo *git.Repo } // DeleteReleaseByID deletes a release and corresponding Git tag by given ID. -func DeleteReleaseByID(ctx context.Context, id int64, doer *user_model.User, delTag bool) error { - rel, err := repo_model.GetReleaseByID(ctx, id) - if err != nil { - return fmt.Errorf("GetReleaseByID: %w", err) - } - - repo, err := repo_model.GetRepositoryByID(ctx, rel.RepoID) - if err != nil { - return fmt.Errorf("GetRepositoryByID: %w", err) - } - +func DeleteReleaseByID(ctx context.Context, repo *repo_model.Repository, rel *repo_model.Release, doer *user_model.User, delTag bool) error { if delTag { protectedTags, err := git_model.GetProtectedTags(ctx, rel.RepoID) if err != nil { @@ -344,19 +334,19 @@ func DeleteReleaseByID(ctx context.Context, id int64, doer *user_model.User, del }, repository.NewPushCommits()) notify_service.DeleteRef(ctx, doer, repo, refName) - if err := repo_model.DeleteReleaseByID(ctx, id); err != nil { + if err := repo_model.DeleteReleaseByID(ctx, rel.ID); err != nil { return fmt.Errorf("DeleteReleaseByID: %w", err) } } else { rel.IsTag = true - if err = repo_model.UpdateRelease(ctx, rel); err != nil { + if err := repo_model.UpdateRelease(ctx, rel); err != nil { return fmt.Errorf("Update: %w", err) } } rel.Repo = repo - if err = rel.LoadAttributes(ctx); err != nil { + if err := rel.LoadAttributes(ctx); err != nil { return fmt.Errorf("LoadAttributes: %w", err) } diff --git a/tests/integration/api_keys_test.go b/tests/integration/api_keys_test.go index 238c3cb823230..03d28c9126656 100644 --- a/tests/integration/api_keys_test.go +++ b/tests/integration/api_keys_test.go @@ -72,6 +72,17 @@ func TestCreateReadOnlyDeployKey(t *testing.T) { Content: rawKeyBody.Key, Mode: perm.AccessModeRead, }) + + // Using the ID of a key that does not belong to the repository must fail + { + req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/%s/keys/%d?token=%s", repoOwner.Name, repo.Name, newDeployKey.ID, token)) + MakeRequest(t, req, http.StatusOK) + + session5 := loginUser(t, "user5") + token5 := getTokenForLoggedInUser(t, session5, auth_model.AccessTokenScopeWriteRepository) + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/user5/repo4/keys/%d?token=%s", newDeployKey.ID, token5)) + MakeRequest(t, req, http.StatusNotFound) + } } func TestCreateReadWriteDeployKey(t *testing.T) { diff --git a/tests/integration/mirror_pull_test.go b/tests/integration/mirror_pull_test.go index e1c7c6b170941..c02e16bfc0b67 100644 --- a/tests/integration/mirror_pull_test.go +++ b/tests/integration/mirror_pull_test.go @@ -88,7 +88,7 @@ func TestMirrorPull(t *testing.T) { release, err := repo_model.GetRelease(db.DefaultContext, repo.ID, "v0.2") assert.NoError(t, err) - assert.NoError(t, release_service.DeleteReleaseByID(ctx, release.ID, user, true)) + assert.NoError(t, release_service.DeleteReleaseByID(ctx, repo, release, user, true)) ok = mirror_service.SyncPullMirror(ctx, mirror.ID) assert.True(t, ok) From a78a6967cea0b5dbd5323f2bee456c8e4dea2aed Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Nov 2023 22:29:35 +0800 Subject: [PATCH 11/18] Add more check --- models/webhook/webhook.go | 75 +++++++++++++---------- routers/api/v1/repo/release_attachment.go | 39 +++++++++--- routers/api/v1/user/app.go | 4 ++ routers/api/v1/user/gpg_key.go | 4 ++ routers/api/v1/user/hook.go | 5 ++ 5 files changed, 86 insertions(+), 41 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 408023507a5f7..d59c7a2d7aae4 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -392,39 +392,40 @@ func CreateWebhooks(ctx context.Context, ws []*Webhook) error { return db.Insert(ctx, ws) } -// getWebhook uses argument bean as query condition, -// ID must be specified and do not assign unnecessary fields. -func getWebhook(ctx context.Context, bean *Webhook) (*Webhook, error) { - has, err := db.GetEngine(ctx).Get(bean) +// GetWebhookByID returns webhook of repository by given ID. +func GetWebhookByID(ctx context.Context, id int64) (*Webhook, error) { + bean := new(Webhook) + has, err := db.GetEngine(ctx).ID(id).Get(bean) if err != nil { return nil, err } else if !has { - return nil, ErrWebhookNotExist{ID: bean.ID} + return nil, ErrWebhookNotExist{ID: id} } return bean, nil } -// GetWebhookByID returns webhook of repository by given ID. -func GetWebhookByID(ctx context.Context, id int64) (*Webhook, error) { - return getWebhook(ctx, &Webhook{ - ID: id, - }) -} - // GetWebhookByRepoID returns webhook of repository by given ID. func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) { - return getWebhook(ctx, &Webhook{ - ID: id, - RepoID: repoID, - }) + webhook, err := GetWebhookByID(ctx, id) + if err != nil { + return nil, err + } + if webhook.RepoID != repoID { + return nil, ErrWebhookNotExist{ID: id} + } + return webhook, nil } // GetWebhookByOwnerID returns webhook of a user or organization by given ID. func GetWebhookByOwnerID(ctx context.Context, ownerID, id int64) (*Webhook, error) { - return getWebhook(ctx, &Webhook{ - ID: id, - OwnerID: ownerID, - }) + webhook, err := GetWebhookByID(ctx, id) + if err != nil { + return nil, err + } + if webhook.OwnerID != ownerID { + return nil, ErrWebhookNotExist{ID: id} + } + return webhook, nil } // ListWebhookOptions are options to filter webhooks on ListWebhooksByOpts @@ -461,20 +462,20 @@ func UpdateWebhookLastStatus(ctx context.Context, w *Webhook) error { return err } -// deleteWebhook uses argument bean as query condition, +// DeleteWebhookByID uses argument bean as query condition, // ID must be specified and do not assign unnecessary fields. -func deleteWebhook(ctx context.Context, bean *Webhook) (err error) { +func DeleteWebhookByID(ctx context.Context, id int64) (err error) { ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - if count, err := db.DeleteByBean(ctx, bean); err != nil { + if count, err := db.DeleteByID(ctx, id, new(Webhook)); err != nil { return err } else if count == 0 { - return ErrWebhookNotExist{ID: bean.ID} - } else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: bean.ID}); err != nil { + return ErrWebhookNotExist{ID: id} + } else if _, err = db.DeleteByBean(ctx, &HookTask{HookID: id}); err != nil { return err } @@ -483,16 +484,24 @@ func deleteWebhook(ctx context.Context, bean *Webhook) (err error) { // DeleteWebhookByRepoID deletes webhook of repository by given ID. func DeleteWebhookByRepoID(ctx context.Context, repoID, id int64) error { - return deleteWebhook(ctx, &Webhook{ - ID: id, - RepoID: repoID, - }) + webhook, err := GetWebhookByID(ctx, id) + if err != nil { + return err + } + if webhook.RepoID != repoID { + return ErrWebhookNotExist{ID: id} + } + return DeleteWebhookByID(ctx, id) } // DeleteWebhookByOwnerID deletes webhook of a user or organization by given ID. func DeleteWebhookByOwnerID(ctx context.Context, ownerID, id int64) error { - return deleteWebhook(ctx, &Webhook{ - ID: id, - OwnerID: ownerID, - }) + webhook, err := GetWebhookByID(ctx, id) + if err != nil { + return err + } + if webhook.OwnerID != ownerID { + return ErrWebhookNotExist{ID: id} + } + return DeleteWebhookByID(ctx, id) } diff --git a/routers/api/v1/repo/release_attachment.go b/routers/api/v1/repo/release_attachment.go index 168ef550c5ea9..c36bf12e6d330 100644 --- a/routers/api/v1/repo/release_attachment.go +++ b/routers/api/v1/repo/release_attachment.go @@ -17,6 +17,23 @@ import ( "code.gitea.io/gitea/services/convert" ) +func checkReleaseMatchRepo(ctx *context.APIContext, releaseID int64) bool { + release, err := repo_model.GetReleaseByID(ctx, releaseID) + if err != nil { + if repo_model.IsErrReleaseNotExist(err) { + ctx.NotFound() + return false + } + ctx.Error(http.StatusInternalServerError, "GetReleaseByID", err) + return false + } + if release.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return false + } + return true +} + // GetReleaseAttachment gets a single attachment of the release func GetReleaseAttachment(ctx *context.APIContext) { // swagger:operation GET /repos/{owner}/{repo}/releases/{id}/assets/{attachment_id} repository repoGetReleaseAttachment @@ -54,6 +71,10 @@ func GetReleaseAttachment(ctx *context.APIContext) { // "$ref": "#/responses/notFound" releaseID := ctx.ParamsInt64(":id") + if !checkReleaseMatchRepo(ctx, releaseID) { + return + } + attachID := ctx.ParamsInt64(":attachment_id") attach, err := repo_model.GetAttachmentByID(ctx, attachID) if err != nil { @@ -176,13 +197,7 @@ func CreateReleaseAttachment(ctx *context.APIContext) { // Check if release exists an load release releaseID := ctx.ParamsInt64(":id") - release, err := repo_model.GetReleaseByID(ctx, releaseID) - if err != nil { - if repo_model.IsErrReleaseNotExist(err) { - ctx.NotFound() - return - } - ctx.Error(http.StatusInternalServerError, "GetReleaseByID", err) + if !checkReleaseMatchRepo(ctx, releaseID) { return } @@ -203,7 +218,7 @@ func CreateReleaseAttachment(ctx *context.APIContext) { attach, err := attachment.UploadAttachment(ctx, file, setting.Repository.Release.AllowedTypes, header.Size, &repo_model.Attachment{ Name: filename, UploaderID: ctx.Doer.ID, - RepoID: release.RepoID, + RepoID: ctx.Repo.Repository.ID, ReleaseID: releaseID, }) if err != nil { @@ -264,6 +279,10 @@ func EditReleaseAttachment(ctx *context.APIContext) { // Check if release exists an load release releaseID := ctx.ParamsInt64(":id") + if !checkReleaseMatchRepo(ctx, releaseID) { + return + } + attachID := ctx.ParamsInt64(":attachment_id") attach, err := repo_model.GetAttachmentByID(ctx, attachID) if err != nil { @@ -328,6 +347,10 @@ func DeleteReleaseAttachment(ctx *context.APIContext) { // Check if release exists an load release releaseID := ctx.ParamsInt64(":id") + if !checkReleaseMatchRepo(ctx, releaseID) { + return + } + attachID := ctx.ParamsInt64(":attachment_id") attach, err := repo_model.GetAttachmentByID(ctx, attachID) if err != nil { diff --git a/routers/api/v1/user/app.go b/routers/api/v1/user/app.go index edbc1d17b4640..f045fb4d5d8c4 100644 --- a/routers/api/v1/user/app.go +++ b/routers/api/v1/user/app.go @@ -342,6 +342,10 @@ func GetOauth2Application(ctx *context.APIContext) { } return } + if app.UID != ctx.Doer.ID { + ctx.NotFound() + return + } app.ClientSecret = "" diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 404b1d221e1d0..374f15389c3e8 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -121,6 +121,10 @@ func GetGPGKey(ctx *context.APIContext) { } return } + if !ctx.Doer.IsAdmin && key.OwnerID != ctx.Doer.ID { + ctx.NotFound() + return + } ctx.JSON(http.StatusOK, convert.ToGPGKey(key)) } diff --git a/routers/api/v1/user/hook.go b/routers/api/v1/user/hook.go index 50be519c815fa..e87385e4a2602 100644 --- a/routers/api/v1/user/hook.go +++ b/routers/api/v1/user/hook.go @@ -62,6 +62,11 @@ func GetHook(ctx *context.APIContext) { return } + if !ctx.Doer.IsAdmin && hook.OwnerID != ctx.Doer.ID { + ctx.NotFound() + return + } + apiHook, err := webhook_service.ToHook(ctx.Doer.HomeLink(), hook) if err != nil { ctx.InternalServerError(err) From c487dc277fe6bb67719cbec000175c4643dadeac Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Nov 2023 22:32:36 +0800 Subject: [PATCH 12/18] Fix fixture --- models/fixtures/issue.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index ccc1fe41fbd97..0c9b6ff4060fe 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -61,7 +61,7 @@ priority: 0 is_closed: true is_pull: false - num_comments: 0 + num_comments: 1 created_unix: 946684830 updated_unix: 978307200 is_locked: false From 1f9338eb6ba6fd7454f7e3aedcc62c5ddf14027a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Nov 2023 22:55:15 +0800 Subject: [PATCH 13/18] Fix test --- tests/integration/api_nodeinfo_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/api_nodeinfo_test.go b/tests/integration/api_nodeinfo_test.go index 4cbd25f5deab1..fb35d72ac2ffc 100644 --- a/tests/integration/api_nodeinfo_test.go +++ b/tests/integration/api_nodeinfo_test.go @@ -34,6 +34,6 @@ func TestNodeinfo(t *testing.T) { assert.Equal(t, "gitea", nodeinfo.Software.Name) assert.Equal(t, 25, nodeinfo.Usage.Users.Total) assert.Equal(t, 20, nodeinfo.Usage.LocalPosts) - assert.Equal(t, 2, nodeinfo.Usage.LocalComments) + assert.Equal(t, 3, nodeinfo.Usage.LocalComments) }) } From 99f4ca3743a4298d144d1c4766d65c1f7bef2803 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 25 Nov 2023 23:22:23 +0800 Subject: [PATCH 14/18] Follow @delvh suggestion --- models/webhook/webhook.go | 12 ++---------- routers/web/repo/projects.go | 2 +- routers/web/repo/release.go | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index d59c7a2d7aae4..152c4c4bf73db 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -484,24 +484,16 @@ func DeleteWebhookByID(ctx context.Context, id int64) (err error) { // DeleteWebhookByRepoID deletes webhook of repository by given ID. func DeleteWebhookByRepoID(ctx context.Context, repoID, id int64) error { - webhook, err := GetWebhookByID(ctx, id) - if err != nil { + if _, err := GetWebhookByRepoID(ctx, repoID, id); err != nil { return err } - if webhook.RepoID != repoID { - return ErrWebhookNotExist{ID: id} - } return DeleteWebhookByID(ctx, id) } // DeleteWebhookByOwnerID deletes webhook of a user or organization by given ID. func DeleteWebhookByOwnerID(ctx context.Context, ownerID, id int64) error { - webhook, err := GetWebhookByID(ctx, id) - if err != nil { + if _, err := GetWebhookByOwnerID(ctx, ownerID, id); err != nil { return err } - if webhook.OwnerID != ownerID { - return ErrWebhookNotExist{ID: id} - } return DeleteWebhookByID(ctx, id) } diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 4c69c77435bba..0ddde26e688c6 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -479,7 +479,7 @@ func AddBoardToProjectPost(ctx *context.Context) { } if project.RepoID != ctx.Repo.Repository.ID { - ctx.NotFound("", nil) + ctx.NotFound("AddBoardToProjectPost", nil) return } diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index e6fffaf30f2db..acbc27bfd60a0 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -633,7 +633,7 @@ func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) { } if rel.RepoID != ctx.Repo.Repository.ID { - redirect() + ctx.NotFound("deleteReleaseOrTag", nil) return } From 470241494957e1290e057a760703d63ca21b173c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 26 Nov 2023 00:20:52 +0800 Subject: [PATCH 15/18] Follow wxiaoguang's suggestion --- models/asymkey/gpg_key.go | 13 +++++-------- models/issues/content_history.go | 7 ++++++- models/issues/content_history_test.go | 4 ++-- models/repo/release.go | 13 +++++++++++++ routers/api/v1/repo/release.go | 21 +++++++++------------ routers/api/v1/user/gpg_key.go | 6 +----- routers/web/repo/issue_content_history.go | 7 +------ routers/web/repo/release.go | 15 +++++++-------- 8 files changed, 44 insertions(+), 42 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 1da7c346def58..7d84f29c7d72f 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -92,8 +92,7 @@ func CountUserGPGKeys(ctx context.Context, userID int64) (int64, error) { return db.GetEngine(ctx).Where("owner_id=? AND primary_key_id=''", userID).Count(&GPGKey{}) } -// GetGPGKeyByID returns public key by given ID. -func GetGPGKeyByID(ctx context.Context, keyID int64) (*GPGKey, error) { +func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, error) { key := new(GPGKey) has, err := db.GetEngine(ctx).ID(keyID).Get(key) if err != nil { @@ -101,6 +100,9 @@ func GetGPGKeyByID(ctx context.Context, keyID int64) (*GPGKey, error) { } else if !has { return nil, ErrGPGKeyNotExist{keyID} } + if key.OwnerID != ownerID { + return nil, ErrGPGKeyNotExist{keyID} + } return key, nil } @@ -225,7 +227,7 @@ func deleteGPGKey(ctx context.Context, keyID string) (int64, error) { // DeleteGPGKey deletes GPG key information in database. func DeleteGPGKey(ctx context.Context, doer *user_model.User, id int64) (err error) { - key, err := GetGPGKeyByID(ctx, id) + key, err := GetGPGKeyForUserByID(ctx, doer.ID, id) if err != nil { if IsErrGPGKeyNotExist(err) { return nil @@ -233,11 +235,6 @@ func DeleteGPGKey(ctx context.Context, doer *user_model.User, id int64) (err err return fmt.Errorf("GetPublicKeyByID: %w", err) } - // Check if user has access to delete this key. - if !doer.IsAdmin && doer.ID != key.OwnerID { - return ErrGPGKeyAccessDenied{doer.ID, key.ID} - } - ctx, committer, err := db.TxContext(ctx) if err != nil { return err diff --git a/models/issues/content_history.go b/models/issues/content_history.go index cc06b184d7809..8a476d035fc5c 100644 --- a/models/issues/content_history.go +++ b/models/issues/content_history.go @@ -218,7 +218,7 @@ func GetIssueContentHistoryByID(dbCtx context.Context, id int64) (*ContentHistor } // GetIssueContentHistoryAndPrev get a history and the previous non-deleted history (to compare) -func GetIssueContentHistoryAndPrev(dbCtx context.Context, id int64) (history, prevHistory *ContentHistory, err error) { +func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (history, prevHistory *ContentHistory, err error) { history = &ContentHistory{} has, err := db.GetEngine(dbCtx).ID(id).Get(history) if err != nil { @@ -229,6 +229,11 @@ func GetIssueContentHistoryAndPrev(dbCtx context.Context, id int64) (history, pr return nil, nil, &ErrIssueContentHistoryNotExist{id} } + if history.IssueID != issueID { + log.Warn("issue content history have wrong issue id. history_id=%v, issue_id=%v", id, issueID) + return nil, nil, &ErrIssueContentHistoryNotExist{id} + } + prevHistory = &ContentHistory{} has, err = db.GetEngine(dbCtx).Where(builder.Eq{"issue_id": history.IssueID, "comment_id": history.CommentID, "is_deleted": false}). And(builder.Lt{"edited_unix": history.EditedUnix}). diff --git a/models/issues/content_history_test.go b/models/issues/content_history_test.go index 53638e967f200..0ea1d0f7b2e20 100644 --- a/models/issues/content_history_test.go +++ b/models/issues/content_history_test.go @@ -58,13 +58,13 @@ func TestContentHistory(t *testing.T) { hasHistory2, _ := issues_model.HasIssueContentHistory(dbCtx, 10, 1) assert.False(t, hasHistory2) - h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6) + h6, h6Prev, _ := issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6) assert.EqualValues(t, 6, h6.ID) assert.EqualValues(t, 5, h6Prev.ID) // soft-delete _ = issues_model.SoftDeleteIssueContentHistory(dbCtx, 5) - h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 6) + h6, h6Prev, _ = issues_model.GetIssueContentHistoryAndPrev(dbCtx, 10, 6) assert.EqualValues(t, 6, h6.ID) assert.EqualValues(t, 4, h6Prev.ID) diff --git a/models/repo/release.go b/models/repo/release.go index ff31ec451025e..c650682f55319 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -207,6 +207,19 @@ func GetReleaseByID(ctx context.Context, id int64) (*Release, error) { return rel, nil } +// GetReleaseForRepoByID returns release with given ID. +func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, error) { + rel, err := GetReleaseByID(ctx, id) + if err != nil { + return nil, err + } + if rel.RepoID != repoID { + return nil, ErrReleaseNotExist{id, ""} + } + + return rel, nil +} + // FindReleasesOptions describes the conditions to Find releases type FindReleasesOptions struct { db.ListOptions diff --git a/routers/api/v1/repo/release.go b/routers/api/v1/repo/release.go index c40b72929711c..6c70bffca33be 100644 --- a/routers/api/v1/repo/release.go +++ b/routers/api/v1/repo/release.go @@ -49,13 +49,12 @@ func GetRelease(ctx *context.APIContext) { // "$ref": "#/responses/notFound" id := ctx.ParamsInt64(":id") - release, err := repo_model.GetReleaseByID(ctx, id) + release, err := repo_model.GetReleaseForRepoByID(ctx, ctx.Repo.Repository.ID, id) if err != nil && !repo_model.IsErrReleaseNotExist(err) { - ctx.Error(http.StatusInternalServerError, "GetReleaseByID", err) + ctx.Error(http.StatusInternalServerError, "GetReleaseForRepoByID", err) return } - if err != nil && repo_model.IsErrReleaseNotExist(err) || - release.IsTag || release.RepoID != ctx.Repo.Repository.ID { + if err != nil && repo_model.IsErrReleaseNotExist(err) || release.IsTag { ctx.NotFound() return } @@ -315,13 +314,12 @@ func EditRelease(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.EditReleaseOption) id := ctx.ParamsInt64(":id") - rel, err := repo_model.GetReleaseByID(ctx, id) + rel, err := repo_model.GetReleaseForRepoByID(ctx, ctx.Repo.Repository.ID, id) if err != nil && !repo_model.IsErrReleaseNotExist(err) { - ctx.Error(http.StatusInternalServerError, "GetReleaseByID", err) + ctx.Error(http.StatusInternalServerError, "GetReleaseForRepoByID", err) return } - if err != nil && repo_model.IsErrReleaseNotExist(err) || - rel.IsTag || rel.RepoID != ctx.Repo.Repository.ID { + if err != nil && repo_model.IsErrReleaseNotExist(err) || rel.IsTag { ctx.NotFound() return } @@ -393,13 +391,12 @@ func DeleteRelease(ctx *context.APIContext) { // "$ref": "#/responses/empty" id := ctx.ParamsInt64(":id") - rel, err := repo_model.GetReleaseByID(ctx, id) + rel, err := repo_model.GetReleaseForRepoByID(ctx, ctx.Repo.Repository.ID, id) if err != nil && !repo_model.IsErrReleaseNotExist(err) { - ctx.Error(http.StatusInternalServerError, "GetReleaseByID", err) + ctx.Error(http.StatusInternalServerError, "GetReleaseForRepoByID", err) return } - if err != nil && repo_model.IsErrReleaseNotExist(err) || - rel.IsTag || rel.RepoID != ctx.Repo.Repository.ID { + if err != nil && repo_model.IsErrReleaseNotExist(err) || rel.IsTag { ctx.NotFound() return } diff --git a/routers/api/v1/user/gpg_key.go b/routers/api/v1/user/gpg_key.go index 374f15389c3e8..4f8bcaca3e6d5 100644 --- a/routers/api/v1/user/gpg_key.go +++ b/routers/api/v1/user/gpg_key.go @@ -112,7 +112,7 @@ func GetGPGKey(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - key, err := asymkey_model.GetGPGKeyByID(ctx, ctx.ParamsInt64(":id")) + key, err := asymkey_model.GetGPGKeyForUserByID(ctx, ctx.Doer.ID, ctx.ParamsInt64(":id")) if err != nil { if asymkey_model.IsErrGPGKeyNotExist(err) { ctx.NotFound() @@ -121,10 +121,6 @@ func GetGPGKey(ctx *context.APIContext) { } return } - if !ctx.Doer.IsAdmin && key.OwnerID != ctx.Doer.ID { - ctx.NotFound() - return - } ctx.JSON(http.StatusOK, convert.ToGPGKey(key)) } diff --git a/routers/web/repo/issue_content_history.go b/routers/web/repo/issue_content_history.go index 9967df024abbe..473ab260f3ea0 100644 --- a/routers/web/repo/issue_content_history.go +++ b/routers/web/repo/issue_content_history.go @@ -122,7 +122,7 @@ func GetContentHistoryDetail(ctx *context.Context) { } historyID := ctx.FormInt64("history_id") - history, prevHistory, err := issues_model.GetIssueContentHistoryAndPrev(ctx, historyID) + history, prevHistory, err := issues_model.GetIssueContentHistoryAndPrev(ctx, issue.ID, historyID) if err != nil { ctx.JSON(http.StatusNotFound, map[string]any{ "message": "Can not find the content history", @@ -130,11 +130,6 @@ func GetContentHistoryDetail(ctx *context.Context) { return } - if history.IssueID != issue.ID { - ctx.NotFound("GetContentHistoryDetail", nil) - return - } - // get the related comment if this history revision is for a comment, otherwise the history revision is for an issue. var comment *issues_model.Comment if history.CommentID != 0 { diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index acbc27bfd60a0..761dadd5444c5 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -625,15 +625,14 @@ func deleteReleaseOrTag(ctx *context.Context, isDelTag bool) { ctx.JSONRedirect(ctx.Repo.RepoLink + "/releases") } - rel, err := repo_model.GetReleaseByID(ctx, ctx.FormInt64("id")) + rel, err := repo_model.GetReleaseForRepoByID(ctx, ctx.Repo.Repository.ID, ctx.FormInt64("id")) if err != nil { - ctx.Flash.Error("DeleteReleaseByID: " + err.Error()) - redirect() - return - } - - if rel.RepoID != ctx.Repo.Repository.ID { - ctx.NotFound("deleteReleaseOrTag", nil) + if repo_model.IsErrReleaseNotExist(err) { + ctx.NotFound("GetReleaseForRepoByID", err) + } else { + ctx.Flash.Error("DeleteReleaseByID: " + err.Error()) + redirect() + } return } From 18e0547190ca94d5981ba394205c25c7db5a7afa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 26 Nov 2023 00:38:57 +0800 Subject: [PATCH 16/18] Follow wxiaoguang suggestion --- models/asymkey/gpg_key.go | 5 +---- models/issues/content_history.go | 7 +------ models/repo/release.go | 8 +++++--- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/models/asymkey/gpg_key.go b/models/asymkey/gpg_key.go index 7d84f29c7d72f..421f24d4de939 100644 --- a/models/asymkey/gpg_key.go +++ b/models/asymkey/gpg_key.go @@ -94,15 +94,12 @@ func CountUserGPGKeys(ctx context.Context, userID int64) (int64, error) { func GetGPGKeyForUserByID(ctx context.Context, ownerID, keyID int64) (*GPGKey, error) { key := new(GPGKey) - has, err := db.GetEngine(ctx).ID(keyID).Get(key) + has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", keyID, ownerID).Get(key) if err != nil { return nil, err } else if !has { return nil, ErrGPGKeyNotExist{keyID} } - if key.OwnerID != ownerID { - return nil, ErrGPGKeyNotExist{keyID} - } return key, nil } diff --git a/models/issues/content_history.go b/models/issues/content_history.go index 8a476d035fc5c..8c333bc6dd37c 100644 --- a/models/issues/content_history.go +++ b/models/issues/content_history.go @@ -220,7 +220,7 @@ func GetIssueContentHistoryByID(dbCtx context.Context, id int64) (*ContentHistor // GetIssueContentHistoryAndPrev get a history and the previous non-deleted history (to compare) func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (history, prevHistory *ContentHistory, err error) { history = &ContentHistory{} - has, err := db.GetEngine(dbCtx).ID(id).Get(history) + has, err := db.GetEngine(dbCtx).Where("id=? AND issue_id=?", id, issueID).Get(history) if err != nil { log.Error("failed to get issue content history %v. err=%v", id, err) return nil, nil, err @@ -229,11 +229,6 @@ func GetIssueContentHistoryAndPrev(dbCtx context.Context, issueID, id int64) (hi return nil, nil, &ErrIssueContentHistoryNotExist{id} } - if history.IssueID != issueID { - log.Warn("issue content history have wrong issue id. history_id=%v, issue_id=%v", id, issueID) - return nil, nil, &ErrIssueContentHistoryNotExist{id} - } - prevHistory = &ContentHistory{} has, err = db.GetEngine(dbCtx).Where(builder.Eq{"issue_id": history.IssueID, "comment_id": history.CommentID, "is_deleted": false}). And(builder.Lt{"edited_unix": history.EditedUnix}). diff --git a/models/repo/release.go b/models/repo/release.go index c650682f55319..223d3f2501922 100644 --- a/models/repo/release.go +++ b/models/repo/release.go @@ -209,11 +209,13 @@ func GetReleaseByID(ctx context.Context, id int64) (*Release, error) { // GetReleaseForRepoByID returns release with given ID. func GetReleaseForRepoByID(ctx context.Context, repoID, id int64) (*Release, error) { - rel, err := GetReleaseByID(ctx, id) + rel := new(Release) + has, err := db.GetEngine(ctx). + Where("id=? AND repo_id=?", id, repoID). + Get(rel) if err != nil { return nil, err - } - if rel.RepoID != repoID { + } else if !has { return nil, ErrReleaseNotExist{id, ""} } From 80496408f49b66412926af3eb32ddc76f44960fd Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 26 Nov 2023 00:49:56 +0800 Subject: [PATCH 17/18] More improvement --- models/project/project.go | 12 ++++++++++++ models/webhook/webhook.go | 12 ++++++------ routers/web/repo/projects.go | 7 +------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/models/project/project.go b/models/project/project.go index becfcbea1e687..d2fca6cdc8a8a 100644 --- a/models/project/project.go +++ b/models/project/project.go @@ -294,6 +294,18 @@ func GetProjectByID(ctx context.Context, id int64) (*Project, error) { return p, nil } +// GetProjectForRepoByID returns the projects in a repository +func GetProjectForRepoByID(ctx context.Context, repoID, id int64) (*Project, error) { + p := new(Project) + has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(p) + if err != nil { + return nil, err + } else if !has { + return nil, ErrProjectNotExist{ID: id} + } + return p, nil +} + // UpdateProject updates project properties func UpdateProject(ctx context.Context, p *Project) error { if !IsCardTypeValid(p.CardType) { diff --git a/models/webhook/webhook.go b/models/webhook/webhook.go index 152c4c4bf73db..a72bd938aacbb 100644 --- a/models/webhook/webhook.go +++ b/models/webhook/webhook.go @@ -406,11 +406,11 @@ func GetWebhookByID(ctx context.Context, id int64) (*Webhook, error) { // GetWebhookByRepoID returns webhook of repository by given ID. func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) { - webhook, err := GetWebhookByID(ctx, id) + webhook := new(Webhook) + has, err := db.GetEngine(ctx).Where("id=? AND repo_id=?", id, repoID).Get(webhook) if err != nil { return nil, err - } - if webhook.RepoID != repoID { + } else if !has { return nil, ErrWebhookNotExist{ID: id} } return webhook, nil @@ -418,11 +418,11 @@ func GetWebhookByRepoID(ctx context.Context, repoID, id int64) (*Webhook, error) // GetWebhookByOwnerID returns webhook of a user or organization by given ID. func GetWebhookByOwnerID(ctx context.Context, ownerID, id int64) (*Webhook, error) { - webhook, err := GetWebhookByID(ctx, id) + webhook := new(Webhook) + has, err := db.GetEngine(ctx).Where("id=? AND owner_id=?", id, ownerID).Get(webhook) if err != nil { return nil, err - } - if webhook.OwnerID != ownerID { + } else if !has { return nil, ErrWebhookNotExist{ID: id} } return webhook, nil diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 0ddde26e688c6..5694575b468de 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -468,7 +468,7 @@ func AddBoardToProjectPost(ctx *context.Context) { return } - project, err := project_model.GetProjectByID(ctx, ctx.ParamsInt64(":id")) + project, err := project_model.GetProjectForRepoByID(ctx, ctx.Repo.Repository.ID, ctx.ParamsInt64(":id")) if err != nil { if project_model.IsErrProjectNotExist(err) { ctx.NotFound("", nil) @@ -478,11 +478,6 @@ func AddBoardToProjectPost(ctx *context.Context) { return } - if project.RepoID != ctx.Repo.Repository.ID { - ctx.NotFound("AddBoardToProjectPost", nil) - return - } - if err := project_model.NewBoard(ctx, &project_model.Board{ ProjectID: project.ID, Title: form.Title, From e9f8eb311d464187cdadf5fd0e7331f247f0ac84 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 26 Nov 2023 00:58:01 +0800 Subject: [PATCH 18/18] Fix typo --- routers/api/v1/repo/issue.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 5a61f069c7fab..0f76a4b4ff26d 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -462,7 +462,7 @@ func ListIssues(ctx *context.APIContext) { isPull = util.OptionalBoolNone } - if isPull != util.OptionalBoolNone && !ctx.Repo.CanWriteIssuesOrPulls(isPull.IsTrue()) { + if isPull != util.OptionalBoolNone && !ctx.Repo.CanReadIssuesOrPulls(isPull.IsTrue()) { ctx.NotFound() return }