hook: Fix potential use of wrong Git in custom hooks

Custom hooks are executed via two different paths: either via a Git
command, or manually by directly calling into the hook manager. When
executing via a Git command, then Git will have known to adapt the PATH
environment variable such that its own path is prepended to it. The
result is that hook scripts will automatically pick up the correct Git
executable if they only call "git" without using a fully-qualified path.
This PATH variable gets picked up by gitaly-hooks and is forwarded to
the hook RPCs such that this mechanism works as expected.

We don't do this in the case where we directly call into the hook
manager though. Instead, we simply take PATH as it is defined in the
server's environment. As a result, it can happen that scripts wouldn't
pick up the correct Git executable, or fail to find it at all.

Fix this bug by prepending the Git executables' directory to PATH.

Changelog: fixed

4 files changed, 45 insertions, 27 deletions

diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index d6b30566b..609c15225 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -49,7 +49,7 @@ func (m *GitLabHookManager) newCustomHooksExecutor(repo *gitalypb.Repository, ho
 	}
 	hookFiles = append(hookFiles, files...)
 
-	globalCustomHooksDir := filepath.Join(m.hooksConfig.CustomHooksDir, fmt.Sprintf("%s.d", hookName))
+	globalCustomHooksDir := filepath.Join(m.cfg.Hooks.CustomHooksDir, fmt.Sprintf("%s.d", hookName))
 	files, err = findHooks(globalCustomHooksDir)
 	if err != nil {
 		return nil, err
@@ -166,6 +166,21 @@ func (m *GitLabHookManager) customHooksEnv(payload git.HooksPayload, pushOptions
 		customEnvs = append(customEnvs, "GIT_ALTERNATE_OBJECT_DIRECTORIES="+alternateObjectDirectories)
 	}
 
+	if gitDir := filepath.Dir(m.cfg.Git.BinPath); gitDir != "." {
+		// By default, we should take PATH from the given set of environment variables, if
+		// it's contained in there. Otherwise, we need to take the current process's PATH
+		// environment, which would also be the default injected by the command package.
+		currentPath := getEnvVar("PATH", envs)
+		if currentPath == "" {
+			currentPath = os.Getenv("PATH")
+		}
+
+		// We want to ensure that custom hooks use the same Git version as used by Gitaly.
+		// Given that our Git version may not be contained in PATH, we thus have to prepend
+		// the directory containing that executable to PATH such that it can be found.
+		customEnvs = append(customEnvs, fmt.Sprintf("PATH=%s:%s", gitDir, currentPath))
+	}
+
 	return append(customEnvs,
 		"GIT_DIR="+repoPath,
 		"GL_REPOSITORY="+payload.Repo.GetGlRepository(),
diff --git a/internal/gitaly/hook/custom_test.go b/internal/gitaly/hook/custom_test.go
index 4cc9e9249..5fb75e43b 100644
--- a/internal/gitaly/hook/custom_test.go
+++ b/internal/gitaly/hook/custom_test.go
@@ -81,20 +81,21 @@ func TestCustomHooksSuccess(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.hookName, func(t *testing.T) {
-			globalCustomHooksDir := testhelper.TempDir(t)
+			cfg := cfg
+			cfg.Hooks.CustomHooksDir = testhelper.TempDir(t)
 
 			locator := config.NewLocator(cfg)
 			// hook is in project custom hook directory <repository>.git/custom_hooks/<hook_name>
 			hookDir := filepath.Join(repoPath, "custom_hooks")
-			callAndVerifyHooks(t, locator, repo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			callAndVerifyHooks(t, cfg, locator, repo, tc.hookName, hookDir, tc.stdin, tc.args, tc.env)
 
 			// hook is in project custom hooks directory <repository>.git/custom_hooks/<hook_name>.d/*
 			hookDir = filepath.Join(repoPath, "custom_hooks", fmt.Sprintf("%s.d", tc.hookName))
-			callAndVerifyHooks(t, locator, repo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			callAndVerifyHooks(t, cfg, locator, repo, tc.hookName, hookDir, tc.stdin, tc.args, tc.env)
 
 			// hook is in global custom hooks directory <global_custom_hooks_dir>/<hook_name>.d/*
-			hookDir = filepath.Join(globalCustomHooksDir, fmt.Sprintf("%s.d", tc.hookName))
-			callAndVerifyHooks(t, locator, repo, tc.hookName, globalCustomHooksDir, hookDir, tc.stdin, tc.args, tc.env)
+			hookDir = filepath.Join(cfg.Hooks.CustomHooksDir, fmt.Sprintf("%s.d", tc.hookName))
+			callAndVerifyHooks(t, cfg, locator, repo, tc.hookName, hookDir, tc.stdin, tc.args, tc.env)
 		})
 	}
 }
@@ -147,11 +148,12 @@ func TestCustomHookPartialFailure(t *testing.T) {
 			cleanup = writeCustomHook(t, tc.hook, globalHookPath, globalHookScript)
 			defer cleanup()
 
+			cfg := cfg
+			cfg.Hooks.CustomHooksDir = globalCustomHooksDir
+
 			mgr := GitLabHookManager{
+				cfg:     cfg,
 				locator: config.NewLocator(cfg),
-				hooksConfig: config.Hooks{
-					CustomHooksDir: globalCustomHooksDir,
-				},
 			}
 
 			caller, err := mgr.newCustomHooksExecutor(repo, tc.hook)
@@ -183,6 +185,8 @@ func TestCustomHooksMultipleHooks(t *testing.T) {
 
 	globalCustomHooksDir := testhelper.TempDir(t)
 
+	cfg.Hooks.CustomHooksDir = globalCustomHooksDir
+
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -206,10 +210,8 @@ func TestCustomHooksMultipleHooks(t *testing.T) {
 	}
 
 	mgr := GitLabHookManager{
+		cfg:     cfg,
 		locator: config.NewLocator(cfg),
-		hooksConfig: config.Hooks{
-			CustomHooksDir: globalCustomHooksDir,
-		},
 	}
 	hooksExecutor, err := mgr.newCustomHooksExecutor(repo, "update")
 	require.NoError(t, err)
@@ -231,6 +233,8 @@ func TestCustomHooksWithSymlinks(t *testing.T) {
 
 	globalCustomHooksDir := testhelper.TempDir(t)
 
+	cfg.Hooks.CustomHooksDir = globalCustomHooksDir
+
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -277,10 +281,8 @@ func TestCustomHooksWithSymlinks(t *testing.T) {
 	expectedExecutedScripts := []string{updateHookPath, updateTildePath}
 
 	mgr := GitLabHookManager{
+		cfg:     cfg,
 		locator: config.NewLocator(cfg),
-		hooksConfig: config.Hooks{
-			CustomHooksDir: globalCustomHooksDir,
-		},
 	}
 	hooksExecutor, err := mgr.newCustomHooksExecutor(repo, "update")
 	require.NoError(t, err)
@@ -301,6 +303,8 @@ func TestMultilineStdin(t *testing.T) {
 
 	globalCustomHooksDir := testhelper.TempDir(t)
 
+	cfg.Hooks.CustomHooksDir = globalCustomHooksDir
+
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -308,10 +312,8 @@ func TestMultilineStdin(t *testing.T) {
 
 	writeCustomHook(t, "pre-receive-script", projectHooksPath, printStdinScript)
 	mgr := GitLabHookManager{
+		cfg:     cfg,
 		locator: config.NewLocator(cfg),
-		hooksConfig: config.Hooks{
-			CustomHooksDir: globalCustomHooksDir,
-		},
 	}
 
 	hooksExecutor, err := mgr.newCustomHooksExecutor(repo, "pre-receive")
@@ -333,6 +335,8 @@ func TestMultipleScriptsStdin(t *testing.T) {
 
 	globalCustomHooksDir := testhelper.TempDir(t)
 
+	cfg.Hooks.CustomHooksDir = globalCustomHooksDir
+
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -345,10 +349,8 @@ func TestMultipleScriptsStdin(t *testing.T) {
 	}
 
 	mgr := GitLabHookManager{
+		cfg:     cfg,
 		locator: config.NewLocator(cfg),
-		hooksConfig: config.Hooks{
-			CustomHooksDir: globalCustomHooksDir,
-		},
 	}
 
 	hooksExecutor, err := mgr.newCustomHooksExecutor(repo, "pre-receive")
@@ -368,7 +370,7 @@ func TestMultipleScriptsStdin(t *testing.T) {
 	}
 }
 
-func callAndVerifyHooks(t *testing.T, locator storage.Locator, repo *gitalypb.Repository, hookName, globalHooksDir, hookDir, stdin string, args, env []string) {
+func callAndVerifyHooks(t *testing.T, cfg config.Cfg, locator storage.Locator, repo *gitalypb.Repository, hookName, hookDir, stdin string, args, env []string) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 	var stdout, stderr bytes.Buffer
@@ -377,10 +379,8 @@ func callAndVerifyHooks(t *testing.T, locator storage.Locator, repo *gitalypb.Re
 	defer cleanup()
 
 	mgr := GitLabHookManager{
+		cfg:     cfg,
 		locator: locator,
-		hooksConfig: config.Hooks{
-			CustomHooksDir: globalHooksDir,
-		},
 	}
 
 	callHooks, err := mgr.newCustomHooksExecutor(repo, hookName)
diff --git a/internal/gitaly/hook/manager.go b/internal/gitaly/hook/manager.go
index eac6fd6b7..5f0d93d36 100644
--- a/internal/gitaly/hook/manager.go
+++ b/internal/gitaly/hook/manager.go
@@ -49,18 +49,18 @@ type Manager interface {
 // GitLabHookManager is a hook manager containing Git hook business logic. It
 // uses the GitLab API to authenticate and track ongoing hook calls.
 type GitLabHookManager struct {
+	cfg          config.Cfg
 	locator      storage.Locator
 	gitlabClient gitlab.Client
-	hooksConfig  config.Hooks
 	txManager    transaction.Manager
 }
 
 // NewManager returns a new hook manager
 func NewManager(locator storage.Locator, txManager transaction.Manager, gitlabClient gitlab.Client, cfg config.Cfg) *GitLabHookManager {
 	return &GitLabHookManager{
+		cfg:          cfg,
 		locator:      locator,
 		gitlabClient: gitlabClient,
-		hooksConfig:  cfg.Hooks,
 		txManager:    txManager,
 	}
 }
diff --git a/internal/gitaly/hook/testhelper_test.go b/internal/gitaly/hook/testhelper_test.go
index 4c67f1cc5..f05093b4b 100644
--- a/internal/gitaly/hook/testhelper_test.go
+++ b/internal/gitaly/hook/testhelper_test.go
@@ -3,6 +3,7 @@ package hook
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"sort"
 	"strings"
 	"testing"
@@ -41,6 +42,8 @@ func getExpectedEnv(t testing.TB, cfg config.Cfg, repo *gitalypb.Repository) []s
 		expectedEnv[kv[0]] = kv[1]
 	}
 
+	expectedEnv["PATH"] = fmt.Sprintf("%s:%s", filepath.Dir(cfg.Git.BinPath), os.Getenv("PATH"))
+
 	result := make([]string, 0, len(expectedEnv))
 	for key, value := range expectedEnv {
 		result = append(result, fmt.Sprintf("%s=%s", key, value))