Isolation sweep: staff-only routes, ids outside the address and the API download of a sent document are never tried #422

New issue

Closed

opened 2026-10-01 10:46:35 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-01 10:46:35 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, following each EXCUSED entry in tests/security/test_isolation_sweep.py to the test it names, checking what each factory makes, and looking for a cross-account test for every id a request carries outside its address.

Checked again on 2026-10-01 against main at 0a5cad587: still present. The code is now at core/server_views.py:212 (PersonAdminView), :235 (PersonActiveView), :259 (PersonDeleteView) and :384 (PersonUsernameView); jobs/forms.py:86-87 (OwnerScopedModelForm.scope_querysets); tests/test_server_settings.py:47-51 (test_every_section_is_for_administrators_only) and :303 (the member's GET of server:person_username). The sweep itself, the API routers, accounts/views.py:284 and the other tests cited have not moved.

Related to #232 (closed), which asked for a sweep of "every view and queryset" and delivered the sweep of ids in a URL, with its EXCUSED list. This issue adds what that sweep excuses without proof or never tries. The code is correct today in every case below: each view has its staff gate and each lookup is narrowed. What is missing is a test that would fail if one of them stopped being so, on the boundary the threat model ranks first.

1. Staff-only routes are excused by citing tests that never ask them as a member

Where: tests/security/test_isolation_sweep.py:40-49 (EXCUSED, seven entries whose reason is "the gate is" in another file); tests/test_invites.py:217-226 (test_invitation_management_requires_staff, parametrised over invite_list and invite_create only, GET only); tests/test_server_settings.py:45-49 (test_every_section_is_for_administrators_only, over SECTION_URLS, none of the person_* routes); tests/test_server_settings.py:255 and tests/test_account_deletion.py:251-254 (a member's GET only); the views at core/server_views.py:211 (PersonAdminView), :234 (PersonActiveView), :258 (PersonDeleteView), :383 (PersonUsernameView) and accounts/views.py:284 (InviteRevokeView).
What happens: nothing asks server:person_admin or server:person_active as an ordinary member, by either method. accounts:invite_revoke is only ever posted to by staff. server:person_delete and server:person_username are asked as a member only with GET. If a refactor swapped StaffRequiredMixin on one of these views for LoginRequiredMixin, a signed-in member could reach the POST that grants administrator rights, suspends or deletes an account, renames a person or withdraws an invitation, and the whole suite would stay green. Members are tested on person_recovery (tests/test_recovery_links.py:235-240) and person_plugins (tests/test_plugin_policy.py:272-280), but not in the file EXCUSED names.
Why it matters: a staff check removed from one of these is a privilege escalation that no test would catch. The sweep's docstring says that adding a page "without deciding fails here"; an excuse that points at a test which does not exist defeats that.
Fix: add tests/security/test_staff_only.py. Walk every server: route and every accounts:invite_* route except invite_accept, filling an argument with another account's pk or a real invitation's pk. Sign in an ordinary member and send GET and an empty POST to each; assert 403 (302 for an anonymous visitor). Assert that is_staff, is_active, username and the Invite rows did not change. Point the seven EXCUSED reasons at that file.

2. Ids that arrive in an API body, a form choice or a formset row are never tried as another account's

Where: tests/security/test_isolation_sweep.py:415-433 (patterns_with_arguments, which finds only ids in an address); api/routers/interviews.py:97-106 (_contacts), :109-113 (add_interview), :151-152 (change_interview); api/routers/reminders.py:38-44 (add_reminder), :72-82 (change_reminder); jobs/forms.py:85-86 (OwnerScopedModelForm.scope_querysets, a no-op marked pragma: no cover); tests/test_api.py:337-345 ("application_id": 999); tests/test_interviews.py:724-741 (the "stranger" contact is made with owner=user); docs/THREAT-MODEL.md:34-35.
What happens: application_id and contact_ids on the interview API and application_id on the reminder API have no cross-account test anywhere. The nearest tests use an id that does not exist, or the same person's contact at another company. If a later change looked the application up with Application.objects instead of owned(request, ...), a token holder could attach interviews or reminders to another account's application and the response would echo that application's company and role back. No test would fail. The same holds for forms: scope_querysets does nothing in the base class, so each subclass has to remember to narrow every related field, and nothing checks that it did. Postal addresses, telephone numbers and web links have no address of their own and are edited through inline formsets, and no test posts another account's row id into one. The referral fields (tests/test_referrals.py:309-331) and the listing named in jobs:capture_bind (tests/security/test_listing_history.py) are already tested this way; they show the pattern the other fields lack.
Documentation: docs/THREAT-MODEL.md:34 says of postal addresses that "the sweep in tests/ covers it". It cannot: no URL names a postal address.
Why it matters: a test of the boundary that covers only URL ids leaves every body id, form choice and formset row to be checked by eye.
Fix:
- Add a body table to tests/security/test_isolation_sweep.py listing route, method, field and a factory for the other person's record: interviews application_id (POST) and contact_ids (POST and PATCH), reminders application_id (POST and PATCH). For each, assert that the answer is identical to the one for a missing id and that nothing was created or changed.
- Add a form sweep that builds every OwnerScopedModelForm subclass for a user while another user also has records, and asserts that no ModelChoiceField or ModelMultipleChoiceField offers the other user's rows.
- Add one formset test per kind of row (postal address, telephone number, web link, identifier) that posts another account's row id and asserts the row is unchanged.
- Reword THREAT-MODEL.md:34 to name the tests that actually cover addresses.
- #303 will give rows their own removal addresses, which the URL sweep will then cover. Edits made through the formset will still need the test above.

3. The API download of a sent document is never asked for, and two entries get their 404 from a missing file

Where: tests/security/test_isolation_sweep.py:404 (postulo-api:document_download, with source="upload" only); :271 (accounts:avatar); :349 (jobs:company_logo with pk_of(company); company() at :63-66 stores no logo); api/routers/documents.py:158-162 (the rendered branch of download_document); accounts/views.py:220-221 (if not picture: raise Http404); jobs/views.py:425-426 (if not company.logo: raise Http404).
What happens: the sweep keys each API route by name and tries one set of arguments, so download_document is tried only for an upload. No test anywhere requests /api/v1/documents/rendered/<pk>/download, for one's own sent document or for somebody else's (tests/test_api.py:175-181 and :194 download uploads). A change to the rendered branch that dropped owned_or_404 would serve another account's sent CVs and letters, with their home address and telephone number, to any token holding documents:read, and nothing would fail. Separately, the sweep's own kept_page factory explains why a file must be behind a record ("so that a 404 is the lookup refusing and not a file that was never there"), and two entries do not follow it: the avatar entry names a user with no picture and the logo entry a company with no logo, so both views answer 404 whatever their ownership check does. Ownership of both is tested with real files in tests/security/test_feeds_and_pictures.py:135-170, so for these two the sweep proves less than it appears to; it is not a gap in coverage.
Why it matters: a sent document is the most sensitive file Postulo holds about a person, and its API download is the only file path no test touches.
Fix: let API entries carry several argument sets, or add a second key such as postulo-api:document_download#rendered with pk_of(rendered, source="rendered") and have the three bookkeeping tests strip the suffix. In tests/test_api.py, add a test that a token's owner can download their own rendered document. Give the sweep's avatar and company factories a stored file, as kept_page does (profile.avatar.save(...), company.logo.save(...)).

Done when

tests/security/test_staff_only.py passes today and fails when StaffRequiredMixin is removed from any one of PersonAdminView, PersonActiveView, PersonDeleteView, PersonUsernameView or InviteRevokeView.
tests/security/test_isolation_sweep.py fails when owned(...) is replaced with Application.objects in add_interview, add_reminder or change_reminder, or with Contact.objects in _contacts.
The new form sweep fails when one field's narrowing is removed from any scope_querysets.
tests/security/test_isolation_sweep.py fails when owned_or_404 in the rendered branch of download_document is replaced with an unscoped lookup, and its avatar and logo entries still fail when the ownership check is removed from AvatarView or CompanyLogoView.
tests/test_api.py downloads one's own rendered document through /api/v1/documents/rendered/<pk>/download and gets the file.
docs/THREAT-MODEL.md no longer says the sweep covers postal addresses.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, following each `EXCUSED` entry in `tests/security/test_isolation_sweep.py` to the test it names, checking what each factory makes, and looking for a cross-account test for every id a request carries outside its address. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. The code is now at `core/server_views.py:212` (`PersonAdminView`), `:235` (`PersonActiveView`), `:259` (`PersonDeleteView`) and `:384` (`PersonUsernameView`); `jobs/forms.py:86-87` (`OwnerScopedModelForm.scope_querysets`); `tests/test_server_settings.py:47-51` (`test_every_section_is_for_administrators_only`) and `:303` (the member's GET of `server:person_username`). The sweep itself, the API routers, `accounts/views.py:284` and the other tests cited have not moved. Related to #232 (closed), which asked for a sweep of "every view and queryset" and delivered the sweep of ids in a URL, with its `EXCUSED` list. This issue adds what that sweep excuses without proof or never tries. The code is correct today in every case below: each view has its staff gate and each lookup is narrowed. What is missing is a test that would fail if one of them stopped being so, on the boundary the threat model ranks first. ## 1. Staff-only routes are excused by citing tests that never ask them as a member - **Where:** `tests/security/test_isolation_sweep.py:40-49` (`EXCUSED`, seven entries whose reason is "the gate is" in another file); `tests/test_invites.py:217-226` (`test_invitation_management_requires_staff`, parametrised over `invite_list` and `invite_create` only, GET only); `tests/test_server_settings.py:45-49` (`test_every_section_is_for_administrators_only`, over `SECTION_URLS`, none of the `person_*` routes); `tests/test_server_settings.py:255` and `tests/test_account_deletion.py:251-254` (a member's GET only); the views at `core/server_views.py:211` (`PersonAdminView`), `:234` (`PersonActiveView`), `:258` (`PersonDeleteView`), `:383` (`PersonUsernameView`) and `accounts/views.py:284` (`InviteRevokeView`). - **What happens:** nothing asks `server:person_admin` or `server:person_active` as an ordinary member, by either method. `accounts:invite_revoke` is only ever posted to by staff. `server:person_delete` and `server:person_username` are asked as a member only with GET. If a refactor swapped `StaffRequiredMixin` on one of these views for `LoginRequiredMixin`, a signed-in member could reach the POST that grants administrator rights, suspends or deletes an account, renames a person or withdraws an invitation, and the whole suite would stay green. Members are tested on `person_recovery` (`tests/test_recovery_links.py:235-240`) and `person_plugins` (`tests/test_plugin_policy.py:272-280`), but not in the file `EXCUSED` names. - **Why it matters:** a staff check removed from one of these is a privilege escalation that no test would catch. The sweep's docstring says that adding a page "without deciding fails here"; an excuse that points at a test which does not exist defeats that. - **Fix:** add `tests/security/test_staff_only.py`. Walk every `server:` route and every `accounts:invite_*` route except `invite_accept`, filling an argument with another account's pk or a real invitation's pk. Sign in an ordinary member and send GET and an empty POST to each; assert 403 (302 for an anonymous visitor). Assert that `is_staff`, `is_active`, `username` and the `Invite` rows did not change. Point the seven `EXCUSED` reasons at that file. ## 2. Ids that arrive in an API body, a form choice or a formset row are never tried as another account's - **Where:** `tests/security/test_isolation_sweep.py:415-433` (`patterns_with_arguments`, which finds only ids in an address); `api/routers/interviews.py:97-106` (`_contacts`), `:109-113` (`add_interview`), `:151-152` (`change_interview`); `api/routers/reminders.py:38-44` (`add_reminder`), `:72-82` (`change_reminder`); `jobs/forms.py:85-86` (`OwnerScopedModelForm.scope_querysets`, a no-op marked `pragma: no cover`); `tests/test_api.py:337-345` (`"application_id": 999`); `tests/test_interviews.py:724-741` (the "stranger" contact is made with `owner=user`); `docs/THREAT-MODEL.md:34-35`. - **What happens:** `application_id` and `contact_ids` on the interview API and `application_id` on the reminder API have no cross-account test anywhere. The nearest tests use an id that does not exist, or the same person's contact at another company. If a later change looked the application up with `Application.objects` instead of `owned(request, ...)`, a token holder could attach interviews or reminders to another account's application and the response would echo that application's company and role back. No test would fail. The same holds for forms: `scope_querysets` does nothing in the base class, so each subclass has to remember to narrow every related field, and nothing checks that it did. Postal addresses, telephone numbers and web links have no address of their own and are edited through inline formsets, and no test posts another account's row id into one. The referral fields (`tests/test_referrals.py:309-331`) and the listing named in `jobs:capture_bind` (`tests/security/test_listing_history.py`) are already tested this way; they show the pattern the other fields lack. - **Documentation:** `docs/THREAT-MODEL.md:34` says of postal addresses that "the sweep in `tests/` covers it". It cannot: no URL names a postal address. - **Why it matters:** a test of the boundary that covers only URL ids leaves every body id, form choice and formset row to be checked by eye. - **Fix:** - Add a body table to `tests/security/test_isolation_sweep.py` listing route, method, field and a factory for the other person's record: interviews `application_id` (POST) and `contact_ids` (POST and PATCH), reminders `application_id` (POST and PATCH). For each, assert that the answer is identical to the one for a missing id and that nothing was created or changed. - Add a form sweep that builds every `OwnerScopedModelForm` subclass for a user while another user also has records, and asserts that no `ModelChoiceField` or `ModelMultipleChoiceField` offers the other user's rows. - Add one formset test per kind of row (postal address, telephone number, web link, identifier) that posts another account's row id and asserts the row is unchanged. - Reword `THREAT-MODEL.md:34` to name the tests that actually cover addresses. - #303 will give rows their own removal addresses, which the URL sweep will then cover. Edits made through the formset will still need the test above. ## 3. The API download of a sent document is never asked for, and two entries get their 404 from a missing file - **Where:** `tests/security/test_isolation_sweep.py:404` (`postulo-api:document_download`, with `source="upload"` only); `:271` (`accounts:avatar`); `:349` (`jobs:company_logo` with `pk_of(company)`; `company()` at `:63-66` stores no logo); `api/routers/documents.py:158-162` (the `rendered` branch of `download_document`); `accounts/views.py:220-221` (`if not picture: raise Http404`); `jobs/views.py:425-426` (`if not company.logo: raise Http404`). - **What happens:** the sweep keys each API route by name and tries one set of arguments, so `download_document` is tried only for an upload. No test anywhere requests `/api/v1/documents/rendered/<pk>/download`, for one's own sent document or for somebody else's (`tests/test_api.py:175-181` and `:194` download uploads). A change to the `rendered` branch that dropped `owned_or_404` would serve another account's sent CVs and letters, with their home address and telephone number, to any token holding `documents:read`, and nothing would fail. Separately, the sweep's own `kept_page` factory explains why a file must be behind a record ("so that a 404 is the lookup refusing and not a file that was never there"), and two entries do not follow it: the avatar entry names a user with no picture and the logo entry a company with no logo, so both views answer 404 whatever their ownership check does. Ownership of both is tested with real files in `tests/security/test_feeds_and_pictures.py:135-170`, so for these two the sweep proves less than it appears to; it is not a gap in coverage. - **Why it matters:** a sent document is the most sensitive file Postulo holds about a person, and its API download is the only file path no test touches. - **Fix:** let `API` entries carry several argument sets, or add a second key such as `postulo-api:document_download#rendered` with `pk_of(rendered, source="rendered")` and have the three bookkeeping tests strip the suffix. In `tests/test_api.py`, add a test that a token's owner can download their own rendered document. Give the sweep's avatar and company factories a stored file, as `kept_page` does (`profile.avatar.save(...)`, `company.logo.save(...)`). ## Done when - `tests/security/test_staff_only.py` passes today and fails when `StaffRequiredMixin` is removed from any one of `PersonAdminView`, `PersonActiveView`, `PersonDeleteView`, `PersonUsernameView` or `InviteRevokeView`. - `tests/security/test_isolation_sweep.py` fails when `owned(...)` is replaced with `Application.objects` in `add_interview`, `add_reminder` or `change_reminder`, or with `Contact.objects` in `_contacts`. - The new form sweep fails when one field's narrowing is removed from any `scope_querysets`. - `tests/security/test_isolation_sweep.py` fails when `owned_or_404` in the `rendered` branch of `download_document` is replaced with an unscoped lookup, and its avatar and logo entries still fail when the ownership check is removed from `AvatarView` or `CompanyLogoView`. - `tests/test_api.py` downloads one's own rendered document through `/api/v1/documents/rendered/<pk>/download` and gets the file. - `docs/THREAT-MODEL.md` no longer says the sweep covers postal addresses.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:46:35 +00:00

tiagoagueda added the

documentation

enhancement

labels

2026-10-01 10:46:35 +00:00

tiagoagueda referenced this issue from a commit

2026-10-06 07:40:10 +00:00

Sweep the staff-only routes, ids in bodies and forms, and the sent-document download

tiagoagueda commented

2026-10-06 09:32:20 +00:00