SEC2-006: Check-then-create without a lock on a grant and on a group name #948

New issue

Closed

opened 2026-10-17 10:37:53 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-17 10:37:53 +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-362 (Race Condition); OWASP A04:2021 (Insecure Design)

Location:

src/postulo/jobs/sharing.py:164-174 (_grant)
src/postulo/core/views_access.py:60-71 (GroupsView.post, create, and the group pk parse)

Issue

    found = Share.objects.filter(**lookup).first()
    if found is None:
        return Share.objects.create(**lookup, scope=scope, granted_by=actor)

Two concurrent shares of the same record to the same grantee (two tabs, or a double submit) both see None and the second hits one_share_per_<record>_per_<grantee>; inside @transaction.atomic that is an IntegrityError and a 500. Likewise Group.objects.filter(name__iexact=name).exists() then Group.objects.create(name=name) races into the unique name column. Finally get_object_or_404(Group, pk=request.POST.get("group", "0") or 0) is handed the raw POST value; a non-numeric string raises ValueError from the field, which is a 500 rather than a 404 (staff only). No information is disclosed by the error page; the answer is simply the wrong status.

Impact

Staff-only or same-account effects; no cross-account consequence. Recorded for completeness beside #544 and #738, which fixed the same pattern where it mattered.

Fix

Use Share.objects.get_or_create(**lookup, defaults={"scope": scope, "granted_by": actor}) in _grant and widen the scope afterwards, which keeps the widening rule intact. Wrap Group.objects.create in try/except IntegrityError inside its own transaction.atomic() savepoint: on PostgreSQL an IntegrityError inside the outer atomic block poisons the transaction otherwise. Parse the pk with int() in a try, or pk.isdigit() as _add already does. Done when a test creates the same share twice and gets one row with the wider scope, and a test posts a non-numeric group value and gets a 404.

#544 (closed) and #738 (closed) - the same pattern on invitations and first-account creation; these two spots are new with #896/#886. #767 (closed) - the same check on other endpoints.

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-362 (Race Condition); OWASP A04:2021 (Insecure Design) **Location:** - `src/postulo/jobs/sharing.py:164-174` (`_grant`) - `src/postulo/core/views_access.py:60-71` (`GroupsView.post`, `create`, and the `group` pk parse) ## Issue ```python found = Share.objects.filter(**lookup).first() if found is None: return Share.objects.create(**lookup, scope=scope, granted_by=actor) ``` Two concurrent shares of the same record to the same grantee (two tabs, or a double submit) both see `None` and the second hits `one_share_per_<record>_per_<grantee>`; inside `@transaction.atomic` that is an `IntegrityError` and a 500. Likewise `Group.objects.filter(name__iexact=name).exists()` then `Group.objects.create(name=name)` races into the unique `name` column. Finally `get_object_or_404(Group, pk=request.POST.get("group", "0") or 0)` is handed the raw POST value; a non-numeric string raises `ValueError` from the field, which is a 500 rather than a 404 (staff only). No information is disclosed by the error page; the answer is simply the wrong status. ## Impact Staff-only or same-account effects; no cross-account consequence. Recorded for completeness beside #544 and #738, which fixed the same pattern where it mattered. ## Fix Use `Share.objects.get_or_create(**lookup, defaults={"scope": scope, "granted_by": actor})` in `_grant` and widen the scope afterwards, which keeps the widening rule intact. Wrap `Group.objects.create` in `try/except IntegrityError` inside its own `transaction.atomic()` savepoint: on PostgreSQL an `IntegrityError` inside the outer atomic block poisons the transaction otherwise. Parse the pk with `int()` in a `try`, or `pk.isdigit()` as `_add` already does. Done when a test creates the same share twice and gets one row with the wider scope, and a test posts a non-numeric `group` value and gets a 404. ## Related #544 (closed) and #738 (closed) - the same pattern on invitations and first-account creation; these two spots are new with #896/#886. #767 (closed) - the same check on other endpoints.

tiagoagueda added the

enhancement

security

labels

2026-10-17 10:37:53 +00:00

tiagoagueda added this to the 1.0.0 milestone

2026-10-17 10:37:54 +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