From 8fe9e3c3fef0049df82281a62afe35dda516dfe2 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Fri, 18 Sep 2026 17:48:26 -0400 Subject: fix: close combat ghost, admin traversal, login throttle, journal double-pay, and quality/economy gaps --- internal/admin/api_entity_path_test.go | 49 ++++++++++++++++++++ internal/admin/api_modules.go | 4 ++ internal/admin/api_rooms.go | 70 +++++++++++++++++++++-------- internal/admin/api_rooms_bulk_delete.go | 3 ++ internal/admin/api_techs.go | 4 ++ internal/admin/api_triggers.go | 6 +++ internal/admin/server.go | 79 ++++++++++++++++++++++++++++++--- internal/admin/undo.go | 41 +++++++++++++++++ 8 files changed, 232 insertions(+), 24 deletions(-) (limited to 'internal/admin') diff --git a/internal/admin/api_entity_path_test.go b/internal/admin/api_entity_path_test.go index 27ecbe8..75fed7a 100644 --- a/internal/admin/api_entity_path_test.go +++ b/internal/admin/api_entity_path_test.go @@ -62,6 +62,55 @@ func TestEntityAPICannotEscapeData(t *testing.T) { } } +// TestTechModuleCreateRejectsTraversalID: POST /api/techs and /api/modules +// once joined an unvalidated id into a filesystem path, allowing an +// arbitrary write outside the data directory (and a destructive undo). +func TestTechModuleCreateRejectsTraversalID(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "data") + for _, sub := range []string{"techs", "modules", "players/accounts"} { + 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}); 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] + + post := func(path, body string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) + req.AddCookie(cookie) + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + return rec + } + + for _, tc := range []struct{ path, body string }{ + {"/api/techs", `{"id":"../../outside_tech","name":"x"}`}, + {"/api/modules", `{"id":"../../outside_mod","name":"x"}`}, + } { + if rec := post(tc.path, tc.body); !strings.Contains(rec.Body.String(), "invalid id") { + t.Fatalf("%s traversal id not rejected: status %d body %s", tc.path, rec.Code, rec.Body.String()) + } + } + if _, err := os.Stat(filepath.Join(root, "outside_tech.yaml")); !os.IsNotExist(err) { + t.Fatal("tech create escaped the data directory") + } + if _, err := os.Stat(filepath.Join(root, "outside_mod.yaml")); !os.IsNotExist(err) { + t.Fatal("module create escaped the data directory") + } +} + // TestEntityRenameRejectsDuplicateIDInOtherDirectory: entity IDs are global // across the type's subdirectory tree (the runtime path indexes resolve by // filename anywhere under data/), so renaming into an ID that diff --git a/internal/admin/api_modules.go b/internal/admin/api_modules.go index 1d8c25b..8356b2d 100644 --- a/internal/admin/api_modules.go +++ b/internal/admin/api_modules.go @@ -30,6 +30,10 @@ func (s *AdminServer) handleModules(w http.ResponseWriter, r *http.Request) { failJSON(w, "missing id") return } + if !validEntityID(id) { + failJSON(w, "invalid id") + return + } delete(m, "id") path := filepath.Join(s.dataDir, "modules", id+".yaml") if _, err := os.Stat(path); err == nil { diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go index d84449b..7f317cc 100644 --- a/internal/admin/api_rooms.go +++ b/internal/admin/api_rooms.go @@ -343,6 +343,11 @@ func (s *AdminServer) handleRoomByID(w http.ResponseWriter, r *http.Request) { s.world.RebuildRoomIndex(s.dataDir) s.world.ClearRoomState(id) + // Drop the room's live mob instances too, or they keep wandering and + // are returned by MobsInRoom for a room that no longer exists. + if s.mobStore != nil { + s.mobStore.RemoveMobsInRoom(id) + } // Drop any cached in-room games (tables/machines) and refund their // outstanding wagers — same eviction semantics as bulk delete. if s.evictRoom != nil { @@ -490,6 +495,11 @@ func (s *AdminServer) handleRenameRoomDir(w http.ResponseWriter, r *http.Request failJSON(w, "rename failed: "+err.Error()) return } + // Undo history stores absolute paths: re-prefix them so a later undo + // does not recreate the old directory and write a duplicate room file. + if s.undoStack != nil { + s.undoStack.RewritePaths(oldPath, newPath) + } s.world.RebuildRoomIndex(s.dataDir) writeJSON(w, map[string]any{"ok": true, "new_dir": newRel}) } @@ -674,39 +684,63 @@ func (s *AdminServer) handleRoomLink(w http.ResponseWriter, r *http.Request) { writeJSONError(w, "room path not found", http.StatusInternalServerError) return } - - oldA, _ := snapshotFile(pathA) - newA, err := writeYAMLFile(pathA, roomA) + newA, err := marshalYAML(roomA) if err != nil { - writeJSONError(w, "write A: "+err.Error(), http.StatusInternalServerError) + writeJSONError(w, "serialize A: "+err.Error(), http.StatusInternalServerError) return } - s.undoStack.Push(ChangeDesc{ - Description: fmt.Sprintf("link %d %s %d", body.From, dir, body.To), - FilePath: pathA, - OldContent: oldA, - NewContent: newA, - }) + oldA, _ := snapshotFile(pathA) + pathB := "" + var oldB, newB []byte if !body.OneWay { - pathB, okB2 := s.world.GetRoomPath(body.To) + var okB2 bool + pathB, okB2 = s.world.GetRoomPath(body.To) if !okB2 { writeJSONError(w, "room B path not found", http.StatusInternalServerError) return } - oldB, _ := snapshotFile(pathB) - newB, err := writeYAMLFile(pathB, roomB) + newB, err = marshalYAML(roomB) if err != nil { + writeJSONError(w, "serialize B: "+err.Error(), http.StatusInternalServerError) + return + } + oldB, _ = snapshotFile(pathB) + } + + // Both sides are resolved and serialized before any write, and both + // are applied in one journaled transaction so a failure cannot leave + // an asymmetric link, and one Undo reverses the whole action. + txn := persistence.NewTransaction(filepath.Join(s.dataDir, ".journal")) + if err := txn.Replace(pathA, newA, 0644); err != nil { + writeJSONError(w, "write A: "+err.Error(), http.StatusInternalServerError) + return + } + if !body.OneWay { + if err := txn.Replace(pathB, newB, 0644); err != nil { writeJSONError(w, "write B: "+err.Error(), http.StatusInternalServerError) return } - s.undoStack.Push(ChangeDesc{ - Description: fmt.Sprintf("link %d %s %d (reverse)", body.To, oppDir, body.From), - FilePath: pathB, - OldContent: oldB, - NewContent: newB, + } + if err := txn.Commit(); err != nil { + writeJSONError(w, "commit link: "+err.Error(), http.StatusInternalServerError) + return + } + + desc := ChangeDesc{ + Description: fmt.Sprintf("link %d %s %d", body.From, dir, body.To), + FilePath: pathA, + OldContent: oldA, + NewContent: newA, + } + if !body.OneWay { + desc.ExtraFiles = append(desc.ExtraFiles, ExtraFile{ + FilePath: pathB, + OldContent: oldB, + NewContent: newB, }) } + s.undoStack.Push(desc) writeJSON(w, map[string]any{"ok": true}) case http.MethodDelete: diff --git a/internal/admin/api_rooms_bulk_delete.go b/internal/admin/api_rooms_bulk_delete.go index 6017eed..bba1879 100644 --- a/internal/admin/api_rooms_bulk_delete.go +++ b/internal/admin/api_rooms_bulk_delete.go @@ -100,6 +100,9 @@ func (s *AdminServer) handleBulkDelete(w http.ResponseWriter, r *http.Request) { s.world.RebuildRoomIndex(s.dataDir) for _, snap := range snapshots { s.world.ClearRoomState(snap.id) + if s.mobStore != nil { + s.mobStore.RemoveMobsInRoom(snap.id) + } if s.evictRoom != nil { s.evictRoom(snap.id) } diff --git a/internal/admin/api_techs.go b/internal/admin/api_techs.go index 91eb929..9e742e1 100644 --- a/internal/admin/api_techs.go +++ b/internal/admin/api_techs.go @@ -38,6 +38,10 @@ func (s *AdminServer) handleTechs(w http.ResponseWriter, r *http.Request) { failJSON(w, "missing id") return } + if !validEntityID(id) { + failJSON(w, "invalid id") + return + } delete(m, "id") if _, _, err := s.findTechFile(id); err == nil { failJSON(w, "tech already exists") diff --git a/internal/admin/api_triggers.go b/internal/admin/api_triggers.go index 08e2855..299ca10 100644 --- a/internal/admin/api_triggers.go +++ b/internal/admin/api_triggers.go @@ -152,6 +152,9 @@ func (s *AdminServer) handleTriggerByID(w http.ResponseWriter, r *http.Request) FilePath: newPath, OldContent: oldContent, NewContent: triggerData, + // A PUT to a missing trigger creates it; undo must delete the + // file rather than overwrite it with the nil before-image. + IsCreate: findErr != nil, } if findErr == nil && newSubdir != oldDir { // A kind change moves the file between subdirectories. Record the @@ -184,6 +187,9 @@ func (s *AdminServer) handleTriggerByID(w http.ResponseWriter, r *http.Request) Description: "Delete trigger " + id, FilePath: path, OldContent: oldContent, + // Redo of a delete must delete again, not write the nil + // new-content image over the file. + IsDelete: true, }) s.rebuildTriggerIndex() writeJSON(w, map[string]any{"ok": true}) diff --git a/internal/admin/server.go b/internal/admin/server.go index 98400c2..05586ef 100644 --- a/internal/admin/server.go +++ b/internal/admin/server.go @@ -473,6 +473,7 @@ type loginLimiter struct { type loginAttempts struct { failures int + inflight int blockedTill time.Time lastSeen time.Time } @@ -481,6 +482,66 @@ func newLoginLimiter() *loginLimiter { return &loginLimiter{entries: make(map[string]loginAttempts), now: time.Now} } +// begin atomically checks the cooldown and reserves an in-flight slot for +// ip. Reserving at admission time closes the TOCTOU where a concurrent burst +// all passes allow() before any of them records a failure: the number of +// simultaneous verifications for one address is bounded by the same +// threshold. Every successful begin must be paired with finish. +func (l *loginLimiter) begin(ip string) bool { + l.mu.Lock() + defer l.mu.Unlock() + now := l.now() + e, ok := l.entries[ip] + if !ok { + if !l.makeRoomLocked(now) { + // The tracked map is full of cooling-down addresses; do not + // reserve (preserves the existing bounded-map behavior). + return true + } + e = loginAttempts{} + } + if now.Before(e.blockedTill) { + return false + } + if e.failures+e.inflight >= adminLoginMaxFailures { + return false + } + e.inflight++ + e.lastSeen = now + l.entries[ip] = e + return true +} + +// finish releases the in-flight slot reserved by begin and records the +// outcome: success clears the address, failure advances the cooldown. +func (l *loginLimiter) finish(ip string, ok bool) { + l.mu.Lock() + defer l.mu.Unlock() + now := l.now() + e, tracked := l.entries[ip] + if !tracked { + return + } + if e.inflight > 0 { + e.inflight-- + } + e.lastSeen = now + if ok { + delete(l.entries, ip) + return + } + if now.Before(e.blockedTill) { + l.entries[ip] = e + return + } + e.failures++ + if e.failures >= adminLoginMaxFailures { + e.blockedTill = now.Add(adminLoginCooldown) + e.failures = 0 + } + l.entries[ip] = e +} + // allow reports whether ip may attempt a login now. func (l *loginLimiter) allow(ip string) bool { l.mu.Lock() @@ -540,7 +601,7 @@ func (l *loginLimiter) makeRoomLocked(now time.Time) bool { return true } for ip, e := range l.entries { - if e.failures == 0 && !now.Before(e.blockedTill) { + if e.failures == 0 && e.inflight == 0 && !now.Before(e.blockedTill) { delete(l.entries, ip) } } @@ -616,10 +677,12 @@ func (s *AdminServer) handleLogin(w http.ResponseWriter, r *http.Request) { // to make password guessing expensive, so it must not cost a parse. limiter := s.limitLogins() ip := clientIP(r.RemoteAddr) - if !limiter.allow(ip) { + if !limiter.begin(ip) { http.Error(w, "too many failed login attempts; try again later", http.StatusTooManyRequests) return } + loginOK := false + defer func() { limiter.finish(ip, loginOK) }() // /login is registered before authMiddleware, so bound and buffer its body // here before r.FormValue can parse it: without the cap a multipart POST @@ -646,20 +709,17 @@ func (s *AdminServer) handleLogin(w http.ResponseWriter, r *http.Request) { // primitive ("account=../../rooms/1"). Every other account-name entry // point validates first. if err := player.ValidName(account); err != nil { - limiter.fail(ip) s.renderPage(w, r, "login.html", map[string]any{"Error": "Invalid account name or password."}) return } acc, err := s.accountStore.LoadAccount(account) if err != nil || !player.CheckPassword(password, acc.PasswordHash) { - limiter.fail(ip) s.renderPage(w, r, "login.html", map[string]any{"Error": "Invalid account name or password."}) return } if !acc.Admin { - limiter.fail(ip) s.renderPage(w, r, "login.html", map[string]any{"Error": "Account is not authorized as admin."}) return } @@ -693,7 +753,7 @@ func (s *AdminServer) handleLogin(w http.ResponseWriter, r *http.Request) { return } s.setAuthCookie(w, account) - limiter.succeed(ip) + loginOK = true http.Redirect(w, r, "/", http.StatusFound) } @@ -898,6 +958,13 @@ func (s *AdminServer) handleEntityDirs(entityType string) http.HandlerFunc { failJSON(w, err.Error()) return } + // Undo history stores absolute paths; re-prefix them so a later + // undo does not recreate the old entity directory. + if s.undoStack != nil { + oldPrefix := filepath.Join(s.dataDir, entityType, body.Dir) + newPrefix := filepath.Join(s.dataDir, entityType, body.NewName) + s.undoStack.RewritePaths(oldPrefix, newPrefix) + } writeJSON(w, map[string]any{"ok": true, "name": body.NewName}) default: failJSON(w, "unknown action") diff --git a/internal/admin/undo.go b/internal/admin/undo.go index 24ce9c1..c2ea61d 100644 --- a/internal/admin/undo.go +++ b/internal/admin/undo.go @@ -115,6 +115,47 @@ func (us *UndoStack) SetPlayerFileOnlineCheck(fn func(relPath string) bool) { us.playerFileOnlineCheck = fn } +// RewritePaths re-prefixes every stored history path under oldPrefix so a +// directory rename does not leave stale absolute paths behind. Without this +// a later undo recreates the old directory and writes a duplicate file while +// the live file stays in the new directory. newPrefix may be empty to leave +// paths untouched. +func (us *UndoStack) RewritePaths(oldPrefix, newPrefix string) { + if oldPrefix == "" || newPrefix == "" || oldPrefix == newPrefix { + return + } + us.mu.Lock() + defer us.mu.Unlock() + sep := string(os.PathSeparator) + rewrite := func(p string) string { + if p == "" { + return p + } + if p == oldPrefix { + return newPrefix + } + if strings.HasPrefix(p, oldPrefix+sep) { + return filepath.Join(newPrefix, strings.TrimPrefix(p, oldPrefix+sep)) + } + return p + } + rewriteDesc := func(c *ChangeDesc) { + c.FilePath = rewrite(c.FilePath) + c.NewFilePath = rewrite(c.NewFilePath) + for i := range c.ExtraFiles { + c.ExtraFiles[i].FilePath = rewrite(c.ExtraFiles[i].FilePath) + c.ExtraFiles[i].NewFilePath = rewrite(c.ExtraFiles[i].NewFilePath) + } + } + for i := range us.history { + rewriteDesc(&us.history[i]) + } + for i := range us.redo { + rewriteDesc(&us.redo[i]) + } + us.save() +} + func (us *UndoStack) Push(desc ChangeDesc) { us.mu.Lock() defer us.mu.Unlock() -- cgit v1.2.3