From 65aae0912a64ce1f31852a9442a6d6c2d78ca3c7 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Fri, 10 Jan 2025 09:31:49 +0800
Subject: [PATCH] Fix raw file API ref handling (#33172)
Fix #33164 and add more tests
---
services/context/api.go | 3 +-
services/context/repo.go | 38 +++++++-------
.../integration/api_repo_get_contents_test.go | 50 +++++++++++--------
3 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/services/context/api.go b/services/context/api.go
index bda705cb48..3a3cbe670e 100644
--- a/services/context/api.go
+++ b/services/context/api.go
@@ -293,8 +293,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) {
diff --git a/services/context/repo.go b/services/context/repo.go
index 4de905ef2c..94b2972c2b 100644
--- a/services/context/repo.go
+++ b/services/context/repo.go
@@ -765,35 +765,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)
@@ -889,7 +884,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)
@@ -914,12 +910,12 @@ func RepoRefByType(detectRefType RepoRefType, opts ...RepoRefByTypeOptions) func
return
}
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)
diff --git a/tests/integration/api_repo_get_contents_test.go b/tests/integration/api_repo_get_contents_test.go
index 68a8608117..d0f61da0c0 100644
--- a/tests/integration/api_repo_get_contents_test.go
+++ b/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"
+ defer tests.PrepareTestEnv(t)()
- 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
+ file := "README.md"
+ sha := "65f1bf27bc3bf70f64657658635e66094edbcb4d"
+ content := "# repo1\n\nDescription for repo1"
- 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, "/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, 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, "/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, refInQuery), 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)
}