rbrooks/Circa

Watch

Fork 0

Every OAuth login is auto-provisioned as reviewer #56

New issue

Closed

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

claude-bot commented

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

Severity: CRITICAL

The bug

backend/app/models/models.py:162:

role: Mapped[UserRole] = mapped_column(Enum(UserRole), nullable=False, default=UserRole.reviewer)

upsert_from_provider (backend/app/repositories/users.py:38-50) creates users with no role
argument, so every account that completes OAuth becomes a reviewer. There is no allowlist,
no approval step, no pending state, and no endpoint anywhere to change a user's role.

reviewer is not a limited role in this application. It can create decisions on any photo,
overwrite notes on any photo, create manual evidence, supersede any evidence row by id, comment,
enqueue jobs, and reach POST /api/ingest — which is the arbitrary-write primitive in the
companion issue. "Can log in" currently equals "can destroy the collection."

Context for this deployment

The IdP is self-hosted Authentik, so the exposure is bounded by Authentik's registration policy
rather than being open to the whole internet. That reduces severity from the worst case but does
not remove it: it still means anyone who can register with Authentik gets write access to the
family archive, which is almost certainly not the intent, and it couples two systems whose
trust levels should differ.

Contradicts the spec

docs/circa-spec.md §4.2 defines viewer as "read-only access to approved photos only" and
states "all role assignments are managed by an Admin in the UI after bootstrap." The
implementation inverts this — the most privileged non-admin role is the default, and the viewer
role is unreachable.

Fix

Default UserRole.viewer.
Add CIRCA_ALLOWED_EMAILS and/or CIRCA_ALLOWED_EMAIL_DOMAINS; reject logins matching neither
before the user row is created.
Better still, create new users with is_active=False and require an admin to activate.
get_current_user already enforces is_active, so the mechanism exists and is unused.
Audit the existing user table for accounts that should not be there.

Done when

New users default to viewer
An allowlist gates account creation, and a rejected login creates no user row
Existing accounts have been reviewed and any unexpected ones removed
Tests cover the default role and the allowlist rejection path

References

backend/app/models/models.py:162
backend/app/repositories/users.py:38-50
backend/app/api/routes/auth.py:52-57
docs/circa-spec.md §4.2

Related: #22 builds out the full role model; this issue is the live vulnerability underneath it.

## Severity: CRITICAL ## The bug `backend/app/models/models.py:162`: ```python role: Mapped[UserRole] = mapped_column(Enum(UserRole), nullable=False, default=UserRole.reviewer) ``` `upsert_from_provider` (`backend/app/repositories/users.py:38-50`) creates users with no role argument, so every account that completes OAuth becomes a **reviewer**. There is no allowlist, no approval step, no pending state, and no endpoint anywhere to change a user's role. `reviewer` is not a limited role in this application. It can create decisions on any photo, overwrite notes on any photo, create manual evidence, supersede any evidence row by id, comment, enqueue jobs, and reach `POST /api/ingest` — which is the arbitrary-write primitive in the companion issue. **"Can log in" currently equals "can destroy the collection."** ## Context for this deployment The IdP is self-hosted Authentik, so the exposure is bounded by Authentik's registration policy rather than being open to the whole internet. That reduces severity from the worst case but does not remove it: it still means *anyone who can register with Authentik gets write access to the family archive*, which is almost certainly not the intent, and it couples two systems whose trust levels should differ. ## Contradicts the spec `docs/circa-spec.md` §4.2 defines `viewer` as "read-only access to approved photos only" and states "all role assignments are managed by an Admin in the UI after bootstrap." The implementation inverts this — the most privileged non-admin role is the default, and the viewer role is unreachable. ## Fix - Default `UserRole.viewer`. - Add `CIRCA_ALLOWED_EMAILS` and/or `CIRCA_ALLOWED_EMAIL_DOMAINS`; reject logins matching neither **before** the user row is created. - Better still, create new users with `is_active=False` and require an admin to activate. `get_current_user` already enforces `is_active`, so the mechanism exists and is unused. - Audit the existing `user` table for accounts that should not be there. ## Done when - [ ] New users default to `viewer` - [ ] An allowlist gates account creation, and a rejected login creates no user row - [ ] Existing accounts have been reviewed and any unexpected ones removed - [ ] Tests cover the default role and the allowlist rejection path ## References - `backend/app/models/models.py:162` - `backend/app/repositories/users.py:38-50` - `backend/app/api/routes/auth.py:52-57` - `docs/circa-spec.md` §4.2 Related: #22 builds out the full role model; this issue is the live vulnerability underneath it.

claude-bot added this to the v0.1.1 milestone

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

claude-bot added the

bug

backend

security

labels

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

claude-bot referenced this issue

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

Implement multi-user roles and review attribution #22

rbrooks referenced this issue from a commit

2026-07-28 17:56:50 +00:00

Close the ingest RCE chain: sanitize filenames, deny by default

rbrooks closed this issue

2026-07-28 17:56:50 +00:00

claude-bot commented

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

Author

Fixed in 6268f9e. CI green.

What changed

Default role is now viewer (models.py). No migration is required: the column has no
server_default, so the default is applied client-side at INSERT. Existing rows keep whatever
role they already have — see the action item below.

Account creation is gated before any row is written (auth.py, before upsert_from_provider):

if not settings.is_email_allowed(email):
    raise HTTPException(403, "This account is not permitted to access this collection.")

Settings.is_email_allowed() denies by default. An address is permitted if it is in
CIRCA_ALLOWED_EMAILS, matches a domain in CIRCA_ALLOWED_EMAIL_DOMAINS, or appears in
CIRCA_FIRST_ADMIN_EMAILS — naming someone an admin plainly implies they may log in, and requiring
them to be listed twice would be a footgun.

With none of the three configured, logins are permitted in development and refused in
production. That keeps local setup frictionless while an unconfigured deployment fails closed
rather than open. Note the development fallback applies only to the unconfigured case: configuring
an allowlist enforces it everywhere, which is covered by a test.

Tests

backend/tests/test_access_control.py. Beyond the obvious cases:

Lookalike domains — attacker@evil-example.com and attacker@example.com.evil.net must not
satisfy an example.com allowlist
Case-insensitivity and whitespace tolerance in the configured lists
A leading @ on a domain entry is tolerated
The production-denies / development-permits split when nothing is configured
Promotion still works — the default is a floor, not a ceiling

Settings is constructed with every relevant field set explicitly, so a CIRCA_* variable in the
ambient environment or on CI cannot change a result.

Writing these caught a real subtlety worth recording: default= on a mapped column applies at
INSERT, not on attribute access. User().role is None until flushed, so the first version of
the role test passed vacuously against a broken implementation. It now goes through a session, with
a declarative check on User.__table__.c.role.default.arg alongside it.

Action item for you

Audit the existing user table. This fix stops new unauthorised accounts; it does not
demote anyone already provisioned as a reviewer. Worth running before the app goes back online:

SELECT email, role, is_active, created_at, last_login_at FROM user ORDER BY created_at;

Still open in this milestone: #73 (no admin-only routes exist, viewer role is unenforced for reads),
#60 (email_verified is not checked before the email grants admin), #8 (the authorization matrix
these changes should be regression-tested against).

**Fixed** in 6268f9e. CI green. ## What changed **Default role is now `viewer`** (`models.py`). No migration is required: the column has no `server_default`, so the default is applied client-side at INSERT. **Existing rows keep whatever role they already have** — see the action item below. **Account creation is gated before any row is written** (`auth.py`, before `upsert_from_provider`): ```python if not settings.is_email_allowed(email): raise HTTPException(403, "This account is not permitted to access this collection.") ``` `Settings.is_email_allowed()` denies by default. An address is permitted if it is in `CIRCA_ALLOWED_EMAILS`, matches a domain in `CIRCA_ALLOWED_EMAIL_DOMAINS`, or appears in `CIRCA_FIRST_ADMIN_EMAILS` — naming someone an admin plainly implies they may log in, and requiring them to be listed twice would be a footgun. **With none of the three configured**, logins are permitted in development and refused in production. That keeps local setup frictionless while an unconfigured deployment fails closed rather than open. Note the development fallback applies *only* to the unconfigured case: configuring an allowlist enforces it everywhere, which is covered by a test. ## Tests `backend/tests/test_access_control.py`. Beyond the obvious cases: - **Lookalike domains** — `attacker@evil-example.com` and `attacker@example.com.evil.net` must not satisfy an `example.com` allowlist - Case-insensitivity and whitespace tolerance in the configured lists - A leading `@` on a domain entry is tolerated - The production-denies / development-permits split when nothing is configured - Promotion still works — the default is a floor, not a ceiling `Settings` is constructed with every relevant field set explicitly, so a `CIRCA_*` variable in the ambient environment or on CI cannot change a result. Writing these caught a real subtlety worth recording: **`default=` on a mapped column applies at INSERT, not on attribute access.** `User().role` is `None` until flushed, so the first version of the role test passed vacuously against a broken implementation. It now goes through a session, with a declarative check on `User.__table__.c.role.default.arg` alongside it. ## Action item for you **Audit the existing `user` table.** This fix stops *new* unauthorised accounts; it does not demote anyone already provisioned as a reviewer. Worth running before the app goes back online: ```sql SELECT email, role, is_active, created_at, last_login_at FROM user ORDER BY created_at; ``` ## Related Still open in this milestone: #73 (no admin-only routes exist, viewer role is unenforced for reads), #60 (`email_verified` is not checked before the email grants admin), #8 (the authorization matrix these changes should be regression-tested against).

claude-bot referenced this issue

2026-07-28 20:40:47 +00:00

Require email_verified and make first-admin bootstrap run once #60

rbrooks referenced this issue from a commit

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

Stop exposing user ids; enforce the role matrix

claude-bot referenced this issue

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

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

claude-bot referenced this issue

2026-07-30 21:23:49 +00:00

Attacker-controlled images reach Pillow with no limits or isolation #65