agent: code-atomiser-fix — validate output_folder against recordings_dir (Trust-2)

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) <noreply@anthropic.com>
This commit is contained in:
2026-05-13 17:56:49 +01:00
parent 07f6755961
commit ed449ccc1f

View File

@@ -1,4 +1,4 @@
use std::path::PathBuf; use std::path::{Path, PathBuf};
use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::atomic::{AtomicU64, Ordering};
use tauri::Manager; use tauri::Manager;
@@ -22,17 +22,36 @@ pub async fn list_audio_devices(window: tauri::WebviewWindow) -> Result<Vec<Devi
/// ensuring the parent directory exists. Used by /// ensuring the parent directory exists. Used by
/// `start_live_transcription_session` to hand the path to the progressive /// `start_live_transcription_session` to hand the path to the progressive
/// WAV writer before any samples arrive (brief item #19). /// WAV writer before any samples arrive (brief item #19).
///
/// Trust boundary (Trust-2): `output_folder` arrives from the webview
/// as a free-form string and is otherwise joined verbatim into a
/// PathBuf that `create_dir_all` + WavWriter then operate on. Without
/// validation the webview can write recordings anywhere the Lumotia
/// process can write — `/etc`, `/var`, somebody else's home dir.
///
/// The fix:
/// 1. None or empty → fall back to the default `recordings` subtree
/// under the app-local data dir. Always safe.
/// 2. Otherwise → canonicalise the supplied path (resolves `..` and
/// symlinks) and ensure it sits inside the trusted base. Reject if
/// it escapes.
///
/// A user who wants recordings somewhere else must configure it via
/// Settings (a different trust boundary owned by the app's persisted
/// preferences); the live-session command surface is constrained.
pub fn resolve_recording_path( pub fn resolve_recording_path(
app: &tauri::AppHandle, app: &tauri::AppHandle,
output_folder: Option<&str>, output_folder: Option<&str>,
) -> Result<PathBuf, String> { ) -> Result<PathBuf, String> {
let recordings_dir = match output_folder.map(str::trim).filter(|s| !s.is_empty()) { let default_base = app
Some(folder) => PathBuf::from(folder),
None => app
.path() .path()
.app_local_data_dir() .app_local_data_dir()
.map_err(|e: tauri::Error| e.to_string())? .map_err(|e: tauri::Error| e.to_string())?
.join("recordings"), .join("recordings");
let recordings_dir = match output_folder.map(str::trim).filter(|s| !s.is_empty()) {
Some(folder) => validate_output_folder(folder, &default_base)?,
None => default_base,
}; };
std::fs::create_dir_all(&recordings_dir) std::fs::create_dir_all(&recordings_dir)
@@ -41,6 +60,57 @@ pub fn resolve_recording_path(
Ok(recordings_dir.join(recording_filename())) 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<PathBuf, String> {
// 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 /// Deterministic recording filename generator. Combines three fields
/// for absolute uniqueness across rapid calls: /// for absolute uniqueness across rapid calls:
/// ///
@@ -73,7 +143,8 @@ static RECORDING_COUNTER: AtomicU64 = AtomicU64::new(0);
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::recording_filename; use super::{recording_filename, validate_output_folder};
use std::path::PathBuf;
#[test] #[test]
fn recording_filenames_are_unique_across_rapid_calls() { 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] #[test]
fn recording_filename_has_expected_shape() { fn recording_filename_has_expected_shape() {
let name = recording_filename(); let name = recording_filename();