From b1b635c1d9a8d052e10b605c7f69048f4e564d4e Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 29 Jan 2025 09:47:37 +0100 Subject: [PATCH 1/2] fix(sec): permission check for project issue - Do an access check when loading issues for a project column, currently this is not done and exposes the title, labels and existence of a private issue that the viewer of the project board may not have access to. - The number of issues cannot be calculated in a efficient manner and stored in the database because their number may vary depending on the visibility of the repositories participating in the project. The previous implementation used the pre-calculated numbers stored in each project, which did not reflect that potential variation. - The code is derived from https://github.com/go-gitea/gitea/pull/22865 --- models/issues/issue_project.go | 62 ++++++++++++++++++++++++++++------ models/project/column.go | 14 -------- models/project/issue.go | 14 -------- routers/web/org/projects.go | 15 +++++++- routers/web/repo/projects.go | 15 +++++++- templates/projects/list.tmpl | 4 +-- templates/projects/view.tmpl | 2 +- 7 files changed, 83 insertions(+), 43 deletions(-) diff --git a/models/issues/issue_project.go b/models/issues/issue_project.go index 835ea1db52..f606b713cf 100644 --- a/models/issues/issue_project.go +++ b/models/issues/issue_project.go @@ -7,8 +7,10 @@ import ( "context" "code.gitea.io/gitea/models/db" + org_model "code.gitea.io/gitea/models/organization" project_model "code.gitea.io/gitea/models/project" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/util" ) @@ -48,22 +50,29 @@ func (issue *Issue) ProjectColumnID(ctx context.Context) int64 { } // LoadIssuesFromColumn load issues assigned to this column -func LoadIssuesFromColumn(ctx context.Context, b *project_model.Column) (IssueList, error) { - issueList, err := Issues(ctx, &IssuesOptions{ +func LoadIssuesFromColumn(ctx context.Context, b *project_model.Column, doer *user_model.User, org *org_model.Organization, isClosed optional.Option[bool]) (IssueList, error) { + issueOpts := &IssuesOptions{ ProjectColumnID: b.ID, ProjectID: b.ProjectID, SortType: "project-column-sorting", - }) + IsClosed: isClosed, + } + if doer != nil { + issueOpts.User = doer + issueOpts.Org = org + } else { + issueOpts.AllPublic = true + } + + issueList, err := Issues(ctx, issueOpts) if err != nil { return nil, err } if b.Default { - issues, err := Issues(ctx, &IssuesOptions{ - ProjectColumnID: db.NoConditionID, - ProjectID: b.ProjectID, - SortType: "project-column-sorting", - }) + issueOpts.ProjectColumnID = db.NoConditionID + + issues, err := Issues(ctx, issueOpts) if err != nil { return nil, err } @@ -78,10 +87,10 @@ func LoadIssuesFromColumn(ctx context.Context, b *project_model.Column) (IssueLi } // LoadIssuesFromColumnList load issues assigned to the columns -func LoadIssuesFromColumnList(ctx context.Context, bs project_model.ColumnList) (map[int64]IssueList, error) { +func LoadIssuesFromColumnList(ctx context.Context, bs project_model.ColumnList, doer *user_model.User, org *org_model.Organization, isClosed optional.Option[bool]) (map[int64]IssueList, error) { issuesMap := make(map[int64]IssueList, len(bs)) for i := range bs { - il, err := LoadIssuesFromColumn(ctx, bs[i]) + il, err := LoadIssuesFromColumn(ctx, bs[i], doer, org, isClosed) if err != nil { return nil, err } @@ -160,3 +169,36 @@ func IssueAssignOrRemoveProject(ctx context.Context, issue *Issue, doer *user_mo }) }) } + +// NumIssuesInProjects returns the amount of issues assigned to one of the project +// in the list which the doer can access. +func NumIssuesInProjects(ctx context.Context, pl []*project_model.Project, doer *user_model.User, org *org_model.Organization, isClosed optional.Option[bool]) (map[int64]int, error) { + numMap := make(map[int64]int, len(pl)) + for _, p := range pl { + num, err := NumIssuesInProject(ctx, p, doer, org, isClosed) + if err != nil { + return nil, err + } + numMap[p.ID] = num + } + + return numMap, nil +} + +// NumIssuesInProject returns the amount of issues assigned to the project which +// the doer can access. +func NumIssuesInProject(ctx context.Context, p *project_model.Project, doer *user_model.User, org *org_model.Organization, isClosed optional.Option[bool]) (int, error) { + numIssuesInProject := int(0) + bs, err := p.GetColumns(ctx) + if err != nil { + return 0, err + } + im, err := LoadIssuesFromColumnList(ctx, bs, doer, org, isClosed) + if err != nil { + return 0, err + } + for _, il := range im { + numIssuesInProject += len(il) + } + return numIssuesInProject, nil +} diff --git a/models/project/column.go b/models/project/column.go index b30aab4699..de06ce8b04 100644 --- a/models/project/column.go +++ b/models/project/column.go @@ -57,20 +57,6 @@ func (Column) TableName() string { return "project_board" // TODO: the legacy table name should be project_column } -// NumIssues return counter of all issues assigned to the column -func (c *Column) NumIssues(ctx context.Context) int { - total, err := db.GetEngine(ctx).Table("project_issue"). - Where("project_id=?", c.ProjectID). - And("project_board_id=?", c.ID). - GroupBy("issue_id"). - Cols("issue_id"). - Count() - if err != nil { - return 0 - } - return int(total) -} - func (c *Column) GetIssues(ctx context.Context) ([]*ProjectIssue, error) { issues := make([]*ProjectIssue, 0, 5) if err := db.GetEngine(ctx).Where("project_id=?", c.ProjectID). diff --git a/models/project/issue.go b/models/project/issue.go index 3361b533b9..984f47ee7c 100644 --- a/models/project/issue.go +++ b/models/project/issue.go @@ -34,20 +34,6 @@ func deleteProjectIssuesByProjectID(ctx context.Context, projectID int64) error return err } -// NumIssues return counter of all issues assigned to a project -func (p *Project) NumIssues(ctx context.Context) int { - c, err := db.GetEngine(ctx).Table("project_issue"). - Where("project_id=?", p.ID). - GroupBy("issue_id"). - Cols("issue_id"). - Count() - if err != nil { - log.Error("NumIssues: %v", err) - return 0 - } - return int(c) -} - // NumClosedIssues return counter of closed issues assigned to a project func (p *Project) NumClosedIssues(ctx context.Context) int { c, err := db.GetEngine(ctx).Table("project_issue"). diff --git a/routers/web/org/projects.go b/routers/web/org/projects.go index 32eb6eeef1..034483aba2 100644 --- a/routers/web/org/projects.go +++ b/routers/web/org/projects.go @@ -126,6 +126,19 @@ func Projects(ctx *context.Context) { ctx.Data["PageIsViewProjects"] = true ctx.Data["SortType"] = sortType + numOpenIssues, err := issues_model.NumIssuesInProjects(ctx, projects, ctx.Doer, ctx.Org.Organization, optional.Some(false)) + if err != nil { + ctx.ServerError("NumIssuesInProjects", err) + return + } + numClosedIssues, err := issues_model.NumIssuesInProjects(ctx, projects, ctx.Doer, ctx.Org.Organization, optional.Some(true)) + if err != nil { + ctx.ServerError("NumIssuesInProjects", err) + return + } + ctx.Data["NumOpenIssuesInProject"] = numOpenIssues + ctx.Data["NumClosedIssuesInProject"] = numClosedIssues + ctx.HTML(http.StatusOK, tplProjects) } @@ -332,7 +345,7 @@ func ViewProject(ctx *context.Context) { return } - issuesMap, err := issues_model.LoadIssuesFromColumnList(ctx, columns) + issuesMap, err := issues_model.LoadIssuesFromColumnList(ctx, columns, ctx.Doer, ctx.Org.Organization, optional.None[bool]()) if err != nil { ctx.ServerError("LoadIssuesOfColumns", err) return diff --git a/routers/web/repo/projects.go b/routers/web/repo/projects.go index 55a422453f..0689b0a721 100644 --- a/routers/web/repo/projects.go +++ b/routers/web/repo/projects.go @@ -125,6 +125,19 @@ func Projects(ctx *context.Context) { ctx.Data["IsProjectsPage"] = true ctx.Data["SortType"] = sortType + numOpenIssues, err := issues_model.NumIssuesInProjects(ctx, projects, ctx.Doer, ctx.Org.Organization, optional.Some(false)) + if err != nil { + ctx.ServerError("NumIssuesInProjects", err) + return + } + numClosedIssues, err := issues_model.NumIssuesInProjects(ctx, projects, ctx.Doer, ctx.Org.Organization, optional.Some(true)) + if err != nil { + ctx.ServerError("NumIssuesInProjects", err) + return + } + ctx.Data["NumOpenIssuesInProject"] = numOpenIssues + ctx.Data["NumClosedIssuesInProject"] = numClosedIssues + ctx.HTML(http.StatusOK, tplProjects) } @@ -310,7 +323,7 @@ func ViewProject(ctx *context.Context) { return } - issuesMap, err := issues_model.LoadIssuesFromColumnList(ctx, columns) + issuesMap, err := issues_model.LoadIssuesFromColumnList(ctx, columns, ctx.Doer, nil, optional.None[bool]()) if err != nil { ctx.ServerError("LoadIssuesOfColumns", err) return diff --git a/templates/projects/list.tmpl b/templates/projects/list.tmpl index 5da2757748..da77f7f6fb 100644 --- a/templates/projects/list.tmpl +++ b/templates/projects/list.tmpl @@ -49,11 +49,11 @@
{{svg "octicon-issue-opened" 14}} - {{ctx.Locale.PrettyNumber (.NumOpenIssues ctx)}} {{ctx.Locale.Tr "repo.issues.open_title"}} + {{ctx.Locale.PrettyNumber (index $.NumOpenIssuesInProject .ID)}} {{ctx.Locale.Tr "repo.issues.open_title"}}
{{svg "octicon-check" 14}} - {{ctx.Locale.PrettyNumber (.NumClosedIssues ctx)}} {{ctx.Locale.Tr "repo.issues.closed_title"}} + {{ctx.Locale.PrettyNumber (index $.NumClosedIssuesInProject .ID)}} {{ctx.Locale.Tr "repo.issues.closed_title"}}
{{if and $.CanWriteProjects (not $.Repository.IsArchived)}} diff --git a/templates/projects/view.tmpl b/templates/projects/view.tmpl index 08b08b95e5..0e2fc87958 100644 --- a/templates/projects/view.tmpl +++ b/templates/projects/view.tmpl @@ -70,7 +70,7 @@
- {{.NumIssues ctx}} + {{len (index $.IssuesMap .ID)}}
{{.Title}}
From 51060d982670183402a12647a90f8d9c1f245511 Mon Sep 17 00:00:00 2001 From: Gusted Date: Wed, 29 Jan 2025 11:50:25 +0100 Subject: [PATCH 2/2] fix(sec): add tests for private issues on projects - Add integration and unit tests to ensure that private issues on projects are not shown in any way, shape or form when the doer has no access to it. --- .../fixtures/PrivateIssueProjects/project.yml | 23 ++++ .../PrivateIssueProjects/project_board.yml | 17 +++ .../PrivateIssueProjects/project_issue.yml | 11 ++ models/fixtures/team_unit.yml | 7 ++ models/issues/issue_project_test.go | 100 ++++++++++++++++++ tests/integration/private_project_test.go | 84 +++++++++++++++ 6 files changed, 242 insertions(+) create mode 100644 models/fixtures/PrivateIssueProjects/project.yml create mode 100644 models/fixtures/PrivateIssueProjects/project_board.yml create mode 100644 models/fixtures/PrivateIssueProjects/project_issue.yml create mode 100644 models/issues/issue_project_test.go create mode 100644 tests/integration/private_project_test.go diff --git a/models/fixtures/PrivateIssueProjects/project.yml b/models/fixtures/PrivateIssueProjects/project.yml new file mode 100644 index 0000000000..cf63e4e413 --- /dev/null +++ b/models/fixtures/PrivateIssueProjects/project.yml @@ -0,0 +1,23 @@ +- + id: 1001 + title: Org project that contains private issues + owner_id: 3 + repo_id: 0 + is_closed: false + creator_id: 2 + board_type: 1 + type: 3 + created_unix: 1738000000 + updated_unix: 1738000000 + +- + id: 1002 + title: User project that contains private issues + owner_id: 2 + repo_id: 0 + is_closed: false + creator_id: 2 + board_type: 1 + type: 1 + created_unix: 1738000000 + updated_unix: 1738000000 diff --git a/models/fixtures/PrivateIssueProjects/project_board.yml b/models/fixtures/PrivateIssueProjects/project_board.yml new file mode 100644 index 0000000000..3f1fe1e705 --- /dev/null +++ b/models/fixtures/PrivateIssueProjects/project_board.yml @@ -0,0 +1,17 @@ +- + id: 1001 + project_id: 1001 + title: Triage + creator_id: 2 + default: true + created_unix: 1738000000 + updated_unix: 1738000000 + +- + id: 1002 + project_id: 1002 + title: Triage + creator_id: 2 + default: true + created_unix: 1738000000 + updated_unix: 1738000000 diff --git a/models/fixtures/PrivateIssueProjects/project_issue.yml b/models/fixtures/PrivateIssueProjects/project_issue.yml new file mode 100644 index 0000000000..222b2e5f71 --- /dev/null +++ b/models/fixtures/PrivateIssueProjects/project_issue.yml @@ -0,0 +1,11 @@ +- + id: 1001 + issue_id: 6 + project_id: 1001 + project_board_id: 1001 + +- + id: 1002 + issue_id: 7 + project_id: 1002 + project_board_id: 1002 diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index de0e8d738b..e8f8d0e422 100644 --- a/models/fixtures/team_unit.yml +++ b/models/fixtures/team_unit.yml @@ -1,42 +1,49 @@ - id: 1 team_id: 1 + org_id: 3 type: 1 access_mode: 4 - id: 2 team_id: 1 + org_id: 3 type: 2 access_mode: 4 - id: 3 team_id: 1 + org_id: 3 type: 3 access_mode: 4 - id: 4 team_id: 1 + org_id: 3 type: 4 access_mode: 4 - id: 5 team_id: 1 + org_id: 3 type: 5 access_mode: 4 - id: 6 team_id: 1 + org_id: 3 type: 6 access_mode: 4 - id: 7 team_id: 1 + org_id: 3 type: 7 access_mode: 4 diff --git a/models/issues/issue_project_test.go b/models/issues/issue_project_test.go new file mode 100644 index 0000000000..6ebc803722 --- /dev/null +++ b/models/issues/issue_project_test.go @@ -0,0 +1,100 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package issues_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/project" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPrivateIssueProjects(t *testing.T) { + defer tests.AddFixtures("models/fixtures/PrivateIssueProjects/")() + require.NoError(t, unittest.PrepareTestDatabase()) + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + t.Run("Organization project", func(t *testing.T) { + org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) + orgProject := unittest.AssertExistsAndLoadBean(t, &project.Project{ID: 1001, OwnerID: org.ID}) + column := unittest.AssertExistsAndLoadBean(t, &project.Column{ID: 1001, ProjectID: orgProject.ID}) + + t.Run("Authenticated user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, org, optional.None[bool]()) + require.NoError(t, err) + assert.Len(t, issueList, 1) + assert.EqualValues(t, 6, issueList[0].ID) + + issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.None[bool]()) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(true)) + require.NoError(t, err) + assert.EqualValues(t, 0, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, orgProject, user2, org, optional.Some(false)) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + }) + + t.Run("Anonymous user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, org, optional.None[bool]()) + require.NoError(t, err) + assert.Empty(t, issueList) + + issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, orgProject, nil, org, optional.None[bool]()) + require.NoError(t, err) + assert.EqualValues(t, 0, issuesNum) + }) + }) + + t.Run("User project", func(t *testing.T) { + userProject := unittest.AssertExistsAndLoadBean(t, &project.Project{ID: 1002, OwnerID: user2.ID}) + column := unittest.AssertExistsAndLoadBean(t, &project.Column{ID: 1002, ProjectID: userProject.ID}) + + t.Run("Authenticated user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, user2, nil, optional.None[bool]()) + require.NoError(t, err) + assert.Len(t, issueList, 1) + assert.EqualValues(t, 7, issueList[0].ID) + + issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.None[bool]()) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(true)) + require.NoError(t, err) + assert.EqualValues(t, 0, issuesNum) + + issuesNum, err = issues.NumIssuesInProject(db.DefaultContext, userProject, user2, nil, optional.Some(false)) + require.NoError(t, err) + assert.EqualValues(t, 1, issuesNum) + }) + + t.Run("Anonymous user", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + issueList, err := issues.LoadIssuesFromColumn(db.DefaultContext, column, nil, nil, optional.None[bool]()) + require.NoError(t, err) + assert.Empty(t, issueList) + + issuesNum, err := issues.NumIssuesInProject(db.DefaultContext, userProject, nil, nil, optional.None[bool]()) + require.NoError(t, err) + assert.EqualValues(t, 0, issuesNum) + }) + }) +} diff --git a/tests/integration/private_project_test.go b/tests/integration/private_project_test.go new file mode 100644 index 0000000000..1a4adb4366 --- /dev/null +++ b/tests/integration/private_project_test.go @@ -0,0 +1,84 @@ +// Copyright 2025 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "net/http" + "strings" + "testing" + + org_model "code.gitea.io/gitea/models/organization" + project_model "code.gitea.io/gitea/models/project" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestPrivateIssueProject(t *testing.T) { + defer tests.AddFixtures("models/fixtures/PrivateIssueProjects/")() + defer tests.PrepareTestEnv(t)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + sess := loginUser(t, user2.Name) + + test := func(t *testing.T, sess *TestSession, username string, projectID int64, hasAccess bool) { + t.Helper() + defer tests.PrintCurrentTest(t, 1)() + + // Test that the projects overview page shows the correct open and close issues. + req := NewRequestf(t, "GET", "%s/-/projects", username) + resp := sess.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + openCloseStats := htmlDoc.Find(".milestone-toolbar .group").First().Text() + if hasAccess { + assert.Contains(t, openCloseStats, "1\u00a0Open") + } else { + assert.Contains(t, openCloseStats, "0\u00a0Open") + } + assert.Contains(t, openCloseStats, "0\u00a0Closed") + + // Check that on the project itself the issue is not shown. + req = NewRequestf(t, "GET", "%s/-/projects/%d", username, projectID) + resp = sess.MakeRequest(t, req, http.StatusOK) + + htmlDoc = NewHTMLParser(t, resp.Body) + htmlDoc.AssertElement(t, ".project-column .issue-card", hasAccess) + + // And that the issue count is correct. + issueCount := strings.TrimSpace(htmlDoc.Find(".project-column-issue-count").Text()) + if hasAccess { + assert.EqualValues(t, "1", issueCount) + } else { + assert.EqualValues(t, "0", issueCount) + } + } + + t.Run("Organization project", func(t *testing.T) { + org := unittest.AssertExistsAndLoadBean(t, &org_model.Organization{ID: 3}) + orgProject := unittest.AssertExistsAndLoadBean(t, &project_model.Project{ID: 1001, OwnerID: org.ID}) + + t.Run("Authenticated user", func(t *testing.T) { + test(t, sess, org.Name, orgProject.ID, true) + }) + + t.Run("Anonymous user", func(t *testing.T) { + test(t, emptyTestSession(t), org.Name, orgProject.ID, false) + }) + }) + + t.Run("User project", func(t *testing.T) { + userProject := unittest.AssertExistsAndLoadBean(t, &project_model.Project{ID: 1002, OwnerID: user2.ID}) + + t.Run("Authenticated user", func(t *testing.T) { + test(t, sess, user2.Name, userProject.ID, true) + }) + + t.Run("Anonymous user", func(t *testing.T) { + test(t, emptyTestSession(t), user2.Name, userProject.ID, false) + }) + }) +}