TheAfriForge/gitforge
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-07-18 23:47:50 +00:00
Code Issues Projects Releases Packages Wiki Activity

[v15.0/forgejo] fix: don't cache write permission on first pushed git reference (#13372)

Browse source 
Forgejo's git pre-receive hook validates whether the authenticated user can write to the target branch during a push operation.  A caching performance optimization in the hook caused the pre-receive hook to validate only the first branch reference in the push.  While typically a push operation has the same rights for all branches on a repository, some exceptions exist which could allow a user to push to a branch, pass the validation, and then bypass checks on additional references due to the cache.  The cache has been removed, forcing all references to be validated for code write permissions.

Thanks to Gitea for identifying this issue in https://github.com/go-gitea/gitea/pull/38103/changes/f25811942cea35233299efdbdfeec40663d4807a. This effort has been re-engineered for Forgejo, consistent with Forgejo's AI Agreement.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13372
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
This commit is contained in:
Mathieu Fenniak 2026-07-09 06:05:51 +02:00 • committed by Beowulf
commit 46f47fd284
4 changed files with 149 additions and 41 deletions
Download patch file Download diff file Expand all files Collapse all files

82
routers/private/hook_pre_receive.go
View file

@ -37,12 +37,6 @@ type preReceiveContext struct {
userPerm access_model.Permission
deployKeyAccessMode perm_model.AccessMode
canCreatePullRequest bool
checkedCanCreatePullRequest bool
canWriteCode bool
checkedCanWriteCode bool
protectedTags []*git_model.ProtectedTag
gotProtectedTags bool
@ -51,34 +45,31 @@ type preReceiveContext struct {
opts *private.HookOptions
isOverQuota bool
branchName string
}
// CanWriteCode returns true if pusher can write code
func (ctx *preReceiveContext) CanWriteCode() bool {
if !ctx.checkedCanWriteCode {
if !ctx.loadPusherAndPermission() {
return false
}
ctx.canWriteCode = issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, ctx.branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
ctx.checkedCanWriteCode = true
// canWriteCodeToBranch returns true if pusher can write code to the specified branch name on the ctx repository.
// Permitted if the user has write access to the code unit of the repository, or if there is a PR that allows maintainer
// edit, or if a write deploy key is in-use.
func (ctx *preReceiveContext) canWriteCodeToBranch(branchName string) bool {
if !ctx.loadPusherAndPermission() {
return false
}
return ctx.canWriteCode
return issues_model.CanMaintainerWriteToBranch(ctx, ctx.userPerm, branchName, ctx.user) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
}
// AssertCanWriteCode returns true if pusher can write code
func (ctx *preReceiveContext) AssertCanWriteCode() bool {
if !ctx.CanWriteCode() {
// assertCanWriteCodeToBranch verifies that the pusher can write code to the specified branch name on the ctx
// repository. If the user cannot write, an error response is written to the HTTP context.
func (ctx *preReceiveContext) assertCanWriteCodeToBranch(branchName string) bool {
if !ctx.canWriteCodeToBranch(branchName) {
if ctx.Written() {
return false
}
var sb strings.Builder
fmt.Fprintf(&sb, "User '%s' is not allowed to push to branch '%s' in '%s/%s'.", ctx.user.Name, ctx.branchName, ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name)
fmt.Fprintf(&sb, "User '%s' is not allowed to push to branch '%s' in '%s/%s'.", ctx.user.Name, branchName, ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name)
if ctx.CanCreatePullRequest() {
fmt.Fprintf(&sb, "\nIf you instead wanted to create a pull request to the branch '%s', please use:", ctx.branchName)
fmt.Fprintf(&sb, "\ngit push origin HEAD:refs/for/%s/choose-a-descriptor", ctx.branchName)
fmt.Fprintf(&sb, "\nIf you instead wanted to create a pull request to the branch '%s', please use:", branchName)
fmt.Fprintf(&sb, "\ngit push origin HEAD:refs/for/%s/choose-a-descriptor", branchName)
sb.WriteString("\nYou might want to replace 'origin' with the name of your Git remote if it is different from origin. You can freely choose the descriptor to set it to a topic.")
sb.WriteString("\nYou can learn about creating pull requests with AGit in the docs: https://forgejo.org/docs/latest/user/agit-support/")
}
@ -90,16 +81,39 @@ func (ctx *preReceiveContext) AssertCanWriteCode() bool {
return true
}
// CanCreatePullRequest returns true if pusher can create pull requests
func (ctx *preReceiveContext) CanCreatePullRequest() bool {
if !ctx.checkedCanCreatePullRequest {
if !ctx.loadPusherAndPermission() {
// canWriteCodeToBranch returns true if pusher can write code to the ctx repository. Use [canWriteCodeToBranch] instead
// if the write is to a named branch. Permitted if the user has write access to the code unit, or if a write deploy key
// is in-use.
func (ctx *preReceiveContext) canWriteCode() bool {
if !ctx.loadPusherAndPermission() {
return false
}
return ctx.userPerm.CanWrite(unit.TypeCode) || ctx.deployKeyAccessMode >= perm_model.AccessModeWrite
}
// assertCanWriteCode verifies that the pusher can write code to the repository. Use [assertCanWriteCodeToBranch]
// instead if the write is to a named branch. If the user cannot write, an error response is written to the HTTP
// context.
func (ctx *preReceiveContext) assertCanWriteCode() bool {
if !ctx.canWriteCode() {
if ctx.Written() {
return false
}
ctx.canCreatePullRequest = ctx.userPerm.CanRead(unit.TypePullRequests)
ctx.checkedCanCreatePullRequest = true
msg := fmt.Sprintf("User '%s' is not allowed to push to repository '%s/%s'.", ctx.user.Name, ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name)
ctx.JSON(http.StatusForbidden, private.Response{
UserMsg: msg,
})
return false
}
return ctx.canCreatePullRequest
return true
}
// CanCreatePullRequest returns true if pusher can create pull requests
func (ctx *preReceiveContext) CanCreatePullRequest() bool {
if !ctx.loadPusherAndPermission() {
return false
}
return ctx.userPerm.CanRead(unit.TypePullRequests)
}
// AssertCreatePullRequest returns true if can create pull requests
@ -222,7 +236,7 @@ func HookPreReceive(ctx *app_context.PrivateContext) {
ourCtx.quotaExceeded()
return
}
ourCtx.AssertCanWriteCode()
ourCtx.assertCanWriteCode()
}
if ctx.Written() {
return
@ -234,9 +248,7 @@ func HookPreReceive(ctx *app_context.PrivateContext) {
func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, refFullName git.RefName) {
branchName := refFullName.BranchName()
ctx.branchName = branchName
if !ctx.AssertCanWriteCode() {
if !ctx.assertCanWriteCodeToBranch(branchName) {
return
}
@ -476,7 +488,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r
}
func preReceiveTag(ctx *preReceiveContext, oldCommitID, newCommitID string, refFullName git.RefName) { //nolint:unparam
if !ctx.AssertCanWriteCode() {
if !ctx.assertCanWriteCode() {
return
}

14
tests/forgery/repo.go
View file

@ -26,9 +26,10 @@ type CreateRepositoryOptions struct {
// Use [MapFS] or [FilesInit] to setup the initial files.
Files fs.FS
ObjectFormat git.ObjectFormat // If nil, SHA1
IsTemplate bool
IsPrivate bool
ObjectFormat git.ObjectFormat // If nil, SHA1
IsTemplate bool
IsPrivate bool
DefaultBranch string
LatestSha *string // if not nil, the commit sha after initializing the repo with the Files will be written to this ref
SkipCleanup bool // if true the repo will not be deleted at the end of the test (can be useful to debug locally)
@ -63,11 +64,16 @@ func CreateRepository(t testing.TB, owner *user_model.User, opts *CreateReposito
gitFormat := cmp.Or(opts.ObjectFormat, git.Sha1ObjectFormat)
defaultBranch := "main"
if opts.DefaultBranch != "" {
defaultBranch = opts.DefaultBranch
}
// Create the repository
createOptions := repo_service.CreateRepoOptions{
Name: repoName,
Description: "Test Repo",
DefaultBranch: "main",
DefaultBranch: defaultBranch,
IsTemplate: opts.IsTemplate,
ObjectFormatName: gitFormat.Name(),
IsPrivate: opts.IsPrivate,

11
tests/integration/git_helper_for_declarative_test.go
View file

@ -22,6 +22,7 @@ import (
"forgejo.org/modules/util"
"forgejo.org/tests"
gouuid "github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@ -194,7 +195,7 @@ func doGitAddSomeCommits(dstPath, branch string) func(*testing.T) {
return func(t *testing.T) {
doGitCheckoutBranch(dstPath, branch)(t)
require.NoError(t, os.WriteFile(filepath.Join(dstPath, fmt.Sprintf("file-%s.txt", branch)), fmt.Appendf(nil, "file %s", branch), 0o644))
require.NoError(t, os.WriteFile(filepath.Join(dstPath, fmt.Sprintf("file-%s.txt", branch)), fmt.Appendf(nil, "file %s %s", branch, gouuid.New().String()), 0o644))
require.NoError(t, git.AddChanges(dstPath, true))
signature := git.Signature{
Email: "test@test.test",
@ -231,3 +232,11 @@ func doGitPull(dstPath string, args ...string) func(*testing.T) {
require.NoError(t, err)
}
}
func doGitFetch(dstPath string, args ...string) func(*testing.T) {
return func(t *testing.T) {
t.Helper()
_, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("fetch").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath})
require.NoError(t, err)
}
}

83
tests/integration/git_push_test.go
View file

@ -11,6 +11,7 @@ import (
"forgejo.org/models/db"
git_model "forgejo.org/models/git"
issues_model "forgejo.org/models/issues"
repo_model "forgejo.org/models/repo"
"forgejo.org/models/unittest"
user_model "forgejo.org/models/user"
@ -18,8 +19,10 @@ import (
"forgejo.org/modules/log"
repo_module "forgejo.org/modules/repository"
"forgejo.org/modules/test"
pull_service "forgejo.org/services/pull"
repo_service "forgejo.org/services/repository"
"forgejo.org/tests"
"forgejo.org/tests/forgery"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -282,7 +285,7 @@ func testOptionsGitPush(t *testing.T, u *url.URL) {
})
// give write access to the collaborator
repo_module.AddCollaborator(db.DefaultContext, repo, collaborator)
require.NoError(t, repo_module.AddCollaborator(db.DefaultContext, repo, collaborator))
t.Run("Collaborator with write access is allowed to push", func(t *testing.T) {
branchName := "branch4"
@ -313,3 +316,81 @@ func testOptionsGitPush(t *testing.T, u *url.URL) {
require.NoError(t, repo_service.DeleteRepositoryDirectly(db.DefaultContext, repo.ID, repo_service.DeleteRepositoryOpts{}))
})
}
func TestGitPushAllowMaintainerEditRestrictedHead(t *testing.T) {
onApplicationRun(t, func(t *testing.T, u *url.URL) {
baseRepoOwner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
// Create a base repository as a target for the pull request
baseRepo := forgery.CreateRepository(t, baseRepoOwner, &forgery.CreateRepositoryOptions{DefaultBranch: "master"})
baseRepoPath := t.TempDir()
doGitInitTestRepository(baseRepoPath, git.Sha1ObjectFormat)(t)
u.Path = baseRepo.FullName() + ".git"
u.User = url.UserPassword(baseRepoOwner.LowerName, userPassword)
doGitAddRemote(baseRepoPath, "origin", u)(t)
doGitPushTestRepository(baseRepoPath, "origin", baseRepo.DefaultBranch)(t)
// Fork the base repo
forkUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
forkRepo, err := repo_service.ForkRepositoryAndUpdates(t.Context(), forkUser, forkUser, repo_service.ForkRepoOptions{
BaseRepo: baseRepo,
Name: "repo-pr-update",
Description: "desc",
})
require.NoError(t, err)
forkRepoPath := t.TempDir()
u.Path = forkRepo.FullName() + ".git"
u.User = url.UserPassword(forkUser.LowerName, userPassword)
doGitClone(forkRepoPath, u)(t)
// Make a modification in the fork repo
branchName := "my-branch-for-pr"
doGitCreateBranch(forkRepoPath, branchName)(t)
doGitAddSomeCommits(forkRepoPath, branchName)(t)
doGitPushTestRepository(forkRepoPath, "origin", branchName)(t)
// Create a pull request in the base repo to incorporate the fork's modification
pullIssue := &issues_model.Issue{
RepoID: baseRepo.ID,
Title: "Test Pull Request from Fork",
PosterID: forkUser.ID,
Poster: forkUser,
IsPull: true,
}
pullRequest := &issues_model.PullRequest{
HeadRepo: forkRepo,
HeadRepoID: forkRepo.ID,
HeadBranch: branchName,
BaseRepo: baseRepo,
BaseRepoID: baseRepo.ID,
BaseBranch: baseRepo.DefaultBranch,
Type: issues_model.PullRequestGitea,
AllowMaintainerEdit: true,
}
err = pull_service.NewPullRequest(git.DefaultContext, baseRepo, pullIssue, nil, nil, pullRequest, nil)
require.NoError(t, err)
// The existence of the pull request allows maintainers of the base repo (baseRepoOwner) to write to the fork
// repo, but *only* to the branch for the pull request. Set up for editing as the baseRepoOwner...
u.Path = forkRepo.FullName() + ".git"
u.User = url.UserPassword(baseRepoOwner.LowerName, userPassword)
doGitAddRemote(baseRepoPath, "fork", u)(t)
doGitFetch(baseRepoPath, "fork")(t)
doGitCheckoutBranch(baseRepoPath, branchName)(t)
// Test writing to the PR branch, should succeed:
doGitAddSomeCommits(baseRepoPath, branchName)(t)
doGitPushTestRepository(baseRepoPath, "fork", branchName)(t)
// We're allowed to write to the PR branch, but not to another branch:
doGitCreateBranch(baseRepoPath, "another-branch")(t)
doGitAddSomeCommits(baseRepoPath, "another-branch")(t)
doGitPushTestRepositoryFail(baseRepoPath, "fork", "another-branch")(t)
// Verify that each branch being pushed is checked independently -- pushing to a branch we're permitted to, and
// then a branch that we're not, does not allow the push:
doGitAddSomeCommits(baseRepoPath, branchName)(t) // Ensure we have new commits ready to push
doGitAddSomeCommits(baseRepoPath, "another-branch")(t) // Ensure we have new commits ready to push
doGitPushTestRepositoryFail(baseRepoPath, "fork", branchName, "another-branch")(t)
})
}