[BUG] Do not allow deletion of internal references

- Currently it's possible to modify remote references such as
`refs/pull/<idx>/head` and `refs/heads/<branch>`.
- Disallow that the pull request reference is deleted, as this should
not be at the control of the user. Doing so would result in
inconsistencies within Forgejo and lead to internal server errors when
trying access the pull request, this action should be reserved for
Forgejo.
- Do this by utilizing the `update` hook, which process each reference
individually and therefore allow to only skip deleting internal
references and still allow other modifications that is being done in
the same push.
- Ref: https://codeberg.org/Codeberg/Community/issues/1517
This commit is contained in:
Gusted 2024-03-26 23:28:10 +01:00
parent 8339b24d61
commit 9d82059840
No known key found for this signature in database
GPG key ID: FD821B732837125F
2 changed files with 41 additions and 2 deletions

View file

@ -293,9 +293,32 @@ Forgejo or set your environment appropriately.`, "")
return nil return nil
} }
// runHookUpdate process the update hook: https://git-scm.com/docs/githooks#update
func runHookUpdate(c *cli.Context) error { func runHookUpdate(c *cli.Context) error {
// Update is empty and is kept only for backwards compatibility // Now if we're an internal don't do anything else
if isInternal, _ := strconv.ParseBool(os.Getenv(repo_module.EnvIsInternal)); isInternal {
return nil return nil
}
ctx, cancel := installSignals()
defer cancel()
// The last three arguments given to the hook are in order: reference name, old commit ID and new commit ID.
args := os.Args[len(os.Args)-3:]
refFullName := git.RefName(args[0])
newCommitID := args[2]
// Only process pull references.
if !refFullName.IsPull() {
return nil
}
// Deletion of the ref means that the new commit ID is only composed of '0'.
if strings.ContainsFunc(newCommitID, func(e rune) bool { return e != '0' }) {
return nil
}
return fail(ctx, fmt.Sprintf("The deletion of %s is skipped as it's an internal reference.", refFullName), "")
} }
func runHookPostReceive(c *cli.Context) error { func runHookPostReceive(c *cli.Context) error {

View file

@ -84,6 +84,7 @@ func testGit(t *testing.T, u *url.URL) {
mediaTest(t, &httpContext, little, big, littleLFS, bigLFS) mediaTest(t, &httpContext, little, big, littleLFS, bigLFS)
t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "master", "test/head")) t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &httpContext, "master", "test/head"))
t.Run("InternalReferences", doInternalReferences(&httpContext, dstPath))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath)) t.Run("BranchProtectMerge", doBranchProtectPRMerge(&httpContext, dstPath))
t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath)) t.Run("AutoMerge", doAutoPRMerge(&httpContext, dstPath))
t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge")) t.Run("CreatePRAndSetManuallyMerged", doCreatePRAndSetManuallyMerged(httpContext, httpContext, dstPath, "master", "test-manually-merge"))
@ -125,6 +126,7 @@ func testGit(t *testing.T, u *url.URL) {
mediaTest(t, &sshContext, little, big, littleLFS, bigLFS) mediaTest(t, &sshContext, little, big, littleLFS, bigLFS)
t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "master", "test/head2")) t.Run("CreateAgitFlowPull", doCreateAgitFlowPull(dstPath, &sshContext, "master", "test/head2"))
t.Run("InternalReferences", doInternalReferences(&sshContext, dstPath))
t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath)) t.Run("BranchProtectMerge", doBranchProtectPRMerge(&sshContext, dstPath))
t.Run("MergeFork", func(t *testing.T) { t.Run("MergeFork", func(t *testing.T) {
defer tests.PrintCurrentTest(t)() defer tests.PrintCurrentTest(t)()
@ -734,6 +736,20 @@ func doAutoPRMerge(baseCtx *APITestContext, dstPath string) func(t *testing.T) {
} }
} }
func doInternalReferences(ctx *APITestContext, dstPath string) func(t *testing.T) {
return func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerName: ctx.Username, Name: ctx.Reponame})
pr1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{HeadRepoID: repo.ID})
_, stdErr, gitErr := git.NewCommand(git.DefaultContext, "push", "origin").AddDynamicArguments(fmt.Sprintf(":refs/pull/%d/head", pr1.Index)).RunStdString(&git.RunOpts{Dir: dstPath})
assert.Error(t, gitErr)
assert.Contains(t, stdErr, fmt.Sprintf("remote: Forgejo: The deletion of refs/pull/%d/head is skipped as it's an internal reference.", pr1.Index))
assert.Contains(t, stdErr, fmt.Sprintf("[remote rejected] refs/pull/%d/head (hook declined)", pr1.Index))
}
}
func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headBranch string) func(t *testing.T) { func doCreateAgitFlowPull(dstPath string, ctx *APITestContext, baseBranch, headBranch string) func(t *testing.T) {
return func(t *testing.T) { return func(t *testing.T) {
defer tests.PrintCurrentTest(t)() defer tests.PrintCurrentTest(t)()