From fd59cd9450cbd511ad4a0790bf51f8d5d2c18aa3 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sat, 13 Apr 2024 23:41:57 +0800 Subject: [PATCH] Avoid losing token when updating mirror settings (#30429) Fix #30416. Before (it shows as "Unset" while there's a token): image After: image The username shows as "oauth2" because of https://github.com/go-gitea/gitea/blob/f9fdac9809335729b2ac3227b2a5f71a62fc64ad/services/migrations/dump.go#L99 I have checked that all usage of `MirrorRemoteAddress` has been updated. image However, it needs to be checked again when backporting. --------- Co-authored-by: Giteabot --- modules/templates/util_misc.go | 38 +++++++++++++++------------- services/mirror/mirror_pull.go | 12 +++++++-- templates/repo/settings/options.tmpl | 2 +- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/modules/templates/util_misc.go b/modules/templates/util_misc.go index 6c1b4ab24..774385483 100644 --- a/modules/templates/util_misc.go +++ b/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 { diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index 21d5f0820..fa23986c5 100644 --- a/services/mirror/mirror_pull.go +++ b/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") } diff --git a/templates/repo/settings/options.tmpl b/templates/repo/settings/options.tmpl index b8fa4759b..df6ccbf6b 100644 --- a/templates/repo/settings/options.tmpl +++ b/templates/repo/settings/options.tmpl @@ -156,7 +156,7 @@ - {{$address := MirrorRemoteAddress $.Context .Repository .PullMirror.GetRemoteName false}} + {{$address := MirrorRemoteAddress $.Context .Repository .PullMirror.GetRemoteName}}