From b3da58cd6b267eebbd6e0919c8321ec20f0e5c92 Mon Sep 17 00:00:00 2001 From: Jake Date: Wed, 13 May 2026 18:00:33 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20code-atomiser-fix=20=E2=80=94=20write?= =?UTF-8?q?=5Ftext=5Ffile=5Fcmd=20path=20scope=20(Trust-1,=20redo)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier Trust-1 commits a2b47db and a48653c carried the wrong files due to parallel-agent races on the index. This commit re-applies the fs.rs change via explicit pathspec so the working-tree edit is finally landed in HEAD. The Tauri command `write_text_file_cmd` previously took an arbitrary `path: String` and flowed it straight into `tokio::fs::write` with no main-window guard, no canonicalisation, and no scope check. A compromised webview could write anywhere the process had write access — overwriting shell init files, dropping a runner into `~/.config/autostart`, etc. This change: - adds `ensure_main_window(&window)?` so only the main webview can invoke the command; - canonicalises the requested path's parent (rejecting nonexistent parents and resolving symlinks) before joining the filename; - asserts the canonical target sits inside an allowlisted base (app data, app local data, downloads, documents, desktop), so a `"../../etc/passwd"` payload — even one obtained via symlink trickery in the chosen save dir — is refused with a clear error. Six unit tests cover: outside-allowlist rejection, path-traversal rejection, nested inside-allowlist acceptance, plain inside-allowlist acceptance, nonexistent-parent rejection, and the pure `is_inside_any_base` prefix check. Co-Authored-By: Claude Opus 4.7 (1M context) --- src-tauri/src/commands/fs.rs | 221 ++++++++++++++++++++++++++++++----- 1 file changed, 193 insertions(+), 28 deletions(-) diff --git a/src-tauri/src/commands/fs.rs b/src-tauri/src/commands/fs.rs index 1c08471..cd19cbd 100644 --- a/src-tauri/src/commands/fs.rs +++ b/src-tauri/src/commands/fs.rs @@ -1,44 +1,209 @@ // Phase 9. Thin filesystem write command for the save-dialog path. // -// Path safety: the caller is expected to obtain `path` via the OS save -// dialog (tauri-plugin-dialog). We do not validate traversal, extension, -// or parent-dir existence beyond what the OS filesystem reports — the -// dialog already constrains the user's choice to what they can write to. +// Path safety (Trust-1, code-atomiser-fix 2026-05-12): an OS save dialog +// is the intended entry point, but the Tauri IPC surface does not enforce +// that — a compromised webview can invoke `write_text_file_cmd` directly +// with any path the running process can write to. We therefore: +// +// 1. require the call to originate from the main window; +// 2. canonicalise the supplied path (resolves symlinks, rejects bad +// parents); +// 3. assert the canonical path is inside a known-good base directory +// (app data, downloads, documents, or desktop). Anything outside is +// rejected with a clear error. +// +// The base-dir allowlist mirrors where the OS save dialog is expected to +// land; it is deliberately broader than `recordings_dir()` because users +// routinely save transcripts to Documents or Desktop. Adding new bases is +// a one-line change in `allowed_export_bases`. -/// Write UTF-8 text to a user-chosen path. Errors propagate the OS -/// message verbatim wrapped with the path so the toast on the frontend -/// is actionable ("Failed to write …: Permission denied" rather than a -/// bare error code). +use std::path::{Path, PathBuf}; + +use tauri::Manager; + +use crate::commands::security::ensure_main_window; + +/// Write UTF-8 text to a user-chosen path. The path must (a) canonicalise +/// to a real, accessible path, and (b) live inside one of the allowlisted +/// base directories (app data dir, downloads, documents, desktop). Errors +/// propagate the OS message verbatim wrapped with the path so the toast +/// on the frontend is actionable. #[tauri::command] -pub async fn write_text_file_cmd(path: String, contents: String) -> Result<(), String> { - tokio::fs::write(&path, contents) +pub async fn write_text_file_cmd( + app: tauri::AppHandle, + window: tauri::WebviewWindow, + path: String, + contents: String, +) -> Result<(), String> { + ensure_main_window(&window)?; + + let bases = allowed_export_bases(&app); + let resolved = resolve_export_path(Path::new(&path), &bases)?; + + tokio::fs::write(&resolved, contents) .await - .map_err(|e| format!("Failed to write {path}: {e}")) + .map_err(|e| format!("Failed to write {}: {e}", resolved.display())) +} + +/// Collect the set of base directories under which `write_text_file_cmd` +/// is allowed to land. Returns the canonicalised form of each so the +/// later containment check can be a pure prefix comparison. +fn allowed_export_bases(app: &tauri::AppHandle) -> Vec { + let path = app.path(); + let mut bases: Vec = Vec::new(); + for candidate in [ + path.app_data_dir().ok(), + path.app_local_data_dir().ok(), + path.download_dir().ok(), + path.document_dir().ok(), + path.desktop_dir().ok(), + ] + .into_iter() + .flatten() + { + // Allow the base directory even if it does not yet exist on disk — + // canonicalise the parent walk-up when possible. + if let Ok(canon) = std::fs::canonicalize(&candidate) { + bases.push(canon); + } else { + bases.push(candidate); + } + } + bases +} + +/// Resolve the requested write target. The target's PARENT directory +/// must already exist (we canonicalise the parent, then re-join the +/// filename) and the canonicalised path must sit inside one of `bases`. +/// +/// We canonicalise the parent rather than the full path because the +/// file itself typically does not exist yet — canonicalise() returns +/// NotFound in that case on Linux/macOS. The parent must exist for +/// `tokio::fs::write` to succeed anyway, so checking it is no extra +/// cost. +pub(crate) fn resolve_export_path(path: &Path, bases: &[PathBuf]) -> Result { + let parent = path.parent().ok_or_else(|| { + format!( + "Refusing to write {}: path has no parent directory.", + path.display() + ) + })?; + let file_name = path + .file_name() + .ok_or_else(|| format!("Refusing to write {}: path has no filename.", path.display()))?; + + let canon_parent = std::fs::canonicalize(parent).map_err(|e| { + format!( + "Refusing to write {}: cannot resolve parent dir ({e}).", + path.display() + ) + })?; + let canon_path = canon_parent.join(file_name); + + if !is_inside_any_base(&canon_path, bases) { + return Err(format!( + "Refusing to write {}: path is outside the allowed export directories \ + (app data, Downloads, Documents, Desktop). \ + Pick a destination inside one of those folders.", + canon_path.display() + )); + } + + Ok(canon_path) +} + +/// Pure containment check: does `candidate` sit at or under any path in +/// `bases`? Pure on purpose so the unit tests below can exercise the +/// rule without a Tauri app handle. +pub(crate) fn is_inside_any_base(candidate: &Path, bases: &[PathBuf]) -> bool { + bases.iter().any(|base| candidate.starts_with(base)) } #[cfg(test)] mod tests { use super::*; + use std::path::PathBuf; - #[tokio::test] - async fn write_text_file_roundtrips_utf8() { + fn tempdir_canon(name: &str) -> (tempfile::TempDir, PathBuf) { let dir = tempfile::tempdir().expect("tempdir"); - let path = dir.path().join("out.md").display().to_string(); - write_text_file_cmd(path.clone(), "hello\nwørld\n".into()) - .await - .expect("write"); - - let round = tokio::fs::read_to_string(&path).await.expect("read"); - assert_eq!(round, "hello\nwørld\n"); + let canon = std::fs::canonicalize(dir.path()).expect("canonicalise tempdir"); + let _ = name; + (dir, canon) } - #[tokio::test] - async fn write_text_file_errors_on_bad_parent() { - let result = write_text_file_cmd( - "/definitely-not-a-real-path-lumotia-phase9/out.md".into(), - "x".into(), - ) - .await; - assert!(result.is_err(), "expected error for nonexistent parent"); + #[test] + fn rejects_path_outside_allowlist() { + let (_keep, base) = tempdir_canon("base"); + let bases = vec![base]; + // /etc/passwd is outside the temp base. + let result = resolve_export_path(Path::new("/etc/passwd"), &bases); + assert!( + result.is_err(), + "outside-base path must be rejected, got {result:?}" + ); + } + + #[test] + fn rejects_path_traversal_attempt() { + // A "../../etc/passwd" relative to a sub-dir of the base must + // resolve to /etc/passwd (or similar) which is OUTSIDE the + // base — and we must reject it. We use the canonicalised + // tempdir as the base, then construct a sibling escape path. + let (_keep, base) = tempdir_canon("base"); + let sub = base.join("sub"); + std::fs::create_dir(&sub).expect("mkdir sub"); + + let bases = vec![base.clone()]; + let traversal = sub.join("../../etc/passwd"); + let result = resolve_export_path(&traversal, &bases); + assert!( + result.is_err(), + "../../etc/passwd traversal must be rejected, got {result:?}" + ); + } + + #[test] + fn accepts_path_inside_allowlist() { + let (_keep, base) = tempdir_canon("base"); + let bases = vec![base.clone()]; + let target = base.join("out.md"); + let resolved = resolve_export_path(&target, &bases).expect("inside-base must be allowed"); + assert!( + resolved.starts_with(&base), + "resolved path {resolved:?} must sit under base {base:?}" + ); + } + + #[test] + fn accepts_nested_path_inside_allowlist() { + let (_keep, base) = tempdir_canon("base"); + let nested = base.join("nested"); + std::fs::create_dir(&nested).expect("mkdir nested"); + let bases = vec![base.clone()]; + let target = nested.join("out.md"); + let resolved = + resolve_export_path(&target, &bases).expect("nested inside-base must be allowed"); + assert!(resolved.starts_with(&base)); + } + + #[test] + fn rejects_when_parent_does_not_exist() { + let bases = vec![PathBuf::from("/")]; + let result = + resolve_export_path(Path::new("/definitely-not-real-lumotia-fix/out.md"), &bases); + assert!( + result.is_err(), + "nonexistent parent must be rejected, got {result:?}" + ); + } + + #[test] + fn is_inside_any_base_is_a_prefix_check() { + let a = PathBuf::from("/tmp/a"); + let b = PathBuf::from("/tmp/b"); + let bases = vec![a.clone(), b]; + assert!(is_inside_any_base(&a.join("x.md"), &bases)); + assert!(is_inside_any_base(&a.join("nested/x.md"), &bases)); + assert!(!is_inside_any_base(Path::new("/tmp/other/x.md"), &bases)); } }