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