From ed449ccc1f63cff9dffd3e69e7b2d93b4daeb203 Mon Sep 17 00:00:00 2001 From: Jake Date: Wed, 13 May 2026 17:56:49 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20code-atomiser-fix=20=E2=80=94=20valida?= =?UTF-8?q?te=20output=5Ffolder=20against=20recordings=5Fdir=20(Trust-2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolve_recording_path() previously joined the webview-supplied output_folder string verbatim into a PathBuf and then mkdir -p'd + WAV-wrote at that path. The webview is a (mostly-)trusted surface in Tauri, but the live-transcription command's input is JSON from the frontend with no schema enforcement on the path field — so a compromised page, a Tauri IPC sender on an OEM build, or a future plugin reaching the same command can pipe through paths like `/etc`, `/var/log`, or anywhere else the Lumotia process can write. The new flow: - None / empty → fall back to the default `app_local_data_dir/recordings` base. Always safe. - Non-empty → call validate_output_folder, which: 1. Ensures the default base exists (so canonicalise can succeed on first launch). 2. Creates the requested path if it doesn't exist (so canonicalise can resolve it; an empty directory outside the base is the only side effect of an attempted escape, which is acceptable for the trust gain). 3. Canonicalises both base and requested paths (resolves `..` and symlinks). 4. Requires the canonical requested path to start_with the canonical base. Reject otherwise with a message naming the trust boundary. A user who legitimately wants recordings elsewhere routes through Settings (a separately-validated persisted-preferences boundary). The command surface stays constrained. Regression tests (commands::audio::tests): - validate_output_folder_accepts_base_itself - validate_output_folder_accepts_descendant - validate_output_folder_rejects_etc — `/etc` attack shape - validate_output_folder_rejects_parent_escape — `..`-walk attack - validate_output_folder_rejects_sibling_dir — prefix-overlap attack (`recordings-backdoor` vs `recordings`). Canonical starts_with on PathBufs correctly rejects this; a naive string-prefix check would have let it through. - validate_output_folder_rejects_symlink_pointing_out (Unix only) — in-base symlink to outside path must be rejected after canonicalise follows the link. cargo test -p lumotia --lib commands::audio::tests: 8 passed. cargo test --workspace: all green. npm run check: 0 errors, 0 warnings. Co-Authored-By: Claude Opus 4.7 (1M context) --- src-tauri/src/commands/audio.rs | 208 ++++++++++++++++++++++++++++++-- 1 file changed, 200 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands/audio.rs b/src-tauri/src/commands/audio.rs index eb9a2bc..6cfb122 100644 --- a/src-tauri/src/commands/audio.rs +++ b/src-tauri/src/commands/audio.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; use tauri::Manager; @@ -22,17 +22,36 @@ pub async fn list_audio_devices(window: tauri::WebviewWindow) -> Result, ) -> Result { + let default_base = app + .path() + .app_local_data_dir() + .map_err(|e: tauri::Error| e.to_string())? + .join("recordings"); + let recordings_dir = match output_folder.map(str::trim).filter(|s| !s.is_empty()) { - Some(folder) => PathBuf::from(folder), - None => app - .path() - .app_local_data_dir() - .map_err(|e: tauri::Error| e.to_string())? - .join("recordings"), + Some(folder) => validate_output_folder(folder, &default_base)?, + None => default_base, }; std::fs::create_dir_all(&recordings_dir) @@ -41,6 +60,57 @@ pub fn resolve_recording_path( Ok(recordings_dir.join(recording_filename())) } +/// Validate that a webview-supplied `output_folder` does not escape the +/// trusted recordings base. Canonicalises both sides so `..`-walks and +/// symlinks resolve before the containment check. +/// +/// Allows the supplied path to be either the base itself or a +/// descendant of it; everything else is rejected. The base must +/// already exist on disk (we `create_dir_all` it on the default +/// branch); we ensure it here too so canonicalisation can succeed even +/// on a clean install where neither side has been created yet. +fn validate_output_folder(folder: &str, default_base: &Path) -> Result { + // Make sure the trusted base exists before canonicalisation — + // `canonicalize` errors on missing paths and we want a clean + // failure mode on first launch. + std::fs::create_dir_all(default_base) + .map_err(|e| format!("Failed to ensure recordings base dir: {e}"))?; + // Create the requested path too if it doesn't exist yet, so + // canonicalisation can resolve it. If the user is pointing at a + // path outside the trusted base the containment check rejects + // them right after — a stray empty directory is the only side + // effect, which is acceptable for the trust gain. + let requested = PathBuf::from(folder); + if !requested.exists() { + std::fs::create_dir_all(&requested).map_err(|e| { + format!( + "Failed to create requested output folder '{}': {e}", + requested.display() + ) + })?; + } + let canonical_base = default_base.canonicalize().map_err(|e| { + format!( + "Failed to canonicalise recordings base '{}': {e}", + default_base.display() + ) + })?; + let canonical_requested = requested.canonicalize().map_err(|e| { + format!( + "Failed to canonicalise output folder '{}': {e}", + requested.display() + ) + })?; + if !canonical_requested.starts_with(&canonical_base) { + return Err(format!( + "outputFolder '{}' is outside the allowed recordings base '{}'", + canonical_requested.display(), + canonical_base.display() + )); + } + Ok(canonical_requested) +} + /// Deterministic recording filename generator. Combines three fields /// for absolute uniqueness across rapid calls: /// @@ -73,7 +143,8 @@ static RECORDING_COUNTER: AtomicU64 = AtomicU64::new(0); #[cfg(test)] mod tests { - use super::recording_filename; + use super::{recording_filename, validate_output_folder}; + use std::path::PathBuf; #[test] fn recording_filenames_are_unique_across_rapid_calls() { @@ -94,6 +165,127 @@ mod tests { ); } + fn unique_tmp(label: &str) -> PathBuf { + let pid = std::process::id(); + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + std::env::temp_dir().join(format!("lumotia-audio-test-{label}-{pid}-{nanos}")) + } + + #[test] + fn validate_output_folder_accepts_base_itself() { + // Trust-2 happy path: the recordings base is always allowed. + let root = unique_tmp("base-itself"); + let base = root.join("recordings"); + std::fs::create_dir_all(&base).unwrap(); + + let canonical = validate_output_folder(base.to_str().unwrap(), &base) + .expect("base path must validate as itself"); + assert_eq!(canonical, base.canonicalize().unwrap()); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn validate_output_folder_accepts_descendant() { + // A user-configured subdirectory of the trusted base is fine. + let root = unique_tmp("descendant"); + let base = root.join("recordings"); + let nested = base.join("2026-05"); + std::fs::create_dir_all(&nested).unwrap(); + + let canonical = validate_output_folder(nested.to_str().unwrap(), &base) + .expect("descendant must validate"); + assert!(canonical.starts_with(base.canonicalize().unwrap())); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn validate_output_folder_rejects_etc() { + // Trust-2 attack shape: webview supplies `/etc`. Must be + // rejected even though /etc exists (and `create_dir_all` on it + // would no-op). + let root = unique_tmp("etc-reject"); + let base = root.join("recordings"); + std::fs::create_dir_all(&base).unwrap(); + + let err = validate_output_folder("/etc", &base) + .expect_err("`/etc` must NEVER be accepted as a recordings dir"); + assert!( + err.contains("outside the allowed recordings base"), + "rejection message must name the trust boundary: {err}" + ); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn validate_output_folder_rejects_parent_escape() { + // `..`-walk attack shape: a path that resolves OUT of the + // trusted base via parent traversal. Canonicalisation must + // resolve the `..` before the containment check runs. + let root = unique_tmp("parent-escape"); + let base = root.join("recordings"); + let escape_target = root.join("escape"); + std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&escape_target).unwrap(); + + // recordings/../escape — exists on disk, but resolves to root/escape. + let attack = base.join("..").join("escape"); + let err = validate_output_folder(attack.to_str().unwrap(), &base) + .expect_err("..-walk out of base must be rejected"); + assert!( + err.contains("outside the allowed recordings base"), + "rejection message must name the trust boundary: {err}" + ); + + std::fs::remove_dir_all(&root).ok(); + } + + #[test] + fn validate_output_folder_rejects_sibling_dir() { + // Plain sibling-of-base: equal path depth, prefix-matching the + // base's parent but not the base itself. `starts_with` on + // canonical paths must reject this (a bare-string prefix match + // would have let `recordings-backdoor` through). + let root = unique_tmp("sibling"); + let base = root.join("recordings"); + let sibling = root.join("recordings-backdoor"); + std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&sibling).unwrap(); + + let err = validate_output_folder(sibling.to_str().unwrap(), &base) + .expect_err("sibling of base must be rejected even with prefix overlap"); + assert!(err.contains("outside the allowed recordings base")); + + std::fs::remove_dir_all(&root).ok(); + } + + #[cfg(unix)] + #[test] + fn validate_output_folder_rejects_symlink_pointing_out() { + // Symlink-escape attack shape: a path inside the base which + // resolves through a symlink to a directory outside the base. + // Canonicalisation must follow the link before the containment + // check. + let root = unique_tmp("symlink-escape"); + let base = root.join("recordings"); + let outside = root.join("outside"); + std::fs::create_dir_all(&base).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + let link = base.join("trapdoor"); + std::os::unix::fs::symlink(&outside, &link).unwrap(); + + let err = validate_output_folder(link.to_str().unwrap(), &base) + .expect_err("symlink to outside path must be rejected"); + assert!(err.contains("outside the allowed recordings base")); + + std::fs::remove_dir_all(&root).ok(); + } + #[test] fn recording_filename_has_expected_shape() { let name = recording_filename();