From fcce196fb8df8c7a051df4f375e80b38c33b77c0 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Sun, 31 May 2026 14:41:28 +0200 Subject: [PATCH] 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 --- modules/git/hook.go | 36 +-- modules/git/repo.go | 7 +- modules/git/utils.go | 10 - .../hooks/pre-receive.sample | 24 ++ tests/integration/api_repo_git_hook_test.go | 240 ++++++++++-------- 5 files changed, 177 insertions(+), 140 deletions(-) create mode 100755 tests/gitea-repositories-meta/user2/git_hooks_test.git/hooks/pre-receive.sample diff --git a/modules/git/hook.go b/modules/git/hook.go index 3b650fe9db..7b3130f1c1 100644 --- a/modules/git/hook.go +++ b/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) diff --git a/modules/git/repo.go b/modules/git/repo.go index 47d65dcf69..6ab16d14e2 100644 --- a/modules/git/repo.go +++ b/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) diff --git a/modules/git/utils.go b/modules/git/utils.go index b84df47916..11697c1bd3 100644 --- a/modules/git/utils.go +++ b/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 { diff --git a/tests/gitea-repositories-meta/user2/git_hooks_test.git/hooks/pre-receive.sample b/tests/gitea-repositories-meta/user2/git_hooks_test.git/hooks/pre-receive.sample new file mode 100755 index 0000000000..dd98f55ea9 --- /dev/null +++ b/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 diff --git a/tests/integration/api_repo_git_hook_test.go b/tests/integration/api_repo_git_hook_test.go index aadc231d82..8ea6d3758e 100644 --- a/tests/integration/api_repo_git_hook_test.go +++ b/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) + }) }