From 62d4d756d3caba5737be0a5e3810f17ca92d7ef8 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 18 May 2022 23:23:49 +0200 Subject: [PATCH] [bugfix] Stop some statuses from being home timelined when they shouldn't be (#585) * recursively check timelineability of parent status * check following status creator * add tests for hometimelineability (whew) * add test with mix of public + unlocked vis --- internal/visibility/statushometimelineable.go | 32 +- .../visibility/statushometimelineable_test.go | 305 ++++++++++++++++++ 2 files changed, 328 insertions(+), 9 deletions(-) create mode 100644 internal/visibility/statushometimelineable_test.go diff --git a/internal/visibility/statushometimelineable.go b/internal/visibility/statushometimelineable.go index 62004cb5f..af871bcaa 100644 --- a/internal/visibility/statushometimelineable.go +++ b/internal/visibility/statushometimelineable.go @@ -33,7 +33,7 @@ func (f *filter) StatusHometimelineable(ctx context.Context, targetStatus *gtsmo }) // status owner should always be able to see their own status in their timeline so we can return early if this is the case - if timelineOwnerAccount != nil && targetStatus.AccountID == timelineOwnerAccount.ID { + if targetStatus.AccountID == timelineOwnerAccount.ID { return true, nil } @@ -54,13 +54,29 @@ func (f *filter) StatusHometimelineable(ctx context.Context, targetStatus *gtsmo } } + // check we follow the originator of the status + if targetStatus.Account == nil { + tsa, err := f.db.GetAccountByID(ctx, targetStatus.AccountID) + if err != nil { + return false, fmt.Errorf("StatusHometimelineable: error getting status author account with id %s: %s", targetStatus.AccountID, err) + } + targetStatus.Account = tsa + } + following, err := f.db.IsFollowing(ctx, timelineOwnerAccount, targetStatus.Account) + if err != nil { + return false, fmt.Errorf("StatusHometimelineable: error checking if %s follows %s: %s", timelineOwnerAccount.ID, targetStatus.AccountID, err) + } + if !following { + return false, nil + } + // Don't timeline a status whose parent hasn't been dereferenced yet or can't be dereferenced. // If we have the reply to URI but don't have an ID for the replied-to account or the replied-to status in our database, we haven't dereferenced it yet. if targetStatus.InReplyToURI != "" && (targetStatus.InReplyToID == "" || targetStatus.InReplyToAccountID == "") { return false, nil } - // if a status replies to an ID we know in the database, we need to make sure we also follow the replied-to status owner account + // if a status replies to an ID we know in the database, we need to check that parent status too if targetStatus.InReplyToID != "" { // pin the reply to status on to this status if it hasn't been done already if targetStatus.InReplyTo == nil { @@ -81,18 +97,16 @@ func (f *filter) StatusHometimelineable(ctx context.Context, targetStatus *gtsmo } // if it's a reply to the timelineOwnerAccount, we don't need to check if the timelineOwnerAccount follows itself, just return true, they can see it - if targetStatus.AccountID == timelineOwnerAccount.ID { + if targetStatus.InReplyToAccountID == timelineOwnerAccount.ID { return true, nil } - // the replied-to account != timelineOwnerAccount, so make sure the timelineOwnerAccount follows the replied-to account - follows, err := f.db.IsFollowing(ctx, timelineOwnerAccount, targetStatus.InReplyToAccount) + // make sure the parent status is also home timelineable, otherwise we shouldn't timeline this one either + parentStatusTimelineable, err := f.StatusHometimelineable(ctx, targetStatus.InReplyTo, timelineOwnerAccount) if err != nil { - return false, fmt.Errorf("StatusHometimelineable: error checking follow from account %s to account %s: %s", timelineOwnerAccount.ID, targetStatus.InReplyToAccountID, err) + return false, fmt.Errorf("StatusHometimelineable: error checking timelineability of parent status %s of status %s: %s", targetStatus.InReplyToID, targetStatus.ID, err) } - - // we don't want to timeline a reply to a status whose owner isn't followed by the requesting account - if !follows { + if !parentStatusTimelineable { return false, nil } } diff --git a/internal/visibility/statushometimelineable_test.go b/internal/visibility/statushometimelineable_test.go new file mode 100644 index 000000000..6161c52c0 --- /dev/null +++ b/internal/visibility/statushometimelineable_test.go @@ -0,0 +1,305 @@ +/* + GoToSocial + Copyright (C) 2021-2022 GoToSocial Authors admin@gotosocial.org + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU Affero General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU Affero General Public License for more details. + + You should have received a copy of the GNU Affero General Public License + along with this program. If not, see . +*/ + +package visibility_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/suite" + "github.com/superseriousbusiness/gotosocial/internal/ap" + "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/testrig" +) + +type StatusStatusHometimelineableTestSuite struct { + FilterStandardTestSuite +} + +func (suite *StatusStatusHometimelineableTestSuite) TestOwnStatusHometimelineable() { + testStatus := suite.testStatuses["local_account_1_status_1"] + testAccount := suite.testAccounts["local_account_1"] + ctx := context.Background() + + timelineable, err := suite.filter.StatusHometimelineable(ctx, testStatus, testAccount) + suite.NoError(err) + + suite.True(timelineable) +} + +func (suite *StatusStatusHometimelineableTestSuite) TestFollowingStatusHometimelineable() { + testStatus := suite.testStatuses["local_account_2_status_1"] + testAccount := suite.testAccounts["local_account_1"] + ctx := context.Background() + + timelineable, err := suite.filter.StatusHometimelineable(ctx, testStatus, testAccount) + suite.NoError(err) + + suite.True(timelineable) +} + +func (suite *StatusStatusHometimelineableTestSuite) TestNotFollowingStatusHometimelineable() { + testStatus := suite.testStatuses["remote_account_1_status_1"] + testAccount := suite.testAccounts["local_account_1"] + ctx := context.Background() + + timelineable, err := suite.filter.StatusHometimelineable(ctx, testStatus, testAccount) + suite.NoError(err) + + suite.False(timelineable) +} + +func (suite *StatusStatusHometimelineableTestSuite) TestChainReplyFollowersOnly() { + ctx := context.Background() + + // This scenario makes sure that we don't timeline a status which is a followers-only + // reply to a followers-only status TO A FOLLOWERS-ONLY STATUS owned by someone the + // timeline owner account doesn't follow. + // + // In other words, remote_account_1 posts a followers-only status, which local_account_1 replies to; + // THEN, local_account_1 replies to their own reply. We don't want this last status to appear + // in the timeline of local_account_2, even though they follow local_account_1, because they + // *don't* follow remote_account_1. + // + // See: https://github.com/superseriousbusiness/gotosocial/issues/501 + + originalStatusParent := suite.testAccounts["remote_account_1"] + replyingAccount := suite.testAccounts["local_account_1"] + timelineOwnerAccount := suite.testAccounts["local_account_2"] + + // put a followers-only status by remote_account_1 in the db + originalStatus := >smodel.Status{ + ID: "01G3957TS7XE2CMDKFG3MZPWAF", + URI: "http://fossbros-anonymous.io/users/foss_satan/statuses/01G3957TS7XE2CMDKFG3MZPWAF", + URL: "http://fossbros-anonymous.io/@foss_satan/statuses/01G3957TS7XE2CMDKFG3MZPWAF", + Content: "didn't expect dog", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:40:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:40:37+02:00"), + Local: false, + AccountURI: "http://fossbros-anonymous.io/users/foss_satan", + AccountID: originalStatusParent.ID, + InReplyToID: "", + InReplyToAccountID: "", + InReplyToURI: "", + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityFollowersOnly, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, originalStatus); err != nil { + suite.FailNow(err.Error()) + } + // this status should not be hometimelineable for local_account_2 + originalStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, originalStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(originalStatusTimelineable) + + // now a followers-only reply from zork + firstReplyStatus := >smodel.Status{ + ID: "01G395ESAYPK9161QSQEZKATJN", + URI: "http://localhost:8080/users/the_mighty_zork/statuses/01G395ESAYPK9161QSQEZKATJN", + URL: "http://localhost:8080/@the_mighty_zork/statuses/01G395ESAYPK9161QSQEZKATJN", + Content: "nbnbdy expects dog", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:41:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:41:37+02:00"), + Local: false, + AccountURI: "http://localhost:8080/users/the_mighty_zork", + AccountID: replyingAccount.ID, + InReplyToID: originalStatus.ID, + InReplyToAccountID: originalStatusParent.ID, + InReplyToURI: originalStatus.URI, + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityFollowersOnly, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, firstReplyStatus); err != nil { + suite.FailNow(err.Error()) + } + // this status should not be hometimelineable for local_account_2 + firstReplyStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, firstReplyStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(firstReplyStatusTimelineable) + + // now a followers-only reply from zork to the status they just replied to + secondReplyStatus := >smodel.Status{ + ID: "01G395NZQZGJYRBAES57KYZ7XP", + URI: "http://localhost:8080/users/the_mighty_zork/statuses/01G395NZQZGJYRBAES57KYZ7XP", + URL: "http://localhost:8080/@the_mighty_zork/statuses/01G395NZQZGJYRBAES57KYZ7XP", + Content: "*nobody", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:42:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:42:37+02:00"), + Local: false, + AccountURI: "http://localhost:8080/users/the_mighty_zork", + AccountID: replyingAccount.ID, + InReplyToID: firstReplyStatus.ID, + InReplyToAccountID: replyingAccount.ID, + InReplyToURI: firstReplyStatus.URI, + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityFollowersOnly, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, secondReplyStatus); err != nil { + suite.FailNow(err.Error()) + } + + // this status should ALSO not be hometimelineable for local_account_2 + secondReplyStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, secondReplyStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(secondReplyStatusTimelineable) +} + +func (suite *StatusStatusHometimelineableTestSuite) TestChainReplyPublicAndUnlocked() { + ctx := context.Background() + + // This scenario is exactly the same as the above test, but for a mix of unlocked + public posts + + originalStatusParent := suite.testAccounts["remote_account_1"] + replyingAccount := suite.testAccounts["local_account_1"] + timelineOwnerAccount := suite.testAccounts["local_account_2"] + + // put an unlocked status by remote_account_1 in the db + originalStatus := >smodel.Status{ + ID: "01G3957TS7XE2CMDKFG3MZPWAF", + URI: "http://fossbros-anonymous.io/users/foss_satan/statuses/01G3957TS7XE2CMDKFG3MZPWAF", + URL: "http://fossbros-anonymous.io/@foss_satan/statuses/01G3957TS7XE2CMDKFG3MZPWAF", + Content: "didn't expect dog", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:40:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:40:37+02:00"), + Local: false, + AccountURI: "http://fossbros-anonymous.io/users/foss_satan", + AccountID: originalStatusParent.ID, + InReplyToID: "", + InReplyToAccountID: "", + InReplyToURI: "", + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityUnlocked, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, originalStatus); err != nil { + suite.FailNow(err.Error()) + } + // this status should not be hometimelineable for local_account_2 + originalStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, originalStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(originalStatusTimelineable) + + // now a public reply from zork + firstReplyStatus := >smodel.Status{ + ID: "01G395ESAYPK9161QSQEZKATJN", + URI: "http://localhost:8080/users/the_mighty_zork/statuses/01G395ESAYPK9161QSQEZKATJN", + URL: "http://localhost:8080/@the_mighty_zork/statuses/01G395ESAYPK9161QSQEZKATJN", + Content: "nbnbdy expects dog", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:41:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:41:37+02:00"), + Local: false, + AccountURI: "http://localhost:8080/users/the_mighty_zork", + AccountID: replyingAccount.ID, + InReplyToID: originalStatus.ID, + InReplyToAccountID: originalStatusParent.ID, + InReplyToURI: originalStatus.URI, + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityPublic, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, firstReplyStatus); err != nil { + suite.FailNow(err.Error()) + } + // this status should not be hometimelineable for local_account_2 + firstReplyStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, firstReplyStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(firstReplyStatusTimelineable) + + // now an unlocked reply from zork to the status they just replied to + secondReplyStatus := >smodel.Status{ + ID: "01G395NZQZGJYRBAES57KYZ7XP", + URI: "http://localhost:8080/users/the_mighty_zork/statuses/01G395NZQZGJYRBAES57KYZ7XP", + URL: "http://localhost:8080/@the_mighty_zork/statuses/01G395NZQZGJYRBAES57KYZ7XP", + Content: "*nobody", + CreatedAt: testrig.TimeMustParse("2021-09-20T12:42:37+02:00"), + UpdatedAt: testrig.TimeMustParse("2021-09-20T12:42:37+02:00"), + Local: false, + AccountURI: "http://localhost:8080/users/the_mighty_zork", + AccountID: replyingAccount.ID, + InReplyToID: firstReplyStatus.ID, + InReplyToAccountID: replyingAccount.ID, + InReplyToURI: firstReplyStatus.URI, + BoostOfID: "", + ContentWarning: "", + Visibility: gtsmodel.VisibilityUnlocked, + Sensitive: false, + Language: "en", + CreatedWithApplicationID: "", + Federated: true, + Boostable: true, + Replyable: true, + Likeable: true, + ActivityStreamsType: ap.ObjectNote, + } + if err := suite.db.PutStatus(ctx, secondReplyStatus); err != nil { + suite.FailNow(err.Error()) + } + + // this status should ALSO not be hometimelineable for local_account_2 + secondReplyStatusTimelineable, err := suite.filter.StatusHometimelineable(ctx, secondReplyStatus, timelineOwnerAccount) + suite.NoError(err) + suite.False(secondReplyStatusTimelineable) +} + +func TestStatusHometimelineableTestSuite(t *testing.T) { + suite.Run(t, new(StatusStatusHometimelineableTestSuite)) +}