From c2e0143bfe35b539bdbec9971e83fa9f9ab78034 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 9 Aug 2023 14:57:45 +0800 Subject: [PATCH] Introduce ctx.PathParamRaw to avoid incorrect unescaping (#26392) Fix #26389 And complete an old TODO: `ctx.Params does un-escaping,..., which is incorrect.` --- modules/context/base.go | 4 ++++ routers/api/v1/repo/wiki.go | 8 ++++---- routers/web/repo/wiki.go | 12 ++++++------ services/wiki/wiki_path.go | 16 ++++++++++------ services/wiki/wiki_test.go | 4 +++- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/modules/context/base.go b/modules/context/base.go index 83e5a214f8..8df1dde866 100644 --- a/modules/context/base.go +++ b/modules/context/base.go @@ -147,6 +147,10 @@ func (b *Base) Params(p string) string { return s } +func (b *Base) PathParamRaw(p string) string { + return chi.URLParam(b.Req, strings.TrimPrefix(p, ":")) +} + // ParamsInt64 returns the param on route as int64 func (b *Base) ParamsInt64(p string) int64 { v, _ := strconv.ParseInt(b.Params(p), 10, 64) diff --git a/routers/api/v1/repo/wiki.go b/routers/api/v1/repo/wiki.go index e33790a378..7f3a7d0674 100644 --- a/routers/api/v1/repo/wiki.go +++ b/routers/api/v1/repo/wiki.go @@ -127,7 +127,7 @@ func EditWikiPage(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.CreateWikiPageOptions) - oldWikiName := wiki_service.WebPathFromRequest(ctx.Params(":pageName")) + oldWikiName := wiki_service.WebPathFromRequest(ctx.PathParamRaw(":pageName")) newWikiName := wiki_service.UserTitleToWebPath("", form.Title) if len(newWikiName) == 0 { @@ -231,7 +231,7 @@ func DeleteWikiPage(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - wikiName := wiki_service.WebPathFromRequest(ctx.Params(":pageName")) + wikiName := wiki_service.WebPathFromRequest(ctx.PathParamRaw(":pageName")) if err := wiki_service.DeleteWikiPage(ctx, ctx.Doer, ctx.Repo.Repository, wikiName); err != nil { if err.Error() == "file does not exist" { @@ -359,7 +359,7 @@ func GetWikiPage(ctx *context.APIContext) { // "$ref": "#/responses/notFound" // get requested pagename - pageName := wiki_service.WebPathFromRequest(ctx.Params(":pageName")) + pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw(":pageName")) wikiPage := getWikiPage(ctx, pageName) if !ctx.Written() { @@ -409,7 +409,7 @@ func ListPageRevisions(ctx *context.APIContext) { } // get requested pagename - pageName := wiki_service.WebPathFromRequest(ctx.Params(":pageName")) + pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw(":pageName")) if len(pageName) == 0 { pageName = "Home" } diff --git a/routers/web/repo/wiki.go b/routers/web/repo/wiki.go index e3c187c33b..4de24e2a38 100644 --- a/routers/web/repo/wiki.go +++ b/routers/web/repo/wiki.go @@ -185,7 +185,7 @@ func renderViewPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) { ctx.Data["Pages"] = pages // get requested page name - pageName := wiki_service.WebPathFromRequest(ctx.Params("*")) + pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) if len(pageName) == 0 { pageName = "Home" } @@ -332,7 +332,7 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) } // get requested pagename - pageName := wiki_service.WebPathFromRequest(ctx.Params("*")) + pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) if len(pageName) == 0 { pageName = "Home" } @@ -415,7 +415,7 @@ func renderEditPage(ctx *context.Context) { }() // get requested pagename - pageName := wiki_service.WebPathFromRequest(ctx.Params("*")) + pageName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) if len(pageName) == 0 { pageName = "Home" } @@ -647,7 +647,7 @@ func WikiRaw(ctx *context.Context) { return } - providedWebPath := wiki_service.WebPathFromRequest(ctx.Params("*")) + providedWebPath := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) providedGitPath := wiki_service.WebPathToGitPath(providedWebPath) var entry *git.TreeEntry if commit != nil { @@ -759,7 +759,7 @@ func EditWikiPost(ctx *context.Context) { return } - oldWikiName := wiki_service.WebPathFromRequest(ctx.Params("*")) + oldWikiName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) newWikiName := wiki_service.UserTitleToWebPath("", form.Title) if len(form.Message) == 0 { @@ -778,7 +778,7 @@ func EditWikiPost(ctx *context.Context) { // DeleteWikiPagePost delete wiki page func DeleteWikiPagePost(ctx *context.Context) { - wikiName := wiki_service.WebPathFromRequest(ctx.Params("*")) + wikiName := wiki_service.WebPathFromRequest(ctx.PathParamRaw("*")) if len(wikiName) == 0 { wikiName = "Home" } diff --git a/services/wiki/wiki_path.go b/services/wiki/wiki_path.go index f2600ad8ba..e51d6c630c 100644 --- a/services/wiki/wiki_path.go +++ b/services/wiki/wiki_path.go @@ -22,15 +22,16 @@ import ( // - "/wiki/100%25+Free" // - "/wiki/2000-01-02+meeting.-" // - If a segment has a suffix "DashMarker(.-)", it means that there is no dash-space conversion for this segment. -// - If a WebPath is a "*.md" pattern, then use it directly as GitPath, to make users can access the raw file. +// - If a WebPath is a "*.md" pattern, then use the unescaped value directly as GitPath, to make users can access the raw file. // * Git Path (only space doesn't need to be escaped): // - "/.wiki.git/Home-Page.md" // - "/.wiki.git/100%25 Free.md" // - "/.wiki.git/2000-01-02 meeting.-.md" // TODO: support subdirectory in the future // -// Although this package now has the ablity to support subdirectory, but the route package doesn't: +// Although this package now has the ability to support subdirectory, but the route package doesn't: // * Double-escaping problem: the URL "/wiki/abc%2Fdef" becomes "/wiki/abc/def" by ctx.Params, which is incorrect +// * This problem should have been 99% fixed, but it needs more tests. // * The old wiki code's behavior is always using %2F, instead of subdirectory, so there are a lot of legacy "%2F" files in user wikis. type WebPath string @@ -91,7 +92,8 @@ func WebPathSegments(s WebPath) []string { func WebPathToGitPath(s WebPath) string { if strings.HasSuffix(string(s), ".md") { - return string(s) + ret, _ := url.PathUnescape(string(s)) + return util.PathJoinRelX(ret) } a := strings.Split(string(s), "/") @@ -124,7 +126,10 @@ func GitPathToWebPath(s string) (wp WebPath, err error) { func WebPathToUserTitle(s WebPath) (dir, display string) { dir = path.Dir(string(s)) display = path.Base(string(s)) - display = strings.TrimSuffix(display, ".md") + if strings.HasSuffix(display, ".md") { + display = strings.TrimSuffix(display, ".md") + display, _ = url.PathUnescape(display) + } display, _ = unescapeSegment(display) return dir, display } @@ -141,8 +146,7 @@ func WebPathFromRequest(s string) WebPath { } func UserTitleToWebPath(base, title string) WebPath { - // TODO: ctx.Params does un-escaping, so the URL "/wiki/abc%2Fdef" becomes "wiki path = `abc/def`", which is incorrect. - // And the old wiki code's behavior is always using %2F, instead of subdirectory. + // TODO: no support for subdirectory, because the old wiki code's behavior is always using %2F, instead of subdirectory. // So we do not add the support for writing slashes in title at the moment. title = strings.TrimSpace(title) title = util.PathJoinRelX(base, escapeSegToWeb(title, false)) diff --git a/services/wiki/wiki_test.go b/services/wiki/wiki_test.go index f126224244..85d99806fe 100644 --- a/services/wiki/wiki_test.go +++ b/services/wiki/wiki_test.go @@ -59,6 +59,7 @@ func TestWebPathToDisplayName(t *testing.T) { {"name with / slash", "name-with %2F slash"}, {"name with % percent", "name-with %25 percent"}, {"2000-01-02 meeting", "2000-01-02+meeting.-.md"}, + {"a b", "a%20b.md"}, } { _, displayName := WebPathToUserTitle(test.WebPath) assert.EqualValues(t, test.Expected, displayName) @@ -73,7 +74,8 @@ func TestWebPathToGitPath(t *testing.T) { for _, test := range []test{ {"wiki-name.md", "wiki%20name"}, {"wiki-name.md", "wiki+name"}, - {"wiki%20name.md", "wiki%20name.md"}, + {"wiki name.md", "wiki%20name.md"}, + {"wiki%20name.md", "wiki%2520name.md"}, {"2000-01-02-meeting.md", "2000-01-02+meeting"}, {"2000-01-02 meeting.-.md", "2000-01-02%20meeting.-"}, } {