From f11d72a8a5faef4e13ac199eb7338d69efbd7b7d Mon Sep 17 00:00:00 2001 From: Adora Date: Wed, 10 Dec 2025 01:21:38 +0100 Subject: [PATCH] fix: issues and pulls route permitted extra characters (#10185) Fix a issue where the `/{owner}/{repo}/issues` and `/{owner}/{repo}/pulls` routes permitted the addition of extra characters in the URL. Resolves forgejo/forgejo#9954. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10185 Reviewed-by: Lucas Reviewed-by: oliverpool Reviewed-by: Gusted Co-authored-by: Adora Co-committed-by: Adora --- modules/web/route_test.go | 6 +-- routers/web/web.go | 10 ++--- tests/integration/issue_test.go | 40 +++++++++++++++++++ tests/integration/pull_test.go | 70 +++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 8 deletions(-) diff --git a/modules/web/route_test.go b/modules/web/route_test.go index 44626ec141..9821116ec2 100644 --- a/modules/web/route_test.go +++ b/modules/web/route_test.go @@ -21,7 +21,7 @@ func TestRoute1(t *testing.T) { recorder.Body = buff r := NewRoute() - r.Get("/{username}/{reponame}/{type:issues|pulls}", func(resp http.ResponseWriter, req *http.Request) { + r.Get("/{username}/{reponame}/{type:^(issues|pulls)$}", func(resp http.ResponseWriter, req *http.Request) { username := chi.URLParam(req, "username") assert.Equal(t, "gitea", username) reponame := chi.URLParam(req, "reponame") @@ -46,7 +46,7 @@ func TestRoute2(t *testing.T) { r := NewRoute() r.Group("/{username}/{reponame}", func() { r.Group("", func() { - r.Get("/{type:issues|pulls}", func(resp http.ResponseWriter, req *http.Request) { + r.Get("/{type:^(issues|pulls)$}", func(resp http.ResponseWriter, req *http.Request) { username := chi.URLParam(req, "username") assert.Equal(t, "gitea", username) reponame := chi.URLParam(req, "reponame") @@ -56,7 +56,7 @@ func TestRoute2(t *testing.T) { hit = 0 }) - r.Get("/{type:issues|pulls}/{index}", func(resp http.ResponseWriter, req *http.Request) { + r.Get("/{type:^(issues|pulls)$}/{index}", func(resp http.ResponseWriter, req *http.Request) { username := chi.URLParam(req, "username") assert.Equal(t, "gitea", username) reponame := chi.URLParam(req, "reponame") diff --git a/routers/web/web.go b/routers/web/web.go index 90cffac552..1fd3ecb37c 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1191,7 +1191,7 @@ func registerRoutes(m *web.Route) { m.Combo("/compare/*", repo.MustBeNotEmpty, reqRepoCodeReader, repo.SetEditorconfigIfExists). Get(repo.SetDiffViewStyle, repo.SetWhitespaceBehavior, repo.CompareDiff). Post(reqSignIn, context.RepoMustNotBeArchived(), reqRepoPullsReader, repo.MustAllowPulls, web.Bind(forms.CreateIssueForm{}), repo.SetWhitespaceBehavior, repo.CompareAndPullRequestPost) - m.Group("/{type:issues|pulls}", func() { + m.Group("/{type:^(issues|pulls)$}", func() { m.Group("/{index}", func() { m.Get("/info", repo.GetIssueInfo) m.Get("/summary-card", repo.DrawIssueSummaryCard) @@ -1216,7 +1216,7 @@ func registerRoutes(m *web.Route) { }, context.RepoMustNotBeArchived(), reqRepoIssueReader) // FIXME: should use different URLs but mostly same logic for comments of issue and pull request. // So they can apply their own enable/disable logic on routers. - m.Group("/{type:issues|pulls}", func() { + m.Group("/{type:^(issues|pulls)$}", func() { m.Group("/{index}", func() { m.Post("/title", repo.UpdateIssueTitle) m.Post("/action-user-trust", reqRepoActionsReader, actions.MustEnableActions, reqRepoDelegateActionTrust, repo.UpdateTrustWithPullRequestActions) @@ -1379,9 +1379,9 @@ func registerRoutes(m *web.Route) { m.Group("/{username}/{reponame}", func() { m.Group("", func() { m.Get("/issues/posters", repo.IssuePosters) // it can't use {type:issues|pulls} because other routes like "/pulls/{index}" has higher priority - m.Get("/{type:issues|pulls}", repo.Issues) - m.Get("/{type:issues|pulls}/{index}", repo.ViewIssue) - m.Group("/{type:issues|pulls}/{index}/content-history", func() { + m.Get("/{type:^(issues|pulls)$}", repo.Issues) + m.Get("/{type:^(issues|pulls)$}/{index}", repo.ViewIssue) + m.Group("/{type:^(issues|pulls)$}/{index}/content-history", func() { m.Get("/overview", repo.GetContentHistoryOverview) m.Get("/list", repo.GetContentHistoryList) m.Get("/detail", repo.GetContentHistoryDetail) diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 9ccb32d7ed..205c290413 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -1591,3 +1591,43 @@ func TestIssueAndPullRedirect(t *testing.T) { req = NewRequest(t, "GET", "/user2/repo1/pulls/9999999") MakeRequest(t, req, http.StatusNotFound) } + +func TestIssueUrlHandling(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + t.Run("Overview correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "user2/repo1/issues") + MakeRequest(t, req, http.StatusOK) + }) + + t.Run("Issue correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "user2/repo1/issues/1") + MakeRequest(t, req, http.StatusOK) + }) + + t.Run("Overview left-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/extra_text_issues") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Overview right-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/issues_extra_text") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Issue left-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/extra_text_issues/5") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Issue right-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/issues_extra_text/5") + MakeRequest(t, req, http.StatusNotFound) + }) +} diff --git a/tests/integration/pull_test.go b/tests/integration/pull_test.go index a22cbc9904..fa26643280 100644 --- a/tests/integration/pull_test.go +++ b/tests/integration/pull_test.go @@ -171,3 +171,73 @@ func TestShowMergeForManualMerge(t *testing.T) { htmlDoc := NewHTMLParser(t, resp.Body) htmlDoc.AssertElement(t, "#pull-request-merge-form", true) } + +func TestPullUrlHandling(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + t.Run("Overview correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/pulls") + MakeRequest(t, req, http.StatusOK) + }) + + t.Run("Pull correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/pulls/5") + MakeRequest(t, req, http.StatusOK) + }) + + t.Run("Overview left-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/extra_text_pulls") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Overview right-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/pulls_extra_text") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Pull left-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/extra_text_pulls/5") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Pull right-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/pulls_extra_text/5") + MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("POST Title correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session := loginUser(t, "user2") + + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/5/title", map[string]string{"title": "test"}) + session.MakeRequest(t, req, http.StatusOK) + }) + + t.Run("POST Title padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + session := loginUser(t, "user2") + + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues_extra_text/5/title", map[string]string{"title": "test"}) + session.MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("Pull content overview correct", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/pulls/5/content-history/overview") + MakeRequest(t, req, http.StatusOK) + }) + + t.Run("Pull content overview left-padded", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + req := NewRequest(t, "GET", "/user2/repo1/extra_text_pulls/5/content-history/overview") + MakeRequest(t, req, http.StatusNotFound) + }) +}