From 699d4f76e066872f6474aaaaf5dfc8a0fc4af708 Mon Sep 17 00:00:00 2001 From: simota Date: Sun, 6 Sep 2026 19:25:15 +0900 Subject: [PATCH 1/2] fix: close six 2026-09 audit findings across config, ipc, grid, session, and links - config writer: a key shadowed by a trailing `config-file` include is also appended at the end so the saved value is what the reader resolves (B01) - config watcher: track `config-file` includes (transitive, optional-absent included) via new `noa_config::config_include_paths` (B02) - ipc token: publish via tmp + hard_link (create-if-absent); concurrent first-run provisioners converge on one token (B03) - grid modes: `ModeState::is_tracked` allowlist; unknown modes are no longer retained in the linear-scanned set (B04) - session: 16 MiB file cap and 128-level JSON depth cap so a corrupt file fails as "no session" instead of overflowing the stack at launch (B05) - link_open: reap the `open` child for URIs on a detached thread (B06) Claude-Session: https://claude.ai/code/session_01GBe292wmocEXM8SjBzjGq7 --- crates/noa-app/src/app/config_reload.rs | 83 +++++++++++++++++++- crates/noa-app/src/link_open.rs | 18 ++++- crates/noa-app/src/session.rs | 61 ++++++++++++++- crates/noa-config/src/lib.rs | 11 +++ crates/noa-config/src/parser.rs | 1 + crates/noa-config/src/parser/includes.rs | 27 +++++++ crates/noa-config/src/parser/tests.rs | 20 +++++ crates/noa-config/src/writer.rs | 56 ++++++++++++++ crates/noa-grid/src/modes.rs | 63 +++++++++++++++ crates/noa-ipc/src/auth.rs | 97 +++++++++++++++++------- crates/noa-ipc/tests/token_tests.rs | 43 +++++++++++ 11 files changed, 447 insertions(+), 33 deletions(-) diff --git a/crates/noa-app/src/app/config_reload.rs b/crates/noa-app/src/app/config_reload.rs index 6bf95e92..772f970d 100644 --- a/crates/noa-app/src/app/config_reload.rs +++ b/crates/noa-app/src/app/config_reload.rs @@ -18,6 +18,13 @@ const CONFIG_WATCH_INTERVAL: Duration = Duration::from_secs(3); pub(super) struct ConfigWatcher { path: Option, signature: Option, + /// Files pulled in via `config-file` (transitively, optional-and-absent + /// ones included) with the signature seen at the last reload. Editing a + /// split-out child config must reload just like editing the main file + /// (B02, 2026-09 audit). The list is re-derived on every reload, so an + /// include added or removed in a watched file is picked up on the next + /// reload that edit triggers. + includes: Vec<(PathBuf, Option)>, next_check: Option, } @@ -48,12 +55,14 @@ impl ConfigWatcher { fn with_path(path: Option) -> Self { let signature = path.as_deref().and_then(config_file_signature); + let includes = path.as_deref().map(include_signatures).unwrap_or_default(); let next_check = path .as_ref() .map(|_| Instant::now() + CONFIG_WATCH_INTERVAL); Self { path, signature, + includes, next_check, } } @@ -72,8 +81,16 @@ impl ConfigWatcher { let next = now + CONFIG_WATCH_INTERVAL; self.next_check = Some(next); let signature = config_file_signature(path); - if signature != self.signature { - self.signature = signature; + let mut changed = signature != self.signature; + self.signature = signature; + for (include, seen) in &mut self.includes { + let current = config_file_signature(include); + if current != *seen { + *seen = current; + changed = true; + } + } + if changed { ConfigWatchTick::Changed(next) } else { ConfigWatchTick::Waiting(next) @@ -95,10 +112,21 @@ impl ConfigWatcher { fn mark_current(&mut self) { if let Some(path) = self.path.as_deref() { self.signature = config_file_signature(path); + self.includes = include_signatures(path); } } } +fn include_signatures(path: &Path) -> Vec<(PathBuf, Option)> { + noa_config::config_include_paths(path) + .into_iter() + .map(|include| { + let signature = config_file_signature(&include); + (include, signature) + }) + .collect() +} + fn config_file_signature(path: &Path) -> Option { let metadata = fs::metadata(path).ok()?; metadata.is_file().then_some(ConfigFileSignature { @@ -809,6 +837,57 @@ mod tests { )); } + /// Editing only a `config-file` child (main file untouched) must still + /// reload; a `?`-optional include that appears later must too, and a + /// newly added include becomes tracked after `mark_current`. + #[test] + fn watcher_detects_edits_to_included_files() { + let dir = temp_config_path("includes"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config"); + let child = dir.join("child.conf"); + fs::write(&child, "font-size = 18\n").unwrap(); + fs::write( + &path, + "font-size = 14\nconfig-file = child.conf\nconfig-file = ?optional.conf\n", + ) + .unwrap(); + let mut watcher = ConfigWatcher::with_path(Some(path.clone())); + let mut now = Instant::now() + CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Waiting(_))); + + // Child edited, main file's mtime/len unchanged. Force a distinct + // signature via length so a same-second mtime cannot mask it. + fs::write(&child, "font-size = 20\n\n").unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Waiting(_))); + + // Optional include comes into existence. + fs::write(dir.join("optional.conf"), "theme = X\n").unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + + // A new include added to the main file is tracked after reload. + let extra = dir.join("extra.conf"); + fs::write(&extra, "font-size = 30\n").unwrap(); + fs::write( + &path, + "config-file = child.conf\nconfig-file = extra.conf\n\n\n", + ) + .unwrap(); + watcher.mark_current(); + assert!(watcher.includes.iter().any(|(p, _)| p == &extra)); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Waiting(_))); + fs::write(&extra, "font-size = 31\n\n").unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + + fs::remove_dir_all(&dir).ok(); + } + /// `expedite` (focus gain / settings-panel commit) must make the very /// next tick stat the file instead of waiting out the slow /// `CONFIG_WATCH_INTERVAL`, and must only re-time the pending check — diff --git a/crates/noa-app/src/link_open.rs b/crates/noa-app/src/link_open.rs index 067f36d7..b99f1f1d 100644 --- a/crates/noa-app/src/link_open.rs +++ b/crates/noa-app/src/link_open.rs @@ -37,9 +37,21 @@ pub fn open_uri(uri: &str) { log::warn!("refusing to open hyperlink with disallowed scheme: {uri}"); return; } - if let Err(err) = std::process::Command::new("open").arg(uri).spawn() { - log::warn!("failed to open hyperlink {uri}: {err}"); - } + // Wait on a detached thread, mirroring `open_path`: a dropped `Child` is + // never reaped by std, so a fire-and-forget `spawn()` here left one + // zombie per opened link for the life of the process (B06, 2026-09 + // audit). `open` returns as soon as the handler is launched, so the + // thread is short-lived; the main thread must not block on it. + let uri = uri.to_owned(); + std::thread::spawn( + move || match std::process::Command::new("open").arg(&uri).status() { + Ok(status) if !status.success() => { + log::warn!("`open` failed for hyperlink {uri} ({status})"); + } + Ok(_) => {} + Err(err) => log::warn!("failed to open hyperlink {uri}: {err}"), + }, + ); } /// Open `path` (already resolved to an absolute filesystem path) with the diff --git a/crates/noa-app/src/session.rs b/crates/noa-app/src/session.rs index 43e4695a..9195b039 100644 --- a/crates/noa-app/src/session.rs +++ b/crates/noa-app/src/session.rs @@ -483,10 +483,26 @@ pub fn save(path: &Path, state: &SessionState) -> std::io::Result<()> { /// Load and parse the session at `path`, or `None` if it is absent, unreadable, /// malformed, or a different schema version. pub fn load(path: &Path) -> Option { + // Refuse to even read an implausibly large file: `load` runs on every + // launch before `window-save-state` is consulted (the scrollback GC needs + // the referenced keys either way), so a corrupt or hostile file must fail + // as a plain "no session" rather than exhaust memory (B05, 2026-09 audit). + let len = fs::metadata(path).ok()?.len(); + if len > MAX_SESSION_FILE_BYTES { + log::warn!( + "session: ignoring {} ({len} bytes exceeds the {MAX_SESSION_FILE_BYTES}-byte limit)", + path.display() + ); + return None; + } let source = fs::read_to_string(path).ok()?; parse(&source) } +/// Upper bound on a session file `load` will read. A real session is a few +/// KiB per pane; this leaves three orders of magnitude of headroom. +const MAX_SESSION_FILE_BYTES: u64 = 16 * 1024 * 1024; + /// A minimal JSON value model + recursive-descent parser, scoped to what the /// session schema needs. Not a general JSON library — no number exponent /// corner cases beyond what `f64::from_str` accepts, and objects preserve @@ -559,6 +575,7 @@ mod json { let mut parser = Parser { chars: source.chars().collect(), pos: 0, + depth: 0, }; parser.skip_whitespace(); let value = parser.parse_value()?; @@ -570,9 +587,17 @@ mod json { Some(value) } + /// Nesting cap for arrays/objects. The session schema nests a handful of + /// levels plus one per split; a document deeper than this is not a + /// session, and the recursive-descent parser must return `None` instead + /// of recursing until the stack overflows (an abort, not a panic, so it + /// would take the whole launch down). + const MAX_DEPTH: usize = 128; + struct Parser { chars: Vec, pos: usize, + depth: usize, } impl Parser { @@ -595,8 +620,8 @@ mod json { fn parse_value(&mut self) -> Option { self.skip_whitespace(); match self.peek()? { - '{' => self.parse_object(), - '[' => self.parse_array(), + '{' => self.nested(Self::parse_object), + '[' => self.nested(Self::parse_array), '"' => self.parse_string().map(Value::String), 't' | 'f' => self.parse_bool(), 'n' => self.parse_null(), @@ -604,6 +629,16 @@ mod json { } } + fn nested(&mut self, parse: fn(&mut Self) -> Option) -> Option { + if self.depth >= MAX_DEPTH { + return None; + } + self.depth += 1; + let value = parse(self); + self.depth -= 1; + value + } + fn parse_object(&mut self) -> Option { self.expect('{')?; let mut entries = Vec::new(); @@ -739,6 +774,28 @@ use json::ObjectExt; mod tests { use super::*; + #[test] + fn deeply_nested_json_is_rejected_not_overflowed() { + let deep = "[".repeat(100_000); + assert!(json::parse(&deep).is_none()); + let deep_closed = format!("{}{}", "[".repeat(100_000), "]".repeat(100_000)); + assert!(json::parse(&deep_closed).is_none()); + // Realistic nesting still parses. + let ok = format!("{}1{}", "[".repeat(50), "]".repeat(50)); + assert!(json::parse(&ok).is_some()); + } + + #[test] + fn oversized_session_file_is_ignored() { + let dir = std::env::temp_dir().join(format!("noa-session-oversize-{}", std::process::id())); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("session.json"); + let file = fs::File::create(&path).unwrap(); + file.set_len(MAX_SESSION_FILE_BYTES + 1).unwrap(); + assert!(load(&path).is_none()); + fs::remove_dir_all(&dir).ok(); + } + fn sample() -> SessionState { SessionState { focused_window: Some(1), diff --git a/crates/noa-config/src/lib.rs b/crates/noa-config/src/lib.rs index 0b28264f..64dcd309 100644 --- a/crates/noa-config/src/lib.rs +++ b/crates/noa-config/src/lib.rs @@ -1762,6 +1762,17 @@ pub fn scrollback_dir_in(data_dir: &Path) -> PathBuf { data_dir.join("noa").join("scrollback") } +/// Every file the config at `path` includes via `config-file`, directly or +/// transitively (optional includes that don't exist yet included), so a +/// live-reload watcher can track edits to split-out config files. An +/// unreadable `path` yields an empty list. +pub fn config_include_paths(path: &Path) -> Vec { + match fs::read_to_string(path) { + Ok(source) => parser::included_file_paths(path, &source), + Err(_) => Vec::new(), + } +} + pub fn load_overrides_from_path(path: &Path) -> anyhow::Result<(ConfigOverrides, Vec)> { let source = fs::read_to_string(path) .with_context(|| format!("failed to read config file {}", path.display()))?; diff --git a/crates/noa-config/src/parser.rs b/crates/noa-config/src/parser.rs index 56d47127..265c7076 100644 --- a/crates/noa-config/src/parser.rs +++ b/crates/noa-config/src/parser.rs @@ -6,6 +6,7 @@ mod values; pub use diagnostics::Diagnostic; pub use directives::{Directive, parse_directives}; +pub(crate) use includes::included_file_paths; pub(crate) use overrides::is_supported_scalar_key; pub use overrides::parse_overrides; diff --git a/crates/noa-config/src/parser/includes.rs b/crates/noa-config/src/parser/includes.rs index 2ad4945b..20e1c875 100644 --- a/crates/noa-config/src/parser/includes.rs +++ b/crates/noa-config/src/parser/includes.rs @@ -44,23 +44,48 @@ pub(super) fn expand_directives( let mut visited = HashSet::new(); visited.insert(canonical_or_self(path)); let mut included_count = 0usize; + let mut included_paths = Vec::new(); let directives = expand( path, source, 0, &mut visited, &mut included_count, + &mut included_paths, &mut diagnostics, ); (directives, diagnostics) } +/// Every file `path` (with contents `source`) would include, directly or +/// transitively, in expansion order — including `?`-optional includes that +/// do not exist yet, so a watcher can notice them being created. Diagnostics +/// are discarded; this is the dependency list for live reload, not a parse. +pub(crate) fn included_file_paths(path: &Path, source: &str) -> Vec { + let mut diagnostics = Vec::new(); + let mut visited = HashSet::new(); + visited.insert(canonical_or_self(path)); + let mut included_count = 0usize; + let mut included_paths = Vec::new(); + let _ = expand( + path, + source, + 0, + &mut visited, + &mut included_count, + &mut included_paths, + &mut diagnostics, + ); + included_paths +} + fn expand( path: &Path, source: &str, depth: usize, visited: &mut HashSet, included_count: &mut usize, + included_paths: &mut Vec, diagnostics: &mut Vec, ) -> Vec { let mut out = Vec::new(); @@ -115,6 +140,7 @@ fn expand( continue; } *included_count += 1; + included_paths.push(resolved.clone()); let included_source = match fs::read_to_string(&resolved) { Ok(text) => text, @@ -132,6 +158,7 @@ fn expand( depth + 1, visited, included_count, + included_paths, diagnostics, )); visited.remove(&canonical); diff --git a/crates/noa-config/src/parser/tests.rs b/crates/noa-config/src/parser/tests.rs index 5a004f89..2a0c49e3 100644 --- a/crates/noa-config/src/parser/tests.rs +++ b/crates/noa-config/src/parser/tests.rs @@ -1858,3 +1858,23 @@ fn unique_temp_dir(name: &str) -> PathBuf { .as_nanos() )) } + +#[test] +fn included_file_paths_lists_transitive_and_missing_optional_includes() { + let dir = unique_temp_dir("config-file-paths"); + fs::create_dir_all(&dir).unwrap(); + let main_path = dir.join("config"); + let child = dir.join("child"); + let grandchild = dir.join("grandchild"); + fs::write(&grandchild, "font-size = 9").unwrap(); + fs::write(&child, "config-file = grandchild").unwrap(); + fs::write(&main_path, "config-file = child\nconfig-file = ?absent").unwrap(); + + let paths = crate::config_include_paths(&main_path); + + assert_eq!( + paths, + vec![child.clone(), grandchild.clone(), dir.join("absent")] + ); + fs::remove_dir_all(&dir).unwrap(); +} diff --git a/crates/noa-config/src/writer.rs b/crates/noa-config/src/writer.rs index dc3dc063..993d3025 100644 --- a/crates/noa-config/src/writer.rs +++ b/crates/noa-config/src/writer.rs @@ -22,6 +22,10 @@ use crate::parser::parse_directives; /// earlier occurrences are left untouched. /// - A key absent from `original` is appended as a new `key = value` line at /// the end. +/// - If a `config-file` include directive appears *after* the key's last +/// occurrence, the key is additionally appended at the end so the +/// included file cannot shadow the new value (includes splice in at the +/// directive's position, so only a trailing line is guaranteed to win). /// - Every other line (comments, unknown keys, blank lines, other keys, and /// the original line order) is preserved byte-for-byte. pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { @@ -33,6 +37,17 @@ pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { let mut replacements: HashMap = HashMap::new(); let mut appended: Vec<&(String, String)> = Vec::new(); + // The reader splices an included file's directives in at the point of + // its `config-file` line, so an include *after* the key's last + // occurrence can still shadow an in-place rewrite. In that case the new + // value is also appended at the end of the file — after every include — + // so it is what the reader resolves. (B01, 2026-09 audit.) + let last_include_line = directives + .iter() + .filter(|directive| directive.key == "config-file") + .map(|directive| directive.line) + .max(); + for update @ (key, value) in updates { match directives .iter() @@ -41,6 +56,9 @@ pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { { Some(directive) => { replacements.insert(directive.line, format!("{key} = {value}")); + if last_include_line.is_some_and(|include| include > directive.line) { + appended.push(update); + } } None => appended.push(update), } @@ -260,6 +278,44 @@ theme = 3024 Day\r assert_eq!(output, "font-size = 12\nfont-size = 16\n"); } + #[test] + fn key_shadowed_by_trailing_include_is_also_appended() { + let original = "font-size = 14\nconfig-file = child.conf\n"; + + let output = apply_updates(original, &[("font-size".to_string(), "22".to_string())]); + + assert_eq!( + output, + "font-size = 22\nconfig-file = child.conf\nfont-size = 22\n" + ); + } + + #[test] + fn key_after_include_is_replaced_in_place_only() { + let original = "config-file = child.conf\nfont-size = 14\n"; + + let output = apply_updates(original, &[("font-size".to_string(), "22".to_string())]); + + assert_eq!(output, "config-file = child.conf\nfont-size = 22\n"); + } + + #[test] + fn saved_value_wins_over_trailing_include_after_reload() { + let dir = unique_temp_dir("include-shadow"); + fs::create_dir_all(&dir).unwrap(); + let main_path = dir.join("config"); + fs::write(dir.join("child.conf"), "font-size = 18\n").unwrap(); + fs::write(&main_path, "font-size = 14\nconfig-file = child.conf\n").unwrap(); + + write_config_updates(&main_path, &[("font-size".to_string(), "22".to_string())]).unwrap(); + + let source = fs::read_to_string(&main_path).unwrap(); + let (overrides, diagnostics) = crate::parse_overrides(&main_path, &source); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!(overrides.font_size, Some(22.0)); + fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn absent_key_is_appended_at_end() { let original = "window-width = 100\n"; diff --git a/crates/noa-grid/src/modes.rs b/crates/noa-grid/src/modes.rs index e6e90b9a..f595875b 100644 --- a/crates/noa-grid/src/modes.rs +++ b/crates/noa-grid/src/modes.rs @@ -57,7 +57,48 @@ impl ModeState { m } + /// Whether `(value, ansi)` is a mode noa implements. Anything else is + /// dropped by [`Self::set`] (Ghostty parity: `modes.zig` has a closed + /// enum and unknown numbers are no-ops) so a hostile stream cycling + /// through thousands of unknown `CSI ? n h` cannot grow the linear- + /// scanned set and slow every subsequent print (B04, 2026-09 audit). + /// Keep in sync with the accessors below, DECRQM in + /// `terminal/handler.rs`, and `seed.rs`'s `REPLAYED_PRIVATE_MODES`. + pub fn is_tracked(value: u16, ansi: bool) -> bool { + if ansi { + matches!(value, 4 | 20) + } else { + matches!( + value, + 1 | 6 + | 7 + | 9 + | 25 + | 47 + | 66 + | 69 + | 1000 + | 1002 + | 1003 + | 1004 + | 1005 + | 1006 + | 1007 + | 1015 + | 1047 + | 1048 + | 1049 + | 2004 + | 2026 + | 2027 + ) + } + } + pub fn set(&mut self, value: u16, ansi: bool, on: bool) { + if !Self::is_tracked(value, ansi) { + return; + } // Mouse-format modes displace each other: setting one clears the // others, and resetting a non-active one leaves the active format // untouched (matching xterm's single extend_coords slot). @@ -167,3 +208,25 @@ impl ModeState { self.get(20, true) } } + +#[cfg(test)] +mod tests { + use super::ModeState; + + #[test] + fn unknown_modes_are_not_retained() { + let mut modes = ModeState::defaults(); + let baseline = modes.set.len(); + for value in 10_000..20_000u16 { + modes.set(value, false, true); + modes.set(value, true, true); + } + assert_eq!(modes.set.len(), baseline); + assert!(!modes.get(12_345, false)); + // Known modes still round-trip. + modes.set(2004, false, true); + assert!(modes.bracketed_paste()); + modes.set(2004, false, false); + assert!(!modes.bracketed_paste()); + } +} diff --git a/crates/noa-ipc/src/auth.rs b/crates/noa-ipc/src/auth.rs index a9f6c76c..56f19d60 100644 --- a/crates/noa-ipc/src/auth.rs +++ b/crates/noa-ipc/src/auth.rs @@ -140,28 +140,45 @@ pub fn load_or_create_token(path: &Path, configured: Option<&str>) -> io::Result } log::warn!("noa-ipc: server-token is empty; falling back to generated token file"); } - if let Ok(existing) = fs::read_to_string(path) { - let trimmed = existing.trim(); - if !trimmed.is_empty() { - repair_token_file_permissions(path); - return Ok(trimmed.to_string()); - } - // R-1: the file exists but is empty, so we fall through to - // regenerate into it below. `OpenOptions::mode(0o600)` in - // `write_token_file` only applies at file *creation*; an existing - // file (e.g. left at 0644 by a restrictive umask never being in - // effect) keeps its old mode across a truncate+write. Repair perms - // now so the freshly generated secret is never written into a - // world/group-readable file, even momentarily. - repair_token_file_permissions(path); + if let Some(existing) = read_existing_token(path) { + return Ok(existing); } if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; } + // Publish the freshly generated token with a create-if-absent link + // rather than a truncating write: two noa processes initializing the + // same config dir at once (B03, 2026-09 audit) must end up agreeing on + // one token, whichever of them wins the `bind` later. The first to link + // owns the file; the loser discards its candidate and adopts the + // winner's. The file is never observable empty or half-written because + // the bytes land in a private temp file before the link. let token = generate_token(); - write_token_file(path, &token)?; + match publish_token_file(path, &token) { + Ok(()) => { + repair_token_file_permissions(path); + Ok(token) + } + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => read_existing_token(path) + .ok_or_else(|| { + io::Error::other("token file appeared during creation but could not be read") + }), + Err(err) => Err(err), + } +} + +/// The token in `path`, if the file exists and holds one. An existing +/// *empty* file is removed (R-1) so `publish_token_file` can create a fresh +/// 0600 file in its place; `OpenOptions::mode` only applies at creation. +fn read_existing_token(path: &Path) -> Option { + let existing = fs::read_to_string(path).ok()?; + let trimmed = existing.trim(); + if trimmed.is_empty() { + let _ = fs::remove_file(path); + return None; + } repair_token_file_permissions(path); - Ok(token) + Some(trimmed.to_string()) } /// Repairs (not rejects) an existing token file's permissions on unix if @@ -200,20 +217,48 @@ fn generate_token() -> String { bytes.iter().map(|b| format!("{b:02x}")).collect() } +/// Writes `token` to a private temp file next to `path` and links it into +/// place with a create-if-absent semantics: fails with `AlreadyExists` (temp +/// file removed) when another process published first. #[cfg(unix)] -fn write_token_file(path: &Path, token: &str) -> io::Result<()> { +fn publish_token_file(path: &Path, token: &str) -> io::Result<()> { use std::io::Write; use std::os::unix::fs::OpenOptionsExt; - let mut file = fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .open(path)?; - file.write_all(token.as_bytes()) + let tmp = staging_path(path); + let result = (|| { + let mut file = fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&tmp)?; + file.write_all(token.as_bytes())?; + file.sync_all()?; + fs::hard_link(&tmp, path) + })(); + let _ = fs::remove_file(&tmp); + result } #[cfg(not(unix))] -fn write_token_file(path: &Path, token: &str) -> io::Result<()> { - fs::write(path, token) +fn publish_token_file(path: &Path, token: &str) -> io::Result<()> { + let tmp = staging_path(path); + let result = fs::write(&tmp, token).and_then(|()| fs::hard_link(&tmp, path)); + let _ = fs::remove_file(&tmp); + result +} + +/// Per-process, per-thread-unique staging name so concurrent publishers +/// never share a temp file. +fn staging_path(path: &Path) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + let name = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "server-token".to_string()); + path.with_file_name(format!( + ".{name}.{}.{}.tmp", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )) } diff --git a/crates/noa-ipc/tests/token_tests.rs b/crates/noa-ipc/tests/token_tests.rs index 53734dfb..eb02c12d 100644 --- a/crates/noa-ipc/tests/token_tests.rs +++ b/crates/noa-ipc/tests/token_tests.rs @@ -151,3 +151,46 @@ fn whitespace_only_configured_token_falls_back_to_generated_file_token() { std::fs::remove_dir_all(&dir).ok(); } + +// ---- B03: concurrent first-run provisioning must converge on one token ---- + +#[test] +fn concurrent_provisioning_agrees_with_the_file() { + let dir = std::env::temp_dir().join(format!( + "noa-ipc-token-test-concurrent-{}", + std::process::id() + )); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("server-token"); + let _ = std::fs::remove_file(&path); + + let barrier = std::sync::Arc::new(std::sync::Barrier::new(8)); + let handles: Vec<_> = (0..8) + .map(|_| { + let path = path.clone(); + let barrier = barrier.clone(); + std::thread::spawn(move || { + barrier.wait(); + load_or_create_token(&path, None).unwrap() + }) + }) + .collect(); + let tokens: Vec = handles.into_iter().map(|h| h.join().unwrap()).collect(); + + let on_disk = std::fs::read_to_string(&path).unwrap(); + for token in &tokens { + assert_eq!( + token, + on_disk.trim(), + "every caller must return the published token" + ); + } + assert!( + std::fs::read_dir(&dir) + .unwrap() + .all(|e| e.unwrap().file_name() == "server-token"), + "no staging files left behind" + ); + + std::fs::remove_dir_all(&dir).ok(); +} From c2aa079f8f2a09218d024cc319795452ed69a3f0 Mon Sep 17 00:00:00 2001 From: simota Date: Sun, 6 Sep 2026 20:14:33 +0900 Subject: [PATCH 2/2] fix: harden audit follow-ups for include watching, repeatable keys, and token staging - config_reload: re-derive include watch list on every detected change so includes added/removed by an edit whose reload fails are still tracked - writer: skip the post-include append for repeatable keys (font-family, palette, keybind, ...) so consecutive saves do not accumulate stale entries - ipc auth: retry the staging temp file on AlreadyExists (stale file from a reused PID) and unify the unix/non-unix publish path Claude-Session: https://claude.ai/code/session_01GBe292wmocEXM8SjBzjGq7 --- crates/noa-app/src/app/config_reload.rs | 78 ++++++++++++++++++- crates/noa-config/src/writer.rs | 78 +++++++++++++++++-- crates/noa-ipc/src/auth.rs | 32 ++++---- .../noa-ipc/tests/token_staging_collision.rs | 38 +++++++++ 4 files changed, 201 insertions(+), 25 deletions(-) create mode 100644 crates/noa-ipc/tests/token_staging_collision.rs diff --git a/crates/noa-app/src/app/config_reload.rs b/crates/noa-app/src/app/config_reload.rs index 772f970d..9ab47caa 100644 --- a/crates/noa-app/src/app/config_reload.rs +++ b/crates/noa-app/src/app/config_reload.rs @@ -19,11 +19,10 @@ pub(super) struct ConfigWatcher { path: Option, signature: Option, /// Files pulled in via `config-file` (transitively, optional-and-absent - /// ones included) with the signature seen at the last reload. Editing a + /// ones included) with the last observed signature. Editing a /// split-out child config must reload just like editing the main file - /// (B02, 2026-09 audit). The list is re-derived on every reload, so an - /// include added or removed in a watched file is picked up on the next - /// reload that edit triggers. + /// (B02, 2026-09 audit). The list is re-derived whenever a watched file + /// changes, even if the resulting configuration cannot be applied. includes: Vec<(PathBuf, Option)>, next_check: Option, } @@ -91,6 +90,9 @@ impl ConfigWatcher { } } if changed { + // Dependencies belong to the files on disk, even when validation + // fails and the application keeps its previous configuration. + self.includes = include_signatures(path); ConfigWatchTick::Changed(next) } else { ConfigWatchTick::Waiting(next) @@ -888,6 +890,74 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + #[test] + fn watcher_detects_new_include_edits_after_failed_reload() { + let dir = temp_config_path("failed-reload"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config"); + let child = dir.join("child.conf"); + let grandchild = dir.join("grandchild.conf"); + let legacy = dir.join("config.toml"); + fs::write(&path, "config-file = child.conf\n").unwrap(); + fs::write(&child, "font-size = 18\n").unwrap(); + let load = || { + noa_config::load_startup_config_from( + &path, + &legacy, + noa_config::ConfigOverrides::default(), + ) + }; + assert_eq!(load().unwrap().0.font_size, 18.0); + let mut watcher = ConfigWatcher::with_path(Some(path.clone())); + let mut now = Instant::now() + CONFIG_WATCH_INTERVAL; + + fs::write( + &grandchild, + format!( + "client-token-file = {}\n", + dir.join("missing-token").display() + ), + ) + .unwrap(); + fs::write(&child, "font-size = 18\nconfig-file = grandchild.conf\n").unwrap(); + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + assert!( + load() + .unwrap_err() + .to_string() + .contains("client-token-file") + ); + + // A failed reload does not call mark_current. Without another edit + // it should stay idle, while still watching the newly added child. + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Waiting(_))); + fs::write(&grandchild, "font-size = 22\n").unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + assert_eq!(load().unwrap().0.font_size, 22.0); + watcher.mark_current(); + + // Removing the include must also remove it from the watch list, + // even when the replacement configuration still fails to load. + fs::write( + &child, + format!( + "client-token-file = {}\n", + dir.join("missing-token").display() + ), + ) + .unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Changed(_))); + assert!(load().is_err()); + fs::write(&grandchild, "font-size = 24\n\n").unwrap(); + now += CONFIG_WATCH_INTERVAL; + assert!(matches!(watcher.tick(now), ConfigWatchTick::Waiting(_))); + + fs::remove_dir_all(&dir).unwrap(); + } + /// `expedite` (focus gain / settings-panel commit) must make the very /// next tick stat the file instead of waiting out the slow /// `CONFIG_WATCH_INTERVAL`, and must only re-time the pending check — diff --git a/crates/noa-config/src/writer.rs b/crates/noa-config/src/writer.rs index 993d3025..d3003574 100644 --- a/crates/noa-config/src/writer.rs +++ b/crates/noa-config/src/writer.rs @@ -18,11 +18,11 @@ use crate::parser::parse_directives; /// /// - A key already present is rewritten in place as `key = value`; if the /// key occurs on multiple lines (duplicate directives), only the **last** -/// occurrence is replaced (Ghostty's last-wins resolution semantics) — -/// earlier occurrences are left untouched. +/// occurrence is replaced; earlier occurrences are left untouched. This +/// preserves last-wins scalar resolution and intentional repeatable entries. /// - A key absent from `original` is appended as a new `key = value` line at /// the end. -/// - If a `config-file` include directive appears *after* the key's last +/// - If a `config-file` include directive appears *after* a scalar key's last /// occurrence, the key is additionally appended at the end so the /// included file cannot shadow the new value (includes splice in at the /// directive's position, so only a trailing line is guaranteed to win). @@ -40,8 +40,9 @@ pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { // The reader splices an included file's directives in at the point of // its `config-file` line, so an include *after* the key's last // occurrence can still shadow an in-place rewrite. In that case the new - // value is also appended at the end of the file — after every include — - // so it is what the reader resolves. (B01, 2026-09 audit.) + // scalar value is also appended after every include. Repeatable keys + // accumulate instead, so appending them would leave stale entries in + // the list on subsequent saves. let last_include_line = directives .iter() .filter(|directive| directive.key == "config-file") @@ -56,7 +57,9 @@ pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { { Some(directive) => { replacements.insert(directive.line, format!("{key} = {value}")); - if last_include_line.is_some_and(|include| include > directive.line) { + if !is_repeatable_key(key) + && last_include_line.is_some_and(|include| include > directive.line) + { appended.push(update); } } @@ -95,6 +98,24 @@ pub fn apply_updates(original: &str, updates: &[(String, String)]) -> String { output } +fn is_repeatable_key(key: &str) -> bool { + matches!( + key, + "font-family" + | "font-family-bold" + | "font-family-italic" + | "font-family-bold-italic" + | "font-feature" + | "font-variation" + | "font-variation-bold" + | "font-variation-italic" + | "font-variation-bold-italic" + | "palette" + | "keybind" + | "config-file" + ) +} + /// Splits `text` into `(content, terminator)` pairs, where `terminator` is /// `"\r\n"`, `"\n"`, or `""` (only the final line, when `text` has no /// trailing newline). Unlike [`str::lines`], the terminator survives per @@ -290,6 +311,51 @@ theme = 3024 Day\r ); } + #[test] + fn repeatable_keys_are_not_duplicated_after_include() { + for (key, first, second) in [ + ("font-family", "Menlo", "Monaco"), + ("font-family-bold", "Menlo", "Monaco"), + ("font-family-italic", "Menlo", "Monaco"), + ("font-family-bold-italic", "Menlo", "Monaco"), + ("font-feature", "calt", "-liga"), + ("font-variation", "wght=400", "wght=500"), + ("font-variation-bold", "wght=600", "wght=700"), + ("font-variation-italic", "slnt=-5", "slnt=-10"), + ("font-variation-bold-italic", "wght=600", "wght=700"), + ("palette", "0=#000000", "1=#ffffff"), + ("keybind", "cmd+t=tab.new", "cmd+w=close_surface"), + ] { + let original = format!("{key} = {first}\nconfig-file = child.conf\n"); + let first_save = apply_updates(&original, &[(key.into(), first.into())]); + let second_save = apply_updates(&first_save, &[(key.into(), second.into())]); + assert_eq!( + second_save, + format!("{key} = {second}\nconfig-file = child.conf\n"), + "{key}" + ); + } + } + + #[test] + fn consecutive_font_saves_before_include_use_the_latest_family() { + let dir = unique_temp_dir("font-include"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config"); + fs::write(dir.join("child.conf"), "font-size = 18\n").unwrap(); + fs::write(&path, "font-family = Courier\nconfig-file = child.conf\n").unwrap(); + + for family in ["Menlo", "Monaco"] { + write_config_updates(&path, &[("font-family".into(), family.into())]).unwrap(); + } + + let source = fs::read_to_string(&path).unwrap(); + let (overrides, diagnostics) = crate::parse_overrides(&path, &source); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!(overrides.font.families, ["Monaco"]); + fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn key_after_include_is_replaced_in_place_only() { let original = "config-file = child.conf\nfont-size = 14\n"; diff --git a/crates/noa-ipc/src/auth.rs b/crates/noa-ipc/src/auth.rs index 56f19d60..872a6e9d 100644 --- a/crates/noa-ipc/src/auth.rs +++ b/crates/noa-ipc/src/auth.rs @@ -220,29 +220,31 @@ fn generate_token() -> String { /// Writes `token` to a private temp file next to `path` and links it into /// place with a create-if-absent semantics: fails with `AlreadyExists` (temp /// file removed) when another process published first. -#[cfg(unix)] fn publish_token_file(path: &Path, token: &str) -> io::Result<()> { use std::io::Write; + #[cfg(unix)] use std::os::unix::fs::OpenOptionsExt; - let tmp = staging_path(path); + + let (tmp, mut file) = loop { + let tmp = staging_path(path); + let mut options = fs::OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + options.mode(0o600); + match options.open(&tmp) { + Ok(file) => break (tmp, file), + // A previous process with a reused PID may have left this name + // behind. Only a collision at the final link is a competing token. + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, + Err(err) => return Err(err), + } + }; let result = (|| { - let mut file = fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(0o600) - .open(&tmp)?; file.write_all(token.as_bytes())?; file.sync_all()?; fs::hard_link(&tmp, path) })(); - let _ = fs::remove_file(&tmp); - result -} - -#[cfg(not(unix))] -fn publish_token_file(path: &Path, token: &str) -> io::Result<()> { - let tmp = staging_path(path); - let result = fs::write(&tmp, token).and_then(|()| fs::hard_link(&tmp, path)); + drop(file); let _ = fs::remove_file(&tmp); result } diff --git a/crates/noa-ipc/tests/token_staging_collision.rs b/crates/noa-ipc/tests/token_staging_collision.rs new file mode 100644 index 00000000..064bad19 --- /dev/null +++ b/crates/noa-ipc/tests/token_staging_collision.rs @@ -0,0 +1,38 @@ +//! Kept in a separate test binary so the first provisioning attempt uses +//! staging sequence zero, independent of other token tests running in parallel. + +use noa_ipc::load_or_create_token; + +#[test] +fn stale_staging_files_do_not_block_token_provisioning() { + let pid = std::process::id(); + let dir = std::env::temp_dir().join(format!("noa-ipc-stale-staging-{pid}")); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("server-token"); + let _ = std::fs::remove_file(&path); + let stale_paths: Vec<_> = (0..3) + .map(|seq| dir.join(format!(".server-token.{pid}.{seq}.tmp"))) + .collect(); + for stale in &stale_paths { + std::fs::write(stale, "stale-candidate").unwrap(); + } + + let token = load_or_create_token(&path, None).unwrap(); + assert_eq!(token.len(), 64); + assert!(token.bytes().all(|byte| byte.is_ascii_hexdigit())); + assert_eq!(std::fs::read_to_string(&path).unwrap(), token); + assert_eq!(load_or_create_token(&path, None).unwrap(), token); + for stale in &stale_paths { + assert_eq!(std::fs::read_to_string(stale).unwrap(), "stale-candidate"); + } + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 4); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } + std::fs::remove_dir_all(&dir).unwrap(); +}