From 781ffac198548ae4f3d1febc613caed8e0e11d01 Mon Sep 17 00:00:00 2001 From: kolaente Date: Sun, 19 Jul 2026 15:29:00 +0200 Subject: [PATCH] fix(security): require Admin to detach a project from its parent (GHSA-44v6-7fxq-vgf4) --- pkg/models/project.go | 99 ++++++++++++------- pkg/models/project_duplicate.go | 2 +- pkg/models/project_permissions.go | 10 +- pkg/models/project_repair.go | 6 +- pkg/models/project_repair_test.go | 4 +- pkg/models/project_test.go | 71 ++++++++++--- pkg/models/user_delete.go | 2 +- pkg/models/user_project.go | 4 +- .../migration/create_from_structure.go | 8 +- .../migration/create_from_structure_test.go | 2 +- pkg/modules/migration/csv/csv.go | 2 +- .../microsoft-todo/microsoft_todo.go | 2 +- .../microsoft-todo/microsoft_todo_test.go | 4 +- pkg/modules/migration/ticktick/ticktick.go | 2 +- .../migration/ticktick/ticktick_test.go | 4 +- pkg/modules/migration/todoist/todoist.go | 2 +- pkg/modules/migration/todoist/todoist_test.go | 6 +- pkg/modules/migration/trello/trello.go | 2 +- pkg/modules/migration/trello/trello_test.go | 8 +- pkg/webtests/project_test.go | 8 ++ 20 files changed, 165 insertions(+), 83 deletions(-) diff --git a/pkg/models/project.go b/pkg/models/project.go index c6b80af03..01d78f788 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -49,7 +49,7 @@ type Project struct { HexColor string `xorm:"varchar(6) null" json:"hex_color" valid:"runelength(0|7)" maxLength:"7" doc:"The hex color of this project, without the leading #."` OwnerID int64 `xorm:"bigint INDEX not null" json:"-"` - ParentProjectID int64 `xorm:"bigint INDEX null" json:"parent_project_id" doc:"The id of the parent project. 0 if this is a top-level project."` + ParentProjectID *int64 `xorm:"bigint INDEX null" json:"parent_project_id,omitempty" doc:"The id of the parent project. 0 or omitted for a top-level project. Sending an explicit 0 detaches the project to the top level and requires admin permission."` ParentProject *Project `xorm:"-" json:"-"` // The user who created this project. @@ -111,6 +111,21 @@ func (p *Project) TableName() string { return "projects" } +// Ptr returns a pointer to v. Useful for optional numeric fields like +// ParentProjectID where nil (omitted) must stay distinct from an explicit 0. +func Ptr[T any](v T) *T { + return &v +} + +// parentID dereferences ParentProjectID, treating nil (field omitted on a +// partial update) as 0 — no parent. +func (p *Project) parentID() int64 { + if p.ParentProjectID == nil { + return 0 + } + return *p.ParentProjectID +} + // ProjectBackgroundType holds a project background type type ProjectBackgroundType struct { Type string @@ -231,7 +246,7 @@ func getAllRawProjects(s *xorm.Session, a web.Auth, search string, page int, per projects := []*Project{project} err = addProjectDetails(s, projects, a) if err == nil && len(projects) > 0 { - projects[0].ParentProjectID = 0 + projects[0].ParentProjectID = Ptr(int64(0)) } return projects, 0, 0, err } @@ -381,7 +396,7 @@ func (p *Project) ReadOne(s *xorm.Session, a web.Auth) (err error) { _, isShareAuth := a.(*LinkSharing) if isShareAuth { - p.ParentProjectID = 0 + p.ParentProjectID = Ptr(int64(0)) } // Get project owner @@ -953,8 +968,8 @@ func addMaxPermissionToProjects(s *xorm.Session, projects []*Project, u *user.Us func (p *Project) CheckIsArchived(s *xorm.Session) (err error) { if p.ID == 0 { // New project — skip checking the project itself but still check the parent. - if p.ParentProjectID > 0 { - parent := &Project{ID: p.ParentProjectID} + if pid := p.parentID(); pid > 0 { + parent := &Project{ID: pid} return parent.CheckIsArchived(s) } return nil @@ -969,8 +984,8 @@ func (p *Project) CheckIsArchived(s *xorm.Session) (err error) { return ErrProjectIsArchived{ProjectID: p.ID} } - if project.ParentProjectID > 0 { - parent := &Project{ID: project.ParentProjectID} + if pid := project.parentID(); pid > 0 { + parent := &Project{ID: pid} return parent.CheckIsArchived(s) } @@ -978,32 +993,33 @@ func (p *Project) CheckIsArchived(s *xorm.Session) (err error) { } func checkProjectBeforeUpdateOrDelete(s *xorm.Session, project *Project) (err error) { - if project.ParentProjectID < 0 { - return &ErrProjectCannotBelongToAPseudoParentProject{ProjectID: project.ID, ParentProjectID: project.ParentProjectID} + parentID := project.parentID() + if parentID < 0 { + return &ErrProjectCannotBelongToAPseudoParentProject{ProjectID: project.ID, ParentProjectID: parentID} } // Check if the parent project exists - if project.ParentProjectID > 0 { - if project.ParentProjectID == project.ID { + if parentID > 0 { + if parentID == project.ID { return &ErrProjectCannotBeChildOfItself{ ProjectID: project.ID, } } - allProjects, err := GetAllParentProjects(s, project.ParentProjectID) + allProjects, err := GetAllParentProjects(s, parentID) if err != nil { return err } var parent *Project - parent = allProjects[project.ParentProjectID] + parent = allProjects[parentID] // Check if there's a cycle in the parent relation parentsVisited := make(map[int64]bool) parentsVisited[project.ID] = true - for parent.ParentProjectID != 0 { + for parent.parentID() != 0 { - parent = allProjects[parent.ParentProjectID] + parent = allProjects[parent.parentID()] if parentsVisited[parent.ID] { return &ErrProjectCannotHaveACyclicRelationship{ @@ -1068,6 +1084,12 @@ func CreateProject(s *xorm.Session, project *Project, auth web.Auth, createBackl project.HexColor = utils.NormalizeHex(project.HexColor) + // Persist top-level projects with an explicit 0 (not NULL) so the stored + // value and the serialized parent_project_id stay a plain number. + if project.ParentProjectID == nil { + project.ParentProjectID = Ptr(int64(0)) + } + _, err = s.Insert(project) if err != nil { return @@ -1154,21 +1176,22 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje return } - // GHSA-2vq4-854f-5c72 / CVE-2026-35595: the recursive permission CTE - // cascades Admin from any owned ancestor, so moving a shared child - // under an attacker-owned root grants Admin on the child. Require - // Admin on both sides of a reparent. + // GHSA-2vq4-854f-5c72 / CVE-2026-35595 and GHSA-44v6-7fxq-vgf4 / + // CVE-2026-55064: the recursive permission CTE cascades Admin from any + // owned ancestor, so moving a shared child under an attacker-owned root + // grants Admin on the child, and detaching a child to the top level + // severs an owner's inherited-permission chain. Both are reparent + // operations that must require Admin on the moved project. // - // Only gate on non-zero ParentProjectID: the generic update handler - // binds a fresh struct, so an omitted parent_project_id is - // indistinguishable from an explicit 0. Detach-to-root is therefore - // out of scope here — a proper fix needs a pointer field. - if project.ParentProjectID > 0 { + // ParentProjectID is a *int64 so an omitted parent_project_id (nil) is + // distinguishable from an explicit 0 (detach-to-root). Only gate when + // the field was sent and actually changes the parent. + if project.ParentProjectID != nil { storedProject, err := GetProjectSimpleByID(s, project.ID) if err != nil { return err } - if project.ParentProjectID != storedProject.ParentProjectID { + if *project.ParentProjectID != storedProject.parentID() { canAdminMoved, err := project.IsAdmin(s, auth) if err != nil { return err @@ -1177,13 +1200,17 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje return ErrGenericForbidden{} } - newParent := &Project{ID: project.ParentProjectID} - canAdminNewParent, err := newParent.IsAdmin(s, auth) - if err != nil { - return err - } - if !canAdminNewParent { - return ErrGenericForbidden{} + // Attaching under a new parent additionally requires Admin on + // that parent; detaching to the top level (0) has no new parent. + if *project.ParentProjectID > 0 { + newParent := &Project{ID: *project.ParentProjectID} + canAdminNewParent, err := newParent.IsAdmin(s, auth) + if err != nil { + return err + } + if !canAdminNewParent { + return ErrGenericForbidden{} + } } } } @@ -1210,9 +1237,13 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje "is_archived", "identifier", "hex_color", - "parent_project_id", "position", } + // Only touch parent_project_id when it was actually sent, otherwise a + // partial update (nil) would silently detach the project to the top level. + if project.ParentProjectID != nil { + colsToUpdate = append(colsToUpdate, "parent_project_id") + } if project.Description != "" { colsToUpdate = append(colsToUpdate, "description") } @@ -1222,7 +1253,7 @@ func UpdateProject(s *xorm.Session, project *Project, auth web.Auth, updateProje } if project.Position < 0.1 { - err = recalculateProjectPositions(s, project.ParentProjectID) + err = recalculateProjectPositions(s, project.parentID()) if err != nil { return err } diff --git a/pkg/models/project_duplicate.go b/pkg/models/project_duplicate.go index f161132fc..c74a4928d 100644 --- a/pkg/models/project_duplicate.go +++ b/pkg/models/project_duplicate.go @@ -85,7 +85,7 @@ func (pd *ProjectDuplicate) Create(s *xorm.Session, doer web.Auth) (err error) { pd.Project.ID = 0 pd.Project.Identifier = "" // Reset the identifier to trigger regenerating a new one - pd.Project.ParentProjectID = pd.ParentProjectID + pd.Project.ParentProjectID = &pd.ParentProjectID // Set the owner to the current user pd.Project.OwnerID = doer.GetID() pd.Project.Title += " - duplicate" diff --git a/pkg/models/project_permissions.go b/pkg/models/project_permissions.go index 4e9e8558c..7ae22257b 100644 --- a/pkg/models/project_permissions.go +++ b/pkg/models/project_permissions.go @@ -159,8 +159,10 @@ func (p *Project) CanUpdate(s *xorm.Session, a web.Auth) (canUpdate bool, err er // permission-check-only callers (buckets, webhooks, task ops) that // pass stub &Project{ID: ...} values with ParentProjectID=0 and never // commit a reparent, which would spuriously trip the gate. - if p.ParentProjectID != 0 && p.ParentProjectID != ol.ParentProjectID { - newProject := &Project{ID: p.ParentProjectID} + // Only a real new parent (> 0) needs a write check here; detach-to-root + // (explicit 0) is gated for Admin in UpdateProject instead. + if p.ParentProjectID != nil && *p.ParentProjectID > 0 && *p.ParentProjectID != ol.parentID() { + newProject := &Project{ID: *p.ParentProjectID} can, err := newProject.CanWrite(s, a) if err != nil { return false, err @@ -193,8 +195,8 @@ func (p *Project) CanCreate(s *xorm.Session, a web.Auth) (bool, error) { if isInstanceAdmin(s, a) { return true, nil } - if p.ParentProjectID != 0 { - parent := &Project{ID: p.ParentProjectID} + if pid := p.parentID(); pid > 0 { + parent := &Project{ID: pid} return parent.CanWrite(s, a) } // Check if we're dealing with a share auth diff --git a/pkg/models/project_repair.go b/pkg/models/project_repair.go index 49e641755..324cc4e4d 100644 --- a/pkg/models/project_repair.go +++ b/pkg/models/project_repair.go @@ -49,17 +49,17 @@ func RepairOrphanedProjects(s *xorm.Session, dryRun bool) (*RepairOrphanedProjec if dryRun { for _, p := range orphans { log.Infof("[dry-run] Would re-parent project %d (%s) from non-existent parent %d to top level", - p.ID, p.Title, p.ParentProjectID) + p.ID, p.Title, p.parentID()) } return result, nil } for _, p := range orphans { log.Infof("Re-parenting project %d (%s) from non-existent parent %d to top level", - p.ID, p.Title, p.ParentProjectID) + p.ID, p.Title, p.parentID()) _, err = s.Where("id = ?", p.ID). Cols("parent_project_id"). - Update(&Project{ParentProjectID: 0}) + Update(&Project{ParentProjectID: Ptr(int64(0))}) if err != nil { return result, err } diff --git a/pkg/models/project_repair_test.go b/pkg/models/project_repair_test.go index 65f2385fb..0cac7f084 100644 --- a/pkg/models/project_repair_test.go +++ b/pkg/models/project_repair_test.go @@ -41,7 +41,7 @@ func TestRepairOrphanedProjects(t *testing.T) { has, err := s.Get(project) require.NoError(t, err) assert.True(t, has) - assert.Equal(t, int64(0), project.ParentProjectID) + assert.Equal(t, int64(0), project.parentID()) }) t.Run("dry run does not modify anything", func(t *testing.T) { @@ -60,7 +60,7 @@ func TestRepairOrphanedProjects(t *testing.T) { has, err := s.Get(project) require.NoError(t, err) assert.True(t, has) - assert.Equal(t, int64(999999), project.ParentProjectID) + assert.Equal(t, int64(999999), project.parentID()) }) t.Run("no orphans returns zero counts", func(t *testing.T) { diff --git a/pkg/models/project_test.go b/pkg/models/project_test.go index b0abc17ad..cbe1565fd 100644 --- a/pkg/models/project_test.go +++ b/pkg/models/project_test.go @@ -79,6 +79,19 @@ func TestProject_CreateOrUpdate(t *testing.T) { "project_view_id": kanbanView.ID, }, false) }) + t.Run("pseudo parent project id is rejected", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + project := Project{ + Title: "pseudo parent create", + ParentProjectID: Ptr(int64(-1)), + } + err := project.Create(s, usr) + require.Error(t, err) + assert.True(t, IsErrProjectCannotBelongToAPseudoParentProject(err)) + db.AssertMissing(t, "projects", map[string]interface{}{"title": "pseudo parent create"}) + }) t.Run("bot project owned by bot owner", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() @@ -148,7 +161,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { project := Project{ Title: "test", Description: "Lorem Ipsum", - ParentProjectID: 999999, + ParentProjectID: Ptr(int64(999999)), } err := project.Create(s, usr) require.Error(t, err) @@ -261,7 +274,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { ID: 6, Title: "Test6", Description: "Lorem Ipsum", - ParentProjectID: 7, // from 6 + ParentProjectID: Ptr(int64(7)), // from 6 } can, err := project.CanUpdate(s, usr) require.NoError(t, err) @@ -274,7 +287,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { "id": project.ID, "title": project.Title, "description": project.Description, - "parent_project_id": project.ParentProjectID, + "parent_project_id": project.parentID(), }, false) }) t.Run("others", func(t *testing.T) { @@ -285,7 +298,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { ID: 1, Title: "Test1", Description: "Lorem Ipsum", - ParentProjectID: 2, // from 1 + ParentProjectID: Ptr(int64(2)), // from 1 } can, _ := project.CanUpdate(s, usr) assert.False(t, can) // project is not writeable by us @@ -304,7 +317,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { ID: 6, Title: "Test6", Description: "Lorem Ipsum", - ParentProjectID: -1, + ParentProjectID: Ptr(int64(-1)), } err := project.Update(s, usr) require.Error(t, err) @@ -322,7 +335,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { project := Project{ ID: 10, Title: "Test10", - ParentProjectID: 1, // attacker-owned root + ParentProjectID: Ptr(int64(1)), // attacker-owned root } err := project.Update(s, usr) require.Error(t, err) @@ -338,7 +351,7 @@ func TestProject_CreateOrUpdate(t *testing.T) { project := Project{ ID: 43, Title: "Reparent Escalation Test Child", - ParentProjectID: 1, + ParentProjectID: Ptr(int64(1)), } err := project.Update(s, usr) require.Error(t, err) @@ -347,22 +360,50 @@ func TestProject_CreateOrUpdate(t *testing.T) { t.Run("non-reparent update with Write still permitted (regression)", func(t *testing.T) { // User 1 has Write (not Admin) on project 43 via project 10; // a rename with parent unchanged must not trip the Admin gate. - // - // ParentProjectID is set to the stored value (10), not 0: the - // gate only fires on non-zero ParentProjectID because the - // generic handler can't distinguish omitted from explicit-zero - // (detach-to-root is a follow-up, needs a pointer field). db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() project := Project{ ID: 43, Title: "Reparent Escalation Test Child renamed", - ParentProjectID: 10, // unchanged — no reparent intent + ParentProjectID: Ptr(int64(10)), // unchanged — no reparent intent } err := project.Update(s, usr) require.NoError(t, err) }) + t.Run("partial update with omitted parent keeps the parent (regression)", func(t *testing.T) { + // A partial update (ParentProjectID nil) must not silently + // detach project 43 from its parent 10. + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + project := Project{ + ID: 43, + Title: "Reparent Escalation Test Child renamed again", + } + err := project.Update(s, usr) + require.NoError(t, err) + stored, err := GetProjectSimpleByID(s, 43) + require.NoError(t, err) + assert.Equal(t, int64(10), stored.parentID()) + }) + t.Run("attacker with Write cannot detach a child to root via parent_project_id=0 (GHSA-44v6-7fxq-vgf4)", func(t *testing.T) { + // User 1 has Write (not Admin) on project 43 via project 10. + // Sending an explicit parent_project_id=0 detaches the child to + // the top level, which severs the inherited-permission chain and + // must require Admin on the moved project. + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + project := Project{ + ID: 43, + Title: "Reparent Escalation Test Child", + ParentProjectID: Ptr(int64(0)), // explicit detach-to-root + } + err := project.Update(s, usr) + require.Error(t, err) + assert.True(t, IsErrGenericForbidden(err)) + }) }) t.Run("archive default project of the same user", func(t *testing.T) { db.LoadAndAssertFixtures(t) @@ -740,7 +781,7 @@ func TestCheckIsArchived(t *testing.T) { s := db.NewSession() defer s.Close() - p := &Project{ID: 40, ParentProjectID: 3} + p := &Project{ID: 40, ParentProjectID: Ptr(int64(3))} err := p.CheckIsArchived(s) require.Error(t, err) assert.True(t, IsErrProjectIsArchived(err)) @@ -762,7 +803,7 @@ func TestCheckIsArchived(t *testing.T) { s := db.NewSession() defer s.Close() - p := &Project{ID: 21, ParentProjectID: 22} + p := &Project{ID: 21, ParentProjectID: Ptr(int64(22))} err := p.CheckIsArchived(s) require.Error(t, err) assert.True(t, IsErrProjectIsArchived(err)) diff --git a/pkg/models/user_delete.go b/pkg/models/user_delete.go index 59bd33107..345db443d 100644 --- a/pkg/models/user_delete.go +++ b/pkg/models/user_delete.go @@ -145,7 +145,7 @@ func DeleteUser(s *xorm.Session, u *user.User) (err error) { } for _, p := range projectsToDelete { - if p.ParentProjectID != 0 { + if p.parentID() != 0 { // Child projects are deleted by p.Delete continue } diff --git a/pkg/models/user_project.go b/pkg/models/user_project.go index 7fe98c772..166cde260 100644 --- a/pkg/models/user_project.go +++ b/pkg/models/user_project.go @@ -93,11 +93,11 @@ func ListUsersFromProject(s *xorm.Session, l *Project, currentUser *user.User, s } userids = append(userids, currentUserIDs...) - if currentProject.ParentProjectID == 0 { + if currentProject.parentID() == 0 { break } - parent, err := GetProjectSimpleByID(s, currentProject.ParentProjectID) + parent, err := GetProjectSimpleByID(s, currentProject.parentID()) if err != nil && !IsErrProjectDoesNotExist(err) { return nil, err } diff --git a/pkg/modules/migration/create_from_structure.go b/pkg/modules/migration/create_from_structure.go index 318d3ee4c..de76f3596 100644 --- a/pkg/modules/migration/create_from_structure.go +++ b/pkg/modules/migration/create_from_structure.go @@ -82,9 +82,9 @@ func insertFromStructure(s *xorm.Session, str []*models.ProjectWithTasksAndBucke oldID := p.ID - if p.ParentProjectID != 0 { - childRelations[p.ParentProjectID] = append(childRelations[p.ParentProjectID], oldID) - p.ParentProjectID = 0 + if p.ParentProjectID != nil && *p.ParentProjectID != 0 { + childRelations[*p.ParentProjectID] = append(childRelations[*p.ParentProjectID], oldID) + p.ParentProjectID = nil } p.ID = 0 @@ -114,7 +114,7 @@ func insertFromStructure(s *xorm.Session, str []*models.ProjectWithTasksAndBucke continue } - child.ParentProjectID = parent.ID + child.ParentProjectID = models.Ptr(parent.ID) err = child.Update(s, user) if err != nil { return err diff --git a/pkg/modules/migration/create_from_structure_test.go b/pkg/modules/migration/create_from_structure_test.go index c449db145..8da335665 100644 --- a/pkg/modules/migration/create_from_structure_test.go +++ b/pkg/modules/migration/create_from_structure_test.go @@ -53,7 +53,7 @@ func TestInsertFromStructure(t *testing.T) { Project: models.Project{ Title: "Testproject1", Description: "Something", - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), }, Buckets: []*models.Bucket{ { diff --git a/pkg/modules/migration/csv/csv.go b/pkg/modules/migration/csv/csv.go index 6d9ded38d..d7242ae9c 100644 --- a/pkg/modules/migration/csv/csv.go +++ b/pkg/modules/migration/csv/csv.go @@ -664,7 +664,7 @@ func convertToVikunja(rows [][]string, config *ImportConfig) []*models.ProjectWi projects[projectName] = &models.ProjectWithTasksAndBuckets{ Project: models.Project{ ID: int64(len(projects)+2) + pseudoParentID, - ParentProjectID: pseudoParentID, + ParentProjectID: &pseudoParentID, Title: projectName, }, } diff --git a/pkg/modules/migration/microsoft-todo/microsoft_todo.go b/pkg/modules/migration/microsoft-todo/microsoft_todo.go index d0f9b98e0..37b0f69a0 100644 --- a/pkg/modules/migration/microsoft-todo/microsoft_todo.go +++ b/pkg/modules/migration/microsoft-todo/microsoft_todo.go @@ -285,7 +285,7 @@ func convertMicrosoftTodoData(todoData []*project) (vikunjsStructure []*models.P Project: models.Project{ Title: l.DisplayName, ID: int64(index+1) + pseudoParentID, - ParentProjectID: pseudoParentID, + ParentProjectID: &pseudoParentID, }, } diff --git a/pkg/modules/migration/microsoft-todo/microsoft_todo_test.go b/pkg/modules/migration/microsoft-todo/microsoft_todo_test.go index a64574f0e..af1a7df45 100644 --- a/pkg/modules/migration/microsoft-todo/microsoft_todo_test.go +++ b/pkg/modules/migration/microsoft-todo/microsoft_todo_test.go @@ -114,7 +114,7 @@ func TestConverting(t *testing.T) { { Project: models.Project{ ID: 2, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "Project 1", }, Tasks: []*models.TaskWithComments{ @@ -172,7 +172,7 @@ func TestConverting(t *testing.T) { Project: models.Project{ Title: "Project 2", ID: 3, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), }, Tasks: []*models.TaskWithComments{ { diff --git a/pkg/modules/migration/ticktick/ticktick.go b/pkg/modules/migration/ticktick/ticktick.go index a7b3f84bc..99138fff0 100644 --- a/pkg/modules/migration/ticktick/ticktick.go +++ b/pkg/modules/migration/ticktick/ticktick.go @@ -199,7 +199,7 @@ func convertTickTickToVikunja(tasks []*tickTickTask) (result []*models.ProjectWi projects[t.ProjectName] = &models.ProjectWithTasksAndBuckets{ Project: models.Project{ ID: int64(index+1) + pseudoParentID, - ParentProjectID: pseudoParentID, + ParentProjectID: &pseudoParentID, Title: t.ProjectName, }, } diff --git a/pkg/modules/migration/ticktick/ticktick_test.go b/pkg/modules/migration/ticktick/ticktick_test.go index 64c60689d..beea505a8 100644 --- a/pkg/modules/migration/ticktick/ticktick_test.go +++ b/pkg/modules/migration/ticktick/ticktick_test.go @@ -104,8 +104,8 @@ func TestConvertTicktickTasksToVikunja(t *testing.T) { assert.Len(t, vikunjaTasks, 3) - assert.Equal(t, vikunjaTasks[1].ParentProjectID, vikunjaTasks[0].ID) - assert.Equal(t, vikunjaTasks[2].ParentProjectID, vikunjaTasks[0].ID) + assert.Equal(t, vikunjaTasks[0].ID, *vikunjaTasks[1].ParentProjectID) + assert.Equal(t, vikunjaTasks[0].ID, *vikunjaTasks[2].ParentProjectID) assert.Len(t, vikunjaTasks[1].Tasks, 4) assert.Equal(t, vikunjaTasks[1].Title, tickTickTasks[0].ProjectName) diff --git a/pkg/modules/migration/todoist/todoist.go b/pkg/modules/migration/todoist/todoist.go index 4e0884de0..dc5517cae 100644 --- a/pkg/modules/migration/todoist/todoist.go +++ b/pkg/modules/migration/todoist/todoist.go @@ -348,7 +348,7 @@ func convertTodoistToVikunja(sync *sync, doneItems map[string]*doneItem) (fullVi project := &models.ProjectWithTasksAndBuckets{ Project: models.Project{ ID: int64(index+1) + pseudoParentID, - ParentProjectID: pseudoParentID, + ParentProjectID: &pseudoParentID, Title: p.Name, HexColor: todoistColors[p.Color], IsArchived: p.IsArchived, diff --git a/pkg/modules/migration/todoist/todoist_test.go b/pkg/modules/migration/todoist/todoist_test.go index c1c7322c1..59d4a3511 100644 --- a/pkg/modules/migration/todoist/todoist_test.go +++ b/pkg/modules/migration/todoist/todoist_test.go @@ -388,7 +388,7 @@ func TestConvertTodoistToVikunja(t *testing.T) { { Project: models.Project{ ID: 2, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "Project1", Description: "Lorem Ipsum dolor sit amet\nLorem Ipsum dolor sit amet 2\nLorem Ipsum dolor sit amet 3", HexColor: todoistColors["berry_red"], @@ -524,7 +524,7 @@ func TestConvertTodoistToVikunja(t *testing.T) { { Project: models.Project{ ID: 3, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "Project2", Description: "Lorem Ipsum dolor sit amet 4\nLorem Ipsum dolor sit amet 5", HexColor: todoistColors["mint_green"], @@ -625,7 +625,7 @@ func TestConvertTodoistToVikunja(t *testing.T) { { Project: models.Project{ ID: 4, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "Project3 - Archived", HexColor: todoistColors["mint_green"], IsArchived: true, diff --git a/pkg/modules/migration/trello/trello.go b/pkg/modules/migration/trello/trello.go index 2e2d5a8c1..273bda555 100644 --- a/pkg/modules/migration/trello/trello.go +++ b/pkg/modules/migration/trello/trello.go @@ -249,7 +249,7 @@ func convertTrelloDataToVikunja(organizationName string, trelloData []*trello.Bo project := &models.ProjectWithTasksAndBuckets{ Project: models.Project{ ID: int64(index+1) + pseudoParentID, - ParentProjectID: pseudoParentID, + ParentProjectID: &pseudoParentID, Title: board.Name, Description: board.Desc, IsArchived: board.Closed, diff --git a/pkg/modules/migration/trello/trello_test.go b/pkg/modules/migration/trello/trello_test.go index 7f64724b9..e20c10826 100644 --- a/pkg/modules/migration/trello/trello_test.go +++ b/pkg/modules/migration/trello/trello_test.go @@ -269,7 +269,7 @@ func TestConvertTrelloToVikunja(t *testing.T) { { Project: models.Project{ ID: 2, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "TestBoard", Description: "This is a description", BackgroundInformation: bytes.NewBuffer(exampleFile), @@ -395,7 +395,7 @@ func TestConvertTrelloToVikunja(t *testing.T) { { Project: models.Project{ ID: 3, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "TestBoard Archived", IsArchived: true, }, @@ -425,7 +425,7 @@ func TestConvertTrelloToVikunja(t *testing.T) { { Project: models.Project{ ID: 2, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "TestBoard 2", }, Buckets: []*models.Bucket{ @@ -454,7 +454,7 @@ func TestConvertTrelloToVikunja(t *testing.T) { { Project: models.Project{ ID: 2, - ParentProjectID: 1, + ParentProjectID: models.Ptr(int64(1)), Title: "Personal Board", }, Buckets: []*models.Bucket{ diff --git a/pkg/webtests/project_test.go b/pkg/webtests/project_test.go index dca9bf81e..4ae02bc41 100644 --- a/pkg/webtests/project_test.go +++ b/pkg/webtests/project_test.go @@ -226,6 +226,14 @@ func TestProject(t *testing.T) { require.Error(t, err) assert.Contains(t, err.(models.ValidationHTTPError).InvalidFields[0], "does not validate as runelength(1|250)") }) + t.Run("Detach to root without admin is forbidden (GHSA-44v6-7fxq-vgf4)", func(t *testing.T) { + // User 1 has Write (not Admin) on project 10 and inherits Write on + // its child project 43. Sending an explicit parent_project_id=0 + // detaches the child to the top level, which must require Admin. + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"project": "43"}, `{"title":"Reparent Escalation Test Child","parent_project_id":0}`) + require.Error(t, err) + assertHandlerErrorCode(t, err, models.ErrorCodeGenericForbidden) + }) t.Run("Permissions check", func(t *testing.T) { t.Run("Forbidden", func(t *testing.T) { // Owned by user13