Fix #205: restrict "Submit Review" options to what the user may actually do (#342)

* Fix #205: restrict "Submit Review" options to what the user may actually do

Users who are neither the change request author nor the article owner
(nor anyone with write access) were offered all four review options.
"Approve" silently failed and "Close" returned a 500 because there is no
"close" review type server-side.

Random users now only get the "Comment" option: "Approve", "Request
changes" and "Close" are no longer rendered for them, in the same spirit
as #192, and the server rejects such submissions with a 403 carrying an
error message instead of failing silently or with a 500.

Behaviour for the article owner / users with write access and for the
change request author (greyed-out approve/reject, per #192) is unchanged.

Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Fix #205: address review feedback on the Submit Review permission check

- derive the permission decision from `form.ReviewType()` instead of the raw
  form strings, so renaming a form value can no longer silently fail open;
  only "close", which has no `ReviewType` of its own, still matches the raw
  value
- replace the package-local `jsonForbidden` helper with a
  `(*context.Context).JSONForbidden` sibling of `JSONError`/`JSONErrorNotFound`
- answer an unknown review type with a 400 `JSONError` instead of a 500
- template: rename `$showSelfTooltip` to `$isChangeRequestAuthor` now that it
  is also a permission input, and merge the duplicated approve/reject guard
- tests: use the exported `HTMLDoc.Find` helper and assert the `disabled`
  attribute the author's approve/reject options are supposed to carry

Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Fix #205: satisfy perfsprint on the unknown-review-type error

The JSONError added while addressing review feedback used fmt.Sprintf with a
single trailing %s, which perfsprint rejects in favour of concatenation. This
was failing lint-backend, lint-go-gogit and lint-go-windows on PR #342; the
tests themselves were green.

Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: pieer

custom/options/locale/locale_en-US.ini

@@ -1935,6 +1935,9 @@ issues.dependency.add_error_cannot_create_circular = You cannot create a depende
 issues.dependency.add_error_dep_not_same_repo = Both issues must be in the same repository.
 issues.review.self.approval = You cannot approve your own change request.
 issues.review.self.rejection = You cannot request changes on your own change request.
+issues.review.no_permission.approval = You do not have permission to approve this change request.
+issues.review.no_permission.rejection = You do not have permission to request changes on this change request.
+issues.review.no_permission.close = You do not have permission to close this change request.
 issues.review.approve = "approved these changes %s"
 issues.review.comment = "reviewed %s"
 issues.review.dismissed = "dismissed %s's review %s"

routers/web/repo/pull_review.go

@@ -220,6 +220,37 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
 	}
 }
 
+// checkReviewDecisionPermission verifies that the doer is allowed to submit the
+// requested review type. Approving and requesting changes require write access
+// to the repository (the article), closing additionally allows the change
+// request author. Everybody else is limited to plain comments.
+// It returns false (and writes the response) when the submission is rejected.
+func checkReviewDecisionPermission(ctx *context.Context, issue *issues_model.Issue, form *forms.SubmitReviewForm) bool {
+	if ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
+		return true
+	}
+
+	switch form.ReviewType() {
+	case issues_model.ReviewTypeApprove:
+		ctx.JSONForbidden(ctx.Locale.TrString("repo.issues.review.no_permission.approval"))
+		return false
+	case issues_model.ReviewTypeReject:
+		ctx.JSONForbidden(ctx.Locale.TrString("repo.issues.review.no_permission.rejection"))
+		return false
+	}
+
+	// "close" has no ReviewType of its own, so it can only be matched on the raw
+	// form value; the change request author may close their own change request.
+	if form.Type == "close" {
+		isPoster := ctx.IsSigned && ctx.Doer != nil && issue.IsPoster(ctx.Doer.ID)
+		if !isPoster {
+			ctx.JSONForbidden(ctx.Locale.TrString("repo.issues.review.no_permission.close"))
+			return false
+		}
+	}
+	return true
+}
+
 // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
 func SubmitReview(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.SubmitReviewForm)
@@ -236,10 +267,15 @@ func SubmitReview(ctx *context.Context) {
 		return
 	}
 
+	if !checkReviewDecisionPermission(ctx, issue, form) {
+		return
+	}
+
 	reviewType := form.ReviewType()
 	switch reviewType {
 	case issues_model.ReviewTypeUnknown:
-		ctx.ServerError("ReviewType", fmt.Errorf("unknown ReviewType: %s", form.Type))
+		// an unknown type is a client error, not a server one
+		ctx.JSONError("unknown review type: " + form.Type)
 		return
 
 	// can not approve/reject your own PR

services/context/context.go

@@ -263,6 +263,12 @@ func (ctx *Context) JSONError(msg any) {
 	}
 }
 
+// JSONForbidden is the 403 sibling of JSONError: the frontend's fetch-action
+// handler renders the "errorMessage" payload as an error toast.
+func (ctx *Context) JSONForbidden(msg string) {
+	ctx.JSON(http.StatusForbidden, map[string]any{"errorMessage": msg, "renderFormat": "text"})
+}
+
 func (ctx *Context) JSONErrorNotFound(optMsg ...string) {
 	msg := util.OptionalArg(optMsg)
 	if msg == "" {

templates/repo/diff/review_submit.tmpl

@@ -36,39 +36,43 @@
 					</div>
 				{{end}}
 				<div class="review-radio-options">
-					{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
+					{{/* Only the article owner (or anyone with write access) may approve or request
+					changes; the change request author may additionally close their own change
+					request. Everybody else only gets the "Comment" option. */}}
+					{{$isChangeRequestAuthor := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
+					{{$canReviewDecision := (or $.HasIssuesOrPullsWritePermission $isChangeRequestAuthor)}}
 					<label class="review-radio-option">
 						<input type="radio" name="type" value="comment" checked>
 						<div class="review-radio-content">
 							<span class="review-radio-label">{{ctx.Locale.Tr "repo.diff.review.comment"}}</span>
 							<span class="review-radio-desc">{{ctx.Locale.Tr "repo.diff.review.comment_desc"}}</span>
 						</div>
 					</label>
-					{{if not $.Issue.IsClosed}}
-						<label class="review-radio-option{{if $showSelfTooltip}} disabled{{end}}" {{if $showSelfTooltip}}data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}"{{end}}>
-							<input type="radio" name="type" value="approve" {{if $showSelfTooltip}}disabled{{end}}>
+					{{if and $canReviewDecision (not $.Issue.IsClosed)}}
+						<label class="review-radio-option{{if $isChangeRequestAuthor}} disabled{{end}}" {{if $isChangeRequestAuthor}}data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_approve"}}"{{end}}>
+							<input type="radio" name="type" value="approve" {{if $isChangeRequestAuthor}}disabled{{end}}>
 							<div class="review-radio-content">
 								<span class="review-radio-label">{{ctx.Locale.Tr "repo.diff.review.approve"}}</span>
 								<span class="review-radio-desc">{{ctx.Locale.Tr "repo.diff.review.approve_desc"}}</span>
 							</div>
 						</label>
-					{{end}}
-					{{if not $.Issue.IsClosed}}
-						<label class="review-radio-option{{if $showSelfTooltip}} disabled{{end}}" {{if $showSelfTooltip}}data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}"{{end}}>
-							<input type="radio" name="type" value="reject" {{if $showSelfTooltip}}disabled{{end}}>
+						<label class="review-radio-option{{if $isChangeRequestAuthor}} disabled{{end}}" {{if $isChangeRequestAuthor}}data-tooltip-content="{{ctx.Locale.Tr "repo.diff.review.self_reject"}}"{{end}}>
+							<input type="radio" name="type" value="reject" {{if $isChangeRequestAuthor}}disabled{{end}}>
 							<div class="review-radio-content">
 								<span class="review-radio-label">{{ctx.Locale.Tr "repo.diff.review.reject"}}</span>
 								<span class="review-radio-desc">{{ctx.Locale.Tr "repo.diff.review.reject_desc"}}</span>
 							</div>
 						</label>
 					{{end}}
-					<label class="review-radio-option">
-						<input type="radio" name="type" value="close">
-						<div class="review-radio-content">
-							<span class="review-radio-label">{{ctx.Locale.Tr "repo.diff.review.close"}}</span>
-							<span class="review-radio-desc">{{ctx.Locale.Tr "repo.diff.review.close_desc"}}</span>
-						</div>
-					</label>
+					{{if $canReviewDecision}}
+						<label class="review-radio-option">
+							<input type="radio" name="type" value="close">
+							<div class="review-radio-content">
+								<span class="review-radio-label">{{ctx.Locale.Tr "repo.diff.review.close"}}</span>
+								<span class="review-radio-desc">{{ctx.Locale.Tr "repo.diff.review.close_desc"}}</span>
+							</div>
+						</label>
+					{{end}}
 				</div>
 			</div>
 			<div class="review-modal-footer">

tests/integration/pull_review_test.go

@@ -23,6 +23,7 @@ import (
 	files_service "code.gitea.io/gitea/services/repository/files"
 	"code.gitea.io/gitea/tests"
 
+	"github.com/PuerkitoBio/goquery"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -251,6 +252,89 @@ func TestPullView_GivenApproveOrRejectReviewOnClosedPR(t *testing.T) {
 	})
 }
 
+// TestPullView_SubmitReviewPermissions checks that a user without write access
+// to the change request, and who isn't its author, can only submit a plain
+// comment review: "approve", "reject" and "close" must be rejected server-side
+// (issue #205) instead of silently doing nothing or returning a 500.
+func TestPullView_SubmitReviewPermissions(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	// user2 owns repo1, user1 is the author of pull request #3, user5 is a
+	// random user with neither write access nor authorship.
+	const owner, repo, pullNumber = "user2", "repo1", "3"
+
+	getCSRF := func(session *TestSession) string {
+		req := NewRequest(t, "GET", path.Join("/", owner, repo, "pulls", pullNumber))
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		return NewHTMLParser(t, resp.Body).GetCSRF()
+	}
+
+	t.Run("RandomUser", func(t *testing.T) {
+		session := loginUser(t, "user5")
+		csrf := getCSRF(session)
+
+		testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "approve", http.StatusForbidden)
+		testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "reject", http.StatusForbidden)
+		testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "close", http.StatusForbidden)
+		// commenting stays available
+		testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "comment", http.StatusOK)
+	})
+
+	t.Run("Author", func(t *testing.T) {
+		// the change request author may not approve/reject (handled elsewhere),
+		// but must not be blocked by the write-access check
+		session := loginUser(t, "user1")
+		csrf := getCSRF(session)
+
+		// "close" is not turned into a permission error for the author. It does
+		// not succeed either — there is no "close" review type server-side, which
+		// is tracked separately as issue #347 — so only the absence of the 403
+		// this PR introduces can be asserted here.
+		resp := testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "close", NoExpectedStatus)
+		assert.NotEqual(t, http.StatusForbidden, resp.Code)
+	})
+}
+
+// TestPullView_SubmitReviewModalOptions checks that the disallowed options are
+// not even rendered in the "Submit Review" modal for a user without permission.
+func TestPullView_SubmitReviewModalOptions(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	filesLink := "/user2/repo1/pulls/3/files"
+	reviewModal := func(session *TestSession) *HTMLDoc {
+		req := NewRequest(t, "GET", filesLink)
+		resp := session.MakeRequest(t, req, http.StatusOK)
+		return NewHTMLParser(t, resp.Body)
+	}
+	reviewOptions := func(htmlDoc *HTMLDoc) []string {
+		var values []string
+		htmlDoc.Find(`.review-radio-options input[name="type"]`).Each(func(_ int, s *goquery.Selection) {
+			value, _ := s.Attr("value")
+			values = append(values, value)
+		})
+		return values
+	}
+
+	// a random user only gets the "comment" option
+	assert.Equal(t, []string{"comment"}, reviewOptions(reviewModal(loginUser(t, "user5"))))
+
+	// the repository owner keeps every option, all of them selectable
+	ownerDoc := reviewModal(loginUser(t, "user2"))
+	assert.ElementsMatch(t, []string{"comment", "approve", "reject", "close"}, reviewOptions(ownerDoc))
+	for _, value := range []string{"approve", "reject"} {
+		_, disabled := ownerDoc.Find(`.review-radio-options input[name="type"][value="` + value + `"]`).Attr("disabled")
+		assert.False(t, disabled, "the owner's %q option should be selectable", value)
+	}
+
+	// the change request author keeps the approve/reject options, but disabled (#192)
+	authorDoc := reviewModal(loginUser(t, "user1"))
+	assert.ElementsMatch(t, []string{"comment", "approve", "reject", "close"}, reviewOptions(authorDoc))
+	for _, value := range []string{"approve", "reject"} {
+		_, disabled := authorDoc.Find(`.review-radio-options input[name="type"][value="` + value + `"]`).Attr("disabled")
+		assert.True(t, disabled, "the author's %q option should be disabled", value)
+	}
+}
+
 func testSubmitReview(t *testing.T, session *TestSession, csrf, owner, repo, pullNumber, commitID, reviewType string, expectedSubmitStatus int) *httptest.ResponseRecorder {
 	options := map[string]string{
 		"_csrf":     csrf,