feat: Use receive.hideRefs (#10015)

In forgejo/forgejo!2834 and forgejo/forgejo!5307 it was made so it's no longer possible to modify and delete internal reference, not having this restriction lead to broken pull requests when people used something like `git push --mirror`. However it now still leads to problem with that command as the git client tries to delete such references. We can solve this by using git's `receive.hideRefs` to make this ref read-only and avoid advertising it when someone does `git push --mirror`.

Resolves forgejo/forgejo#9942

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10015
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: Gusted <postmaster@gusted.xyz>
Co-committed-by: Gusted <postmaster@gusted.xyz>

.deadcode-out

@@ -108,6 +108,7 @@ forgejo.org/modules/git
 	AddChangesWithArgs
 	CommitChanges
 	CommitChangesWithArgs
+	IsErrMoreThanOne
 	SetUpdateHook
 	openRepositoryWithDefaultContext
 	ToEntryMode

cmd/hook.go

@@ -295,35 +295,7 @@ Forgejo or set your environment appropriately.`, "")
 
 // runHookUpdate process the update hook: https://git-scm.com/docs/githooks#update
 func runHookUpdate(ctx context.Context, c *cli.Command) error {
-	// Now if we're an internal don't do anything else
-	if isInternal, _ := strconv.ParseBool(os.Getenv(repo_module.EnvIsInternal)); isInternal {
-		return nil
-	}
-
-	ctx, cancel := installSignals(ctx)
-	defer cancel()
-
-	if c.NArg() != 3 {
-		return nil
-	}
-	args := c.Args().Slice()
-
-	// The arguments given to the hook are in order: reference name, old commit ID and new commit ID.
-	refFullName := git.RefName(args[0])
-	newCommitID := args[2]
-
-	// Only process pull references.
-	if !refFullName.IsPull() {
-		return nil
-	}
-
-	// Empty new commit ID means deletion.
-	if git.IsEmptyCommitID(newCommitID, nil) {
-		return fail(ctx, fmt.Sprintf("The deletion of %s is skipped as it's an internal reference.", refFullName), "")
-	}
-
-	// If the new comment isn't empty it means modification.
-	return fail(ctx, fmt.Sprintf("The modification of %s is skipped as it's an internal reference.", refFullName), "")
+	return nil
 }
 
 func runHookPostReceive(ctx context.Context, c *cli.Command) error {

cmd/hook_test.go

@@ -161,42 +161,3 @@ func TestDelayWriter(t *testing.T) {
 		require.Empty(t, out)
 	})
 }
-
-func TestRunHookUpdate(t *testing.T) {
-	app := cli.Command{}
-	app.Commands = []*cli.Command{subcmdHookUpdate()}
-
-	t.Run("Removal of internal reference", func(t *testing.T) {
-		defer test.MockVariableValue(&cli.OsExiter, func(code int) {})()
-		defer test.MockVariableValue(&setting.IsProd, false)()
-		finish := captureOutput(t, os.Stderr)
-
-		err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/pull/1/head", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000000"})
-		out := finish()
-		require.Error(t, err)
-
-		assert.Contains(t, out, "The deletion of refs/pull/1/head is skipped as it's an internal reference.")
-	})
-
-	t.Run("Update of internal reference", func(t *testing.T) {
-		defer test.MockVariableValue(&cli.OsExiter, func(code int) {})()
-		defer test.MockVariableValue(&setting.IsProd, false)()
-		finish := captureOutput(t, os.Stderr)
-
-		err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/pull/1/head", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000001"})
-		out := finish()
-		require.Error(t, err)
-
-		assert.Contains(t, out, "The modification of refs/pull/1/head is skipped as it's an internal reference.")
-	})
-
-	t.Run("Removal of branch", func(t *testing.T) {
-		err := app.Run(t.Context(), []string{"./forgejo", "update", "refs/head/main", "0a51ae26bc73c47e2f754560c40904cf14ed51a9", "0000000000000000000000000000000000000000"})
-		require.NoError(t, err)
-	})
-
-	t.Run("Not enough arguments", func(t *testing.T) {
-		err := app.Run(t.Context(), []string{"./forgejo", "update"})
-		require.NoError(t, err)
-	})
-}

modules/git/fetch.go

@@ -0,0 +1,51 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+package git
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+)
+
+var ErrRemoteRefNotFound = errors.New("unable to find remote ref")
+
+// Fetch executes git-fetch(1) for the repository, it will fetch the refspec
+// from the remote into the repository.
+//
+// Valid remote URLs are denoted in section GIT URLS of git-fetch(1).
+//
+// The return value, on success, is the object ID of the remote reference. If
+// no local reference is given in `refspec` then do not assume it's available in
+// the `FETCH_HEAD` reference, it might have been overwritten by the time you
+// read or use it.
+func (repo *Repository) Fetch(remoteURL, refspec string) (string, error) {
+	objectFormat, err := repo.GetObjectFormat()
+	if err != nil {
+		return "", err
+	}
+
+	stdout, stderr, err := NewCommand(repo.Ctx, "fetch", "--porcelain", "--end-of-options").AddDynamicArguments(remoteURL, refspec).RunStdString(&RunOpts{Dir: repo.Path})
+	if err != nil {
+		if strings.HasPrefix(stderr, "fatal: couldn't find remote ref ") {
+			return "", ErrRemoteRefNotFound
+		}
+		return "", err
+	}
+
+	localReference, _, ok := strings.Cut(refspec, ":")
+	if !ok {
+		localReference = "FETCH_HEAD"
+	}
+
+	// The porcelain format, per section OUTPUT of git-fetch(1), is expected to be:
+	// <flag><space><old-object-id><space><new-object-id><space><local-reference>\n
+	// flag is one character.
+	if expectedLen := 1 + 1 + objectFormat.FullLength() + 1 + objectFormat.FullLength() + 1 + len(localReference) + 1; len(stdout) != expectedLen {
+		return "", fmt.Errorf("output of git-fetch(1) is unexpected, we expected it to be %d bytes but it is %d bytes. stdout: %s", expectedLen, len(stdout), stdout)
+	}
+
+	// Extract the new objectID.
+	newObjectID := stdout[1+1+objectFormat.FullLength()+1 : 1+1+objectFormat.FullLength()+1+objectFormat.FullLength()]
+	return newObjectID, nil
+}

modules/git/fetch_test.go

@@ -0,0 +1,105 @@
+// Copyright 2025 The Forgejo Authors. All rights reserved.
+// SPDX-License-Identifier: GPL-3.0-or-later
+package git
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestFetch(t *testing.T) {
+	t.Run("SHA1", func(t *testing.T) {
+		dstDir := t.TempDir()
+
+		require.NoError(t, Clone(t.Context(), filepath.Join(testReposDir, "repo1_bare"), dstDir, CloneRepoOptions{}))
+
+		repo, err := OpenRepository(t.Context(), dstDir)
+		require.NoError(t, err)
+		defer repo.Close()
+
+		t.Run("Reference", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "language_stats_repo"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/master")
+			require.NoError(t, err)
+			assert.Equal(t, "95d3505f2db273e40be79f84416051ae85e9ea0d", fetchedCommitID)
+
+			c, err := repo.getCommit(MustIDFromString(fetchedCommitID))
+			require.NoError(t, err)
+			assert.NotNil(t, c)
+		})
+
+		t.Run("CommitID", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "45fb6cbc12f970b04eacd5cd4165edd11c8d7376")
+			require.NoError(t, err)
+			assert.Equal(t, "45fb6cbc12f970b04eacd5cd4165edd11c8d7376", fetchedCommitID)
+
+			c, err := repo.getCommit(MustIDFromString(fetchedCommitID))
+			require.NoError(t, err)
+			assert.NotNil(t, c)
+		})
+
+		t.Run("Invalid reference", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/does-not-exist")
+			require.ErrorIs(t, err, ErrRemoteRefNotFound)
+			assert.Empty(t, fetchedCommitID)
+		})
+	})
+
+	t.Run("SHA256", func(t *testing.T) {
+		skipIfSHA256NotSupported(t)
+
+		dstDir := t.TempDir()
+
+		require.NoError(t, Clone(t.Context(), filepath.Join(testReposDir, "repo1_bare_sha256"), dstDir, CloneRepoOptions{}))
+
+		repo, err := OpenRepository(t.Context(), dstDir)
+		require.NoError(t, err)
+		defer repo.Close()
+
+		t.Run("Reference", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame_sha256"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/main")
+			require.NoError(t, err)
+			assert.Equal(t, "e2f5660e15159082902960af0ed74fc144921d2b0c80e069361853b3ece29ba3", fetchedCommitID)
+
+			c, err := repo.getCommit(MustIDFromString(fetchedCommitID))
+			require.NoError(t, err)
+			assert.NotNil(t, c)
+		})
+
+		t.Run("CommitID", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_merge_sha256"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "d2e5609f630dd8db500f5298d05d16def282412e3e66ed68cc7d0833b29129a1")
+			require.NoError(t, err)
+			assert.Equal(t, "d2e5609f630dd8db500f5298d05d16def282412e3e66ed68cc7d0833b29129a1", fetchedCommitID)
+
+			c, err := repo.getCommit(MustIDFromString(fetchedCommitID))
+			require.NoError(t, err)
+			assert.NotNil(t, c)
+		})
+
+		t.Run("Invalid reference", func(t *testing.T) {
+			otherRepoPath, err := filepath.Abs(filepath.Join(testReposDir, "repo6_blame_sha256"))
+			require.NoError(t, err)
+
+			fetchedCommitID, err := repo.Fetch(otherRepoPath, "refs/heads/does-not-exist")
+			require.ErrorIs(t, err, ErrRemoteRefNotFound)
+			assert.Empty(t, fetchedCommitID)
+		})
+	})
+}

modules/git/git.go

@@ -303,6 +303,10 @@ func syncGitConfig() (err error) {
 		}
 	}
 
+	if err = configSet("receive.hideRefs", "refs/pull/"); err != nil {
+		return err
+	}
+
 	if !setting.Git.DisablePartialClone {
 		if err = configSet("uploadpack.allowfilter", "true"); err != nil {
 			return err

routers/web/repo/issue.go

@@ -41,7 +41,6 @@ import (
 	"forgejo.org/modules/markup"
 	"forgejo.org/modules/markup/markdown"
 	"forgejo.org/modules/optional"
-	repo_module "forgejo.org/modules/repository"
 	"forgejo.org/modules/setting"
 	api "forgejo.org/modules/structs"
 	"forgejo.org/modules/templates"
@@ -3167,13 +3166,7 @@ func NewComment(ctx *context.Context) {
 
 					if prHeadCommitID != headBranchCommitID {
 						// force push to base repo
-						err := git.Push(ctx, pull.HeadRepo.RepoPath(), git.PushOptions{
-							Remote: pull.BaseRepo.RepoPath(),
-							Branch: pull.HeadBranch + ":" + prHeadRef,
-							Force:  true,
-							Env:    repo_module.InternalPushingEnvironment(pull.Issue.Poster, pull.BaseRepo),
-						})
-						if err != nil {
+						if err := pull_service.PushToBaseRepo(ctx, pull); err != nil {
 							ctx.ServerError("force push error", err)
 							return
 						}

services/pull/comment.go

@@ -79,6 +79,10 @@ func CreatePushPullComment(ctx context.Context, pusher *user_model.User, pr *iss
 		return nil, err
 	}
 
+	if err := pr.LoadIssue(ctx); err != nil {
+		return nil, err
+	}
+
 	ops.Issue = pr.Issue
 
 	dataJSON, err := json.Marshal(data)

services/pull/pull.go

@@ -25,7 +25,6 @@ import (
 	"forgejo.org/modules/json"
 	"forgejo.org/modules/log"
 	"forgejo.org/modules/process"
-	repo_module "forgejo.org/modules/repository"
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/sync"
 	app_context "forgejo.org/services/context"
@@ -430,10 +429,6 @@ func ValidatePullRequest(ctx context.Context, pr *issues_model.PullRequest, newC
 // corresponding branches of base repository.
 // FIXME: Only push branches that are actually updates?
 func PushToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) (err error) {
-	return pushToBaseRepoHelper(ctx, pr, "")
-}
-
-func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, prefixHeadBranch string) (err error) {
 	log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitRefName())
 
 	if err := pr.LoadHeadRepo(ctx); err != nil {
@@ -448,44 +443,18 @@ func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, pre
 	}
 	baseRepoPath := pr.BaseRepo.RepoPath()
 
-	if err = pr.LoadIssue(ctx); err != nil {
-		return fmt.Errorf("unable to load issue %d for pr %d: %w", pr.IssueID, pr.ID, err)
+	baseRepo, err := git.OpenRepository(ctx, baseRepoPath)
+	if err != nil {
+		return err
 	}
-	if err = pr.Issue.LoadPoster(ctx); err != nil {
-		return fmt.Errorf("unable to load poster %d for pr %d: %w", pr.Issue.PosterID, pr.ID, err)
+	defer baseRepo.Close()
+
+	fetchedCommitID, err := baseRepo.Fetch("file://"+headRepoPath, git.BranchPrefix+pr.HeadBranch)
+	if err != nil {
+		return err
 	}
 
-	gitRefName := pr.GetGitRefName()
-
-	if err := git.Push(ctx, headRepoPath, git.PushOptions{
-		Remote: baseRepoPath,
-		Branch: prefixHeadBranch + pr.HeadBranch + ":" + gitRefName,
-		Force:  true,
-		// Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/...
-		Env: repo_module.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo),
-	}); err != nil {
-		if git.IsErrPushOutOfDate(err) {
-			// This should not happen as we're using force!
-			log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err)
-			return err
-		} else if git.IsErrPushRejected(err) {
-			rejectErr := err.(*git.ErrPushRejected)
-			log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err)
-			return err
-		} else if git.IsErrMoreThanOne(err) {
-			if prefixHeadBranch != "" {
-				log.Info("Can't push with %s%s", prefixHeadBranch, pr.HeadBranch)
-				return err
-			}
-			log.Info("Retrying to push with %s%s", git.BranchPrefix, pr.HeadBranch)
-			err = pushToBaseRepoHelper(ctx, pr, git.BranchPrefix)
-			return err
-		}
-		log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, gitRefName, err)
-		return fmt.Errorf("Push: %s:%s %s:%s %w", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), gitRefName, err)
-	}
-
-	return nil
+	return baseRepo.SetReference(pr.GetGitRefName(), fetchedCommitID)
 }
 
 // UpdateRef update refs/pull/id/head directly for agit flow pull request

tests/integration/git_test.go

@@ -778,13 +778,11 @@ func doInternalReferences(ctx *APITestContext, dstPath string) func(t *testing.T
 
 		_, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(fmt.Sprintf(":refs/pull/%d/head", pr1.Index)).RunStdString(&git.RunOpts{Dir: dstPath})
 		require.Error(t, gitErr)
-		assert.Contains(t, stdErr, fmt.Sprintf("remote: Forgejo: The deletion of refs/pull/%d/head is skipped as it's an internal reference.", pr1.Index))
-		assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] refs/pull/%d/head (hook declined)", pr1.Index))
+		assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] refs/pull/%d/head (deny deleting a hidden ref)", pr1.Index))
 
 		_, stdErr, gitErr = git.NewCommand(git.DefaultContext, "push", "origin", "--force").AddDynamicArguments(fmt.Sprintf("HEAD~1:refs/pull/%d/head", pr1.Index)).RunStdString(&git.RunOpts{Dir: dstPath})
 		require.Error(t, gitErr)
-		assert.Contains(t, stdErr, fmt.Sprintf("remote: Forgejo: The modification of refs/pull/%d/head is skipped as it's an internal reference.", pr1.Index))
-		assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] HEAD~1 -> refs/pull/%d/head (hook declined)", pr1.Index))
+		assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] HEAD~1 -> refs/pull/%d/head (deny updating a hidden ref)", pr1.Index))
 	}
 }