From 5022be3029ea975a88ca462a17e1e536e5363254 Mon Sep 17 00:00:00 2001 From: famfo Date: Fri, 8 May 2026 00:40:01 +0200 Subject: [PATCH] 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 Reviewed-by: elle <0xllx0@noreply.codeberg.org> --- routers/api/v1/api.go | 45 ++++++++-------- .../integration/api_activitypub_actor_test.go | 3 ++ .../api_activitypub_person_test.go | 1 + .../api_federation_httpsig_test.go | 53 +++++++++++++++++++ 4 files changed, 78 insertions(+), 24 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 8910babc29..55ea6762e3 100644 --- a/routers/api/v1/api.go +++ b/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)) } diff --git a/tests/integration/api_activitypub_actor_test.go b/tests/integration/api_activitypub_actor_test.go index 2a758f5424..25c55e75a8 100644 --- a/tests/integration/api_activitypub_actor_test.go +++ b/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) diff --git a/tests/integration/api_activitypub_person_test.go b/tests/integration/api_activitypub_person_test.go index c0f297a7d3..75746d1a17 100644 --- a/tests/integration/api_activitypub_person_test.go +++ b/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") diff --git a/tests/integration/api_federation_httpsig_test.go b/tests/integration/api_federation_httpsig_test.go index 2f62efb2af..431676acee 100644 --- a/tests/integration/api_federation_httpsig_test.go +++ b/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) +}