Only use supported sort order for "explore/users" page #29430
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/M
wxiaoguang
force-pushed
the
fix-explore-user
branch
from
bf83a41 to
f6b68be
Compare
GiteaBot
added
lgtm/need 1
| ctx.Data["SortType"] = "recentupdate" | ||
| orderBy = "`user`.updated_unix DESC" | ||
| } | ||
|
|
||
| if opts.SupportedSortOrders != nil && !opts.SupportedSortOrders.Contains(sortOrder) { | ||
| ctx.NotFound("unsupported sort order", nil) |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
Should be better?
There was a problem hiding this comment.
I agree to use it, but at the moment Gitea doesn't have the ability to render such a page .....
It needs other framework-level refacotring first.
|
And maybe extends the supportedSortOrders to all for admins? |
GiteaBot
added
lgtm/done
Agree, I also design API's to just ignore unknown search params. We should do whatever is consistent with existing APIs. One problem with param validation on APIs is that if you remove a previously known param, you have broken the API. This is also why things like JSON Schema by default allows additional properties to be present, they don't hurt. |
|
Not only because of consistency, imagine a user that enters a URL manually and fatfingers for some reason. |
|
Yeah, additional params could in theory even be introduced by systems outside our control. I think some analytic engines like google like to append tracking params to URLs, assuming the destination will just ignore them. |
|
This change is not ideal enough. But I would like to keep it as-is to make it easier to backport. I think we can propose new improvements later. Actually the "RenderUser" related code was much too over-abstracted and there are various problems in it. Sooner or later it would be refactored. |
wxiaoguang
added
the
reviewed/wait-merge
wxiaoguang
added
type/bug
and removed
type/enhancement
|
@wxiaoguang please fix the merge conflicts. 🍵 |
GiteaBot
removed
the
reviewed/wait-merge
# Conflicts: # routers/web/explore/org.go # routers/web/explore/user.go
|
I was unable to create a backport for 1.21. @wxiaoguang, please send one manually. 🍵 |
GiteaBot
added
the
backport/manual
Thanks to inferenceus : some sort orders on the "explore/users" page could list users by their lastlogintime/updatetime. It leaks user's activity unintentionally. This PR makes that page only use "supported" sort orders. Removing the "sort orders" could also be a good solution, while IMO at the moment keeping the "create time" and "name" orders is also fine, in case some users would like to find a target user in the search result, the "sort order" might help.  # Conflicts: # routers/web/explore/org.go # routers/web/explore/user.go
* giteaofficial/main: (23 commits) Fix wrong test usage of `AppSubURL` (go-gitea#29459) Improve contrast on blame timestamp, fix double border (go-gitea#29482) Fix/Improve `processWindowErrorEvent` (go-gitea#29407) Apply compact padding to small buttons with svg icons (go-gitea#29471) Fix counter display number incorrectly displayed on the page (go-gitea#29448) Fix incorrect user location link on profile page (go-gitea#29474) Fix workflow trigger event bugs (go-gitea#29467) Fix URL calculation in clone input box (go-gitea#29470) Remove jQuery from the "find file" page (go-gitea#29456) Move generate from module to service (go-gitea#29465) The job should always run when `if` is `always()` (go-gitea#29464) Recolor dark theme to blue shade (go-gitea#29283) Let ctx.FormOptionalBool() return optional.Option[bool] (go-gitea#29461) Implement actions badge svgs (go-gitea#28102) Fix missed return (go-gitea#29450) Use tailwind instead of `gt-[wh]-` helper classes (go-gitea#29423) Lock issues and pulls faster (go-gitea#29436) Allow to change primary email before account activation (go-gitea#29412) Update docs about `DEFAULT_ACTIONS_URL` (go-gitea#29442) Only use supported sort order for "explore/users" page (go-gitea#29430) ...
Backport #29430 Thanks to inferenceus : some sort orders on the "explore/users" page could list users by their lastlogintime/updatetime. It leaks user's activity unintentionally. This PR makes that page only use "supported" sort orders. Removing the "sort orders" could also be a good solution, while IMO at the moment keeping the "create time" and "name" orders is also fine, in case some users would like to find a target user in the search result, the "sort order" might help.
| ) | ||
| sortOrder := ctx.FormString("sort") | ||
| if sortOrder == "" { | ||
| sortOrder = "newest" |
There was a problem hiding this comment.
setting.UI.ExploreDefaultSort is not respected anymore!!!
Thanks to inferenceus : some sort orders on the "explore/users" page could list users by their lastlogintime/updatetime.
It leaks user's activity unintentionally. This PR makes that page only use "supported" sort orders.
Removing the "sort orders" could also be a good solution, while IMO at the moment keeping the "create time" and "name" orders is also fine, in case some users would like to find a target user in the search result, the "sort order" might help.