From 5ee660e437c1fe7c2b72fb403847c3d257d45765 Mon Sep 17 00:00:00 2001 From: Basavaraj S m Date: Fri, 30 Oct 2026 05:27:51 +0530 Subject: [PATCH] fix(pull): refuse review comments on locked pull requests (#39960) Co-authored-by: silverwind Co-authored-by: wxiaoguang --- routers/api/v1/repo/issue_comment.go | 3 +- routers/api/v1/repo/pull_review.go | 46 ++++++++++++++------ routers/web/repo/issue.go | 16 ------- routers/web/repo/issue_comment.go | 20 +++------ routers/web/repo/pull_review.go | 8 ++++ routers/web/web.go | 2 +- services/issue/issue.go | 5 +++ services/mailer/incoming/incoming_handler.go | 3 +- tests/integration/pull_review_test.go | 30 ++++++++++++- web_src/js/features/repo-diff.ts | 5 ++- 10 files changed, 86 insertions(+), 52 deletions(-) diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 65599005e5f..819f7c99244 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -354,8 +354,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 } diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 59e4c58d30b..e8bc2398a29 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -262,6 +262,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, @@ -491,8 +494,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 } @@ -616,8 +623,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 } @@ -653,11 +664,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 { @@ -665,26 +683,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 } } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 1db9b83fbfd..5732194fd34 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -74,22 +74,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) && diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 0749cd0ee46..25a88bd8879 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -35,26 +35,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) diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 757ac1fe84b..90382eb0095 100644 --- a/routers/web/repo/pull_review.go +++ b/routers/web/repo/pull_review.go @@ -69,6 +69,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()) @@ -228,6 +232,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)) diff --git a/routers/web/web.go b/routers/web/web.go index d04815fd98c..77ebb76e19b 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1382,7 +1382,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) diff --git a/services/issue/issue.go b/services/issue/issue.go index 0621b65a995..4aa39a0ce40 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -334,3 +334,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) +} diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 2999985646b..71286b0ccc6 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/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 } diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 8c78eed61ac..bf205ce1eb7 100644 --- a/tests/integration/pull_review_test.go +++ b/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}) diff --git a/web_src/js/features/repo-diff.ts b/web_src/js/features/repo-diff.ts index b97ce11cad1..47fb2af69b0 100644 --- a/web_src/js/features/repo-diff.ts +++ b/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, type DiffFileTreeLocale, diffTreeStore} from '../modules/diff-file.ts'; import {initImageDiff} from './imagediff.ts'; import DiffCommitSelector from '../components/DiffCommitSelector.vue'; @@ -54,7 +54,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');