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 <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

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
 }
 

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.

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
 	}

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
 	}

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)

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})