phase: 83_chat_save_payload_limits
All verification criteria are confirmed green. Final report: **Phase 83 — final verification pass (all tasks already complete; verified, nothing to fix)** - Verified `app/schemas.py` carries every A1 cap (text/thinking 32 000, sources 20, suggestions 50×200, tools 50, SourceRef 120/1000/500, ToolCall 100/2000, messages ≤200) — boundary-only, `app/api/chats.py` untouched - Verified all test pins exist and pass: 32 unit boundary tests (both edges of every cap + round-trip), 4 oversized-422 integration pins (text/201-msgs/21-sources/PUT, each with nothing-stored/row-unchanged assertions), A3 SSE done-event pin at column maxima, dedicated E2E (anon 40 000-char POST → 422 + small save → 201) - `uv run pytest tests/unit/test_schemas.py -v` — 32 passed - `uv run pytest tests/integration/test_chats_api.py` — exit 0; A3 pin — passed - `uv run pytest tests/e2e/test_chat_save_payload_limits.py -v --no-cov` — 2 passed (isolation); `uv run pytest tests/e2e/test_chat_history.py -v --no-cov` — 5 passed - `uv run pytest` — exit 0 (~1 704 tests, 0 fail/skip); `uv run pytest --cov=app` — TOTAL 99%, `app/schemas.py` 100% (>90% ✓) - `uv run ruff check . && uv run pyright` — clean (0 errors) - `git diff --stat` — only `app/schemas.py`, 3 test files (+`test_chat_api.py` A3 pin, sanctioned by task 02), phase files; no `app/api/chats.py`/`alembic`/`frontend`/`pyproject`/`uv.lock` diff ✓ - All completion criteria met; commit + phase move left to the harness per pipeline rules (changes stay in working tree) - Deviation note: list caps use `max_length` instead of `max_items` — identical in pydantic 2.13 (`max_items` is a deprecated alias); both boundaries behaviorally pinned - Next pending phase: `84_docs_push_error_sanitization`
This commit is contained in:
@@ -1,65 +0,0 @@
|
||||
# Phase 83 — Bound the anonymous saved-chat payload sizes at the schema boundary
|
||||
|
||||
**Source:** `.agents/remediation_plan.md` SEC-05 (security audit 2026-09-07, severity Medium — "Unbounded payload sizes on the anonymous public chat-save endpoints")
|
||||
**Story:** n/a (security hardening — audit-derived, no user story)
|
||||
**Context:** `app/schemas.py` — `ChatMessage` (`who`/`text: Field(min_length=1)` **no max**/`sources: list[SourceRef] | None`/`suggestions: list[str] | None`/`thinking: str | None` **no max**/`tools: list[ToolCall] | None`/`stopped`, `extra="forbid"`), `SourceRef` (`source`/`path`/`title` — all bare `str`, **no max** — shared with the server-built `ChatDoneEvent.sources` SSE shape), `ToolCall` (`name: str`/`argument: str | None` — bare, **no max**), `SavedChatCreate` (`title: max_length=500`/`messages: Field(min_length=1)` **no max_items**/`share`), `SavedChatUpdate` (same message list) — consumed by the **public** (no-session) routes `POST /api/chats` and `PUT /api/chats/{chat_id}` in `app/api/chats.py` (phase 55 A1: the write surface is public — the row id is the credential). `ChatMessage` is shared with `HistoryTurn` (`text: max_length=32_000`, `thinking: max_length=32_000` — the existing caps to mirror) and with the server-built `ChatDoneEvent`/SSE shapes. The DB side: `documents.source String(120)`, `documents.path String(1000)`, `documents.title String(500)` (the column lengths the caps mirror), `saved_chats.messages` is JSONB (unbounded at the DB level — the pydantic boundary is the only gate). `tests/unit/` has no existing `test_schemas.py`; `tests/integration/test_chats_api.py` exists (the phase-50/51/53/55 contract pins — the 201/404/409/share matrix).
|
||||
|
||||
## Objective
|
||||
An anonymous `POST/PUT /api/chats` can no longer carry a single arbitrarily large string, an unbounded message list, or unbounded nested lists: every `ChatMessage`/`SourceRef`/`ToolCall`/messages-list field gets a pydantic cap (422 at the boundary, house style), sized to the realistic `bor.chat.v1` record the UI produces — while a normal save (the E2E suites' payloads) still lands 201 byte-for-byte.
|
||||
|
||||
## Audit basis (read this, not the chat)
|
||||
- `ChatMessage.text` has `min_length=1` and **no `max_length`** (unlike its sibling `HistoryTurn.text` at 32 000), and NO list field anywhere in the saved-chat shape has `max_items` — an anonymous caller can POST one 200 MB JSON string (`{"messages":[{"who":"user","text":"aaa…"}]}`) and create unlimited such rows: an unauthenticated memory + storage DoS on the app's only anonymous write surface (audit PoC in the plan).
|
||||
- The fix is boundary-only (the phase-56 house style: fail loud at the schema with a 422 — no route code changes at all; FastAPI's request validation rejects before the handler runs, nothing is stored).
|
||||
- The caps mirror the EXISTING `HistoryTurn` caps for the text fields (32 000 — a single chat message longer than that is already rejected on the chat path, so a saved chat can never legitimately carry more) and the DB column lengths for the source-ref fields (a `SourceRef` is built from `documents` rows server-side — `source ≤120`, `path ≤1000`, `title ≤500` — so the SSE `done` event can never trip the new caps: the server's own values always fit).
|
||||
|
||||
## Owner decisions (chat, 2026-09-07 — recorded per AGENTS.md rule 3)
|
||||
- **A1 — the exact caps** (pydantic boundary, 422 on overflow):
|
||||
- `ChatMessage.text`: `max_length=32_000` (mirror `HistoryTurn.text`);
|
||||
- `ChatMessage.thinking`: `max_length=32_000` (mirror `HistoryTurn.thinking`);
|
||||
- `ChatMessage.sources`: `max_items=20` (top-N docs + agent reads — the UI shows a handful; 20 is 10× the realistic max);
|
||||
- `ChatMessage.suggestions`: `max_items=50`, each item `max_length=200` (chips are short deterministic strings — `derive_suggestions` produces ≤ ~80 chars);
|
||||
- `ChatMessage.tools`: `max_items=50` (one entry per tool call; the round cap is 10 and even generous multi-call turns stay far below 50);
|
||||
- `SourceRef`: `source max_length=120`, `path max_length=1000`, `title max_length=500` (mirror the `documents` column lengths — server-built SSE values always fit);
|
||||
- `ToolCall`: `name max_length=100`, `argument max_length=2000 | None` (the combined `source/path` identity is ≤ 120 + 1 + 1000; 2 000 is 2× headroom for a grep pattern);
|
||||
- `SavedChatCreate.messages` / `SavedChatUpdate.messages`: `max_items=200` (well past any realistic conversation — the chat history budget itself is 40 turns — and far below a DoS-sized list).
|
||||
- **A2 — boundary-only:** NO route/handler changes in `app/api/chats.py` (FastAPI validates the pydantic model before the handler — the 422 is the framework's standard validation response); the stored-row contract (JSONB round-trip, `extra="forbid"`, the null-safe restore path) is untouched.
|
||||
- **A3 — `SourceRef` is shared:** the caps on `SourceRef` also constrain the client-side `sources` inside saved chats AND are satisfied by every server-built `ChatDoneEvent.sources` (column-length mirror) — the SSE path is provably unaffected (pinned by an integration test).
|
||||
|
||||
## Design (shared by all tasks — the executor reads this, not the chat)
|
||||
- **`app/schemas.py`** — the ONLY file changed in `app/`:
|
||||
- `SourceRef`: `source: str = Field(max_length=120)`, `path: str = Field(max_length=1000)`, `title: str = Field(max_length=500)` (docstring: caps mirror the `documents` column lengths — server-built SSE refs always fit; client-saved refs are bounded at the boundary).
|
||||
- `ToolCall`: `name: str = Field(max_length=100)`, `argument: str | None = Field(default=None, max_length=2000)`.
|
||||
- `ChatMessage`: `text: str = Field(min_length=1, max_length=32_000)`, `thinking: str | None = Field(default=None, max_length=32_000)`, `sources: list[SourceRef] | None = Field(default=None, max_items=20)`, `suggestions: list[str] | None = Field(default=None, max_items=50)` with item length enforced by `Field(max_length=200)` on the list item type (pydantic v2: annotate `list[Annotated[str, Field(max_length=200)]]` or a small `_Chip = Annotated[str, Field(max_length=200)]` alias — keep the JSON shape identical), `tools: list[ToolCall] | None = Field(default=None, max_items=50)`.
|
||||
- `SavedChatCreate.messages` / `SavedChatUpdate.messages`: `Field(min_length=1, max_items=200)`.
|
||||
- Update the affected docstrings: each cap's one-line rationale (the A1 mirror sources) — the file's dense-docstring house style.
|
||||
- **Not touched:** `app/api/chats.py` (validation happens before the handlers — zero route diff), the frontend (the UI's real payloads are far inside every cap — the E2E save suites prove it), the DB (no migration — JSONB stays unbounded at rest; the boundary is the gate, A2).
|
||||
- **422 shape:** FastAPI's standard validation error body (the house boundary response — same shape the existing `min_length` violations already produce; no custom error copy).
|
||||
|
||||
## Dependencies
|
||||
— (none; boundary-only hardening on the completed phase-50/51/55 saved-chat surface — the 201/404/409/share contract is unchanged for in-cap payloads)
|
||||
|
||||
## Tasks
|
||||
1. `01_schema_caps.md` — the `app/schemas.py` cap changes + the unit suite (one test per cap, boundary values included).
|
||||
2. `02_integration_and_e2e.md` — the oversized-422 integration pins + the dedicated Playwright suite + the SSE `done`-event unaffected pin.
|
||||
3. `03_verify_and_commit.md` — full gate (incl. the existing chat-save E2E suites) + atomic commit.
|
||||
|
||||
## Testing & Quality
|
||||
- Unit — `tests/unit/test_schemas.py` (new; the first schema-boundary suite): for EACH cap — a value exactly at the cap validates; one past it raises a pydantic `ValidationError` naming the field (`text` 32_000/32_001, `thinking` same, `sources` 20/21 items, `suggestions` 50/51 items + a 200/201-char item, `tools` 50/51, `SourceRef` source/path/title 120/1000/500 boundaries, `ToolCall` name 100/101 + argument 2000/2001, `SavedChatCreate.messages` 200/201 items, `SavedChatUpdate.messages` 200/201) — plus a regression pin: a realistic `bor.chat.v1` payload (a few messages, sources, tools, thinking) validates cleanly and round-trips `model_dump()` (the stored-shape contract).
|
||||
- Integration — `tests/integration/test_chats_api.py` (extend, existing pins intact): anonymous `POST /api/chats` with a 32_001-char text → **422** AND no row created (list stays the same length); `messages` with 201 items → 422; a 21-item `sources` list → 422; the existing 201 create / share / unshare / stale pins green; an SSE regression pin: a `ChatDoneEvent` built from a full-length `Document` row (source 120 / path 1000 / title 500 — construct the row values at the column maxima) still serializes (A3: the server-built refs fit the new caps — build the event and `model_dump()` it in an integration test next to the existing chat pins).
|
||||
- E2E (dedicated suite — this phase's contract is reachable from a browser's network layer): `tests/e2e/test_chat_save_payload_limits.py` — using Playwright's API request context against the running app (the house pattern — `page.request` or the context's request API): anonymous `POST /api/chats` with an oversized text (e.g. 40_000 chars) → **422** (no session needed — the surface is public, exactly the audit vector); a normal small save → 201 + `id` in the body (the happy path still works end-to-end). Run in isolation per AGENTS.md rule 9.
|
||||
- Regression E2E: the existing save flows stay green — `tests/e2e/test_chat_history.py` (save/restore through the real UI — the in-cap proof).
|
||||
- Coverage: **>90%** on `app/` (the changed file is `app/schemas.py` — declarative, exercised by every unit/integration test).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] Every A1 cap is pinned at both boundaries (at-cap passes, over-cap 422s) in `tests/unit/test_schemas.py`.
|
||||
- [ ] Anonymous oversized `POST /api/chats` → 422 with **no row stored** (integration pin); the same vector from the Playwright suite → 422 (E2E pin).
|
||||
- [ ] The SSE `done`-event pin passes at the column-maximum source-ref lengths (A3).
|
||||
- [ ] `uv run pytest tests/e2e/test_chat_save_payload_limits.py -v --no-cov` green in isolation; `uv run pytest tests/e2e/test_chat_history.py -v --no-cov` green (real UI saves unaffected).
|
||||
- [ ] `uv run pytest` green; `uv run pytest --cov=app --cov-report=term-missing` >90%; `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] `git diff --stat` limited to `app/schemas.py`, the three test files, phase files (NO `app/api/chats.py` diff, no migration, no `pyproject.toml`/`uv.lock`/`frontend/` diff).
|
||||
- [ ] One atomic `--no-gpg-sign` commit (e.g. `fix(api): bound anonymous saved-chat payload sizes at the schema boundary`); phase dir moved to `.agents/phases/complete/`.
|
||||
|
||||
## Locked decisions
|
||||
- **A2 boundary-only** — no route/handler/migration change; the pydantic 422 IS the control (house style, phase 56 precedent).
|
||||
- **Caps mirror existing invariants** (A1) — `HistoryTurn`'s 32 000 text cap and the `documents` column lengths are the sizing sources, so no legitimate payload (chat-path history OR server-built SSE refs) can ever trip a cap; only oversized anonymous input does.
|
||||
- **JSONB stored shape untouched** — `extra="forbid"` and the null-safe round-trip contract (the phase-14/50 restore path) are byte-identical for in-cap payloads (the unit round-trip pin).
|
||||
@@ -1,32 +0,0 @@
|
||||
# Task 01 — The schema caps + unit boundary suite
|
||||
|
||||
**Phase:** `83_chat_save_payload_limits` · **Story:** n/a (security hardening — audit SEC-05)
|
||||
|
||||
## Objective
|
||||
`app/schemas.py` carries every A1 cap from the phase overview, and `tests/unit/test_schemas.py` pins each cap at both boundaries plus the realistic-payload round-trip regression.
|
||||
|
||||
## Work
|
||||
1. `app/schemas.py` — apply the caps exactly as the phase overview's design block lists them:
|
||||
- `SourceRef` — `source`/`path`/`title` → `Field(max_length=120/1000/500)`;
|
||||
- `ToolCall` — `name` → `Field(max_length=100)`, `argument` → `Field(default=None, max_length=2000)`;
|
||||
- `ChatMessage` — `text` → `Field(min_length=1, max_length=32_000)`, `thinking` → `Field(default=None, max_length=32_000)`, `sources` → `Field(default=None, max_items=20)`, `suggestions` → a list of `_Chip` (`Annotated[str, Field(max_length=200)]`) with `Field(default=None, max_items=50)`, `tools` → `Field(default=None, max_items=50)`;
|
||||
- `SavedChatCreate.messages` + `SavedChatUpdate.messages` → `Field(min_length=1, max_items=200)`;
|
||||
- docstrings: one rationale line per cap group (the A1 mirror sources — `HistoryTurn` caps / `documents` column lengths), matching the file's dense style.
|
||||
- Verify no JSON-shape change: the models still accept/reject exactly the same KEYS (`extra="forbid"` untouched) — only value bounds are added.
|
||||
2. `tests/unit/test_schemas.py` (new) — one test per cap at BOTH boundaries (at-cap validates; one-over raises `ValidationError` — assert the failing field name via `e.errors()[0]["loc"]`):
|
||||
- `ChatMessage.text` 32_000 / 32_001; `thinking` 32_000 / 32_001 (and `None` still valid);
|
||||
- `sources` 20 / 21 items; `suggestions` 50 / 51 items + a single 200 / 201-char item; `tools` 50 / 51;
|
||||
- `SourceRef.source` 120 / 121, `.path` 1000 / 1001, `.title` 500 / 501;
|
||||
- `ToolCall.name` 100 / 101, `.argument` 2000 / 2001 (and `None` still valid);
|
||||
- `SavedChatCreate.messages` 200 / 201 items, `SavedChatUpdate.messages` 200 / 201;
|
||||
- the realistic-payload regression: a full `bor.chat.v1`-shaped `SavedChatCreate` (4–8 messages mixing user/brain, one brain message with `thinking` + `tools` + `sources`, one with `suggestions` + `stopped`) → validates, and `model_dump()` of the messages equals the input dict (the stored-shape round-trip contract, `None`-keys preserved).
|
||||
|
||||
## Testing & Quality
|
||||
- `uv run pytest tests/unit/test_schemas.py -v` green.
|
||||
- Coverage: **>90%** on `app/schemas.py` (declarative — exercised by every test in the new file).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest tests/unit/test_schemas.py -v` — every boundary test green (at-cap passes, over-cap 422-shaped `ValidationError` with the right `loc`).
|
||||
- [ ] The round-trip regression test green (stored shape unchanged for in-cap payloads).
|
||||
- [ ] `uv run pytest tests/unit/ -q` green (no unit regression — in particular any test that constructs `ChatMessage`/`SourceRef` values still passes).
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -1,31 +0,0 @@
|
||||
# Task 02 — Integration pins + the dedicated Playwright suite
|
||||
|
||||
**Phase:** `83_chat_save_payload_limits` · **Story:** n/a (security hardening — audit SEC-05)
|
||||
|
||||
## Objective
|
||||
The HTTP contract is pinned: anonymous oversized saves 422 and store nothing, the SSE `done`-event shape is provably unaffected at the column maxima, and the dedicated E2E suite drives the exact audit vector through a real browser's network layer.
|
||||
|
||||
## Work
|
||||
1. `tests/integration/test_chats_api.py` — extend (every existing pin stays green; if the file's fixtures reset the DB between tests, the new tests follow the same pattern):
|
||||
- anonymous `POST /api/chats` with one message whose `text` is 32_001 chars → **422** AND the subsequent admin `GET /api/chats` list length is unchanged (nothing stored);
|
||||
- `POST /api/chats` with 201 messages (minimal valid each) → 422;
|
||||
- `POST /api/chats` with one message carrying a 21-item `sources` list (valid `SourceRef` shapes) → 422;
|
||||
- `PUT /api/chats/{id}` (an existing row) with an oversized message → 422 and the row content unchanged (a GET shows the original text);
|
||||
- the happy path regression: a normal small save → 201 (the existing pins already cover this — confirm green).
|
||||
2. **The A3 SSE pin** — in the same file (or the existing chat-API integration file where the `ChatDoneEvent` shape is already exercised — `tests/integration/test_chat_api.py`): build the event from maximum-length values — `SourceRef(source="s"*120, path="p"*1000, title="t"*500)` inside a `ChatDoneEvent(deflected=False, sources=[…], suggestions=[])` → `model_dump()` succeeds (the server-built refs fit the new caps; a failure here would mean the caps broke the SSE contract).
|
||||
3. `tests/e2e/test_chat_save_payload_limits.py` (new) — house E2E conventions (read `tests/e2e/conftest.py` + `test_chat_history.py` for the server/client fixtures):
|
||||
- the audit vector, end-to-end and anonymous (NO login): via the Playwright request API (`page.request.post("/api/chats", data={...})` on a fresh page) POST one message with a 40_000-char `text` → expect **422**;
|
||||
- the happy path in the same suite: a small valid save → **201** with an `id` field (proves the boundary didn't break the real flow from the browser layer);
|
||||
- no LLM dependency (the endpoints are DB-only).
|
||||
|
||||
## Testing & Quality
|
||||
- `uv run pytest tests/integration/test_chats_api.py -v` green (new + existing).
|
||||
- The A3 pin green wherever it lands.
|
||||
- `uv run pytest tests/e2e/test_chat_save_payload_limits.py -v --no-cov` green **in isolation** (this phase's mandatory E2E).
|
||||
- Coverage: **>90%** on `app/` (no new `app/` code — the gate is regression + boundary proof).
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] All four oversized-422 integration pins pass (text / message-count / sources / PUT), each with the "nothing stored / row unchanged" assertion.
|
||||
- [ ] The A3 SSE pin passes at the column-maximum lengths.
|
||||
- [ ] The dedicated E2E suite green in isolation (422 anonymous + 201 small save).
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
@@ -1,28 +0,0 @@
|
||||
# Task 03 — Full gate + atomic commit
|
||||
|
||||
**Phase:** `83_chat_save_payload_limits` · **Story:** n/a (security hardening — audit SEC-05)
|
||||
|
||||
## Objective
|
||||
Run the complete phase gate (including the real-UI save regression), land the phase as one atomic commit, and move the phase directory to `complete/`.
|
||||
|
||||
## Work
|
||||
1. **Full regression gate** (AGENTS.md rule 9):
|
||||
- `uv run pytest` — unit + integration green.
|
||||
- `uv run pytest --cov=app --cov-report=term-missing` — `app/` coverage **>90%**.
|
||||
- `uv run pytest tests/e2e/test_chat_save_payload_limits.py -v --no-cov` — green **in isolation** (this phase's E2E).
|
||||
- `uv run pytest tests/e2e/test_chat_history.py -v --no-cov` — green (the real UI save/restore flow with in-cap payloads — the no-regression proof for the surface the caps sit on).
|
||||
- `uv run ruff check . && uv run pyright` — clean.
|
||||
2. **Commit** (AGENTS.md rule 8 — one atomic, Conventional-Commits commit, always `--no-gpg-sign`), staging `app/schemas.py`, `tests/unit/test_schemas.py`, `tests/integration/test_chats_api.py` (+ the chat-API file if the A3 pin landed there), `tests/e2e/test_chat_save_payload_limits.py`, and the phase files:
|
||||
`fix(api): bound anonymous saved-chat payload sizes at the schema boundary`
|
||||
— body: security audit SEC-05 (2026-09-07) — `POST/PUT /api/chats` are public (phase 55) and every `ChatMessage`/list field was unbounded, so an anonymous caller could store arbitrarily large JSONB rows (memory + storage DoS). Pydantic caps at the boundary (text/thinking 32 000 mirroring `HistoryTurn`, `SourceRef` mirroring the `documents` column lengths, list `max_items`, `messages` ≤ 200) → 422 on overflow, nothing stored; the 201/404/share contract and the stored JSONB shape are unchanged for in-cap payloads (round-trip pin + real-UI E2E regression). Boundary-only: no route, migration, or frontend change.
|
||||
3. Move the phase directory: `mv .agents/phases/todo/83_chat_save_payload_limits .agents/phases/complete/` and include the move in the same commit.
|
||||
|
||||
## Testing & Quality
|
||||
- This task IS the phase-level gate — the commands above are the completion evidence.
|
||||
- Coverage: >90% held.
|
||||
|
||||
## Completion Criteria
|
||||
- [ ] `uv run pytest` green; coverage >90%; both E2E suites green (the dedicated one in isolation, `test_chat_history.py` as the regression).
|
||||
- [ ] `uv run ruff check . && uv run pyright` clean.
|
||||
- [ ] Exactly one new commit; `git show --stat HEAD` lists the files above + the phase files (todo → complete move) — in particular NO `app/api/chats.py`, NO `alembic/`, NO `frontend/`, NO `pyproject.toml`/`uv.lock`.
|
||||
- [ ] `.agents/phases/complete/83_chat_save_payload_limits/` exists; `todo/` no longer contains it.
|
||||
Reference in New Issue
Block a user