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 <active-data-root>/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.
This commit is contained in:
claude@clouddev1
+181
-13
@@ -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 `<data_root>/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
|
||||
|
||||
+24
-20
@@ -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<std::path::PathBuf> = 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",
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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();
|
||||
|
||||
Reference in New Issue
Block a user