From 3920bc87d1398893feda21f32e4f59129b2cc9cd Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Tue, 8 Aug 2023 12:26:34 +0100 Subject: [PATCH] [bugfix] don't accept unrelated statuses (#2078) Co-authored-by: Daenney Co-authored-by: tsmethurst --- internal/federation/federatingdb/create.go | 233 ++++++++++++------ .../federation/federatingdb/create_test.go | 2 +- internal/processing/fromfederator.go | 19 +- internal/typeutils/astointernal.go | 8 +- test/run-postgres.sh | 7 +- test/run-sqlite.sh | 7 +- testrig/testmodels.go | 7 +- 7 files changed, 189 insertions(+), 94 deletions(-) diff --git a/internal/federation/federatingdb/create.go b/internal/federation/federatingdb/create.go index 3650f8c3d..0075aa97a 100644 --- a/internal/federation/federatingdb/create.go +++ b/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() - if object == nil { - return errors.New("Create had no Object") + // Create must have an Object. + objectProp := create.GetActivityStreamsObject() + if objectProp == nil { + return gtserror.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") + // 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 { + // 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") - } - - // 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 + 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) } - 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 + // 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 + } + + // 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 { - 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 - return nil - } - // pass the note iri into the processor and have it do the dereferencing instead of doing it here + // 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 */ diff --git a/internal/federation/federatingdb/create_test.go b/internal/federation/federatingdb/create_test.go index fa7aca0a2..6c18f5bd0 100644 --- a/internal/federation/federatingdb/create_test.go +++ b/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) diff --git a/internal/processing/fromfederator.go b/internal/processing/fromfederator.go index abe292cae..2790d31ee 100644 --- a/internal/processing/fromfederator.go +++ b/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 { diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go index ea77bb65c..7b1ba0396 100644 --- a/internal/typeutils/astointernal.go +++ b/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 diff --git a/test/run-postgres.sh b/test/run-postgres.sh index 54e4970ed..029a72793 100755 --- a/test/run-postgres.sh +++ b/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 ${@} \ No newline at end of file +go test ./... -p 1 ${ARGS} \ No newline at end of file diff --git a/test/run-sqlite.sh b/test/run-sqlite.sh index fb5502432..666f88c28 100755 --- a/test/run-sqlite.sh +++ b/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 ./... ${@} \ No newline at end of file +go test ${ARGS} \ No newline at end of file diff --git a/testrig/testmodels.go b/testrig/testmodels.go index fd26b514e..f5f63f6fa 100644 --- a/testrig/testmodels.go +++ b/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, )