Fix "redirect link" handling (#33440)

`a%2fb` should not redirect to `a/b`

---------

Co-authored-by: delvh <[email protected]>

Author: wxiaoguang

routers/web/repo/branch.go

@@ -93,7 +93,7 @@ func Branches(ctx *context.Context) {
 
 // DeleteBranchPost responses for delete merged branch
 func DeleteBranchPost(ctx *context.Context) {
-	defer redirect(ctx)
+	defer jsonRedirectBranches(ctx)
 	branchName := ctx.FormString("name")
 
 	if err := repo_service.DeleteBranch(ctx, ctx.Doer, ctx.Repo.Repository, ctx.Repo.GitRepo, branchName, nil); err != nil {
@@ -120,7 +120,7 @@ func DeleteBranchPost(ctx *context.Context) {
 
 // RestoreBranchPost responses for delete merged branch
 func RestoreBranchPost(ctx *context.Context) {
-	defer redirect(ctx)
+	defer jsonRedirectBranches(ctx)
 
 	branchID := ctx.FormInt64("branch_id")
 	branchName := ctx.FormString("name")
@@ -170,7 +170,7 @@ func RestoreBranchPost(ctx *context.Context) {
 	ctx.Flash.Success(ctx.Tr("repo.branch.restore_success", deletedBranch.Name))
 }
 
-func redirect(ctx *context.Context) {
+func jsonRedirectBranches(ctx *context.Context) {
 	ctx.JSONRedirect(ctx.Repo.RepoLink + "/branches?page=" + url.QueryEscape(ctx.FormString("page")))
 }
 

routers/web/repo/view_home.go

@@ -413,8 +413,19 @@ func Home(ctx *context.Context) {
 	ctx.HTML(http.StatusOK, tplRepoHome)
 }
 
-// HomeRedirect redirects from /tree/* to /src/* in order to maintain a similar URL structure.
-func HomeRedirect(ctx *context.Context) {
-	remainder := ctx.PathParam("*")
-	ctx.Redirect(ctx.Repo.RepoLink + "/src/" + util.PathEscapeSegments(remainder))
+func RedirectRepoTreeToSrc(ctx *context.Context) {
+	// Redirect "/owner/repo/tree/*" requests to "/owner/repo/src/*",
+	// then use the deprecated "/src/*" handler to guess the ref type and render a file list page.
+	// This is done intentionally so that Gitea's repo URL structure matches other forges (GitHub/GitLab) provide,
+	// allowing us to construct submodule URLs across forges easily.
+	// For example, when viewing a submodule, we can simply construct the link as:
+	// * "https://gitea/owner/repo/tree/{CommitID}"
+	// * "https://github/owner/repo/tree/{CommitID}"
+	// * "https://gitlab/owner/repo/tree/{CommitID}"
+	// Then no matter which forge the submodule is using, the link works.
+	redirect := ctx.Repo.RepoLink + "/src/" + ctx.PathParamRaw("*")
+	if ctx.Req.URL.RawQuery != "" {
+		redirect += "?" + ctx.Req.URL.RawQuery
+	}
+	ctx.Redirect(redirect)
 }

routers/web/web.go

@@ -1583,13 +1583,7 @@ func registerRoutes(m *web.Router) {
 			m.Get("/commit/*", context.RepoRefByType(git.RefTypeCommit), repo.Home)
 			m.Get("/*", context.RepoRefByType(""), repo.Home) // "/*" route is deprecated, and kept for backward compatibility
 		}, repo.SetEditorconfigIfExists)
-
-		// Add a /tree/* path to redirect to the /src/* path, which
-		// will redirect to the canonical URL for that ref. This is
-		// included so that Gitea's repo URL structure matches what
-		// other forges provide, allowing clients to construct URLs
-		// that work across forges.
-		m.Get("/tree/*", repo.HomeRedirect)
+		m.Get("/tree/*", repo.RedirectRepoTreeToSrc) // redirect "/owner/repo/tree/*" requests to "/owner/repo/src/*"
 
 		m.Get("/forks", context.RepoRef(), repo.Forks)
 		m.Get("/commit/{sha:([a-f0-9]{7,64})}.{ext:patch|diff}", repo.MustBeNotEmpty, repo.RawDiff)

services/context/api.go

@@ -293,7 +293,7 @@ func RepoRefForAPI(next http.Handler) http.Handler {
 			return
 		}
 
-		refName, _ := getRefNameLegacy(ctx.Base, ctx.Repo, ctx.PathParam("*"), 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

@@ -686,24 +686,24 @@ func getRefNameFromPath(repo *Repository, path string, isExist func(string) bool
 	return ""
 }
 
-func getRefNameLegacy(ctx *Base, repo *Repository, reqPath, extraRef string) (string, git.RefType) {
+func getRefNameLegacy(ctx *Base, repo *Repository, reqPath, extraRef string) (refName string, refType git.RefType, fallbackDefaultBranch bool) {
 	reqRefPath := path.Join(extraRef, reqPath)
 	reqRefPathParts := strings.Split(reqRefPath, "/")
 	if refName := getRefName(ctx, repo, reqRefPath, git.RefTypeBranch); refName != "" {
-		return refName, git.RefTypeBranch
+		return refName, git.RefTypeBranch, false
 	}
 	if refName := getRefName(ctx, repo, reqRefPath, git.RefTypeTag); refName != "" {
-		return refName, git.RefTypeTag
+		return refName, git.RefTypeTag, false
 	}
 	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(reqRefPathParts[1:], "/")
-		return reqRefPathParts[0], git.RefTypeCommit
+		return reqRefPathParts[0], git.RefTypeCommit, false
 	}
 	// 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, git.RefTypeBranch
+	return repo.Repository.DefaultBranch, git.RefTypeBranch, true
 }
 
 func getRefName(ctx *Base, repo *Repository, path string, refType git.RefType) string {
@@ -838,8 +838,9 @@ func RepoRefByType(detectRefType git.RefType) func(*Context) {
 			}
 		} else { // there is a path in request
 			guessLegacyPath := refType == ""
+			fallbackDefaultBranch := false
 			if guessLegacyPath {
-				refShortName, refType = getRefNameLegacy(ctx.Base, ctx.Repo, reqPath, "")
+				refShortName, refType, fallbackDefaultBranch = getRefNameLegacy(ctx.Base, ctx.Repo, reqPath, "")
 			} else {
 				refShortName = getRefName(ctx.Base, ctx.Repo, reqPath, refType)
 			}
@@ -897,12 +898,24 @@ func RepoRefByType(detectRefType git.RefType) func(*Context) {
 
 			if guessLegacyPath {
 				// redirect from old URL scheme to new URL scheme
-				prefix := strings.TrimPrefix(setting.AppSubURL+strings.ToLower(strings.TrimSuffix(ctx.Req.URL.Path, ctx.PathParam("*"))), strings.ToLower(ctx.Repo.RepoLink))
-				redirect := path.Join(
-					ctx.Repo.RepoLink,
-					util.PathEscapeSegments(prefix),
-					ctx.Repo.RefTypeNameSubURL(),
-					util.PathEscapeSegments(ctx.Repo.TreePath))
+				// * /user2/repo1/commits/master => /user2/repo1/commits/branch/master
+				// * /user2/repo1/src/master => /user2/repo1/src/branch/master
+				// * /user2/repo1/src/README.md => /user2/repo1/src/branch/master/README.md (fallback to default branch)
+				var redirect string
+				refSubPath := "src"
+				// remove the "/subpath/owner/repo/" prefix, the names are case-insensitive
+				remainingLowerPath, cut := strings.CutPrefix(setting.AppSubURL+strings.ToLower(ctx.Req.URL.Path), strings.ToLower(ctx.Repo.RepoLink)+"/")
+				if cut {
+					refSubPath, _, _ = strings.Cut(remainingLowerPath, "/") // it could be "src" or "commits"
+				}
+				if fallbackDefaultBranch {
+					redirect = fmt.Sprintf("%s/%s/%s/%s/%s", ctx.Repo.RepoLink, refSubPath, refType, util.PathEscapeSegments(refShortName), ctx.PathParamRaw("*"))
+				} else {
+					redirect = fmt.Sprintf("%s/%s/%s/%s", ctx.Repo.RepoLink, refSubPath, refType, ctx.PathParamRaw("*"))
+				}
+				if ctx.Req.URL.RawQuery != "" {
+					redirect += "?" + ctx.Req.URL.RawQuery
+				}
 				ctx.Redirect(redirect)
 				return
 			}

tests/integration/links_test.go

@@ -52,9 +52,11 @@ func TestRedirectsNoLogin(t *testing.T) {
 	redirects := []struct{ from, to string }{
 		{"/user2/repo1/commits/master", "/user2/repo1/commits/branch/master"},
 		{"/user2/repo1/src/master", "/user2/repo1/src/branch/master"},
-		{"/user2/repo1/src/master/file.txt", "/user2/repo1/src/branch/master/file.txt"},
-		{"/user2/repo1/src/master/directory/file.txt", "/user2/repo1/src/branch/master/directory/file.txt"},
-		{"/user/avatar/Ghost/-1", "/assets/img/avatar_default.png"},
+		{"/user2/repo1/src/master/a%2fb.txt", "/user2/repo1/src/branch/master/a%2fb.txt"},
+		{"/user2/repo1/src/master/directory/file.txt?a=1", "/user2/repo1/src/branch/master/directory/file.txt?a=1"},
+		{"/user2/repo1/tree/a%2fb?a=1", "/user2/repo1/src/a%2fb?a=1"},
+		{"/user/avatar/GhosT/-1", "/assets/img/avatar_default.png"},
+		{"/user/avatar/Gitea-ActionS/0", "/assets/img/avatar_default.png"},
 		{"/api/v1/swagger", "/api/swagger"},
 	}
 	for _, c := range redirects {

tests/integration/nonascii_branches_test.go

@@ -46,21 +46,21 @@ func TestNonAsciiBranches(t *testing.T) {
 		{
 			from:   "master/badfile",
 			to:     "branch/master/badfile",
-			status: http.StatusNotFound, // it does not exists
+			status: http.StatusNotFound, // it does not exist
 		},
 		{
 			from:   "ГлавнаяВетка",
-			to:     "branch/%D0%93%D0%BB%D0%B0%D0%B2%D0%BD%D0%B0%D1%8F%D0%92%D0%B5%D1%82%D0%BA%D0%B0",
+			to:     "branch/%d0%93%d0%bb%d0%b0%d0%b2%d0%bd%d0%b0%d1%8f%d0%92%d0%b5%d1%82%d0%ba%d0%b0",
 			status: http.StatusOK,
 		},
 		{
 			from:   "а/б/в",
-			to:     "branch/%D0%B0/%D0%B1/%D0%B2",
+			to:     "branch/%d0%b0/%d0%b1/%d0%b2",
 			status: http.StatusOK,
 		},
 		{
 			from:   "Grüßen/README.md",
-			to:     "branch/Gr%C3%BC%C3%9Fen/README.md",
+			to:     "branch/Gr%c3%bc%c3%9fen/README.md",
 			status: http.StatusOK,
 		},
 		{
@@ -70,7 +70,7 @@ func TestNonAsciiBranches(t *testing.T) {
 		},
 		{
 			from:   "Plus+Is+Not+Space/Файл.md",
-			to:     "branch/Plus+Is+Not+Space/%D0%A4%D0%B0%D0%B9%D0%BB.md",
+			to:     "branch/Plus+Is+Not+Space/%d0%a4%d0%b0%d0%b9%d0%bb.md",
 			status: http.StatusOK,
 		},
 		{
@@ -80,29 +80,29 @@ func TestNonAsciiBranches(t *testing.T) {
 		},
 		{
 			from:   "ブランチ",
-			to:     "branch/%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81",
+			to:     "branch/%e3%83%96%e3%83%a9%e3%83%b3%e3%83%81",
 			status: http.StatusOK,
 		},
 
 		// Tags
 		{
 			from:   "Тэг",
-			to:     "tag/%D0%A2%D1%8D%D0%B3",
+			to:     "tag/%d0%a2%d1%8d%d0%b3",
 			status: http.StatusOK,
 		},
 		{
 			from:   "Ё/人",
-			to:     "tag/%D0%81/%E4%BA%BA",
+			to:     "tag/%d0%81/%e4%ba%ba",
 			status: http.StatusOK,
 		},
 		{
 			from:   "タグ",
-			to:     "tag/%E3%82%BF%E3%82%B0",
+			to:     "tag/%e3%82%bf%e3%82%b0",
 			status: http.StatusOK,
 		},
 		{
 			from:   "タグ/ファイル.md",
-			to:     "tag/%E3%82%BF%E3%82%B0/%E3%83%95%E3%82%A1%E3%82%A4%E3%83%AB.md",
+			to:     "tag/%e3%82%bf%e3%82%b0/%e3%83%95%e3%82%a1%e3%82%a4%e3%83%ab.md",
 			status: http.StatusOK,
 		},
 
@@ -114,12 +114,12 @@ func TestNonAsciiBranches(t *testing.T) {
 		},
 		{
 			from:   "Файл.md",
-			to:     "branch/Plus+Is+Not+Space/%D0%A4%D0%B0%D0%B9%D0%BB.md",
+			to:     "branch/Plus+Is+Not+Space/%d0%a4%d0%b0%d0%b9%d0%bb.md",
 			status: http.StatusOK,
 		},
 		{
 			from:   "ファイル.md",
-			to:     "branch/Plus+Is+Not+Space/%E3%83%95%E3%82%A1%E3%82%A4%E3%83%AB.md",
+			to:     "branch/Plus+Is+Not+Space/%e3%83%95%e3%82%a1%e3%82%a4%e3%83%ab.md",
 			status: http.StatusNotFound, // it's not on default branch
 		},
 
@@ -131,7 +131,7 @@ func TestNonAsciiBranches(t *testing.T) {
 		},
 		{
 			from:   "%E3%82%BF%E3%82%b0",
-			to:     "tag/%E3%82%BF%E3%82%B0",
+			to:     "tag/%E3%82%BF%E3%82%b0",
 			status: http.StatusOK,
 		},
 		{
@@ -141,12 +141,12 @@ func TestNonAsciiBranches(t *testing.T) {
 		},
 		{
 			from:   "%D0%81%2F%E4%BA%BA",
-			to:     "tag/%D0%81/%E4%BA%BA",
+			to:     "tag/%D0%81%2F%E4%BA%BA",
 			status: http.StatusOK,
 		},
 		{
 			from:   "Ё%2F%E4%BA%BA",
-			to:     "tag/%D0%81/%E4%BA%BA",
+			to:     "tag/%d0%81%2F%E4%BA%BA",
 			status: http.StatusOK,
 		},
 		{