From 47d48eb208ad7573f570a0d12d18f1468ae051a4 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 15 Jun 2026 02:26:22 +0800 Subject: [PATCH] chore: fix form string abuse (#38106) --- modules/sitemap/sitemap.go | 12 ++++++++---- modules/sitemap/sitemap_test.go | 8 ++++---- routers/web/admin/orgs.go | 6 ++---- routers/web/admin/users.go | 7 ++----- routers/web/explore/org.go | 9 +++------ routers/web/explore/user.go | 15 ++++----------- routers/web/user/setting/security/openid_test.go | 10 ++-------- services/context/base_form.go | 5 ----- 8 files changed, 25 insertions(+), 47 deletions(-) diff --git a/modules/sitemap/sitemap.go b/modules/sitemap/sitemap.go index 280ca1d7100..5ac37033d92 100644 --- a/modules/sitemap/sitemap.go +++ b/modules/sitemap/sitemap.go @@ -63,20 +63,24 @@ func (s *Sitemap) Add(u URL) { // WriteTo writes the sitemap to a response func (s *Sitemap) WriteTo(w io.Writer) (int64, error) { if l := len(s.URLs); l > urlsLimit { - return 0, fmt.Errorf("The sitemap contains %d URLs, but only %d are allowed", l, urlsLimit) + return 0, fmt.Errorf("sitemap contains %d URLs, but only %d are allowed", l, urlsLimit) } if l := len(s.Sitemaps); l > urlsLimit { - return 0, fmt.Errorf("The sitemap contains %d sub-sitemaps, but only %d are allowed", l, urlsLimit) + return 0, fmt.Errorf("sitemap contains %d sub-sitemaps, but only %d are allowed", l, urlsLimit) } buf := bytes.NewBufferString(xml.Header) - if err := xml.NewEncoder(buf).Encode(s); err != nil { + encoder := xml.NewEncoder(buf) + defer encoder.Close() + if err := encoder.Encode(s); err != nil { return 0, err } + _ = encoder.Flush() if err := buf.WriteByte('\n'); err != nil { return 0, err } + // FIXME: such limit is not right, the content has been written, it would have already caused OOM if buf.Len() > sitemapFileLimit { - return 0, fmt.Errorf("The sitemap has %d bytes, but only %d are allowed", buf.Len(), sitemapFileLimit) + return 0, fmt.Errorf("sitemap has %d bytes, but only %d are allowed", buf.Len(), sitemapFileLimit) } return buf.WriteTo(w) } diff --git a/modules/sitemap/sitemap_test.go b/modules/sitemap/sitemap_test.go index 1180463cd79..9ff97939014 100644 --- a/modules/sitemap/sitemap_test.go +++ b/modules/sitemap/sitemap_test.go @@ -61,14 +61,14 @@ func TestNewSitemap(t *testing.T) { { name: "too many urls", urls: make([]URL, 50001), - wantErr: "The sitemap contains 50001 URLs, but only 50000 are allowed", + wantErr: "sitemap contains 50001 URLs, but only 50000 are allowed", }, { name: "too big file", urls: []URL{ {URL: strings.Repeat("b", 50*1024*1024+1)}, }, - wantErr: "The sitemap has 52428932 bytes, but only 52428800 are allowed", + wantErr: "sitemap has 52428932 bytes, but only 52428800 are allowed", }, } for _, tt := range tests { @@ -137,14 +137,14 @@ func TestNewSitemapIndex(t *testing.T) { { name: "too many sitemaps", urls: make([]URL, 50001), - wantErr: "The sitemap contains 50001 sub-sitemaps, but only 50000 are allowed", + wantErr: "sitemap contains 50001 sub-sitemaps, but only 50000 are allowed", }, { name: "too big file", urls: []URL{ {URL: strings.Repeat("b", 50*1024*1024+1)}, }, - wantErr: "The sitemap has 52428952 bytes, but only 52428800 are allowed", + wantErr: "sitemap has 52428952 bytes, but only 52428800 are allowed", }, } for _, tt := range tests { diff --git a/routers/web/admin/orgs.go b/routers/web/admin/orgs.go index 1474038c915..02037af89fa 100644 --- a/routers/web/admin/orgs.go +++ b/routers/web/admin/orgs.go @@ -23,10 +23,7 @@ func Organizations(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("admin.organizations") ctx.Data["PageIsAdminOrganizations"] = true - if ctx.FormString("sort") == "" { - ctx.SetFormString("sort", UserSearchDefaultAdminSort) - } - + sortOrder := ctx.FormString("sort", UserSearchDefaultAdminSort) explore.RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Types: []user_model.UserType{user_model.UserTypeOrganization}, @@ -35,5 +32,6 @@ func Organizations(ctx *context.Context) { PageSize: setting.UI.Admin.OrgPagingNum, }, Visible: []structs.VisibleType{structs.VisibleTypePublic, structs.VisibleTypeLimited, structs.VisibleTypePrivate}, + OrderBy: db.SearchOrderBy(sortOrder), }, tplOrgs) } diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index 4b345945089..f918c8b5d31 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -55,11 +55,7 @@ func Users(ctx *context.Context) { statusFilterMap[filterKey] = paramVal } - sortType := ctx.FormString("sort") - if sortType == "" { - sortType = UserSearchDefaultAdminSort - ctx.SetFormString("sort", sortType) - } + sortType := ctx.FormString("sort", UserSearchDefaultAdminSort) ctx.PageData["adminUserListSearchForm"] = map[string]any{ "StatusFilterMap": statusFilterMap, "SortType": sortType, @@ -78,6 +74,7 @@ func Users(ctx *context.Context) { IsTwoFactorEnabled: optional.ParseBool(statusFilterMap["is_2fa_enabled"]), IsProhibitLogin: optional.ParseBool(statusFilterMap["is_prohibit_login"]), IncludeReserved: true, // administrator needs to list all accounts include reserved, bot, remote ones + OrderBy: db.SearchOrderBy(sortType), }, tplUsers) } diff --git a/routers/web/explore/org.go b/routers/web/explore/org.go index 621f6bd97a5..687d83ff36c 100644 --- a/routers/web/explore/org.go +++ b/routers/web/explore/org.go @@ -38,17 +38,14 @@ func Organizations(ctx *context.Context) { "alphabetically", "reversealphabetically", ) - sortOrder := ctx.FormString("sort") - if sortOrder == "" { - sortOrder = util.Iif(supportedSortOrders.Contains(setting.UI.ExploreDefaultSort), setting.UI.ExploreDefaultSort, "newest") - ctx.SetFormString("sort", sortOrder) - } - + sortOrderDefault := util.Iif(supportedSortOrders.Contains(setting.UI.ExploreDefaultSort), setting.UI.ExploreDefaultSort, "newest") + sortOrder := ctx.FormString("sort", sortOrderDefault) RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Types: []user_model.UserType{user_model.UserTypeOrganization}, ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum}, Visible: visibleTypes, + OrderBy: db.SearchOrderBy(sortOrder), SupportedSortOrders: supportedSortOrders, }, tplExploreUsers) diff --git a/routers/web/explore/user.go b/routers/web/explore/user.go index 00217d662c1..64e2a92d685 100644 --- a/routers/web/explore/user.go +++ b/routers/web/explore/user.go @@ -55,11 +55,7 @@ func RenderUserSearch(ctx *context.Context, opts user_model.SearchUserOptions, t ) // we can not set orderBy to `models.SearchOrderByXxx`, because there may be a JOIN in the statement, different tables may have the same name columns - - sortOrder := ctx.FormString("sort") - if sortOrder == "" { - sortOrder = setting.UI.ExploreDefaultSort - } + sortOrder := util.IfZero(string(opts.OrderBy), ctx.FormString("sort", setting.UI.ExploreDefaultSort)) ctx.Data["SortType"] = sortOrder switch sortOrder { @@ -145,18 +141,15 @@ func Users(ctx *context.Context) { "alphabetically", "reversealphabetically", ) - sortOrder := ctx.FormString("sort") - if sortOrder == "" { - sortOrder = util.Iif(supportedSortOrders.Contains(setting.UI.ExploreDefaultSort), setting.UI.ExploreDefaultSort, "newest") - ctx.SetFormString("sort", sortOrder) - } - + sortOrderDefault := util.Iif(supportedSortOrders.Contains(setting.UI.ExploreDefaultSort), setting.UI.ExploreDefaultSort, "newest") + sortOrder := ctx.FormString("sort", sortOrderDefault) RenderUserSearch(ctx, user_model.SearchUserOptions{ Actor: ctx.Doer, Types: []user_model.UserType{user_model.UserTypeIndividual}, ListOptions: db.ListOptions{PageSize: setting.UI.ExplorePagingNum}, IsActive: optional.Some(true), Visible: []structs.VisibleType{structs.VisibleTypePublic, structs.VisibleTypeLimited, structs.VisibleTypePrivate}, + OrderBy: db.SearchOrderBy(sortOrder), SupportedSortOrders: supportedSortOrders, }, tplExploreUsers) diff --git a/routers/web/user/setting/security/openid_test.go b/routers/web/user/setting/security/openid_test.go index 046e7357e1b..f00506effaf 100644 --- a/routers/web/user/setting/security/openid_test.go +++ b/routers/web/user/setting/security/openid_test.go @@ -15,22 +15,16 @@ import ( func TestDeleteOpenIDReturnsNotFoundForOtherUsersAddress(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "POST /user/settings/security") + ctx, _ := contexttest.MockContext(t, "POST /user/settings/security?id=1") contexttest.LoadUser(t, ctx, 2) - ctx.SetFormString("id", "1") - DeleteOpenID(ctx) - assert.Equal(t, http.StatusNotFound, ctx.Resp.WrittenStatus()) } func TestToggleOpenIDVisibilityReturnsNotFoundForOtherUsersAddress(t *testing.T) { unittest.PrepareTestEnv(t) - ctx, _ := contexttest.MockContext(t, "POST /user/settings/security") + ctx, _ := contexttest.MockContext(t, "POST /user/settings/security?id=1") contexttest.LoadUser(t, ctx, 2) - ctx.SetFormString("id", "1") - ToggleOpenIDVisibility(ctx) - assert.Equal(t, http.StatusNotFound, ctx.Resp.WrittenStatus()) } diff --git a/services/context/base_form.go b/services/context/base_form.go index 088888b461e..c6a70991297 100644 --- a/services/context/base_form.go +++ b/services/context/base_form.go @@ -78,8 +78,3 @@ func (b *Base) FormOptionalBool(key string) optional.Option[bool] { v = v || strings.EqualFold(s, "on") return optional.Some(v) } - -func (b *Base) SetFormString(key, value string) { - _ = b.Req.FormValue(key) // force parse form - b.Req.Form.Set(key, value) -}