From 0a2d618d850df61c05fc22cd27119372771dc820 Mon Sep 17 00:00:00 2001 From: singuliere <35190819+singuliere@users.noreply.github.com> Date: Sun, 8 May 2022 14:05:40 +0100 Subject: [PATCH] GetFeeds must always discard actions with dangling repo_id (#19598) (#19629) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Loïc Dachary (cherry picked from commit b536b65189319544939da9b6537919a4fc838d71) Conflicts: models/action_test.go The GetFeeds function does not have a Context argument in 1.16. models/action.go The SQL statement is essentially the same in 1.16 but structured differently. The Join() was copied and the created_unix field prefixed with `action`. models/action_list.go in 1.16 the loadRepoOwner method did not exist and it was done in the RetrieveFeeds method of web/feed/profile.go. The safeguard to skip when act.Repo == nil was moved there. --- models/action.go | 8 ++++---- models/action_test.go | 17 +++++++++++++++++ models/fixtures/action.yml | 8 ++++++++ models/unittest/consistency.go | 6 ++++-- routers/web/feed/profile.go | 3 +++ 5 files changed, 36 insertions(+), 6 deletions(-) diff --git a/models/action.go b/models/action.go index 26d05730c5..951999e8e9 100644 --- a/models/action.go +++ b/models/action.go @@ -337,7 +337,7 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) { actions := make([]*Action, 0, setting.UI.FeedPagingNum) - if err := db.GetEngine(db.DefaultContext).Limit(setting.UI.FeedPagingNum).Desc("created_unix").Where(cond).Find(&actions); err != nil { + if err := db.GetEngine(db.DefaultContext).Limit(setting.UI.FeedPagingNum).Desc("`action`.created_unix").Where(cond).Join("INNER", "repository", "`repository`.id = `action`.repo_id").Find(&actions); err != nil { return nil, fmt.Errorf("Find: %v", err) } @@ -401,7 +401,7 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { cond = cond.And(builder.Eq{"act_user_id": opts.RequestedUser.ID}) } if !opts.IncludePrivate { - cond = cond.And(builder.Eq{"is_private": false}) + cond = cond.And(builder.Eq{"`action`.is_private": false}) } if !opts.IncludeDeleted { cond = cond.And(builder.Eq{"is_deleted": false}) @@ -414,8 +414,8 @@ func activityQueryCondition(opts GetFeedsOptions) (builder.Cond, error) { } else { dateHigh := dateLow.Add(86399000000000) // 23h59m59s - cond = cond.And(builder.Gte{"created_unix": dateLow.Unix()}) - cond = cond.And(builder.Lte{"created_unix": dateHigh.Unix()}) + cond = cond.And(builder.Gte{"`action`.created_unix": dateLow.Unix()}) + cond = cond.And(builder.Lte{"`action`.created_unix": dateHigh.Unix()}) } } diff --git a/models/action_test.go b/models/action_test.go index 306d382364..dec3e81f1a 100644 --- a/models/action_test.go +++ b/models/action_test.go @@ -129,3 +129,20 @@ func TestNotifyWatchers(t *testing.T) { OpType: action.OpType, }) } + +func TestGetFeedsCorrupted(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}).(*user_model.User) + unittest.AssertExistsAndLoadBean(t, &Action{ + ID: 8, + RepoID: 1700, + }) + + actions, err := GetFeeds(GetFeedsOptions{ + RequestedUser: user, + Actor: user, + IncludePrivate: true, + }) + assert.NoError(t, err) + assert.Len(t, actions, 0) +} diff --git a/models/fixtures/action.yml b/models/fixtures/action.yml index e3f3d2a971..32179079bb 100644 --- a/models/fixtures/action.yml +++ b/models/fixtures/action.yml @@ -56,3 +56,11 @@ repo_id: 8 is_private: false created_unix: 1603011540 # grouped with id:7 + +- id: 8 + user_id: 1 + op_type: 12 # close issue + act_user_id: 1 + repo_id: 1700 # dangling intentional + is_private: false + created_unix: 1603011541 diff --git a/models/unittest/consistency.go b/models/unittest/consistency.go index 2645084d3e..af05348868 100644 --- a/models/unittest/consistency.go +++ b/models/unittest/consistency.go @@ -175,8 +175,10 @@ func init() { checkForActionConsistency := func(t assert.TestingT, bean interface{}) { action := reflectionWrap(bean) - repoRow := AssertExistsAndLoadMap(t, "repository", builder.Eq{"id": action.int("RepoID")}) - assert.Equal(t, parseBool(repoRow["is_private"]), action.bool("IsPrivate"), "action: %+v", action) + if action.int("RepoID") != 1700 { // dangling intentional + repoRow := AssertExistsAndLoadMap(t, "repository", builder.Eq{"id": action.int("RepoID")}) + assert.Equal(t, parseBool(repoRow["is_private"]), action.bool("IsPrivate"), "action: %+v", action) + } } consistencyCheckMap["user"] = checkForUserConsistency diff --git a/routers/web/feed/profile.go b/routers/web/feed/profile.go index 1a7f4ad24b..bd6e381143 100644 --- a/routers/web/feed/profile.go +++ b/routers/web/feed/profile.go @@ -34,6 +34,9 @@ func RetrieveFeeds(ctx *context.Context, options models.GetFeedsOptions) []*mode } for _, act := range actions { + if act.Repo == nil { + continue + } repoOwner, ok := userCache[act.Repo.OwnerID] if !ok { repoOwner, err = user_model.GetUserByID(act.Repo.OwnerID)