From 5ebe40409fb2e7c884a0b0751d40e5212ee45132 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 15:47:59 -0500 Subject: [PATCH] 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 | 98 +++++++++++++++++++++++++++------ crates/fonts-build/src/tests.rs | 95 ++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 18 deletions(-) diff --git a/crates/fonts-build/src/embed.rs b/crates/fonts-build/src/embed.rs index f931ad569..c06e6d525 100644 --- a/crates/fonts-build/src/embed.rs +++ b/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> { + 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::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, 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..ttf`) @@ -181,14 +223,17 @@ impl Embed { pub(crate) fn run(&self, env: &Env, out_file: &Path, cargo: &mut Vec) -> Result, 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) -> Result, 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>)> = 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) -> 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"), } } } diff --git a/crates/fonts-build/src/tests.rs b/crates/fonts-build/src/tests.rs index a8c48deb6..f8754c4ee 100644 --- a/crates/fonts-build/src/tests.rs +++ b/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"").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"", "{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); +}