[bugfix] harden checks for remotes masquerading as local, and return correct local account redirects early (#3706)

3 files changed, 146 insertions, 22 deletions

diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go
index a47284c34..a9a816b4c 100644
--- a/internal/federation/dereferencing/account.go
+++ b/internal/federation/dereferencing/account.go
@@ -639,7 +639,16 @@ func (d *Dereferencer) enrichAccount(
 				return nil, nil, gtserror.Newf("db error getting account after redirects: %w", err)
 			}
 
-			if alreadyAcc != nil {
+			switch {
+			case alreadyAcc == nil:
+				// nothing to do
+
+			case alreadyAcc.IsLocal():
+				// Request eventually redirected to a
+				// local account. Return it as-is here.
+				return alreadyAcc, nil, nil
+
+			default:
 				// We had this account stored
 				// under discovered final URI.
 				//
@@ -718,12 +727,6 @@ func (d *Dereferencer) enrichAccount(
 		latestAcc.Username = cmp.Or(latestAcc.Username, accUsername)
 	}
 
-	if latestAcc.Domain == "" {
-		// Ensure we have a domain set by this point,
-		// otherwise it gets stored as a local user!
-		return nil, nil, gtserror.Newf("empty domain for %s", uri)
-	}
-
 	// Ensure the final parsed account URI matches
 	// the input URI we fetched (or received) it as.
 	if matches, err := util.URIMatches(
@@ -740,10 +743,16 @@ func (d *Dereferencer) enrichAccount(
 	} else if !matches {
 		return nil, nil, gtserror.Newf(
 			"account uri %s does not match %s",
-			latestAcc.URI, uri.String(),
+			latestAcc.URI, uri,
 		)
 	}
 
+	// Ensure this isn't a local account,
+	// or a remote masquerading as such!
+	if latestAcc.IsLocal() {
+		return nil, nil, gtserror.Newf("cannot dereference local account %s", uri)
+	}
+
 	// Get current time.
 	now := time.Now()
 
diff --git a/internal/federation/dereferencing/account_test.go b/internal/federation/dereferencing/account_test.go
index 309771758..e6bd7eab7 100644
--- a/internal/federation/dereferencing/account_test.go
+++ b/internal/federation/dereferencing/account_test.go
@@ -18,11 +18,15 @@
 package dereferencing_test
 
 import (
+	"bytes"
 	"context"
 	"crypto/rsa"
 	"crypto/x509"
+	"encoding/json"
 	"encoding/pem"
 	"fmt"
+	"io"
+	"net/http"
 	"net/url"
 	"testing"
 	"time"
@@ -33,6 +37,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
+	"github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/testrig"
 )
@@ -214,6 +219,111 @@ func (suite *AccountTestSuite) TestDereferenceLocalAccountWithUnknownUserURI() {
 	suite.Nil(fetchedAccount)
 }
 
+func (suite *AccountTestSuite) TestDereferenceLocalAccountByRedirect() {
+	ctx, cncl := context.WithCancel(context.Background())
+	defer cncl()
+
+	fetchingAccount := suite.testAccounts["local_account_1"]
+	targetAccount := suite.testAccounts["local_account_2"]
+
+	// Convert the target account to ActivityStreams model for dereference.
+	targetAccountable, err := suite.converter.AccountToAS(ctx, targetAccount)
+	suite.NoError(err)
+	suite.NotNil(targetAccountable)
+
+	// Serialize to "raw" JSON map for response.
+	rawJSON, err := ap.Serialize(targetAccountable)
+	suite.NoError(err)
+
+	// Finally serialize to actual bytes.
+	json, err := json.Marshal(rawJSON)
+	suite.NoError(err)
+
+	// Replace test HTTP client with one that always returns the target account AS model.
+	suite.client = testrig.NewMockHTTPClient(func(req *http.Request) (*http.Response, error) {
+		return &http.Response{
+			Status:        http.StatusText(http.StatusOK),
+			StatusCode:    http.StatusOK,
+			ContentLength: int64(len(json)),
+			Header:        http.Header{"Content-Type": {"application/activity+json"}},
+			Body:          io.NopCloser(bytes.NewReader(json)),
+			Request:       &http.Request{URL: testrig.URLMustParse(targetAccount.URI)},
+		}, nil
+	}, "")
+
+	// Update dereferencer to use new test HTTP client.
+	suite.dereferencer = dereferencing.NewDereferencer(
+		&suite.state,
+		suite.converter,
+		testrig.NewTestTransportController(&suite.state, suite.client),
+		suite.visFilter,
+		suite.intFilter,
+		suite.media,
+	)
+
+	// Use any old input test URI, this doesn't actually matter what it is.
+	uri := testrig.URLMustParse("https://this-will-be-redirected.butts/")
+
+	// Try dereference the test URI, since it correctly redirects to us it should return our account.
+	account, accountable, err := suite.dereferencer.GetAccountByURI(ctx, fetchingAccount.Username, uri)
+	suite.NoError(err)
+	suite.Nil(accountable)
+	suite.NotNil(account)
+	suite.Equal(targetAccount.ID, account.ID)
+}
+
+func (suite *AccountTestSuite) TestDereferenceMasqueradingLocalAccount() {
+	ctx, cncl := context.WithCancel(context.Background())
+	defer cncl()
+
+	fetchingAccount := suite.testAccounts["local_account_1"]
+	targetAccount := suite.testAccounts["local_account_2"]
+
+	// Convert the target account to ActivityStreams model for dereference.
+	targetAccountable, err := suite.converter.AccountToAS(ctx, targetAccount)
+	suite.NoError(err)
+	suite.NotNil(targetAccountable)
+
+	// Serialize to "raw" JSON map for response.
+	rawJSON, err := ap.Serialize(targetAccountable)
+	suite.NoError(err)
+
+	// Finally serialize to actual bytes.
+	json, err := json.Marshal(rawJSON)
+	suite.NoError(err)
+
+	// Use any old input test URI, this doesn't actually matter what it is.
+	uri := testrig.URLMustParse("https://this-will-be-redirected.butts/")
+
+	// Replace test HTTP client with one that returns OUR account, but at their URI endpoint.
+	suite.client = testrig.NewMockHTTPClient(func(req *http.Request) (*http.Response, error) {
+		return &http.Response{
+			Status:        http.StatusText(http.StatusOK),
+			StatusCode:    http.StatusOK,
+			ContentLength: int64(len(json)),
+			Header:        http.Header{"Content-Type": {"application/activity+json"}},
+			Body:          io.NopCloser(bytes.NewReader(json)),
+			Request:       &http.Request{URL: uri},
+		}, nil
+	}, "")
+
+	// Update dereferencer to use new test HTTP client.
+	suite.dereferencer = dereferencing.NewDereferencer(
+		&suite.state,
+		suite.converter,
+		testrig.NewTestTransportController(&suite.state, suite.client),
+		suite.visFilter,
+		suite.intFilter,
+		suite.media,
+	)
+
+	// Try dereference the test URI, since it correctly redirects to us it should return our account.
+	account, accountable, err := suite.dereferencer.GetAccountByURI(ctx, fetchingAccount.Username, uri)
+	suite.NotNil(err)
+	suite.Nil(account)
+	suite.Nil(accountable)
+}
+
 func (suite *AccountTestSuite) TestDereferenceRemoteAccountWithNonMatchingURI() {
 	fetchingAccount := suite.testAccounts["local_account_1"]
 
diff --git a/internal/federation/dereferencing/dereferencer_test.go b/internal/federation/dereferencing/dereferencer_test.go
index 9878a1b50..f7627eca0 100644
--- a/internal/federation/dereferencing/dereferencer_test.go
+++ b/internal/federation/dereferencing/dereferencer_test.go
@@ -26,6 +26,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/filter/interaction"
 	"github.com/superseriousbusiness/gotosocial/internal/filter/visibility"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/media"
 	"github.com/superseriousbusiness/gotosocial/internal/state"
 	"github.com/superseriousbusiness/gotosocial/internal/storage"
 	"github.com/superseriousbusiness/gotosocial/internal/typeutils"
@@ -34,10 +35,14 @@ import (
 
 type DereferencerStandardTestSuite struct {
 	suite.Suite
-	db      db.DB
-	storage *storage.Driver
-	state   state.State
-	client  *testrig.MockHTTPClient
+	db        db.DB
+	storage   *storage.Driver
+	state     state.State
+	client    *testrig.MockHTTPClient
+	converter *typeutils.Converter
+	visFilter *visibility.Filter
+	intFilter *interaction.Filter
+	media     *media.Manager
 
 	testRemoteStatuses    map[string]vocab.ActivityStreamsNote
 	testRemotePeople      map[string]vocab.ActivityStreamsPerson
@@ -67,12 +72,15 @@ func (suite *DereferencerStandardTestSuite) SetupTest() {
 
 	suite.db = testrig.NewTestDB(&suite.state)
 
-	converter := typeutils.NewConverter(&suite.state)
+	suite.converter = typeutils.NewConverter(&suite.state)
+	suite.visFilter = visibility.NewFilter(&suite.state)
+	suite.intFilter = interaction.NewFilter(&suite.state)
+	suite.media = testrig.NewTestMediaManager(&suite.state)
 
 	testrig.StartTimelines(
 		&suite.state,
-		visibility.NewFilter(&suite.state),
-		converter,
+		suite.visFilter,
+		suite.converter,
 	)
 
 	suite.client = testrig.NewMockHTTPClient(nil, "../../../testrig/media")
@@ -81,19 +89,16 @@ func (suite *DereferencerStandardTestSuite) SetupTest() {
 	suite.state.AdminActions = admin.New(suite.state.DB, &suite.state.Workers)
 	suite.state.Storage = suite.storage
 
-	visFilter := visibility.NewFilter(&suite.state)
-	intFilter := interaction.NewFilter(&suite.state)
-	media := testrig.NewTestMediaManager(&suite.state)
 	suite.dereferencer = dereferencing.NewDereferencer(
 		&suite.state,
-		converter,
+		suite.converter,
 		testrig.NewTestTransportController(
 			&suite.state,
 			suite.client,
 		),
-		visFilter,
-		intFilter,
-		media,
+		suite.visFilter,
+		suite.intFilter,
+		suite.media,
 	)
 	testrig.StandardDBSetup(suite.db, nil)
 }