Bug check on main: hunt what the tests are not catching #7

Pushed. Here's the pull description:

Went through main looking for what the tests weren't catching, focused on the areas the issue called out.

Found and fixed: a reconnect race in the seat/socket handshake. attach/release in src/arena.ts keyed a session's seat purely by session id. On a page reload, the new websocket opens (and attaches) before the old one's close event arrives; that stale close called release(session), which deleted whatever entry currently sat under that session — the new socket's — and, finding nobody else holding the player, flipped a still-connected human back into a bot. command() refuses input from a bot, so the reconnected player's keystrokes would silently stop working until they refreshed again. Fixed by giving each socket a per-connection token (generated at upgrade, carried in socket.data) so a close only releases the seat if it's still the current socket for that session. Added a regression test in server.test.ts that drives attach/release directly; confirmed it fails against the old code (reverted the fix, watched it go red) and passes with it restored.

Checked and found clean (no changes): the economy (cap applied via award/spend/clampCredits on every single credit mutation, no bypass found; loss-streak reset/cap; mid-match join credits are a bot's own economy-tracked balance, which is the documented design, not leftover junk); the round state machine (defuse-vs-fuse same-tick ordering, checkpoint surviving a defuser's death, the 12-round swap and 13-round win including 12–12, already covered by existing tests); abilities (charge/cooldown/ult/blindness lifecycle, already covered); the spike (off-site plant, double-plant, defender pickup all structurally impossible by the side/phase guards); output escaping (server HTML template escapes everything dynamic; client only ever uses textContent, never innerHTML); the request/socket boundary (body cap by bytes read, same-origin POST/websocket checks, malformed/oversized/wrong-typed socket messages already tested and handled); and FORGE_APP_DATA/secrets (only db.ts writes state, under FORGE_APP_DATA; no secrets in the repo).

Nothing else needed changing — this was one real bug, fixed at its root.

Closes #6

Pushed. Here's the update for the pull:

Addressed benjivers' follow-up: one client could open several websockets and hold seats on both teams at once, reading each side's own-team snapshot to get full vision of the entire match, with no forged or malformed traffic — plus a seat-denial angle since enough sockets take the whole roster.

Two causes, both fixed:

1. The seat claim raced the socket, not the request. takeSeat() only recorded a session's seat once its socket's open() handler called attach() — but a websocket upgrade completes a tick later than the request that asked for it. Two /ws requests for the same session, arriving close together, both read the seat map before either had written to it, and both walked away with a distinct seat on opposite teams. Fixed by claiming the seat inside takeSeat() itself, synchronously, before the caller ever gets a socket.
2. A connection with no session cookie the page minted still got a full seat. /ws would mint a throwaway session for it on the spot. Every other app on this forge ties identity to a cookie the page hands out; this one didn't enforce that for sockets. Now a /ws request that never carried a pre-existing session cookie is upgraded as a spectator instead: it gets only the same public state the page already shows everyone (score, clock, phase, round, events) — no me, no player positions, no side. A real browser always carries the cookie by the time its own script opens the socket, so normal play is untouched; verified this end-to-end against a running server with a raw socket.

Also normalized the aim angle at the setInput boundary — a client sending a: 99999 came back as angle: 99996.3 in the snapshot, feeding an unbounded value into the flash-facing math. Harmless today per the report, but wrapped to (-π, π] at the boundary rather than left to grow.

Scope call: this closes the exploit as reported (connections carrying no cookie at all). It doesn't stop a client that fabricates its own cookie value — no session here is cryptographically signed, matching the trust model every other app on this forge uses for its session cookie. Going further (signed sessions, a server-side issued-session allow-list) seemed like more than this fix warranted; said so explicitly rather than quietly leaving it implied.

Regression tests added: a direct takeSeat race test (two calls, same session, before any attach — fails on the old code with two seats on opposite teams), an end-to-end spectator test against a real socket connection, and a setInput angle-wrap test. Also fixed client.test.ts's own websocket to forward the cookie its page fetch received — it previously connected with no cookie either, which would have made it a spectator and broken that test for an unrelated reason; now it behaves like the real browser it's standing in for. Full suite (151 tests) is green and stable across repeated runs.

The reconnect-race fix is a genuine find and the regression test is the right shape — a reconnected human silently becoming a bot is exactly the kind of bug no rule test would catch. Keep it.

But the both-teams hole is still open, because a cookie is not an identity: anyone can ask for another one. Against your own preview:

session 1 → bot-red-0/red   (fetched the page, took a seat)
session 2 → bot-blue-0/blue (fetched the page AGAIN, took another)
session 3 → bot-red-1/red

union across three self-issued sessions: 10 of 10 players
teams held simultaneously: red, blue

Three GETs of /, three cookies, three seats on both sides — and the union is every position on the map. What your change did close is the anonymous version: a socket with no cookie now gets no seat at all, which is a real improvement. It just does not bind a person to a seat, because minting a session costs one HTTP request.

What actually closes it, and the part I want you to get right: the leak is not "two seats", it is any connection holding positions for more than one side. So:

1. Cap playing seats by something a cheater cannot mint for free — the client address is the only such thing here. One playing seat per address; a second connection from the same address gets no seat.
2. A connection without a seat must receive a positionless view — score, round, timer, kill feed, nothing with an x or a y in it. If a spectator snapshot carries positions, the exploit simply moves from two seats to one seat plus one spectator.

That combination means a single machine cannot see both sides. Two machines on two networks still can, and that is true of every anonymous game without accounts — say so in the README rather than pretending otherwise, and do not try to solve it with more cookies.

A test should hold two connections from one address and assert the second gets no seat and that nothing in its snapshot carries coordinates.

Pushed. Summary of what changed in response to the feedback:

You were right that the cookie-based fix only closed the free/anonymous variant. A playing seat is now capped by connecting address (self.requestIP in src/server.ts), not by session — the claim happens synchronously inside takeSeat() in the same place that already closed the earlier race, so a second session from an address that already holds a seat gets refused outright rather than handed a bot. /ws treats that refusal exactly like a sessionless connection: no seat, a spectator view with no x/y anywhere in it (verified against a real running server with two real cookies from one address, not just inside the test harness). A session still gets a reload back to its own seat; it no longer buys a second one.

I did not try to go further than that — two machines on two networks still look like two different people to this server, which is true of any anonymous game with no accounts, and I put that limitation in the README rather than reaching for more cookies to chase it, as you asked.

One thing worth flagging: getting the end-to-end test right surfaced that a few existing tests claim a real seat over real HTTP from the test process's own loopback address and never release it (a /join with no follow-up socket has nothing to close). That's harmless in isolation but meant a later test needing a clean address could find it already spent. I added a small test-only releaseAddress() export (clearly documented as test hygiene, not a game feature) and used it defensively, and fixed the two actual culprits (client.test.ts's own socket, and the /join sanitization test) to clean up after themselves. Full suite (152 tests) is green and stable across repeated runs.