fix: harden engine across casino, economy, concurrency, security, and content

7 files changed, 383 insertions, 5 deletions

diff --git a/internal/player/correct_skill_validity_test.go b/internal/player/correct_skill_validity_test.go
new file mode 100644
index 0000000..941bfab
--- /dev/null
+++ b/internal/player/correct_skill_validity_test.go
@@ -0,0 +1,21 @@
+package player
+
+// Regression: craft recipes declare their skill as the recipe TYPE, and not
+// every type is a skill (a combine recipe declares "combine"). Callers must be
+// able to tell a real skill name from an action name, because Level reads an
+// unknown name as a map miss (level 1) and AddSkillXP persists it.
+
+import "testing"
+
+func TestValidSkillRecognizesDeclaredSkillsOnly(t *testing.T) {
+	for _, s := range AllSkills {
+		if !ValidSkill(s) {
+			t.Errorf("ValidSkill(%q) = false, want true", s)
+		}
+	}
+	for _, name := range []SkillName{"combine", "fletch", "craft", "", "not_a_skill"} {
+		if ValidSkill(name) {
+			t.Errorf("ValidSkill(%q) = true, want false", name)
+		}
+	}
+}
diff --git a/internal/player/hardening_max_inventory_slots_test.go b/internal/player/hardening_max_inventory_slots_test.go
new file mode 100644
index 0000000..8cd9c37
--- /dev/null
+++ b/internal/player/hardening_max_inventory_slots_test.go
@@ -0,0 +1,86 @@
+package player
+
+// Regression coverage for the inventory slot accounting helpers: the capacity
+// is a named constant, FirstFreeSlot stays inside it, and FreeSlots ignores
+// entries a corrupt save can carry that no gameplay path can free.
+
+import "testing"
+
+func TestMaxInventorySlotsMatchesSlotHelpers(t *testing.T) {
+	if MaxInventorySlots != 28 {
+		t.Fatalf("MaxInventorySlots = %d, want 28 (on-disk capacity)", MaxInventorySlots)
+	}
+	p := New("alice")
+	if got := p.FreeSlots(); got != MaxInventorySlots {
+		t.Fatalf("empty inventory free slots = %d, want %d", got, MaxInventorySlots)
+	}
+	if got := p.FirstFreeSlot(); got != 0 {
+		t.Fatalf("empty inventory first free slot = %d, want 0", got)
+	}
+
+	// The last valid slot is addressable, and the first free slot walks up to
+	// the capacity and only then fails.
+	for i := 0; i < MaxInventorySlots-1; i++ {
+		p.SetInvSlot(i, &InventorySlot{ItemID: "rock", Quantity: 1})
+	}
+	if got := p.FirstFreeSlot(); got != MaxInventorySlots-1 {
+		t.Fatalf("first free slot = %d, want %d", got, MaxInventorySlots-1)
+	}
+	if got := p.FreeSlots(); got != 1 {
+		t.Fatalf("free slots = %d, want 1", got)
+	}
+	p.SetInvSlot(MaxInventorySlots-1, &InventorySlot{ItemID: "rock", Quantity: 1})
+	if got := p.FirstFreeSlot(); got != -1 {
+		t.Fatalf("full inventory first free slot = %d, want -1", got)
+	}
+	if got := p.FreeSlots(); got != 0 {
+		t.Fatalf("full inventory free slots = %d, want 0", got)
+	}
+}
+
+// TestFreeSlotsIgnoresInvalidEntries: out-of-range keys (a negative one, and
+// one past the capacity) and nil-valued entries must not consume capacity or
+// drive the count negative, while the valid slots keep counting exactly as
+// before.
+func TestFreeSlotsIgnoresInvalidEntries(t *testing.T) {
+	p := New("alice")
+	p.Inventory = map[int]*InventorySlot{
+		-1:                    {ItemID: "rock", Quantity: 1},
+		MaxInventorySlots:     {ItemID: "rock", Quantity: 1},
+		MaxInventorySlots + 5: {ItemID: "rock", Quantity: 1},
+		3:                     nil, // an explicitly nil entry frees its slot
+		7:                     {ItemID: "rock", Quantity: 1},
+	}
+
+	if got := p.FreeSlots(); got != MaxInventorySlots-1 {
+		t.Fatalf("free slots with invalid entries = %d, want %d", got, MaxInventorySlots-1)
+	}
+	if got := p.FirstFreeSlot(); got != 0 {
+		t.Fatalf("first free slot = %d, want 0", got)
+	}
+
+	// Only valid, non-nil slots are occupied: with every in-range slot filled
+	// the invalid keys must not make the count negative.
+	p.Inventory = make(map[int]*InventorySlot, MaxInventorySlots+2)
+	for i := 0; i < MaxInventorySlots; i++ {
+		p.Inventory[i] = &InventorySlot{ItemID: "rock", Quantity: 1}
+	}
+	p.Inventory[-7] = &InventorySlot{ItemID: "rock", Quantity: 1}
+	p.Inventory[99] = &InventorySlot{ItemID: "rock", Quantity: 1}
+	if got := p.FreeSlots(); got != 0 {
+		t.Fatalf("free slots of a full inventory with stray keys = %d, want 0", got)
+	}
+	if got := p.FirstFreeSlot(); got != -1 {
+		t.Fatalf("first free slot of a full inventory with stray keys = %d, want -1", got)
+	}
+}
+
+func TestFreeSlotsCountsValidSlotsLikeLength(t *testing.T) {
+	p := New("alice")
+	for _, i := range []int{0, 5, 27} {
+		p.SetInvSlot(i, &InventorySlot{ItemID: "rock", Quantity: 1})
+	}
+	if got, want := p.FreeSlots(), MaxInventorySlots-3; got != want {
+		t.Fatalf("free slots = %d, want %d", got, want)
+	}
+}
diff --git a/internal/player/hardening_password_params_test.go b/internal/player/hardening_password_params_test.go
new file mode 100644
index 0000000..4c171f1
--- /dev/null
+++ b/internal/player/hardening_password_params_test.go
@@ -0,0 +1,62 @@
+package player
+
+// Regression: Argon2id parameters parsed from a stored hash must be bounded
+// before they reach argon2.IDKey. A hand-edited or corrupted account file
+// (e.g. argon2id:1:4294967295:...) must fail the check, not request a
+// multi-TiB allocation that kills the process.
+
+import (
+	"crypto/sha256"
+	"encoding/base64"
+	"encoding/hex"
+	"testing"
+)
+
+func TestCheckPasswordRejectsOutOfRangeArgonParams(t *testing.T) {
+	// Enormous memory cost (0xFFFFFFFF KiB) with an otherwise well-formed
+	// hash: must be rejected before hashing.
+	huge := "argon2id:1:4294967295:2:AAAAAAAAAAAAAAAAAAAAAA:0011223344556677889900112233445566778899001122334455667788990011"
+	if CheckPassword("pw", huge) {
+		t.Fatal("out-of-range Argon2 memory cost was accepted")
+	}
+
+	// Negative values would wrap through uint32 under a naive conversion.
+	negative := "argon2id:-1:-1:-1:AAAAAAAAAAAAAAAAAAAAAA:0011223344556677889900112233445566778899001122334455667788990011"
+	if CheckPassword("pw", negative) {
+		t.Fatal("negative Argon2 parameters were accepted")
+	}
+
+	// Absurd time cost is bounded too.
+	slow := "argon2id:4294967295:65536:2:AAAAAAAAAAAAAAAAAAAAAA:0011223344556677889900112233445566778899001122334455667788990011"
+	if CheckPassword("pw", slow) {
+		t.Fatal("out-of-range Argon2 time cost was accepted")
+	}
+}
+
+func TestCheckPasswordAcceptsNormalAndLegacyHashes(t *testing.T) {
+	hashed, err := HashPassword("s3cret")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !CheckPassword("s3cret", hashed) {
+		t.Fatal("valid Argon2id hash rejected")
+	}
+	if CheckPassword("wrong", hashed) {
+		t.Fatal("wrong password accepted")
+	}
+
+	// Legacy single-pass sha256 hashes must keep verifying so old accounts
+	// can still log in and be upgraded.
+	salt := []byte("sixteen-byte-slt")
+	sum := sha256.Sum256(append(append([]byte{}, salt...), []byte("pw")...))
+	legacy := "sha256:" + base64.RawStdEncoding.EncodeToString(salt) + ":" + hex.EncodeToString(sum[:])
+	if !CheckPassword("pw", legacy) {
+		t.Fatal("legacy sha256 hash rejected")
+	}
+	if CheckPassword("nope", legacy) {
+		t.Fatal("wrong password accepted against legacy sha256 hash")
+	}
+	if !NeedsRehash(legacy) {
+		t.Fatal("legacy sha256 hash not flagged for rehash")
+	}
+}
diff --git a/internal/player/hardening_store_sanitize_test.go b/internal/player/hardening_store_sanitize_test.go
new file mode 100644
index 0000000..9416856
--- /dev/null
+++ b/internal/player/hardening_store_sanitize_test.go
@@ -0,0 +1,114 @@
+package player
+
+// Regression: LoadCharacter must sanitize the inventory/bank slot maps. A
+// hand-edited or corrupt save with out-of-range keys, nil entries, or
+// non-positive quantities used to leave items no gameplay loop could reach
+// while still consuming capacity, and could drive FreeSlots negative.
+
+import (
+	"os"
+	"testing"
+)
+
+func TestLoadCharacterSanitizesSlotMaps(t *testing.T) {
+	s := testAccountStore(t)
+	content := `name: Corrupt
+room_id: 1
+inventory:
+  0:
+    item_id: rope
+    quantity: 2
+  3: null
+  5:
+    item_id: ghost
+    quantity: 0
+  7:
+    item_id: negative
+    quantity: -4
+  99:
+    item_id: out_of_range
+    quantity: 1
+bank:
+  0:
+    item_id: coins
+    quantity: 100
+  30:
+    item_id: far_slot
+    quantity: 1
+  -2:
+    item_id: bogus
+    quantity: 3
+  4: null
+  6:
+    item_id: empty
+    quantity: 0
+`
+	if err := os.WriteFile(s.CharPath("Corrupt"), []byte(content), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	p, err := s.LoadCharacter("Corrupt")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(p.Inventory) != 1 {
+		t.Fatalf("inventory = %v, want only the valid slot 0", p.Inventory)
+	}
+	slot := p.Inventory[0]
+	if slot == nil || slot.ItemID != "rope" || slot.Quantity != 2 {
+		t.Errorf("valid slot 0 changed: %+v", slot)
+	}
+	if got := p.FreeSlots(); got != 27 {
+		t.Errorf("FreeSlots() = %d, want 27", got)
+	}
+	if got := p.FreeSlots(); got < 0 {
+		t.Errorf("FreeSlots() went negative: %d", got)
+	}
+
+	if len(p.Bank) != 2 {
+		t.Fatalf("bank = %v, want only slots 0 and 30", p.Bank)
+	}
+	if bs := p.Bank[0]; bs == nil || bs.ItemID != "coins" || bs.Quantity != 100 {
+		t.Errorf("valid bank slot 0 changed: %+v", bs)
+	}
+	// Bank slots past the inventory's 28 are valid: the bank grows unbounded.
+	if bs := p.Bank[30]; bs == nil || bs.ItemID != "far_slot" {
+		t.Errorf("valid bank slot 30 dropped: %+v", bs)
+	}
+	if _, ok := p.Bank[-2]; ok {
+		t.Error("negative bank slot survived sanitization")
+	}
+}
+
+func TestSaveLoadKeepsValidSlots(t *testing.T) {
+	s := testAccountStore(t)
+	p := New("Alice")
+	p.SetInvSlot(0, &InventorySlot{ItemID: "rope", Quantity: 2, Quality: 3, MaxQuality: 5})
+	p.SetInvSlot(27, &InventorySlot{ItemID: "torch", Quantity: 1})
+	p.SetBankSlot(0, &InventorySlot{ItemID: "coins", Quantity: 100})
+	p.SetBankSlot(30, &InventorySlot{ItemID: "spade", Quantity: 1})
+	if err := s.SaveCharacter(p); err != nil {
+		t.Fatal(err)
+	}
+
+	loaded, err := s.LoadCharacter("Alice")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded.Inventory) != 2 || len(loaded.Bank) != 2 {
+		t.Fatalf("slots lost on a round trip: inventory=%v bank=%v", loaded.Inventory, loaded.Bank)
+	}
+	if got := loaded.Inventory[0]; got == nil || got.ItemID != "rope" || got.Quantity != 2 || got.Quality != 3 || got.MaxQuality != 5 {
+		t.Errorf("inventory slot 0 changed: %+v", got)
+	}
+	if got := loaded.Inventory[27]; got == nil || got.ItemID != "torch" {
+		t.Errorf("inventory slot 27 changed: %+v", got)
+	}
+	if got := loaded.Bank[30]; got == nil || got.ItemID != "spade" {
+		t.Errorf("bank slot 30 changed: %+v", got)
+	}
+	if loaded.FreeSlots() != 26 {
+		t.Errorf("FreeSlots() = %d, want 26", loaded.FreeSlots())
+	}
+}
diff --git a/internal/player/password.go b/internal/player/password.go
index 9be39fb..5a43e1b 100644
--- a/internal/player/password.go
+++ b/internal/player/password.go
@@ -31,6 +31,23 @@ const (
 	argonSaltLen = 16
 )
 
+// Sanity bounds applied to Argon2id parameters parsed from a stored hash.
+// HashPassword always writes the constants above, but a hand-edited or
+// corrupted account file can carry arbitrary numbers; without these bounds a
+// value like 4294967295 (or a negative that wraps through uint32) makes
+// argon2.IDKey request a multi-TiB allocation and kills the process instead of
+// failing the login. Anything outside these ranges is rejected before hashing.
+const (
+	argonMinTime    = 1
+	argonMaxTime    = 8
+	argonMinMemory  = 8 * 1024   // KiB (8 MiB)
+	argonMaxMemory  = 256 * 1024 // KiB (256 MiB)
+	argonMinThreads = 1
+	argonMaxThreads = 16
+	argonMinKeyLen  = 16
+	argonMaxKeyLen  = 64
+)
+
 // hashLimiter bounds concurrent Argon2id computations. Each hash costs
 // ~64 MiB and tens of milliseconds, so an unbounded burst of logins (or
 // login attempts) would spike memory and CPU in parallel. Password work
@@ -81,6 +98,14 @@ func CheckPassword(password, stored string) bool {
 		if errT != nil || errM != nil || errP != nil || errSalt != nil || errKey != nil {
 			return false
 		}
+		if t < argonMinTime || t > argonMaxTime ||
+			m < argonMinMemory || m > argonMaxMemory ||
+			p < argonMinThreads || p > argonMaxThreads ||
+			len(expected) < argonMinKeyLen || len(expected) > argonMaxKeyLen {
+			// Out-of-range parameters: fail the check instead of trusting the
+			// stored values and requesting an enormous allocation.
+			return false
+		}
 		acquireHashSlot()
 		key := argon2.IDKey([]byte(password), salt, uint32(t), uint32(m), uint8(p), uint32(len(expected)))
 		releaseHashSlot()
diff --git a/internal/player/player.go b/internal/player/player.go
index 0a48b04..f8d9a68 100644
--- a/internal/player/player.go
+++ b/internal/player/player.go
@@ -43,6 +43,21 @@ var AllSkills = []SkillName{
 	Pharmacy, Thieving, Agility, Construction, Scavenging, Hacking, Assassin, Farming,
 }
 
+// ValidSkill reports whether name is a skill this game defines. A craft
+// recipe's declared skill is item.CraftDef.EffectiveSkill, which is the recipe
+// TYPE, and not every type is a skill: a combine recipe carries type "combine".
+// Level reads an undefined name as a map miss (level 1) and AddSkillXP would
+// persist a phantom entry under it, so callers that roll success or award XP
+// against a recipe type must check with this first.
+func ValidSkill(name SkillName) bool {
+	for _, s := range AllSkills {
+		if s == name {
+			return true
+		}
+	}
+	return false
+}
+
 var SkillAbbr = map[SkillName]string{
 	Accuracy:     "acc",
 	Strength:     "str",
@@ -353,8 +368,13 @@ func (p *Player) SetInvSlot(i int, slot *InventorySlot) {
 	}
 }
 
+// MaxInventorySlots is the inventory capacity: the valid slots are
+// 0..MaxInventorySlots-1. characterInventorySlots in store.go mirrors this
+// value for on-disk sanitization.
+const MaxInventorySlots = 28
+
 func (p *Player) FirstFreeSlot() int {
-	for i := 0; i < 28; i++ {
+	for i := 0; i < MaxInventorySlots; i++ {
 		if p.Inventory[i] == nil {
 			return i
 		}
@@ -362,9 +382,19 @@ func (p *Player) FirstFreeSlot() int {
 	return -1
 }
 
+// FreeSlots counts the empty inventory slots. Only valid slots (0..27) with a
+// non-nil entry count as used: a corrupt, hand-edited, or partially-migrated
+// save can carry out-of-range or nil-valued map entries that no gameplay path
+// can free, and counting those would understate the free space (or drive the
+// count negative). For a valid character the result is unchanged.
 func (p *Player) FreeSlots() int {
-	used := len(p.Inventory)
-	return 28 - used
+	used := 0
+	for i := 0; i < MaxInventorySlots; i++ {
+		if p.Inventory[i] != nil {
+			used++
+		}
+	}
+	return MaxInventorySlots - used
 }
 
 func New(name string) *Player {
diff --git a/internal/player/store.go b/internal/player/store.go
index d461485..4c8dce5 100644
--- a/internal/player/store.go
+++ b/internal/player/store.go
@@ -41,7 +41,18 @@ func (s *AccountStore) SaveAccount(acc *Account) error {
 	if err != nil {
 		return err
 	}
-	return persistence.WriteFile(s.AccountPath(acc.Name), data, 0600)
+	return writePrivate(s.AccountPath(acc.Name), data)
+}
+
+// writePrivate writes data atomically and then forces 0600. persistence.WriteFile
+// deliberately preserves an existing file's mode, so an account/character file
+// created before this rule (or by an external tool) would otherwise stay
+// world-readable through every later save and password rehash.
+func writePrivate(path string, data []byte) error {
+	if err := persistence.WriteFile(path, data, 0600); err != nil {
+		return err
+	}
+	return os.Chmod(path, 0600)
 }
 
 // MarshalAccount serializes an account; callers that own the account (the
@@ -78,6 +89,10 @@ func (s *AccountStore) FindAccount(name string) (string, error) {
 	return "", fmt.Errorf("account not found: %s", name)
 }
 
+// characterInventorySlots is the on-disk inventory capacity, matching
+// Player.FreeSlots and Player.FirstFreeSlot in player.go.
+const characterInventorySlots = MaxInventorySlots
+
 func (s *AccountStore) CharPath(name string) string {
 	return filepath.Join(s.dataDir, "players", "characters", name+".yaml")
 }
@@ -101,18 +116,43 @@ func (s *AccountStore) LoadCharacter(name string) (*Player, error) {
 	if p.Inventory == nil {
 		p.Inventory = make(map[int]*InventorySlot)
 	}
+	sanitizeInventory(p.Inventory)
+	sanitizeBank(p.Bank)
 	if p.MapSymbols == nil {
 		p.MapSymbols = make(map[int]MapSymbolData)
 	}
 	return &p, nil
 }
 
+// sanitizeInventory drops inventory entries a hand-edited or corrupt save can
+// carry that no gameplay path can reach or free: nil slots, keys outside the
+// 0..characterInventorySlots-1 range, and non-positive quantities. FreeSlots
+// already ignores invalid entries, but FirstFreeSlot and the on-disk save still
+// need the map itself to be clean, so sanitize here as well.
+func sanitizeInventory(slots map[int]*InventorySlot) {
+	for i, slot := range slots {
+		if slot == nil || slot.Quantity <= 0 || i < 0 || i >= characterInventorySlots {
+			delete(slots, i)
+		}
+	}
+}
+
+// sanitizeBank applies the same rule to the bank, which unlike the inventory
+// grows past 28 slots (NextBankSlot), so only negative keys are out of range.
+func sanitizeBank(slots map[int]*InventorySlot) {
+	for i, slot := range slots {
+		if slot == nil || slot.Quantity <= 0 || i < 0 {
+			delete(slots, i)
+		}
+	}
+}
+
 func (s *AccountStore) SaveCharacter(p *Player) error {
 	data, err := s.MarshalCharacter(p)
 	if err != nil {
 		return err
 	}
-	return persistence.WriteFile(s.CharPath(p.Name), data, 0600)
+	return writePrivate(s.CharPath(p.Name), data)
 }
 
 // MarshalCharacter serializes a character for a same-directory temporary