[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

@@ -148,7 +148,7 @@ var Start action.GTSAction = func(ctx context.Context) error {
 	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.

internal/federation/dereferencing/dereferencer.go

@@ -22,6 +22,7 @@ import (
 	"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),

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)
 }
 

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.
+	// Check if this is a permitted status we should accept.
-	if err := d.fetchStatusPoll(ctx, status, latestStatus); err != nil {
+	permit, err := d.isPermittedStatus(ctx, status, latestStatus)
-		return nil, nil, gtserror.Newf("error populating poll for status %s: %w", uri, err)
+	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

internal/federation/federatingactor_test.go

@@ -27,6 +27,7 @@ import (
 
 	"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)

internal/federation/federator.go

@@ -22,6 +22,7 @@ import (
 	"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

internal/gtserror/error.go

@@ -39,13 +39,13 @@ const (
 	malformedKey
 	notRelevantKey
 	spamKey
+	notPermittedKey
 )
 
 // IsUnretrievable indicates that a call to retrieve a resource
-// (account, status, attachment, etc) could not be fulfilled,
+// (account, status, attachment, etc) could not be fulfilled, either
-// either because it was not found locally, or because some
+// because it was not found locally, or because some prerequisite
-// prerequisite remote resource call failed, making it impossible
+// remote resource call failed, making it impossible to return it.
-// to return the item.
 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:

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.

testrig/federator.go

@@ -19,6 +19,7 @@ package testrig
 
 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 @@ import (
 
 // 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)
 }