From e16450d4e25ac6c8deebbc8a95b4ab0f8567da5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= <55043719+hnoerenberg@users.noreply.github.com> Date: Tue, 3 Nov 2026 15:22:58 +0100 Subject: [PATCH] fix(plugins): the install error names the format the file actually is (#10362) (#10633) * fix(plugins): the install error names the format the file actually is (#10362) Plug-ins > Install Plug-in pointed at an old Photoshop plug-in (.8bt) answered with "not a valid plug-in module: not a WebAssembly binary (missing \0asm header)". That was one fixed message for every file that is not wasm: it said what we wanted and nothing about the file the person is holding. format_hint in runtime.rs now reads the magic number and the error says what the file looks like. For a Windows module the message is (one line in the code, wrapped here): not a valid plug-in module: not a WebAssembly binary (missing \0asm header); this file looks like a Windows native executable or library (a .dll, or a Photoshop plug-in such as .8bf/.8bt) - PhotoCraft plug-ins are WebAssembly modules (.wasm) 7F 45 4C 46 says "a Linux native library (ELF)", CE FA ED FE / CF FA ED FE / CA FE BA BE / BE BA FE CA say "a macOS native binary (Mach-O, thin or fat)", 50 4B 03 04 / 50 4B 05 06 say "a zip archive". Any other header keeps today's message byte for byte, so nothing keyed on it changes. The signatures are public format facts, read with starts_with/get so a short or hostile file cannot panic. The error variant, the enum, the ABI check, check_code_section and the wasmi setup are untouched, and native-code loading stays out of scope (PR #7907). Fail-first, before the production change: cargo test -p photocraft-plugins --lib --tests lib: test result: ok. 10 passed; 0 failed abi: test result: FAILED. 19 passed; 1 failed; 0 ignored; 0 measured assertion line: "windows dll or .8bt: not a valid plug-in module: not a WebAssembly binary (missing \0asm header)" (the MZ fixture that was already in the file) cargo test -p photocraft-engine --lib -- plugin test result: FAILED. 7 passed; 1 failed; 0 ignored; 0 measured; 2518 filtered out After: cargo test -p photocraft-plugins lib 10 passed; abi 20 passed; doc-tests 0 cargo test -p photocraft-engine --lib -- plugin test result: ok. 8 passed; 0 failed; 2518 filtered out The new engine test drives the real plugin.install command with a `path` whose file starts with MZ bytes, so it covers the menu path (registry::load_file) end to end, not only the library check. Gates: cargo clippy -p photocraft-plugins --all-targets -- -D warnings and cargo clippy -p photocraft-engine --all-targets -- -D warnings (clean; the only stderr noise is the pre-existing "patch `egui_*` was not used in the crate graph" cargo warnings); cargo xtask layers ("OK: 31 crates, no layering violations."); cargo xtask wasm (" ok 24 crates", " ok photocraft- codecs --features heif", no FAIL line); cargo fmt -p photocraft-plugins -- --check and cargo fmt -p photocraft-engine -- --check (silent, exit 0). The whole engine --lib suite is not run here: it fails on clean main in this checkout for font/i18n reasons (no CRAFT_FONTS_DIR). No doc quoted the old message (git grep "WebAssembly binary" finds only this crate), so nothing to update there. Co-Authored-By: Claude Opus 5.5 * test(plugins): one install-error test per recognised format, tempfile in the engine test (#10362) Review feedback on #10633. The single looped test in abi.rs is now one test per signature (PE/COFF MZ, ELF, Mach-O in all four byte orders, zip), one for an empty or unrecognised file (including signatures cut short, which keep the old message byte for byte) and one showing a valid WebAssembly module still loads. The engine test uses tempfile::tempdir (already a dependency of photocraft-engine) so the directory is removed on a panic, and asserts the message does not contain the temp path: the hint is static text, so the message adds no path. Gates: cargo test -p photocraft-plugins --lib --tests (10 lib, 26 tests); cargo test -p photocraft-engine --lib -- plugin (8 passed); clippy -D warnings on both crates; cargo fmt --all -- --check; cargo xtask layers (OK). Co-Authored-By: Claude Opus 5.5 * test(engine): keep the non-wasm install-error test off wasm32, as tempfile is (#10362) tempfile is a non-wasm32 dependency of photocraft-engine, so the test that uses it is gated like the crate's other file-system tests. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- crates/engine/src/cmd/plugin_cmds/tests.rs | 17 +++++++ crates/plugins/src/runtime.rs | 26 +++++++++- crates/plugins/tests/abi.rs | 58 ++++++++++++++++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/plugin_cmds/tests.rs b/crates/engine/src/cmd/plugin_cmds/tests.rs index 5f93ce7909..44b129bc5c 100644 --- a/crates/engine/src/cmd/plugin_cmds/tests.rs +++ b/crates/engine/src/cmd/plugin_cmds/tests.rs @@ -133,6 +133,23 @@ fn example_invert_matches_builtin_invert_and_round_trips_the_registry() { std::fs::remove_dir_all(&dir).ok(); } +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn installing_a_file_that_is_not_wasm_names_what_it_is() { + // #10362: the Plug-ins > Install Plug-in dialog pointed at an old Photoshop plug-in (.8bt, a + // native Windows module) failed with a message that said nothing about that file. `path` goes + // through `registry::load_file`, so this is the exact user path. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("Old Plug-in.8bt"); + std::fs::write(&path, b"MZ\x90\x00\x03\0").unwrap(); + let e = Session::new().execute("plugin.install", json!({"path": path.to_str().unwrap()})).unwrap_err(); + let msg = e.to_string(); + assert!(msg.contains("Windows") && msg.contains("8bf"), "{msg}"); + assert!(msg.contains("WebAssembly"), "{msg}"); + // The hint is static text: no path the person did not choose is added to the message. + assert!(!msg.contains(dir.path().to_str().unwrap()), "{msg}"); +} + #[test] fn params_are_validated_and_passed() { let bytes = wat_plugin("test.cmds.grey", r#"{"amount":{"type":"number","min":0,"max":1,"default":1}}"#, GREY); diff --git a/crates/plugins/src/runtime.rs b/crates/plugins/src/runtime.rs index c3dd8d9ccc..af3b3b8655 100644 --- a/crates/plugins/src/runtime.rs +++ b/crates/plugins/src/runtime.rs @@ -167,6 +167,26 @@ fn check_code_section(bytes: &[u8]) -> Result<()> { Ok(()) } +/// What a file that is not WebAssembly actually is, judged from its magic number, so the install +/// error can name the format the person picked instead of only the one we wanted (public format +/// facts: the PE/COFF, ELF, Mach-O and zip signatures). `None` for anything else. Reads with +/// `starts_with`/`get`, so a file shorter than the signature is fine. +fn format_hint(bytes: &[u8]) -> Option<&'static str> { + if bytes.starts_with(b"MZ") { + return Some("a Windows native executable or library (a .dll, or a Photoshop plug-in such as .8bf/.8bt)"); + } + if bytes.get(..4) == Some(&[0x7f, b'E', b'L', b'F']) { + return Some("a Linux native library (ELF)"); + } + if matches!(bytes.get(..4), Some(&[0xce, 0xfa, 0xed, 0xfe] | &[0xcf, 0xfa, 0xed, 0xfe] | &[0xca, 0xfe, 0xba, 0xbe] | &[0xbe, 0xba, 0xfe, 0xca])) { + return Some("a macOS native binary (Mach-O, thin or fat)"); + } + if matches!(bytes.get(..4), Some(&[0x50, 0x4b, 0x03, 0x04] | &[0x50, 0x4b, 0x05, 0x06])) { + return Some("a zip archive"); + } + None +} + impl Plugin { /// Validates and loads a module: checks the ABI version and reads the manifest. pub fn load(bytes: &[u8], limits: Limits) -> Result { @@ -174,7 +194,11 @@ impl Plugin { return Err(Error::Module(format!("module is {} bytes (limit {})", bytes.len(), limits.max_module_bytes))); } if !bytes.starts_with(b"\0asm") { - return Err(Error::Module("not a WebAssembly binary (missing \\0asm header)".into())); + let msg = "not a WebAssembly binary (missing \\0asm header)"; + return Err(Error::Module(match format_hint(bytes) { + Some(hint) => format!("{msg}; this file looks like {hint} - PhotoCraft plug-ins are WebAssembly modules (.wasm)"), + None => msg.to_string(), + })); } check_code_section(bytes)?; let mut config = Config::default(); diff --git a/crates/plugins/tests/abi.rs b/crates/plugins/tests/abi.rs index bb06069ac8..113d401dcf 100644 --- a/crates/plugins/tests/abi.rs +++ b/crates/plugins/tests/abi.rs @@ -219,6 +219,64 @@ fn malformed_and_oversized_modules_are_refused() { assert!(matches!(e, Error::Abi(_)), "{e}"); } +/// The message `Plugin::load` gives a file that is not WebAssembly and not recognised (#10362). +const NOT_WASM: &str = "not a valid plug-in module: not a WebAssembly binary (missing \\0asm header)"; + +/// Loads `bytes`, which must be refused as a module, and returns the message. +fn refusal(bytes: &[u8]) -> String { + let Err(e) = Plugin::load(bytes, Limits::default()) else { panic!("{bytes:?} was accepted") }; + assert!(matches!(e, Error::Module(_)), "{e}"); + e.to_string() +} + +/// The message names the format (`format`), says plug-ins are WebAssembly, and stays on one line +/// (`Plugin::load` has only bytes, so the hint cannot carry a path). +fn assert_names(bytes: &[u8], format: &str) { + let m = refusal(bytes); + assert!(m.starts_with(NOT_WASM), "{m}"); + assert!(m.contains(format), "{m}"); + assert!(m.contains("PhotoCraft plug-ins are WebAssembly modules"), "{m}"); + assert!(!m.contains('\n'), "{m}"); +} + +#[test] +fn a_pe_coff_file_is_named_as_a_windows_binary() { + // #10362: an old Photoshop plug-in (.8bt) is a native Windows module. + assert_names(b"MZ\x90\x00\x03\0", "Windows"); + assert!(refusal(b"MZ").contains("8bf")); +} + +#[test] +fn an_elf_file_is_named_as_a_linux_library() { + assert_names(&[0x7f, b'E', b'L', b'F', 2, 1, 1], "Linux"); +} + +#[test] +fn a_mach_o_file_is_named_as_a_macos_binary() { + for head in [[0xce, 0xfa, 0xed, 0xfe], [0xcf, 0xfa, 0xed, 0xfe], [0xca, 0xfe, 0xba, 0xbe], [0xbe, 0xba, 0xfe, 0xca]] { + assert_names(&[&head[..], &[7, 1][..]].concat(), "macOS"); + } +} + +#[test] +fn a_zip_file_is_named_as_a_zip_archive() { + assert_names(b"PK\x03\x04\x14\0", "zip"); + assert_names(b"PK\x05\x06\0\0", "zip"); +} + +#[test] +fn an_empty_or_unrecognised_file_keeps_the_plain_message() { + // Nothing recognised (including a signature cut short): today's message, exactly. + for bytes in [&b""[..], &b"x"[..], &b"junkjunk"[..], &[0x7f, b'E'][..], &b"PK"[..]] { + assert_eq!(refusal(bytes), NOT_WASM, "{bytes:?}"); + } +} + +#[test] +fn a_valid_wasm_module_still_loads_unchanged() { + assert!(Plugin::load(INVERT, Limits::default()).is_ok()); +} + #[test] fn a_code_section_claiming_no_functions_but_many_bytes_is_refused() { // Header, then a code section (id 10) declaring 2000 bytes whose function count is 0, cut off