diff --git a/issues/001-csrf-behind-reverse-proxy.md b/issues/001-csrf-behind-reverse-proxy.md deleted file mode 100644 index ba28e14..0000000 --- a/issues/001-csrf-behind-reverse-proxy.md +++ /dev/null @@ -1,71 +0,0 @@ -# 001 — CSRF verification fails behind a TLS-terminating reverse proxy - -**Status:** Fixed - -## Description - -Deploying Daybook behind Apache (TLS-terminating reverse proxy → plain HTTP -to the container) on `cloud-vm-1`, login failed with: - -``` -Forbidden (403) -CSRF verification failed. Request aborted. -``` - -**Root cause:** `daybook_project/settings.py` set `ALLOWED_HOSTS` from an env -var but never set `CSRF_TRUSTED_ORIGINS` or `SECURE_PROXY_SSL_HEADER`. Without -the latter, Django has no way to know the original request was HTTPS — the -proxy terminates TLS and forwards plain HTTP — so it treats every request as -insecure, which breaks CSRF checks on any POST (including login) once a -browser connects over `https://`. - -This isn't specific to `cloud-vm-1` — it'll happen on any reverse-proxied -deployment (Apache, Nginx, Traefik, a cloud load balancer), which is the -explicitly recommended setup in `deploy/cloud_agent_deploy.md` §8. The -runbook doesn't mention CSRF or proxy-awareness anywhere. - -## Fix - -**App (`daybook_project/settings.py`):** - -```python -CSRF_TRUSTED_ORIGINS = [origin.strip() for origin in os.environ.get("DJANGO_CSRF_TRUSTED_ORIGINS", "").split(",") if origin.strip()] -SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https") -``` - -Committed locally as `d458cd3` ("Fix CSRF failure behind a TLS-terminating -reverse proxy") — **needs to be pushed to `main`**, the deploy agent that -made this fix didn't have push access to the Forgejo repo. - -**Deploy env (`deploy/env.example` / runbook):** needs a new var, -`DJANGO_CSRF_TRUSTED_ORIGINS`, set to the public HTTPS origin, e.g.: - -``` -DJANGO_CSRF_TRUSTED_ORIGINS=https://daybook.example.com -``` - -**Proxy config:** `SECURE_PROXY_SSL_HEADER` only works if the proxy actually -sends that header. The Apache example in `cloud_agent_deploy.md` §8 is -missing it — needs: - -```apache -RequestHeader set X-Forwarded-Proto "https" -``` - -inside the `` block (requires `a2enmod headers`). The -equivalent for Nginx is `proxy_set_header X-Forwarded-Proto $scheme;`, worth -adding to `docs/deployment.md`'s reverse-proxy notes too since that doc -mentions Nginx/Traefik as options without this caveat. - -## Resolution - -- [x] Pushed commit `d458cd3` to `main`. -- [x] Added `DJANGO_CSRF_TRUSTED_ORIGINS` to `deploy/env.example` with an - explanatory comment. -- [x] Updated `cloud_agent_deploy.md` §8's Apache example to include - `RequestHeader set X-Forwarded-Proto "https"`, plus a Caddy equivalent - and a checklist item that verifies by actually logging in through the - proxy (not just curling `/api/capabilities`, which is unauthenticated - GET and wouldn't have caught this). -- [x] Updated `docs/deployment.md`'s "Reverse Proxy Notes" and checklist with - the same requirement, plus an Nginx/Apache/Caddy header cheat sheet. diff --git a/issues/002-env-example-not-actually-loaded.md b/issues/002-env-example-not-actually-loaded.md deleted file mode 100644 index 6b09c4e..0000000 --- a/issues/002-env-example-not-actually-loaded.md +++ /dev/null @@ -1,54 +0,0 @@ -# 002 — `.env.example` implies persistent config that `daybook_post.py` never loads - -**Status:** Fixed - -## Description - -`skills/daybook-posting/.env.example` ships alongside the posting helper: - -``` -DAYBOOK_URL=http://127.0.0.1:8001 -DAYBOOK_TOKEN=your-personal-api-token -DAYBOOK_SOURCE=agent -``` - -The presence of a `.env.example` file conventionally means "copy this to -`.env`, fill it in, and the tool picks it up automatically." That's not -true here. `daybook_post.py` is intentionally zero-dependency (no -`python-dotenv`, no manual `.env` parsing) and only reads real process -environment variables via `os.environ.get(...)` (confirmed: the only -`.env`-related string in the whole `skills/` tree is this example file -itself — nothing in `daybook_post.py` reads it). - -Found while doing a first interactive posting session against the `test` -account from a sibling project (`AGENT_DAYBOOK__LLM_TEST`): copying -`.env.example` to `.env` and populating it did nothing; the script still -required the vars to be exported into the shell (or set with `set`/ -`$env:` on Windows) for every new session/terminal. Neither `SKILL.md` -nor `docs/llm_posting_helper.md` mention this — both just say "use -environment variables" without clarifying that a `.env` file specifically -will not work unless the caller's shell/agent harness sources it itself. - -This is minor but wastes a round trip for exactly the audience this skill -targets (an agent following the docs literally). - -## Suggested Fix (pick one) - -- Simplest: delete `.env.example`, or rename/annotate it clearly as "copy - these lines into your shell profile / CI secrets / agent harness env - config — this file itself is not read automatically." -- Alternative: add ~10 lines of manual `.env` parsing to `daybook_post.py` - (still zero third-party deps — just read `.env` in the working directory - if present, without overriding already-set real env vars) if the goal - really is per-project persistent config. - -## Resolution - -Took the simplest option: deleted `skills/daybook-posting/.env.example` -entirely rather than adding `.env` parsing to a helper that's deliberately -zero-dependency. Its content was already duplicated in both `SKILL.md` and -`docs/llm_posting_helper.md`'s Configuration sections, so nothing was lost. -Also reworded both of those sections to say explicitly that these must be -real process/shell environment variables — the helper reads `os.environ` -directly and has no `.env` file support — rather than just "use environment -variables" without ruling out the natural `.env`-file assumption. diff --git a/issues/003-model-max-length-silently-unenforced.md b/issues/003-model-max-length-silently-unenforced.md deleted file mode 100644 index 2ca9996..0000000 --- a/issues/003-model-max-length-silently-unenforced.md +++ /dev/null @@ -1,68 +0,0 @@ -# 003 — `CharField(max_length=...)` limits are silently unenforced on ingest - -**Status:** Fixed - -## Description - -`artifacts/models.py` declares length caps on several `Artifact` fields — -`source` (80), `title` (200), `source_name` (120), `source_id` (120), -`theme` (40) — but nothing on the ingest path enforces them. `views.py`'s -`create_text_artifact`/`create_binary_artifact` call -`Artifact.objects.create(...)` directly, which does **not** run Django's -`full_clean()`/validators. SQLite (the DB in both dev and production per -`docs/deployment.md` line 197) also doesn't enforce `VARCHAR(n)` length as -a hard constraint on `INSERT`, so oversized values are written and read -back verbatim with no error at any layer. - -Confirmed against the local `test` account: - -1. `POST /api/artifacts` with a 250-character `title` (limit 200) → - `201`, title stored and returned in full, uncut. -2. `POST /api/artifacts` with a 150-character `source` (limit 80) → - `201`, `source` stored in full. Because `source_name` defaults to - `source` when omitted (`source_fields()` in `views.py`), this also - silently blew past `source_name`'s own 120-char limit in the same - request. - -For contrast, other validation on the same endpoint works correctly and -returns a clean `400` — e.g. an invalid `tag_colors` hex value -(`{"tag_colors": {"urgent": "notacolor"}}` → `400` with a helpful -message). So the endpoint clearly has a validation convention; these -five fields just aren't part of it. - -Practically: a title long enough could break feed-row layout in the UI; -`source` feeds directly into the on-disk storage path -(`artifacts_store/{source}/{day}/...`) and the sidebar/label system, so -an extreme value there has a larger blast radius than just cosmetics. - -## Reproduction - -```bash -curl -X POST http://127.0.0.1:8001/api/artifacts \ - -H "Authorization: Bearer " \ - -H "Content-Type: application/json" \ - -d "{\"source\":\"edge-case-test\",\"title\":\"$(python3 -c 'print("A"*250)')\",\"kind\":\"markdown\",\"content\":\"x\"}" -# -> 201, title returned uncut at 250 chars -``` - -## Suggested Fix - -Add explicit length checks alongside the existing required-field/kind -checks in `ArtifactListCreateView.post` and `ArtifactDetailView.patch` -(and the two `create_*_artifact` helpers), returning `400` with a message -naming the field and its limit — consistent with how `tag_colors` and -`source_type` are already handled. A shared `validate_field_lengths(data)` -helper (mirroring `validate_source_fields`) covering `source`, `title`, -`source_name`, `source_id`, `theme` would keep it in one place. - -## Resolution - -Added `validate_field_lengths(data, fields)` in `artifacts/views.py`, called -from `validate_source_fields` (covers `source`, `title`, `source_name`, -`source_id`, `theme` on both text and binary `POST`, since both paths already -routed through that function) and directly in `ArtifactDetailView.patch` -(covers `title`, `source_name`, `source_id` — the only length-limited fields -`PATCH` can touch). Returns `400` naming the field and its limit, matching -the existing `tag_colors`/`source_type` convention. Tests added in -`tests/api/test_artifacts.py` covering oversized `title`/`source` on create, -an exact-limit boundary case, and oversized `title` on patch. diff --git a/issues/004-html-fragment-content-not-validated-server-side.md b/issues/004-html-fragment-content-not-validated-server-side.md deleted file mode 100644 index 5c2e492..0000000 --- a/issues/004-html-fragment-content-not-validated-server-side.md +++ /dev/null @@ -1,71 +0,0 @@ -# 004 — `html_fragment`/`deck`/`html_full` content-shape rules exist only in the client helper, not the server - -**Status:** Fixed - -## Description - -`docs/api.md` and `skills/daybook-posting/SKILL.md` both describe a -content-shape contract per text `kind`: - -- `html_fragment` — "Wrapped with a Daybook theme" (body-only HTML, no - `doctype`/`html`/`body`). -- `html_full` — complete document, must contain ``. -- `deck` — top-level `
` blocks. - -`skills/daybook-posting/scripts/daybook_post.py`'s `validate_payload()` -enforces all three of these client-side before ever sending the request. -**`artifacts/views.py`'s `ArtifactListCreateView.post` / `create_text_artifact` -enforce none of them** — the only server-side checks are "kind is one of -the 4 text kinds" and "required fields present." Any client that talks to -`POST /api/artifacts` directly (curl, a different language, a -misconfigured agent) bypasses the contract entirely, and the server -accepts and stores whatever is sent. - -Confirmed: posting `kind: "html_fragment"` with a full -`......` as -`content` returns `201`, and the stored page (`GET /a/{id}`) is: - -```html - - - - ... - nested html fragment - - - -Injected

Nested doc

- - -``` - -i.e. a doctype and a full `` nested inside the theme -wrapper's own `` — invalid markup that browsers will -silently "fix up" in unpredictable ways (duplicate/conflicting ``, -nested `<head>`, etc.), rather than the clean, themed fragment the kind -is supposed to guarantee. - -This is the same class of gap as #003: validation the docs promise is -implemented only in the optional client helper, not the server that's -supposed to be the actual contract enforcement point. Related to (but -distinct from) issue #003, which is about unenforced *length* limits -rather than unenforced *content shape*. - -## Suggested Fix - -Port `daybook_post.py`'s `validate_payload()` checks (or equivalent) into -`create_text_artifact`/`ArtifactListCreateView.post` server-side, returning -`400` with the same messages the helper already uses. This also closes the -gap for any future non-Python client. - -## Resolution - -Added `validate_content_shape(kind, content)` in `artifacts/views.py`, -ported directly from `daybook_post.py`'s `validate_payload()` (same rules, -same case-insensitive substring checks) so client and server agree: `deck` -requires `<section`, `html_fragment` rejects `<html`/`<!doctype`/`<body`, -`html_full` requires `<html`, and `markdown` rejects whitespace-only content -(which the old required-field check missed, since a non-empty -whitespace-only string is truthy). Called from `ArtifactListCreateView.post` -after the existing field/source validation. Tests added covering each -rejection case plus a well-formed `html_fragment` still succeeding. diff --git a/issues/005-patch-silently-mishandles-invalid-input.md b/issues/005-patch-silently-mishandles-invalid-input.md deleted file mode 100644 index 5fb034d..0000000 --- a/issues/005-patch-silently-mishandles-invalid-input.md +++ /dev/null @@ -1,72 +0,0 @@ -# 005 — `PATCH /api/artifacts/{id}` silently mishandles invalid/immutable input instead of rejecting it - -**Status:** Fixed - -## Description - -`ArtifactDetailView.patch` (`artifacts/views.py`) applies whatever -recognized keys are present in the request body without validating their -type or rejecting fields it can't act on. Two confirmed effects: - -**1. A wrong-typed `tags` value silently wipes existing tags (data loss).** - -`normalize_tags()` returns `[]` for anything that isn't a `str` or -`list`. `ArtifactDetailView.patch` calls it unconditionally whenever -`"tags"` is a key in the request body, with no type check first. - -Reproduced: - -```bash -# Artifact has tags ["important", "keep-me"] -curl -X PATCH .../api/artifacts/a74de286 -d '{"tags": 12345}' ... -# -> 200, tags silently become [] -``` - -An agent that sends a malformed `tags` value (e.g. a stray int from a bad -template substitution) gets a `200` and loses the artifact's existing -tags with no indication anything went wrong. - -**2. Immutable fields (`content`, `source`, `kind`, `day`) are silently -no-ops instead of rejected.** - -`docs/api.md`'s "Not editable yet" table lists these fields and why, but -doesn't say what happens if a client sends them anyway. In practice: -`PATCH` with `{"content": "...", "source": "...", "kind": "...", "day": -"..."}` returns `200` with the artifact **unchanged** — the fields are -silently dropped, not rejected. An agent attempting to fix a typo in -`content` via `PATCH` (a very natural thing to try, since `PATCH` is the -only mutate-after-create endpoint) gets a success response that looks -like it worked but did nothing. - -Both are instances of the same root cause: `patch()` doesn't validate -before applying, unlike `POST`'s `validate_source_fields()`. - -## Suggested Fix - -- Type-check `tags` (must be `str` or `list`) before calling - `normalize_tags`; return `400` on other types instead of silently - emptying it. Same principle likely applies to `metadata` (already - checked) and could apply to `source_prompt`/`source_name`/`source_id` - (currently accept anything via implicit str coercion via Django, but - worth a pass). -- Either make sending an immutable field (`content`/`source`/`kind`/`day`) - in a `PATCH` body a `400` ("field is not editable"), or explicitly - document that unknown/immutable keys are ignored — right now the - behavior is undocumented and easy to mistake for success. - -## Resolution - -Went with explicit rejection for both, over silent-ignore-and-document: - -- `tags` is now type-checked (`str` or `list`) before `normalize_tags` runs; - any other type returns `400` and leaves existing tags untouched. -- Sending `content`, `source`, `kind`, or `day` in a `PATCH` body now returns - `400` naming the offending field(s), via a new `IMMUTABLE_PATCH_FIELDS` - check at the top of `ArtifactDetailView.patch`, before any other - processing — chosen over documenting the no-op because a `200` that does - nothing is easy to mistake for success. - -`docs/api.md`'s "Not editable" table now says so explicitly instead of just -listing reasons. Tests added: wrong-typed `tags` is rejected and leaves -existing tags intact, and each of the four immutable fields is rejected -individually. diff --git a/issues/006-failed-post-still-evicts-artifacts.md b/issues/006-failed-post-still-evicts-artifacts.md deleted file mode 100644 index a5d56d1..0000000 --- a/issues/006-failed-post-still-evicts-artifacts.md +++ /dev/null @@ -1,81 +0,0 @@ -# 006 — A `POST` that ultimately fails with 413 still permanently deletes unkept artifacts - -**Status:** Fixed -**Severity:** High — irreversible data loss on a failed request - -## Description - -`make_room_for()` (`artifacts/storage.py`) is called from `create_text_artifact` -/`create_binary_artifact` *before* the new artifact is persisted. When the -incoming artifact doesn't fit even after evicting every unkept artifact, it -raises `QuotaExceeded` and the view returns `413`. But the eviction loop -inside `make_room_for` deletes each unkept artifact **as it iterates**, -*before* it can know whether the eventual total will succeed: - -```python -for artifact in unkept: - if used + incoming_bytes <= quota: - break - used -= artifact_total_bytes(artifact) - delete_artifact_files(artifact) - artifact.delete() - evicted.append(artifact) - -if used + incoming_bytes > quota: - raise QuotaExceeded("Quota is full of kept artifacts; free space to post.") -``` - -If the loop runs out of unkept artifacts to evict and still hasn't freed -enough room (because the remaining `is_kept=True` artifacts alone exceed -the new/current quota), it raises *after* having already deleted every -unkept artifact — there's no rollback. The caller gets a `413` that reads -like "nothing happened, free up space and retry," but in reality **every -unkept artifact the user had was just destroyed** for a request that -still failed. - -Reproduced against the `test` account (quota temporarily lowered to -demonstrate an unwinnable state — 1 kept artifact at 851 bytes vs. an -800-byte quota): - -``` -BEFORE: 1 kept artifact (851B) + 5 unkept artifacts (582/321/311/401/428B) -POST small markdown -> 413 "Quota is full of kept artifacts; free space to post." -AFTER: only the 1 kept artifact remains — all 5 unkept artifacts are gone - (both DB rows and on-disk files, confirmed via Django shell) -``` - -This is a normal, reachable state — not just an artificially shrunk quota. -Any user whose kept artifacts alone approach their quota will hit this the -moment they try to post anything that doesn't fit, and lose every unkept -artifact they had in the process, with a `413` response giving no -indication that a destructive side effect occurred. - -## Suggested Fix - -`make_room_for` should compute whether eviction *can* succeed before -deleting anything — e.g. sum `unkept` sizes up front and compare -`used - unkept_total + incoming_bytes` against `quota`; raise -`QuotaExceeded` immediately if even a hypothetical full eviction wouldn't -fit, without touching any artifact. Only perform actual deletions once -it's known they'll be sufficient. This makes the failure path side-effect -free, matching what a `413` response implies. - -## Resolution - -Fixed in `artifacts/storage.py`'s `make_room_for`: computes the total -recoverable bytes across all unkept artifacts up front and raises -`QuotaExceeded` immediately if even a full eviction wouldn't fit — no -artifact is touched on that path. The incremental eviction loop only runs -once success is already guaranteed. - -While testing this fix, found and fixed a related latent bug: the function -returned the evicted `Artifact` *objects* after calling `.delete()` on them, -but Django clears an instance's pk on delete, so every returned object's -`.id` was silently `None`. Nothing currently reads this return value, so it -had no live impact, but it broke the function's own documented contract. -Now returns the evicted ids (captured before deletion) instead of objects. - -Regression tests added in `tests/unit/test_storage.py`: an unwinnable -eviction leaves all unkept artifacts intact, a successful eviction only -evicts as many (oldest-first) as needed, and an over-quota single artifact -deletes nothing. diff --git a/issues/008-source-fields-untyped-and-mangled.md b/issues/008-source-fields-untyped-and-mangled.md deleted file mode 100644 index e7cbee7..0000000 --- a/issues/008-source-fields-untyped-and-mangled.md +++ /dev/null @@ -1,70 +0,0 @@ -# 008 — `source_name`/`source_id`/`source_prompt` accept any JSON type and get silently mangled into `repr()` strings - -**Status:** Fixed - -Follow-up to the #007 handover, which specifically asked to probe other -wrong-typed `PATCH` fields beyond `tags` now that #005 is fixed. - -## Description - -Unlike `tags`, `metadata`, and `tag_colors` (all now type-checked, `tags` -since #005), `source_name`, `source_id`, and `source_prompt` accept any -JSON type — object, array, number — on both `POST` (`source_fields()` in -`views.py`) and `PATCH` (`ArtifactDetailView.patch`). Nothing validates -they're strings before assignment. - -`source_name`/`source_id` are `CharField`, `source_prompt` is `TextField`. -Django's `Field.get_prep_value()` silently coerces non-string input via -plain `str(value)` at save time — which for a `dict`/`list` produces -**Python `repr()` syntax, not JSON**: `{"a": 1}` becomes the *string* -`"{'a': 1}"` (single quotes, unparseable as JSON). This is lossy and -irreversible — there's no way to recover the original structured value. - -**Worse: the API response is internally inconsistent.** Immediately after -a `POST`/`PATCH` with a non-string value, the response echoes back the -*raw, pre-save Python object* (still a real JSON object/array in the -response body) — but a subsequent `GET` of the same artifact returns the -mangled `repr()` string instead. Same request, two different apparent -types for the same field depending only on *when* you read it. - -Reproduced: - -```bash -# PATCH source_name with an int, source_id with a list, source_prompt with a dict -curl -X PATCH .../api/artifacts/a74de286 -d '{"source_name": 12345}' ... -# -> 200, response shows "source_name": 12345 (int) - -curl .../api/artifacts/a74de286 # fresh GET, session auth -# -> "source_name": "12345" (string) - -# Actual DB values (confirmed via shell): -# source_name = '12345' -# source_id = "['a', 'b']" <- repr() of the list, not JSON -# source_prompt = "{'nested': 'object'}" <- repr() of the dict, not JSON -``` - -Also reproduced identically on `POST` (create), not just `PATCH` — a -freshly created artifact's immediate response shows -`"source_name": {"a": 1}` (real object) even though what's stored (and -what any later `GET` returns) is the mangled string form. - -## Suggested Fix - -Add a type check for `source_name`, `source_id`, and `source_prompt` -(must be `str` or absent/null) alongside the existing `tags`/`metadata`/ -`tag_colors` checks in `validate_source_fields` (create path) and -`ArtifactDetailView.patch` (update path) — return `400` naming the field, -consistent with #005's approach for `tags`. This also removes the -response/DB inconsistency, since invalid types would never reach `save()` -in the first place. - -## Resolution - -Added `validate_string_fields(data, fields)` in `artifacts/views.py`, -rejecting any non-`None`, non-`str` value with `400`. Called from -`validate_source_fields` (covers `POST`, both text and binary paths) and -directly in `ArtifactDetailView.patch` for `source_name`, `source_id`, -`source_prompt` — matching #005's approach for `tags`. Invalid values never -reach `save()`, so the response/DB inconsistency the issue described can no -longer occur. Verified live against the running dev instance and with new -tests in `tests/api/test_artifacts.py`. diff --git a/issues/009-content-shape-check-false-positive-in-script-strings.md b/issues/009-content-shape-check-false-positive-in-script-strings.md deleted file mode 100644 index 55fd83e..0000000 --- a/issues/009-content-shape-check-false-positive-in-script-strings.md +++ /dev/null @@ -1,89 +0,0 @@ -# 009 — `html_fragment` content-shape check false-positives on safe text inside `<script>` string literals - -**Status:** Fixed (documented as a known limitation) - -Follow-up to the #007 handover, which specifically asked to probe the -`<html`/`<!doctype`/`<body`/`<section` substring checks for content -containing those strings "inside a code block/string literal rather than -as real markup." - -## Description - -`validate_content_shape` (added for #004) rejects `html_fragment` content -containing the literal substrings `<html`, `<!doctype`, or `<body` -anywhere in the text, case-insensitively — with no awareness of HTML -structure/context. This correctly catches genuinely dangerous cases (see -#004's original report and the escaped-vs-unescaped `<pre>` test below), -but also rejects content where those substrings are inert — e.g. a JS -string literal inside `<script>`, which can never become a real DOM -element: - -```json -{ - "kind": "html_fragment", - "content": "<p>Config example:</p><script>const tpl = \"<html><body></body></html>\";</script>" -} -``` - -→ `400 {"detail":"html_fragment must be body-only HTML; do not include doctype/html/body"}` - -This content is legitimate, safe, body-only HTML (a `<script>` block -whose string contents are never parsed as markup by the browser) — a -completely reasonable pattern for an artifact demonstrating/generating -HTML client-side. It's rejected purely because the substring check has -no idea it's inside a JS string. - -For contrast/completeness, confirmed correct behavior on two adjacent -cases while investigating this: -- Properly **escaped** literal HTML text in a `<pre>` block (`<html>`) - is accepted, as it should be — the check only fires on unescaped `<`. -- **Unescaped** `<html>` inside a `<pre>` block is correctly *rejected* — - that's not actually a false positive, since unescaped tags inside - `<pre>` still get parsed as real nested elements by the browser (same - root cause as #004), so blocking it is correct. -- Uppercase `<HTML>`/`<!DOCTYPE HTML>` is correctly still caught - (case-insensitive check works both directions — no case-based bypass), - and uppercase `<SECTION>` in a `deck` is correctly still accepted. - -So the check's case-insensitivity and escaping-awareness are both solid; -the gap is specifically "text inside script/string context is -indistinguishable from real markup" to a substring check. - -## Severity / Scope - -Low-to-moderate. Narrow trigger (an artifact literally about generating -or documenting HTML client-side), and there's a workaround (escape the -inner `<` as `<` even inside `<script>`, since JS accepts HTML -entities... actually it doesn't — entities are only decoded by the HTML -parser, so escaping breaks the JS string value itself; the real -workaround is string concatenation, e.g. `"<" + "html>"`, which is -awkward). Worth a conscious call on whether this is worth fixing (real -HTML parsing) or documenting as a known limitation, rather than treating -it as a must-fix — a proper fix means parsing the content as HTML and -checking node structure instead of substring matching, which is a bigger -change than the substring-check approach #004 shipped. - -## Suggested Fix (pick one) - -- **Document as a known limitation** in `docs/api.md`'s kind table: - "content-shape validation is substring-based, not a real HTML parse — - avoid the literal strings `<html`, `<!doctype`, `<body` anywhere in - `html_fragment` content, including inside `<script>`/`<pre>` text." - Cheapest, matches the spirit of the #004 fix (mirrors the client - helper's own substring approach). -- **Real fix**: parse `content` with an HTML parser (e.g. Python's - `html.parser` or `lxml`) and check for actual top-level `<html>`/ - `<body>` *elements* rather than substrings. More correct, more work, - and adds a dependency if not using stdlib `html.parser`. - -## Resolution - -Took the cheap option, per the issue's own framing: documented as a known -limitation in `docs/api.md` rather than implementing real HTML parsing. The -narrow trigger (an artifact literally about generating/documenting HTML -client-side) doesn't justify the bigger, riskier change a proper fix would -need (parse `content` and check actual node structure instead of substrings) -— substring matching is already the deliberate approach #004 shipped and -that `daybook_post.py`'s own client-side check uses, so this keeps -server/client behavior consistent rather than making the server stricter -in a way the client can't warn about ahead of time. diff --git a/issues/010-concurrent-posts-race-past-quota.md b/issues/010-concurrent-posts-race-past-quota.md deleted file mode 100644 index 616bc15..0000000 --- a/issues/010-concurrent-posts-race-past-quota.md +++ /dev/null @@ -1,124 +0,0 @@ -# 010 — Concurrent `POST`s race past the quota check, landing usage over quota - -**Status:** Fixed -**Severity:** High — the #006 fix's own writeup flagged this as an -unhandled case; now confirmed as a real, reproducible violation, not just -a theoretical gap. - -Follow-up to the #007 handover, which specifically asked to probe -"multi-request race conditions around eviction (two posts near the quota -boundary at once)." - -## Description - -`make_room_for()` (`artifacts/storage.py`) — even after the #006 fix — -still does a plain read-then-act sequence with no locking: - -```python -quota = user_quota_bytes(user) -if incoming_bytes > quota: raise QuotaExceeded(...) -used = user_usage_bytes(user) # <- read -... compute what needs evicting based on `used` ... -... delete evicted artifacts ... -... caller then creates the new Artifact row ... -``` - -Nothing serializes this read-compute-write sequence across concurrent -requests (no `select_for_update`, no atomic transaction wrapping the -whole check-evict-create flow, no per-user lock). Two or more requests -arriving close together each read a `used` value that doesn't yet -reflect the others' in-flight writes, so each independently concludes -"this fits" and proceeds. - -## Reproduction - -Set `test` account quota to 1551 bytes: enough for the one existing kept -artifact (851B) plus exactly **one** more ~658B artifact — no more. -Fired 5 truly concurrent `POST` requests (backgrounded shell jobs, `wait`), -each creating a ~658B markdown artifact: - -``` -[1] 201 [2] 201 [3] 413 [4] 201 [5] 201 -``` - -4 of 5 succeeded where only 1 should have fit. Final measured state: - -``` -quota: 1551 -final usage: 2167 <- 616 bytes OVER quota -kept artifact (851B) + 2 surviving "Race" artifacts (658B each) = 2167 -``` - -Note it's not simply "4 got created and stayed" — the responses claimed -4 successes, but only 2 of those 4 artifacts still existed afterward -(the other 2 got deleted by a *different* concurrent request's own -eviction pass, racing against them). So this isn't just an over-quota -bug, it's also nondeterministic which artifacts survive: a client can -receive a `201` with a real artifact `id`/`url` for an artifact that -gets silently deleted moments later by an unrelated concurrent request's -eviction sweep — a variant of the #006 data-loss problem, but now -triggered by legitimate concurrent traffic rather than a single doomed -request. - -## Suggested Fix - -Wrap the read-evict-create sequence in a transaction with a row lock -scoped per user — e.g. `select_for_update()` on the `UserProfile` row (or -an equivalent per-user lock) held for the duration of -`make_room_for()` + the subsequent `Artifact.objects.create(...)`, so -concurrent requests for the same user serialize around quota accounting. -Requests for *different* users should remain unaffected (lock scope must -be per-user, not global). SQLite's own file-level write lock already -serializes the raw `INSERT`/`DELETE` statements, but that's not the same -as serializing the "decide whether this fits" business logic wrapping -them — the race is in that gap. - -## Resolution - -`make_room_for` now performs a real `UPDATE` on the user's `UserProfile` row -(a no-op `quota_bytes=F("quota_bytes")`) as its first statement, before -reading `used` - this forces SQLite to acquire its write lock immediately -rather than after the read, closing the stale-read window. Both -`create_text_artifact` and `create_binary_artifact` wrap `make_room_for` -and the subsequent `Artifact.objects.create()` in one `transaction.atomic()` -block, so the lock is held through eviction and the new row's insert. -Added `OPTIONS: {"timeout": 20}` to `DATABASES` in `settings.py` as -headroom for legitimate contention. - -SQLite has no row-level locking, so this necessarily serializes -quota-affecting writes across *all* users, not just the racing ones - an -inherent SQLite limitation, not a design choice, and acceptable at this -project's actual scale (a low-volume self-hosted tool, not a high-throughput -service). Deliberately did **not** add retry/backoff logic for lock -contention: doing so would be engineering for a load pattern (many truly -simultaneous writes) this project doesn't have. Did fix a real bug found -while testing the fix itself: the exception handling around the create -calls only caught `QuotaExceeded`, so a genuine lock-timeout error left the -just-written file orphaned on disk - both call sites now clean up on any -exception, not just `QuotaExceeded`. - -Verified live against the actual dev instance (real file-based SQLite, not -Django's in-memory shared-cache test database) by firing 5 truly -simultaneous `POST`s at a scratch account with a quota tight enough for -exactly one artifact, twice: - -- Run 1: 1 succeeded, 4 hit a lock-timeout `500` (expected under forced - exact-simultaneity - not something real usage produces). Final usage: - 466/900 bytes, no orphaned files. -- Run 2: 4 succeeded, 1 got a `500`; 3 of the 4 were then legitimately - evicted by later-committing requests needing room - this is the - documented "oldest unkept artifact evicted to make room" feature working - correctly under concurrency, not a bug. Final usage: still 466/900 bytes, - one artifact, no orphaned files. - -In both runs the actual invariant - **usage never exceeds quota** - held. -The original bug (measured 616 bytes over quota) does not reproduce. - -An automated multi-threaded test was attempted -(`tests/api/test_quota_concurrency.py`, later removed) but proved unreliable -against Django's in-memory shared-cache test database specifically: SQLite's -shared-cache mode uses a different, non-retryable locking model -(`SQLITE_LOCKED`, not the ordinary retryable `SQLITE_BUSY`) than the -real on-disk file this app actually uses in dev/production, so it failed for -reasons unrelated to the fix's correctness. Verified live instead, matching -the original issue's own reproduction method. diff --git a/issues/011-vetted-cdn-snippets-for-html-full.md b/issues/011-vetted-cdn-snippets-for-html-full.md deleted file mode 100644 index ced54ef..0000000 --- a/issues/011-vetted-cdn-snippets-for-html-full.md +++ /dev/null @@ -1,79 +0,0 @@ -# 011 — Document vetted CDN framework snippets for `html_full`, scoped to that kind only - -**Status:** Fixed - -## Description - -`html_full` is documented (`SKILL.md`'s kind table) as the kind for -"complete custom pages/frameworks" — Daybook injects nothing into it -(`create_text_artifact`: `rendered = content` verbatim), so a poster -wanting Tailwind, Chart.js, or similar has to already know a CDN URL and -hand-write the `<link>`/`<script>` tags themselves. There's currently no -guidance anywhere in `docs/` or `skills/daybook-posting/` pointing to -known-good CDN URLs, so an agent has to search for one from scratch every -time. - -Separately (found while investigating this): `daybook_post.py`'s own -`--theme` argparse help text reads `"Example: report, tasks, bootstrap"` -— but `bootstrap` isn't a real theme (only `report`/`tasks`/`deck`/`base` -exist as files under `static/themes/`). Requesting it silently produces a -`<link>` to a 404'ing stylesheet, rendering unstyled with no error. Worth -fixing alongside this, since it's the same "help the agent avoid a dead -end" spirit. - -Update: the `bootstrap` example wasn't a random typo — `planning/framework_artifact_rendering.md` -already plans a vendored-Bootstrap-theme feature that just hasn't been built -yet, and the help text was almost certainly written ahead of that. Worth -linking that doc (and `planning/2026-09-02-theme_variants_and_user_palettes.md`, -a related-but-distinct design conversation about palette variants within the -existing theme families) when this is picked up, rather than treating the -example as simply wrong. - -**Explicitly out of scope / not being requested here**: injecting CDN -references into the *themed* kinds (`markdown`/`html_fragment`/`deck`). -Those exist specifically so Daybook — not the poster — owns styling -(stated principle in Daybook's own `CLAUDE.md`: "Do not make agents -responsible for styling"), and giving agents a default-reach-for CDN -snippet for those kinds would undermine that. This is scoped to -`html_full` only, where the poster has already opted out of managed -styling by choosing that kind. - -## Suggested Fix - -- Add a short, curated list of CDN snippets (Tailwind, and whatever else - comes up often — Chart.js is a reasonable second) to `SKILL.md`'s - `html_full` row or a new subsection, so agents can copy-paste instead - of searching. Note Tailwind's own CDN build - (`cdn.tailwindcss.com`) is marked "not for production" by Tailwind - themselves (compiles in-browser on every load) — worth linking a - precompiled build instead, or explicitly caveating if using the Play - CDN anyway for convenience. -- Note the self-hosted/offline tension explicitly: Daybook runs locally - and can be deployed air-gapped; any CDN-linked artifact won't render - fully offline. A one-line caveat in the docs is enough — this is a - tradeoff to flag, not something to solve. -- Fix the `--theme` help text's `bootstrap` example to name a real theme - (or drop the example value entirely and just say "see docs/api.md for - available themes"). - -## Resolution - -Added a "CDN Snippets For `html_full`" section to `SKILL.md` with Tailwind -(Play CDN, with a one-line caveat that it's not a production-grade build) -and Chart.js, an explicit offline/air-gapped caveat, and a reminder that -this is scoped to `html_full` only — never the themed kinds. Fixed -`daybook_post.py`'s `--theme` help text (`bootstrap` was never a real theme; -it was written ahead of the vendored-Bootstrap-theme feature described in -`planning/framework_artifact_rendering.md`, not a random typo) to name real -themes instead. - -While in `SKILL.md`'s kind cheat sheet, also found and fixed a second stale -spot: it never mentioned the `json`/`csv` kinds added this session, and -still told agents to use the `--as-markdown` fenced-code-block workaround -for them even though they now have real first-class support. Updated the -cheat sheet accordingly. - -That surfaced a real, separate gap: the posting *script* itself -(`daybook_post.py`) doesn't actually support `--kind json`/`csv` yet — filed -as issue #012 rather than fixing inline here, since it needs real script -changes (TEXT_KINDS, auto-detection, validation), not a doc tweak. diff --git a/issues/012-posting-script-missing-json-csv-support.md b/issues/012-posting-script-missing-json-csv-support.md deleted file mode 100644 index 0667a64..0000000 --- a/issues/012-posting-script-missing-json-csv-support.md +++ /dev/null @@ -1,68 +0,0 @@ -# 012 — `daybook_post.py` doesn't know about the `json`/`csv` kinds - -**Status:** Fixed - -## Description - -Found while updating `SKILL.md`'s kind cheat sheet for issue #011. -`artifacts/views.py` added first-class `json` and `csv` kinds, but the -posting helper script wasn't updated alongside it: - -- `skills/daybook-posting/scripts/daybook_post.py`'s `TEXT_KINDS = {"markdown", - "html_fragment", "html_full", "deck"}` doesn't include `json`/`csv`, so - `--kind json` is rejected by argparse before a request is ever sent. -- `TEXT_SUFFIXES` (used by positional auto-mode to sniff a file's kind from - its extension) doesn't include `.json`/`.csv` either — posting - `data.json` via the simple `daybook_post.py data.json` auto-mode path - uploads it as a generic downloadable `binary` artifact instead of using - the new interactive kind. -- `validate_payload()` has no shape-check branch for `json` (mirroring the - server's `json.loads` validation, per how every other kind's client-side - check mirrors its server-side one). - -Net effect: the API itself supports `json`/`csv` fully (confirmed working -via direct HTTP), but an agent following the documented, recommended -workflow ("always use the helper script") currently cannot reach either -kind at all. - -## Suggested Fix - -- Add `"json"`, `"csv"` to `TEXT_KINDS`. -- Add `.json`/`.csv` to `TEXT_SUFFIXES` with matching `sniff_text_kind` - logic (or a clear rule — e.g. `.csv` always maps to `kind=csv`; `.json` - maps to `kind=json` unless some other signal suggests it's meant as - metadata). -- Add a `json` branch to `validate_payload()`: attempt to parse and raise a - clear `ValueError` on invalid JSON, mirroring the server. -- Add a usage example to `SKILL.md`/`docs/llm_posting_helper.md` for both - kinds (e.g. `daybook_post.py --kind json --content-file data.json`). -- No test suite currently exists for this script at all (it's exercised - manually / via the API tests on the server side only) — worth deciding - whether this is finally worth a small script-level test file, or whether - server-side coverage is considered sufficient. - -## Resolution - -Added `"json"`/`"csv"` to `TEXT_KINDS` and `.json`/`.csv` to `TEXT_SUFFIXES` -with matching `sniff_text_kind` branches, so both explicit `--kind -json`/`csv` and plain positional auto-mode (`daybook_post.py data.json`) -work. Added a `json` branch to `validate_payload()` mirroring the server's -`json.loads` check (no branch needed for `csv` - matches the server's own -lenient stance). Updated `SKILL.md` and `docs/llm_posting_helper.md` with -real usage examples, replacing the stale `--as-markdown` workaround -guidance for CSV/JSON (kept for genuinely unstructured "code" only, which -still has no dedicated kind). Also fixed an unrelated pre-existing doc -inaccuracy noticed in the same table: `html_full`'s validation was -documented as requiring both `<!doctype` and `<html`, but the code (and -server) only require `<html`. - -Verified via dry-run for both explicit and auto-mode paths, invalid-JSON -rejection, and one real end-to-end post through the script against the -live dev instance. - -Added `tests/scripts/test_daybook_post.py` (plain `unittest`, zero new -dependencies, loaded via `importlib` since the script isn't a package) - -17 tests covering `sniff_text_kind`, `validate_payload`, and -`resolve_auto_path`. Answers this issue's open question about test -coverage: yes, worth having now that the script's logic has real -per-kind branches worth protecting against regression. diff --git a/issues/014-stored-xss-in-rendered-text-artifacts.md b/issues/014-stored-xss-in-rendered-text-artifacts.md new file mode 100644 index 0000000..948b349 --- /dev/null +++ b/issues/014-stored-xss-in-rendered-text-artifacts.md @@ -0,0 +1,132 @@ +# 014 — Stored XSS: markdown (and deck/html_fragment) render unsanitized script; no CSP; now reachable unauthenticated via public share links + +**Status:** Open +**Severity:** High — stored XSS executing on the Daybook origin, with a +concrete escalation chain against the owner's session, and (since the +sharing feature shipped in `91ea19d`) reachable by unauthenticated third +parties. + +Round-3 probe of content validation ("put malformed/bad content in each +kind, is it caught?"). Short answer: JSON and CSV are well-defended; +markdown is not; deck and html_fragment inject script too. + +## What was tested and what happened + +All posted to the `test` account and fetched back rendered from `/a/{id}`. + +| Kind | Injected | Result | +|---|---|---| +| `markdown` | `<script>…</script>`, `<img src=x onerror=…>`, `<a href="javascript:…">` | **All pass through verbatim into rendered HTML — executes.** | +| `deck` | `<section>…<script>alert(1)</script>` | **Script passes through into the themed deck page — executes.** | +| `html_fragment` | `<p>hi</p><script>…</script><svg onload=…>` | **Script/`svg onload` pass through into the themed page — executes.** (The #004 content-shape check blocks `<html>/<body>/<!doctype>` but never `<script>`.) | +| `html_full` | broken/garbage doc with `<script>` | Stored verbatim. **By design** — documented as "you own all presentation." Not claimed as a bug here. | +| `json` | `{"note":"</SCRIPT><img src=x onerror=…>"}` | **Defended.** `escape_script_body` (`re.compile(r"</script", re.IGNORECASE)`) turns it into `<\/script` inside the `<script type="application/json">` data block; renderjson shows it as inert text. Case-variation bypass (`</SCRIPT>`) correctly caught. | +| `csv` | `</script>` in a cell | **Defended.** Same escaping via `json.dumps` + `escape_script_body`. | +| `csv` | `=cmd|/c calc`, `+SUM(...)` as cell values | Passes through into the `?raw=1` sidecar verbatim (classic CSV/formula injection). Low severity / debatable — see below. | + +## Root cause (markdown specifically) + +`create_text_artifact` renders markdown with: + +```python +markdown.markdown(content, extensions=["extra", "sane_lists", "codehilite"], ...) +``` + +Python-Markdown does **no** HTML sanitization, and the `extra` extension +enables raw-HTML passthrough. Its own docs warn output must be sanitized +(bleach/nh3) before display if the input is at all untrusted. The rendered +HTML is then written to the artifact's `.html` file and served as-is. + +Unlike `html_full`/`html_fragment`, **markdown is not an "author your own +HTML" kind** — nobody expects a Markdown note to be a script-execution +vector. In an agent-driven system this is exactly the kind most likely to +carry *indirectly* untrusted content: an agent that summarizes a scraped +page, an email, or an ingested document can emit a summary containing an +attacker-controlled `<script>`/`<img onerror>` that ends up verbatim in the +posted markdown. + +## Why this is High, not self-XSS + +Two things raise the stakes well past "you attacked your own account": + +1. **No `Content-Security-Policy` header.** Rendered artifact responses + carry only `X-Frame-Options: SAMEORIGIN` and `X-Content-Type-Options: + nosniff` — nothing stops inline script execution. (A CSP was probably + avoided because the json/csv/deck viewers themselves use inline + scripts; a nonce-based CSP that tags Daybook's own inline scripts but + not user content would fix both this and the by-design kinds at once.) + +2. **Escalation chain against the owner's own session.** When the owner + views the malicious artifact at `/a/{id}` (their normal feed preview), + the script runs same-origin with the owner's session cookie. Reads are + session-authed, so it can `GET /api/artifacts` and exfiltrate every + private artifact's metadata. Worse, the `csrftoken` cookie is not + HttpOnly, so injected JS can read it and forge session+CSRF POSTs — e.g. + hit `POST /a/{id}/share` for every artifact, minting public share links + for the owner's entire private feed and exfiltrating them. (Bearer-token + writes like PATCH/DELETE are safe — the token isn't in the browser — but + share-toggle, mark-read, and keep are session+CSRF and thus reachable.) + +3. **Unauthenticated blast radius via sharing (`91ea19d`).** A shared + artifact at `/s/{token}` serves this same rendered HTML with no auth, so + a malicious/compromised artifact runs arbitrary JS in the browser of + anyone the owner sends the link to — a drive-by/phishing vector on the + Daybook origin, not just a self-inflicted one. + +## Suggested Fix + +- **Sanitize rendered output for the non-authoring kinds** — at minimum + `markdown`, and I'd argue `deck` too (a "slide sections" kind isn't + obviously an arbitrary-script host). Run the generated HTML through an + **allowlist** sanitizer that keeps normal formatting (headings, lists, + tables, code, images, links) but drops everything else — `<script>`, + `on*` handlers, `javascript:`/`data:` URLs. Pygments/codehilite output is + a fixed structure, so an allowlist that permits `<pre>/<code>/<span>` plus + the highlight `class` names preserves highlighting. + + Two things to get right, both confirmed against current (2026) tooling: + + 1. **Use `nh3`, not `bleach`.** `bleach` — the library most people reach + for first — is **deprecated and no longer receives security fixes** + (it was built on the unmaintained `html5lib`). `nh3` is the Python + binding to Rust's `ammonia` sanitizer: allowlist-based, actively + maintained, ~20× faster, and single-purpose. It exposes exactly the + knobs needed here (`tags`, `attributes`, `allowed_classes`, + `url_schemes`). + 2. **Allowlist, never blocklist.** Do *not* try to fix this by detecting + or stripping "bad" patterns (`<script>`, known `on*` handlers, etc.) — + that approach loses to case tricks, encoding, novel event handlers, + and SVG/MathML vectors, and is the historical source of XSS filter + bypasses. Parse-to-DOM + keep-only-known-safe (what `nh3`/`ammonia` + does) is the only robust model. Validity checking is a *separate* + concern and gives no security benefit — a perfectly valid document can + host a malicious `<script>` — so it must not be mistaken for the fix + here (it has a legitimate but lesser role as poster-facing "your + html_fragment is malformed" UX feedback, tracked separately if wanted). +- **Decide `html_fragment` explicitly.** It's marketed as themed + "reports/cards/tables," not full pages, and it's shareable — either + sanitize it like markdown, or if script is intended, document that + `html_fragment` executes arbitrary JS so posters treat it like + `html_full` trust-wise. Right now it's an in-between with neither + sanitization nor a warning. +- **Add a `Content-Security-Policy`** to artifact/shared views. A + nonce-based policy (nonce on Daybook's own renderjson/gridjs/deck inline + scripts, `script-src 'self' 'nonce-…'`) neuters injected inline scripts + even in the HTML-authoring kinds, and is the single highest-leverage + mitigation since it covers every kind at once. +- **CSV formula injection** (low/debatable): consider prefixing cells that + begin with `= + - @` with a `'` in the raw sidecar, or just document it. + Many tools treat this as the spreadsheet app's problem; noting it only + for completeness, not urging a fix. + +## Notes + +- html_full is intentionally verbatim and is **not** being reported as a + bug — only flagged so the fix doesn't accidentally start sanitizing it + and break its documented contract. +- json/csv defenses are genuinely solid; this issue is not about them + beyond recording that they were tested and held. + +## Resolution + +_(pending)_ diff --git a/issues/015-deeply-nested-json-recursionerror-500.md b/issues/015-deeply-nested-json-recursionerror-500.md new file mode 100644 index 0000000..f5109fa --- /dev/null +++ b/issues/015-deeply-nested-json-recursionerror-500.md @@ -0,0 +1,94 @@ +# 015 — Deeply nested JSON raises `RecursionError` → unhandled HTTP 500 (two code paths) + +**Status:** Open +**Severity:** Medium — unhandled 500 where a 400 is correct; small (~30 KB) +request crashes the handling worker (minor DoS shape); full traceback +exposed when `DEBUG=True`. + +Round-3 probe of the `json` kind ("deeply nested / very large" from the +#013 handover). + +## Description + +Two distinct code paths turn deeply-nested JSON into an uncaught +`RecursionError` and a 500. The root cause is shared: Python's +`RecursionError` is a subclass of `RuntimeError`, **not** `ValueError`, so +the usual `except (TypeError, ValueError)` guards around JSON parsing don't +catch it. + +### Path 1 — `json` kind content validation + +`validate_content_shape` (`artifacts/views.py:987`): + +```python +if kind == Artifact.JSON: + try: + json.loads(content) + except (TypeError, ValueError) as exc: # <-- misses RecursionError + return Response({"detail": f"content must be valid JSON: {exc}"}, status=400) +``` + +Posting a `json` artifact whose `content` is `"[" * 5000 + "]" * 5000` +(valid, just deep) raises `RecursionError: maximum recursion depth +exceeded while decoding a JSON array from a unicode string`, which escapes +the `except` and becomes a 500. + +Reproduced: +``` +POST /api/artifacts kind=json content="[[[...5000...]]]" +-> 500 (RecursionError at views.py:987, via post() at views.py:133) +``` + +### Path 2 — DRF request-body parser (affects ALL kinds) + +The same class of payload in the **request body itself** (e.g. a deeply +nested `metadata` object, or nesting anywhere in the JSON body) crashes in +DRF's `JSONParser` *before the view runs*, so it isn't `json`-kind +specific: + +``` +POST /api/artifacts kind=markdown metadata={"n":{"n":{...5000...}}} +(body ~30 KB, built as a raw string so it's well-formed) +-> 500 (RecursionError inside DRF JSONParser) + +Control: same shape at depth 100 -> 201 (confirms it's depth, not size) +``` + +### Not a size issue + +A 50,000-key flat object (~978 KB) posts fine (`201`). It's specifically +nesting *depth* that triggers it, so a byte-size limit wouldn't address +this. + +## Impact + +- Wrong status: an unhandled 500 where malformed/pathological input should + be a clean 400. +- Minor DoS shape: a single ~30 KB request reliably errors out the worker + handling it. Not a resource-exhaustion amplifier, but a cheap way to + generate server-side 500s. +- Info exposure *only* on a `DEBUG=True` instance (like the local dev one + this was found on): the full Django traceback page is returned. Prod + should have `DEBUG=False`, so there this degrades to a generic 500 — the + status-code bug remains either way. (Flagging the traceback exposure only + as a reminder to confirm prod `DEBUG` is off, not as the core issue.) + +## Suggested Fix + +- **Path 1 (cheap, local):** add `RecursionError` to the caught exceptions + at `views.py:988` (it's a `RuntimeError`, so either list it explicitly or + catch `(TypeError, ValueError, RecursionError)`), returning the same 400. +- **Path 2 (needs a global guard):** DRF's default exception handler + doesn't map `RecursionError` to a 4xx. Options: a custom + `REST_FRAMEWORK["EXCEPTION_HANDLER"]` that converts `RecursionError` + (and other parse-time `RuntimeError`s) to `400`; and/or a bounded JSON + parser / max-nesting-depth check applied to request bodies. A depth cap + (e.g. reject bodies nested beyond N levels) is the most robust since it + rejects before the recursive descent exhausts the stack. +- Consider a single shared "parse JSON safely" helper used by both the + content validator and any metadata/tag_colors parsing, so the guard + lives in one place. + +## Resolution + +_(pending)_ diff --git a/issues/016-csv-ragged-rows-silent-data-loss-in-render.md b/issues/016-csv-ragged-rows-silent-data-loss-in-render.md new file mode 100644 index 0000000..467fb68 --- /dev/null +++ b/issues/016-csv-ragged-rows-silent-data-loss-in-render.md @@ -0,0 +1,85 @@ +# 016 — Ragged CSV rows silently lose data in the rendered table (server doesn't normalize row width) + +**Status:** Open +**Severity:** Low-Medium — silent data loss in the primary rendered view; +raw sidecar is unaffected, so it's a fidelity/display bug, not permanent +loss. + +Round-3 probe of the `csv` kind (ragged rows, from the #013 handover). +Confirmed with an actual headless-Chrome render, not just server inspection. + +## Description + +`wrap_csv_viewer` (`artifacts/views.py`) parses CSV server-side and hands +Grid.js `{columns: rows[0], data: rows[1:]}` **without reconciling each +data row's width against the header**. Grid.js keys body cells by +column-id, so any row that doesn't match the header column count renders +wrong: + +- **Row longer than header → extra cells silently dropped.** +- **Row shorter than header → missing cells rendered blank.** + +Daybook's stated principle is "the server owns normalization," but row +width isn't normalized here, so the display silently disagrees with the +data. + +## Reproduction (verified via headless Chrome render) + +Posted `csv` content: + +``` +a,b,c +1,2,3,4,5 +6,7 +``` + +Server hands Grid.js: `{"columns": ["a","b","c"], "data": [["1","2","3","4","5"], ["6","7"]]}` + +Rendered DOM (headless Chrome, JS executed): + +``` +| a | b | c | +| 1 | 2 | 3 | <- values 4 and 5 are GONE, not shown anywhere +| 6 | 7 | | <- third cell blank +``` + +The `4` and `5` from the first data row do not appear in the rendered +table at all. `GET /a/{id}?raw=1` still returns the original CSV intact, so +the data isn't destroyed — but a user browsing the artifact (the normal +path, and the shared-link path) sees fewer values than the CSV contains, +with no truncation indicator. + +This is realistic input: trailing commas, inconsistent exports, and +hand-edited CSVs routinely produce rows that don't all match the header +width. + +## Other CSV edge cases tested (all fine) + +For the record — these were probed in the same pass and behaved correctly: + +- Embedded commas, quotes (`""` escaping), and newlines inside quoted + fields: parsed correctly (Python's `csv` module). +- `duplicate columns` (`id,id,id`): rendered (3 same-named headers) — a + bit odd for sorting but not a data bug. +- `header only`: empty table, fine. +- `empty content`: rejected `400`, consistent with `json`. +- `single giant cell` (20 KB): rendered fine. +- `just commas`: empty-named columns, rendered. + +So the parsing is robust; the gap is specifically row-width normalization +before the data reaches Grid.js. + +## Suggested Fix + +In `wrap_csv_viewer`, normalize each data row to `len(columns)` before +serializing — pad short rows with `""`, and for over-long rows either (a) +truncate but surface it, or better (b) widen `columns` to the maximum row +width (auto-naming the extra columns, e.g. `column_4`, `column_5`) so no +cell is ever dropped. Option (b) preserves all data, which matches the +"server owns normalization, never silently lose content" spirit. Whichever +is chosen, the rendered table should never contain fewer values than the +raw sidecar. + +## Resolution + +_(pending)_ diff --git a/issues/README.md b/issues/README.md index 662618b..8e169d8 100644 --- a/issues/README.md +++ b/issues/README.md @@ -2,4 +2,11 @@ One file per issue: `NNN-short-slug.md`, numbered sequentially. Each file has a title, status (Open / Fixed / By design), description, and resolution -once closed. +once closed. Numbering keeps climbing — archived numbers are never reused. + +Resolved issues are moved to `_archive_/` to keep this directory focused on +what's open. That subdirectory is gitignored (it matches the repo-wide +`_archive_/` rule), so archived issues stay on local disk and in git +history but are not carried in the committed tree. This directory is read +by the sibling `AGENT_DAYBOOK__LLM_TEST` project via its local path, so it +still sees the archived files.