Fix handling of symlinks in custom hooks directory

Previously if there were symlinks in the custom hook directory pointing
to a directory or some invalid executable, Gitaly would attempt to
execute that file. This would result in a "permission denied" error
because you can't execute a directory, for example.

We now resolve the symlinks and only execute them if they point to valid
executables.

Closes https://gitlab.com/gitlab-org/gitaly/-/issues/3282

3 files changed, 105 insertions, 5 deletions

diff --git a/changelogs/unreleased/sh-fix-symlinks-to-dirs-custom-hooks.yml b/changelogs/unreleased/sh-fix-symlinks-to-dirs-custom-hooks.yml
new file mode 100644
index 000000000..911bd4049
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-symlinks-to-dirs-custom-hooks.yml
@@ -0,0 +1,5 @@
+---
+title: Fix handling of symlinks in custom hooks directory
+merge_request: 2790
+author:
+type: fixed
diff --git a/internal/gitaly/hook/custom.go b/internal/gitaly/hook/custom.go
index 1e295159a..428a6fcba 100644
--- a/internal/gitaly/hook/custom.go
+++ b/internal/gitaly/hook/custom.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+	"golang.org/x/sys/unix"
 )
 
 // customHooksExecutor executes all custom hooks for a given repository and hook name
@@ -95,17 +96,41 @@ func matchFiles(dir string) ([]string, error) {
 	var hookFiles []string
 
 	for _, fi := range fis {
-		if fi.IsDir() {
+		if fi.IsDir() || strings.HasSuffix(fi.Name(), "~") {
 			continue
 		}
-		if fi.Mode()&0100 == 0 {
-			continue
+
+		filename := filepath.Join(dir, fi.Name())
+
+		if fi.Mode()&os.ModeSymlink != 0 {
+			path, err := filepath.EvalSymlinks(filename)
+			if err != nil {
+				continue
+			}
+
+			fi, err = os.Lstat(path)
+			if err != nil {
+				continue
+			}
 		}
-		if strings.HasSuffix(fi.Name(), "~") {
+
+		if !validHook(fi, filename) {
 			continue
 		}
-		hookFiles = append(hookFiles, filepath.Join(dir, fi.Name()))
+
+		hookFiles = append(hookFiles, filename)
 	}
 
 	return hookFiles, nil
 }
+
+func validHook(fi os.FileInfo, filename string) bool {
+	if fi.IsDir() {
+		return false
+	}
+	if unix.Access(filename, unix.X_OK) != nil {
+		return false
+	}
+
+	return true
+}
diff --git a/internal/gitaly/hook/custom_test.go b/internal/gitaly/hook/custom_test.go
index c20675d78..c9bac564d 100644
--- a/internal/gitaly/hook/custom_test.go
+++ b/internal/gitaly/hook/custom_test.go
@@ -223,6 +223,76 @@ func TestCustomHooksMultipleHooks(t *testing.T) {
 	}
 }
 
+func TestCustomHooksWithSymlinks(t *testing.T) {
+	testRepo, _, cleanup := testhelper.NewTestRepo(t)
+	defer cleanup()
+
+	globalCustomHooksDir, cleanup := testhelper.TempDir(t)
+	defer cleanup()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	globalHooksPath := filepath.Join(globalCustomHooksDir, "update.d")
+
+	// Test directory structure:
+	//
+	// first_dir/update
+	// first_dir/update~
+	// second_dir -> first_dir
+	// update -> second_dir/update         GOOD
+	// update_tilde -> first_dir/update~   GOOD
+	// update~ -> first_dir/update         BAD
+	// something -> not-executable         BAD
+	// bad -> /path/to/nowhere             BAD
+	firstDir := filepath.Join(globalHooksPath, "first_dir")
+	secondDir := filepath.Join(globalHooksPath, "second_dir")
+	require.NoError(t, os.MkdirAll(firstDir, 0755))
+	require.NoError(t, os.Symlink(firstDir, secondDir))
+	filename := filepath.Join(firstDir, "update")
+
+	updateTildePath := filepath.Join(globalHooksPath, "update_tilde")
+	require.NoError(t, os.Symlink(filename, updateTildePath))
+
+	updateHookPath := filepath.Join(globalHooksPath, "update")
+	require.NoError(t, os.Symlink(filename, updateHookPath))
+
+	badUpdatePath := filepath.Join(globalHooksPath, "update~")
+	badUpdateHook := filepath.Join(firstDir, "update~")
+	require.NoError(t, os.Symlink(badUpdateHook, badUpdatePath))
+
+	notExecPath := filepath.Join(globalHooksPath, "not-executable")
+	badExecHook := filepath.Join(firstDir, "something")
+	os.Create(notExecPath)
+	require.NoError(t, os.Symlink(notExecPath, badExecHook))
+
+	badPath := filepath.Join(globalHooksPath, "bad")
+	require.NoError(t, os.Symlink("/path/to/nowhere", badPath))
+
+	writeCustomHook(t, "update", firstDir, successScript)
+	writeCustomHook(t, "update~", firstDir, successScript)
+
+	expectedExecutedScripts := []string{updateHookPath, updateTildePath}
+
+	mgr := GitLabHookManager{
+		hooksConfig: config.Hooks{
+			CustomHooksDir: globalCustomHooksDir,
+		},
+	}
+	hooksExecutor, err := mgr.newCustomHooksExecutor(testRepo, "update")
+	require.NoError(t, err)
+
+	var stdout, stderr bytes.Buffer
+	require.NoError(t, hooksExecutor(ctx, nil, nil, &bytes.Buffer{}, &stdout, &stderr))
+	require.Empty(t, stderr.Bytes())
+
+	outputScanner := bufio.NewScanner(&stdout)
+	for _, expectedScript := range expectedExecutedScripts {
+		require.True(t, outputScanner.Scan())
+		require.Equal(t, expectedScript, outputScanner.Text())
+	}
+}
+
 func TestMultilineStdin(t *testing.T) {
 	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()