-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only use supported sort order for "explore/users" page #29430
Conversation
bf83a41
to
f6b68be
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
Should be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that
- unsupported search params should simply ignore the param instead of throwing a 404
- the test is prone to fail once we refactor the templates
However, these changes are not important enough to warrant not giving an LGTM.
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 please fix the merge conflicts. 🍵 |
# 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. 🍵
|
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. ![image](https://github.com/go-gitea/gitea/assets/2114189/ce5c39c1-1e86-484a-80c3-33cac6419af8) # 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.
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.