fix(pull): refuse review comments on locked pull requests (#39960) (#40084)

Backport https://github.com/go-gitea/gitea/pull/39960

Locking a pull request stops users without write access from commenting,
but the review paths never checked the lock, so any signed-in user could
still post reviews, code comments and replies on a locked pull request.

The cherry-pick conflicted on the `repo-diff.ts` imports, because
`release/v28` has no `DiffFileTreeLocale`.

Co-authored-by: Basavaraj S m <basavaraj@digiscrypt.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/api/v1/repo/issue_comment.go

@@ -358,8 +358,7 @@ func CreateIssueComment(ctx *context.APIContext) {
 		return
 	}
 
-	if issue.IsLocked && !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
-		ctx.APIError(http.StatusForbidden, ctx.Locale.TrString("repo.issues.comment_on_locked"))
+	if isIssueLockedForDoer(ctx, issue) {
 		return
 	}
 

routers/api/v1/repo/pull_review.go

@@ -266,6 +266,9 @@ func CreatePullReviewCommentReply(ctx *context.APIContext) {
 		ctx.APIError(http.StatusBadRequest, "comment is not a review comment")
 		return
 	}
+	if isIssueLockedForDoer(ctx, parent.Issue) {
+		return
+	}
 
 	comment, err := pull_service.CreateCodeComment(ctx,
 		ctx.Doer, ctx.Repo.GitRepo, parent.Issue,
@@ -507,8 +510,12 @@ func CreatePullReview(ctx *context.APIContext) {
 	}
 
 	// determine review type
-	reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body)
-	if isWrong {
+	reviewType := preparePullReviewType(ctx, pr, opts.Event, opts.Body)
+	if reviewType == issues_model.ReviewTypeUnknown {
+		return
+	}
+
+	if isIssueLockedForDoer(ctx, pr.Issue) {
 		return
 	}
 
@@ -634,8 +641,12 @@ func SubmitPullReview(ctx *context.APIContext) {
 	}
 
 	// determine review type
-	reviewType, isWrong := preparePullReviewType(ctx, pr, opts.Event, opts.Body)
-	if isWrong {
+	reviewType := preparePullReviewType(ctx, pr, opts.Event, opts.Body)
+	if reviewType == issues_model.ReviewTypeUnknown {
+		return
+	}
+
+	if isIssueLockedForDoer(ctx, pr.Issue) {
 		return
 	}
 
@@ -671,11 +682,18 @@ func SubmitPullReview(ctx *context.APIContext) {
 	ctx.JSON(http.StatusOK, apiReview)
 }
 
-// preparePullReviewType return ReviewType and false or nil and true if an error happen
-func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest, event api.ReviewStateType, body string) (issues_model.ReviewType, bool) {
+func isIssueLockedForDoer(ctx *context.APIContext, issue *issues_model.Issue) bool {
+	if issue_service.IsLockedForDoer(issue, ctx.Repo.Permission) {
+		ctx.APIError(http.StatusForbidden, "cannot comment on a locked issue")
+		return true
+	}
+	return false
+}
+
+func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest, event api.ReviewStateType, body string) issues_model.ReviewType {
 	if err := pr.LoadIssue(ctx); err != nil {
 		ctx.APIErrorInternal(err)
-		return -1, true
+		return issues_model.ReviewTypeUnknown
 	}
 
 	switch event {
@@ -683,26 +701,26 @@ func preparePullReviewType(ctx *context.APIContext, pr *issues_model.PullRequest
 		// can not approve your own PR
 		if pr.Issue.IsPoster(ctx.Doer.ID) {
 			ctx.APIError(http.StatusUnprocessableEntity, "approve your own pull is not allowed")
-			return -1, true
+			return issues_model.ReviewTypeUnknown
 		}
-		return issues_model.ReviewTypeApprove, false
+		return issues_model.ReviewTypeApprove
 
 	case api.ReviewStateRequestChanges:
 		// can not reject your own PR
 		if pr.Issue.IsPoster(ctx.Doer.ID) {
 			ctx.APIError(http.StatusUnprocessableEntity, "reject your own pull is not allowed")
-			return -1, true
+			return issues_model.ReviewTypeUnknown
 		}
 		if strings.TrimSpace(body) == "" {
 			ctx.APIError(http.StatusUnprocessableEntity, "review event REQUEST_CHANGES requires a body")
-			return -1, true
+			return issues_model.ReviewTypeUnknown
 		}
-		return issues_model.ReviewTypeReject, false
+		return issues_model.ReviewTypeReject
 
 	case api.ReviewStateComment:
-		return issues_model.ReviewTypeComment, false
+		return issues_model.ReviewTypeComment
 	default:
-		return issues_model.ReviewTypePending, false
+		return issues_model.ReviewTypePending
 	}
 }
 

routers/web/repo/issue.go

@@ -73,22 +73,6 @@ var IssueTemplateCandidates = []string{
 	".github/issue_template.yml",
 }
 
-// MustAllowUserComment checks to make sure if an issue is locked.
-// If locked and user has permissions to write to the repository,
-// then the comment is allowed, else it is blocked
-func MustAllowUserComment(ctx *context.Context) {
-	issue := GetActionIssue(ctx)
-	if ctx.Written() {
-		return
-	}
-
-	if issue.IsLocked && !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
-		ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked"))
-		ctx.Redirect(issue.Link())
-		return
-	}
-}
-
 // MustEnableIssues check if repository enable internal issues
 func MustEnableIssues(ctx *context.Context) {
 	if !ctx.Repo.Permission.CanRead(unit.TypeIssues) &&

routers/web/repo/issue_comment.go

@@ -36,26 +36,18 @@ func NewComment(ctx *context.Context) {
 		return
 	}
 
+	if issue_service.IsLockedForDoer(issue, ctx.Repo.Permission) {
+		ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
+		return
+	}
+
+	form := web.GetForm[*forms.CreateCommentForm](ctx)
 	if ctx.HasError() {
 		ctx.JSONError(ctx.GetErrMsg())
 		return
 	}
 
-	form := web.GetForm[*forms.CreateCommentForm](ctx)
 	issueType := util.Iif(issue.IsPull, "pulls", "issues")
-
-	if !ctx.IsSigned || (ctx.Doer.ID != issue.PosterID && !ctx.Repo.Permission.CanReadIssuesOrPulls(issue.IsPull)) {
-		log.Trace("Permission Denied: User %-v not the Poster (ID: %d) and cannot read %s in Repo %-v.\n"+
-			"User in Repo has Permissions: %-+v", ctx.Doer, issue.PosterID, issueType, ctx.Repo.Repository, ctx.Repo.Permission)
-		ctx.HTTPError(http.StatusForbidden)
-		return
-	}
-
-	if issue.IsLocked && !ctx.Repo.Permission.CanWriteIssuesOrPulls(issue.IsPull) && !ctx.Doer.IsAdmin {
-		ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
-		return
-	}
-
 	redirect := fmt.Sprintf("%s/%s/%d", ctx.Repo.RepoLink, issueType, issue.Index)
 	attachments := util.Iif(setting.Attachment.Enabled, form.Files, nil)
 

routers/web/repo/pull_review.go

@@ -70,6 +70,10 @@ func CreateCodeComment(ctx *context.Context) {
 	if !issue.IsPull {
 		return
 	}
+	if issue_service.IsLockedForDoer(issue, ctx.Repo.Permission) {
+		ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
+		return
+	}
 
 	if ctx.HasError() {
 		ctx.Flash.Error(ctx.GetErrMsg())
@@ -229,6 +233,10 @@ func SubmitReview(ctx *context.Context) {
 	if !issue.IsPull {
 		return
 	}
+	if issue_service.IsLockedForDoer(issue, ctx.Repo.Permission) {
+		ctx.JSONError(ctx.Tr("repo.issues.comment_on_locked"))
+		return
+	}
 	if ctx.HasError() {
 		ctx.Flash.Error(ctx.GetErrMsg())
 		ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))

routers/web/web.go

@@ -1361,7 +1361,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) {
 					m.Post("/add", repo.AddDependency)
 					m.Post("/delete", repo.RemoveDependency)
 				})
-				m.Combo("/comments").Post(repo.MustAllowUserComment, web.Bind[*forms.CreateCommentForm](), repo.NewComment)
+				m.Combo("/comments").Post(web.Bind[*forms.CreateCommentForm](), repo.NewComment)
 				m.Group("/times", func() {
 					m.Post("/add", web.Bind[*forms.AddTimeManuallyForm](), repo.AddTimeManually)
 					m.Post("/{timeid}/delete", repo.DeleteTime)

services/issue/issue.go

@@ -332,3 +332,8 @@ func CanSelfChangeOrAccessContent(doer *user_model.User, doerPerm access_model.P
 	}
 	return true
 }
+
+// IsLockedForDoer returns whether the lock keeps a doer with perm from commenting
+func IsLockedForDoer(issue *issues_model.Issue, perm access_model.Permission) bool {
+	return issue.IsLocked && !perm.CanWriteIssuesOrPulls(issue.IsPull)
+}

services/mailer/incoming/incoming_handler.go

@@ -72,8 +72,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
 		return err
 	}
 
-	// Locked issues require write permissions
-	if issue.IsLocked && !perm.CanWriteIssuesOrPulls(issue.IsPull) && !doer.IsAdmin {
+	if issue_service.IsLockedForDoer(issue, perm) {
 		log.Debug("can't write issue or pull")
 		return nil
 	}

tests/integration/pull_review_test.go

@@ -4,6 +4,7 @@
 package integration
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -12,6 +13,7 @@ import (
 	"testing"
 	"time"
 
+	auth_model "gitea.dev/models/auth"
 	"gitea.dev/models/db"
 	git_model "gitea.dev/models/git"
 	issues_model "gitea.dev/models/issues"
@@ -19,6 +21,7 @@ import (
 	"gitea.dev/models/unittest"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/git"
+	api "gitea.dev/modules/structs"
 	"gitea.dev/modules/test"
 	issue_service "gitea.dev/services/issue"
 	pull_service "gitea.dev/services/pull"
@@ -30,8 +33,13 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestPullView_ReviewerMissed(t *testing.T) {
+func TestPullReview(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
+	t.Run("ReviewerMissed", testPullReviewReviewerMissed)
+	t.Run("LockedPR", testPullReviewLockedPR)
+}
+
+func testPullReviewReviewerMissed(t *testing.T) {
 	session := loginUser(t, "user1")
 
 	req := NewRequest(t, "GET", "/pulls")
@@ -50,6 +58,26 @@ func TestPullView_ReviewerMissed(t *testing.T) {
 	assert.True(t, test.IsNormalPageCompleted(resp.Body.String()))
 }
 
+func testPullReviewLockedPR(t *testing.T) {
+	const reviewsURL = "/api/v1/repos/user2/repo1/pulls/3/reviews"
+	token := getUserToken(t, "user4", auth_model.AccessTokenScopeWriteRepository)
+	resp := MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, &api.CreatePullReviewOptions{Event: api.ReviewStatePending, Body: "test"}).AddTokenAuth(token), http.StatusOK)
+	pendingReview := DecodeJSON(t, resp, &api.PullReview{})
+	require.NoError(t, issues_model.UpdateIssueCols(t.Context(), &issues_model.Issue{ID: 3, IsLocked: true}, "is_locked"))
+	commentCount := unittest.GetCount(t, &issues_model.Comment{IssueID: 3})
+	reviewOptions := &api.CreatePullReviewOptions{Event: api.ReviewStateComment, Body: "test"}
+
+	session := loginUser(t, "user4")
+	testSubmitReview(t, session, "user2", "repo1", "3", "", "comment", http.StatusBadRequest)
+	session.MakeRequest(t, NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{"content": "test"}), http.StatusBadRequest)
+	MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, reviewOptions).AddTokenAuth(token), http.StatusForbidden)
+	MakeRequest(t, NewRequestWithJSON(t, "POST", fmt.Sprintf("%s/%d", reviewsURL, pendingReview.ID), &api.SubmitPullReviewOptions{Event: api.ReviewStateComment, Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
+	MakeRequest(t, NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/pulls/3/comments/7/replies", &api.CreatePullReviewCommentReplyOptions{Body: "test"}).AddTokenAuth(token), http.StatusForbidden)
+	unittest.AssertCount(t, &issues_model.Comment{IssueID: 3}, commentCount)
+
+	MakeRequest(t, NewRequestWithJSON(t, "POST", reviewsURL, reviewOptions).AddTokenAuth(getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository)), http.StatusOK)
+}
+
 func TestPullView_CodeOwner(t *testing.T) {
 	onGiteaRun(t, func(t *testing.T, u *url.URL) {
 		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})

web_src/js/features/repo-diff.ts

@@ -11,7 +11,7 @@ import {createTippy} from '../modules/tippy.ts';
 import {invertFileFolding} from './file-fold.ts';
 import {parseDom} from '../utils.ts';
 import {registerGlobalEventFunc, registerGlobalInitFunc} from '../modules/observer.ts';
-import {performFetchActionTrigger} from '../modules/fetch-action.ts';
+import {performFetchActionRequest, performFetchActionTrigger} from '../modules/fetch-action.ts';
 import {applyFiltersToFileBoxes, diffTreeStore} from '../modules/diff-file.ts';
 import {initImageDiff} from './imagediff.ts';
 
@@ -52,7 +52,8 @@ function initRepoDiffConversationForm() {
       // on the diff page, the form is inside a "tr" and need to get the line-type ahead
       // but on the conversation page, there is no parent "tr"
       const trLineType = form.closest('tr')?.getAttribute('data-line-type');
-      const response = await POST(form.getAttribute('action')!, {data: formData});
+      const response = await performFetchActionRequest(form, {method: 'POST', url: form.getAttribute('action')!, data: formData});
+      if (!response) return;
       const newConversationHolder = createElementFromHTML(await response.text());
       const path = newConversationHolder.getAttribute('data-path');
       const side = newConversationHolder.getAttribute('data-side');