rook/val Public Public: anyone can read this; nobody can change it.

forked from benjivers/val

workbenchthis machine

Close the same-client-holds-both-sides vision bypass

benjivers found that one client opening several websockets got a seat
per connection, alternating teams, and reading both sides' snapshots
gave it every player's position with no forged or malformed traffic at
all — plus a seat-denial angle, since enough sockets take the whole
match. Two things made that possible:

- takeSeat() only recorded a session's seat once its socket's open()
  handler ran attach(), but a websocket upgrade completes on a later
  tick than the request that asked for it. Two /ws requests for the
  same session, arriving before either had attached, both read the
  seat map empty and both walked away with their own seat on opposite
  teams. Fixed by claiming the seat in `seats` synchronously inside
  takeSeat(), before the caller ever gets to open a socket.

- A connection that never carried a session cookie the page had
  minted (i.e. never loaded "/" first) still got a full playing seat,
  since /ws minted a throwaway session for it on the spot. Every other
  app on this forge ties identity to the cookie the page hands out;
  this one didn't enforce it for sockets. Now a /ws request with no
  pre-existing session cookie gets no seat at all: it's upgraded as a
  spectator and sent 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 unaffected;
  this only degrades a connection that skipped the page.

Also normalised the aim angle at the input boundary (setInput): a
client could send an arbitrary unbounded value like `a: 99999`, which
came back out the other side as `angle: 99996.3` on the snapshot and
fed into the flash facing check downstream. Harmless today per the
report, but left to grow it risks quietly losing precision wherever an
unbounded angle gets used next. Wrapped to (-pi, pi] at the boundary
instead.

Scope note: this closes the reported exploit (which used connections
carrying no cookie at all) by requiring a session the page minted. It
does not defend against a client that fabricates its own cookie
value — no session cookie in this app is cryptographically signed, the
same trust model as everywhere else on this forge. Raising that bar
further (signed sessions, a server-side allow-list) felt like more
than this fix needed.

Regression tests: a direct test that two takeSeat() calls for the same
session before any attach() land on the same player (fails on the old
code — two distinct seats on opposite teams); an end-to-end test that
a real /ws connection with no cookie gets a spectator payload with no
`me`/`players` and never flips a bot to human; and a setInput test for
the angle wrap. client.test.ts's own websocket now forwards the cookie
its own page fetch received, the way an actual browser would — it
previously connected with no cookie either, which the fix would
otherwise have turned into a spectator and broken that test for the
wrong reason.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Pike committed 71fdca9 Browse files
6 files changed +136 −8
src/arena.ts +48 −1
@@ -65,7 +65,16 @@ function freeName(want: string, exceptId: string): string {
6565 return want;
6666 }
6767
68 /** Sit somebody down in the emptiest team, replacing a bot where it stands. */
68 /**
69 * Sit somebody down in the emptiest team, replacing a bot where it stands.
70 *
71 * The claim goes into `seats` right here, synchronously, before the caller
72 * ever opens a socket. A websocket upgrade completes on a later tick, so two
73 * requests for the same session arriving close together both used to read
74 * `seats` before either had written to it, and both walked away with their
75 * own seat on opposite teams — one client, two sides, the vision of both.
76 * Claiming inline closes that window: the second call sees the first's claim.
77 */
6978 export function takeSeat(session: string, name: string): Player | null {
7079 const existing = seats.get(session);
7180 const held = existing ? findPlayer(existing.playerId) : undefined;
@@ -83,9 +92,11 @@ export function takeSeat(session: string, name: string): Player | null {
8392 seat.name = freeName(name, seat.id);
8493 seat.think = 0;
8594 seat.input = { mx: 0, my: 0, angle: seat.angle, fire: false, use: false, reload: false, cast: null };
95 seats.set(session, { playerId: seat.id, token: "", send: () => {} });
8696 return seat;
8797 }
8898
99 /** The socket that actually opened fills in how to reach it and its token. */
89100 export function attach(session: string, playerId: string, token: string, send: (data: string) => void): void {
90101 seats.set(session, { playerId, token, send });
91102 }
@@ -115,6 +126,35 @@ const stillHeld = (playerId: string): boolean =>
115126
116127 export const seatCount = (): number => seats.size;
117128
129 // -------------------------------------------------------------- spectators
130
131 // A connection that never carried a session the page minted gets no side and
132 // no seat: only the public state everyone already sees on the page itself.
133 const spectators = new Map<string, (data: string) => void>();
134
135 export function spectatorSnapshot(): string {
136 return JSON.stringify({
137 t: "s",
138 spectator: true,
139 phase: state.phase,
140 clock: round2(Math.max(0, state.clock)),
141 round: state.round,
142 score: state.score,
143 attackers: state.attackers,
144 result: state.result,
145 events: state.events.slice(0, 5).map((e) => e.text),
146 });
147 }
148
149 export function watch(token: string, send: (data: string) => void): void {
150 spectators.set(token, send);
151 send(spectatorSnapshot());
152 }
153
154 export function unwatch(token: string): void {
155 spectators.delete(token);
156 }
157
118158 // ------------------------------------------------------------- the commands
119159
120160 export type Command = { t: string; [key: string]: unknown };
@@ -297,6 +337,13 @@ function step(): void {
297337 // handler gives the seat back.
298338 }
299339 }
340 for (const send of spectators.values()) {
341 try {
342 send(spectatorSnapshot());
343 } catch {
344 // Same story: the close handler drops it from the set.
345 }
346 }
300347 }
301348
302349 let timer: ReturnType<typeof setInterval> | null = null;
src/client.test.ts +16 −1
@@ -149,6 +149,15 @@ const realFetch = globalThis.fetch;
149149 globalThis.fetch = ((input: string, init?: RequestInit) =>
150150 realFetch(typeof input === "string" && input.startsWith("/") ? base + input : input, init)) as typeof fetch;
151151
152 // A real browser attaches the session cookie to the websocket handshake on
153 // its own; this stub is not a browser, so it carries the cookie the page's
154 // own fetch was handed, the way one actually would.
155 let sessionCookie = "";
156 const RealWebSocket = globalThis.WebSocket;
157 globalThis.WebSocket = function (url: string) {
158 return new RealWebSocket(url, { headers: { cookie: sessionCookie } } as unknown as string[]);
159 } as unknown as typeof WebSocket;
160
152161 const runFrame = () => {
153162 const pending = frames.splice(0, frames.length);
154163 for (const fn of pending) fn(performance.now());
@@ -166,7 +175,9 @@ const until = async (what: () => boolean, ms = 4000) => {
166175 // ------------------------------------------------------------------ the run
167176
168177 test("the bundled client connects, paints the HUD and draws a frame", async () => {
169 const page = await (await fetch(base + "/")).text();
178 const first = await fetch(base + "/");
179 sessionCookie = (first.headers.get("set-cookie") ?? "").split(";")[0] ?? "";
180 const page = await first.text();
170181 const src = /src="(\/static\/val\.[0-9a-f]+\.js)"/.exec(page)![1]!;
171182 const script = await (await fetch(base + src)).text();
172183 const path = join(process.env.FORGE_APP_DATA!, "client.mjs");
@@ -174,6 +185,10 @@ test("the bundled client connects, paints the HUD and draws a frame", async () =
174185
175186 await import(path);
176187 expect(await until(() => nodes.get("conn")!.textContent === "live")).toBe(true);
188 // The one socket this module opens is already under way; put the global
189 // back so another test file's raw `new WebSocket(...)` is not silently
190 // handed a cookie this test happened to pick up.
191 globalThis.WebSocket = RealWebSocket;
177192 expect(await until(() => String(nodes.get("hp")!.textContent).includes("hp"))).toBe(true);
178193
179194 // The HUD says what the match says.
src/match.test.ts +12
@@ -12,6 +12,7 @@ import {
1212 newMatch,
1313 PLANT_TIME,
1414 ROUND_TIME,
15 setInput,
1516 SWAP_AFTER,
1617 tick,
1718 visible,
@@ -393,6 +394,17 @@ test("a seat made on its own still gets a name of its own", () => {
393394 expect(names.size).toBe(10);
394395 });
395396
397 test("an aim angle from a client is kept in range, not taken as given", () => {
398 const state = newMatch(9);
399 const player = state.players[0]!;
400 setInput(player, { angle: 99999 });
401 expect(player.input.angle).toBeGreaterThanOrEqual(-Math.PI);
402 expect(player.input.angle).toBeLessThanOrEqual(Math.PI);
403 // A plain, already-sane angle is unchanged in any way that matters.
404 setInput(player, { angle: 1.2 });
405 expect(player.input.angle).toBeCloseTo(1.2, 9);
406 });
407
396408 test("the deal is the seeded shuffle, so the same match replays the same", () => {
397409 expect(newMatch(77).players.map((p) => p.name)).toEqual(newMatch(77).players.map((p) => p.name));
398410 expect(newMatch(77).players.map((p) => p.name)).not.toEqual(newMatch(78).players.map((p) => p.name));
src/match.ts +5 −1
@@ -670,11 +670,15 @@ export function visible(state: MatchState, viewer: Player, other: Player): boole
670670 return !sightBlocked(viewer, other, state.zones);
671671 }
672672
673 /** Wraps to (-pi, pi]: a client's own claim of where it looks, kept in range
674 * rather than left to grow without bound and lose precision against it. */
675 const normalizeAngle = (angle: number): number => ((angle + Math.PI * 3) % (Math.PI * 2)) - Math.PI;
676
673677 export function setInput(player: Player, input: Partial<Input>): void {
674678 const target = player.input;
675679 if (typeof input.mx === "number" && Number.isFinite(input.mx)) target.mx = Math.max(-1, Math.min(1, input.mx));
676680 if (typeof input.my === "number" && Number.isFinite(input.my)) target.my = Math.max(-1, Math.min(1, input.my));
677 if (typeof input.angle === "number" && Number.isFinite(input.angle)) target.angle = input.angle;
681 if (typeof input.angle === "number" && Number.isFinite(input.angle)) target.angle = normalizeAngle(input.angle);
678682 if (typeof input.fire === "boolean") target.fire = input.fire;
679683 if (typeof input.use === "boolean") target.use = input.use;
680684 if (typeof input.reload === "boolean") target.reload = input.reload;
src/server.test.ts +27
@@ -218,6 +218,33 @@ test("a stale socket closing does not evict a live reconnect's seat", async () =
218218 expect(player.bot).toBe(false);
219219 });
220220
221 test("one session cannot claim two seats on opposite teams", () => {
222 // A websocket upgrade finishes on a later tick than the request that asked
223 // for it, so two requests for the same session used to both read the seat
224 // map before either had written to it — one client, two seats, opposite
225 // sides, the vision of both. Nothing here waits for a socket to open:
226 // the claim has to land the moment a seat is handed out.
227 const first = takeSeat("cheat-session", "Cheat")!;
228 const second = takeSeat("cheat-session", "Cheat")!;
229 expect(second.id).toBe(first.id);
230 expect(match().players.filter((p) => !p.bot && p.name === "Cheat").length).toBe(1);
231 });
232
233 test("a socket with no session the page minted watches instead of playing", async () => {
234 const before = match().players.filter((p) => !p.bot).length;
235 const ws = new WebSocket(base.replace(/^http/, "ws") + "/ws");
236 const first = await new Promise<unknown>((resolve, reject) => {
237 ws.onmessage = (event) => resolve(JSON.parse(String(event.data)));
238 ws.onerror = reject;
239 });
240 expect(first).toMatchObject({ t: "s", spectator: true });
241 expect(first).not.toHaveProperty("me");
242 expect(first).not.toHaveProperty("players");
243 // No bot was converted to serve a connection that carried no identity.
244 expect(match().players.filter((p) => !p.bot).length).toBe(before);
245 ws.close();
246 });
247
221248 test("two people who pick the same name do not end up with the same name", () => {
222249 // Everything that addresses a player — the kill feed, the round log, the
223250 // scoreboard — does it by name, so the match may never hold two of one.
src/server.ts +28 −5
@@ -10,7 +10,19 @@
1010 import { createHash, randomUUID } from "node:crypto";
1111 import { join } from "node:path";
1212 import { AGENTS, SLOTS } from "./abilities.ts";
13 import { attach, command, findPlayer, match, release, snapshot, startArena, takeSeat, type Command } from "./arena.ts";
13 import {
14 attach,
15 command,
16 findPlayer,
17 match,
18 release,
19 snapshot,
20 startArena,
21 takeSeat,
22 unwatch,
23 watch,
24 type Command,
25 } from "./arena.ts";
1426 import { recentResults } from "./db.ts";
1527 import { gridRows, TILE } from "./map.ts";
1628 import { gamePage } from "./pages/game.ts";
@@ -145,7 +157,7 @@ const cleanName = (raw: unknown): string => {
145157 const setCookie = (name: string, value: string, httpOnly: boolean): string =>
146158 `${name}=${encodeURIComponent(value)}; Path=/; SameSite=Lax; Max-Age=604800${httpOnly ? "; HttpOnly" : ""}${publicOrigin.startsWith("https:") ? "; Secure" : ""}`;
147159
148 type SocketData = { session: string; playerId: string; token: string };
160 type SocketData = { session: string; playerId: string; token: string; spectator: boolean };
149161
150162 // ---------------------------------------------------------------- routing
151163
@@ -175,13 +187,21 @@ export const server = Bun.serve<SocketData, Record<string, never>>({
175187 }
176188
177189 // The socket: one seat per session, taken from a bot where it stands.
190 // A connection with no session the page minted gets no seat at all — the
191 // page always carries the cookie by the time its own script opens this,
192 // so this is a raw connection that skipped it, and it watches instead of
193 // playing, on both sides of the vision a seat would otherwise hand it.
178194 if (url.pathname === "/ws") {
179195 if (!sameOrigin(request)) return new Response("forbidden", { status: 403, headers: SECURITY });
196 const token = randomUUID();
197 if (fresh) {
198 const upgraded = self.upgrade(request, { data: { session, playerId: "", token, spectator: true } });
199 return upgraded ? undefined : new Response("expected a websocket", { status: 426, headers: SECURITY });
200 }
180201 const name = cleanName(jar[NAME]) || `Guest-${session.slice(0, 4)}`;
181202 const player = takeSeat(session, name);
182203 if (!player) return new Response("no seats", { status: 503, headers: SECURITY });
183 const token = randomUUID();
184 const upgraded = self.upgrade(request, { data: { session, playerId: player.id, token } });
204 const upgraded = self.upgrade(request, { data: { session, playerId: player.id, token, spectator: false } });
185205 return upgraded ? undefined : new Response("expected a websocket", { status: 426, headers: SECURITY });
186206 }
187207
@@ -265,11 +285,13 @@ export const server = Bun.serve<SocketData, Record<string, never>>({
265285 websocket: {
266286 maxPayloadLength: 4 * 1024,
267287 open(socket) {
268 const { session, playerId, token } = socket.data;
288 const { session, playerId, token, spectator } = socket.data;
289 if (spectator) return void watch(token, (data) => socket.send(data));
269290 attach(session, playerId, token, (data) => socket.send(data));
270291 socket.send(snapshot(playerId));
271292 },
272293 message(socket, raw) {
294 if (socket.data.spectator) return; // a watcher has no seat to drive
273295 // Anything a client sends is a claim: it is parsed, bounded and given
274296 // to the arena, which decides what it is allowed to mean.
275297 if (typeof raw !== "string" || raw.length > 4096) return;
@@ -283,6 +305,7 @@ export const server = Bun.serve<SocketData, Record<string, never>>({
283305 command(socket.data.playerId, parsed as Command);
284306 },
285307 close(socket) {
308 if (socket.data.spectator) return void unwatch(socket.data.token);
286309 release(socket.data.session, socket.data.token);
287310 },
288311 },
↑↓ navigate ↵ open esc close

Keyboard shortcuts