[bugfix] Relax `Mention` parsing, allowing either href or name (#2320)

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 &gtsmodel.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
+			}
 		}
 	}