Handle more pathological branch and tag names (#11843) (#11863)

Backport #11843

It's possible to push quite pathological appearing branch names to gitea
using git push gitea reasonable-branch:refs/heads/-- at which point
large parts of the UI will break. Similarly you can git push origin
reasonable-tag:refs/tags/-- which wil return an error.

This PR fixes the problems these cause. It also changes the code from
creating branches to pushing to ensure that branch restoration has to
pass hooks.

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>

7 files changed, 41 insertions, 87 deletions

diff --git a/integrations/branches_test.go b/integrations/branches_test.go
index ed6a73fdb4..2b9fc8dda5 100644
--- a/integrations/branches_test.go
+++ b/integrations/branches_test.go
@@ -32,14 +32,14 @@ func TestDeleteBranch(t *testing.T) {
 }
 
 func TestUndoDeleteBranch(t *testing.T) {
-	defer prepareTestEnv(t)()
-
-	deleteBranch(t)
-	htmlDoc, name := branchAction(t, ".undo-button")
-	assert.Contains(t,
-		htmlDoc.doc.Find(".ui.positive.message").Text(),
-		i18n.Tr("en", "repo.branch.restore_success", name),
-	)
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		deleteBranch(t)
+		htmlDoc, name := branchAction(t, ".undo-button")
+		assert.Contains(t,
+			htmlDoc.doc.Find(".ui.positive.message").Text(),
+			i18n.Tr("en", "repo.branch.restore_success", name),
+		)
+	})
 }
 
 func deleteBranch(t *testing.T) {
diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go
index c5f6d6cdd6..18c732c6f2 100644
--- a/modules/git/repo_commit.go
+++ b/modules/git/repo_commit.go
@@ -46,7 +46,7 @@ func (repo *Repository) GetBranchCommitID(name string) (string, error) {
 
 // GetTagCommitID returns last commit ID string of given tag.
 func (repo *Repository) GetTagCommitID(name string) (string, error) {
-	stdout, err := NewCommand("rev-list", "-n", "1", name).RunInDir(repo.Path)
+	stdout, err := NewCommand("rev-list", "-n", "1", TagPrefix+name).RunInDir(repo.Path)
 	if err != nil {
 		if strings.Contains(err.Error(), "unknown revision or path") {
 			return "", ErrNotExist{name, ""}
diff --git a/modules/repository/branch.go b/modules/repository/branch.go
index 418ba25c89..358e59f39f 100644
--- a/modules/repository/branch.go
+++ b/modules/repository/branch.go
@@ -9,7 +9,6 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/git"
-	"code.gitea.io/gitea/modules/log"
 )
 
 // GetBranch returns a branch by its name
@@ -74,39 +73,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName,
 		return fmt.Errorf("OldBranch: %s does not exist. Cannot create new branch from this", oldBranchName)
 	}
 
-	basePath, err := models.CreateTemporaryPath("branch-maker")
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if err := models.RemoveTemporaryPath(basePath); err != nil {
-			log.Error("CreateNewBranch: RemoveTemporaryPath: %s", err)
-		}
-	}()
-
-	if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
-		Bare:   true,
-		Shared: true,
-	}); err != nil {
-		log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
-		return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
-	}
-
-	gitRepo, err := git.OpenRepository(basePath)
-	if err != nil {
-		log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
-		return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
-	}
-	defer gitRepo.Close()
-
-	if err = gitRepo.CreateBranch(branchName, oldBranchName); err != nil {
-		log.Error("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
-		return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, oldBranchName, err)
-	}
-
-	if err = git.Push(basePath, git.PushOptions{
-		Remote: "origin",
-		Branch: branchName,
+	if err := git.Push(repo.RepoPath(), git.PushOptions{
+		Remote: repo.RepoPath(),
+		Branch: fmt.Sprintf("%s:%s%s", oldBranchName, git.BranchPrefix, branchName),
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
 		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
@@ -124,39 +93,10 @@ func CreateNewBranchFromCommit(doer *models.User, repo *models.Repository, commi
 	if err := checkBranchName(repo, branchName); err != nil {
 		return err
 	}
-	basePath, err := models.CreateTemporaryPath("branch-maker")
-	if err != nil {
-		return err
-	}
-	defer func() {
-		if err := models.RemoveTemporaryPath(basePath); err != nil {
-			log.Error("CreateNewBranchFromCommit: RemoveTemporaryPath: %s", err)
-		}
-	}()
-
-	if err := git.Clone(repo.RepoPath(), basePath, git.CloneRepoOptions{
-		Bare:   true,
-		Shared: true,
-	}); err != nil {
-		log.Error("Failed to clone repository: %s (%v)", repo.FullName(), err)
-		return fmt.Errorf("Failed to clone repository: %s (%v)", repo.FullName(), err)
-	}
-
-	gitRepo, err := git.OpenRepository(basePath)
-	if err != nil {
-		log.Error("Unable to open temporary repository: %s (%v)", basePath, err)
-		return fmt.Errorf("Failed to open new temporary repository in: %s %v", basePath, err)
-	}
-	defer gitRepo.Close()
-
-	if err = gitRepo.CreateBranch(branchName, commit); err != nil {
-		log.Error("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
-		return fmt.Errorf("Unable to create branch: %s from %s. (%v)", branchName, commit, err)
-	}
 
-	if err = git.Push(basePath, git.PushOptions{
-		Remote: "origin",
-		Branch: branchName,
+	if err := git.Push(repo.RepoPath(), git.PushOptions{
+		Remote: repo.RepoPath(),
+		Branch: fmt.Sprintf("%s:%s%s", commit, git.BranchPrefix, branchName),
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
 		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
diff --git a/routers/repo/branch.go b/routers/repo/branch.go
index e7eac04bce..4d8b9158fe 100644
--- a/routers/repo/branch.go
+++ b/routers/repo/branch.go
@@ -6,6 +6,7 @@
 package repo
 
 import (
+	"fmt"
 	"strings"
 
 	"code.gitea.io/gitea/models"
@@ -102,7 +103,11 @@ func RestoreBranchPost(ctx *context.Context) {
 		return
 	}
 
-	if err := ctx.Repo.GitRepo.CreateBranch(deletedBranch.Name, deletedBranch.Commit); err != nil {
+	if err := git.Push(ctx.Repo.Repository.RepoPath(), git.PushOptions{
+		Remote: ctx.Repo.Repository.RepoPath(),
+		Branch: fmt.Sprintf("%s:%s%s", deletedBranch.Commit, git.BranchPrefix, deletedBranch.Name),
+		Env:    models.PushingEnvironment(ctx.User, ctx.Repo.Repository),
+	}); err != nil {
 		if strings.Contains(err.Error(), "already exists") {
 			ctx.Flash.Error(ctx.Tr("repo.branch.already_exists", deletedBranch.Name))
 			return
@@ -112,12 +117,6 @@ func RestoreBranchPost(ctx *context.Context) {
 		return
 	}
 
-	if err := ctx.Repo.Repository.RemoveDeletedBranch(deletedBranch.ID); err != nil {
-		log.Error("RemoveDeletedBranch: %v", err)
-		ctx.Flash.Error(ctx.Tr("repo.branch.restore_failed", deletedBranch.Name))
-		return
-	}
-
 	// Don't return error below this
 	if err := repofiles.PushUpdate(
 		ctx.Repo.Repository,
@@ -216,7 +215,7 @@ func loadBranches(ctx *context.Context) []*Branch {
 			}
 		}
 
-		divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, branchName)
+		divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, git.BranchPrefix+branchName)
 		if divergenceError != nil {
 			ctx.ServerError("CountDivergingCommits", divergenceError)
 			return nil
@@ -331,6 +330,8 @@ func CreateBranch(ctx *context.Context, form auth.NewBranchForm) {
 	var err error
 	if ctx.Repo.IsViewBranch {
 		err = repo_module.CreateNewBranch(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
+	} else if ctx.Repo.IsViewTag {
+		err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.CommitID, form.NewBranchName)
 	} else {
 		err = repo_module.CreateNewBranchFromCommit(ctx.User, ctx.Repo.Repository, ctx.Repo.BranchName, form.NewBranchName)
 	}
diff --git a/routers/repo/compare.go b/routers/repo/compare.go
index 97bb5e6b18..3ffabc14a1 100644
--- a/routers/repo/compare.go
+++ b/routers/repo/compare.go
@@ -381,7 +381,20 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 		return nil, nil, nil, nil, "", ""
 	}
 
-	compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranch, headBranch)
+	baseBranchRef := baseBranch
+	if baseIsBranch {
+		baseBranchRef = git.BranchPrefix + baseBranch
+	} else if baseIsTag {
+		baseBranchRef = git.TagPrefix + baseBranch
+	}
+	headBranchRef := headBranch
+	if headIsBranch {
+		headBranchRef = git.BranchPrefix + headBranch
+	} else if headIsTag {
+		headBranchRef = git.TagPrefix + headBranch
+	}
+
+	compareInfo, err := headGitRepo.GetCompareInfo(baseRepo.RepoPath(), baseBranchRef, headBranchRef)
 	if err != nil {
 		ctx.ServerError("GetCompareInfo", err)
 		return nil, nil, nil, nil, "", ""
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index ec82d14f18..da826026b3 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -484,7 +484,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
 	}
 
 	compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
-		pull.BaseBranch, pull.GetGitRefName())
+		git.BranchPrefix+pull.BaseBranch, pull.GetGitRefName())
 	if err != nil {
 		if strings.Contains(err.Error(), "fatal: Not a valid object name") {
 			ctx.Data["IsPullRequestBroken"] = true
diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go
index 91f48d0626..45cd10b65b 100644
--- a/services/pull/temp_repo.go
+++ b/services/pull/temp_repo.go
@@ -100,7 +100,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
 	outbuf.Reset()
 	errbuf.Reset()
 
-	if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
+	if err := git.NewCommand("fetch", "origin", "--no-tags", "--", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
 		log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String())
 		if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
 			log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)
@@ -140,7 +140,7 @@ func createTemporaryRepo(pr *models.PullRequest) (string, error) {
 
 	trackingBranch := "tracking"
 	// Fetch head branch
-	if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
+	if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, git.BranchPrefix+pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil {
 		log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String())
 		if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
 			log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err)