From 00a4282f194dbb860859689c34db281bdc5e70ee Mon Sep 17 00:00:00 2001 From: Lahfir <70215676+lahfir@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:12:55 -0700 Subject: [PATCH] fix: write the trace html export through the user-output primitive (#113) The export hand-rolled temp+rename with a predictable {pid}.{counter}.tmp name and, on non-unix, a bare fs::write that follows reparse points. The unix arm opened the temp with create+truncate rather than create_new, so a pre-created temp at the guessable name kept its own mode while receiving the trace bytes. Routing through write_user_file adopts the hashed-nonce create_new temp, destination validation, and cleanup that screenshot and clipboard output already use. Normalizing an empty parent in write_atomic_with is required for the swap: Path::parent yields Some("") for a bare relative filename and metadata("") fails NotFound, so --out report.html would otherwise be rejected. That bug was already latent for screenshot --out and clipboard get --out. Unifies the trace-export test modules on one lock so the process-global JSON-size clamp cannot race across them. --- crates/core/src/private_file.rs | 10 ++ crates/core/src/private_file_tests.rs | 41 +++++ crates/core/src/trace_read/html.rs | 52 +----- .../src/trace_read/html_export_stats_tests.rs | 7 +- .../src/trace_read/html_screenshot_tests.rs | 7 +- crates/core/src/trace_read/html_tests.rs | 2 +- .../core/src/trace_read/html_write_tests.rs | 166 ++++++++++++++++++ 7 files changed, 226 insertions(+), 59 deletions(-) create mode 100644 crates/core/src/trace_read/html_write_tests.rs diff --git a/crates/core/src/private_file.rs b/crates/core/src/private_file.rs index da7ae68..0222e9c 100644 --- a/crates/core/src/private_file.rs +++ b/crates/core/src/private_file.rs @@ -98,6 +98,11 @@ pub(crate) fn write_user_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<() ) } +/// `Path::parent` yields `Some("")` for a bare relative filename, and both +/// `std::fs::metadata("")` and opening `""` fail with `NotFound`, so the empty +/// parent is normalized to the current directory before `ensure_parent` and +/// `sync_parent` see it; without that, a legitimate relative output path such +/// as `--out report.html` is rejected. fn write_atomic_with( path: &Path, bytes: &[u8], @@ -108,6 +113,11 @@ fn write_atomic_with( let parent = path .parent() .ok_or_else(|| invalid_input("private file path has no parent"))?; + let parent = if parent.as_os_str().is_empty() { + Path::new(".") + } else { + parent + }; ensure_parent(parent)?; validate_destination(path)?; let (temporary, mut file) = create_temporary(path)?; diff --git a/crates/core/src/private_file_tests.rs b/crates/core/src/private_file_tests.rs index 293b16d..976e657 100644 --- a/crates/core/src/private_file_tests.rs +++ b/crates/core/src/private_file_tests.rs @@ -7,6 +7,34 @@ use std::os::fd::AsRawFd; use std::os::unix::fs::OpenOptionsExt; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; +use std::sync::{Mutex, MutexGuard}; + +static CURRENT_DIRECTORY_LOCK: Mutex<()> = Mutex::new(()); + +struct CurrentDirectoryGuard { + previous: PathBuf, + _lock: MutexGuard<'static, ()>, +} + +impl CurrentDirectoryGuard { + fn enter(target: &Path) -> Self { + let lock = CURRENT_DIRECTORY_LOCK + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let previous = std::env::current_dir().unwrap(); + std::env::set_current_dir(target).unwrap(); + Self { + previous, + _lock: lock, + } + } +} + +impl Drop for CurrentDirectoryGuard { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.previous); + } +} fn directory(label: &str) -> PathBuf { let path = std::env::temp_dir().join(format!( @@ -241,6 +269,19 @@ fn private_lock_creates_and_reopens_the_lock_file() { std::fs::remove_dir_all(directory).unwrap(); } +#[test] +fn user_write_accepts_a_bare_relative_filename() { + let directory = directory("bare-relative"); + let guard = CurrentDirectoryGuard::enter(&directory); + + write_user_atomic(Path::new("bare-name.txt"), b"relative").unwrap(); + + assert_eq!(std::fs::read("bare-name.txt").unwrap(), b"relative"); + assert!(directory.join("bare-name.txt").is_file()); + drop(guard); + std::fs::remove_dir_all(directory).unwrap(); +} + #[test] fn ensure_private_creates_the_nested_parent_chain() { let directory = directory("nested-parents"); diff --git a/crates/core/src/trace_read/html.rs b/crates/core/src/trace_read/html.rs index a8c41ac..a7eefe2 100644 --- a/crates/core/src/trace_read/html.rs +++ b/crates/core/src/trace_read/html.rs @@ -5,6 +5,7 @@ use base64::Engine; use serde_json::{Value, json}; use std::collections::HashSet; use std::path::{Path, PathBuf}; +#[cfg(test)] use std::sync::atomic::{AtomicU64, Ordering}; const VIEWER_HTML: &str = include_str!("viewer.html"); @@ -15,8 +16,6 @@ pub const TRACE_EXPORT_DEFAULT_LIMIT: usize = 5000; const MAX_EMBED_SCREENSHOT_BYTES: u64 = 100 * 1024 * 1024; const MAX_JSON_BYTES: u64 = 200 * 1024 * 1024; -static EXPORT_TMP_COUNTER: AtomicU64 = AtomicU64::new(0); - #[cfg(test)] static TEST_MAX_JSON_BYTES: AtomicU64 = AtomicU64::new(0); @@ -118,7 +117,7 @@ pub fn export_html( .join(format!("trace-{session_id}.html")) }); - write_export_file(&path, html.as_bytes())?; + crate::refs::write_user_file(&path, html.as_bytes())?; let bytes = html.len(); let warnings: Vec = merged @@ -211,49 +210,6 @@ fn escape_for_json_island(json: &str) -> String { out } -fn write_export_file(path: &Path, bytes: &[u8]) -> Result<(), AppError> { - if path.is_symlink() { - return Err(AppError::invalid_input_with_suggestion( - "Refusing to write trace export through a symlink", - "Choose a different --out path.", - )); - } - if let Some(parent) = path.parent() { - if !parent.as_os_str().is_empty() { - std::fs::create_dir_all(parent)?; - } - } - let unique = EXPORT_TMP_COUNTER.fetch_add(1, Ordering::Relaxed); - let tmp = path.with_extension(format!("{}.{unique}.tmp", std::process::id())); - let result = write_export_tmp_then_rename(&tmp, path, bytes); - if result.is_err() { - let _ = std::fs::remove_file(&tmp); - } - result -} - -fn write_export_tmp_then_rename(tmp: &Path, path: &Path, bytes: &[u8]) -> Result<(), AppError> { - #[cfg(unix)] - { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut file = std::fs::OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .mode(0o600) - .custom_flags(libc::O_NOFOLLOW) - .open(tmp)?; - file.write_all(bytes)?; - file.flush()?; - } - #[cfg(not(unix))] - std::fs::write(tmp, bytes)?; - - std::fs::rename(tmp, path)?; - Ok(()) -} - #[cfg(test)] #[path = "html_tests.rs"] mod tests; @@ -265,3 +221,7 @@ mod screenshot_tests; #[cfg(test)] #[path = "html_export_stats_tests.rs"] mod export_stats_tests; + +#[cfg(test)] +#[path = "html_write_tests.rs"] +mod write_tests; diff --git a/crates/core/src/trace_read/html_export_stats_tests.rs b/crates/core/src/trace_read/html_export_stats_tests.rs index ada7b59..47830d5 100644 --- a/crates/core/src/trace_read/html_export_stats_tests.rs +++ b/crates/core/src/trace_read/html_export_stats_tests.rs @@ -4,9 +4,6 @@ use crate::trace_read::{ExportOptions, export_html}; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use std::sync::Mutex; - -static HTML_EXPORT_STATS_TEST_LOCK: Mutex<()> = Mutex::new(()); fn write_segment(trace_dir: &Path, name: &str, lines: &[&str]) { fs::create_dir_all(trace_dir).unwrap(); @@ -22,9 +19,7 @@ fn setup_trace_session() -> ( String, PathBuf, ) { - let lock = HTML_EXPORT_STATS_TEST_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); + let lock = super::tests::html_test_guard(); super::clear_test_max_json_bytes(); let home = HomeGuard::new(); let manifest = start_session(StartSessionOptions { diff --git a/crates/core/src/trace_read/html_screenshot_tests.rs b/crates/core/src/trace_read/html_screenshot_tests.rs index 344e442..8daf863 100644 --- a/crates/core/src/trace_read/html_screenshot_tests.rs +++ b/crates/core/src/trace_read/html_screenshot_tests.rs @@ -5,9 +5,6 @@ use serde_json::Value; use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; -use std::sync::Mutex; - -static HTML_SCREENSHOT_TEST_LOCK: Mutex<()> = Mutex::new(()); fn write_segment(trace_dir: &Path, name: &str, lines: &[&str]) { fs::create_dir_all(trace_dir).unwrap(); @@ -23,9 +20,7 @@ fn setup_trace_session() -> ( String, PathBuf, ) { - let lock = HTML_SCREENSHOT_TEST_LOCK - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); + let lock = super::tests::html_test_guard(); super::clear_test_max_json_bytes(); let home = HomeGuard::new(); let manifest = start_session(StartSessionOptions { diff --git a/crates/core/src/trace_read/html_tests.rs b/crates/core/src/trace_read/html_tests.rs index aa21aeb..42a01d5 100644 --- a/crates/core/src/trace_read/html_tests.rs +++ b/crates/core/src/trace_read/html_tests.rs @@ -10,7 +10,7 @@ use std::sync::Mutex; static HTML_TEST_LOCK: Mutex<()> = Mutex::new(()); -fn html_test_guard() -> std::sync::MutexGuard<'static, ()> { +pub(super) fn html_test_guard() -> std::sync::MutexGuard<'static, ()> { HTML_TEST_LOCK .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()) diff --git a/crates/core/src/trace_read/html_write_tests.rs b/crates/core/src/trace_read/html_write_tests.rs new file mode 100644 index 0000000..9ebd9af --- /dev/null +++ b/crates/core/src/trace_read/html_write_tests.rs @@ -0,0 +1,166 @@ +use crate::trace_read::{ExportOptions, export_html}; +use std::fs; +use std::io::Write; +use std::path::{Path, PathBuf}; + +const SEGMENT_LINE: &str = r#"{"event":"command.start","command":"snapshot","ts_ms":1,"seq":1}"#; +const SESSION_ID: &str = "s-export-write"; +const SENTINEL: &[u8] = b"planted-sentinel"; +const LEGACY_COUNTER_RANGE: u64 = 16; + +fn export_test_guard() -> std::sync::MutexGuard<'static, ()> { + let lock = super::tests::html_test_guard(); + super::clear_test_max_json_bytes(); + lock +} + +fn export_scratch(label: &str) -> PathBuf { + let path = std::env::temp_dir().join(format!( + "agent-desktop-export-{label}-{}", + crate::refs::new_snapshot_id() + )); + fs::create_dir_all(&path).unwrap(); + path +} + +fn trace_dir_with_one_event(root: &Path) -> PathBuf { + let trace_dir = root.join("trace"); + fs::create_dir_all(&trace_dir).unwrap(); + let mut file = fs::File::create(trace_dir.join("100-1000.jsonl")).unwrap(); + writeln!(file, "{SEGMENT_LINE}").unwrap(); + trace_dir +} + +fn export_to(trace_dir: &Path, out: &Path) -> Result<(), crate::AppError> { + export_html( + trace_dir, + SESSION_ID, + &ExportOptions { + limit: 0, + out: Some(out.to_path_buf()), + }, + ) + .map(|_| ()) +} + +fn legacy_temporary_path(out: &Path, counter: u64) -> PathBuf { + out.with_extension(format!("{}.{counter}.tmp", std::process::id())) +} + +#[test] +fn export_refuses_a_directory_destination() { + let _guard = export_test_guard(); + let root = export_scratch("directory-destination"); + let trace_dir = trace_dir_with_one_event(&root); + let out = root.join("report.html"); + fs::create_dir(&out).unwrap(); + + let error = export_to(&trace_dir, &out).unwrap_err(); + + assert_eq!(error.code(), "INVALID_ARGS"); + assert!(out.is_dir()); + fs::remove_dir_all(root).unwrap(); +} + +#[cfg(unix)] +#[test] +fn export_refuses_a_symlink_destination() { + let _guard = export_test_guard(); + let root = export_scratch("symlink-destination"); + let trace_dir = trace_dir_with_one_event(&root); + let target = root.join("target.html"); + fs::write(&target, b"kept").unwrap(); + let out = root.join("report.html"); + std::os::unix::fs::symlink(&target, &out).unwrap(); + + let error = export_to(&trace_dir, &out).unwrap_err(); + + assert_eq!(error.code(), "INVALID_ARGS"); + assert_eq!(fs::read(&target).unwrap(), b"kept"); + assert!(out.is_symlink()); + fs::remove_dir_all(root).unwrap(); +} + +#[test] +fn export_does_not_write_through_the_legacy_predictable_temporary_name() { + let _guard = export_test_guard(); + let root = export_scratch("legacy-temporary"); + let trace_dir = trace_dir_with_one_event(&root); + let out = root.join("report.html"); + let planted: Vec = (0..LEGACY_COUNTER_RANGE) + .map(|counter| legacy_temporary_path(&out, counter)) + .collect(); + for path in &planted { + fs::write(path, SENTINEL).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + fs::set_permissions(path, fs::Permissions::from_mode(0o666)).unwrap(); + } + } + + export_to(&trace_dir, &out).unwrap(); + + assert!(out.is_file()); + for path in &planted { + assert!( + path.is_file(), + "the export consumed the predictable temporary name {}", + path.display() + ); + assert_eq!( + fs::read(path).unwrap(), + SENTINEL, + "the export overwrote the predictable temporary name {}", + path.display() + ); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + fs::metadata(path).unwrap().permissions().mode() & 0o777, + 0o666, + "the export re-permissioned the predictable temporary name {}", + path.display() + ); + } + } + fs::remove_dir_all(root).unwrap(); +} + +#[test] +fn a_successful_export_leaves_no_temporary_residue() { + let _guard = export_test_guard(); + let root = export_scratch("no-residue"); + let trace_dir = trace_dir_with_one_event(&root); + let out = root.join("report.html"); + + export_to(&trace_dir, &out).unwrap(); + + let mut names: Vec = fs::read_dir(&root) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + assert_eq!(names, vec!["report.html".to_string(), "trace".to_string()]); + fs::remove_dir_all(root).unwrap(); +} + +#[cfg(unix)] +#[test] +fn an_exported_file_lands_owner_only_under_an_explicit_out_path() { + use std::os::unix::fs::PermissionsExt; + let _guard = export_test_guard(); + let root = export_scratch("owner-only"); + fs::set_permissions(&root, fs::Permissions::from_mode(0o777)).unwrap(); + let trace_dir = trace_dir_with_one_event(&root); + let out = root.join("report.html"); + + export_to(&trace_dir, &out).unwrap(); + + assert_eq!( + fs::metadata(&out).unwrap().permissions().mode() & 0o777, + 0o600 + ); + fs::remove_dir_all(root).unwrap(); +}