[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)
This commit is contained in:
Earl Warren 2024-01-11 11:54:16 +01:00
parent 2baec139fa
commit e658a6a9cd
No known key found for this signature in database
GPG key ID: 0579CB2928A78A00
5 changed files with 61 additions and 159 deletions

View file

@ -11,6 +11,7 @@ import (
"net/url" "net/url"
"strings" "strings"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
mc "code.gitea.io/gitea/modules/cache" 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 ContextUser *user_model.User // the user which is being visited, in most cases it differs from Doer
Repo *Repository Repo *Repository
Comment *issues_model.Comment
Org *APIOrganization Org *APIOrganization
Package *Package Package *Package
} }

View file

@ -73,6 +73,7 @@ import (
actions_model "code.gitea.io/gitea/models/actions" actions_model "code.gitea.io/gitea/models/actions"
auth_model "code.gitea.io/gitea/models/auth" auth_model "code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/db" "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/organization"
"code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/perm"
access_model "code.gitea.io/gitea/models/perm/access" 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) { func reqPackageAccess(accessMode perm.AccessMode) func(ctx *context.APIContext) {
return func(ctx *context.APIContext) { return func(ctx *context.APIContext) {
if ctx.Package.AccessMode < accessMode && !ctx.IsUserSiteAdmin() { if ctx.Package.AccessMode < accessMode && !ctx.IsUserSiteAdmin() {
@ -1333,7 +1367,7 @@ func Routes() *web.Route {
Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment). Patch(reqToken(), mustNotBeArchived, bind(api.EditAttachmentOptions{}), repo.EditIssueCommentAttachment).
Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueCommentAttachment) Delete(reqToken(), mustNotBeArchived, repo.DeleteIssueCommentAttachment)
}, mustEnableAttachments) }, mustEnableAttachments)
}) }, commentAssignment(":id"))
}) })
m.Group("/{index}", func() { m.Group("/{index}", func() {
m.Combo("").Get(repo.GetIssue). m.Combo("").Get(repo.GetIssue).

View file

@ -450,29 +450,7 @@ func GetIssueComment(ctx *context.APIContext) {
// "404": // "404":
// "$ref": "#/responses/notFound" // "$ref": "#/responses/notFound"
comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) comment := ctx.Comment
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
}
if comment.Type != issues_model.CommentTypeComment { if comment.Type != issues_model.CommentTypeComment {
ctx.Status(http.StatusNoContent) ctx.Status(http.StatusNoContent)
@ -583,25 +561,7 @@ func EditIssueCommentDeprecated(ctx *context.APIContext) {
} }
func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) { func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption) {
comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) comment := ctx.Comment
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
}
if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
ctx.Status(http.StatusForbidden) ctx.Status(http.StatusForbidden)
@ -613,7 +573,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
return return
} }
err = comment.LoadIssue(ctx) err := comment.LoadIssue(ctx)
if err != nil { if err != nil {
ctx.Error(http.StatusInternalServerError, "LoadIssue", err) ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
return return
@ -707,25 +667,7 @@ func DeleteIssueCommentDeprecated(ctx *context.APIContext) {
} }
func deleteIssueComment(ctx *context.APIContext) { func deleteIssueComment(ctx *context.APIContext) {
comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) comment := ctx.Comment
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
}
if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) { if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
ctx.Status(http.StatusForbidden) ctx.Status(http.StatusForbidden)
@ -735,7 +677,7 @@ func deleteIssueComment(ctx *context.APIContext) {
return 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) ctx.Error(http.StatusInternalServerError, "DeleteCommentByID", err)
return return
} }

View file

@ -55,11 +55,8 @@ func GetIssueCommentAttachment(ctx *context.APIContext) {
// "404": // "404":
// "$ref": "#/responses/error" // "$ref": "#/responses/error"
comment := getIssueCommentSafe(ctx) comment := ctx.Comment
if comment == nil { attachment := getIssueCommentAttachmentSafeRead(ctx)
return
}
attachment := getIssueCommentAttachmentSafeRead(ctx, comment)
if attachment == nil { if attachment == nil {
return return
} }
@ -101,10 +98,7 @@ func ListIssueCommentAttachments(ctx *context.APIContext) {
// "$ref": "#/responses/AttachmentList" // "$ref": "#/responses/AttachmentList"
// "404": // "404":
// "$ref": "#/responses/error" // "$ref": "#/responses/error"
comment := getIssueCommentSafe(ctx) comment := ctx.Comment
if comment == nil {
return
}
if err := comment.LoadAttachments(ctx); err != nil { if err := comment.LoadAttachments(ctx); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) ctx.Error(http.StatusInternalServerError, "LoadAttachments", err)
@ -166,14 +160,12 @@ func CreateIssueCommentAttachment(ctx *context.APIContext) {
// "$ref": "#/responses/repoArchivedError" // "$ref": "#/responses/repoArchivedError"
// Check if comment exists and load comment // Check if comment exists and load comment
comment := getIssueCommentSafe(ctx)
if comment == nil { if !canUserWriteIssueCommentAttachment(ctx) {
return return
} }
if !canUserWriteIssueCommentAttachment(ctx, comment) { comment := ctx.Comment
return
}
updatedAt := ctx.Req.FormValue("updated_at") updatedAt := ctx.Req.FormValue("updated_at")
if len(updatedAt) != 0 { if len(updatedAt) != 0 {
@ -341,42 +333,17 @@ func DeleteIssueCommentAttachment(ctx *context.APIContext) {
ctx.Status(http.StatusNoContent) 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 { func getIssueCommentAttachmentSafeWrite(ctx *context.APIContext) *repo_model.Attachment {
comment := getIssueCommentSafe(ctx) if !canUserWriteIssueCommentAttachment(ctx) {
if comment == nil {
return nil return nil
} }
if !canUserWriteIssueCommentAttachment(ctx, comment) { return getIssueCommentAttachmentSafeRead(ctx)
return nil
}
return getIssueCommentAttachmentSafeRead(ctx, comment)
} }
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) canEditComment := ctx.IsSigned && (ctx.Doer.ID == comment.PosterID || ctx.IsUserRepoAdmin() || ctx.IsUserSiteAdmin()) && ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)
if !canEditComment { if !canEditComment {
ctx.Error(http.StatusForbidden, "", "user should have permission to edit comment") ctx.Error(http.StatusForbidden, "", "user should have permission to edit comment")
@ -386,7 +353,10 @@ func canUserWriteIssueCommentAttachment(ctx *context.APIContext, comment *issues
return true 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")) attachment, err := repo_model.GetAttachmentByID(ctx, ctx.ParamsInt64("attachment_id"))
if err != nil { if err != nil {
ctx.NotFoundOrServerError("GetAttachmentByID", repo_model.IsErrAttachmentNotExist, err) ctx.NotFoundOrServerError("GetAttachmentByID", repo_model.IsErrAttachmentNotExist, err)

View file

@ -49,30 +49,7 @@ func GetIssueCommentReactions(ctx *context.APIContext) {
// "404": // "404":
// "$ref": "#/responses/notFound" // "$ref": "#/responses/notFound"
comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) comment := ctx.Comment
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
}
reactions, _, err := issues_model.FindCommentReactions(ctx, comment.IssueID, comment.ID) reactions, _, err := issues_model.FindCommentReactions(ctx, comment.IssueID, comment.ID)
if err != nil { if err != nil {
@ -186,30 +163,7 @@ func DeleteIssueCommentReaction(ctx *context.APIContext) {
} }
func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOption, isCreateType bool) { func changeIssueCommentReaction(ctx *context.APIContext, form api.EditReactionOption, isCreateType bool) {
comment, err := issues_model.GetCommentByID(ctx, ctx.ParamsInt64(":id")) comment := ctx.Comment
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
}
if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) { if comment.Issue.IsLocked && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull) {
ctx.Error(http.StatusForbidden, "ChangeIssueCommentReaction", errors.New("no permission to change reaction")) 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 { } else {
// DeleteIssueCommentReaction part // 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 { if err != nil {
ctx.Error(http.StatusInternalServerError, "DeleteCommentReaction", err) ctx.Error(http.StatusInternalServerError, "DeleteCommentReaction", err)
return return