From 58a964da8c85a4360ac503b3805d41b8e6db84d4 Mon Sep 17 00:00:00 2001 From: "claude@clouddev1" Date: Fri, 8 May 2026 06:59:26 +0000 Subject: [PATCH] Harden temp-project cleanup with stacked safety guards The previous remove_dir_all on a path returned by Project::path() was too trusting: an unusual CLI argument or a hand-edited project.yaml could in principle have steered cleanup into deleting the wrong directory. Replace it with safely_delete_temp_project, which refuses unless every one of the following passes: 1. Path is not a symlink (checked before canonicalize so a symlink can't smuggle a different target through). 2. Path is a directory. 3. Canonical path is under /projects/ (canonical-prefix containment). 4. Directory basename contains the literal `[temp]` marker. 5. Direct children are exclusively well-known project artefacts (project.yaml, data/, history.log, playground.db, .gitignore, lock file) plus migration .bak files and atomic-write .tmp files. Any stranger file (notes.md, .git/, screenshots, etc.) makes the helper refuse. is_unmodified_temp now also requires data/ to be empty, in addition to project.yaml's tables and relationships being empty. A hand-edited yaml that drops the schema list but leaves CSVs in data/ no longer passes. Failure to delete is non-fatal -- the helper returns SafeDeleteError, the runtime logs a tracing::warn!, and the project stays on disk. Leaving an unexpected directory alone is always preferable to a wrong delete. Tests: 345 passing (272 lib + 9 + 5 + 6 + 27 + 9 + 17), 0 failing, 0 skipped. 7 new tests covering each guard, including a unix-only symlink-rejection test. --- src/project/mod.rs | 194 ++++++++++++++++++++++-- src/runtime.rs | 44 +++--- tests/iteration4b_lifecycle_commands.rs | 111 +++++++++++++- 3 files changed, 315 insertions(+), 34 deletions(-) diff --git a/src/project/mod.rs b/src/project/mod.rs index ba6adb8..539fdc8 100644 --- a/src/project/mod.rs +++ b/src/project/mod.rs @@ -349,19 +349,23 @@ impl Project { /// Is this an auto-named temp project that the user has /// not modified? /// - /// Used to clean up the inevitable accumulation of - /// auto-named temp directories left behind when the user - /// launches the app, immediately loads another project - /// (or quits without doing anything), and never returns - /// to the temp. + /// Used by `safely_delete_temp_project` to clean up the + /// inevitable accumulation of auto-named temp directories + /// left behind when the user launches the app, immediately + /// loads another project (or quits without doing + /// anything), and never returns to the temp. /// - /// "Unmodified" is defined as: kind is Temp AND - /// `project.yaml` lists no tables and no relationships. - /// The user-visible schema is what counts — show queries - /// only append to history.log and don't trip this check. - /// Errors reading or parsing the YAML default to "not - /// unmodified" (false), so a corrupted project is never - /// auto-deleted. + /// "Unmodified" requires *all* of: + /// + /// - `kind` is `Temp`. + /// - `project.yaml` parses successfully and lists no + /// tables and no relationships. + /// - `data/` is empty (so we don't delete CSVs the user + /// might still want even if the YAML schema list is + /// empty for any reason). + /// + /// Anything that fails defaults to "not unmodified" + /// (false), so a corrupted project is never auto-deleted. #[must_use] pub fn is_unmodified_temp(&self) -> bool { if !matches!(self.kind, ProjectKind::Temp) { @@ -374,7 +378,16 @@ impl Project { let Ok(snapshot) = crate::persistence::parse_schema(&body) else { return false; }; - snapshot.tables.is_empty() && snapshot.relationships.is_empty() + if !snapshot.tables.is_empty() || !snapshot.relationships.is_empty() { + return false; + } + // Defensive belt-and-braces: data/ must also be empty. + // This catches edge cases like a hand-edited YAML that + // lists no tables but the data dir still contains a + // user's CSV. + let data_dir = self.path.join(DATA_DIR); + // Missing data dir → also "no data" → empty. + fs::read_dir(&data_dir).map_or(true, |mut iter| iter.next().is_none()) } /// Path to the SQLite database for this project. Always @@ -385,6 +398,161 @@ impl Project { } } +/// Whitelisted file/directory names allowed inside a temp +/// project we're about to auto-delete. Anything else makes +/// `safely_delete_temp_project` refuse. +const ALLOWED_PROJECT_ENTRIES: &[&str] = &[ + PROJECT_YAML, + DATA_DIR, + HISTORY_LOG, + PLAYGROUND_DB, + GITIGNORE, + ".rdbms-playground.lock", +]; + +/// Reasons `safely_delete_temp_project` refuses to delete a +/// path. Each variant carries enough detail to surface in a +/// log warning. +#[derive(Debug, thiserror::Error)] +pub enum SafeDeleteError { + #[error("refusing to delete `{}`: {reason}", path.display())] + Refused { + path: PathBuf, + reason: &'static str, + }, + #[error("io error on `{}`: {source}", path.display())] + Io { + path: PathBuf, + source: std::io::Error, + }, +} + +/// Conservatively delete a temp project's directory. +/// +/// Stacks every guard we can think of so a bug elsewhere (or +/// a maliciously crafted CLI argument) can never escalate +/// into deleting the wrong directory: +/// +/// 1. **Canonicalize** the path — resolves symlinks and `..`, +/// normalises against the actual filesystem. +/// 2. **Symlink rejection** at the top level — if the path +/// *is* a symlink, refuse. +/// 3. **Must be a directory** — `is_dir()` after the symlink +/// check. +/// 4. **Containment** — the canonical path must start with +/// the canonical `/projects/`. Anything outside +/// that prefix (a user's home, /tmp, the system root) +/// cannot pass. +/// 5. **Marker-segment match** — the directory's basename +/// must contain the literal `[temp]` segment, the same +/// marker `validate_user_name` reserves so user-named +/// projects can never collide. +/// 6. **Allowlisted contents** — every direct child must be +/// one of the well-known project artefacts (or a migration +/// backup or atomic-write `.tmp` file). Refuse if anything +/// foreign is present (a user's `notes.md`, a stray +/// `.git/`, etc.). +/// +/// Any single guard failing produces a `SafeDeleteError` and +/// no `remove_dir_all` runs. The caller is expected to log +/// the refusal and move on — leaving an unexpected directory +/// alone is always preferable to a wrong delete. +pub fn safely_delete_temp_project( + project_path: &Path, + data_root: &Path, +) -> Result<(), SafeDeleteError> { + // 1. Reject symlinks at the top level (before we + // canonicalize). canonicalize() would silently follow + // the link and pass the rest of the checks against + // the *target*, which is not what we want. + let top_meta = fs::symlink_metadata(project_path).map_err(|source| SafeDeleteError::Io { + path: project_path.to_path_buf(), + source, + })?; + if top_meta.file_type().is_symlink() { + return Err(SafeDeleteError::Refused { + path: project_path.to_path_buf(), + reason: "path is a symbolic link", + }); + } + if !top_meta.is_dir() { + return Err(SafeDeleteError::Refused { + path: project_path.to_path_buf(), + reason: "path is not a directory", + }); + } + + // 2. Canonicalize for the containment check. We do this + // only after the symlink-at-top check so we can't be + // tricked by a top-level symlink. + let project_canon = + fs::canonicalize(project_path).map_err(|source| SafeDeleteError::Io { + path: project_path.to_path_buf(), + source, + })?; + + // 3. Containment: canonical path must be inside the + // canonical data-root projects dir. + let projects_root = projects_dir(data_root); + let projects_root_canon = fs::canonicalize(&projects_root).unwrap_or(projects_root); + if !project_canon.starts_with(&projects_root_canon) { + return Err(SafeDeleteError::Refused { + path: project_canon, + reason: "path is not inside the active data dir's projects folder", + }); + } + + // 4. Marker segment: basename must contain `[temp]` per + // naming::is_temp_dirname. + let dirname = match project_canon.file_name().and_then(|n| n.to_str()) { + Some(s) => s, + None => { + return Err(SafeDeleteError::Refused { + path: project_canon, + reason: "directory has no usable basename", + }); + } + }; + if !naming::is_temp_dirname(dirname) { + return Err(SafeDeleteError::Refused { + path: project_canon, + reason: "directory name does not contain the [temp] marker", + }); + } + + // 5. Contents allowlist. Any direct child not in + // ALLOWED_PROJECT_ENTRIES (and not a migration backup + // or staged-write tmp) makes us refuse. + let entries = fs::read_dir(&project_canon).map_err(|source| SafeDeleteError::Io { + path: project_canon.clone(), + source, + })?; + for entry in entries { + let entry = entry.map_err(|source| SafeDeleteError::Io { + path: project_canon.clone(), + source, + })?; + let name = entry.file_name(); + let name_str = name.to_string_lossy().into_owned(); + let is_allowed = ALLOWED_PROJECT_ENTRIES.iter().any(|a| *a == name_str) + || name_str.starts_with("project.yaml.v") && name_str.ends_with(".bak") + || name_str.ends_with(".tmp"); + if !is_allowed { + return Err(SafeDeleteError::Refused { + path: project_canon.clone(), + reason: "directory contains an unexpected file", + }); + } + } + + // All guards passed. Remove the directory. + fs::remove_dir_all(&project_canon).map_err(|source| SafeDeleteError::Io { + path: project_canon, + source, + })?; + Ok(()) +} + /// Copy a project directory to a new location. /// /// Used by `save` / `save as` (ADR-0015 §11). Excludes the diff --git a/src/runtime.rs b/src/runtime.rs index d613b7a..8d15920 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -34,7 +34,7 @@ use crate::dsl::Command; use crate::event::AppEvent; use crate::project::{ Project, ProjectKind, copy_project, list_projects, open_or_create, projects_dir, - resolve_data_root, + resolve_data_root, safely_delete_temp_project, }; use crate::theme::Theme; use crate::ui; @@ -276,7 +276,9 @@ async fn run_loop( // Auto-delete the active project on quit if it's an // unmodified temp — same rule as on project switch (see // perform_switch). Captures the path first, drops the - // project (releasing the lock), then removes the dir. + // project (releasing the lock), then asks + // safely_delete_temp_project to verify the directory + // before removing it. let cleanup_on_quit: Option = session .project .as_ref() @@ -284,17 +286,16 @@ async fn run_loop( let _ = session.database.take(); let _ = session.project.take(); if let Some(stale) = cleanup_on_quit { - if let Err(e) = std::fs::remove_dir_all(&stale) { - tracing::warn!( - path = %stale.display(), - error = %e, - "could not clean up unmodified temp project on quit", - ); - } else { - tracing::info!( + match safely_delete_temp_project(&stale, &session.data_root) { + Ok(()) => tracing::info!( path = %stale.display(), "cleaned up unmodified temp project on quit", - ); + ), + Err(e) => tracing::warn!( + path = %stale.display(), + error = %e, + "did not clean up unmodified temp project on quit", + ), } } @@ -409,18 +410,21 @@ async fn perform_switch( // The outgoing project's lock is now released; it's // safe to remove its directory if it was unmodified. + // The safely_delete_temp_project helper layers multiple + // guards (containment under data root, [temp] marker, + // contents allowlist, no symlinks) so a bug elsewhere + // can't escalate into deleting the wrong directory. if let Some(stale) = outgoing_cleanup_path { - if let Err(e) = std::fs::remove_dir_all(&stale) { - tracing::warn!( - path = %stale.display(), - error = %e, - "could not clean up unmodified temp project", - ); - } else { - tracing::info!( + match safely_delete_temp_project(&stale, &session.data_root) { + Ok(()) => tracing::info!( path = %stale.display(), "cleaned up unmodified temp project on switch", - ); + ), + Err(e) => tracing::warn!( + path = %stale.display(), + error = %e, + "did not clean up unmodified temp project on switch", + ), } } diff --git a/tests/iteration4b_lifecycle_commands.rs b/tests/iteration4b_lifecycle_commands.rs index d330828..c5aa3ba 100644 --- a/tests/iteration4b_lifecycle_commands.rs +++ b/tests/iteration4b_lifecycle_commands.rs @@ -20,7 +20,9 @@ use rdbms_playground::event::AppEvent; use rdbms_playground::db::Database; use rdbms_playground::dsl::{ColumnSpec, Type}; use rdbms_playground::persistence::Persistence; -use rdbms_playground::project::{self, Project, ProjectKind, copy_project}; +use rdbms_playground::project::{ + self, Project, ProjectKind, copy_project, safely_delete_temp_project, +}; const fn key(code: KeyCode) -> AppEvent { AppEvent::Key(KeyEvent { @@ -387,6 +389,113 @@ fn named_project_is_never_unmodified_temp() { assert!(!opened.is_unmodified_temp()); } +#[test] +fn safely_delete_removes_genuine_unmodified_temp() { + let data = tempdir(); + let project = project::open_or_create(None, Some(data.path())).unwrap(); + let path = project.path().to_path_buf(); + drop(project); // release lock so we can delete + assert!(path.exists()); + safely_delete_temp_project(&path, data.path()).expect("should delete"); + assert!(!path.exists()); +} + +#[test] +fn safely_delete_refuses_path_outside_data_root() { + let data = tempdir(); + let other = tempdir(); + // Construct a directory outside the data root that LOOKS + // like a temp project (has [temp] marker + project.yaml). + let foreign = other.path().join("20260507-[temp]-fake-fake-fake"); + fs::create_dir_all(&foreign).unwrap(); + fs::write( + foreign.join("project.yaml"), + "version: 1\nproject:\n created_at: x\ntables: []\nrelationships: []\n", + ) + .unwrap(); + + let err = safely_delete_temp_project(&foreign, data.path()).expect_err("must refuse"); + assert!(format!("{err}").contains("not inside"), "got: {err}"); + assert!(foreign.exists(), "foreign dir must still exist"); +} + +#[test] +fn safely_delete_refuses_directory_without_temp_marker() { + let data = tempdir(); + // Create a project directory under the data root that + // doesn't carry the [temp] marker. + let projects_dir = data.path().join(project::PROJECTS_SUBDIR); + fs::create_dir_all(&projects_dir).unwrap(); + let named = projects_dir.join("MyOrders"); + fs::create_dir(&named).unwrap(); + fs::write(named.join("project.yaml"), "version: 1\n").unwrap(); + + let err = safely_delete_temp_project(&named, data.path()).expect_err("must refuse"); + assert!(format!("{err}").contains("[temp]"), "got: {err}"); + assert!(named.exists()); +} + +#[test] +fn safely_delete_refuses_directory_with_unexpected_file() { + let data = tempdir(); + let project = project::open_or_create(None, Some(data.path())).unwrap(); + let path = project.path().to_path_buf(); + // Drop a stranger file into the project dir. + fs::write(path.join("notes.md"), "user notes\n").unwrap(); + drop(project); + + let err = safely_delete_temp_project(&path, data.path()).expect_err("must refuse"); + assert!(format!("{err}").contains("unexpected file"), "got: {err}"); + assert!(path.exists()); + assert!(path.join("notes.md").exists()); +} + +#[test] +fn safely_delete_allows_migration_backups_and_tmp_files() { + let data = tempdir(); + let project = project::open_or_create(None, Some(data.path())).unwrap(); + let path = project.path().to_path_buf(); + fs::write(path.join("project.yaml.v1.bak"), "old\n").unwrap(); + fs::write(path.join("project.yaml.tmp"), "stage\n").unwrap(); + drop(project); + + safely_delete_temp_project(&path, data.path()).expect("should delete"); + assert!(!path.exists()); +} + +#[cfg(unix)] +#[test] +fn safely_delete_refuses_symlink_top_level() { + use std::os::unix::fs::symlink; + let data = tempdir(); + let real_target = tempdir(); + let projects_dir = data.path().join(project::PROJECTS_SUBDIR); + fs::create_dir_all(&projects_dir).unwrap(); + let link = projects_dir.join("20260507-[temp]-aaa-bbb-ccc"); + symlink(real_target.path(), &link).unwrap(); + + let err = safely_delete_temp_project(&link, data.path()).expect_err("must refuse"); + assert!(format!("{err}").contains("symbolic link"), "got: {err}"); + // Real target untouched. + assert!(real_target.path().exists()); + // Symlink itself untouched. + assert!(link.exists()); +} + +#[test] +fn unmodified_temp_with_residual_csv_in_data_dir_is_not_unmodified() { + let data = tempdir(); + let project = project::open_or_create(None, Some(data.path())).unwrap(); + // Hand-drop a CSV into the data dir without going through + // the persistence layer. Schema in yaml is still empty. + let csv = project.path().join("data").join("Stranger.csv"); + fs::write(&csv, "id\n1\n").unwrap(); + assert!( + !project.is_unmodified_temp(), + "non-empty data dir must disqualify the unmodified-temp check", + ); +} + #[test] fn list_projects_sorts_by_mtime() { let data = tempdir();