ynkmr/gitea
1
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-08-22 03:19:44 +09:00
Code Issues Projects Releases Wiki Activity

fix: avoid enumerating every public repository in issue search (#38992) (#39000)

Browse Source 
Backport #38992 by @lunny

Both issue search endpoints resolve their repository filter with
`SearchRepositoryIDs` and pass the result to the indexer as `RepoIDs`.
They mean
to leave public repositories to the indexer, but
`SearchRepoOptions.AllPublic` is
only read when `OwnerID > 0`, so without an `owner` filter the flag does
nothing
and every public repository is enumerated, without a `LIMIT`, into
`repo_id IN (...)`.

Those IDs are redundant, as `allPublic` is passed to the indexer, which
already
matches every public repository. On a large instance this binds tens of
thousands
of parameters and can fail in the driver, making the endpoint return 500
for every
filter. Admins are worst hit, as `SearchRepositoryCondition` skips their
accessible-repository condition and enumerates the whole table.

Restrict the enumeration to private repositories. The result set is
unchanged, as
the dropped IDs are a subset of what `allPublic` matches.

Both endpoints held copies of this block, so it moves to
`routers/common`.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
Giteabot
2026-08-20 19:51:20 +02:00
committed by GitHub
co-authored by Lunny Xiao silverwind wxiaoguang
parent 4e515cce99
commit eee37e0560
5 changed files with 189 additions and 162 deletions
Download Patch File Download Diff File Expand all files Collapse all files
models/repo/repo_list.go
+9
View File
@@ -771,6 +771,15 @@ func PublicRepoUnderPublicOwnerCond() builder.Cond {
)
}
// NotPublicRepoUnderPublicOwnerCond complements PublicRepoUnderPublicOwnerCond. Spelled positively so
// the owner subquery hashes the limited/private minority, not every public user.
func NotPublicRepoUnderPublicOwnerCond() builder.Cond {
return builder.Or(
builder.Eq{"`repository`.is_private": true},
builder.In("`repository`.owner_id", builder.Select("id").From("`user`").Where(builder.Neq{"visibility": structs.VisibleTypePublic})),
)
}
// UserActionsAccessibleOwnerRepoCond selects the repos owned by ownerID whose Actions `user` may read.
// It is used to list an org/user's Actions runs and jobs (see the callers in routers/api/v1/shared).
// - owner_id = ownerID: only that owner's repos.
routers/api/v1/repo/issue.go
+7 -71
View File
@@ -14,7 +14,6 @@ import (
"gitea.dev/models/db"
issues_model "gitea.dev/models/issues"
"gitea.dev/models/organization"
access_model "gitea.dev/models/perm/access"
repo_model "gitea.dev/models/repo"
"gitea.dev/models/unit"
@@ -33,61 +32,6 @@ import (
issue_service "gitea.dev/services/issue"
)
// buildSearchIssuesRepoIDs builds the list of repository IDs for issue search based on query parameters.
// It returns repoIDs, allPublic flag, and any error that occurred.
func buildSearchIssuesRepoIDs(ctx *context.APIContext) (repoIDs []int64, allPublic bool, err error) {
opts := repo_model.SearchRepoOptions{
Private: false,
AllPublic: true,
TopicOnly: false,
Collaborate: optional.None[bool](),
// This needs to be a column that is not nil in fixtures or
// MySQL will return different results when sorting by null in some cases
OrderBy: db.SearchOrderByAlphabetically,
Actor: ctx.Doer,
}
if ctx.IsSigned {
opts.Private = true
opts.AllLimited = true
}
opts.ApplyPublicOnly(ctx.PublicOnly)
if ctx.FormString("owner") != "" {
owner, err := user_model.GetUserByName(ctx, ctx.FormString("owner"))
if err != nil {
return nil, false, err
}
opts.OwnerID = owner.ID
opts.AllLimited = false
opts.AllPublic = false
opts.Collaborate = optional.Some(false)
}
if ctx.FormString("team") != "" {
if ctx.FormString("owner") == "" {
return nil, false, util.NewInvalidArgumentErrorf("owner organisation is required for filtering on team")
}
team, err := organization.GetTeam(ctx, opts.OwnerID, ctx.FormString("team"))
if err != nil {
return nil, false, err
}
opts.TeamID = team.ID
}
if opts.AllPublic {
allPublic = true
opts.AllPublic = false // set it false to avoid returning too many repos, we could filter by indexer
}
repoIDs, _, err = repo_model.SearchRepositoryIDs(ctx, opts)
if err != nil {
return nil, false, err
}
if len(repoIDs) == 0 {
// no repos found, don't let the indexer return all repos
repoIDs = []int64{0}
}
return repoIDs, allPublic, nil
}
// SearchIssues searches for issues across the repositories that the user has access to
func SearchIssues(ctx *context.APIContext) {
// swagger:operation GET /repos/issues/search issue issueSearchIssues
@@ -193,7 +137,12 @@ func SearchIssues(ctx *context.APIContext) {
isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
repoIDs, allPublic, err := buildSearchIssuesRepoIDs(ctx)
repoIDs, allPublic, err := common.SearchIssuesRepoIDs(ctx, common.SearchIssuesRepoIDsOptions{
Doer: ctx.Doer,
PublicOnly: ctx.PublicOnly,
OwnerName: ctx.FormString("owner"),
TeamName: ctx.FormString("team"),
})
if err != nil {
if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) {
ctx.APIError(http.StatusBadRequest, err.Error())
@@ -204,10 +153,6 @@ func SearchIssues(ctx *context.APIContext) {
}
keyword := ctx.FormTrim("q")
if strings.IndexByte(keyword, 0) >= 0 {
keyword = ""
}
isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type"))
var includedAnyLabels []int64
@@ -390,9 +335,6 @@ func ListIssues(ctx *context.APIContext) {
isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
keyword := ctx.FormTrim("q")
if strings.IndexByte(keyword, 0) >= 0 {
keyword = ""
}
var labelIDs []int64
if splitted := strings.Split(ctx.FormString("labels"), ","); len(splitted) > 0 {
@@ -435,13 +377,7 @@ func ListIssues(ctx *context.APIContext) {
listOptions := utils.GetListOptions(ctx)
isPull := optional.None[bool]()
switch ctx.FormString("type") {
case "pulls":
isPull = optional.Some(true)
case "issues":
isPull = optional.Some(false)
}
isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type"))
if isPull.Has() && !ctx.Repo.Permission.CanReadIssuesOrPulls(isPull.Value()) {
ctx.APIErrorNotFound()
routers/common/issue_filter.go
+62
View File
@@ -4,7 +4,13 @@
package common
import (
"context"
"gitea.dev/models/organization"
repo_model "gitea.dev/models/repo"
user_model "gitea.dev/models/user"
"gitea.dev/modules/optional"
"gitea.dev/modules/util"
)
func ParseIssueFilterStateIsClosed(state string) optional.Option[bool] {
@@ -23,3 +29,59 @@ func ParseIssueFilterStateIsClosed(state string) optional.Option[bool] {
func ParseIssueFilterTypeIsPull(typ string) optional.Option[bool] {
return optional.FromMapLookup(map[string]bool{"pulls": true, "issues": false}, typ)
}
type SearchIssuesRepoIDsOptions struct {
Doer *user_model.User
PublicOnly bool
OwnerName string
TeamName string
}
// SearchIssuesRepoIDs resolves the repository filter of an issue search. allPublic makes the indexer
// match everything its own is_public covers (modules/indexer/issues/util.go), so repoIDs omits those.
func SearchIssuesRepoIDs(ctx context.Context, opts SearchIssuesRepoIDsOptions) (repoIDs []int64, allPublic bool, err error) {
searchOpts := repo_model.SearchRepoOptions{
Private: opts.Doer != nil,
Collaborate: optional.None[bool](),
Actor: opts.Doer,
}
searchOpts.ApplyPublicOnly(opts.PublicOnly)
if opts.OwnerName != "" {
owner, err := user_model.GetUserByName(ctx, opts.OwnerName)
if err != nil {
return nil, false, err
}
searchOpts.OwnerID = owner.ID
searchOpts.Collaborate = optional.Some(false)
}
if opts.TeamName != "" {
if opts.OwnerName == "" {
return nil, false, util.NewInvalidArgumentErrorf("owner organisation is required for filtering on team")
}
team, err := organization.GetTeam(ctx, searchOpts.OwnerID, opts.TeamName)
if err != nil {
return nil, false, err
}
searchOpts.TeamID = team.ID
}
// SearchRepoOptions.AllPublic and AllLimited only apply under an owner filter, so the indexer covers them
allPublic = opts.OwnerName == ""
cond := repo_model.SearchRepositoryCondition(searchOpts)
if allPublic {
if !searchOpts.Private {
return []int64{0}, allPublic, nil // sees nothing beyond is_public, so skip the query
}
cond = cond.And(repo_model.NotPublicRepoUnderPublicOwnerCond()) // enumerating them scales with the instance
}
repoIDs, err = repo_model.SearchRepositoryIDsByCondition(ctx, cond)
if err != nil {
return nil, false, err
}
if len(repoIDs) == 0 {
// no repos found, don't let the indexer return all repos
repoIDs = []int64{0}
}
return repoIDs, allPublic, nil
}
routers/common/issue_filter_test.go
+93
View File
@@ -0,0 +1,93 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package common
import (
"testing"
"gitea.dev/models/unittest"
user_model "gitea.dev/models/user"
"gitea.dev/modules/util"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestSearchIssuesRepoIDs(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
// the indexer's is_public covers repo 1 (public under a public owner) but misses repo 38 (public
// under a limited org) and repo 40 (public under a private org)
cases := []struct {
name string
doerID int64
opts SearchIssuesRepoIDsOptions
allPublic bool
want []int64
wantErr error
}{
{
name: "site admin", // admins skip the accessible repository condition entirely
doerID: 1,
allPublic: true,
want: []int64{2, 38, 40},
},
{
name: "regular user",
doerID: 2,
allPublic: true,
want: []int64{2, 38},
},
{
name: "private org member",
doerID: 5,
allPublic: true,
want: []int64{38, 40},
},
{
name: "anonymous",
allPublic: true,
want: []int64{0}, // the placeholder keeps the indexer off "every repository"
},
{
name: "public-only token",
doerID: 2,
opts: SearchIssuesRepoIDsOptions{PublicOnly: true},
allPublic: true,
want: []int64{0},
},
{
name: "owner filter", // turns allPublic off, so public repos must still be enumerated
doerID: 2,
opts: SearchIssuesRepoIDsOptions{OwnerName: "user2"},
want: []int64{1, 2},
},
{
name: "team without owner",
opts: SearchIssuesRepoIDsOptions{TeamName: "team1"},
wantErr: util.ErrInvalidArgument,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
opts := tc.opts
if tc.doerID != 0 {
opts.Doer = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: tc.doerID})
}
repoIDs, allPublic, err := SearchIssuesRepoIDs(t.Context(), opts)
if tc.wantErr != nil {
assert.ErrorIs(t, err, tc.wantErr)
return
}
require.NoError(t, err)
assert.Equal(t, tc.allPublic, allPublic)
assert.Subset(t, repoIDs, tc.want)
if allPublic {
assert.NotContains(t, repoIDs, int64(1), "already matched by the indexer's is_public")
}
})
}
}
routers/web/repo/issue_list.go
+18 -91
View File
@@ -5,6 +5,7 @@ package repo
import (
"bytes"
"errors"
"maps"
"net/http"
"slices"
@@ -15,7 +16,6 @@ import (
"gitea.dev/models/db"
git_model "gitea.dev/models/git"
issues_model "gitea.dev/models/issues"
"gitea.dev/models/organization"
repo_model "gitea.dev/models/repo"
"gitea.dev/models/unit"
user_model "gitea.dev/models/user"
@@ -56,85 +56,22 @@ func SearchIssues(ctx *context.Context) {
isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
var (
repoIDs []int64
allPublic bool
)
{
// find repos user can access (for issue search)
opts := repo_model.SearchRepoOptions{
Private: false,
AllPublic: true,
TopicOnly: false,
Collaborate: optional.None[bool](),
// This needs to be a column that is not nil in fixtures or
// MySQL will return different results when sorting by null in some cases
OrderBy: db.SearchOrderByAlphabetically,
Actor: ctx.Doer,
}
if ctx.IsSigned {
opts.Private = true
opts.AllLimited = true
}
if ctx.FormString("owner") != "" {
owner, err := user_model.GetUserByName(ctx, ctx.FormString("owner"))
if err != nil {
if user_model.IsErrUserNotExist(err) {
ctx.HTTPError(http.StatusBadRequest, "Owner not found", err.Error())
} else {
ctx.HTTPError(http.StatusInternalServerError, "GetUserByName", err.Error())
}
return
}
opts.OwnerID = owner.ID
opts.AllLimited = false
opts.AllPublic = false
opts.Collaborate = optional.Some(false)
}
if ctx.FormString("team") != "" {
if ctx.FormString("owner") == "" {
ctx.HTTPError(http.StatusBadRequest, "", "Owner organisation is required for filtering on team")
return
}
team, err := organization.GetTeam(ctx, opts.OwnerID, ctx.FormString("team"))
if err != nil {
if organization.IsErrTeamNotExist(err) {
ctx.HTTPError(http.StatusBadRequest, "Team not found", err.Error())
} else {
ctx.HTTPError(http.StatusInternalServerError, "GetUserByName", err.Error())
}
return
}
opts.TeamID = team.ID
}
if opts.AllPublic {
allPublic = true
opts.AllPublic = false // set it false to avoid returning too many repos, we could filter by indexer
}
repoIDs, _, err = repo_model.SearchRepositoryIDs(ctx, opts)
if err != nil {
ctx.HTTPError(http.StatusInternalServerError, "SearchRepositoryIDs", err.Error())
return
}
if len(repoIDs) == 0 {
// no repos found, don't let the indexer return all repos
repoIDs = []int64{0}
repoIDs, allPublic, err := common.SearchIssuesRepoIDs(ctx, common.SearchIssuesRepoIDsOptions{
Doer: ctx.Doer,
OwnerName: ctx.FormString("owner"),
TeamName: ctx.FormString("team"),
})
if err != nil {
if errors.Is(err, util.ErrNotExist) || errors.Is(err, util.ErrInvalidArgument) {
ctx.HTTPError(http.StatusBadRequest, err.Error())
} else {
ctx.ServerError("SearchIssuesRepoIDs", err)
}
return
}
keyword := ctx.FormTrim("q")
if strings.IndexByte(keyword, 0) >= 0 {
keyword = ""
}
isPull := optional.None[bool]()
switch ctx.FormString("type") {
case "pulls":
isPull = optional.Some(true)
case "issues":
isPull = optional.Some(false)
}
isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type"))
var includedAnyLabels []int64
{
@@ -145,7 +82,7 @@ func SearchIssues(ctx *context.Context) {
}
includedAnyLabels, err = issues_model.GetLabelIDsByNames(ctx, includedLabelNames)
if err != nil {
ctx.HTTPError(http.StatusInternalServerError, "GetLabelIDsByNames", err.Error())
ctx.ServerError("GetLabelIDsByNames", err)
return
}
}
@@ -159,7 +96,7 @@ func SearchIssues(ctx *context.Context) {
}
includedMilestones, err = issues_model.GetMilestoneIDsByNames(ctx, includedMilestoneNames)
if err != nil {
ctx.HTTPError(http.StatusInternalServerError, "GetMilestoneIDsByNames", err.Error())
ctx.ServerError("GetMilestoneIDsByNames", err)
return
}
}
@@ -223,12 +160,12 @@ func SearchIssues(ctx *context.Context) {
ids, total, err := issue_indexer.SearchIssues(ctx, searchOpt)
if err != nil {
ctx.HTTPError(http.StatusInternalServerError, "SearchIssues", err.Error())
ctx.ServerError("SearchIssues", err)
return
}
issues, err := issues_model.GetIssuesByIDs(ctx, ids, true)
if err != nil {
ctx.HTTPError(http.StatusInternalServerError, "FindIssuesByIDs", err.Error())
ctx.ServerError("FindIssuesByIDs", err)
return
}
@@ -267,11 +204,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) {
}
isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormString("state"))
keyword := ctx.FormTrim("q")
if strings.IndexByte(keyword, 0) >= 0 {
keyword = ""
}
var mileIDs []int64
if part := strings.Split(ctx.FormString("milestones"), ","); len(part) > 0 {
@@ -303,13 +236,7 @@ func SearchRepoIssuesJSON(ctx *context.Context) {
}
}
isPull := optional.None[bool]()
switch ctx.FormString("type") {
case "pulls":
isPull = optional.Some(true)
case "issues":
isPull = optional.Some(false)
}
isPull := common.ParseIssueFilterTypeIsPull(ctx.FormString("type"))
// FIXME: we should be more efficient here
createdByID := getUserIDForFilter(ctx, "created_by")