fix(git): treat missing hooks folder the same as missing hook file (#12833)

Extracted from #12824 on suggestion of @limiting-factor; refactored after @Gusted pointed out forgejo/forgejo!12335.

Behavior change: previously a missing `hooks` folder in a repository tree (should not happen before forgejo/forgejo!12335) would return a 500 on `/api/v1/repos/%s/hooks/git`. It now returns a 200, with the same reply as an empty `hooks` folder.

Test has been added to ensure correct handling of missing `hooks` folder and of its creation if necessary.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12833
Reviewed-by: limiting-factor <limiting-factor@noreply.codeberg.org>

modules/git/hook.go

@@ -6,6 +6,7 @@ package git
 
 import (
 	"errors"
+	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
@@ -49,21 +50,28 @@ func GetHook(repoPath, name string) (*Hook, error) {
 		name: name,
 		path: path.Join(repoPath, "hooks", name+".d", name),
 	}
-	samplePath := filepath.Join(repoPath, "hooks", name+".sample")
-	if isFile(h.path) {
-		data, err := os.ReadFile(h.path)
-		if err != nil {
-			return nil, err
-		}
+
+	data, err := os.ReadFile(h.path)
+	if err == nil {
 		h.IsActive = true
 		h.Content = string(data)
-	} else if isFile(samplePath) {
-		data, err := os.ReadFile(samplePath)
-		if err != nil {
-			return nil, err
-		}
-		h.Sample = string(data)
+		return h, nil
+	} else if !errors.Is(err, fs.ErrNotExist) {
+		return nil, err
 	}
+
+	// Hook not found, look for sample.
+	// This should be rare, since git init is called with empty --template.
+	samplePath := filepath.Join(repoPath, "hooks", name+".sample")
+	data, err = os.ReadFile(samplePath)
+	if err == nil {
+		h.Sample = string(data)
+		return h, nil
+	} else if !errors.Is(err, fs.ErrNotExist) {
+		return nil, err
+	}
+
+	// neither hook, nor sample found
 	return h, nil
 }
 
@@ -99,10 +107,6 @@ func (h *Hook) Update() error {
 
 // ListHooks returns a list of Git hooks of given repository.
 func ListHooks(repoPath string) (_ []*Hook, err error) {
-	if !isDir(path.Join(repoPath, "hooks")) {
-		return nil, errors.New("hooks path does not exist")
-	}
-
 	hooks := make([]*Hook, len(hookNames))
 	for i, name := range hookNames {
 		hooks[i], err = GetHook(repoPath, name)

modules/git/repo.go

@@ -71,9 +71,10 @@ func InitRepository(ctx context.Context, repoPath string, bare bool, objectForma
 		return err
 	}
 
-	cmd := NewCommand(ctx, "init")
-	// Set template to an empty string so that example hooks, and other info files are not created
-	cmd.AddArguments("--template", "")
+	cmd := NewCommand(ctx, "init",
+		// Set template to an empty string so that example hooks, and other info files are not created
+		"--template", "",
+	)
 
 	if !IsValidObjectFormat(objectFormatName) {
 		return fmt.Errorf("invalid object format: %s", objectFormatName)

modules/git/utils.go

@@ -52,16 +52,6 @@ func isDir(dir string) bool {
 	return f.IsDir()
 }
 
-// isFile returns true if given path is a file,
-// or returns false when it's a directory or does not exist.
-func isFile(filePath string) bool {
-	f, e := os.Stat(filePath)
-	if e != nil {
-		return false
-	}
-	return !f.IsDir()
-}
-
 // isExist checks whether a file or directory exists.
 // It returns false when the file or directory does not exist.
 func isExist(path string) bool {

tests/gitea-repositories-meta/user2/git_hooks_test.git/hooks/pre-receive.sample

@@ -0,0 +1,24 @@
+#!/bin/bash
+#
+# An example hook script to make use of push options.
+# The example simply echoes all push options that start with 'echoback='
+# and rejects all pushes when the "reject" push option is used.
+#
+# To enable this hook, rename this file to "pre-receive".
+
+if test -n "$GIT_PUSH_OPTION_COUNT"
+then
+	i=0
+	while test "$i" -lt "$GIT_PUSH_OPTION_COUNT"
+	do
+		eval "value=\$GIT_PUSH_OPTION_$i"
+		case "$value" in
+		echoback=*)
+			echo "echo from the pre-receive-hook: ${value#*=}" >&2
+			;;
+		reject)
+			exit 1
+		esac
+		i=$((i + 1))
+	done
+fi

tests/integration/api_repo_git_hook_test.go

@@ -6,16 +6,19 @@ package integration
 import (
 	"fmt"
 	"net/http"
+	"os"
+	"slices"
 	"testing"
 
 	auth_model "forgejo.org/models/auth"
 	repo_model "forgejo.org/models/repo"
 	"forgejo.org/models/unittest"
-	user_model "forgejo.org/models/user"
 	api "forgejo.org/modules/structs"
 	"forgejo.org/tests"
+	"forgejo.org/tests/forgery"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 const testHookContent = `#!/bin/bash
@@ -23,16 +26,17 @@ const testHookContent = `#!/bin/bash
 echo Hello, World!
 `
 
+const repositoryIDWithPreReceiveHook = 37
+
 func TestAPIListGitHooks(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 37})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repositoryIDWithPreReceiveHook})
 
 	// user1 is an admin user
 	session := loginUser(t, "user1")
 	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git", owner.Name, repo.Name).
+	req := NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git", repo.FullName()).
 		AddTokenAuth(token)
 	resp := MakeRequest(t, req, http.StatusOK)
 	var apiGitHooks []*api.GitHook
@@ -49,50 +53,15 @@ func TestAPIListGitHooks(t *testing.T) {
 	}
 }
 
-func TestAPIListGitHooksNoHooks(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-
-	// user1 is an admin user
-	session := loginUser(t, "user1")
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git", owner.Name, repo.Name).
-		AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
-	var apiGitHooks []*api.GitHook
-	DecodeJSON(t, resp, &apiGitHooks)
-	assert.Len(t, apiGitHooks, 3)
-	for _, apiGitHook := range apiGitHooks {
-		assert.False(t, apiGitHook.IsActive)
-		assert.Empty(t, apiGitHook.Content)
-	}
-}
-
-func TestAPIListGitHooksNoAccess(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-
-	session := loginUser(t, owner.Name)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git", owner.Name, repo.Name).
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusForbidden)
-}
-
 func TestAPIGetGitHook(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 37})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repositoryIDWithPreReceiveHook})
 
 	// user1 is an admin user
 	session := loginUser(t, "user1")
 	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
+	req := NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
 		AddTokenAuth(token)
 	resp := MakeRequest(t, req, http.StatusOK)
 	var apiGitHook *api.GitHook
@@ -101,96 +70,145 @@ func TestAPIGetGitHook(t *testing.T) {
 	assert.Equal(t, testHookContent, apiGitHook.Content)
 }
 
-func TestAPIGetGitHookNoAccess(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-
-	session := loginUser(t, owner.Name)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
-	req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusForbidden)
-}
-
-func TestAPIEditGitHook(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-
-	// user1 is an admin user
-	session := loginUser(t, "user1")
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
-
-	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/hooks/git/pre-receive",
-		owner.Name, repo.Name)
-	req := NewRequestWithJSON(t, "PATCH", urlStr, &api.EditGitHookOption{
-		Content: testHookContent,
-	}).AddTokenAuth(token)
-	resp := MakeRequest(t, req, http.StatusOK)
-	var apiGitHook *api.GitHook
-	DecodeJSON(t, resp, &apiGitHook)
-	assert.True(t, apiGitHook.IsActive)
-	assert.Equal(t, testHookContent, apiGitHook.Content)
-
-	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
-		AddTokenAuth(token)
-	resp = MakeRequest(t, req, http.StatusOK)
-	var apiGitHook2 *api.GitHook
-	DecodeJSON(t, resp, &apiGitHook2)
-	assert.True(t, apiGitHook2.IsActive)
-	assert.Equal(t, testHookContent, apiGitHook2.Content)
-}
-
-func TestAPIEditGitHookNoAccess(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-
-	session := loginUser(t, owner.Name)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
-	urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name)
-	req := NewRequestWithJSON(t, "PATCH", urlStr, &api.EditGitHookOption{
-		Content: testHookContent,
-	}).AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusForbidden)
-}
-
 func TestAPIDeleteGitHook(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 37})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: repositoryIDWithPreReceiveHook})
 
 	// user1 is an admin user
 	session := loginUser(t, "user1")
 	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
 
-	req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
+	req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
 		AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusNoContent)
 
-	req = NewRequestf(t, "GET", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
+	req = NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
 		AddTokenAuth(token)
 	resp := MakeRequest(t, req, http.StatusOK)
 	var apiGitHook2 *api.GitHook
 	DecodeJSON(t, resp, &apiGitHook2)
 	assert.False(t, apiGitHook2.IsActive)
 	assert.Empty(t, apiGitHook2.Content)
+
+	// after deletion, the sample webhook should be shown in the web interface
+	resp = session.MakeRequest(t, NewRequestf(t, "GET", "/%s/settings/hooks/git/pre-receive", repo.FullName()), http.StatusOK)
+	htmlDoc := NewHTMLParser(t, resp.Body)
+	sampleHook := htmlDoc.doc.Find("#content").Text()
+	assert.Contains(t, sampleHook, "#!/bin/bash")
 }
 
-func TestAPIDeleteGitHookNoAccess(t *testing.T) {
+func TestAPIGitHooksFromEmpty(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
-	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
+	admin := forgery.CreateUser(t, &forgery.CreateUserOptions{
+		IsAdmin: true, // only admin can view git hooks
+	})
+	session := loginUser(t, admin.Name)
 
-	session := loginUser(t, owner.Name)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
-	req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/%s/hooks/git/pre-receive", owner.Name, repo.Name).
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusForbidden)
+	repo := forgery.CreateRepository(t, nil, nil) // admin does not need to own the repo
+
+	t.Run("initially empty", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
+
+		req := NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git", repo.FullName()).
+			AddTokenAuth(token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		var apiGitHooks []*api.GitHook
+		DecodeJSON(t, resp, &apiGitHooks)
+		assert.Len(t, apiGitHooks, 3)
+		for _, apiGitHook := range apiGitHooks {
+			assert.False(t, apiGitHook.IsActive)
+			assert.Empty(t, apiGitHook.Content)
+		}
+
+		// no hooks folder or description file should be present
+		entries, err := os.ReadDir(repo.RepoPath())
+		require.NoError(t, err)
+		assert.False(t, slices.ContainsFunc(entries, func(entry os.DirEntry) bool {
+			return entry.Name() == "hooks"
+		}), "hooks folder should be missing")
+		assert.False(t, slices.ContainsFunc(entries, func(entry os.DirEntry) bool {
+			return entry.Name() == "description"
+		}), "description file should be missing")
+	})
+
+	t.Run("create pre-receive", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+
+		urlStr := fmt.Sprintf("/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName())
+		req := NewRequestWithJSON(t, "PATCH", urlStr, &api.EditGitHookOption{
+			Content: testHookContent,
+		}).AddTokenAuth(token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		var apiGitHook *api.GitHook
+		DecodeJSON(t, resp, &apiGitHook)
+		assert.True(t, apiGitHook.IsActive)
+		assert.Equal(t, testHookContent, apiGitHook.Content)
+
+		req = NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
+			AddTokenAuth(token)
+		resp = MakeRequest(t, req, http.StatusOK)
+		var apiGitHook2 *api.GitHook
+		DecodeJSON(t, resp, &apiGitHook2)
+		assert.True(t, apiGitHook2.IsActive)
+		assert.Equal(t, testHookContent, apiGitHook2.Content)
+
+		// no description file should be present
+		entries, err := os.ReadDir(repo.RepoPath())
+		require.NoError(t, err)
+		assert.True(t, slices.ContainsFunc(entries, func(entry os.DirEntry) bool {
+			return entry.Name() == "hooks"
+		}), "hooks folder should be present")
+		assert.False(t, slices.ContainsFunc(entries, func(entry os.DirEntry) bool {
+			return entry.Name() == "description"
+		}), "description file should be missing")
+	})
+
+	t.Run("delete pre-receive", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+
+		req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusNoContent)
+	})
+}
+
+func TestAPIGitHookNoAccess(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	repo := forgery.CreateRepository(t, nil, nil)
+	owner := repo.Owner
+	require.False(t, owner.IsAdmin)
+	session := loginUser(t, owner.Name)
+
+	t.Run("Get", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
+		req := NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusForbidden)
+	})
+
+	t.Run("List", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository)
+		req := NewRequestf(t, "GET", "/api/v1/repos/%s/hooks/git", repo.FullName()).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusForbidden)
+	})
+
+	t.Run("Edit", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+		urlStr := fmt.Sprintf("/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName())
+		req := NewRequestWithJSON(t, "PATCH", urlStr, &api.EditGitHookOption{
+			Content: testHookContent,
+		}).AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusForbidden)
+	})
+
+	t.Run("Delete", func(t *testing.T) {
+		token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
+		req := NewRequestf(t, "DELETE", "/api/v1/repos/%s/hooks/git/pre-receive", repo.FullName()).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusForbidden)
+	})
 }