Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions routers/web/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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") &&
Expand Down
9 changes: 5 additions & 4 deletions services/issue/comments.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,14 @@ 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)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to return 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 reloadedIssue, err := issues_model.GetIssueByID(ctx, issue.ID); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to return err

log.Error("GetIssueByID: %v", err)
} else {
issue = reloadedIssue
}

notify_service.CreateIssueComment(ctx, doer, repo, issue, comment, mentions)
Expand Down
35 changes: 35 additions & 0 deletions tests/integration/issue_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Expand Down