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/fontinfo.rs b/crates/shift-backends/src/ufo/fontinfo.rs new file mode 100644 index 00000000..3b1c29a3 --- /dev/null +++ b/crates/shift-backends/src/ufo/fontinfo.rs @@ -0,0 +1,262 @@ +//! 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") +} + +/// 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, +/// 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}" + ); + } + + #[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 341eed23..a56f0af0 100644 --- a/crates/shift-backends/src/ufo/mod.rs +++ b/crates/shift-backends/src/ufo/mod.rs @@ -1,9 +1,19 @@ +mod fontinfo; mod reader; mod writer; +pub use fontinfo::validate_fontinfo_remainder; 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; @@ -264,6 +274,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/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 3114e79a..83ea99d6 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}; @@ -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)); @@ -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,23 @@ 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. + 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())) })?; @@ -496,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"); 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 }],