Add a layer of safety around required ActivityPub Actor fields (#4703)

* fix(ap): add safe constructors, getters, and validators to ActivityPub actors to address #4701

* chore: replace nil check with the new Validate() method

* chore(test): added test to verify the values extracted from actors

* fix: return the specific error type + test for it

* fix(ap): add recovery for each AP inbox worker for a worst case scenario

* fix(ap): add additional safe accessor methods to other AP entities other than actors

* chore(tests): add tests for the new safe accessors

* fix(ap): handle empty public keys in AP actors

activitypub/inbox/announce.go

@@ -5,16 +5,24 @@ import (
 	"time"
 
 	"github.com/go-fed/activity/streams/vocab"
+	"github.com/owncast/owncast/activitypub/apmodels"
 	"github.com/owncast/owncast/activitypub/persistence"
 	"github.com/owncast/owncast/core/chat/events"
 	"github.com/pkg/errors"
 )
 
 func handleAnnounceRequest(c context.Context, activity vocab.ActivityStreamsAnnounce) error {
-	object := activity.GetActivityStreamsObject()
+	objectIRI, err := apmodels.GetIRIStringFromObjectProperty(activity.GetActivityStreamsObject())
+	if err != nil {
+		return errors.Wrap(err, "announce activity is missing object IRI")
+	}
+
+	actorIRI, err := apmodels.GetIRIStringFromActorProperty(activity.GetActivityStreamsActor())
+	if err != nil {
+		return errors.Wrap(err, "announce activity is missing actor IRI")
+	}
+
 	actorReference := activity.GetActivityStreamsActor()
-	objectIRI := object.At(0).GetIRI().String()
-	actorIRI := actorReference.At(0).GetIRI().String()
 
 	if hasPreviouslyhandled, err := persistence.HasPreviouslyHandledInboundActivity(objectIRI, actorIRI, events.FediverseEngagementRepost); hasPreviouslyhandled || err != nil {
 		return errors.Wrap(err, "inbound activity of share/re-post has already been handled")

activitypub/inbox/chat.go

@@ -24,14 +24,17 @@ func handleEngagementActivity(eventType events.EventType, isLiveNotification boo
 	}
 
 	// Get actor of the action
-	actor, _ := resolvers.GetResolvedActorFromActorProperty(actorReference)
+	actor, err := resolvers.GetResolvedActorFromActorProperty(actorReference)
+	if err != nil {
+		return fmt.Errorf("unable to resolve actor for engagement activity: %w", err)
+	}
 
 	// Send chat message
 	actorName := actor.Name
 	if actorName == "" {
 		actorName = actor.Username
 	}
-	actorIRI := actorReference.Begin().GetIRI().String()
+	actorIRI := actor.ActorIriString()
 
 	userPrefix := fmt.Sprintf("%s ", actorName)
 	var suffix string
@@ -51,9 +54,8 @@ func handleEngagementActivity(eventType events.EventType, isLiveNotification boo
 	body := fmt.Sprintf("%s %s", userPrefix, suffix)
 
 	var image *string
-	if actor.Image != nil {
-		s := actor.Image.String()
-		image = &s
+	if imageStr := actor.ImageString(); imageStr != "" {
+		image = &imageStr
 	}
 
 	if err := chat.SendFediverseAction(eventType, actor.FullUsername, image, body, actorIRI); err != nil {

activitypub/inbox/create.go

@@ -4,10 +4,14 @@ import (
 	"context"
 
 	"github.com/go-fed/activity/streams/vocab"
+	"github.com/owncast/owncast/activitypub/apmodels"
 	"github.com/pkg/errors"
 )
 
 func handleCreateRequest(c context.Context, activity vocab.ActivityStreamsCreate) error {
-	iri := activity.GetJSONLDId().GetIRI().String()
+	iri, err := apmodels.GetIRIStringFromJSONLDIdProperty(activity.GetJSONLDId())
+	if err != nil {
+		return errors.Wrap(err, "create activity is missing IRI")
+	}
 	return errors.New("not handling create request of: " + iri)
 }

activitypub/inbox/follow.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/go-fed/activity/streams/vocab"
+	"github.com/owncast/owncast/activitypub/apmodels"
 	"github.com/owncast/owncast/activitypub/persistence"
 	"github.com/owncast/owncast/activitypub/requests"
 	"github.com/owncast/owncast/activitypub/resolvers"
@@ -40,10 +41,18 @@ func handleFollowInboxRequest(c context.Context, activity vocab.ActivityStreamsF
 	}
 
 	localAccountName := configRepository.GetDefaultFederationUsername()
+
+	objectIRI, err := apmodels.GetIRIStringFromObjectProperty(activity.GetActivityStreamsObject())
+	if err != nil {
+		return errors.Wrap(err, "follow activity is missing object IRI")
+	}
+
+	actorIRI, err := apmodels.GetIRIStringFromActorProperty(activity.GetActivityStreamsActor())
+	if err != nil {
+		return errors.Wrap(err, "follow activity is missing actor IRI")
+	}
+
 	actorReference := activity.GetActivityStreamsActor()
-	object := activity.GetActivityStreamsObject()
-	objectIRI := object.At(0).GetIRI().String()
-	actorIRI := actorReference.At(0).GetIRI().String()
 
 	if approved {
 		if err := requests.SendFollowAccept(follow.Inbox, activity, localAccountName); err != nil {

activitypub/inbox/like.go

@@ -5,30 +5,25 @@ import (
 	"time"
 
 	"github.com/go-fed/activity/streams/vocab"
+	"github.com/owncast/owncast/activitypub/apmodels"
 	"github.com/owncast/owncast/activitypub/persistence"
 	"github.com/owncast/owncast/core/chat/events"
 	"github.com/pkg/errors"
 )
 
 func handleLikeRequest(c context.Context, activity vocab.ActivityStreamsLike) error {
-	object := activity.GetActivityStreamsObject()
+	objectIRI, err := apmodels.GetIRIStringFromObjectProperty(activity.GetActivityStreamsObject())
+	if err != nil {
+		return errors.Wrap(err, "like activity is missing object IRI")
+	}
+
+	actorIRI, err := apmodels.GetIRIStringFromActorProperty(activity.GetActivityStreamsActor())
+	if err != nil {
+		return errors.Wrap(err, "like activity is missing actor IRI")
+	}
+
 	actorReference := activity.GetActivityStreamsActor()
 
-	if object.Len() < 1 {
-		return errors.New("like activity is missing object")
-	}
-
-	if actorReference.Len() < 1 {
-		return errors.New("like activity is missing actor")
-	}
-
-	if object.At(0).GetIRI() == nil {
-		return errors.New("like activity iri is missing")
-	}
-
-	objectIRI := object.At(0).GetIRI().String()
-	actorIRI := actorReference.At(0).GetIRI().String()
-
 	if hasPreviouslyhandled, err := persistence.HasPreviouslyHandledInboundActivity(objectIRI, actorIRI, events.FediverseEngagementLike); hasPreviouslyhandled || err != nil {
 		return errors.Wrap(err, "inbound activity of like has already been handled")
 	}

activitypub/inbox/nil_safety_test.go

@@ -0,0 +1,251 @@
+package inbox
+
+import (
+	"context"
+	"net/url"
+	"testing"
+
+	"github.com/go-fed/activity/streams"
+)
+
+func mustParseURL(s string) *url.URL {
+	u, err := url.Parse(s)
+	if err != nil {
+		panic(err)
+	}
+	return u
+}
+
+// These tests verify that handler functions don't panic when given
+// ActivityPub activities with nil or missing properties that could
+// cause nil pointer dereferences.
+
+func TestHandleFollowWithNilObject(t *testing.T) {
+	activity := streams.NewActivityStreamsFollow()
+	// Don't set object or actor - they will be nil
+
+	// This should return an error, not panic
+	err := handleFollowInboxRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleFollowInboxRequest with nil object should return error")
+	}
+}
+
+func TestHandleFollowWithEmptyObject(t *testing.T) {
+	activity := streams.NewActivityStreamsFollow()
+	object := streams.NewActivityStreamsObjectProperty()
+	activity.SetActivityStreamsObject(object)
+	// Object is set but empty (no items)
+
+	err := handleFollowInboxRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleFollowInboxRequest with empty object should return error")
+	}
+}
+
+func TestHandleFollowWithNilActorIRI(t *testing.T) {
+	activity := streams.NewActivityStreamsFollow()
+
+	// Set a valid object with IRI
+	object := streams.NewActivityStreamsObjectProperty()
+	objectNote := streams.NewActivityStreamsNote()
+	objectID := streams.NewJSONLDIdProperty()
+	objectID.SetIRI(mustParseURL("https://example.com/note/1"))
+	objectNote.SetJSONLDId(objectID)
+	object.AppendActivityStreamsNote(objectNote)
+	activity.SetActivityStreamsObject(object)
+
+	// Actor is nil
+	err := handleFollowInboxRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleFollowInboxRequest with nil actor should return error")
+	}
+}
+
+func TestHandleAnnounceWithNilObject(t *testing.T) {
+	activity := streams.NewActivityStreamsAnnounce()
+	// Don't set object or actor
+
+	err := handleAnnounceRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleAnnounceRequest with nil object should return error")
+	}
+}
+
+func TestHandleAnnounceWithEmptyObject(t *testing.T) {
+	activity := streams.NewActivityStreamsAnnounce()
+	object := streams.NewActivityStreamsObjectProperty()
+	activity.SetActivityStreamsObject(object)
+
+	err := handleAnnounceRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleAnnounceRequest with empty object should return error")
+	}
+}
+
+func TestHandleAnnounceWithNilActorIRI(t *testing.T) {
+	activity := streams.NewActivityStreamsAnnounce()
+
+	// Set object with IRI
+	object := streams.NewActivityStreamsObjectProperty()
+	object.AppendIRI(mustParseURL("https://example.com/note/1"))
+	activity.SetActivityStreamsObject(object)
+
+	// Actor is nil
+	err := handleAnnounceRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleAnnounceRequest with nil actor should return error")
+	}
+}
+
+func TestHandleLikeWithNilObject(t *testing.T) {
+	activity := streams.NewActivityStreamsLike()
+	// Don't set object or actor
+
+	err := handleLikeRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleLikeRequest with nil object should return error")
+	}
+}
+
+func TestHandleLikeWithEmptyObject(t *testing.T) {
+	activity := streams.NewActivityStreamsLike()
+	object := streams.NewActivityStreamsObjectProperty()
+	activity.SetActivityStreamsObject(object)
+
+	err := handleLikeRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleLikeRequest with empty object should return error")
+	}
+}
+
+func TestHandleLikeWithNilActorIRI(t *testing.T) {
+	activity := streams.NewActivityStreamsLike()
+
+	// Set object with IRI
+	object := streams.NewActivityStreamsObjectProperty()
+	object.AppendIRI(mustParseURL("https://example.com/note/1"))
+	activity.SetActivityStreamsObject(object)
+
+	// Actor is nil
+	err := handleLikeRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleLikeRequest with nil actor should return error")
+	}
+}
+
+func TestHandleCreateWithNilId(t *testing.T) {
+	activity := streams.NewActivityStreamsCreate()
+	// Don't set JSONLD ID
+
+	err := handleCreateRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleCreateRequest with nil ID should return error")
+	}
+}
+
+func TestHandleCreateWithIdButNilIRI(t *testing.T) {
+	activity := streams.NewActivityStreamsCreate()
+	id := streams.NewJSONLDIdProperty()
+	// Set the ID property but don't set an IRI on it
+	activity.SetJSONLDId(id)
+
+	err := handleCreateRequest(context.Background(), activity)
+	if err == nil {
+		t.Error("handleCreateRequest with ID but nil IRI should return error")
+	}
+}
+
+func TestHandleUpdateWithNilObject(t *testing.T) {
+	activity := streams.NewActivityStreamsUpdate()
+	// Don't set object - should return nil (not an error, just skip)
+
+	// This should not panic and should return nil since we only care about Person updates
+	err := handleUpdateRequest(context.Background(), activity)
+	if err != nil {
+		t.Errorf("handleUpdateRequest with nil object should return nil (skip), got %v", err)
+	}
+}
+
+func TestHandleUpdateWithEmptyObject(t *testing.T) {
+	activity := streams.NewActivityStreamsUpdate()
+	object := streams.NewActivityStreamsObjectProperty()
+	activity.SetActivityStreamsObject(object)
+
+	// Should return nil since empty object means it's not a Person update
+	err := handleUpdateRequest(context.Background(), activity)
+	if err != nil {
+		t.Errorf("handleUpdateRequest with empty object should return nil (skip), got %v", err)
+	}
+}
+
+func TestHandleUpdateWithNonPersonObject(t *testing.T) {
+	activity := streams.NewActivityStreamsUpdate()
+	object := streams.NewActivityStreamsObjectProperty()
+	note := streams.NewActivityStreamsNote()
+	object.AppendActivityStreamsNote(note)
+	activity.SetActivityStreamsObject(object)
+
+	// Should return nil since it's not a Person update
+	err := handleUpdateRequest(context.Background(), activity)
+	if err != nil {
+		t.Errorf("handleUpdateRequest with non-Person object should return nil (skip), got %v", err)
+	}
+}
+
+// TestNilSafetyNoPanic verifies that none of the handlers panic when given
+// completely empty activities. This is the most important test - we want to
+// ensure that malformed ActivityPub payloads don't crash the server.
+func TestNilSafetyNoPanic(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("Follow with nil properties", func(t *testing.T) {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("handleFollowInboxRequest panicked: %v", r)
+			}
+		}()
+		activity := streams.NewActivityStreamsFollow()
+		_ = handleFollowInboxRequest(ctx, activity)
+	})
+
+	t.Run("Announce with nil properties", func(t *testing.T) {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("handleAnnounceRequest panicked: %v", r)
+			}
+		}()
+		activity := streams.NewActivityStreamsAnnounce()
+		_ = handleAnnounceRequest(ctx, activity)
+	})
+
+	t.Run("Like with nil properties", func(t *testing.T) {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("handleLikeRequest panicked: %v", r)
+			}
+		}()
+		activity := streams.NewActivityStreamsLike()
+		_ = handleLikeRequest(ctx, activity)
+	})
+
+	t.Run("Create with nil properties", func(t *testing.T) {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("handleCreateRequest panicked: %v", r)
+			}
+		}()
+		activity := streams.NewActivityStreamsCreate()
+		_ = handleCreateRequest(ctx, activity)
+	})
+
+	t.Run("Update with nil properties", func(t *testing.T) {
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("handleUpdateRequest panicked: %v", r)
+			}
+		}()
+		activity := streams.NewActivityStreamsUpdate()
+		_ = handleUpdateRequest(ctx, activity)
+	})
+}

activitypub/inbox/update.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/go-fed/activity/streams/vocab"
+	"github.com/owncast/owncast/activitypub/apmodels"
 	"github.com/owncast/owncast/activitypub/persistence"
 	"github.com/owncast/owncast/activitypub/resolvers"
 	log "github.com/sirupsen/logrus"
@@ -11,7 +12,7 @@ import (
 
 func handleUpdateRequest(c context.Context, activity vocab.ActivityStreamsUpdate) error {
 	// We only care about update events to followers.
-	if !activity.GetActivityStreamsObject().At(0).IsActivityStreamsPerson() {
+	if !apmodels.IsFirstObjectActivityStreamsPerson(activity.GetActivityStreamsObject()) {
 		return nil
 	}
 
@@ -21,5 +22,5 @@ func handleUpdateRequest(c context.Context, activity vocab.ActivityStreamsUpdate
 		return err
 	}
 
-	return persistence.UpdateFollower(actor.ActorIri.String(), actor.Inbox.String(), actor.Name, actor.FullUsername, actor.Image.String())
+	return persistence.UpdateFollower(actor.ActorIriString(), actor.InboxString(), actor.Name, actor.FullUsername, actor.ImageString())
 }

activitypub/inbox/worker.go

@@ -96,7 +96,10 @@ func Verify(request *http.Request) (bool, error) {
 		return false, err
 	}
 
-	key := publicKey.GetW3IDSecurityV1PublicKeyPem().Get()
+	key, err := apmodels.GetPublicKeyPem(publicKey)
+	if err != nil {
+		return false, errors.Wrap(err, "failed to get public key PEM")
+	}
 	block, _ := pem.Decode([]byte(key))
 	if block == nil {
 		log.Errorln("failed to parse PEM block containing the public key")

activitypub/inbox/workerpool.go

@@ -37,7 +37,14 @@ func worker(workerID int, queue <-chan Job) {
 	log.Debugf("Started ActivityPub worker %d", workerID)
 
 	for job := range queue {
-		handle(job.request)
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					log.Errorf("Recovered from panic in ActivityPub worker %d: %v", workerID, r)
+				}
+			}()
+			handle(job.request)
+		}()
 
 		log.Tracef("Done with ActivityPub inbox handler using worker %d", workerID)
 	}