Merge pull request '[GITEA] API commentAssignment() to verify the id belongs' (#2126) from earl-warren/forgejo:wip-api-comment-middleware into forgejo-dependency

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2126
Reviewed-by: Loïc Dachary <dachary@noreply.codeberg.org>

modules/context/api.go

@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"strings"
 
+	issues_model "code.gitea.io/gitea/models/issues"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -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
 }

routers/api/v1/api.go

@@ -71,6 +71,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"
@@ -228,6 +229,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() {
@@ -1322,7 +1356,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).

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"))
+	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 {
 		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"))
+	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)) {
 		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"))
+	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)) {
 		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
 	}

routers/api/v1/repo/issue_comment_attachment.go

@@ -55,11 +55,8 @@ func GetIssueCommentAttachment(ctx *context.APIContext) {
 	//   "404":
 	//     "$ref": "#/responses/error"
 
-	comment := getIssueCommentSafe(ctx)
+	comment := ctx.Comment
-	if comment == nil {
+	attachment := getIssueCommentAttachmentSafeRead(ctx)
-		return
-	}
-	attachment := getIssueCommentAttachmentSafeRead(ctx, comment)
 	if attachment == nil {
 		return
 	}
@@ -101,10 +98,7 @@ func ListIssueCommentAttachments(ctx *context.APIContext) {
 	//     "$ref": "#/responses/AttachmentList"
 	//   "404":
 	//     "$ref": "#/responses/error"
-	comment := getIssueCommentSafe(ctx)
+	comment := ctx.Comment
-	if comment == nil {
-		return
-	}
 
 	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) {
+	comment := ctx.Comment
-		return
-	}
 
 	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 !canUserWriteIssueCommentAttachment(ctx) {
-	if comment == 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)
 	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)

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"))
+	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)
 	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"))
+	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) {
 		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