From 9342bc914f267c5e05ee75a34f8d204bd2a9c4de Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 17 Feb 2026 12:33:43 -0800 Subject: [PATCH 1/3] Fix track time list permission check --- services/convert/issue.go | 18 ++++++++++++++++++ services/convert/issue_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/services/convert/issue.go b/services/convert/issue.go index b396dd0737..53d05a509a 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -226,7 +226,25 @@ func ToStopWatches(ctx context.Context, doer *user_model.User, sws []*issues_mod // ToTrackedTimeList converts TrackedTimeList to API format func ToTrackedTimeList(ctx context.Context, doer *user_model.User, tl issues_model.TrackedTimeList) api.TrackedTimeList { result := make([]*api.TrackedTime, 0, len(tl)) + permCache := make(map[int64]access_model.Permission) for _, t := range tl { + if t.Issue != nil { + if err := t.Issue.LoadRepo(ctx); err != nil { + continue + } + perm, ok := permCache[t.Issue.RepoID] + if !ok { + var err error + perm, err = access_model.GetUserRepoPermission(ctx, t.Issue.Repo, doer) + if err != nil { + continue + } + permCache[t.Issue.RepoID] = perm + } + if !perm.CanReadIssuesOrPulls(t.Issue.IsPull) { + continue + } + } result = append(result, ToTrackedTime(ctx, doer, t)) } return result diff --git a/services/convert/issue_test.go b/services/convert/issue_test.go index a12a69288a..6202af2ad2 100644 --- a/services/convert/issue_test.go +++ b/services/convert/issue_test.go @@ -83,3 +83,30 @@ func TestToStopWatchesRespectsPermissions(t *testing.T) { assert.Len(t, visibleAdmin, 2) assert.ElementsMatch(t, []string{"repo1", "repo3"}, []string{visibleAdmin[0].RepoName, visibleAdmin[1].RepoName}) } + +func TestToTrackedTimeListRespectsPermissions(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + publicIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: 1}) + privateIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: 3}) + + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + adminUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + + publicTT := &issues_model.TrackedTime{IssueID: publicIssue.ID, UserID: regularUser.ID, Time: 3600} + privateTT := &issues_model.TrackedTime{IssueID: privateIssue.ID, UserID: regularUser.ID, Time: 1800} + assert.NoError(t, db.Insert(ctx, publicTT)) + assert.NoError(t, db.Insert(ctx, privateTT)) + + trackedTimes := issues_model.TrackedTimeList{publicTT, privateTT} + assert.NoError(t, trackedTimes.LoadAttributes(ctx)) + + visible := ToTrackedTimeList(ctx, regularUser, trackedTimes) + assert.Len(t, visible, 1) + assert.Equal(t, "repo1", visible[0].Issue.Repo.Name) + + visibleAdmin := ToTrackedTimeList(ctx, adminUser, trackedTimes) + assert.Len(t, visibleAdmin, 2) + assert.ElementsMatch(t, []string{"repo1", "repo3"}, []string{visibleAdmin[0].Issue.Repo.Name, visibleAdmin[1].Issue.Repo.Name}) +} From 085b892aec0137ae3dd754c9226a795ea0083b01 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 19 Feb 2026 14:09:53 -0800 Subject: [PATCH 2/3] Update services/convert/issue.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Lunny Xiao --- services/convert/issue.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/services/convert/issue.go b/services/convert/issue.go index 53d05a509a..5ff720fe01 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -228,22 +228,24 @@ func ToTrackedTimeList(ctx context.Context, doer *user_model.User, tl issues_mod result := make([]*api.TrackedTime, 0, len(tl)) permCache := make(map[int64]access_model.Permission) for _, t := range tl { - if t.Issue != nil { - if err := t.Issue.LoadRepo(ctx); err != nil { - continue - } - perm, ok := permCache[t.Issue.RepoID] - if !ok { - var err error - perm, err = access_model.GetUserRepoPermission(ctx, t.Issue.Repo, doer) - if err != nil { - continue - } - permCache[t.Issue.RepoID] = perm - } - if !perm.CanReadIssuesOrPulls(t.Issue.IsPull) { + // If the issue is not loaded, conservatively skip this entry to avoid bypassing permission checks. + if t.Issue == nil { + continue + } + if err := t.Issue.LoadRepo(ctx); err != nil { + continue + } + perm, ok := permCache[t.Issue.RepoID] + if !ok { + var err error + perm, err = access_model.GetUserRepoPermission(ctx, t.Issue.Repo, doer) + if err != nil { continue } + permCache[t.Issue.RepoID] = perm + } + if !perm.CanReadIssuesOrPulls(t.Issue.IsPull) { + continue } result = append(result, ToTrackedTime(ctx, doer, t)) } From c8a36b3a0835cd6a13ea4cb3955e01883f80d758 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 19 Feb 2026 20:09:35 -0800 Subject: [PATCH 3/3] Add more tests --- services/convert/issue_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/services/convert/issue_test.go b/services/convert/issue_test.go index 6202af2ad2..85d7e3d795 100644 --- a/services/convert/issue_test.go +++ b/services/convert/issue_test.go @@ -110,3 +110,33 @@ func TestToTrackedTimeListRespectsPermissions(t *testing.T) { assert.Len(t, visibleAdmin, 2) assert.ElementsMatch(t, []string{"repo1", "repo3"}, []string{visibleAdmin[0].Issue.Repo.Name, visibleAdmin[1].Issue.Repo.Name}) } + +func TestToTrackedTimeListSkipsUnloadedIssues(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + publicIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{RepoID: 1}) + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + + publicTT := &issues_model.TrackedTime{IssueID: publicIssue.ID, UserID: regularUser.ID, Time: 3600} + assert.NoError(t, db.Insert(ctx, publicTT)) + + trackedTimes := issues_model.TrackedTimeList{publicTT} + visible := ToTrackedTimeList(ctx, regularUser, trackedTimes) + assert.Empty(t, visible) +} + +func TestToTrackedTimeListSkipsRepoLoadErrors(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + regularUser := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5}) + + trackedTimes := issues_model.TrackedTimeList{ + { + Issue: &issues_model.Issue{RepoID: 999999, IsPull: false}, + }, + } + visible := ToTrackedTimeList(ctx, regularUser, trackedTimes) + assert.Empty(t, visible) +}