Filter private users from reviewer list#37414
Draft
pisarz77 wants to merge 2 commits intogo-gitea:mainfrom
Draft
Filter private users from reviewer list#37414pisarz77 wants to merge 2 commits intogo-gitea:mainfrom
pisarz77 wants to merge 2 commits intogo-gitea:mainfrom
Conversation
`services/pull.GetReviewers` returned every active candidate without
checking `user.visibility` or `user.is_restricted`. Non-admin callers of
`GET /api/v1/repos/{owner}/{repo}/reviewers` and the web reviewer picker
therefore saw users with `visibility=private` and `is_restricted=true`
that would otherwise be hidden from them.
The pre-existing direct-join already lacked the visibility check, but
the helper-based query introduced in go-gitea#37365 widened the candidate set
(team-level `authorize` OR per-unit `access_mode`), making the leak
more reproducible. This change applies the standard
`user_model.BuildCanSeeUserCondition` for admin/visibility handling and
additionally excludes `is_restricted=true` users for non-admin doers,
which the helper does not cover.
`GetReviewers` now takes the doer `*user_model.User` directly instead
of `doerID int64`; both callers (`routers/api/v1/repo/collaborators.go`
and `routers/web/repo/issue_page_meta.go`) already had `ctx.Doer`
available, so no extra DB lookup is needed.
Adds `TestRepoGetReviewersHidesPrivateAndRestricted` as a regression
guard using existing fixtures (repo4 owned by user5, with restricted
user29 as collaborator).
Fix go-gitea#37371
Signed-off-by: Jakub Pisarczyk <pisarz77@gmail.com>
GiteaBot
added
the
lgtm/need 2
wxiaoguang
reviewed
Apr 25, 2026
| cond = cond.And(visCond) | ||
| } | ||
| if doer == nil || !doer.IsAdmin { | ||
| cond = cond.And(builder.Eq{"`user`.is_restricted": false}) |
Contributor
Author
There was a problem hiding this comment.
You're right — that filter is too aggressive. is_restricted=true only restricts what a user can see, not whether they can be requested as a reviewer. Restricted users only enter uniqueUserIDs when they have explicit collaborator/team access to the repo, so excluding them blocks legitimate reviewers (e.g. an outside collaborator who is restricted at the instance level).
The actual leak in #37371 is visibility=private users showing up — that's covered by BuildCanSeeUserCondition. I've dropped the is_restricted=false clause and updated the regression test (TestRepoGetReviewersAppliesVisibility) to assert that a restricted collaborator stays selectable. Pushed as 88f7dd7. PR title/description updated accordingly.
Filtering out is_restricted=true users was too aggressive: restricted users only enter the candidate set when they have explicit collaborator or team access to the repo, so excluding them blocks legitimate reviewers. The actual leak in go-gitea#37371 is visibility=private users, which is already covered by BuildCanSeeUserCondition. Update TestRepoGetReviewersHidesPrivateAndRestricted accordingly: rename to TestRepoGetReviewersAppliesVisibility and assert that a restricted collaborator (user29) stays selectable for both admin and non-admin doers.
pisarz77
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix #37371:
services/pull.GetReviewersreturned every active candidate without checkinguser.visibility. Non-admin callers ofGET /api/v1/repos/{owner}/{repo}/reviewersand the web reviewer picker therefore saw users withvisibility=privatethat should be hidden.What changed
services/pull/reviewer.go— applyuser_model.BuildCanSeeUserCondition(doer)for admin/visibility handling.GetReviewers(ctx, repo, doer *user_model.User, posterID int64)— both callers already hadctx.Doer, so no extra DB lookup is required.routers/api/v1/repo/collaborators.goandrouters/web/repo/issue_page_meta.go.TestRepoGetReviewersAppliesVisibilityregression test using existing fixtures (repo4 owned by user5, restricted user29 as collaborator). Restricted collaborators remain selectable; only visibility hides users.Why this PR
The visibility filter was missing in the original query, but the helper-based path introduced in #37365 widened the candidate set (
team.authorize >= ? OR team_unit.access_mode >= ?), making the leak more reproducible — private team members previously hidden by the strict per-unit access check are now reached. Filtering at the finaluserquery closes the gap without reverting #37365.is_restrictedis intentionally not used as a filter: a restricted user only enters the candidate set when they have explicit collaborator or team access to the repo, in which case they are a legitimate reviewer.Test plan
go test -run TestRepoGetReviewers -count=1 -tags="sqlite sqlite_unlock_notify" ./services/pull/...— passgo vet ./services/pull/... ./routers/api/v1/repo/... ./routers/web/repo/...— cleanFixes #37371