fix: only redirect to a new owner (organization or user) if the user has permissions to view the new owner (#9072)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9072
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Reviewed-by: 0ko <0ko@noreply.codeberg.org>

models/fixtures/repo_redirect.yml

@@ -3,3 +3,9 @@
   owner_id: 2
   lower_name: oldrepo1
   redirect_repo_id: 1
+
+-
+  id: 2
+  owner_id: 17
+  lower_name: oldrepo24
+  redirect_repo_id: 24

models/fixtures/user_redirect.yml

@@ -3,3 +3,15 @@
   lower_name: olduser1
   redirect_user_id: 1
   created_unix: 1730000000
+
+-
+  id: 2
+  lower_name: oldorg22
+  redirect_user_id: 22
+  created_unix: 1730000000
+
+-
+  id: 3
+  lower_name: oldorg23
+  redirect_user_id: 23
+  created_unix: 1730000000

models/repo/redirect.go

@@ -14,8 +14,9 @@ import (
 
 // ErrRedirectNotExist represents a "RedirectNotExist" kind of error.
 type ErrRedirectNotExist struct {
-	OwnerID  int64
+	OwnerID           int64
-	RepoName string
+	RepoName          string
+	MissingPermission bool
 }
 
 // IsErrRedirectNotExist check if an error is an ErrRepoRedirectNotExist.
@@ -49,8 +50,8 @@ func init() {
 	db.RegisterModel(new(Redirect))
 }
 
-// LookupRedirect look up if a repository has a redirect name
+// GetRedirect returns the redirect for a given pair of ownerID and repository name.
-func LookupRedirect(ctx context.Context, ownerID int64, repoName string) (int64, error) {
+func GetRedirect(ctx context.Context, ownerID int64, repoName string) (int64, error) {
 	repoName = strings.ToLower(repoName)
 	redirect := &Redirect{OwnerID: ownerID, LowerName: repoName}
 	if has, err := db.GetEngine(ctx).Get(redirect); err != nil {

models/repo/redirect_test.go

@@ -10,21 +10,9 @@ import (
 	repo_model "forgejo.org/models/repo"
 	"forgejo.org/models/unittest"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestLookupRedirect(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-
-	repoID, err := repo_model.LookupRedirect(db.DefaultContext, 2, "oldrepo1")
-	require.NoError(t, err)
-	assert.EqualValues(t, 1, repoID)
-
-	_, err = repo_model.LookupRedirect(db.DefaultContext, unittest.NonexistentID, "doesnotexist")
-	assert.True(t, repo_model.IsErrRedirectNotExist(err))
-}
-
 func TestNewRedirect(t *testing.T) {
 	// redirect to a completely new name
 	require.NoError(t, unittest.PrepareTestDatabase())

models/user/redirect.go

@@ -21,7 +21,8 @@ import (
 
 // ErrUserRedirectNotExist represents a "UserRedirectNotExist" kind of error.
 type ErrUserRedirectNotExist struct {
-	Name string
+	Name              string
+	MissingPermission bool
 }
 
 // IsErrUserRedirectNotExist check if an error is an ErrUserRedirectNotExist.
@@ -81,15 +82,6 @@ func GetUserRedirect(ctx context.Context, userName string) (*Redirect, error) {
 	return redirect, nil
 }
 
-// LookupUserRedirect look up userID if a user has a redirect name
-func LookupUserRedirect(ctx context.Context, userName string) (int64, error) {
-	redirect, err := GetUserRedirect(ctx, userName)
-	if err != nil {
-		return 0, err
-	}
-	return redirect.RedirectUserID, nil
-}
-
 // NewUserRedirect create a new user redirect
 func NewUserRedirect(ctx context.Context, ID int64, oldUserName, newUserName string) error {
 	oldUserName = strings.ToLower(oldUserName)

models/user/redirect_test.go

@@ -1,26 +0,0 @@
-// Copyright 2020 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package user_test
-
-import (
-	"testing"
-
-	"forgejo.org/models/db"
-	"forgejo.org/models/unittest"
-	user_model "forgejo.org/models/user"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-)
-
-func TestLookupUserRedirect(t *testing.T) {
-	require.NoError(t, unittest.PrepareTestDatabase())
-
-	userID, err := user_model.LookupUserRedirect(db.DefaultContext, "olduser1")
-	require.NoError(t, err)
-	assert.EqualValues(t, 1, userID)
-
-	_, err = user_model.LookupUserRedirect(db.DefaultContext, "doesnotexist")
-	assert.True(t, user_model.IsErrUserRedirectNotExist(err))
-}

routers/api/v1/api.go

@@ -99,6 +99,7 @@ import (
 	"forgejo.org/services/auth"
 	"forgejo.org/services/context"
 	"forgejo.org/services/forms"
+	redirect_service "forgejo.org/services/redirect"
 
 	_ "forgejo.org/routers/api/v1/swagger" // for swagger generation
 
@@ -153,12 +154,12 @@ func repoAssignment() func(ctx *context.APIContext) {
 			owner, err = user_model.GetUserByName(ctx, userName)
 			if err != nil {
 				if user_model.IsErrUserNotExist(err) {
-					if redirectUserID, err := user_model.LookupUserRedirect(ctx, userName); err == nil {
+					if redirectUserID, err := redirect_service.LookupUserRedirect(ctx, ctx.Doer, userName); err == nil {
 						context.RedirectToUser(ctx.Base, userName, redirectUserID)
 					} else if user_model.IsErrUserRedirectNotExist(err) {
 						ctx.NotFound("GetUserByName", err)
 					} else {
-						ctx.Error(http.StatusInternalServerError, "LookupUserRedirect", err)
+						ctx.Error(http.StatusInternalServerError, "LookupRedirect", err)
 					}
 				} else {
 					ctx.Error(http.StatusInternalServerError, "GetUserByName", err)
@@ -173,7 +174,7 @@ func repoAssignment() func(ctx *context.APIContext) {
 		repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, repoName)
 		if err != nil {
 			if repo_model.IsErrRepoNotExist(err) {
-				redirectRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, repoName)
+				redirectRepoID, err := redirect_service.LookupRepoRedirect(ctx, ctx.Doer, owner.ID, repoName)
 				if err == nil {
 					context.RedirectToRepo(ctx.Base, redirectRepoID)
 				} else if repo_model.IsErrRedirectNotExist(err) {
@@ -641,13 +642,13 @@ func orgAssignment(args ...bool) func(ctx *context.APIContext) {
 			ctx.Org.Organization, err = organization.GetOrgByName(ctx, ctx.Params(":org"))
 			if err != nil {
 				if organization.IsErrOrgNotExist(err) {
-					redirectUserID, err := user_model.LookupUserRedirect(ctx, ctx.Params(":org"))
+					redirectUserID, err := redirect_service.LookupUserRedirect(ctx, ctx.Doer, ctx.Params(":org"))
 					if err == nil {
 						context.RedirectToUser(ctx.Base, ctx.Params(":org"), redirectUserID)
 					} else if user_model.IsErrUserRedirectNotExist(err) {
 						ctx.NotFound("GetOrgByName", err)
 					} else {
-						ctx.Error(http.StatusInternalServerError, "LookupUserRedirect", err)
+						ctx.Error(http.StatusInternalServerError, "LookupRedirect", err)
 					}
 				} else {
 					ctx.Error(http.StatusInternalServerError, "GetOrgByName", err)

routers/api/v1/user/helper.go

@@ -8,6 +8,7 @@ import (
 
 	user_model "forgejo.org/models/user"
 	"forgejo.org/services/context"
+	redirect_service "forgejo.org/services/redirect"
 )
 
 // GetUserByParamsName get user by name
@@ -16,7 +17,7 @@ func GetUserByParamsName(ctx *context.APIContext, name string) *user_model.User
 	user, err := user_model.GetUserByName(ctx, username)
 	if err != nil {
 		if user_model.IsErrUserNotExist(err) {
-			if redirectUserID, err2 := user_model.LookupUserRedirect(ctx, username); err2 == nil {
+			if redirectUserID, err2 := redirect_service.LookupUserRedirect(ctx, ctx.Doer, username); err2 == nil {
 				context.RedirectToUser(ctx.Base, username, redirectUserID)
 			} else {
 				ctx.NotFound("GetUserByName", err)

routers/web/repo/githttp.go

@@ -31,6 +31,7 @@ import (
 	"forgejo.org/modules/structs"
 	"forgejo.org/modules/util"
 	"forgejo.org/services/context"
+	redirect_service "forgejo.org/services/redirect"
 	repo_service "forgejo.org/services/repository"
 
 	"github.com/go-chi/cors"
@@ -111,7 +112,7 @@ func httpBase(ctx *context.Context) *serviceHandler {
 			return nil
 		}
 
-		if redirectRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, reponame); err == nil {
+		if redirectRepoID, err := redirect_service.LookupRepoRedirect(ctx, ctx.Doer, owner.ID, reponame); err == nil {
 			context.RedirectToRepo(ctx.Base, redirectRepoID)
 			return nil
 		}

services/context/org.go

@@ -16,6 +16,7 @@ import (
 	"forgejo.org/modules/markup/markdown"
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/structs"
+	redirect_service "forgejo.org/services/redirect"
 )
 
 // Organization contains organization context
@@ -48,13 +49,13 @@ func GetOrganizationByParams(ctx *Context) {
 	ctx.Org.Organization, err = organization.GetOrgByName(ctx, orgName)
 	if err != nil {
 		if organization.IsErrOrgNotExist(err) {
-			redirectUserID, err := user_model.LookupUserRedirect(ctx, orgName)
+			redirectUserID, err := redirect_service.LookupUserRedirect(ctx, ctx.Doer, orgName)
 			if err == nil {
 				RedirectToUser(ctx.Base, orgName, redirectUserID)
 			} else if user_model.IsErrUserRedirectNotExist(err) {
 				ctx.NotFound("GetUserByName", err)
 			} else {
-				ctx.ServerError("LookupUserRedirect", err)
+				ctx.ServerError("LookupRedirect", err)
 			}
 		} else {
 			ctx.ServerError("GetUserByName", err)

services/context/repo.go

@@ -35,6 +35,7 @@ import (
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/util"
 	asymkey_service "forgejo.org/services/asymkey"
+	redirect_service "forgejo.org/services/redirect"
 
 	"github.com/editorconfig/editorconfig-core-go/v2"
 )
@@ -477,12 +478,12 @@ func RepoAssignment(ctx *Context) context.CancelFunc {
 					return nil
 				}
 
-				if redirectUserID, err := user_model.LookupUserRedirect(ctx, userName); err == nil {
+				if redirectUserID, err := redirect_service.LookupUserRedirect(ctx, ctx.Doer, userName); err == nil {
 					RedirectToUser(ctx.Base, userName, redirectUserID)
 				} else if user_model.IsErrUserRedirectNotExist(err) {
 					ctx.NotFound("GetUserByName", nil)
 				} else {
-					ctx.ServerError("LookupUserRedirect", err)
+					ctx.ServerError("LookupRedirect", err)
 				}
 			} else {
 				ctx.ServerError("GetUserByName", err)
@@ -519,7 +520,7 @@ func RepoAssignment(ctx *Context) context.CancelFunc {
 	repo, err := repo_model.GetRepositoryByName(ctx, owner.ID, repoName)
 	if err != nil {
 		if repo_model.IsErrRepoNotExist(err) {
-			redirectRepoID, err := repo_model.LookupRedirect(ctx, owner.ID, repoName)
+			redirectRepoID, err := redirect_service.LookupRepoRedirect(ctx, ctx.Doer, owner.ID, repoName)
 			if err == nil {
 				RedirectToRepo(ctx.Base, redirectRepoID)
 			} else if repo_model.IsErrRedirectNotExist(err) {

services/context/user.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	user_model "forgejo.org/models/user"
+	redirect_service "forgejo.org/services/redirect"
 )
 
 // UserAssignmentWeb returns a middleware to handle context-user assignment for web routes
@@ -68,12 +69,12 @@ func userAssignment(ctx *Base, doer *user_model.User, errCb func(int, string, an
 		contextUser, err = user_model.GetUserByName(ctx, username)
 		if err != nil {
 			if user_model.IsErrUserNotExist(err) {
-				if redirectUserID, err := user_model.LookupUserRedirect(ctx, username); err == nil {
+				if redirectUserID, err := redirect_service.LookupUserRedirect(ctx, doer, username); err == nil {
 					RedirectToUser(ctx, username, redirectUserID)
 				} else if user_model.IsErrUserRedirectNotExist(err) {
 					errCb(http.StatusNotFound, "GetUserByName", err)
 				} else {
-					errCb(http.StatusInternalServerError, "LookupUserRedirect", err)
+					errCb(http.StatusInternalServerError, "LookupRedirect", err)
 				}
 			} else {
 				errCb(http.StatusInternalServerError, "GetUserByName", err)

services/redirect/main_test.go

@@ -0,0 +1,18 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+package redirect
+
+import (
+	"testing"
+
+	"forgejo.org/models/unittest"
+
+	_ "forgejo.org/models"
+	_ "forgejo.org/models/actions"
+	_ "forgejo.org/models/activities"
+	_ "forgejo.org/models/forgefed"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m)
+}

services/redirect/repo.go

@@ -0,0 +1,37 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+package redirect
+
+import (
+	"context"
+
+	access_model "forgejo.org/models/perm/access"
+	repo_model "forgejo.org/models/repo"
+	user_model "forgejo.org/models/user"
+)
+
+// LookupRepoRedirect returns the repository ID if there's a redirect registered for
+// the ownerID repository name pair. It checks if the doer has permission to view
+// the new repository.
+func LookupRepoRedirect(ctx context.Context, doer *user_model.User, ownerID int64, repoName string) (int64, error) {
+	redirectID, err := repo_model.GetRedirect(ctx, ownerID, repoName)
+	if err != nil {
+		return 0, err
+	}
+
+	redirectRepo, err := repo_model.GetRepositoryByID(ctx, redirectID)
+	if err != nil {
+		return 0, err
+	}
+
+	perm, err := access_model.GetUserRepoPermission(ctx, redirectRepo, doer)
+	if err != nil {
+		return 0, err
+	}
+
+	if !perm.HasAccess() {
+		return 0, repo_model.ErrRedirectNotExist{OwnerID: ownerID, RepoName: repoName, MissingPermission: true}
+	}
+
+	return redirectID, nil
+}

services/redirect/repo_test.go

@@ -0,0 +1,55 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+package redirect
+
+import (
+	"testing"
+
+	repo_model "forgejo.org/models/repo"
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLookupRepoRedirect(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	normalUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
+	ownerUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 20})
+
+	testOk := func(t *testing.T, doer *user_model.User, ownerID int64, repoName string, expectedRedirectID int64) {
+		t.Helper()
+
+		redirectID, err := LookupRepoRedirect(t.Context(), doer, ownerID, repoName)
+		require.NoError(t, err)
+		assert.Equal(t, expectedRedirectID, redirectID)
+	}
+
+	testFail := func(t *testing.T, doer *user_model.User, ownerID int64, repoName string) {
+		t.Helper()
+
+		redirectID, err := LookupRepoRedirect(t.Context(), doer, ownerID, repoName)
+		require.ErrorIs(t, err, repo_model.ErrRedirectNotExist{OwnerID: ownerID, RepoName: repoName, MissingPermission: true})
+		assert.Zero(t, redirectID)
+	}
+
+	t.Run("Public repository", func(t *testing.T) {
+		ownerID := int64(2)
+		reponame := "oldrepo1"
+
+		testOk(t, nil, ownerID, reponame, 1)
+		testOk(t, normalUser, ownerID, reponame, 1)
+		testOk(t, ownerUser, ownerID, reponame, 1)
+	})
+
+	t.Run("Private repository", func(t *testing.T) {
+		ownerID := int64(17)
+		reponame := "oldrepo24"
+
+		testFail(t, nil, ownerID, reponame)
+		testFail(t, normalUser, ownerID, reponame)
+		testOk(t, ownerUser, ownerID, reponame, 24)
+	})
+}

services/redirect/user.go

@@ -0,0 +1,30 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+package redirect
+
+import (
+	"context"
+
+	user_model "forgejo.org/models/user"
+)
+
+// LookupUserRedirect returns the userID if there's a redirect registered for the
+// username. It additionally checks if the doer has permission to view the new
+// user.
+func LookupUserRedirect(ctx context.Context, doer *user_model.User, userName string) (int64, error) {
+	redirect, err := user_model.GetUserRedirect(ctx, userName)
+	if err != nil {
+		return 0, err
+	}
+
+	redirectUser, err := user_model.GetUserByID(ctx, redirect.RedirectUserID)
+	if err != nil {
+		return 0, err
+	}
+
+	if !user_model.IsUserVisibleToViewer(ctx, redirectUser, doer) {
+		return 0, user_model.ErrUserRedirectNotExist{Name: userName, MissingPermission: true}
+	}
+
+	return redirect.RedirectUserID, nil
+}

services/redirect/user_test.go

@@ -0,0 +1,65 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+package redirect
+
+import (
+	"testing"
+
+	"forgejo.org/models/unittest"
+	user_model "forgejo.org/models/user"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLookupUserRedirect(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+	normalUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	testOk := func(t *testing.T, doer *user_model.User, username string, expectedRedirectID int64) {
+		t.Helper()
+
+		redirectID, err := LookupUserRedirect(t.Context(), doer, username)
+		require.NoError(t, err)
+		assert.Equal(t, expectedRedirectID, redirectID)
+	}
+
+	testFail := func(t *testing.T, doer *user_model.User, username string) {
+		t.Helper()
+
+		redirectID, err := LookupUserRedirect(t.Context(), doer, username)
+		require.ErrorIs(t, err, user_model.ErrUserRedirectNotExist{Name: username, MissingPermission: true})
+		assert.Zero(t, redirectID)
+	}
+
+	t.Run("Public visibility", func(t *testing.T) {
+		username := "olduser1"
+		redirectID := int64(1)
+
+		testOk(t, nil, username, redirectID)
+		testOk(t, normalUser, username, redirectID)
+		testOk(t, adminUser, username, redirectID)
+	})
+
+	t.Run("Limited visibility", func(t *testing.T) {
+		username := "oldorg22"
+		redirectID := int64(22)
+
+		testFail(t, nil, username)
+		testOk(t, normalUser, username, redirectID)
+		testOk(t, adminUser, username, redirectID)
+	})
+
+	t.Run("Private visibility", func(t *testing.T) {
+		orgUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
+		username := "oldorg23"
+		redirectID := int64(23)
+
+		testFail(t, nil, username)
+		testFail(t, normalUser, username)
+		testOk(t, orgUser, username, redirectID)
+		testOk(t, adminUser, username, redirectID)
+	})
+}

services/user/user_test.go

@@ -26,6 +26,7 @@ import (
 	"forgejo.org/modules/test"
 	"forgejo.org/modules/timeutil"
 	"forgejo.org/services/auth/source/oauth2"
+	redirect_service "forgejo.org/services/redirect"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -184,7 +185,7 @@ func TestRenameUser(t *testing.T) {
 		require.NoError(t, RenameUser(db.DefaultContext, user, newUsername))
 		unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID, Name: newUsername, LowerName: strings.ToLower(newUsername)})
 
-		redirectUID, err := user_model.LookupUserRedirect(db.DefaultContext, oldUsername)
+		redirectUID, err := redirect_service.LookupUserRedirect(db.DefaultContext, user, oldUsername)
 		require.NoError(t, err)
 		assert.Equal(t, user.ID, redirectUID)