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:
@@ -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<Vec<Devi
|
||||
/// ensuring the parent directory exists. Used by
|
||||
/// `start_live_transcription_session` to hand the path to the progressive
|
||||
/// 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(
|
||||
app: &tauri::AppHandle,
|
||||
output_folder: Option<&str>,
|
||||
) -> Result<PathBuf, String> {
|
||||
let recordings_dir = match output_folder.map(str::trim).filter(|s| !s.is_empty()) {
|
||||
Some(folder) => PathBuf::from(folder),
|
||||
None => app
|
||||
let default_base = app
|
||||
.path()
|
||||
.app_local_data_dir()
|
||||
.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)
|
||||
@@ -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<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
|
||||
/// 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();
|
||||
|
||||
Reference in New Issue
Block a user