[bugfix] add stricter checks during all stages of dereferencing remote AS objects (#2639)

* add stricter checks during all stages of dereferencing remote AS objects

* a comment

internal/federation/dereferencing/account.go

@@ -606,6 +606,16 @@ func (d *Dereferencer) enrichAccount(
 	}
 
 	if account.Username == "" {
+		// Assume the host from the
+		// ActivityPub representation.
+		id := ap.GetJSONLDId(apubAcc)
+		if id == nil {
+			return nil, nil, gtserror.New("no id property found on person, or id was not an iri")
+		}
+
+		// Get IRI host value.
+		accHost := id.Host
+
 		// No username was provided, so no webfinger was attempted earlier.
 		//
 		// Now we have a username we can attempt again, to ensure up-to-date
@@ -616,42 +626,37 @@ func (d *Dereferencer) enrichAccount(
 		// https://example.org/@someone@somewhere.else and we've been redirected
 		// from example.org to somewhere.else: we want to take somewhere.else
 		// as the accountDomain then, not the example.org we were redirected from.
-
-		// Assume the host from the returned
-		// ActivityPub representation.
-		id := ap.GetJSONLDId(apubAcc)
-		if id == nil {
-			return nil, nil, gtserror.New("no id property found on person, or id was not an iri")
-		}
-
-		// Get IRI host value.
-		accHost := id.Host
-
 		latestAcc.Domain, _, err = d.fingerRemoteAccount(ctx,
 			tsport,
 			latestAcc.Username,
 			accHost,
 		)
 		if err != nil {
-			// We still couldn't webfinger the account, so we're not certain
-			// what the accountDomain actually is. Still, we can make a solid
-			// guess that it's the Host of the ActivityPub URI of the account.
-			// If we're wrong, we can just try again in a couple days.
-			log.Errorf(ctx, "error webfingering[2] remote account %s@%s: %v", latestAcc.Username, accHost, err)
-			latestAcc.Domain = accHost
+			// Webfingering account still failed, so we're not certain
+			// what the accountDomain actually is. Exit here for safety.
+			return nil, nil, gtserror.Newf(
+				"error webfingering remote account %s@%s: %w",
+				latestAcc.Username, accHost, err,
+			)
 		}
 	}
 
 	if latestAcc.Domain == "" {
 		// Ensure we have a domain set by this point,
 		// otherwise it gets stored as a local user!
-		//
-		// TODO: there is probably a more granular way
-		// way of checking this in each of the above parts,
-		// and honestly it could do with a smol refactor.
 		return nil, nil, gtserror.Newf("empty domain for %s", uri)
 	}
 
+	// Ensure the final parsed account URI / URL matches
+	// the input URI we fetched (or received) it as.
+	if expect := uri.String(); latestAcc.URI != expect &&
+		latestAcc.URL != expect {
+		return nil, nil, gtserror.Newf(
+			"dereferenced account uri %s does not match %s",
+			latestAcc.URI, expect,
+		)
+	}
+
 	/*
 		BY THIS POINT we have more or less a fullly-formed
 		representation of the target account, derived from

internal/federation/dereferencing/account_test.go

@@ -19,6 +19,7 @@ package dereferencing_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -207,6 +208,28 @@ func (suite *AccountTestSuite) TestDereferenceLocalAccountWithUnknownUserURI() {
 	suite.Nil(fetchedAccount)
 }
 
+func (suite *AccountTestSuite) TestDereferenceRemoteAccountWithNonMatchingURI() {
+	fetchingAccount := suite.testAccounts["local_account_1"]
+
+	const (
+		remoteURI    = "https://turnip.farm/users/turniplover6969"
+		remoteAltURI = "https://turnip.farm/users/turniphater420"
+	)
+
+	// Create a copy of this remote account at alternative URI.
+	remotePerson := suite.client.TestRemotePeople[remoteURI]
+	suite.client.TestRemotePeople[remoteAltURI] = remotePerson
+
+	// Attempt to fetch account at alternative URI, it should fail!
+	fetchedAccount, _, err := suite.dereferencer.GetAccountByURI(
+		context.Background(),
+		fetchingAccount.Username,
+		testrig.URLMustParse(remoteAltURI),
+	)
+	suite.Equal(err.Error(), fmt.Sprintf("enrichAccount: dereferenced account uri %s does not match %s", remoteURI, remoteAltURI))
+	suite.Nil(fetchedAccount)
+}
+
 func TestAccountTestSuite(t *testing.T) {
 	suite.Run(t, new(AccountTestSuite))
 }

internal/federation/dereferencing/dereferencer_test.go

@@ -35,6 +35,7 @@ type DereferencerStandardTestSuite struct {
 	db      db.DB
 	storage *storage.Driver
 	state   state.State
+	client  *testrig.MockHTTPClient
 
 	testRemoteStatuses    map[string]vocab.ActivityStreamsNote
 	testRemotePeople      map[string]vocab.ActivityStreamsPerson
@@ -72,11 +73,12 @@ func (suite *DereferencerStandardTestSuite) SetupTest() {
 		converter,
 	)
 
+	suite.client = testrig.NewMockHTTPClient(nil, "../../../testrig/media")
 	suite.storage = testrig.NewInMemoryStorage()
 	suite.state.DB = suite.db
 	suite.state.Storage = suite.storage
 	media := testrig.NewTestMediaManager(&suite.state)
-	suite.dereferencer = dereferencing.NewDereferencer(&suite.state, converter, testrig.NewTestTransportController(&suite.state, testrig.NewMockHTTPClient(nil, "../../../testrig/media")), media)
+	suite.dereferencer = dereferencing.NewDereferencer(&suite.state, converter, testrig.NewTestTransportController(&suite.state, suite.client), media)
 	testrig.StandardDBSetup(suite.db, nil)
 }
 

internal/federation/dereferencing/finger.go

@@ -21,9 +21,9 @@ import (
 	"context"
 	"encoding/json"
 	"net/url"
-	"strings"
 
 	apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model"
+	apiutil "github.com/superseriousbusiness/gotosocial/internal/api/util"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
 	"github.com/superseriousbusiness/gotosocial/internal/transport"
@@ -74,10 +74,12 @@ func (d *Dereferencer) fingerRemoteAccount(
 		return "", nil, err
 	}
 
-	_, accountDomain, err := util.ExtractWebfingerParts(resp.Subject)
+	accUsername, accDomain, err := util.ExtractWebfingerParts(resp.Subject)
 	if err != nil {
 		err = gtserror.Newf("error extracting subject parts for %s: %w", target, err)
 		return "", nil, err
+	} else if accUsername != username {
+		return "", nil, gtserror.Newf("response username does not match input for %s: %w", target, err)
 	}
 
 	// Look through links for the first
@@ -92,8 +94,7 @@ func (d *Dereferencer) fingerRemoteAccount(
 			continue
 		}
 
-		if !strings.EqualFold(link.Type, "application/activity+json") &&
-			!strings.EqualFold(link.Type, "application/ld+json; profile=\"https://www.w3.org/ns/activitystreams\"") {
+		if !apiutil.ASContentType(link.Type) {
 			// Not an AP type, ignore.
 			continue
 		}
@@ -121,7 +122,7 @@ func (d *Dereferencer) fingerRemoteAccount(
 		}
 
 		// All looks good, return happily!
-		return accountDomain, uri, nil
+		return accDomain, uri, nil
 	}
 
 	return "", nil, gtserror.Newf("no suitable self, AP-type link found in webfinger response for %s", target)

internal/federation/dereferencing/status.go

@@ -413,7 +413,7 @@ func (d *Dereferencer) enrichStatus(
 	}
 
 	// Ensure we have the author account of the status dereferenced (+ up-to-date). If this is a new status
-	// (i.e. status.AccountID == "") then any error here is irrecoverable. AccountID must ALWAYS be set.
+	// (i.e. status.AccountID == "") then any error here is irrecoverable. status.AccountID must ALWAYS be set.
 	if _, _, err := d.getAccountByURI(ctx, requestUser, attributedTo); err != nil && status.AccountID == "" {
 		return nil, nil, gtserror.Newf("failed to dereference status author %s: %w", uri, err)
 	}
@@ -425,11 +425,30 @@ func (d *Dereferencer) enrichStatus(
 		return nil, nil, gtserror.Newf("error converting statusable to gts model for status %s: %w", uri, err)
 	}
 
+	// Ensure final status isn't attempting
+	// to claim being authored by local user.
+	if latestStatus.Account.IsLocal() {
+		return nil, nil, gtserror.Newf(
+			"dereferenced status %s claiming to be local",
+			latestStatus.URI,
+		)
+	}
+
+	// Ensure the final parsed status URI / URL matches
+	// the input URI we fetched (or received) it as.
+	if expect := uri.String(); latestStatus.URI != expect &&
+		latestStatus.URL != expect {
+		return nil, nil, gtserror.Newf(
+			"dereferenced status uri %s does not match %s",
+			latestStatus.URI, expect,
+		)
+	}
+
+	var isNew bool
+
 	// Based on the original provided
 	// status model, determine whether
 	// this is a new insert / update.
-	var isNew bool
-
 	if isNew = (status.ID == ""); isNew {
 
 		// Generate new status ID from the provided creation date.

internal/federation/dereferencing/status_test.go

@@ -19,6 +19,7 @@ package dereferencing_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
@@ -218,6 +219,28 @@ func (suite *StatusTestSuite) TestDereferenceStatusWithImageAndNoContent() {
 	suite.NoError(err)
 }
 
+func (suite *StatusTestSuite) TestDereferenceStatusWithNonMatchingURI() {
+	fetchingAccount := suite.testAccounts["local_account_1"]
+
+	const (
+		remoteURI    = "https://turnip.farm/users/turniplover6969/statuses/70c53e54-3146-42d5-a630-83c8b6c7c042"
+		remoteAltURI = "https://turnip.farm/users/turniphater420/statuses/70c53e54-3146-42d5-a630-83c8b6c7c042"
+	)
+
+	// Create a copy of this remote account at alternative URI.
+	remoteStatus := suite.client.TestRemoteStatuses[remoteURI]
+	suite.client.TestRemoteStatuses[remoteAltURI] = remoteStatus
+
+	// Attempt to fetch account at alternative URI, it should fail!
+	fetchedStatus, _, err := suite.dereferencer.GetStatusByURI(
+		context.Background(),
+		fetchingAccount.Username,
+		testrig.URLMustParse(remoteAltURI),
+	)
+	suite.Equal(err.Error(), fmt.Sprintf("enrichStatus: dereferenced status uri %s does not match %s", remoteURI, remoteAltURI))
+	suite.Nil(fetchedStatus)
+}
+
 func TestStatusTestSuite(t *testing.T) {
 	suite.Run(t, new(StatusTestSuite))
 }