diff --git a/cmd/hook.go b/cmd/hook.go index f8184f9697..eec93aece7 100644 --- a/cmd/hook.go +++ b/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 } - - key, value, found := strings.Cut(string(rs.Data), "=") - if !found { - value = "true" - } - hookOptions.GitPushOptions[key] = value + pushOptions.Parse(string(rs.Data)) } } diff --git a/cmd/hook_test.go b/cmd/hook_test.go index 91731f77c0..36149eb91f 100644 --- a/cmd/hook_test.go +++ b/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} diff --git a/modules/git/pushoptions/pushoptions.go b/modules/git/pushoptions/pushoptions.go new file mode 100644 index 0000000000..9709a8be79 --- /dev/null +++ b/modules/git/pushoptions/pushoptions.go @@ -0,0 +1,113 @@ +// Copyright twenty-panda +// 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 +} diff --git a/modules/git/pushoptions/pushoptions_test.go b/modules/git/pushoptions/pushoptions_test.go new file mode 100644 index 0000000000..49bf2d275b --- /dev/null +++ b/modules/git/pushoptions/pushoptions_test.go @@ -0,0 +1,125 @@ +// Copyright twenty-panda +// 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) + } +} diff --git a/modules/private/hook.go b/modules/private/hook.go index 1d0ef4e3a9..93cbcd469d 100644 --- a/modules/private/hook.go +++ b/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 diff --git a/release-notes/8.0.0/fix/4253.md b/release-notes/8.0.0/fix/4253.md new file mode 100644 index 0000000000..1533c2a734 --- /dev/null +++ b/release-notes/8.0.0/fix/4253.md @@ -0,0 +1 @@ +- unknown git push options are rejected instead of being ignored diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index b78f19d51e..11d1161e85 100644 --- a/routers/private/hook_post_receive.go +++ b/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.IsTemplate = opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate, repo.IsTemplate) + repo.IsPrivate = opts.GetGitPushOptions().GetBool(pushoptions.RepoPrivate, repo.IsPrivate) + 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{ diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 456a288b00..8f7575c1db 100644 --- a/routers/private/hook_pre_receive.go +++ b/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 { - 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 { + if opts.GetGitPushOptions().ChangeRepoSettings() { return ctx.canChangeSettings() } diff --git a/services/agit/agit.go b/services/agit/agit.go index e46a5771e1..bd8cc82e55 100644 --- a/services/agit/agit.go +++ b/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"] - _, forcePush := opts.GitPushOptions["force-push"] - title, hasTitle := opts.GitPushOptions["title"] - description, hasDesc := opts.GitPushOptions["description"] + topicBranch, _ := opts.GetGitPushOptions().GetString(pushoptions.AgitTopic) + _, forcePush := opts.GetGitPushOptions().GetString(pushoptions.AgitForcePush) + title, hasTitle := opts.GetGitPushOptions().GetString(pushoptions.AgitTitle) + description, hasDesc := opts.GetGitPushOptions().GetString(pushoptions.AgitDescription) objectFormat := git.ObjectFormatFromName(repo.ObjectFormatName) diff --git a/tests/integration/git_push_test.go b/tests/integration/git_push_test.go index 8a9fe81e24..06d985cff7 100644 --- a/tests/integration/git_push_test.go +++ b/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) { - logChecker, cleanup := test.NewLogChecker(log.DEFAULT, log.TRACE) - logChecker.Filter("unknown option").StopMark("Git push options validation") - defer cleanup() + t.Run("Unknown push options are silently ignored", func(t *testing.T) { branchName := "branch0" doGitCreateBranch(gitPath, branchName)(t) - doGitPushTestRepositoryFail(gitPath, "origin", branchName, "-o", "repo.template=false", "-o", "uknownoption=randomvalue")(t) - logFiltered, logStopped := logChecker.Check(5 * time.Second) - assert.True(t, logStopped) - assert.True(t, logFiltered[0]) + doGitPushTestRepository(gitPath, "origin", branchName, "-o", "uknownoption=randomvalue", "-o", "repo.private=true")(t) + repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, "repo-to-push") + require.NoError(t, err) + 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) {