chore: fix form string abuse (#38106)

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)
 }

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 {

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)
 }

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)
 }
 

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)

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)

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())
 }

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)
-}