CP4.1: review — record rebuilt atomically in place (and when missing), staging never deletes another process's folder, zstd frames capped, neutral index errors, plain index text, optional source

- read_installed copies an unreadable record to .bad and writes the rebuilt one atomically over it; a missing record with pack folders is rebuilt too; install reads the record before moving the pack in.
- Stage::new uses create_dir and takes the next name on AlreadyExists (PID namespaces in Flatpak/containers); stale folders are left to sweep_staging.
- At most 4096 zstd frames per archive, empty ones included.
- IndexError and the sha256 mismatch say "the index", not "the font index".
- licence, release, version and class lose control and Cf characters like name.
- PackEntry.source: Option<String>, an https URL or the index is refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FypgCX8LsYYE8fsj4byBm2

crates/font-store/README.md

@@ -138,7 +138,7 @@ let installed = store.installed_language_packs();
 | `installed_language_packs()` | The recorded packs whose folder is there. |
 | `language_packs_dir()` | `<store>/language-packs`, for `craft_proof::packs::set_packs_dir`. |
 
-**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; pack names lose control and bidi/format characters before they are stored or listed. An unreadable `language-packs.json` is kept as `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`), so no installed pack drops out of the list.
+**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.
 
 **Developer override (testing only — bypasses the pin):** `CRAFT_LANGUAGE_PACK_INDEX=<url or path>` replaces the pinned index URL with an `http://` URL on a loopback host (`127.0.0.1`, `localhost`, `[::1]`), an `https://` URL or a local file path (`file://` URLs are not taken), and **skips the index's sha256 pin**. Archives are still checked against the sha256 and size in that index, and may then also be loopback `http://` URLs or local paths. Off unless the variable is set; it works in release builds too (for end-to-end tests of a real build), a warning is logged whenever it is used, and `language_pack_index_overridden() -> Option<String>` returns the value so apps can show that the pin is bypassed. Never set it for users.
 

crates/font-store/src/index.rs

@@ -24,25 +24,25 @@ pub const LICENSES: [&str; 3] = ["OFL", "APACHE2", "UFL"];
 #[derive(Debug, thiserror::Error)]
 pub enum IndexError {
     /// More than [`MAX_INDEX_BYTES`].
-    #[error("the font index is larger than {max} bytes")]
+    #[error("the index is larger than {max} bytes")]
     TooLarge {
         /// The limit.
         max: usize,
     },
     /// Not JSON, or a field has the wrong type.
-    #[error("the font index can't be read: {0}")]
+    #[error("the index can't be read: {0}")]
     Json(String),
     /// A major version this crate doesn't know.
-    #[error("the font index has version {0}; this build reads version {MAJOR_VERSION}.x")]
+    #[error("the index has version {0}; this build reads version {MAJOR_VERSION}.x")]
     Version(String),
     /// An entry breaks the format's rules.
-    #[error("the font index has an invalid entry: {0}")]
+    #[error("the index has an invalid entry: {0}")]
     Invalid(String),
     /// A gzipped index that doesn't decompress.
-    #[error("the gzipped font index can't be read: {0}")]
+    #[error("the gzipped index can't be read: {0}")]
     Gzip(String),
     /// The bytes aren't the pinned ones.
-    #[error("the font index doesn't have the sha256 this build expects")]
+    #[error("the index doesn't have the sha256 this build expects")]
     Checksum,
 }
 

crates/font-store/src/language_packs.rs

@@ -97,6 +97,9 @@ pub struct PackEntry {
     /// Whether it has hyphenation patterns.
     #[serde(default)]
     pub hyphenation: bool,
+    /// Where the data comes from (an `https://` page), for display; optional.
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub source: Option<String>,
 }
 
 /// An installed pack, as recorded when it was installed.
@@ -151,9 +154,18 @@ impl PackIndex {
             if !(p.url.starts_with("https://") || local && local_source(&p.url).is_some()) {
                 return Err(bad(format!("pack {}: {} is not an https URL", p.locale, p.url.chars().take(200).collect::<String>())));
             }
+            if let Some(src) = &p.source
+                && !(src.starts_with("https://") && src.len() <= 2048 && !src.chars().any(|c| c.is_control() || is_format(c) || c == ' '))
+            {
+                return Err(bad(format!("pack {}: source {:?} is not an https URL", p.locale, src.chars().take(200).collect::<String>())));
+            }
         }
+        // Text shown to people loses control and bidi/format characters.
+        index.release = plain_text(&index.release);
         for p in &mut index.packs {
-            p.name = plain_text(&p.name);
+            for t in [&mut p.name, &mut p.version, &mut p.licence, &mut p.class] {
+                *t = plain_text(t);
+            }
         }
         Ok(index)
     }
@@ -366,13 +378,15 @@ impl FontStore {
         let record = InstalledLanguagePack {
             locale: pack.locale.clone(),
             name: plain_text(&pack.name),
-            release: index.release.chars().take(200).collect(),
-            version: pack.version.chars().take(200).collect(),
-            licence: pack.licence.chars().take(200).collect(),
+            release: plain_text(&index.release),
+            version: plain_text(&pack.version),
+            licence: plain_text(&pack.licence),
             sha256: pack.sha256.clone(),
             size: pack.size,
         };
         let _lock = self.lock()?;
+        // The record first: rebuilding a damaged one must not count the pack being installed.
+        let mut installed = self.read_installed()?;
         let packs = self.language_packs_dir();
         std::fs::create_dir_all(&packs).map_err(io("creating the Language packs folder"))?;
         let dest = packs.join(&pack.locale);
@@ -387,7 +401,6 @@ impl FontStore {
             }
             return Err(io(format!("installing the {} pack", pack.locale))(e));
         }
-        let mut installed = self.read_installed()?;
         installed.packs.retain(|p| p.locale != record.locale);
         installed.packs.push(record.clone());
         installed.packs.sort_by(|a, b| a.locale.cmp(&b.locale));
@@ -410,7 +423,13 @@ impl FontStore {
             .packs
             .into_iter()
             .filter(|p| is_locale(&p.locale) && dir.join(&p.locale).is_dir())
-            .map(|p| InstalledLanguagePack { name: plain_text(&p.name), ..p })
+            .map(|p| InstalledLanguagePack {
+                name: plain_text(&p.name),
+                release: plain_text(&p.release),
+                version: plain_text(&p.version),
+                licence: plain_text(&p.licence),
+                ..p
+            })
             .collect()
     }
 
@@ -436,20 +455,24 @@ impl FontStore {
         // `stage` deletes the old folder when dropped.
     }
 
-    /// The record of installed packs (none yet: empty). One that can't be read is kept aside as
-    /// `language-packs.json.bad` and rebuilt from the pack folders on disk, so no installed pack
-    /// is ever dropped from it.
+    /// The record of installed packs. One that can't be read is copied aside as
+    /// `language-packs.json.bad` and rebuilt from the pack folders on disk (written atomically
+    /// over it), so no installed pack is ever dropped from it; a missing one is rebuilt the same
+    /// way when pack folders are there (none: empty).
     fn read_installed(&self) -> Result<Installed, Error> {
         let path = self.dir().join(INSTALLED);
-        if std::fs::symlink_metadata(&path).is_err() {
-            return Ok(Installed::default());
-        }
-        if let Some(i) = read_limited(&path, MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::<Installed>(&b).ok()) {
+        let there = std::fs::symlink_metadata(&path).is_ok();
+        if there && let Some(i) = read_limited(&path, MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::<Installed>(&b).ok()) {
             return Ok(i);
         }
-        log::warn!("{}: can't be read; rebuilt from the Language pack folders", path.display());
         let rebuilt = self.installed_from_disk();
-        let _ = std::fs::rename(&path, self.dir().join(format!("{INSTALLED}.bad")));
+        if !there && rebuilt.packs.is_empty() {
+            return Ok(Installed::default());
+        }
+        log::warn!("{}: {}; rebuilt from the Language pack folders", path.display(), if there { "can't be read" } else { "missing" });
+        if there {
+            let _ = std::fs::copy(&path, self.dir().join(format!("{INSTALLED}.bad")));
+        }
         self.write_installed(&rebuilt)?;
         Ok(rebuilt)
     }
@@ -495,11 +518,16 @@ impl FontStore {
 /// a concatenation of frames is a valid `.zst` too, and must not stop after the first).
 struct Frames<'a> {
     decoder: Option<ruzstd::decoding::StreamingDecoder<&'a [u8], ruzstd::decoding::FrameDecoder>>,
+    /// Frames started so far, empty ones included (at most [`MAX_PACK_FRAMES`]).
+    frames: usize,
 }
 
+/// Most zstd frames in a pack archive (`zstd` writes one; empty frames count too).
+const MAX_PACK_FRAMES: usize = 4096;
+
 impl<'a> Frames<'a> {
     fn new(archive: &'a [u8]) -> Result<Self, String> {
-        Ok(Self { decoder: Some(ruzstd::decoding::StreamingDecoder::new(archive).map_err(|e| format!("not zstd: {e}"))?) })
+        Ok(Self { decoder: Some(ruzstd::decoding::StreamingDecoder::new(archive).map_err(|e| format!("not zstd: {e}"))?), frames: 1 })
     }
 }
 
@@ -512,6 +540,12 @@ impl Read for Frames<'_> {
                 return Ok(n);
             }
             let rest: &[u8] = d.get_ref();
+            if !rest.is_empty() {
+                self.frames += 1;
+                if self.frames > MAX_PACK_FRAMES {
+                    return Err(std::io::Error::other(format!("more than {MAX_PACK_FRAMES} zstd frames")));
+                }
+            }
             self.decoder = if rest.is_empty() { None } else { Some(ruzstd::decoding::StreamingDecoder::new(rest).map_err(std::io::Error::other)?) };
         }
     }

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

@@ -348,6 +348,13 @@ fn hostile_archives_are_refused() {
     let dest = temp("frames").join("nl");
     unpack(&[zst(a), zst(b)].concat(), &dest, &nl).unwrap();
     assert!(dest.join("LICENCES/COPYING").is_file());
+    // Empty frames are read past, but only so many.
+    let one = zst(&tar_of(&[("pack.toml", FILE, toml)]));
+    let empty = zst(b"");
+    unpack(&[one.clone(), empty.clone(), empty.clone()].concat(), &temp("empties").join("nl"), &nl).unwrap();
+    let many: Vec<u8> = std::iter::once(one).chain(std::iter::repeat_n(empty, MAX_PACK_FRAMES)).flatten().collect();
+    let err = unpack(&many, &temp("frames-cap").join("nl"), &nl).unwrap_err();
+    assert!(err.contains("frames"), "{err}");
     // The index's promises are checked against the files.
     let bare = zst(&tar_of(&[("pack.toml", FILE, toml)]));
     for e in [PackEntry { spelling: true, ..nl.clone() }, PackEntry { hyphenation: true, ..nl.clone() }] {
@@ -389,13 +396,42 @@ fn a_corrupt_record_is_rebuilt_from_the_folders() {
     let mut locales: Vec<String> = store.installed_language_packs().into_iter().map(|p| p.locale).collect();
     locales.sort();
     assert_eq!(locales, ["de", "nl"]);
-    assert!(store.dir().join("language-packs.json.bad").is_file(), "the unreadable record is kept aside");
+    assert_eq!(std::fs::read(store.dir().join("language-packs.json.bad")).unwrap(), b"{ not json", "the unreadable record is kept aside");
+    assert!(serde_json::from_slice::<serde_json::Value>(&std::fs::read(&record).unwrap()).is_ok(), "rebuilt in place");
+    // A missing record with pack folders is rebuilt too.
+    std::fs::remove_file(&record).unwrap();
+    assert_eq!(store.installed_language_packs().len(), 2);
+    let nl_again = PackEntry { url: format!("{RELEASE}nl.tar.zst"), ..nl.clone() };
+    std::fs::remove_file(&record).unwrap();
+    store.install_pack_from(false, &index, &nl_again, &Cancel::new(), &mut |_| {}).unwrap();
+    assert_eq!(store.installed_language_packs().len(), 2, "de kept when the record was missing");
     // Removing works on a rebuilt record too.
     std::fs::write(&record, b"\xff").unwrap();
     store.remove_language_pack("nl").unwrap();
     assert_eq!(store.installed_language_packs().into_iter().map(|p| p.locale).collect::<Vec<_>>(), ["de"]);
 }
 
+#[test]
+fn index_text_is_plain_and_sources_are_checked() {
+    let a = pack_archive("nl", "x");
+    let mut e = entry("nl", &format!("{RELEASE}nl.tar.zst"), &a);
+    e["licence"] = serde_json::json!("MIT\u{202e}");
+    e["version"] = serde_json::json!("ab\u{0}c");
+    e["class"] = serde_json::json!("\u{200f}Embeddable");
+    e["source"] = serde_json::json!("https://example.org/nl");
+    let json = serde_json::to_vec(&serde_json::json!({"format": 1, "release": "r1\u{2066}", "packs": [e.clone()]})).unwrap();
+    let index = PackIndex::from_json(&json).unwrap();
+    let p = &index.packs[0];
+    assert_eq!((index.release.as_str(), p.licence.as_str(), p.version.as_str(), p.class.as_str()), ("r1", "MIT", "abc", "Embeddable"));
+    assert_eq!(p.source.as_deref(), Some("https://example.org/nl"));
+    assert_eq!(PackIndex::from_json(&index_json(&[entry("nl", &format!("{RELEASE}nl.tar.zst"), &a)])).unwrap().packs[0].source, None);
+    for bad in ["javascript:alert(1)", "http://example.org", "https://a b", "https://a\u{202e}b"] {
+        e["source"] = serde_json::json!(bad);
+        let err = PackIndex::from_json(&index_json(std::slice::from_ref(&e))).unwrap_err();
+        assert!(!err.to_string().contains("font"), "{err}");
+    }
+}
+
 #[test]
 fn bad_hand_made_entries_are_index_errors() {
     let (store, _server, pin, _) = setup("variant");
@@ -413,8 +449,8 @@ fn the_override_is_visible() {
     assert_eq!(language_pack_index_overridden(), set);
 }
 
-/// A staging folder an earlier process with the same id left behind is cleared, so an unpack
-/// (`create_new`) can't fail on, or mix in, its files.
+/// A staging folder of the same name (an earlier process, or another one in a PID namespace) is
+/// never used or deleted: the next name is taken, so an unpack can't mix in its files.
 #[test]
 fn a_stale_stage_folder_is_cleared() {
     let store = temp("stale-stage");
@@ -426,4 +462,7 @@ fn a_stale_stage_folder_is_cleared() {
     }
     let stage = Stage::new(&store).unwrap();
     assert_eq!(std::fs::read_dir(&stage.dir).unwrap().count(), 0, "{}", stage.dir.display());
+    // The others are left alone: they may be another process's (PID namespaces).
+    let left = std::fs::read_dir(&staging).unwrap().flatten().filter(|e| e.path().join("nl/pack.toml").is_file()).count();
+    assert_eq!(left, 512);
 }

crates/font-store/src/store.rs

@@ -839,13 +839,21 @@ pub(crate) struct Stage {
 impl Stage {
     pub(crate) fn new(store: &Path) -> Result<Self, Error> {
         static N: AtomicU64 = AtomicU64::new(0);
-        let dir = store.join(STAGING).join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed)));
-        // A folder left by an earlier process with the same id is cleared, not mixed in.
-        if std::fs::symlink_metadata(&dir).is_ok() {
-            std::fs::remove_dir_all(&dir).map_err(io("clearing an old download folder"))?;
+        let staging = store.join(STAGING);
+        std::fs::create_dir_all(&staging).map_err(io("creating a download 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
+        // (stale ones go in `sweep_staging`).
+        let mut last = None;
+        for _ in 0..1000 {
+            let dir = staging.join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed)));
+            match std::fs::create_dir(&dir) {
+                Ok(()) => return Ok(Self { dir }),
+                Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e),
+                Err(e) => return Err(io("creating a download folder")(e)),
+            }
         }
-        std::fs::create_dir_all(&dir).map_err(io("creating a download folder"))?;
-        Ok(Self { dir })
+        Err(io("creating a download folder")(last.unwrap_or_else(|| std::io::Error::other("no free name"))))
     }
 
     /// Writes a verified download as `<name>.part`, which no font scan picks up.

crates/font-store/src/verify.rs

@@ -20,7 +20,7 @@ pub enum VerifyError {
         actual: u64,
     },
     /// The sha256 differs.
-    #[error("the file's sha256 doesn't match the font index")]
+    #[error("the file's sha256 doesn't match the index")]
     Sha256,
     /// It doesn't parse as a font with outlines.
     #[error("the file is not a font, or a damaged one")]