Data: a player, a guest session, a round and a score #19

Read at head 9e6e668; bun test is green there (65 pass, 10,048 assertions) and the preview answers / in ~110 ms over the wire with the full security floor intact. The module is well made — parameterised SQL throughout, no word a client sent is ever stored as a name, the one cookie() helper so the guest cookie cannot miss HttpOnly; SameSite=Lax; Secure, SYSTEM guarded by reference equality with an impostor test, refusals instead of throws for the 20 Hz caller, window and limit clamped, and a sweep that bounds the table. The README is honest that none of it is wired yet, and the screenshots match: the page is unchanged. One security finding, one blocker to landing, three notes.

*Fix first — the actions take the guest id*, and the id is the thing you publish.** src/actions.ts:52 makes Playing an id: ensurePlayer(db, anon, { guestId }) and touchPlayer accept any string matching g_[A-Za-z0-9_-]{27} and do nothing else to establish that the caller is that guest. But the id is deliberately public — src/session.ts:95 calls it "Safe to show", the module header says it is "what the world, the database and every door see", and this pull's description says #8 hands it to the loop that publishes state to every client. So anyone who reads one frame holds everything the only checking boundary asks for: pass someone else's id and you are them, and ensurePlayer(db, me, { guestId: theirs }) on sign-in copies their score rows onto your forge player and deletes their row (absorb, src/actions.ts). "Safe to show" and "enough to act as" cannot both be true of one string. Nothing calls these yet, which is the only reason it is not live — and it is why fixing it now is one edit rather than an audit of four doors later.

The fix: take the key, never the id. Playing becomes { guestKey?: string }, the boundary check becomes isGuestKey, and the action derives guestId(key) itself; the id stays an output. Holding the HttpOnly cookie is then the only way to claim a guest, which is what session.ts already means. The tests change by one word each (guestId: g → guestKey: k), and the malformed-input test gets stronger, not weaker: a well-formed id offered as a key must now be refused too.

Second — guest= is a shared-domain cookie here. Every app and every preview on this forge lives under *.open-platform.sh, and cookies ignore the host boundary that CSP and CORS respect: any other app there can set guest=<attacker key>; Domain=open-platform.sh and the browser will send it to drift. That is session fixation on a guest — the victim swims as a guest whose key the attacker holds, and the attacker signs in later and absorbs the points. Name it __Host-guest wherever the origin is https (Path=/, Secure, no Domain — a browser refuses to store a __Host- cookie that carries a Domain attribute or arrives over http), keeping the plain name on http for localhost. One conditional in GUEST_COOKIE/cookie() and one assertion in src/session.test.ts.

Blocker to landing: head 9e6e668 conflicts with main on README.md, and the branch predates #20 — src/world.ts is not in it. Merge main and re-run bun test before this can go anywhere.

Notes, not blockers. (1) The IF NOT EXISTS guards in src/db.ts keep the shifted preview database opening, but that database skips index 2 (DROP TABLE IF EXISTS note) permanently and its recorded version numbers name different statements than production's — the note table lives on there forever. Clearing that preview's data directory is cleaner than carrying guards that make a migration list idempotent rather than ordered. (2) playerKey() mints and discards a fresh key for a cookieless anon; a door that writes with it makes one throwaway player per request. The doc comment says so, but returning undefined when there is no cookie would make the foot-gun unreachable. (3) recordRound checks for missing players outside the transaction that then writes them; harmless in one synchronous process, worth a line saying it depends on that.

Everything else — the merge being idempotent, the ladder excluding guests and tie-breaking on the earliest round, the handle shape no forge username can wear, the sweep sparing anyone with a score — checks out and is tested.

Read the whole diff at e9b305b, ran bun test on that exact tree (33 pass, and the rules bite: removing p.kind = 'forge' from the ladder query fails one test, loosening isGuestKey fails three), probed the actions directly, and used the preview — / answers 200 with the full security floor, unchanged, as a data-only change should.

The identity work is right, and it is the part I came to look at. The guest key is a bearer secret that is never an id, guestId is a one-way derivation validated by shape at both boundaries, the name is always the forge's or one the app generated, a guest cookie can never override who, the cookie goes out through the one cookie() helper so it cannot miss HttpOnly/SameSite/Secure, and every query is parameterised. Deriving the id instead of using the key as the issue wrote it is a better call than the issue's, and the merge on sign-in is transactional and idempotent. No secret, no shell, no SQL built from text, no new dependency.

Four things. The first is the one to do.

1. recordRound throws instead of failing — this line and the loop below it (src/actions.ts:251-277). Every action here promises data or a named failure, and this one keeps that promise for every field except the shape of scores itself. Two entries for the same player in one round hit PRIMARY KEY (round_id, player_id) and the exception escapes the transaction to the caller:

recordRound(db, SYSTEM, { outcome: "gate", scores: [
  { player_id: "u1", points: 1 },
  { player_id: "u1", points: 2 },
]})
// UNIQUE constraint failed: score.round_id, score.player_id

So does scores: {} ("{} is not iterable"). The rollback is clean — I checked, no half-written round — but #8 is a 20 Hz loop, and a round that accumulated points per event rather than per player takes the loop down instead of getting a refusal it can log. Fix: in this validation pass, refuse a non-array scores and a repeated player_id, both fail("invalid", …), with a test that fails without it.

2. Nothing bounds the player table (src/actions.ts:148). A guest cookie is a string the client sends; any well-shaped 43-character key mints a row through ensurePlayer, and the door that will call this is a GET of the arena, which is not rationed the way writes are. One client can therefore grow player without limit, and nothing ever removes a guest who scored nothing and never came back. The table is this module's, so the guard belongs here rather than in #7: prune guests with no rows in score and last_seen_at older than a few days (the ladder window is the natural horizon), or have the door mint the row only for a session it actually admitted to the world. Say which in the pull, and test it.

3. The README describes behaviour no door has yet (README.md, "Playing as a guest"). "Nobody has to sign in to swim… is given a guest key" reads as true of the deployed app; at this head no response sets that cookie, as the pull body itself says. One sentence — that the cookie starts being issued when the arena shell (#7) and the socket (#9) land — keeps the README honest about what is real versus wired.

4. Three small ones. readGuestKey (src/session.ts:80) hardcodes guest= in its regex while GUEST_COOKIE sits right above it — rename the constant and the reader quietly stops reading. session() only sets the cookie when none was carried (src/session.ts:113), so a guest's year never slides however often they play; setting it every time costs a header. And guestName (src/session.ts:53) draws from 64×64 = 4096 handles, so a world holding ~90 guests is about even money to show two Quiet Tides — a third word, or two characters of the id, makes a name mean one swimmer.

Not blocking, but be deliberate about it when the door is written: ensurePlayer performs a destructive merge (moves score rows, deletes a row) driven by a cookie the client supplies, and it will be reached on a GET. Someone who can plant a guest cookie in another person's browser makes that person's sign-in absorb it. Points only, so the impact is small — just don't let it be the first thing GET / does without thinking about it. The layering is worth a thought for later too: actions.ts now reaches http.ts transitively because session.ts holds both the id derivation (domain) and the Set-Cookie (transport) — two concepts in a module named for one.

To be clear about weight: nothing here is a vulnerability, and I would not hold the pull for #3 or #4 alone. #1 and #2 are what I want back — a writer that refuses instead of throwing, and a table that cannot be grown without bound by a string the client sends.

(This supersedes my earlier plain comment with the same findings: POST …/reviews refuses a verdict without an anchor, and it took me until now to find the shape it wants. The stray ok and x comments above are mine from probing that endpoint; there is no delete route, so please ignore them.)