[bugfix] Federate status delete using just the URI (#1584)

internal/processing/fromclientapi.go

@@ -467,38 +467,16 @@ func (p *Processor) federateStatusDelete(ctx context.Context, status *gtsmodel.S
 		return nil
 	}
 
-	asStatus, err := p.tc.StatusToAS(ctx, status)
+	delete, err := p.tc.StatusToASDelete(ctx, status)
 	if err != nil {
-		return fmt.Errorf("federateStatusDelete: error converting status to as format: %s", err)
+		return fmt.Errorf("federateStatusDelete: error creating Delete: %w", err)
 	}
 
 	outboxIRI, err := url.Parse(status.Account.OutboxURI)
 	if err != nil {
-		return fmt.Errorf("federateStatusDelete: error parsing outboxURI %s: %s", status.Account.OutboxURI, err)
+		return fmt.Errorf("federateStatusDelete: error parsing outboxURI %s: %w", status.Account.OutboxURI, err)
 	}
 
-	actorIRI, err := url.Parse(status.Account.URI)
-	if err != nil {
-		return fmt.Errorf("federateStatusDelete: error parsing actorIRI %s: %s", status.Account.URI, err)
-	}
-
-	// create a delete and set the appropriate actor on it
-	delete := streams.NewActivityStreamsDelete()
-
-	// set the actor for the delete
-	deleteActor := streams.NewActivityStreamsActorProperty()
-	deleteActor.AppendIRI(actorIRI)
-	delete.SetActivityStreamsActor(deleteActor)
-
-	// Set the status as the 'object' property.
-	deleteObject := streams.NewActivityStreamsObjectProperty()
-	deleteObject.AppendActivityStreamsNote(asStatus)
-	delete.SetActivityStreamsObject(deleteObject)
-
-	// set the to and cc as the original to/cc of the original status
-	delete.SetActivityStreamsTo(asStatus.GetActivityStreamsTo())
-	delete.SetActivityStreamsCc(asStatus.GetActivityStreamsCc())
-
 	_, err = p.federator.FederatingActor().Send(ctx, outboxIRI, delete)
 	return err
 }

internal/typeutils/converter.go

@@ -148,6 +148,9 @@ type TypeConverter interface {
 	AccountToASMinimal(ctx context.Context, a *gtsmodel.Account) (vocab.ActivityStreamsPerson, error)
 	// StatusToAS converts a gts model status into an activity streams note, suitable for federation
 	StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.ActivityStreamsNote, error)
+	// StatusToASDelete converts a gts model status into a Delete of that status, using just the
+	// URI of the status as object, and addressing the Delete appropriately.
+	StatusToASDelete(ctx context.Context, status *gtsmodel.Status) (vocab.ActivityStreamsDelete, error)
 	// FollowToASFollow converts a gts model Follow into an activity streams Follow, suitable for federation
 	FollowToAS(ctx context.Context, f *gtsmodel.Follow, originAccount *gtsmodel.Account, targetAccount *gtsmodel.Account) (vocab.ActivityStreamsFollow, error)
 	// MentionToAS converts a gts model mention into an activity streams Mention, suitable for federation

internal/typeutils/converter_test.go

@@ -477,6 +477,7 @@ type TypeUtilsTestSuite struct {
 	testPeople      map[string]vocab.ActivityStreamsPerson
 	testEmojis      map[string]*gtsmodel.Emoji
 	testReports     map[string]*gtsmodel.Report
+	testMentions    map[string]*gtsmodel.Mention
 
 	typeconverter typeutils.TypeConverter
 }
@@ -495,6 +496,7 @@ func (suite *TypeUtilsTestSuite) SetupSuite() {
 	suite.testPeople = testrig.NewTestFediPeople()
 	suite.testEmojis = testrig.NewTestEmojis()
 	suite.testReports = testrig.NewTestReports()
+	suite.testMentions = testrig.NewTestMentions()
 	suite.typeconverter = typeutils.NewConverter(suite.db)
 }
 

internal/typeutils/internaltoas.go

@@ -22,6 +22,7 @@ import (
 	"context"
 	"crypto/x509"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"net/url"
 
@@ -29,6 +30,7 @@ import (
 	"github.com/superseriousbusiness/activity/streams"
 	"github.com/superseriousbusiness/activity/streams/vocab"
 	"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/log"
@@ -414,18 +416,10 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 	status.SetActivityStreamsSummary(statusSummaryProp)
 
 	// inReplyTo
-	if s.InReplyToID != "" {
-		// fetch the replied status if we don't have it on hand already
-		if s.InReplyTo == nil {
-			rs, err := c.db.GetStatusByID(ctx, s.InReplyToID)
-			if err != nil {
-				return nil, fmt.Errorf("StatusToAS: error getting replied to status %s: %s", s.InReplyToID, err)
-			}
-			s.InReplyTo = rs
-		}
-		rURI, err := url.Parse(s.InReplyTo.URI)
+	if s.InReplyToURI != "" {
+		rURI, err := url.Parse(s.InReplyToURI)
 		if err != nil {
-			return nil, fmt.Errorf("StatusToAS: error parsing url %s: %s", s.InReplyTo.URI, err)
+			return nil, fmt.Errorf("StatusToAS: error parsing url %s: %s", s.InReplyToURI, err)
 		}
 
 		inReplyToProp := streams.NewActivityStreamsInReplyToProperty()
@@ -465,13 +459,9 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 	// tag -- mentions
 	mentions := s.Mentions
 	if len(s.MentionIDs) > len(mentions) {
-		mentions = []*gtsmodel.Mention{}
-		for _, mentionID := range s.MentionIDs {
-			mention, err := c.db.GetMention(ctx, mentionID)
-			if err != nil {
-				return nil, fmt.Errorf("StatusToAS: error getting mention %s from database: %s", mentionID, err)
-			}
-			mentions = append(mentions, mention)
+		mentions, err = c.db.GetMentions(ctx, s.MentionIDs)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToAS: error getting mentions: %w", err)
 		}
 	}
 	for _, m := range mentions {
@@ -524,7 +514,7 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 	switch s.Visibility {
 	case gtsmodel.VisibilityDirect:
 		// if DIRECT, then only mentioned users should be added to TO, and nothing to CC
-		for _, m := range s.Mentions {
+		for _, m := range mentions {
 			iri, err := url.Parse(m.TargetAccount.URI)
 			if err != nil {
 				return nil, fmt.Errorf("StatusToAS: error parsing uri %s: %s", m.TargetAccount.URI, err)
@@ -536,7 +526,7 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 	case gtsmodel.VisibilityFollowersOnly:
 		// if FOLLOWERS ONLY then we want to add followers to TO, and mentions to CC
 		toProp.AppendIRI(authorFollowersURI)
-		for _, m := range s.Mentions {
+		for _, m := range mentions {
 			iri, err := url.Parse(m.TargetAccount.URI)
 			if err != nil {
 				return nil, fmt.Errorf("StatusToAS: error parsing uri %s: %s", m.TargetAccount.URI, err)
@@ -547,7 +537,7 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 		// if UNLOCKED, we want to add followers to TO, and public and mentions to CC
 		toProp.AppendIRI(authorFollowersURI)
 		ccProp.AppendIRI(publicURI)
-		for _, m := range s.Mentions {
+		for _, m := range mentions {
 			iri, err := url.Parse(m.TargetAccount.URI)
 			if err != nil {
 				return nil, fmt.Errorf("StatusToAS: error parsing uri %s: %s", m.TargetAccount.URI, err)
@@ -558,7 +548,7 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 		// if PUBLIC, we want to add public to TO, and followers and mentions to CC
 		toProp.AppendIRI(publicURI)
 		ccProp.AppendIRI(authorFollowersURI)
-		for _, m := range s.Mentions {
+		for _, m := range mentions {
 			iri, err := url.Parse(m.TargetAccount.URI)
 			if err != nil {
 				return nil, fmt.Errorf("StatusToAS: error parsing uri %s: %s", m.TargetAccount.URI, err)
@@ -617,6 +607,141 @@ func (c *converter) StatusToAS(ctx context.Context, s *gtsmodel.Status) (vocab.A
 	return status, nil
 }
 
+func (c *converter) StatusToASDelete(ctx context.Context, s *gtsmodel.Status) (vocab.ActivityStreamsDelete, error) {
+	// Parse / fetch some information
+	// we need to create the Delete.
+
+	if s.Account == nil {
+		var err error
+		s.Account, err = c.db.GetAccountByID(ctx, s.AccountID)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToASDelete: error retrieving author account from db: %w", err)
+		}
+	}
+
+	actorIRI, err := url.Parse(s.AccountURI)
+	if err != nil {
+		return nil, fmt.Errorf("StatusToASDelete: error parsing actorIRI %s: %w", s.AccountURI, err)
+	}
+
+	statusIRI, err := url.Parse(s.URI)
+	if err != nil {
+		return nil, fmt.Errorf("StatusToASDelete: error parsing statusIRI %s: %w", s.URI, err)
+	}
+
+	// Create a Delete.
+	delete := streams.NewActivityStreamsDelete()
+
+	// Set appropriate actor for the Delete.
+	deleteActor := streams.NewActivityStreamsActorProperty()
+	deleteActor.AppendIRI(actorIRI)
+	delete.SetActivityStreamsActor(deleteActor)
+
+	// Set the status IRI as the 'object' property.
+	// We should avoid serializing the whole status
+	// when doing a delete because it's wasteful and
+	// could accidentally leak the now-deleted status.
+	deleteObject := streams.NewActivityStreamsObjectProperty()
+	deleteObject.AppendIRI(statusIRI)
+	delete.SetActivityStreamsObject(deleteObject)
+
+	// Address the Delete appropriately.
+	toProp := streams.NewActivityStreamsToProperty()
+	ccProp := streams.NewActivityStreamsCcProperty()
+
+	// Unless the status was a direct message, we can
+	// address the Delete To the ActivityPub Public URI.
+	// This ensures that the Delete will have as wide an
+	// audience as possible.
+	//
+	// Because we're using just the status URI, not the
+	// whole status, it won't leak any sensitive info.
+	// At worst, a remote instance becomes aware of the
+	// URI for a status which is now deleted anyway.
+	if s.Visibility != gtsmodel.VisibilityDirect {
+		publicURI, err := url.Parse(pub.PublicActivityPubIRI)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToASDelete: error parsing url %s: %w", pub.PublicActivityPubIRI, err)
+		}
+		toProp.AppendIRI(publicURI)
+
+		actorFollowersURI, err := url.Parse(s.Account.FollowersURI)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToASDelete: error parsing url %s: %w", s.Account.FollowersURI, err)
+		}
+		ccProp.AppendIRI(actorFollowersURI)
+	}
+
+	// Always include the replied-to account and any
+	// mentioned accounts as addressees as well.
+	//
+	// Worst case scenario here is that a replied account
+	// who wasn't mentioned (and perhaps didn't see the
+	// message), sees that someone has now deleted a status
+	// in which they were replied to but not mentioned. In
+	// other words, they *might* see that someone subtooted
+	// about them, but they won't know what was said.
+
+	// Ensure mentions are populated.
+	mentions := s.Mentions
+	if len(s.MentionIDs) > len(mentions) {
+		mentions, err = c.db.GetMentions(ctx, s.MentionIDs)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToASDelete: error getting mentions: %w", err)
+		}
+	}
+
+	// Remember which accounts were mentioned
+	// here to avoid duplicating them later.
+	mentionedAccountIDs := make(map[string]interface{}, len(mentions))
+
+	// For direct messages, add URI
+	// to To, else just add to CC.
+	var f func(v *url.URL)
+	if s.Visibility == gtsmodel.VisibilityDirect {
+		f = toProp.AppendIRI
+	} else {
+		f = ccProp.AppendIRI
+	}
+
+	for _, m := range mentions {
+		mentionedAccountIDs[m.TargetAccountID] = nil // Remember this ID.
+
+		iri, err := url.Parse(m.TargetAccount.URI)
+		if err != nil {
+			return nil, fmt.Errorf("StatusToAS: error parsing uri %s: %s", m.TargetAccount.URI, err)
+		}
+
+		f(iri)
+	}
+
+	if s.InReplyToAccountID != "" {
+		if _, ok := mentionedAccountIDs[s.InReplyToAccountID]; !ok {
+			// Only address to this account if it
+			// wasn't already included as a mention.
+			if s.InReplyToAccount == nil {
+				s.InReplyToAccount, err = c.db.GetAccountByID(ctx, s.InReplyToAccountID)
+				if err != nil && !errors.Is(err, db.ErrNoEntries) {
+					return nil, fmt.Errorf("StatusToASDelete: db error getting account %s: %w", s.InReplyToAccountID, err)
+				}
+			}
+
+			if s.InReplyToAccount != nil {
+				inReplyToAccountURI, err := url.Parse(s.InReplyToAccount.URI)
+				if err != nil {
+					return nil, fmt.Errorf("StatusToASDelete: error parsing url %s: %w", s.InReplyToAccount.URI, err)
+				}
+				ccProp.AppendIRI(inReplyToAccountURI)
+			}
+		}
+	}
+
+	delete.SetActivityStreamsTo(toProp)
+	delete.SetActivityStreamsCc(ccProp)
+
+	return delete, nil
+}
+
 func (c *converter) FollowToAS(ctx context.Context, f *gtsmodel.Follow, originAccount *gtsmodel.Account, targetAccount *gtsmodel.Account) (vocab.ActivityStreamsFollow, error) {
 	// parse out the various URIs we need for this
 	// origin account (who's doing the follow)

internal/typeutils/internaltoas_test.go

@@ -430,6 +430,116 @@ func (suite *InternalToASTestSuite) TestStatusToASWithMentions() {
 }`, string(bytes))
 }
 
+func (suite *InternalToASTestSuite) TestStatusToASDeletePublicReply() {
+	testStatus := suite.testStatuses["admin_account_status_3"]
+	ctx := context.Background()
+
+	asDelete, err := suite.typeconverter.StatusToASDelete(ctx, testStatus)
+	suite.NoError(err)
+
+	ser, err := streams.Serialize(asDelete)
+	suite.NoError(err)
+
+	bytes, err := json.MarshalIndent(ser, "", "  ")
+	suite.NoError(err)
+
+	suite.Equal(`{
+  "@context": "https://www.w3.org/ns/activitystreams",
+  "actor": "http://localhost:8080/users/admin",
+  "cc": [
+    "http://localhost:8080/users/admin/followers",
+    "http://localhost:8080/users/the_mighty_zork"
+  ],
+  "object": "http://localhost:8080/users/admin/statuses/01FF25D5Q0DH7CHD57CTRS6WK0",
+  "to": "https://www.w3.org/ns/activitystreams#Public",
+  "type": "Delete"
+}`, string(bytes))
+}
+
+func (suite *InternalToASTestSuite) TestStatusToASDeletePublicReplyOriginalDeleted() {
+	testStatus := suite.testStatuses["admin_account_status_3"]
+	ctx := context.Background()
+
+  // Delete the status this replies to.
+  if err := suite.db.DeleteStatusByID(ctx, testStatus.ID); err != nil {
+    suite.FailNow(err.Error())
+  }
+
+  // Delete the mention the reply created.
+  mention := suite.testMentions["admin_account_mention_zork"]
+  if err := suite.db.DeleteByID(ctx, mention.ID, mention); err != nil {
+    suite.FailNow(err.Error())
+  }
+
+  // The delete should still be created OK.
+	asDelete, err := suite.typeconverter.StatusToASDelete(ctx, testStatus)
+	suite.NoError(err)
+
+	ser, err := streams.Serialize(asDelete)
+	suite.NoError(err)
+
+	bytes, err := json.MarshalIndent(ser, "", "  ")
+	suite.NoError(err)
+
+	suite.Equal(`{
+  "@context": "https://www.w3.org/ns/activitystreams",
+  "actor": "http://localhost:8080/users/admin",
+  "cc": [
+    "http://localhost:8080/users/admin/followers",
+    "http://localhost:8080/users/the_mighty_zork"
+  ],
+  "object": "http://localhost:8080/users/admin/statuses/01FF25D5Q0DH7CHD57CTRS6WK0",
+  "to": "https://www.w3.org/ns/activitystreams#Public",
+  "type": "Delete"
+}`, string(bytes))
+}
+
+func (suite *InternalToASTestSuite) TestStatusToASDeletePublic() {
+	testStatus := suite.testStatuses["admin_account_status_1"]
+	ctx := context.Background()
+
+	asDelete, err := suite.typeconverter.StatusToASDelete(ctx, testStatus)
+	suite.NoError(err)
+
+	ser, err := streams.Serialize(asDelete)
+	suite.NoError(err)
+
+	bytes, err := json.MarshalIndent(ser, "", "  ")
+	suite.NoError(err)
+
+	suite.Equal(`{
+  "@context": "https://www.w3.org/ns/activitystreams",
+  "actor": "http://localhost:8080/users/admin",
+  "cc": "http://localhost:8080/users/admin/followers",
+  "object": "http://localhost:8080/users/admin/statuses/01F8MH75CBF9JFX4ZAD54N0W0R",
+  "to": "https://www.w3.org/ns/activitystreams#Public",
+  "type": "Delete"
+}`, string(bytes))
+}
+
+func (suite *InternalToASTestSuite) TestStatusToASDeleteDirectMessage() {
+	testStatus := suite.testStatuses["local_account_2_status_6"]
+	ctx := context.Background()
+
+	asDelete, err := suite.typeconverter.StatusToASDelete(ctx, testStatus)
+	suite.NoError(err)
+
+	ser, err := streams.Serialize(asDelete)
+	suite.NoError(err)
+
+	bytes, err := json.MarshalIndent(ser, "", "  ")
+	suite.NoError(err)
+
+	suite.Equal(`{
+  "@context": "https://www.w3.org/ns/activitystreams",
+  "actor": "http://localhost:8080/users/1happyturtle",
+  "cc": [],
+  "object": "http://localhost:8080/users/1happyturtle/statuses/01FN3VJGFH10KR7S2PB0GFJZYG",
+  "to": "http://localhost:8080/users/the_mighty_zork",
+  "type": "Delete"
+}`, string(bytes))
+}
+
 func (suite *InternalToASTestSuite) TestStatusesToASOutboxPage() {
 	testAccount := suite.testAccounts["admin_account"]
 	ctx := context.Background()