From 67e01aab85607b9e601185c7d62dff576af3bb43 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Fri, 18 Sep 2026 15:42:23 -0400 Subject: fix: close map OOM, admin traversal, transport safespot, room rekey, and legacy payout recovery --- internal/admin/api_players.go | 18 ++++- internal/admin/api_players_path_test.go | 87 ++++++++++++++++++++++ internal/admin/server.go | 6 ++ internal/game/cmd_map.go | 24 +++++- internal/game/cmd_trigger_transport.go | 16 ++++ internal/game/economy.go | 62 ++++++++------- internal/game/economy_reanchor_test.go | 31 ++++++++ internal/game/map_bounds_regression_test.go | 34 +++++++++ .../game/transport_safespot_regression_test.go | 34 +++++++++ internal/player/store.go | 14 ++++ internal/world/rekey_objects_test.go | 37 +++++++++ internal/world/world.go | 14 +++- 12 files changed, 348 insertions(+), 29 deletions(-) create mode 100644 internal/admin/api_players_path_test.go create mode 100644 internal/game/economy_reanchor_test.go create mode 100644 internal/game/map_bounds_regression_test.go create mode 100644 internal/game/transport_safespot_regression_test.go create mode 100644 internal/world/rekey_objects_test.go (limited to 'internal') 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/.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/.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/.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 } } -- cgit v1.2.3