collection_id scoping is assumed rather than enforced on by-id routes

claude-bot commented

2026-07-28 05:57:14 +00:00

Severity: MEDIUM

The bug

Only list_photos filters by collection (photos.py:129). Every single-object fetch —
photo, evidence, comment, job — is a bare primary-key lookup with no collection predicate:
photos.py:68-72 (_get_photo_or_404), :340, :508, and jobs.py:81,93. GET /api/jobs has
no collection filter at all and lists jobs across every collection.

POST /api/evidence/{id}/supersede (photos.py:333-355) is the worst case: it fetches evidence
by raw id, does not load the parent photo, does not verify collection membership, and — unlike
every sibling route — performs no review_version conflict check despite mutating
reviewer-visible state.

Why it matters

The single-collection deployment masks all of this. But the schema carries collection_id on
all seven owned tables specifically to support multiple collections, and list_photos already
resolves "the default collection" as though choice existed. The moment a second collection is
created, every by-id route becomes a cross-tenant read/write.

Even today, the supersede route lets any reviewer deactivate any evidence row by id with no
concurrency guard — quietly changing the evidentiary basis of a photo's date. Deactivated rows
are still returned but rendered at 40% opacity, so it is easy to miss.

Fixing this now is nearly free; retrofitting after multi-collection ships means touching every
route, with no authorization tests (#8) to catch regressions.

Fix

A request-scoped "active collection" dependency and a get_scoped(model, id, collection_id)
repository helper.
Forbid bare db.get for user-supplied ids by convention, and enforce it in review.
Add collection_id filtering to JobRepository.list_jobs.
Add the missing review_version check to the supersede route.

Done when

Every by-id route verifies collection membership
GET /api/jobs is collection-scoped
Supersede performs a conflict check like its sibling routes
Tests assert cross-collection access is refused

References

backend/app/api/routes/photos.py:68-72,333-355,508; jobs.py:81,93

Related: #8 (authorization tests) should cover this matrix.

## Severity: MEDIUM ## The bug Only `list_photos` filters by collection (`photos.py:129`). **Every single-object fetch** — photo, evidence, comment, job — is a bare primary-key lookup with no collection predicate: `photos.py:68-72` (`_get_photo_or_404`), `:340`, `:508`, and `jobs.py:81,93`. `GET /api/jobs` has no collection filter at all and lists jobs across every collection. `POST /api/evidence/{id}/supersede` (`photos.py:333-355`) is the worst case: it fetches evidence by raw id, does not load the parent photo, does not verify collection membership, and — unlike every sibling route — performs **no `review_version` conflict check** despite mutating reviewer-visible state. ## Why it matters The single-collection deployment masks all of this. But the schema carries `collection_id` on all seven owned tables specifically to support multiple collections, and `list_photos` already resolves "the default collection" as though choice existed. The moment a second collection is created, every by-id route becomes a cross-tenant read/write. Even today, the supersede route lets any reviewer deactivate any evidence row by id with no concurrency guard — quietly changing the evidentiary basis of a photo's date. Deactivated rows are still returned but rendered at 40% opacity, so it is easy to miss. Fixing this now is nearly free; retrofitting after multi-collection ships means touching every route, with no authorization tests (#8) to catch regressions. ## Fix - A request-scoped "active collection" dependency and a `get_scoped(model, id, collection_id)` repository helper. - Forbid bare `db.get` for user-supplied ids by convention, and enforce it in review. - Add `collection_id` filtering to `JobRepository.list_jobs`. - Add the missing `review_version` check to the supersede route. ## Done when - [ ] Every by-id route verifies collection membership - [ ] `GET /api/jobs` is collection-scoped - [ ] Supersede performs a conflict check like its sibling routes - [ ] Tests assert cross-collection access is refused ## References - `backend/app/api/routes/photos.py:68-72,333-355,508`; `jobs.py:81,93` Related: #8 (authorization tests) should cover this matrix.

claude-bot added this to the v0.1.1 milestone

2026-07-28 05:57:14 +00:00

claude-bot added the

backend

security

labels

2026-07-28 05:57:14 +00:00

claude-bot referenced this issue

2026-07-28 06:07:54 +00:00

Add API contract and authorization tests for the Phase 1 surface #8

rbrooks referenced this issue from a commit

2026-07-30 20:52:25 +00:00

Enforce collection scoping on every by-id route

rbrooks closed this issue

2026-07-30 20:52:25 +00:00

claude-bot commented

2026-07-30 20:52:38 +00:00

Author

Done in ebd6121. All four "done when" items covered, 45 tests in backend/tests/test_collection_scoping.py.

The fix has two halves, and the second is the one that lasts.

get_active_collection (backend/app/api/deps.py) is the request-scoped dependency — one place that decides which collection a request operates in, which grows a membership check when multi-collection lands without the routes changing.

Then collection_id became a required keyword argument on every repository by-id lookup, not an optional filter. A call site that forgets to scope its query is a TypeError at import or test time rather than a silent cross-collection read. There's a test that reflects over the signatures and asserts exactly that — required, keyword-only, no default — because the point is that the property holds for call sites nobody has written yet. That's a stronger version of the "forbid bare db.get by convention" in the issue: conventions need someone to remember them in review.

Client-supplied pagination cursors were bare db.get too, so those resolve within the collection now as well. Foreign rows return 404, not 403 — confirming a photo exists is itself a disclosure, same reasoning as the existing viewer rule.

Supersede. As you flagged, this was the worst one, and fixing it properly meant a bit more than scoping. It now resolves evidence within the collection, loads the parent photo (so the viewer rule applies too), takes review_version, and increments it. The bump isn't in the issue text but is load-bearing: without it two concurrent supersedes both pass a check against the same version, which defeats the point of adding the check. There's a test for that specific race. review_version is now a required body field — a breaking API change, but supersedeEvidence was defined in the frontend client and called from no component, so nothing in the UI breaks. Client signature updated anyway.

UserRepository.get_by_id is deliberately left unscoped — the user table has no collection_id; users are global, not collection-owned.

One behaviour change worth knowing about: GET /api/photos used to return an empty list when no collection existed; it now 404s, because collection resolution is a dependency. Only reachable before the first login, which is what creates the default collection. Three TestBodyLimits tests needed the collection fixture for the same reason — dependencies are solved before request-validation errors are raised, so they were seeing the scoping 404 instead of the 422 they assert on.

Related #8 (authorization tests) — this covers the collection axis of that matrix; the role axis is already in test_authz_matrix.py.

Done in ebd6121. All four "done when" items covered, 45 tests in `backend/tests/test_collection_scoping.py`. **The fix has two halves, and the second is the one that lasts.** `get_active_collection` (`backend/app/api/deps.py`) is the request-scoped dependency — one place that decides which collection a request operates in, which grows a membership check when multi-collection lands without the routes changing. Then `collection_id` became a **required keyword argument** on every repository by-id lookup, not an optional filter. A call site that forgets to scope its query is a `TypeError` at import or test time rather than a silent cross-collection read. There's a test that reflects over the signatures and asserts exactly that — required, keyword-only, no default — because the point is that the property holds for call sites nobody has written yet. That's a stronger version of the "forbid bare `db.get` by convention" in the issue: conventions need someone to remember them in review. Client-supplied pagination cursors were bare `db.get` too, so those resolve within the collection now as well. Foreign rows return **404, not 403** — confirming a photo exists is itself a disclosure, same reasoning as the existing viewer rule. **Supersede.** As you flagged, this was the worst one, and fixing it properly meant a bit more than scoping. It now resolves evidence within the collection, loads the parent photo (so the viewer rule applies too), takes `review_version`, and **increments it**. The bump isn't in the issue text but is load-bearing: without it two concurrent supersedes both pass a check against the same version, which defeats the point of adding the check. There's a test for that specific race. `review_version` is now a required body field — a breaking API change, but `supersedeEvidence` was defined in the frontend client and called from no component, so nothing in the UI breaks. Client signature updated anyway. **`UserRepository.get_by_id` is deliberately left unscoped** — the `user` table has no `collection_id`; users are global, not collection-owned. **One behaviour change worth knowing about:** `GET /api/photos` used to return an empty list when no collection existed; it now 404s, because collection resolution is a dependency. Only reachable before the first login, which is what creates the default collection. Three `TestBodyLimits` tests needed the `collection` fixture for the same reason — dependencies are solved before request-validation errors are raised, so they were seeing the scoping 404 instead of the 422 they assert on. Related #8 (authorization tests) — this covers the collection axis of that matrix; the role axis is already in `test_authz_matrix.py`.

claude-bot referenced this issue

2026-08-01 05:52:37 +00:00

review_version guard is check-then-write and races #77

claude-bot referenced this issue

2026-08-01 05:53:39 +00:00

supersede_evidence never sets superseded_by #80

rbrooks referenced this issue from a commit

2026-08-02 05:31:19 +00:00

Stop the tests writing into the real archive directory

claude-bot referenced this issue

2026-08-02 05:39:58 +00:00

Add the backend test harness: fixtures, factories, and DB isolation #5