diff --git a/.changes/unreleased/fixed-20261015-083325.yaml b/.changes/unreleased/fixed-20261015-083325.yaml new file mode 100644 index 000000000..487525259 --- /dev/null +++ b/.changes/unreleased/fixed-20261015-083325.yaml @@ -0,0 +1,3 @@ +kind: Fixed +body: '**`apply_companions` writes each companion name into the stitcher as an escaped string literal, and rejects a name with `/` or `\` in release builds too** (#718, closes #708). The name went into `include!("…")` unescaped, so a name with a `"` gave a stitcher that did not compile or named a different file, and the only check on the name was a `debug_assert!`, which the release build of a code generator skips. The `include!` is now written with an escaped literal, as at buffa''s own include sites. A name that contains `/` or `\`, or that is empty, `.` or `..`, panics in every build, before any output is changed; a generator that used such a name in a release build should switch to a bare name such as `.__myplugin.rs`.' +time: 2026-10-15T08:33:25.768858+02:00 diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index c96285b7f..6beaf7ac4 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -5393,32 +5393,48 @@ pub fn proto_path_to_stem(proto_path: &str) -> String { /// /// `name` must be a bare-sibling filename — the same convention buffa uses /// for its own `include!` calls, so it resolves relative to the stitcher -/// without any `OUT_DIR` prefix. Names must not contain `"`, `\`, `/`, or -/// newlines (the function `debug_assert!`s this in debug builds), and must -/// not collide with any of buffa's own generated filenames for the same -/// package (`.rs`, `.__view.rs`, etc.) — pick an unused suffix -/// such as `.__myplugin.rs`. +/// without any `OUT_DIR` prefix. Names must not contain `/` or `\`, must not +/// be empty, `.` or `..`, and must not collide with any of buffa's own +/// generated filenames for the same package (`.rs`, `.__view.rs`, +/// etc.) — pick an unused suffix such as `.__myplugin.rs`. The name is +/// written into the `include!` as an escaped string literal, as buffa's own +/// include paths are. /// /// Companion files with no matching `PackageMod` (e.g. for a package buffa /// did not generate any output for) are still appended to `files` but no /// `include!` is emitted; the caller is responsible for wiring them up. If /// you don't expect orphans, check that every companion's `package` appears /// in `files` as a `PackageMod` after calling. +/// +/// # Panics +/// +/// Panics if a companion's `name` contains `/` or `\`, or is empty, `.` or +/// `..`. Every name is checked before `files` is changed. +#[track_caller] pub fn apply_companions(files: &mut Vec, companions: Vec) { for comp in &companions { - debug_assert!( - !comp.name.contains(['"', '\\', '/', '\n']), - "companion file name {:?} contains a character that would break \ - the generated include!() literal or its bare-sibling resolution", + assert!( + !comp.name.contains(['/', '\\']), + "companion file name {:?} contains a path separator ('/' or '\\'); use a \ + bare file name such as `.__myplugin.rs`, which include!() resolves \ + next to the stitcher", comp.name ); + assert!( + !matches!(comp.name.as_str(), "" | "." | ".."), + "companion file name {:?} is empty, `.` or `..`, which names a directory, \ + not a file; use a bare file name such as `.__myplugin.rs`", + comp.name + ); + } + for comp in &companions { if let Some(pkg_mod) = files .iter_mut() .find(|f| f.kind == GeneratedFileKind::PackageMod && f.package == comp.package) { pkg_mod .content - .push_str(&format!("include!(\"{}\");\n", comp.name)); + .push_str(&format!("include!({:?});\n", comp.name)); } } files.extend(companions); diff --git a/buffa-codegen/src/tests/generation.rs b/buffa-codegen/src/tests/generation.rs index c54456829..91a969a43 100644 --- a/buffa-codegen/src/tests/generation.rs +++ b/buffa-codegen/src/tests/generation.rs @@ -2898,14 +2898,130 @@ fn apply_companions_empty_is_noop() { } #[test] -#[cfg(debug_assertions)] -#[should_panic(expected = "contains a character that would break")] -fn apply_companions_rejects_unsafe_name() { +fn apply_companions_escapes_name_in_include() { + // The include! must name exactly this file, as buffa's own include sites + // do: the quote must not end the string literal early, and the control + // and bidi characters must be escaped. + let mut file = proto3_file("svc/echo.proto"); + file.package = Some("svc".to_string()); + let mut files = generate( + &[file], + &["svc/echo.proto".to_string()], + &CodeGenConfig::default(), + ) + .expect("generate ok"); + + let name = "svc.we\"ird'\n\r\t\u{301}\u{202e}.__service.rs"; + let companion = GeneratedFile { + name: name.to_string(), + package: "svc".to_string(), + kind: GeneratedFileKind::Companion, + content: "pub struct EchoService;".to_string(), + }; + let stitcher = |files: &[GeneratedFile]| { + files + .iter() + .find(|f| f.kind == GeneratedFileKind::PackageMod) + .expect("stitcher present") + .content + .clone() + }; + let before = stitcher(&files); + apply_companions(&mut files, vec![companion]); + let after = stitcher(&files); + + let parsed = syn::parse_file(&after).expect("stitcher still parses"); + let included: Vec = parsed + .items + .iter() + .filter_map(|item| match item { + syn::Item::Macro(m) if m.mac.path.is_ident("include") => m + .mac + .parse_body::() + .ok() + .map(|lit| lit.value()), + _ => None, + }) + .collect(); + assert!( + included.iter().any(|path| path == name), + "no include! names {name:?}; included: {included:?}" + ); + // A raw newline, tab or bidi character parses to the same value as its + // escape (a raw carriage return does not lex at all), so the appended + // text is checked as well: it is the escaped literal, and every control + // and non-ASCII character of the name is escaped in it. + let appended = after + .strip_prefix(before.as_str()) + .expect("apply_companions only appends to the stitcher"); + assert_eq!(appended, format!("include!({name:?});\n")); + let raw: Vec = appended + .trim_end_matches('\n') + .chars() + .filter(|c| !c.is_ascii() || c.is_ascii_control()) + .collect(); + assert!(raw.is_empty(), "unescaped in the include line: {raw:?}"); +} + +#[test] +fn apply_companions_rejects_names_that_are_not_files() { + // A good companion comes first: its include! must not be written before + // the panic for the bad name. + for bad in ["", ".", ".."] { + let mut file = proto3_file("svc/echo.proto"); + file.package = Some("svc".to_string()); + let mut files = generate( + &[file], + &["svc/echo.proto".to_string()], + &CodeGenConfig::default(), + ) + .expect("generate ok"); + let before: Vec = files.iter().map(|f| f.content.clone()).collect(); + + let companion = |name: &str| GeneratedFile { + name: name.to_string(), + package: "svc".to_string(), + kind: GeneratedFileKind::Companion, + content: String::new(), + }; + let companions = vec![companion("svc.echo.__good.rs"), companion(bad)]; + let payload = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + apply_companions(&mut files, companions) + })) + .expect_err("must panic"); + + let message = payload + .downcast_ref::() + .expect("the assert! message is a String"); + assert!(message.contains("names a directory"), "{bad:?}: {message}"); + let after: Vec = files.iter().map(|f| f.content.clone()).collect(); + assert_eq!(before, after, "{bad:?}: files changed before the panic"); + } +} + +#[test] +#[should_panic(expected = "contains a path separator")] +fn apply_companions_rejects_name_with_slash() { + let mut files: Vec = vec![]; + apply_companions( + &mut files, + vec![GeneratedFile { + name: "sub/bad.rs".to_string(), + package: "svc".to_string(), + kind: GeneratedFileKind::Companion, + content: String::new(), + }], + ); +} + +#[test] +#[should_panic(expected = "contains a path separator")] +fn apply_companions_rejects_name_with_backslash() { let mut files: Vec = vec![]; apply_companions( &mut files, vec![GeneratedFile { - name: r#"bad".rs"#.to_string(), + name: r"sub\bad.rs".to_string(), package: "svc".to_string(), kind: GeneratedFileKind::Companion, content: String::new(), @@ -2913,6 +3029,36 @@ fn apply_companions_rejects_unsafe_name() { ); } +#[test] +fn apply_companions_checks_every_name_before_changing_files() { + // The bad name is second: the first companion's include! must not be + // written before the panic. + let mut file = proto3_file("svc/echo.proto"); + file.package = Some("svc".to_string()); + let mut files = generate( + &[file], + &["svc/echo.proto".to_string()], + &CodeGenConfig::default(), + ) + .expect("generate ok"); + let before: Vec = files.iter().map(|f| f.content.clone()).collect(); + + let companion = |name: &str| GeneratedFile { + name: name.to_string(), + package: "svc".to_string(), + kind: GeneratedFileKind::Companion, + content: String::new(), + }; + let companions = vec![companion("svc.echo.__good.rs"), companion("sub/bad.rs")]; + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + apply_companions(&mut files, companions) + })); + + assert!(result.is_err(), "a name with '/' must panic"); + let after: Vec = files.iter().map(|f| f.content.clone()).collect(); + assert_eq!(before, after, "files changed before the panic"); +} + #[test] fn apply_companions_file_per_package() { let mut file = proto3_file("svc/msg.proto");