| 1 | LIVE https://todo-nim.bfj.oncarlos.com 290dbb4 |
| 2 | |
| 3 | ## Where it stands |
| 4 | |
| 5 | - Deployed `290dbb4` (the second review round's fix) to `todo-nim` on |
| 6 | account `bfj`, channel `edge`. `carlos deploy` confirmed it through the |
| 7 | `/api/version` build probe. Earlier deploys: `df803f7`, then `6aa1f35`. |
| 8 | - Shared behaviour suite: **67 passed, 3 skipped** both locally |
| 9 | (`dist/app --listen`) and against |
| 10 | `https://todo-nim.bfj.oncarlos.com`. The skips are the suite's own: blur |
| 11 | and Escape editing with JavaScript off, and the screenshot test with |
| 12 | JavaScript off. |
| 13 | - `make test`: 17 unit tests pass (store scoping, filters, toggle-all, |
| 14 | Unicode and 10,000-character titles, Unicode-whitespace trimming, migrations, cookies, 303s, 403 on |
| 15 | a foreign Origin, 404 on a foreign id, the edit view, fragments, static |
| 16 | files). |
| 17 | - `scripts/boot-check.sh dist/app-linux-arm64`: `/healthz` and |
| 18 | `/api/version` answer 200 on a unix socket under qemu, and SIGTERM |
| 19 | exits 0. |
| 20 | - The binary is a 1.7 MB static aarch64 ELF (zig cc, musl). SQLite 3.54.1 |
| 21 | is compiled in. |
| 22 | |
| 23 | ## Worth knowing |
| 24 | |
| 25 | - `src/httpserver.nim` is a copy of Nim 2.2.12's `std/asynchttpserver` |
| 26 | with a single change, marked "todo-nim": chunked bodies are limited. |
| 27 | When upgrading Nim, re-copy it and re-apply that change. |
| 28 | - `asynchttpserver` has no unix-socket mode. `unixServer` in `src/app.nim` |
| 29 | binds the socket and gives it to the server through |
| 30 | `std/importutils.privateAccess`, and it has to set `maxFDs` too: |
| 31 | without it `shouldAcceptRequest` is always false and the server |
| 32 | silently never accepts. That's the one place this app reaches into the |
| 33 | standard library's internals. A `listen(domain = AF_UNIX)` option |
| 34 | upstream would remove it. |
| 35 | - The README and AGENTS.md prose didn't go through a copy review. This |
| 36 | session ran unattended, and that review needs a person. |
| 37 | |
| 38 | ## Review round 2 |
| 39 | |
| 40 | The one finding is fixed in `290dbb4`: |
| 41 | |
| 42 | 1. **Invalid UTF-8 in titles (security).** `title=%80` made |
| 43 | `unicode.strip` index out of bounds. The `IndexDefect` isn't a |
| 44 | `CatchableError`, so it got past the handler and stopped the server. |
| 45 | Now `action` runs `unicode.validateUtf8` on the decoded title before |
| 46 | anything else touches it, and answers **400** if it isn't valid. That |
| 47 | check is bounds-safe. Browsers always send UTF-8, so a real visitor |
| 48 | never hits this. A unit test covers truncated 2-, 3- and 4-byte |
| 49 | sequences and stray bytes for both add and edit, and the list is |
| 50 | unchanged afterwards. Before the fix, `title=%80` killed the local |
| 51 | binary. After it, the binary and the live host both answer 400 and |
| 52 | keep serving. |
| 53 | |
| 54 | After the fix the shared suite gave 67 passed and 3 skipped both locally |
| 55 | and against the live host, and the boot check passed. |
| 56 | |
| 57 | ## Review round 1 |
| 58 | |
| 59 | All four findings are fixed in `6aa1f35`: |
| 60 | |
| 61 | 1. **Chunked bodies (security).** The parser lives inside the standard |
| 62 | library, so the server is now vendored as `src/httpserver.nim`. Each |
| 63 | chunk's declared size is checked against what's left of the body limit |
| 64 | before `recv` allocates anything. Oversized, overflowing and |
| 65 | many-small-chunk bodies all get a 413 and the connection closes. The |
| 66 | limit is 1 MiB for both chunked and `Content-Length` bodies (it was |
| 67 | 8 MiB); a 10,000-character title needs at most about 120 KB. Checked |
| 68 | with raw-socket requests against the binary: it stayed at 7 MB RSS. |
| 69 | 2. **Unicode trim.** Add and edit now strip the whitespace JavaScript's |
| 70 | `String.prototype.trim` removes (U+00A0, U+2000–U+200A, U+3000, U+FEFF |
| 71 | and the rest). A title of only non-breaking spaces adds nothing, and |
| 72 | editing to that deletes the item. There's a unit test for both. |
| 73 | 3. **Edits across re-renders.** `render()` keeps the item being edited |
| 74 | (text, cursor, focus) and restores it on the same item in the new |
| 75 | list, and the blur that removing the old field may fire doesn't |
| 76 | count as a save. I checked it with Playwright: toggle one item with |
| 77 | the response held for 800 ms, double-click another, type, and wait. |
| 78 | The edit is still open, focused and holding the text, and Enter saves |
| 79 | it. |
| 80 | 4. **Zig's caches.** The Makefile exports `ZIG_GLOBAL_CACHE_DIR` and |
| 81 | `ZIG_LOCAL_CACHE_DIR` under `.cache/`, and `clean-deps` removes them. |
| 82 | After `clean-deps`, `make build-arm64` rebuilt musl and compiler-rt |
| 83 | (61 MB of cache) from scratch. |
| 84 | |
| 85 | After the fixes the shared suite gave 67 passed and 3 skipped both |
| 86 | locally and against the live host, and the boot check passed. |
| 87 | |
| 88 | ## Review findings not fixed |
| 89 | |
| 90 | None. |
| 91 | |
amadan