From 0cb1b749522aee7851853c6b387077ff23d30c29 Mon Sep 17 00:00:00 2001 From: tiennm99 Date: Tue, 29 Sep 2026 20:33:16 +0700 Subject: [PATCH] docs(reports): record the whole-project review and fixes --- ...eviewer-260929-1939-security-ops-review.md | 429 ++++++++++++++++++ ...reviewer-260929-1939-server-core-review.md | 132 ++++++ .../code-reviewer-260929-1939-web-review.md | 139 ++++++ .../code-reviewer-260929-1939-wsapi-review.md | 277 +++++++++++ ...-developer-260929-1939-server-ops-fixes.md | 81 ++++ ...llstack-developer-260929-1939-web-fixes.md | 132 ++++++ ...stack-developer-260929-1939-wsapi-fixes.md | 70 +++ ...929-1939-whole-project-review-and-fixes.md | 84 ++++ 8 files changed, 1344 insertions(+) create mode 100644 plans/reports/code-reviewer-260929-1939-security-ops-review.md create mode 100644 plans/reports/code-reviewer-260929-1939-server-core-review.md create mode 100644 plans/reports/code-reviewer-260929-1939-web-review.md create mode 100644 plans/reports/code-reviewer-260929-1939-wsapi-review.md create mode 100644 plans/reports/fullstack-developer-260929-1939-server-ops-fixes.md create mode 100644 plans/reports/fullstack-developer-260929-1939-web-fixes.md create mode 100644 plans/reports/fullstack-developer-260929-1939-wsapi-fixes.md create mode 100644 plans/reports/review-260929-1939-whole-project-review-and-fixes.md diff --git a/plans/reports/code-reviewer-260929-1939-security-ops-review.md b/plans/reports/code-reviewer-260929-1939-security-ops-review.md new file mode 100644 index 0000000..12d7daa --- /dev/null +++ b/plans/reports/code-reviewer-260929-1939-security-ops-review.md @@ -0,0 +1,429 @@ +# Security, dependency and operations review + +Date: 2026-09-29. Branch `dev` at `d3eb13e`. Read-only review; no project file changed +except this report. + +Threat model used: a public, internet-facing hobby game server behind Coolify/Traefik. No +accounts, no payments, no ambient credentials (no cookies; the resume token lives in +`localStorage` and is sent inside the protocol). The only personal data is a nickname and +chat text. What an attacker can realistically take from this service is **availability** +(everyone else's ability to play), **log/disk hygiene**, and **what strangers see rendered**. +Findings are ranked against that, not against a generic checklist. + +Prior decisions respected (from `plans/reports/fullstack-developer-260921-0027-server-ops-observability.md` +and `docs/deployment.md`): trusted-proxy mode is opt-in; per-IP connection cap is off by +default because of NAT/CGNAT; rejected/reported words are logged normalised and capped; the +upstream dump is deliberately unpinned; `/debug/vars` lives on a separate listener; moving +major tags over exact pins (no SHA pinning recommended here). + +## Scope + +- `server/cmd/noitu-server/main.go` +- `server/internal/wsapi/` abuse surfaces only: `server.go`, `session.go`, `dispatch.go`, + `codec.go`, `ratelimit.go`, `nickname.go`, `hub.go`, `room_chat.go`, parts of `room.go` / + `room_game.go` (log lines only) +- `Dockerfile`, `.dockerignore`, `.github/workflows/ci.yml`, `.github/workflows/proto.yml`, + `.github/dependabot.yml`, `Makefile`, `docs/deployment.md` +- `server/go.mod`, `web/package.json`, `web/package-lock.json` +- Licensing: `LICENSE`, `NOTICE`, `data/LICENSE`, `data/ATTRIBUTION.md`, README licence + section, `web/src/lib/components/AttributionFooter.svelte` +- GitHub repo settings (read via `gh api`) + +## Commands run and results + +| Check | Result | +|---|---| +| `cd server && go list -m -u all \| grep '\['` | Updates available, none security-flagged: `modernc.org/sqlite` 1.58.0 -> 1.60.1, `modernc.org/libc` 1.75.6 -> 1.77.1, `golang.org/x/text` 0.41.0 -> 0.42.0, `golang.org/x/sys` 0.47.0 -> 0.48.0, `dustin/go-humanize` 1.0.1 -> 1.1.0, plus tool-only modules (`x/tools`, `x/mod`, `x/sync`, `modernc.org/cc,ccgo,gc`, `google/pprof`, deprecated `golang/protobuf` 1.5.0 as an indirect of the protoc tool) | +| `govulncheck ./...` (installed binary) | Fails: the binary was built with go1.26 and the host Go is go1.27.1. Not a project defect. | +| `go run golang.org/x/vuln/cmd/govulncheck@latest ./...` (v1.8.0, DB updated 2026-09-28, go1.27.1 stdlib) | **No vulnerabilities found** (12 modules scanned) | +| `cd web && npm audit --omit=dev` | **0 vulnerabilities** (the only runtime dependency is `@bufbuild/protobuf`) | +| `cd web && npm audit` | 5 findings (3 low, 2 moderate), two real advisories, both dev-only; see "Dependency advisories" | +| Live probe: built `noitu-server` in scratchpad, `NOITU_MAX_CONNECTIONS=2`, opened 2 sockets that never send `Hello`, waited 65 s (past two keepalive rounds) | Both sockets still alive; a third upgrade got **HTTP 503**. Confirms finding H1. Server process stopped afterwards. | +| `curl` of `/healthz`, `/readyz`, `/version` on the probe server | 200s; no security headers on any response (L3) | +| `gh api repos/tiennm99/noitu/...` | Public repo; default workflow token permission `write`, Actions may approve PRs; vulnerability alerts disabled (404), Dependabot security updates disabled, secret scanning disabled | +| `git ls-tree origin/main -- .github` | `dependabot.yml` is **not** on `main` (only on `dev`) | +| `gh api repos//releases/latest` | checkout v7.0.1, setup-go v7.0.0, setup-node v7.0.0, upload-artifact v7.0.1, golangci-lint-action v9.3.0; `bufbuild/buf-setup-action` is **archived** | + +## Findings + +Legend for "Action": **Fix now**, **Document as accepted**, **Non-issue** (under this threat model). + +| ID | Sev | Location | Action | Title | +|---|---|---|---|---| +| H1 | High | `server/internal/wsapi/server.go:163-177`, `session.go:290-354`, `dispatch.go:164-170`, `session.go:171` | Fix now | One client can exhaust the global connection and room ceilings: no per-IP default, no Hello deadline, room budget is per socket | +| M1 | Med | `docs/deployment.md:93-176` | Fix now (docs + live config) | No Coolify/Traefik recipe; the real deployment likely runs with one shared join bucket and no per-IP cap | +| M2 | Med | `Dockerfile:67-87`, `docs/deployment.md:241-271` | Fix now | Distroless image cannot pass a Coolify HTTP health check, and a drain longer than the container stop grace ends in SIGKILL | +| M3 | Med | `.github/dependabot.yml` (dev only), repo settings, `ci.yml:26,27,69,97,101,124` | Fix now | Dependabot is not running at all; alerts/security updates/secret scanning off; actions three majors behind | +| L1 | Low | `server/internal/wsapi/server.go:268-291,336-342` | Fix now | IPv6 clients are keyed on the full /128, so every per-IP limit is free to bypass from one /64 | +| L2 | Low | `room_game.go:223,239,288-300`, `dispatch.go:343-344` | Fix now | `word_rejected` / `word_reported` have no aggregate bound; one script can flood the log | +| L3 | Low | `server/internal/wsapi/server.go:209-247` | Fix now | No security response headers (framing, sniffing, referrer) | +| L4 | Low | `.github/workflows/proto.yml:213-215` | Fix now | `bufbuild/buf-setup-action` is archived | +| L5 | Low | `Dockerfile:73-79`, `ci.yml:162`, `AttributionFooter.svelte:10-23` | Fix now (small) / Document | Apache-2.0 `LICENSE` and third-party notices not in the image; UI credit does not point at the modification record | +| L6 | Low | `ci.yml:27-29,97-100`, `proto.yml:217-220` | Fix now | CI tests on Go 1.25.0 while the image ships Go 1.27.x; no vuln scan in CI | +| L7 | Low | GitHub repo settings | Fix now | Default `GITHUB_TOKEN` is `write` and Actions may approve PRs | +| L8 | Low | `server/cmd/noitu-server/main.go:104-108` | Fix now | Public `http.Server` has no `IdleTimeout` | +| N1 | Nit | `server/internal/wsapi/nickname.go:62-72` | Document or fix | Blank-rendering letters (U+3164, U+115F, U+2800, ...) survive the sanitiser | +| N2 | Nit | `main.go:168-175`, `docs/deployment.md:180-189` | Document | `NOITU_DEBUG_ADDR` inside a container is reachable from the whole Docker network | +| N3 | Nit | `.dockerignore` | Fix now | `.claude/` and `.env*` are not excluded from the build context | +| N4 | Nit | `ci.yml:26,68,96,138`, `proto.yml:205` | Fix now | `actions/checkout` persists the token into `.git/config` before `npm ci` runs dependency scripts | +| N5 | Nit | `Dockerfile:67` | Optional | `static-debian12` is the previous distroless base; `static-debian13` exists | +| N6 | Nit | `session.go:334-343` | Non-issue | Client control-frame pings bypass the frame limiter | +| D1 | Low | `web/package-lock.json` (`cookie@0.6.0` via `@sveltejs/kit@2.70.3`) | Non-issue | GHSA-pxg6-pf52-xh8x, unreachable | +| D2 | Low | `web/package-lock.json` (`vitest@3.2.7`, `@vitest/mocker`) | Non-issue for prod; bump when offered | GHSA-82fw-gwwq-j7x9, dev-only | + +--- + +### H1 (High, fix now): one client can take the whole server offline + +**Where.** `server/internal/wsapi/server.go:163-177` (global cap, per-IP cap off unless +configured), `session.go:290-354` (reads have no deadline; nothing closes a socket that never +says `Hello`), `session.go:171` + `dispatch.go:164-170` (room budget `roomLimiter` is per +session, so it resets on reconnect). + +**What is wrong.** The two process-wide ceilings (`NOITU_MAX_CONNECTIONS`=2000, +`NOITU_MAX_ROOMS`=1000) exist to protect memory, but nothing stops a single address from +spending all of them: + +1. The per-IP connection cap defaults to off (a deliberate decision, for CGNAT), and behind + the proxy it *cannot* be turned on until trusted-proxy mode is on. +2. A socket that upgrades and never sends `Hello` is held forever. `readLoop` has no deadline + by design, and the keepalive only proves the peer is alive; every WebSocket library + answers pings automatically. Verified live: two silent sockets were still open after 65 s + and the next upgrade got 503. +3. Past `Hello`, an idle session in no room is also held forever. +4. The room budget (`roomsPerSecond`=0.2, burst 5) sits on the session, not the address. + Reconnecting gets a fresh budget, and one connection holds roughly one lobby, so about 1000 + connections from one host fill `MaxRooms`. + +**Scenario.** A 20-line script on one laptop opens 2000 sockets and sends nothing. Every real +player's browser now gets `503 server full` on `/ws` and sits on "Đang kết nối…" until the +script stops. The variant that opens 1000 lobbies makes every "create room / bot game / quick +match" answer `server_full`. It needs no bandwidth, no amplification and no skill. Under this +threat model availability is the asset, so this is the finding that matters most. + +**Fix (small).** +- Add a handshake deadline: in `session.run`, arm `time.AfterFunc(helloTimeout, ...)` (for + example 10 s) that calls `s.close()` unless the handshake finished. Stop it in `handleHello`. + `greeted` is dispatch-only, so either stop the timer there or read an `atomic.Bool`. Add a + test next to the limits tests. +- Deployment: set `NOITU_TRUSTED_PROXIES` to the Traefik network (see M1) and + `NOITU_MAX_CONNECTIONS_PER_IP` to a CGNAT-tolerant value (for example 32). Consider + making a non-zero per-IP default apply automatically whenever `TrustedProxies` is non-empty. + That keeps the NAT reasoning intact, because the cap only applies once the address is the + real client. +- Key the room-creation budget on the address as well, the same way `joinLimiter` is keyed: + a `hub.roomLimiter *keyedLimiter` alongside the per-session bucket, swept by + `sweepLimiters`. +- Optional: close a greeted session that has sat in no room and sent no frame for, say, + 15 minutes. + +### M1 (Med, fix now): the documented proxies are nginx and Caddy, the real one is Traefik + +**Where.** `docs/deployment.md:93-176`. No mention of Coolify or Traefik anywhere in `README.md` +or `docs/`. + +**What is wrong.** The client-address section is correct, but it only tells an operator what +to do for nginx and Caddy. The deployment target is Coolify/Traefik. If +`NOITU_TRUSTED_PROXIES` is unset there (the default), the docs themselves say the result: +every player shares Traefik's address, so they share one join bucket (`joinsPerSecond`=5, +burst 20), and H1's per-IP cap cannot be enabled. + +**Scenario.** One client sends `JoinRoom` with random codes at 5/s, well under the 20/s frame +limit, so it is never disconnected. Every other player trying to join a friend's room by code +gets `too_many_attempts` for as long as the loop runs. The same client can also brute-force +room codes with the whole server's budget. + +**Fix.** Add a short "Coolify / Traefik" subsection: +- Traefik, with its default `forwardedHeaders` (no `trustedIPs`), strips client-sent + `X-Forwarded-*` and appends the real peer, so it is safe to trust. Set + `NOITU_TRUSTED_PROXIES` to the Docker network the Traefik container reaches the app on + (`docker network inspect coolify`, or the app's own network), never a public range. +- If Cloudflare or another CDN sits in front, either configure Traefik + `forwardedHeaders.trustedIPs` for the CDN ranges or list them here too. Otherwise every + player keys on a CDN edge address. +- Then set `NOITU_MAX_CONNECTIONS_PER_IP`. +- Confirm in the live Coolify app that this is actually set (unresolved question 1). + +### M2 (Med, fix now): health checks and drain do not fit the Coolify lifecycle + +**Where.** `Dockerfile:67-87` (distroless, no `HEALTHCHECK`), `docs/deployment.md:219-271`. + +**What is wrong.** +1. Coolify runs its dashboard HTTP health check from **inside** the container with + `curl`/`wget` (per the Coolify health-check docs). The distroless image has neither, so + the check fails. The operator then has to disable it, and without it a rolling update does + not wait for the new container before removing the old one. +2. The docs suggest `NOITU_DRAIN_TIMEOUT=60s` but never mention that the container runtime + stop grace is what actually bounds it. Docker defaults to 10 s; check what Coolify uses. + Past the grace the process gets SIGKILL: no `server_restarting` notice, no final log line. + That is exactly the behaviour drain mode was built to remove. +3. `/readyz` flipping to 503 only helps if something polls it. Traefik's Docker provider + keeps routing to a still-running container unless a Traefik health check is configured, + so during a drain new players can land on the old instance and be refused with + `server_restarting` while the new one is already up. + +**Fix.** +- Add a `-healthcheck` mode to `noitu-server` (a GET to `http://127.0.0.1$NOITU_ADDR/healthz`, + exit 0/1), plus + `HEALTHCHECK CMD ["/app/noitu-server","-healthcheck"]` in the Dockerfile. It works on + distroless and Coolify honours a Dockerfile `HEALTHCHECK`. +- Document: keep `NOITU_DRAIN_TIMEOUT` below the stop grace (or raise the grace in Coolify), + and optionally point a Traefik load-balancer health check + (`traefik.http.services..loadbalancer.healthcheck.path=/readyz`) at `/readyz`. + +### M3 (Med, fix now): the dependency-update bot is configured but not running + +**Where.** `.github/dependabot.yml` exists on `dev` only (added in `8223f40`, 2026-09-21); +`origin/main` has no such file. Repo settings: vulnerability alerts disabled, Dependabot +security updates disabled, secret scanning and push protection disabled. + +**What is wrong.** Dependabot version updates read their configuration from the default +branch only, so none of the four ecosystems is being watched. The house rule ("moving tag +over exact pin, a bot is the mechanism that rule assumes", `dependabot.yml:1-4`) is not +actually being served. Evidence: every workflow is on `checkout@v4`, `setup-go@v5`, +`setup-node@v4` and `upload-artifact@v4`, while v7 of each shipped in April-July 2026. There +are no open PRs. + +**Scenario.** The next Go stdlib or `coder/websocket` advisory, or a Node base-image CVE, +arrives and nothing opens a PR or an alert. The repo is public, so these features are free. + +**Fix.** Merge `dev` into `main` (or cherry-pick `dependabot.yml`). Enable Dependabot alerts, +security updates, secret scanning and push protection in the repo settings. Then accept the +actions major bumps Dependabot proposes. The groups only batch minor/patch, so majors arrive +as individual PRs, which is correct. + +### L1 (Low, fix now): IPv6 keys on /128 + +**Where.** `server/internal/wsapi/server.go:268-291` (`clientIP` returns the address verbatim), +`server.go:336-342`. + +**What is wrong.** Every per-address control (join limiter, per-IP connection cap, and the +per-IP room budget recommended in H1) keys on the literal address. A single IPv6 host +normally controls a /64 (2^64 addresses), and source-address rotation is trivial. + +**Scenario.** With 1000 live rooms the chance of guessing a code is about 1.1e-6 per +attempt. One /64 at 2000 sockets x 5 joins/s finds a stranger's private lobby every couple of +minutes, and fills the per-IP cap once per address. The impact is limited, because the host +can kick and joins are refused mid-game, but it voids the limiter's "centuries" claim +(`session.go:53-61`). + +**Fix.** Add a `limiterKey(ip string) string` that returns the IPv4 address unchanged, maps +IPv4-mapped IPv6 back to IPv4, and returns the `/64` prefix string for IPv6. Use it for +`remoteIP` and `reserveIP`/`releaseIP`. This is about 10 lines plus a table test. + +### L2 (Low, fix now): corpus log lines are bounded per session, not in total + +**Where.** `room_game.go:223` and `:239` (`recordRejection` on every rejection, including +not-your-turn), `room_game.go:288-300`, `dispatch.go:343-344`. + +**What is wrong.** Each rejected submission writes one Info line. The per-session submit +limiter (5/s) is the only bound. The 20-distinct-reports cap is per session and resets on +reconnect. Rejections do not eliminate a player, so a bot game can be fed garbage +indefinitely. + +**Scenario.** 2000 sockets x 5 rejected words/s is about 10k lines/s. Even with the host's +Docker log rotation (Coolify normally sets `max-size`, so verify it), the real corpus signal +the lines exist for is rotated out within minutes. Without rotation, the disk fills. + +**Fix.** Put one process-wide `bucket` (for example 20 lines/s, burst 100) in front of both +log calls. Count suppressed lines in a new `noitu_corpus_log_suppressed` expvar so an +operator can see it happened. The metrics stay exact; only the log is sampled. Optionally +drop the not-your-turn reason from the corpus log, since it says nothing about the dictionary. + +### L3 (Low, fix now): no security headers + +**Where.** `server/internal/wsapi/server.go:209-247` (static handler), `:111-131`. + +**What is wrong.** Responses carry none of `X-Content-Type-Options`, a framing policy, or +`Referrer-Policy`. Traefik/Coolify add none by default. + +**Scenario.** A hostile page frames the game and overlays a decoy to trick a host into +clicking "kick" or "resign". Impact is low because there are no accounts and nothing of +value, but the fix is three lines. + +**Fix.** In `mountStatic`'s handler (and the small text endpoints) set +`X-Content-Type-Options: nosniff`, `Content-Security-Policy: frame-ancestors 'self'` and +`Referrer-Policy: no-referrer`. A full script CSP can come later through SvelteKit's `kit.csp` +(hash mode works for prerendered pages). Leave HSTS to the proxy, and document that. + +### L4 (Low, fix now): archived action in the proto workflow + +**Where.** `.github/workflows/proto.yml:213-215`. + +**What is wrong.** `bufbuild/buf-setup-action` is archived: no fixes and no Node runtime +bumps. Buf's replacement is `bufbuild/buf-action`. + +**Fix.** `uses: bufbuild/buf-action@v1` with `setup_only: true` (and `version: latest` if you +want to keep that behaviour). Keep the existing `buf lint` / `buf breaking` / `buf generate` +steps. Moving major tag, in line with the house rule. + +### L5 (Low): licence files in the image and the UI credit + +**Where.** `Dockerfile:73-79`, `ci.yml:162`, `web/src/lib/components/AttributionFooter.svelte:10-23`. + +**What is met (verified).** The CC BY-SA 4.0 obligation for the data is handled well: +`data/LICENSE` (full 4.0 text), `data/ATTRIBUTION.md` (source, licence, dated provenance via +SHA-256 in `meta`, and a nine-item modification list) and `NOTICE` are copied into the image. +`.dockerignore`'s `*.md` exclusion correctly re-includes `data/ATTRIBUTION.md`. CI asserts all +three plus the database, and asserts that no `.bz2`/`.xml` reached the image. The startup log +prints the licence from `meta`. A credit footer with links to vi.wiktionary.org and the CC BY-SA +4.0 deed is on every page (`+layout.svelte`), and chat and definitions are rendered by +interpolation, not `{@html}`. + +**Gaps.** +1. The root Apache-2.0 `LICENSE` is not copied. `NOTICE` says "See the LICENSE file" and the + image does not contain it. Apache-2.0 section 4(a) asks redistributors to include it. This + only bites if the image is ever published or redistributed, but it is one line. +2. No third-party notices ship. The binary statically links BSD-3 (`x/text`, `protobuf`, + `modernc.org/sqlite`), ISC (`coder/websocket`) and others. The web bundle contains Svelte's + MIT runtime and `@bufbuild/protobuf` (Apache-2.0). Those licences ask for the notice in + binary redistributions. Document as accepted while the image is not published; generate a + `THIRD_PARTY_NOTICES` if it ever is. +3. CC BY-SA 4.0 section 3(a)(1)(B) asks to indicate that the material was modified. The + footer says "dựa trên" (based on), which arguably does that, but the modification record is + only reachable from the repository. Section 3(a)(2) allows satisfying this with a URI, so + add a third link in the footer to `data/ATTRIBUTION.md` on GitHub (for example + "những thay đổi"). + +**Fix.** `COPY LICENSE /app/LICENSE`, add `app/LICENSE` to the CI `for required in ...` +list, add the footer link, and note item 2 as accepted in the README licence section. + +### L6 (Low, fix now): CI verifies a toolchain that does not ship + +**Where.** `ci.yml:27-29`, `ci.yml:97-100`, `proto.yml:217-220` (`go-version-file: server/go.mod` +with `go 1.25.0` and no `toolchain` line); `Dockerfile:19` (`golang:1-alpine`, Go 1.27.x today). + +**What is wrong.** CI tests and lints on Go 1.25.0. That version carries stdlib advisories +fixed in later 1.25.x releases (it would fail a govulncheck of its own stdlib). The shipped +binary is built with whatever `golang:1` is. Race and vet results come from a different +compiler and stdlib than production. No workflow runs `govulncheck` or `npm audit`. + +**Fix.** Use `go-version: stable` (a moving tag, matching `golang:1`) while keeping `go 1.25.0` +as the module minimum. Add a `govulncheck` step (`golang/govulncheck-action@v1` or +`go run golang.org/x/vuln/cmd/govulncheck@latest ./...`) and `npm audit --omit=dev` in the +web job. + +### L7 (Low, fix now): repository Actions defaults + +**Where.** Repo settings: `default_workflow_permissions: write`, +`can_approve_pull_request_reviews: true`. + +**What is wrong.** Both current workflows declare `permissions: contents: read`, so they are +fine today. Any future workflow that forgets the block gets a write token and can approve its +own PRs. + +**Fix.** Settings, then Actions, then Workflow permissions: read-only, and untick "Allow +GitHub Actions to create and approve pull requests". + +### L8 (Low, fix now): no idle timeout on the public listener + +**Where.** `server/cmd/noitu-server/main.go:104-108`. + +**What is wrong.** Only `ReadHeaderTimeout` is set. Idle keep-alive HTTP connections (not +WebSockets) are never reaped. Behind Traefik this is mostly Traefik's pool, but a direct +exposure (a Coolify port mapping, a dev box) lets idle sockets accumulate. + +**Fix.** Add `IdleTimeout: 120 * time.Second`. Do **not** add `ReadTimeout`/`WriteTimeout`: +net/http leaves those deadlines on a hijacked connection, and `coder/websocket` would inherit +them and drop every game after that interval. + +### Nits + +- **N1** `nickname.go:62-72`: `unicode.IsPrint` accepts letters that render as blank (U+3164 + HANGUL FILLER, U+115F/U+1160, U+FFA0, U+2800 BRAILLE BLANK). The result is a nickname or + chat line that looks empty, or one that impersonates "Người chơi" plus padding. Either strip + that short list in the `strings.Map` or require at least one rune that is neither space nor + in it. Low value; document if not fixed. +- **N2** `main.go:168-175`: in a container, `NOITU_DEBUG_ADDR=:6060` is reachable from every + container on the same Docker network, which in Coolify can be the shared `coolify` + network. expvar serves the command line and memstats. Add one sentence to the Observability + section. +- **N3** `.dockerignore`: add `.claude` and `**/.env*`. `web/.claude/` is currently copied into + the `web` build stage by `COPY web/ ./`. It does not reach the final image, but it is in the + build context and cache. +- **N4** `actions/checkout` defaults to `persist-credentials: true`, writing the (read-only) + token into `.git/config` before `npm ci` runs dependency install scripts. Set + `persist-credentials: false`. `proto.yml` needs no push, so it can take the same setting. +- **N5** `Dockerfile:67`: `gcr.io/distroless/static-debian13:nonroot` is the current base. + For a static binary the difference is only CA certificates and tzdata. Dependabot will not + propose this because the suite is in the image name. +- **N6** `session.go:334-343`: client-sent WebSocket ping control frames are answered inside + `coder/websocket` and never reach `frameLimiter`. The cost is one small pong per ping, + bounded by TCP. Non-issue. + +### Dependency advisories + +| Advisory | Package (installed) | Severity | Reachable? | Action | +|---|---|---|---|---| +| GHSA-pxg6-pf52-xh8x | `cookie@0.6.0` via `@sveltejs/kit@2.70.3` (and `adapter-static@3.0.10`) | Low | **No.** The frontend is prerendered by `adapter-static`; the image copies only `web/build` and the Go binary serves the files. No SvelteKit server runtime, and so no `cookie.serialize`, exists in production. | Non-issue. Do **not** run `npm audit fix --force`: its proposed "fix" is `@sveltejs/kit@0.0.30`. If a clean audit is wanted, add `"overrides": {"cookie": "^0.7.0"}` (house rule: security pins live in `overrides`). | +| GHSA-82fw-gwwq-j7x9 | `vitest@3.2.7` / `@vitest/mocker` (range 2.1.0 to 4.1.10) | Moderate | **No** in production (devDependency, test runner only, not in the image). Relevant only on a developer machine or CI running the test server. | Non-issue for the service. Take the vitest major bump when Dependabot offers it (once M3 is fixed). | +| Go modules and stdlib | all | none | govulncheck v1.8.0, DB 2026-09-28: no vulnerabilities in the call graph | Clean. Routine bumps available (`modernc.org/sqlite` 1.60.1, `x/text` 0.42.0, ...), which Dependabot will propose. | + +## Checked and clean + +- **Origin check.** `coder/websocket` defaults to same host (`authenticateOrigin` compares + `Origin` host with `r.Host`, and Traefik preserves Host). With no cookies or ambient auth, + cross-site WebSocket hijacking has nothing to steal anyway. Permessage-deflate is disabled + by default in v1.8.15, so there is no decompression bomb. +- **Frame limits.** `SetReadLimit(4096)`; text frames rejected; unparseable frames close the + socket; a flat 20/s (burst 40) frame limiter closes floods; per-action budgets for + submit, chat, lobby actions, reports and joins. +- **Memory per connection and room.** Outbox capped at 32 frames (session closed, never + grown); chat delivered with `trySend`; chat history 20 lines x 200 runes; reported words at + most 20 per session; `connsByIP` entries deleted at zero; `keyedLimiter` swept every 5 min; + resume tokens expire after the grace window; pre-Hello sockets are never registered in the + hub; quick-match queue cleaned on disconnect; bounded room inbox. Nothing grows without a + bound other than through the ceilings in H1. +- **Room codes.** `crypto/rand` with rejection sampling, 31^6 (about 29.7 bits), fine for + a lobby code given the join limiter (modulo L1). Session IDs and resume tokens are 128-bit + `crypto/rand`. +- **Nickname and chat sanitisation.** NFC first, strips Cc/Cf (bidi overrides, zero-width + characters) and anything non-printable, caps combining marks at 2, collapses whitespace, + rune caps (20/200/64); fuzzed. Rendered by interpolation, never `{@html}` (`ChatPanel.svelte:182`). +- **Logging.** Only normalised, capped words go into `word_rejected`/`word_reported`; no + nickname, chat or raw input. `slog` TextHandler quotes values, so no log injection. Accept + rejections log at Debug only. Client errors are UI keys, never internal strings. +- **Trusted proxies.** Right-to-left `X-Forwarded-For` walk, skipping trusted hops, falling + back to the peer on a malformed hop, IPv4-mapped handling via `Unmap`, unparseable entries + warned and skipped. Opt-in and documented honestly. +- **Debug endpoint.** Separate `http.Server`, only when `NOITU_DEBUG_ADDR` is set. The public + handler is its own mux, so expvar's `DefaultServeMux` registration is never exposed (test + `TestDebugVarsNotOnPublicMux`). +- **Static serving.** Path-boundary check (`underRoot`), directories fall to the SPA shell (no + listing), immutable caching only under `/_app/immutable/`. `/version` exposing a + `git describe` string is a non-issue for an open-source project. +- **Env handling.** Invalid values are logged and fall back; none of the variables is a + secret, so logging the raw value is fine. +- **Image.** Multi-stage; distroless `static`, `nonroot` user, `CGO_ENABLED=0`, `-trimpath`; + `.git` excluded from the context; upstream dump never shipped (asserted in CI). The unpinned + dump over HTTPS with SHA-256 recorded in `meta` is a documented, accepted decision: a + poisoned dump could only inject text that is rendered safely. +- **Workflows.** Top-level `permissions: contents: read` on both; no `pull_request_target`, + no `secrets.*`, no step that pushes, tags, publishes or comments; the only expression + interpolated into `run:` is `github.event_name` (not attacker-controlled); moving major + tags throughout (per house rule; no SHA pinning recommended). +- **Makefile.** No secrets; the `.part` download plus rename is atomic; `VERSION` comes from + the repository's own tags. + +## Recommended order + +1. H1: Hello deadline, per-IP room budget, and (with M1) turn on trusted proxies plus a + per-IP cap in the live Coolify app. +2. M3: get `dependabot.yml` onto `main` and switch on the free GitHub security features. +3. M2: `-healthcheck` mode plus `HEALTHCHECK`; document the stop grace against the drain + timeout. +4. L1, L2, L3, L8: each is under 20 lines in `wsapi`/`main.go`. +5. L4, L6, L7, N3, N4: workflow and settings hygiene. +6. L5: `COPY LICENSE`, CI assertion, footer link to the modification record. + +## Unresolved questions + +1. Does the live Coolify application set `NOITU_TRUSTED_PROXIES` and + `NOITU_MAX_CONNECTIONS_PER_IP`? I did not inspect the deployment. If it does not, M1's + shared join bucket is live today. +2. What stop grace does Coolify give this container, and what is `NOITU_DRAIN_TIMEOUT` set to + there? This decides whether M2's SIGKILL case happens on every deploy. +3. Is Cloudflare (or another CDN) in front of Traefik? That changes which ranges must be + trusted. +4. Is the built image ever pushed to a registry, or only built by Coolify on the host? This + decides whether L5 item 2 is "accepted" or "fix". diff --git a/plans/reports/code-reviewer-260929-1939-server-core-review.md b/plans/reports/code-reviewer-260929-1939-server-core-review.md new file mode 100644 index 0000000..594ef50 --- /dev/null +++ b/plans/reports/code-reviewer-260929-1939-server-core-review.md @@ -0,0 +1,132 @@ +# Server core review: whole codebase, 2026-09-29 + +Branch `dev` @ d3eb13e. This was a read-only review. Scope: `server/cmd/noitu-server`, `server/cmd/build-dictionary`, +`server/internal/{game,dictionary,vietnamese,bot}`, `Dockerfile`, `Makefile`, +`.github/workflows/ci.yml`. I read the two earlier reports first +(`server-core-refactor-260928-1348`, `code-reviewer-260921-1529-server-architecture-review`) +and do not repeat what they already fixed or deferred. + +Baseline, re-run at the start of this review: `go vet`, `go test -race -count=1`, `gofmt -l` and +`golangci-lint` (0 issues) are all clean on the scoped packages. Coverage: vietnamese 100, +game 95.3, bot 91.2, dictionary 88.9, build-dictionary 88.6, **noitu-server 29.5** +(`run()` has no test at all). + +Every finding below was reproduced. Experiments ran against a copy of the module in the +session scratchpad. No project file was changed. + +## Findings + +| # | Sev | Location | Finding | +|---|-----|----------|---------| +| 1 | **High** | `server/cmd/noitu-server/main.go:89-92` | The signal context is passed to `wsapi.NewServer`. On SIGTERM every room and every session is cancelled before `StartDraining` runs. As a result `NOITU_DRAIN_TIMEOUT` never does anything, and players never receive `server_restarting` | +| 2 | Med | `server/internal/bot/strategy_hard.go:155` | Negamax gives a loss the same score whatever its depth. In a position its search sees as lost, Hard plays a move that hands the opponent an immediate kill when a slower loss was available | +| 3 | Med (belongs to wsapi) | `server/internal/wsapi/nickname.go:68`, used before `Submit` at `room_game.go:232` and `dispatch.go:320` | `sanitizeText` deletes U+00A0 and every other non-ASCII space instead of turning it into a space. "ngữ pháp" reaches the engine as "ngữpháp" and is rejected as `fewer than two syllables`. This breaks the NBSP promise in `vietnamese.Normalize` (`normalize.go:35-36, 49-50`) | +| 4 | Low | `server/cmd/noitu-server/main.go:89-90, 131` | `stop()` is only called by the defer. A second SIGTERM or SIGINT during a drain is caught and ignored, so an operator cannot cut a long drain short | +| 5 | Low | `server/internal/game/engine.go:479-483` | `Resign` by a player who is not to act calls `settle()`. That eliminates the player to act at a dead end straight away, instead of on their own clock as `settle`'s doc and the README describe | +| 6 | Low | `server/cmd/build-dictionary/wikitext.go:129` | In `refElement`, the self-closing branch `[^>/]*` fails when an attribute contains `/`. The paired branch then swallows real definition text up to the next `` | +| 7 | Low | `server/internal/dictionary/store.go:38-40, 163-194`; `store_test.go:670` | `builder_version` is never read. The only guard against an older database is that a meta key is missing. The test named `TestOpenRefusesOlderBuilderVersion` only covers a missing `meaning_count`. This was raised on 2026-09-21 and is still open | +| 8 | Low | `server/cmd/noitu-server/main.go:104-108, 174` | Neither `http.Server` sets `IdleTimeout`, and `ReadTimeout` is 0 too, so idle keep-alive connections are never closed. They are not counted against `MaxConnections`, which only applies to `/ws` | +| 9 | Low (test) | `server/internal/bot/realcorpus_test.go:33, 43` | The `seed` parameter of `playRealGame` is never used, and openings come from the global `rand`. The real-corpus ladder, the one measurement the synthetic test defers to, cannot be reproduced run to run | +| 10 | Nit | `server/cmd/build-dictionary/syllable.go:30, 39, 69` | `"ngh"` is listed as a coda, which Vietnamese never has. `"ao"` and `"eu"` appear twice in the nuclei list. `r == 0x031B` is already inside the range before it | + +### 1. High: SIGTERM kills every game before the drain begins + +The path in code: + +- `main.go:89` creates `ctx` with `signal.NotifyContext`, and `main.go:92` passes it to `wsapi.NewServer`. +- `NewServer` wraps that ctx (`server.go:88`) and gives it to the hub. Each room derives from the hub (`room.go:215`), and so does each session (`server.go:193`). +- Rooms `return` on `<-r.ctx.Done()` (`room.go:343-345`). Sessions tear down on `s.ctx.Done()`. +- So the moment the signal arrives, `ctx` is cancelled. Every room exits, `stopCountingLive` drops `liveGames` to 0, and every session starts closing. All of this happens before `main.go:140` `StartDraining()` runs. + +What I measured, using a real process with the fixture DB, one bot game in progress, and SIGTERM: + +| Binary | `NOITU_DRAIN_TIMEOUT` | Server log | Client saw | +|---|---|---|---| +| current `main.go` | 3s | `draining rooms=0 live_games=0` right away | raw EOF, 8/8 runs with no `server_restarting` | +| `NewServer(context.Background(), …)` | 3s | `draining rooms=1 live_games=1` → `drain timed out` → `shutting down` | `server_restarting`, 8/8 | + +Impact: all four steps of "Draining on deploy" in `docs/deployment.md` (lines 241-270) are false in production. Even with `DRAIN_TIMEOUT=0` the "tells players the server is restarting" claim (line 287) is false. Every deploy drops live games without telling the players. The wsapi drain tests do not catch this because `newTestServer` never cancels the ctx it passes in. The `waitForGamesToFinish` tests use a fake counter. + +Fix (main.go only): + +```go +ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) +defer stop() +api := wsapi.NewServer(context.Background(), store, wsapi.Config{ ... }) +... +case <-ctx.Done(): +} +stop() // finding 4: a second signal now terminates instead of being swallowed +``` + +`api.Shutdown()` already cancels the server's own ctx (`server.go:141-144`), so rooms and the limiter sweeper still stop at shutdown. + +Test: pull the post-signal sequence out into a helper, for example `drainAndShutdown(api, drainTimeout)`. Then add a test that starts a game on `NewServer(context.Background(), …)`, cancels a separate "signal" ctx, and asserts `LiveGameCount()==1` and that the client gets `server_restarting`. You can also add a `docker stop` step to the CI image job that greps the log for `draining rooms=1`. + +Remaining risk after the fix, which I measured and could not reproduce: `srv.Shutdown` does not wait for hijacked WebSocket connections. `main` could therefore return before a session's writer has flushed the notice. It never happened in 8 of 8 runs with one client. With thousands of clients it could. + +### 2. Med: Hard walks into immediate kills when every line looks lost + +`negamax` returns `loseScore` (-1000) for "no reply" at any depth (`strategy_hard.go:155`). When every root move loses within the 4-ply horizon, all of them score the same. Hard then plays the first one, and the first in tightest-first order is often the move that lets the opponent kill immediately. Against a human, a slower loss often never happens, because the human does not find the forced line. + +Measured on 3000 seeded random graphs (12 syllables, degree ≤6, so the search is full width and the budget never runs out): 41 times Hard played a move that loses at once while a move that does not was available. `nodeCap` from 3 to 20000 all gave 41 at full budget. An exact 4-ply solver confirmed that all 41 positions were forced losses, so this is a missing preference, not a search bug. I also checked the budget itself: on larger graphs (60 syllables, degree ≤30, 224 boards) `nodeCap=20000` never changed the chosen move. The budget is not a problem. + +Fix: `return loseScore - float64(depth)`. `depth` is the remaining depth, so a sooner loss scores lower, and by negation a sooner win scores higher. Result: 41 → 0. The existing suite passes with it (`TestDifficultyLadder`: hard-vs-easy 95%, medium-vs-easy 93%, hard-vs-medium 78%, against 94/93/78 today). Add a `fakeBoard` test with two moves that both lose within the horizon, one in 1 ply and one in 3, and assert Hard picks the slower one. + +### 3. Med (for the wsapi owner): NBSP and other Unicode spaces merge syllables + +`sanitizeText` drops runes for which `!unicode.IsPrint(r)` is true (`nickname.go:68`). `IsPrint` counts only ASCII U+0020 as a printable space, so U+00A0, U+2009, U+202F and U+3000 are deleted rather than turned into spaces. Measured: + +``` +"ngữ pháp" -> sanitized "ngữpháp" -> Normalize "ngữpháp" (1 syllable) +Normalize alone -> "ngữ pháp" (2 syllables) +``` + +`vietnamese.Normalize` promises exactly this case ("non-breaking spaces that IMEs and copy-paste routinely introduce"), but the transport throws the space away first. A player who pastes a word is refused. Fix, in `sanitizeText`'s map: `case unicode.IsSpace(r): return ' '` before the `IsPrint` arm. The trailing `strings.Fields` join already collapses the result. Add a table case with ` ` to the sanitizeText tests. + +### 5. Low: an out-of-turn resignation settles the player to act at once + +Three seats. Alice plays into a dead end, so Bob is to act with 20s left. Carol resigns 1s later. The result I measured: `over=true winner=alice`, Bob eliminated with `no legal move` 1s into his turn. The final result would be the same after his clock ran out. But `settle`'s doc (`engine.go:451-458`) says the first player to face a dead end "still loses it on their own clock", and the README says the same. The UI also never gets to show Bob the board as his turn. Fix: in `Resign`, call `settle()` only when `e.Turn() != before`. The resignation then moved the turn, and the new player to act has already seen the board, which is the argument `settle` itself makes. No existing test covers resign-at-dead-end. + +### 6. Low: `` eats definition text + +Measured: `Một từ. Nghĩa thêm x cuối.` → `Một từ. cuối.` Verified replacement: +`(?s)"]|"[^"]*")*/>|]*>.*?`. On five shapes it removes the self-closing ref with a slash, a bare `/>`, a named pair, a pair followed by a self-closing ref, and a pair whose attribute contains a slash, and keeps the text between them. I did not measure how common the shape is, because the dump is not on this host. Add the case to `TestStripWikitext`. + +### 7. Low: the builder version is never checked + +`requiredBuilderVersion` only appears in an error message. A v4 database that happens to carry both count keys, or a future v6 with the same keys and different semantics, loads without complaint. The test fixtures have no `builder_version` row and still open. Fix: read `builder_version` in `loadMeta` and refuse a mismatch. Add a `builder_version` row to `fixtureSchema` inserts. Add a builder test asserting `builderVer` equals the store's constant (export it, or put the check in `verify()`). + +### 8. Low: no `IdleTimeout` + +Go uses `ReadTimeout` when `IdleTimeout` is 0, and when both are 0 an idle keep-alive connection is kept forever. Static-asset and `/healthz` clients therefore hold file descriptors without limit, outside every connection cap. Fix: `IdleTimeout: 120 * time.Second` on both servers. It does not touch WebSockets, which are hijacked. + +## Test gaps (beyond those tied to findings) + +- `cmd/noitu-server` `run()`: its shutdown ordering has no test. That is what let finding 1 in. +- `bot`: no test covers "prefer the slower loss" or "a sooner win beats a later one". `TestSimulatedGamesAlwaysTerminate` only uses Hard against Hard. `TestAllStrategiesChooseLegalMoves` covers the other strategies on one small board, which is enough for legality. +- `game`: nothing covers resigning while a dead end is pending (finding 5). +- `dictionary`: `TestOpenRefusesOlderBuilderVersion` tests something other than what its name says (finding 7). + +## Checked and clean + +- **engine**: order of validation (turn → length → dictionary → link → reuse); the used set covers the opening word and alias reuse; the chain cap, the rarity ladder (1→15 … 32→0) and the parts sum after capping; speed clamps; `expire`/`settle` in 2 and 4 seats; turn skips eliminated seats; the winner is always `players[turnIndex]` once `aliveN<=1`; Standings order; Snapshot copies. The `Move.Syllables` count from the typed input is safe because every alias keeps the syllable count (`variantsFor` swaps within a syllable only). +- **vietnamese**: the double NFC is needed, as documented earlier. The fuzz properties imply idempotence. Cf and ZW characters are stripped by the caller, and the builder rejects them via `!IsLetter`. +- **dictionary**: `DSN` escapes `#`, `?` and `%`. `mode=ro` with rollback journalling works on read-only filesystems, and no WAL is used. `validate` covers counts, orphan meanings, out-degree and dangling aliases, and the builder's `verify` covers first-syllable presence. The opener prefix and binary search are correct for `minOutDegree<=0`. The `NearMiss` exclusion and ambiguity rule are correct. `WordsStartingWith` does not expose the backing slice. +- **bot**: all strategies return only `LegalMoves` entries. Kill-decline removes kills from the search as documented. Fail-soft budget exhaustion did not change any choice in my measurements. The thinking-delay range is fine. +- **build-dictionary**: bzip2 magic check; truncation and mid-page EOF errors report their location; the hash covers the whole file; the atomic temp+rename with its Windows-only fallback. I also tested a crash-left `noitu.db.tmp-journal`: SQLite discards a journal next to a zero-length file, so the next build is not poisoned. The alias poisoning does not depend on iteration order. The tone-shift is limited to open syllables and the `qu` exclusion holds. The gloss cap is in runes and matches SQLite `LENGTH`. +- **noitu-server env parsing**: blank values count as unset; invalid or negative values fall back and log a warning; zero drain is accepted; the list is trimmed and empty items dropped. All of it matches the README. +- **Dockerfile, Makefile, CI**: exec-form ENTRYPOINT, so SIGTERM reaches PID 1; distroless nonroot; `.dockerignore` keeps dumps and DBs out; moving major tags; the licence and no-dump image assertions; `.part` download then rename. + +## Recommended order + +1. Finding 1 together with finding 4: one small `main.go` change and one test. The deploy path depends on it. +2. Finding 3: hand to the wsapi owner. It is a one-line fix. +3. Finding 2: a one-line change and one test. +4. Findings 5, 6 and 7: each is a few lines with a test. +5. Findings 8, 9 and 10 whenever that code is next touched. + +## Unresolved questions + +- Finding 1: once it is fixed, should `main` also wait a short, bounded time after `api.Shutdown()` so session writers can flush before the process exits? That is only needed if many-client deploys show lost notices. +- Finding 5: is the immediate settle on an out-of-turn resignation intentional? If so, the `settle` doc and the README sentence need changing instead of the code. diff --git a/plans/reports/code-reviewer-260929-1939-web-review.md b/plans/reports/code-reviewer-260929-1939-web-review.md new file mode 100644 index 0000000..431722a --- /dev/null +++ b/plans/reports/code-reviewer-260929-1939-web-review.md @@ -0,0 +1,139 @@ +# Web frontend review: whole codebase + +Branch `dev` @ d3eb13e, 2026-09-29. Read-only review. The scope was `web/src` (excluding `lib/proto`), `web/tests`, `web/e2e` (read as code only), and the web config files. +Gates at start and end: `npm run lint` 0/0, `npm run check` 0 errors / 0 warnings (385 files), `npm test` 270 passed (16 files). +Playwright was not run (no browser on this host). + +This review builds on these reports and does not repeat what they found: +- `web-refactor-260928-1348-review-and-refactor.md` +- `code-reviewer-260921-1529-web-architecture-review.md` +- `ui-ux-designer-260921-1529-whole-game-ux-review.md` + +## Verdict + +The store and reducer are careful and well tested for the message streams they expect. The real defects are all on **reconnect paths where the server's replay does not describe the state the player is actually in**. In each one the UI keeps showing a room or game that the server has already moved past: + +- The game ended while the player was away. +- The player was eliminated before dropping. +- The resume was refused while the player was still in a room. +- A reload of `/play`. +- A held lobby action flushed straight after `Hello`. + +None of these is covered by a unit test or an e2e test. Findings 1, 2 and 3 were confirmed against the real store with a throwaway Vitest file. The file fed the store the exact frame sequence `handleResume` / `resumeFrom` emit (`server/internal/wsapi/room_presence.go:113-173`, `dispatch.go:267-307`), all four assertions reproduced the stale state, and the file was deleted afterwards. + +## Findings + +| # | Sev | Where | What is wrong | +|---|---|---|---| +| 1 | High | `stores/game-apply.js:81-98` + server `room_presence.go:159-165` | A player who drops mid-game and comes back inside the grace window, after that game has **ended**, is resumed into the lobby with `RoomState` + `ChatHistory` only. No `GameOver` arrives and `roomState` deliberately does not move `phase`, so the client stays in `phase: 'playing'`. Result: a frozen board, possibly a stale `myTurn: true`, and no Lobby (the Lobby is only rendered for `lobby`/`over`), so there is no Ready or Leave button. A guest in that state blocks the owner's Start until they navigate away. | +| 2 | High | `stores/game-apply.js:25` (`LEAVES_ROOM`), `:252-264` | A resume refused while the UI still shows a room is only rendered as a banner. The two cases are `session_not_resumable` (dropped for longer than the grace window, or the server restarted) and `game_already_over` (a bot room that closed on game over while the player was offline). The room or board stays on screen with `connection: open`. On `/play` the dead board has no rematch button (GameOverPanel needs `phase === 'over'`), and the input and resign stay enabled if `myTurn` was true. The `/online` page's own recovery (`+page.svelte:272-278`) only fires while `session.state.resuming`, which is set only on page mount, not on an in-page socket drop. | +| 3 | Med | `stores/game.svelte.js:117-119` (`iAmOut`) | `iAmOut` is `phase === 'playing' && elimination !== null`. A player eliminated in a game of 3 or more who refreshes or reconnects gets the replayed `GameStarted`. That runs `reset()`, which nulls `elimination`, even though the replayed `players` row has `isMe && eliminated: true`. So `iAmOut` is false, the spectator box is replaced by a permanently waiting WordInput, and a disabled claim/resign row is shown. This was confirmed in the store: `gamePlayers[me].eliminated === true`, `iAmOut === false`. | +| 4 | Med | `routes/play/+page.svelte:42-52, 70-78` | A reload mid-bot-game never runs the teardown, so the tab's resume token survives. The mount then sends `Hello{resumeToken}` and, as soon as the status reaches OPEN, `StartBotGame` on the same socket. The server resumes asynchronously (`resumeFrom` posts to the room goroutine). If the resume attaches first, the player sees the resumed board under a red `already_in_a_game` banner. If `StartBotGame` wins, a second bot room is opened, then displaced by the resume, and the client receives two `GameStarted` frames. Either outcome is wrong. The same happens if the tab holds a token from `/online` and the player opens `/play` by URL: it resumes the PvP seat on the bot screen. | +| 5 | Med | `routes/online/+page.svelte:221-227` (`flushAction`), `room-session.svelte.js:188-193` | A held lobby action is flushed the moment the status reaches OPEN, which is right after `Hello`. For an in-page reconnect, `Hello` carries the token and the server attaches the seat asynchronously on the room goroutine, so `KickPlayer`/`SetReady`/`StartGame` is most likely read before the attach and answered `not_in_a_room` (`dispatch.go:224-227`). The player sees the red `lobby-error` "Bạn không ở trong phòng nào." in a room they are in, and the held action is discarded (the send "succeeded"). Kick is reachable because its button is not gated on `offline` (`Lobby.svelte:113-122`). `flush()` for join/create already guards `resuming`; `flushAction` has no equivalent. | +| 6 | Med | `routes/online/+page.svelte:406-414` (`leave`) | Pressing Leave while the socket is down holds `leaveRoom`, then clears the room and **forgets the token**. The reconnect therefore opens a fresh session, and the held `LeaveRoom` it flushes is answered `not_in_a_room`. That lands as a red `join-error` on the join form the player just returned to. The held message cannot do anything useful: without the token, the seat is released by grace expiry either way. | +| 7 | Med | `components/ChatPanel.svelte:128-144, 213`; `online/+page.svelte:416-419` | A chat line sent while the socket is down is lost silently. `say()` ignores `send()`'s `false`, and `submit()` clears both `draft` and the field unconditionally. The send button is not gated on the connection, unlike WordInput (`WordInput.svelte:22-24`). The player types a message during a blip, presses Gửi, the text vanishes, and nothing is ever posted or explained. | +| 8 | Med (a11y) | `components/PlayerStatus.svelte:66-75` | Each away player's banner is `role="status"` (implicitly `aria-live="polite"`) and its text holds a countdown that changes every second. A screen reader announces "X mất kết nối, còn N giây" once a second for up to 30 s, once per dropped player, on both the board and the lobby. This is the same defect the UX review fixed for the quick-match counter (`online/+page.svelte:522-527`), but this one was missed. | +| 9 | Low | `components/GameOverPanel.svelte:33-35` | `panel.focus()` fires on every new result, with no guard. In the wide layout (or for a knocked-out spectator), a player typing in the chat input when the game ends has focus pulled off the chat mid-sentence, and the rest of their keystrokes go nowhere. WordInput already has the right guard (`typingElsewhere`, `WordInput.svelte:52-59`). | +| 10 | Low | `ws/client.js:291-299` | `clockOffsetMs` is overwritten by every pong, one sample every 5 s. On a jittery mobile link the error is up to ±RTT/2 per sample, so the ring and the seconds label jump by a few hundred ms every 5 s, and can tick back up a second. `SETTLE_MS` (300) only covers one side of that. | +| 11 | Low | `history-export.js:62-63`; `ChainHistory.svelte:68-73` | After any mid-game resume the chain holds only `[opening, lastMove]`, because `GameStarted` has no chain field. The transcript numbers the last move "2." even when `result.chainLength` is 30, and the live chain shows two unrelated words as if they were adjacent. | +| 12 | Low (a11y) | `components/ChatPanel.svelte:170-186`; `game-apply.js:244-250` | `chatHistory` renumbers every line (`++chatOrdinal`), so on each reconnect all up to 20 `
  • ` are re-keyed and re-inserted inside an `aria-live="polite" aria-relevant="additions"` log. A screen reader re-reads the whole conversation after every blip. | +| 13 | Low (tests) | `tests/word-input.test.js:3-8` vs the rest of the file | The header says the submit-clear is "pinned down", but no test submits. Also untested: clearing only when `onsubmit` returns true, refusing while composing, and the out-of-turn `beforeinput` guard / `undoInput` revert. `leaves an in-progress composition alone` (`:103-118`) would pass with the composition handling deleted, because on the player's own turn nothing else touches the value. | +| 14 | Low (tests) | none | No coverage for findings 1 to 8, which are all reconnect paths. `CountdownRing`, `PlayerStatus`, `GameOverPanel` (focus, export click), `Lobby` and `ArmedButton` have no component test. `client.js` storage guards (`safeSessionStorage`, `storeToken` throwing) have no test, unlike `settings.svelte.js`. No test asserts that each `fill(t.x, {…})` supplies exactly `t.x`'s placeholders. A scan run for this review found no mismatches today. | +| 15 | Nit | `vite.config.js:27` | "The two suites that need a DOM": five do (chat-panel, settings-store, ws-client, game-board, word-input). | +| 16 | Nit | `routes/online/+page.svelte:679` | `h1 { font-size: 1.3rem }` is still off the type ramp (deferred last time). `var(--text-5)` (1.5rem) or `--text-4` (1.125rem) is the nearest step. | + +## Recommended fixes (each small) + +1. **Game ended while away.** The client cannot detect this with the current protocol: a mid-game `RoomState` is normal. This needs a server change. + - In `handleResume`'s `inLobby()` branch, send the seat its `GameOver` if a game finished while it was detached. The room would keep the last standings and reason per game. + - Alternatively, add `bool in_game = 13;` to `RoomState`, and have `roomState` set `phase = 'lobby'` when it is false and `phase === 'playing'`. + - Add a server test: 2 players, one drops, the game ends, the dropped player resumes inside grace, and it must receive `GameOver` (or `in_game: false`). + - This crosses the web/server boundary, so it is the lead's call. +2. **Refused resume.** + - Add `'session_not_resumable'` to `LEAVES_ROOM`. It is only ever sent in reply to `Hello` (`dispatch.go:277`, `room_presence.go:120`), so leaving on it is always right. `game.leave()` then lands `/online` on the join form with the banner. The `resuming` effect still clears it on mount as today. + - `game_already_over` is shared with a dropped `Resign`, so do not blanket-leave on it. In `client.js`, remember that the last `Hello` carried a token. If the frame right after `welcome` is an `error` whose code is `session_not_resumable` or `game_already_over`, report it through a new `onResumeRefused` callback. `resumeFrom` sends the error in the same call as the Welcome, so nothing can come between them. `connection.svelte.js` then calls `game.leave()`. + - On `/play`, the resulting `idle` phase needs a "start again" affordance, or simply `startGame(difficulty)`. +3. **`iAmOut`:** `return state.phase === 'playing' && (state.elimination !== null || state.gamePlayers.some((p) => p.isMe && p.eliminated));`. GameBoard's spectating box already handles `elimination === null`: it shows `youAreOut` without suggestions. +4. **`/play` reload:** call `forgetSession()` (already exported) in `startGame()` before `connect()`. Reload-means-new-game on the same rung is what the page's own comment describes (`play/+page.svelte:22-24`). Otherwise, if resuming a bot game is the intent, mirror `/online`: when `hasStoredSession()`, hold `session.request` until the resume answers. That choice is a product decision (see Unresolved). +5. **Held action vs resume:** gate `flushAction` on the seat being re-established, not on the socket being OPEN. The smallest version is to flush from an effect keyed on `game.state.roomPlayers`, which changes on the `RoomState` the resume broadcasts, when `heldAction` is set and the status is OPEN. The alternative is to set `session.startResume()` whenever the status leaves OPEN while `inRoom`, and let the existing `noteRoom()` clear it. +6. **Leave while offline:** in `leave()`, send when possible and never hold: `if (!dispatchAction({ kind: 'leaveRoom' })) {/* token is forgotten; grace frees the seat */}`, and drop the `act` wrapper there. +7. **Chat offline:** make `onsend` return `boolean` (`say = (text) => send(sendChat(text))`). In `submit()`, only clear on `true`. Add `disabled={!sendable || offline}` to the button, using the same `connection.status` read as WordInput. Add a chat-panel test with `onsend: () => false` that expects the field to still hold the text. +8. **Away countdown:** + - Split each banner into a live sentence without the number, "X mất kết nối", inside `role="status"`. + - Move the seconds into an `aria-hidden="true"` span, as the quick-match counter already does. + - Optionally announce once at 10 s. +9. **GameOverPanel focus:** `if (game.state.result && !typingElsewhere()) panel?.focus();`. Hoist WordInput's `typingElsewhere` into a tiny `$lib/focus.js` so both components use it. +10. **Clock offset:** keep the sample with the smallest RTT among the last ~5 pongs (a ring of `{rtt, offset}`) and use that offset. This is the standard NTP-style filter, about 10 lines, and `tests/ws-client.test.js:278` already has the harness to cover it. +11. **Partial chain:** + - In `chainToText`, number from the server: `const n = chainLength - (chain.length - 1 - index)`, with `chainLength` passed in from `game.state.chainLength`. + - Insert a `…` line when `chain.length < chainLength`. + - Optionally render one "…" row in ChainHistory under the same condition. +12. **Chat re-announce:** in `chatHistory`, reuse the ordinal of an existing line with the same `(atMs, playerId, text)`, or give the replacement a fresh `{#key}` wrapper outside the live region. The first keeps Svelte from re-inserting the nodes. +13. **WordInput tests:** add three. + - Submit clears the field when `onsubmit` returns true and keeps it when it returns false. + - Submit is refused during `compositionstart`. + - An out-of-turn `input` is reverted to `lockedValue`. + + Replace or delete the vacuous composition test. + +## The deferred global `.primary` class: worth doing now, partially + +The same accent fill, hover, press and disabled block (about 15 lines) appears in: +- `online/+page.svelte:699-721` +- `ChatPanel.svelte:330-351` +- `+page.svelte:84-97` +- `Lobby.svelte:383-407` +- WordInput and GameOverPanel + +**Recommendation:** +- Add a global `.primary` to `app.css` carrying only colour and state: background, colour, a transparent border, the transition, and `:hover`/`:active`/`:disabled` with `:not(:disabled)`. Components keep their own sizing and padding. +- Apply it to the online page, the landing page, ChatPanel (add `class="primary"` to its send button), WordInput and GameOverPanel. Their local rules only restate the colours, so deleting them changes nothing visually. +- **Leave Lobby alone** for now. Its scoped `.actions button { background: var(--surface) }` compiles to (0,2,1), which beats a global (0,1,0) `.primary`. Adopting the global class there first means moving that background into `.actions button:not(.primary)`, which is the part that needs a visual check. +- Do not use `:where(.primary)`: zero specificity would lose to every scoped base `button` rule. + +**`online/+page.svelte` (808 lines).** About 390 of those lines are CSS. The concrete split that is still worth doing is the join-form branch (`:483-590` plus its about 120 lines of styles) into `JoinPanel.svelte`, which receives `session`, `named`, `codeInput` and the three request callbacks. It was deferred by a task decision last time, so it is not listed as a finding. + +## Checked and clean + +- **`game-apply.js`:** + - `roomState` applies the whole snapshot at once and never merges it with the old one. + - `turnUpdate` without `played` keeps the rejection. + - `LEAVES_ROOM` runs before the error is set. + - The chat window is trimmed. + - The try/catch boundary in `apply()` is intact. + - `MoveRejected.turn_seq` is ignored, but every path that moves `turnSeq` on the server also sends a `TurnUpdate` or replay on the same ordered socket. No failure scenario was found, so this is not reported. +- **`client.js`:** + - Backoff resets on `welcome` and not on open. + - Liveness ignores throttled ticks. + - `reconnectNow` cannot create a second socket. + - The terminal-error stop works. + - `close()` and a later `connect()` never share a client instance, because `disconnect()` discards it, so the stale-`onclose` clobber is unreachable today. A late `onclose` from a discarded client cannot touch the status, since its own status is already CLOSED. + - Both storage accessors are guarded at the property and at the call. +- **`settings.svelte.js`:** + - Every read and write is guarded. + - Corrupt JSON and non-finite scores degrade cleanly. + - The nickname cap counts code points. + - The theme is normalised, and matches the inline script in `app.html`. +- **Countdown maths** (`countdown.js`): clamped, rounded up, settle margin applied, and the rAF loop runs only while `running` and is cancelled on cleanup. +- **Runes:** + - Every `$effect` that writes state either writes state it does not read or uses `untrack`. The online, play, PlayerStatus and ChatPanel effects were checked individually. + - Timers and listeners are cleaned up: matchMedia, the quick-match interval, the stall, resume and arm timers, the PlayerStatus interval, the ResizeObserver, rAF, and the RoomCodePanel timer. + - No `$effect` that should be a `$derived` was found. The ones that assign are bindable props or trigger-style latches. +- **i18n:** + - `checkJs` + `strict` makes a mistyped `t.key` a `svelte-check` error. + - No Vietnamese prose outside `vi.js`, except the static ``/`` in `app.html`. + - A throwaway scan of every `fill(t.x, {…})` found no missing or extra placeholder. + - Server error codes are guarded by `tests/error-codes.test.js`. +- **The rules page scoring constants** match `server/internal/game/engine.go:28-51` and `vietnamese.MinSyllables`. +- **Focus:** + - A turn arriving focuses the word field unless the player is typing elsewhere. + - A reconnect re-enables the field and focuses it. + - Game over focuses the panel, apart from finding 9. + - ArmedButton announces its armed state through `aria-pressed`. +- **Keyboard:** the difficulty picker uses native radios, the chain rows are buttons with `aria-expanded`/`aria-controls`, and the skip link targets `#main`. +- **Configs:** the Vitest alias and `browser` condition are correct, the Playwright server env is sane, and dependencies use caret ranges (moving, as the workspace rule prefers). + +## Unresolved questions + +1. Finding 4: should reloading `/play` mid-game resume the bot game or start a fresh one on the same rung? The page comment implies a fresh one, while `hasStoredSession()`'s doc implies a resume. +2. Finding 1: server replay of `GameOver` or a new `RoomState.in_game` field? The first needs no proto change. The second is a smaller change on each side but bumps the contract. diff --git a/plans/reports/code-reviewer-260929-1939-wsapi-review.md b/plans/reports/code-reviewer-260929-1939-wsapi-review.md new file mode 100644 index 0000000..1e5bf85 --- /dev/null +++ b/plans/reports/code-reviewer-260929-1939-wsapi-review.md @@ -0,0 +1,277 @@ +# wsapi transport review: whole package, read-only + +Date: 2026-09-29 · branch `dev` @ d3eb13e · scope `server/internal/wsapi/**`, `proto/noitu/v1/game.proto` (read for contract only) + +## Verdict + +The actor model is sound. Every engine and seat mutation stays on the room goroutine, all +sends are non-blocking, and `-race` is clean. The findings below are lifecycle and +identity bugs at the edges of that model: what happens to a seat *id* after its seat is +freed, what happens to inputs queued behind a room's last one, and what happens when a +control notice has to share the inbox with player spam. Each one was reproduced with a +throwaway test in a scratch copy of the module, and the suggested fixes for #1, #2 and #4 +were applied in that copy and pass the whole package under `-race` (the only failure was +`TestCrossLanguageFixtures`, and only because the copy has no `proto/testdata`). + +Baseline on the real tree: `go vet ./internal/wsapi` clean, `go test ./internal/wsapi -race -count=1` ok (29.6s). +No project file was modified. + +## Findings + +| # | Sev | Where | Finding | +|---|---|---|---| +| 1 | High | room_presence.go:473, :380-385; room_lobby.go:189-191 | A kicked player's token resumes into whoever now holds the same seat id: their name, wins and chat window, and the real occupant is locked out | +| 2 | Med | room.go:262 (defer order), exits at :410/:425/:431 | Inputs queued behind a room's final input are never answered. A join or resume that loses the race waits forever | +| 3 | Med | dispatch.go:350-356, session.go:203-205, dispatch.go:296-298, room.go:236-258 | Disconnect and resume notices share the lossy 32-slot inbox. Four seats bursting unmetered `Resign` overflow it, a dropped disconnect leaves a ghost "connected" seat, and a dropped resume is reported as `game_already_over` | +| 4 | Low | room_lobby.go:66-69, :78-94 | `autoStart` stays armed after a quick-match pairing that never started, so the next code joiner starts a game nobody readied for | +| 5 | Low | room.go:330, :433-435 | The idle window is reset by *any* input, refused ones included. A refused `Resign` every few minutes holds a lobby open forever | +| 6 | Low | server.go:268-291, dispatch.go:68 | The join limiter is keyed on the full IPv6 address, so one /64 has 2^64 buckets (and F10's unmapped-v4 key split is still open) | +| 7 | Low | hub.go:192-197 | Quick match still queues a player while draining, and they wait until shutdown | +| 8 | Low | dispatch.go:28-157 | No `default` arm: an unknown or empty payload is silently dropped (prior C5, recorded "not done", never rejected) | +| 9 | Low (test) | chat_test.go:317 | `TestChatDoesNotKeepARoomAlive` passes with chat resetting the idle clock | +| 10 | Low (test) | limits_test.go:203 | `detachAll` has no test at all. The whole suite passes with `defer r.detachAll()` deleted, even though the prior review cited this test as its closure | +| 11 | Nit (test) | convert_test.go:141 | A swapped `PointKind` arm (CHAIN<->SYLLABLES) passes the whole suite | +| 12 | Nit | room_presence.go:482 | A resume does not leave the quick-match queue, unlike every `takeSeat` path | +| 13 | Nit (test) | lobby_test.go:474-488 | A hand-rolled copy of `awaitNoRooms` | + +--- + +### 1. High: a stale token resumes into another player's seat + +**What is wrong.** `handleResume` looks the seat up by id (`seatOf(m.player)`) and accepts +when `s.sess == nil`. Seat ids `p1..p4` are reused. A seat vacated while its player is in +the grace window never releases that player's prior session: `vacate` only calls `release` +when `s.sess != nil`, and `holdSeat` has already nil'd it. So the prior session still points +at the room with id `p2`, and its token stays live for the rest of *its own* grace window. + +**Failure scenario (reproduced end to end).** Alice (p2) drops. The owner kicks her +offline seat, which is allowed because she is not ready. Carol joins and gets p2, chats, +then refreshes, which puts p2 in grace for Carol. Alice's tab reconnects within 30s and +`web/src/lib/ws/client.js:228` presents her stored token automatically. +Result: `BUG: kicked Alice resumed into seat p2 named "Carol"`. Alice's replayed +`ChatHistory` contained `"carol-only secret" from "Carol"`, and Carol's own resume was then +answered `session_not_resumable`. That is three README invariants broken at once: resume +into the wrong seat, take another seat's series score, and chat replay leaking. Grace +expiry followed by a rejoin hits the same root cause, but only in the microsecond gap +before the token expires. + +**Fix (verified in scratch).** Remember which connection a window is being held for: + +```go +// seat +heldFor *session // the connection whose drop opened graceUntil + +// holdSeat +s.heldFor = s.sess +s.sess = nil + +// handleResume +if s == nil || (s.sess != m.prior && (s.sess != nil || s.heldFor != m.prior)) { + m.sess.send(errorMsg(codeSessionNotResumable)) + return +} +``` + +A refilled seat is a new `*seat`, so its `heldFor` can never be the kicked player's +session. Add the scratch reproduction as a regression test in resume_test.go. + +### 2. Med: inputs queued at room exit are never answered + +**What is wrong.** `run` returns as soon as the room is empty, idle, or a bot game ends, +and anything still in `r.inputs` is dropped without a reply. `r.cancel()` is also the +*first* defer registered, so it runs *last*. Until then `send` still returns true into a +room nobody reads, which is exactly the "caller waits forever" case `send`'s own comment +describes. + +**Failure scenarios.** (a) The owner is alone and clicks Leave while a friend's +`JoinRoom` is behind it in the inbox. `hub.joinRoom` returned nil, so dispatch answers +nothing and the friend never gets a `room_state` or an error. Reproduced by queuing +create, leave, join and running the room: the joiner received **zero** frames. +(b) The last seat's grace timer and that player's `resumeInput` are ready on the same +`select`. If the grace arm wins, the room exits and the resume is never answered, which +leaves the client's resume latch hanging (the case handleHello:271-277 was written to prevent). + +**Fix (verified in scratch).** Cancel first, then answer whatever is left: + +```go +func (r *room) run() { + defer r.refusePending() // runs last: after cancel, so nothing new is accepted + ... existing defers ... + defer r.cancel() // registered last, so it runs first +``` + +`refusePending` drains non-blockingly and answers `joinInput` with `room_not_found`, +`resumeInput` with `session_not_resumable`, submit/resign/claim with `not_in_a_game`, and +lobby/chat with `not_in_a_room`. + +### 3. Med: control notices can be dropped by a full inbox + +**What is wrong.** `leaveRoom`'s `disconnectInput`, `attach`'s release of the previous room, +and `resumeFrom`'s `resumeInput` all go through `room.send`, which drops on a full inbox. +`Resign` is the one room input with no per-action limiter (`dispatch.go:118`, `nil`). Each +connection may burst 40 frames, and every refused `Resign` in a lobby still takes an inbox +slot. + +**Measured.** With four seats each writing 39 `Resign` frames at once, the room logged +`room inbox full, dropping message` 4, 86 and 38 times across three runs, and 357 times +over three runs at `GOMAXPROCS=1`. One connection alone never overflowed it. Constructed +consequence (scratch test): the guest's disconnect was dropped, and after the grace window +had passed the seat was **still bound to its dead session** with `graceUntil` zero. +`allConnected()` then reports true, so `canStart` goes green against a dead socket, the +seat never expires, and a ready ghost cannot be kicked (`player_is_ready`). A dropped +`resumeInput` is also reported as `game_already_over` (dispatch.go:296-298), which is a +false statement: the room is alive, only busy. + +**Fix.** (a) Charge `Resign` on `s.submitLimiter`, like `ClaimDeadEnd`. (b) Deliver the +two teardown notices reliably. `leaveRoom` runs on a dying session goroutine and can block +safely: `select { case r.inputs <- m: case <-r.ctx.Done(): }`. For `attach`, which runs on +*another* room's goroutine, do the same inside `go func(){...}()` so two rooms can never +wait on each other. (c) In `resumeFrom`, answer a busy room with `busy` rather than +`game_already_over`. + +### 4. Low: quick-match `autoStart` outlives its pairing + +**What is wrong.** `r.autoStart` is cleared only when the auto-start actually fires. If +the pairing's joiner was already torn down (the `holdSeat` return at :66-69), or either +side is offline at join time, the flag stays set. The next `handleJoin` that fills a second +connected seat starts a game with no readiness and no `StartGame`. +**Scenario (reproduced at handler level).** The quick-match partner dies before seating +and grace expires. The waiter shares the room code (it is in their `RoomState`). The +friend who joins is dropped straight into `game_started` with `ready=false`. +**Fix (verified).** The first `handleJoin` in the room is the pairing join, so consume the +flag there: `autoStart := r.autoStart; r.autoStart = false`, and also clear it on the +`holdSeat` early return. + +### 5. Low: the idle window is not "ten minutes with no game started" + +`idleActivity` is true for every input except chat and report, and for the grace arm. +Refused `Resign`, `SubmitWord` or `ClaimDeadEnd` (`game_not_started`), a stranger's refused +join (`room_full`) and ready toggles all restart it. Reproduced: with `IdleFor: 300ms`, a +refused `Resign` every 150ms kept the lobby open for 5x the window. **Fix:** reset only +when the input changed room state. Capture `changed := r.lobbyChanged` before the broadcast +block and call `resetIdleTimer()` only when `changed`. `handleChat` never sets +`lobbyChanged`, so the chat special case goes away. + +### 6. Low: the join limiter treats every IPv6 address as its own client + +`clientIP` returns the peer or hop address as a raw string, and `hub.joinLimiter` keys on +it. A single residential IPv6 customer controls a /64, which is 2^64 distinct buckets, and +that makes the 5/s code-walk guard (session.go:53-61) meaningless on a dual-stack +deployment. The same raw string also keeps prior finding F10 open, where `::ffff:a.b.c.d` +and `a.b.c.d` count as two keys. **Fix:** derive the limiter key once: +`addr.Unmap()`, and for `Is6()` use `netip.PrefixFrom(addr, 64).Masked().String()`. Use it +for `reserveIP` too. + +### 7. Low: quick match queues while draining + +`newRegisteredRoom` refuses rooms when draining, but the enqueue path (`hub.go:192-197`) +does not check. A lone player is told `queued:true` and then waits until shutdown. **Fix:** +`if h.draining.Load() { return errDraining }` at the top of `quickMatch`. dispatch +already maps that error to `server_restarting`. + +### 8. Low: `dispatch` has no `default` arm (still open) + +This was C5 in the 2026-09-21 review. The action report recorded it as "not done", not as +rejected. A `ClientMessage` with no payload, or one from a newer client, costs a frame and +gets no reply. **Fix:** `default: s.send(errorMsg(codeUnknownMessage))`, adding the code +to errcodes.go and `vi.js` (the web error-codes test will insist). + +### 9. Low (test): `TestChatDoesNotKeepARoomAlive` cannot fail + +Mutation check: with `idleActivity = false` removed from the chat arm, the test still +passes 3/3. The idle close merely arrives about 300ms later, and `await` allows 5s. +**Fix:** record `start` before the chats and fail if `room_idle_closed` arrives later than +`IdleFor + 150ms`. + +### 10. Low (test): `detachAll` is untested + +With `defer r.detachAll()` deleted, the **whole package passes**. `TestIdleRoomReleasesItsSeats` +proves the connection can create a new room, but `CreateRoom` succeeds either way, because +`attach` just overwrites the dead room pointer. **Fix:** after the idle close, have the +host `say("x")` and require `not_in_a_room`. Without `detachAll`, `toRoom` finds the dead +room and answers `busy`. + +### 11. Nit (test): enum mapping is pinned for shape, not meaning + +The exhaustiveness tests in convert_test.go prove each `game.PointKind` maps to a distinct +non-UNSPECIFIED wire value, but a swapped pair is still a bijection. Mutation check: CHAIN +and SYLLABLES swapped, suite green. (Swapping two reject reasons *is* caught, by +`TestRejectionsCarryTheRightReason`.) **Fix:** one line in `TestPointKindMappingIsExhaustive`: +`if want := "POINT_KIND_" + strings.ToUpper(k.String()); got.String() != want { t.Errorf(...) }`. +This works because `game.PointKind.String()` values are single words matching the wire suffixes. + +### 12. Nit: a resume does not leave the quick-match queue + +Every `takeSeat` path calls `hub.cancelQuickMatch`, but `handleResume` binds with +`attach` directly. A client that sends `QuickMatch` between its Hello and the room +draining the resume ends up both seated and queued. A later pairing would then pull it out +of a running game past `refuseMidGame`. **Fix:** `r.hub.cancelQuickMatch(m.sess)` next to +`m.sess.attach` at :482. + +### 13. Nit (test) + +`TestOneConnectionCannotStrandRooms` (lobby_test.go:474-488) re-implements `awaitNoRooms` +with its own lock-and-poll. Replace it with the helper. + +## Test gaps with no coverage today + +- A kicked or expired seat id being resumed by its old token (#1). +- Any input queued behind a room's final input (#2). +- Inbox overflow dropping a lifecycle notice (#3). The scratch test builds it directly on + `newRoom` plus `offlineSession`. +- `autoStart` after a failed pairing (#4). `TestQuickMatchAutoStartSkipsAGhostSeat` stops + one step short of it. +- The idle clock under refused actions (#5). + +## Checked and clean + +- **Engine ownership:** every `*room` handler is reached only from `run`'s switch. The bot + worker sees a `frozenBoard` copy and exits on `r.ctx`. `strategy` is never used by two + workers at once, because only the bot's own turn schedules one. +- **Timers:** all three are recreated rather than reset, stopped in a defer, and + recomputed after every input. `graceC` is nil'd before rearming. A negative + `time.Until` fires immediately, which is correct. +- **Session teardown:** `readCtx` is cancelled only after flush or 2s, with no leak of the + three goroutines (`wg.Wait`). `close` is idempotent (`CancelFunc`). The outbox never + blocks the room. +- **Hub:** the `rooms` and `sessions` maps are touched only under `mu`. Code draw, cap + check and registration share one critical section. `evict` runs on exit. `liveGames` + is decremented exactly once via the CAS. +- **Join semantics:** a running game refuses latecomers (`game_in_progress`), a full or + empty room refuses (`room_full`), and a second join to your own room is refused. + Start needs 2+ seats, everyone connected and every guest ready, and the owner has no + ready flag. Promotion clears ready, and every game clears ready again. +- **Turn authority:** submit, resign, claim and chat all check `occupies(sess, id)` (the + seat, not the claimed id) and the turn owner. A stale `turn_seq` is refused with the + server's sequence. `turnSeq` never restarts across rematches, and it moves on an + out-of-turn forfeit only when the turn does. +- **Resume races:** concurrent presentation of one token is guarded + (`s.sess != m.prior`). A dead new connection reopens the window. A stale + `disconnectInput` from the replaced connection is ignored. +- **Quick match:** it never pairs a connection with itself (`w == s`) and skips dead + waiters. Teardown and every seating path dequeue. The status is sent before any room frame. +- **Chat:** `chatFrom` scopes replay to the seat's tenure. Vacating scrubs author and name + together and re-syncs the remaining seats. Bot rooms have no chat. Chat uses `trySend`, + history uses `send`. +- **Sanitisation:** NFC first, then Cc/Cf/non-printing dropped (which also removes + Zl/Zp/NBSP), marks capped at 2, whitespace collapsed, and a rune cap on nickname (20), + chat (200), echoed word and report (64). `distinguish` includes seats in grace. +- **Limits:** a 4 KiB read limit, 20/s frame limiter closing on flood, per-session room, + submit and chat buckets, the join bucket per IP, a 20-report cap per session, a 32-frame + outbox, and a limiter sweep. +- **Enum mapping:** `RejectReason` and `EndReason` are pinned both ways, including + semantically through e2e tests. `PointKind` is pinned for shape only (#11). +- **Contract:** `errorMsg` carries only UI keys (`TestErrorMessagesAreUIKeysNotProse`), + and the wire round-trip and oneof coverage tests are present. No proto change is needed + for any fix above (#8 adds only an error-code string). +- **Test hygiene:** the remaining sleeps are deadline-bounded polls. `settle()` is used + only before goroutine and count sampling, and not as a correctness wait, except in + `TestQuickMatchDropsADisconnectedWaiter`, whose dead-waiter skip is covered separately by + `TestQuickMatchSkipsAWaiterWhoseConnectionEnded`. + +## Unresolved questions + +- #1's fix answers `session_not_resumable` to a kicked player who comes back. Is that the + right message, or should a kick explicitly revoke the token (`hub.unregister`) so the + client learns it at Hello? +- #5: should a ready toggle count as activity? The proposed `lobbyChanged` rule says yes. diff --git a/plans/reports/fullstack-developer-260929-1939-server-ops-fixes.md b/plans/reports/fullstack-developer-260929-1939-server-ops-fixes.md new file mode 100644 index 0000000..fab9f20 --- /dev/null +++ b/plans/reports/fullstack-developer-260929-1939-server-ops-fixes.md @@ -0,0 +1,81 @@ +# Server core, packaging, CI and deployment fixes + +Date: 2026-09-29, branch `dev`, nothing committed. + +## Changes per finding + +Server core review: + +- **1 (shutdown order).** `serve()` in `server/cmd/noitu-server/main.go` now builds the wsapi server on `context.Background()`. The signal context is only the "stop now" trigger, so rooms and sessions survive until `StartDraining` has run and the drain has waited. The sequence lives in `drainAndShutdown` (drain, wait up to `NOITU_DRAIN_TIMEOUT`, `Shutdown`, then a bounded 2s flush). `run()` was split into `run` (store, listener, signal context) and `serve` so the sequence is testable. +- **4 (second signal).** `serve` calls the signal context's `stop` as soon as the context fires, so a second SIGTERM/SIGINT kills the process. +- **Shutdown flush.** After `api.Shutdown()` the process sleeps `shutdownFlush` (2s). wsapi exposes no live-session count, so this is a fixed bound, as the brief allowed. +- **2 (Hard, equal-score losses).** `negamax` returns `loseScore - depth`. The doc comment on `loseScore` explains it. +- **5 (out-of-turn resign).** `Engine.Resign` only calls `settle()` (and restarts the clock) when the turn moved. The README was right, so only code changed. +- **6 (`<ref name="a/b"/>`).** `refElement` in `wikitext.go` reads quoted attribute values whole. +- **7 (builder version).** The store now reads `builder_version` in `loadMeta` and refuses a missing or different value, naming the version found. The constant is exported as `dictionary.RequiredBuilderVersion`, and the builder's `builderVer` is now that same constant, so the two cannot drift. Test fixtures write the row. +- **8 (IdleTimeout).** `newHTTPServer` sets `IdleTimeout: 120s` (with the existing `ReadHeaderTimeout`) for both the public and debug listeners. No Read/WriteTimeout. +- **9 (real-corpus seed).** `playRealGame` now draws openings from a PCG seeded by `seed`, through a new `Store.RandomOpeningWordFrom(rng, min)` (`RandomOpeningWord` delegates to the same helper). +- **10 (syllable nits).** Dropped the `ngh` coda, the duplicate `ao` and `eu` nuclei, and the redundant `0x031B` clause. +- **3.** Belongs to wsapi. Not touched. + +Security and ops review: + +- **M1.** New "Coolify and Traefik" section in `docs/deployment.md`. It gives the exact env vars and what each changes (`NOITU_TRUSTED_PROXIES` as the Traefik subnet, `NOITU_MAX_CONNECTIONS_PER_IP=32` as a starting point, `NOITU_DRAIN_TIMEOUT`). It also covers Cloudflare (Traefik `forwardedHeaders.trustedIPs`) and a Traefik `/readyz` load-balancer health check label. +- **M2.** Added `noitu-server -healthcheck` (`checkHealth`: GET `/healthz` on `NOITU_ADDR`, wildcard or bare-port host dialled on 127.0.0.1, exit 0 on 200, else 1 with the reason on stderr). Added a Dockerfile `HEALTHCHECK` (interval 30s, timeout 5s, start-period 10s, retries 3). The docs explain how Coolify uses it and state the stop-grace rule: `NOITU_DRAIN_TIMEOUT + 2s < container stop grace`, so at most about 6s with Docker's 10s default. Also documented that a second signal ends the process. +- **L4.** `proto.yml` now uses `bufbuild/buf-action@v1` with `setup_only: true`. I confirmed `setup_only` in the action's `action.yml`. I dropped `version: latest`: the `version` input is optional and I could not confirm that `latest` is a valid value, so an unset version is the safer way to get the newest buf. +- **L5.** `COPY LICENSE /app/LICENSE` in the Dockerfile, and `app/LICENSE` added to the CI "licence travels with the data" check. Docs note the licence file now ships. The third-party notices gap is accepted while the image is unpublished (recorded in `deployment.md` and here; no code change). The web footer link is the web agent's job. +- **L6.** `go-version: stable` in every `setup-go` step (ci.yml x2, proto.yml), with `go.mod` left as the minimum. Added `go run golang.org/x/vuln/cmd/govulncheck@latest ./...` to the Go job and `npm audit --omit=dev --audit-level=high` to the web job. Moving major tags only, no SHA pins. +- **L8.** Covered with finding 8. +- **N2.** Documented only, in the Observability section. +- **N3.** `.dockerignore` now excludes `.claude`, `**/.claude`, `.env*` and `**/.env*`. +- **N4.** `persist-credentials: false` on all five checkout steps. +- I also added `docker exec noitu /app/noitu-server -healthcheck` to the CI image job, so the HEALTHCHECK command is exercised. + +## Skipped + +- H1, L1, L2, L3, N1: wsapi agent. +- M3, L7: GitHub settings, manual (below). +- D1, D2, N5, N6: non-issues or optional, per the brief. +- Web footer link for the modification record (L5 item 3): web agent. + +## Verification + +- `go vet ./...` clean, `gofmt -l .` empty, `golangci-lint run ./...` 0 issues. +- `go test ./... -race -count=1`: every package passes. The first full run had one wsapi failure (`TestFrameFloodClosesTheConnection`) while the other agent was mid-edit. A single rerun of `./internal/wsapi` passed. +- Tests that fail without their fix (I reverted each fix and confirmed the failure): + - `TestServeDrainsLiveGamesBeforeShuttingDown` drives `serve` over a real WebSocket bot game. With the signal context passed to `NewServer` it fails with EOF and `draining rooms=0 live_games=0`. + - `TestResignOutOfTurnDoesNotSettleAPendingDeadEnd` + - `TestHardPrefersTheSlowerLossWhenEveryLineLoses` + - the new `TestStripWikitext` case + - `TestOpenRefusesAMismatchedBuilderVersion` +- Tests added that do not depend on a reverted fix: + - `TestDrainAndShutdownOrdersDrainBeforeShutdown` + - `TestServersSetOnlyAnIdleTimeout` + - `TestCheckHealth` + - `TestRandomOpeningWordFromIsReproducible` + - `TestOpenRefusesADatabaseWithNoBuilderVersion` +- Docker is available. `docker build --build-arg FIXTURE_DICT=1 -t noitu:review .` succeeded. In the running container, `docker exec ... -healthcheck` returned 0, the container became `healthy`, `app/LICENSE` was in the image, and `docker stop` produced `draining` then `shutting down` log lines and exited in 2.3s. I removed the test container and image afterwards. +- `make help` still lists all 14 lines. Workflow YAML parses. +- The real-corpus ladder test is skipped here (no real dictionary), so the effect of the negamax change on it was not re-measured. + +## Manual steps for the maintainer + +Coolify (app "noitu", currently zero env vars, health check off): + +1. Run `docker network inspect coolify` on the host and note the subnet Traefik reaches the app on (or the app's own network). +2. Set `NOITU_TRUSTED_PROXIES=<that CIDR>` and `NOITU_MAX_CONNECTIONS_PER_IP=32`. +3. Set `NOITU_DRAIN_TIMEOUT` below the container's stop grace minus 2s. With Docker's 10s default that means at most `6s`. Check what Coolify's stop timeout actually is before going higher. +4. Leave the dashboard HTTP health check off. After the next deploy, confirm Coolify picks up the Dockerfile `HEALTHCHECK`. +5. Optional: add the Traefik `loadbalancer.healthcheck.path=/readyz` custom label, using the generated service name. +6. If Cloudflare is in front, configure Traefik `forwardedHeaders.trustedIPs` for its ranges. + +GitHub: + +7. M3: merge `dev` into `main`, or cherry-pick `.github/dependabot.yml`. Enable Dependabot alerts and security updates, secret scanning and push protection. Then accept the action major bumps Dependabot proposes. +8. L7: Settings, Actions, Workflow permissions: set read-only and untick "Allow GitHub Actions to create and approve pull requests". + +## Unresolved questions + +- The stop grace Coolify applies to this container is unverified. The docs state the rule and Docker's default only. +- The Coolify claims (Dockerfile `HEALTHCHECK` honoured, `docker network inspect coolify`) come from the review reports and Coolify's public behaviour, not from a deploy. The docs say to confirm after the first deploy. +- `buf-action` with no `version` input is assumed to resolve to the newest buf. The first CI run on `proto.yml` will confirm it. diff --git a/plans/reports/fullstack-developer-260929-1939-web-fixes.md b/plans/reports/fullstack-developer-260929-1939-web-fixes.md new file mode 100644 index 0000000..b432f0e --- /dev/null +++ b/plans/reports/fullstack-developer-260929-1939-web-fixes.md @@ -0,0 +1,132 @@ +# Web fixes from the review + +Branch `dev`, 2026-09-29. Not committed. Only files under `web/` changed. Playwright was not run (no browser), and no e2e spec was added or changed. + +## Gates + +`cd web && npm run check && npm run lint && npm test`: check 0 errors and 0 warnings (386 files), lint clean, 326 tests passed in 18 files (was 270 in 16). `tests/bundle.test.js` is in that run and passes. + +Fail-without-fix check: I reverted each behavioural fix one at a time and ran its test file. Every revert made at least one test fail, and each file was restored afterwards. The reverts were: +- `session_not_resumable` in `LEAVES_ROOM` +- the `iAmOut` row check +- the ordinal reuse in the chat history +- the numbering in `chainToText` +- `freshSession` +- the `game.leave()` on a refused resume +- the `onResumeRefused()` call in `client.js` (4 tests fail) +- the shortest-RTT clock filter +- the GameOverPanel focus guard +- the ChatPanel refused-send guard +- the ChatPanel offline disable +- the away banner's `role="status"` + +Finding 1 is the exception: it needed no client change, so there was nothing to revert. + +## Per finding + +1. **Game ended while away.** No client change was needed. The reducer already handles the replay sequence from `playing`: `RoomState` fills in the room and leaves the phase alone, `ChatHistory` replaces the chat, and `GameOver` then moves the phase to `over` with the result and standings. The lobby actions appear because `over` renders the Lobby. Three store tests pin it: the exact sequence lands on `over` with standings, the room snapshot survives (so Ready and Leave are available), and a mid-game `RoomState` alone does not end the game. These tests pass on the unmodified reducer, so they are regression pins and not fail-without proofs. +2. **Refused resume.** + - `session_not_resumable` is now in `LEAVES_ROOM` (`game-apply.js`). + - `client.js` tracks whether the current Hello carried a token. After the Welcome, an `error` that arrives before any other non-Pong frame is a refused resume. The client then drops the spent token and calls the new `onResumeRefused` option. Pongs are skipped because the client pings right after Hello. `connection.svelte.js` wires the option to `game.leave()`. It runs before the error is forwarded, so the board clears and the error banner still shows. + - Tests in `ws-client.test.js` cover: both codes, a Pong in between, an error after the restored room (no report), a Hello without a token (no report), a spent token, and error forwarding. Two tests in `connection.test.js` and one in `game-store.test.js` cover the rest. + - On `/play`, an in-game refusal now lands on an idle board with the error banner. I did not add a "start again" button. +3. **`iAmOut`.** It is now true when `elimination !== null` or the player's own row in `gamePlayers` is eliminated. Two tests: out from the row alone, and another player's elimination does not count. +4. **`/play` reload.** + - `connect({ freshSession: true })` forgets the stored token before a new client is created. `startGame()` uses it. + - I did not call `forgetSession()` unconditionally before `connect()`. A rematch calls `startGame()` again on the socket that already exists. Forgetting there would delete the token this game's Welcome had just stored, and a later socket drop would open a fresh session instead of resuming the game. The flag only acts when a new client is created, so an existing socket keeps its token. + - `forgetStoredSession()` is now a module-level export in `client.js`, and the client's `forgetSession` is the same function. It is needed because there is no client yet on the first mount. + - `connection.test.js` checks the first Hello carries no token with the flag, carries the stored token without it, and that a second `connect({freshSession})` on a live socket keeps the Welcome's token. + - The `+page.svelte` call site itself is not covered, because there is no page-level test harness. +5. **Held action vs resume.** In `online/+page.svelte` the flush effect now waits for the resume's `RoomState` before flushing a held action, if the socket dropped while in a room. + - It notes the roster when the socket drops. After reopening, it releases the action once the roster object changes, since each `RoomState` replaces it. + - If the resume is refused and the room is gone, the held action is cleared instead of sent. + - An action held outside a room (cancelling the queue) is still flushed as soon as the socket opens. + - This is page logic and has no unit test. It needs a human or e2e check. +6. **Leave while offline.** `leave()` now sends `LeaveRoom` if the socket can carry it and otherwise drops it, never holds it. It also clears any other held action. The seat is freed by grace expiry either way. +7. **Chat offline.** `onsend` returns a boolean and `say()` returns `send()`'s result. The draft is cleared only when the send went out. The send button is disabled while `connection.status !== OPEN`, and `submit()` also refuses offline. New tests: a refused send keeps the text, and the button is disabled and nothing is sent while reconnecting. The existing chat tests now set the connection to open. +8. **Away countdown.** Each banner's `role="status"` span holds only "X mất kết nối…". The seconds sit in a sibling `aria-hidden` span. `data-testid="away-…"` is on the outer `<p>`, so the e2e `toContainText('… mất kết nối')` still matches. `vi.js` swaps `playerDisconnectedIn` for `playerDisconnectedSeconds: '({n}s)'`. +9. **GameOverPanel focus.** New `src/lib/focus.js` exports `typingElsewhere(own)`. WordInput imports it instead of its private copy, and GameOverPanel only focuses when the player is not typing elsewhere. Tested with a focused external input. +10. **Clock offset.** The client keeps the last 5 pong samples `{rtt, offset}` and uses the offset of the one with the smallest RTT. The existing single-sample test is unchanged. New tests: a slow pong does not move the offset, a faster pong takes over, and an old fast sample ages out. +11. **Partial chain.** + - `chainToText` numbers from `result.chainLength`: the opening word is always 1, and later words are numbered back from the total. It inserts a `…` line after the opening when words are missing (new string `exportGap`). Two tests cover a partial and a complete chain. + - Skipped: the optional "…" row in `ChainHistory` on screen. It was marked optional. +12. **Chat re-announce.** A `chatHistory` line matching an on-screen line on `(atMs, playerId, text)` keeps that line's ordinal, so its keyed row is not re-inserted into the live region. Identical duplicate lines each claim one old line. Two tests. +13. **WordInput tests.** + - Added: submit clears the field only when `onsubmit` returns true, and submit is refused during composition (and works after `compositionend`). + - Added: `beforeinput` is cancelled out of turn and not on the player's turn, and a composition's text is reverted out of turn. + - The vacuous composition test is rewritten as "leaves the player's own text alone on their turn". It fails if the revert ignores `enabled`, but its purpose is to pin the on-turn behaviour. +14. **Tests.** + - New `tests/status-components.test.js` covers `CountdownRing`, `PlayerStatus`, `GameOverPanel` and `ArmedButton`. Details are below. + - `ws-client.test.js` now covers storage that throws on read, on write, on remove, and a `sessionStorage` property that throws. + - New `tests/reactive-props.svelte.js` is a helper only, so a test can change a mounted component's props (the `$state` rune needs a `.svelte.js` module). + - `i18n.test.js` has a new test that scans `src/**` for `fill(t.key, {…})` calls and checks the object's keys equal the template's placeholders. It asserts more than 10 calls were matched, and it cannot see calls whose template is chosen by a conditional, which the test comment notes. + - Skipped: a `Lobby` component test. It was marked "if time allows", and Lobby is large and mostly wiring. +15. **`vite.config.js`.** The comment no longer says "two suites". +16. **`h1` on `/online`.** Now `var(--text-5)`, which is what the rules page `h1` uses. The size goes from 1.3rem to 1.5rem, a small visible change. + +`status-components.test.js` contents: +- `CountdownRing`: idle dash, rounded-up seconds and label, urgent only on the player's own turn, stalled instead of urgent offline, and the spoken 10 s mark. +- `PlayerStatus`: banner structure, countdown to zero, banner removed on return. +- `GameOverPanel`: standings and rank classes, no table for a one-row result, focus taken, focus refused while typing elsewhere, rematch button only when supplied, and export producing a `.txt` download. +- `ArmedButton`: arm, confirm, timeout disarm, disarm on `disabled`. + +## Other changes + +- **Footer link (security review L5, footer part).** `data/ATTRIBUTION.md` (the nine-item modification list) is not served over HTTP by the server or the web build. `AttributionFooter` now links "xem danh sách thay đổi" to `https://github.com/tiennm99/noitu/blob/main/data/ATTRIBUTION.md`. I confirmed the file exists on `main`. Strings `attributionModified` and `attributionChanges` are in `vi.js`. The rules page was not changed. +- **New error message.** `errcodes.go` now has `unknown_message`, added concurrently by the server agent. It made `tests/error-codes.test.js` fail, so I added a Vietnamese message for it in `vi.js`: "Máy chủ không hiểu yêu cầu này. Hãy tải lại trang." Please confirm the wording. + +## `.primary` consolidation + +`app.css` now has a global `.primary`: `border-color: transparent`, `background: var(--accent)`, `color: var(--accent-text)`, `transition: background-color 150ms ease-out`, plus `:hover:not(:disabled)` to `--accent-hover`, `:active:not(:disabled)` to `--accent-pressed`, and `:disabled` to `--surface-alt` background and `--text-muted` colour. It does not use `:where()`. Focus-visible is unchanged, since the global `:where(...)` focus rule was never touched. Sizing, padding, radius, weight and border width stay local. + +Rules removed: +- **`online/+page.svelte`:** + - `.primary` lost `background`, `color` and `transition`, keeping `min-height`, `padding`, `border: 0`, `border-radius` and `font-weight`. + - Removed `.primary:hover:not(:disabled)`, `.primary:active:not(:disabled)` and `.primary:disabled`. + - These were identical to the global ones. +- **`ChatPanel.svelte`:** + - `.row button` lost `background`, `color` and `transition`. + - Removed `.row button:hover:not(:disabled)`, `.row button:active:not(:disabled)` and `.row button:disabled`. + - The send button gained `class="primary"`. +- **`WordInput.svelte`:** + - `.input-row button` lost `background`, `color` and `transition`. + - Removed `.input-row button:hover:not(:disabled)`, `:active:not(:disabled)` and `:disabled`. + - The submit button gained `class="primary"`. + - The `.fix.suggestion` rules are untouched. +- **`+page.svelte` (landing):** + - Removed `.actions .primary`, `.actions .primary:hover` and `.actions .primary:active`. + - The old hover and active had no `:not(:disabled)`, but that button is never disabled. + - `.actions > *` kept size, padding, radius and weight. It no longer sets the border colour, background or `color: inherit`. + - A new `.actions > :where(:not(.primary))` sets `border-color`, `background: var(--surface)` and `color: inherit`. + - `border` was split into `border-width: 1px; border-style: solid`, so the shorthand's `currentcolor` cannot beat the global transparent. +- **`GameOverPanel.svelte`:** + - Removed `.actions .primary`, `.actions .primary:hover` and `.actions .primary:active`. + - `.actions button` kept its sizing and `border-width`/`border-style`. The border colour and `background: var(--surface)` moved to `.actions button:where(:not(.primary))`. + - `:where()` keeps that rule's specificity at (0,1,1), so `.actions .export` still overrides it as before. + +I checked the compiled selectors of the landing page and GameOverPanel with the Svelte compiler. + +The report claimed the local rules only restated the colours. For the landing page and GameOverPanel that was wrong: `.actions > *` and `.actions button` set a background that would have beaten the global class. The `:where(:not(.primary))` split above is the fix. Lobby is untouched. + +## Needs a human visual check (no browser here) + +- The four `.primary` surfaces: landing "Chơi với máy", `/online` quick match and create buttons, the chat send button, and the word submit button. Check rest, hover, press, disabled and the dark theme. +- The GameOverPanel action row: the rematch button is accent-filled, home is a bordered button, and the export button is transparent and muted. +- The `/online` `h1` at 1.5rem. +- The away banner: the seconds should sit inline after the sentence, with a space between them. +- The footer: the extra sentence and link should still fit on a phone width. +- Behaviour that needs a real reconnect, ideally an e2e run when a browser exists: + - Finding 5: kick or ready right after a socket cut. + - Finding 6: leave while offline. + - Finding 7: chat button disabled during a cut. + - Finding 4: reload `/play` mid-game. + - Finding 1 end to end, once the server change lands. + +## Notes + +- After a refused resume on `/play` the player sees an idle board with a banner and must use Home or reload. An automatic restart would be a product call. +- The server's replay for Finding 1 was not verified against the wsapi change, which is being written concurrently. The client relies only on the frame order given in the task. + +Status: DONE_WITH_CONCERNS +Summary: All 16 findings, the `.primary` consolidation and the footer link are done in `web/`; check, lint and 326 tests pass, and each behavioural fix has a test that fails when reverted (Finding 1 needed no client change). +Concerns: The Finding 5 page logic, the `/play` `freshSession` call site and all CSS changes are unverified without a browser (list above). I added a Vietnamese string for the new server code `unknown_message`. I did not add an e2e spec or a Lobby component test. diff --git a/plans/reports/fullstack-developer-260929-1939-wsapi-fixes.md b/plans/reports/fullstack-developer-260929-1939-wsapi-fixes.md new file mode 100644 index 0000000..c04a079 --- /dev/null +++ b/plans/reports/fullstack-developer-260929-1939-wsapi-fixes.md @@ -0,0 +1,70 @@ +# wsapi fixes + +Scope: `server/internal/wsapi/` only, no commit. All listed findings are fixed; nothing skipped. + +## What changed per finding + +wsapi review: +1. Stale token resuming into a reused seat. `seat.heldFor` records the connection whose drop opened the window. `handleResume` accepts only the live prior or the connection the window is held for. A kicked player is answered `session_not_resumable`, and tokens are not revoked at kick. +2. Inputs queued behind a room's last one. `run` now registers `cancel` last (so it runs first) and `refusePending` first (so it runs last). It answers joins with `room_not_found`, resumes with `session_not_resumable`, create/bot start with `room_start_failed`, submit/resign/claim with `not_in_a_game`, and lobby/chat/report with `not_in_a_room`. +3. Control notices in the lossy inbox. Resign is charged on `submitLimiter`. `leaveRoom` uses the new blocking `room.sendReliably`. `attach` calls it from its own goroutine so two rooms cannot wait on each other. `resumeFrom` answers `busy` for a live but full room and `game_already_over` only for a finished one. +4. Quick-match `autoStart` outliving its pairing. The first `handleJoin` consumes the flag, including on early returns. +5. Idle window. It restarts only when the room changed (`lobbyChanged` read before the broadcast). It also starts or stops when the room crosses between lobby and game, because `beginGame` does not set `lobbyChanged`. A ready toggle counts as activity, as decided. +6. IPv6 and unmapped-IPv4 keying. `limiterKey` unmaps IPv4-mapped addresses, drops the zone, and folds IPv6 to its /64. `clientIP` applies it, so the join limiter, the connection cap and the room budget share one key. +7. Quick match while draining. `hub.quickMatch` returns `errDraining` first. +8. No `default` arm. It now answers `unknown_message`. The code is in `errcodes.go`, and `vi.js` already has it (added by the web implementer). +9. `TestChatDoesNotKeepARoomAlive` now fails if the idle close comes later than `IdleFor` + 200ms. +10. `TestIdleRoomReleasesItsSeats` now requires `not_in_a_room` after the idle close. +11. `TestPointKindMappingIsExhaustive` now checks each kind maps to the wire name derived from `String()`. +12. A resume calls `hub.cancelQuickMatch`. +13. `TestOneConnectionCannotStrandRooms` uses `awaitNoRooms`. + +Security review: +- H1a. `session.run` arms a 10s `helloTimeout` timer that sends `handshake_required` and closes the socket. `handleHello` stops it. Tests override it through `hub.helloTimeout`, which is unexported. +- H1b. `hub.roomLimiter` is a keyed limiter on the client address. It sits alongside the per-connection budget, is charged in `allowRoom`, and is swept in `sweepLimiters`. Budget is 0.5/s with burst 30 (`addressRoomsPerSecond`, `addressRoomBurst`). The comment gives the NAT and no-trusted-proxy reasoning, and notes that 0.5/s over the 10 minute idle window is 300 rooms, about 30% of the default ceiling. The per-IP connection cap default is unchanged. +- L1. Same as wsapi #6. +- L2. New `corpuslog.go`: one process-wide bucket (20/s, burst 100) in front of all three `word_rejected` and `word_reported` sites. Suppressed lines feed a new expvar `noitu_corpus_log_suppressed`. The next line to get through carries `suppressed_before=N`. The per-session limit is kept. +- L3. `Server.ServeHTTP` sets `X-Content-Type-Options: nosniff`, `Content-Security-Policy: frame-ancestors 'self'` and `Referrer-Policy: strict-origin-when-cross-origin` on every response. The static handler lives in `server.go`, so `main.go` is untouched. HSTS is left to the proxy. +- N1. `blankLetters` drops U+115F, U+1160, U+3164, U+FFA0, U+2800, and the Khmer inherent vowels U+17B4 and U+17B5. A name made only of these falls back to the default nickname. + +Server-core #3: `sanitizeText` maps every `unicode.IsSpace` rune to `' '` first. This also turns VT, FF, NEL and U+2028/2029 into spaces where they used to be dropped. "ngữ pháp" typed with NBSP now reaches the engine as two syllables. + +Web #1: a resume into a lobby replays the seat's own GameOver right after RoomState, in the seat's own rendering. `broadcastGameOver` now renders for every human seat. A detached seat keeps its version in `seat.missedResult`. It is cleared when replayed, when the game overtakes it, or when the next game begins. The replay is queued in `room.resumeReplays` and flushed by the run loop right after `broadcastRoomState`. + +## Resume frame order for the web client + +Resume into a lobby after a game ended during the absence: `welcome`, `chat_history`, `room_state`, `game_over`. + +- The other seats also get their own `room_state` broadcast. +- A resume into a running game is unchanged: `welcome`, `chat_history`, `game_started`, `turn_update` (if a move exists), then `room_state`. +- A resume into a lobby whose game the player saw live gets no extra frame. +- The replayed GameOver has the same shape as a live one. A client that already handles `game_over` while sitting on a lobby screen needs no change beyond accepting it after `room_state`. +- Resume errors: `busy` is now a possible answer to a resume (a full inbox). `unknown_message` is new. Both are already keys in `vi.js`. + +## Verification + +- `go vet ./...`, `gofmt -l .` and `golangci-lint run ./...` are all clean (0 issues). +- `go test ./internal/wsapi -race -count=3` passes (96s). +- `go test ./... -race -count=1` passes, all packages. +- Old-behaviour check: I copied the module to a scratch directory and reverted each fix by hand, one at a time. The corresponding test failed for every one of these: + - heldFor, refusePending, resign limiter, reliable send, busy answer, autoStart, idle rule + - limiterKey (both the mapped-address and /64 halves), quick-match drain, default arm, hello timer (two variants), address room budget, corpus limiter, headers + - NBSP mapping, blank letters, missed GameOver, resume dequeue + - chat resetting idle (the tightened chat test), `detachAll` removal, swapped PointKind arm +- Existing tests changed: + - `TestFrameFloodClosesTheConnection` reads until the close instead of five frames, because in-burst empty frames now get `unknown_message` replies. + - `newTestServer` takes optional `func(*Server)` options. +- No bare sleeps as synchronisation: + - The idle tests use pacing sleeps with wall-clock assertions in the direction that a stall cannot fool. + - The hello test orders itself through the silent socket's close. + - The handler-level tests wait on the outbox. + +## Notes for others + +- `docs/deployment.md` (docs owner): document the new `noitu_corpus_log_suppressed` counter, the 10s hello deadline, and the per-address room budget. The per-IP connection cap default is unchanged, so H1's deployment half (set `NOITU_TRUSTED_PROXIES` and `NOITU_MAX_CONNECTIONS_PER_IP`) is still an operator task. +- Limitation in the exit fix: `room.send` checks `ctx.Done` and then enqueues, so a send that passes the check exactly as the room exits can still land after the final drain. This is a much narrower window than before (nanoseconds, not the whole queue), and closing it fully would need a lock on the hot path. +- No proto change. + +Status: DONE +Summary: All 13 wsapi findings, security H1, L1, L2, L3 and N1, server-core #3 and the web resume GameOver replay are implemented in `server/internal/wsapi/` with tests. Vet, gofmt, golangci-lint, `wsapi -race -count=3` and the full suite are clean. +Concerns/Blockers: none blocking. The tiny residual window in room exit is noted above. The deployment half of H1 (trusted proxies and per-IP cap) still needs the operator. diff --git a/plans/reports/review-260929-1939-whole-project-review-and-fixes.md b/plans/reports/review-260929-1939-whole-project-review-and-fixes.md new file mode 100644 index 0000000..a821be8 --- /dev/null +++ b/plans/reports/review-260929-1939-whole-project-review-and-fixes.md @@ -0,0 +1,84 @@ +# Whole-project review and fixes (dev, 2026-09-29) + +Four parallel reviews (server core, wsapi, web, security/ops) followed by three +parallel implementation passes with disjoint file ownership. Nothing is +committed. Baseline before and after: Go vet, gofmt, golangci-lint and +`go test ./... -race` clean; web check, lint and vitest clean (270 → 326 tests). + +## Reviews + +- [Server core](code-reviewer-260929-1939-server-core-review.md) — 10 findings +- [wsapi](code-reviewer-260929-1939-wsapi-review.md) — 13 findings +- [Web](code-reviewer-260929-1939-web-review.md) — 16 findings +- [Security and ops](code-reviewer-260929-1939-security-ops-review.md) — 8 fix-now, 6 nits, 2 non-issues + +## Implementation + +- [Server core and ops fixes](fullstack-developer-260929-1939-server-ops-fixes.md) +- [wsapi fixes](fullstack-developer-260929-1939-wsapi-fixes.md) +- [Web fixes](fullstack-developer-260929-1939-web-fixes.md) + +Highest-impact fixes: + +1. SIGTERM no longer kills every game before the drain: the restart notice and + `NOITU_DRAIN_TIMEOUT` now work; a second signal exits immediately. +2. A kicked player's token can no longer resume into whoever now holds that + seat. +3. One client can no longer hold the global connection or room caps: sockets + that never send Hello close after 10s, and the room budget is charged per + address. +4. A player who reconnects into the lobby after their game ended while away is + now replayed that game's GameOver, so the UI is no longer stuck on a frozen + board; refused resumes now clear the stale room or board on the client. +5. Hard bot prefers the slower loss in lost positions (41/3000 boards fixed). +6. Non-breaking spaces in a typed word are now treated as spaces. +7. Container HEALTHCHECK via `noitu-server -healthcheck`; LICENSE in the image; + security headers; IdleTimeout; CI runs govulncheck and npm audit; buf action + replaced; checkout without persisted credentials. + +Decisions taken in this session (product questions the reviewers raised): + +- Resume-after-game-over replays GameOver rather than adding a proto field. +- Reloading `/play` starts a fresh game on the same rung (the page's documented + intent); a socket drop inside the same tab still resumes. +- A kicked player who returns gets `session_not_resumable`; kicks do not revoke + tokens. +- Ready toggles count as lobby activity for the idle window. +- Resigning out of turn follows the README: no immediate knock-out. +- Added after the web pass: `/play` shows a "Chơi lại" button when a refused + resume leaves the board idle under the error banner. + +## Manual steps outside the repo + +Coolify (verified today: the app deploys `main` with no env vars and the +dashboard health check off): + +- Set `NOITU_TRUSTED_PROXIES` to the Traefik/Docker network range, then + `NOITU_MAX_CONNECTIONS_PER_IP=32`. +- Set `NOITU_DRAIN_TIMEOUT` so that drain + 2s is under the container stop + grace (6s or less under Docker's 10s default), or raise the grace. +- After the next deploy, confirm the Dockerfile HEALTHCHECK is picked up. + +GitHub: + +- The maintainer chose to drop `dependabot.yml` rather than move it to main; + the CI govulncheck and npm audit steps are the dependency signal instead. + Enable Dependabot alerts and secret scanning in repo settings if wanted. +- Set default workflow permissions to read-only and disable "Allow GitHub + Actions to create and approve pull requests". + +## Needs a human visual check (no browser on this host) + +- The `.primary` buttons in light and dark, all states, on the online page, + landing page, ChatPanel, WordInput and GameOverPanel. +- The `/online` heading at 1.5rem (was 1.3rem); the away banner and footer on a + phone width; the new `/play` restart button. +- Reconnect flows on `/online` (held actions after a drop, leave while offline, + chat while offline) and the `/play` refused-resume path. +- Playwright was not run. + +## Unresolved questions + +- Confirm the wording of the new `unknown_message` string in `vi.js`. +- Whether the first `proto.yml` run with `bufbuild/buf-action@v1` and no + `version` input resolves to the newest buf.