fix(hook): ignore unknown push options instead of failing

If a repository has

git config --add push.pushOption submit=".sourcehut/*.yml"

it failed when pushed because of the unknown submit push
option. It will be ignored instead.

Filtering out the push options is done in an earlier stage, when the
hook command runs, before it submits the options map to the private
endpoint.

* move all the push options logic to modules/git/pushoptions
* add 100% test coverage for modules/git/pushoptions

Test coverage for the code paths from which code was moved to the
modules/git/pushoptions package:

* cmd/hook.go:runHookPreReceive
* routers/private/hook_pre_receive.go:validatePushOptions
  tests/integration/git_push_test.go:TestOptionsGitPush runs through
  both. The test verifying the option is rejected was removed and, if
  added again, will fail because the option is now ignored instead of
  being rejected.

* cmd/hook.go:runHookProcReceive
* services/agit/agit.go:ProcReceive
  tests/integration/git_test.go: doCreateAgitFlowPull runs through
  both. It uses variations of AGit related push options.

* cmd/hook.go:runHookPostReceive
* routers/private/hook_post_receive.go:HookPostReceive
  tests/integration/git_test.go:doPushCreate called by TestGit/HTTP/sha1/PushCreate
  runs through both.
  Note that although it provides coverage for this code path it does not use push options.

Fixes: https://codeberg.org/forgejo/forgejo/issues/3651

cmd/hook.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/git/pushoptions"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
 	repo_module "code.gitea.io/gitea/modules/repository"
@@ -192,7 +193,7 @@ Forgejo or set your environment appropriately.`, "")
 		GitAlternativeObjectDirectories: os.Getenv(private.GitAlternativeObjectDirectories),
 		GitObjectDirectory:              os.Getenv(private.GitObjectDirectory),
 		GitQuarantinePath:               os.Getenv(private.GitQuarantinePath),
-		GitPushOptions:                  pushOptions(),
+		GitPushOptions:                  pushoptions.New().ReadEnv().Map(),
 		PullRequestID:                   prID,
 		DeployKeyID:                     deployKeyID,
 		ActionPerm:                      int(actionPerm),
@@ -375,7 +376,7 @@ Forgejo or set your environment appropriately.`, "")
 		GitAlternativeObjectDirectories: os.Getenv(private.GitAlternativeObjectDirectories),
 		GitObjectDirectory:              os.Getenv(private.GitObjectDirectory),
 		GitQuarantinePath:               os.Getenv(private.GitQuarantinePath),
-		GitPushOptions:                  pushOptions(),
+		GitPushOptions:                  pushoptions.New().ReadEnv().Map(),
 		PullRequestID:                   prID,
 		PushTrigger:                     repo_module.PushTrigger(os.Getenv(repo_module.EnvPushTrigger)),
 	}
@@ -488,21 +489,6 @@ func hookPrintResults(results []private.HookPostReceiveBranchResult) {
 	}
 }
 
-func pushOptions() map[string]string {
-	opts := make(map[string]string)
-	if pushCount, err := strconv.Atoi(os.Getenv(private.GitPushOptionCount)); err == nil {
-		for idx := 0; idx < pushCount; idx++ {
-			opt := os.Getenv(fmt.Sprintf("GIT_PUSH_OPTION_%d", idx))
-			key, value, found := strings.Cut(opt, "=")
-			if !found {
-				value = "true"
-			}
-			opts[key] = value
-		}
-	}
-	return opts
-}
-
 func runHookProcReceive(c *cli.Context) error {
 	ctx, cancel := installSignals()
 	defer cancel()
@@ -627,6 +613,7 @@ Forgejo or set your environment appropriately.`, "")
 	hookOptions.GitPushOptions = make(map[string]string)
 
 	if hasPushOptions {
+		pushOptions := pushoptions.NewFromMap(&hookOptions.GitPushOptions)
 		for {
 			rs, err = readPktLine(ctx, reader, pktLineTypeUnknown)
 			if err != nil {
@@ -636,12 +623,7 @@ Forgejo or set your environment appropriately.`, "")
 			if rs.Type == pktLineTypeFlush {
 				break
 			}
-
+			pushOptions.Parse(string(rs.Data))
-			key, value, found := strings.Cut(string(rs.Data), "=")
-			if !found {
-				value = "true"
-			}
-			hookOptions.GitPushOptions[key] = value
 		}
 	}
 

cmd/hook_test.go

@@ -15,7 +15,6 @@ import (
 	"testing"
 	"time"
 
-	"code.gitea.io/gitea/modules/private"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/test"
 
@@ -164,20 +163,6 @@ func TestDelayWriter(t *testing.T) {
 	})
 }
 
-func TestPushOptions(t *testing.T) {
-	require.NoError(t, os.Setenv(private.GitPushOptionCount, "3"))
-	require.NoError(t, os.Setenv("GIT_PUSH_OPTION_0", "force-push"))
-	require.NoError(t, os.Setenv("GIT_PUSH_OPTION_1", "option=value"))
-	require.NoError(t, os.Setenv("GIT_PUSH_OPTION_2", "option-double=another=value"))
-	require.NoError(t, os.Setenv("GIT_PUSH_OPTION_3", "not=valid"))
-
-	assert.Equal(t, map[string]string{
-		"force-push":    "true",
-		"option":        "value",
-		"option-double": "another=value",
-	}, pushOptions())
-}
-
 func TestRunHookUpdate(t *testing.T) {
 	app := cli.NewApp()
 	app.Commands = []*cli.Command{subcmdHookUpdate}

modules/git/pushoptions/pushoptions.go

@@ -0,0 +1,113 @@
+// Copyright twenty-panda <twenty-panda@posteo.com>
+// SPDX-License-Identifier: MIT
+
+package pushoptions
+
+import (
+	"fmt"
+	"os"
+	"strconv"
+	"strings"
+)
+
+type Key string
+
+const (
+	RepoPrivate     = Key("repo.private")
+	RepoTemplate    = Key("repo.template")
+	AgitTopic       = Key("topic")
+	AgitForcePush   = Key("force-push")
+	AgitTitle       = Key("title")
+	AgitDescription = Key("description")
+
+	envPrefix = "GIT_PUSH_OPTION"
+	EnvCount  = envPrefix + "_COUNT"
+	EnvFormat = envPrefix + "_%d"
+)
+
+type Interface interface {
+	ReadEnv() Interface
+	Parse(string) bool
+	Map() map[string]string
+
+	ChangeRepoSettings() bool
+
+	Empty() bool
+
+	GetBool(key Key, def bool) bool
+	GetString(key Key) (val string, ok bool)
+}
+
+type gitPushOptions map[string]string
+
+func New() Interface {
+	pushOptions := gitPushOptions(make(map[string]string))
+	return &pushOptions
+}
+
+func NewFromMap(o *map[string]string) Interface {
+	return (*gitPushOptions)(o)
+}
+
+func (o *gitPushOptions) ReadEnv() Interface {
+	if pushCount, err := strconv.Atoi(os.Getenv(EnvCount)); err == nil {
+		for idx := 0; idx < pushCount; idx++ {
+			_ = o.Parse(os.Getenv(fmt.Sprintf(EnvFormat, idx)))
+		}
+	}
+	return o
+}
+
+func (o *gitPushOptions) Parse(data string) bool {
+	key, value, found := strings.Cut(data, "=")
+	if !found {
+		value = "true"
+	}
+	switch Key(key) {
+	case RepoPrivate:
+	case RepoTemplate:
+	case AgitTopic:
+	case AgitForcePush:
+	case AgitTitle:
+	case AgitDescription:
+	default:
+		return false
+	}
+	(*o)[key] = value
+	return true
+}
+
+func (o gitPushOptions) Map() map[string]string {
+	return o
+}
+
+func (o gitPushOptions) ChangeRepoSettings() bool {
+	if o.Empty() {
+		return false
+	}
+	for _, key := range []Key{RepoPrivate, RepoTemplate} {
+		_, ok := o[string(key)]
+		if ok {
+			return true
+		}
+	}
+	return false
+}
+
+func (o gitPushOptions) Empty() bool {
+	return len(o) == 0
+}
+
+func (o gitPushOptions) GetBool(key Key, def bool) bool {
+	if val, ok := o[string(key)]; ok {
+		if b, err := strconv.ParseBool(val); err == nil {
+			return b
+		}
+	}
+	return def
+}
+
+func (o gitPushOptions) GetString(key Key) (string, bool) {
+	val, ok := o[string(key)]
+	return val, ok
+}

modules/git/pushoptions/pushoptions_test.go

@@ -0,0 +1,125 @@
+// Copyright twenty-panda <twenty-panda@posteo.com>
+// SPDX-License-Identifier: MIT
+
+package pushoptions
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEmpty(t *testing.T) {
+	options := New()
+	assert.True(t, options.Empty())
+	options.Parse(fmt.Sprintf("%v", RepoPrivate))
+	assert.False(t, options.Empty())
+}
+
+func TestToAndFromMap(t *testing.T) {
+	options := New()
+	options.Parse(fmt.Sprintf("%v", RepoPrivate))
+	actual := options.Map()
+	expected := map[string]string{string(RepoPrivate): "true"}
+	assert.EqualValues(t, expected, actual)
+	assert.EqualValues(t, expected, NewFromMap(&actual).Map())
+}
+
+func TestChangeRepositorySettings(t *testing.T) {
+	options := New()
+	assert.False(t, options.ChangeRepoSettings())
+	assert.True(t, options.Parse(fmt.Sprintf("%v=description", AgitDescription)))
+	assert.False(t, options.ChangeRepoSettings())
+
+	options.Parse(fmt.Sprintf("%v", RepoPrivate))
+	assert.True(t, options.ChangeRepoSettings())
+
+	options = New()
+	options.Parse(fmt.Sprintf("%v", RepoTemplate))
+	assert.True(t, options.ChangeRepoSettings())
+}
+
+func TestParse(t *testing.T) {
+	t.Run("no key", func(t *testing.T) {
+		options := New()
+
+		val, ok := options.GetString(RepoPrivate)
+		assert.False(t, ok)
+		assert.Equal(t, "", val)
+
+		assert.True(t, options.GetBool(RepoPrivate, true))
+		assert.False(t, options.GetBool(RepoPrivate, false))
+	})
+
+	t.Run("key=value", func(t *testing.T) {
+		options := New()
+
+		topic := "TOPIC"
+		assert.True(t, options.Parse(fmt.Sprintf("%v=%s", AgitTopic, topic)))
+		val, ok := options.GetString(AgitTopic)
+		assert.True(t, ok)
+		assert.Equal(t, topic, val)
+	})
+
+	t.Run("key=true", func(t *testing.T) {
+		options := New()
+
+		assert.True(t, options.Parse(fmt.Sprintf("%v=true", RepoPrivate)))
+		assert.True(t, options.GetBool(RepoPrivate, false))
+		assert.True(t, options.Parse(fmt.Sprintf("%v=TRUE", RepoTemplate)))
+		assert.True(t, options.GetBool(RepoTemplate, false))
+	})
+
+	t.Run("key=false", func(t *testing.T) {
+		options := New()
+
+		assert.True(t, options.Parse(fmt.Sprintf("%v=false", RepoPrivate)))
+		assert.False(t, options.GetBool(RepoPrivate, true))
+	})
+
+	t.Run("key", func(t *testing.T) {
+		options := New()
+
+		assert.True(t, options.Parse(fmt.Sprintf("%v", RepoPrivate)))
+		assert.True(t, options.GetBool(RepoPrivate, false))
+	})
+
+	t.Run("unknown keys are ignored", func(t *testing.T) {
+		options := New()
+
+		assert.True(t, options.Empty())
+		assert.False(t, options.Parse("unknown=value"))
+		assert.True(t, options.Empty())
+	})
+}
+
+func TestReadEnv(t *testing.T) {
+	t.Setenv(envPrefix+"_0", fmt.Sprintf("%v=true", AgitForcePush))
+	t.Setenv(envPrefix+"_1", fmt.Sprintf("%v", RepoPrivate))
+	t.Setenv(envPrefix+"_2", fmt.Sprintf("%v=equal=in string", AgitTitle))
+	t.Setenv(envPrefix+"_3", "not=valid")
+	t.Setenv(envPrefix+"_4", fmt.Sprintf("%v=description", AgitDescription))
+	t.Setenv(EnvCount, "5")
+
+	options := New().ReadEnv()
+
+	assert.True(t, options.GetBool(AgitForcePush, false))
+	assert.True(t, options.GetBool(RepoPrivate, false))
+	assert.False(t, options.GetBool(RepoTemplate, false))
+
+	{
+		val, ok := options.GetString(AgitTitle)
+		assert.True(t, ok)
+		assert.Equal(t, "equal=in string", val)
+	}
+	{
+		val, ok := options.GetString(AgitDescription)
+		assert.True(t, ok)
+		assert.Equal(t, "description", val)
+	}
+	{
+		_, ok := options.GetString(AgitTopic)
+		assert.False(t, ok)
+	}
+}

modules/private/hook.go

@@ -7,10 +7,10 @@ import (
 	"context"
 	"fmt"
 	"net/url"
-	"strconv"
 	"time"
 
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/git/pushoptions"
 	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
 )
@@ -20,28 +20,8 @@ const (
 	GitAlternativeObjectDirectories = "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 	GitObjectDirectory              = "GIT_OBJECT_DIRECTORY"
 	GitQuarantinePath               = "GIT_QUARANTINE_PATH"
-	GitPushOptionCount              = "GIT_PUSH_OPTION_COUNT"
 )
 
-// GitPushOptions is a wrapper around a map[string]string
-type GitPushOptions map[string]string
-
-// GitPushOptions keys
-const (
-	GitPushOptionRepoPrivate  = "repo.private"
-	GitPushOptionRepoTemplate = "repo.template"
-)
-
-// Bool checks for a key in the map and parses as a boolean
-func (g GitPushOptions) Bool(key string, def bool) bool {
-	if val, ok := g[key]; ok {
-		if b, err := strconv.ParseBool(val); err == nil {
-			return b
-		}
-	}
-	return def
-}
-
 // HookOptions represents the options for the Hook calls
 type HookOptions struct {
 	OldCommitIDs                    []string
@@ -52,7 +32,7 @@ type HookOptions struct {
 	GitObjectDirectory              string
 	GitAlternativeObjectDirectories string
 	GitQuarantinePath               string
-	GitPushOptions                  GitPushOptions
+	GitPushOptions                  map[string]string
 	PullRequestID                   int64
 	PushTrigger                     repository.PushTrigger
 	DeployKeyID                     int64 // if the pusher is a DeployKey, then UserID is the repo's org user.
@@ -60,6 +40,10 @@ type HookOptions struct {
 	ActionPerm                      int
 }
 
+func (o *HookOptions) GetGitPushOptions() pushoptions.Interface {
+	return pushoptions.NewFromMap(&o.GitPushOptions)
+}
+
 // SSHLogOption ssh log options
 type SSHLogOption struct {
 	IsError bool

release-notes/8.0.0/fix/4253.md

@@ -0,0 +1 @@
+- unknown git push options are rejected instead of being ignored

routers/private/hook_post_receive.go

@@ -18,6 +18,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/cache"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/git/pushoptions"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
@@ -170,7 +171,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 	}
 
 	// Handle Push Options
-	if len(opts.GitPushOptions) > 0 {
+	if !opts.GetGitPushOptions().Empty() {
 		// load the repository
 		if repo == nil {
 			repo = loadRepository(ctx, ownerName, repoName)
@@ -181,8 +182,8 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
 			wasEmpty = repo.IsEmpty
 		}
 
-		repo.IsPrivate = opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate, repo.IsPrivate)
+		repo.IsPrivate = opts.GetGitPushOptions().GetBool(pushoptions.RepoPrivate, repo.IsPrivate)
-		repo.IsTemplate = opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate, repo.IsTemplate)
+		repo.IsTemplate = opts.GetGitPushOptions().GetBool(pushoptions.RepoTemplate, repo.IsTemplate)
 		if err := repo_model.UpdateRepositoryCols(ctx, repo, "is_private", "is_template"); err != nil {
 			log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
 			ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{

routers/private/hook_pre_receive.go

@@ -123,23 +123,7 @@ func (ctx *preReceiveContext) canChangeSettings() error {
 func (ctx *preReceiveContext) validatePushOptions() error {
 	opts := web.GetForm(ctx).(*private.HookOptions)
 
-	if len(opts.GitPushOptions) == 0 {
+	if opts.GetGitPushOptions().ChangeRepoSettings() {
-		return nil
-	}
-
-	changesRepoSettings := false
-	for key := range opts.GitPushOptions {
-		switch key {
-		case private.GitPushOptionRepoPrivate, private.GitPushOptionRepoTemplate:
-			changesRepoSettings = true
-		case "topic", "force-push", "title", "description":
-			// Agit options
-		default:
-			return fmt.Errorf("unknown option %s", key)
-		}
-	}
-
-	if changesRepoSettings {
 		return ctx.canChangeSettings()
 	}
 

services/agit/agit.go

@@ -13,6 +13,7 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/git/pushoptions"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
 	notify_service "code.gitea.io/gitea/services/notify"
@@ -23,10 +24,10 @@ import (
 func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Repository, opts *private.HookOptions) ([]private.HookProcReceiveRefResult, error) {
 	results := make([]private.HookProcReceiveRefResult, 0, len(opts.OldCommitIDs))
 
-	topicBranch := opts.GitPushOptions["topic"]
+	topicBranch, _ := opts.GetGitPushOptions().GetString(pushoptions.AgitTopic)
-	_, forcePush := opts.GitPushOptions["force-push"]
+	_, forcePush := opts.GetGitPushOptions().GetString(pushoptions.AgitForcePush)
-	title, hasTitle := opts.GitPushOptions["title"]
+	title, hasTitle := opts.GetGitPushOptions().GetString(pushoptions.AgitTitle)
-	description, hasDesc := opts.GitPushOptions["description"]
+	description, hasDesc := opts.GetGitPushOptions().GetString(pushoptions.AgitDescription)
 
 	objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName)
 

tests/integration/git_push_test.go

@@ -225,16 +225,14 @@ func testOptionsGitPush(t *testing.T, u *url.URL) {
 		u.User = url.UserPassword(user.LowerName, userPassword)
 		doGitAddRemote(gitPath, "origin", u)(t)
 
-		t.Run("Unknown push options are rejected", func(t *testing.T) {
+		t.Run("Unknown push options are silently ignored", func(t *testing.T) {
-			logChecker, cleanup := test.NewLogChecker(log.DEFAULT, log.TRACE)
-			logChecker.Filter("unknown option").StopMark("Git push options validation")
-			defer cleanup()
 			branchName := "branch0"
 			doGitCreateBranch(gitPath, branchName)(t)
-			doGitPushTestRepositoryFail(gitPath, "origin", branchName, "-o", "repo.template=false", "-o", "uknownoption=randomvalue")(t)
+			doGitPushTestRepository(gitPath, "origin", branchName, "-o", "uknownoption=randomvalue", "-o", "repo.private=true")(t)
-			logFiltered, logStopped := logChecker.Check(5 * time.Second)
+			repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push")
-			assert.True(t, logStopped)
+			require.NoError(t, err)
-			assert.True(t, logFiltered[0])
+			require.True(t, repo.IsPrivate)
+			require.False(t, repo.IsTemplate)
 		})
 
 		t.Run("Owner sets private & template to true via push options", func(t *testing.T) {