From 984ae973a1e5a4a1c8d00660652800d2b87331a3 Mon Sep 17 00:00:00 2001 From: sven-bitwarden Date: Tue, 24 Feb 2026 09:31:56 -0600 Subject: [PATCH] [PM-31004]: Fix Stackoverflow from Circular Group References (#991) * Fix circular groups * Simplify tests --- src/services/sync.service.spec.ts | 132 ++++++++ src/services/sync.service.ts | 17 +- .../directory-circular-groups.ldif | 308 ++++++++++++++++++ 3 files changed, 455 insertions(+), 2 deletions(-) create mode 100644 utils/openldap/example-ldifs/directory-circular-groups.ldif diff --git a/src/services/sync.service.spec.ts b/src/services/sync.service.spec.ts index 3cff089c..4e84afd7 100644 --- a/src/services/sync.service.spec.ts +++ b/src/services/sync.service.spec.ts @@ -6,6 +6,8 @@ import { MessagingService } from "@/jslib/common/src/abstractions/messaging.serv import { OrganizationImportRequest } from "@/jslib/common/src/models/request/organizationImportRequest"; import { ApiService } from "@/jslib/common/src/services/api.service"; +import { GroupEntry } from "@/src/models/groupEntry"; + import { getSyncConfiguration } from "../../utils/openldap/config-fixtures"; import { DirectoryFactoryService } from "../abstractions/directory-factory.service"; import { DirectoryType } from "../enums/directoryType"; @@ -134,4 +136,134 @@ describe("SyncService", () => { expect(apiService.postPublicImportDirectory).not.toHaveBeenCalled(); }); + + describe("nested and circular group handling", () => { + function createGroup( + name: string, + userExternalIds: string[] = [], + groupMemberReferenceIds: string[] = [], + ) { + return GroupEntry.fromJSON({ + name, + referenceId: name, + externalId: name, + userMemberExternalIds: userExternalIds, + groupMemberReferenceIds: groupMemberReferenceIds, + users: [], + }); + } + + function setupSyncWithGroups(groups: GroupEntry[]) { + const mockDirectoryService = mock(); + mockDirectoryService.getEntries.mockResolvedValue([groups, []]); + directoryFactory.createService.mockReturnValue(mockDirectoryService); + + stateService.getSync.mockResolvedValue(getSyncConfiguration({ groups: true, users: true })); + cryptoFunctionService.hash.mockResolvedValue(new ArrayBuffer(1)); + stateService.getLastSyncHash.mockResolvedValue("unique hash"); + singleRequestBuilder.buildRequest.mockReturnValue([ + { members: [], groups: [], overwriteExisting: true, largeImport: false }, + ]); + } + + it("should handle simple circular reference (A ↔ B) without stack overflow", async () => { + const groupA = createGroup("GroupA", ["userA"], ["GroupB"]); + const groupB = createGroup("GroupB", ["userB"], ["GroupA"]); + setupSyncWithGroups([groupA, groupB]); + + const [groups] = await syncService.sync(true, true); + + const [a, b] = groups; + expect(a.userMemberExternalIds).toEqual(new Set(["userA", "userB"])); + expect(b.userMemberExternalIds).toEqual(new Set(["userA", "userB"])); + }); + + it("should handle longer circular chain (A → B → C → A) without stack overflow", async () => { + const groupA = createGroup("GroupA", ["userA"], ["GroupB"]); + const groupB = createGroup("GroupB", ["userB"], ["GroupC"]); + const groupC = createGroup("GroupC", ["userC"], ["GroupA"]); + setupSyncWithGroups([groupA, groupB, groupC]); + + const [groups] = await syncService.sync(true, true); + + const allUsers = new Set(["userA", "userB", "userC"]); + for (const group of groups) { + expect(group.userMemberExternalIds).toEqual(allUsers); + } + }); + + it("should handle diamond structure (A → [B, C] → D)", async () => { + const groupA = createGroup("GroupA", ["userA"], ["GroupB", "GroupC"]); + const groupB = createGroup("GroupB", ["userB"], ["GroupD"]); + const groupC = createGroup("GroupC", ["userC"], ["GroupD"]); + const groupD = createGroup("GroupD", ["userD"], []); + setupSyncWithGroups([groupA, groupB, groupC, groupD]); + + const [groups] = await syncService.sync(true, true); + + const [a, b, c, d] = groups; + expect(a.userMemberExternalIds).toEqual(new Set(["userA", "userB", "userC", "userD"])); + expect(b.userMemberExternalIds).toEqual(new Set(["userB", "userD"])); + expect(c.userMemberExternalIds).toEqual(new Set(["userC", "userD"])); + expect(d.userMemberExternalIds).toEqual(new Set(["userD"])); + }); + + it("should handle deep nesting with circular reference at leaf", async () => { + // Structure: A → B → C → D → B (cycle back to B) + const groupA = createGroup("GroupA", ["userA"], ["GroupB"]); + const groupB = createGroup("GroupB", ["userB"], ["GroupC"]); + const groupC = createGroup("GroupC", ["userC"], ["GroupD"]); + const groupD = createGroup("GroupD", ["userD"], ["GroupB"]); + setupSyncWithGroups([groupA, groupB, groupC, groupD]); + + const [groups] = await syncService.sync(true, true); + + const [a, b, c, d] = groups; + const cycleUsers = new Set(["userB", "userC", "userD"]); + expect(a.userMemberExternalIds).toEqual(new Set(["userA", ...cycleUsers])); + expect(b.userMemberExternalIds).toEqual(cycleUsers); + expect(c.userMemberExternalIds).toEqual(cycleUsers); + expect(d.userMemberExternalIds).toEqual(cycleUsers); + }); + + it("should handle complex structure with multiple cycles and shared members", async () => { + // Structure: + // A → [B, C] + // B → [D, E] + // C → [E, F] + // D → A (cycle) + // E → C (cycle) + // F → (leaf) + const groupA = createGroup("GroupA", ["userA"], ["GroupB", "GroupC"]); + const groupB = createGroup("GroupB", ["userB"], ["GroupD", "GroupE"]); + const groupC = createGroup("GroupC", ["userC"], ["GroupE", "GroupF"]); + const groupD = createGroup("GroupD", ["userD"], ["GroupA"]); + const groupE = createGroup("GroupE", ["userE"], ["GroupC"]); + const groupF = createGroup("GroupF", ["userF"], []); + setupSyncWithGroups([groupA, groupB, groupC, groupD, groupE, groupF]); + + const [groups] = await syncService.sync(true, true); + + const allUsers = new Set(["userA", "userB", "userC", "userD", "userE", "userF"]); + const a = groups.find((g) => g.name === "GroupA"); + const b = groups.find((g) => g.name === "GroupB"); + const c = groups.find((g) => g.name === "GroupC"); + const d = groups.find((g) => g.name === "GroupD"); + const e = groups.find((g) => g.name === "GroupE"); + const f = groups.find((g) => g.name === "GroupF"); + + // A can reach all groups, so it gets all users + expect(a.userMemberExternalIds).toEqual(allUsers); + // B reaches D, E, and through cycles reaches everything + expect(b.userMemberExternalIds).toEqual(allUsers); + // C reaches E (which cycles back to C) and F + expect(c.userMemberExternalIds).toEqual(new Set(["userC", "userE", "userF"])); + // D cycles to A, which reaches everything + expect(d.userMemberExternalIds).toEqual(allUsers); + // E cycles to C, picking up C's descendants + expect(e.userMemberExternalIds).toEqual(new Set(["userC", "userE", "userF"])); + // F is a leaf + expect(f.userMemberExternalIds).toEqual(new Set(["userF"])); + }); + }); }); diff --git a/src/services/sync.service.ts b/src/services/sync.service.ts index c7c55d48..1b99a550 100644 --- a/src/services/sync.service.ts +++ b/src/services/sync.service.ts @@ -196,14 +196,27 @@ export class SyncService { return users == null ? null : users.filter((u) => u.email?.length <= 256); } - private flattenUsersToGroups(levelGroups: GroupEntry[], allGroups: GroupEntry[]): Set { + private flattenUsersToGroups( + levelGroups: GroupEntry[], + allGroups: GroupEntry[], + visitedGroups?: Set, + ): Set { let allUsers = new Set(); if (allGroups == null) { return allUsers; } + for (const group of levelGroups) { + const visited = visitedGroups ?? new Set(); + + if (visited.has(group.referenceId)) { + continue; + } + + visited.add(group.referenceId); + const childGroups = allGroups.filter((g) => group.groupMemberReferenceIds.has(g.referenceId)); - const childUsers = this.flattenUsersToGroups(childGroups, allGroups); + const childUsers = this.flattenUsersToGroups(childGroups, allGroups, visited); childUsers.forEach((id) => group.userMemberExternalIds.add(id)); allUsers = new Set([...allUsers, ...group.userMemberExternalIds]); } diff --git a/utils/openldap/example-ldifs/directory-circular-groups.ldif b/utils/openldap/example-ldifs/directory-circular-groups.ldif new file mode 100644 index 00000000..4580ca9b --- /dev/null +++ b/utils/openldap/example-ldifs/directory-circular-groups.ldif @@ -0,0 +1,308 @@ +version: 1 + +dn: dc=bitwarden,dc=com +dc: bitwarden +objectClass: dcObject +objectClass: organization +o: Bitwarden + +# Organizational Units +dn: ou=Human Resources,dc=bitwarden,dc=com +changetype: add +ou: Human Resources +objectClass: top +objectClass: organizationalUnit + +dn: ou=Engineering,dc=bitwarden,dc=com +changetype: add +ou: Engineering +objectClass: top +objectClass: organizationalUnit + +dn: ou=Marketing,dc=bitwarden,dc=com +changetype: add +ou: Marketing +objectClass: top +objectClass: organizationalUnit + +# Users - Human Resources +dn: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Roland Dyke +sn: Dyke +description: This is Roland Dyke's description +facsimileTelephoneNumber: +1 804 674-5794 +l: San Francisco +ou: Human Resources +postalAddress: Human Resources$San Francisco +telephoneNumber: +1 804 831-5121 +title: Supreme Human Resources Writer +userPassword: Password1 +uid: DykeR +givenName: Roland +mail: DykeR@220af87272f04218bb8dd81d50fb19f5.bitwarden.com +carLicense: 4CMGOJ +departmentNumber: 2838 +employeeType: Contract +homePhone: +1 804 936-4965 +initials: R. D. +mobile: +1 804 592-3734 +pager: +1 804 285-2962 +roomNumber: 9890 + +dn: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Teirtza Kara +sn: Kara +description: This is Teirtza Kara's description +facsimileTelephoneNumber: +1 206 759-2040 +l: San Francisco +ou: Human Resources +postalAddress: Human Resources$San Francisco +telephoneNumber: +1 206 562-1407 +title: Junior Human Resources President +userPassword: Password1 +uid: KaraT +givenName: Teirtza +mail: KaraT@c2afe8b3509f4a20b2b784841685bd74.bitwarden.com +carLicense: O9GAN2 +departmentNumber: 3880 +employeeType: Employee +homePhone: +1 206 154-4842 +initials: T. K. +mobile: +1 206 860-1835 +pager: +1 206 684-1438 +roomNumber: 9079 + +# Users - Engineering +dn: cn=Alice Chen,ou=Engineering,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Alice Chen +sn: Chen +description: Senior DevOps Engineer +l: Seattle +ou: Engineering +telephoneNumber: +1 206 555-0101 +title: Senior DevOps Engineer +userPassword: Password1 +uid: ChenA +givenName: Alice +mail: ChenA@bitwarden.com +employeeType: Employee + +dn: cn=Bob Martinez,ou=Engineering,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Bob Martinez +sn: Martinez +description: Platform Engineer +l: Austin +ou: Engineering +telephoneNumber: +1 512 555-0102 +title: Platform Engineer +userPassword: Password1 +uid: MartinezB +givenName: Bob +mail: MartinezB@bitwarden.com +employeeType: Employee + +dn: cn=Carol Williams,ou=Engineering,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Carol Williams +sn: Williams +description: QA Lead +l: Denver +ou: Engineering +telephoneNumber: +1 303 555-0103 +title: QA Lead +userPassword: Password1 +uid: WilliamsC +givenName: Carol +mail: WilliamsC@bitwarden.com +employeeType: Employee + +dn: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: David Kim +sn: Kim +description: QA Engineer +l: Portland +ou: Engineering +telephoneNumber: +1 503 555-0104 +title: QA Engineer +userPassword: Password1 +uid: KimD +givenName: David +mail: KimD@bitwarden.com +employeeType: Contractor + +# Users - Marketing +dn: cn=Eva Johnson,ou=Marketing,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Eva Johnson +sn: Johnson +description: Marketing Director +l: New York +ou: Marketing +telephoneNumber: +1 212 555-0105 +title: Marketing Director +userPassword: Password1 +uid: JohnsonE +givenName: Eva +mail: JohnsonE@bitwarden.com +employeeType: Employee + +dn: cn=Frank Lee,ou=Marketing,dc=bitwarden,dc=com +changetype: add +objectClass: top +objectClass: person +objectClass: organizationalPerson +objectClass: inetOrgPerson +cn: Frank Lee +sn: Lee +description: Content Strategist +l: Chicago +ou: Marketing +telephoneNumber: +1 312 555-0106 +title: Content Strategist +userPassword: Password1 +uid: LeeF +givenName: Frank +mail: LeeF@bitwarden.com +employeeType: Employee + +# ============================================================ +# GROUP HIERARCHY +# ============================================================ +# Structure (arrows show "contains" relationship): +# +# AllStaff +# ├── Engineering ◄────────────────┐ (CYCLE from Platform) +# │ ├── DevOps │ +# │ │ └── Platform ────────┘ +# │ └── QA +# ├── Marketing +# └── HR +# +# Contractors ─── DevOps (diamond: second path to Platform) +# +# TestNestA ◄──► TestNestB (simple bidirectional cycle) +# +# ============================================================ + +# Leaf group - Platform team (CYCLES BACK to Engineering) +dn: cn=Platform,dc=bitwarden,dc=com +changetype: add +cn: Platform +member: cn=Bob Martinez,ou=Engineering,dc=bitwarden,dc=com +member: cn=Engineering,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# DevOps group - contains Platform subgroup +dn: cn=DevOps,dc=bitwarden,dc=com +changetype: add +cn: DevOps +member: cn=Alice Chen,ou=Engineering,dc=bitwarden,dc=com +member: cn=Platform,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# QA group +dn: cn=QA,dc=bitwarden,dc=com +changetype: add +cn: QA +member: cn=Carol Williams,ou=Engineering,dc=bitwarden,dc=com +member: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# Engineering group - contains DevOps and QA subgroups +dn: cn=Engineering,dc=bitwarden,dc=com +changetype: add +cn: Engineering +member: cn=DevOps,dc=bitwarden,dc=com +member: cn=QA,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# Marketing group +dn: cn=Marketing,dc=bitwarden,dc=com +changetype: add +cn: Marketing +member: cn=Eva Johnson,ou=Marketing,dc=bitwarden,dc=com +member: cn=Frank Lee,ou=Marketing,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# HR group +dn: cn=HR,dc=bitwarden,dc=com +changetype: add +cn: HR +member: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com +member: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# AllStaff - top-level group containing all departments +dn: cn=AllStaff,dc=bitwarden,dc=com +changetype: add +cn: AllStaff +member: cn=Engineering,dc=bitwarden,dc=com +member: cn=Marketing,dc=bitwarden,dc=com +member: cn=HR,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# Contractors group - creates diamond pattern (second path to Platform via DevOps) +dn: cn=Contractors,dc=bitwarden,dc=com +changetype: add +cn: Contractors +member: cn=DevOps,dc=bitwarden,dc=com +member: cn=David Kim,ou=Engineering,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +# Simple bidirectional cycle test groups (preserved from original) +dn: cn=TestNestA,dc=bitwarden,dc=com +changetype: add +cn: TestNestA +member: cn=TestNestB,dc=bitwarden,dc=com +member: cn=Roland Dyke,ou=Human Resources,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top + +dn: cn=TestNestB,dc=bitwarden,dc=com +changetype: add +cn: TestNestB +member: cn=TestNestA,dc=bitwarden,dc=com +member: cn=Teirtza Kara,ou=Human Resources,dc=bitwarden,dc=com +objectclass: groupOfNames +objectclass: top