rbrooks/Circa

Watch

Fork 0

No admin-only routes exist and the viewer role is unenforced #73

New issue

Closed

opened 2026-07-28 05:57:16 +00:00 by claude-bot · 1 comment

claude-bot commented

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

Severity: MEDIUM

The problem

The route × role matrix built during the audit found:

Zero admin-only routes exist. _ADMIN = Depends(require_role(UserRole.admin)) is defined
at backend/app/api/routes/ingest.py:27 and never used. require_role(UserRole.admin) appears
on no route. The admin role currently grants exactly one extra capability: deleting other
users' comments.
The viewer role is unreachable. No user is ever assigned it, and no route distinguishes
it from reviewer for reads. docs/circa-spec.md §4.2 ("read-only access to approved photos
only") is entirely unimplemented — a viewer, if one existed, could read pending and
disputed photos.
/api/ingest's own docstring says "Admin-only in Phase 1" while the code enforces
require_role(reviewer, admin). Given this is the arbitrary-write route, the docstring is
probably the intent and the code is the bug.

Fix

Decide and document the real permission matrix for viewer / reviewer / admin, then enforce it.
Restrict /api/ingest to admin, matching its docstring and the spec.
Implement viewer read scoping: approved photos only.
Add the role-management endpoints the spec assumes exist, so an admin can actually assign roles.

This overlaps #22 (multi-user roles and attribution), which builds the full model. Keep this
issue for the enforcement gaps that exist today; #22 can build on it.

Done when

A written permission matrix exists and matches the code
Ingest is admin-only
Viewers can read only approved photos
An admin can change another user's role through the API
#8's authorization tests cover every cell of the matrix

References

backend/app/api/routes/ingest.py:27,49
backend/app/auth/session.py (require_role)
docs/circa-spec.md §4.2

Related: #8, #22.

## Severity: MEDIUM ## The problem The route × role matrix built during the audit found: - **Zero admin-only routes exist.** `_ADMIN = Depends(require_role(UserRole.admin))` is defined at `backend/app/api/routes/ingest.py:27` and never used. `require_role(UserRole.admin)` appears on no route. The admin role currently grants exactly one extra capability: deleting other users' comments. - **The `viewer` role is unreachable.** No user is ever assigned it, and no route distinguishes it from reviewer for reads. `docs/circa-spec.md` §4.2 ("read-only access to approved photos only") is entirely unimplemented — a viewer, if one existed, could read `pending` and `disputed` photos. - **`/api/ingest`'s own docstring says "Admin-only in Phase 1"** while the code enforces `require_role(reviewer, admin)`. Given this is the arbitrary-write route, the docstring is probably the intent and the code is the bug. ## Fix - Decide and document the real permission matrix for viewer / reviewer / admin, then enforce it. - Restrict `/api/ingest` to admin, matching its docstring and the spec. - Implement viewer read scoping: approved photos only. - Add the role-management endpoints the spec assumes exist, so an admin can actually assign roles. This overlaps #22 (multi-user roles and attribution), which builds the full model. Keep this issue for the *enforcement gaps that exist today*; #22 can build on it. ## Done when - [ ] A written permission matrix exists and matches the code - [ ] Ingest is admin-only - [ ] Viewers can read only approved photos - [ ] An admin can change another user's role through the API - [ ] #8's authorization tests cover every cell of the matrix ## References - `backend/app/api/routes/ingest.py:27,49` - `backend/app/auth/session.py` (`require_role`) - `docs/circa-spec.md` §4.2 Related: #8, #22.

claude-bot added this to the v0.1.1 milestone

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

claude-bot added the

backend

security

labels

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

claude-bot referenced this issue

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

Verify Immich import compatibility and a standalone Circa archive view #121

claude-bot referenced this issue

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

Implement multi-user roles and review attribution #22

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

claude-bot referenced this issue

2026-07-28 16:02:34 +00:00

Family-facing archive view: browse the finished collection #129

claude-bot referenced this issue

2026-07-28 17:59:09 +00:00

Arbitrary file write via unsanitized upload filename (RCE) #55

claude-bot referenced this issue

2026-07-28 17:59:09 +00:00

Every OAuth login is auto-provisioned as reviewer #56

rbrooks referenced this issue from a commit

2026-07-30 18:21:01 +00:00

Stop exposing user ids; enforce the role matrix

rbrooks closed this issue

2026-07-30 18:21:01 +00:00

claude-bot commented

2026-07-30 18:21:47 +00:00

Author

Fixed in 5f2cbdb. 144 tests passing.

All three gaps from the audit's route matrix.

Ingest is now admin-only. Its docstring said "Admin-only in Phase 1" while the code enforced
reviewer. Since this is the arbitrary-write primitive from #55, the docstring was right and the
code was wrong — and that mismatch is what made the RCE chain reachable by any account the IdP
accepted.

Viewer reads are enforced. Viewers see approved photos only, per spec §4.2. Two details:

By-id reads return 404, not 403. Confirming that a photo exists is itself a disclosure, and
the difference between the two responses is exactly the information being protected.
The list endpoint filters rather than trusting the client's status parameter, and an explicit
request for a non-approved status returns empty rather than erroring — a viewer asking for
disputed photos gets nothing, not a hint that such photos exist.

Admin routes exist. /api/users supports listing and role/activation changes. This was not
optional once #56 made viewer the default: without it there is no path from "someone logged in"
to "someone can review", and the spec's "role assignments are managed by an Admin in the UI"
was unimplementable.

Lockout guard. The last active admin cannot be demoted or deactivated. Checked before either
change is applied, because demotion and deactivation are two routes to the same outcome — and
admin_exists() filters on is_active, so the two guards agree with each other.

All role changes emit user_role_changed audit events with before/after context. Migration 002 adds
that enum value; on SQLite sa.Enum is a CHECK constraint so it rebuilds the table, and its
downgrade refuses if any such event exists rather than deleting append-only history.

Tested: the full role matrix, viewer scoping across all four photo statuses, ingest refusing a
reviewer, and both lockout paths.

**Fixed** in 5f2cbdb. 144 tests passing. All three gaps from the audit's route matrix. **Ingest is now admin-only.** Its docstring said "Admin-only in Phase 1" while the code enforced `reviewer`. Since this is the arbitrary-write primitive from #55, the docstring was right and the code was wrong — and that mismatch is what made the RCE chain reachable by any account the IdP accepted. **Viewer reads are enforced.** Viewers see approved photos only, per spec §4.2. Two details: - By-id reads return **404, not 403**. Confirming that a photo exists is itself a disclosure, and the difference between the two responses is exactly the information being protected. - The list endpoint *filters* rather than trusting the client's `status` parameter, and an explicit request for a non-approved status returns empty rather than erroring — a viewer asking for disputed photos gets nothing, not a hint that such photos exist. **Admin routes exist.** `/api/users` supports listing and role/activation changes. This was not optional once #56 made `viewer` the default: without it there is no path from "someone logged in" to "someone can review", and the spec's "role assignments are managed by an Admin in the UI" was unimplementable. **Lockout guard.** The last active admin cannot be demoted or deactivated. Checked *before* either change is applied, because demotion and deactivation are two routes to the same outcome — and `admin_exists()` filters on `is_active`, so the two guards agree with each other. All role changes emit `user_role_changed` audit events with before/after context. Migration 002 adds that enum value; on SQLite `sa.Enum` is a CHECK constraint so it rebuilds the table, and its downgrade refuses if any such event exists rather than deleting append-only history. Tested: the full role matrix, viewer scoping across all four photo statuses, ingest refusing a reviewer, and both lockout paths.

claude-bot referenced this issue

2026-08-02 07:17:21 +00:00

Add optimistic-concurrency (409 review_version_conflict) tests #9

claude-bot referenced this issue

2026-08-08 17:26:08 +00:00

Authorization should come from Authentik: drop the email_verified gate, take roles from groups #149