From aa073fb1bea0dba33dd55c5d01918328ce243bc2 Mon Sep 17 00:00:00 2001 From: Shivansh Garg Date: Wed, 11 Nov 2026 17:53:39 +0000 Subject: [PATCH] fix(web): avoid compare output route collisions (#40307) Allow compare refs named diff or patch to use the normal compare page while preserving explicit .diff and .patch downloads. Compare refs ending in diff or patch were incorrectly matched as raw .diff/.patch routes. This caused normal compare pages such as main...diff and main...patch to return an empty or invalid response. The route matching is now constrained so valid raw diff/patch downloads continue working while branches named diff or patch use the normal compare page. Assisted-by: Agent:GPT-5.6 Luna Fixes https://github.com/go-gitea/gitea/issues/40303 --------- Signed-off-by: wxiaoguang Co-authored-by: wxiaoguang --- routers/web/repo/compare.go | 54 ++++++++++++++----------------- routers/web/web.go | 8 ++--- tests/integration/compare_test.go | 16 +++++++++ 3 files changed, 43 insertions(+), 35 deletions(-) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index aebfffb1508..89c36c699cf 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -148,10 +148,11 @@ func setCsvCompareContext(ctx *context.Context) { } type comparePageInfoType struct { - compareInfo *git_service.CompareInfo - nothingToCompare bool - allowCreatePull bool - allowEmptyPull bool + compareInfo *git_service.CompareInfo + headRefDownloadSuffix string + nothingToCompare bool + allowCreatePull bool + allowEmptyPull bool } func newComparePageInfo() *comparePageInfoType { @@ -204,6 +205,18 @@ func (cpi *comparePageInfoType) parseCompareInfo(ctx *context.Context, comparePa } headRef, err := common.ResolveRefWithSuffix(ctx, headGitRepo, headRefName, compareReq.HeadOriRefSuffix) + if errors.Is(err, util.ErrNotExist) && compareReq.HeadOriRefSuffix == "" { + // try to parse "base...head.patch" or "base...head.diff" download suffix + var hasDownloadSuffix bool + if headRefName, hasDownloadSuffix = strings.CutSuffix(headRefName, ".patch"); hasDownloadSuffix { + cpi.headRefDownloadSuffix = ".patch" + } else if headRefName, hasDownloadSuffix = strings.CutSuffix(headRefName, ".diff"); hasDownloadSuffix { + cpi.headRefDownloadSuffix = ".diff" + } + if hasDownloadSuffix { + headRef, err = common.ResolveRefWithSuffix(ctx, headGitRepo, headRefName, "") + } + } if err != nil { return err } @@ -522,6 +535,12 @@ func CompareDiff(ctx *context.Context) { ctx.ServerErrorAuto(err) return } + + if comparePageInfo.headRefDownloadSuffix != "" { + downloadCompareDiffOrPatch(ctx, comparePageInfo.compareInfo, comparePageInfo.headRefDownloadSuffix) + return + } + ci := comparePageInfo.compareInfo ctx.Data["PageIsViewCode"] = true ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes @@ -577,35 +596,12 @@ func CompareDiff(ctx *context.Context) { ctx.HTML(http.StatusOK, tplCompare) } -// DownloadCompareDiff render a comparison's raw unified diff -func DownloadCompareDiff(ctx *context.Context) { - downloadCompareDiffOrPatch(ctx, false) -} - -// DownloadComparePatch render a comparison as a git format-patch -func DownloadComparePatch(ctx *context.Context) { - downloadCompareDiffOrPatch(ctx, true) -} - -func downloadCompareDiffOrPatch(ctx *context.Context, patch bool) { - // The route captures `basehead` separately so the `.diff`/`.patch` suffix is - // stripped from the catch-all `*` param parseCompareInfo would otherwise read. - cpi := newComparePageInfo() - if err := cpi.parseCompareInfo(ctx, ctx.PathParam("basehead")); err != nil { - if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) { - ctx.NotFound() - } else { - ctx.ServerErrorAuto(err) - } - return - } - ci := cpi.compareInfo - +func downloadCompareDiffOrPatch(ctx *context.Context, ci *git_service.CompareInfo, downloadSuffix string) { ctx.Resp.Header().Set("Content-Type", "text/plain; charset=utf-8") compareArg := ci.BaseCommitID + ci.CompareSeparator + ci.HeadCommitID var err error - if patch { + if downloadSuffix == ".patch" { err = ci.HeadGitRepo.GetPatch(ctx, compareArg, ctx.Resp) } else { err = ci.HeadGitRepo.GetDiff(ctx, compareArg, ctx.Resp) diff --git a/routers/web/web.go b/routers/web/web.go index f9c5b68c68e..bf4697ac2a3 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1280,12 +1280,8 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Get("/commit/*", context.RepoRefByType(git.RefTypeCommit), repo.TreeViewNodes) }) m.Get("/compare", repo.MustBeNotEmpty, repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.CompareDiff) - m.PathGroup("/compare/*", func(g *web.RouterPathGroup) { - g.MatchPath("GET", "/.diff", repo.MustBeNotEmpty, repo.DownloadCompareDiff) - g.MatchPath("GET", "/.patch", repo.MustBeNotEmpty, repo.DownloadComparePatch) - g.MatchPath("GET", "/<*:*>", repo.MustBeNotEmpty, repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.CompareDiff) - g.MatchPath("POST", "/<*:*>", repo.MustBeNotEmpty, repo.SetEditorconfigIfExists, reqSignIn, context.RepoMustNotBeArchived(), reqUnitPullsReader, repo.MustAllowPulls, web.Bind[*forms.CreateIssueForm](), repo.SetWhitespaceBehavior, repo.CompareAndPullRequestPost) - }) + m.Get("/compare/*", repo.MustBeNotEmpty, repo.SetEditorconfigIfExists, repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.CompareDiff) + m.Post("/compare/*", repo.MustBeNotEmpty, repo.SetEditorconfigIfExists, reqSignIn, context.RepoMustNotBeArchived(), reqUnitPullsReader, repo.MustAllowPulls, web.Bind[*forms.CreateIssueForm](), repo.SetWhitespaceBehavior, repo.CompareAndPullRequestPost) m.Get("/pulls/new/*", repo.PullsNewRedirect) }, optSignIn, context.RepoAssignment, reqUnitCodeReader) // end "/{username}/{reponame}": repo code: find, compare, list diff --git a/tests/integration/compare_test.go b/tests/integration/compare_test.go index a42a3758e70..4e498842f39 100644 --- a/tests/integration/compare_test.go +++ b/tests/integration/compare_test.go @@ -236,6 +236,22 @@ func TestCompare(t *testing.T) { assert.Contains(t, resp.Body.String(), "diff --git ") }) + t.Run("BranchNamesConflictingWithOutputSuffix", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 31}) + for _, branch := range []string{"diff", "patch", "feature.diff", "feature.patch"} { + _, _, err := gitcmd.NewCommand("branch").AddDynamicArguments(branch, "add-csv").WithRepo(repo).RunStdString(t.Context()) + require.NoError(t, err) + } + + for _, branch := range []string{"diff", "patch", "feature.diff", "feature.patch"} { + req := NewRequest(t, "GET", "/user2/repo20/compare/master..."+branch) + resp := session.MakeRequest(t, req, http.StatusOK) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String()), "branch %q should render the compare page", branch) + } + }) + t.Run("InvalidBaseRef", func(t *testing.T) { defer tests.PrintCurrentTest(t)()