SEC2-047: Removing the last members of a record-owning group is check-then-act without a lock, so concurrent removals can orphan its records #989

New issue

Closed

opened 2026-10-17 10:39:27 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-17 10:39:27 +00:00

Owner

Found on 2026-10-16 in a read-only security review of postulo at 56fc9d531, checked by a second, independent pass.

Severity: Informational · Category: Business Logic · Reference: CWE-367 (Time-of-check Time-of-use Race Condition); OWASP A04:2021 (Insecure Design)

Location:

src/postulo/core/groups.py:110-122 (_before_leaving)
src/postulo/core/groups.py:143-154 (membership_changed, pre_remove)
src/postulo/core/views_access.py:90-98 (members set inside transaction.atomic(), no lock)

Issue

def _before_leaving(pairs) -> None:
    for group, person in pairs:
        stays = group.user_set.exclude(pk=person.pk)
        ...
        if not stays.exists() and owns_anything(group):
            raise LastMemberOwns(...)

The module promises that "the last member of a group that still owns records cannot leave it". The check reads the membership table and the removal then writes it, inside the request transaction, with no select_for_update on the group or its memberships. On PostgreSQL two requests removing the two remaining members (two administrators on Server settings → Groups, or an administrator and the member leaving from Your groups) both see one member staying, both pass, and the group is left owning companies, contacts or listings with nobody in it. SQLite's single writer makes it moot there, as accounts/adapter.py:21-27 notes for the invitation lock.

Impact

Records owned by a group nobody belongs to are reachable only through the operator's conflicts page, and jobs/migrations/0036 and core/migrations/0037 refuse to reverse ("has no member to give them to") while such a group exists. Integrity rather than exposure, and it needs PostgreSQL and two simultaneous actions, hence Informational.

Fix

In membership_changed on pre_remove/pre_clear, lock each group in pairs before _before_leaving runs (Group.objects.select_for_update().get(pk=group.pk); both callers are inside an atomic block), the way accounts/adapter.py:33-36 locks the invitation, so the second removal waits for the first's commit and then sees no member staying. Django ignores select_for_update on SQLite, so no branch is needed there. Done when: a test in the PostgreSQL CI job removes the last two members from two connections and one of them is refused (a SQLite unit test cannot show it).

#896 (closed) - introduced the rule. #826 (closed) - fixed the same shape of race for the first-account-becomes-administrator check with select_for_update.

Found on 2026-10-16 in a read-only security review of `postulo` at `56fc9d531`, checked by a second, independent pass. **Severity:** Informational · **Category:** Business Logic · **Reference:** CWE-367 (Time-of-check Time-of-use Race Condition); OWASP A04:2021 (Insecure Design) **Location:** - `src/postulo/core/groups.py:110-122` (`_before_leaving`) - `src/postulo/core/groups.py:143-154` (`membership_changed`, `pre_remove`) - `src/postulo/core/views_access.py:90-98` (members set inside `transaction.atomic()`, no lock) ## Issue ```python def _before_leaving(pairs) -> None: for group, person in pairs: stays = group.user_set.exclude(pk=person.pk) ... if not stays.exists() and owns_anything(group): raise LastMemberOwns(...) ``` The module promises that "the last member of a group that still owns records cannot leave it". The check reads the membership table and the removal then writes it, inside the request transaction, with no `select_for_update` on the group or its memberships. On PostgreSQL two requests removing the two remaining members (two administrators on *Server settings → Groups*, or an administrator and the member leaving from *Your groups*) both see one member staying, both pass, and the group is left owning companies, contacts or listings with nobody in it. SQLite's single writer makes it moot there, as `accounts/adapter.py:21-27` notes for the invitation lock. ## Impact Records owned by a group nobody belongs to are reachable only through the operator's conflicts page, and `jobs/migrations/0036` and `core/migrations/0037` refuse to reverse ("has no member to give them to") while such a group exists. Integrity rather than exposure, and it needs PostgreSQL and two simultaneous actions, hence Informational. ## Fix In `membership_changed` on `pre_remove`/`pre_clear`, lock each group in `pairs` before `_before_leaving` runs (`Group.objects.select_for_update().get(pk=group.pk)`; both callers are inside an atomic block), the way `accounts/adapter.py:33-36` locks the invitation, so the second removal waits for the first's commit and then sees no member staying. Django ignores `select_for_update` on SQLite, so no branch is needed there. Done when: a test in the PostgreSQL CI job removes the last two members from two connections and one of them is refused (a SQLite unit test cannot show it). ## Related #896 (closed) - introduced the rule. #826 (closed) - fixed the same shape of race for the first-account-becomes-administrator check with `select_for_update`.

tiagoagueda added the

enhancement

security

labels

2026-10-17 10:39:27 +00:00

tiagoagueda added this to the 1.0.0 milestone

2026-10-17 10:39:28 +00:00

tiagoagueda referenced this issue

2026-10-17 10:41:23 +00:00

SEC2 index: security review of 2026-10-16 (86 issues across the core, plugins and extensions) #992

tiagoagueda referenced this issue from a commit

2026-10-17 15:33:37 +00:00