From 4b8293aa094e725b372329a19da687a6d1550069 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 2 Mar 2024 00:46:02 +0800 Subject: [PATCH] Fix issue & comment history bugs (#29525) * Follow #17746: `HasIssueContentHistory` should use expr builder to make sure zero value (0) be respected. * Add "doer" check to make sure `canSoftDeleteContentHistory` only be called by sign-in users. --- models/issues/content_history.go | 8 ++------ models/issues/content_history_test.go | 19 +++++++++++++++++++ routers/web/repo/issue_content_history.go | 6 +++++- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/models/issues/content_history.go b/models/issues/content_history.go index 8b00adda99..31c80d2cea 100644 --- a/models/issues/content_history.go +++ b/models/issues/content_history.go @@ -172,13 +172,9 @@ func FetchIssueContentHistoryList(dbCtx context.Context, issueID, commentID int6 // HasIssueContentHistory check if a ContentHistory entry exists func HasIssueContentHistory(dbCtx context.Context, issueID, commentID int64) (bool, error) { - exists, err := db.GetEngine(dbCtx).Cols("id").Exist(&ContentHistory{ - IssueID: issueID, - CommentID: commentID, - }) + exists, err := db.GetEngine(dbCtx).Where(builder.Eq{"issue_id": issueID, "comment_id": commentID}).Exist(&ContentHistory{}) if err != nil { - log.Error("can not fetch issue content history. err=%v", err) - return false, err + return false, fmt.Errorf("can not check issue content history. err: %w", err) } return exists, err } diff --git a/models/issues/content_history_test.go b/models/issues/content_history_test.go index 0ea1d0f7b2..1caa73a948 100644 --- a/models/issues/content_history_test.go +++ b/models/issues/content_history_test.go @@ -78,3 +78,22 @@ func TestContentHistory(t *testing.T) { assert.EqualValues(t, 7, list2[1].HistoryID) assert.EqualValues(t, 4, list2[2].HistoryID) } + +func TestHasIssueContentHistoryForCommentOnly(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + _ = db.TruncateBeans(db.DefaultContext, &issues_model.ContentHistory{}) + + hasHistory1, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 10, 0) + assert.False(t, hasHistory1) + hasHistory2, _ := issues_model.HasIssueContentHistory(db.DefaultContext, 10, 100) + assert.False(t, hasHistory2) + + _ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 10, 100, timeutil.TimeStampNow(), "c-a", true) + _ = issues_model.SaveIssueContentHistory(db.DefaultContext, 1, 10, 100, timeutil.TimeStampNow().Add(5), "c-b", false) + + hasHistory1, _ = issues_model.HasIssueContentHistory(db.DefaultContext, 10, 0) + assert.False(t, hasHistory1) + hasHistory2, _ = issues_model.HasIssueContentHistory(db.DefaultContext, 10, 100) + assert.True(t, hasHistory2) +} diff --git a/routers/web/repo/issue_content_history.go b/routers/web/repo/issue_content_history.go index fce0eccc7b..1ec497658f 100644 --- a/routers/web/repo/issue_content_history.go +++ b/routers/web/repo/issue_content_history.go @@ -94,7 +94,7 @@ func canSoftDeleteContentHistory(ctx *context.Context, issue *issues_model.Issue // CanWrite means the doer can manage the issue/PR list if ctx.Repo.IsOwner() || ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) { canSoftDelete = true - } else { + } else if ctx.Doer != nil { // for read-only users, they could still post issues or comments, // they should be able to delete the history related to their own issue/comment, a case is: // 1. the user posts some sensitive data @@ -186,6 +186,10 @@ func SoftDeleteContentHistory(ctx *context.Context) { if ctx.Written() { return } + if ctx.Doer == nil { + ctx.NotFound("Require SignIn", nil) + return + } commentID := ctx.FormInt64("comment_id") historyID := ctx.FormInt64("history_id")