[bugfix] don't accept unrelated statuses (#2078)

Co-authored-by: Daenney <daenney@users.noreply.github.com>
Co-authored-by: tsmethurst <tobi.smethurst@protonmail.com>

internal/federation/federatingdb/create.go

@@ -21,13 +21,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 
-	"codeberg.org/gruf/go-kv"
 	"codeberg.org/gruf/go-logger/v2/level"
+	"github.com/superseriousbusiness/activity/pub"
 	"github.com/superseriousbusiness/activity/streams/vocab"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
+	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
@@ -47,14 +47,16 @@ import (
 // Under certain conditions and network activities, Create may be called
 // multiple times for the same ActivityStreams object.
 func (f *federatingDB) Create(ctx context.Context, asType vocab.Type) error {
-	if log.Level() >= level.DEBUG {
+	if log.Level() >= level.TRACE {
 		i, err := marshalItem(asType)
 		if err != nil {
 			return err
 		}
-		l := log.WithContext(ctx).
-			WithField("create", i)
-		l.Trace("entering Create")
+
+		log.
+			WithContext(ctx).
+			WithField("create", i).
+			Trace("entering Create")
 	}
 
 	receivingAccount, requestingAccount, internal := extractFromCtx(ctx)
@@ -116,92 +118,125 @@ func (f *federatingDB) activityBlock(ctx context.Context, asType vocab.Type, rec
 	CREATE HANDLERS
 */
 
-func (f *federatingDB) activityCreate(ctx context.Context, asType vocab.Type, receivingAccount *gtsmodel.Account, requestingAccount *gtsmodel.Account) error {
+// activityCreate handles asType Create by checking
+// the Object entries of the Create and calling other
+// handlers as appropriate.
+func (f *federatingDB) activityCreate(
+	ctx context.Context,
+	asType vocab.Type,
+	receivingAccount *gtsmodel.Account,
+	requestingAccount *gtsmodel.Account,
+) error {
 	create, ok := asType.(vocab.ActivityStreamsCreate)
 	if !ok {
-		return errors.New("activityCreate: could not convert type to create")
+		return gtserror.Newf("could not convert asType %T to ActivityStreamsCreate", asType)
 	}
 
-	// create should have an object
-	object := create.GetActivityStreamsObject()
+	// Create must have an Object.
+	objectProp := create.GetActivityStreamsObject()
+	if objectProp == nil {
+		return gtserror.New("create had no Object")
+	}
+
+	// Iterate through the Object property and process FIRST provided statusable.
+	// todo: https://github.com/superseriousbusiness/gotosocial/issues/1905
+	for iter := objectProp.Begin(); iter != objectProp.End(); iter = iter.Next() {
+		object := iter.GetType()
 		if object == nil {
-		return errors.New("Create had no Object")
-	}
-
-	errs := []string{}
-	// iterate through the object(s) to see what we're meant to be creating
-	for objectIter := object.Begin(); objectIter != object.End(); objectIter = objectIter.Next() {
-		asObjectType := objectIter.GetType()
-		if asObjectType == nil {
-			// currently we can't do anything with just a Create of something that's not an Object with a type
-			// TODO: process a Create with an Object that's just a URI or something
-			errs = append(errs, "object of Create was not a Type")
+			// Can't do Create with Object that's just a URI.
+			// Warn log this because it's an AP error.
+			log.Warn(ctx, "object entry was not a type: %[1]T%[1]+v", iter)
 			continue
 		}
 
-		// we have a type -- what is it?
-		asObjectTypeName := asObjectType.GetTypeName()
-		switch asObjectTypeName {
-		case ap.ObjectNote:
-			// CREATE A NOTE
-			if err := f.createNote(ctx, objectIter.GetActivityStreamsNote(), receivingAccount, requestingAccount); err != nil {
-				errs = append(errs, err.Error())
-			}
-		default:
-			errs = append(errs, fmt.Sprintf("received an object on a Create that we couldn't handle: %s", asObjectType.GetTypeName()))
-		}
+		// Ensure given object type is a statusable.
+		statusable, ok := object.(ap.Statusable)
+		if !ok {
+			// Can't (currently) Create anything other than a Statusable. ([1] is a format arg index)
+			log.Debugf(ctx, "object entry type (currently) unsupported: %[1]T%[1]+v", object)
+			continue
 		}
 
-	if len(errs) != 0 {
-		return fmt.Errorf("activityCreate: one or more errors while processing activity: %s", strings.Join(errs, "; "))
+		// Handle creation of statusable.
+		return f.createStatusable(ctx,
+			statusable,
+			receivingAccount,
+			requestingAccount,
+		)
 	}
 
 	return nil
 }
 
-// createNote handles a Create activity with a Note type.
-func (f *federatingDB) createNote(ctx context.Context, note vocab.ActivityStreamsNote, receivingAccount *gtsmodel.Account, requestingAccount *gtsmodel.Account) error {
-	l := log.WithContext(ctx).
-		WithFields(kv.Fields{
-			{"receivingAccount", receivingAccount.URI},
-			{"requestingAccount", requestingAccount.URI},
-		}...)
+// createStatusable handles a Create activity for a Statusable.
+// This function won't insert anything in the database yet,
+// but will pass the Statusable (if appropriate) through to
+// the processor for further asynchronous processing.
+func (f *federatingDB) createStatusable(
+	ctx context.Context,
+	statusable ap.Statusable,
+	receivingAccount *gtsmodel.Account,
+	requestingAccount *gtsmodel.Account,
+) error {
+	// Statusable must have an attributedTo.
+	attrToProp := statusable.GetActivityStreamsAttributedTo()
+	if attrToProp == nil {
+		return gtserror.Newf("statusable had no attributedTo")
+	}
 
-	// Check if we have a forward.
-	// In other words, was the note posted to our inbox by at least one actor who actually created the note, or are they just forwarding it?
+	// Statusable must have an ID.
+	idProp := statusable.GetJSONLDId()
+	if idProp == nil || !idProp.IsIRI() {
+		return gtserror.Newf("statusable had no id, or id was not a URI")
+	}
+
+	statusableURI := idProp.GetIRI()
+
+	// Check if we have a forward. In other words, was the
+	// statusable posted to our inbox by at least one actor
+	// who actually created it, or are they forwarding it?
 	forward := true
-
-	// note should have an attributedTo
-	noteAttributedTo := note.GetActivityStreamsAttributedTo()
-	if noteAttributedTo == nil {
-		return errors.New("createNote: note had no attributedTo")
+	for iter := attrToProp.Begin(); iter != attrToProp.End(); iter = iter.Next() {
+		actorURI, err := pub.ToId(iter)
+		if err != nil {
+			return gtserror.Newf("error extracting id from attributedTo entry: %w", err)
 		}
 
-	// compare the attributedTo(s) with the actor who posted this to our inbox
-	for attributedToIter := noteAttributedTo.Begin(); attributedToIter != noteAttributedTo.End(); attributedToIter = attributedToIter.Next() {
-		if !attributedToIter.IsIRI() {
-			continue
-		}
-		iri := attributedToIter.GetIRI()
-		if requestingAccount.URI == iri.String() {
-			// at least one creator of the note, and the actor who posted the note to our inbox, are the same, so it's not a forward
+		if requestingAccount.URI == actorURI.String() {
+			// The actor who posted this statusable to our inbox is
+			// (one of) its creator(s), so this is not a forward.
 			forward = false
+			break
 		}
 	}
 
-	// If we do have a forward, we should ignore the content for now and just dereference based on the URL/ID of the note instead, to get the note straight from the horse's mouth
-	if forward {
-		l.Trace("note is a forward")
-		id := note.GetJSONLDId()
-		if !id.IsIRI() {
-			// if the note id isn't an IRI, there's nothing we can do here
+	// Check if we already have a status entry
+	// for this statusable, based on the ID/URI.
+	statusableURIStr := statusableURI.String()
+	status, err := f.state.DB.GetStatusByURI(ctx, statusableURIStr)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		return gtserror.Newf("db error checking existence of status %s: %w", statusableURIStr, err)
+	}
+
+	if status != nil {
+		// We already had this status in the db, no need for further action.
+		log.Trace(ctx, "status already exists: %s", statusableURIStr)
 		return nil
 	}
-		// pass the note iri into the processor and have it do the dereferencing instead of doing it here
+
+	// If we do have a forward, we should ignore the content
+	// and instead deref based on the URI of the statusable.
+	//
+	// In other words, don't automatically trust whoever sent
+	// this status to us, but fetch the authentic article from
+	// the server it originated from.
+	if forward {
+		// Pass the statusable URI (APIri) into the processor worker
+		// and do the rest of the processing asynchronously.
 		f.state.Workers.EnqueueFederator(ctx, messages.FromFederator{
 			APObjectType:     ap.ObjectNote,
 			APActivityType:   ap.ActivityCreate,
-			APIri:            id.GetIRI(),
+			APIri:            statusableURI,
 			APObjectModel:    nil,
 			GTSModel:         nil,
 			ReceivingAccount: receivingAccount,
@@ -209,34 +244,58 @@ func (f *federatingDB) createNote(ctx context.Context, note vocab.ActivityStream
 		return nil
 	}
 
-	// if we reach this point, we know it's not a forwarded status, so proceed with processing it as normal
-
-	status, err := f.typeConverter.ASStatusToStatus(ctx, note)
+	// This is a non-forwarded status we can trust the requester on,
+	// convert this provided statusable data to a useable gtsmodel status.
+	status, err = f.typeConverter.ASStatusToStatus(ctx, statusable)
 	if err != nil {
-		return fmt.Errorf("createNote: error converting note to status: %s", err)
+		return gtserror.Newf("error converting statusable to status: %w", err)
 	}
 
-	// id the status based on the time it was created
-	statusID, err := id.NewULIDFromTime(status.CreatedAt)
+	// Check whether we should accept this new status.
+	accept, err := f.shouldAcceptStatusable(ctx,
+		receivingAccount,
+		requestingAccount,
+		status,
+	)
+	if err != nil {
+		return gtserror.Newf("error checking status acceptibility: %w", err)
+	}
+
+	if !accept {
+		// This is a status sent with no relation to receiver, i.e.
+		// - receiving account does not follow requesting account
+		// - received status does not mention receiving account
+		//
+		// We just pretend that all is fine (dog with cuppa, flames everywhere)
+		log.Trace(ctx, "status failed acceptability check")
+		return nil
+	}
+
+	// ID the new status based on the time it was created.
+	status.ID, err = id.NewULIDFromTime(status.CreatedAt)
 	if err != nil {
 		return err
 	}
-	status.ID = statusID
 
+	// Put this newly parsed status in the database.
 	if err := f.state.DB.PutStatus(ctx, status); err != nil {
 		if errors.Is(err, db.ErrAlreadyExists) {
-			// the status already exists in the database, which means we've already handled everything else,
-			// so we can just return nil here and be done with it.
+			// The status already exists in the database, which
+			// means we've already processed it and some race
+			// condition means we didn't catch it yet. We can
+			// just return nil here and be done with it.
 			return nil
 		}
-		// an actual error has happened
-		return fmt.Errorf("createNote: database error inserting status: %s", err)
+		return gtserror.Newf("db error inserting status: %w", err)
 	}
 
+	// Do the rest of the processing asynchronously. The processor
+	// will handle inserting/updating + further dereferencing the status.
 	f.state.Workers.EnqueueFederator(ctx, messages.FromFederator{
 		APObjectType:     ap.ObjectNote,
 		APActivityType:   ap.ActivityCreate,
-		APObjectModel:    note,
+		APIri:            nil,
+		APObjectModel:    statusable,
 		GTSModel:         status,
 		ReceivingAccount: receivingAccount,
 	})
@@ -244,6 +303,26 @@ func (f *federatingDB) createNote(ctx context.Context, note vocab.ActivityStream
 	return nil
 }
 
+func (f *federatingDB) shouldAcceptStatusable(ctx context.Context, receiver *gtsmodel.Account, requester *gtsmodel.Account, status *gtsmodel.Status) (bool, error) {
+	// Check whether status mentions the receiver,
+	// this is the quickest check so perform it first.
+	for _, mention := range status.Mentions {
+		if mention.TargetAccountURI == receiver.URI {
+			return true, nil
+		}
+	}
+
+	// Check whether receiving account follows the requesting account.
+	follows, err := f.state.DB.IsFollowing(ctx, receiver.ID, requester.ID)
+	if err != nil {
+		return false, gtserror.Newf("error checking follow status: %w", err)
+	}
+
+	// Status will only be acceptable
+	// if receiver follows requester.
+	return follows, nil
+}
+
 /*
 	FOLLOW HANDLERS
 */

internal/federation/federatingdb/create_test.go

@@ -54,7 +54,7 @@ func (suite *CreateTestSuite) TestCreateNote() {
 
 	// status should have some expected values
 	suite.Equal(requestingAccount.ID, status.AccountID)
-	suite.Equal("hey zork here's a new private note for you", status.Content)
+	suite.Equal("@the_mighty_zork@localhost:8080 hey zork here's a new private note for you", status.Content)
 
 	// status should be in the database
 	_, err = suite.db.GetStatusByID(context.Background(), status.ID)

internal/processing/fromfederator.go

@@ -108,20 +108,23 @@ func (p *Processor) ProcessFromFederator(ctx context.Context, federatorMsg messa
 
 // processCreateStatusFromFederator handles Activity Create and Object Note.
 func (p *Processor) processCreateStatusFromFederator(ctx context.Context, federatorMsg messages.FromFederator) error {
-	// Check the federatorMsg for either an already
-	// dereferenced and converted status pinned to
-	// the message, or an AP IRI that we need to deref.
 	var (
 		status *gtsmodel.Status
 		err    error
+
+		// Check the federatorMsg for either an already dereferenced
+		// and converted status pinned to the message, or a forwarded
+		// AP IRI that we still need to deref.
+		forwarded = (federatorMsg.GTSModel == nil)
 	)
 
-	if federatorMsg.GTSModel != nil {
-		// Model is set, use that.
-		status, err = p.statusFromGTSModel(ctx, federatorMsg)
-	} else {
-		// Model is not set, use IRI.
+	if forwarded {
+		// Model was not set, deref with IRI.
+		// This will also cause the status to be inserted into the db.
 		status, err = p.statusFromAPIRI(ctx, federatorMsg)
+	} else {
+		// Model is set, ensure we have the most up-to-date model.
+		status, err = p.statusFromGTSModel(ctx, federatorMsg)
 	}
 
 	if err != nil {

internal/typeutils/astointernal.go

@@ -283,7 +283,7 @@ func (c *converter) ASStatusToStatus(ctx context.Context, statusable ap.Statusab
 	//
 	// Hashtags for later dereferencing.
 	if hashtags, err := ap.ExtractHashtags(statusable); err != nil {
-		l.Infof("error extracting hashtags: %q", err)
+		l.Warnf("error extracting hashtags: %v", err)
 	} else {
 		status.Tags = hashtags
 	}
@@ -292,7 +292,7 @@ func (c *converter) ASStatusToStatus(ctx context.Context, statusable ap.Statusab
 	//
 	// Custom emojis for later dereferencing.
 	if emojis, err := ap.ExtractEmojis(statusable); err != nil {
-		l.Infof("error extracting emojis: %q", err)
+		l.Warnf("error extracting emojis: %v", err)
 	} else {
 		status.Emojis = emojis
 	}
@@ -301,7 +301,7 @@ func (c *converter) ASStatusToStatus(ctx context.Context, statusable ap.Statusab
 	//
 	// Mentions of other accounts for later dereferencing.
 	if mentions, err := ap.ExtractMentions(statusable); err != nil {
-		l.Infof("error extracting mentions: %q", err)
+		l.Warnf("error extracting mentions: %v", err)
 	} else {
 		status.Mentions = mentions
 	}
@@ -322,7 +322,7 @@ func (c *converter) ASStatusToStatus(ctx context.Context, statusable ap.Statusab
 	// db defaults, will fall back to now if not set.
 	published, err := ap.ExtractPublished(statusable)
 	if err != nil {
-		l.Infof("error extracting published: %q", err)
+		l.Warnf("error extracting published: %v", err)
 	} else {
 		status.CreatedAt = published
 		status.UpdatedAt = published

test/run-postgres.sh

@@ -2,6 +2,11 @@
 
 set -e
 
+# Ensure test args are set.
+ARGS=${@}; [ -z "$ARGS" ] && \
+ARGS='./...'
+
+# Database config.
 DB_NAME='postgres'
 DB_USER='postgres'
 DB_PASS='postgres'
@@ -34,4 +39,4 @@ GTS_DB_PORT=${DB_PORT} \
 GTS_DB_USER=${DB_USER} \
 GTS_DB_PASSWORD=${DB_PASS} \
 GTS_DB_DATABASE=${DB_NAME} \
-go test ./... -p 1 ${@}
+go test ./... -p 1 ${ARGS}

test/run-sqlite.sh

@@ -2,6 +2,11 @@
 
 set -e
 
+# Ensure test args are set.
+ARGS=${@}; [ -z "$ARGS" ] && \
+ARGS='./...'
+
+# Run the SQLite tests.
 GTS_DB_TYPE=sqlite \
 GTS_DB_ADDRESS=':memory:' \
-go test ./... ${@}
+go test ${ARGS}

testrig/testmodels.go

@@ -2055,13 +2055,16 @@ func NewTestActivities(accounts map[string]*gtsmodel.Account) map[string]Activit
 		URLMustParse("http://fossbros-anonymous.io/users/foss_satan/statuses/5424b153-4553-4f30-9358-7b92f7cd42f6"),
 		URLMustParse("http://fossbros-anonymous.io/@foss_satan/5424b153-4553-4f30-9358-7b92f7cd42f6"),
 		TimeMustParse("2022-07-13T12:13:12+02:00"),
-		"hey zork here's a new private note for you",
+		"@the_mighty_zork@localhost:8080 hey zork here's a new private note for you",
 		"new note for zork",
 		URLMustParse("http://fossbros-anonymous.io/users/foss_satan"),
 		[]*url.URL{URLMustParse("http://localhost:8080/users/the_mighty_zork")},
 		nil,
 		true,
-		[]vocab.ActivityStreamsMention{},
+		[]vocab.ActivityStreamsMention{newAPMention(
+			URLMustParse("http://localhost:8080/users/the_mighty_zork"),
+			"@the_mighty_zork@localhost:8080",
+		)},
 		[]vocab.TootHashtag{},
 		nil,
 	)