From a4d17117775e8a1a90c48faad607d6b1793b71ba Mon Sep 17 00:00:00 2001 From: sarankumar <120080287+sarankumar1325@users.noreply.github.com> Date: Sat, 31 Oct 2026 23:17:36 +0530 Subject: [PATCH] fix(api): remove git remote when deleting a push mirror (#40049) Fixes https://github.com/go-gitea/gitea/issues/40037 `DELETE /repos/{owner}/{repo}/push_mirrors/{name}` only deleted the `push_mirror` row and left the `remote_mirror_*` git remotes, which embed the mirror credentials, in the repository and wiki config. Each create/delete cycle through the API left more credentials behind. - Remove the git remotes before deleting the row and record it in the audit log, like the web settings handler. - Return 404 for an unknown name, as documented, instead of 204. - Ignore an already missing code remote, so the wiki remote is still removed and such mirrors can be deleted through both the API and the web settings. --- AI disclosure: I used an AI coding assistant to help write this change and the PR description. I reviewed the code and ran the tests myself. --------- Signed-off-by: sarankumar1325 <120080287+sarankumar1325@users.noreply.github.com> Co-authored-by: silverwind Co-authored-by: wxiaoguang --- models/repo/mirror.go | 6 +- models/repo/pushmirror.go | 91 ++++++++------------------- routers/api/v1/repo/mirror.go | 28 +++------ routers/web/repo/setting/setting.go | 38 ++++------- services/mirror/mirror_push.go | 32 +++++++++- tests/integration/mirror_push_test.go | 34 ++++++++++ 6 files changed, 116 insertions(+), 113 deletions(-) diff --git a/models/repo/mirror.go b/models/repo/mirror.go index 1985b49963f..403c2b26187 100644 --- a/models/repo/mirror.go +++ b/models/repo/mirror.go @@ -100,9 +100,9 @@ func TouchMirror(ctx context.Context, m *Mirror) error { return err } -// DeleteMirrorByRepoID deletes a mirror by repoID -func DeleteMirrorByRepoID(ctx context.Context, repoID int64) error { - _, err := db.GetEngine(ctx).Delete(&Mirror{RepoID: repoID}) +// DeleteRepoPullMirror deletes the repo's mirror (pull mirror) +func DeleteRepoPullMirror(ctx context.Context, repoID int64) error { + _, err := db.GetEngine(ctx).Where("repo_id=?", repoID).Delete(&Mirror{}) return err } diff --git a/models/repo/pushmirror.go b/models/repo/pushmirror.go index 8aea602e6a8..ffa5a93fb2b 100644 --- a/models/repo/pushmirror.go +++ b/models/repo/pushmirror.go @@ -9,7 +9,6 @@ import ( "gitea.dev/models/db" "gitea.dev/modules/log" - "gitea.dev/modules/optional" "gitea.dev/modules/timeutil" "gitea.dev/modules/util" @@ -31,31 +30,6 @@ type PushMirror struct { LastError string `xorm:"text"` } -type PushMirrorOptions struct { - db.ListOptions - ID int64 - RepoID int64 - RemoteName string -} - -func (opts PushMirrorOptions) ToOrders() string { - return "id" -} - -func (opts PushMirrorOptions) ToConds() builder.Cond { - cond := builder.NewCond() - if opts.RepoID > 0 { - cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) - } - if opts.RemoteName != "" { - cond = cond.And(builder.Eq{"remote_name": opts.RemoteName}) - } - if opts.ID > 0 { - cond = cond.And(builder.Eq{"id": opts.ID}) - } - return cond -} - func init() { db.RegisterModel(new(PushMirror)) } @@ -90,58 +64,47 @@ func UpdatePushMirrorInterval(ctx context.Context, m *PushMirror) error { return err } -func DeletePushMirrors(ctx context.Context, opts PushMirrorOptions) error { - if opts.RepoID > 0 { - _, err := db.Delete[PushMirror](ctx, opts) - return err +func GetRepoPushMirrorByRemoteName(ctx context.Context, repoID int64, name string) (*PushMirror, error) { + var m PushMirror + has, err := db.GetEngine(ctx).Where(builder.Eq{"repo_id": repoID, "remote_name": name}).Get(&m) + if err != nil { + return nil, err + } else if !has { + return nil, util.NewNotExistErrorf("PushMirror does not exist [repo_id: %d, remote_name: %s]", repoID, name) } - return util.NewInvalidArgumentErrorf("repoID required and must be set") + return &m, nil } -type findPushMirrorOptions struct { - db.ListOptions - RepoID int64 - SyncOnCommit optional.Option[bool] -} - -func (opts findPushMirrorOptions) ToOrders() string { - return "id" -} - -func (opts findPushMirrorOptions) ToConds() builder.Cond { - cond := builder.NewCond() - if opts.RepoID > 0 { - cond = cond.And(builder.Eq{"repo_id": opts.RepoID}) - } - if opts.SyncOnCommit.Has() { - cond = cond.And(builder.Eq{"sync_on_commit": opts.SyncOnCommit.Value()}) - } - return cond +func DeleteRepoPushMirrorByID(ctx context.Context, repoID, mirrorID int64) error { + _, err := db.GetEngine(ctx).Where("id = ?", mirrorID).And("repo_id = ?", repoID).Delete(new(PushMirror)) + return err } // GetPushMirrorsByRepoID returns push-mirror information of a repository. -func GetPushMirrorsByRepoID(ctx context.Context, repoID int64, listOptions db.ListOptions) ([]*PushMirror, int64, error) { - return db.FindAndCount[PushMirror](ctx, findPushMirrorOptions{ - ListOptions: listOptions, - RepoID: repoID, - }) +func GetPushMirrorsByRepoID(ctx context.Context, repoID int64, listOptions db.ListOptions) (mirrors []*PushMirror, count int64, err error) { + sess := db.GetEngine(ctx).Where("repo_id = ?", repoID).OrderBy("id") + if !listOptions.IsListAll() && listOptions.PageSize > 0 && listOptions.Page >= 1 { + sess.Limit(listOptions.PageSize, (listOptions.Page-1)*listOptions.PageSize) + } + count, err = sess.FindAndCount(&mirrors) + return mirrors, count, err } -func GetPushMirrorByIDAndRepoID(ctx context.Context, id, repoID int64) (*PushMirror, bool, error) { +func GetPushMirrorByRepoID(ctx context.Context, repoID, id int64) (*PushMirror, error) { var pushMirror PushMirror has, err := db.GetEngine(ctx).Where("id = ?", id).And("repo_id = ?", repoID).Get(&pushMirror) - if !has || err != nil { - return nil, has, err + if err != nil { + return nil, err + } else if !has { + return nil, util.NewNotExistErrorf("PushMirror does not exist [repo_id: %d, mirror: %d]", repoID, id) } - return &pushMirror, true, nil + return &pushMirror, nil } // GetPushMirrorsSyncedOnCommit returns push-mirrors for this repo that should be updated by new commits -func GetPushMirrorsSyncedOnCommit(ctx context.Context, repoID int64) ([]*PushMirror, error) { - return db.Find[PushMirror](ctx, findPushMirrorOptions{ - RepoID: repoID, - SyncOnCommit: optional.Some(true), - }) +func GetPushMirrorsSyncedOnCommit(ctx context.Context, repoID int64) (mirrors []*PushMirror, err error) { + err = db.GetEngine(ctx).Where("repo_id = ? AND sync_on_commit = ?", repoID, true).OrderBy("id").Find(&mirrors) + return mirrors, err } // PushMirrorsIterate iterates all push-mirror repositories. diff --git a/routers/api/v1/repo/mirror.go b/routers/api/v1/repo/mirror.go index 1834db8a8b1..a67ff1eda29 100644 --- a/routers/api/v1/repo/mirror.go +++ b/routers/api/v1/repo/mirror.go @@ -215,16 +215,9 @@ func GetPushMirrorByName(ctx *context.APIContext) { } mirrorName := ctx.PathParam("name") - // Get push mirror of a specific repo by remoteName - pushMirror, exist, err := db.Get[repo_model.PushMirror](ctx, repo_model.PushMirrorOptions{ - RepoID: ctx.Repo.Repository.ID, - RemoteName: mirrorName, - }.ToConds()) + pushMirror, err := repo_model.GetRepoPushMirrorByRemoteName(ctx, ctx.Repo.Repository.ID, mirrorName) if err != nil { - ctx.APIErrorInternal(err) - return - } else if !exist { - ctx.APIErrorNotFound() + ctx.APIErrorAuto(err) return } @@ -308,19 +301,22 @@ func DeletePushMirrorByRemoteName(ctx *context.APIContext) { // "$ref": "#/responses/empty" // default: // "$ref": "#/responses/error" - if !setting.Mirror.Enabled { ctx.APIError(http.StatusBadRequest, "Mirror feature is disabled") return } - remoteName := ctx.PathParam("name") - // Delete push mirror on repo by name. - err := repo_model.DeletePushMirrors(ctx, repo_model.PushMirrorOptions{RepoID: ctx.Repo.Repository.ID, RemoteName: remoteName}) + pushMirror, err := repo_model.GetRepoPushMirrorByRemoteName(ctx, ctx.Repo.Repository.ID, ctx.PathParam("name")) if err != nil { - ctx.APIError(http.StatusNotFound, err.Error()) + ctx.APIErrorAuto(err) return } + err = mirror_service.DeleteRepoPushMirrorByID(ctx, ctx.Repo.Repository.ID, pushMirror.ID) + if err != nil { + ctx.APIErrorAuto(err) + return + } + ctx.Status(http.StatusNoContent) } @@ -370,10 +366,6 @@ func CreatePushMirror(ctx *context.APIContext, mirrorOption *api.CreatePushMirro // if the registration of the push mirrorOption fails remove it from the database if err = mirror_service.AddPushMirrorRemote(ctx, pushMirror, address); err != nil { - if err := repo_model.DeletePushMirrors(ctx, repo_model.PushMirrorOptions{ID: pushMirror.ID, RepoID: pushMirror.RepoID}); err != nil { - ctx.APIErrorInternal(err) - return - } ctx.APIErrorInternal(err) return } diff --git a/routers/web/repo/setting/setting.go b/routers/web/repo/setting/setting.go index 51ac3191e45..0fd708ae121 100644 --- a/routers/web/repo/setting/setting.go +++ b/routers/web/repo/setting/setting.go @@ -398,9 +398,9 @@ func handleSettingsPostPushMirrorSync(ctx *context.Context) { return } - m, _, _ := repo_model.GetPushMirrorByIDAndRepoID(ctx, form.PushMirrorID, repo.ID) - if m == nil { - ctx.NotFound(nil) + m, err := repo_model.GetPushMirrorByRepoID(ctx, repo.ID, form.PushMirrorID) + if err != nil { + ctx.ServerError("GetPushMirrorByRepoID", err) return } @@ -429,9 +429,9 @@ func handleSettingsPostPushMirrorUpdate(ctx *context.Context) { return } - m, _, _ := repo_model.GetPushMirrorByIDAndRepoID(ctx, form.PushMirrorID, repo.ID) - if m == nil { - ctx.NotFound(nil) + m, err := repo_model.GetPushMirrorByRepoID(ctx, repo.ID, form.PushMirrorID) + if err != nil { + ctx.ServerError("GetPushMirrorByRepoID", err) return } @@ -461,28 +461,17 @@ func handleSettingsPostPushMirrorRemove(ctx *context.Context) { return } - // This section doesn't require repo_name/RepoName to be set in the form, don't show it - // as an error on the UI for this action - ctx.Data["Err_RepoName"] = nil - - m, _, _ := repo_model.GetPushMirrorByIDAndRepoID(ctx, form.PushMirrorID, repo.ID) - if m == nil { - ctx.NotFound(nil) + m, err := repo_model.GetPushMirrorByRepoID(ctx, repo.ID, form.PushMirrorID) + if err != nil { + ctx.ServerError("GetPushMirrorByRepoID", err) return } - if err := mirror_service.RemovePushMirrorRemote(ctx, m); err != nil { - ctx.ServerError("RemovePushMirrorRemote", err) + if err := mirror_service.DeleteRepoPushMirrorByID(ctx, repo.ID, m.ID); err != nil { + ctx.ServerError("DeleteRepoPushMirrorByID", err) return } - if err := repo_model.DeletePushMirrors(ctx, repo_model.PushMirrorOptions{ID: m.ID, RepoID: m.RepoID}); err != nil { - ctx.ServerError("DeletePushMirrorByID", err) - return - } - - audit.Record(ctx, audit_model.RepositoryMirrorPushRemove, repo, "mirror_id", m.ID, "remote_address", m.RemoteAddress) - ctx.Flash.Success(ctx.Tr("repo.settings.update_settings_success")) ctx.Redirect(repo.Link() + "/settings") } @@ -540,9 +529,6 @@ func handleSettingsPostPushMirrorAdd(ctx *context.Context) { } if err := mirror_service.AddPushMirrorRemote(ctx, m, address); err != nil { - if err := repo_model.DeletePushMirrors(ctx, repo_model.PushMirrorOptions{ID: m.ID, RepoID: m.RepoID}); err != nil { - log.Error("DeletePushMirrors %v", err) - } ctx.ServerError("AddPushMirrorRemote", err) return } @@ -810,7 +796,7 @@ func handleSettingsPostConvert(ctx *context.Context) { if _, err := repo_service.CleanUpMigrateInfo(ctx, repo); err != nil { ctx.ServerError("CleanUpMigrateInfo", err) return - } else if err = repo_model.DeleteMirrorByRepoID(ctx, ctx.Repo.Repository.ID); err != nil { + } else if err = repo_model.DeleteRepoPullMirror(ctx, ctx.Repo.Repository.ID); err != nil { ctx.ServerError("DeleteMirrorByRepoID", err) return } diff --git a/services/mirror/mirror_push.go b/services/mirror/mirror_push.go index f3431468f60..517ac728578 100644 --- a/services/mirror/mirror_push.go +++ b/services/mirror/mirror_push.go @@ -11,6 +11,7 @@ import ( "regexp" "time" + audit_model "gitea.dev/models/audit" "gitea.dev/models/db" repo_model "gitea.dev/models/repo" "gitea.dev/modules/git" @@ -22,6 +23,7 @@ import ( "gitea.dev/modules/setting" "gitea.dev/modules/timeutil" "gitea.dev/modules/util" + "gitea.dev/services/audit" "gitea.dev/services/migrations" repo_service "gitea.dev/services/repository" ) @@ -29,7 +31,14 @@ import ( var stripExitStatus = regexp.MustCompile(`exit status \d+ - `) // AddPushMirrorRemote registers the push mirror remote. -func AddPushMirrorRemote(ctx context.Context, m *repo_model.PushMirror, addr string) error { +func AddPushMirrorRemote(ctx context.Context, m *repo_model.PushMirror, addr string) (retErr error) { + defer func() { + if retErr != nil { + if err := repo_model.DeleteRepoPushMirrorByID(ctx, m.RepoID, m.ID); err != nil { + log.Error("DeletePushMirrorByID %v", err) + } + } + }() addRemoteAndConfig := func(storageRepo git.RepositoryFacade, addr string) error { if err := git.ManagedRemoteAdd(ctx, storageRepo, m.RemoteName, addr, git.RemoteOptionMirrorPush); err != nil { return err @@ -59,7 +68,7 @@ func AddPushMirrorRemote(ctx context.Context, m *repo_model.PushMirror, addr str // RemovePushMirrorRemote removes the push mirror remote. func RemovePushMirrorRemote(ctx context.Context, m *repo_model.PushMirror) error { _ = m.GetRepository(ctx) - if err := git.ManagedRemoteRemove(ctx, m.Repo, m.RemoteName); err != nil { + if err := git.ManagedRemoteRemove(ctx, m.Repo, m.RemoteName); err != nil && !git.IsRemoteNotExistError(err) { return err } @@ -73,6 +82,25 @@ func RemovePushMirrorRemote(ctx context.Context, m *repo_model.PushMirror) error return nil } +// DeleteRepoPushMirrorByID removes the push mirror's git remotes, deletes the push mirror record +// and records the audit event. The web UI and the API share it. +func DeleteRepoPushMirrorByID(ctx context.Context, repoID, pushMirrorID int64) error { + pushMirror, err := repo_model.GetPushMirrorByRepoID(ctx, repoID, pushMirrorID) + if err != nil { + return err + } + if err := RemovePushMirrorRemote(ctx, pushMirror); err != nil { + return fmt.Errorf("RemovePushMirrorRemote: %w", err) + } + + if err := repo_model.DeleteRepoPushMirrorByID(ctx, pushMirror.RepoID, pushMirror.ID); err != nil { + return fmt.Errorf("DeletePushMirrors: %w", err) + } + + audit.Record(ctx, audit_model.RepositoryMirrorPushRemove, pushMirror.GetRepository(ctx), "mirror_id", pushMirror.ID, "remote_address", pushMirror.RemoteAddress) + return nil +} + // SyncPushMirror starts the sync of the push mirror and schedules the next run. func SyncPushMirror(ctx context.Context, mirrorID int64) bool { log.Trace("SyncPushMirror [mirror: %d]", mirrorID) diff --git a/tests/integration/mirror_push_test.go b/tests/integration/mirror_push_test.go index 88f1a626bd3..04ee0b0fb00 100644 --- a/tests/integration/mirror_push_test.go +++ b/tests/integration/mirror_push_test.go @@ -11,18 +11,21 @@ import ( "testing" "time" + auth_model "gitea.dev/models/auth" "gitea.dev/models/db" repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" "gitea.dev/modules/git" "gitea.dev/modules/setting" + api "gitea.dev/modules/structs" "gitea.dev/modules/test" mirror_service "gitea.dev/services/mirror" repo_service "gitea.dev/services/repository" wiki_service "gitea.dev/services/wiki" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestMirrorPush(t *testing.T) { @@ -56,6 +59,9 @@ func TestMirrorPush(t *testing.T) { assert.True(t, doRemovePushMirror(t, session, "user2", "repo2", repo2PushMirrorID)) unittest.AssertNotExistsBean(t, &repo_model.PushMirror{ID: repo2PushMirrorID}) }) + t.Run("APIDelete", func(t *testing.T) { + testMirrorPushAPIDelete(t, serverURL) + }) t.Run("Push", func(t *testing.T) { testMirrorPush(t, serverURL) }) @@ -65,6 +71,34 @@ func TestMirrorPush(t *testing.T) { }) } +func testMirrorPushAPIDelete(t *testing.T, u *url.URL) { + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository) + createPushMirror := func() string { + req := NewRequestWithJSON(t, "POST", "/api/v1/repos/user2/repo1/push_mirrors", &api.CreatePushMirrorOption{ + RemoteAddress: u.String() + "user2/repo1.git", + RemoteUsername: "user2", + RemotePassword: userPassword, + Interval: "0", + }).AddTokenAuth(token) + return DecodeJSON(t, MakeRequest(t, req, http.StatusOK), &api.PushMirror{}).RemoteName + } + + remoteName := createPushMirror() + MakeRequest(t, NewRequest(t, "DELETE", "/api/v1/repos/user2/repo1/push_mirrors/"+remoteName).AddTokenAuth(token), http.StatusNoContent) + _, err := git.GetRemoteAddress(t.Context(), repo1, remoteName) + assert.True(t, git.IsRemoteNotExistError(err)) + MakeRequest(t, NewRequest(t, "DELETE", "/api/v1/repos/user2/repo1/push_mirrors/"+remoteName).AddTokenAuth(token), http.StatusNotFound) + + remoteName = createPushMirror() + _, err = git.GetRemoteAddress(t.Context(), repo1.WikiStorageRepo(), remoteName) + require.NoError(t, err) + require.NoError(t, git.ManagedRemoteRemove(t.Context(), repo1, remoteName)) + MakeRequest(t, NewRequest(t, "DELETE", "/api/v1/repos/user2/repo1/push_mirrors/"+remoteName).AddTokenAuth(token), http.StatusNoContent) + _, err = git.GetRemoteAddress(t.Context(), repo1.WikiStorageRepo(), remoteName) + assert.True(t, git.IsRemoteNotExistError(err)) +} + func testMirrorPush(t *testing.T, u *url.URL) { _ = db.TruncateBeans(t.Context(), &repo_model.PushMirror{}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})