fix(activitypub): cover all routes with signature checks (#12339)

This changes the ReqHTTPSignature middleware to cover the entire activitypub
route group to not miss any new routes again in the future. Further, this adds
a tests iterating through all activitypub routes to test that the signature
verification is actually done.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12339
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: elle <0xllx0@noreply.codeberg.org>

routers/api/v1/api.go

@@ -873,31 +873,28 @@ func Routes() *web.Route {
 		if setting.Federation.Enabled {
 			m.Get("/nodeinfo", misc.NodeInfo)
 			m.Group("/activitypub", func() {
-				m.Group("/user-id/{user-id}", func() {
-					m.Get("", activitypub.ReqHTTPSignature(), activitypub.Person)
-					m.Post("/inbox",
-						activitypub.ReqHTTPSignature(),
-						bind(ap.Activity{}),
-						activitypub.PersonInbox)
-					m.Group("/activities/{activity-id}", func() {
-						m.Get("", activitypub.PersonActivityNote)
-						m.Get("/activity", activitypub.PersonActivity)
+				// The instance actor must always be fetchable without signatures
+				m.Get("/actor", activitypub.Actor)
+				m.Group("", func() {
+					m.Group("/actor", func() {
+						m.Post("/inbox", activitypub.ActorInbox)
+						m.Get("/outbox", activitypub.ActorOutbox)
 					})
-					m.Get("/outbox", activitypub.ReqHTTPSignature(), activitypub.PersonFeed)
-				}, context.UserIDAssignmentAPI(), checkTokenPublicOnly())
-				m.Group("/actor", func() {
-					m.Get("", activitypub.Actor)
-					m.Post("/inbox", activitypub.ReqHTTPSignature(), activitypub.ActorInbox)
-					m.Get("/outbox", activitypub.ActorOutbox)
-				})
-				m.Group("/repository-id/{repository-id}", func() {
-					m.Get("", activitypub.ReqHTTPSignature(), activitypub.Repository)
-					m.Post("/inbox",
-						bind(ap.Activity{}),
-						activitypub.ReqHTTPSignature(),
-						activitypub.RepositoryInbox)
-					m.Get("/outbox", activitypub.ReqHTTPSignature(), activitypub.RepositoryOutbox)
-				}, context.RepositoryIDAssignmentAPI())
+					m.Group("/user-id/{user-id}", func() {
+						m.Get("", activitypub.Person)
+						m.Post("/inbox", bind(ap.Activity{}), activitypub.PersonInbox)
+						m.Get("/outbox", activitypub.PersonFeed)
+						m.Group("/activities/{activity-id}", func() {
+							m.Get("", activitypub.PersonActivityNote)
+							m.Get("/activity", activitypub.PersonActivity)
+						})
+					}, context.UserIDAssignmentAPI(), checkTokenPublicOnly())
+					m.Group("/repository-id/{repository-id}", func() {
+						m.Get("", activitypub.Repository)
+						m.Post("/inbox", bind(ap.Activity{}), activitypub.RepositoryInbox)
+						m.Get("/outbox", activitypub.RepositoryOutbox)
+					}, context.RepositoryIDAssignmentAPI())
+				}, activitypub.ReqHTTPSignature())
 			}, tokenRequiresScopes(auth_model.AccessTokenScopeCategoryActivityPub))
 		}
 

tests/integration/api_activitypub_actor_test.go

@@ -49,6 +49,9 @@ func TestActivityPubActor(t *testing.T) {
 	assert.Regexp(t, "^-----BEGIN PUBLIC KEY-----", pubKeyPem)
 
 	t.Run("ActorOutboxEmpty", func(t *testing.T) {
+		// /inbox and /outbox routes also require signature checks
+		defer test.MockVariableValue(&setting.Federation.SignatureEnforced, false)()
+
 		req := NewRequest(t, "GET", actor.Outbox.GetID().String())
 		resp := MakeRequest(t, req, http.StatusOK)
 

tests/integration/api_activitypub_person_test.go

@@ -80,6 +80,7 @@ func TestActivityPubPerson(t *testing.T) {
 func TestActivityPubMissingPerson(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	defer test.MockVariableValue(&setting.Federation.Enabled, true)()
+	defer test.MockVariableValue(&setting.Federation.SignatureEnforced, false)()
 	defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())()
 
 	req := NewRequest(t, "GET", "/api/v1/activitypub/user-id/999999999")

tests/integration/api_federation_httpsig_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -20,6 +21,7 @@ import (
 	"forgejo.org/routers"
 	"forgejo.org/services/contexttest"
 
+	"github.com/go-chi/chi/v5"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -94,3 +96,54 @@ func TestFederationHttpSigValidation(t *testing.T) {
 		})
 	})
 }
+
+func TestFederationAllRoutesCovered(t *testing.T) {
+	defer test.MockVariableValue(&setting.Federation.Enabled, true)()
+	defer test.MockVariableValue(&testWebRoutes, routers.NormalRoutes())()
+
+	routes := routers.NormalRoutes().R.Routes()
+
+	var r *chi.Route
+	for _, route := range routes {
+		if route.Pattern == "/api/v1/*" {
+			r = &route
+			break
+		}
+	}
+
+	require.NotNil(t, r)
+
+	ranOne := false
+	for _, route := range r.SubRoutes.Routes() {
+		if !strings.HasPrefix(route.Pattern, "/activitypub/") {
+			continue
+		}
+
+		ranOne = true
+		if route.Pattern == "/activitypub/actor" {
+			// unsigned request to the actor should always succed
+			req := NewRequest(t, "GET", fmt.Sprintf("%sapi/v1/activitypub/actor", setting.AppURL))
+			MakeRequest(t, req, http.StatusOK)
+		} else {
+			// this just puts in something for the replacements to be able to make a request
+			url := fmt.Sprintf("%sapi/v1%s", setting.AppURL, route.Pattern)
+			for strings.Contains(url, "{") {
+				before, after, _ := strings.Cut(url, "/{")
+				_, after, _ = strings.Cut(after, "}/")
+				url = fmt.Sprintf("%s/1/%s", before, after)
+			}
+
+			var req *RequestWrapper
+			if strings.Contains(route.Pattern, "inbox") {
+				req = NewRequestWithJSON(t, "POST", url, "{}")
+			} else {
+				req = NewRequest(t, "GET", url)
+			}
+
+			resp := MakeRequest(t, req, http.StatusBadRequest)
+			assert.Contains(t, resp.Body.String(), "request signature verification failed")
+		}
+	}
+
+	require.True(t, ranOne)
+}