From 83a5ae3db563d64b918abcc68354433a160886c6 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Wed, 26 Mar 2025 22:23:21 +0100 Subject: [PATCH 1/8] fix users being able bypass limits with repo transfers --- routers/api/v1/repo/transfer.go | 4 ++++ routers/web/repo/setting/setting.go | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 7b890c9e5cdff..f83ba4be8692c 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -68,6 +68,10 @@ func Transfer(ctx *context.APIContext) { return } + if !newOwner.CanCreateRepo() { + ctx.APIError(http.StatusForbidden, "The new owner cannot have more repositories") + } + if newOwner.Type == user_model.UserTypeOrganization { if !ctx.Doer.IsAdmin && newOwner.Visibility == api.VisibleTypePrivate && !organization.OrgFromUser(newOwner).HasMemberWithUserID(ctx, ctx.Doer.ID) { // The user shouldn't know about this organization diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index e8da443b67fa9..ac294a4359453 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -864,6 +864,12 @@ func handleSettingsPostTransfer(ctx *context.Context) { return } + if !newOwner.CanCreateRepo() { + limit := util.Iif(newOwner.MaxRepoCreation != -1, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit) + ctx.RenderWithErr(ctx.TrN(limit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", limit), tplSettingsOptions, nil) + return + } + if newOwner.Type == user_model.UserTypeOrganization { if !ctx.Doer.IsAdmin && newOwner.Visibility == structs.VisibleTypePrivate && !organization.OrgFromUser(newOwner).HasMemberWithUserID(ctx, ctx.Doer.ID) { // The user shouldn't know about this organization From 1476185306b0e6913bbfea34c3e626203c4f2c04 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Wed, 26 Mar 2025 23:13:52 +0100 Subject: [PATCH 2/8] add missing return --- routers/api/v1/repo/transfer.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index f83ba4be8692c..ab6ab603310b1 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -70,6 +70,7 @@ func Transfer(ctx *context.APIContext) { if !newOwner.CanCreateRepo() { ctx.APIError(http.StatusForbidden, "The new owner cannot have more repositories") + return } if newOwner.Type == user_model.UserTypeOrganization { From ef502632640da7253f1cb1fc963a6a0f4f03eefb Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Thu, 27 Mar 2025 22:53:34 +0100 Subject: [PATCH 3/8] fix comparison --- routers/web/repo/setting/setting.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index ac294a4359453..05ccb0420aa39 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -865,7 +865,7 @@ func handleSettingsPostTransfer(ctx *context.Context) { } if !newOwner.CanCreateRepo() { - limit := util.Iif(newOwner.MaxRepoCreation != -1, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit) + limit := util.Iif(newOwner.MaxRepoCreation >= 0, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit) ctx.RenderWithErr(ctx.TrN(limit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", limit), tplSettingsOptions, nil) return } From 67aa916baa953899af802df61e69957713ff3239 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Fri, 28 Mar 2025 22:35:57 +0100 Subject: [PATCH 4/8] move check to lower functions add admin exemption and custom error for the case --- routers/api/v1/repo/transfer.go | 12 +++++++----- routers/web/repo/repo.go | 10 +++++++--- routers/web/repo/setting/setting.go | 9 +++------ services/repository/transfer.go | 22 ++++++++++++++++++++++ 4 files changed, 39 insertions(+), 14 deletions(-) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index ab6ab603310b1..17e963ff2cb1c 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -68,11 +68,6 @@ func Transfer(ctx *context.APIContext) { return } - if !newOwner.CanCreateRepo() { - ctx.APIError(http.StatusForbidden, "The new owner cannot have more repositories") - return - } - if newOwner.Type == user_model.UserTypeOrganization { if !ctx.Doer.IsAdmin && newOwner.Visibility == api.VisibleTypePrivate && !organization.OrgFromUser(newOwner).HasMemberWithUserID(ctx, ctx.Doer.ID) { // The user shouldn't know about this organization @@ -123,6 +118,11 @@ func Transfer(ctx *context.APIContext) { return } + if repo_service.IsRepositoryLimitReached(err) { + ctx.APIError(http.StatusForbidden, err) + return + } + if errors.Is(err, user_model.ErrBlockedUser) { ctx.APIError(http.StatusForbidden, err) } else { @@ -174,6 +174,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) } diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 54b7448a8986b..5d0aa240121d8 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -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.RepositoryLimitReachedError).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) } } diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 05ccb0420aa39..bf9b7a24572d3 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -864,12 +864,6 @@ func handleSettingsPostTransfer(ctx *context.Context) { return } - if !newOwner.CanCreateRepo() { - limit := util.Iif(newOwner.MaxRepoCreation >= 0, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit) - ctx.RenderWithErr(ctx.TrN(limit, "repo.form.reach_limit_of_creation_1", "repo.form.reach_limit_of_creation_n", limit), tplSettingsOptions, nil) - return - } - if newOwner.Type == user_model.UserTypeOrganization { if !ctx.Doer.IsAdmin && newOwner.Visibility == structs.VisibleTypePrivate && !organization.OrgFromUser(newOwner).HasMemberWithUserID(ctx, ctx.Doer.ID) { // The user shouldn't know about this organization @@ -890,6 +884,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.RepositoryLimitReachedError).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 { diff --git a/services/repository/transfer.go b/services/repository/transfer.go index a589bc469d87f..8ad00604afc33 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -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 RepositoryLimitReachedError struct{ Limit int } + +func (RepositoryLimitReachedError) Error() string { + return "Repository limit has been reached" +} + +func IsRepositoryLimitReached(err error) bool { + _, ok := err.(RepositoryLimitReachedError) + return ok +} + func getRepoWorkingLockKey(repoID int64) string { return fmt.Sprintf("repo_working_%d", repoID) } @@ -42,6 +54,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 RepositoryLimitReachedError{Limit: limit} + } + oldOwnerName := repo.OwnerName if err := db.WithTx(ctx, func(ctx context.Context) error { @@ -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 RepositoryLimitReachedError{Limit: limit} + } + var isDirectTransfer bool oldOwnerName := repo.OwnerName From 15172172f4db218908804370f1d01955e1a6a2a8 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Fri, 28 Mar 2025 23:44:44 +0100 Subject: [PATCH 5/8] test draft --- services/repository/transfer.go | 10 +++---- services/repository/transfer_test.go | 42 +++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 8ad00604afc33..510907affea82 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -54,11 +54,6 @@ 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 RepositoryLimitReachedError{Limit: limit} - } - oldOwnerName := repo.OwnerName if err := db.WithTx(ctx, func(ctx context.Context) error { @@ -66,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 RepositoryLimitReachedError{Limit: limit} + } + if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { return util.ErrPermissionDenied } diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 16a8fb6e1eed2..52f503945249f 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -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 @@ -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 @@ -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: 3}) + + 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)) +} From 70174d761946555bcfc5ae95f50dc7aa15d3bc74 Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Sat, 29 Mar 2025 10:41:55 +0100 Subject: [PATCH 6/8] rename error to avoid stutter --- routers/web/repo/repo.go | 2 +- routers/web/repo/setting/setting.go | 2 +- services/repository/transfer.go | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index 5d0aa240121d8..e260ea36ddd76 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -309,7 +309,7 @@ func handleActionError(ctx *context.Context, err error) { case errors.Is(err, user_model.ErrBlockedUser): ctx.Flash.Error(ctx.Tr("repo.action.blocked_user")) case repo_service.IsRepositoryLimitReached(err): - limit := err.(repo_service.RepositoryLimitReachedError).Limit + 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) diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index bf9b7a24572d3..a0701ee9c1611 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -885,7 +885,7 @@ func handleSettingsPostTransfer(ctx *context.Context) { } 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.RepositoryLimitReachedError).Limit + 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) diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 510907affea82..4e44b90df205c 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -25,14 +25,14 @@ import ( notify_service "code.gitea.io/gitea/services/notify" ) -type RepositoryLimitReachedError struct{ Limit int } +type LimitReachedError struct{ Limit int } -func (RepositoryLimitReachedError) Error() string { +func (LimitReachedError) Error() string { return "Repository limit has been reached" } func IsRepositoryLimitReached(err error) bool { - _, ok := err.(RepositoryLimitReachedError) + _, ok := err.(LimitReachedError) return ok } @@ -63,7 +63,7 @@ func AcceptTransferOwnership(ctx context.Context, repo *repo_model.Repository, d if !doer.IsAdmin && !repoTransfer.Recipient.CanCreateRepo() { limit := util.Iif(repoTransfer.Recipient.MaxRepoCreation >= 0, repoTransfer.Recipient.MaxRepoCreation, setting.Repository.MaxCreationLimit) - return RepositoryLimitReachedError{Limit: limit} + return LimitReachedError{Limit: limit} } if !repoTransfer.CanUserAcceptOrRejectTransfer(ctx, doer) { @@ -418,7 +418,7 @@ func StartRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.Use if !doer.IsAdmin && !newOwner.CanCreateRepo() { limit := util.Iif(newOwner.MaxRepoCreation >= 0, newOwner.MaxRepoCreation, setting.Repository.MaxCreationLimit) - return RepositoryLimitReachedError{Limit: limit} + return LimitReachedError{Limit: limit} } var isDirectTransfer bool From ec7fb12e80ffb2b100c9ba7786a53ec4a246a62a Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Sat, 29 Mar 2025 12:10:36 +0100 Subject: [PATCH 7/8] give up and add fixture --- models/fixtures/repo_transfer.yml | 8 ++++++++ services/repository/transfer_test.go | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/models/fixtures/repo_transfer.yml b/models/fixtures/repo_transfer.yml index db92c952482c8..b12e6b207f4c5 100644 --- a/models/fixtures/repo_transfer.yml +++ b/models/fixtures/repo_transfer.yml @@ -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 diff --git a/services/repository/transfer_test.go b/services/repository/transfer_test.go index 52f503945249f..bf71c7ca2e30e 100644 --- a/services/repository/transfer_test.go +++ b/services/repository/transfer_test.go @@ -137,7 +137,7 @@ func TestRepositoryTransferRejection(t *testing.T) { // Admin case doerAdmin := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 5}) transfer, err := repo_model.GetPendingRepositoryTransfer(db.DefaultContext, repo) require.NoError(t, err) From 60817b20118b41d4f86b925cb1908661565c408d Mon Sep 17 00:00:00 2001 From: TheFox0x7 Date: Sat, 29 Mar 2025 21:22:09 +0100 Subject: [PATCH 8/8] port to switch --- routers/api/v1/repo/transfer.go | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/routers/api/v1/repo/transfer.go b/routers/api/v1/repo/transfer.go index 17e963ff2cb1c..8643d0c2cafbf 100644 --- a/routers/api/v1/repo/transfer.go +++ b/routers/api/v1/repo/transfer.go @@ -108,27 +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 repo_service.IsRepositoryLimitReached(err) { + case repo_service.IsRepositoryLimitReached(err): ctx.APIError(http.StatusForbidden, err) - return - } - - if errors.Is(err, user_model.ErrBlockedUser) { + case errors.Is(err, user_model.ErrBlockedUser): ctx.APIError(http.StatusForbidden, err) - } else { + default: ctx.APIErrorInternal(err) + return } - return } if ctx.Repo.Repository.Status == repo_model.RepositoryPendingTransfer {