[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
This commit is contained in:
kim 2024-04-03 13:57:07 +01:00 committed by GitHub
parent 65b5366031
commit 15ede4c1ea
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 31 additions and 8 deletions

View file

@ -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.
)
}

View file

@ -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)
}

View file

@ -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 {

View file

@ -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())
}