From 79764732406d6484f67d3dec045483dcb5d08c28 Mon Sep 17 00:00:00 2001 From: historia <[not public]> Date: Fri, 18 Sep 2026 20:03:13 -0400 Subject: feat: better card ascii art --- internal/admin/api_colors.go | 7 +- internal/admin/api_entity_path_test.go | 21 +++++ internal/admin/api_files.go | 9 ++- internal/admin/api_modules.go | 9 ++- internal/admin/api_room_courses.go | 9 ++- internal/admin/api_room_insert_remove.go | 3 +- internal/admin/api_rooms.go | 2 +- internal/admin/api_rooms_bulk_delete.go | 5 ++ internal/admin/api_techs.go | 11 ++- internal/admin/api_triggers.go | 3 +- internal/admin/server.go | 5 +- internal/admin/undo.go | 134 +++++++++++++++++-------------- internal/admin/yaml_util.go | 21 +++++ 13 files changed, 155 insertions(+), 84 deletions(-) (limited to 'internal/admin') diff --git a/internal/admin/api_colors.go b/internal/admin/api_colors.go index 7989e35..b6285c4 100644 --- a/internal/admin/api_colors.go +++ b/internal/admin/api_colors.go @@ -12,6 +12,7 @@ import ( "thehouseoficarus/internal/color" "thehouseoficarus/internal/config" "thehouseoficarus/internal/game" + "thehouseoficarus/internal/persistence" ) // colorCategory describes one editable color category: its config key, a @@ -595,13 +596,13 @@ func (s *AdminServer) writeConfigFile() error { if merr != nil { return merr } - return os.WriteFile(s.configPath, out, 0644) + return persistence.WriteFile(s.configPath, out, 0644) } out, err := patchColorMaps(data, s.cfg.DefaultColors, s.cfg.ConstantColors) if err != nil { return err } - return os.WriteFile(s.configPath, out, 0644) + return persistence.WriteFile(s.configPath, out, 0644) } // backupConfigFile copies the current config.yaml next to itself before an @@ -615,7 +616,7 @@ func (s *AdminServer) backupConfigFile() error { } return err } - return os.WriteFile(s.configPath+".bak", data, 0600) + return persistence.WriteFile(s.configPath+".bak", data, 0600) } // patchColorMaps updates the default_colors and constant_colors mappings in a diff --git a/internal/admin/api_entity_path_test.go b/internal/admin/api_entity_path_test.go index 75fed7a..3a0f87b 100644 --- a/internal/admin/api_entity_path_test.go +++ b/internal/admin/api_entity_path_test.go @@ -56,10 +56,31 @@ func TestEntityAPICannotEscapeData(t *testing.T) { if !strings.Contains(rec.Body.String(), "error") { t.Fatalf("traversal rename id not rejected: status %d body %s", rec.Code, rec.Body.String()) } + // Traversal SOURCE id via the rename and move routes: the source is + // joined into a path too, so it must be rejected before it can move a + // file outside data/ (e.g. the server's own config.yaml). + sentinel := filepath.Join(root, "sentinel.yaml") + if err := os.WriteFile(sentinel, []byte("name: Sentinel\n"), 0o600); err != nil { + t.Fatal(err) + } + for _, tc := range []struct{ path, body string }{ + {"/api/objects/rename", `{"id":"../../sentinel","new_id":"stolen"}`}, + {"/api/objects/move", `{"id":"../../sentinel","dir":"sub"}`}, + } { + if rec := post(tc.path, tc.body); !strings.Contains(rec.Body.String(), "error") { + t.Fatalf("traversal source id not rejected at %s: status %d body %s", tc.path, rec.Code, rec.Body.String()) + } + } + if _, err := os.Stat(sentinel); err != nil { + t.Fatalf("traversal source id moved a file outside data/: %v", err) + } // Nothing may exist outside the data directory. if _, err := os.Stat(filepath.Join(root, "outside.yaml")); !os.IsNotExist(err) { t.Fatal("entity API escaped the data directory") } + if _, err := os.Stat(filepath.Join(root, "stolen.yaml")); !os.IsNotExist(err) { + t.Fatal("traversal rename escaped the data directory") + } } // TestTechModuleCreateRejectsTraversalID: POST /api/techs and /api/modules diff --git a/internal/admin/api_files.go b/internal/admin/api_files.go index 2aa0e5f..b9f54c5 100644 --- a/internal/admin/api_files.go +++ b/internal/admin/api_files.go @@ -227,9 +227,10 @@ func (s *AdminServer) handleFiles(w http.ResponseWriter, r *http.Request) { // writing, so the raw editor can never disclose or overwrite it. body = restoreAccountSecret(relPath, fullPath, body) - var oldContent []byte - if existing, err := snapshotFile(fullPath); err == nil { - oldContent = existing + oldContent, existed, err := snapshotBefore(fullPath) + if err != nil { + failJSON(w, "read existing file: "+err.Error()) + return } dir := filepath.Dir(fullPath) @@ -243,7 +244,7 @@ func (s *AdminServer) handleFiles(w http.ResponseWriter, r *http.Request) { return } - isCreate := oldContent == nil + isCreate := !existed desc := "Updated file " + relPath if isCreate { desc = "Created file " + relPath diff --git a/internal/admin/api_modules.go b/internal/admin/api_modules.go index 8356b2d..ea547c5 100644 --- a/internal/admin/api_modules.go +++ b/internal/admin/api_modules.go @@ -96,9 +96,10 @@ func (s *AdminServer) handleModuleByID(w http.ResponseWriter, r *http.Request) { delete(body, "_raw") path := filepath.Join(s.dataDir, "modules", id+".yaml") - var oldContent []byte - if existing, err := snapshotFile(path); err == nil { - oldContent = existing + oldContent, existed, err := snapshotBefore(path) + if err != nil { + failJSON(w, "read existing file: "+err.Error()) + return } newContent, err := writeMapAsYAML(path, body) @@ -107,7 +108,7 @@ func (s *AdminServer) handleModuleByID(w http.ResponseWriter, r *http.Request) { return } - isCreate := oldContent == nil + isCreate := !existed desc := "Updated module " + id if isCreate { desc = "Created module " + id diff --git a/internal/admin/api_room_courses.go b/internal/admin/api_room_courses.go index 5a33e6a..db74adf 100644 --- a/internal/admin/api_room_courses.go +++ b/internal/admin/api_room_courses.go @@ -204,16 +204,17 @@ func (s *AdminServer) handleCourseByID(w http.ResponseWriter, r *http.Request) { delete(body, "_raw") delete(body, "_path") - var oldContent []byte - if existing, err := snapshotFile(path); err == nil { - oldContent = existing + oldContent, existed, err := snapshotBefore(path) + if err != nil { + failJSON(w, "read existing file: "+err.Error()) + return } newContent, err := writeMapAsYAML(path, body) if err != nil { failJSON(w, "write error: "+err.Error()) return } - isCreate := oldContent == nil + isCreate := !existed desc := "Updated course " + id if isCreate { desc = "Created course " + id diff --git a/internal/admin/api_room_insert_remove.go b/internal/admin/api_room_insert_remove.go index 45537d6..5f03b72 100644 --- a/internal/admin/api_room_insert_remove.go +++ b/internal/admin/api_room_insert_remove.go @@ -11,6 +11,7 @@ import ( "gopkg.in/yaml.v3" "thehouseoficarus/internal/behavior" + "thehouseoficarus/internal/persistence" "thehouseoficarus/internal/world" ) @@ -368,7 +369,7 @@ func rollbackWrite(path string, oldContent []byte) { if oldContent == nil { return } - if err := os.WriteFile(path, oldContent, 0644); err != nil { + if err := persistence.WriteFile(path, oldContent, 0644); err != nil { fmt.Printf("admin: rollback write %s failed: %v\n", path, err) } } diff --git a/internal/admin/api_rooms.go b/internal/admin/api_rooms.go index 32a6f1c..863af47 100644 --- a/internal/admin/api_rooms.go +++ b/internal/admin/api_rooms.go @@ -365,7 +365,7 @@ func (s *AdminServer) handleRoomByID(w http.ResponseWriter, r *http.Request) { if loadErr != nil { continue } - changed := false + changed := otherRoom.RemoveRoomRefs(id) for _, dir := range world.ExitOrder { exit, exists := otherRoom.Exits[dir] if !exists || exit.Room != id { diff --git a/internal/admin/api_rooms_bulk_delete.go b/internal/admin/api_rooms_bulk_delete.go index bba1879..d15549e 100644 --- a/internal/admin/api_rooms_bulk_delete.go +++ b/internal/admin/api_rooms_bulk_delete.go @@ -120,6 +120,11 @@ func (s *AdminServer) handleBulkDelete(w http.ResponseWriter, r *http.Request) { continue } changed := false + for deletedID := range deletedSet { + if otherRoom.RemoveRoomRefs(deletedID) { + changed = true + } + } for _, dir := range world.ExitOrder { exit, exists := otherRoom.Exits[dir] if !exists || !deletedSet[exit.Room] { diff --git a/internal/admin/api_techs.go b/internal/admin/api_techs.go index 9e742e1..d2c5f65 100644 --- a/internal/admin/api_techs.go +++ b/internal/admin/api_techs.go @@ -111,9 +111,16 @@ func (s *AdminServer) handleTechByID(w http.ResponseWriter, r *http.Request) { var oldContent []byte if !isCreate { - if existing, err := snapshotFile(path); err == nil { - oldContent = existing + existing, existed, err := snapshotBefore(path) + if err != nil { + failJSON(w, "read existing file: "+err.Error()) + return } + if !existed { + failJSON(w, "not found: "+id) + return + } + oldContent = existing } newContent, err := writeMapAsYAML(path, body) diff --git a/internal/admin/api_triggers.go b/internal/admin/api_triggers.go index 299ca10..0dd086c 100644 --- a/internal/admin/api_triggers.go +++ b/internal/admin/api_triggers.go @@ -11,6 +11,7 @@ import ( "gopkg.in/yaml.v3" "thehouseoficarus/internal/behavior" + "thehouseoficarus/internal/persistence" ) func (s *AdminServer) handleTriggers(w http.ResponseWriter, r *http.Request) { @@ -138,7 +139,7 @@ func (s *AdminServer) handleTriggerByID(w http.ResponseWriter, r *http.Request) return } - if err := os.WriteFile(newPath, triggerData, 0644); err != nil { + if err := persistence.WriteFile(newPath, triggerData, 0644); err != nil { failJSON(w, "write error: "+err.Error()) return } diff --git a/internal/admin/server.go b/internal/admin/server.go index 05586ef..f35be84 100644 --- a/internal/admin/server.go +++ b/internal/admin/server.go @@ -982,8 +982,7 @@ func (s *AdminServer) handleEntityRename(entityType string) http.HandlerFunc { ID string `json:"id"` NewID string `json:"new_id"` } - if err := readJSON(r, &body); err != nil || body.ID == "" || body.NewID == "" || - !validEntityID(body.NewID) { + if err := readJSON(r, &body); err != nil || !validEntityID(body.ID) || !validEntityID(body.NewID) { failJSON(w, "invalid") return } @@ -1015,7 +1014,7 @@ func (s *AdminServer) handleEntityMove(entityType string) http.HandlerFunc { ID string `json:"id"` Dir string `json:"dir"` } - if err := readJSON(r, &body); err != nil || body.ID == "" { + if err := readJSON(r, &body); err != nil || !validEntityID(body.ID) { failJSON(w, "invalid") return } diff --git a/internal/admin/undo.go b/internal/admin/undo.go index c2ea61d..3001c8b 100644 --- a/internal/admin/undo.go +++ b/internal/admin/undo.go @@ -227,6 +227,17 @@ func (us *UndoStack) Redo() *ChangeDesc { return &last } +// requireSnapshot rejects a nil before/after image on a change that must +// restore real content. A failed before-image read leaves nil, and writing +// it would truncate the file to zero bytes (which parses as a zero-value +// def), so a missing image must fail the undo/redo instead. +func requireSnapshot(content []byte, path, action string) error { + if content == nil { + return fmt.Errorf("%s %s: no snapshot recorded", action, path) + } + return nil +} + // applyUndo reverts a change: the primary file, then every extra file. // Extra-file failures propagate: a multi-file undo that restored only part // of a change must be reported as failed, never as success. @@ -270,6 +281,9 @@ func (us *UndoStack) applyUndo(c ChangeDesc) error { return fmt.Errorf("remove created %s: %w", c.FilePath, err) } } else { + if err := requireSnapshot(c.OldContent, c.FilePath, "revert"); err != nil { + return err + } if err := us.writeSnapshot(c.FilePath, restored); err != nil { return fmt.Errorf("revert %s: %w", c.FilePath, err) } @@ -332,6 +346,9 @@ func (us *UndoStack) applyRedo(c ChangeDesc) error { if c.NewFilePath != "" { target = c.NewFilePath } + if err := requireSnapshot(c.NewContent, target, "reapply"); err != nil { + return err + } if err := us.writeSnapshot(target, restored); err != nil { return fmt.Errorf("reapply %s: %w", target, err) } @@ -362,6 +379,9 @@ func (us *UndoStack) applyUndoExtraFile(ef ExtraFile) error { return fmt.Errorf("undo restore %s: %w", ef.FilePath, err) } case ef.NewFilePath != "": + if err := requireSnapshot(ef.OldContent, ef.FilePath, "undo restore"); err != nil { + return err + } if err := os.Remove(ef.NewFilePath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("undo remove %s: %w", ef.NewFilePath, err) } @@ -369,6 +389,9 @@ func (us *UndoStack) applyUndoExtraFile(ef ExtraFile) error { return fmt.Errorf("undo restore %s: %w", ef.FilePath, err) } default: + if err := requireSnapshot(ef.OldContent, ef.FilePath, "undo revert"); err != nil { + return err + } if err := us.writeRestored(ef.FilePath, ef.OldContent); err != nil { return fmt.Errorf("undo revert %s: %w", ef.FilePath, err) } @@ -387,6 +410,9 @@ func (us *UndoStack) applyRedoExtraFile(ef ExtraFile) error { return fmt.Errorf("redo create %s: %w", ef.FilePath, err) } case ef.NewFilePath != "": + if err := requireSnapshot(ef.NewContent, ef.NewFilePath, "redo create"); err != nil { + return err + } if err := os.Remove(ef.FilePath); err != nil && !os.IsNotExist(err) { return fmt.Errorf("redo remove %s: %w", ef.FilePath, err) } @@ -394,6 +420,9 @@ func (us *UndoStack) applyRedoExtraFile(ef ExtraFile) error { return fmt.Errorf("redo create %s: %w", ef.NewFilePath, err) } default: + if err := requireSnapshot(ef.NewContent, ef.FilePath, "redo reapply"); err != nil { + return err + } if err := us.writeRestored(ef.FilePath, ef.NewContent); err != nil { return fmt.Errorf("redo reapply %s: %w", ef.FilePath, err) } @@ -450,7 +479,9 @@ func (us *UndoStack) writeSnapshot(path string, content []byte) error { if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil { return err } - if err := os.WriteFile(path, content, us.fileMode(path)); err != nil { + // Atomic temp+fsync+rename: a crash mid-undo must never leave a + // truncated world file (empty YAML parses as a zero-value def). + if err := persistence.WriteFile(path, content, us.fileMode(path)); err != nil { return err } if us.isPlayerDataPath(path) { @@ -513,39 +544,46 @@ func (us *UndoStack) Info() UndoInfo { return info } +// undoExtraEntry and undoEntry are the persisted undo/redo wire types. They +// are declared ONCE and shared by save and load: when each function declared +// its own copy, a field added to one and not the other silently dropped +// persisted history on the next restart. +type undoExtraEntry struct { + FilePath string `json:"file_path"` + NewFilePath string `json:"new_file_path,omitempty"` + OldContent string `json:"old_content"` + NewContent string `json:"new_content"` + IsDelete bool `json:"is_delete,omitempty"` + IsCreate bool `json:"is_create,omitempty"` + IsDirDelete bool `json:"is_dir_delete,omitempty"` +} + +type undoEntry struct { + Time string `json:"time"` + Description string `json:"description"` + FilePath string `json:"file_path"` + NewFilePath string `json:"new_file_path,omitempty"` + OldContent string `json:"old_content"` + NewContent string `json:"new_content"` + IsDelete bool `json:"is_delete"` + IsCreate bool `json:"is_create"` + IsDirDelete bool `json:"is_dir_delete,omitempty"` + ExtraFiles []undoExtraEntry `json:"extra_files,omitempty"` + IDMap map[int]int `json:"id_map,omitempty"` +} + +type undoFile struct { + History []undoEntry `json:"history"` + Redo []undoEntry `json:"redo"` +} + func (us *UndoStack) save() { - type extraEntry struct { - FilePath string `json:"file_path"` - NewFilePath string `json:"new_file_path,omitempty"` - OldContent string `json:"old_content"` - NewContent string `json:"new_content"` - IsDelete bool `json:"is_delete,omitempty"` - IsCreate bool `json:"is_create,omitempty"` - IsDirDelete bool `json:"is_dir_delete,omitempty"` - } - type entry struct { - Time string `json:"time"` - Description string `json:"description"` - FilePath string `json:"file_path"` - NewFilePath string `json:"new_file_path,omitempty"` - OldContent string `json:"old_content"` - NewContent string `json:"new_content"` - IsDelete bool `json:"is_delete"` - IsCreate bool `json:"is_create"` - IsDirDelete bool `json:"is_dir_delete,omitempty"` - ExtraFiles []extraEntry `json:"extra_files,omitempty"` - IDMap map[int]int `json:"id_map,omitempty"` - } - type saveData struct { - History []entry `json:"history"` - Redo []entry `json:"redo"` - } - toEntries := func(changes []ChangeDesc) []entry { - var entries []entry + toEntries := func(changes []ChangeDesc) []undoEntry { + var entries []undoEntry for _, c := range changes { - var extra []extraEntry + var extra []undoExtraEntry for _, ef := range c.ExtraFiles { - extra = append(extra, extraEntry{ + extra = append(extra, undoExtraEntry{ FilePath: ef.FilePath, NewFilePath: ef.NewFilePath, OldContent: string(us.redactContent(ef.OldContent, ef.FilePath, ef.NewFilePath)), @@ -555,7 +593,7 @@ func (us *UndoStack) save() { IsDirDelete: ef.IsDirDelete, }) } - entries = append(entries, entry{ + entries = append(entries, undoEntry{ Time: c.Time, Description: c.Description, FilePath: c.FilePath, @@ -571,7 +609,7 @@ func (us *UndoStack) save() { } return entries } - data := saveData{ + data := undoFile{ History: toEntries(us.history), Redo: toEntries(us.redo), } @@ -708,38 +746,12 @@ func (us *UndoStack) load() { if err != nil { return } - type extraEntry struct { - FilePath string `json:"file_path"` - NewFilePath string `json:"new_file_path,omitempty"` - OldContent string `json:"old_content"` - NewContent string `json:"new_content"` - IsDelete bool `json:"is_delete,omitempty"` - IsCreate bool `json:"is_create,omitempty"` - IsDirDelete bool `json:"is_dir_delete,omitempty"` - } - type entry struct { - Time string `json:"time"` - Description string `json:"description"` - FilePath string `json:"file_path"` - NewFilePath string `json:"new_file_path,omitempty"` - OldContent string `json:"old_content"` - NewContent string `json:"new_content"` - IsDelete bool `json:"is_delete"` - IsCreate bool `json:"is_create"` - IsDirDelete bool `json:"is_dir_delete,omitempty"` - ExtraFiles []extraEntry `json:"extra_files,omitempty"` - IDMap map[int]int `json:"id_map,omitempty"` - } - type saveData struct { - History []entry `json:"history"` - Redo []entry `json:"redo"` - } - var sd saveData + var sd undoFile if err := json.Unmarshal(data, &sd); err != nil { log.Printf("undo: failed to load history: %v", err) return } - toChanges := func(entries []entry) []ChangeDesc { + toChanges := func(entries []undoEntry) []ChangeDesc { out := make([]ChangeDesc, 0, len(entries)) for _, e := range entries { c := ChangeDesc{ diff --git a/internal/admin/yaml_util.go b/internal/admin/yaml_util.go index 8cd3252..e1d4942 100644 --- a/internal/admin/yaml_util.go +++ b/internal/admin/yaml_util.go @@ -157,6 +157,21 @@ func snapshotFile(path string) ([]byte, error) { return os.ReadFile(path) } +// snapshotBefore reads a file's content for an undo record and reports +// whether it existed. A missing file returns (nil, false, nil) — a create. A +// read failure on an EXISTING file returns an error: treating it as a create +// would make the undo record delete the real file, so the caller must abort. +func snapshotBefore(path string) ([]byte, bool, error) { + data, err := os.ReadFile(path) + if err == nil { + return data, true, nil + } + if os.IsNotExist(err) { + return nil, false, nil + } + return nil, false, err +} + func findEntityPath(dataDir, subdir, id string) (string, error) { _, path, err := readYAMLFile(dataDir, subdir, id) if err == nil { @@ -167,6 +182,9 @@ func findEntityPath(dataDir, subdir, id string) (string, error) { } func moveEntityFile(dataDir, subdir, id, targetDir string) (string, string, []byte, error) { + if !validEntityID(id) { + return "", "", nil, fmt.Errorf("invalid id: %q", id) + } if targetDir != "" && !validEntityID(targetDir) { return "", "", nil, fmt.Errorf("invalid directory: %q", targetDir) } @@ -219,6 +237,9 @@ func createEntityDir(dataDir, subdir, name string) error { } func renameEntityFile(dataDir, subdir, oldID, newID string) (string, string, []byte, error) { + if !validEntityID(oldID) { + return "", "", nil, fmt.Errorf("invalid id: %q", oldID) + } if !validEntityID(newID) { return "", "", nil, fmt.Errorf("invalid new id: %q", newID) } -- cgit v1.2.3