From 065f51a02048dca4da958702a52524e78277ecac Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Fri, 3 Jul 2026 07:21:01 +0100 Subject: [PATCH 1/3] fix(backends): save through symlinks without destroying the link Atomic UFO and designspace writes operated on the symlink path itself, so the rename-exchange replaced the link with a real directory or file and orphaned the linked location. Existing targets are now canonicalized before staging, so the write lands at the resolved path and the link survives. --- crates/shift-backends/src/atomic.rs | 31 ++++++++++++++++++++- crates/shift-backends/src/ufo/mod.rs | 36 +++++++++++++++++++++++++ crates/shift-backends/src/ufo/writer.rs | 7 ++++- 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/crates/shift-backends/src/atomic.rs b/crates/shift-backends/src/atomic.rs index f0ea2e6b..509f57ed 100644 --- a/crates/shift-backends/src/atomic.rs +++ b/crates/shift-backends/src/atomic.rs @@ -1,7 +1,19 @@ //! Crash-safe filesystem writes shared by the format backends. use std::io::Write; -use std::path::Path; +use std::path::{Path, PathBuf}; + +/// Resolves an existing target to its real path so that writing through a +/// symlink updates the linked file or directory instead of replacing the +/// link itself (the rename/exchange syscalls operate on the link inode). +/// A target that does not exist yet is returned unchanged. +pub(crate) fn resolve_existing_target(target: &Path) -> std::io::Result { + if target.exists() { + std::fs::canonicalize(target) + } else { + Ok(target.to_path_buf()) + } +} /// Fsyncs the parent directory so a rename into it is durable. #[cfg(unix)] @@ -25,6 +37,7 @@ pub(crate) fn sync_parent(_path: &Path) -> std::io::Result<()> { /// over the target, and the parent directory is fsynced so the rename /// itself is durable. If any step fails, the existing file is untouched. pub(crate) fn write_file_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + let path = &resolve_existing_target(path)?; let parent = match path.parent() { Some(parent) if !parent.as_os_str().is_empty() => parent, _ => Path::new("."), @@ -78,4 +91,20 @@ mod tests { result.expect_err("write into a read-only directory should fail"); assert_eq!(std::fs::read(&target).unwrap(), b"original"); } + + #[cfg(unix)] + #[test] + fn writing_through_symlink_updates_target_and_keeps_link() { + let temp = tempfile::tempdir().unwrap(); + let real = temp.path().join("real.xml"); + let link = temp.path().join("link.xml"); + std::fs::write(&real, b"original").unwrap(); + std::os::unix::fs::symlink(&real, &link).unwrap(); + + write_file_atomic(&link, b"replacement").unwrap(); + + assert!(std::fs::symlink_metadata(&link).unwrap().is_symlink()); + assert_eq!(std::fs::read(&real).unwrap(), b"replacement"); + assert_eq!(std::fs::read(&link).unwrap(), b"replacement"); + } } diff --git a/crates/shift-backends/src/ufo/mod.rs b/crates/shift-backends/src/ufo/mod.rs index 341eed23..76dbf0fc 100644 --- a/crates/shift-backends/src/ufo/mod.rs +++ b/crates/shift-backends/src/ufo/mod.rs @@ -264,6 +264,42 @@ mod tests { assert_eq!(entries, vec!["target.ufo"], "no staging leftovers"); } + #[cfg(unix)] + #[test] + fn save_through_symlink_updates_target_and_keeps_link() { + let font = create_test_font(); + let temp_dir = tempfile::tempdir().unwrap(); + let real_ufo = temp_dir.path().join("real.ufo"); + let link_ufo = temp_dir.path().join("link.ufo"); + + UfoWriter::new() + .save(&font, real_ufo.to_str().unwrap()) + .unwrap(); + std::os::unix::fs::symlink(&real_ufo, &link_ufo).unwrap(); + + let mut updated = create_test_font(); + updated.metadata_mut().family_name = Some("UpdatedFamily".to_string()); + UfoWriter::new() + .save(&updated, link_ufo.to_str().unwrap()) + .unwrap(); + + assert!( + fs::symlink_metadata(&link_ufo).unwrap().is_symlink(), + "saving through the symlink must not replace the link" + ); + let reloaded = UfoReader::new().load(real_ufo.to_str().unwrap()).unwrap(); + assert_eq!( + reloaded.metadata().family_name.as_deref(), + Some("UpdatedFamily") + ); + + let entries: Vec<_> = fs::read_dir(temp_dir.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(entries.len(), 2, "no staging leftovers: {entries:?}"); + } + #[test] fn round_trip_preserves_feature_source() { let mut font = create_test_font(); diff --git a/crates/shift-backends/src/ufo/writer.rs b/crates/shift-backends/src/ufo/writer.rs index 3114e79a..2adf4e84 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -1,4 +1,4 @@ -use crate::atomic::sync_parent; +use crate::atomic::{resolve_existing_target, sync_parent}; use crate::errors::{FormatBackendError, FormatBackendResult}; use crate::traits::{FontView, FontWriter}; use norad::{Font as NoradFont, Glyph as NoradGlyph, Line, Name}; @@ -477,6 +477,11 @@ impl UfoWriter { /// swapped into place — atomically where the platform supports a rename /// exchange, otherwise via move-aside-and-replace with restore on failure. fn write_atomic(norad_font: &NoradFont, target: &Path) -> FormatBackendResult<()> { + // Saving through a symlink must update the linked UFO, not turn the + // link itself into a real directory, so the target is resolved to its + // real path and staging happens next to the resolved location. + let target = &resolve_existing_target(target) + .map_err(|e| io_error("resolve UFO path", target, e))?; let file_name = target.file_name().ok_or_else(|| { FormatBackendError::Ufo(format!("invalid UFO path '{}'", target.display())) })?; From cb96f4bb431761731c128a2371096570246479a8 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Fri, 3 Jul 2026 07:35:54 +0100 Subject: [PATCH 2/3] fix(backends): preserve glif image refs, notes, and unknown fontinfo keys Two UFO round-trip gaps: glyph-level placement elements and were dropped when the writer regenerated glyphs (orphaning the preserved image bytes), and any non-spec fontinfo.plist key failed the whole load via norad's deny_unknown_fields. The glif image reference and note now ride in the layer lib passthrough as opaque records under com.shift-editor.preserved.* keys; the writer pops them back out into the written glif, so they survive UFO, .shift, and working-store round-trips without being modeled. fontinfo.plist is partitioned into norad-known keys and an unknown remainder before norad sees it: unknown keys join the fontinfo remainder passthrough (loading through a symlinked shadow directory when needed) and are merged back into the staged fontinfo.plist after norad writes it. --- crates/shift-backends/src/ufo/fontinfo.rs | 220 ++++++++++++++++++ crates/shift-backends/src/ufo/mod.rs | 9 + crates/shift-backends/src/ufo/reader.rs | 67 +++++- crates/shift-backends/src/ufo/writer.rs | 98 ++++++-- crates/shift-backends/tests/round_trip/ufo.rs | 128 ++++++++++ 5 files changed, 506 insertions(+), 16 deletions(-) create mode 100644 crates/shift-backends/src/ufo/fontinfo.rs diff --git a/crates/shift-backends/src/ufo/fontinfo.rs b/crates/shift-backends/src/ufo/fontinfo.rs new file mode 100644 index 00000000..c5fc0fe7 --- /dev/null +++ b/crates/shift-backends/src/ufo/fontinfo.rs @@ -0,0 +1,220 @@ +//! Tolerant `fontinfo.plist` handling. +//! +//! norad deserializes fontinfo with `deny_unknown_fields`, so a single +//! non-spec key would fail the whole UFO load. Shift instead splits fontinfo +//! into the keys norad models and an unknown remainder: the known part flows +//! through norad, the unknown part is carried in the fontinfo remainder +//! passthrough and merged back into the written `fontinfo.plist` on save. + +use crate::errors::{FormatBackendError, FormatBackendResult}; +use std::path::{Path, PathBuf}; + +/// Splits a fontinfo dictionary into the keys norad models and the unknown +/// remainder. A key norad models but whose value it cannot parse is a hard +/// error, matching norad's own verdict on the file. +pub(crate) fn partition_fontinfo( + dict: plist::Dictionary, + context: &str, +) -> FormatBackendResult<(plist::Dictionary, plist::Dictionary)> { + let mut known = plist::Dictionary::new(); + let mut unknown = plist::Dictionary::new(); + + for (key, value) in dict { + let mut probe = plist::Dictionary::new(); + probe.insert(key.clone(), value.clone()); + + match plist::from_value::(&plist::Value::Dictionary(probe)) { + Ok(_) => { + known.insert(key, value); + } + Err(error) if is_unknown_field(&error) => { + unknown.insert(key, value); + } + Err(error) => { + return Err(FormatBackendError::Ufo(format!( + "invalid {context} value for key {key:?}: {error}" + ))); + } + } + } + + Ok((known, unknown)) +} + +fn is_unknown_field(error: &plist::Error) -> bool { + error.to_string().contains("unknown field") +} + +/// Loads a UFO through norad even when `fontinfo.plist` carries keys norad +/// does not model. Returns the loaded font, the unknown fontinfo keys, and — +/// when a sanitized shadow was needed — the temp directory backing the load, +/// which must stay alive until the font's lazy data/images stores have been +/// consumed. +pub(crate) fn load_norad_font_tolerant( + ufo_path: &Path, +) -> FormatBackendResult<(norad::Font, plist::Dictionary, Option)> { + let load = + |path: &Path| norad::Font::load(path).map_err(|e| FormatBackendError::Ufo(e.to_string())); + + let fontinfo_path = ufo_path.join("fontinfo.plist"); + if !fontinfo_path.exists() { + return Ok((load(ufo_path)?, plist::Dictionary::new(), None)); + } + + let fontinfo = plist::Value::from_file(&fontinfo_path) + .map_err(|e| FormatBackendError::Ufo(format!("failed to parse fontinfo.plist: {e}")))?; + let plist::Value::Dictionary(dict) = fontinfo else { + return Err(FormatBackendError::Ufo( + "fontinfo.plist is not a dictionary".to_string(), + )); + }; + + let (known, unknown) = partition_fontinfo(dict, "fontinfo.plist")?; + if unknown.is_empty() { + return Ok((load(ufo_path)?, unknown, None)); + } + + let (shadow, shadow_ufo) = shadow_ufo_with_fontinfo(ufo_path, known)?; + Ok((load(&shadow_ufo)?, unknown, Some(shadow))) +} + +/// Builds a temp-directory view of the UFO whose `fontinfo.plist` holds only +/// the norad-known keys; every other entry points at the real UFO, so no +/// glyph or binary data is copied. +fn shadow_ufo_with_fontinfo( + ufo_path: &Path, + known: plist::Dictionary, +) -> FormatBackendResult<(tempfile::TempDir, PathBuf)> { + let io = |action: &str, e: std::io::Error| { + FormatBackendError::Ufo(format!( + "failed to {action} for tolerant fontinfo load of '{}': {e}", + ufo_path.display() + )) + }; + + let real = std::fs::canonicalize(ufo_path).map_err(|e| io("resolve UFO path", e))?; + let file_name = real + .file_name() + .ok_or_else(|| FormatBackendError::Ufo(format!("invalid UFO path '{}'", real.display())))?; + + let shadow = tempfile::Builder::new() + .prefix(".shift-ufo-fontinfo-") + .tempdir() + .map_err(|e| io("create shadow directory", e))?; + let shadow_ufo = shadow.path().join(file_name); + std::fs::create_dir(&shadow_ufo).map_err(|e| io("create shadow UFO directory", e))?; + + for entry in std::fs::read_dir(&real).map_err(|e| io("read UFO directory", e))? { + let entry = entry.map_err(|e| io("read UFO directory", e))?; + if entry.file_name() == "fontinfo.plist" { + continue; + } + link_entry(&entry.path(), &shadow_ufo.join(entry.file_name())) + .map_err(|e| io("mirror UFO entry", e))?; + } + + plist::Value::Dictionary(known) + .to_file_xml(shadow_ufo.join("fontinfo.plist")) + .map_err(|e| { + FormatBackendError::Ufo(format!("failed to write sanitized fontinfo.plist: {e}")) + })?; + + Ok((shadow, shadow_ufo)) +} + +#[cfg(unix)] +fn link_entry(source: &Path, destination: &Path) -> std::io::Result<()> { + std::os::unix::fs::symlink(source, destination) +} + +// Windows symlinks need elevated privileges; this path is only reached for +// UFOs with unknown fontinfo keys, so a plain copy is acceptable there. +#[cfg(not(unix))] +fn link_entry(source: &Path, destination: &Path) -> std::io::Result<()> { + if source.is_dir() { + for entry in std::fs::read_dir(source)? { + let entry = entry?; + std::fs::create_dir_all(destination)?; + link_entry(&entry.path(), &destination.join(entry.file_name()))?; + } + Ok(()) + } else { + std::fs::copy(source, destination).map(|_| ()) + } +} + +/// Merges the unknown fontinfo keys into a written `fontinfo.plist`. norad +/// cannot represent them, so they are re-applied to the staged file after +/// norad has serialized the known fields. +pub(crate) fn merge_unknown_fontinfo( + fontinfo_path: &Path, + unknown: &plist::Dictionary, +) -> FormatBackendResult<()> { + if unknown.is_empty() { + return Ok(()); + } + + let mut dict = if fontinfo_path.exists() { + match plist::Value::from_file(fontinfo_path).map_err(|e| { + FormatBackendError::Ufo(format!("failed to parse staged fontinfo.plist: {e}")) + })? { + plist::Value::Dictionary(dict) => dict, + _ => plist::Dictionary::new(), + } + } else { + plist::Dictionary::new() + }; + + for (key, value) in unknown { + dict.insert(key.clone(), value.clone()); + } + + plist::Value::Dictionary(dict) + .to_file_xml(fontinfo_path) + .map_err(|e| FormatBackendError::Ufo(format!("failed to write staged fontinfo.plist: {e}"))) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn partitions_unknown_keys_away_from_norad_known_keys() { + let mut dict = plist::Dictionary::new(); + dict.insert("familyName".into(), plist::Value::String("Test".into())); + dict.insert( + "openTypeOS2WeightClass".into(), + plist::Value::Integer(400.into()), + ); + dict.insert( + "com.example.customTool".into(), + plist::Value::String("kept".into()), + ); + + let (known, unknown) = partition_fontinfo(dict, "fontinfo.plist").unwrap(); + + assert!(known.contains_key("familyName")); + assert!(known.contains_key("openTypeOS2WeightClass")); + assert_eq!( + unknown.get("com.example.customTool"), + Some(&plist::Value::String("kept".into())) + ); + assert_eq!(unknown.len(), 1); + } + + #[test] + fn known_key_with_invalid_value_is_a_hard_error() { + let mut dict = plist::Dictionary::new(); + dict.insert( + "openTypeOS2WeightClass".into(), + plist::Value::String("heavy".into()), + ); + + let error = partition_fontinfo(dict, "preserved fontinfo") + .expect_err("invalid value for a norad-known key should fail"); + assert!( + error.to_string().contains("openTypeOS2WeightClass"), + "unexpected error: {error}" + ); + } +} diff --git a/crates/shift-backends/src/ufo/mod.rs b/crates/shift-backends/src/ufo/mod.rs index 76dbf0fc..fe7fecc8 100644 --- a/crates/shift-backends/src/ufo/mod.rs +++ b/crates/shift-backends/src/ufo/mod.rs @@ -1,9 +1,18 @@ +mod fontinfo; mod reader; mod writer; pub use reader::UfoReader; pub use writer::UfoWriter; +/// Layer-lib key carrying a glif `` element (file name, transform, +/// color) as an opaque record, so image placements survive a round-trip +/// alongside the image bytes in `images/`. Shift does not model or edit it. +pub(crate) const PRESERVED_GLYPH_IMAGE_KEY: &str = "com.shift-editor.preserved.image"; + +/// Layer-lib key carrying a glif `` as an opaque record. +pub(crate) const PRESERVED_GLYPH_NOTE_KEY: &str = "com.shift-editor.preserved.note"; + use crate::traits::{FontReader, FontWriter}; use crate::FormatBackendResult; use shift_font::Font; diff --git a/crates/shift-backends/src/ufo/reader.rs b/crates/shift-backends/src/ufo/reader.rs index e405ee80..0fdfb269 100644 --- a/crates/shift-backends/src/ufo/reader.rs +++ b/crates/shift-backends/src/ufo/reader.rs @@ -179,6 +179,45 @@ impl UfoReader { LibData::from_map(data) } + fn convert_image_record(image: &norad::Image) -> LibValue { + let mut record = HashMap::new(); + record.insert( + "fileName".to_string(), + LibValue::String(image.file_name().to_string_lossy().into_owned()), + ); + record.insert( + "xScale".to_string(), + LibValue::Float(image.transform.x_scale), + ); + record.insert( + "xyScale".to_string(), + LibValue::Float(image.transform.xy_scale), + ); + record.insert( + "yxScale".to_string(), + LibValue::Float(image.transform.yx_scale), + ); + record.insert( + "yScale".to_string(), + LibValue::Float(image.transform.y_scale), + ); + record.insert( + "xOffset".to_string(), + LibValue::Float(image.transform.x_offset), + ); + record.insert( + "yOffset".to_string(), + LibValue::Float(image.transform.y_offset), + ); + if let Some(color) = &image.color { + record.insert( + "color".to_string(), + LibValue::String(color.to_rgba_string()), + ); + } + LibValue::Dict(record) + } + fn convert_glyph_layer( norad_glyph: &norad::Glyph, layer_id: LayerId, @@ -211,6 +250,21 @@ impl UfoReader { *glyph_layer.lib_mut() = Self::convert_lib(&norad_glyph.lib); } + // The glif `` element and `` ride along in the layer + // lib as opaque records; the writer pops them back out on save. + if let Some(image) = &norad_glyph.image { + glyph_layer.lib_mut().set( + super::PRESERVED_GLYPH_IMAGE_KEY.to_string(), + Self::convert_image_record(image), + ); + } + if let Some(note) = &norad_glyph.note { + glyph_layer.lib_mut().set( + super::PRESERVED_GLYPH_NOTE_KEY.to_string(), + LibValue::String(note.clone()), + ); + } + let mut glyph = Glyph::new(norad_glyph.name().to_string()); for codepoint in norad_glyph.codepoints.iter() { glyph.add_unicode(u32::from(codepoint)); @@ -315,9 +369,11 @@ impl Default for UfoReader { impl FontReader for UfoReader { fn load(&self, path: &str) -> FormatBackendResult { - let norad_font = - NoradFont::load(path).map_err(|e| FormatBackendError::Ufo(e.to_string()))?; let ufo_path = Path::new(path); + // The shadow directory (if any) must outlive the loop below that + // drains norad's lazy data/images stores. + let (norad_font, unknown_fontinfo, _shadow) = + super::fontinfo::load_norad_font_tolerant(ufo_path)?; let mut font = Font::new(); let default_source_id = font @@ -360,6 +416,13 @@ impl FontReader for UfoReader { *font.fontinfo_remainder_mut() = remainder; } + // Keys norad does not model join the remainder passthrough so they + // survive the round-trip alongside the norad-known unmapped keys. + for (key, value) in &unknown_fontinfo { + font.fontinfo_remainder_mut() + .set(key.clone(), Self::convert_plist_to_lib_value(value)); + } + let norad_default_layer_name = norad_font.layers.default_layer().name().clone(); let mut pending_components = Vec::new(); for layer in norad_font.layers.iter() { diff --git a/crates/shift-backends/src/ufo/writer.rs b/crates/shift-backends/src/ufo/writer.rs index 2adf4e84..b1fdc217 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -277,12 +277,64 @@ impl UfoWriter { .push(Self::convert_guideline(guideline)?); } - if !layer.lib().is_empty() { - norad_glyph.lib = Self::convert_lib(layer.lib()); + let mut lib = layer.lib().clone(); + if let Some(record) = lib.remove(super::PRESERVED_GLYPH_IMAGE_KEY) { + norad_glyph.image = Some(Self::convert_image_from_record(glyph.name(), &record)?); + } + if let Some(LibValue::String(note)) = lib.remove(super::PRESERVED_GLYPH_NOTE_KEY) { + norad_glyph.note = Some(note); + } + if !lib.is_empty() { + norad_glyph.lib = Self::convert_lib(&lib); } Ok(norad_glyph) } + + /// Rebuilds a glif `` element from the opaque record the reader + /// stashed in the layer lib. + fn convert_image_from_record( + glyph_name: &str, + record: &LibValue, + ) -> FormatBackendResult { + let invalid = || { + FormatBackendError::Ufo(format!( + "invalid preserved image record on glyph {glyph_name:?}" + )) + }; + let LibValue::Dict(record) = record else { + return Err(invalid()); + }; + + let file_name = match record.get("fileName") { + Some(LibValue::String(file_name)) => file_name, + _ => return Err(invalid()), + }; + let number = |key: &str| match record.get(key) { + Some(LibValue::Float(value)) => Ok(*value), + Some(LibValue::Integer(value)) => Ok(*value as f64), + _ => Err(invalid()), + }; + let transform = norad::AffineTransform { + x_scale: number("xScale")?, + xy_scale: number("xyScale")?, + yx_scale: number("yxScale")?, + y_scale: number("yScale")?, + x_offset: number("xOffset")?, + y_offset: number("yOffset")?, + }; + let color = match record.get("color") { + Some(LibValue::String(color)) => Some(color.parse().map_err(|_| invalid())?), + Some(_) => return Err(invalid()), + None => None, + }; + + norad::Image::new(PathBuf::from(file_name), color, transform).map_err(|e| { + FormatBackendError::Ufo(format!( + "invalid preserved image file name on glyph {glyph_name:?}: {e}" + )) + }) + } } impl Default for UfoWriter { @@ -315,21 +367,28 @@ impl UfoWriter { } pub fn save_view(&self, font: &impl FontView, path: &str) -> FormatBackendResult<()> { - let norad_font = Self::build_norad_font(font)?; - Self::write_atomic(&norad_font, Path::new(path)) + let (norad_font, unknown_fontinfo) = Self::build_norad_font(font)?; + Self::write_atomic(&norad_font, &unknown_fontinfo, Path::new(path)) } /// Rebuilds `fontinfo.plist` from the carried remainder, then writes every /// Shift-modeled field from the IR so the IR always wins for those keys. - fn build_font_info(font: &impl FontView) -> FormatBackendResult { + /// Remainder keys norad does not model cannot pass through + /// [`norad::FontInfo`]; they are returned separately and merged into the + /// staged `fontinfo.plist` after norad has written it. + fn build_font_info( + font: &impl FontView, + ) -> FormatBackendResult<(norad::FontInfo, plist::Dictionary)> { let remainder = font.fontinfo_remainder(); - let mut font_info = if remainder.is_empty() { - norad::FontInfo::default() + let (mut font_info, unknown) = if remainder.is_empty() { + (norad::FontInfo::default(), plist::Dictionary::new()) } else { let dict = Self::convert_lib(remainder); - plist::from_value(&plist::Value::Dictionary(dict)).map_err(|e| { + let (known, unknown) = super::fontinfo::partition_fontinfo(dict, "preserved fontinfo")?; + let font_info = plist::from_value(&plist::Value::Dictionary(known)).map_err(|e| { FormatBackendError::Ufo(format!("invalid preserved fontinfo data: {e}")) - })? + })?; + (font_info, unknown) }; font_info.family_name = font.metadata().family_name.clone(); @@ -355,12 +414,15 @@ impl UfoWriter { font_info.italic_angle = font.metrics().italic_angle; font_info.guidelines = None; - Ok(font_info) + Ok((font_info, unknown)) } - fn build_norad_font(font: &impl FontView) -> FormatBackendResult { + fn build_norad_font( + font: &impl FontView, + ) -> FormatBackendResult<(NoradFont, plist::Dictionary)> { let mut norad_font = NoradFont::new(); - norad_font.font_info = Self::build_font_info(font)?; + let (font_info, unknown_fontinfo) = Self::build_font_info(font)?; + norad_font.font_info = font_info; let groups = font .kerning() @@ -469,14 +531,18 @@ impl UfoWriter { })?; } - Ok(norad_font) + Ok((norad_font, unknown_fontinfo)) } /// Writes the UFO without ever destroying the existing target: the new /// UFO is fully staged (and fsynced) in a temp sibling directory, then /// swapped into place — atomically where the platform supports a rename /// exchange, otherwise via move-aside-and-replace with restore on failure. - fn write_atomic(norad_font: &NoradFont, target: &Path) -> FormatBackendResult<()> { + fn write_atomic( + norad_font: &NoradFont, + unknown_fontinfo: &plist::Dictionary, + target: &Path, + ) -> FormatBackendResult<()> { // Saving through a symlink must update the linked UFO, not turn the // link itself into a real directory, so the target is resolved to its // real path and staging happens next to the resolved location. @@ -501,6 +567,10 @@ impl UfoWriter { norad_font .save(&staged_ufo) .map_err(|e| FormatBackendError::Ufo(e.to_string()))?; + super::fontinfo::merge_unknown_fontinfo( + &staged_ufo.join("fontinfo.plist"), + unknown_fontinfo, + )?; sync_tree(&staged_ufo).map_err(|e| io_error("sync staged UFO at", &staged_ufo, e))?; if target.exists() { diff --git a/crates/shift-backends/tests/round_trip/ufo.rs b/crates/shift-backends/tests/round_trip/ufo.rs index de1a5eba..01777cd2 100644 --- a/crates/shift-backends/tests/round_trip/ufo.rs +++ b/crates/shift-backends/tests/round_trip/ufo.rs @@ -185,6 +185,22 @@ fn preservation_fixture_ufo(dir: &Path) -> PathBuf { "com.shift.glyphDate".into(), plist::Value::Date(plist::Date::from_xml_format("2025-12-31T23:59:59Z").unwrap()), ); + glyph.note = Some("traced from a scan".to_string()); + glyph.image = Some( + norad::Image::new( + PathBuf::from("glyph-photo.png"), + Some("0,1,0,0.25".parse().unwrap()), + norad::AffineTransform { + x_scale: 0.5, + xy_scale: 0.0, + yx_scale: 0.0, + y_scale: 0.75, + x_offset: 10.0, + y_offset: -20.5, + }, + ) + .unwrap(), + ); norad_font.layers.default_layer_mut().insert_glyph(glyph); let background = norad_font.layers.new_layer("background").unwrap(); @@ -215,6 +231,29 @@ fn preservation_fixture_ufo(dir: &Path) -> PathBuf { let ufo_path = dir.join("Preservation.ufo"); norad_font.save(&ufo_path).expect("fixture UFO should save"); + + // Non-spec fontinfo keys (as other tools write them) go in behind + // norad's back — norad itself refuses to serialize unknown keys. + let fontinfo_path = ufo_path.join("fontinfo.plist"); + let plist::Value::Dictionary(mut fontinfo) = + plist::Value::from_file(&fontinfo_path).expect("fixture fontinfo should parse") + else { + panic!("fixture fontinfo should be a dictionary"); + }; + fontinfo.insert( + "com.example.customFontinfo".into(), + plist::Value::String("survives".into()), + ); + let mut custom = plist::Dictionary::new(); + custom.insert("depth".into(), plist::Value::Integer(2.into())); + fontinfo.insert( + "com.example.customDict".into(), + plist::Value::Dictionary(custom), + ); + plist::Value::Dictionary(fontinfo) + .to_file_xml(&fontinfo_path) + .expect("fixture fontinfo should be writable"); + ufo_path } @@ -379,6 +418,95 @@ fn preserves_libs_binaries_layerinfo_and_fontinfo_through_round_trip() { assert_eq!(reloaded_guideline.color(), original_guideline.color()); } +#[test] +fn preserves_glyph_image_reference_note_and_unknown_fontinfo_keys() { + let temp_dir = tempfile::tempdir().expect("tempdir should be created"); + let fixture = preservation_fixture_ufo(temp_dir.path()); + + // Loading at all is the first regression: an unknown fontinfo key used + // to fail the entire UFO load. + let original = load_font(&fixture); + assert_eq!( + original + .fontinfo_remainder() + .get("com.example.customFontinfo"), + Some(&LibValue::String("survives".into())) + ); + match original.fontinfo_remainder().get("com.example.customDict") { + Some(LibValue::Dict(custom)) => { + assert_eq!(custom.get("depth"), Some(&LibValue::Integer(2))) + } + other => panic!("expected custom dict in remainder, got {other:?}"), + } + + let out_dir = tempfile::tempdir().expect("tempdir should be created"); + let saved = out_dir.path().join("saved.ufo"); + FontLoader::new() + .write_font(&original, saved.to_str().unwrap()) + .expect("font with image reference and unknown fontinfo keys should save"); + + let glif = std::fs::read_to_string(saved.join("glyphs/A_.glif")).expect("A glif should exist"); + assert!(glif.contains(" record.clone(), + other => panic!("expected preserved image record, got {other:?}"), + }; + let original_record = image_record(original_layer); + assert_eq!( + original_record.get("fileName"), + Some(&LibValue::String("glyph-photo.png".into())) + ); + assert_eq!(original_record.get("xScale"), Some(&LibValue::Float(0.5))); + assert_eq!( + original_record.get("yOffset"), + Some(&LibValue::Float(-20.5)) + ); + assert_eq!( + original_record.get("color"), + Some(&LibValue::String("0,1,0,0.25".into())) + ); + assert_eq!(image_record(reloaded_layer), original_record); + assert_eq!( + reloaded_layer.lib().get("com.shift-editor.preserved.note"), + Some(&LibValue::String("traced from a scan".into())) + ); +} + #[test] fn edited_ir_fields_win_over_preserved_fontinfo_on_save() { let temp_dir = tempfile::tempdir().expect("tempdir should be created"); From d7bcffc7a59714cff238a8ee831d123453d79478 Mon Sep 17 00:00:00 2001 From: Kostya Farber Date: Fri, 3 Jul 2026 07:42:30 +0100 Subject: [PATCH 3/3] fix: validate unsavable font state at creation instead of save time Source names that could never be written as UFO layer names (control characters, the reserved public.default) are now rejected when the source is created, and a fontinfo remainder that norad cannot serialize fails at package open / store resume with a clear UnsavableFontState error. The save-time checks remain as backstops. --- crates/shift-backends/src/ufo/fontinfo.rs | 42 +++++++++ crates/shift-backends/src/ufo/mod.rs | 1 + crates/shift-backends/src/ufo/writer.rs | 2 +- crates/shift-font/src/intents.rs | 16 +++- crates/shift-workspace/src/workspace.rs | 13 +++ .../shift-workspace/tests/workspace_test.rs | 92 ++++++++++++++++++- 6 files changed, 161 insertions(+), 5 deletions(-) diff --git a/crates/shift-backends/src/ufo/fontinfo.rs b/crates/shift-backends/src/ufo/fontinfo.rs index c5fc0fe7..3b1c29a3 100644 --- a/crates/shift-backends/src/ufo/fontinfo.rs +++ b/crates/shift-backends/src/ufo/fontinfo.rs @@ -45,6 +45,22 @@ fn is_unknown_field(error: &plist::Error) -> bool { error.to_string().contains("unknown field") } +/// Checks that a carried fontinfo remainder can be written back out as +/// `fontinfo.plist`. Callers that load persisted font state run this where +/// the state is created, so a tampered remainder surfaces at load time +/// instead of failing the next save. +pub fn validate_fontinfo_remainder(remainder: &shift_font::LibData) -> FormatBackendResult<()> { + if remainder.is_empty() { + return Ok(()); + } + + let dict = super::writer::UfoWriter::convert_lib(remainder); + let (known, _unknown) = partition_fontinfo(dict, "preserved fontinfo")?; + plist::from_value::(&plist::Value::Dictionary(known)) + .map(|_| ()) + .map_err(|e| FormatBackendError::Ufo(format!("invalid preserved fontinfo data: {e}"))) +} + /// Loads a UFO through norad even when `fontinfo.plist` carries keys norad /// does not model. Returns the loaded font, the unknown fontinfo keys, and — /// when a sanitized shadow was needed — the temp directory backing the load, @@ -217,4 +233,30 @@ mod tests { "unexpected error: {error}" ); } + + #[test] + fn validate_accepts_unknown_keys_and_rejects_unwritable_known_keys() { + let mut valid = shift_font::LibData::new(); + valid.set( + "com.example.customTool".to_string(), + shift_font::LibValue::String("kept".to_string()), + ); + valid.set( + "openTypeOS2WeightClass".to_string(), + shift_font::LibValue::Integer(700), + ); + validate_fontinfo_remainder(&valid).expect("unknown keys are writable"); + + let mut tampered = shift_font::LibData::new(); + tampered.set( + "openTypeOS2WeightClass".to_string(), + shift_font::LibValue::String("heavy".to_string()), + ); + let error = validate_fontinfo_remainder(&tampered) + .expect_err("a remainder norad cannot write must be rejected"); + assert!( + error.to_string().contains("openTypeOS2WeightClass"), + "unexpected error: {error}" + ); + } } diff --git a/crates/shift-backends/src/ufo/mod.rs b/crates/shift-backends/src/ufo/mod.rs index fe7fecc8..a56f0af0 100644 --- a/crates/shift-backends/src/ufo/mod.rs +++ b/crates/shift-backends/src/ufo/mod.rs @@ -2,6 +2,7 @@ mod fontinfo; mod reader; mod writer; +pub use fontinfo::validate_fontinfo_remainder; pub use reader::UfoReader; pub use writer::UfoWriter; diff --git a/crates/shift-backends/src/ufo/writer.rs b/crates/shift-backends/src/ufo/writer.rs index b1fdc217..83ea99d6 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -232,7 +232,7 @@ impl UfoWriter { } } - fn convert_lib(lib: &LibData) -> plist::Dictionary { + pub(crate) fn convert_lib(lib: &LibData) -> plist::Dictionary { let mut dict = plist::Dictionary::new(); for (k, v) in lib.iter() { dict.insert(k.clone(), Self::convert_lib_value_to_plist(v)); diff --git a/crates/shift-font/src/intents.rs b/crates/shift-font/src/intents.rs index 3073a546..eb40c814 100644 --- a/crates/shift-font/src/intents.rs +++ b/crates/shift-font/src/intents.rs @@ -401,7 +401,7 @@ impl Font { changes: &mut FontChangeSet, ) -> CoreResult> { let name = name.trim(); - if name.is_empty() { + if !is_valid_source_name(name) { return Err(CoreError::InvalidSourceName(name.to_string())); } if self.sources().iter().any(|source| source.name() == name) { @@ -813,3 +813,17 @@ impl Font { .ok_or(CoreError::LayerNotFound(layer_id.clone())) } } + +/// A source name must be a valid UFO layer name, or the font can be built +/// but never saved: no control characters (the UFO name rule) and not the +/// reserved default-layer name. +fn is_valid_source_name(name: &str) -> bool { + if name.is_empty() || name == "public.default" { + return false; + } + + !name.chars().any(|c| { + let c = c as u32; + c <= 0x1f || c == 0x7f || (0x80..=0x9f).contains(&c) + }) +} diff --git a/crates/shift-workspace/src/workspace.rs b/crates/shift-workspace/src/workspace.rs index b40a0862..8be7ebdc 100644 --- a/crates/shift-workspace/src/workspace.rs +++ b/crates/shift-workspace/src/workspace.rs @@ -53,6 +53,9 @@ pub enum WorkspaceError { #[error("corrupt working store: {0}")] CorruptWorkingStore(String), + #[error("loaded font state cannot be saved back to source: {0}")] + UnsavableFontState(String), + #[error("invalid UTF-8 in workspace path: {0}")] InvalidPathUtf8(PathBuf), @@ -148,6 +151,7 @@ impl FontWorkspace { .workspace_state()? .ok_or_else(|| WorkspaceError::CorruptWorkingStore("missing workspace_state".into()))?; let font = store.load_font_state()?; + validate_loaded_font(&font)?; let source = source_from_workspace_state(&state)?; Ok(Self { @@ -562,6 +566,7 @@ impl FontWorkspace { let source_package = ShiftSourcePackage::open(source_path)?; let mut store = ShiftStore::open(store_path)?; let font = ShiftSourcePackage::load_font(source_package.path())?; + validate_loaded_font(&font)?; store.set_font_info(font_info_from_font(&font))?; store.replace_font_state(&font)?; let identity = source_identity_snapshot(source_package.path())?; @@ -892,6 +897,14 @@ fn replay_glyph_layer( Ok(()) } +/// Rejects loaded font state that could never be saved back out, so a +/// tampered `.shift` package or working store fails at open/resume with a +/// clear error instead of at the user's next save. +fn validate_loaded_font(font: &shift_font::Font) -> Result<(), WorkspaceError> { + shift_backends::ufo::validate_fontinfo_remainder(font.fontinfo_remainder()) + .map_err(|error| WorkspaceError::UnsavableFontState(error.to_string())) +} + fn font_info_from_font(font: &shift_font::Font) -> shift_store::FontInfo { let metadata = font.metadata(); let metrics = font.metrics(); diff --git a/crates/shift-workspace/tests/workspace_test.rs b/crates/shift-workspace/tests/workspace_test.rs index ec4bdc33..b7ad7b9a 100644 --- a/crates/shift-workspace/tests/workspace_test.rs +++ b/crates/shift-workspace/tests/workspace_test.rs @@ -1,9 +1,9 @@ use std::{fs, path::PathBuf}; use shift_font::{ - AnchorId, AnchorSeed, Axis, AxisId, BooleanOp, ContourId, FontChange, FontIntent, - FontIntentSet, GlyphId, GlyphName, LayerId, Location, PointId, PointSeed, PointType, SourceId, - error::CoreError, + AnchorId, AnchorSeed, Axis, AxisId, BooleanOp, ContourId, Font, FontChange, FontIntent, + FontIntentSet, GlyphId, GlyphName, LayerId, LibValue, Location, PointId, PointSeed, PointType, + SourceId, error::CoreError, }; use shift_source::ShiftSourcePackage; use shift_workspace::{ @@ -425,6 +425,92 @@ fn clean_package_workspace_is_not_recovered_after_source_file_changes() { assert_eq!(recovery, RecoverySelection::None); } +#[test] +fn create_source_rejects_names_unwritable_as_ufo_layers() { + let temp = tempfile::tempdir().unwrap(); + let store_path = temp.path().join("working.sqlite"); + let mut workspace = FontWorkspace::create_untitled(&store_path, NewWorkspace::new()).unwrap(); + + for bad_name in ["Bo\u{0001}ld", "public.default"] { + let result = workspace.apply( + FontIntentSet { + intents: vec![FontIntent::CreateSource { + source_id: SourceId::new(), + name: bad_name.to_string(), + location: Location::new(), + }], + }, + None, + ); + let Err(error) = result else { + panic!("source name {bad_name:?} unwritable as a UFO layer must be rejected"); + }; + + assert!( + matches!( + &error, + WorkspaceError::Font(CoreError::InvalidSourceName(name)) if name == bad_name + ), + "unexpected error for {bad_name:?}: {error:?}" + ); + } + + assert_eq!(workspace.font().sources().len(), 1); +} + +#[test] +fn tampered_package_fontinfo_remainder_fails_at_open() { + let temp = tempfile::tempdir().unwrap(); + let source_path = temp.path().join("Tampered.shift"); + let store_path = temp.path().join("working.sqlite"); + + // A package can carry a remainder value norad could never write back to + // fontinfo.plist (as if the file was edited outside Shift). Opening it + // must fail up front instead of leaving a workspace that errors on save. + let mut font = Font::new(); + font.fontinfo_remainder_mut().set( + "openTypeOS2WeightClass".to_string(), + LibValue::String("heavy".to_string()), + ); + ShiftSourcePackage::save_font(&source_path, &font).unwrap(); + + let Err(error) = FontWorkspace::open(&source_path, &store_path) else { + panic!("tampered fontinfo remainder should fail at open"); + }; + + assert!( + matches!(&error, WorkspaceError::UnsavableFontState(message) + if message.contains("openTypeOS2WeightClass")), + "unexpected error: {error:?}" + ); +} + +#[test] +fn tampered_store_fontinfo_remainder_fails_at_resume() { + let temp = tempfile::tempdir().unwrap(); + let store_path = temp.path().join("working.sqlite"); + drop(FontWorkspace::create_untitled(&store_path, NewWorkspace::new()).unwrap()); + + let mut store = shift_store::ShiftStore::open(&store_path).unwrap(); + let mut font = Font::new(); + font.fontinfo_remainder_mut().set( + "openTypeOS2WeightClass".to_string(), + LibValue::String("heavy".to_string()), + ); + store.replace_font_state(&font).unwrap(); + drop(store); + + let Err(error) = FontWorkspace::resume(&store_path) else { + panic!("tampered fontinfo remainder should fail at resume"); + }; + + assert!( + matches!(&error, WorkspaceError::UnsavableFontState(message) + if message.contains("openTypeOS2WeightClass")), + "unexpected error: {error:?}" + ); +} + fn set_x_advance_intents(layer_id: LayerId, width: f64) -> FontIntentSet { FontIntentSet { intents: vec![FontIntent::SetXAdvance { layer_id, width }],