diff --git a/src-tauri/src/commands/fs.rs b/src-tauri/src/commands/fs.rs index 7651e30..0bbb424 100644 --- a/src-tauri/src/commands/fs.rs +++ b/src-tauri/src/commands/fs.rs @@ -72,36 +72,58 @@ fn allowed_export_bases(app: &tauri::AppHandle) -> Vec { 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`. +/// Resolve the requested write target. 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. +/// Two-mode canonicalisation: +/// +/// - **If the target already exists**, canonicalise the WHOLE path. This +/// follows any symlink at the target itself, so a symlink that sits +/// inside an allowlisted base but points outside it gets rejected +/// here. `tokio::fs::write` follows symlinks on open(2), so without +/// this branch a symlink at `~/Downloads/notes.md -> ~/.bashrc` would +/// pass the containment check (the path string is inside Downloads) +/// and the subsequent write would silently land in `~/.bashrc`. +/// Trust-1 audit residual closed in Phase B.5 (2026-05-14); parallels +/// `validate_output_folder`'s full-path-canonicalize in +/// `commands/audio.rs` (Trust-2). +/// +/// - **If the target does not yet exist** (the typical save-dialog +/// case), canonicalise the parent and re-join the filename. 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_path = match std::fs::canonicalize(path) { + Ok(canon) => canon, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + 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); + let canon_parent = std::fs::canonicalize(parent).map_err(|e| { + format!( + "Refusing to write {}: cannot resolve parent dir ({e}).", + path.display() + ) + })?; + canon_parent.join(file_name) + } + Err(e) => { + return Err(format!( + "Refusing to write {}: cannot resolve path ({e}).", + path.display() + )); + } + }; if !is_inside_any_base(&canon_path, bases) { return Err(format!( @@ -209,4 +231,65 @@ mod tests { assert!(is_inside_any_base(&a.join("nested/x.md"), &bases)); assert!(!is_inside_any_base(Path::new("/tmp/other/x.md"), &bases)); } + + /// Phase B.5 audit regression (2026-05-14). The Trust-1 fix + /// canonicalised only the parent of the target path because the file + /// typically does not exist yet. But if a symlink ALREADY exists at + /// the target path pointing outside the allowlisted base, + /// `tokio::fs::write` follows it on `File::create -> open(2)` and + /// silently writes through to the outside target. The path-string + /// containment check passed because the SYMLINK lives inside the + /// base, even though the symlink's target does not. + /// + /// The fix canonicalises the full path when it exists (so the + /// symlink resolves before the containment check) and only falls + /// back to parent-canonicalize on NotFound. This regression test + /// pins that contract. + #[cfg(unix)] + #[test] + fn rejects_symlink_target_outside_allowlist() { + let (_keep_base, base) = tempdir_canon("base"); + let (_keep_out, outside) = tempdir_canon("outside"); + // Real, writable file outside the base — the symlink target. + let outside_target = outside.join("victim.txt"); + std::fs::write(&outside_target, b"original").expect("seed outside target"); + + // Inside-base symlink pointing to the outside file. + let symlink_path = base.join("notes.md"); + std::os::unix::fs::symlink(&outside_target, &symlink_path) + .expect("create symlink inside base pointing outside"); + + let bases = vec![base.clone()]; + let result = resolve_export_path(&symlink_path, &bases); + assert!( + result.is_err(), + "symlink target outside allowlist must be rejected, got {result:?}" + ); + let err = result.err().unwrap(); + assert!( + err.contains("outside the allowed export directories"), + "unexpected error shape: {err}" + ); + } + + /// Symmetric to the rejection above: a symlink whose target stays + /// inside the same base must still be accepted, so legitimate users + /// of in-base symlinks (e.g. a tilde-expansion alias inside the base) + /// are not regressed. + #[cfg(unix)] + #[test] + fn accepts_symlink_target_inside_allowlist() { + let (_keep, base) = tempdir_canon("base"); + let real_path = base.join("real.md"); + std::fs::write(&real_path, b"original").expect("seed real target"); + + let symlink_path = base.join("alias.md"); + std::os::unix::fs::symlink(&real_path, &symlink_path) + .expect("create in-base alias symlink"); + + let bases = vec![base.clone()]; + let resolved = resolve_export_path(&symlink_path, &bases) + .expect("in-base symlink must resolve and pass"); + assert!(resolved.starts_with(&base)); + } }