PST-096: Some security tests look like proof but do not discriminate, and some promises have no mechanical check #825

New issue

Closed

opened 2026-10-10 10:57:02 +00:00 by tiagoagueda · 1 comment

tiagoagueda commented

2026-10-10 10:57:02 +00:00

Owner

Found in the security audit of 2026-10-10 (SECURITY-AUDIT-2026-10-10.md), run against main at c30851b from 8 to 10 October 2026, as PST-096. Severity: Informational.

The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it.

Severity: Informational
Category: test-coverage
Location: tests/security/test_isolation_sweep.py (FACTORIES entries at ~390-420 and 'jobs:company_logo_action')
Found by: completeness review: tests against claims

Who can do it, and what they need. Not an attack; a future regression that these tests would not catch.

How it works.

Verified by running each FACTORIES entry against the owner's own record: the sweep asserts 404 for a foreign record, but for several entries the owner's own record is also 404/405, so the assertion would pass even if ownership scoping were removed. jobs:company_logo_action uses action 'find', which the view does not accept (valid: remove, website, refresh) -> own company 404, foreign 404, while real actions give own 302 and foreign 404. connections:backfill and connections:sync_now build a 'notifier' connection but the views want store/sync kinds -> own POST 404. jobs:capture_page_source -> own GET 404 with that factory. jobs:capture_bind -> own POST 404 without a listing in the body (tests/security/test_listing_history.py covers it elsewhere). …

Impact. The README and SECURITY.md promise 'ownership sweeps across every view and queryset'. Other gaps in what pins that promise: (a) nothing statically enforces for_user() on a new query: an AST scan found about 180 .objects calls on OwnedModel classes with no owner or for_user in the statement (triaged: scheduler, metrics and scrub jobs, subqueries hanging off a scoped parent, and generic-FK …

Verification (verdict: confirmed; severity after verification: Informational). Keep informational (test-coverage, no attack path). Corrections: (1) Locations: the sweep entries are at tests/security/test_isolation_sweep.py:424, 428, 470, 474 and 479, FACTORIES starts at 387, and the sweep tests at 616-628 and 652; the logo action view is src/postulo/jobs/views.py:526-534. (2) The vacuousness is per entry, not systemic: four of the five have a discriminating test elsewhere (tests/test_logos.py:501, tests/test_syncs.py:242, tests/test_stores.py:556, tests/security/test_captured_pages.py:350). The one real gap is the capture side of jobs:capture_bind. Nothing pins it, an unscoped lookup there passes 375 existing tests, and history.py:220-221 turns the regression into a 500 rather than a cross-account write. …

Suggested remediation. Make the sweep prove it can fail: for each FACTORIES entry also build the owner's own record and assert that the same request is not 404 (or list the entry in a documented exception), use a valid action and connection kind for the four entries above, add a lint test that fails on <OwnedModel>.objects. without for_user/owner in src/ unless annotated, and add a header-walk test over all download routes.

Found in the security audit of 2026-10-10 (`SECURITY-AUDIT-2026-10-10.md`), run against `main` at `c30851b` from 8 to 10 October 2026, as **PST-096**. Severity: **Informational**. The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it. --- - **Severity:** Informational - **Category:** test-coverage - **Location:** `tests/security/test_isolation_sweep.py` (FACTORIES entries at ~390-420 and 'jobs:company_logo_action') - **Found by:** completeness review: tests against claims **Who can do it, and what they need.** Not an attack; a future regression that these tests would not catch. **How it works.** Verified by running each FACTORIES entry against the owner's own record: the sweep asserts 404 for a foreign record, but for several entries the owner's own record is also 404/405, so the assertion would pass even if ownership scoping were removed. jobs:company_logo_action uses action 'find', which the view does not accept (valid: remove, website, refresh) -> own company 404, foreign 404, while real actions give own 302 and foreign 404. connections:backfill and connections:sync_now build a 'notifier' connection but the views want store/sync kinds -> own POST 404. jobs:capture_page_source -> own GET 404 with that factory. jobs:capture_bind -> own POST 404 without a listing in the body (tests/security/test_listing_history.py covers it elsewhere). … **Impact.** The README and SECURITY.md promise 'ownership sweeps across every view and queryset'. Other gaps in what pins that promise: (a) nothing statically enforces for_user() on a new query: an AST scan found about 180 `.objects` calls on OwnedModel classes with no owner or for_user in the statement (triaged: scheduler, metrics and scrub jobs, subqueries hanging off a scoped parent, and generic-FK … **Verification** (verdict: confirmed; severity after verification: Informational). Keep informational (test-coverage, no attack path). Corrections: (1) Locations: the sweep entries are at tests/security/test_isolation_sweep.py:424, 428, 470, 474 and 479, FACTORIES starts at 387, and the sweep tests at 616-628 and 652; the logo action view is src/postulo/jobs/views.py:526-534. (2) The vacuousness is per entry, not systemic: four of the five have a discriminating test elsewhere (tests/test_logos.py:501, tests/test_syncs.py:242, tests/test_stores.py:556, tests/security/test_captured_pages.py:350). The one real gap is the capture side of jobs:capture_bind. Nothing pins it, an unscoped lookup there passes 375 existing tests, and history.py:220-221 turns the regression into a 500 rather than a cross-account write. … **Suggested remediation.** Make the sweep prove it can fail: for each FACTORIES entry also build the owner's own record and assert that the same request is not 404 (or list the entry in a documented exception), use a valid action and connection kind for the four entries above, add a lint test that fails on `<OwnedModel>.objects.` without for_user/owner in src/ unless annotated, and add a header-walk test over all download routes.

tiagoagueda added the

enhancement

security

labels

2026-10-10 10:57:02 +00:00

tiagoagueda added this to the 1.0.0 milestone

2026-10-10 10:58:22 +00:00

tiagoagueda referenced this issue

2026-10-10 10:58:40 +00:00

Security audit of 2026-10-10: index of PST-001 to PST-098 (#730–#827) #828

tiagoagueda referenced this issue from a commit

2026-10-12 17:31:54 +00:00