From 51d0a0bba5cc7c5c170b2137f169d65e49966c84 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Tue, 31 Oct 2023 12:05:17 +0100 Subject: [PATCH] [bugfix] Relax `Mention` parsing, allowing either href or name (#2320) --- docs/federation/federating_with_gotosocial.md | 55 +++++++++ internal/ap/extract.go | 27 +++-- internal/ap/extractmentions_test.go | 99 +++++++++++++++- internal/federation/dereferencing/account.go | 38 ++++-- internal/federation/dereferencing/status.go | 108 +++++++++++++++--- internal/federation/federatingdb/create.go | 30 ++++- 6 files changed, 317 insertions(+), 40 deletions(-) diff --git a/docs/federation/federating_with_gotosocial.md b/docs/federation/federating_with_gotosocial.md index f5796b58e..a977ee0e9 100644 --- a/docs/federation/federating_with_gotosocial.md +++ b/docs/federation/federating_with_gotosocial.md @@ -427,3 +427,58 @@ With just one tag, the `tag` property will be an object rather than an array, wh The `href` URL provided by GoToSocial in outgoing tags points to a web URL that serves `text/html`. GoToSocial makes no guarantees whatsoever about what the content of the given `text/html` will be, and remote servers should not interpret the URL as a canonical ActivityPub ID/URI property. The `href` URL is provided merely as an endpoint which *might* contain more information about the given hashtag. + +## Mentions + +GoToSocial users can Mention other users in their posts, using the common `@[username]@[domain]` format. For example, if a GoToSocial user wanted to mention user `someone` on instance `example.org`, they could do this by including `@someone@example.org` in their post somewhere. + +!!! info "Mentions and activity addressing" + + Mentions are not just aesthetic, they affect addressing of Activities as well. + + If a GoToSocial user explicitly mentions another user in a Note, the URI of that user will always be included in the `To` or `Cc` property of the Note's Create activity. + + If the Note is direct (ie., not `To` public or followers), each mention target URI will be in the `To` property of the wrapping Activity + + In all other cases, mentions will be included in the `Cc` property of the wrapping Activity. + +### Outgoing + +When a GoToSocial user Mentions another account, the Mention is included in outgoing federated messages as an entry in the `tag` property. + +For example, say a user on a GoToSocial instance Mentions `@someone@example.org`, the `tag` property of the outgoing Note might look like the following: + +```json +"tag": { + "href": "http://example.org/users/someone", + "name": "@someone@example.org", + "type": "Mention" +} +``` + +If a user Mentions a local user they share an instance with, the full `name` of the local user will still be included. + +For example, a GoToSocial user on domain `some.gotosocial.instance` mentions another user on the same instance called `user2`. They also mention `@someone@example.org` as above. The `tag` property of the outgoing Note would look like the following: + +```json +"tag": [ + { + "href": "http://example.org/users/someone", + "name": "@someone@example.org", + "type": "Mention" + }, + { + "href": "http://some.gotosocial.instance/users/user2", + "name": "@user2@some.gotosocial.instance", + "type": "Mention" + } +] +``` + +For the convenience of remote servers, GoToSocial will always provide both the `href` and the `name` properties on outgoing Mentions. The `href` property used by GoToSocial will always be the ActivityPub ID/URI of the target account, not the web URL. + +### Incoming + +GoToSocial tries to parse incoming Mentions in the same way it sends them out: as a `Mention` type entry in the `tag` property. However, when parsing incoming Mentions it's a bit more relaxed with regards to which properties must be set. + +GoToSocial will prefer the `href` property, which can be either the ActivityPub ID/URI or the web URL of the target; if `href` is not present, it will fall back to using the `name` property. If neither property is present, the mention will be considered invalid and discarded. diff --git a/internal/ap/extract.go b/internal/ap/extract.go index 6d224e9a8..74c4497b3 100644 --- a/internal/ap/extract.go +++ b/internal/ap/extract.go @@ -32,7 +32,6 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/text" - "github.com/superseriousbusiness/gotosocial/internal/util" ) // ExtractObjects will extract object vocab.Types from given implementing interface. @@ -841,27 +840,27 @@ func ExtractMentions(i WithTag) ([]*gtsmodel.Mention, error) { // ExtractMention extracts a minimal gtsmodel.Mention from a Mentionable. func ExtractMention(i Mentionable) (*gtsmodel.Mention, error) { + // See if a name has been set in the + // format `@someone@example.org`. nameString := ExtractName(i) - if nameString == "" { - return nil, gtserror.New("name prop empty") - } - - // Ensure namestring is valid so we - // can handle it properly later on. - if _, _, err := util.ExtractNamestringParts(nameString); err != nil { - return nil, err - } // The href prop should be the AP URI - // of the target account. + // of the target account; it could also + // be the URL, but we'll check this later. + var href string hrefProp := i.GetActivityStreamsHref() - if hrefProp == nil || !hrefProp.IsIRI() { - return nil, gtserror.New("no href prop") + if hrefProp != nil && hrefProp.IsIRI() { + href = hrefProp.GetIRI().String() + } + + // One of nameString and hrefProp must be set. + if nameString == "" && href == "" { + return nil, gtserror.Newf("neither Name nor Href were set") } return >smodel.Mention{ NameString: nameString, - TargetAccountURI: hrefProp.GetIRI().String(), + TargetAccountURI: href, }, nil } diff --git a/internal/ap/extractmentions_test.go b/internal/ap/extractmentions_test.go index fbfee34f5..e49347e5c 100644 --- a/internal/ap/extractmentions_test.go +++ b/internal/ap/extractmentions_test.go @@ -21,14 +21,16 @@ import ( "testing" "github.com/stretchr/testify/suite" + "github.com/superseriousbusiness/activity/streams" "github.com/superseriousbusiness/gotosocial/internal/ap" + "github.com/superseriousbusiness/gotosocial/testrig" ) type ExtractMentionsTestSuite struct { APTestSuite } -func (suite *ExtractMentionsTestSuite) TestExtractMentions() { +func (suite *ExtractMentionsTestSuite) TestExtractMentionsFromNote() { note := suite.noteWithMentions1 mentions, err := ap.ExtractMentions(note) @@ -44,6 +46,101 @@ func (suite *ExtractMentionsTestSuite) TestExtractMentions() { suite.Equal("https://gts.superseriousbusiness.org/users/f0x", m2.TargetAccountURI) } +func (suite *ExtractMentionsTestSuite) TestExtractMentions() { + newMention := func(nameString string, href string) ap.Mentionable { + mention := streams.NewActivityStreamsMention() + + if nameString != "" { + nameProp := streams.NewActivityStreamsNameProperty() + nameProp.AppendXMLSchemaString(nameString) + mention.SetActivityStreamsName(nameProp) + } + + if href != "" { + hrefProp := streams.NewActivityStreamsHrefProperty() + hrefProp.SetIRI(testrig.URLMustParse(href)) + mention.SetActivityStreamsHref(hrefProp) + } + + return mention + } + + type test struct { + nameString string + href string + expectedNameString string + expectedHref string + expectedErr string + } + + for i, t := range []test{ + { + // Mention with both Name and Href set, should be fine. + nameString: "@someone@example.org", + href: "https://example.org/@someone", + expectedNameString: "@someone@example.org", + expectedHref: "https://example.org/@someone", + expectedErr: "", + }, + { + // Mention with just Href set, should be fine. + nameString: "", + href: "https://example.org/@someone", + expectedNameString: "", + expectedHref: "https://example.org/@someone", + expectedErr: "", + }, + { + // Mention with just Name set, should be fine. + nameString: "@someone@example.org", + href: "", + expectedNameString: "@someone@example.org", + expectedHref: "", + expectedErr: "", + }, + { + // Mention with nothing set, not fine! + nameString: "", + href: "", + expectedNameString: "", + expectedHref: "", + expectedErr: "ExtractMention: neither Name nor Href were set", + }, + } { + apMention := newMention(t.nameString, t.href) + mention, err := ap.ExtractMention(apMention) + + if err != nil { + if errString := err.Error(); errString != t.expectedErr { + suite.Fail("", + "test %d expected error %s, got %s", + i+1, t.expectedErr, errString, + ) + } + continue + } else if t.expectedErr != "" { + suite.Fail("", + "test %d expected error %s, got no error", + i+1, t.expectedErr, + ) + } + + if mention.NameString != t.expectedNameString { + suite.Fail("", + "test %d expected nameString %s, got %s", + i+1, t.expectedNameString, mention.NameString, + ) + } + + if mention.TargetAccountURI != t.expectedHref { + suite.Fail("", + "test %d expected href %s, got %s", + i+1, t.expectedHref, mention.TargetAccountURI, + ) + } + } +} + func TestExtractMentionsTestSuite(t *testing.T) { suite.Run(t, &ExtractMentionsTestSuite{}) } diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go index f94dbaffa..670c8e2c8 100644 --- a/internal/federation/dereferencing/account.go +++ b/internal/federation/dereferencing/account.go @@ -163,6 +163,37 @@ func (d *Dereferencer) getAccountByURI(ctx context.Context, requestUser string, // or a remote model whose last_fetched date is beyond a certain interval, the account will be dereferenced. In the case of dereferencing, some low-priority // account information may be enqueued for asynchronous fetching, e.g. featured account statuses (pins). An ActivityPub object indicates the account was dereferenced. func (d *Dereferencer) GetAccountByUsernameDomain(ctx context.Context, requestUser string, username string, domain string) (*gtsmodel.Account, ap.Accountable, error) { + account, apubAcc, err := d.getAccountByUsernameDomain( + ctx, + requestUser, + username, + domain, + ) + if err != nil { + return nil, nil, err + } + + if apubAcc != nil { + // This account was updated, enqueue re-dereference featured posts. + d.state.Workers.Federator.MustEnqueueCtx(ctx, func(ctx context.Context) { + if err := d.dereferenceAccountFeatured(ctx, requestUser, account); err != nil { + log.Errorf(ctx, "error fetching account featured collection: %v", err) + } + }) + } + + return account, apubAcc, nil +} + +// getAccountByUsernameDomain is a package internal form +// of .GetAccountByUsernameDomain() that doesn't bother +// dereferencing featured posts. +func (d *Dereferencer) getAccountByUsernameDomain( + ctx context.Context, + requestUser string, + username string, + domain string, +) (*gtsmodel.Account, ap.Accountable, error) { if domain == config.GetHost() || domain == config.GetAccountDomain() { // We do local lookups using an empty domain, // else it will fail the db search below. @@ -196,13 +227,6 @@ func (d *Dereferencer) GetAccountByUsernameDomain(ctx context.Context, requestUs return nil, nil, err } - // This account was updated, enqueue dereference featured posts. - d.state.Workers.Federator.MustEnqueueCtx(ctx, func(ctx context.Context) { - if err := d.dereferenceAccountFeatured(ctx, requestUser, account); err != nil { - log.Errorf(ctx, "error fetching account featured collection: %v", err) - } - }) - return account, apubAcc, nil } diff --git a/internal/federation/dereferencing/status.go b/internal/federation/dereferencing/status.go index 89b3088c8..adc73e843 100644 --- a/internal/federation/dereferencing/status.go +++ b/internal/federation/dereferencing/status.go @@ -36,6 +36,7 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/transport" + "github.com/superseriousbusiness/gotosocial/internal/util" ) // statusUpToDate returns whether the given status model is both updateable @@ -342,35 +343,110 @@ func (d *Dereferencer) enrichStatus( return latestStatus, apubStatus, nil } -func (d *Dereferencer) fetchStatusMentions(ctx context.Context, requestUser string, existing, status *gtsmodel.Status) error { - // Allocate new slice to take the yet-to-be created mention IDs. - status.MentionIDs = make([]string, len(status.Mentions)) - - for i := range status.Mentions { - mention := status.Mentions[i] - - // Look for existing mention with target account URI first. - existing, ok := existing.GetMentionByTargetURI(mention.TargetAccountURI) - if ok && existing.ID != "" { - status.Mentions[i] = existing - status.MentionIDs[i] = existing.ID - continue +// populateMentionTarget tries to populate the given +// mention with the correct TargetAccount and (if not +// yet set) TargetAccountURI, returning the populated +// mention. +// +// Will check on the existing status if the mention +// is already there and populated; if so, existing +// mention will be returned along with `true`. +// +// Otherwise, this function will try to parse first +// the Href of the mention, and then the namestring, +// to see who it targets, and go fetch that account. +func (d *Dereferencer) populateMentionTarget( + ctx context.Context, + mention *gtsmodel.Mention, + requestUser string, + existing, status *gtsmodel.Status, +) ( + *gtsmodel.Mention, + bool, // True if mention already exists in the DB. + error, +) { + // Mentions can be created using Name or Href. + // Prefer Href (TargetAccountURI), fall back to Name. + if mention.TargetAccountURI != "" { + // Look for existing mention with this URI. + // If we already have it we can return early. + existingMention, ok := existing.GetMentionByTargetURI(mention.TargetAccountURI) + if ok && existingMention.ID != "" { + return existingMention, true, nil } // Ensure that mention account URI is parseable. accountURI, err := url.Parse(mention.TargetAccountURI) if err != nil { - log.Errorf(ctx, "invalid account uri %q: %v", mention.TargetAccountURI, err) - continue + err = gtserror.Newf("invalid account uri %q: %w", mention.TargetAccountURI, err) + return nil, false, err } // Ensure we have the account of the mention target dereferenced. mention.TargetAccount, _, err = d.getAccountByURI(ctx, requestUser, accountURI) if err != nil { - log.Errorf(ctx, "failed to dereference account %s: %v", accountURI, err) + err = gtserror.Newf("failed to dereference account %s: %w", accountURI, err) + return nil, false, err + } + } else { + // Href wasn't set. Find the target account using namestring. + username, domain, err := util.ExtractNamestringParts(mention.NameString) + if err != nil { + err = gtserror.Newf("failed to parse namestring %s: %w", mention.NameString, err) + return nil, false, err + } + + mention.TargetAccount, _, err = d.getAccountByUsernameDomain(ctx, requestUser, username, domain) + if err != nil { + err = gtserror.Newf("failed to dereference account %s: %w", mention.NameString, err) + return nil, false, err + } + + // Look for existing mention with this URI. + mention.TargetAccountURI = mention.TargetAccount.URI + existingMention, ok := existing.GetMentionByTargetURI(mention.TargetAccountURI) + if ok && existingMention.ID != "" { + return existingMention, true, nil + } + } + + // At this point, mention.TargetAccountURI + // and mention.TargetAccount must be set. + return mention, false, nil +} + +func (d *Dereferencer) fetchStatusMentions(ctx context.Context, requestUser string, existing, status *gtsmodel.Status) error { + // Allocate new slice to take the yet-to-be created mention IDs. + status.MentionIDs = make([]string, len(status.Mentions)) + + for i := range status.Mentions { + var ( + mention = status.Mentions[i] + alreadyExists bool + err error + ) + + mention, alreadyExists, err = d.populateMentionTarget( + ctx, + mention, + requestUser, + existing, + status, + ) + if err != nil { + log.Errorf(ctx, "failed to derive mention: %v", err) continue } + if alreadyExists { + // This mention was already attached + // to the status, use it and continue. + status.Mentions[i] = mention + status.MentionIDs[i] = mention.ID + continue + } + + // This mention didn't exist yet. // Generate new ID according to status creation. // TODO: update this to use "edited_at" when we add // support for edited status revision history. diff --git a/internal/federation/federatingdb/create.go b/internal/federation/federatingdb/create.go index 6cb230589..14e846b15 100644 --- a/internal/federation/federatingdb/create.go +++ b/internal/federation/federatingdb/create.go @@ -21,17 +21,20 @@ import ( "context" "errors" "fmt" + "strings" "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/config" "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" "github.com/superseriousbusiness/gotosocial/internal/messages" + "github.com/superseriousbusiness/gotosocial/internal/util" ) // Create adds a new entry to the database which must be able to be @@ -288,11 +291,34 @@ func (f *federatingDB) createStatusable( } func (f *federatingDB) shouldAcceptStatusable(ctx context.Context, receiver *gtsmodel.Account, requester *gtsmodel.Account, status *gtsmodel.Status) (bool, error) { + host := config.GetHost() + accountDomain := config.GetAccountDomain() + // Check whether status mentions the receiver, // this is the quickest check so perform it first. + // Prefer checking using mention Href, fall back to Name. for _, mention := range status.Mentions { - if mention.TargetAccountURI == receiver.URI { - return true, nil + targetURI := mention.TargetAccountURI + nameString := mention.NameString + + if targetURI != "" { + if targetURI == receiver.URI || targetURI == receiver.URL { + // Target URI or URL match; + // receiver is mentioned. + return true, nil + } + } else if nameString != "" { + username, domain, err := util.ExtractNamestringParts(nameString) + if err != nil { + return false, gtserror.Newf("error checking if mentioned: %w", err) + } + + if (domain == host || domain == accountDomain) && + strings.EqualFold(username, receiver.Username) { + // Username + domain match; + // receiver is mentioned. + return true, nil + } } }