Skip to content

fix users being able bypass limits with repo transfers #34031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Mar 31, 2025
8 changes: 8 additions & 0 deletions models/fixtures/repo_transfer.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,11 @@
repo_id: 32
created_unix: 1553610671
updated_unix: 1553610671

-
id: 4
doer_id: 3
recipient_id: 1
repo_id: 5
created_unix: 1553610671
updated_unix: 1553610671
21 changes: 10 additions & 11 deletions routers/api/v1/repo/transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,22 +108,19 @@ func Transfer(ctx *context.APIContext) {
oldFullname := ctx.Repo.Repository.FullName()

if err := repo_service.StartRepositoryTransfer(ctx, ctx.Doer, newOwner, ctx.Repo.Repository, teams); err != nil {
if repo_model.IsErrRepoTransferInProgress(err) {
switch {
case repo_model.IsErrRepoTransferInProgress(err):
ctx.APIError(http.StatusConflict, err)
return
}

if repo_model.IsErrRepoAlreadyExist(err) {
case repo_model.IsErrRepoAlreadyExist(err):
ctx.APIError(http.StatusUnprocessableEntity, err)
return
}

if errors.Is(err, user_model.ErrBlockedUser) {
case repo_service.IsRepositoryLimitReached(err):
ctx.APIError(http.StatusForbidden, err)
case errors.Is(err, user_model.ErrBlockedUser):
ctx.APIError(http.StatusForbidden, err)
} else {
default:
ctx.APIErrorInternal(err)
return
Comment on lines +111 to +122
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These case branches lose return?

}
return
}

if ctx.Repo.Repository.Status == repo_model.RepositoryPendingTransfer {
Expand Down Expand Up @@ -169,6 +166,8 @@ func AcceptTransfer(ctx *context.APIContext) {
ctx.APIError(http.StatusNotFound, err)
case errors.Is(err, util.ErrPermissionDenied):
ctx.APIError(http.StatusForbidden, err)
case repo_service.IsRepositoryLimitReached(err):
ctx.APIError(http.StatusForbidden, err)
default:
ctx.APIErrorInternal(err)
}
Expand Down
10 changes: 7 additions & 3 deletions routers/web/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,15 @@ func CreatePost(ctx *context.Context) {
}

func handleActionError(ctx *context.Context, err error) {
if errors.Is(err, user_model.ErrBlockedUser) {
switch {
case errors.Is(err, user_model.ErrBlockedUser):
ctx.Flash.Error(ctx.Tr("repo.action.blocked_user"))
} else if errors.Is(err, util.ErrPermissionDenied) {
case repo_service.IsRepositoryLimitReached(err):
limit := err.(repo_service.LimitReachedError).Limit
ctx.Flash.Error(ctx.TrN(limit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", limit))
case errors.Is(err, util.ErrPermissionDenied):
ctx.HTTPError(http.StatusNotFound)
} else {
default:
ctx.ServerError(fmt.Sprintf("Action (%s)", ctx.PathParam("action")), err)
}
}
Expand Down
3 changes: 3 additions & 0 deletions routers/web/repo/setting/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,9 @@ func handleSettingsPostTransfer(ctx *context.Context) {
ctx.RenderWithErr(ctx.Tr("repo.settings.new_owner_has_same_repo"), tplSettingsOptions, nil)
} else if repo_model.IsErrRepoTransferInProgress(err) {
ctx.RenderWithErr(ctx.Tr("repo.settings.transfer_in_progress"), tplSettingsOptions, nil)
} else if repo_service.IsRepositoryLimitReached(err) {
limit := err.(repo_service.LimitReachedError).Limit
ctx.RenderWithErr(ctx.TrN(limit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", limit), tplSettingsOptions, nil)
} else if errors.Is(err, user_model.ErrBlockedUser) {
ctx.RenderWithErr(ctx.Tr("repo.settings.transfer.blocked_user"), tplSettingsOptions, nil)
} else {
Expand Down
22 changes: 22 additions & 0 deletions services/repository/transfer.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,22 @@ import (
"code.gitea.io/gitea/modules/gitrepo"
"code.gitea.io/gitea/modules/globallock"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
notify_service "code.gitea.io/gitea/services/notify"
)

type LimitReachedError struct{ Limit int }

func (LimitReachedError) Error() string {
return "Repository limit has been reached"
}

func IsRepositoryLimitReached(err error) bool {
_, ok := err.(LimitReachedError)
return ok
}

func getRepoWorkingLockKey(repoID int64) string {
return fmt.Sprintf("repo_working_%d", repoID)
}
Expand All @@ -49,6 +61,11 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d
return err
}

if !doer.IsAdmin && !repoTransfer.Recipient.CanCreateRepo() {
limit := util.Iif(repoTransfer.Recipient.MaxRepoCreation >= 0, repoTransfer.Recipient.MaxRepoCreation, setting.Repository.MaxCreationLimit)
return LimitReachedError{Limit: limit}
}

if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) {
return util.ErrPermissionDenied
}
Expand Down Expand Up @@ -399,6 +416,11 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use
return err
}

if !doer.IsAdmin && !newOwner.CanCreateRepo() {
limit := util.Iif(newOwner.MaxRepoCreation >= 0, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit)
return LimitReachedError{Limit: limit}
}

var isDirectTransfer bool
oldOwnerName := repo.OwnerName

Expand Down
42 changes: 41 additions & 1 deletion services/repository/transfer_test.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package repository
Expand All @@ -14,11 +14,14 @@ import (
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/services/feed"
notify_service "code.gitea.io/gitea/services/notify"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

var notifySync sync.Once
Expand Down Expand Up @@ -125,3 +128,40 @@ func TestRepositoryTransfer(t *testing.T) {
err = RejectRepositoryTransfer(db.DefaultContext, repo2, doer)
assert.True(t, repo_model.IsErrNoPendingTransfer(err))
}

// Test transfer rejections
func TestRepositoryTransferRejection(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
// Set limit to 0 repositories so no repositories can be transferred
defer test.MockVariableValue(&setting.Repository.MaxCreationLimit, 0)()

// Admin case
doerAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 5})

transfer, err := repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo)
require.NoError(t, err)
require.NotNil(t, transfer)
require.NoError(t, transfer.LoadRecipient(db.DefaultContext))

require.True(t, transfer.Recipient.CanCreateRepo()) // admin is not subject to limits

// Administrator should not be affected by the limits so transfer should be successful
assert.NoError(t, AcceptTransferOwnership(db.DefaultContext, repo, doerAdmin))

// Non admin user case
doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 10})
repo = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 21})

transfer, err = repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo)
require.NoError(t, err)
require.NotNil(t, transfer)
require.NoError(t, transfer.LoadRecipient(db.DefaultContext))

require.False(t, transfer.Recipient.CanCreateRepo()) // regular user is subject to limits

// Cannot accept because of the limit
err = AcceptTransferOwnership(db.DefaultContext, repo, doer)
assert.Error(t, err)
assert.True(t, IsRepositoryLimitReached(err))
}