0
0
mirror of https://github.com/go-gitea/gitea.git synced 2026-05-24 04:02:38 +02:00

Fix garbled error response when issue comment creation fails

When NewComment's CreateIssueComment call failed, ctx.ServerError wrote
an HTML 500 page but the deferred function still ran and appended a JSON
redirect to the response body, producing an invalid mixed HTML+JSON
response shown as a toast error.

Fix by adding a ctx.Written() guard in the defer to skip writing when a
response was already sent. Also make post-creation errors in
CreateIssueComment (FindAndUpdateIssueMentions, GetIssueByID) non-fatal
since the comment is already persisted at that point, preventing
duplicate comments on retry.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
silverwind 2026-02-18 05:08:34 +01:00
parent 2cb8f6a9a5
commit 648c1714fe
No known key found for this signature in database
GPG Key ID: 2E62B41C93869443
3 changed files with 42 additions and 4 deletions

View File

@ -79,6 +79,10 @@ func NewComment(ctx *context.Context) {
var comment *issues_model.Comment
defer func() {
if ctx.Written() {
return
}
// Check if issue admin/poster changes the status of issue.
if (ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) || (ctx.IsSigned && issue.IsPoster(ctx.Doer.ID))) &&
(form.Status == "reopen" || form.Status == "close") &&

View File

@ -78,13 +78,12 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, doer, comment.Content)
if err != nil {
return nil, err
log.Error("FindAndUpdateIssueMentions: %v", err)
}
// reload issue to ensure it has the latest data, especially the number of comments
issue, err = issues_model.GetIssueByID(ctx, issue.ID)
if err != nil {
return nil, err
if issue, err = issues_model.GetIssueByID(ctx, issue.ID); err != nil {
log.Error("GetIssueByID: %v", err)
}
notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)

View File

@ -21,6 +21,7 @@ import (
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/indexer/issues"
"code.gitea.io/gitea/modules/json"
"code.gitea.io/gitea/modules/references"
"code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs"
@ -248,6 +249,40 @@ func TestIssueCommentClose(t *testing.T) {
assert.Equal(t, "Description", val)
}
// Test that error responses from NewComment are clean JSON without
// appended redirect JSON from the deferred function.
func TestIssueCommentErrorResponse(t *testing.T) {
defer tests.PrepareTestEnv(t)()
// user34 is blocked by user2 (repo1 owner), so commenting should fail
session := loginUser(t, "the_34-user.with.all.allowedChars")
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 1})
req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/1/comments", map[string]string{
"content": "blocked comment",
})
resp := session.MakeRequest(t, req, http.StatusBadRequest)
// Before the fix, the response body contained both a JSON error and a
// JSON redirect appended by the deferred function, producing an invalid
// response. Verify the body is valid JSON with only the error.
var result map[string]any
err := json.Unmarshal(resp.Body.Bytes(), &result)
assert.NoError(t, err, "response body should be valid JSON, got: %s", resp.Body.String())
assert.Contains(t, result, "errorMessage")
assert.NotContains(t, result, "redirect")
// Verify no comment was created
comments, err := issues_model.FindComments(t.Context(), &issues_model.FindCommentsOptions{
IssueID: issue.ID,
Type: issues_model.CommentTypeComment,
})
assert.NoError(t, err)
for _, c := range comments {
assert.NotEqual(t, "blocked comment", c.Content)
}
}
func TestIssueCommentDelete(t *testing.T) {
defer tests.PrepareTestEnv(t)()
session := loginUser(t, "user2")