font-store: confine recovery cleanup and preserve valid receipts

Cargo.lock

@@ -63,6 +63,12 @@ version = "0.2.21"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "683d7910e743518b0e34f1186f92494becacb047c7b6bf616c96772180fef923"
 
+[[package]]
+name = "ambient-authority"
+version = "0.0.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e9d4ee0d472d1cd2e28c97dfa124b3d8d992e10eb0a035f33f5d12e3a177ba3b"
+
 [[package]]
 name = "android-activity"
 version = "0.6.1"
@@ -514,6 +520,24 @@ dependencies = [
  "thiserror 1.0.69",
 ]
 
+[[package]]
+name = "cap-primitives"
+version = "4.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8b5f74729fd2f44701d1a8eb47e906cdb3ccd9ec0f02baad85a744b791940b18"
+dependencies = [
+ "ambient-authority",
+ "fs-set-times",
+ "io-extras",
+ "io-lifetimes 3.0.1",
+ "ipnet",
+ "maybe-owned",
+ "rustix 1.1.5",
+ "rustix-linux-procfs",
+ "windows-sys 0.61.2",
+ "winx",
+]
+
 [[package]]
 name = "cast"
 version = "0.3.0"
@@ -878,6 +902,7 @@ dependencies = [
 name = "craft-font-store"
 version = "0.1.0"
 dependencies = [
+ "cap-primitives",
  "craft-atomic",
  "craft-font-catalog",
  "flate2",
@@ -1944,6 +1969,17 @@ dependencies = [
  "percent-encoding",
 ]
 
+[[package]]
+name = "fs-set-times"
+version = "0.20.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "94e7099f6313ecacbe1256e8ff9d617b75d1bcb16a6fddef94866d225a01a14a"
+dependencies = [
+ "io-lifetimes 2.0.4",
+ "rustix 1.1.5",
+ "windows-sys 0.59.0",
+]
+
 [[package]]
 name = "fs_extra"
 version = "1.3.0"
@@ -2825,6 +2861,28 @@ dependencies = [
  "hashbrown 0.17.1",
 ]
 
+[[package]]
+name = "io-extras"
+version = "0.19.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "20fd6de4ccfcc187e38bc21cfa543cb5a302cb86a8b114eb7f0bf0dc9f8ac00f"
+dependencies = [
+ "io-lifetimes 3.0.1",
+ "windows-sys 0.59.0",
+]
+
+[[package]]
+name = "io-lifetimes"
+version = "2.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "06432fb54d3be7964ecd3649233cddf80db2832f47fec34c01f65b3d9d774983"
+
+[[package]]
+name = "io-lifetimes"
+version = "3.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2f0fb0570afe1fed943c5c3d4102d5358592d8625fda6a0007fdbe65a92fba96"
+
 [[package]]
 name = "ipnet"
 version = "2.12.2"
@@ -3145,6 +3203,12 @@ version = "0.1.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "4050469837a6ff301cd14c1f8f24f88549e6d548f24f64e2148eb0f72cebc51f"
 
+[[package]]
+name = "maybe-owned"
+version = "0.3.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "4facc753ae494aeb6e3c22f839b158aebd4f9270f55cd3c79906c45476c47ab4"
+
 [[package]]
 name = "memchr"
 version = "2.8.3"
@@ -4618,6 +4682,16 @@ dependencies = [
  "windows-sys 0.61.2",
 ]
 
+[[package]]
+name = "rustix-linux-procfs"
+version = "0.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2fc84bf7e9aa16c4f2c758f27412dc9841341e16aa682d9c7ac308fe3ee12056"
+dependencies = [
+ "once_cell",
+ "rustix 1.1.5",
+]
+
 [[package]]
 name = "rustls"
 version = "0.23.46"
@@ -6816,6 +6890,16 @@ version = "0.0.29"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "9ef0ffc427f045c0cc9ebffd6f4f91153dcd2a1547b5c3c29ee3f541e16e95c6"
 
+[[package]]
+name = "winx"
+version = "0.36.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "3f3fd376f71958b862e7afb20cfe5a22830e1963462f3a17f49d82a6c1d1f42d"
+dependencies = [
+ "bitflags 2.13.2",
+ "windows-sys 0.59.0",
+]
+
 [[package]]
 name = "wit-bindgen"
 version = "0.57.1"

crates/font-store/Cargo.toml

@@ -23,6 +23,7 @@ thiserror = { workspace = true }
 
 # The store and the network are native only; the web keeps fonts elsewhere.
 [target.'cfg(not(target_arch = "wasm32"))'.dependencies]
+cap-primitives = "4.0.3"
 craft-atomic = { workspace = true }
 craft-font-catalog = { workspace = true }
 log = "0.4"

crates/font-store/README.md

@@ -152,6 +152,10 @@ Receipts stay until cleanup succeeds and a later record write prunes them; their
 reused for a new removal while they remain recorded.
 An installed record rebuilt after corruption has no cleanup receipts; unreceipted backups are
 retained for manual recovery.
+Invalid receipt names are discarded without losing readable pack metadata or other valid receipts.
+Cleanup retains open store, Language pack, and recovery-directory handles; it rejects symlinked
+recovery ancestors. Unix cleanup resolves receipt names relative to the open recovery directory;
+Windows retains ancestor handles that prevent their replacement during cleanup.
 
 **Unpacking is hostile-input safe:** pure-Rust zstd (`ruzstd`, MIT; every frame is read, and nothing but tar padding may follow the archive) and `tar` (MIT OR Apache-2.0); regular files and the `LICENCES` folder only (no links or devices). Only the contract's names are allowed: `pack.toml`, `spelling.aff`, `spelling.dic`, `hyphenation.pat` at the root and `LICENCES/<name>` (`[A-Za-z0-9._-]`, no leading or trailing dot, no Windows device names such as `CON` or `nul.txt`). Each file at most 64 MiB, 256 MiB and 256 entries per pack. `pack.toml` is required and must name the entry's locale; the files the index promises (`spelling`, `hyphenation`) must be there. Index entries need a lowercase BCP 47 locale (listed once, not a Windows device name), a sha256 and an `https://` URL; names, licences, versions, classes and the release lose control and bidi/format characters before they are stored or listed; an optional `source` must be an `https://` URL. At most 4096 zstd frames per archive. An unreadable `language-packs.json` is copied to `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`) atomically in place, as is a missing one when pack folders are there, so no installed pack drops out of the list. Only an install or removal (under the store lock) rewrites it; listing reads the folders meanwhile and writes nothing.
 

crates/font-store/src/language_packs.rs

@@ -478,8 +478,17 @@ impl FontStore {
         Stage::in_dir_excluding(&self.pack_removal_dir()?, excluded)
     }
 
+    /// Reject redirected or non-directory recovery ancestors before staging a removal.
     fn pack_removal_dir(&self) -> Result<PathBuf, Error> {
-        let parent = self.language_packs_dir().join(".removing");
+        let packs = self.language_packs_dir();
+        match std::fs::symlink_metadata(&packs) {
+            Ok(metadata) if !metadata.is_dir() || metadata.file_type().is_symlink() => {
+                return Err(io("using the Language pack folder")(std::io::Error::other("not a plain directory")));
+            }
+            Err(error) if error.kind() != std::io::ErrorKind::NotFound => return Err(io("reading the Language pack folder")(error)),
+            _ => {}
+        }
+        let parent = packs.join(".removing");
         match std::fs::symlink_metadata(&parent) {
             Ok(metadata) if !metadata.is_dir() || metadata.file_type().is_symlink() => {
                 return Err(io("using the Language pack recovery folder")(std::io::Error::other("not a plain directory")));
@@ -497,6 +506,7 @@ impl FontStore {
             return;
         }
         let Ok(parent) = self.pack_removal_dir() else { return };
+        let Ok(root) = PackRemovalRoot::open(self.dir()) else { return };
         let ids = &installed.committed_removals;
         if ids.is_empty() {
             return;
@@ -517,7 +527,7 @@ impl FontStore {
             }
             let id = &ids[next];
             next += 1;
-            if let Err(error) = remove_committed_backup(&parent.join(id))
+            if let Err(error) = root.remove(id, &parent)
                 && error.kind() != std::io::ErrorKind::NotFound
             {
                 log::warn!("cannot clean up committed Language pack removal {id}: {error}");
@@ -549,10 +559,12 @@ impl FontStore {
         Ok(rebuilt)
     }
 
-    /// The record as written, if there is one that can be read.
+    /// Preserve readable pack metadata and valid receipts when an optional receipt is malformed.
     fn read_record(&self) -> Option<Installed> {
-        let installed: Installed = read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice(&b).ok())?;
-        installed.committed_removals.iter().all(|name| Stage::valid_name(name)).then_some(installed)
+        let mut installed: Installed =
+            read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice(&b).ok())?;
+        installed.committed_removals.retain(|name| Stage::valid_name(name));
+        Some(installed)
     }
 
     /// The installed packs as their folders say: each folder named by a locale, with the name
@@ -606,10 +618,31 @@ impl FontStore {
     }
 }
 
-fn remove_committed_backup(path: &Path) -> std::io::Result<()> {
-    #[cfg(test)]
-    tests::before_cleanup(path)?;
-    std::fs::remove_dir_all(path)
+/// Keep recovery ancestors open until cleanup ends. Unix operations use directory-relative
+/// handles; Windows directory handles deny deletion and renaming while they remain open.
+/// The caller-selected store is the ambient authority boundary, not an untrusted child path.
+struct PackRemovalRoot {
+    _store: std::fs::File,
+    _packs: std::fs::File,
+    recovery: std::fs::File,
+}
+
+impl PackRemovalRoot {
+    /// Open each named child without following a symlink; preserve every ancestor handle.
+    fn open(store: &Path) -> std::io::Result<Self> {
+        use cap_primitives::fs::{open_ambient_dir, open_dir_nofollow};
+        let root = open_ambient_dir(store, cap_primitives::ambient_authority())?;
+        let packs = open_dir_nofollow(&root, Path::new("language-packs"))?;
+        let recovery = open_dir_nofollow(&packs, Path::new(".removing"))?;
+        Ok(Self { _store: root, _packs: packs, recovery })
+    }
+
+    /// Resolve only a validated receipt relative to the retained recovery directory.
+    fn remove(&self, id: &str, _display_parent: &Path) -> std::io::Result<()> {
+        #[cfg(test)]
+        tests::before_cleanup(&_display_parent.join(id))?;
+        cap_primitives::fs::remove_dir_all(&self.recovery, Path::new(id))
+    }
 }
 
 fn finish_pack_removal(stage: Stage, backup: &Path, dest: &Path, moved: bool, saved: Result<(), Error>) -> Result<(), Error> {

crates/font-store/src/language_packs/tests.rs

@@ -21,6 +21,8 @@ pub(super) enum IoFailure {
 thread_local! {
     static IO_FAILURE: Cell<Option<IoFailure>> = const { Cell::new(None) };
     static CLEANUP_ATTEMPTS: RefCell<Vec<PathBuf>> = const { RefCell::new(Vec::new()) };
+    #[cfg(unix)]
+    static REPLACE_CLEANUP_PARENT: RefCell<Option<(PathBuf, PathBuf, PathBuf)>> = const { RefCell::new(None) };
 }
 
 struct InjectFailure(Option<IoFailure>);
@@ -45,8 +47,14 @@ pub(super) fn fail_io(failure: IoFailure) -> std::io::Result<()> {
     }
 }
 
+/// Record attempts and inject deterministic failures or a single ancestor replacement.
 pub(super) fn before_cleanup(path: &Path) -> std::io::Result<()> {
     CLEANUP_ATTEMPTS.with_borrow_mut(|attempts| attempts.push(path.to_path_buf()));
+    #[cfg(unix)]
+    if let Some((parent, moved, outside)) = REPLACE_CLEANUP_PARENT.take() {
+        std::fs::rename(&parent, moved)?;
+        std::os::unix::fs::symlink(outside, parent)?;
+    }
     fail_io(IoFailure::Cleanup)
 }
 
@@ -468,6 +476,114 @@ fn recovery_cleanup_does_not_follow_a_replaced_parent_symlink() {
     std::fs::remove_dir_all(store.dir()).unwrap();
 }
 
+/// The language-packs ancestor must not redirect a committed cleanup outside the store.
+#[cfg(unix)]
+#[test]
+fn residual_cleanup_rejects_a_symlinked_language_packs_ancestor() {
+    let (store, _server, _pin, _) = setup("remove-symlink-packs");
+    let outside = temp("remove-symlink-packs-outside");
+    std::fs::create_dir_all(outside.join(".removing/11-12")).unwrap();
+    std::fs::write(outside.join(".removing/11-12/keep"), b"untouched").unwrap();
+    std::fs::create_dir_all(store.dir()).unwrap();
+    std::os::unix::fs::symlink(&outside, store.language_packs_dir()).unwrap();
+    let mut record = Installed::default();
+    record.committed_removals.push("11-12".into());
+    store.write_installed(&record).unwrap();
+    drop(store.lock().unwrap());
+    assert_eq!(std::fs::read(outside.join(".removing/11-12/keep")).unwrap(), b"untouched");
+    assert!(store.pack_removal_dir().is_err());
+    std::fs::remove_file(store.language_packs_dir()).unwrap();
+    std::fs::remove_dir_all(outside).unwrap();
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+/// A replacement after validation must not redirect recursive deletion to the new target.
+#[cfg(unix)]
+#[test]
+fn residual_cleanup_stays_anchored_after_recovery_parent_replacement() {
+    let (store, _server, _pin, _) = setup("remove-parent-race");
+    let parent = store.language_packs_dir().join(".removing");
+    let moved = store.language_packs_dir().join(".removing-old");
+    let outside = temp("remove-parent-race-outside");
+    for base in [&parent, &outside] {
+        std::fs::create_dir_all(base.join("11-12")).unwrap();
+        std::fs::write(base.join("11-12/keep"), b"untouched").unwrap();
+    }
+    let mut record = Installed::default();
+    record.committed_removals.push("11-12".into());
+    store.write_installed(&record).unwrap();
+    REPLACE_CLEANUP_PARENT.set(Some((parent.clone(), moved.clone(), outside.clone())));
+    drop(store.lock().unwrap());
+    assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched");
+    assert!(!moved.join("11-12").exists(), "the originally opened recovery folder is cleaned");
+    std::fs::remove_file(parent).unwrap();
+    std::fs::remove_dir_all(outside).unwrap();
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+/// Retained Windows handles prevent replacement of every recovery ancestor during cleanup.
+#[cfg(windows)]
+#[test]
+fn recovery_cleanup_locks_all_windows_ancestors_until_removal_finishes() {
+    let (store, _server, _pin, _) = setup("remove-windows-parent-locks");
+    let parent = store.language_packs_dir().join(".removing");
+    let outside = temp("remove-windows-parent-outside");
+    std::fs::create_dir_all(parent.join("11-12")).unwrap();
+    std::fs::create_dir_all(outside.join("11-12")).unwrap();
+    std::fs::write(outside.join("11-12/keep"), b"untouched").unwrap();
+    let root = PackRemovalRoot::open(store.dir()).unwrap();
+    let packs = store.language_packs_dir();
+    for ancestor in [parent.as_path(), packs.as_path(), store.dir()] {
+        assert!(
+            std::fs::rename(ancestor, ancestor.with_extension("replaced")).is_err(),
+            "an open ancestor cannot be replaced: {}",
+            ancestor.display()
+        );
+    }
+    root.remove("11-12", &parent).unwrap();
+    assert!(!parent.join("11-12").exists());
+    assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched");
+    drop(root);
+    std::fs::remove_dir_all(outside).unwrap();
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+/// A malformed optional receipt must not discard valid receipts or installed-pack metadata.
+#[test]
+fn residual_one_bad_receipt_preserves_valid_metadata_and_cleanup() {
+    let (store, _server, pin, _) = setup("remove-mixed-receipts");
+    let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+    store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+    let original = store.read_record().unwrap();
+    let parent = store.language_packs_dir().join(".removing");
+    std::fs::create_dir_all(parent.join("11-12")).unwrap();
+    std::fs::write(parent.join("11-12/keep"), b"committed").unwrap();
+    let mut raw = serde_json::to_value(&original).unwrap();
+    raw["committed_removals"] = serde_json::json!(["11-12", "../outside"]);
+    std::fs::write(store.dir().join(INSTALLED), serde_json::to_vec(&raw).unwrap()).unwrap();
+    let record = store.read_record().expect("one invalid receipt must not invalidate the whole record");
+    assert_eq!(record.committed_removals, ["11-12"]);
+    assert_eq!(serde_json::to_value(&record.packs).unwrap(), serde_json::to_value(&original.packs).unwrap());
+    drop(store.lock().unwrap());
+    assert!(!parent.join("11-12").exists());
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+/// Exclusions do not consume filesystem candidates; hostile iterator input remains bounded.
+#[test]
+fn residual_stage_exclusions_leave_the_filesystem_candidate_budget() {
+    let parent = temp("stage-excluded-budget");
+    let excluded: Vec<String> = (0..1000).map(|n| format!("0-{n}")).collect();
+    let stage = Stage::create_in(&parent, &excluded, excluded.iter().cloned().chain(std::iter::once("0-1000".into())))
+        .expect("a free name after excluded receipts is still considered");
+    drop(stage);
+    assert!(Stage::create_in(&parent, &excluded, std::iter::repeat_with(|| "0-0".into())).is_err());
+    assert!(Stage::create_in(&parent, &[], std::iter::repeat_with(|| "../invalid".into())).is_err());
+    std::fs::create_dir(parent.join("0-1001")).unwrap();
+    assert!(Stage::create_in(&parent, &[], std::iter::repeat_with(|| "0-1001".into())).is_err());
+    std::fs::remove_dir_all(parent).unwrap();
+}
+
 #[test]
 fn network_off_pins_and_checksums_are_enforced() {
     let (store, server, pin, archive) = setup("pins");

crates/font-store/src/store.rs

@@ -865,17 +865,24 @@ impl Stage {
         name.split_once('-').is_some_and(|(pid, counter)| pid.parse::<u32>().is_ok() && counter.parse::<u64>().is_ok())
     }
 
-    /// The candidate iterator also lets tests reproduce a restarted PID/counter sequence.
+    /// Allow distinct receipt exclusions in addition to 1,000 filesystem candidates.
+    /// Bound total iterator reads too, including repeated excluded or malformed names.
     pub(crate) fn create_in(staging: &Path, excluded: &[String], names: impl Iterator<Item = String>) -> Result<Self, Error> {
         std::fs::create_dir_all(staging).map_err(io("creating a temporary folder"))?;
         // A folder of that name may be another process's (PID namespaces in Flatpak or
         // containers repeat ids) or a stale one: never touched here, the next name is taken
         // (`sweep_staging` handles ordinary download staging, not recovery folders).
         let mut last = None;
-        for name in names.take(1000) {
-            if !Self::valid_name(&name) || excluded.contains(&name) {
+        let mut candidates = 0;
+        let excluded: std::collections::HashSet<&str> = excluded.iter().map(String::as_str).collect();
+        for name in names.take(1000usize.saturating_add(excluded.len())) {
+            if !Self::valid_name(&name) || excluded.contains(name.as_str()) {
                 continue;
             }
+            if candidates == 1000 {
+                break;
+            }
+            candidates += 1;
             let dir = staging.join(name);
             match std::fs::create_dir(&dir) {
                 Ok(()) => return Ok(Self { dir, remove_on_drop: true }),