SEC2-004: Giving a record to a group fails with an unhandled exception when what it points at points at something the group cannot see #946

New issue

Closed

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

tiagoagueda commented

2026-10-17 10:37:48 +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-754 (Improper Check for Unusual or Exceptional Conditions); OWASP A04:2021 (Insecure Design)

Location:

src/postulo/jobs/ownership.py:77-95 (move)
src/postulo/jobs/sharing.py:127-150 (share)
src/postulo/jobs/share_views.py:205-209 (MoveView.post)

Issue

references() walks transitively, so for a listing whose company has a parent the plan is also = [company, parent]. move then shares each one on its own, without also_share:

        for target in plan.also:
            sharing.share(target, principal, Share.Scope.READ, actor)

share(company, group, READ, actor) re-plans for the company, finds the parent not yet seen by the group, and since also_share defaults to False:

    if plan.also and not also_share:
        raise ShareRefused(_("Share what it points at too, or nothing is shared: %(names)s") ...)

MoveView.post catches only ownership.MoveRefused, so the ShareRefused escapes as a 500. The transaction rolls back, so nothing is shared or moved; the giver sees the error page after having confirmed the move. The same page already computed and showed the full plan.also, so the person agreed to all of it.

Impact

No security consequence: nothing is moved or shared. A member who tries to give a listing (or a contact at a subsidiary) to a group hits a server error and cannot complete the action through the interface. Recorded because an unhandled error on a confirmed state change tends to be retried in ways that bypass the confirmation.

Fix

In ownership.move, pass the flag through at ownership.py:95: sharing.share(target, principal, Share.Scope.READ, actor, also_share=also_share). Sharing a target's references is idempotent through _grant, and they are already in plan.also. (Sharing plan.also in reverse order would also work for a chain but not for a diamond.) Catch sharing.ShareRefused beside MoveRefused in MoveView.post regardless. Done when a test moves a listing whose company has a parent into a group neither is visible to, with confirmed=1, and gets a redirect with the record owned by the group and both companies shared read-only.

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-754 (Improper Check for Unusual or Exceptional Conditions); OWASP A04:2021 (Insecure Design) **Location:** - `src/postulo/jobs/ownership.py:77-95` (`move`) - `src/postulo/jobs/sharing.py:127-150` (`share`) - `src/postulo/jobs/share_views.py:205-209` (`MoveView.post`) ## Issue `references()` walks transitively, so for a listing whose company has a parent the plan is `also = [company, parent]`. `move` then shares each one on its own, without `also_share`: ```python for target in plan.also: sharing.share(target, principal, Share.Scope.READ, actor) ``` `share(company, group, READ, actor)` re-plans for the company, finds the parent not yet seen by the group, and since `also_share` defaults to `False`: ```python if plan.also and not also_share: raise ShareRefused(_("Share what it points at too, or nothing is shared: %(names)s") ...) ``` `MoveView.post` catches only `ownership.MoveRefused`, so the `ShareRefused` escapes as a 500. The transaction rolls back, so nothing is shared or moved; the giver sees the error page after having confirmed the move. The same page already computed and showed the full `plan.also`, so the person agreed to all of it. ## Impact No security consequence: nothing is moved or shared. A member who tries to give a listing (or a contact at a subsidiary) to a group hits a server error and cannot complete the action through the interface. Recorded because an unhandled error on a confirmed state change tends to be retried in ways that bypass the confirmation. ## Fix In `ownership.move`, pass the flag through at `ownership.py:95`: `sharing.share(target, principal, Share.Scope.READ, actor, also_share=also_share)`. Sharing a target's references is idempotent through `_grant`, and they are already in `plan.also`. (Sharing `plan.also` in reverse order would also work for a chain but not for a diamond.) Catch `sharing.ShareRefused` beside `MoveRefused` in `MoveView.post` regardless. Done when a test moves a listing whose company has a parent into a group neither is visible to, with `confirmed=1`, and gets a redirect with the record owned by the group and both companies shared read-only.

tiagoagueda added the

enhancement

security

labels

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

tiagoagueda added this to the 1.0.0 milestone

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