fonts-build: publish licences as .txt, refuse line breaks in watched paths, bound manifest and font reads (#837)

Ports vectorcraft 16d7fed6 (licence served as text), lightcraft 0e3b3ed5 (line break in rerun-if-changed) and 9cb2c721 / cadcraft #2732 27b4d723 (bounded regular-file reads).

crates/fonts-build/src/embed.rs

@@ -14,6 +14,46 @@ use crate::manifest::{ManifestEntry, ParseError, parse_manifest, script_matches}
 const DIR_VAR: &str = "CRAFT_FONTS_DIR";
 const REQUIRED_VAR: &str = "CRAFT_FONTS_REQUIRED";
 const EMPTY_TABLE: &str = "&[]\n";
+/// The most a manifest may hold; the real one is a few KiB.
+const MAX_MANIFEST_BYTES: u64 = 1 << 20;
+/// The most a font or licence file may hold (bytes): the largest CJK collections are a few tens of MiB.
+const MAX_FILE_BYTES: u64 = 64 << 20;
+
+/// Read the regular file `path`, at most `limit` bytes: a device, a FIFO or a file that grows while
+/// it is read can't fill memory or block the build.
+pub(crate) fn read_bounded(path: &Path, limit: u64) -> io::Result<Vec<u8>> {
+    use std::io::Read as _;
+    // Opening a FIFO with no writer blocks, so refuse anything that isn't a file before opening.
+    if !std::fs::metadata(path)?.is_file() {
+        return Err(io::Error::other("not a regular file"));
+    }
+    let file = std::fs::File::open(path)?;
+    if !file.metadata()?.is_file() {
+        return Err(io::Error::other("not a regular file"));
+    }
+    let mut bytes = Vec::new();
+    file.take(limit.saturating_add(1)).read_to_end(&mut bytes)?;
+    if bytes.len() as u64 > limit {
+        return Err(io::Error::other(format!("larger than {limit} bytes")));
+    }
+    Ok(bytes)
+}
+
+/// [`read_bounded`] as UTF-8 text.
+fn read_text_bounded(path: &Path, limit: u64) -> io::Result<String> {
+    String::from_utf8(read_bounded(path, limit)?).map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))
+}
+
+/// Print `cargo::rerun-if-changed` for `path`. Cargo reads one instruction a line, so a path with a
+/// line break (a supplier's file name, or a checkout directory) would print a second one.
+fn rerun_if_changed(cargo: &mut Vec<String>, path: &Path) -> Result<(), Error> {
+    let shown = path.display().to_string();
+    if shown.contains(['\n', '\r']) {
+        return Err(Error::LineBreakInPath { path: path.to_path_buf() });
+    }
+    cargo.push(format!("cargo::rerun-if-changed={shown}"));
+    Ok(())
+}
 
 /// Whether the build script is building for wasm32 (web builds usually embed fewer faces).
 pub fn target_is_wasm() -> bool {
@@ -149,8 +189,10 @@ impl Embed {
     /// Copy the selected faces into `dir` instead of embedding them, for a web build that serves
     /// them beside its wasm, and write a table of [`ServedFace`](crate::ServedFace)s.
     ///
-    /// Each font file and its licence file keep their path under the checkout's `fonts/`
-    /// directory (`noto-sans-arabic/NotoSansArabic.ttf`), and a face's `url` is `url_prefix`
+    /// Each font file keeps its path under the checkout's `fonts/` directory
+    /// (`noto-sans-arabic/NotoSansArabic.ttf`), and its licence file keeps its path and stem but is
+    /// always published as `.txt` (`noto-sans-arabic/OFL.txt`; `LICENSE.html` becomes `LICENSE.txt`), so a
+    /// supplier's name can't make the site serve a page. A face's `url` is `url_prefix`
     /// followed by that path, so `serve("web/fonts", "fonts/")` suits a page that serves
     /// `web/fonts` as `fonts/`. An interface face cut by `Embed::subset_to` is served from its cut,
     /// with the start of the cut's SHA-256 in the file name (`ui/NotoSansTCUI-Regular.<hash>.ttf`)
@@ -181,14 +223,17 @@ impl Embed {
     pub(crate) fn run(&self, env: &Env, out_file: &Path, cargo: &mut Vec<String>) -> Result<Vec<ManifestEntry>, Error> {
         cargo.push(format!("cargo::rerun-if-env-changed={DIR_VAR}"));
         cargo.push(format!("cargo::rerun-if-env-changed={REQUIRED_VAR}"));
-        for text in self.checked_files() {
-            cargo.push(format!("cargo::rerun-if-changed={}", text.display()));
-        }
         let out = match (out_file.is_absolute(), &env.out_dir) {
             (true, _) => out_file.to_path_buf(),
             (false, Some(out_dir)) => Path::new(out_dir).join(out_file),
             (false, None) => return Err(Error::NoOutDir),
         };
+        for text in self.checked_files() {
+            if let Err(e) = rerun_if_changed(cargo, text) {
+                write(&out, EMPTY_TABLE)?;
+                return Err(e);
+            }
+        }
         let required = self.required.unwrap_or_else(|| env.required.as_ref().is_some_and(|v| v == "1"));
         let dir = self.dir.clone().or_else(|| env.dir.clone().filter(|d| !d.is_empty()).map(PathBuf::from));
 
@@ -236,9 +281,9 @@ impl Embed {
     /// The wanted manifest entries with their canonical file paths.
     fn select(&self, dir: &Path, cargo: &mut Vec<String>) -> Result<Vec<Face>, Error> {
         let manifest = dir.join("fonts/manifest.txt");
-        let text = std::fs::read_to_string(&manifest).map_err(|source| Error::Read { path: manifest.clone(), source })?;
+        let text = read_text_bounded(&manifest, MAX_MANIFEST_BYTES).map_err(|source| Error::Read { path: manifest.clone(), source })?;
         // Cargo treats a missing watched file as changed, which would rerun the build every time.
-        cargo.push(format!("cargo::rerun-if-changed={}", manifest.display()));
+        rerun_if_changed(cargo, &manifest)?;
         let entries = parse_manifest(&text).map_err(|error| Error::Parse { manifest: manifest.clone(), error })?;
         // The manifest read, so the checkout exists. Faces and licences must resolve under it.
         let root = dir.canonicalize().map_err(|source| Error::Read { path: dir.to_path_buf(), source })?;
@@ -297,7 +342,10 @@ impl Embed {
             if !entry.licence_file.is_empty() && dir.join(&entry.licence_file).canonicalize().is_ok_and(|licence| !licence.starts_with(&root)) {
                 return Err(Error::OutsideCheckout { manifest, file: entry.licence_file });
             }
-            cargo.push(format!("cargo::rerun-if-changed={}", path.display()));
+            if !path.is_file() {
+                return Err(Error::OutsideCheckout { manifest, file: entry.file });
+            }
+            rerun_if_changed(cargo, &path)?;
             let (index, sha256) = (entry.index, entry.sha256.clone());
             faces.push(Face { entry, path, index, cut, sha256 });
         }
@@ -338,7 +386,7 @@ impl Embed {
         let dir = out_dir.join("craft-fonts-ui");
         std::fs::create_dir_all(&dir).map_err(|source| Error::Write { path: dir.clone(), source })?;
         for face in faces.iter_mut().filter(|face| face.cut) {
-            let source = std::fs::read(&face.path).map_err(|source| Error::Read { path: face.path.clone(), source })?;
+            let source = read_bounded(&face.path, MAX_FILE_BYTES).map_err(|source| Error::Read { path: face.path.clone(), source })?;
             let bytes = crate::subset::subset(&source, face.index, &chars).map_err(|failure| match failure {
                 crate::subset::Failure::NotTrueType => Error::NotTrueType { path: face.path.clone() },
                 crate::subset::Failure::Other(reason) => Error::Subset { path: face.path.clone(), reason },
@@ -382,7 +430,7 @@ impl Embed {
         }
         let manifest = dir.join("fonts/manifest.txt");
         // `select` read and parsed this manifest already.
-        let entries = std::fs::read_to_string(&manifest).ok().and_then(|text| parse_manifest(&text).ok()).unwrap_or_default();
+        let entries = read_text_bounded(&manifest, MAX_MANIFEST_BYTES).ok().and_then(|text| parse_manifest(&text).ok()).unwrap_or_default();
         for family in self.elsewhere.iter().filter(|f| !entries.iter().any(|e| &e.family == *f)) {
             cargo.push(format!("cargo::warning=craft-fonts: embedded_elsewhere names {family}, which {} lacks", manifest.display()));
         }
@@ -412,8 +460,8 @@ impl Embed {
         let elsewhere = self.elsewhere(dir, cargo);
         let fonts: Vec<(&ManifestEntry, u32, Option<Vec<u8>>)> = faces
             .iter()
-            .map(|face| (&face.entry, face.index, std::fs::read(&face.path).ok()))
-            .chain(elsewhere.iter().map(|entry| (entry, entry.index, std::fs::read(dir.join(&entry.file)).ok())))
+            .map(|face| (&face.entry, face.index, read_bounded(&face.path, MAX_FILE_BYTES).ok()))
+            .chain(elsewhere.iter().map(|entry| (entry, entry.index, read_bounded(&dir.join(&entry.file), MAX_FILE_BYTES).ok())))
             .collect();
         let mut cmaps = Vec::new();
         for (entry, index, bytes) in &fonts {
@@ -512,12 +560,13 @@ impl Serve {
         for face in faces {
             let (entry, path) = (&face.entry, &face.path);
             let served = served_file(face);
-            let font = std::fs::read(path).map_err(|source| Error::Read { path: path.clone(), source })?;
+            let licence_name = served_licence(&entry.licence_file);
+            let font = read_bounded(path, MAX_FILE_BYTES).map_err(|source| Error::Read { path: path.clone(), source })?;
             let size = font.len();
             if !copied.contains(&served) {
                 copied.push(served.clone());
                 // `select` checked the manifest paths; the served names must stay under `dir` too.
-                for name in [served.as_str(), served_path(&entry.licence_file)] {
+                for name in [served.as_str(), licence_name.as_str()] {
                     if !name.is_empty() && !is_checkout_relative(name) {
                         return Err(Error::OutsideCheckout { manifest, file: name.to_owned() });
                     }
@@ -537,7 +586,7 @@ impl Serve {
                     Some(licence) if !licence.starts_with(&root) => {
                         return Err(Error::OutsideCheckout { manifest, file: entry.licence_file.clone() });
                     }
-                    Some(licence) => copy_file(&licence, &self.dir.join(served_path(&entry.licence_file)), cargo)?,
+                    Some(licence) => copy_file(&licence, &self.dir.join(&licence_name), cargo)?,
                 }
             }
             let coverage = crate::coverage::of_face(&font, face.index).unwrap_or_else(|| {
@@ -597,6 +646,15 @@ fn served_file(face: &Face) -> String {
     }
 }
 
+/// Where a face's licence is served: beside its font, always as `.txt`, whatever the supplier named
+/// it. A `LICENSE.html` published as it is would be a page on the site's own origin.
+fn served_licence(file: &str) -> String {
+    if file.is_empty() {
+        return String::new();
+    }
+    Path::new(served_path(file)).with_extension("txt").to_string_lossy().into_owned()
+}
+
 /// Whether a manifest path stays under the directory it is joined to: relative, without `..`, a
 /// root or a drive prefix.
 fn is_checkout_relative(file: &str) -> bool {
@@ -622,11 +680,11 @@ fn served_path(file: &str) -> &str {
 /// Copy `from` unless `to` already holds the same bytes. The copy keeps `from`'s modification time
 /// and is watched, so the build copies it again once it is deleted, without running every build.
 fn copy_file(from: &Path, to: &Path, cargo: &mut Vec<String>) -> Result<(), Error> {
-    cargo.push(format!("cargo::rerun-if-changed={}", to.display()));
+    rerun_if_changed(cargo, to)?;
     let write_error = |source| Error::Write { path: to.to_path_buf(), source };
     let read_error = |source| Error::Read { path: from.to_path_buf(), source };
-    let bytes = std::fs::read(from).map_err(read_error)?;
-    if std::fs::read(to).is_ok_and(|old| old == bytes) {
+    let bytes = read_bounded(from, MAX_FILE_BYTES).map_err(read_error)?;
+    if read_bounded(to, MAX_FILE_BYTES).is_ok_and(|old| old == bytes) {
         return Ok(());
     }
     if let Some(parent) = to.parent() {
@@ -689,6 +747,9 @@ pub enum Error {
     /// A manifest font or licence path is absolute, climbs out with `..`, or resolves (through a
     /// symlink) to a file outside the checkout.
     OutsideCheckout { manifest: PathBuf, file: String },
+    /// A path that goes into a `cargo::rerun-if-changed` line has a line break, which would start
+    /// another cargo instruction.
+    LineBreakInPath { path: PathBuf },
 }
 
 impl fmt::Display for Error {
@@ -724,6 +785,7 @@ impl fmt::Display for Error {
             Self::OutsideCheckout { manifest, file } => {
                 write!(f, "{}: {file:?} is not a file inside the checkout", manifest.display())
             }
+            Self::LineBreakInPath { path } => write!(f, "{path:?} has a line break: cargo could not be told to watch it"),
         }
     }
 }

crates/fonts-build/src/tests.rs

@@ -824,3 +824,98 @@ fn manifest_paths_stay_inside_the_checkout() {
     run(Embed::new().serve(&served, "fonts/"), Some(&dir), Some("1")).result.unwrap();
     assert_eq!(std::fs::read(served.join("OFL.txt")).unwrap(), b"licence");
 }
+
+/// A supplier's licence is published as plain text under its own stem, whatever extension it has:
+/// an `.html` licence would be a page on the site's origin.
+#[test]
+fn a_served_licence_is_always_plain_text() {
+    let scratch = Scratch::new("licence-name");
+    let dir = scratch.0.join("craft-fonts");
+    std::fs::create_dir_all(dir.join("fonts/a")).unwrap();
+    std::fs::write(dir.join("fonts/a/a.ttf"), b"a").unwrap();
+    let served = scratch.0.join("served");
+    for (licence, published) in [("LICENSE.html", "LICENSE.txt"), ("COPYING", "COPYING.txt"), ("OFL.txt", "OFL.txt"), ("NOTICE.svg", "NOTICE.txt")] {
+        std::fs::write(dir.join("fonts/a").join(licence), b"<script>alert(1)</script>").unwrap();
+        let line = format!("Sans | Regular | fonts/a/a.ttf | Latn | OFL-1.1 | fonts/a/{licence} | 00 | src\n");
+        std::fs::write(dir.join("fonts/manifest.txt"), line).unwrap();
+        run(Embed::new().serve(&served, "fonts/"), Some(&dir), Some("1")).result.unwrap();
+        assert_eq!(std::fs::read(served.join("a").join(published)).unwrap(), b"<script>alert(1)</script>", "{licence}");
+        if published != licence {
+            assert!(!served.join("a").join(licence).exists(), "{licence} is published under its own name");
+        }
+        std::fs::remove_dir_all(&served).unwrap();
+    }
+}
+
+/// `cargo::rerun-if-changed` is one line: a path with a line break would print a second instruction.
+#[cfg(unix)]
+#[test]
+fn a_path_with_a_line_break_is_refused_before_cargo_is_told_to_watch_it() {
+    let scratch = Scratch::new("line-break");
+    let served = scratch.0.join("served");
+    let dir = scratch.0.join("craft-fonts");
+    std::fs::create_dir_all(dir.join("fonts")).unwrap();
+    std::fs::write(dir.join("fonts/OFL.txt"), b"licence").unwrap();
+    // A font whose name has a carriage return (the manifest is read by lines, so a line feed can't be in it).
+    std::fs::write(dir.join("fonts/a\rcargo::warning=x.ttf"), b"a").unwrap();
+    let line = "Sans | Regular | fonts/a\rcargo::warning=x.ttf | Latn | OFL-1.1 | fonts/OFL.txt | 00 | src\n";
+    std::fs::write(dir.join("fonts/manifest.txt"), line).unwrap();
+    for embed in [Embed::new(), Embed::new().serve(&served, "fonts/")] {
+        let r = run(embed, Some(&dir), Some("1"));
+        assert!(matches!(r.result, Err(Error::LineBreakInPath { .. })), "{:?}", r.result);
+        assert!(r.cargo.iter().all(|line| !line.contains('\r') && !line.contains('\n')), "{:?}", r.cargo);
+        assert!(!r.table.contains("include_bytes!"));
+    }
+    // The same through a checkout directory, and a catalog, whose names are the caller's.
+    let odd = scratch.0.join("odd\rdir");
+    std::fs::create_dir_all(odd.join("fonts")).unwrap();
+    std::fs::write(odd.join("fonts/manifest.txt"), "# no faces\n").unwrap();
+    let r = run(Embed::new(), Some(&odd), None);
+    assert!(r.cargo.iter().all(|line| !line.starts_with("cargo::rerun-if-changed=") || !line.contains('\r')), "{:?}", r.cargo);
+    let r = run(Embed::new().check_text([scratch.0.join("t\rext.ftl")]), Some(&dir), None);
+    assert!(matches!(r.result, Err(Error::LineBreakInPath { .. })), "{:?}", r.result);
+}
+
+/// Reads of the manifest, fonts and licences are bounded and take regular files only. The tests use
+/// small limits: nothing here allocates the hostile size.
+#[test]
+fn reads_are_bounded_and_refuse_what_is_not_a_regular_file() {
+    use crate::embed::read_bounded;
+    let scratch = Scratch::new("bounded");
+    let file = scratch.0.join("f");
+    std::fs::write(&file, b"0123456789").unwrap();
+    assert_eq!(read_bounded(&file, 10).unwrap(), b"0123456789");
+    assert!(read_bounded(&file, 9).unwrap_err().to_string().contains("larger than 9 bytes"));
+    assert!(read_bounded(&scratch.0, 10).is_err(), "a directory");
+    assert!(read_bounded(&scratch.0.join("missing"), 10).is_err());
+    #[cfg(unix)]
+    {
+        // Opening a FIFO with no writer would block for ever.
+        let fifo = scratch.0.join("fifo");
+        if std::process::Command::new("mkfifo").arg(&fifo).status().is_ok_and(|s| s.success()) {
+            assert!(read_bounded(&fifo, 10).unwrap_err().to_string().contains("not a regular file"));
+        }
+    }
+}
+
+/// A manifest past 1 MiB is an error (the real one is a few KiB), and a face that is a directory is
+/// not a file inside the checkout.
+#[test]
+fn an_oversized_manifest_and_a_directory_face_are_errors() {
+    let scratch = Scratch::new("manifest-cap");
+    let dir = scratch.0.join("craft-fonts");
+    std::fs::create_dir_all(dir.join("fonts/a.ttf")).unwrap();
+    std::fs::write(dir.join("fonts/OFL.txt"), b"licence").unwrap();
+    let valid = "Sans | Regular | fonts/a.ttf | Latn | OFL-1.1 | fonts/OFL.txt | 00 | src\n";
+    std::fs::write(dir.join("fonts/manifest.txt"), valid).unwrap();
+    let r = run(Embed::new(), Some(&dir), Some("1"));
+    assert!(matches!(r.result, Err(Error::OutsideCheckout { .. })), "{:?}", r.result);
+
+    let padding = format!("# {}\n", "x".repeat(1 << 20));
+    std::fs::write(dir.join("fonts/manifest.txt"), format!("{padding}{valid}")).unwrap();
+    let r = run(Embed::new(), Some(&dir), Some("1"));
+    assert!(matches!(&r.result, Err(Error::Read { source, .. }) if source.to_string().contains("larger than")), "{:?}", r.result);
+    // Not required: a warning and an empty table, as for any unreadable manifest.
+    let r = run(Embed::new(), Some(&dir), None);
+    assert!(r.result.is_ok() && warnings(&r).iter().any(|w| w.contains("larger than")), "{:?}", r.cargo);
+}