No length limits on any request body #64

New issue

Closed

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

claude-bot commented

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

Severity: HIGH

The bug

No request body field has a length limit. Grepped every route module for max_length, constr,
Field(, min_length — zero matches. CreateCommentBody.body is a bare str; same for notes,
rationale, and evidence notes. The DB columns are Text (unbounded in SQLite) and the migration
adds no CheckConstraint. FastAPI/Starlette impose no default body cap either.

Impact

An authenticated user POSTs a 500 MB comment; it is buffered in memory and written to SQLite.
Repeat to exhaust disk — the same disk holding the photo archive and the database. Under WAL, a
full disk risks write failures across the app.

More insidiously, a few multi-megabyte comments make GET /api/photos/{id}/comments enormous,
and the frontend renders every comment unconditionally with no pagination
(ReviewWorkspacePage.tsx:311-316) — hanging every reviewer's browser on that photo. Comment
deletion is own-or-admin, so an admin must intervene.

Fix

body: str = Field(..., min_length=1, max_length=10_000) and equivalents on notes, rationale,
and evidence notes.
A global request-body size limit at the ASGI layer (and at the reverse proxy once #50 lands).
Paginate comments server-side and in the UI.

Done when

Every user-supplied text field has an explicit maximum length
A global body-size cap rejects oversized requests before buffering
Comments are paginated
Tests cover the over-length rejection

References

backend/app/api/routes/photos.py:198-200,225-228,289-294,388-394,467-468
frontend/src/pages/ReviewWorkspacePage.tsx:311-316

## Severity: HIGH ## The bug No request body field has a length limit. Grepped every route module for `max_length`, `constr`, `Field(`, `min_length` — zero matches. `CreateCommentBody.body` is a bare `str`; same for notes, rationale, and evidence notes. The DB columns are `Text` (unbounded in SQLite) and the migration adds no `CheckConstraint`. FastAPI/Starlette impose no default body cap either. ## Impact An authenticated user POSTs a 500 MB comment; it is buffered in memory and written to SQLite. Repeat to exhaust disk — the same disk holding the photo archive and the database. Under WAL, a full disk risks write failures across the app. More insidiously, a few multi-megabyte comments make `GET /api/photos/{id}/comments` enormous, and the frontend renders every comment unconditionally with no pagination (`ReviewWorkspacePage.tsx:311-316`) — hanging every reviewer's browser on that photo. Comment deletion is own-or-admin, so an admin must intervene. ## Fix - `body: str = Field(..., min_length=1, max_length=10_000)` and equivalents on notes, rationale, and evidence notes. - A global request-body size limit at the ASGI layer (and at the reverse proxy once #50 lands). - Paginate comments server-side and in the UI. ## Done when - [ ] Every user-supplied text field has an explicit maximum length - [ ] A global body-size cap rejects oversized requests before buffering - [ ] Comments are paginated - [ ] Tests cover the over-length rejection ## References - `backend/app/api/routes/photos.py:198-200,225-228,289-294,388-394,467-468` - `frontend/src/pages/ReviewWorkspacePage.tsx:311-316`

claude-bot added this to the v0.1.1 milestone

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

claude-bot added the

backend

security

labels

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

rbrooks referenced this issue from a commit

2026-07-28 20:33:20 +00:00

Harden the HTTP surface: CSRF, content sniffing, body limits, docs

rbrooks closed this issue

2026-07-28 20:33:21 +00:00

claude-bot commented

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

Author

Fixed in 791d117. CI green.

Two layers, because they address different failure modes.

Transport cap (BodySizeLimitMiddleware): 1 MiB for JSON, a separate larger allowance for
/api/ingest. Written as pure ASGI rather than BaseHTTPMiddleware specifically so it can stop
reading mid-stream — checking after the fact would still force the memory allocation the limit
exists to prevent. Declared Content-Length is rejected up front; a chunked body with no declared
length is counted during receive.

Honest limitation, documented in the code: for a chunked body the overrun is only detectable
mid-stream, when the app may already be producing a response, so a clean 413 is not always
injectable. The middleware disconnects instead. The request fails either way, and the property that
matters — the oversized payload is never fully buffered — holds.

Field ceilings: notes 20,000; rationale 10,000; comments 10,000 with min_length=1 so empty
comments are refused too.

Comment pagination is not done here — it belongs with the API work in #76 and the UI in v0.3.0.
The field ceiling removes the "one comment hangs every browser" case; a large number of comments
is still unpaginated.

**Fixed** in 791d117. CI green. Two layers, because they address different failure modes. **Transport cap** (`BodySizeLimitMiddleware`): 1 MiB for JSON, a separate larger allowance for `/api/ingest`. Written as pure ASGI rather than `BaseHTTPMiddleware` specifically so it can stop reading **mid-stream** — checking after the fact would still force the memory allocation the limit exists to prevent. Declared `Content-Length` is rejected up front; a chunked body with no declared length is counted during receive. Honest limitation, documented in the code: for a chunked body the overrun is only detectable mid-stream, when the app may already be producing a response, so a clean 413 is not always injectable. The middleware disconnects instead. The request fails either way, and the property that matters — the oversized payload is never fully buffered — holds. **Field ceilings**: notes 20,000; rationale 10,000; comments 10,000 with `min_length=1` so empty comments are refused too. Comment pagination is **not** done here — it belongs with the API work in #76 and the UI in v0.3.0. The field ceiling removes the "one comment hangs every browser" case; a large *number* of comments is still unpaginated.