From e658a6a9cdfc0787427f8fc4dc90d38060c1dc71 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Thu, 11 Jan 2024 11:54:16 +0100 Subject: [PATCH] [GITEA] API commentAssignment() to verify the id belongs Instead of repeating the tests that verify the ID of a comment is related to the repository of the API endpoint, add the middleware function commentAssignment() to assign ctx.Comment if the ID of the comment is verified to be related to the repository. There already are integration tests for cases of potential unrelated comment IDs that cover some of the modified endpoints which covers the commentAssignment() function logic. * TestAPICommentReactions - GetIssueCommentReactions * TestAPICommentReactions - PostIssueCommentReaction * TestAPICommentReactions - DeleteIssueCommentReaction * TestAPIEditComment - EditIssueComment * TestAPIDeleteComment - DeleteIssueComment * TestAPIGetCommentAttachment - GetIssueCommentAttachment The other modified endpoints do not have tests to verify cases of potential unrelated comment IDs. They no longer need to because they no longer implement the logic to enforce this. They however all have integration tests that verify the commentAssignment() they now rely on does not introduce a regression. * TestAPIGetComment - GetIssueComment * TestAPIListCommentAttachments - ListIssueCommentAttachments * TestAPICreateCommentAttachment - CreateIssueCommentAttachment * TestAPIEditCommentAttachment - EditIssueCommentAttachment * TestAPIDeleteCommentAttachment - DeleteIssueCommentAttachment (cherry picked from commit d414376d749041da1be288c02fdaa24fddeafd5c) (cherry picked from commit 09db07aeaed167edc66cb832b0aa54b31d14f0d8) (cherry picked from commit f44830c3cba0b9416505a2b0b560cfa096ffeb7c) Conflicts: modules/context/api.go https://codeberg.org/forgejo/forgejo/pulls/2249 (cherry picked from commit 9d1bf7be15420ce4ca6e92a8bd048d483172de3b) --- modules/context/api.go | 2 + routers/api/v1/api.go | 36 +++++++++- routers/api/v1/repo/issue_comment.go | 68 ++----------------- .../api/v1/repo/issue_comment_attachment.go | 62 +++++------------ routers/api/v1/repo/issue_reaction.go | 52 +------------- 5 files changed, 61 insertions(+), 159 deletions(-) diff --git a/modules/context/api.go b/modules/context/api.go index e226264a8..05b6a7a53 100644 --- a/modules/context/api.go +++ b/modules/context/api.go @@ -11,6 +11,7 @@ import ( "net/url" "strings" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" mc "code.gitea.io/gitea/modules/cache" @@ -38,6 +39,7 @@ type APIContext struct { ContextUser *user_model.User // the user which is being visited, in most cases it differs from Doer Repo *Repository + Comment *issues_model.Comment Org *APIOrganization Package *Package } diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 72eb96465..c5399473c 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -73,6 +73,7 @@ import ( actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" @@ -230,6 +231,39 @@ func repoAssignment() func(ctx *context.APIContext) { } } +// must be used within a group with a call to repoAssignment() to set ctx.Repo +func commentAssignment(idParam string) func(ctx *context.APIContext) { + return func(ctx *context.APIContext) { + comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(idParam)) + if err != nil { + if issues_model.IsErrCommentNotExist(err) { + ctx.NotFound(err) + } else { + ctx.InternalServerError(err) + } + return + } + + if err = comment.LoadIssue(ctx); err != nil { + ctx.InternalServerError(err) + return + } + if comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID { + ctx.NotFound() + return + } + + if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { + ctx.NotFound() + return + } + + comment.Issue.Repo = ctx.Repo.Repository + + ctx.Comment = comment + } +} + func reqPackageAccess(accessMode perm.AccessMode) func(ctx *context.APIContext) { return func(ctx *context.APIContext) { if ctx.Package.AccessMode < accessMode && !ctx.IsUserSiteAdmin() { @@ -1333,7 +1367,7 @@ func Routes() *web.Route { Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment). Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueCommentAttachment) }, mustEnableAttachments) - }) + }, commentAssignment(":id")) }) m.Group("/{index}", func() { m.Combo("").Get(repo.GetIssue). diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index a58935473..91de21db4 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -450,29 +450,7 @@ func GetIssueComment(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) - if err != nil { - if issues_model.IsErrCommentNotExist(err) { - ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetCommentByID", err) - } - return - } - - if err = comment.LoadIssue(ctx); err != nil { - ctx.InternalServerError(err) - return - } - if comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.Status(http.StatusNotFound) - return - } - - if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { - ctx.NotFound() - return - } + comment := ctx.Comment if comment.Type != issues_model.CommentTypeComment { ctx.Status(http.StatusNoContent) @@ -583,25 +561,7 @@ func EditIssueCommentDeprecated(ctx *context.APIContext) { } func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) { - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) - if err != nil { - if issues_model.IsErrCommentNotExist(err) { - ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetCommentByID", err) - } - return - } - - 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 - } + comment := ctx.Comment if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) @@ -613,7 +573,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) return } - err = comment.LoadIssue(ctx) + err := comment.LoadIssue(ctx) if err != nil { ctx.Error(http.StatusInternalServerError, "LoadIssue", err) return @@ -707,25 +667,7 @@ func DeleteIssueCommentDeprecated(ctx *context.APIContext) { } func deleteIssueComment(ctx *context.APIContext) { - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) - if err != nil { - if issues_model.IsErrCommentNotExist(err) { - ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetCommentByID", err) - } - return - } - - 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 - } + comment := ctx.Comment if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { ctx.Status(http.StatusForbidden) @@ -735,7 +677,7 @@ func deleteIssueComment(ctx *context.APIContext) { return } - if err = issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { + if err := issue_service.DeleteComment(ctx, ctx.Doer, comment); err != nil { ctx.Error(http.StatusInternalServerError, "DeleteCommentByID", err) return } diff --git a/routers/api/v1/repo/issue_comment_attachment.go b/routers/api/v1/repo/issue_comment_attachment.go index 4f4b88750..5622a9292 100644 --- a/routers/api/v1/repo/issue_comment_attachment.go +++ b/routers/api/v1/repo/issue_comment_attachment.go @@ -55,11 +55,8 @@ func GetIssueCommentAttachment(ctx *context.APIContext) { // "404": // "$ref": "#/responses/error" - comment := getIssueCommentSafe(ctx) - if comment == nil { - return - } - attachment := getIssueCommentAttachmentSafeRead(ctx, comment) + comment := ctx.Comment + attachment := getIssueCommentAttachmentSafeRead(ctx) if attachment == nil { return } @@ -101,10 +98,7 @@ func ListIssueCommentAttachments(ctx *context.APIContext) { // "$ref": "#/responses/AttachmentList" // "404": // "$ref": "#/responses/error" - comment := getIssueCommentSafe(ctx) - if comment == nil { - return - } + comment := ctx.Comment if err := comment.LoadAttachments(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) @@ -166,14 +160,12 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) { // "$ref": "#/responses/repoArchivedError" // Check if comment exists and load comment - comment := getIssueCommentSafe(ctx) - if comment == nil { + + if !canUserWriteIssueCommentAttachment(ctx) { return } - if !canUserWriteIssueCommentAttachment(ctx, comment) { - return - } + comment := ctx.Comment updatedAt := ctx.Req.FormValue("updated_at") if len(updatedAt) != 0 { @@ -341,42 +333,17 @@ func DeleteIssueCommentAttachment(ctx *context.APIContext) { ctx.Status(http.StatusNoContent) } -func getIssueCommentSafe(ctx *context.APIContext) *issues_model.Comment { - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64("id")) - if err != nil { - ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err) - return nil - } - if err := comment.LoadIssue(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "comment.LoadIssue", err) - return nil - } - if comment.Issue == nil || comment.Issue.RepoID != ctx.Repo.Repository.ID { - ctx.Error(http.StatusNotFound, "", "no matching issue comment found") - return nil - } - - if !ctx.Repo.CanReadIssuesOrPulls(comment.Issue.IsPull) { - return nil - } - - comment.Issue.Repo = ctx.Repo.Repository - - return comment -} - func getIssueCommentAttachmentSafeWrite(ctx *context.APIContext) *repo_model.Attachment { - comment := getIssueCommentSafe(ctx) - if comment == nil { + if !canUserWriteIssueCommentAttachment(ctx) { return nil } - if !canUserWriteIssueCommentAttachment(ctx, comment) { - return nil - } - return getIssueCommentAttachmentSafeRead(ctx, comment) + return getIssueCommentAttachmentSafeRead(ctx) } -func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues_model.Comment) bool { +func canUserWriteIssueCommentAttachment(ctx *context.APIContext) bool { + // ctx.Comment is assumed to be set in a safe way via a middleware + comment := ctx.Comment + canEditComment := ctx.IsSigned && (ctx.Doer.ID == comment.PosterID || ctx.IsUserRepoAdmin() || ctx.IsUserSiteAdmin()) && ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) if !canEditComment { ctx.Error(http.StatusForbidden, "", "user should have permission to edit comment") @@ -386,7 +353,10 @@ func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues return true } -func getIssueCommentAttachmentSafeRead(ctx *context.APIContext, comment *issues_model.Comment) *repo_model.Attachment { +func getIssueCommentAttachmentSafeRead(ctx *context.APIContext) *repo_model.Attachment { + // ctx.Comment is assumed to be set in a safe way via a middleware + comment := ctx.Comment + attachment, err := repo_model.GetAttachmentByID(ctx, ctx.ParamsInt64("attachment_id")) if err != nil { ctx.NotFoundOrServerError("GetAttachmentByID", repo_model.IsErrAttachmentNotExist, err) diff --git a/routers/api/v1/repo/issue_reaction.go b/routers/api/v1/repo/issue_reaction.go index c886bd71b..33724ebbd 100644 --- a/routers/api/v1/repo/issue_reaction.go +++ b/routers/api/v1/repo/issue_reaction.go @@ -49,30 +49,7 @@ func GetIssueCommentReactions(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) - if err != nil { - if issues_model.IsErrCommentNotExist(err) { - ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetCommentByID", err) - } - return - } - - 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) { - ctx.Error(http.StatusForbidden, "GetIssueCommentReactions", errors.New("no permission to get reactions")) - return - } + comment := ctx.Comment reactions, _, err := issues_model.FindCommentReactions(ctx, comment.IssueID, comment.ID) if err != nil { @@ -186,30 +163,7 @@ func DeleteIssueCommentReaction(ctx *context.APIContext) { } func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOption, isCreateType bool) { - comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) - if err != nil { - if issues_model.IsErrCommentNotExist(err) { - ctx.NotFound(err) - } else { - ctx.Error(http.StatusInternalServerError, "GetCommentByID", err) - } - return - } - - 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 - } + comment := ctx.Comment if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) { ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction")) @@ -241,7 +195,7 @@ func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOp }) } else { // DeleteIssueCommentReaction part - err = issues_model.DeleteCommentReaction(ctx, ctx.Doer.ID, comment.Issue.ID, comment.ID, form.Reaction) + err := issues_model.DeleteCommentReaction(ctx, ctx.Doer.ID, comment.Issue.ID, comment.ID, form.Reaction) if err != nil { ctx.Error(http.StatusInternalServerError, "DeleteCommentReaction", err) return