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.

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