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

[pieer]

Closes #205

Problem

A "random user" — neither the change request author nor the article owner / someone with write access — was offered all four options in the Submit Review modal (Comment, Approve, Request changes, Close). Approve silently did nothing and Close returned a 500 (there is no close review type server-side, so it fell through to ReviewTypeUnknown → ctx.ServerError).

Decision

Random users get Comment only. Approve, Request changes and Close are all unavailable to them.

Changes

• templates/repo/diff/review_submit.tmpl: the Approve / Request changes / Close options are now only rendered when the viewer has write access to the change request or is its author ($canReviewDecision). They are hidden, not merely disabled — same spirit as Fix 180: Improve UX of "Submit review" modal #192.
• routers/web/repo/pull_review.go: server-side enforcement in SubmitReview. approve / reject require write access; close additionally allows the change request author. Anything else gets a 403 with an errorMessage payload, which the form-fetch-action frontend renders as an error toast. This turns the old 500 on Close into a clean rejection.
• custom/options/locale/locale_en-US.ini: three new repo.issues.review.no_permission.* strings.
• tests/integration/pull_review_test.go: two new integration tests — one asserting 403 for approve/reject/close and 200 for comment from a random user (and that the author is not blocked by the new check), one asserting the rendered radio options per role.

Unchanged

Article owner / users with write access keep all four options. The change request author keeps the greyed-out Approve / Request changes options and Close, exactly as #192 left them.

Testing

make test-sqlite#TestPullView_SubmitReview — both new tests pass.

Model used: Claude Opus 5 (1M context)

🤖 Generated with Claude Code

[pieer]

/review

---

AI review started.

[github-actions]

Advanced AI Review

• Type: Agentic (crush)
• Model: glm-5.3

Click to expand review

Let me try to empirically confirm the "close → 500 for author/owner" path by running the new tests.Static evidence is conclusive for the main finding; no need for a multi-minute build. Let me do final checks on the API route middleware.# Review: Fix #205 — restrict "Submit Review" options

1. 🔴 The "Close" option still 500s for everyone it is now shown to

• Addressed
• Dismissed — out of scope for Improve UX/permissions of "Submit Review" modal #205 (which is about the random-user path); the pre-existing close 500 is filed as issue Submitting a "Close" review returns a 500 #347 and fixed in PR Fix #347: implement the "Close" review option instead of returning a 500 #350.

The permission gate lets write-access users and the change-request author through for close, but SubmitReview has never had a "close" review type — so those users fall straight into the pre-existing 500 that this PR claims to fix.

routers/web/repo/pull_review.go:247-251 (new code explicitly allows these users):

	case "close":
		if !isPoster {
			jsonForbidden(ctx, ctx.Locale.TrString("repo.issues.review.no_permission.close"))
			return false
		}

routers/web/repo/pull_review.go:280-284:

	reviewType := form.ReviewType()
	switch reviewType {
	case issues_model.ReviewTypeUnknown:
		ctx.ServerError("ReviewType", fmt.Errorf("unknown ReviewType: %s", form.Type))
		return

services/forms/repo_form.go:598-611 — no "close" case exists, so it hits default: return issues_model.ReviewTypeUnknown:

func (f SubmitReviewForm) ReviewType() issues_model.ReviewType {
	switch f.Type {
	case "approve":
		return issues_model.ReviewTypeApprove
	case "comment":
		return issues_model.ReviewTypeComment
	case "reject":
		return issues_model.ReviewTypeReject
	case "":
		return issues_model.ReviewTypeComment // default to comment when doing quick-submit (Ctrl+Enter) on the review form
	default:
		return issues_model.ReviewTypeUnknown
	}
}

(ReviewTypeClose doesn't exist anywhere in the codebase, and there is no client-side interception of type=close — the modal form POSTs directly to {{.Link}}/reviews/submit.)

Yet the modal still advertises the option to exactly those users, with templates/repo/diff/review_submit.tmpl:69-77 and custom/options/locale/locale_en-US.ini:2888:

					{{if $canReviewDecision}}
						<label class="review-radio-option">
							<input type="radio" name="type" value="close">

diff.review.close_desc = Submit feedback and close the change request without approving.

So the article owner — the primary user of this feature — selects "Close", and gets a 500 error page. The PR description's claim "This turns the old 500 on Close into a clean rejection" only holds for random users.

Suggested fix (Option A — implement it, matching the advertised semantics "submit feedback and close"), in routers/web/repo/pull_review.go (issue_service is already imported):

	reviewType := form.ReviewType()
	if form.Type == "close" {
		// "Close" submits the pending review as a comment and then closes the
		// change request, see `diff.review.close_desc`.
		reviewType = issues_model.ReviewTypeComment
	}

and after the successful pull_service.SubmitReview(...) call (before the final ctx.JSONRedirect):

	if form.Type == "close" && !issue.IsClosed {
		if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
			ctx.ServerError("CloseIssue", err)
			return
		}
	}

(issue_service.CloseIssue is the exact path the regular comment form uses at routers/web/repo/issue_comment.go:168-169.)

Option B — if "Close" from the review modal is not actually wanted, delete the radio option, the "close" case in checkReviewDecisionPermission, and the issues.review.no_permission.close locale string. But per #192 the author is supposed to keep it, so Option A is the real fix.

2. 🟡 New test masks the remaining 500 with assert.NotEqual(403)

• Addressed
• Dismissed — the NotEqual(403) assertion is the contract this PR owns; asserting a successful close belongs to PR Fix #347: implement the "Close" review option instead of returning a 500 #350 — the test comment now points at Submitting a "Close" review returns a 500 #347 instead of leaving the reason implicit.

tests/integration/pull_review_test.go:283-292:

	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
		resp := testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "close", NoExpectedStatus)
		assert.NotEqual(t, http.StatusForbidden, resp.Code)
	})

Given issue 1, this assertion passes precisely because the response is a 500 — the test enshrines the broken behavior as "anything but 403". Once issue 1 is fixed, assert the actual contract instead:

		resp := testSubmitReview(t, session, csrf, owner, repo, pullNumber, "", "close", http.StatusOK)

		// the change request is now closed
		req := NewRequest(t, "GET", path.Join("/", owner, repo, "pulls", pullNumber))
		resp = session.MakeRequest(t, req, http.StatusOK)
		NewHTMLParser(t, resp.Body).AssertElement(t, ".timeline-item .octicon-git-merge", false)

3. 🟡 jsonForbidden duplicates the existing context JSON-error helpers (DRY)

• Addressed — added (*context.Context).JSONForbidden next to JSONError/JSONErrorNotFound and deleted the package-local jsonForbidden.
• Dismissed

routers/web/repo/pull_review.go:223-227:

// jsonForbidden responds with a 403 carrying an "errorMessage" the frontend's
// fetch-action handler renders as an error toast.
func jsonForbidden(ctx *context.Context, msg string) {
	ctx.JSON(http.StatusForbidden, map[string]any{"errorMessage": msg, "renderFormat": "text"})
}

This hand-rolls the exact payload that services/context/context.go:255-272 already centralizes:

func (ctx *Context) JSONError(msg any) {
	switch v := msg.(type) {
	case string:
		ctx.JSON(http.StatusBadRequest, map[string]any{"errorMessage": v, "renderFormat": "text"})
	...
}

func (ctx *Context) JSONErrorNotFound(optMsg ...string) {
	...
	ctx.JSON(http.StatusNotFound, map[string]any{"errorMessage": msg, "renderFormat": "text"})
}

Follow the established pattern and add the 403 sibling next to them, then delete the package-local helper (also avoids a generic jsonForbidden name landing in the big repo package):

func (ctx *Context) JSONForbidden(msg string) {
	ctx.JSON(http.StatusForbidden, map[string]any{"errorMessage": msg, "renderFormat": "text"})
}

and in checkReviewDecisionPermission:

	case "approve":
		ctx.JSONForbidden(ctx.Locale.TrString("repo.issues.review.no_permission.approval"))
		return false

4. 🟡 The new policy is not enforced on the v1 API — approve/reject remain open to read-only users

• Addressed
• Dismissed — the v1 API path is untouched upstream Gitea behaviour and out of scope for Improve UX/permissions of "Submit Review" modal #205, which is about the Submit Review modal; tightening the API needs its own issue and its own product decision.

The PR bills the router change as "server-side enforcement", but it only covers the web form. routers/api/v1/api.go:1380-1384 exposes review submission with just reqToken() (no write requirement):

					m.Group("/reviews", func() {
						m.Combo("").
							Get(repo.ListPullReviews).
							Post(reqToken(), bind(api.CreatePullReviewOptions{}), repo.CreatePullReview)

and routers/api/v1/repo/pull_review.go:495-511 (preparePullReviewType) only blocks self-review, so any signed-in user with read access can still APPROVED/REQUEST_CHANGES someone else's change request via the API, bypassing the very rule this PR adds:

	case api.ReviewStateApproved:
		// can not approve your own PR
		if pr.Issue.IsPoster(ctx.Doer.ID) {
			ctx.APIError(http.StatusUnprocessableEntity, errors.New("approve your own pull is not allowed"))
			return -1, true
		}
		reviewType = issues_model.ReviewTypeApprove

If the API is intentionally exempt, that should be stated in the PR; otherwise add the equivalent check, e.g. in preparePullReviewType:

	case api.ReviewStateApproved, api.ReviewStateRequestChanges:
		if !ctx.Repo.CanWriteIssuesOrPulls(pr.Issue.IsPull) {
			ctx.APIError(http.StatusForbidden, errors.New("review decisions require write access"))
			return -1, true
		}

5. 🟡 Switching on the raw form.Type string creates a second source of truth for review types; inline comment overpromises

• Addressed — the helper now switches on form.ReviewType() for approve/reject (only close, which has no ReviewType, still matches the raw string), the redundant inline comment is gone and the doc comment says "write access to the repository".
• Dismissed

routers/web/repo/pull_review.go:240-252 validates the literal "approve"/"reject"/"close" strings, duplicating the mapping that SubmitReviewForm.ReviewType() already owns:

	switch formType {
	case "approve":
		jsonForbidden(ctx, ctx.Locale.TrString("repo.issues.review.no_permission.approval"))
		return false
	case "reject":
		jsonForbidden(ctx, ctx.Locale.TrString("repo.issues.review.no_permission.rejection"))
		return false
	case "close":

If a type is ever added/renamed in the form, this switch silently diverges. Prefer switching on the parsed type (works naturally once issue 1 lands):

func checkReviewDecisionPermission(ctx *context.Context, issue *issues_model.Issue, formType string) bool {
	if ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) {
		return true
	}
	isPoster := ctx.IsSigned && ctx.Doer != nil && issue.IsPoster(ctx.Doer.ID)

	switch formType {
	case "approve", "reject":
		jsonForbidden(ctx, ctx.Locale.TrString("repo.issues.review.no_permission."+formType))
		return false
	case "close":
		if !isPoster {
			jsonForbidden(ctx, ctx.Locale.TrString("repo.issues.review.no_permission.close"))
			return false
		}
	}
	return true
}

Also, the inline comment at routers/web/repo/pull_review.go:272-275 both duplicates the helper's doc comment and is factually wrong today (see issue 1 — permitted users still get a 500 on close):

	// Forkana: only the article owner (or anyone with write access to the change
	// request) may approve or request changes, and only they or the change
	// request author may close it. Everybody else is limited to plain comments,
	// so reject anything else instead of failing silently or with a 500.

Delete it — the call checkReviewDecisionPermission(ctx, issue, form.Type) plus the helper's doc comment already say this. And in the helper's doc, "require write access to the change request" should be "write access to the repository" (write access is to the article/repo, not to the change request).

6. ⚪️ Template: $showSelfTooltip is now overloaded as a permission gate under a presentation name

• Addressed — renamed to $isChangeRequestAuthor.
• Dismissed

templates/repo/diff/review_submit.tmpl:39,43:

					{{$showSelfTooltip := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
					...
					{{$canReviewDecision := (or $.HasIssuesOrPullsWritePermission $showSelfTooltip)}}

The variable is named after a tooltip presentation detail, but is now also the "is the change-request author" authorization fact. Rename it to what it means; the tooltip usages on lines 52-53 and 61-62 become self-explanatory:

					{{$isChangeRequestAuthor := (and $.IsSigned ($.Issue.IsPoster $.SignedUser.ID))}}
					{{$canReviewDecision := (or $.HasIssuesOrPullsWritePermission $isChangeRequestAuthor)}}

7. ⚪️ Template: duplicated gating condition for the approve/reject blocks

• Addressed — approve and reject now share a single {{if and $canReviewDecision (not $.Issue.IsClosed)}} block.
• Dismissed

templates/repo/diff/review_submit.tmpl:51 and :60 repeat the identical condition:

					{{if and $canReviewDecision (not $.Issue.IsClosed)}}

Wrap both labels in a single block:

					{{if and $canReviewDecision (not $.Issue.IsClosed)}}
						<label class="review-radio-option{{if $isChangeRequestAuthor}} disabled{{end}}" ...>
							... approve ...
						</label>
						<label class="review-radio-option{{if $isChangeRequestAuthor}} disabled{{end}}" ...>
							... reject ...
						</label>
					{{end}}

8. ⚪️ Tests: bypasses the Find helper; asserts less than the comment claims

• Addressed — uses the exported htmlDoc.Find helper and now asserts the disabled attribute — present for the author, absent for the owner.
• Dismissed

tests/integration/pull_review_test.go:306 reaches into the unexported field although HTMLDoc exposes a Find method:

		htmlDoc.doc.Find(`.review-radio-options input[name="type"]`).Each(func(_ int, s *goquery.Selection) {

Use the helper like the rest of the suite:

		htmlDoc.Find(`.review-radio-options input[name="type"]`).Each(func(_ int, s *goquery.Selection) {

Also, tests/integration/pull_review_test.go:319-320 says the author "keeps the (disabled) approve/reject options" but never asserts the disabled attribute:

	// the change request author keeps the (disabled) approve/reject options and close
	assert.ElementsMatch(t, []string{"comment", "approve", "reject", "close"}, reviewOptions(loginUser(t, "user1")))

Consider an extra assertion so a regression that re-enables self-approval can't slip through:

	for _, value := range []string{"approve", "reject"} {
		sel := htmlDoc.Find(`input[name="type"][value="` + value + `"]`)
		disabled, _ := sel.Attr("disabled")
		assert.Equal(t, "disabled", disabled, "author's %s option should be disabled", value)
	}

9. ⚪️ Any signed-in user can still force a 500 on this endpoint with a bogus type

• Addressed — an unknown review type is now a 400 ctx.JSONError instead of a 500.
• Dismissed

A random user posting type=garbage sails through the new check (no case matches) into routers/web/repo/pull_review.go:282-284:

	case issues_model.ReviewTypeUnknown:
		ctx.ServerError("ReviewType", fmt.Errorf("unknown ReviewType: %s", form.Type))
		return

Since a stated goal of this PR is eliminating 500s on this route, an unknown type is a client error — respond 400 instead:

	case issues_model.ReviewTypeUnknown:
		ctx.JSONError(fmt.Sprintf("unknown review type: %s", form.Type))
		return

No spelling or grammar problems found in the new locale strings (custom/options/locale/locale_en-US.ini:1938-1940) — they read well and match the tone of the neighboring issues.review.self.* and transfer.no_permission_* entries, and the keys correctly resolve under the [repo] section.

---

Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

[pieer]

Approved