activitypub: restrict HTTP redirects (#13369)

Restricts the ActivityPub `Client`'s inner HTTP client to disallow HTTP redirects.

Resolves: #13368

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests for Go changes

- I added test coverage for Go changes...
  - [x] in their respective `*_test.go` for unit tests.
- I ran...
  - [x] `make pr-go` before pushing

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13369
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>

modules/activitypub/client.go

@@ -72,6 +72,16 @@ func NewClientFactory() (c *ClientFactory, err error) {
 	return NewClientFactoryWithTimeout(5 * time.Second)
 }
 
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	// NOTE: we don't want to allow any redirects for the ActivityPub client.
+	// For our use-case there is no legitimate context for a redirect,
+	// e.g. fetching keys, posting mailbox messages, etc.
+	//
+	// At some point in the future, we may want to support limited redirects,
+	// possibly configurable through a settings option.
+	return errors.New("activitypub: client: redirects are not allowed")
+}
+
 // NewClient function
 func NewClientFactoryWithTimeout(timeout time.Duration) (c *ClientFactory, err error) {
 	if err = containsRequiredHTTPHeaders(http.MethodGet, setting.Federation.GetHeaders); err != nil {
@@ -85,7 +95,8 @@ func NewClientFactoryWithTimeout(timeout time.Duration) (c *ClientFactory, err e
 			Transport: &http.Transport{
 				Proxy: proxy.Proxy(),
 			},
-			Timeout: timeout,
+			Timeout:       timeout,
+			CheckRedirect: checkRedirect,
 		},
 		algs:        setting.HttpsigAlgs,
 		digestAlg:   httpsig.DigestAlgorithm(setting.Federation.DigestAlgorithm),

modules/activitypub/client_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strconv"
 	"testing"
 	"time"
 
@@ -168,3 +169,30 @@ func TestActivityPubSignedPost(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, expected, string(body))
 }
+
+func TestActivityPubRedirect(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	pubID := "https://example.com/pubID"
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		statusCode, _ := strconv.ParseInt(r.URL.Query()["code"][0], 10, 32)
+		w.WriteHeader(int(statusCode))
+		w.Header().Add("Location", "/evil-redirect")
+		w.Write([]byte("/evil-redirect"))
+	}))
+
+	cf, err := activitypub.NewClientFactory()
+	require.NoError(t, err)
+	srvURL, err := url.Parse(srv.URL)
+	require.NoError(t, err)
+	c, err := cf.WithKeys(db.DefaultContext, user, pubID, []*url.URL{srvURL})
+	require.NoError(t, err)
+
+	// try each HTTP status code (encoded in a query param for convenience)
+	// to ensure all redirects result in an error
+	for _, code := range []int{http.StatusMovedPermanently, http.StatusFound, http.StatusTemporaryRedirect, http.StatusPermanentRedirect, http.StatusMultipleChoices, http.StatusNotModified} {
+		_, err = c.Get(fmt.Sprintf("%s?code=%d", srv.URL, code))
+		require.Error(t, err)
+	}
+}