fix(gameserver): P0 security & integrity hardening #46

Merged

marco merged 1 commit from fix/p0-security-hardening into main

2026-07-18 23:09:16 +02:00

marco commented

2026-07-18 23:04:56 +02:00

Owner

The four HIGH security/integrity findings from the critical code review (report in chat). All are server-authority / anti-tamper / data-integrity.

Enum DoS — a client sending {"type":"move","direction":99} deserialized to an undefined Direction and threw inside the shared World.Tick(), aborting the tick for everyone (re-sendable → repeatable world freeze). Fixed at the boundary: JsonStringEnumConverter(allowIntegerValues:false) rejects out-of-range enums at parse (covers all wire enums) + Enum.IsDefined guard on moves.
Slow-client head-of-line stall — the tick awaited each send inline with no timeout, so one client that stopped reading froze the whole simulation. WebSocketConnection now has a bounded outbound queue drained by a background pump (RunSendPumpAsync); sends are non-blocking enqueues, overflow (256 behind) closes the connection, and the Closed token ends the receive loop so a dead send side tears the session down.
Reconnect race → zombie session — the disconnect finally freed the connection slot before tearing down world state, so a reconnect could TryAdd and have its fresh state wiped. Now teardown runs while still holding the slot, which is released last and identity-checked (ConnectionManager.TryRemove(id, connection)).
Persist data loss — WorldPersister re-read inventory/skills lazily per player after snapshotting the online set, so a mid-flush disconnect made it persist a blanked record (items + skills wiped on next login). Now one atomic World.PersistableSnapshot() captures position+inventory+skills under a single lock.

Regression tests: enum rejection (ProtocolJsonTests), outbound-queue overflow→close (WebSocketConnectionTests), identity-checked removal (ConnectionManagerTests), atomic snapshot (WorldTests). Whole suite green (GameServer 113), 0 warnings, csharpier clean.

Definition of Done

A crafted out-of-range enum over the wire cannot throw in the tick loop (rejected at parse; guarded on move).
A client that stops reading its socket no longer stalls the tick — it is dropped after falling 256 messages behind.
The disconnect teardown releases the connection slot only after world teardown, identity-checked, so a reconnect is never wiped.
The periodic persister writes an atomic per-player record; a mid-flush disconnect cannot blank inventory/skills.
Regression tests cover all four; suite green, 0 warnings.

The four **HIGH** security/integrity findings from the critical code review (report in chat). All are server-authority / anti-tamper / data-integrity. 1. **Enum DoS** — a client sending `{"type":"move","direction":99}` deserialized to an undefined `Direction` and threw inside the shared `World.Tick()`, aborting the tick for everyone (re-sendable → repeatable world freeze). Fixed at the boundary: `JsonStringEnumConverter(allowIntegerValues:false)` rejects out-of-range enums at parse (covers all wire enums) + `Enum.IsDefined` guard on moves. 2. **Slow-client head-of-line stall** — the tick awaited each send inline with no timeout, so one client that stopped reading froze the whole simulation. `WebSocketConnection` now has a bounded outbound queue drained by a background pump (`RunSendPumpAsync`); sends are non-blocking enqueues, overflow (256 behind) closes the connection, and the `Closed` token ends the receive loop so a dead send side tears the session down. 3. **Reconnect race → zombie session** — the disconnect `finally` freed the connection slot *before* tearing down world state, so a reconnect could `TryAdd` and have its fresh state wiped. Now teardown runs while still holding the slot, which is released **last** and identity-checked (`ConnectionManager.TryRemove(id, connection)`). 4. **Persist data loss** — `WorldPersister` re-read inventory/skills lazily per player after snapshotting the online set, so a mid-flush disconnect made it persist a blanked record (items + skills wiped on next login). Now one atomic `World.PersistableSnapshot()` captures position+inventory+skills under a single lock. Regression tests: enum rejection (`ProtocolJsonTests`), outbound-queue overflow→close (`WebSocketConnectionTests`), identity-checked removal (`ConnectionManagerTests`), atomic snapshot (`WorldTests`). Whole suite green (GameServer 113), 0 warnings, csharpier clean. ## Definition of Done - [x] A crafted out-of-range enum over the wire cannot throw in the tick loop (rejected at parse; guarded on move). - [x] A client that stops reading its socket no longer stalls the tick — it is dropped after falling 256 messages behind. - [x] The disconnect teardown releases the connection slot only after world teardown, identity-checked, so a reconnect is never wiped. - [x] The periodic persister writes an atomic per-player record; a mid-flush disconnect cannot blank inventory/skills. - [x] Regression tests cover all four; suite green, 0 warnings.

marco added 1 commit

2026-07-18 23:04:56 +02:00

fix(gameserver): P0 security & integrity hardening

ci / Lint & Test (pull_request) Successful in 1m46s

Details

e6e1a87578

Four findings from the code review, all server-authority / integrity:

- Enum DoS: reject out-of-range wire enums at parse (JsonStringEnumConverter
  allowIntegerValues:false) + Enum.IsDefined guard on moves, so a crafted
  {"direction":99} can no longer throw inside the shared tick and freeze the world.
- Slow-client stall: WebSocketConnection now has a bounded outbound queue drained by a
  background pump; sends never touch the socket inline, so one stuck client can't block
  the tick loop. Overflow closes the connection (Closed token also ends the receive loop).
- Reconnect race: tear down world/interest state while still holding the connection slot
  and release it last, identity-checked (ConnectionManager.TryRemove(id, connection)), so
  a reconnect can't have its fresh state wiped by the old session's teardown.
- Persist data loss: WorldPersister reads position+inventory+skills via one atomic
  World.PersistableSnapshot() instead of lazy per-player re-reads, so a mid-flush
  disconnect can't blank a player's record.

marco merged commit fda4eee56f into main

2026-07-18 23:09:16 +02:00

marco deleted branch fix/p0-security-hardening

2026-07-18 23:09:16 +02:00

marco referenced this pull request from a commit

2026-07-18 23:09:17 +02:00

fix(gameserver): P0 security & integrity hardening (#46)