Avoid losing token when updating mirror settings (#30429)

modules/templates/util_misc.go

@@ -142,35 +142,39 @@ type remoteAddress struct {
 	Password string
 }
 
-func mirrorRemoteAddress(ctx context.Context, m *repo_model.Repository, remoteName string, ignoreOriginalURL bool) remoteAddress {
-	a := remoteAddress{}
-
-	remoteURL := m.OriginalURL
-	if ignoreOriginalURL || remoteURL == "" {
-		var err error
-		remoteURL, err = git.GetRemoteAddress(ctx, m.RepoPath(), remoteName)
-		if err != nil {
-			log.Error("GetRemoteURL %v", err)
-			return a
-		}
+func mirrorRemoteAddress(ctx context.Context, m *repo_model.Repository, remoteName string) remoteAddress {
+	ret := remoteAddress{}
+	remoteURL, err := git.GetRemoteAddress(ctx, m.RepoPath(), remoteName)
+	if err != nil {
+		log.Error("GetRemoteURL %v", err)
+		return ret
 	}
 
 	u, err := giturl.Parse(remoteURL)
 	if err != nil {
 		log.Error("giturl.Parse %v", err)
-		return a
+		return ret
 	}
 
 	if u.Scheme != "ssh" && u.Scheme != "file" {
 		if u.User != nil {
-			a.Username = u.User.Username()
-			a.Password, _ = u.User.Password()
+			ret.Username = u.User.Username()
+			ret.Password, _ = u.User.Password()
 		}
-		u.User = nil
 	}
-	a.Address = u.String()
 
-	return a
+	// The URL stored in the git repo could contain authentication,
+	// erase it, or it will be shown in the UI.
+	u.User = nil
+	ret.Address = u.String()
+	// Why not use m.OriginalURL to set ret.Address?
+	// It should be OK to use it, since m.OriginalURL should be the same as the authentication-erased URL from the Git repository.
+	// However, the old code has already stored authentication in m.OriginalURL when updating mirror settings.
+	// That means we need to use "giturl.Parse" for m.OriginalURL again to ensure authentication is erased.
+	// Instead of doing this, why not directly use the authentication-erased URL from the Git repository?
+	// It should be the same as long as there are no bugs.
+
+	return ret
 }
 
 func FilenameIsImage(filename string) bool {

services/mirror/mirror_pull.go

@@ -13,6 +13,7 @@ import (
 	system_model "code.gitea.io/gitea/models/system"
 	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/git"
+	giturl "code.gitea.io/gitea/modules/git/url"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/log"
@@ -30,10 +31,15 @@ const gitShortEmptySha = "0000000"
 
 // UpdateAddress writes new address to Git repository and database
 func UpdateAddress(ctx context.Context, m *repo_model.Mirror, addr string) error {
+	u, err := giturl.Parse(addr)
+	if err != nil {
+		return fmt.Errorf("invalid addr: %v", err)
+	}
+
 	remoteName := m.GetRemoteName()
 	repoPath := m.GetRepository(ctx).RepoPath()
 	// Remove old remote
-	_, _, err := git.NewCommand(ctx, "remote", "rm").AddDynamicArguments(remoteName).RunStdString(&git.RunOpts{Dir: repoPath})
+	_, _, err = git.NewCommand(ctx, "remote", "rm").AddDynamicArguments(remoteName).RunStdString(&git.RunOpts{Dir: repoPath})
 	if err != nil && !strings.HasPrefix(err.Error(), "exit status 128 - fatal: No such remote ") {
 		return err
 	}
@@ -70,7 +76,9 @@ func UpdateAddress(ctx context.Context, m *repo_model.Mirror, addr string) error
 		}
 	}
 
-	m.Repo.OriginalURL = addr
+	// erase authentication before storing in database
+	u.User = nil
+	m.Repo.OriginalURL = u.String()
 	return repo_model.UpdateRepositoryCols(ctx, m.Repo, "original_url")
 }
 

templates/repo/settings/options.tmpl

@@ -156,7 +156,7 @@
 											<label for="interval">{{ctx.Locale.Tr "repo.mirror_interval" .MinimumMirrorInterval}}</label>
 											<input id="interval" name="interval" value="{{.PullMirror.Interval}}">
 										</div>
-										{{$address := MirrorRemoteAddress $.Context .Repository .PullMirror.GetRemoteName false}}
+										{{$address := MirrorRemoteAddress $.Context .Repository .PullMirror.GetRemoteName}}
 										<div class="field {{if .Err_MirrorAddress}}error{{end}}">
 											<label for="mirror_address">{{ctx.Locale.Tr "repo.mirror_address"}}</label>
 											<input id="mirror_address" name="mirror_address" value="{{$address.Address}}" required>