SEC2-014: Deleting a shared contact deletes other members' reference-letter uploads #956

New issue

Closed

opened 2026-10-17 10:38:13 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-17 10:38:13 +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: High · Category: Authentication/Authorization · Reference: CWE-285 (Improper Authorization); CWE-639 (Authorization Bypass Through User-Controlled Key); OWASP A01:2021 (Broken Access Control)

Location:

src/postulo/jobs/views.py:1764-1821 (ContactDeleteView: get_context_data, and form_valid with the single letters assignment at 1796)
src/postulo/documents/models.py:896-922 (ReferenceLetter, an OwnedModel whose referee is a SET_NULL link to the contact)
src/postulo/core/gdpr.py:179-201 (references) and :473-510 (erase_contact, whose report counts every owner's rows)

Issue

Since #896 a contact may be owned by one person or group and shared with others. A
ReferenceLetter stays person-owned (OwnedModel) and points at the referee contact with
SET_NULL, so two members who both know a shared referee may each hold their own uploaded
letter from them. The delete view reads the letters through the contact's reverse relation
with no owner narrowing, both to count them and to delete them:

# get_context_data
number = self.object.reference_letters.count()
...
# form_valid
letters = self.object.reference_letters.select_related("upload")
delete_letters = bool(self.request.POST.get("delete_letters"))
...
if delete_letters:
    for letter in letters:
        letter.upload.delete()

Every other place that reads this relation narrows it (ContactUpdateView.get_context_data
filters reference_letters by owner=self.request.user, with the comment "The person's own
letters only (#896)"). The delete path was not updated with #896. access = MANAGE means the
owner (or a member of the owning group) reaches this view; the letters it deletes belong to
whoever the contact was shared with. KeepsOthersCopies and detach.DEPENDENTS run only after
the uploads are already gone, so nothing hands the grantee a copy.

With the data-protection module on, the same view goes through gdpr.erase_contact, whose
report (references(): ReferenceLetter.objects.filter(referee=contact), and the same for
applications, referrals, interviews, appointments and listing events) counts every owner's rows
and is shown to the deleter as the summary.

Impact

A person who owns a contact and shares it (or any member of a group that owns it) can, by
deleting the contact with "Delete the letters they wrote as well" ticked, delete the
UploadedDocument rows and files of every other member who recorded a letter from that
referee. The count in the dialog, and the erasure summary when the data-protection module is
on, also tell the owner how many such rows other people hold. This breaks the threat-model
promise that documents are "theirs whatever is shared" and is a cross-account deletion of
another member's files.

Fix

Narrow both reads to the requester's own rows, at the single assignment the two branches share:

letters = self.object.reference_letters.filter(owner=self.request.user).select_related("upload")
number = self.object.reference_letters.filter(owner=self.request.user).count()

and let detach.after_losing (already listing ReferenceLetter.referee) give other members
what they need. In gdpr.erase_contact, narrow the counts the deleter is shown to the
deleter's rows, or word the summary without numbers for rows that are not theirs. Done when a
test in tests/security/test_sharing_sweep.py, run with the data-protection module both on and
off, shares a contact read-only and read/write with another person who uploads a reference
letter from it, has the owner delete the contact with delete_letters=1, and asserts the
grantee's UploadedDocument still exists and the dialog and summary counted only the owner's
rows.

#896 (closed) - introduced shared contacts; its text does not mention letters. #666 and #696 (closed) - the delete-letters option and the referee rules, both written before sharing. SEC2-019 - the same view's data-protection branch also skips the copy other members are promised.

Found on 2026-10-16 in a read-only security review of `postulo` at `56fc9d531`, checked by a second, independent pass. **Severity:** High · **Category:** Authentication/Authorization · **Reference:** CWE-285 (Improper Authorization); CWE-639 (Authorization Bypass Through User-Controlled Key); OWASP A01:2021 (Broken Access Control) **Location:** - `src/postulo/jobs/views.py:1764-1821` (`ContactDeleteView`: `get_context_data`, and `form_valid` with the single `letters` assignment at 1796) - `src/postulo/documents/models.py:896-922` (`ReferenceLetter`, an `OwnedModel` whose `referee` is a `SET_NULL` link to the contact) - `src/postulo/core/gdpr.py:179-201` (`references`) and `:473-510` (`erase_contact`, whose report counts every owner's rows) ## Issue Since #896 a contact may be owned by one person or group and shared with others. A `ReferenceLetter` stays person-owned (`OwnedModel`) and points at the referee contact with `SET_NULL`, so two members who both know a shared referee may each hold their own uploaded letter from them. The delete view reads the letters through the contact's reverse relation with no owner narrowing, both to count them and to delete them: ```python # get_context_data number = self.object.reference_letters.count() ... # form_valid letters = self.object.reference_letters.select_related("upload") delete_letters = bool(self.request.POST.get("delete_letters")) ... if delete_letters: for letter in letters: letter.upload.delete() ``` Every other place that reads this relation narrows it (`ContactUpdateView.get_context_data` filters `reference_letters` by `owner=self.request.user`, with the comment "The person's own letters only (#896)"). The delete path was not updated with #896. `access = MANAGE` means the owner (or a member of the owning group) reaches this view; the letters it deletes belong to whoever the contact was shared with. `KeepsOthersCopies` and `detach.DEPENDENTS` run only after the uploads are already gone, so nothing hands the grantee a copy. With the data-protection module on, the same view goes through `gdpr.erase_contact`, whose report (`references()`: `ReferenceLetter.objects.filter(referee=contact)`, and the same for applications, referrals, interviews, appointments and listing events) counts every owner's rows and is shown to the deleter as the summary. ## Impact A person who owns a contact and shares it (or any member of a group that owns it) can, by deleting the contact with "Delete the letters they wrote as well" ticked, delete the `UploadedDocument` rows and files of every other member who recorded a letter from that referee. The count in the dialog, and the erasure summary when the data-protection module is on, also tell the owner how many such rows other people hold. This breaks the threat-model promise that documents are "theirs whatever is shared" and is a cross-account deletion of another member's files. ## Fix Narrow both reads to the requester's own rows, at the single assignment the two branches share: ```python letters = self.object.reference_letters.filter(owner=self.request.user).select_related("upload") number = self.object.reference_letters.filter(owner=self.request.user).count() ``` and let `detach.after_losing` (already listing `ReferenceLetter.referee`) give other members what they need. In `gdpr.erase_contact`, narrow the counts the deleter is shown to the deleter's rows, or word the summary without numbers for rows that are not theirs. Done when a test in `tests/security/test_sharing_sweep.py`, run with the data-protection module both on and off, shares a contact read-only and read/write with another person who uploads a reference letter from it, has the owner delete the contact with `delete_letters=1`, and asserts the grantee's `UploadedDocument` still exists and the dialog and summary counted only the owner's rows. ## Related #896 (closed) - introduced shared contacts; its text does not mention letters. #666 and #696 (closed) - the delete-letters option and the referee rules, both written before sharing. SEC2-019 - the same view's data-protection branch also skips the copy other members are promised.

tiagoagueda added the

bug

security

labels

2026-10-17 10:38:14 +00:00

tiagoagueda added this to the 0.5.0 milestone

2026-10-17 10:38:15 +00:00

tiagoagueda referenced this issue

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

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

tiagoagueda referenced this issue

2026-10-17 13:26:22 +00:00