From 15ede4c1ea4e5a9f69b3f0ed72d94ce764ffed1d Mon Sep 17 00:00:00 2001
From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com>
Date: Wed, 3 Apr 2024 13:57:07 +0100
Subject: [PATCH] [bugfix] improved authenticate post inbox error handling
 (#2803)

* improved PostInboxScheme() error handling / logging in case of failed auth

* dumbass kim. returning err instead of errWithCode...

* add checks for the slightly changed error handling in tests, add notes in codebase about the odd way of working
---
 .../api/activitypub/users/inboxpost_test.go     |  2 +-
 internal/federation/federatingactor.go          | 17 ++++++++++++++++-
 internal/federation/federatingprotocol.go       |  9 ++++-----
 internal/federation/federatingprotocol_test.go  | 11 ++++++++++-
 4 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/internal/api/activitypub/users/inboxpost_test.go b/internal/api/activitypub/users/inboxpost_test.go
index 89e1cec41..9d863f234 100644
--- a/internal/api/activitypub/users/inboxpost_test.go
+++ b/internal/api/activitypub/users/inboxpost_test.go
@@ -590,7 +590,7 @@ func (suite *InboxPostTestSuite) TestPostUnauthorized() {
 		requestingAccount,
 		targetAccount,
 		http.StatusUnauthorized,
-		`{"error":"Unauthorized: not authenticated"}`,
+		`{"error":"Unauthorized: http request wasn't signed or http signature was invalid: (verifier)"}`,
 		// Omit signature check middleware.
 	)
 }
diff --git a/internal/federation/federatingactor.go b/internal/federation/federatingactor.go
index bf54962db..18cee1666 100644
--- a/internal/federation/federatingactor.go
+++ b/internal/federation/federatingactor.go
@@ -80,8 +80,23 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr
 	}
 
 	// Authenticate request by checking http signature.
+	//
+	// NOTE: the behaviour here is a little strange as we have
+	// the competing code styles of the go-fed interface expecting
+	// that any 'err' is fatal, but 'authenticated' bool is intended to
+	// be the main passer of whether failed auth occurred, but we in
+	// the gts codebase use errors to pass-back non-200 status codes,
+	// so we specifically have to check for already wrapped with code.
+	//
 	ctx, authenticated, err := f.sideEffectActor.AuthenticatePostInbox(ctx, w, r)
-	if err != nil {
+	if errors.As(err, new(gtserror.WithCode)) {
+		// If it was already wrapped with an
+		// HTTP code then don't bother rewrapping
+		// it, just return it as-is for caller to
+		// handle. AuthenticatePostInbox already
+		// calls WriteHeader() in some situations.
+		return false, err
+	} else if err != nil {
 		err := gtserror.Newf("error authenticating post inbox: %w", err)
 		return false, gtserror.NewErrorInternalError(err)
 	}
diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go
index 2c2da7b7b..f8e5b4c09 100644
--- a/internal/federation/federatingprotocol.go
+++ b/internal/federation/federatingprotocol.go
@@ -216,7 +216,6 @@ func (f *Federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 			// If codes 400, 401, or 403, obey the go-fed
 			// interface by writing the header and bailing.
 			w.WriteHeader(errWithCode.Code())
-			return ctx, false, nil
 		case http.StatusGone:
 			// If the requesting account's key has gone
 			// (410) then likely inbox post was a delete.
@@ -225,11 +224,11 @@ func (f *Federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 			// know about the account anyway, so we can't
 			// do any further processing.
 			w.WriteHeader(http.StatusAccepted)
-			return ctx, false, nil
-		default:
-			// Proper error.
-			return ctx, false, err
 		}
+
+		// We still return the error
+		// for later request logging.
+		return ctx, false, errWithCode
 	}
 
 	if pubKeyAuth.Handshaking {
diff --git a/internal/federation/federatingprotocol_test.go b/internal/federation/federatingprotocol_test.go
index 999569c85..f975cd7d6 100644
--- a/internal/federation/federatingprotocol_test.go
+++ b/internal/federation/federatingprotocol_test.go
@@ -21,6 +21,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -30,6 +31,7 @@ import (
 	"github.com/stretchr/testify/suite"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
+	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/testrig"
 	"github.com/superseriousbusiness/httpsig"
@@ -99,7 +101,14 @@ func (suite *FederatingProtocolTestSuite) authenticatePostInbox(
 
 	recorder := httptest.NewRecorder()
 	newContext, authed, err := suite.federator.AuthenticatePostInbox(ctx, recorder, request)
-	if err != nil {
+	if withCode := new(gtserror.WithCode); (errors.As(err, withCode) &&
+		(*withCode).Code() >= 500) || (err != nil && (*withCode) == nil) {
+		// NOTE: the behaviour here is a little strange as we have
+		// the competing code styles of the go-fed interface expecting
+		// that any err is a no-go, but authed bool is intended to be
+		// the main passer of whether failed auth occurred, but we in
+		// the gts codebase use errors to pass-back non-200 status codes,
+		// so we specifically have to check for an internal error code.
 		suite.FailNow(err.Error())
 	}