Merge pull request 'fix(hook): ignore unknown push options instead of failing' (#4253) from twenty-panda/forgejo:pr-3706 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4253
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

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) {