rbrooks/TeaLeaves

Watch

Fork 0

Media access is uploader-scoped, not project-scoped — collaborator attachments are invisible to the project owner #135

New issue

Closed

opened 2026-07-17 02:30:36 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-17 02:30:36 +00:00

Contributor

Severity: Medium · Confidence: High · Effort: M · Category: code (access control)

Problem

Every media route authorizes on media.user_id = <viewer> rather than on access to the media's project:

api/src/routes/media.ts:153 — GET /:id (serve file): WHERE id = $1 AND user_id = $2
api/src/routes/media.ts:179 — GET /entry/:entryId (list an entry's media): WHERE entry_id = $1 AND user_id = $2
also :115 thumbnail, :137 transcription, :193 delete, :217, :239, :316, :357, :370 similar

Entries moved to project-scoped access in v4 (ACCESSIBLE_PROJECTS_SQL / ACTIVE_PROJECT_SQL, migration 026). Media never followed — it still uses the pre-project ownership model.

Impact

Today, with no deletion involved: if a collaborator uploads media to an entry in a project you own, media.user_id is their id — so GET /api/media/entry/:entryId returns nothing for you and GET /api/media/:id 404s. You can see their entry but not its attachments.

Interaction with #98 (v7.2.0): #98 changed media.user_id to ON DELETE SET NULL so a departed collaborator's attachments are no longer destroyed along with the entries that survive them. That fix is correct and necessary — the rows and files are now preserved rather than silently deleted — but user_id = NULL never matches user_id = $viewer, so those preserved attachments remain unreachable through the API until access is project-scoped. #98 stops the data loss; this issue is what makes the data usable again.

Fix

Re-scope the media routes from user_id = $viewer to the entry's project, mirroring entries:

JOIN entries e ON e.id = media.entry_id
WHERE e.project_id IN (${ACCESSIBLE_PROJECTS_SQL})

user_id then becomes what it should be — provenance/attribution, not authorization. Worth auditing whether delete should stay stricter (uploader or project owner) rather than any project member.

Acceptance criteria

A project owner can list, view and thumbnail media attached to an entry in their project regardless of which collaborator uploaded it.
Media attached to entries whose uploader was deleted (user_id IS NULL) is reachable by users with access to that project.
A user with no access to the project still gets 404 on every media route.
Delete authorization is explicit (decide: uploader only, or project owner too) and tested.

Notes

Found while implementing #98 during v7.2.0 — the audit didn't catch this because it read the FK change in isolation. Filed separately rather than widening #98's scope: #98 is a one-migration data-preservation fix, this is a multi-route access-control change with its own authorization decisions.

**Severity:** Medium · **Confidence:** High · **Effort:** M · Category: code (access control) ## Problem Every media route authorizes on **`media.user_id = <viewer>`** rather than on access to the media's *project*: - `api/src/routes/media.ts:153` — `GET /:id` (serve file): `WHERE id = $1 AND user_id = $2` - `api/src/routes/media.ts:179` — `GET /entry/:entryId` (list an entry's media): `WHERE entry_id = $1 AND user_id = $2` - also `:115` thumbnail, `:137` transcription, `:193` delete, `:217`, `:239`, `:316`, `:357`, `:370` similar Entries moved to project-scoped access in v4 (`ACCESSIBLE_PROJECTS_SQL` / `ACTIVE_PROJECT_SQL`, migration 026). Media never followed — it still uses the pre-project ownership model. ## Impact **Today, with no deletion involved:** if a collaborator uploads media to an entry in a project you own, `media.user_id` is *their* id — so `GET /api/media/entry/:entryId` returns nothing for you and `GET /api/media/:id` 404s. You can see their entry but not its attachments. **Interaction with #98 (v7.2.0):** #98 changed `media.user_id` to `ON DELETE SET NULL` so a departed collaborator's attachments are no longer destroyed along with the entries that survive them. That fix is correct and necessary — the rows and files are now **preserved** rather than silently deleted — but `user_id = NULL` never matches `user_id = $viewer`, so those preserved attachments remain **unreachable** through the API until access is project-scoped. #98 stops the data loss; this issue is what makes the data usable again. ## Fix Re-scope the media routes from `user_id = $viewer` to the entry's project, mirroring entries: ```sql JOIN entries e ON e.id = media.entry_id WHERE e.project_id IN (${ACCESSIBLE_PROJECTS_SQL}) ``` `user_id` then becomes what it should be — provenance/attribution, not authorization. Worth auditing whether **delete** should stay stricter (uploader or project owner) rather than any project member. ## Acceptance criteria - [ ] A project owner can list, view and thumbnail media attached to an entry in their project regardless of which collaborator uploaded it. - [ ] Media attached to entries whose uploader was deleted (`user_id IS NULL`) is reachable by users with access to that project. - [ ] A user with no access to the project still gets 404 on every media route. - [ ] Delete authorization is explicit (decide: uploader only, or project owner too) and tested. ## Notes Found while implementing #98 during v7.2.0 — the audit didn't catch this because it read the FK change in isolation. Filed separately rather than widening #98's scope: #98 is a one-migration data-preservation fix, this is a multi-route access-control change with its own authorization decisions.

claude-bot added this to the v7.2.0 - Reliability, data integrity & docs milestone

2026-07-17 02:30:47 +00:00

claude-bot added the

bug

label

2026-07-17 02:30:47 +00:00

claude-bot referenced this issue

2026-07-17 02:31:43 +00:00

media.user_id ON DELETE CASCADE strips attachments from entries that survive user deletion #98

rbrooks referenced this issue from a commit

2026-07-17 16:29:34 +00:00

fix(v7.2): scope media access to the project, not the uploader (#135)

claude-bot commented

2026-07-17 16:34:03 +00:00

Author

Contributor

Fixed in c7ff1a2 (v7.2.0).

Every media route authorized on media.user_id = <viewer> — a pre-v4 ownership check that never followed entries into the project model. Re-scoped the whole surface to the media's project:

Reads (GET /:id serve, /:id/thumbnail, /:id/transcription, /entry/:entryId list, /:id/analyze-region, /:id/similar — both the source lookup and the candidate set) now go through a loadAccessibleMedia() helper that joins entry → project and checks ACCESSIBLE_PROJECTS_SQL. Upload verifies the target entry is in an accessible project rather than authored by the uploader.
Delete — explicit decision per the acceptance criteria: uploader OR project owner, not an arbitrary editor. So collaborators can't remove each other's attachments, while the owner manages their whole project and you always manage your own uploads.
annotations.ts carried the same uploader-scoped media guard — fixed, so you can annotate any media you can access. Annotation ownership is left per-user; whether annotations should be shared project-wide is a separate product question, flagged not decided.

Acceptance criteria:

A project owner can list, view and thumbnail media attached to an entry in their project regardless of which collaborator uploaded it.
Media attached to entries whose uploader was deleted (user_id IS NULL) is reachable by users with access to that project — this is the #98 interaction, now closed: #98 stopped the deletion, this makes the preserved media reachable.
A user with no access to the project gets 404 on every media route.
Delete authorization is explicit (uploader or project owner) and tested.

Tests — api/src/test/integration/mediaAccess.test.ts, a real four-user matrix (owner / collaborator / second-editor / outsider via the /__test_login backdoor): owner sees the collaborator's upload, null-uploader media stays reachable, a non-member gets 404, and the full delete matrix (uploader ✓, owner ✓, other-editor ✗ 404, outsider ✗ 404).

Caught while doing it: the /similar rewrite would have used .replace('$1', '$2') on ACCESSIBLE_PROJECTS_SQL, which references $1 twice — String.replace only swaps the first, leaving the second $1 bound to the embedding vector. Reordered the params so userId is $1 instead. Worth noting since that fragment is used across the codebase — a .replace on it is a trap.

Verified on the dev server: tsc clean, 268/268, deployed and healthy. All three CI jobs green on c7ff1a2.

Note on delete policy: I went uploader-or-owner. If you'd rather any project editor be able to manage all project media (consistent with editors editing entries), it's a one-line change — say so and I'll flip it.

Fixed in `c7ff1a2` (v7.2.0). Every media route authorized on `media.user_id = <viewer>` — a pre-v4 ownership check that never followed entries into the project model. Re-scoped the whole surface to the media's **project**: - **Reads** (`GET /:id` serve, `/:id/thumbnail`, `/:id/transcription`, `/entry/:entryId` list, `/:id/analyze-region`, `/:id/similar` — both the source lookup and the candidate set) now go through a `loadAccessibleMedia()` helper that joins `entry → project` and checks `ACCESSIBLE_PROJECTS_SQL`. **Upload** verifies the target entry is in an accessible project rather than authored by the uploader. - **Delete** — explicit decision per the acceptance criteria: **uploader OR project owner**, not an arbitrary editor. So collaborators can't remove each other's attachments, while the owner manages their whole project and you always manage your own uploads. - **`annotations.ts`** carried the same uploader-scoped media guard — fixed, so you can annotate any media you can access. Annotation *ownership* is left per-user; whether annotations should be shared project-wide is a separate product question, flagged not decided. **Acceptance criteria:** - [x] A project owner can list, view and thumbnail media attached to an entry in their project regardless of which collaborator uploaded it. - [x] Media attached to entries whose uploader was deleted (`user_id IS NULL`) is reachable by users with access to that project — this is the #98 interaction, now closed: #98 stopped the deletion, this makes the preserved media reachable. - [x] A user with no access to the project gets 404 on every media route. - [x] Delete authorization is explicit (uploader or project owner) and tested. **Tests** — `api/src/test/integration/mediaAccess.test.ts`, a real four-user matrix (owner / collaborator / second-editor / outsider via the `/__test_login` backdoor): owner sees the collaborator's upload, null-uploader media stays reachable, a non-member gets 404, and the full delete matrix (uploader ✓, owner ✓, other-editor ✗ 404, outsider ✗ 404). **Caught while doing it:** the `/similar` rewrite would have used `.replace('$1', '$2')` on `ACCESSIBLE_PROJECTS_SQL`, which references `$1` **twice** — `String.replace` only swaps the first, leaving the second `$1` bound to the embedding vector. Reordered the params so `userId` is `$1` instead. Worth noting since that fragment is used across the codebase — a `.replace` on it is a trap. Verified on the dev server: tsc clean, **268/268**, deployed and healthy. All three CI jobs green on `c7ff1a2`. **Note on delete policy:** I went uploader-or-owner. If you'd rather any project editor be able to manage all project media (consistent with editors editing entries), it's a one-line change — say so and I'll flip it.

claude-bot closed this issue

2026-07-17 16:34:03 +00:00

rbrooks referenced this issue from a commit

2026-07-17 16:37:40 +00:00

chore(release): v7.2.0 — Reliability, data integrity & docs