forks/forgejo
1
0
Fork 0
Code Releases Activity

[GITEA] API commentAssignment() to verify the id belongs

Browse source 
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 d414376d74)
(cherry picked from commit 09db07aeae)
(cherry picked from commit f44830c3cb)

Conflicts:
	modules/context/api.go
	https://codeberg.org/forgejo/forgejo/pulls/2249
This commit is contained in:
Earl Warren 2024-01-11 11:54:16 +01:00
parent aefa77f917
commit 9d1bf7be15
No known key found for this signature in database
GPG key ID: 0579CB2928A78A00
5 changed files with 61 additions and 159 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
modules/context/api.go
View file

@ -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
}

36
routers/api/v1/api.go
View file

@ -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).

68
routers/api/v1/repo/issue_comment.go
View file

@ -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
}

62
routers/api/v1/repo/issue_comment_attachment.go
View file

@ -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)

52
routers/api/v1/repo/issue_reaction.go
View file

@ -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