rbrooks/Circa

Watch

Fork 0

API discloses other users UUIDs, enabling session forgery #57

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 (amplifier for the default-secret-key issue)

The bug

The session cookie contains only a user UUID, signed with CIRCA_SECRET_KEY. Separately, the
API returns other users' UUIDs to any authenticated caller:

_evidence_to_dict → created_by (photos.py:271)
_decision_to_dict → created_by (photos.py:372)
_comment_to_dict → created_by (photos.py:450)
_job_to_dict → created_by (jobs.py:41)

The frontend even renders c.created_by.slice(0, 8) as the comment author label.

Why it matters

On its own, a known signing key is not immediately exploitable if the attacker must guess a
UUIDv4. This disclosure removes that obstacle entirely and turns it into a two-step operation:

Authenticate (trivially, per the auto-provisioning issue)
GET /api/photos/{id}/decisions → collect created_by values. Decisions are made by
reviewers and admins, so the admin's UUID is very likely present.
Sign a cookie for that UUID locally with the known key → authenticated as admin.

This is what converts the default CIRCA_SECRET_KEY from poor hygiene into practical admin
takeover, and it is the piece not described in #14.

Fix

Stop returning raw user identifiers. Resolve created_by to a display name at the serializer
boundary — which is needed for the attribution work anyway — and return an opaque or omitted id.
If a client-side identifier is genuinely required (e.g. "is this my comment?"), return a
per-collection opaque id or a boolean is_own rather than the primary key.

Note this also improves the UI: the review workspace currently shows UUID fragments as author
labels, which is developer debris in a family member's tool.

Done when

No API response contains another user's primary-key UUID
Comment, decision, evidence, and job serializers return display names
"Is this mine?" logic still works without exposing ids
A test asserts no created_by UUID appears in any response body

References

backend/app/api/routes/photos.py:271,372,450; jobs.py:41
frontend/src/pages/ReviewWorkspacePage.tsx:313

Related: #14 (fail fast on insecure config), #22 (attribution display names).

## Severity: CRITICAL (amplifier for the default-secret-key issue) ## The bug The session cookie contains only a user UUID, signed with `CIRCA_SECRET_KEY`. Separately, the API returns **other users' UUIDs** to any authenticated caller: - `_evidence_to_dict` → `created_by` (`photos.py:271`) - `_decision_to_dict` → `created_by` (`photos.py:372`) - `_comment_to_dict` → `created_by` (`photos.py:450`) - `_job_to_dict` → `created_by` (`jobs.py:41`) The frontend even renders `c.created_by.slice(0, 8)` as the comment author label. ## Why it matters On its own, a known signing key is not immediately exploitable if the attacker must guess a UUIDv4. This disclosure removes that obstacle entirely and turns it into a two-step operation: 1. Authenticate (trivially, per the auto-provisioning issue) 2. `GET /api/photos/{id}/decisions` → collect `created_by` values. Decisions are made by reviewers and admins, so the admin's UUID is very likely present. 3. Sign a cookie for that UUID locally with the known key → authenticated as admin. This is what converts the default `CIRCA_SECRET_KEY` from poor hygiene into practical admin takeover, and it is the piece not described in #14. ## Fix Stop returning raw user identifiers. Resolve `created_by` to a display name at the serializer boundary — which is needed for the attribution work anyway — and return an opaque or omitted id. If a client-side identifier is genuinely required (e.g. "is this my comment?"), return a per-collection opaque id or a boolean `is_own` rather than the primary key. Note this also improves the UI: the review workspace currently shows UUID fragments as author labels, which is developer debris in a family member's tool. ## Done when - [ ] No API response contains another user's primary-key UUID - [ ] Comment, decision, evidence, and job serializers return display names - [ ] "Is this mine?" logic still works without exposing ids - [ ] A test asserts no `created_by` UUID appears in any response body ## References - `backend/app/api/routes/photos.py:271,372,450`; `jobs.py:41` - `frontend/src/pages/ReviewWorkspacePage.tsx:313` Related: #14 (fail fast on insecure config), #22 (attribution display names).

claude-bot added this to the v0.1.1 milestone

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

claude-bot added the

backend

security

labels

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

claude-bot referenced this issue

2026-07-28 06:03:16 +00:00

Surface display names instead of UUID fragments #107

claude-bot referenced this issue

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

Fail fast on insecure configuration in production #14

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-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:46 +00:00

Author

Fixed in 5f2cbdb. 144 tests passing.

New app/api/attribution.py. Evidence, decision, comment, and job responses now carry
created_by_display_name and is_own instead of the raw user id.

Two design points worth recording:

Batched resolution. resolve_display_names() collects the distinct actor ids from a result set
and issues one query, rather than resolving per row. The performance audit specifically noted
that the projection pattern means there are currently no N+1 patterns anywhere in the API — this
fix does not introduce the first one.

is_own instead of comparing ids client-side. The UI needs to know "is this my comment?" to
offer deletion. Returning a boolean answers that without handing back an identifier, which is the
whole point of the issue.

Unresolvable actors degrade to "Unknown" and system-generated rows to "System", both without
leaking the id. Tested including the deleted-user case and an end-to-end assertion that the
reviewer's UUID appears nowhere in a comments response body.

This also does the security half of #107 (display names in the UI). #107 remains open for the
frontend work — rendering the names, and the decision history that currently shows no author at all.

**Fixed** in 5f2cbdb. 144 tests passing. New `app/api/attribution.py`. Evidence, decision, comment, and job responses now carry `created_by_display_name` and `is_own` instead of the raw user id. Two design points worth recording: **Batched resolution.** `resolve_display_names()` collects the distinct actor ids from a result set and issues **one** query, rather than resolving per row. The performance audit specifically noted that the projection pattern means there are currently no N+1 patterns anywhere in the API — this fix does not introduce the first one. **`is_own` instead of comparing ids client-side.** The UI needs to know "is this my comment?" to offer deletion. Returning a boolean answers that without handing back an identifier, which is the whole point of the issue. Unresolvable actors degrade to `"Unknown"` and system-generated rows to `"System"`, both without leaking the id. Tested including the deleted-user case and an end-to-end assertion that the reviewer's UUID appears nowhere in a comments response body. This also does the security half of #107 (display names in the UI). #107 remains open for the frontend work — rendering the names, and the decision history that currently shows no author at all.

rbrooks referenced this issue from a commit

2026-08-03 14:51:44 +00:00

Refuse to start on an insecure production configuration

claude-bot referenced this issue

2026-08-04 19:52:26 +00:00

Surface display names instead of UUID fragments #107

rbrooks referenced this issue from a commit

2026-08-04 20:21:21 +00:00

Say who did what, and stop printing identifiers at people