From 3bfec89085b44179d878966d81f13e7849f2e839 Mon Sep 17 00:00:00 2001 From: Sam Sung <51686767+samwhosung@users.noreply.github.com> Date: Mon, 12 Oct 2026 03:36:57 +0700 Subject: [PATCH] A template name registers once, case-folded, as in 1.12.1 (#937) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FrameXML template registered under a name already taken replaced the first, nothing was logged, and names differing only in case were stored twice. In 1.12.1 the registrar `0x6ee500` folds case (`0x64b3f0`, `0x64a4c0`) and keeps the first entry, logging "Virtual object named %s already exists" (`0x871188`), unless the old entry carries a non-zero script context and the new one zero (`0x6ee594`). The stock files load at context 0; `AddOn_Load` (`0x51f240`) loads an addon under its name unless its signature checked SECURE (`0x51dbfb`). Templates are now keyed case-folded with their context, an addon's load sets the context, an addon counts as secure only from the player's own data with `## Secure: 1`, and the refusal and the unnamed-node line (`0x871020`) reach FrameXML.log. Tests: duplicates and case variants refused, the context rule, the log lines. Fixes #556 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5.5 --- crates/benilla-app/src/addon_harness/mod.rs | 15 +- crates/benilla-app/src/ui_script/addons.rs | 94 ++++++- .../src/ui_script/bagnon_render_tests.rs | 2 +- .../benilla-app/src/ui_script/quiver_tests.rs | 2 +- crates/benilla-app/src/ui_script/test_ui.rs | 3 +- .../src/ui_script/trainer_tests.rs | 6 +- crates/benilla-ui/src/framexml.rs | 161 +++++++++--- crates/benilla-ui/src/loader/mod.rs | 69 +++-- .../src/loader/template_registry_tests.rs | 245 ++++++++++++++++++ crates/benilla-ui/src/script/addon.rs | 32 ++- crates/benilla-ui/src/script/mod.rs | 6 +- crates/benilla-ui/src/script/model.rs | 8 +- crates/benilla-ui/src/script/object.rs | 13 +- .../src/script/object/events_regions.rs | 11 +- crates/benilla-ui/src/status.rs | 3 + 15 files changed, 570 insertions(+), 100 deletions(-) create mode 100644 crates/benilla-ui/src/loader/template_registry_tests.rs diff --git a/crates/benilla-app/src/addon_harness/mod.rs b/crates/benilla-app/src/addon_harness/mod.rs index 3a48574c..62220287 100644 --- a/crates/benilla-app/src/addon_harness/mod.rs +++ b/crates/benilla-app/src/addon_harness/mod.rs @@ -162,7 +162,7 @@ fn corpus( let toc = Toc::parse(&benilla_ui::source::decode( &std::fs::read(manifest_path(root, n)?).unwrap_or_default(), )); - Some(crate::ui_script::addons::info_from_toc(n, &toc)) + Some(crate::ui_script::addons::info_from_toc(n, &toc, false)) }) .collect(); @@ -1397,6 +1397,19 @@ struct FileLoad { /// FrameXML, in the install-relative path space (`Interface/AddOns/`, as /// `ui_script::addons::Addon::prefix`), so chunk names match a live session's. fn load_addon_files(script: &UiScript, root: &Path, name: &str, toc: &Toc) -> FileLoad { + // Under the addon's own context, as the live walk loads it. + let outer = script.swap_exec_context(benilla_ui::script::addon_load_context( + name, + // The corpus is folder addons, never the chain. + crate::ui_script::addons::secure(false, toc), + )); + let load = load_addon_files_in_context(script, root, name, toc); + script.swap_exec_context(outer); + load +} + +/// [`load_addon_files`]' body. +fn load_addon_files_in_context(script: &UiScript, root: &Path, name: &str, toc: &Toc) -> FileLoad { let provider = |req: &str| -> Option> { read_under(root, req) }; let base = addon_base(name); let mut errors = Vec::new(); diff --git a/crates/benilla-app/src/ui_script/addons.rs b/crates/benilla-app/src/ui_script/addons.rs index cdbf2117..7e6d1630 100644 --- a/crates/benilla-app/src/ui_script/addons.rs +++ b/crates/benilla-app/src/ui_script/addons.rs @@ -88,6 +88,11 @@ impl Addon { } } + /// Whether the files come off the player's patch chain. + fn is_chain(&self) -> bool { + matches!(self.source, Source::Chain) + } + /// The base this addon's manifest entries resolve against: `Interface/AddOns/` for a /// `Dir`, empty for the shipped flat tree and the chain's full paths. fn prefix(&self) -> String { @@ -395,21 +400,34 @@ fn manifest_in(dir: &Path, name: &str) -> Option { /// The AddOn API's registry row for one discovered addon. fn info_for(addon: &Addon) -> benilla_ui::script::AddOnInfo { - let mut info = info_from_toc(&addon.name, &addon.toc); - info.chain = matches!(addon.source, Source::Chain); - info + info_from_toc(&addon.name, &addon.toc, addon.is_chain()) +} + +/// The `## Secure: 1` directive. It grants nothing by itself: the client sends every such addon +/// in `CMSG_AUTH_SESSION`'s addon block, and only [`secure`] makes one SECURE. +pub(crate) fn toc_secure(toc: &Toc) -> bool { + toc.directive("Secure").map(str::trim) == Some("1") +} + +/// Whether an addon is SECURE, the security state `[rec+0x24] == 0`: `GetAddOnInfo`'s security +/// answer and the context it loads under ([`benilla_ui::script::addon_load_context`]). The +/// reference sets it only when the `SMSG_ADDON_INFO` handler's `%s.sig` check `0x6f10f0` answers 3 +/// (`0x51dbfb`); the state starts INSECURE (`0x5205b6`). benilla verifies no signature, and the +/// `.sig` files ship only with Blizzard's own addons in the player's patch chain, so this stands in +/// for the check: off the chain and `## Secure: 1`. A folder addon claiming it stays INSECURE. +pub(crate) fn secure(chain: bool, toc: &Toc) -> bool { + chain && toc_secure(toc) } /// [`info_for`]'s body, shared with `addon_harness` so its survey seats the registry row -/// production does. -pub(crate) fn info_from_toc(name: &str, toc: &Toc) -> benilla_ui::script::AddOnInfo { +/// production does. `chain` is whether the addon comes off the player's patch chain. +pub(crate) fn info_from_toc(name: &str, toc: &Toc, chain: bool) -> benilla_ui::script::AddOnInfo { benilla_ui::script::AddOnInfo { name: name.to_owned(), title: toc.directive("Title").map(str::to_owned), notes: toc.directive("Notes").map(str::to_owned), url: toc.directive("URL").map(str::to_owned), - // `## Secure: 1` grants nothing; it only sets `GetAddOnInfo`'s security answer. - secure: toc.directive("Secure").map(str::trim) == Some("1"), + secure: secure(chain, toc), load_on_demand: toc.load_on_demand(), dependencies: toc.dependencies().into_iter().map(str::to_owned).collect(), directives: toc.directives.clone(), @@ -432,7 +450,7 @@ pub(crate) fn info_from_toc(name: &str, toc: &Toc) -> benilla_ui::script::AddOnI loaded: false, // The version gate's dword: the leading integer, 0 when absent (out of date, not unknown). interface: toc.interface_version(), - chain: false, // `info_for` stamps the chain rows; the harness's rows are folder addons + chain, } } @@ -908,6 +926,11 @@ impl Walk { // (`0x51f464`), merged into the UI load's. let debug = script.framexml_debug(); let mut toc = Status::default(); + // The files, `Bindings.xml` and the saved variables run under the addon's context. + let outer = script.swap_exec_context(benilla_ui::script::addon_load_context( + &addon.name, + secure(addon.is_chain(), &addon.toc), + )); self.failures.extend(addon.load(script, &mut toc)); let mut own = Status::default(); let toc_path = status::addon_toc_path(&addon.name); @@ -932,6 +955,7 @@ impl Walk { // The saved variables, account then per-character (`0x51f4b5`, `0x51f53b`): after the // files set their defaults, before `ADDON_LOADED`. script.load_addon_saved_variables(&addon.name); + script.swap_exec_context(outer); self.loaded.insert(addon.name.clone()); script.mark_addon_loaded(&addon.name); // `arg1` is the addon's own folder name, whatever case a dependent used. Marked loaded @@ -1969,6 +1993,60 @@ mod tests { script.note_addon_info_reply(&hidden); } + /// A folder addon that writes `## Secure: 1` has no signature behind it, so it stays INSECURE + /// (`[rec+0x24]` starts 1 at `0x5205b6`; only the `.sig` check `0x6f10f0` answering 3 clears it, + /// `0x51dbfb`): it loads under its own context, so it cannot take a name an insecure addon + /// registered, and a later context-0 registration replaces its template. + #[test] + fn a_folder_addon_claiming_secure_loads_insecure() { + let _l = crate::local_state::test_env::ENV_LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let _c = crate::local_state::test_env::EnvGuard::unset("WOW_CAPTURE"); + let (home, _h) = hermetic_root("claimsecure"); + let tmpl = |width: u32| { + format!( + r#" + + "# + ) + }; + let (plain, claim) = (tmpl(10), tmpl(20)); + write_addon( + &home, + "Plain", + "## Interface: 11200\nPlain.xml\n", + &[("Plain.xml", &plain)], + ); + write_addon( + &home, + "Zclaim", + "## Interface: 11200\n## Secure: 1\nZclaim.xml\n", + &[("Zclaim.xml", &claim)], + ); + let mut script = UiScript::new().unwrap(); + script.set_screen_size(1024.0, 768.0); + let _ = load_third_party(&mut script, None, &[], true); + seat_stock_addon_reply(&mut script); + let width = |s: &UiScript| { + s.eval::("return CreateFrame('Frame', nil, nil, 'ClaimTemplate'):GetWidth()") + .unwrap() + }; + assert_eq!( + script + .eval::("local _, _, _, _, _, _, s = GetAddOnInfo('Zclaim') return s") + .ok(), + Some("INSECURE".to_string()) + ); + assert_eq!(width(&script), 10.0, "the claim replaced Plain's template"); + // A context-0 registration replaces it, which a SECURE entry would refuse. + let doc = benilla_ui::framexml::parse(&tmpl(30)).unwrap(); + let report = benilla_ui::loader::load(&script, &doc, &|_| None); + assert!(report.status.is_empty(), "{:?}", report.status.lines()); + assert_eq!(width(&script), 30.0); + let _ = home; + } + /// The in-game `GetAddOnInfo` (`0x48e390`) answers seven values, `name, title, notes, enabled, /// loadable, reason, security`; glue's (`0x46d460`) answers eight, with `url` at slot 4. #[test] diff --git a/crates/benilla-app/src/ui_script/bagnon_render_tests.rs b/crates/benilla-app/src/ui_script/bagnon_render_tests.rs index 8cf89622..19cc1ba8 100644 --- a/crates/benilla-app/src/ui_script/bagnon_render_tests.rs +++ b/crates/benilla-app/src/ui_script/bagnon_render_tests.rs @@ -129,7 +129,7 @@ fn seat(root: &Path, seat: Seat) -> UiScript { let mut registry: Vec = INSTALLED .iter() - .map(|n| super::addons::info_from_toc(n, &read_toc(root, n))) + .map(|n| super::addons::info_from_toc(n, &read_toc(root, n), false)) .collect(); s.register_addons(registry.clone(), Some(root.to_path_buf()), None, None); s.set_realm_name("Harness"); diff --git a/crates/benilla-app/src/ui_script/quiver_tests.rs b/crates/benilla-app/src/ui_script/quiver_tests.rs index 2351dbea..4869dcb1 100644 --- a/crates/benilla-app/src/ui_script/quiver_tests.rs +++ b/crates/benilla-app/src/ui_script/quiver_tests.rs @@ -133,7 +133,7 @@ fn seat_a_hunter(root: &Path) -> UiScript { }); } - let info = super::addons::info_from_toc("Quiver", &read_toc(root, "Quiver")); + let info = super::addons::info_from_toc("Quiver", &read_toc(root, "Quiver"), false); s.register_addons(vec![info], Some(root.to_path_buf()), None, None); // The in-game UI loads on world entry, so a player always exists by then. s.set_unit( diff --git a/crates/benilla-app/src/ui_script/test_ui.rs b/crates/benilla-app/src/ui_script/test_ui.rs index 89be697e..be1c3c6c 100644 --- a/crates/benilla-app/src/ui_script/test_ui.rs +++ b/crates/benilla-app/src/ui_script/test_ui.rs @@ -819,8 +819,7 @@ pub(crate) fn seat_chain_addon(s: &mut UiScript, name: &str) { let toc = super::reference_ui::read(&format!("Interface/AddOns/{name}/{name}.toc")) .map(|b| benilla_ui::toc::Toc::parse(&benilla_ui::source::decode(&b))) .unwrap_or_else(|| panic!("{name}: no toc off the chain")); - let mut info = super::addons::info_from_toc(name, &toc); - info.chain = true; + let info = super::addons::info_from_toc(name, &toc, true); s.set_addon_chain_reader(Box::new(super::reference_ui::read)); s.register_addons(vec![info], None, None, None); } diff --git a/crates/benilla-app/src/ui_script/trainer_tests.rs b/crates/benilla-app/src/ui_script/trainer_tests.rs index 7af7d278..b1c0e4e0 100644 --- a/crates/benilla-app/src/ui_script/trainer_tests.rs +++ b/crates/benilla-app/src/ui_script/trainer_tests.rs @@ -654,11 +654,7 @@ fn the_state_filter_survives_a_restart_through_the_saved_variables_file() { .map(|b| benilla_ui::toc::Toc::parse(&benilla_ui::source::decode(&b))) .expect("the addon's toc off the chain"); assert!(toc.load_on_demand(), "the reference ships it LoadOnDemand"); - let info = || { - let mut i = super::addons::info_from_toc("Blizzard_TrainerUI", &toc); - i.chain = true; - i - }; + let info = || super::addons::info_from_toc("Blizzard_TrainerUI", &toc, true); let saved_dir = std::env::temp_dir().join(format!("benilla-trainer-saved-{}", std::process::id())); let _ = std::fs::remove_dir_all(&saved_dir); diff --git a/crates/benilla-ui/src/framexml.rs b/crates/benilla-ui/src/framexml.rs index 0296fd23..882d7ab8 100644 --- a/crates/benilla-ui/src/framexml.rs +++ b/crates/benilla-ui/src/framexml.rs @@ -81,22 +81,125 @@ pub enum TopLevel { #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct ParsedDocument { pub items: Vec, - /// An unnamed virtual, an `` or `` missing its attribute, a root that is not - /// ``: logged, and the file goes on. + /// An `` or `` missing its attribute, a root that is not ``: logged, and the + /// file goes on. pub warnings: Vec, } impl ParsedDocument { - /// The named templates, for [`expand`]; an unnamed one (the reference's "Unnamed virtual - /// node" error, `0x6ee500`) is absent. - pub fn templates(&self) -> HashMap<&str, &Element> { - self.items - .iter() - .filter_map(|item| match item { - TopLevel::Template(el) => el.name().map(|name| (name, el)), - _ => None, - }) - .collect() + /// The named templates, registered in document order as the loader registers them, for + /// [`expand`]; an unnamed one (the reference's "Unnamed virtual node" error, `0x6ee1c4`) is + /// absent. + pub fn templates(&self) -> Templates { + let mut templates = Templates::default(); + for item in &self.items { + if let TopLevel::Template(el) = item { + if let Some(name) = el.name().filter(|n| !n.is_empty()) { + templates.register(name, el.clone(), None); + } + } + } + templates + } +} + +/// What an `inherits=` name resolves against: the template table, or the font namespace. +pub trait Inheritable { + /// The element registered under `name`, compared case-insensitively, ASCII only. + fn find(&self, name: &str) -> Option<&Element>; +} + +impl Inheritable for HashMap<&str, &Element> { + fn find(&self, name: &str) -> Option<&Element> { + self.get(name).copied().or_else(|| { + self.iter() + .find(|(k, _)| k.eq_ignore_ascii_case(name)) + .map(|(_, v)| *v) + }) + } +} + +/// The FrameXML template table (`0x6ee500`), global across a UI load's files. A name registers +/// once: the hash upper-cases it (`SStrHash 0x64b3f0`) and the bucket walk compares with +/// `SStrCmpI 0x64a4c0`, so the key is the name with ASCII case folded, and the lookup `0x6ee6f0` +/// matches the same way. Each entry keeps the script-execution context it was registered under. +#[derive(Debug, Clone, Default)] +pub struct Templates { + entries: HashMap, +} + +#[derive(Debug, Clone)] +struct Template { + element: Element, + /// The context word at registration (`[entry+0x1c]`, `0x6ee6be`); `None` is the reference's 0. + context: Option, +} + +/// What [`Templates::register`] did with a definition. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Registration { + /// The name was free. + Added, + /// The name's entry came from a non-zero context and this one from context 0 (`0x6ee5c1`). + Replaced, + /// The name was taken: the reference logs `"Virtual object named %s already exists"` + /// (`0x871188`, severity 1) and keeps the first (`0x6ee5a4`). + Refused, +} + +impl Templates { + /// Register `element` under `name` from `context` (`None` is context 0). A taken name is + /// replaced only when its entry's context is non-zero and this one is 0 (`0x6ee594`-`0x6ee5a2`); + /// every other repeat is refused and leaves the entry as it was. + pub fn register( + &mut self, + name: &str, + element: Element, + context: Option<&str>, + ) -> Registration { + let template = Template { + element, + context: context.map(str::to_owned), + }; + match self.entries.entry(name.to_ascii_lowercase()) { + std::collections::hash_map::Entry::Vacant(slot) => { + slot.insert(template); + Registration::Added + } + std::collections::hash_map::Entry::Occupied(mut slot) => { + if slot.get().context.is_some() && context.is_none() { + slot.insert(template); + Registration::Replaced + } else { + Registration::Refused + } + } + } + } + + /// The template registered under `name`, case-folded (`0x6ee6f0`). + pub fn get(&self, name: &str) -> Option<&Element> { + self.entries + .get(&name.to_ascii_lowercase()) + .map(|t| &t.element) + } + + pub fn contains(&self, name: &str) -> bool { + self.entries.contains_key(&name.to_ascii_lowercase()) + } + + pub fn len(&self) -> usize { + self.entries.len() + } + + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } +} + +impl Inheritable for Templates { + fn find(&self, name: &str) -> Option<&Element> { + self.get(name) } } @@ -177,14 +280,7 @@ pub fn parse(text: &str) -> Result { } else { let el = element_from_node(child); if el.attr_bool("virtual") { - if el.name().is_none() { - warnings.push(format!( - "virtual <{}> without a name — a load error in the real client \ - (\"Unnamed virtual node\"); kept but unregistered, \ - so nothing can inherit it", - el.tag - )); - } + // An unnamed one is kept: the loader reports it where the reference does. items.push(TopLevel::Template(el)); } else { items.push(TopLevel::Instance(el)); @@ -321,7 +417,7 @@ fn element_from_node(node: roxmltree::Node) -> Element { /// registration. A cycle is skipped with a warning. pub fn expand( element: &Element, - templates: &HashMap<&str, &Element>, + templates: &dyn Inheritable, warnings: &mut Vec, ) -> Element { expand_known(element, templates, &HashSet::new(), warnings) @@ -331,7 +427,7 @@ pub fn expand( /// `` takes a font, applied later, not a template. pub fn expand_known( element: &Element, - templates: &HashMap<&str, &Element>, + templates: &dyn Inheritable, fonts: &HashSet<&str>, warnings: &mut Vec, ) -> Element { @@ -341,7 +437,7 @@ pub fn expand_known( fn expand_inner( element: &Element, - templates: &HashMap<&str, &Element>, + templates: &dyn Inheritable, fonts: &HashSet<&str>, warnings: &mut Vec, active: &mut HashSet, @@ -362,13 +458,7 @@ fn expand_inner( )); continue; } - let hit = templates.get(name).copied().or_else(|| { - templates - .iter() - .find(|(k, _)| k.eq_ignore_ascii_case(name)) - .map(|(_, v)| *v) - }); - let Some(template) = hit else { + let Some(template) = templates.find(name) else { // A font object is not an unknown template; it is applied after this. if !fonts.contains(name) { warnings.push(format!( @@ -608,8 +698,8 @@ print(x) "virtual attribute name/value compare is case-insensitive" ); let templates = doc.templates(); - assert!(templates.contains_key("MyTemplate")); - assert!(templates.contains_key("CasedTemplate")); + assert!(templates.contains("MyTemplate")); + assert!(templates.contains("CasedTemplate")); assert_eq!(templates.len(), 2); } @@ -617,11 +707,8 @@ print(x) fn unnamed_virtual_warns_but_does_not_fail() { let doc = parse(r#""#).unwrap(); assert!(matches!(doc.items[0], TopLevel::Template(_))); - assert_eq!(doc.warnings.len(), 1); - assert!( - doc.warnings[0].contains("Unnamed virtual node") - || doc.warnings[0].contains("without a name") - ); + // Reported by the loader into the document's record, not at parse. + assert!(doc.warnings.is_empty(), "{:?}", doc.warnings); assert!( doc.templates().is_empty(), "an unnamed template cannot be inherited" diff --git a/crates/benilla-ui/src/loader/mod.rs b/crates/benilla-ui/src/loader/mod.rs index 9feebc6d..f49e98c3 100644 --- a/crates/benilla-ui/src/loader/mod.rs +++ b/crates/benilla-ui/src/loader/mod.rs @@ -170,14 +170,8 @@ pub fn apply_template(lua: &mlua::Lua, wrapper: &Table, kind: &str, template: &s { let model = loader.model(); let templates = model.framexml_templates.borrow(); - // Looked up as `framexml::expand` looks it up (exact, then case-folded); keep them in step. for name in [template].into_iter().filter(|s| !s.is_empty()) { - let Some(el) = templates.get(name).or_else(|| { - templates - .iter() - .find(|(k, _)| k.eq_ignore_ascii_case(name)) - .map(|(_, v)| v) - }) else { + let Some(el) = templates.get(name) else { continue; }; any_resolved = true; @@ -412,16 +406,7 @@ impl Loader<'_> { } } TopLevel::Font(el) => self.do_font(el), - TopLevel::Template(el) => { - // Registered raw; `expand` resolves its own `inherits` at instantiation. An - // unnamed one was warned at parse time. - if let Some(name) = el.name() { - self.model() - .framexml_templates - .borrow_mut() - .insert(name.to_string(), el.clone()); - } - } + TopLevel::Template(el) => self.register_template(el), TopLevel::Instance(el) => { let expanded = self.expand(el); self.materialize(&expanded, None, framexml::DEFAULT_PARENT_NAME); @@ -430,6 +415,43 @@ impl Loader<'_> { } } + /// A top-level `virtual` element, registered raw (`0x6ee500`, called at `0x6ee1bd` under the + /// live context word): `expand` resolves its own `inherits` at instantiation. + fn register_template(&mut self, el: &Element) { + // No name, or an empty one (`0x6ee1a9` tests its first byte): `0x871020` at severity 1 + // into the document's record (`0x6ee1c4`-`0x6ee1cf`), and nothing registers. + let Some(name) = el.name().filter(|n| !n.is_empty()) else { + let line = "Unnamed virtual node at top level"; + self.report.status.report(status::WARNING, line); + self.report.warnings.push(line.to_string()); + return; + }; + let model = self.model(); + let context = model.exec_context.borrow().clone(); + let registered = + model + .framexml_templates + .borrow_mut() + .register(name, el.clone(), context.as_deref()); + drop(model); + match registered { + framexml::Registration::Refused => { + // `0x871188` over the refused spelling, at severity 1 into the document's record. + let line = format!("Virtual object named {name} already exists"); + self.report.status.report(status::WARNING, line.clone()); + self.report.warnings.push(line); + } + // Either way the entry now holds this node, traced at `0x6ee6dd` (`0x87116c`). + framexml::Registration::Added | framexml::Registration::Replaced => { + if self.debug() { + let trace = format!("-- Added virtual frame {name}"); + self.report.status.report(status::TRACE, trace.clone()); + self.report.traces.push(trace); + } + } + } + } + pub(super) fn do_include(&mut self, path: &str) { let joined = join_ref(self.base(), path); let Some(bytes) = (self.files)(&joined) else { @@ -480,17 +502,14 @@ impl Loader<'_> { pub(super) fn expand(&mut self, el: &Element) -> Element { let model = self.model(); let templates = model.framexml_templates.borrow(); - let view: HashMap<&str, &Element> = - templates.iter().map(|(k, v)| (k.as_str(), v)).collect(); // A font object in an inherit chain is skipped, not warned as an unknown template. let fonts = model.framexml_fonts.borrow(); let font_names: std::collections::HashSet<&str> = fonts.keys().map(|k| k.as_str()).collect(); let mut warns = Vec::new(); - let out = framexml::expand_known(el, &view, &font_names, &mut warns); + let out = framexml::expand_known(el, &*templates, &font_names, &mut warns); drop(font_names); drop(fonts); - drop(view); drop(templates); drop(model); self.report.warnings.extend(warns); @@ -502,11 +521,7 @@ impl Loader<'_> { pub(super) fn expand_region(&mut self, el: &Element) -> Element { // One verbatim name, matched case-insensitively, as `framexml::expand` looks it up. let hit = el.attr("inherits").is_some_and(|name| { - let model = self.model(); - let templates = model.framexml_templates.borrow(); - !name.is_empty() - && (templates.contains_key(name) - || templates.keys().any(|k| k.eq_ignore_ascii_case(name))) + !name.is_empty() && self.model().framexml_templates.borrow().contains(name) }); if hit { self.expand(el) @@ -924,4 +939,6 @@ pub(super) fn font_object_from_element(el: &Element) -> FontObject { #[cfg(test)] mod template_name_tests; #[cfg(test)] +mod template_registry_tests; +#[cfg(test)] mod tests; diff --git a/crates/benilla-ui/src/loader/template_registry_tests.rs b/crates/benilla-ui/src/loader/template_registry_tests.rs new file mode 100644 index 00000000..4e7a6f43 --- /dev/null +++ b/crates/benilla-ui/src/loader/template_registry_tests.rs @@ -0,0 +1,245 @@ +//! The template registry `0x6ee500`: a name registers once, matched case-insensitively +//! (`SStrHash 0x64b3f0`, `SStrCmpI 0x64a4c0`), and a second registration is refused with +//! `"Virtual object named %s already exists"` (`0x871188`, severity 1), unless the entry was +//! registered under a non-zero script-execution context and the new one comes under context 0 +//! (`0x6ee594`-`0x6ee5a2`), which replaces it. An addon loads under context 0 when secure and under +//! its own name otherwise (`AddOn_Load`, `0x51f37e`-`0x51f387`); the UI's own files load under the +//! resting context, 0. + +use crate::framexml; +use crate::loader::load; +use crate::script::{AddOnInfo, UiScript}; +use crate::status::LogWrite; + +/// A frame template `name` whose width is `width`, the probe of which definition is registered. +fn template(name: &str, width: u32) -> String { + format!( + r#" + + "# + ) +} + +fn script() -> UiScript { + let mut s = UiScript::new().unwrap(); + s.set_screen_size(1024.0, 768.0); + s +} + +/// Load `xml` as one of the UI's own documents, returning its load-status lines. +fn load_doc(s: &UiScript, xml: &str) -> Vec { + let report = load(s, &framexml::parse(xml).unwrap(), &|_| None); + assert!(report.errors.is_empty(), "{:?}", report.errors); + report.status.lines().to_vec() +} + +/// The width a frame built from `inherits` gets: which definition the lookup answers. +fn width_through(s: &UiScript, inherits: &str) -> f64 { + s.eval::(&format!( + "return CreateFrame('Frame', nil, nil, '{inherits}'):GetWidth()" + )) + .unwrap() +} + +const REFUSED: &str = "Virtual object named DupTemplate already exists"; + +#[test] +fn a_second_template_of_a_registered_name_is_refused_and_the_first_kept() { + let s = script(); + assert!(load_doc(&s, &template("DupTemplate", 10)).is_empty()); + let lines = load_doc(&s, &template("DupTemplate", 20)); + assert_eq!(lines, vec![REFUSED.to_string()]); + assert_eq!(width_through(&s, "DupTemplate"), 10.0); +} + +/// Within one document too: the registrar is called per top-level element (`0x6ee1bd`). +#[test] +fn a_repeat_within_one_document_is_refused_as_well() { + let s = script(); + let xml = r#" + + + "#; + assert_eq!(load_doc(&s, xml), vec![REFUSED.to_string()]); + assert_eq!(width_through(&s, "DupTemplate"), 10.0); +} + +/// The hash upper-cases and the compare folds case, so a name differing only in case is the same +/// name; the message names the refused spelling, the registrar's own argument (`0x6ee5a9`). +#[test] +fn a_name_differing_only_in_case_is_the_same_name() { + let s = script(); + load_doc(&s, &template("DupTemplate", 10)); + let lines = load_doc(&s, &template("DUPTEMPLATE", 20)); + assert_eq!( + lines, + vec!["Virtual object named DUPTEMPLATE already exists".to_string()] + ); + assert_eq!(width_through(&s, "duptemplate"), 10.0); + assert_eq!(width_through(&s, "DUPTEMPLATE"), 10.0); +} + +/// Severity 1 is above a trace, so the refused document's record opens under its banner in +/// `FrameXML.log` with `FrameXML_Debug` off (`0x6ee207`). +#[test] +fn the_refusal_puts_the_document_in_the_load_log() { + let s = script(); + load_doc(&s, &template("DupTemplate", 10)); + let report = load( + &s, + &framexml::parse(&template("DupTemplate", 20)).unwrap(), + &|_| None, + ); + let mut parent = crate::status::Status::default(); + report + .status + .close_into(&mut parent, false, "++ Loading file X".into()); + assert_eq!(parent.lines(), ["++ Loading file X", REFUSED]); +} + +/// `"-- Added virtual frame %s"` (`0x87116c`) at severity 0, only while `FrameXML_Debug` is on +/// (`0x6ee6c6`), and not for a refused one, which returns before it (`0x6ee5be`). +#[test] +fn a_registration_is_traced_under_framexml_debug() { + let s = script(); + assert!(load_doc(&s, &template("QuietTemplate", 10)).is_empty()); + s.run("FrameXML_Debug(1)").unwrap(); + let lines = load_doc(&s, &template("TracedTemplate", 10)); + assert_eq!( + lines, + vec!["-- Added virtual frame TracedTemplate".to_string()] + ); + let lines = load_doc(&s, &template("TracedTemplate", 20)); + assert_eq!( + lines, + vec!["Virtual object named TracedTemplate already exists".to_string()] + ); +} + +/// Two demand-loaded chain addons, each one XML file declaring `DupTemplate` at its own width; +/// `secure` is each one's `GetAddOnInfo` security. +fn with_addons(rows: &[(&'static str, bool, u32)]) -> UiScript { + let mut s = script(); + let files: Vec<(String, String)> = rows + .iter() + .map(|&(name, _, width)| { + ( + format!("Interface/AddOns/{name}/{name}.xml"), + template("DupTemplate", width), + ) + }) + .collect(); + s.set_addon_chain_reader(Box::new(move |path| { + files + .iter() + .find(|(p, _)| p == path) + .map(|(_, xml)| xml.clone().into_bytes()) + })); + s.register_addons( + rows.iter() + .map(|&(name, secure, _)| AddOnInfo { + name: name.into(), + interface: 11200, + load_on_demand: true, + chain: true, + secure, + enabled: true, + files: vec![format!("{name}.xml")], + ..Default::default() + }) + .collect(), + None, + None, + None, + ); + s.note_addon_info_reply(&[]); + s +} + +fn load_addon(s: &mut UiScript, name: &str) -> Vec { + assert_eq!( + s.eval::>(&format!("return LoadAddOn('{name}')")) + .unwrap(), + Some(1) + ); + s.take_load_log_writes() + .into_iter() + .flat_map(|w| match w { + LogWrite::Append(lines) | LogWrite::Rewrite(lines) => lines, + }) + .collect() +} + +fn refused_in(lines: &[String]) -> bool { + lines.iter().any(|l| l == REFUSED) +} + +/// The UI's own templates register under context 0, which nothing replaces. +#[test] +fn an_addon_cannot_replace_a_template_of_the_ui_s_own() { + let mut s = with_addons(&[("Probe", false, 20), ("Blizzard_Probe", true, 30)]); + load_doc(&s, &template("DupTemplate", 10)); + assert!(refused_in(&load_addon(&mut s, "Probe"))); + assert!(refused_in(&load_addon(&mut s, "Blizzard_Probe"))); + assert_eq!(width_through(&s, "DupTemplate"), 10.0); +} + +/// Two insecure addons: both contexts are non-zero, so the second is refused. +#[test] +fn of_two_insecure_addons_the_first_keeps_the_name() { + let mut s = with_addons(&[("ProbeA", false, 10), ("ProbeB", false, 20)]); + assert!(!refused_in(&load_addon(&mut s, "ProbeA"))); + assert!(refused_in(&load_addon(&mut s, "ProbeB"))); + assert_eq!(width_through(&s, "DupTemplate"), 10.0); +} + +/// The one replacing case: an entry from a non-zero context, a registration under context 0. +#[test] +fn a_secure_addon_replaces_an_insecure_addons_template() { + let mut s = with_addons(&[("Probe", false, 10), ("Blizzard_Probe", true, 20)]); + assert!(!refused_in(&load_addon(&mut s, "Probe"))); + assert!(!refused_in(&load_addon(&mut s, "Blizzard_Probe"))); + assert_eq!(width_through(&s, "DupTemplate"), 20.0); +} + +/// And never the reverse: a secure addon's entry is context 0. +#[test] +fn an_insecure_addon_cannot_replace_a_secure_addons_template() { + let mut s = with_addons(&[("Blizzard_Probe", true, 10), ("Probe", false, 20)]); + assert!(!refused_in(&load_addon(&mut s, "Blizzard_Probe"))); + assert!(refused_in(&load_addon(&mut s, "Probe"))); + assert_eq!(width_through(&s, "DupTemplate"), 10.0); +} + +/// The context is the loading addon's, set and restored around its load (`0x51f36d`, `0x51f584`): +/// a document loaded after an insecure addon is back at context 0, so it replaces that addon's. +#[test] +fn the_context_is_restored_after_an_addon_loads() { + let mut s = with_addons(&[("Probe", false, 10)]); + load_addon(&mut s, "Probe"); + let lines = load_doc(&s, &template("DupTemplate", 20)); + assert!(lines.is_empty(), "{lines:?}"); + assert_eq!(width_through(&s, "DupTemplate"), 20.0); +} + +/// A top-level `virtual` element with no name, or an empty one (the first-byte test at +/// `0x6ee1a9`), is reported as `"Unnamed virtual node at top level"` (`0x871020`) at severity 1 +/// into the document's record (`0x6ee1c4`-`0x6ee1cf`), in document order, and registers nothing. +#[test] +fn an_unnamed_virtual_node_is_reported_into_the_load_log() { + let s = script(); + let xml = r#" + + + + "#; + let report = load(&s, &framexml::parse(xml).unwrap(), &|_| None); + let unnamed = "Unnamed virtual node at top level"; + assert_eq!(report.status.lines(), [unnamed, unnamed]); + assert_eq!(report.warnings, [unnamed, unnamed]); + let mut parent = crate::status::Status::default(); + report + .status + .close_into(&mut parent, false, "++ Loading file X".into()); + assert_eq!(parent.lines(), ["++ Loading file X", unnamed, unnamed]); +} diff --git a/crates/benilla-ui/src/script/addon.rs b/crates/benilla-ui/src/script/addon.rs index ca5d9024..8de48227 100644 --- a/crates/benilla-ui/src/script/addon.rs +++ b/crates/benilla-ui/src/script/addon.rs @@ -28,7 +28,9 @@ pub struct AddOnInfo { pub title: Option, pub notes: Option, pub url: Option, - /// `## Secure: 1`, which Blizzard's own addons carry; drives the `security` token. + /// The security state SECURE (`[rec+0x24] == 0`), which the reference sets only for a valid + /// `%s.sig` (`0x6f10f0`, `0x51dbfb`); the host stands in for that check. Drives the `security` + /// token and the context the addon loads under ([`addon_load_context`]). pub secure: bool, pub load_on_demand: bool, /// `## Dependencies` / `## RequiredDeps`. @@ -417,7 +419,7 @@ pub(super) fn install(lua: &Lua) -> mlua::Result<()> { /// `AddOn_Load` (`0x51f240`), dependencies first; `Err` is the reason token. Synchronous inside /// the Lua call, as stock callers use the addon on the next line (`UIParent.lua:577`). fn load_addon(lua: &Lua, i: usize) -> Result<(), String> { - let (name, root, files, deps, already, chain, chain_reader) = { + let (name, root, files, deps, already, chain, secure, chain_reader) = { let model = lua.app_data_ref::().expect("model"); let a = &model.addons[i]; ( @@ -427,6 +429,7 @@ fn load_addon(lua: &Lua, i: usize) -> Result<(), String> { a.dependencies.clone(), a.loaded, a.chain, + a.secure, model.addons_chain_reader.clone(), ) }; @@ -494,6 +497,8 @@ fn load_addon(lua: &Lua, i: usize) -> Result<(), String> { } } + // The files, `Bindings.xml` and the saved variables run under the addon's context. + let outer = swap_exec_context(lua, addon_load_context(&name, secure)); let mut toc = crate::status::Status::default(); run_files(lua, &name, &read, &files, &mut toc); // The toc's record under its banner, then the add-on's (`0x6eddc8`, `0x51f464`). @@ -509,6 +514,7 @@ fn load_addon(lua: &Lua, i: usize) -> Result<(), String> { // `Bindings.xml` loads between the files and the saved variables. load_bindings(lua, &name, &read); load_saved_variables(lua, i); + swap_exec_context(lua, outer); // `ADDON_LOADED` closes the addon's load (`0x51f5ad`). super::event::fire_global(lua, "ADDON_LOADED", &[super::ScriptValue::Str(name)]); Ok(()) @@ -701,8 +707,30 @@ fn log_error(lua: &Lua, msg: &str, handler: &str) { model.errors.push(msg); } +/// The script-execution context an addon's load runs under: `AddOn_Load` sets `[0xceeac0]` to the +/// record's name (`[rec+0x14]`) when its security state `[rec+0x24]` is non-zero, else to 0 +/// (`0x51f37e`-`0x51f387`), so only a secure addon loads under the UI's own context. `None` is 0. +pub fn addon_load_context(name: &str, secure: bool) -> Option { + (!secure).then(|| name.to_owned()) +} + +/// Set the context word, returning the one it replaces for the caller to restore, as `AddOn_Load` +/// saves it (`0x51f36d`) and restores it before `ADDON_LOADED` (`0x51f584`). +fn swap_exec_context(lua: &Lua, context: Option) -> Option { + lua.app_data_ref::() + .expect("model") + .exec_context + .replace(context) +} + /// The host's side: the registry, where its files live, and what to write back. impl super::UiScript { + /// Set the script-execution context for the startup walk's addon loads + /// ([`addon_load_context`]), returning the previous one to restore after. + pub fn swap_exec_context(&self, context: Option) -> Option { + swap_exec_context(self.lua(), context) + } + /// Seat the reader chain-sourced addons (`AddOnInfo::chain`) are read through. pub fn set_addon_chain_reader(&self, reader: AddonChainReader) { self.model_mut().addons_chain_reader = Some(std::rc::Rc::from(reader)); diff --git a/crates/benilla-ui/src/script/mod.rs b/crates/benilla-ui/src/script/mod.rs index 7d68dca4..e76f5934 100644 --- a/crates/benilla-ui/src/script/mod.rs +++ b/crates/benilla-ui/src/script/mod.rs @@ -153,7 +153,7 @@ mod worn_display; mod zone_text; pub use action::{ActionSlot, ActionState, ActionUse}; -pub use addon::AddOnInfo; +pub use addon::{addon_load_context, AddOnInfo}; pub use addon_enable::EnableHash; pub use addon_message::{AddonDistribution, AddonSend}; pub use auction::{ @@ -1131,9 +1131,7 @@ impl UiScript { /// Whether `name` is a registered FrameXML template, case-folded as the loader resolves it, for /// the corpus harness: an addon naming a missing template gets a bare frame and no load error. pub fn has_framexml_template(&self, name: &str) -> bool { - let model = self.model_ref(); - let templates = model.framexml_templates.borrow(); - templates.contains_key(name) || templates.keys().any(|k| k.eq_ignore_ascii_case(name)) + self.model_ref().framexml_templates.borrow().contains(name) } /// Whether `name` is a registered font object, the other namespace an `inherits=` may name diff --git a/crates/benilla-ui/src/script/model.rs b/crates/benilla-ui/src/script/model.rs index a5eb6f74..fd865f95 100644 --- a/crates/benilla-ui/src/script/model.rs +++ b/crates/benilla-ui/src/script/model.rs @@ -97,8 +97,11 @@ pub(crate) struct Model { pub(crate) addons_saved_character: Option, /// FrameXML templates, global across files as the client's (`0x6ee500`); held here so the /// loader runs from a bare `&Lua`, which lets `LoadAddOn` load inside a binding. - pub(crate) framexml_templates: - std::cell::RefCell>, + pub(crate) framexml_templates: std::cell::RefCell, + /// The script-execution context word `[0xceeac0]`, as far as template registration reads it + /// (`0x6ee1ae`): `None` is its resting 0, under which the UI's own files load; an insecure + /// addon's load sets its name ([`crate::script::addon_load_context`]). + pub(crate) exec_context: std::cell::RefCell>, /// `FrameXML_Debug`'s flag (`0x488440` over `[0xceea30]`, boots 0); the loader's trace lines /// print while it is greater than 0 (`0x6ee298`), so it is signed. pub(crate) framexml_debug: std::cell::Cell, @@ -1090,6 +1093,7 @@ impl Model { addons_saved_account: None, addons_saved_character: None, framexml_templates: Default::default(), + exec_context: Default::default(), framexml_debug: Default::default(), load_log: Default::default(), framexml_fonts: Default::default(), diff --git a/crates/benilla-ui/src/script/object.rs b/crates/benilla-ui/src/script/object.rs index 88f9960c..e50418ed 100644 --- a/crates/benilla-ui/src/script/object.rs +++ b/crates/benilla-ui/src/script/object.rs @@ -554,12 +554,13 @@ pub(super) fn create_frame( // leaves no partial widget and no global. The raise never returns and `pcall` catches it, as // the widget dispatcher (`0x704f10`) does for every handler. if let Some(t) = template.as_deref().filter(|t| !t.is_empty()) { - let known = { - let model = lua.app_data_ref::().expect("model app_data"); - let templates = model.framexml_templates.borrow(); - // One name, verbatim, case-folded, as `framexml::expand` resolves it. - templates.contains_key(t) || templates.keys().any(|k| k.eq_ignore_ascii_case(t)) - }; + // One name, verbatim, case-folded, as `framexml::expand` resolves it. + let known = lua + .app_data_ref::() + .expect("model app_data") + .framexml_templates + .borrow() + .contains(t); if !known { return Err(mlua::Error::runtime(format!( "CreateFrame(): Couldn't find inherited node \"{t}\"" diff --git a/crates/benilla-ui/src/script/object/events_regions.rs b/crates/benilla-ui/src/script/object/events_regions.rs index 37849150..90b809db 100644 --- a/crates/benilla-ui/src/script/object/events_regions.rs +++ b/crates/benilla-ui/src/script/object/events_regions.rs @@ -296,11 +296,12 @@ fn apply_region_inherits(lua: &Lua, wrapper: &Table, from: &str) -> mlua::Result if wrapper.call_method::<()>("SetFontObject", from).is_ok() { return Ok(()); } - let is_template = { - let model = lua.app_data_ref::().expect("model"); - let templates = model.framexml_templates.borrow(); - templates.contains_key(from) || templates.keys().any(|k| k.eq_ignore_ascii_case(from)) - }; + let is_template = lua + .app_data_ref::() + .expect("model") + .framexml_templates + .borrow() + .contains(from); if is_template { let mut model = lua.app_data_mut::().expect("model"); model.record_warning(format!( diff --git a/crates/benilla-ui/src/status.rs b/crates/benilla-ui/src/status.rs index 8e27bce8..cfc6228e 100644 --- a/crates/benilla-ui/src/status.rs +++ b/crates/benilla-ui/src/status.rs @@ -5,6 +5,9 @@ /// A trace, printed only under `FrameXML_Debug` (`"-- Creating %s named %s"`, `0x6ee2bc`). pub const TRACE: u8 = 0; +/// An error the load goes on past: an unnamed template (`0x6ee1cc`), a template name already +/// registered (`0x6ee5af`). +pub const WARNING: u8 = 1; /// A file-level failure: a file that did not open or did not parse (`0x6edad4`, `0x704c15`). pub const FAILURE: u8 = 2;