From 67e11a1a613bea9b7f5ac81a5f3bb2cd6c47b105 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Thu, 18 Jan 2024 16:11:13 +0000 Subject: [PATCH] [chore] chore rationalise http return codes for activitypub handlers (#2540) * some small code fixups and changes * add check in ResolveIncomingActivity for transient activity types (i.e. activity ID is nil) * update test to handle new transient behaviour --- internal/ap/resolve.go | 23 +++++++++++-------- .../api/activitypub/users/inboxpost_test.go | 8 ++++--- internal/federation/federatingactor.go | 6 +++-- internal/federation/federatingdb/reject.go | 20 ++++------------ internal/federation/federatingdb/undo.go | 2 +- 5 files changed, 29 insertions(+), 30 deletions(-) diff --git a/internal/ap/resolve.go b/internal/ap/resolve.go index 61f187da0..4ff4f87fc 100644 --- a/internal/ap/resolve.go +++ b/internal/ap/resolve.go @@ -56,8 +56,9 @@ func putMap(m map[string]any) { mapPool.Put(m) } -// ResolveActivity is a util function for pulling a pub.Activity type out of an incoming request body. -func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode) { +// ResolveActivity is a util function for pulling a pub.Activity type out of an incoming request body, +// returning the resolved activity type, error and whether to accept activity (false = transient i.e. ignore). +func ResolveIncomingActivity(r *http.Request) (pub.Activity, bool, gtserror.WithCode) { // Get "raw" map // destination. raw := getMap() @@ -68,7 +69,7 @@ func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode) // Decode the JSON body stream into "raw" map. if err := json.NewDecoder(r.Body).Decode(&raw); err != nil { err := gtserror.Newf("error decoding json: %w", err) - return nil, gtserror.NewErrorInternalError(err) + return nil, false, gtserror.NewErrorInternalError(err) } // Resolve "raw" JSON to vocab.Type. @@ -76,25 +77,29 @@ func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode) if err != nil { if !streams.IsUnmatchedErr(err) { err := gtserror.Newf("error matching json to type: %w", err) - return nil, gtserror.NewErrorInternalError(err) + return nil, false, gtserror.NewErrorInternalError(err) } // Respond with bad request; we just couldn't // match the type to one that we know about. const text = "body json not resolvable as ActivityStreams type" - return nil, gtserror.NewErrorBadRequest(errors.New(text), text) + return nil, false, gtserror.NewErrorBadRequest(errors.New(text), text) } // Ensure this is an Activity type. activity, ok := t.(pub.Activity) if !ok { text := fmt.Sprintf("cannot resolve vocab type %T as pub.Activity", t) - return nil, gtserror.NewErrorBadRequest(errors.New(text), text) + return nil, false, gtserror.NewErrorBadRequest(errors.New(text), text) } if activity.GetJSONLDId() == nil { - const text = "missing ActivityStreams id property" - return nil, gtserror.NewErrorBadRequest(errors.New(text), text) + // missing ID indicates a transient ID as per: + // + // all objects distributed by the ActivityPub protocol MUST have unique global identifiers, + // unless they are intentionally transient (short lived activities that are not intended to + // be able to be looked up, such as some kinds of chat messages or game notifications). + return nil, false, nil } // Normalize any Statusable, Accountable, Pollable fields found. @@ -104,7 +109,7 @@ func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode) // Release. putMap(raw) - return activity, nil + return activity, true, nil } // ResolveStatusable tries to resolve the given bytes into an ActivityPub Statusable representation. diff --git a/internal/api/activitypub/users/inboxpost_test.go b/internal/api/activitypub/users/inboxpost_test.go index 2791f8110..715231ecc 100644 --- a/internal/api/activitypub/users/inboxpost_test.go +++ b/internal/api/activitypub/users/inboxpost_test.go @@ -478,15 +478,17 @@ func (suite *InboxPostTestSuite) TestPostEmptyCreate() { targetAccount = suite.testAccounts["local_account_1"] ) - // Post a create with no object. + // Post a create with no object, this + // should get accepted and silently dropped + // as the lack of ID marks it as transient. create := streams.NewActivityStreamsCreate() suite.inboxPost( create, requestingAccount, targetAccount, - http.StatusBadRequest, - `{"error":"Bad Request: missing ActivityStreams id property"}`, + http.StatusAccepted, + `{"status":"Accepted"}`, suite.signatureCheck, ) } diff --git a/internal/federation/federatingactor.go b/internal/federation/federatingactor.go index 81f3c3281..8fc47462d 100644 --- a/internal/federation/federatingactor.go +++ b/internal/federation/federatingactor.go @@ -143,10 +143,12 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr have not yet applied authorization (ie., blocks). */ - // Obtain the activity; reject unknown activities. - activity, errWithCode := ap.ResolveIncomingActivity(r) + // Resolve the activity, rejecting badly formatted / transient. + activity, ok, errWithCode := ap.ResolveIncomingActivity(r) if errWithCode != nil { return false, errWithCode + } else if !ok { // transient + return false, nil } // Set additional context data. Primarily this means diff --git a/internal/federation/federatingdb/reject.go b/internal/federation/federatingdb/reject.go index e02db18e0..437741584 100644 --- a/internal/federation/federatingdb/reject.go +++ b/internal/federation/federatingdb/reject.go @@ -45,16 +45,11 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR return nil // Already processed. } - rejectObject := reject.GetActivityStreamsObject() - if rejectObject == nil { - return errors.New("Reject: no object set on vocab.ActivityStreamsReject") - } + for _, obj := range ap.ExtractObjects(reject) { - for iter := rejectObject.Begin(); iter != rejectObject.End(); iter = iter.Next() { - // check if the object is an IRI - if iter.IsIRI() { + if obj.IsIRI() { // we have just the URI of whatever is being rejected, so we need to find out what it is - rejectedObjectIRI := iter.GetIRI() + rejectedObjectIRI := obj.GetIRI() if uris.IsFollowPath(rejectedObjectIRI) { // REJECT FOLLOW followReq, err := f.state.DB.GetFollowRequestByURI(ctx, rejectedObjectIRI.String()) @@ -71,15 +66,10 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR } } - // check if iter is an AP object / type - if iter.GetType() == nil { - continue - } - - if iter.GetType().GetTypeName() == ap.ActivityFollow { + if t := obj.GetType(); t != nil { // we have the whole object so we can figure out what we're rejecting // REJECT FOLLOW - asFollow, ok := iter.GetType().(vocab.ActivityStreamsFollow) + asFollow, ok := t.(vocab.ActivityStreamsFollow) if !ok { return errors.New("Reject: couldn't parse follow into vocab.ActivityStreamsFollow") } diff --git a/internal/federation/federatingdb/undo.go b/internal/federation/federatingdb/undo.go index a7a0f077a..6bc4cd7aa 100644 --- a/internal/federation/federatingdb/undo.go +++ b/internal/federation/federatingdb/undo.go @@ -78,7 +78,7 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) } } - return nil + return errs.Combine() } func (f *federatingDB) undoFollow(