From d85727e184a8398ce0ffa40dfd01207342889076 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Mon, 4 Mar 2024 12:30:12 +0000 Subject: [PATCH] [bugfix] check remote status permissibility (#2703) * add more stringent checks for remote status permissibility * add check for inreplyto of a remote status being a boost * do not permit inReplyTo boost wrapper statuses * change comment wording * fix calls to NewFederator() * add code comments for NotPermitted() and SetNotPermitted() * improve comment * check that existing != nil before attempting delete * ensure replying account isn't suspended * use a debug log instead of info. check for boost using ID * shorten log string length. make info level * add note that replying to boost wrapper status shouldn't be able to happen anyways * update to use onFail() function --- cmd/gotosocial/action/server/server.go | 2 +- .../federation/dereferencing/dereferencer.go | 4 + .../dereferencing/dereferencer_test.go | 4 +- internal/federation/dereferencing/status.go | 96 ++++++++++++++++++- internal/federation/federatingactor_test.go | 22 ++++- internal/federation/federator.go | 4 +- internal/gtserror/error.go | 23 ++++- .../processing/workers/fromfediapi_test.go | 12 ++- testrig/federator.go | 3 +- 9 files changed, 154 insertions(+), 16 deletions(-) diff --git a/cmd/gotosocial/action/server/server.go b/cmd/gotosocial/action/server/server.go index 608a56a92..420264e97 100644 --- a/cmd/gotosocial/action/server/server.go +++ b/cmd/gotosocial/action/server/server.go @@ -148,7 +148,7 @@ func(context.Context, time.Time) { spamFilter := spam.NewFilter(&state) federatingDB := federatingdb.New(&state, typeConverter, visFilter, spamFilter) transportController := transport.NewController(&state, federatingDB, &federation.Clock{}, client) - federator := federation.NewFederator(&state, federatingDB, transportController, typeConverter, mediaManager) + federator := federation.NewFederator(&state, federatingDB, transportController, typeConverter, visFilter, mediaManager) // Decide whether to create a noop email // sender (won't send emails) or a real one. diff --git a/internal/federation/dereferencing/dereferencer.go b/internal/federation/dereferencing/dereferencer.go index f4596935e..24e579408 100644 --- a/internal/federation/dereferencing/dereferencer.go +++ b/internal/federation/dereferencing/dereferencer.go @@ -22,6 +22,7 @@ "sync" "time" + "github.com/superseriousbusiness/gotosocial/internal/filter/visibility" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/state" "github.com/superseriousbusiness/gotosocial/internal/transport" @@ -72,6 +73,7 @@ type Dereferencer struct { converter *typeutils.Converter transportController transport.Controller mediaManager *media.Manager + visibility *visibility.Filter // all protected by State{}.FedLocks. derefAvatars map[string]*media.ProcessingMedia @@ -87,6 +89,7 @@ func NewDereferencer( state *state.State, converter *typeutils.Converter, transportController transport.Controller, + visFilter *visibility.Filter, mediaManager *media.Manager, ) Dereferencer { return Dereferencer{ @@ -94,6 +97,7 @@ func NewDereferencer( converter: converter, transportController: transportController, mediaManager: mediaManager, + visibility: visFilter, derefAvatars: make(map[string]*media.ProcessingMedia), derefHeaders: make(map[string]*media.ProcessingMedia), derefEmojis: make(map[string]*media.ProcessingEmoji), diff --git a/internal/federation/dereferencing/dereferencer_test.go b/internal/federation/dereferencing/dereferencer_test.go index 7f79e8ac0..293118167 100644 --- a/internal/federation/dereferencing/dereferencer_test.go +++ b/internal/federation/dereferencing/dereferencer_test.go @@ -77,8 +77,10 @@ func (suite *DereferencerStandardTestSuite) SetupTest() { suite.storage = testrig.NewInMemoryStorage() suite.state.DB = suite.db suite.state.Storage = suite.storage + + visFilter := visibility.NewFilter(&suite.state) media := testrig.NewTestMediaManager(&suite.state) - suite.dereferencer = dereferencing.NewDereferencer(&suite.state, converter, testrig.NewTestTransportController(&suite.state, suite.client), media) + suite.dereferencer = dereferencing.NewDereferencer(&suite.state, converter, testrig.NewTestTransportController(&suite.state, suite.client), visFilter, media) testrig.StandardDBSetup(suite.db, nil) } diff --git a/internal/federation/dereferencing/status.go b/internal/federation/dereferencing/status.go index 397d2aa28..c8012178c 100644 --- a/internal/federation/dereferencing/status.go +++ b/internal/federation/dereferencing/status.go @@ -503,9 +503,16 @@ func (d *Dereferencer) enrichStatus( latestStatus.FetchedAt = time.Now() latestStatus.Local = status.Local - // Ensure the status' poll remains consistent, else reset the poll. - if err := d.fetchStatusPoll(ctx, status, latestStatus); err != nil { - return nil, nil, gtserror.Newf("error populating poll for status %s: %w", uri, err) + // Check if this is a permitted status we should accept. + permit, err := d.isPermittedStatus(ctx, status, latestStatus) + if err != nil { + return nil, nil, gtserror.Newf("error checking permissibility for status %s: %w", uri, err) + } + + if !permit { + // Return a checkable error type that can be ignored. + err := gtserror.Newf("dropping unpermitted status: %s", uri) + return nil, nil, gtserror.SetNotPermitted(err) } // Ensure the status' mentions are populated, and pass in existing to check for changes. @@ -513,6 +520,11 @@ func (d *Dereferencer) enrichStatus( return nil, nil, gtserror.Newf("error populating mentions for status %s: %w", uri, err) } + // Ensure the status' poll remains consistent, else reset the poll. + if err := d.fetchStatusPoll(ctx, status, latestStatus); err != nil { + return nil, nil, gtserror.Newf("error populating poll for status %s: %w", uri, err) + } + // Now that we know who this status replies to (handled by ASStatusToStatus) // and who it mentions, we can add a ThreadID to it if necessary. if err := d.threadStatus(ctx, latestStatus); err != nil { @@ -550,6 +562,84 @@ func (d *Dereferencer) enrichStatus( return latestStatus, apubStatus, nil } +// isPermittedStatus returns whether the given status +// is permitted to be stored on this instance, checking +// whether the author is suspended, and passes visibility +// checks against status being replied-to (if any). +func (d *Dereferencer) isPermittedStatus( + ctx context.Context, + existing *gtsmodel.Status, + status *gtsmodel.Status, +) ( + permitted bool, // is permitted? + err error, +) { + + // our failure condition handling + // at the end of this function for + // the case of permission = false. + onFail := func() (bool, error) { + if existing != nil { + log.Infof(ctx, "deleting unpermitted: %s", existing.URI) + + // Delete existing status from database as it's no longer permitted. + if err := d.state.DB.DeleteStatusByID(ctx, existing.ID); err != nil { + log.Errorf(ctx, "error deleting %s after permissivity fail: %v", existing.URI, err) + } + } + return false, nil + } + + if !status.Account.SuspendedAt.IsZero() { + // The status author is suspended, + // this shouldn't have reached here + // but it's a fast check anyways. + return onFail() + } + + if status.InReplyToURI == "" { + // This status isn't in + // reply to anything! + return true, nil + } + + if status.InReplyTo == nil { + // If no inReplyTo has been set, + // we return here for now as we + // can't perform further checks. + // + // Worst case we allow something + // through, and later on during + // refetch it will get deleted. + return true, nil + } + + if status.InReplyTo.BoostOfID != "" { + // We do not permit replies to + // boost wrapper statuses. (this + // shouldn't be able to happen). + return onFail() + } + + // Check visibility of inReplyTo to status author. + permitted, err = d.visibility.StatusVisible(ctx, + status.Account, + status.InReplyTo, + ) + if err != nil { + return false, gtserror.Newf("error checking in-reply-to visibility: %w", err) + } + + if permitted && + *status.InReplyTo.Replyable { + // This status is visible AND + // replyable, in this economy?! + return true, nil + } + + return onFail() +} + // populateMentionTarget tries to populate the given // mention with the correct TargetAccount and (if not // yet set) TargetAccountURI, returning the populated diff --git a/internal/federation/federatingactor_test.go b/internal/federation/federatingactor_test.go index b6814862f..0c805a2c6 100644 --- a/internal/federation/federatingactor_test.go +++ b/internal/federation/federatingactor_test.go @@ -27,6 +27,7 @@ "github.com/stretchr/testify/suite" "github.com/superseriousbusiness/gotosocial/internal/federation" + "github.com/superseriousbusiness/gotosocial/internal/filter/visibility" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/util" "github.com/superseriousbusiness/gotosocial/testrig" @@ -60,14 +61,21 @@ func (suite *FederatingActorTestSuite) TestSendNoRemoteFollowers() { tc := testrig.NewTestTransportController(&suite.state, httpClient) // setup module being tested - federator := federation.NewFederator(&suite.state, testrig.NewTestFederatingDB(&suite.state), tc, suite.typeconverter, testrig.NewTestMediaManager(&suite.state)) + federator := federation.NewFederator( + &suite.state, + testrig.NewTestFederatingDB(&suite.state), + tc, + suite.typeconverter, + visibility.NewFilter(&suite.state), + testrig.NewTestMediaManager(&suite.state), + ) activity, err := federator.FederatingActor().Send(ctx, testrig.URLMustParse(testAccount.OutboxURI), testActivity) suite.NoError(err) suite.NotNil(activity) // because zork has no remote followers, sent messages should be empty (no messages sent to own instance) - suite.Empty(httpClient.SentMessages) + suite.Empty(&httpClient.SentMessages) } func (suite *FederatingActorTestSuite) TestSendRemoteFollower() { @@ -105,8 +113,16 @@ func (suite *FederatingActorTestSuite) TestSendRemoteFollower() { httpClient := testrig.NewMockHTTPClient(nil, "../../testrig/media") tc := testrig.NewTestTransportController(&suite.state, httpClient) + // setup module being tested - federator := federation.NewFederator(&suite.state, testrig.NewTestFederatingDB(&suite.state), tc, suite.typeconverter, testrig.NewTestMediaManager(&suite.state)) + federator := federation.NewFederator( + &suite.state, + testrig.NewTestFederatingDB(&suite.state), + tc, + suite.typeconverter, + visibility.NewFilter(&suite.state), + testrig.NewTestMediaManager(&suite.state), + ) activity, err := federator.FederatingActor().Send(ctx, testrig.URLMustParse(testAccount.OutboxURI), testActivity) suite.NoError(err) diff --git a/internal/federation/federator.go b/internal/federation/federator.go index 8377546a1..f97d73cf8 100644 --- a/internal/federation/federator.go +++ b/internal/federation/federator.go @@ -22,6 +22,7 @@ "github.com/superseriousbusiness/gotosocial/internal/db" "github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing" "github.com/superseriousbusiness/gotosocial/internal/federation/federatingdb" + "github.com/superseriousbusiness/gotosocial/internal/filter/visibility" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/state" "github.com/superseriousbusiness/gotosocial/internal/transport" @@ -50,6 +51,7 @@ func NewFederator( federatingDB federatingdb.DB, transportController transport.Controller, converter *typeutils.Converter, + visFilter *visibility.Filter, mediaManager *media.Manager, ) *Federator { clock := &Clock{} @@ -60,7 +62,7 @@ func NewFederator( converter: converter, transportController: transportController, mediaManager: mediaManager, - Dereferencer: dereferencing.NewDereferencer(state, converter, transportController, mediaManager), + Dereferencer: dereferencing.NewDereferencer(state, converter, transportController, visFilter, mediaManager), } actor := newFederatingActor(f, f, federatingDB, clock) f.actor = actor diff --git a/internal/gtserror/error.go b/internal/gtserror/error.go index dc4c5a504..a39b5475c 100644 --- a/internal/gtserror/error.go +++ b/internal/gtserror/error.go @@ -39,13 +39,13 @@ malformedKey notRelevantKey spamKey + notPermittedKey ) // IsUnretrievable indicates that a call to retrieve a resource -// (account, status, attachment, etc) could not be fulfilled, -// either because it was not found locally, or because some -// prerequisite remote resource call failed, making it impossible -// to return the item. +// (account, status, attachment, etc) could not be fulfilled, either +// because it was not found locally, or because some prerequisite +// remote resource call failed, making it impossible to return it. func IsUnretrievable(err error) bool { _, ok := errors.Value(err, unrtrvableKey).(struct{}) return ok @@ -57,6 +57,21 @@ func SetUnretrievable(err error) error { return errors.WithValue(err, unrtrvableKey, struct{}{}) } +// NotPermitted indicates that some call failed due to failed permission +// or acceptibility checks. For example an attempt to dereference remote +// status in which the status author does not have permission to reply +// to the status it is intended to be replying to. +func NotPermitted(err error) bool { + _, ok := errors.Value(err, notPermittedKey).(struct{}) + return ok +} + +// SetNotPermitted will wrap the given error to store a "not permitted" +// flag, returning wrapped error. See NotPermitted() for example use-cases. +func SetNotPermitted(err error) error { + return errors.WithValue(err, notPermittedKey, struct{}{}) +} + // IsWrongType checks error for a stored "wrong type" flag. // Wrong type indicates that an ActivityPub URI returned a // type we weren't expecting. For example: diff --git a/internal/processing/workers/fromfediapi_test.go b/internal/processing/workers/fromfediapi_test.go index 446355628..60a9e785e 100644 --- a/internal/processing/workers/fromfediapi_test.go +++ b/internal/processing/workers/fromfediapi_test.go @@ -90,9 +90,17 @@ func (suite *FromFediAPITestSuite) TestProcessReplyMention() { replyingAccount := suite.testAccounts["remote_account_1"] // Set the replyingAccount's last fetched_at - // date to something recent so no refresh is attempted. + // date to something recent so no refresh is attempted, + // and ensure it isn't a suspended account. replyingAccount.FetchedAt = time.Now() - err := suite.state.DB.UpdateAccount(context.Background(), replyingAccount, "fetched_at") + replyingAccount.SuspendedAt = time.Time{} + replyingAccount.SuspensionOrigin = "" + err := suite.state.DB.UpdateAccount(context.Background(), + replyingAccount, + "fetched_at", + "suspended_at", + "suspension_origin", + ) suite.NoError(err) // Get replying statusable to use from remote test statuses. diff --git a/testrig/federator.go b/testrig/federator.go index 8519d724c..f90aa99ab 100644 --- a/testrig/federator.go +++ b/testrig/federator.go @@ -19,6 +19,7 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/federation" + "github.com/superseriousbusiness/gotosocial/internal/filter/visibility" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/state" "github.com/superseriousbusiness/gotosocial/internal/transport" @@ -27,5 +28,5 @@ // NewTestFederator returns a federator with the given database and (mock!!) transport controller. func NewTestFederator(state *state.State, tc transport.Controller, mediaManager *media.Manager) *federation.Federator { - return federation.NewFederator(state, NewTestFederatingDB(state), tc, typeutils.NewConverter(state), mediaManager) + return federation.NewFederator(state, NewTestFederatingDB(state), tc, typeutils.NewConverter(state), visibility.NewFilter(state), mediaManager) }