Issues: file in-app guide (020); archive completed issues
- Add issue 020: in-app user guide (Settings -> Guide), Open, for later. - Archive to _archive_/ (gitignored, local only): fixed bugs 014/015/016 and the completed LLM-test handover docs 007/013/018/019. - Tracked issues now: 017 (open), 020 (open), README. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
4fa20e05c2
commit
4cacd8f64e
8 changed files with 43 additions and 719 deletions
|
|
@ -1,54 +0,0 @@
|
|||
# 007 — Handover to AGENT_DAYBOOK__LLM_TEST: issues #002-#006 fixed
|
||||
|
||||
**Status:** For information — not a bug report, a handback after fixing
|
||||
what #002-#006 found.
|
||||
|
||||
## What Changed
|
||||
|
||||
All five issues your last testing round filed are fixed, committed
|
||||
(`07328a3` on `main`, pushed to `origin`), and confirmed live on the running
|
||||
local dev instance (`http://127.0.0.1:8001`, same one you test against —
|
||||
no restart needed on your end, Django's autoreloader already picked it up):
|
||||
|
||||
- **#002** — `.env.example` deleted; `SKILL.md`/`docs/llm_posting_helper.md`
|
||||
now say explicitly these must be real process/shell env vars.
|
||||
- **#003** — `source`/`title`/`source_name`/`source_id`/`theme` now
|
||||
enforce their model length limits on both `POST` and `PATCH`, returning
|
||||
`400` naming the field.
|
||||
- **#004** — server now validates content shape per `kind`
|
||||
(`html_fragment`/`html_full`/`deck`/`markdown`), mirroring
|
||||
`daybook_post.py`'s existing client-side checks.
|
||||
- **#005** — `PATCH` now rejects wrong-typed `tags` (400, instead of
|
||||
silently wiping existing tags) and rejects immutable fields
|
||||
(`content`/`source`/`kind`/`day`) with 400 instead of silently no-op'ing.
|
||||
- **#006** — the quota-eviction data-loss bug: a doomed `POST` no longer
|
||||
destroys unkept artifacts on its way to still failing with `413`.
|
||||
|
||||
Full detail, root cause, and exact fix for each is in its own issue file
|
||||
(`002` through `006`) — this doc is just the summary + what to do next.
|
||||
|
||||
## Suggested Next Round
|
||||
|
||||
Worth specifically re-probing (not just re-running the original repros —
|
||||
try to break the fixes themselves):
|
||||
|
||||
- **#003/#004 boundary/edge cases**: exact-limit-plus-one values, unicode
|
||||
characters (does length-checking count characters or bytes?), mixed valid
|
||||
content with edge-case whitespace/casing on the `<html`/`<!doctype`/
|
||||
`<body`/`<section` substring checks (e.g. `<HTML>`, `< html`, content
|
||||
containing those strings inside a code block/string literal rather than as
|
||||
real markup).
|
||||
- **#005**: other wrong-typed fields on `PATCH` beyond `tags` — e.g.
|
||||
`source_prompt`/`source_name`/`source_id` as non-strings (currently
|
||||
accepted via implicit coercion, noted as untested in #005's original
|
||||
writeup).
|
||||
- **#006**: multi-request race conditions around eviction (two posts near
|
||||
the quota boundary at once) — not covered by the fix, which only handles
|
||||
the single-request case cleanly.
|
||||
- General: anything from the original `issues/002-006` reproductions that
|
||||
might have a sibling case not explicitly covered by the tests added
|
||||
alongside each fix (`tests/api/test_artifacts.py`, `tests/unit/test_storage.py`).
|
||||
|
||||
As always: use the `test`/`testpwd` account, clean up what you create, and
|
||||
file anything real as a new numbered issue rather than reopening a closed
|
||||
one (link back to the original if it's a variant of the same root cause).
|
||||
|
|
@ -1,121 +0,0 @@
|
|||
# 013 — Handover to AGENT_DAYBOOK__LLM_TEST (round 2)
|
||||
|
||||
**Status:** For information — a handback covering everything shipped since
|
||||
the round-1 handoff (#007).
|
||||
|
||||
Round 1 (#007) closed out issues #002–#006. Since then, your round-2
|
||||
testing filed #008–#012 (all now fixed), and several new capabilities
|
||||
landed. This doc summarizes all of it and points at where the fresh
|
||||
surface area is worth probing.
|
||||
|
||||
All of this is committed on `main` (pushed to Forgejo) and live on the
|
||||
local dev instance you test against (`http://127.0.0.1:8001`) — Django's
|
||||
autoreloader has already picked it up, no restart needed on your end.
|
||||
|
||||
## New Capabilities To Exercise
|
||||
|
||||
These are new since #007 and largely untested by you yet:
|
||||
|
||||
1. **`json` and `csv` artifact kinds** (commit `e5d2f19`). Text kinds like
|
||||
markdown. `json` renders as a collapsible tree (renderjson), `csv` as a
|
||||
sortable/searchable table (Grid.js). Both validate server-side (`json`
|
||||
must parse; `csv` only needs non-blank content) and preserve original
|
||||
text as a `?raw=1` sidecar. Both vendored libs are served from
|
||||
`/static/vendor/…`.
|
||||
|
||||
2. **`.xlsx` upload → `csv`** (commit `4b5ec09`). Uploading an `.xlsx` file
|
||||
(multipart, like any file) is converted server-side via openpyxl into a
|
||||
`csv` artifact rather than stored as `binary`. Active sheet only; cached
|
||||
formula results only; `.xls` unsupported; corrupt file → `400`.
|
||||
|
||||
3. **Per-artifact public sharing** (commit `91ea19d`). Owner toggles a
|
||||
share link from the feed UI; `GET /s/{share_token}` then serves that one
|
||||
artifact with **no authentication at all**. `is_shared`/`share_url` now
|
||||
appear in every artifact API response. Embedded images in a shared
|
||||
artifact are also served unauthenticated while it's shared. **This is
|
||||
the highest-value target for adversarial testing** — see below.
|
||||
|
||||
4. **Syntax-highlighted code + styled tables** (commits `af9ecf2`,
|
||||
`696cfe7`). Markdown fenced code blocks get Pygments highlighting;
|
||||
tables in markdown and `html_fragment` render styled. Cosmetic, low-risk.
|
||||
|
||||
5. **Posting script supports `json`/`csv`/`.xlsx`** (commit `e137725`).
|
||||
`daybook_post.py --kind json/csv`, positional auto-detect of
|
||||
`.json`/`.csv`/`.xlsx`.
|
||||
|
||||
6. **Light/dark UI toggle** (commit `9a3b381`). UI-chrome only, no API
|
||||
surface — not relevant to API testing.
|
||||
|
||||
## Fixes Since #007 (issues you filed)
|
||||
|
||||
- **#008** — `source_name`/`source_id`/`source_prompt` now reject
|
||||
non-string values with `400` instead of silently mangling them.
|
||||
- **#009** — the `html_fragment` content-shape check is substring-based;
|
||||
documented as a known limitation rather than switched to real HTML
|
||||
parsing (deliberate — matches the client helper's own approach).
|
||||
- **#010** — concurrent posts could race past quota; fixed with a
|
||||
write-lock-before-read + atomic transaction. Verified live but **not**
|
||||
covered by an automated test (SQLite shared-cache test DB behaves
|
||||
differently — see the issue). Worth re-probing.
|
||||
- **#011** — CDN snippets documented for `html_full`; `--theme` help typo
|
||||
fixed.
|
||||
- **#012** — posting script gained `json`/`csv` support.
|
||||
|
||||
## Suggested Round-3 Probes
|
||||
|
||||
Prioritized by where the new surface area is riskiest.
|
||||
|
||||
### Sharing (#91ea19d) — security boundary, probe hardest
|
||||
|
||||
- Can an **unshared** artifact ever be reached via `/s/<token>`? Brute/
|
||||
guessed/empty/malformed tokens should all `404`.
|
||||
- Does **unsharing immediately** kill the old link? (Confirmed manually,
|
||||
but re-verify — and try racing an unshare against an in-flight request.)
|
||||
- Is `share_token` ever leaked in a response for an artifact you *don't*
|
||||
own, or in the list endpoint, or anywhere a non-owner could see it?
|
||||
- Can a shared artifact's **embedded assets** be reached unauthenticated —
|
||||
and, importantly, can an *un*shared artifact's assets be reached that way
|
||||
(they must not)? Also: does an orphaned/unattached asset stay private
|
||||
even when some unrelated artifact is shared?
|
||||
- Does `?raw=1` on `/s/<token>` correctly serve raw source for the kinds
|
||||
that support it, and not leak anything for kinds that don't?
|
||||
- Cross-user: user A shares an artifact; can user B toggle/unshare it, or
|
||||
only view it?
|
||||
|
||||
### json / csv (#e5d2f19)
|
||||
|
||||
- `json`: valid-but-not-an-object payloads (bare array, string, number,
|
||||
`null`); deeply nested / very large; unicode; the `</script>` injection
|
||||
vector (confirmed handled for json — verify **csv** escapes its embedded
|
||||
data block the same way, it's a separate code path).
|
||||
- `csv`: ragged rows (fewer/more columns than the header), embedded commas/
|
||||
quotes/newlines, duplicate column names, header-only, empty, a single
|
||||
giant cell, BOM/encoding oddities.
|
||||
- Quota accounting: json/csv now write a sidecar file — confirm size
|
||||
accounting and eviction include/delete the sidecar (this generalized the
|
||||
markdown-only logic; worth verifying it didn't regress markdown either).
|
||||
|
||||
### .xlsx conversion (#4b5ec09)
|
||||
|
||||
- Multi-sheet workbook — verify only the active sheet survives and the
|
||||
others are silently dropped (documented behavior, confirm it's true).
|
||||
- Formula cells never calculated by a real spreadsheet app (openpyxl
|
||||
doesn't evaluate) → should be blank, not error.
|
||||
- A non-`.xlsx` file renamed to `.xlsx`; a real `.xlsx` sent with a wrong/
|
||||
generic content type; a huge sheet; empty workbook; `.xls` (must not
|
||||
convert).
|
||||
- Field mangling regression: this path rebuilds the payload dict manually
|
||||
because multipart `QueryDict` + `{**data}` was silently list-wrapping
|
||||
values (the bug that made this a multi-fix). Re-probe that title/tags/
|
||||
source_* all come through correctly on `.xlsx` posts specifically.
|
||||
|
||||
### #010 quota race (no automated coverage)
|
||||
|
||||
- Fire N truly-concurrent posts at a tight quota; confirm final usage never
|
||||
exceeds quota and no artifact is left half-created (row without files or
|
||||
files without row).
|
||||
|
||||
As always: use the `test` / `testpwd` account, clean up what you create,
|
||||
check `artifacts/views.py` before calling a surprise a bug (some, like
|
||||
#009, are documented-by-design), and file anything real as a new numbered
|
||||
issue (next is #014), linking back if it's a variant of a closed one.
|
||||
|
|
@ -1,169 +0,0 @@
|
|||
# 014 — Stored XSS: markdown (and deck/html_fragment) render unsanitized script; no CSP; now reachable unauthenticated via public share links
|
||||
|
||||
**Status:** Fixed
|
||||
**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
|
||||
|
||||
Fixed with two layers (both in `artifacts/views.py`):
|
||||
|
||||
1. **Allowlist sanitization at create time** for the themed kinds
|
||||
(`markdown`, `deck`, `html_fragment`) via `nh3` (the maintained
|
||||
`ammonia`/Rust sanitizer, per the issue's recommendation — not the
|
||||
deprecated `bleach`). `sanitize_html()` keeps ordinary formatting,
|
||||
tables, images, links, and the codehilite/Pygments markup, and drops
|
||||
`<script>`, `on*` handlers, `<style>`, unknown tags, and
|
||||
`javascript:`/`data:` URLs (`url_schemes` limited to http/https/mailto;
|
||||
relative `/assets/<id>` image URLs pass through). Verified live that
|
||||
markdown/deck/html_fragment strip script/onerror/onload/svg while tables,
|
||||
syntax highlighting, and relative images survive. `html_full` is
|
||||
deliberately **not** sanitized — it remains the documented verbatim
|
||||
escape hatch.
|
||||
|
||||
2. **Script-blocking CSP at serve time** for the sanitized kinds:
|
||||
`default-src 'self'; script-src 'none'; object-src 'none'; base-uri
|
||||
'none'`, set in `serve_artifact_file` (so it covers both `/a/{id}` and
|
||||
the unauthenticated `/s/{token}` share route). Because it's a response
|
||||
header, it also neutralizes any script in artifacts stored *before* this
|
||||
fix — closing the retroactive gap that create-time sanitization alone
|
||||
would miss. Not applied to `html_full` (its scripts are by contract) or
|
||||
`json`/`csv` (their viewers need JS; their user data is separately
|
||||
escaped via `escape_script_body`).
|
||||
|
||||
Regression tests added in `tests/api/test_artifacts.py` (7): script/handler
|
||||
stripping for each themed kind, formatting/highlighting/relative-image
|
||||
preservation, html_full staying verbatim, and the CSP header being present
|
||||
for sanitized kinds and absent for html_full.
|
||||
|
||||
**Not addressed here (separate, lower-priority items the issue noted):**
|
||||
- CSV formula injection (`=cmd|…` in a cell) — left as documented behavior.
|
||||
- `CSRF_COOKIE_HTTPONLY` — moot now that the XSS vector is closed, but a
|
||||
cheap extra hardening if wanted (our JS reads the token from a form input,
|
||||
not the cookie, so enabling it wouldn't break anything).
|
||||
- A nonce/hash CSP for the `json`/`csv`/`html_full` pages — those keep their
|
||||
current behavior; the strict CSP is scoped to the no-JS themed kinds where
|
||||
it's provably safe.
|
||||
|
|
@ -1,111 +0,0 @@
|
|||
# 015 — Deeply nested JSON raises `RecursionError` → unhandled HTTP 500 (two code paths)
|
||||
|
||||
**Status:** Fixed
|
||||
**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
|
||||
|
||||
Both paths fixed:
|
||||
|
||||
- **Path 1** (`json`-kind content validation): `validate_content_shape` now
|
||||
catches `RecursionError` alongside `(TypeError, ValueError)`, returning
|
||||
the same `400 content must be valid JSON: …`.
|
||||
- **Path 2** (deep nesting anywhere in the request body, crashing DRF's
|
||||
parser before the view): added a custom DRF `EXCEPTION_HANDLER`
|
||||
(`artifacts/exception_handlers.py`, wired via `REST_FRAMEWORK` in
|
||||
settings) that maps otherwise-unhandled `RecursionError` to
|
||||
`400 Request payload is nested too deeply to parse.` This also covers the
|
||||
same class of error in `metadata`/`tag_colors` string parsing. Anything
|
||||
that isn't a RecursionError still falls through to Django's normal 500.
|
||||
|
||||
Verified live: deep `json` content and a ~5000-deep `metadata` body both
|
||||
return 400; shallow payloads still 201. Tests added in
|
||||
`tests/api/test_artifacts.py`. (A depth-cap parser was considered but the
|
||||
exception-handler approach is simpler and covers every recursive-descent
|
||||
site at once; prod `DEBUG=False` already suppresses the traceback page.)
|
||||
|
|
@ -1,94 +0,0 @@
|
|||
# 016 — Ragged CSV rows silently lose data in the rendered table (server doesn't normalize row width)
|
||||
|
||||
**Status:** Fixed
|
||||
**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
|
||||
|
||||
Implemented option (b) in `wrap_csv_viewer`: normalize every row to the
|
||||
maximum row width before handing data to Grid.js. Rows longer than the
|
||||
header widen the column set (extra columns auto-named `Column N`); rows
|
||||
shorter than the width are padded with empty strings. No cell is ever
|
||||
dropped, matching "server owns normalization, never silently lose content."
|
||||
|
||||
Verified live with the issue's exact input (`a,b,c` / `1,2,3,4,5` / `6,7`):
|
||||
grid data becomes `columns=[a,b,c,Column 4,Column 5]`,
|
||||
`data=[[1,2,3,4,5],[6,7,"","",""]]` — the previously-dropped `4` and `5`
|
||||
now render. Test added in `tests/api/test_artifacts.py`.
|
||||
|
|
@ -1,93 +0,0 @@
|
|||
# 018 — Handover to AGENT_DAYBOOK__LLM_TEST (round 3)
|
||||
|
||||
**Status:** For information — a handback covering everything fixed/changed
|
||||
since the round-2 handoff (#013).
|
||||
|
||||
Round-2 testing (against #013's suggestions) filed #014, #015, #016 — all
|
||||
now fixed. This round-3 handoff summarizes those fixes and points at where
|
||||
the fresh adversarial surface is, plus one new design issue (#017) that is
|
||||
**not** a bug and shouldn't be re-reported.
|
||||
|
||||
All changes are on `main` (pushed to Forgejo) and live on the local dev
|
||||
instance (`http://127.0.0.1:8001`) via autoreload — no restart needed.
|
||||
Test as always with the `test` / `testpwd` account and clean up after
|
||||
yourself.
|
||||
|
||||
## Fixes since #013 (issues you filed)
|
||||
|
||||
- **#014 (High) — stored XSS.** `markdown`, `html_fragment`, and `deck` are
|
||||
now allowlist-sanitized (nh3) at create time: `<script>`, `on*` handlers,
|
||||
`<style>`, and `javascript:`/`data:` URLs are stripped, while formatting/
|
||||
tables/links/images (incl. relative `/assets/<id>`) survive. Those kinds
|
||||
are also served with `Content-Security-Policy: script-src 'none'` (on both
|
||||
`/a/{id}` and the public `/s/{token}` route). `html_full` is deliberately
|
||||
**not** sanitized (verbatim escape hatch) and `json`/`csv` keep their
|
||||
existing `escape_script_body` defense.
|
||||
- **#015 (Med) — deeply nested JSON → 500.** Now returns `400` on both
|
||||
paths: the `json`-kind validator catches `RecursionError`, and a new DRF
|
||||
`EXCEPTION_HANDLER` maps parser-level `RecursionError` (deep nesting
|
||||
anywhere in the request body) to `400`.
|
||||
- **#016 (Low-Med) — ragged CSV rows.** `wrap_csv_viewer` now normalizes
|
||||
every row to the max width (long rows widen the columns with auto-named
|
||||
`Column N`; short rows pad with `""`), so no cell is dropped.
|
||||
|
||||
## Other changes (context, mostly not API-surface)
|
||||
|
||||
- Document artifact backgrounds unified to one off-white (`#faf9f6`) across
|
||||
markdown/html_fragment/json/csv (report/tasks/base themes). Deck stays
|
||||
dark; html_full/pdf unchanged. Only relevant to you as: the rendered HTML
|
||||
background value changed — no behavioral/API change.
|
||||
- UI-only work (dark mode toggle, top-bar logo, sidebar width behavior,
|
||||
greyscale chrome). No API surface.
|
||||
- **#017 filed — customizable theme variants.** A *design issue* (feature
|
||||
request), not a bug. Don't re-report theme/color inflexibility as a bug;
|
||||
it's tracked.
|
||||
|
||||
## Suggested Round-4 Probes
|
||||
|
||||
Prioritized by where the new surface is riskiest.
|
||||
|
||||
### The sanitizer (#014) — now the top adversarial target
|
||||
|
||||
The XSS fix is an allowlist sanitizer; the classic follow-up is trying to
|
||||
get past it. Worth hard probing on `markdown`, `html_fragment`, `deck`:
|
||||
|
||||
- **Mutation/parser-confusion XSS**: malformed nesting, stray `<`/`>`,
|
||||
comments (`<!-- -->`), CDATA, `<svg>`/`<math>` foreign-content vectors,
|
||||
`<noscript>`/`<template>`, deprecated/odd tags — anything that might
|
||||
re-parse into script after nh3 serializes.
|
||||
- **Attribute vectors**: `on*` variants, `style=` (should be stripped),
|
||||
`href`/`src` with `javascript:`, `data:`, `vbscript:`, whitespace/newline/
|
||||
encoding tricks (`java\tscript:`, `javascript:`), `srcset`, `formaction`.
|
||||
- **Allowlisted-tag abuse**: does anything survive on the *permitted* tags
|
||||
(a/img/etc.) that shouldn't — e.g. `<a href>` protocol bypasses, `<img
|
||||
src>` with odd schemes?
|
||||
- **CSP presence**: confirm `script-src 'none'` is actually on the response
|
||||
for markdown/deck/html_fragment (both `/a/` and `/s/`), and absent for
|
||||
html_full and json/csv. Confirm markdown/deck/html_fragment pages contain
|
||||
no executable inline script at all after sanitization.
|
||||
- **html_full is out of scope** — it's verbatim by design; a `<script>`
|
||||
surviving there is not a bug (documented). Don't file it.
|
||||
- Verify the sanitizer didn't over-strip: tables, code highlighting, links,
|
||||
relative `/assets` images, headings/lists still render.
|
||||
|
||||
### #015 recursion — other sites
|
||||
|
||||
- Deeply nested `metadata` / `tag_colors` (string-parsed JSON), deeply
|
||||
nested arrays vs objects, right-at-the-limit depths. Confirm 400 not 500.
|
||||
|
||||
### #016 CSV — other raggedness
|
||||
|
||||
- Header shorter than data rows, entirely empty first row, mixed quoting
|
||||
with ragged widths, a row that's all empty cells. Confirm no data lost and
|
||||
the auto-named columns behave.
|
||||
|
||||
### Regression sweep
|
||||
|
||||
- Re-run the #002–#012 repros briefly to confirm none regressed (esp. the
|
||||
#010 quota race and #006 eviction, which have thin/no automated coverage).
|
||||
|
||||
As always: use `test`/`testpwd`, clean up, check `artifacts/views.py` before
|
||||
calling a surprise a bug (some behavior is documented/by-design), and file
|
||||
anything real as a new numbered issue (next is #019), linking back if it's
|
||||
a variant of a closed one.
|
||||
|
|
@ -1,77 +0,0 @@
|
|||
# 019 — Handover to AGENT_DAYBOOK__LLM_TEST (round 4 — pre-deploy sign-off)
|
||||
|
||||
**Status:** For information — final regression pass before deployment.
|
||||
|
||||
This is the **last test round before the app is deployed**. Unlike previous
|
||||
handoffs, there are essentially no new features to probe: since round-3
|
||||
(#018), the only changes are cosmetic CSS (table fonts → monospace cells /
|
||||
Arial headers; document body prose → system sans-serif). **No API or
|
||||
behavioral surface changed.** So the goal here is a **broad regression
|
||||
sweep** to confirm nothing has broken, so the maintainer can deploy with
|
||||
confidence.
|
||||
|
||||
Everything is on `main` (pushed) and live on the local dev instance
|
||||
(`http://127.0.0.1:8001`). Use the `test` / `testpwd` account and clean up
|
||||
what you create.
|
||||
|
||||
## What to do: full regression sweep
|
||||
|
||||
Confirm the whole API still behaves. Don't just re-run one thing — cover the
|
||||
surface, since a clean pass here is the deploy gate.
|
||||
|
||||
### Core CRUD, every kind
|
||||
- `POST` each text kind (markdown, html_fragment, html_full, deck, json,
|
||||
csv) → `201`, correct `kind`, renders, `raw_available` correct.
|
||||
- `POST` each file kind (image, pdf, binary) and `.xlsx` (→ converts to csv).
|
||||
- `GET /api/artifacts` (list, filters: source/label/day/kind/q) and
|
||||
`GET /api/artifacts/{id}` (session-authed).
|
||||
- `PATCH` (title/tags/metadata/source_* fields) and the immutable-field
|
||||
rejections (content/source/kind/day → 400).
|
||||
- `DELETE` → 204, files gone, second delete 404s.
|
||||
- `/api/capabilities` returns the current kind/method/filter lists.
|
||||
|
||||
### Auth boundaries (important pre-deploy)
|
||||
- No token / bad token on writes → 403.
|
||||
- One user's token cannot read/patch/delete another user's artifact (404).
|
||||
- List/detail reads require a session, not just a bearer token.
|
||||
|
||||
### Confirm every closed issue stays fixed
|
||||
Spot-check the earlier repros — these are the regression-risk areas:
|
||||
- **#014** — script/onerror in markdown/deck/html_fragment still stripped;
|
||||
CSP `script-src 'none'` present on those (and `/s/`); html_full still
|
||||
verbatim.
|
||||
- **#015** — deeply nested JSON (content *and* request body) → 400, not 500.
|
||||
- **#016** — ragged CSV rows don't drop cells.
|
||||
- **#010 / #006** — quota race and eviction (thin/no automated coverage):
|
||||
concurrent posts never exceed quota; a doomed post doesn't destroy unkept
|
||||
artifacts.
|
||||
- **#008** — non-string source_* → 400.
|
||||
- **#003 / #004** — length limits and content-shape checks still enforced.
|
||||
|
||||
### Sharing (the unauthenticated surface — worth re-checking before going live)
|
||||
- Share toggle creates/clears the token; `/s/{token}` serves unauthenticated;
|
||||
unsharing kills the old link immediately; a bad/guessed token 404s; an
|
||||
unshared artifact's embedded assets are not reachable anonymously.
|
||||
|
||||
## Cosmetic changes since #018 (not API-testable, FYI only)
|
||||
|
||||
- Table cells render in monospace, headers in Arial.
|
||||
- Document body prose is system sans-serif (was serif in the report theme).
|
||||
- Earlier UI-only work: unified off-white artifact backgrounds, greyscale
|
||||
chrome, dark-mode toggle, top-bar logo, sidebar width behavior.
|
||||
|
||||
None of these change API responses; verify only if a render looks broken.
|
||||
|
||||
## Out of scope / don't report
|
||||
- **#017** (customizable theme variants) is an accepted design issue, not a
|
||||
bug.
|
||||
- `html_full` being verbatim/unsanitized is by design.
|
||||
- `pdf` using the browser's grey viewer chrome is expected.
|
||||
|
||||
## Outcome
|
||||
- **Clean sweep → the maintainer deploys.** If you find anything, file it as
|
||||
a new numbered issue (next is **#020**), severity-tagged, so it can be
|
||||
triaged as deploy-blocking or deploy-after.
|
||||
|
||||
As always: `test`/`testpwd`, clean up, and check `artifacts/views.py` before
|
||||
calling a surprise a bug.
|
||||
43
issues/020-in-app-user-guide.md
Normal file
43
issues/020-in-app-user-guide.md
Normal file
|
|
@ -0,0 +1,43 @@
|
|||
# 020 — In-app user guide (Settings → Guide)
|
||||
|
||||
**Status:** Open
|
||||
**Type:** Feature
|
||||
|
||||
## Motivation
|
||||
|
||||
Accounts get created for other people (via Settings → Users), who then log in
|
||||
with **zero context**. The repo `docs/` are invisible to anyone without
|
||||
repository access, and the About page only shows the version. A new user has
|
||||
no in-product answer to "what is this and how do I use it?"
|
||||
|
||||
The fix is a short, task-oriented **in-app user guide** — not a copy of the
|
||||
repo docs (those target agent builders and operators and would rot if
|
||||
duplicated).
|
||||
|
||||
## Proposed approach
|
||||
|
||||
- A new **Settings → Guide** page, in the nav under the "You" group near
|
||||
About (see the You/Admin grouping added in v0.2.1).
|
||||
- Concise, human-facing content: what a feed is, labels (source + tags),
|
||||
getting an API key (link to Settings → API Keys), sharing links, dark mode.
|
||||
- A "Posting from an agent" section that points at (or inlines) the bundled
|
||||
posting skill — which is already served at `GET /api/skill` — so the
|
||||
agent-facing part never drifts from the real skill.
|
||||
|
||||
Two implementation options for the body:
|
||||
1. Hand-written template content (simplest, fully styled).
|
||||
2. Hybrid: a written human intro + the skill rendered/linked for the agent
|
||||
part (keeps agent instructions auto-synced with `SKILL.md`).
|
||||
|
||||
Recommend the hybrid.
|
||||
|
||||
## Scope / notes
|
||||
|
||||
- Keep it a **user** guide; leave API reference, deployment, and `/admin/`
|
||||
in the repo `docs/`. Link out rather than duplicate.
|
||||
- No new models needed; a view + template + one nav entry.
|
||||
- Candidate release: a future minor (e.g. v0.2.2), not urgent.
|
||||
|
||||
## Resolution
|
||||
|
||||
_(open)_
|
||||
Loading…
Add table
Add a link
Reference in a new issue