From c4d7948e68744ada469c692d38179209fe68f88e Mon Sep 17 00:00:00 2001 From: Martin Ball Date: Mon, 2 Nov 2026 00:06:43 +0000 Subject: [PATCH] =?UTF-8?q?FU.2:=20language=20packs=20=E2=80=94=20tag=20ch?= =?UTF-8?q?aracters=20and=20line/paragraph=20separators=20are=20stripped?= =?UTF-8?q?=20from=20shown=20text;=20font=20store=20errors=20go=20through?= =?UTF-8?q?=20the=20same=20sanitiser?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FypgCX8LsYYE8fsj4byBm2 --- crates/ui-egui/src/language_packs.rs | 126 +++++++++++++++++++++++++-- 1 file changed, 119 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/language_packs.rs b/crates/ui-egui/src/language_packs.rs index 170750e7e6..66e736d4c8 100644 --- a/crates/ui-egui/src/language_packs.rs +++ b/crates/ui-egui/src/language_packs.rs @@ -32,10 +32,33 @@ fn shown_text(text: &str) -> String { shown_keeping(text, usize::MAX, true) } +/// Most characters of a failure the window shows: the font store's errors can quote the index +/// URL or the server's answer, which aren't ours to trust either. +const MAX_ERROR: usize = 500; + +/// A failure from the engine or the font store as shown: [`shown`], at most [`MAX_ERROR`] +/// characters. +fn shown_error(e: &str) -> String { + shown(e, MAX_ERROR) +} + fn shown_keeping(text: &str, max: usize, lines: bool) -> String { + // Line and paragraph separators (U+2028/9) break lines even where `lines` is off, and tag + // characters (U+E0000–U+E007F) are invisible text an index could hide words in. let format = |c: char| { (c.is_control() && !(lines && matches!(c, '\n' | '\t'))) - || matches!(c, '\u{061C}' | '\u{00AD}' | '\u{180E}' | '\u{200B}'..='\u{200F}' | '\u{202A}'..='\u{202E}' | '\u{2060}'..='\u{206F}' | '\u{FEFF}' | '\u{FFF9}'..='\u{FFFB}') + || matches!( + c, + '\u{061C}' + | '\u{00AD}' + | '\u{180E}' + | '\u{200B}'..='\u{200F}' + | '\u{2028}'..='\u{202E}' + | '\u{2060}'..='\u{206F}' + | '\u{FEFF}' + | '\u{FFF9}'..='\u{FFFB}' + | '\u{E0000}'..='\u{E007F}' + ) }; let clean: String = text.chars().filter(|c| !format(*c)).collect(); if clean.chars().count() <= max { @@ -165,9 +188,9 @@ fn error_of(app: &WordApp, locale: &str) -> Option { } let e = packs::error(locale)?; Some(if locale.is_empty() { - crate::i18n::fmt(tl!("Couldn't get the list of languages: {error}"), &[("error", &e)]) + crate::i18n::fmt(tl!("Couldn't get the list of languages: {error}"), &[("error", &shown_error(&e))]) } else { - crate::i18n::fmt(tl!("Couldn't install: {error}"), &[("error", &e)]) + crate::i18n::fmt(tl!("Couldn't install: {error}"), &[("error", &shown_error(&e))]) }) } @@ -184,7 +207,7 @@ fn consent(app: &mut WordApp, ctx: &egui::Context, key: &str) -> bool { return false; } if let Err(e) = backend.enable_network() { - app.packs_rt.errors.insert(key.to_string(), crate::i18n::fmt(tl!("Couldn't turn on downloading: {error}"), &[("error", &e)])); + app.packs_rt.errors.insert(key.to_string(), crate::i18n::fmt(tl!("Couldn't turn on downloading: {error}"), &[("error", &shown_error(&e))])); return false; } true @@ -197,7 +220,7 @@ fn start_install(app: &mut WordApp, ctx: &egui::Context, pack: &Pack) { return; } if let Err(e) = packs::start_install(&pack.locale) { - app.packs_rt.errors.insert(pack.locale.clone(), crate::i18n::fmt(tl!("Couldn't install: {error}"), &[("error", &e)])); + app.packs_rt.errors.insert(pack.locale.clone(), crate::i18n::fmt(tl!("Couldn't install: {error}"), &[("error", &shown_error(&e))])); } app.packs_rt.missing = None; } @@ -210,7 +233,9 @@ fn start_fetch(app: &mut WordApp, ctx: &egui::Context) { return; } if let Err(e) = packs::start_refresh() { - app.packs_rt.errors.insert(String::new(), crate::i18n::fmt(tl!("Couldn't get the list of languages: {error}"), &[("error", &e)])); + app.packs_rt + .errors + .insert(String::new(), crate::i18n::fmt(tl!("Couldn't get the list of languages: {error}"), &[("error", &shown_error(&e))])); } } @@ -221,7 +246,7 @@ fn remove(app: &mut WordApp, locale: &str) { app.packs_rt.missing = None; } Err(e) => { - app.packs_rt.errors.insert(locale.to_string(), crate::i18n::fmt(tl!("Couldn't remove: {error}"), &[("error", &e)])); + app.packs_rt.errors.insert(locale.to_string(), crate::i18n::fmt(tl!("Couldn't remove: {error}"), &[("error", &shown_error(&e))])); } } } @@ -556,11 +581,98 @@ mod tests { assert_eq!(super::language_of("ja", "日本語"), "Japanese"); } + /// Tests that set the process-wide pack backend take turns. + static BACKEND_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Characters a pack index or a server's answer could use to hide, reorder or break text. + fn hostile(c: char) -> bool { + matches!(c, '\u{2028}' | '\u{2029}' | '\u{202E}' | '\u{E0000}'..='\u{E007F}') || (c.is_control() && c != '\n' && c != '\t') + } + + /// FU.2: tag characters (U+E0000–U+E007F) and line/paragraph separators (U+2028/9) are + /// stripped like the other format characters, from names and from licence texts alike. + #[test] + fn shown_strips_tag_characters_and_separators() { + let name = "Ned\u{E0001}\u{E0041}\u{E007F}erlands\u{2028}\u{2029}"; + assert_eq!(super::shown(name, 100), "Nederlands"); + assert_eq!(super::shown_text("one\u{2028}two\nthree\u{2029}\tfour\u{E0020}"), "onetwo\nthree\tfour"); + // A cut never splits what's left, and counts characters, not bytes. + assert_eq!(super::shown("\u{E0041}ab\u{2028}cd", 3), "ab…"); + } + + /// A store that fails with a hostile message: the index URL with a right-to-left override, + /// separators, tag characters, and far too long. + struct Hostile { + inner: FixedBackend, + message: String, + } + + impl Backend for Hostile { + fn index(&self) -> Result, String> { + self.inner.index() + } + fn fetch_index(&self, _: &packs::Cancel) -> Result, String> { + Err(self.message.clone()) + } + fn network_on(&self) -> bool { + self.inner.network_on() + } + fn enable_network(&self) -> Result<(), String> { + Err(self.message.clone()) + } + fn install(&self, _: &packs::Pack, _: &packs::Cancel, _: &mut dyn FnMut(u64, u64)) -> Result<(), String> { + Err(self.message.clone()) + } + fn remove(&self, _: &str) -> Result<(), String> { + Err(self.message.clone()) + } + fn packs_dir(&self) -> Option { + None + } + } + + /// FU.2: the font store's failures (which can quote the index URL and the server's answer) + /// are shown through the same sanitiser as the index's text, whether the window had them + /// (turning downloading on) or the engine did (an install, the list). + #[test] + fn font_store_errors_are_sanitised() { + let _g = BACKEND_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); + struct Reset; + impl Drop for Reset { + fn drop(&mut self) { + packs::set_background(None); + packs::set_backend(None); + } + } + let _reset = Reset; + let message = format!("https://evil.example/\u{202E}gnp.exe\u{2028}second line\u{E0041}\u{E0042}{}", "x".repeat(5000)); + let backend = std::sync::Arc::new(Hostile { inner: FixedBackend::sample(), message }); + packs::set_backend(Some(backend.clone() as std::sync::Arc)); + let ctx = egui::Context::default(); + let mut a = WordApp::new(wordcraft_engine::Session::new(wordcraft_doc::Document::new()), crate::Services::default()); + let clean = |shown: &str| { + assert!(!shown.chars().any(hostile), "{shown:?}"); + assert!(shown.contains("https://evil.example/gnp.exe"), "{shown:?}"); + assert!(shown.chars().count() < super::MAX_ERROR + 100, "capped: {} characters", shown.chars().count()); + }; + // Turning downloading on fails: the window's own failure. + assert!(!super::consent(&mut a, &ctx, "nl")); + clean(&super::error_of(&a, "nl").expect("a failure is shown")); + // An install and the list fail in the engine. + a.packs_rt.errors.clear(); + backend.inner.network.store(true, Ordering::Relaxed); + assert!(packs::start_install("nl").is_err()); + clean(&super::error_of(&a, "nl").expect("the install's failure is shown")); + assert!(packs::start_refresh().is_err()); + clean(&super::error_of(&a, "").expect("the list's failure is shown")); + } + /// CP4.3 review: a click the control channel made (MCP's `click`, `ui.click`) on the bar's /// Install never turns on downloading, which is the font store's one setting for fonts too; /// it says why instead. The person's own click on the same button does, and installs. #[test] fn a_faked_click_never_turns_on_downloading() { + let _g = BACKEND_LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner); let ctx = egui::Context::default(); crate::theme::install_fonts(&ctx); crate::theme::apply(&ctx, crate::theme::Appearance::Light);