Various bug fixes and performance fixes

[1brucben]

Multiple bug fixes.
This pull request introduces several improvements and refactors across the codebase, focusing on UI/UX enhancements, code maintainability, and backend integration. The most notable changes include the addition of a robust lobby update system with WebSocket and HTTP fallback, improvements to game configuration update handling, and a series of map and naming consistency updates.

Lobby system and event handling improvements:

• Added a new PublicLobbySocket class in src/client/LobbySocket.ts to manage real-time lobby updates via WebSocket, with automatic fallback to HTTP polling if the WebSocket connection fails. This ensures users always see up-to-date lobby information.
• Integrated a new custom event "update-game-config" in HostLobbyModal (src/client/HostLobbyModal.ts) and wired it into the main client event system (src/client/Main.ts). This decouples the UI from direct HTTP calls and allows for more flexible game configuration updates. [1] [2] [3] [4] [5]

Gameplay and UI enhancements:

• Improved boat attack logic in ClientGameRunner (src/client/ClientGameRunner.ts), including a new canAutoBoat method for smarter auto-boat attack decisions and refactored related logic for clarity and maintainability. [1] [2] [3]
• Added a "Spectate" option to the English language resources, enhancing the user interface for game spectators.

Map and naming consistency updates:

• Standardized nation names across multiple map files for clarity (e.g., "DR Congo" instead of full country names, "Algeria" instead of longer forms, etc.). [1] [2] [3] [4] [5] [6]
• Added a comprehensive test map map-generator/assets/test_maps/world/info.json listing nations and their coordinates, likely for development or testing purposes.

Build and project structure:

• Removed the gatekeeper submodule from .gitmodules, indicating a change in how dependencies are managed.
• Updated ESLint configuration to ignore additional test playground files, reducing unnecessary linting noise.

General fix: Avoid constructing the internal request URL directly from user input. Instead, use the user input only as a key into trusted server-side data (for instance, a map of active games to worker ports and worker endpoints). Use the result of that lookup to build the URL (or even just post to a known admin endpoint on the worker), and keep the path and host under full server control.

Best concrete fix here: publicLobbiesData already exists and presumably tracks current games. We can use gameID and clientID to look up a corresponding public lobby entry and derive trusted data (like worker port and the canonical game/client IDs) from that object. We then construct the URL using those trusted values. This way, the request URL is no longer based directly on the untrusted params but on server-side state that must already be valid to exist. This addresses CodeQL’s taint-tracking while preserving functionality (we still kick the same player in the same game, but only if that game/client pair exists in our tracked lobbies).

Concretely in src/server/Master.ts around the /api/kick_player/:gameID/:clientID handler:

1. After validating gameID and clientID, look up the corresponding lobby in publicLobbiesData (or whatever structure is holding active games). If no entry matches, return 404 (or 400).
2. Derive a trusted workerPort from the lobby’s stored gameID (or stored port if present).
3. Construct the URL using the lobby’s stored gameID and clientID instead of the raw req.params, so the taint stops at the lookup.
4. Keep the rest of the logic (admin header check, fetch, response handling) unchanged.

This requires only edits inside the shown handler body and uses existing imports and variables; no new external dependencies are needed.

In general, to fix externally controlled format string issues with Node’s console (or util.format), ensure that untrusted data is never part of the format string argument. Instead, use a constant format string containing %s (or appropriate specifier) and pass the untrusted value as a separate argument, or avoid formatting features by concatenating into a single string and passing only one argument.

For this specific case in tests/pathfinding/playground/server.ts, we should change the console.error call in the /api/maps/:name route so that the first argument is a constant string containing %s, and pass req.params.name and error as subsequent arguments. Functionality remains the same—the logged message content is equivalent—but we no longer allow user input to influence the format string. No new imports or helper methods are needed; we only adjust the single logging statement at line 53.

Concretely:

• In tests/pathfinding/playground/server.ts, locate the console.error line inside the catch block of the /api/maps/:name route.
• Replace:
  • console.error(`Error loading map ${req.params.name}:`, error);
• With:
  • console.error("Error loading map %s:", req.params.name, error);

No other lines or files need to change.

In general, the fix is to introduce a rate-limiting middleware that caps how many requests a client can make to handlers that perform filesystem access, in this case the /api/maps/:name/thumbnail route. The usual way in Express is to use a library such as express-rate-limit to define a limiter (specifying a time window and a max number of requests per IP) and then apply that limiter either globally or to specific routes.

The minimal, non-breaking change here is:

• Import express-rate-limit.
• Create a dedicated limiter for thumbnail requests (e.g., allow a reasonable number per minute).
• Apply that limiter only to the /api/maps/:name/thumbnail route so existing behavior of other endpoints is unchanged.

Concretely in tests/pathfinding/playground/server.ts:

1. Add an import for express-rate-limit near the top.
2. After creating the Express app and before defining routes, define a thumbnailRateLimiter using rateLimit({ windowMs: ..., max: ... }).
3. Modify the app.get("/api/maps/:name/thumbnail", ...) registration to include the limiter as middleware: app.get("/api/maps/:name/thumbnail", thumbnailRateLimiter, (req, res) => { ... }).

No other logic inside the handler needs to change.

In general, to fix uncontrolled path usage, normalize the computed path and enforce that it remains within a designated root directory, or restrict the user input to a simple, safe filename pattern (e.g., alphanumerics and dashes only).

For this specific code, the least intrusive fix is to treat ../../../resources/maps as a root directory, resolve any requested thumbnail path relative to that root using path.resolve, and then verify that the final path still starts with the root directory. If it does not, return a 400/403 error instead of calling res.sendFile. This preserves existing behavior for valid map names while blocking traversal attempts. We can reuse the existing dirname and join imports; we need to also import resolve from "path" to build safe absolute paths.

Concretely, in tests/pathfinding/playground/server.ts:

• Update the import from "path" to also import resolve.
• Inside the /api/maps/:name/thumbnail route:
  • Compute a mapsRoot directory once (using join(dirname(fileURLToPath(import.meta.url)), "../../../resources/maps")).
  • Compute requestedPath using resolve(mapsRoot, name, "thumbnail.webp"), which normalizes any .. segments.
  • Check that requestedPath starts with mapsRoot (after normalizing both to absolute paths); if not, respond with 400 or 403.
  • Call res.sendFile(requestedPath) instead of the old thumbnailPath.

This keeps functionality the same for legitimate names but blocks malicious ones.

In general, to fix externally-controlled format string issues with console.* or util.format, avoid embedding untrusted input directly into the format string. Instead, use literal format strings (e.g. "Error loading thumbnail for %s:") and pass untrusted data as subsequent arguments, or concatenate using +/template literals without relying on formatting semantics when multiple arguments are used.

Here, the safest and smallest change is to keep the log message semantics but move req.params.name out of the template literal and into a separate argument. For example, change:

console.error(`Error loading thumbnail for ${req.params.name}:`, error);

to:

console.error("Error loading thumbnail for %s:", req.params.name, error);

This ensures the format string is a constant, and the user-controlled name is only substituted into the %s placeholder. No change in behavior is introduced: logs will still show the map name followed by the error. This change is localized to tests/pathfinding/playground/server.ts line 84, and no new imports or helper methods are required.

To fix uncontrolled path usage, we should constrain user-provided map identifiers (mapName / name / map) so they cannot escape the intended maps directory. The safest and least intrusive approach here is:

1. Treat mapName as a simple identifier (no path separators or ..), and reject anything that doesn’t match a strict pattern.
2. Additionally, resolve the final paths under a known maps root (mapDirectory / maps dir for thumbnails) and verify that the resolved path still lies within that root before accessing it.

This preserves existing behavior for legitimate map names but prevents directory traversal.

Concretely:

• In tests/pathfinding/utils.ts:

  • Add a small helper to validate mapName (e.g., only alphanumerics, _, -, .) and throw if invalid.
  • In setupFromPath, before constructing paths, validate mapName.
  • Build the base directory as const baseDir = path.resolve(mapDirectory);.
  • Build mapBinPath and miniMapBinPath as path.resolve(baseDir, mapName, "map.bin") / "map4x.bin".
  • After resolving, check that each path starts with baseDir + path.sep (or equals baseDir) to enforce containment. If not, throw an error.

• In tests/pathfinding/playground/server.ts:

  • For /api/maps/:name and /api/maps/:name/thumbnail, validate name with the same pattern; if invalid, respond with 400.
  • For /api/pathfind and /api/spatial-query, validate map similarly before passing it to computePath / computeSpatialQuery (which eventually reach setupFromPath), returning 400 on invalid input.
  • For the thumbnail endpoint, compute a safe root maps directory (the same one getMapsDirectory() uses; since we don’t see that code, we approximate with the existing join(dirname(fileURLToPath(import.meta.url)), "../../../resources/maps") resolved with path.resolve), then:
    • Build the thumbnail path with path.resolve(mapsRoot, name, "thumbnail.webp").
    • Ensure the resolved path starts with mapsRoot + path.sep; if not, return 400 or 404.
    • Only then call res.sendFile.

We will implement a single central validator function (e.g., isValidMapName) in server.ts and a similar or identical copy in utils.ts (because they are in different modules and we must not assume extra wiring). These changes only add validation and containment checks and do not otherwise alter the logic of loading and using maps.

---

In general, the fix is to ensure that any user-controlled path component is constrained to a safe root directory or strictly sanitized. Here we should (1) compute a canonical path under a known root (the maps directory), (2) normalize it via path.resolve, and (3) verify that the resulting path still resides under that root. If it does not, we should reject the request. This keeps behavior the same for valid map names but prevents directory traversal via .. or absolute paths. We can implement this once in setupFromPath, since both getMapMetadata and the /api/pathfind and /api/spatial-query endpoints funnel map loading through that function. Additionally, we should harden the thumbnail route to ensure the name parameter can only select files inside the maps directory.

Concrete changes:

1. In tests/pathfinding/utils.ts:

   • Introduce a small helper (or inline logic) in setupFromPath that:
     • Resolves mapDirectory to an absolute path.
     • Resolves a candidate map directory as path.resolve(rootDir, mapName).
     • Verifies that this resolved path starts with the root directory path plus a path separator (to avoid false positives on prefixes like /maps/foo vs /maps/foobar).
     • If the check fails, throws an error (in tests, throwing is fine; callers already handle errors).
   • Then build mapBinPath and miniMapBinPath using that safe resolved directory (e.g., const mapDir = ...; const mapBinPath = path.join(mapDir, "map.bin");), instead of joining raw mapDirectory and mapName.
   • This preserves existing functionality for valid map names while preventing a user from escaping the maps directory.

2. In tests/pathfinding/playground/server.ts:

   • For the thumbnail route /api/maps/:name/thumbnail, make sure name is validated or constrained. Since we already have a safe way of resolving map directories via getMapsDirectory and setupFromPath, the safest and most consistent approach is:
     • Import getMapsDirectory (if not already).
     • Resolve the maps root directory inside the route (or reuse a module-level constant).
     • Use the same root-directory enforcement strategy as in setupFromPath: resolve name against the maps root, ensure the resulting path is still under the root, and then build the thumbnail path as join(safeMapDir, "thumbnail.webp").
   • This prevents directory traversal via the name parameter in the thumbnail route as well.

These changes require no new imports beyond path, which is already imported in both files. No extra npm dependencies are needed.

---

In general, to fix uncontrolled path usage, either (a) normalize the constructed path and ensure it stays within a known root directory, or (b) restrict user input to a simple, validated name (or allow‑listed set), never allowing path separators or traversal sequences.

Here, the simplest, least invasive and most robust fix is:

1. In tests/pathfinding/utils.ts’s setupFromPath, treat mapDirectory as the trusted root and validate mapName:

   • Resolve safeBase = path.resolve(mapDirectory).
   • Resolve mapPath = path.resolve(safeBase, mapName).
   • Check that mapPath starts with safeBase + path.sep (or equals safeBase if you want to allow that); if not, throw an error.
   • Build mapBinPath and miniMapBinPath using this validated mapPath.
     This ensures mapName cannot escape the intended mapDirectory.

2. In tests/pathfinding/playground/server.ts, validate name used for thumbnails:

   • Add a small helper function like isValidMapName(name: string): boolean that enforces a conservative pattern (e.g. ^[A-Za-z0-9_-]+$), rejecting any path separators or traversal characters.
   • Before building thumbnailPath, check isValidMapName(name) and return 400 if invalid.
   • Optionally reuse this helper for other routes that accept :name to keep behavior consistent.

These changes avoid altering existing functionality for valid map names (they still resolve to the same directories/files), while preventing directory traversal via malicious values. No new external dependencies are strictly required; Node’s built‑in path is sufficient.

---

In general, to fix uncontrolled use of user input in path expressions, we need to (1) define a trusted root directory, (2) normalize any path derived from user input relative to that root, and (3) verify after normalization that the resulting path stays within the root. For simple “map name” semantics, we can also validate that mapName is a simple filename-style identifier (for example, only letters, numbers, underscores, and dashes) and reject anything containing path separators or ...

In this codebase, the single best fix without changing existing functionality is to sanitize and validate mapName inside setupFromPath in tests/pathfinding/utils.ts, because all the tainted flows ultimately converge there. We will introduce a small helper sanitizeMapName that enforces a strict allowlist of characters and rejects names with path separators or traversal patterns. This keeps the semantics “one directory per logical map” intact while preventing ../../ or similar tricks. Then we’ll use the sanitized name to build mapBinPath and miniMapBinPath. Since mapDirectory comes from getMapsDirectory() and is not user-controlled in the shown snippets, validating mapName is sufficient to break the taint flow and prevent traversal.

Concretely:

• In tests/pathfinding/utils.ts, add a sanitizeMapName function near the top of the file.
• In setupFromPath, before building paths, derive const safeMapName = sanitizeMapName(mapName); and use safeMapName in the path.join calls.
• The helper will:
  • Accept only names that match /^[A-Za-z0-9_\-]+$/.
  • Reject any name containing /, \, or ...
  • Throw a descriptive error if validation fails.
    This adds a narrow, explicit validation layer and leaves all other behavior unchanged.

---

[El-Magico777]

Hey, nice work on this PR — the pathfinding rewrite and perf optimizations are solid. Found a few things worth flagging:

---

🔴 1. Build-Breaking TypeScript Error in RadialMenu.ts

SendBoatAttackIntentEvent constructor was simplified from 4 params to 2 (dst, troops), but the call site in RadialMenu.ts at line 487–492 still passes the old 4 arguments (targetID, dst, troops, src):

// RadialMenu.ts:487-492 — passes 4 args
new SendBoatAttackIntentEvent(
  this.g.owner(tile).id(),  // targetID — removed from constructor
  dst,
  this.uiState.attackRatio * myPlayer.troops(),
  src,                       // src — removed from constructor
)

// Transport.ts:99-103 — constructor only accepts 2
export class SendBoatAttackIntentEvent implements GameEvent {
  constructor(
    public readonly dst: TileRef,
    public readonly troops: number,
  ) {}
}

TSC error: TS2554: Expected 2 arguments, but got 4.
This prevents the project from building.

---

🟡 2. Gatekeeper / Rate Limiting Fully Removed

Gatekeeper.ts is deleted entirely and all gatekeeper.httpHandler() / gatekeeper.wsHandler() wrappers are stripped from every API endpoint in Worker.ts and every WebSocket message handler in GameServer.ts. This means the server currently has no rate limiting — no protection against endpoint flooding, WebSocket spam, or other abuse.

If this branch is heading toward production, some form of rate limiting should be in place or planned as a follow-up.

---

🟡 3. Trade Ship Capture Behavior Changed

The old huntDownTradeShip() in WarshipExecution.ts had a nuanced 3-way check at the moment of capture:

1. Enemy trade ships → capture + clear trade route metadata + spawn CapturedTradeShipReturnExecution to navigate it home
2. Neutral trade ships with war/embargo endpoints → don't capture, just turn the ship around (setReturning(true))
3. Otherwise → disengage entirely

The new code unconditionally captures any trade ship the warship reaches:

case PathStatus.COMPLETE:
  this.warship.owner().captureUnit(this.warship.targetUnit()!);
  this.warship.setTargetUnit(undefined);
  this.warship.move(this.warship.tile());
  return;

Specifically:

• No secondary war/embargo re-check at the moment of capture (diplomacy could change during the multi-tick chase)
• The neutral "turn around" behavior is gone — neutral ships are now captured outright
• setTradeRouteOwners(null, null) is no longer called on capture to clear trade route metadata

The new TradeShipExecution.ts does handle the post-capture routing to a port, so that part is covered. Just wanted to confirm the simplification of the engagement rules at capture time is intentional.

[El-Magico777]

Related to #215