fix: close map OOM, admin traversal, transport safespot, room rekey, and legacy payout recovery

12 files changed, 348 insertions, 29 deletions

diff --git a/internal/admin/api_players.go b/internal/admin/api_players.go
index 66142c5..8ac0dd2 100644
--- a/internal/admin/api_players.go
+++ b/internal/admin/api_players.go
@@ -87,7 +87,23 @@ func (s *AdminServer) handlePlayerByID(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if strings.HasPrefix(path, "account/") {
-		s.getAccount(w, strings.TrimPrefix(path, "account/"))
+		acct := strings.TrimPrefix(path, "account/")
+		if err := player.ValidName(acct); err != nil {
+			http.Error(w, `{"error":"invalid account id"}`, http.StatusBadRequest)
+			return
+		}
+		s.getAccount(w, acct)
+		return
+	}
+
+	// Validate before the first join into players/characters/<name>.yaml.
+	// ServeMux cleans the ESCAPED path but handlers read the DECODED
+	// r.URL.Path, so a percent-encoded segment like %2e%2e%2f reaches this
+	// handler as "../" and would otherwise escape the data dir (and bypass
+	// the password_hash redaction in getAccount). ValidName is the same
+	// invariant every other name join in the codebase enforces.
+	if err := player.ValidName(path); err != nil {
+		http.Error(w, `{"error":"invalid player id"}`, http.StatusBadRequest)
 		return
 	}
 
diff --git a/internal/admin/api_players_path_test.go b/internal/admin/api_players_path_test.go
new file mode 100644
index 0000000..dca156e
--- /dev/null
+++ b/internal/admin/api_players_path_test.go
@@ -0,0 +1,87 @@
+package admin
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"thehouseoficarus/internal/config"
+	"thehouseoficarus/internal/player"
+)
+
+// TestPlayerAPICannotEscapeData: /api/players/{name} joins the raw URL
+// remainder into players/characters/<name>.yaml. ServeMux cleans the ESCAPED
+// path but handlers read the DECODED r.URL.Path, so a percent-encoded segment
+// (%2e%2e%2f) arrives as "../" and would otherwise read arbitrary *.yaml —
+// including account files, bypassing the password_hash redaction that
+// getAccount applies. The route must validate the name before the first join.
+func TestPlayerAPICannotEscapeData(t *testing.T) {
+	root := t.TempDir()
+	dir := filepath.Join(root, "data")
+	for _, sub := range []string{"players/accounts", "players/characters"} {
+		if err := os.MkdirAll(filepath.Join(dir, sub), 0o700); err != nil {
+			t.Fatal(err)
+		}
+	}
+	accounts := player.NewAccountStore(dir)
+	if err := accounts.SaveAccount(&player.Account{Name: "auditor", Admin: true, PasswordHash: "argon2id$auditor-hash"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := accounts.SaveAccount(&player.Account{Name: "victim", PasswordHash: "argon2id$victim-hash"}); err != nil {
+		t.Fatal(err)
+	}
+	victim := player.New("victim")
+	victim.Account = "victim"
+	if err := accounts.SaveCharacter(victim); err != nil {
+		t.Fatal(err)
+	}
+	// A canary file outside data/ that traversal could otherwise reach.
+	if err := os.WriteFile(filepath.Join(root, "outside.yaml"), []byte("secret: canary\n"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	s, err := NewServer(config.Default(), false, accounts, nil, nil, nil, nil, dir,
+		nil, nil, nil, nil, filepath.Join(dir, "config.yaml"),
+		func(fn func()) bool { fn(); return true })
+	if err != nil {
+		t.Fatal(err)
+	}
+	cookieRec := httptest.NewRecorder()
+	s.setAuthCookie(cookieRec, "auditor")
+	cookie := cookieRec.Result().Cookies()[0]
+
+	get := func(target string) *httptest.ResponseRecorder {
+		req := httptest.NewRequest(http.MethodGet, target, nil)
+		req.AddCookie(cookie)
+		rec := httptest.NewRecorder()
+		s.ServeHTTP(rec, req)
+		return rec
+	}
+
+	// Encoded traversal to an account file must be rejected, and the raw
+	// password hash must never appear in the response.
+	rec := get("/api/players/%2e%2e%2faccounts%2fvictim")
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("encoded traversal: status %d body %s", rec.Code, rec.Body.String())
+	}
+	if strings.Contains(rec.Body.String(), "victim-hash") {
+		t.Fatal("encoded traversal leaked a password hash")
+	}
+
+	// Traversal out of the data dir to a canary must be rejected.
+	rec = get("/api/players/%2e%2e%2f%2e%2e%2foutside")
+	if rec.Code != http.StatusBadRequest {
+		t.Fatalf("outside traversal: status %d body %s", rec.Code, rec.Body.String())
+	}
+	if strings.Contains(rec.Body.String(), "canary") {
+		t.Fatal("encoded traversal read a file outside the data dir")
+	}
+
+	// A legitimate lookup still works.
+	if rec = get("/api/players/victim"); rec.Code != http.StatusOK {
+		t.Fatalf("legit player lookup: status %d body %s", rec.Code, rec.Body.String())
+	}
+}
diff --git a/internal/admin/server.go b/internal/admin/server.go
index 4ea22d8..c13fe96 100644
--- a/internal/admin/server.go
+++ b/internal/admin/server.go
@@ -389,6 +389,12 @@ func (s *AdminServer) checkAuth(r *http.Request) bool {
 		return false
 	}
 	account := parts[0]
+	// The account name is attacker-controlled (it comes from the cookie) and
+	// is joined into players/accounts/<account>.yaml by isAdminAccount below,
+	// so reject path-like names before the first join.
+	if err := player.ValidName(account); err != nil {
+		return false
+	}
 	issued, err := strconv.ParseInt(parts[1], 10, 64)
 	if err != nil {
 		return false
diff --git a/internal/game/cmd_map.go b/internal/game/cmd_map.go
index fc4ebf1..e84d1ce 100644
--- a/internal/game/cmd_map.go
+++ b/internal/game/cmd_map.go
@@ -2,14 +2,22 @@ package game
 
 import (
 	"thehouseoficarus/internal/net"
+	"thehouseoficarus/internal/player"
 )
 
 func (g *Game) executeMap(sess *net.Session, args []string, rawInput string) {
 	g.doMap(sess)
 }
 
-func (g *Game) doMap(sess *net.Session) {
-	p := sess.Player
+// maxMapDimension bounds the grid a player may request. buildFullMap
+// allocates mapHeight x mapWidth cells, so an unbounded option value would let
+// any authenticated player trigger a fatal (unrecoverable) out-of-memory
+// allocation that takes the whole server down. The option parser accepts any
+// int and the value persists in the account file, so the clamp must live here.
+const maxMapDimension = 200
+
+// mapDimensions returns the clamped map grid size requested by the player.
+func mapDimensions(p *player.Player) (int, int) {
 	mapWidth := p.OptionInt("map_width")
 	mapHeight := p.OptionInt("map_height")
 	if mapWidth < 5 {
@@ -18,6 +26,18 @@ func (g *Game) doMap(sess *net.Session) {
 	if mapHeight < 5 {
 		mapHeight = 5
 	}
+	if mapWidth > maxMapDimension {
+		mapWidth = maxMapDimension
+	}
+	if mapHeight > maxMapDimension {
+		mapHeight = maxMapDimension
+	}
+	return mapWidth, mapHeight
+}
+
+func (g *Game) doMap(sess *net.Session) {
+	p := sess.Player
+	mapWidth, mapHeight := mapDimensions(p)
 
 	lines := buildFullMap(g, sess, p.RoomID, mapWidth, mapHeight, mapGlyphsForPlayer(p.OptionBool("unicode")))
 
diff --git a/internal/game/cmd_trigger_transport.go b/internal/game/cmd_trigger_transport.go
index f6c9c77..1aa3089 100644
--- a/internal/game/cmd_trigger_transport.go
+++ b/internal/game/cmd_trigger_transport.go
@@ -44,6 +44,14 @@ func (g *Game) executeTransportEffect(sess *net.Session, p *player.Player, mod *
 		}
 	}
 
+	// A transport changes rooms just like a walk or teleport, so it must
+	// clear any safespot cover: the safespot predicates are room-agnostic,
+	// so leaving the state active would grant melee/hazard immunity in the
+	// destination and keep the player as a phantom occupant of the old cover.
+	if ss, ok := g.safespot.Get(p.Name); ok {
+		g.forceLeaveSafespot(sess, p, &ss, "")
+	}
+
 	sess.WriteLine(fmt.Sprintf("You activate %s...", mod.Name))
 
 	p.RoomID = mod.Destination
@@ -53,6 +61,12 @@ func (g *Game) executeTransportEffect(sess *net.Session, p *player.Player, mod *
 		g.Hub.EnterRoom(sess, p.RoomID)
 	}
 
+	// Seed the destination exactly as the other teleport paths do, or the
+	// room's mobs, objects and ground spawns never appear.
+	g.World.SeedGroundItems(p.RoomID)
+	g.seedRoomMobs(p.RoomID)
+	g.seedRoomObjects(p.RoomID)
+
 	room, _ := g.World.LoadRoom(p.RoomID)
 	destName := fmt.Sprintf("room %d", p.RoomID)
 	if room != nil {
@@ -61,4 +75,6 @@ func (g *Game) executeTransportEffect(sess *net.Session, p *player.Player, mod *
 	sess.WriteLine(fmt.Sprintf("You arrive at %s.", destName))
 	g.saveCharacter(p)
 	g.doLook(sess)
+	g.runEnterSteps(sess, p.RoomID)
+	g.writePrompt(sess)
 }
diff --git a/internal/game/economy.go b/internal/game/economy.go
index 3505a86..9ce4404 100644
--- a/internal/game/economy.go
+++ b/internal/game/economy.go
@@ -885,6 +885,24 @@ func pruneOpenPayouts(j *economyJournal, p *player.Player) bool {
 //     written later proves the round resolved (a win/push/refund already
 //     returned the stake inside that payout) → settled with no refund.
 //     Anything else stays open for operator inspection.
+// reanchorOwedPayout rewrites an open payout record's checkpoints to the
+// current durable balance so a later recovery reads it as still owed (the
+// balance sits at the record's before-checkpoint). It is used when a recovery
+// run could not pay every sibling in an identical-signature group: the paid
+// siblings moved the durable balance, so leaving the unpaid remainder at its
+// original checkpoints would let a later run mistake the balance for its own
+// durability and settle the liability without paying it. The post-checkpoint
+// respects the economy cap: when the credit cannot fit, before == after, so
+// the record keeps reading as owed at this balance instead of claiming an
+// impossible post-state.
+func reanchorOwedPayout(rec *economyRecord, credits, credit int) {
+	rec.CreditsBefore = credits
+	rec.CreditsAfter = credits
+	if after, err := addCreditsChecked(credits, credit); err == nil {
+		rec.CreditsAfter = after
+	}
+}
+
 func (g *Game) recoverEconomy() {
 	if g.Economy == nil {
 		return
@@ -1099,35 +1117,29 @@ func (g *Game) recoverEconomy() {
 				settled[other.ID] = true
 				continue
 			}
-			if capBlocked {
-				// The credit cap blocked re-application. Re-anchor the
-				// record's checkpoints to the current durable balance so
-				// a later recovery re-applies it against a balance it can
-				// actually be paid from — without this, the balance a PAID
-				// sibling moved reads as this record's own durability and
-				// the liability is silently settled instead of paid.
-				// The re-anchored post-state must respect the cap: when it
-				// would exceed it, no reachable balance can pay the record,
-				// so the current balance is recorded as both checkpoints —
-				// the record then keeps reading as owed at this balance
-				// (retrying the re-application) instead of claiming an
-				// impossible post-state that a coincidental balance could
-				// be mistaken for as this record's own durability.
-				other.CreditsBefore = p.Credits
-				other.CreditsAfter = p.Credits
-				if after, err := addCreditsChecked(p.Credits, other.Credit); err == nil {
-					other.CreditsAfter = after
-				}
+			if capBlocked || paid > 0 {
+				// Either the credit cap blocked re-application, or an
+				// earlier sibling WAS paid before this record's save failed
+				// transiently. In both cases the durable balance already
+				// moved away from this record's original before-checkpoint,
+				// so re-anchor the checkpoints to the current durable balance
+				// — without this, the balance a PAID sibling moved reads as
+				// this record's own durability and the liability is silently
+				// settled instead of paid (and compaction then erases the
+				// settled sibling evidence, so a later run sees len(group)==1
+				// and settles the remainder as "already durable").
+				reanchorOwedPayout(other, p.Credits, other.Credit)
 				settled[other.ID] = true // handled for the rest of this run
 				_ = g.Economy.append(*other)
 				continue
 			}
-			// A TRANSIENT save failure blocked re-application and the
-			// balance still has room below the cap: keep the record open
-			// with its ORIGINAL checkpoints. Re-anchoring here would make
-			// the record unreadable against the unchanged durable balance
-			// (before-checkpoint mismatch) and permanently ambiguous; the
-			// original checkpoints let a later recovery retry cleanly.
+			// A TRANSIENT save failure blocked the FIRST re-application
+			// (paid == 0) and the balance still sits at the original
+			// before-checkpoint: keep the record open with its ORIGINAL
+			// checkpoints so a later recovery reads it as owed and retries
+			// cleanly. Re-anchoring here would make the record unreadable
+			// against the unchanged durable balance (before-checkpoint
+			// mismatch) and permanently ambiguous.
 			settled[other.ID] = true // handled for the rest of this run
 		}
 	}
diff --git a/internal/game/economy_reanchor_test.go b/internal/game/economy_reanchor_test.go
new file mode 100644
index 0000000..86d7bf1
--- /dev/null
+++ b/internal/game/economy_reanchor_test.go
@@ -0,0 +1,31 @@
+package game
+
+import (
+	"testing"
+
+	"thehouseoficarus/internal/casino"
+)
+
+// TestReanchorOwedPayoutReadsAsOwed: when recovery cannot pay every sibling in
+// an identical-signature payout group, the unpaid remainder must be re-anchored
+// to the current durable balance. Otherwise a later run (after compaction drops
+// the settled sibling evidence) reads the balance as the record's own
+// durability and settles the liability without paying it.
+func TestReanchorOwedPayoutReadsAsOwed(t *testing.T) {
+	rec := &economyRecord{Credit: 20, CreditsBefore: 90, CreditsAfter: 110}
+	// One sibling was paid: the durable balance now sits at 110.
+	reanchorOwedPayout(rec, 110, rec.Credit)
+	if rec.CreditsBefore != 110 || rec.CreditsAfter != 130 {
+		t.Fatalf("re-anchored checkpoints = before %d after %d, want before 110 after 130", rec.CreditsBefore, rec.CreditsAfter)
+	}
+
+	// At the economy cap the post-checkpoint must stay reachable: before ==
+	// after so the record keeps reading as owed instead of claiming an
+	// impossible balance that a coincidental balance could be mistaken for.
+	capped := &economyRecord{Credit: 20}
+	reanchorOwedPayout(capped, casino.MaxCredits, capped.Credit)
+	if capped.CreditsBefore != casino.MaxCredits || capped.CreditsAfter != casino.MaxCredits {
+		t.Fatalf("cap re-anchor = before %d after %d, want both %d",
+			capped.CreditsBefore, capped.CreditsAfter, casino.MaxCredits)
+	}
+}
diff --git a/internal/game/map_bounds_regression_test.go b/internal/game/map_bounds_regression_test.go
new file mode 100644
index 0000000..dd3423b
--- /dev/null
+++ b/internal/game/map_bounds_regression_test.go
@@ -0,0 +1,34 @@
+package game
+
+import (
+	"testing"
+
+	"thehouseoficarus/internal/player"
+)
+
+// TestMapDimensionsClamped: the player-settable map size drives a direct
+// mapHeight x mapWidth allocation in buildFullMap, so an unbounded option
+// value is a fatal out-of-memory kill reachable by any authenticated player
+// (and persisted in the account file, so it repeats on login). The clamp must
+// cap both dimensions while leaving ordinary values alone.
+func TestMapDimensionsClamped(t *testing.T) {
+	p := player.New("mapper")
+
+	p.Options = map[string]any{"map_width": 100000, "map_height": 100000}
+	w, h := mapDimensions(p)
+	if w != maxMapDimension || h != maxMapDimension {
+		t.Fatalf("huge map dimensions = (%d,%d), want (%d,%d)", w, h, maxMapDimension, maxMapDimension)
+	}
+
+	p.Options = map[string]any{"map_width": -5, "map_height": 0}
+	w, h = mapDimensions(p)
+	if w != 5 || h != 5 {
+		t.Fatalf("undersized map dimensions = (%d,%d), want (5,5)", w, h)
+	}
+
+	p.Options = map[string]any{"map_width": 30, "map_height": 20}
+	w, h = mapDimensions(p)
+	if w != 30 || h != 20 {
+		t.Fatalf("normal map dimensions = (%d,%d), want (30,20)", w, h)
+	}
+}
diff --git a/internal/game/transport_safespot_regression_test.go b/internal/game/transport_safespot_regression_test.go
new file mode 100644
index 0000000..fe77994
--- /dev/null
+++ b/internal/game/transport_safespot_regression_test.go
@@ -0,0 +1,34 @@
+package game
+
+import "testing"
+
+// TestTransportClearsSafespot: the safespot predicates are room-agnostic, so a
+// transport that changes rooms without clearing cover grants melee/hazard
+// immunity in the destination and leaves a phantom occupant in the old room.
+// Every other teleport path clears it; the transport effect must too.
+func TestTransportClearsSafespot(t *testing.T) {
+	h := newHarness(t)
+	h.seedAccount("alice")
+	h.seedCharacter("alice", "alice")
+	sess, _ := h.connect("alice")
+
+	h.g.RunOnOwner(func() {
+		p := sess.Player
+		h.g.safespot.Set(p.Name, SafespotState{Active: true, ObjectDefID: "rock", ObjectIndex: 0, RoomID: p.RoomID})
+		if !h.g.isSafespotted(p.Name) {
+			t.Fatal("safespot not active before transport")
+		}
+
+		h.g.executeTransportEffect(sess, p, &ModDef{ID: "test_transport", Name: "test shuttle", Destination: 2})
+
+		if h.g.safespot.Has(p.Name) {
+			t.Fatal("safespot state survived transport; hazard/melee immunity persists in the destination")
+		}
+		if h.g.isSafespotted(p.Name) {
+			t.Fatal("player still counts as safespotted after transport")
+		}
+		if p.RoomID != 2 {
+			t.Fatalf("room after transport = %d, want 2", p.RoomID)
+		}
+	})
+}
diff --git a/internal/player/store.go b/internal/player/store.go
index 4c8dce5..0b0e70e 100644
--- a/internal/player/store.go
+++ b/internal/player/store.go
@@ -66,7 +66,18 @@ func (s *AccountStore) AccountExists(name string) bool {
 	return err == nil
 }
 
+// isPathLikeName reports whether name is unsafe to join into a data-file path.
+// Callers are expected to validate names (player.ValidName) before calling the
+// store; this is a defence-in-depth backstop for the raw path primitive.
+func isPathLikeName(name string) bool {
+	return name == "" || name == "." || name == ".." ||
+		strings.ContainsAny(name, "/\\") || strings.ContainsRune(name, 0)
+}
+
 func (s *AccountStore) FindAccount(name string) (string, error) {
+	if isPathLikeName(name) {
+		return "", fmt.Errorf("account not found: %s", name)
+	}
 	path := s.AccountPath(name)
 	if _, err := os.Stat(path); err == nil {
 		return name, nil
@@ -176,6 +187,9 @@ func (s *AccountStore) CharacterExists(name string) bool {
 }
 
 func (s *AccountStore) FindCharacter(name string) (string, error) {
+	if isPathLikeName(name) {
+		return "", fmt.Errorf("character not found: %s", name)
+	}
 	path := s.CharPath(name)
 	if _, err := os.Stat(path); err == nil {
 		return name, nil
diff --git a/internal/world/rekey_objects_test.go b/internal/world/rekey_objects_test.go
new file mode 100644
index 0000000..7ee2de3
--- /dev/null
+++ b/internal/world/rekey_objects_test.go
@@ -0,0 +1,37 @@
+package world
+
+import "testing"
+
+// TestRekeyRoomsUpdatesObjStateRoomID: object lookups filter on ObjState.RoomID
+// while GetObjState looks up the map key, so the invariant
+// mapKey == ObjStateKey(st.RoomID, ...) must survive a rekey. Leaving RoomID
+// stale makes room objects vanish from a renamed/swapped room for the rest of
+// the process and lets TickObjWander create a duplicate entry for the state.
+func TestRekeyRoomsUpdatesObjStateRoomID(t *testing.T) {
+	w := New(t.TempDir())
+	w.objStates[w.ObjStateKey(1, "anvil", 0)] = &ObjState{
+		RoomID:      1,
+		DefID:       "anvil",
+		Index:       0,
+		WanderRooms: []int{1, 5, 9},
+	}
+
+	w.RekeyRooms(map[int]int{1: 3, 5: 7})
+
+	inst := w.GetObjState(3, "anvil", 0)
+	if inst == nil {
+		t.Fatal("object state lost after rekey")
+	}
+	if inst.RoomID != 3 {
+		t.Fatalf("ObjState.RoomID = %d, want 3 (map key and state desynchronised)", inst.RoomID)
+	}
+	if got := inst.WanderRooms; len(got) != 3 || got[0] != 3 || got[1] != 7 || got[2] != 9 {
+		t.Fatalf("WanderRooms after rekey = %v, want [3 7 9]", got)
+	}
+	if got := w.AllObjInstances(3); len(got) != 1 {
+		t.Fatalf("AllObjInstances(3) = %d entries, want 1 (object invisible in renamed room)", len(got))
+	}
+	if got := w.AllObjInstances(1); len(got) != 0 {
+		t.Fatalf("AllObjInstances(1) = %d entries, want 0 (stale old room)", len(got))
+	}
+}
diff --git a/internal/world/world.go b/internal/world/world.go
index 6a0313d..7475ce0 100644
--- a/internal/world/world.go
+++ b/internal/world/world.go
@@ -217,6 +217,7 @@ func (w *World) RekeyRooms(idMap map[int]int) {
 	type objKeyMove struct {
 		oldKey, newKey string
 		st             *ObjState
+		newID          int
 	}
 	var moves []objKeyMove
 	for k, st := range w.objStates {
@@ -233,12 +234,23 @@ func (w *World) RekeyRooms(idMap map[int]int) {
 			continue
 		}
 		newKey := fmt.Sprintf("%d:%s", newID, k[len(roomStr)+1:])
-		moves = append(moves, objKeyMove{k, newKey, st})
+		moves = append(moves, objKeyMove{k, newKey, st, newID})
 	}
 	for _, m := range moves {
 		delete(w.objStates, m.oldKey)
 	}
 	for _, m := range moves {
+		// Keep the package invariant mapKey == ObjStateKey(st.RoomID, ...):
+		// object lookups filter on st.RoomID while GetObjState looks up the
+		// key, so a rekey that leaves RoomID stale makes the object vanish
+		// from the renamed room (and TickObjWander then creates a second
+		// entry for the same state). Remap wander rooms like MobStore does.
+		m.st.RoomID = m.newID
+		for i, room := range m.st.WanderRooms {
+			if mapped, ok := idMap[room]; ok && mapped != room {
+				m.st.WanderRooms[i] = mapped
+			}
+		}
 		w.objStates[m.newKey] = m.st
 	}
 }