From d5d75e63edb49c3819e0666c6a7b230ad72d5124 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 8 Jul 2026 18:21:46 -0400 Subject: [PATCH] fix(persistence): symlink confinement, real test-inertness, view_top restore Addresses the PR #98 review: - HIGH symlink escape: resolve() did only a lexical starts_with, so a base/autosave symlink -> /tmp/out let state.write("autosave/x") write outside the state dir. Now every existing component the key adds under base is lstat'd and a symlink (live OR broken) is rejected; base itself may still be a symlink (dotfile-managed ~/.local/state). Unix symlink escape test added (live + broken + plain-subdir-ok). - MEDIUM integration-test state leak: the state/history dir wiring moved out of EditorState::new() into EditorState::install_state_dirs(), called only by the real entry points (editor::run, run_daemon). Unit AND integration tests construct EditorState directly, so they never configure a real dir -> default-on recentf/saveplace write nothing to ~/.local/state/pmacs during cargo test. The inertness test now asserts a bare new() leaves StateDir unconfigured (direct proof). - MEDIUM saveplace never recorded view_top: exposed the missing pmacs.editor.view_top() getter (set_view_top existed but no getter, so the Lua stored 0). saveplace now records+restores the viewport; acceptance asserts view_top restores, not just the cursor byte. - MEDIUM/LOW relative XDG_STATE_HOME / PMACS_STATE_HOME: a relative value rooted state at a cwd-relative pmacs/... (same footgun class as the empty case). Both are now required absolute; relative values are ignored (XDG falls through to HOME). Test added. - LOW trailing blank line at recentf.lua EOF (git diff --check). Gates: fmt + workspace clippy clean; lib 1483; crdt 1654; persistence 5; m4 90; m8_1/m8_2 daemon 10/15; query-replace/completion/listview/overlay/ cua green; GPU 58; git diff --check clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- builtin/runtime/recentf.lua | 1 - builtin/runtime/saveplace.lua | 4 +- src/daemon.rs | 2 + src/editor.rs | 47 +++++++++++------- src/lua_bindings/mod.rs | 12 +++++ src/state.rs | 87 ++++++++++++++++++++++++++++++--- tests/persistence_acceptance.rs | 37 ++++++++++---- 7 files changed, 150 insertions(+), 40 deletions(-) diff --git a/builtin/runtime/recentf.lua b/builtin/runtime/recentf.lua index c607cf0..ac4dc29 100644 --- a/builtin/runtime/recentf.lua +++ b/builtin/runtime/recentf.lua @@ -83,4 +83,3 @@ pmacs.command.define { } pmacs.keymap.bind { scope = "global", sequence = "C-x C-r", command = "recent-files" } - diff --git a/builtin/runtime/saveplace.lua b/builtin/runtime/saveplace.lua index dba7b95..84174e6 100644 --- a/builtin/runtime/saveplace.lua +++ b/builtin/runtime/saveplace.lua @@ -57,7 +57,7 @@ local function record_active() if not active_ready() then return end local path = pmacs.editor.file_path() local cursor = pmacs.editor.cursor() - local view_top = pmacs.editor.view_top and pmacs.editor.view_top() or 0 + local view_top = pmacs.editor.view_top() local list, index = load_places() if index[path] then table.remove(list, index[path]) end table.insert(list, 1, { path = path, cursor = cursor, view_top = view_top }) @@ -73,7 +73,7 @@ local function restore_active() if not i then return end local e = list[i] pmacs.editor.goto_byte(e.cursor) - if pmacs.editor.set_view_top then pmacs.editor.set_view_top(e.view_top) end + pmacs.editor.set_view_top(e.view_top) end -- Record on save and on quit; restore on open. before-save / diff --git a/src/daemon.rs b/src/daemon.rs index c025d43..c2148cb 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -466,6 +466,8 @@ pub fn run_daemon(socket_path: PathBuf, instance_name: Option) -> Result // The editor outlives any single attachment; constructed once on // the dispatcher thread and used until daemon shutdown. let mut editor = EditorState::new(); + // Real session: wire up on-disk persistence (history + pmacs.state). + editor.install_state_dirs(); // Mirror the daemon's `--socket NAME` and start time into the // editor's `LocalInstanceInfo` so `pmacs.instance.identity()` // (T M5.6f) reports the same identity the daemon hands back over diff --git a/src/editor.rs b/src/editor.rs index 4627152..855e0bf 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -163,25 +163,14 @@ impl EditorState { lua_host .attach_editor(&core) .expect("editor bindings + builtin chunks"); - // Resolve the on-disk history directory before user config - // runs (so the user could in principle override it from - // `init.lua`). Skipped in test mode for the same reason as - // user config: don't touch the developer's real state dir. - #[cfg(not(test))] - { - if let Some(dir) = crate::minibuffer::user_history_dir() { - core.borrow_mut().minibuffer.history_dir = Some(dir); - } - // Arc 3 (Q#PS2): configure the `pmacs.state.*` base dir. Its - // absence under `cfg(test)` (this whole block is skipped) is - // what keeps default-on saveplace/recentf from writing to a - // developer's real state dir during the lib suite. - if let Some(dir) = crate::state::user_state_dir() { - lua_host - .lua() - .set_app_data(crate::lua_bindings::StateDir(dir)); - } - } + // The on-disk state dirs (minibuffer history + pmacs.state) are + // deliberately NOT configured here — see `install_state_dirs`, + // called by the real entry points (`run` / `run_daemon`) only. + // Constructing an `EditorState` — which unit AND integration + // tests do directly — leaves them unconfigured, so default-on + // persistence (recentf/saveplace) writes nothing to a + // developer's real state dir during `cargo test`. Tests that + // exercise persistence inject a `StateDir` app-data explicitly. // The async runtime: install pmacs._async raw helpers, then // load the friendly Lua surface (`pmacs.async`, Handle class, // `pmacs.workers.*`). Both must run before user config so a @@ -457,6 +446,24 @@ impl EditorState { .eval(Some("@pmacs/runtime/async.lua:tick"), "pmacs._async.tick()"); } + /// Configure the on-disk state directories (minibuffer history + + /// `pmacs.state`) from the environment. The **real** entry points + /// (`run`, `run_daemon`) call this after construction; tests do not, + /// so neither the unit suite nor integration tests (which link the + /// lib without `cfg(test)`) touch a developer's real + /// `~/.local/state/pmacs`. Honors the `PMACS_STATE_HOME` override + /// (see [`crate::state::user_state_dir`]). + pub fn install_state_dirs(&self) { + if let Some(dir) = crate::minibuffer::user_history_dir() { + self.core.borrow_mut().minibuffer.history_dir = Some(dir); + } + if let Some(dir) = crate::state::user_state_dir() { + self.lua_host + .lua() + .set_app_data(crate::lua_bindings::StateDir(dir)); + } + } + /// Construct an editor for a path. Empty buffer with `[new file]` /// status if the path does not exist; loaded contents otherwise. pub fn open(path: PathBuf) -> io::Result { @@ -1516,6 +1523,8 @@ pub fn run(file: Option) -> io::Result<()> { Some(path) => EditorState::open(path)?, None => EditorState::new(), }; + // Real session: wire up on-disk persistence (history + pmacs.state). + state.install_state_dirs(); // Post-init dispatch: read whatever init.lua left in the // RequestedAttach slot and decide whether to run local or hand diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 874d8e3..ef73963 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -10768,6 +10768,18 @@ fn install_session(editor: &Table, lua: &Lua, core: &SharedCore) -> mlua::Result })?, )?; } + { + // view_top(): the active window's first visible source line. + // The saveplace getter (Arc 3) — pairs with set_view_top so a + // reopen restores the viewport, not just the cursor. + let cc = core.clone(); + editor.set( + "view_top", + lua.create_function(move |_, ()| { + i64::try_from(cc.borrow().view_top()).map_err(mlua::Error::external) + })?, + )?; + } { // set_view_top(line): set the first visible source line // (clamped to the buffer's line count) — desktop restore. diff --git a/src/state.rs b/src/state.rs index 88a9bea..d0b6865 100644 --- a/src/state.rs +++ b/src/state.rs @@ -25,11 +25,21 @@ use std::path::PathBuf; /// `None` rather than a relative path. #[must_use] pub fn state_dir(xdg_state: Option<&OsStr>, home: Option<&OsStr>) -> Option { + // `XDG_STATE_HOME` must be an absolute path per the XDG spec; a + // relative value would root state at a *cwd-relative* `pmacs/...` + // (the same latent bug the empty case had). Ignore relative values + // and fall through to `HOME`, which likewise must be absolute. if let Some(xdg) = xdg_state.filter(|s| !is_blank(s)) { - return Some(PathBuf::from(xdg).join("pmacs")); + let p = PathBuf::from(xdg); + if p.is_absolute() { + return Some(p.join("pmacs")); + } } - home.filter(|s| !is_blank(s)) - .map(|h| PathBuf::from(h).join(".local").join("state").join("pmacs")) + home.filter(|s| !is_blank(s)).and_then(|h| { + let p = PathBuf::from(h); + p.is_absolute() + .then(|| p.join(".local").join("state").join("pmacs")) + }) } /// Resolve the base state directory from the process environment. @@ -46,7 +56,12 @@ pub fn user_state_dir() -> Option { .as_deref() .filter(|s| !is_blank(s)) { - return Some(PathBuf::from(over).join("pmacs")); + // The override must also be absolute — a relative redirect would + // reintroduce the cwd-relative-state footgun. + let p = PathBuf::from(over); + if p.is_absolute() { + return Some(p.join("pmacs")); + } } state_dir( std::env::var_os("XDG_STATE_HOME").as_deref(), @@ -113,18 +128,37 @@ pub fn validate_name(name: &str) -> Result<(), &'static str> { Ok(()) } -/// Resolve a validated key to its absolute path under `base`, with a -/// canonical-prefix belt: the joined path must still start with `base`. +/// Resolve a validated key to its path under `base`, refusing any +/// route that could escape the state directory. +/// +/// Two guards beyond [`validate_name`]'s lexical rules: +/// 1. a `starts_with(base)` belt (redundant with `validate_name`, kept +/// as defense in depth); +/// 2. **symlink confinement** — every *existing* component the key adds +/// under `base` is `lstat`'d, and a symlink (live *or* broken) is +/// rejected. Without this, a `base/autosave` symlink pointing at +/// `/tmp/out` would let `state.write("autosave/x", …)` write outside +/// `base` — the lexical check alone can't catch it. `base` itself may +/// be a symlink (a dotfile-managed `~/.local/state`); only the +/// components the *key* contributes are guarded. /// /// # Errors -/// Propagates [`validate_name`], or errors if the join escapes `base` -/// (which [`validate_name`] already prevents — this is defense in depth). +/// Propagates [`validate_name`], or errors on an escaping / symlinked key. pub fn resolve(base: &Path, name: &str) -> Result { validate_name(name)?; let path = base.join(name); if !path.starts_with(base) { return Err("state key escapes the state directory"); } + let mut cur = base.to_path_buf(); + for part in name.split('/') { + cur.push(part); + if let Ok(meta) = std::fs::symlink_metadata(&cur) + && meta.file_type().is_symlink() + { + return Err("state key traverses a symlink"); + } + } Ok(path) } @@ -253,6 +287,43 @@ mod tests { assert!(resolve(&base, "../escape").is_err()); } + #[cfg(unix)] + #[test] + fn resolve_rejects_symlink_components() { + let root = std::env::temp_dir().join(format!("pmacs-symlink-{}", std::process::id())); + let base = root.join("pmacs"); + let outside = root.join("outside"); + std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + + // A live symlink `base/evil -> outside` must be refused (else a + // write through it escapes the state dir). + let evil = base.join("evil"); + std::os::unix::fs::symlink(&outside, &evil).unwrap(); + assert!(resolve(&base, "evil/x").is_err(), "live symlink escape"); + assert!(write(&base, "evil/x", b"nope").is_err()); + assert!(!outside.join("x").exists(), "nothing was written outside"); + + // A broken symlink component is also refused (lstat sees it). + let broken = base.join("broken"); + std::os::unix::fs::symlink(root.join("does-not-exist"), &broken).unwrap(); + assert!(resolve(&base, "broken/y").is_err(), "broken symlink escape"); + + // A plain subdir is fine. + assert!(resolve(&base, "autosave/ok").is_ok()); + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn relative_xdg_and_home_are_ignored() { + // A relative XDG_STATE_HOME (spec violation) must not root state + // at a cwd-relative path; it falls through to HOME. + let d = state_dir(Some(OsStr::new("relstate")), Some(OsStr::new("/home/u"))).unwrap(); + assert_eq!(d, PathBuf::from("/home/u/.local/state/pmacs")); + // Relative XDG and relative HOME → None, never a relative root. + assert!(state_dir(Some(OsStr::new("rel")), Some(OsStr::new("relhome"))).is_none()); + } + #[test] fn write_read_remove_round_trip() { let dir = std::env::temp_dir().join(format!("pmacs-state-{}", std::process::id())); diff --git a/tests/persistence_acceptance.rs b/tests/persistence_acceptance.rs index 8b0a5f6..c3a6c9d 100644 --- a/tests/persistence_acceptance.rs +++ b/tests/persistence_acceptance.rs @@ -12,6 +12,7 @@ use pmacs::editor::EditorState; use pmacs::lua_bindings::StateDir; +use std::fmt::Write as _; use std::path::PathBuf; /// A fresh editor whose state dir is a private, empty tempdir (unique @@ -72,9 +73,16 @@ fn state_round_trips_and_rejects_escapes() { #[test] fn state_is_inert_when_unconfigured() { + // A plain EditorState::new() must NOT configure a state dir — that + // is what keeps the whole integration-test suite (which links the + // lib without cfg(test)) from writing to a developer's real + // ~/.local/state/pmacs. Only the real entry points call + // install_state_dirs(); tests construct EditorState directly. let s = EditorState::new(); - // Simulate no state dir (the cfg(test) lib case, or no HOME). - s.lua_host.lua().remove_app_data::(); + assert!( + s.lua_host.lua().app_data_ref::().is_none(), + "new() must leave the state dir unconfigured" + ); let (avail, wrote, read): (bool, bool, Option) = s .lua_host .lua() @@ -116,29 +124,38 @@ fn recentf_records_dedups_and_orders_mru() { } #[test] -fn saveplace_restores_cursor_on_reopen() { +fn saveplace_restores_cursor_and_view_top_on_reopen() { let (s, dir) = editor_with_state_dir(); - let f = write_file(&dir, "place.rs", "line0\nline1\nline2\nline3\n"); - // Open, move to byte 12 ("line2"), save (before-save records the - // place), then KILL the buffer — so the reopen is a fresh load + // A tall file so view_top can be non-zero. + let mut body = String::new(); + for i in 0..40 { + let _ = writeln!(body, "line{i}"); + } + let f = write_file(&dir, "place.rs", &body); + // Open, scroll to view_top 7, put the cursor at byte 48 (start of + // "line8", within that viewport), save (before-save records the + // place), then KILL — so the reopen is a fresh load // (buffer.after-load), the cross-session path saveplace targets. - let cursor: i64 = s + // "line0\n".."line9\n" are 6 bytes each → line8 begins at byte 48. + let (cursor, view_top): (i64, i64) = s .lua_host .lua() .load(format!( r#" local b = pmacs.buffer.find_or_open({f:?}) - pmacs.editor.goto_byte(12) + pmacs.editor.set_view_top(7) + pmacs.editor.goto_byte(48) pmacs.command.invoke("buffer.save") pmacs.buffer.kill(b) -- Reopen from scratch: after-load fires → saveplace restores. pmacs.buffer.find_or_open({f:?}) - return pmacs.editor.cursor() + return pmacs.editor.cursor(), pmacs.editor.view_top() "# )) .eval() .expect("place + save + kill + reopen"); - assert_eq!(cursor, 12, "saveplace restored the cursor byte on reload"); + assert_eq!(cursor, 48, "saveplace restored the cursor byte on reload"); + assert_eq!(view_top, 7, "saveplace restored the viewport (view_top)"); std::fs::remove_dir_all(&dir).ok(); }