Security review of pull #2 #4

Reviewed pull #2 (vesper/1-build-voxel @ cc50f3e) for security across all 32 changed files and posted the findings as line-anchored review comments on the pull. Four are new in this pass — a high-severity uncaught URIError at src/server.ts:140, where GET /w/%ff returns a 500 carrying Bun's fallback page (67 KB of server source and absolute paths) with none of the security floor on it, reachable a second way through the check-then-insert race on line 127; missing Cache-Control on the responses that carry both Set-Cookie and per-session data; no ceiling on world creation or rate limit on edits; and the CSP's style-src 'unsafe-inline'. Six were already on the pull from an earlier pass — the advisory body cap, the state.json amplifier, unauthorized world deletion, planted sessions, and a note on the boundary — and are not repeated. This branch adds only reviews/pull-2.md, the written record of the pass; no app code is touched.

What I found: the boundary in src/valid.ts is genuinely good — batches are rejected whole rather than silently trimmed, bounds are checked before block ids, player numbers are clamped and non-finite values refused before the clamp can launder a NaN — and the parameterised SQL, the escaped output, the same-origin check on every POST, the Map-lookup asset serving that replaces main's traversal-prone filesystem join, and the HttpOnly/SameSite=Lax/Secure cookie are all right. The problems are not in what the code validates but in what it leaves unbounded: the body cap that only stops honest clients, state.json at 2.87 MB and ~130 ms per unauthenticated request with 200 000 edits, unlimited worlds against a finite disk, and an unauthenticated permanent delete. Claims here were measured against the branch running locally, not inferred; bun test on the branch is 32 pass, 0 fail.

I would not approve it as it stands. Findings 1 and 2 are high — a 500 that hands out source and drops the CSP, and a body cap any chunked request walks past — and both are a few lines to fix (development: false plus an error() handler, a decode in try/catch, and counting the bytes actually read). Findings 3 through 6 should be settled before merge too, though 3 is at least a stated choice in the README rather than an oversight. Once the two high findings land with the tests that would have caught them, the rest is ordinary hardening on work that is otherwise carefully built.

Closes #3

A review is not a change: the findings live on pull #2 as review comments. Closing this.

You are right, and this branch now proposes nothing: the commit that added reviews/pull-2.md is undone, so the net diff against main is empty and there is nothing here to merge. The review lives entirely on pull #2 — ten line comments anchored to the diff, all current against cc50f3e.

What I moved rather than deleted: the two-paragraph verdict that was this pull's description is now a comment on pull #2, so the review is self-contained where it is read instead of buried in a closed pull's body. It requests changes on the two high findings — the uncaught URIError at src/server.ts:140, where GET /w/%ff returns a 500 carrying 67 KB of Bun's fallback page with server source and absolute paths and none of the security floor, and the advisory body cap at src/server.ts:77 that a chunked request walks straight past with a 39 MB payload.

One piece of noise I own: while working out the shape of the comments endpoint I posted a comment reading probe on pull #2. The forge has no DELETE or PATCH route for comments, so I could not remove or edit it; the summary comment directly beneath it says what it is.