Fix raw file API ref handling (go-gitea#33172) (go-gitea#33189)

Backport go-gitea#33172 by wxiaoguang Fix go-gitea#33164 and add more tests Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

Author: GiteaBot

services/context/api.go

@@ -305,8 +305,7 @@ func RepoRefForAPI(next http.Handler) http.Handler {
 return
 }
 
-// NOTICE: the "ref" here for internal usage only (e.g. woodpecker)
-refName, _ := getRefNameLegacy(ctx.Base, ctx.Repo, ctx.FormTrim("ref"))
+refName, _ := getRefNameLegacy(ctx.Base, ctx.Repo, ctx.PathParam("*"), ctx.FormTrim("ref"))
 var err error
 
 if ctx.Repo.GitRepo.IsBranchExist(refName) {

services/context/repo.go

@@ -769,35 +769,30 @@ func getRefNameFromPath(repo *Repository, path string, isExist func(string) bool
 return ""
 }
 
-func getRefNameLegacy(ctx *Base, repo *Repository, optionalExtraRef ...string) (string, RepoRefType) {
-extraRef := util.OptionalArg(optionalExtraRef)
-reqPath := ctx.PathParam("*")
-reqPath = path.Join(extraRef, reqPath)
-
-if refName := getRefName(ctx, repo, RepoRefBranch); refName != "" {
+func getRefNameLegacy(ctx *Base, repo *Repository, reqPath, extraRef string) (string, RepoRefType) {
+reqRefPath := path.Join(extraRef, reqPath)
+reqRefPathParts := strings.Split(reqRefPath, "/")
+if refName := getRefName(ctx, repo, reqRefPath, RepoRefBranch); refName != "" {
 return refName, RepoRefBranch
 }
-if refName := getRefName(ctx, repo, RepoRefTag); refName != "" {
+if refName := getRefName(ctx, repo, reqRefPath, RepoRefTag); refName != "" {
 return refName, RepoRefTag
 }
-
-// For legacy support only full commit sha
-parts := strings.Split(reqPath, "/")
-if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), parts[0]) {
+if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), reqRefPathParts[0]) {
 // FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists
-repo.TreePath = strings.Join(parts[1:], "/")
-return parts[0], RepoRefCommit
+repo.TreePath = strings.Join(reqRefPathParts[1:], "/")
+return reqRefPathParts[0], RepoRefCommit
 }
-
-if refName := getRefName(ctx, repo, RepoRefBlob); len(refName) > 0 {
+if refName := getRefName(ctx, repo, reqPath, RepoRefBlob); refName != "" {
 return refName, RepoRefBlob
 }
+// FIXME: the old code falls back to default branch if "ref" doesn't exist, there could be an edge case:
+// "README?ref=no-such" would read the README file from the default branch, but the user might expect a 404
 repo.TreePath = reqPath
 return repo.Repository.DefaultBranch, RepoRefBranch
 }
 
-func getRefName(ctx *Base, repo *Repository, pathType RepoRefType) string {
-path := ctx.PathParam("*")
+func getRefName(ctx *Base, repo *Repository, path string, pathType RepoRefType) string {
 switch pathType {
 case RepoRefBranch:
 ref := getRefNameFromPath(repo, path, repo.GitRepo.IsBranchExist)
@@ -900,7 +895,8 @@ func RepoRefByType(detectRefType RepoRefType, opts ...RepoRefByTypeOptions) func
 }
 
 // Get default branch.
-if len(ctx.PathParam("*")) == 0 {
+reqPath := ctx.PathParam("*")
+if reqPath == "" {
 refName = ctx.Repo.Repository.DefaultBranch
 if !ctx.Repo.GitRepo.IsBranchExist(refName) {
 brs, _, err := ctx.Repo.GitRepo.GetBranches(0, 1)
@@ -925,12 +921,12 @@ func RepoRefByType(detectRefType RepoRefType, opts ...RepoRefByTypeOptions) func
 return cancel
 }
 ctx.Repo.IsViewBranch = true
-} else {
+} else { // there is a path in request
 guessLegacyPath := refType == RepoRefUnknown
 if guessLegacyPath {
-refName, refType = getRefNameLegacy(ctx.Base, ctx.Repo)
+refName, refType = getRefNameLegacy(ctx.Base, ctx.Repo, reqPath, "")
 } else {
-refName = getRefName(ctx.Base, ctx.Repo, refType)
+refName = getRefName(ctx.Base, ctx.Repo, reqPath, refType)
 }
 ctx.Repo.RefName = refName
 isRenamedBranch, has := ctx.Data["IsRenamedBranch"].(bool)

tests/integration/api_repo_get_contents_test.go

@@ -18,6 +18,7 @@ import (
 "code.gitea.io/gitea/modules/setting"
 api "code.gitea.io/gitea/modules/structs"
 repo_service "code.gitea.io/gitea/services/repository"
+"code.gitea.io/gitea/tests"
 
 "github.com/stretchr/testify/assert"
 )
@@ -168,28 +169,37 @@ func testAPIGetContents(t *testing.T, u *url.URL) {
 }
 
 func TestAPIGetContentsRefFormats(t *testing.T) {
-onGiteaRun(t, func(t *testing.T, u *url.URL) {
-file := "README.md"
-sha := "65f1bf27bc3bf70f64657658635e66094edbcb4d"
-content := "# repo1\n\nDescription for repo1"
-
-noRef := setting.AppURL + "api/v1/repos/user2/repo1/raw/" + file
-refInPath := setting.AppURL + "api/v1/repos/user2/repo1/raw/" + sha + "/" + file
-refInQuery := setting.AppURL + "api/v1/repos/user2/repo1/raw/" + file + "?ref=" + sha
-
-resp := MakeRequest(t, NewRequest(t, http.MethodGet, noRef), http.StatusOK)
-raw, err := io.ReadAll(resp.Body)
-assert.NoError(t, err)
-assert.EqualValues(t, content, string(raw))
-
-resp = MakeRequest(t, NewRequest(t, http.MethodGet, refInPath), http.StatusOK)
-raw, err = io.ReadAll(resp.Body)
-assert.NoError(t, err)
-assert.EqualValues(t, content, string(raw))
-
-resp = MakeRequest(t, NewRequest(t, http.MethodGet, refInQuery), http.StatusOK)
-raw, err = io.ReadAll(resp.Body)
-assert.NoError(t, err)
-assert.EqualValues(t, content, string(raw))
-})
+defer tests.PrepareTestEnv(t)()
+
+file := "README.md"
+sha := "65f1bf27bc3bf70f64657658635e66094edbcb4d"
+content := "# repo1\n\nDescription for repo1"
+
+resp := MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/"+file), http.StatusOK)
+raw, err := io.ReadAll(resp.Body)
+assert.NoError(t, err)
+assert.EqualValues(t, content, string(raw))
+
+resp = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/"+sha+"/"+file), http.StatusOK)
+raw, err = io.ReadAll(resp.Body)
+assert.NoError(t, err)
+assert.EqualValues(t, content, string(raw))
+
+resp = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/"+file+"?ref="+sha), http.StatusOK)
+raw, err = io.ReadAll(resp.Body)
+assert.NoError(t, err)
+assert.EqualValues(t, content, string(raw))
+
+resp = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/"+file+"?ref=master"), http.StatusOK)
+raw, err = io.ReadAll(resp.Body)
+assert.NoError(t, err)
+assert.EqualValues(t, content, string(raw))
+
+_ = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/docs/README.md?ref=main"), http.StatusNotFound)
+_ = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/README.md?ref=main"), http.StatusOK)
+_ = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/docs/README.md?ref=sub-home-md-img-check"), http.StatusOK)
+_ = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/README.md?ref=sub-home-md-img-check"), http.StatusNotFound)
+
+// FIXME: this is an incorrect behavior, non-existing branch falls back to default branch
+_ = MakeRequest(t, NewRequest(t, http.MethodGet, "/api/v1/repos/user2/repo1/raw/README.md?ref=no-such"), http.StatusOK)
 }