git724/forgejo
1
0
Fork 0
Code Issues Pull requests 3 Projects 1 Releases Packages Wiki Activity

feat: make API pull and compare endpoint references to head more robust (#8332)

Browse source 
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8332
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
This commit is contained in:
Earl Warren 2025-06-29 07:35:47 +02:00
commit ba37b69252
3 changed files with 84 additions and 36 deletions
Download patch file Download diff file Expand all files Collapse all files

31
routers/api/v1/repo/pull.go
View file

@ -1084,7 +1084,6 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
err error
)
// If there is no head repository, it means pull request between same repository.
headInfos := strings.Split(form.Head, ":")
if len(headInfos) == 1 {
isSameRepo = true
@ -1094,7 +1093,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
headUser, err = user_model.GetUserByName(ctx, headInfos[0])
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.NotFound("GetUserByName")
ctx.NotFound(fmt.Errorf("the owner %s does not exist", headInfos[0]))
} else {
ctx.Error(http.StatusInternalServerError, "GetUserByName", err)
}
@ -1104,7 +1103,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
// The head repository can also point to the same repo
isSameRepo = ctx.Repo.Owner.ID == headUser.ID
} else {
ctx.NotFound()
ctx.NotFound(fmt.Errorf("the head part of {basehead} %s must contain zero or one colon (:) but contains %d", form.Head, len(headInfos)-1))
return nil, nil, nil, "", ""
}
@ -1116,13 +1115,8 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(baseBranch)
baseIsTag := ctx.Repo.GitRepo.IsTagExist(baseBranch)
if !baseIsCommit && !baseIsBranch && !baseIsTag {
// Check for short SHA usage
if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(baseBranch); baseCommit != nil {
baseBranch = baseCommit.ID.String()
} else {
ctx.NotFound("BaseNotExist")
return nil, nil, nil, "", ""
}
ctx.NotFound(fmt.Errorf("could not find '%s' to be a commit, branch or tag in the base repository %s/%s", baseBranch, baseRepo.Owner.Name, baseRepo.Name))
return nil, nil, nil, "", ""
}
headRepo := repo_model.GetForkedRepo(ctx, headUser.ID, baseRepo.ID)
@ -1135,7 +1129,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID {
log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID)
ctx.NotFound("GetBaseRepo")
ctx.NotFound(fmt.Errorf("%[1]s does not have a fork of %[2]s/%[3]s and %[2]s/%[3]s is not a fork of a repository from %[1]s", headUser.Name, baseRepo.Owner.Name, baseRepo.Name))
return nil, nil, nil, "", ""
}
headRepo = baseRepo.BaseRepo
@ -1203,17 +1197,16 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption)
headIsBranch := headGitRepo.IsBranchExist(headBranch)
headIsTag := headGitRepo.IsTagExist(headBranch)
if !headIsCommit && !headIsBranch && !headIsTag {
// Check if headBranch is short sha commit hash
if headCommit, _ := headGitRepo.GetCommit(headBranch); headCommit != nil {
headBranch = headCommit.ID.String()
} else {
headGitRepo.Close()
ctx.NotFound("IsRefExist", nil)
return nil, nil, nil, "", ""
}
ctx.NotFound(fmt.Errorf("could not find '%s' to be a commit, branch or tag in the head repository %s/%s", headBranch, headRepo.Owner.Name, headRepo.Name))
return nil, nil, nil, "", ""
}
headBranchRef := headBranch
if headIsBranch {
headBranchRef = git.BranchPrefix + headBranch
} else if headIsTag {
headBranchRef = git.TagPrefix + headBranch
}
compareInfo, err := headGitRepo.GetCompareInfo(repo_model.RepoPath(baseRepo.Owner.Name, baseRepo.Name), baseBranchRef, headBranchRef, false, false)
if err != nil {

21
routers/web/repo/compare.go
View file

@ -312,22 +312,16 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo {
baseIsTag := ctx.Repo.GitRepo.IsTagExist(ci.BaseBranch)
if !baseIsCommit && !baseIsBranch && !baseIsTag {
// Check if baseBranch is short sha commit hash
if baseCommit, _ := ctx.Repo.GitRepo.GetCommit(ci.BaseBranch); baseCommit != nil {
ci.BaseBranch = baseCommit.ID.String()
ctx.Data["BaseBranch"] = ci.BaseBranch
baseIsCommit = true
} else if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() {
if ci.BaseBranch == ctx.Repo.GetObjectFormat().EmptyObjectID().String() {
if isSameRepo {
ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadBranch))
} else {
ctx.Redirect(ctx.Repo.RepoLink + "/compare/" + util.PathEscapeSegments(ci.HeadRepo.FullName()) + ":" + util.PathEscapeSegments(ci.HeadBranch))
}
return nil
} else {
ctx.NotFound("IsRefExist", nil)
return nil
}
return nil
}
ctx.Data["BaseIsCommit"] = baseIsCommit
ctx.Data["BaseIsBranch"] = baseIsBranch
@ -514,15 +508,8 @@ func ParseCompareInfo(ctx *context.Context) *common.CompareInfo {
headIsBranch := ci.HeadGitRepo.IsBranchExist(ci.HeadBranch)
headIsTag := ci.HeadGitRepo.IsTagExist(ci.HeadBranch)
if !headIsCommit && !headIsBranch && !headIsTag {
// Check if headBranch is short sha commit hash
if headCommit, _ := ci.HeadGitRepo.GetCommit(ci.HeadBranch); headCommit != nil {
ci.HeadBranch = headCommit.ID.String()
ctx.Data["HeadBranch"] = ci.HeadBranch
headIsCommit = true
} else {
ctx.NotFound("IsRefExist", nil)
return nil
}
ctx.NotFound("IsRefExist", nil)
return nil
}
ctx.Data["HeadIsCommit"] = headIsCommit
ctx.Data["HeadIsBranch"] = headIsBranch

68
tests/integration/api_repo_compare_test.go
View file

@ -7,7 +7,9 @@ import (
"encoding/base64"
"fmt"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"time"
@ -50,6 +52,28 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) {
}
}
requireErrorContains := func(t *testing.T, resp *httptest.ResponseRecorder, expected string) {
t.Helper()
type response struct {
Message string `json:"message"`
Errors []string `json:"errors"`
}
var bodyResp response
DecodeJSON(t, resp, &bodyResp)
if strings.Contains(bodyResp.Message, expected) {
return
}
for _, error := range bodyResp.Errors {
if strings.Contains(error, expected) {
return
}
}
t.Log(fmt.Sprintf("expected %s in %+v", expected, bodyResp))
t.Fail()
}
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
user2repo := "repoA"
user2Ctx := NewAPITestContext(t, user2.Name, user2repo, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
@ -123,6 +147,14 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) {
user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
user4Ctx := NewAPITestContext(t, user4.Name, user2repo, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser)
t.Run("ForkNotFound", func(t *testing.T) {
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/%s...%s:%s", user2.Name, user2repo, "master", user4.Name, user2branchName).
AddTokenAuth(user2Ctx.Token)
resp := MakeRequest(t, req, http.StatusNotFound)
requireErrorContains(t, resp, "user4 does not have a fork of user2/repoA and user2/repoA is not a fork of a repository from user4")
})
t.Run("User4ForksUser2Repository", doAPIForkRepository(user4Ctx, user2.Name))
user4branchName := "user4branch"
t.Run("CreateUser4RepositoryBranch", newBranchAndFile(user4Ctx, user4, user4branchName, "user4branchfilename.txt"))
@ -199,5 +231,41 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) {
assert.Len(t, apiResp.Files, 1)
})
}
t.Run("ForkUserDoesNotExist", func(t *testing.T) {
notUser := "notauser"
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/master...%s:branchname", user2.Name, user2repo, notUser).
AddTokenAuth(user2Ctx.Token)
resp := MakeRequest(t, req, http.StatusNotFound)
requireErrorContains(t, resp, fmt.Sprintf("the owner %s does not exist", notUser))
})
t.Run("HeadHasTooManyColon", func(t *testing.T) {
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/master...one:two:many", user2.Name, user2repo).
AddTokenAuth(user2Ctx.Token)
resp := MakeRequest(t, req, http.StatusNotFound)
requireErrorContains(t, resp, fmt.Sprintf("must contain zero or one colon (:) but contains 2"))
})
for _, testCase := range []struct {
what string
baseHead string
}{
{
what: "base",
baseHead: "notexists...master",
},
{
what: "head",
baseHead: "master...notexists",
},
} {
t.Run("BaseHeadNotExists "+testCase.what, func(t *testing.T) {
req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/%s", user2.Name, user2repo, testCase.baseHead).
AddTokenAuth(user2Ctx.Token)
resp := MakeRequest(t, req, http.StatusNotFound)
requireErrorContains(t, resp, fmt.Sprintf("could not find 'notexists' to be a commit, branch or tag in the %s", testCase.what))
})
}
})
}