diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-02-12 09:57:47 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-02-12 13:38:08 +0300 |
commit | eb49052f9e7edac32c4ad3f36a6a7e49f955bf87 (patch) | |
tree | 52fd40f90f7483e22880fc7d57b6cc32e2cd8f77 | |
parent | f759f16177f4d54419cade648336c9c7beda325d (diff) |
gitaly: Set up requested hooks bitmap
With the infrastructure being in place to support executing requested
hooks only, this commit converts all callsites of `NewHooksPayload()` to
also pass the bitmap of hooks they want to execute. This finally fixes
an issue where we could've accidentally executed an unwanted hook only
because we set up `core.hooksPath` for a git command.
-rw-r--r-- | changelogs/unreleased/pks-hooks-requested-hook-bitmap.yml | 5 | ||||
-rw-r--r-- | cmd/gitaly-hooks/hooks_test.go | 38 | ||||
-rw-r--r-- | internal/git/hooks_options.go | 9 | ||||
-rw-r--r-- | internal/git/hooks_payload.go | 5 | ||||
-rw-r--r-- | internal/git/hooks_payload_test.go | 20 | ||||
-rw-r--r-- | internal/gitaly/hook/postreceive_test.go | 6 | ||||
-rw-r--r-- | internal/gitaly/hook/prereceive_test.go | 6 | ||||
-rw-r--r-- | internal/gitaly/hook/transactions_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/hook/update_test.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/hook/post_receive_test.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/hook/pre_receive_test.go | 7 | ||||
-rw-r--r-- | internal/gitaly/service/hook/reference_transaction_test.go | 1 | ||||
-rw-r--r-- | internal/gitaly/service/hook/update_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/operations/update_with_hooks.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/operations/update_with_hooks_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/smarthttp/receive_pack_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/ssh/receive_pack_test.go | 2 |
17 files changed, 87 insertions, 29 deletions
diff --git a/changelogs/unreleased/pks-hooks-requested-hook-bitmap.yml b/changelogs/unreleased/pks-hooks-requested-hook-bitmap.yml new file mode 100644 index 000000000..05f14c34a --- /dev/null +++ b/changelogs/unreleased/pks-hooks-requested-hook-bitmap.yml @@ -0,0 +1,5 @@ +--- +title: 'hooks: Fix inadvertent execution of hooks' +merge_request: 3119 +author: +type: fixed diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go index 96240ee28..b64e23cf0 100644 --- a/cmd/gitaly-hooks/hooks_test.go +++ b/cmd/gitaly-hooks/hooks_test.go @@ -43,7 +43,7 @@ func envForHooks(t testing.TB, gitlabShellDir string, repo *gitalypb.Repository, UserID: glHookValues.GLID, Username: glHookValues.GLUsername, Protocol: glHookValues.GLProtocol, - }).Env() + }, git.AllHooks).Env() require.NoError(t, err) env := append(os.Environ(), []string{ @@ -469,6 +469,7 @@ func TestHooksPostReceiveFailed(t *testing.T) { Username: glUsername, Protocol: glProtocol, }, + git.PostReceiveHook, ).Env() require.NoError(t, err) @@ -726,3 +727,38 @@ func TestGitalyHooksPackObjects(t *testing.T) { }) } } + +func TestRequestedHooks(t *testing.T) { + for hook, hookName := range map[git.Hook]string{ + git.ReferenceTransactionHook: "reference-transaction", + git.UpdateHook: "update", + git.PreReceiveHook: "pre-receive", + git.PostReceiveHook: "post-receive", + git.PackObjectsHook: "git", + } { + t.Run(hookName, func(t *testing.T) { + t.Run("unrequested hook is ignored", func(t *testing.T) { + payload, err := git.NewHooksPayload(config.Config, &gitalypb.Repository{}, nil, nil, nil, git.AllHooks&^hook).Env() + require.NoError(t, err) + + cmd := exec.Command(filepath.Join(config.Config.BinDir, "gitaly-hooks"), hookName) + cmd.Env = []string{payload} + require.NoError(t, cmd.Run()) + }) + + t.Run("requested hook runs", func(t *testing.T) { + payload, err := git.NewHooksPayload(config.Config, &gitalypb.Repository{}, nil, nil, nil, hook).Env() + require.NoError(t, err) + + cmd := exec.Command(filepath.Join(config.Config.BinDir, "gitaly-hooks"), hookName) + cmd.Env = []string{payload} + + // We simply check that there is an error here as an indicator that + // the hook logic ran. We don't care for the actual error because we + // know that in the previous testcase without the hook being + // requested, there was no error. + require.Error(t, cmd.Run(), "hook should have run and failed due to incomplete setup") + }) + }) + } +} diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go index bfd7780d1..bad8d4645 100644 --- a/internal/git/hooks_options.go +++ b/internal/git/hooks_options.go @@ -41,7 +41,7 @@ func WithRefTxHook(ctx context.Context, repo repository.GitRepo, cfg config.Cfg) GitAlternateObjectDirectories: repo.GetGitAlternateObjectDirectories(), GitObjectDirectory: repo.GetGitObjectDirectory(), RelativePath: repo.GetRelativePath(), - }, cfg, nil); err != nil { + }, cfg, nil, ReferenceTransactionHook); err != nil { return fmt.Errorf("ref hook env var: %w", err) } @@ -56,7 +56,7 @@ func WithPackObjectsHookEnv(ctx context.Context, repo *gitalypb.Repository, cfg return fmt.Errorf("missing repo: %w", ErrInvalidArg) } - if err := cc.configureHooks(ctx, repo, cfg, nil); err != nil { + if err := cc.configureHooks(ctx, repo, cfg, nil, PackObjectsHook); err != nil { return fmt.Errorf("pack-objects hook configuration: %w", err) } @@ -77,6 +77,7 @@ func (cc *cmdCfg) configureHooks( repo *gitalypb.Repository, cfg config.Cfg, receiveHooksPayload *ReceiveHooksPayload, + requestedHooks Hook, ) error { if cc.hooksConfigured { return errors.New("hooks already configured") @@ -87,7 +88,7 @@ func (cc *cmdCfg) configureHooks( return err } - payload, err := NewHooksPayload(cfg, repo, transaction, praefect, receiveHooksPayload).Env() + payload, err := NewHooksPayload(cfg, repo, transaction, praefect, receiveHooksPayload, requestedHooks).Env() if err != nil { return err } @@ -123,7 +124,7 @@ func WithReceivePackHooks(ctx context.Context, cfg config.Cfg, req ReceivePackRe UserID: req.GetGlId(), Username: req.GetGlUsername(), Protocol: protocol, - }); err != nil { + }, ReceivePackHooks); err != nil { return err } diff --git a/internal/git/hooks_payload.go b/internal/git/hooks_payload.go index 7fd9d9ade..97e33ca80 100644 --- a/internal/git/hooks_payload.go +++ b/internal/git/hooks_payload.go @@ -46,6 +46,9 @@ const ( // AllHooks is the bitwise set of all hooks supported by Gitaly. AllHooks = ReferenceTransactionHook | UpdateHook | PreReceiveHook | PostReceiveHook | PackObjectsHook + // ReceivePackHooks includes the set of hooks which shall be executed in + // a typical "push" or an emulation thereof (e.g. `updateReferenceWithHooks()`). + ReceivePackHooks = ReferenceTransactionHook | UpdateHook | PreReceiveHook | PostReceiveHook ) // HooksPayload holds parameters required for all hooks. @@ -106,6 +109,7 @@ func NewHooksPayload( tx *metadata.Transaction, praefect *metadata.PraefectServer, receiveHooksPayload *ReceiveHooksPayload, + requestedHooks Hook, ) HooksPayload { return HooksPayload{ Repo: repo, @@ -116,6 +120,7 @@ func NewHooksPayload( Transaction: tx, Praefect: praefect, ReceiveHooksPayload: receiveHooksPayload, + RequestedHooks: requestedHooks, } } diff --git a/internal/git/hooks_payload_test.go b/internal/git/hooks_payload_test.go index 8df82cebf..1b00a6f39 100644 --- a/internal/git/hooks_payload_test.go +++ b/internal/git/hooks_payload_test.go @@ -28,13 +28,13 @@ func TestHooksPayload(t *testing.T) { } t.Run("envvar has proper name", func(t *testing.T) { - env, err := NewHooksPayload(config.Config, repo, nil, nil, nil).Env() + env, err := NewHooksPayload(config.Config, repo, nil, nil, nil, AllHooks).Env() require.NoError(t, err) require.True(t, strings.HasPrefix(env, EnvHooksPayload+"=")) }) t.Run("roundtrip succeeds", func(t *testing.T) { - env, err := NewHooksPayload(config.Config, repo, nil, nil, nil).Env() + env, err := NewHooksPayload(config.Config, repo, nil, nil, nil, PreReceiveHook).Env() require.NoError(t, err) payload, err := HooksPayloadFromEnv([]string{ @@ -50,12 +50,12 @@ func TestHooksPayload(t *testing.T) { BinDir: config.Config.BinDir, GitPath: config.Config.Git.BinPath, InternalSocket: config.Config.GitalyInternalSocketPath(), - RequestedHooks: AllHooks, + RequestedHooks: PreReceiveHook, }, payload) }) t.Run("roundtrip with transaction succeeds", func(t *testing.T) { - env, err := NewHooksPayload(config.Config, repo, &tx, &praefect, nil).Env() + env, err := NewHooksPayload(config.Config, repo, &tx, &praefect, nil, UpdateHook).Env() require.NoError(t, err) payload, err := HooksPayloadFromEnv([]string{env}) @@ -68,7 +68,7 @@ func TestHooksPayload(t *testing.T) { InternalSocket: config.Config.GitalyInternalSocketPath(), Transaction: &tx, Praefect: &praefect, - RequestedHooks: AllHooks, + RequestedHooks: UpdateHook, }, payload) }) @@ -84,7 +84,7 @@ func TestHooksPayload(t *testing.T) { }) t.Run("payload with missing Praefect", func(t *testing.T) { - env, err := NewHooksPayload(config.Config, repo, &tx, nil, nil).Env() + env, err := NewHooksPayload(config.Config, repo, &tx, nil, nil, AllHooks).Env() require.NoError(t, err) _, err = HooksPayloadFromEnv([]string{env}) @@ -96,7 +96,7 @@ func TestHooksPayload(t *testing.T) { UserID: "1234", Username: "user", Protocol: "ssh", - }).Env() + }, PostReceiveHook).Env() require.NoError(t, err) payload, err := HooksPayloadFromEnv([]string{ @@ -118,7 +118,7 @@ func TestHooksPayload(t *testing.T) { Username: "user", Protocol: "ssh", }, - RequestedHooks: AllHooks, + RequestedHooks: PostReceiveHook, }, payload) }) @@ -128,7 +128,7 @@ func TestHooksPayload(t *testing.T) { }(config.Config.Git.BinPath) config.Config.Git.BinPath = "" - env, err := NewHooksPayload(config.Config, repo, nil, nil, nil).Env() + env, err := NewHooksPayload(config.Config, repo, nil, nil, nil, ReceivePackHooks).Env() require.NoError(t, err) payload, err := HooksPayloadFromEnv([]string{ @@ -141,7 +141,7 @@ func TestHooksPayload(t *testing.T) { BinDir: config.Config.BinDir, GitPath: "/foo/bar", InternalSocket: config.Config.GitalyInternalSocketPath(), - RequestedHooks: AllHooks, + RequestedHooks: ReceivePackHooks, }, payload) }) } diff --git a/internal/gitaly/hook/postreceive_test.go b/internal/gitaly/hook/postreceive_test.go index 67e853770..433b780c8 100644 --- a/internal/gitaly/hook/postreceive_test.go +++ b/internal/gitaly/hook/postreceive_test.go @@ -70,7 +70,7 @@ func TestPostReceive_customHook(t *testing.T) { Protocol: "web", } - payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload).Env() + payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload, git.PostReceiveHook).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -84,6 +84,7 @@ func TestPostReceive_customHook(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.PostReceiveHook, ).Env() require.NoError(t, err) @@ -98,6 +99,7 @@ func TestPostReceive_customHook(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.PostReceiveHook, ).Env() require.NoError(t, err) @@ -246,7 +248,7 @@ func TestPostReceive_gitlab(t *testing.T) { UserID: "1234", Username: "user", Protocol: "web", - }).Env() + }, git.PostReceiveHook).Env() require.NoError(t, err) standardEnv := []string{payload} diff --git a/internal/gitaly/hook/prereceive_test.go b/internal/gitaly/hook/prereceive_test.go index 3b916b240..1209571b7 100644 --- a/internal/gitaly/hook/prereceive_test.go +++ b/internal/gitaly/hook/prereceive_test.go @@ -29,7 +29,7 @@ func TestPrereceive_customHooks(t *testing.T) { Protocol: "web", } - payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload).Env() + payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload, git.PreReceiveHook).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -43,6 +43,7 @@ func TestPrereceive_customHooks(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.PreReceiveHook, ).Env() require.NoError(t, err) @@ -57,6 +58,7 @@ func TestPrereceive_customHooks(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.PreReceiveHook, ).Env() require.NoError(t, err) @@ -208,7 +210,7 @@ func TestPrereceive_gitlab(t *testing.T) { UserID: "1234", Username: "user", Protocol: "web", - }).Env() + }, git.PreReceiveHook).Env() require.NoError(t, err) standardEnv := []string{payload} diff --git a/internal/gitaly/hook/transactions_test.go b/internal/gitaly/hook/transactions_test.go index 474c46c44..2467aae41 100644 --- a/internal/gitaly/hook/transactions_test.go +++ b/internal/gitaly/hook/transactions_test.go @@ -55,6 +55,7 @@ func TestHookManager_stopCalled(t *testing.T) { Username: "user", Protocol: "web", }, + git.ReferenceTransactionHook, ).Env() require.NoError(t, err) @@ -149,6 +150,7 @@ func TestHookManager_contextCancellationCancelsVote(t *testing.T) { Token: "matter", }, nil, + git.ReferenceTransactionHook, ).Env() require.NoError(t, err) diff --git a/internal/gitaly/hook/update_test.go b/internal/gitaly/hook/update_test.go index 6fa5bb0c0..4ca861e8b 100644 --- a/internal/gitaly/hook/update_test.go +++ b/internal/gitaly/hook/update_test.go @@ -26,7 +26,7 @@ func TestUpdate_customHooks(t *testing.T) { Protocol: "web", } - payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload).Env() + payload, err := git.NewHooksPayload(config.Config, repo, nil, nil, receiveHooksPayload, git.UpdateHook).Env() require.NoError(t, err) primaryPayload, err := git.NewHooksPayload( @@ -40,6 +40,7 @@ func TestUpdate_customHooks(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.UpdateHook, ).Env() require.NoError(t, err) @@ -54,6 +55,7 @@ func TestUpdate_customHooks(t *testing.T) { Token: "secret", }, receiveHooksPayload, + git.UpdateHook, ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/post_receive_test.go b/internal/gitaly/service/hook/post_receive_test.go index fc50119aa..24adb9469 100644 --- a/internal/gitaly/service/hook/post_receive_test.go +++ b/internal/gitaly/service/hook/post_receive_test.go @@ -123,6 +123,7 @@ func TestHooksMissingStdin(t *testing.T) { Username: "username", Protocol: "protocol", }, + git.PostReceiveHook, ).Env() require.NoError(t, err) @@ -253,7 +254,7 @@ To create a merge request for okay, visit: UserID: "key_id", Username: "username", Protocol: "protocol", - }).Env() + }, git.PostReceiveHook).Env() require.NoError(t, err) envVars := []string{ diff --git a/internal/gitaly/service/hook/pre_receive_test.go b/internal/gitaly/service/hook/pre_receive_test.go index aaa1f8da4..091af1e9f 100644 --- a/internal/gitaly/service/hook/pre_receive_test.go +++ b/internal/gitaly/service/hook/pre_receive_test.go @@ -149,7 +149,7 @@ func TestPreReceiveHook_GitlabAPIAccess(t *testing.T) { UserID: glID, Username: "username", Protocol: protocol, - }).Env() + }, git.PreReceiveHook).Env() require.NoError(t, err) stdin := bytes.NewBufferString(changes) @@ -256,7 +256,7 @@ func TestPreReceive_APIErrors(t *testing.T) { UserID: "key-123", Username: "username", Protocol: "web", - }).Env() + }, git.PreReceiveHook).Env() require.NoError(t, err) stream, err := client.PreReceiveHook(ctx) @@ -325,7 +325,7 @@ exit %d UserID: "key-123", Username: "username", Protocol: "web", - }).Env() + }, git.PreReceiveHook).Env() require.NoError(t, err) stream, err := client.PreReceiveHook(ctx) @@ -464,6 +464,7 @@ func TestPreReceiveHook_Primary(t *testing.T) { Username: "username", Protocol: "web", }, + git.PreReceiveHook, ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/reference_transaction_test.go b/internal/gitaly/service/hook/reference_transaction_test.go index c9ab63318..f105a7e1c 100644 --- a/internal/gitaly/service/hook/reference_transaction_test.go +++ b/internal/gitaly/service/hook/reference_transaction_test.go @@ -142,6 +142,7 @@ func TestReferenceTransactionHook(t *testing.T) { ListenAddr: "tcp://" + listener.Addr().String(), }, nil, + git.ReferenceTransactionHook, ).Env() require.NoError(t, err) diff --git a/internal/gitaly/service/hook/update_test.go b/internal/gitaly/service/hook/update_test.go index 0767a7321..929722a22 100644 --- a/internal/gitaly/service/hook/update_test.go +++ b/internal/gitaly/service/hook/update_test.go @@ -48,7 +48,7 @@ func TestUpdate_CustomHooks(t *testing.T) { UserID: "key-123", Username: "username", Protocol: "web", - }).Env() + }, git.UpdateHook).Env() require.NoError(t, err) envVars := []string{ diff --git a/internal/gitaly/service/operations/update_with_hooks.go b/internal/gitaly/service/operations/update_with_hooks.go index b77b14d53..751747e98 100644 --- a/internal/gitaly/service/operations/update_with_hooks.go +++ b/internal/gitaly/service/operations/update_with_hooks.go @@ -53,7 +53,7 @@ func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re UserID: user.GetGlId(), Username: user.GetGlUsername(), Protocol: "web", - }).Env() + }, git.ReceivePackHooks).Env() if err != nil { return err } diff --git a/internal/gitaly/service/operations/update_with_hooks_test.go b/internal/gitaly/service/operations/update_with_hooks_test.go index 1a72f4c81..4b60d14d7 100644 --- a/internal/gitaly/service/operations/update_with_hooks_test.go +++ b/internal/gitaly/service/operations/update_with_hooks_test.go @@ -140,7 +140,7 @@ func TestUpdateReferenceWithHooks(t *testing.T) { UserID: "1234", Username: "Username", Protocol: "web", - }).Env() + }, git.ReceivePackHooks).Env() require.NoError(t, err) expectedEnv := []string{ diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go index ab8acb358..e2e0917c5 100644 --- a/internal/gitaly/service/smarthttp/receive_pack_test.go +++ b/internal/gitaly/service/smarthttp/receive_pack_test.go @@ -98,7 +98,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) { Username: "user", Protocol: "http", }, - RequestedHooks: git.AllHooks, + RequestedHooks: git.ReceivePackHooks, }, payload) } diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go index 7338e93fe..95061e27f 100644 --- a/internal/gitaly/service/ssh/receive_pack_test.go +++ b/internal/gitaly/service/ssh/receive_pack_test.go @@ -135,7 +135,7 @@ func TestReceivePackPushSuccess(t *testing.T) { Username: "user", Protocol: "ssh", }, - RequestedHooks: git.AllHooks, + RequestedHooks: git.ReceivePackHooks, }, payload) } |