From dcd2dfe5a35b78351e0fb576c24240881dd5426a Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 13:11:42 -0700 Subject: [PATCH] Stop a project id from steering a Docker API DELETE, and stop trusting a store that lost its list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit C-2 (critical). `destroy_ownerless_rollback_pin` validated its tag and not its project id, then interpolated both into `triple-c-snapshot-{id}:{tag}` and handed the result to bollard. bollard does not percent-encode: `Uri::parse` joins an absolute path onto the base URL, which replaces the path outright and applies RFC 3986 dot-segment removal. An id of `a/../../v1.47/volumes/?` turns a "remove image tag" into `DELETE /v1.47/volumes/`. That arm is reached *because* `find_project` failed, so the id is unconstrained IPC input, and the typed confirmation is no barrier — it compares the caller's own two strings. Reproduced against the live daemon, and now a test: with the check removed the volume is gone and the test fails; with it, the volume survives and a legitimate ownerless pin still deletes. The reference that reaches `remove_image` is now the daemon's own repo_tag, matched on the parsed pair, so nothing built from IPC input addresses the API at all. The same id check now guards the owned arms of `destroy` and `compact_snapshot`, which build volume names and image references from a `projects.json` field. H-1. The ownerless arm decided ownership from the in-memory list alone and then called `sweep_orphaned_snapshots()`, which deletes the freshly dangling image on the same pass — so a corrupt `projects.json` could reap a pin whose migration is still awaiting confirmation, the one thing `pin_is_reapable` orders its conditions to prevent. It now re-reads the store from disk, runs `project_store_trust`, refuses an id the store knows, takes the project lock before reading anything a decision rests on, and checks `has_record`. H-3. The corrupt-store guard keyed on "empty list + file exists", and `ProjectsStore::new()` swallows a corrupt file without rewriting it — so the first `save()`, as little as starting a project, wrote `[{new}]` over it and the guard passed with every other project's volumes unclaimed. A corrupt load is now recorded in a sticky `projects.json.corrupt` marker beside the file, and the existing `.bak` is no longer clobbered by a second corruption. A missing `projects.json` is refused too: it cannot be told from a moved or partially restored data directory, and the genuinely fresh case has nothing to find. Also: the three migration commands surface the lock's real refusal instead of substituting "a migration is already running"; `note_ownerless_since` re-checks `has_record` after writing a tombstone, closing the window that could plant one behind a valid record and reap the pin with zero grace; corrupt migration-record copies are capped at four; `reconcile_migration` yields to any lock holder, not only a migration; and a 22-space run in a refusal string is gone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- .../src/commands/migration_commands.rs | 71 ++-- app/src-tauri/src/docker/disk.rs | 352 ++++++++++++++-- app/src-tauri/src/docker/disk_tests.rs | 391 +++++++++++++++++- app/src-tauri/src/docker/migration.rs | 8 + app/src-tauri/src/storage/migration_store.rs | 112 ++++- app/src-tauri/src/storage/projects_store.rs | 185 ++++++++- 6 files changed, 1034 insertions(+), 85 deletions(-) diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index a9ee7a2..1b606db 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -256,10 +256,25 @@ pub(crate) fn is_migrating(project_id: &str) -> bool { struct ActiveGuard(#[allow(dead_code)] crate::project_lock::ProjectGuard); impl ActiveGuard { - /// `None` when a migration — or anything else — already holds this project. - fn acquire(project_id: &str) -> Option { + /// `Err` with the registry's own refusal when a migration — **or anything + /// else** — already holds this project. + /// + /// The error string is the point. This returned `Option`, and all three + /// callers replaced the discarded reason with a sentence about a migration + /// — so a user blocked by a *compaction*, a reset or a cache clear was told + /// to wait for a base update that was not running, with nothing in the UI + /// that could ever name what actually held the project. + /// `project_lock::try_acquire` already composes "what holds it" with "what + /// you were trying to do"; there is nothing to add to it here. + /// + /// The tail it composes is `ProjectOp::Migration`'s — "…before starting a + /// base update" — for confirm and rollback as well as for the migration + /// itself. That is the class all three belong to, and splitting it would + /// mean a `ProjectOp` variant per command: the wrong place to encode a + /// verb, for a phrase that is at worst imprecise where the old one was + /// simply wrong. + fn acquire(project_id: &str) -> Result { crate::project_lock::try_acquire(project_id, crate::project_lock::ProjectOp::Migration) - .ok() .map(Self) } } @@ -276,10 +291,9 @@ pub async fn migrate_project_to_base( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result { - let Some(_guard) = ActiveGuard::acquire(&project_id) else { - return Ok(MigrationReport::failed_preflight( - "A migration is already running for this project.", - )); + let _guard = match ActiveGuard::acquire(&project_id) { + Ok(guard) => guard, + Err(busy) => return Ok(MigrationReport::failed_preflight(&busy)), }; let existing = migration_store::load(&project_id)?; @@ -817,12 +831,7 @@ pub async fn confirm_migration( let _ = &state; // Confirming drops the only way back. Doing that underneath a running // migration would delete the pin it is relying on mid-flight. - let Some(_guard) = ActiveGuard::acquire(&project_id) else { - return Err( - "A container base update is running for this project right now. Wait for it to finish." - .to_string(), - ); - }; + let _guard = ActiveGuard::acquire(&project_id)?; let Some(mstate) = migration_store::load(&project_id)? else { return Ok(()); }; @@ -869,12 +878,7 @@ pub async fn rollback_migration( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result<(), String> { - let Some(_guard) = ActiveGuard::acquire(&project_id) else { - return Err( - "A container base update is running for this project right now. Wait for it to finish." - .to_string(), - ); - }; + let _guard = ActiveGuard::acquire(&project_id)?; let mut project = state .projects_store @@ -987,7 +991,21 @@ pub async fn get_migration_state( pub async fn reconcile_migration(project: &Project, app_handle: &tauri::AppHandle) { // A migration running right now is indistinguishable from a crashed one // from the outside; only this process knows the difference. - if is_migrating(&project.id) { + // + // **Any holder, not just a migration.** This asked `is_migrating`, which is + // `held() == Some(Migration)` — so a compaction, a reset or a destroy + // holding the project made this fall straight through and start rewriting + // the migration record's phase and untagging its rollback image underneath + // whatever was running. `reconcile_project_statuses` is a command, not just + // a startup step, so "nothing else can be running yet" is not available as + // an argument. + if let Some(holder) = crate::project_lock::held(&project.id) { + log::debug!( + "Skipping migration reconcile for '{}' ({}): {}", + project.name, + project.id, + holder.describe() + ); return; } let state = match migration_store::load(&project.id) { @@ -1897,16 +1915,23 @@ mod tests { { let g = ActiveGuard::acquire(id).expect("first acquire must succeed"); assert!(is_migrating(id)); + let refused = ActiveGuard::acquire(id) + .err() + .expect("a second concurrent migration must be refused"); + // The refusal has to say what is holding the project, not what the + // caller happens to be — the three commands used to substitute + // their own sentence for this and lost the distinction. assert!( - ActiveGuard::acquire(id).is_none(), - "a second concurrent migration must be refused" + refused.contains("base update"), + "the refusal must name the holder: {}", + refused ); drop(g); } assert!(!is_migrating(id), "the guard must release on drop"); // …including when the migration bailed out through an early return. fn early_return(id: &str) -> Option<()> { - let _g = ActiveGuard::acquire(id)?; + let _g = ActiveGuard::acquire(id).ok()?; None } assert!(early_return(id).is_none()); diff --git a/app/src-tauri/src/docker/disk.rs b/app/src-tauri/src/docker/disk.rs index bb4de65..da37e02 100644 --- a/app/src-tauri/src/docker/disk.rs +++ b/app/src-tauri/src/docker/disk.rs @@ -1059,6 +1059,48 @@ pub fn confirmation_matches(expected_project_name: &str, typed: &str) -> bool { !expected_project_name.is_empty() && typed.trim() == expected_project_name.trim() } +/// Longest project id this will accept. A UUID is 36 characters; the slack is +/// for a hand-edited `projects.json`, not for anything structural. +const PROJECT_ID_MAX_LEN: usize = 64; + +/// Whether a string may be interpolated into a Docker object name. +/// +/// ## This is a path-injection guard, not a tidiness check +/// +/// `project_id` reaches [`destroy`] as a free-form string over IPC, and the +/// destructive paths interpolate it into `triple-c-snapshot-{id}:{tag}` — which +/// bollard puts straight into a request path. **bollard does not percent-encode +/// it.** `bollard::uri::Uri::parse` builds `unix://…/v1.47/images/{reference}` +/// and then calls `Url::join(path)` on it; `path` starts with `/`, so the join +/// *replaces* the whole path and RFC 3986 dot-segment removal is applied to the +/// result. An id of `a/../../v1.47/volumes/{name}?` therefore turns +/// `DELETE /v1.47/images/triple-c-snapshot-a/../../v1.47/volumes/{name}?…` +/// into `DELETE /v1.47/volumes/{name}` — the trailing `?` swallowing the tag +/// into a query string that bollard then overwrites with its own. Reproduced +/// against the live daemon: it answered 204 and the volume was gone. +/// +/// The typed confirmation is no defence, because [`confirmation_matches`] +/// compares the caller's own two strings and an attacker-shaped id can be typed +/// back verbatim. +/// +/// ## Why a character class and not a UUID parse +/// +/// Ids have been `uuid::Uuid::new_v4().to_string()` since the first commit, so +/// a UUID parse would be correct today — and would silently strand a project +/// whose id a user or an import tool wrote by hand. What actually has to hold +/// is that the id cannot leave the path segment it is interpolated into, and +/// alphanumerics plus `-`/`_` cannot: no `/`, no `.` (so no `..`), no `?`, `#`, +/// `%`, `:` or `@`. That alphabet is also exactly what +/// `migration_store::sanitize` leaves untouched, so an id that passes here +/// names the same tombstone file it did before. +pub fn is_project_id(candidate: &str) -> bool { + !candidate.is_empty() + && candidate.len() <= PROJECT_ID_MAX_LEN + && candidate + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_') +} + /// The Windows/WSL2 caveat, in one place so the UI and the logs cannot drift. /// /// Docker Desktop keeps the whole daemon inside `ext4.vhdx` under @@ -1112,14 +1154,36 @@ pub fn is_docker_desktop(operating_system: &str) -> bool { /// volume looks unclaimed. Deleting them would take the user's credentials, /// transcripts and toolchains for every project they have. /// -/// So an empty list is only believed when the file is *also* absent, which is -/// the genuine fresh-install case and the one where there is nothing on the -/// daemon to mis-attribute anyway. Anything else returns the reason, and orphan -/// detection is suppressed rather than run optimistically. +/// ## Why an empty list is never believed, and why the shape of the list is not +/// the test any more +/// +/// The old rule was "an empty list is believed when the file is *also* absent", +/// on the reasoning that a missing file is the fresh-install case and a fresh +/// install has nothing on the daemon to mis-attribute. Both halves were wrong: +/// +/// * **A missing `projects.json` is not the same as a new one.** A data +/// directory that was moved, restored from a partial backup, or reached with +/// a different `XDG_DATA_HOME` than the one the app ran under before looks +/// identical, and every one of those has a daemon full of live volumes behind +/// it. And a genuinely fresh install loses nothing by being refused, because +/// there is by definition nothing for it to find — which is what makes +/// refusing the cheap side of the trade. +/// * **"The list is empty" stops being true the moment anything is saved.** +/// `ProjectsStore::new()` swallows a corrupt file into an empty list *without +/// rewriting it*, and the next `save()` — `update_status()`, i.e. merely +/// starting a project — writes `[{that one project}]` over it. The list is +/// then non-empty, this guard passes, and every pre-existing project's +/// volumes are offered as orphans. Reproduced against a byte-exact replica. +/// +/// So the corrupt load is recorded on disk by the store itself and read back +/// here through `corrupt_since`, rather than being inferred from a symptom that +/// erases itself; see [`crate::storage::projects_store::corrupt_since`]. And an +/// empty `known` set is refused unconditionally. pub fn project_store_trust( projects: &[Project], json_exists: bool, json_ids: Option<&[String]>, + corrupt_since: Option<&str>, ) -> Result, String> { let Some(json_ids) = json_ids else { return Err( @@ -1141,19 +1205,39 @@ pub fn project_store_trust( let mut known: HashSet = json_ids.iter().cloned().collect(); known.extend(projects.iter().map(|p| p.id.clone())); - if known.is_empty() && json_exists { - return Err( + // **Checked before the list is looked at, because the list looks fine.** + // This is the state where `known` is non-empty, parses, and is still + // missing every project that was in the file before it stopped parsing. + if let Some(since) = corrupt_since { + return Err(format!( + "This data directory loaded a projects.json it could not parse ({}), so the project \ + list here is known to be incomplete and a live project's volumes cannot be told \ + from an orphan. A copy of the unreadable file was kept beside it as \ + projects.json.bak. Restore it, or delete the projects.json.corrupt marker next to \ + it to say the loss is accepted.", + since + )); + } + + if known.is_empty() { + return Err(if json_exists { "The project list loaded empty from a projects.json that exists, which is what a \ recovered-from-corrupt store looks like. Orphan detection is suppressed rather than \ treat every project's volumes as unclaimed." - .to_string(), - ); + .to_string() + } else { + "There is no projects.json in this data directory, so there is no list to tell a \ + live project's volumes from an orphan — a moved or partially restored data \ + directory looks exactly like a fresh install. Nothing is listed here until one \ + exists." + .to_string() + }); } Ok(known) } -/// Re-read `projects.json` from disk: whether it exists, and the project ids it -/// actually holds. +/// Re-read `projects.json` from disk: whether it exists, the project ids it +/// actually holds, and whether this data directory has ever failed to parse it. /// /// The in-memory store cannot answer either question. By the time it is /// consulted a corrupt file has already been swallowed into an empty list, and @@ -1164,14 +1248,19 @@ pub fn project_store_trust( /// is not a string is skipped rather than failing the whole read — the file is /// still parseable, so the honest answer is the ids it does carry. /// +/// The third element is the sticky corrupt-load marker +/// (`projects_store::corrupt_since`), read on the same pass because a file that +/// parses *now* says nothing about whether the ids in it are all of them. +/// /// Blocking `std::fs`, so every async caller goes through /// [`projects_json_snapshot_async`]. -fn projects_json_snapshot() -> (bool, Option>) { +fn projects_json_snapshot() -> (bool, Option>, Option) { + let corrupt_since = crate::storage::projects_store::corrupt_since(); let Some(path) = dirs::data_dir().map(|d| d.join("triple-c").join("projects.json")) else { - return (false, None); + return (false, None, corrupt_since); }; if !path.exists() { - return (false, Some(Vec::new())); + return (false, Some(Vec::new()), corrupt_since); } let parsed = std::fs::read_to_string(&path) .ok() @@ -1182,7 +1271,7 @@ fn projects_json_snapshot() -> (bool, Option>) { .filter_map(|entry| entry.get("id")?.as_str().map(str::to_string)) .collect::>() }); - (true, parsed) + (true, parsed, corrupt_since) } /// [`projects_json_snapshot`] off the async worker. @@ -1191,10 +1280,10 @@ fn projects_json_snapshot() -> (bool, Option>) { /// Windows volume behind an antivirus filter blocks the whole tokio worker /// thread it lands on, and this one is called from inside [`scan`] — the /// longest-running command in the app. -async fn projects_json_snapshot_async() -> (bool, Option>) { +async fn projects_json_snapshot_async() -> (bool, Option>, Option) { tokio::task::spawn_blocking(projects_json_snapshot) .await - .unwrap_or((false, None)) + .unwrap_or((false, None, None)) } // --------------------------------------------------------------------------- @@ -1445,9 +1534,14 @@ pub async fn scan(projects: &[Project]) -> Result { // as exact. let base_images_bytes = base_images.iter().map(|b| b.bytes).sum(); - let (json_exists, json_ids) = projects_json_snapshot_async().await; + let (json_exists, json_ids, corrupt_since) = projects_json_snapshot_async().await; let (orphan_volumes_list, orphan_volumes_unavailable) = - match project_store_trust(projects, json_exists, json_ids.as_deref()) { + match project_store_trust( + projects, + json_exists, + json_ids.as_deref(), + corrupt_since.as_deref(), + ) { Ok(known) => { let facts: Vec = volumes.iter().map(volume_facts).collect(); (orphan_volumes(&facts, &known, true), None) @@ -2843,7 +2937,9 @@ async fn reclaim_migration_pins() -> ReclaimResult { continue; } // Starts the clock when nothing has sighted this pin before, which - // is what makes the first pass a no-op for it. + // is what makes the first pass a no-op for it. The window between + // the `has_record` above and this write is closed inside + // `note_ownerless_since` — see its docs. let ownerless_since = migration_store::note_ownerless_since(&project_id, &tag, &now); if pin_disposition(&tag, has_record, ownerless_since, &now) != PinDisposition::Reapable @@ -3026,34 +3122,103 @@ async fn reclaim_containers( } } -/// Remove one orphaned volume, re-checking every safety condition first. -/// -/// The plan that offered this was computed against a `df()` from some seconds -/// ago. A project could have been added since — by this app or by a second copy -/// of it — and a container could have attached. So the store is re-consulted -/// *from disk*, the name is re-parsed and the live ref count is re-read here: -/// the typed confirmation is permission to act, not a promise that the world -/// stood still. Both of those re-checks predate the move to the destructive -/// path and are deliberately unchanged. /// Drop a rollback pin whose project is no longer in `projects.json`. /// -/// The owned case ([`destroy`]'s `RollbackPin` arm) takes the project's claim -/// and clears the ownerless marker. Neither applies here: there is no project -/// to claim, and nothing else can be mid-operation on an id the store does not -/// know. What *does* still apply is the tag validation — this is the one -/// destructive variant carrying a free-form string over IPC, and `latest` would -/// name a live snapshot rather than a pin. +/// ## Both of its strings come from IPC, and both are checked +/// +/// This arm is reached *because* `find_project` failed, which means +/// `project_id` matched nothing and is an unconstrained string straight off the +/// wire — and it was interpolated into `triple-c-snapshot-{id}:{tag}` and handed +/// to bollard, which does not percent-encode a path. See [`is_project_id`] for +/// the traversal that bought and the daemon transcript that confirmed it. The +/// tag check was always here (`latest` would name the project's live snapshot); +/// the id check is the other half of the same rule and is applied first, +/// because a malformed id makes every later refusal message a lie about what +/// would have been touched. +/// +/// [`confirmation_matches`] is not a barrier for either: it compares the +/// caller's own two strings, so a crafted id is simply typed back verbatim. +/// +/// ## And it re-derives the reference from the daemon +/// +/// Validation says the id *could* name a pin; it does not say one exists. So +/// the daemon is asked for `triple-c-snapshot-*:pre-migration-*` and the +/// reference that is actually removed is **the daemon's own string**, matched +/// on the `(project_id, tag)` pair. Nothing built out of IPC input reaches +/// `remove_image` at all — the same shape as [`destroy_orphan_volume`], which +/// discards the frontend's `project_id` and re-derives it from a volume name +/// the daemon reported. +/// +/// ## Guards its sibling applies, that this used to skip +/// +/// Ownership was decided from the *in-memory* project list alone, and that was +/// the only check: no `project_store_trust`, no `has_record`, no lock. A +/// corrupt `projects.json` makes every live project's pin look ownerless — and +/// this arm calls `sweep_orphaned_snapshots()`, which deletes the freshly +/// dangling image on the same pass. That was the one construction that could +/// reap a pin whose migration is still **awaiting confirmation**, the invariant +/// `pin_is_reapable` orders its conditions to protect. So, in order: +/// +/// 1. The store is re-read *from disk* and put through `project_store_trust`, +/// which refuses outright when it cannot be trusted. +/// 2. An id the trusted store *does* know is refused: this is the ownerless +/// path, and a pin with an owner belongs to [`destroy`]'s owned arm, which +/// asks for the project's name rather than its id. +/// 3. The lock is taken **before** anything is read that a decision rests on. +/// The old comment justified returning before the lock with "nothing else +/// can be mid-operation on an id the store does not know", which is false: +/// a migration holds the lock under exactly the id being typed here, and it +/// is the thing most likely to be writing the record checked next. +/// 4. `has_record` — filesystem presence, not `load`, the same conservative +/// question both reapers ask. A record still claims this pin. async fn destroy_ownerless_rollback_pin( project_id: &str, tag: &str, + projects: &[Project], ) -> Result { + if !is_project_id(project_id) { + return Err(format!( + "{:?} is not a project id. Nothing was removed.", + project_id + )); + } if migration::parse_rollback_tag(tag).is_none() { return Err(format!( "{:?} is not a rollback pin tag. Nothing was removed.", tag )); } - let reference = format!("triple-c-snapshot-{}:{}", project_id, tag); + + let (json_exists, json_ids, corrupt_since) = projects_json_snapshot_async().await; + let known = project_store_trust( + projects, + json_exists, + json_ids.as_deref(), + corrupt_since.as_deref(), + )?; + if known.contains(project_id) { + return Err(format!( + "Project {} is in your project list after all, so this pin is not ownerless. \ + Reopen the disk panel and confirm it by project name instead. Nothing was removed.", + project_id + )); + } + + let _guard = + crate::project_lock::try_acquire(project_id, crate::project_lock::ProjectOp::Destroy)?; + + if migration_store::has_record(project_id).unwrap_or(true) { + return Err(format!( + "A migration record for {} is still on disk, so something can still ask for this \ + rollback. Nothing was removed.", + project_id + )); + } + + // The reference the daemon itself reports for this pin. `None` means the + // plan is stale — the pin was reaped or confirmed since the panel drew it. + let reference = live_rollback_pin_reference(project_id, tag).await?; + migration::untag_image(&reference).await?; // The grace clock is meaningless once the tag is gone, and the marker file // would otherwise outlive everything that could ever read it. @@ -3081,11 +3246,68 @@ async fn destroy_ownerless_rollback_pin( }) } +/// The daemon's own `repo_tag` for one rollback pin, or why it is not there. +/// +/// The point is the *provenance* of the returned string, not the existence +/// check: it is a tag Docker listed, so removing it cannot address anything but +/// an image. A reference assembled here from a caller's id would be a string +/// the daemon never vouched for, which is the whole of C-2. +/// +/// The `reference` filter matches on `repo:tag` and is the same one every other +/// pin path uses, so this asks for the shape `rollback_tag` produces and +/// nothing else. The match is still made on the parsed `(project_id, tag)` pair +/// rather than on the filter's word, because a filter is never the only guard +/// on a removal here. +async fn live_rollback_pin_reference(project_id: &str, tag: &str) -> Result { + let docker = get_docker()?; + let images = docker + .list_images(Some(ListImagesOptions { + all: false, + filters: HashMap::from([( + "reference".to_string(), + vec!["triple-c-snapshot-*:pre-migration-*".to_string()], + )]), + ..Default::default() + })) + .await + .map_err(|e| format!("Could not re-check rollback pins: {}", e))?; + + images + .iter() + .flat_map(|image| image.repo_tags.iter()) + .find(|reference| { + migration::parse_snapshot_reference(reference) + .is_some_and(|(id, t)| id == project_id && t == tag) + }) + .cloned() + .ok_or_else(|| { + format!( + "There is no rollback pin {} for {} on the daemon any more. Nothing was \ + removed.", + tag, project_id + ) + }) +} + +/// Remove one orphaned volume, re-checking every safety condition first. +/// +/// The plan that offered this was computed against a `df()` from some seconds +/// ago. A project could have been added since — by this app or by a second copy +/// of it — and a container could have attached. So the store is re-consulted +/// *from disk*, the name is re-parsed and the live ref count is re-read here: +/// the typed confirmation is permission to act, not a promise that the world +/// stood still. Both of those re-checks predate the move to the destructive +/// path and are deliberately unchanged. async fn destroy_orphan_volume(name: &str, projects: &[Project]) -> Result { let docker = get_docker()?; - let (json_exists, json_ids) = projects_json_snapshot_async().await; - let known = project_store_trust(projects, json_exists, json_ids.as_deref())?; + let (json_exists, json_ids, corrupt_since) = projects_json_snapshot_async().await; + let known = project_store_trust( + projects, + json_exists, + json_ids.as_deref(), + corrupt_since.as_deref(), + )?; let usage = docker .df() @@ -3154,6 +3376,22 @@ pub async fn compact_snapshot(project: &Project) -> ReclaimResult { project_id: project.id.clone(), }; + // The same guard [`destroy`] applies, for the same reason: this resolves + // `triple-c-snapshot-{id}:latest`, builds `…:compacting`, names a + // `triple-c-compact-*` container and commits back over `:latest`. An id + // that cannot be safely interpolated into a Docker name is refused before + // any of that starts rather than at whichever step happens to notice. + if !is_project_id(&project.id) { + return failed( + target, + format!( + "Project {:?} has an id that is not a project id, so its snapshot cannot be \ + addressed safely on the daemon.", + project.name + ), + ); + } + // **Acquired, and held for the whole rewrite.** This is the operation the // per-project lock was written for. Compaction resolves // `triple-c-snapshot-{id}:latest` when its build starts and commits back @@ -3830,15 +4068,35 @@ pub async fn destroy( if find_project(projects, target.project_id()).is_err() { if !confirmation_matches(project_id, confirmation) { return Err(format!( - "This pin's project is no longer in Triple-C, so there is no name to type. Type the project id ({}) exactly to confirm. Nothing was removed.", + "This pin's project is no longer in Triple-C, so there is no name to type. \ + Type the project id ({}) exactly to confirm. Nothing was removed.", project_id )); } - return destroy_ownerless_rollback_pin(project_id, tag).await; + // **The in-memory list is not the ownership decision.** It is only + // what routes to this arm; `destroy_ownerless_rollback_pin` re-reads + // `projects.json` and refuses if the store cannot be trusted, or if + // it turns out to know this id after all. + return destroy_ownerless_rollback_pin(project_id, tag, projects).await; } } let project = find_project(projects, target.project_id())?; + // **Every arm below interpolates this id into a Docker object name** — two + // volume names, a snapshot reference, a rollback reference — and bollard + // puts those straight into a request path without encoding them. The id + // came from the store rather than from IPC here, which is a weaker source + // than it sounds: `projects.json` is a plain file a user or an import tool + // can write. One check in front of all four is cheaper than four, and the + // only thing it can refuse is a project whose id was never one this app + // generated. See [`is_project_id`]. + if !is_project_id(&project.id) { + return Err(format!( + "Project {:?} has an id that is not a project id, so nothing of its can be addressed \ + safely on the daemon. Nothing was removed.", + project.name + )); + } if !confirmation_matches(&project.name, confirmation) { return Err(format!( "Type the project name ({}) exactly to confirm. Nothing was removed.", @@ -3941,13 +4199,19 @@ pub async fn destroy( Err("An orphaned volume is not a project object.".to_string()) } DestructiveTarget::RollbackPin { tag, .. } => { - // **The one destructive variant carrying a free-form string.** - // Every other arm builds its target from constants; this one takes - // a tag over IPC and interpolates it into an image reference that - // is then removed. Unvalidated, `tag: "latest"` names the project's - // live snapshot — deleted under a dialog that says "rollback pin". + // **The variant carrying a free-form string.** Every other arm + // builds its target from constants; this one takes a tag over IPC + // and interpolates it into an image reference that is then removed. + // Unvalidated, `tag: "latest"` names the project's live snapshot — + // deleted under a dialog that says "rollback pin". // `parse_rollback_tag` accepts only `pre-migration-`, // which is exactly what `rollback_tag` produces and nothing else. + // + // The *id* in the reference below is `project.id`, taken from the + // record `find_project` matched — not the one the caller sent. That + // is what makes this arm safe and is the difference from + // `destroy_ownerless_rollback_pin`, which has no record to match + // and must validate the id itself; see [`is_project_id`]. if migration::parse_rollback_tag(tag).is_none() { return Err(format!( "{:?} is not a rollback pin tag. Nothing was removed.", diff --git a/app/src-tauri/src/docker/disk_tests.rs b/app/src-tauri/src/docker/disk_tests.rs index 5afe99d..02066c2 100644 --- a/app/src-tauri/src/docker/disk_tests.rs +++ b/app/src-tauri/src/docker/disk_tests.rs @@ -482,7 +482,7 @@ fn project(id: &str, name: &str) -> Project { #[test] fn an_unreadable_projects_json_is_never_trusted() { - let err = project_store_trust(&[project("a", "api")], true, None).unwrap_err(); + let err = project_store_trust(&[project("a", "api")], true, None, None).unwrap_err(); assert!(err.contains("could not be read"), "{}", err); } @@ -492,18 +492,53 @@ fn an_empty_list_from_an_existing_file_is_treated_as_a_failed_load() { // up and starts empty. That is right for the app and catastrophic here, so // the combination "empty list + file present" is refused rather than read as // "the user has no projects". - let err = project_store_trust(&[], true, Some(&[])).unwrap_err(); + let err = project_store_trust(&[], true, Some(&[]), None).unwrap_err(); assert!(err.contains("suppressed"), "{}", err); +} - // No file at all is a genuine fresh install, and there is nothing on the - // daemon to mis-attribute in that state. - assert!(project_store_trust(&[], false, Some(&[])).unwrap().is_empty()); +#[test] +fn a_missing_projects_json_is_not_evidence_of_a_fresh_install() { + // H-3: this used to return `Ok(empty)` here, on the reasoning that no file + // means a fresh install and a fresh install has nothing to mis-attribute. + // A data directory that was moved, partially restored, or reached under a + // different XDG_DATA_HOME is indistinguishable from that — and has a daemon + // full of live volumes behind it. The genuinely fresh case loses nothing by + // being refused, because there is nothing there for it to find. + let err = project_store_trust(&[], false, Some(&[]), None).unwrap_err(); + assert!(err.contains("no projects.json"), "{}", err); +} + +#[test] +fn a_recorded_corrupt_load_is_not_undone_by_the_next_write() { + // H-3, the shape that made the old guard evaporate: `ProjectsStore::new()` + // swallows a corrupt file into an empty list *without rewriting it*, and + // the first `save()` after that — `update_status()`, i.e. merely starting a + // project — writes `[{that one project}]` over it. `known` is then + // non-empty and the "empty list + file present" guard passes, so every + // pre-existing project's volumes are offered as orphans. + // + // The store's sticky marker is what survives that write, so it is checked + // before the list is looked at at all. + let err = project_store_trust( + &[project("the-one-project-started-since", "api")], + true, + Some(&["the-one-project-started-since".to_string()]), + Some("2026-08-23T00:00:00Z"), + ) + .unwrap_err(); + assert!(err.contains("could not parse"), "{}", err); + assert!( + err.contains("projects.json.corrupt"), + "the refusal must name the marker the user has to delete: {}", + err + ); } #[test] fn a_healthy_store_yields_its_ids() { let ids = - project_store_trust(&[project("a", "api"), project("b", "web")], true, Some(&[])).unwrap(); + project_store_trust(&[project("a", "api"), project("b", "web")], true, Some(&[]), None) + .unwrap(); assert_eq!(ids, HashSet::from(["a".to_string(), "b".to_string()])); } @@ -518,6 +553,7 @@ fn a_project_only_the_file_knows_about_still_counts_as_live() { &[project("a", "api")], true, Some(&["a".to_string(), "b-from-the-other-window".to_string()]), + None, ) .unwrap(); assert!( @@ -1592,3 +1628,346 @@ async fn an_ownerless_rollback_pin_still_refuses_a_tag_that_is_not_a_pin() { err ); } + +/// The exact string that was reproduced against the live daemon: an id built to +/// climb out of `/images/` and land on `/volumes/`, with a trailing `?` to +/// swallow the tag into a query string bollard then overwrites with its own. +const C2_TRAVERSAL_ID: &str = "a/../../v1.47/volumes/tcaudit-victim?"; + +#[test] +fn a_project_id_cannot_leave_the_path_segment_it_is_interpolated_into() { + // C-2. `format!("triple-c-snapshot-{}:{}", project_id, tag)` goes to + // bollard, which does not percent-encode: `Uri::parse` joins an absolute + // path onto the base URL, which *replaces* the path and applies RFC 3986 + // dot-segment removal. Verified against the daemon: this id turned a + // `DELETE /v1.47/images/…` into `DELETE /v1.47/volumes/tcaudit-victim`, + // answered 204, and the volume was gone. + assert!(!is_project_id(C2_TRAVERSAL_ID)); + // Each ingredient on its own, so a future relaxation of the alphabet has to + // fail here rather than only in the composed payload. + assert!(!is_project_id("a/b"), "a path separator escapes the segment"); + assert!(!is_project_id(".."), "dot segments are what the join removes"); + assert!(!is_project_id("a?x=1"), "a query terminates the path"); + assert!(!is_project_id("a#f"), "a fragment terminates the path"); + assert!(!is_project_id("a%2f"), "percent-encoding is decoded by the daemon"); + assert!(!is_project_id("a:b"), "a colon would forge the tag separator"); + assert!(!is_project_id("a b")); + assert!(!is_project_id("")); + assert!(!is_project_id(&"a".repeat(PROJECT_ID_MAX_LEN + 1))); + + // The real shape has to survive untouched, or every existing pin becomes + // undeletable through the panel that exists to find it. + assert!(is_project_id("dead0000-0000-0000-0000-000000000000")); + assert!(is_project_id(&Project::new("api".to_string(), Vec::new()).id)); + // And the alphabet is the one `migration_store::sanitize` leaves alone, so + // an id that passes here names the same tombstone file it always did. + assert!(is_project_id("legacy_id-2")); +} + +#[tokio::test] +async fn an_ownerless_rollback_pin_refuses_an_id_that_is_not_a_project_id() { + // C-2 end to end, through the public entry point. Note the confirmation: + // it is the crafted id typed back verbatim, which `confirmation_matches` + // accepts — it compares the caller's own two strings and was never a + // barrier here. The refusal has to come from the id check. + let projects: Vec = Vec::new(); + let target = DestructiveTarget::RollbackPin { + project_id: C2_TRAVERSAL_ID.to_string(), + tag: "pre-migration-20260101-101500".to_string(), + }; + let err = destroy(&target, C2_TRAVERSAL_ID, &projects) + .await + .expect_err("a traversal id must never reach the Docker API"); + // Remove `is_project_id` from `destroy_ownerless_rollback_pin` and this is + // the assertion that fails: the call gets as far as the store re-read and + // comes back with some other refusal — or, with a store present and a + // daemon behind it, with a 204 and somebody's volume gone. + assert!( + err.contains("not a project id"), + "the id must be refused before anything else is consulted, got: {}", + err + ); + assert!(err.contains("Nothing was removed"), "{}", err); +} + +// --------------------------------------------------------------------------- +// C-2 and H-3, against a real daemon and a real data directory +// --------------------------------------------------------------------------- + +/// Serialises the tests that redirect `XDG_DATA_HOME`. +/// +/// The process environment is process-wide and `cargo test` runs test functions +/// on a thread pool, so two of these racing would have one reading the other's +/// data directory. `#[ignore]` keeps them out of the default run; this keeps +/// them out of each other's way when the whole ignored set is run at once. +fn env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::OnceLock> = std::sync::OnceLock::new(); + LOCK.get_or_init(|| std::sync::Mutex::new(())) + .lock() + .unwrap_or_else(|e| e.into_inner()) +} + +/// H-3 as it actually unfolds, against a real `ProjectsStore` on a real disk. +/// +/// `#[ignore]` because it redirects `XDG_DATA_HOME` for the whole process — +/// `cargo test` has to stay free of that. Run it with +/// `cargo test -- --ignored a_corrupt_projects_json_stays_untrusted`. +/// +/// The sequence is the one that was reproduced against a byte-exact replica of +/// a real data directory, and every step of it is ordinary app behaviour: +/// +/// 1. `projects.json` holds two live projects and stops parsing. +/// 2. `ProjectsStore::new()` backs it up, starts empty, and the app runs on. +/// 3. The user starts a project. `update_status()` calls `save()`, which writes +/// `[{that one project}]` over the file. +/// 4. `projects.json` now parses and holds one id — so "the list loaded empty +/// from a file that exists" is false, the guard passes, and both original +/// projects' home and config volumes are offered as orphans at +/// `Safety::Safe`. +/// +/// Step 4 is what this asserts is no longer reachable. +#[test] +#[ignore] +fn a_corrupt_projects_json_stays_untrusted_after_the_next_write() { + let _env = env_lock(); + let data_home = + std::env::temp_dir().join(format!("triple-c-h3-{}", uuid::Uuid::new_v4().simple())); + let dir = data_home.join("triple-c"); + std::fs::create_dir_all(&dir).expect("temp data dir"); + let previous = std::env::var_os("XDG_DATA_HOME"); + std::env::set_var("XDG_DATA_HOME", &data_home); + + // 1. Two live projects, and a file that no longer parses. + std::fs::write(dir.join("projects.json"), "[{\"id\":\"live-a\"},{\"id\":\"liv") + .expect("corrupt projects.json"); + + // 2. The app starts and recovers. + let store = crate::storage::projects_store::ProjectsStore::new().expect("store"); + assert!(store.list().is_empty(), "the corrupt file loads as an empty list"); + + // 3. The user adds and starts a project — the ordinary write that used to + // erase the only evidence of step 1. + let started = store + .add(project("new-project-added-after-the-corruption", "api")) + .expect("add"); + store + .update_status(&started.id, crate::models::ProjectStatus::Running) + .expect("update_status"); + let rewritten = std::fs::read_to_string(dir.join("projects.json")).unwrap(); + assert!( + rewritten.contains("new-project-added-after-the-corruption") + && !rewritten.contains("live-a"), + "the precondition for H-3 is that the file now parses and has lost the old ids: {}", + rewritten + ); + + // 4. The state the old guard could not see. + let (json_exists, json_ids, corrupt_since) = projects_json_snapshot(); + assert!(json_exists); + assert_eq!(json_ids.as_deref().map(<[String]>::len), Some(1)); + assert!( + corrupt_since.is_some(), + "the corrupt load must still be on the record after the rewrite" + ); + let err = project_store_trust( + &store.list(), + json_exists, + json_ids.as_deref(), + corrupt_since.as_deref(), + ) + .unwrap_err(); + assert!(err.contains("could not parse"), "{}", err); + + // And the consequence: `live-a`'s volumes are not offered to anyone. + let volumes = vec![VolumeFacts { + name: "triple-c-home-live-a".to_string(), + bytes: 8_000_000_000, + links: 0, + created_at: None, + }]; + assert!( + orphan_volumes(&volumes, &HashSet::new(), false).is_empty(), + "a suppressed store must offer nothing" + ); + + std::fs::remove_dir_all(&data_home).ok(); + match previous { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +/// The traversal, run for real, plus the legitimate case it must not break. +/// +/// `#[ignore]` for the same reason as the compaction test: `cargo test` has to +/// stay daemon-free. Run it with +/// `cargo test -- --ignored a_traversal_project_id_cannot_reach_the_docker_api`. +/// +/// It exists because the unit test above can only assert a *message*. The thing +/// that made C-2 critical is not that a string was unvalidated, it is that the +/// daemon honoured what the string turned the request into — and no pure test +/// can show that. This one creates a volume, points a rollback-pin removal at +/// it through `destroy`, and checks the volume is still there afterwards. +/// +/// It also does the other half, which is the half a validator can easily break: +/// a genuinely ownerless pin still has to be removable, or the panel that +/// exists to find multi-gigabyte orphans can no longer act on them. +/// +/// `XDG_DATA_HOME` is redirected for the duration so the store re-read, the +/// migration records and the ownerless tombstones all land in a temp directory +/// rather than the developer's real one. +#[tokio::test] +#[ignore] +async fn a_traversal_project_id_cannot_reach_the_docker_api() { + let _env = env_lock(); + let run = uuid::Uuid::new_v4().simple().to_string(); + let data_home = std::env::temp_dir().join(format!("triple-c-c2-{}", run)); + std::fs::create_dir_all(data_home.join("triple-c")).expect("temp data dir"); + // A store that parses and knows about *some other* project, so + // `project_store_trust` is satisfied and the id under test is genuinely not + // in it. `projects_json_snapshot` reads ids as opaque JSON strings. + std::fs::write( + data_home.join("triple-c").join("projects.json"), + r#"[{"id":"c2-some-other-project"}]"#, + ) + .expect("temp projects.json"); + let previous_data_home = std::env::var_os("XDG_DATA_HOME"); + std::env::set_var("XDG_DATA_HOME", &data_home); + + let docker = get_docker().expect("a daemon"); + + // ---- 1. The attack ----------------------------------------------------- + let victim = format!("triple-c-c2-victim-{}", run); + docker + .create_volume(bollard::volume::CreateVolumeOptions { + name: victim.clone(), + ..Default::default() + }) + .await + .expect("create the victim volume"); + + // `a/` gives the join something to climb out of; the two `..` segments eat + // `triple-c-snapshot-a` and `images`; the trailing `?` turns `:{tag}` into + // a query string, which bollard then replaces with `force`/`noprune`. + let traversal_id = format!("a/../../v1.47/volumes/{}?", victim); + let target = DestructiveTarget::RollbackPin { + project_id: traversal_id.clone(), + tag: "pre-migration-20250101-000000".to_string(), + }; + // The confirmation is the crafted id itself — which is exactly why the + // typed confirmation was never a defence here. + let err = destroy(&target, &traversal_id, &[]) + .await + .expect_err("a traversal id must be refused"); + assert!(err.contains("not a project id"), "got: {}", err); + assert!( + docker.inspect_volume(&victim).await.is_ok(), + "the volume was deleted by an image-tag removal — C-2 is not fixed" + ); + + // ---- 2. The legitimate case, which must still work --------------------- + let orphan_id = uuid::Uuid::new_v4().to_string(); + let tag = "pre-migration-20250101-000000"; + let pin = format!("triple-c-snapshot-{}:{}", orphan_id, tag); + // Deliberately *not* labelled `triple-c.managed`: this test's leftovers + // must never be something `sweep_orphaned_snapshots` can act on, and the + // sweep is called on the success path. + build_from_dockerfile("FROM busybox:1.36 +RUN touch /c2-pin +", &pin) + .await + .expect("build the pin image"); + + let result = destroy( + &DestructiveTarget::RollbackPin { + project_id: orphan_id.clone(), + tag: tag.to_string(), + }, + &orphan_id, + &[], + ) + .await + .expect("a well-formed ownerless pin must still be removable"); + assert!(result.ok, "{}", result.message); + assert!( + docker.inspect_image(&pin).await.is_err(), + "the pin tag survived the removal that reported success" + ); + + // ---- cleanup ----------------------------------------------------------- + let _ = docker.remove_volume(&victim, None).await; + let _ = docker + .remove_image( + &pin, + Some(RemoveImageOptions { + force: true, + noprune: false, + }), + None, + ) + .await; + let _ = std::fs::remove_dir_all(&data_home); + match previous_data_home { + Some(v) => std::env::set_var("XDG_DATA_HOME", v), + None => std::env::remove_var("XDG_DATA_HOME"), + } +} + +#[tokio::test] +async fn an_owned_target_is_refused_when_the_stored_id_is_not_a_project_id() { + // The owned arms take their id from the record `find_project` matched + // rather than from IPC — which is a weaker source than it sounds, because + // `projects.json` is a plain file. All four of them (two volume names, the + // snapshot reference, the rollback reference) go on to be interpolated into + // a Docker request path, so one check stands in front of all four. + let mut bad = project(C2_TRAVERSAL_ID, "api"); + bad.name = "api".to_string(); + let projects = vec![bad]; + for target in [ + DestructiveTarget::HomeVolume { + project_id: C2_TRAVERSAL_ID.to_string(), + }, + DestructiveTarget::SnapshotImage { + project_id: C2_TRAVERSAL_ID.to_string(), + }, + DestructiveTarget::RollbackPin { + project_id: C2_TRAVERSAL_ID.to_string(), + tag: "pre-migration-20260101-101500".to_string(), + }, + ] { + let err = destroy(&target, "api", &projects) + .await + .expect_err("a stored id that is not a project id must be refused"); + assert!(err.contains("not a project id"), "{:?}: {}", target, err); + // And before the confirmation is even compared, so the refusal cannot + // be walked past by typing the right name. + assert!(err.contains("Nothing was removed"), "{}", err); + } +} + +#[tokio::test] +async fn the_ownerless_arm_will_not_touch_a_pin_the_store_still_claims() { + // H-1. Ownership was decided from the in-memory list alone, and this arm + // then calls `sweep_orphaned_snapshots()`, which deletes the freshly + // dangling image on the same pass — so a `projects.json` that failed to + // load made every live project's pin "ownerless", including one whose + // migration is still awaiting confirmation. The re-read is the fix; here it + // is the union with the in-memory list that carries the id. + // + // Called directly rather than through `destroy`, because `destroy` routes + // by the in-memory list and a project that is *in* it never reaches this + // arm. The point is that the arm refuses on its own account. + let projects = vec![project("dead0000-0000-0000-0000-000000000000", "api")]; + let err = destroy_ownerless_rollback_pin( + "dead0000-0000-0000-0000-000000000000", + "pre-migration-20260101-101500", + &projects, + ) + .await + .expect_err("a pin whose project the store knows is not ownerless"); + assert!( + err.contains("not ownerless"), + "the store's answer must override the caller's, got: {}", + err + ); +} diff --git a/app/src-tauri/src/docker/migration.rs b/app/src-tauri/src/docker/migration.rs index 65b08b8..b338faa 100644 --- a/app/src-tauri/src/docker/migration.rs +++ b/app/src-tauri/src/docker/migration.rs @@ -1233,6 +1233,14 @@ pub async fn reap_stale_migration_pins() -> usize { } // Records the first sighting when there is none, which is why this // returns `None` on that pass and the pin survives it. + // + // A `save` can land between the `has_record` above and this write, + // which would plant a tombstone dated *now* behind a perfectly + // valid record — invisible until that record is legitimately lost, + // at which point the pin is already past its grace period and is + // reaped on the first check. `note_ownerless_since` re-asks + // `has_record` after the write and removes the marker again; the + // reasoning for why that closes the window is on it. let ownerless_since = crate::storage::migration_store::note_ownerless_since(&project_id, &tag, &now); if !pin_is_reapable(&tag, has_record, ownerless_since, &now) { diff --git a/app/src-tauri/src/storage/migration_store.rs b/app/src-tauri/src/storage/migration_store.rs index 7a7e03d..1212828 100644 --- a/app/src-tauri/src/storage/migration_store.rs +++ b/app/src-tauri/src/storage/migration_store.rs @@ -83,9 +83,9 @@ pub fn load(project_id: &str) -> Result, String> { Ok(state) => Ok(Some(state)), Err(e) => { let backup = corrupt_backup_path(&path, &chrono::Utc::now()); - let copied = if backup.exists() { - // Already kept a copy of this exact corruption this second; - // nothing to add. + let copied = if backup.exists() || corrupt_backups_full(&path) { + // Already kept a copy of this exact corruption this second, or + // kept as many as are worth keeping. Either way nothing to add. Ok(()) } else { fs::copy(&path, &backup).map(|_| ()) @@ -114,6 +114,50 @@ fn corrupt_backup_path(path: &std::path::Path, now: &chrono::DateTime bool { + let (Some(dir), Some(stem)) = (path.parent(), path.file_stem()) else { + return false; + }; + // `{stem}.json.corrupt-` — the same shape `corrupt_backup_path` builds, so + // this can never match another project's copies or an unrelated `.bak`. + let prefix = format!("{}.json.corrupt-", stem.to_string_lossy()); + let Ok(entries) = fs::read_dir(dir) else { + return false; + }; + entries + .flatten() + .filter(|e| { + let name = e.file_name().to_string_lossy().to_string(); + name.starts_with(&prefix) && name.ends_with(".bak") + }) + .count() + >= MAX_CORRUPT_BACKUPS +} + /// Whether a project has a migration record on disk *at all*, without parsing /// it. /// @@ -251,6 +295,22 @@ pub fn peek_ownerless_since( /// the period below what has actually elapsed on the *marker's* terms, because /// there is nothing to compare against but wall time; what it cannot do any /// more is make every pin instantly reapable, which dating from the tag did. +/// +/// ## Why the write re-checks `has_record` +/// +/// Both reapers ask [`has_record`] and only call this when the answer is no, +/// which leaves a window: a [`save`] landing between the two runs its +/// `clear_ownerless_for_project` against a marker that does not exist yet, and +/// this then plants one — dated *now* — behind a perfectly valid record. The +/// marker is invisible while the record stands, so nothing notices. It only +/// matters later, if that record is legitimately lost: the pin is then already +/// fourteen days ownerless on its very first check and is reaped with **zero** +/// grace, which is the exact failure the tombstone exists to prevent. +/// +/// So the write is followed by a second `has_record`, and a marker that turns +/// out to sit behind a record is removed again. The two orderings that remain +/// are both safe: a `save` completing *after* this re-check clears the marker +/// itself, and one completing before it is what the re-check sees. pub fn note_ownerless_since( project_id: &str, tag: &str, @@ -274,6 +334,19 @@ pub fn note_ownerless_since( tag, e ); + return None; + } + // A record that appeared while this was being written owns the pin, + // and a tombstone behind an owned pin is a fourteen-day head start + // on reaping it the moment that record is next lost. + if has_record(project_id).unwrap_or(false) { + log::debug!( + "A migration record for {} appeared while marking {} ownerless; \ + the marker was dropped again", + project_id, + tag + ); + clear_ownerless(project_id, tag); } None } @@ -339,6 +412,39 @@ pub fn clear_staging(project_id: &str) -> Result<(), String> { mod tests { use super::*; + #[test] + fn corrupt_copies_of_one_record_are_capped() { + // `load` runs on every reconcile, every survey and every reaper pass, + // and nothing repairs an unparseable record — so a persistently corrupt + // one minted a new timestamped copy every time the clock's second + // changed, and nothing ever removed them. + let dir = std::env::temp_dir().join(format!( + "triple-c-corrupt-cap-{}", + uuid::Uuid::new_v4().simple() + )); + fs::create_dir_all(&dir).expect("temp dir"); + let record = dir.join("some-project.json"); + + assert!(!corrupt_backups_full(&record), "an empty directory is not full"); + for n in 0..MAX_CORRUPT_BACKUPS { + fs::write( + dir.join(format!("some-project.json.corrupt-2026010{}-000000.bak", n)), + "x", + ) + .unwrap(); + } + assert!(corrupt_backups_full(&record)); + + // Another project's copies, and an unrelated `.bak`, are not this + // record's — the prefix is the whole point of the naming. + let other = dir.join("other-project.json"); + assert!(!corrupt_backups_full(&other)); + fs::write(dir.join("some-project.json.bak"), "x").unwrap(); + assert!(!corrupt_backups_full(&other)); + + fs::remove_dir_all(&dir).ok(); + } + #[test] fn project_ids_cannot_escape_the_migrations_directory() { assert_eq!(sanitize("../../etc/passwd"), "______etc_passwd"); diff --git a/app/src-tauri/src/storage/projects_store.rs b/app/src-tauri/src/storage/projects_store.rs index dbf4eb6..0a87bdd 100644 --- a/app/src-tauri/src/storage/projects_store.rs +++ b/app/src-tauri/src/storage/projects_store.rs @@ -1,9 +1,96 @@ use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Mutex; use crate::models::Project; +/// The sticky marker for `projects.json`: `projects.json.corrupt`, beside it. +/// +/// Derived from the file rather than from `dirs::data_dir()` so the marker +/// always lands in the directory the store is actually using — and so the +/// writer can be tested against a temp directory. +fn corrupt_marker_for(file_path: &Path) -> PathBuf { + file_path.with_extension("json.corrupt") +} + +/// `/triple-c/projects.json.corrupt`, whether or not it exists. +pub fn corrupt_marker_path() -> Option { + dirs::data_dir().map(|d| corrupt_marker_for(&d.join("triple-c").join("projects.json"))) +} + +/// When this data directory last loaded a `projects.json` it could not parse, +/// as the RFC3339 instant recorded in the marker. +/// +/// ## Why this outlives the load that wrote it +/// +/// A corrupt load is *recoverable for the app* — the list starts empty and +/// everything keeps working — and that recovery is precisely what makes it +/// dangerous for anything that reasons about which projects exist. The +/// in-memory symptom does not survive: the first [`ProjectsStore::save`] after +/// the failure, which is as little as starting one project (`update_status`), +/// writes `[{that one project}]` over the file. From then on `projects.json` +/// parses, holds one id, and looks exactly like a user with one project — while +/// every *other* project's home and config volume is on the daemon claimed by +/// nobody. +/// +/// The guard in `project_store_trust` keyed on "the list is empty and the file +/// exists", which that write silently ends. So the fact is recorded on disk +/// instead of inferred from the list's shape, and it is **sticky**: nothing in +/// this app clears it, because nothing in this app can reconstruct what the +/// unreadable file held. The refusal names the marker so a user who has +/// restored their list — or accepted the loss — can delete it deliberately. +pub fn corrupt_since() -> Option { + let raw = fs::read_to_string(corrupt_marker_path()?).ok()?; + let trimmed = raw.trim(); + if trimmed.is_empty() { + // The marker's presence is the signal; an empty one still means a + // corrupt load happened, it just cannot say when. + return Some("an unknown time".to_string()); + } + Some(trimmed.lines().next().unwrap_or(trimmed).to_string()) +} + +/// Keep the bytes of an unparseable `projects.json`, and record that it +/// happened. +/// +/// **The existing `.bak` is never overwritten.** A second corruption used to +/// clobber the first, and the first is the valuable one: it was taken before +/// the app rewrote the file with whatever it had in memory, so it is the only +/// copy that can still hold the full project list. Later ones are copies of an +/// already-degraded file and get a timestamped name. +fn record_corrupt_load(file_path: &Path, now: &chrono::DateTime) { + let first = file_path.with_extension("json.bak"); + let backup = if first.exists() { + file_path.with_extension(format!("json.corrupt-{}.bak", now.format("%Y%m%d-%H%M%S"))) + } else { + first + }; + if !backup.exists() { + if let Err(e) = fs::copy(file_path, &backup) { + log::error!("Failed to back up corrupted projects.json: {}", e); + } else { + log::error!( + "A copy of the unreadable projects.json was kept at {}", + backup.display() + ); + } + } + + let marker = corrupt_marker_for(file_path); + if marker.exists() { + // Sticky: the *first* corruption is the one that dates the loss. + return; + } + if let Err(e) = fs::write(&marker, now.to_rfc3339()) { + log::error!( + "Could not record the corrupt projects.json load at {}: {} — orphan detection will \ + not know the project list is incomplete", + marker.display(), + e + ); + } +} + pub struct ProjectsStore { projects: Mutex>, file_path: PathBuf, @@ -43,20 +130,14 @@ impl ProjectsStore { Ok(parsed) => (parsed, migrated), Err(e) => { log::error!("Failed to parse migrated projects.json: {}. Starting with empty list.", e); - let backup = file_path.with_extension("json.bak"); - if let Err(be) = fs::copy(&file_path, &backup) { - log::error!("Failed to back up corrupted projects.json: {}", be); - } + record_corrupt_load(&file_path, &chrono::Utc::now()); (Vec::new(), false) } } } Err(e) => { log::error!("Failed to parse projects.json: {}. Starting with empty list.", e); - let backup = file_path.with_extension("json.bak"); - if let Err(be) = fs::copy(&file_path, &backup) { - log::error!("Failed to back up corrupted projects.json: {}", be); - } + record_corrupt_load(&file_path, &chrono::Utc::now()); (Vec::new(), false) } } @@ -203,3 +284,89 @@ impl ProjectsStore { } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_dir(tag: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "triple-c-store-{}-{}", + tag, + uuid::Uuid::new_v4().simple() + )); + fs::create_dir_all(&dir).expect("temp dir"); + dir + } + + #[test] + fn a_corrupt_load_leaves_a_marker_the_next_write_cannot_erase() { + // H-3, the whole chain in one test. `ProjectsStore::new()` swallows an + // unparseable file into an empty list *without rewriting it*, and the + // first `save()` after that — as little as `update_status()` — writes + // `[{one project}]` over it. Everything the old guard keyed on ("the + // list is empty and the file exists") is gone at that point, while + // every *other* project's volumes are still on the daemon claimed by + // nobody. + let dir = temp_dir("corrupt"); + let file = dir.join("projects.json"); + fs::write(&file, "{ this is not a project list").unwrap(); + + let now = chrono::Utc::now(); + record_corrupt_load(&file, &now); + + let marker = corrupt_marker_for(&file); + assert!(marker.exists(), "the corrupt load must be recorded on disk"); + assert_eq!(fs::read_to_string(&marker).unwrap(), now.to_rfc3339()); + assert!( + dir.join("projects.json.bak").exists(), + "the unreadable bytes must be kept" + ); + + // The write that used to erase the evidence. The marker is a separate + // file, so it does not care. + fs::write(&file, r#"[{"id":"the-one-project-started-since"}]"#).unwrap(); + assert!(marker.exists()); + + fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_second_corruption_keeps_the_first_copy_and_the_first_date() { + // The `.bak` used to be a fixed name, so a second corruption clobbered + // the first — and the first is the only copy taken before the app + // rewrote the file with whatever it had in memory, i.e. the only one + // that can still hold the full project list. + let dir = temp_dir("second"); + let file = dir.join("projects.json"); + fs::write(&file, "original bytes").unwrap(); + let first = chrono::DateTime::parse_from_rfc3339("2026-01-01T00:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc); + record_corrupt_load(&file, &first); + + fs::write(&file, "degraded bytes").unwrap(); + let second = chrono::DateTime::parse_from_rfc3339("2026-06-01T00:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc); + record_corrupt_load(&file, &second); + + assert_eq!( + fs::read_to_string(dir.join("projects.json.bak")).unwrap(), + "original bytes", + "the first copy must survive the second corruption" + ); + assert_eq!( + fs::read_to_string(dir.join("projects.json.corrupt-20260601-000000.bak")).unwrap(), + "degraded bytes" + ); + // And the marker still dates the loss from the first failure, which is + // when the project list actually stopped being complete. + assert_eq!( + fs::read_to_string(corrupt_marker_for(&file)).unwrap(), + first.to_rfc3339() + ); + + fs::remove_dir_all(&dir).ok(); + } +}