From 6b9a4819feaa366199607aea57080b423d1bf698 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 19 Oct 2026 13:43:28 -0700 Subject: [PATCH 1/9] Replace serde_yaml with serde-saphyr to drop unsafe-libyaml unsafe-libyaml is unmaintained (RUSTSEC-2024-0449) and reaches us only through serde_yaml, which is itself deprecated. serde-saphyr is a maintained serde deserializer written in safe Rust, and it is the serde option the advisory suggests. All YAML parsing now goes through src/yaml.rs. Callers read the whole file and pass the bytes to crate::yaml::from_slice, which calls from_slice_with_options with options that keep serde_yaml's semantics where serde-saphyr allows it: a repeated key keeps its last value instead of failing the file, `<<` stays an ordinary key, non-finite floats and `!!binary` are not interpreted, only true and false are booleans, and errors stay on one line. Resource budgets stay at serde-saphyr's defaults. Two deserialize_with helpers keep empty values in string fields and list items working as before. The config tests now go through Config::load_from_path, so they cover the same path as production. New tests pin each option, the helpers, empty package.yml files and the depth limit. Fixes #141 --- Cargo.lock | 141 +++++++++++++++++++++++++++-------------- Cargo.toml | 4 +- src/common_test.rs | 9 +-- src/config.rs | 85 +++++++++++++++++-------- src/lib.rs | 1 + src/project.rs | 85 +++++++++++++++++++++++-- src/project_builder.rs | 45 ++++++++++++- src/yaml.rs | 104 ++++++++++++++++++++++++++++++ 8 files changed, 385 insertions(+), 89 deletions(-) create mode 100644 src/yaml.rs diff --git a/Cargo.lock b/Cargo.lock index eac3781..3fe878a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,6 +11,17 @@ dependencies = [ "memchr", ] +[[package]] +name = "annotate-snippets" +version = "0.12.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c86cd1c51b95d71dde52bca69ed225008f6ff4c8cc825b08042aa1ef823e1980" +dependencies = [ + "anstyle", + "memchr", + "unicode-width 0.2.2", +] + [[package]] name = "anstream" version = "0.6.18" @@ -66,6 +77,12 @@ version = "1.0.104" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" +[[package]] +name = "arraydeque" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" + [[package]] name = "arrayvec" version = "0.7.6" @@ -189,8 +206,8 @@ dependencies = [ "regex", "rusty-hook", "serde", + "serde-saphyr", "serde_json", - "serde_yaml", "similar", "tempfile", "tracing", @@ -203,6 +220,12 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "acbf1af155f9b9ef647e42cdc158db4b64a1b61f743629225fde6f3e0be2a7c7" +[[package]] +name = "core_detect" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7f8f80099a98041a3d1622845c271458a2d73e688351bf3cb999266764b81d48" + [[package]] name = "crossbeam-channel" version = "0.5.17" @@ -256,6 +279,29 @@ version = "1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7fcaabb2fef8c910e7f4c7ce9f67a1283a1715879a7c230ca9d6d1ae31f16d91" +[[package]] +name = "encoding_rs" +version = "0.8.42" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e985e0451871ad22fb8d2b6b076e2028a502a0d3950998c2c5c0a4f9b5d9679" +dependencies = [ + "cfg-if", + "core_detect", + "multiversion_no_op", + "rustversion", + "scopeguard", + "simdutf8", +] + +[[package]] +name = "encoding_rs_io" +version = "0.1.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fba3fe847045ecff794b9c138293a80db914678c453ad63fbf0c6a9eb6e00b22" +dependencies = [ + "encoding_rs", +] + [[package]] name = "enum_dispatch" version = "0.3.13" @@ -275,15 +321,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2d328fc287c61314c4a61af7cfdcbd7e678e39778488c7cb13ec133ce0f4059" dependencies = [ "fsio", - "indexmap 1.9.3", + "indexmap", ] -[[package]] -name = "equivalent" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5" - [[package]] name = "errno" version = "0.3.13" @@ -340,7 +380,7 @@ version = "0.2.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "14dbbfd5c71d70241ecf9e6f13737f7b5ce823821063188d7e46c41d371eebd5" dependencies = [ - "unicode-width", + "unicode-width 0.1.10", ] [[package]] @@ -375,16 +415,20 @@ dependencies = [ ] [[package]] -name = "hashbrown" -version = "0.12.3" +name = "granit-parser" +version = "2.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" +checksum = "da55feca5d1625466bae068f9fd4b87892caeae70c3fe671ddb0f50d41aebf37" +dependencies = [ + "arraydeque", + "smallvec", +] [[package]] name = "hashbrown" -version = "0.15.1" +version = "0.12.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3a9bfc1af68b1726ea47d3d5109de126281def866b33970e10fbab11b5dafab3" +checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" [[package]] name = "heck" @@ -415,17 +459,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99" dependencies = [ "autocfg", - "hashbrown 0.12.3", -] - -[[package]] -name = "indexmap" -version = "2.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "707907fe3c25f5424cce2cb7e1cbcafee6bdbe735ca90ef77c29e84591e5b9da" -dependencies = [ - "equivalent", - "hashbrown 0.15.1", + "hashbrown", ] [[package]] @@ -519,6 +553,12 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "multiversion_no_op" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "743fb55ba31b18fb1ecef6bdc9aa2743314978ac084044301a7eee33fb99a20d" + [[package]] name = "nias" version = "0.5.0" @@ -720,12 +760,6 @@ dependencies = [ "toml", ] -[[package]] -name = "ryu" -version = "1.0.13" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f91339c0467de62360649f8d3e185ca8de4224ff281f66000de5eb2a77a79041" - [[package]] name = "same-file" version = "1.0.6" @@ -735,6 +769,12 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "scopeguard" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" + [[package]] name = "semver" version = "1.0.17" @@ -751,6 +791,20 @@ dependencies = [ "serde_derive", ] +[[package]] +name = "serde-saphyr" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a253edfdea34032da21af84e16fcc94f4c7a6ba237f9f3bc2dbf43cea8535c44" +dependencies = [ + "annotate-snippets", + "encoding_rs_io", + "granit-parser", + "num-traits", + "serde_core", + "smallvec", +] + [[package]] name = "serde_core" version = "1.0.229" @@ -784,19 +838,6 @@ dependencies = [ "zmij", ] -[[package]] -name = "serde_yaml" -version = "0.9.34+deprecated" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" -dependencies = [ - "indexmap 2.6.0", - "itoa", - "ryu", - "serde", - "unsafe-libyaml", -] - [[package]] name = "sharded-slab" version = "0.1.4" @@ -806,6 +847,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "simdutf8" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3a9fe34e3e7a50316060351f37187a3f546bce95496156754b601a5fa71b76e" + [[package]] name = "similar" version = "3.2.0" @@ -972,10 +1019,10 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c0edd1e5b14653f783770bce4a4dabb4a5108a5370a5f5d8cfe8710c361f6c8b" [[package]] -name = "unsafe-libyaml" -version = "0.2.11" +name = "unicode-width" +version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" +checksum = "b4ac048d71ede7ee76d585517add45da530660ef4390e49b098733c6e897f254" [[package]] name = "utf8parse" diff --git a/Cargo.toml b/Cargo.toml index 1f4937b..6bcf46c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -38,7 +38,9 @@ rayon = "1.10.0" regex = "1.11.1" serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.143" -serde_yaml = "0.9.34" +# Deserialize only: `default-features = false` leaves out the serializer and YAML comment +# capture, which we do not use. +serde-saphyr = { version = "2.0.0", default-features = false, features = ["deserialize"] } similar = "3.2.0" tempfile = "3.21.0" tracing = "0.1.41" diff --git a/src/common_test.rs b/src/common_test.rs index 57a8bd8..fdb0ffa 100644 --- a/src/common_test.rs +++ b/src/common_test.rs @@ -1,10 +1,6 @@ #[cfg(test)] pub mod tests { - use std::{ - error::Error, - fs::{self, File}, - path::PathBuf, - }; + use std::{error::Error, fs, path::PathBuf}; use indoc::indoc; @@ -111,8 +107,7 @@ pub mod tests { fs::write(test_config.temp_dir_path.join(&project_file.relative_path), &project_file.content)?; } - let config_file = File::open(test_config.temp_dir_path.join(test_config.relative_code_ownership_config_yml_path))?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&test_config.temp_dir_path.join(test_config.relative_code_ownership_config_yml_path))?; let codeowners_file_path = &test_config.temp_dir_path.join(".github/CODEOWNERS"); let cache: Cache = NoopCache::default().into(); diff --git a/src/config.rs b/src/config.rs index 620185f..ab72acb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,35 +1,44 @@ use serde::Deserialize; -use std::{fs::File, path::Path}; +use std::path::Path; #[derive(Deserialize, Debug, Clone)] pub struct Config { + #[serde(deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_globs: Vec, - #[serde(default = "ruby_package_paths")] + #[serde(default = "ruby_package_paths", deserialize_with = "crate::yaml::strings_or_nulls")] pub ruby_package_paths: Vec, - #[serde(alias = "js_package_paths", default = "javascript_package_paths")] + #[serde( + alias = "js_package_paths", + default = "javascript_package_paths", + deserialize_with = "crate::yaml::strings_or_nulls" + )] pub javascript_package_paths: Vec, - #[serde(default = "team_file_glob")] + #[serde(default = "team_file_glob", deserialize_with = "crate::yaml::strings_or_nulls")] pub team_file_glob: Vec, - #[serde(default = "unowned_globs")] + #[serde(default = "unowned_globs", deserialize_with = "crate::yaml::strings_or_nulls")] pub unowned_globs: Vec, - #[serde(alias = "unbuilt_gems_path", default = "vendored_gems_path")] + #[serde( + alias = "unbuilt_gems_path", + default = "vendored_gems_path", + deserialize_with = "crate::yaml::string_or_null" + )] pub vendored_gems_path: String, - #[serde(default = "default_cache_directory")] + #[serde(default = "default_cache_directory", deserialize_with = "crate::yaml::string_or_null")] pub cache_directory: String, - #[serde(default = "default_ignore_dirs")] + #[serde(default = "default_ignore_dirs", deserialize_with = "crate::yaml::strings_or_nulls")] pub ignore_dirs: Vec, - #[serde(default = "default_executable_name")] + #[serde(default = "default_executable_name", deserialize_with = "crate::yaml::string_or_null")] pub executable_name: String, - #[serde(default = "default_codeowners_path")] + #[serde(default = "default_codeowners_path", deserialize_with = "crate::yaml::string_or_null")] pub codeowners_path: String, } @@ -93,17 +102,14 @@ fn default_codeowners_path() -> String { impl Config { pub fn load_from_path(path: &Path) -> std::result::Result { - let file = File::open(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; - serde_yaml::from_reader(file).map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) + let bytes = std::fs::read(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; + crate::yaml::from_slice(&bytes).map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) } } #[cfg(test)] mod tests { - use std::{ - error::Error, - fs::{self, File}, - }; + use std::{error::Error, fs}; use indoc::indoc; use tempfile::tempdir; @@ -120,8 +126,7 @@ mod tests { - \"{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!( config.owned_globs, vec!["{app,components,config,frontend,lib,packs,spec}/**/*.{rb,rake,js,jsx,ts,tsx,json,yml}"] @@ -149,8 +154,7 @@ mod tests { executable_name: my-custom-codeowners "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.executable_name, "my-custom-codeowners"); Ok(()) } @@ -165,8 +169,7 @@ mod tests { - \"**/*.rb\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.executable_name, "codeowners generate"); Ok(()) } @@ -181,8 +184,7 @@ mod tests { - \"**/*.rb\" "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.codeowners_path, ".github"); Ok(()) } @@ -198,9 +200,40 @@ mod tests { codeowners_path: docs "}; fs::write(&config_path, config_str)?; - let config_file = File::open(&config_path)?; - let config: Config = serde_yaml::from_reader(config_file)?; + let config = Config::load_from_path(&config_path)?; assert_eq!(config.codeowners_path, "docs"); Ok(()) } + + #[test] + fn test_parse_config_with_empty_values() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + - + unowned_globs: + vendored_gems_path: + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!(config.owned_globs, vec!["**/*.rb", ""]); + assert!(config.unowned_globs.is_empty()); + assert_eq!(config.vendored_gems_path, ""); + Ok(()) + } + + #[test] + fn test_load_from_path_reports_parse_error_on_one_line() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + fs::write(&config_path, "owned_globs: {app: true}\n")?; + let error = Config::load_from_path(&config_path).unwrap_err(); + assert!(error.starts_with("Can't parse config file: "), "{error}"); + assert!(error.contains("line 1"), "{error}"); + assert!(!error.contains('\n'), "{error}"); + Ok(()) + } } diff --git a/src/lib.rs b/src/lib.rs index 13d53a4..eb35bd2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,3 +9,4 @@ pub mod project_builder; pub mod project_file_builder; pub mod runner; pub(crate) mod tracked_files; +pub(crate) mod yaml; diff --git a/src/project.rs b/src/project.rs index 12b09a6..4ffaa3d 100644 --- a/src/project.rs +++ b/src/project.rs @@ -2,7 +2,6 @@ use core::fmt; use std::{ collections::HashMap, fmt::Display, - fs::File, path::{Path, PathBuf}, }; @@ -45,8 +44,8 @@ pub struct Team { impl Team { pub fn from_team_file_path(absolute_path: PathBuf) -> Result> { - let file = File::open(&absolute_path).change_context(Error::Io)?; - let deserializer: deserializers::Team = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; + let bytes = std::fs::read(&absolute_path).change_context(Error::Io)?; + let deserializer: deserializers::Team = crate::yaml::from_slice(&bytes).change_context(Error::SerdeYaml)?; Ok(Self { path: absolute_path.to_owned(), name: deserializer.name, @@ -117,6 +116,7 @@ pub mod deserializers { #[derive(Deserialize)] pub struct Github { + #[serde(deserialize_with = "crate::yaml::string_or_null")] pub team: String, #[serde(default = "bool_false")] pub do_not_add_to_codeowners_file: bool, @@ -124,20 +124,25 @@ pub mod deserializers { #[derive(Deserialize)] pub struct Ruby { - #[serde(default = "empty_string_vec")] + #[serde(default = "empty_string_vec", deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_gems: Vec, } #[derive(Deserialize)] pub struct Team { + #[serde(deserialize_with = "crate::yaml::string_or_null")] pub name: String, pub github: Github, pub ruby: Option, - #[serde(default = "empty_string_vec")] + #[serde(default = "empty_string_vec", deserialize_with = "crate::yaml::strings_or_nulls")] pub owned_globs: Vec, - #[serde(alias = "unowned_globs", default = "empty_string_vec")] + #[serde( + alias = "unowned_globs", + default = "empty_string_vec", + deserialize_with = "crate::yaml::strings_or_nulls" + )] pub subtracted_globs: Vec, } @@ -200,6 +205,8 @@ impl Project { #[cfg(test)] mod tests { + use indoc::indoc; + use super::*; #[test] @@ -229,4 +236,70 @@ mod tests { assert_eq!(map.get("a").unwrap().name, vg1.name); assert_eq!(map.get("b").unwrap().name, vg2.name); } + + fn team_from_yaml(yaml: &str) -> Result> { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + Team::from_team_file_path(temp_file.path().to_path_buf()) + } + + #[test] + fn test_team_from_team_file_path_accepts_empty_values() { + let yaml = indoc! {" + name: Payroll + github: + team: + do_not_add_to_codeowners_file: true + owned_globs: + - app/payroll/** + - + subtracted_globs: + ruby: + owned_gems: + "}; + + let team = team_from_yaml(yaml).unwrap(); + assert_eq!(team.name, "Payroll"); + assert_eq!(team.github_team, ""); + assert!(team.avoid_ownership); + assert_eq!(team.owned_globs, vec!["app/payroll/**".to_string(), "".to_string()]); + assert!(team.subtracted_globs.is_empty()); + assert!(team.owned_gems.is_empty()); + } + + #[test] + fn test_team_from_team_file_path_ignores_duplicate_unknown_keys() { + let yaml = indoc! {" + name: Payroll + github: + team: '@acme/payroll' + members: + - a + members: + - b + slack_channel: '#a' + slack_channel: '#b' + "}; + + let team = team_from_yaml(yaml).unwrap(); + assert_eq!(team.github_team, "@acme/payroll"); + } + + #[test] + fn test_team_from_team_file_path_reads_only_true_and_false_as_booleans() { + let yaml = "name: Payroll\ngithub:\n team: '@acme/payroll'\n do_not_add_to_codeowners_file: True\n"; + assert!(team_from_yaml(yaml).unwrap().avoid_ownership); + + let yaml = "name: Payroll\ngithub:\n team: '@acme/payroll'\n do_not_add_to_codeowners_file: yes\n"; + assert!(team_from_yaml(yaml).is_err()); + } + + #[test] + fn test_team_from_team_file_path_reports_missing_field_once() { + let error = team_from_yaml("name: Payroll\n").unwrap_err(); + + let message = format!("{error:?}"); + assert!(message.contains("missing field `github`"), "{message}"); + assert_eq!(message.matches("missing field").count(), 1, "{message}"); + } } diff --git a/src/project_builder.rs b/src/project_builder.rs index e0d06ee..667a9b1 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -337,8 +337,8 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { } pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { - let file = File::open(path).change_context(Error::Io)?; - let deserializer: deserializers::RubyPackage = serde_yaml::from_reader(file).change_context(Error::SerdeYaml)?; + let bytes = std::fs::read(path).change_context(Error::Io)?; + let deserializer: deserializers::RubyPackage = crate::yaml::from_slice(&bytes).change_context(Error::SerdeYaml)?; let top_level_owner = deserializer.owner; let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner); @@ -364,6 +364,8 @@ fn javascript_package_owner(path: &Path) -> Result, Report #[cfg(test)] mod tests { + use indoc::indoc; + use super::*; const OWNED_GLOB: &str = "{app,components,config,frontend,lib,packs,spec,danger,script}/**/*.{rb,arb,erb,rake,js,jsx,ts,tsx}"; @@ -427,4 +429,43 @@ mod tests { let owner = ruby_package_owner(temp_file.path()).unwrap(); assert_eq!(owner, None); } + + #[test] + fn test_ruby_package_owner_empty_file_has_no_owner() { + for yaml in ["", "# no keys yet\n", "---\n"] { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).expect(yaml); + assert_eq!(owner, None, "{yaml:?}"); + } + } + + #[test] + fn test_ruby_package_owner_ignores_duplicate_unknown_keys() { + let yaml = indoc! {" + enforce_dependencies: true + enforce_privacy: true + enforce_privacy: false + metadata: + slack: '#a' + slack: '#b' + owner: TeamA + "}; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, Some("TeamA".to_string())); + } + + #[test] + fn test_ruby_package_owner_duplicate_owner_last_wins() { + let yaml = "owner: TeamA\nowner: TeamB\n"; + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), yaml).unwrap(); + + let owner = ruby_package_owner(temp_file.path()).unwrap(); + assert_eq!(owner, Some("TeamB".to_string())); + } } diff --git a/src/yaml.rs b/src/yaml.rs new file mode 100644 index 0000000..47dbb46 --- /dev/null +++ b/src/yaml.rs @@ -0,0 +1,104 @@ +//! Options keep serde_yaml 0.9's semantics where serde-saphyr allows it: these files are written for Ruby's Psych, +//! and most package.yml keys are ones we never read, so rejecting a file over one would abort the whole run. +use serde::de::DeserializeOwned; +use serde::{Deserialize, Deserializer}; +use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options}; + +/// Budgets stay at their defaults: the depth limit is what keeps deeply nested input from overflowing the stack. +fn options() -> Options { + serde_saphyr::options! { + // The default rejects a repeated key anywhere, including in values we ignore. Last wins, as in Psych. + duplicate_keys: DuplicateKeyPolicy::LastWins, + // serde_yaml treated `<<` as an ordinary key. + merge_keys: MergeKeyPolicy::AsOrdinary, + // Ignored values are still parsed, and the default rejects `.inf`, `.nan` and `1e999` in them. + non_finite_float_policy: NonFiniteFloatPolicy::PassThrough, + // serde_yaml neither decoded nor validated `!!binary`. + ignore_binary_tag_for_string: true, + // serde_yaml rejected YAML 1.1 booleans such as `yes` and `off`. + strict_booleans: true, + // Snippets span several lines, and error_stack prints them twice through `source()`. + with_snippet: false, + } +} + +/// Takes bytes, not a reader: `from_reader` transcodes after a BOM, replacing invalid UTF-8 and accepting UTF-16, unlike serde_yaml. +pub(crate) fn from_slice(bytes: &[u8]) -> Result { + serde_saphyr::from_slice_with_options(bytes, options()) +} + +/// Reads an empty, `~` or `null` value as `""` instead of failing. serde_yaml accepted these, keeping `~` and `null` as text. +pub(crate) fn string_or_null<'de, D: Deserializer<'de>>(deserializer: D) -> Result { + Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) +} + +/// [`string_or_null`] for each item of a list. +pub(crate) fn strings_or_nulls<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { + Ok(Vec::>::deserialize(deserializer)? + .into_iter() + .map(Option::unwrap_or_default) + .collect()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::project::deserializers::RubyPackage; + + fn owner(yaml: &[u8]) -> Result, serde_saphyr::Error> { + from_slice::(yaml).map(|package| package.owner) + } + + #[test] + fn test_ignores_unusual_values_in_unknown_keys() { + for unknown in [ + "<<: 5", + "error_budget: .inf", + "sample_rate: .nan", + "huge: 1e999", + "blob: !!binary not base64", + "legacy: !ruby/object:OpenStruct {table: {}}", + ] { + let yaml = format!("owner: TeamA\n{unknown}\n"); + assert_eq!(owner(yaml.as_bytes()).expect(unknown), Some("TeamA".to_string()), "{unknown}"); + } + } + + #[test] + fn test_does_not_apply_merge_keys() { + let yaml = "defaults: &defaults\n owner: TeamA\n<<: *defaults\nmetadata:\n <<: {owner: TeamA}\n"; + let package: RubyPackage = from_slice(yaml.as_bytes()).unwrap(); + assert_eq!(package.owner, None); + assert_eq!(package.metadata.and_then(|metadata| metadata.owner), None); + } + + #[test] + fn test_reads_utf8_bom_before_several_keys() { + assert_eq!( + owner("\u{feff}enforce_dependencies: true\nowner: TeamA\n".as_bytes()).unwrap(), + Some("TeamA".to_string()) + ); + } + + #[test] + fn test_rejects_invalid_utf8_after_bom() { + assert!(owner(b"\xef\xbb\xbfowner: Caf\xe9\n").is_err()); + } + + #[test] + fn test_rejects_deeply_nested_values() { + let owner_at_depth = |depth: usize| { + let yaml = format!("owner: TeamA\nignored: {}{}\n", "[".repeat(depth), "]".repeat(depth)); + // A large stack makes a lost depth limit fail the assertion below instead of overflowing the test thread. + std::thread::Builder::new() + .stack_size(16 * 1024 * 1024) + .spawn(move || owner(yaml.as_bytes())) + .unwrap() + .join() + .unwrap() + }; + + assert_eq!(owner_at_depth(10).unwrap(), Some("TeamA".to_string())); + assert!(owner_at_depth(100).is_err()); + } +} From a95ff598821c63aa350f57a52e063026ee3a75ab Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 19 Oct 2026 22:47:31 -0700 Subject: [PATCH 2/9] Bound YAML budgets and pin every helper with a test With DuplicateKeyPolicy::LastWins, serde-saphyr buffers mappings before serde sees them, and nested mappings once per level. Under the default budget a crafted 480 KB package.yml made `generate` peak at 2.4 GB and take 15 s, against 35 MB and 0.5 s on main. Cap depth at 16 and nodes at 50,000, and lower the scalar-byte and anchor-copy caps. The worst file that fits now peaks at about 195 MB. The 1,015 files read in two large private monorepos nest at most 5 deep with at most 2,162 nodes, and `generate` output for both is unchanged. strings_or_nulls now reads an empty, `~` or `null` list as empty itself, rather than relying on serde-saphyr to do so (serde-saphyr#233 asks it to stop). The empty-value tests now cover every deserialize_with field, so removing any of them fails a test; before, 11 of the 15 could be removed with the suite still passing. The depth test now nests 20 deep, between the new limit and the default, and asserts a budget error; a new test does the same for the node limit. Dependabot's cargo group now excludes serde-saphyr, so its bumps get their own pull request. --- .github/dependabot.yml | 4 ++++ src/config.rs | 31 +++++++++++++++++++++++-- src/project.rs | 10 ++++---- src/yaml.rs | 52 ++++++++++++++++++++++++++++++------------ 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 17aafe5..6ec0feb 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -8,6 +8,10 @@ updates: cargo: patterns: - "*" + # serde-saphyr patches only its latest release, and its releases can change how YAML parses, + # so its bumps get their own pull request instead of waiting on unrelated breakage. + exclude-patterns: + - "serde-saphyr" cooldown: default-days: 7 - package-ecosystem: "github-actions" diff --git a/src/config.rs b/src/config.rs index ab72acb..0ba7951 100644 --- a/src/config.rs +++ b/src/config.rs @@ -214,14 +214,41 @@ mod tests { owned_globs: - \"**/*.rb\" - + ruby_package_paths: + - + javascript_package_paths: + - + team_file_glob: + - unowned_globs: + - + ignore_dirs: + - vendored_gems_path: + cache_directory: + executable_name: + codeowners_path: "}; fs::write(&config_path, config_str)?; let config = Config::load_from_path(&config_path)?; assert_eq!(config.owned_globs, vec!["**/*.rb", ""]); - assert!(config.unowned_globs.is_empty()); - assert_eq!(config.vendored_gems_path, ""); + for globs in [ + &config.ruby_package_paths, + &config.javascript_package_paths, + &config.team_file_glob, + &config.unowned_globs, + &config.ignore_dirs, + ] { + assert_eq!(globs, &[""]); + } + for value in [ + &config.vendored_gems_path, + &config.cache_directory, + &config.executable_name, + &config.codeowners_path, + ] { + assert_eq!(value, ""); + } Ok(()) } diff --git a/src/project.rs b/src/project.rs index 4ffaa3d..915cc88 100644 --- a/src/project.rs +++ b/src/project.rs @@ -246,7 +246,7 @@ mod tests { #[test] fn test_team_from_team_file_path_accepts_empty_values() { let yaml = indoc! {" - name: Payroll + name: github: team: do_not_add_to_codeowners_file: true @@ -254,17 +254,19 @@ mod tests { - app/payroll/** - subtracted_globs: + - ruby: owned_gems: + - "}; let team = team_from_yaml(yaml).unwrap(); - assert_eq!(team.name, "Payroll"); + assert_eq!(team.name, ""); assert_eq!(team.github_team, ""); assert!(team.avoid_ownership); assert_eq!(team.owned_globs, vec!["app/payroll/**".to_string(), "".to_string()]); - assert!(team.subtracted_globs.is_empty()); - assert!(team.owned_gems.is_empty()); + assert_eq!(team.subtracted_globs, vec!["".to_string()]); + assert_eq!(team.owned_gems, vec!["".to_string()]); } #[test] diff --git a/src/yaml.rs b/src/yaml.rs index 47dbb46..d353700 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -4,9 +4,17 @@ use serde::de::DeserializeOwned; use serde::{Deserialize, Deserializer}; use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options}; -/// Budgets stay at their defaults: the depth limit is what keeps deeply nested input from overflowing the stack. fn options() -> Options { serde_saphyr::options! { + // LastWins re-buffers nested mappings once per level, so the default budget lets a 500 KB file take gigabytes. + // Real files nest about 5 deep in a few thousand nodes; the depth limit also prevents stack overflow. + budget: serde_saphyr::budget! { + max_depth: 16, + max_nodes: 50_000, + max_total_scalar_bytes: 4 << 20, + max_recorded_anchor_events: 100_000, + max_recorded_anchor_bytes: 1 << 20, + }, // The default rejects a repeated key anywhere, including in values we ignore. Last wins, as in Psych. duplicate_keys: DuplicateKeyPolicy::LastWins, // serde_yaml treated `<<` as an ordinary key. @@ -32,9 +40,11 @@ pub(crate) fn string_or_null<'de, D: Deserializer<'de>>(deserializer: D) -> Resu Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) } -/// [`string_or_null`] for each item of a list. +/// [`string_or_null`] for each item of a list. Reads an empty, `~` or `null` list as empty itself, since serde-saphyr#233 asks +/// serde-saphyr to stop doing that for a `Vec`, and `unowned_globs:` with no value is common. pub(crate) fn strings_or_nulls<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { - Ok(Vec::>::deserialize(deserializer)? + Ok(Option::>>::deserialize(deserializer)? + .unwrap_or_default() .into_iter() .map(Option::unwrap_or_default) .collect()) @@ -64,6 +74,20 @@ mod tests { } } + #[test] + fn test_reads_empty_list_as_empty() { + #[derive(Deserialize)] + struct Globs { + #[serde(deserialize_with = "strings_or_nulls")] + globs: Vec, + } + + for yaml in ["globs:\n", "globs: ~\n", "globs: null\n", "globs: []\n"] { + let parsed: Globs = from_slice(yaml.as_bytes()).expect(yaml); + assert!(parsed.globs.is_empty(), "{yaml:?}"); + } + } + #[test] fn test_does_not_apply_merge_keys() { let yaml = "defaults: &defaults\n owner: TeamA\n<<: *defaults\nmetadata:\n <<: {owner: TeamA}\n"; @@ -87,18 +111,18 @@ mod tests { #[test] fn test_rejects_deeply_nested_values() { - let owner_at_depth = |depth: usize| { - let yaml = format!("owner: TeamA\nignored: {}{}\n", "[".repeat(depth), "]".repeat(depth)); - // A large stack makes a lost depth limit fail the assertion below instead of overflowing the test thread. - std::thread::Builder::new() - .stack_size(16 * 1024 * 1024) - .spawn(move || owner(yaml.as_bytes())) - .unwrap() - .join() - .unwrap() - }; + let owner_at_depth = + |depth: usize| owner(format!("owner: TeamA\nignored: {}{}\n", "[".repeat(depth), "]".repeat(depth)).as_bytes()); assert_eq!(owner_at_depth(10).unwrap(), Some("TeamA".to_string())); - assert!(owner_at_depth(100).is_err()); + assert!(matches!(owner_at_depth(20), Err(serde_saphyr::Error::Budget { .. }))); + } + + #[test] + fn test_rejects_too_many_nodes() { + let owner_with_items = |count: usize| owner(format!("owner: TeamA\nignored: [{}]\n", vec!["x"; count].join(", ")).as_bytes()); + + assert_eq!(owner_with_items(10_000).unwrap(), Some("TeamA".to_string())); + assert!(matches!(owner_with_items(100_000), Err(serde_saphyr::Error::Budget { .. }))); } } From 17a22d68a798360fc0394025c904d7ddcf1d0259 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 19 Oct 2026 23:40:05 -0700 Subject: [PATCH 3/9] Make the YAML node limit configurable with yaml_max_nodes A team file or package.yml over the node limit aborts the whole run, and until now raising the limit took a new release. The new `yaml_max_nodes` key in config/code_ownership.yml (default 50,000) sets it for every path that reads those files: generate and validate, for-file, and for-file --from-codeowners. The team cache is keyed on it, so teams loaded under one limit are not reused under another. A file over the limit now also gets a hint naming the key. The config file itself is still read with the default, since it can't raise its own limit and has at most a few hundred nodes. --- README.md | 1 + src/config.rs | 28 ++++++++- src/ownership/codeowners_file_parser.rs | 7 ++- src/ownership/codeowners_query.rs | 2 + src/ownership/file_owner_resolver.rs | 55 ++++++++++++++--- src/project.rs | 6 +- src/project_builder.rs | 26 ++++---- src/runner/api.rs | 1 + src/yaml.rs | 60 ++++++++++++++---- tests/yaml_max_nodes_test.rs | 82 +++++++++++++++++++++++++ 10 files changed, 227 insertions(+), 41 deletions(-) create mode 100644 tests/yaml_max_nodes_test.rs diff --git a/README.md b/README.md index ebbafcf..193365f 100644 --- a/README.md +++ b/README.md @@ -214,6 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. +- `yaml_max_nodes` (default: `50000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to `config/code_ownership.yml` itself. Example configuration with custom executable name: diff --git a/src/config.rs b/src/config.rs index 0ba7951..1941ffa 100644 --- a/src/config.rs +++ b/src/config.rs @@ -40,6 +40,9 @@ pub struct Config { #[serde(default = "default_codeowners_path", deserialize_with = "crate::yaml::string_or_null")] pub codeowners_path: String, + + #[serde(default = "default_yaml_max_nodes")] + pub yaml_max_nodes: usize, } #[allow(dead_code)] @@ -100,10 +103,16 @@ fn default_codeowners_path() -> String { ".github".to_string() } +fn default_yaml_max_nodes() -> usize { + crate::yaml::DEFAULT_MAX_NODES +} + impl Config { pub fn load_from_path(path: &Path) -> std::result::Result { let bytes = std::fs::read(path).map_err(|e| format!("Can't open config file: {} ({})", path.to_string_lossy(), e))?; - crate::yaml::from_slice(&bytes).map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) + // The config can't raise its own limit, and at a few hundred nodes it never needs to. + crate::yaml::from_slice(&bytes, crate::yaml::DEFAULT_MAX_NODES) + .map_err(|e| format!("Can't parse config file: {} ({})", path.to_string_lossy(), e)) } } @@ -140,6 +149,23 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); + assert_eq!(config.yaml_max_nodes, 50_000); + Ok(()) + } + + #[test] + fn test_parse_config_with_custom_yaml_max_nodes() -> Result<(), Box> { + let temp_dir = tempdir()?; + let config_path = temp_dir.path().join("config.yml"); + let config_str = indoc! {" + --- + owned_globs: + - \"**/*.rb\" + yaml_max_nodes: 200000 + "}; + fs::write(&config_path, config_str)?; + let config = Config::load_from_path(&config_path)?; + assert_eq!(config.yaml_max_nodes, 200_000); Ok(()) } diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index fd25691..1dc538a 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -14,6 +14,7 @@ pub struct Parser { pub project_root: PathBuf, pub codeowners_file_path: PathBuf, pub team_file_globs: Vec, + pub yaml_max_nodes: usize, } impl Parser { @@ -40,7 +41,7 @@ impl Parser { let codeowners_entries = parse_codeowners_entries(self.codeowners_file_path.to_string_lossy().into_owned()); - let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs()); + let teams_by_name = teams_by_github_team_name(self.absolute_team_files_globs(), self.yaml_max_nodes); let result: HashMap> = file_inputs .par_iter() @@ -65,13 +66,13 @@ impl Parser { } #[memoize] -fn teams_by_github_team_name(team_file_glob: Vec) -> HashMap { +fn teams_by_github_team_name(team_file_glob: Vec, yaml_max_nodes: usize) -> HashMap { let mut teams = HashMap::new(); for glob in team_file_glob { match glob::glob(&glob) { Ok(paths) => { for path in paths.filter_map(Result::ok) { - let team = match Team::from_team_file_path(path.clone()) { + let team = match Team::from_team_file_path(path.clone(), yaml_max_nodes) { Ok(team) => team, Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index 562bb48..3486e25 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -8,6 +8,7 @@ pub(crate) fn teams_for_files_from_codeowners( project_root: &Path, codeowners_file_path: &Path, team_file_globs: &[String], + yaml_max_nodes: usize, file_paths: &[String], ) -> Result>, String> { let relative_file_paths: Vec = file_paths @@ -26,6 +27,7 @@ pub(crate) fn teams_for_files_from_codeowners( codeowners_file_path: codeowners_file_path.to_path_buf(), project_root: project_root.to_path_buf(), team_file_globs: team_file_globs.to_vec(), + yaml_max_nodes, }; parser.teams_from_files_paths(&relative_file_paths).map_err(|e| e.to_string()) diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index cc7f9fa..463d2d4 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -20,7 +20,7 @@ pub fn find_file_owners(project_root: &Path, config: &Config, file_path: &Path) }; let relative_file_path = crate::path_utils::relative_to_buf(project_root, &absolute_file_path); - let loaded = loaded_teams(teams_cache_root(project_root), config.team_file_glob.clone())?; + let loaded = loaded_teams(teams_cache_root(project_root), config.team_file_glob.clone(), config.yaml_max_nodes)?; let teams = &loaded.teams; let teams_by_name = &loaded.teams_by_name; @@ -105,7 +105,7 @@ struct LoadedTeams { teams_by_name: HashMap, } -type TeamCacheKey = (PathBuf, Vec); +type TeamCacheKey = (PathBuf, Vec, usize); // Parsing every team file dominates a lookup, so load them once per project root and glob list and share // the result across threads for the life of the process. @@ -122,8 +122,12 @@ fn team_cache() -> MutexGuard<'static, TeamCache> { TEAM_CACHE.lock().unwrap_or_else(PoisonError::into_inner) } -fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::result::Result, String> { - let key = (project_root, team_file_globs); +fn loaded_teams( + project_root: PathBuf, + team_file_globs: Vec, + yaml_max_nodes: usize, +) -> std::result::Result, String> { + let key = (project_root, team_file_globs, yaml_max_nodes); let generation = { let cache = team_cache(); if let Some(loaded) = cache.loaded.get(&key) { @@ -132,7 +136,7 @@ fn loaded_teams(project_root: PathBuf, team_file_globs: Vec) -> std::res cache.generation }; - let load = load_teams(&key.0, &key.1)?; + let load = load_teams(&key.0, &key.1, key.2)?; let teams_by_name = build_teams_by_name_map(&load.teams); let loaded = Arc::new(LoadedTeams { teams: load.teams, @@ -178,7 +182,7 @@ struct TeamLoad { skipped_team_file: bool, } -fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::Result { +fn load_teams(project_root: &Path, team_file_globs: &[String], yaml_max_nodes: usize) -> std::result::Result { let mut teams: Vec = Vec::new(); let mut skipped_team_file = false; for glob_str in team_file_globs { @@ -193,7 +197,7 @@ fn load_teams(project_root: &Path, team_file_globs: &[String]) -> std::result::R continue; } }; - match Team::from_team_file_path(path.clone()) { + match Team::from_team_file_path(path.clone(), yaml_max_nodes) { Ok(team) => teams.push(team), Err(e) => { eprintln!("Error parsing team file: {e:?}, path: {}", path.display()); @@ -266,7 +270,7 @@ fn nearest_package_owner( if glob_list_matches(rel_str, &config.ruby_package_paths) { let pkg_yml = current.join("package.yml"); if pkg_yml.exists() { - match crate::project_builder::ruby_package_owner(&pkg_yml) { + match crate::project_builder::ruby_package_owner(&pkg_yml, config.yaml_max_nodes) { Ok(owner) => { if let Some(team) = owner.and_then(|owner| teams_by_name.get(&owner)) { let package_path = parent_rel.join("package.yml"); @@ -377,6 +381,7 @@ mod tests { ignore_dirs: vec![], executable_name: "codeowners".to_string(), codeowners_path: ".github".to_string(), + yaml_max_nodes: crate::yaml::DEFAULT_MAX_NODES, } } @@ -678,7 +683,11 @@ mod tests { #[test] fn test_clear_team_cache_during_a_load_is_not_undone_by_its_result() { let _guard = TEAM_CACHE_TEST_LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); - let key: TeamCacheKey = (PathBuf::from("/clear-during-load"), vec!["config/teams/**/*.yml".to_string()]); + let key: TeamCacheKey = ( + PathBuf::from("/clear-during-load"), + vec!["config/teams/**/*.yml".to_string()], + crate::yaml::DEFAULT_MAX_NODES, + ); let loaded = || { Arc::new(LoadedTeams { teams: Vec::new(), @@ -826,4 +835,32 @@ mod tests { assert_eq!(owner_of(&config_b), Some("Billing".to_string())); assert_eq!(owner_of(&config_a), Some("Payroll".to_string())); } + + #[test] + fn test_team_cache_is_keyed_on_yaml_max_nodes() { + let td = tempdir().unwrap(); + let root = td.path(); + fs::create_dir_all(root.join("config/teams")).unwrap(); + fs::write( + root.join("config/teams/payroll.yml"), + "name: Payroll\ngithub:\n team: '@PayrollTeam'\nowned_globs:\n - app/**/*\n", + ) + .unwrap(); + let config_with_max_nodes = |yaml_max_nodes: usize| crate::config::Config { + yaml_max_nodes, + ..build_config_for_temp("frontend/**/*", "packs/**/*", "vendored") + }; + let owner_of = |config: &crate::config::Config| { + find_file_owners(root, config, Path::new("app/a.rb")) + .unwrap() + .first() + .map(|owner| owner.team.name.clone()) + }; + + assert_eq!( + owner_of(&config_with_max_nodes(crate::yaml::DEFAULT_MAX_NODES)), + Some("Payroll".to_string()) + ); + assert_eq!(owner_of(&config_with_max_nodes(3)), None, "a team file over the limit is skipped"); + } } diff --git a/src/project.rs b/src/project.rs index 915cc88..3deb58e 100644 --- a/src/project.rs +++ b/src/project.rs @@ -43,9 +43,9 @@ pub struct Team { } impl Team { - pub fn from_team_file_path(absolute_path: PathBuf) -> Result> { + pub fn from_team_file_path(absolute_path: PathBuf, yaml_max_nodes: usize) -> Result> { let bytes = std::fs::read(&absolute_path).change_context(Error::Io)?; - let deserializer: deserializers::Team = crate::yaml::from_slice(&bytes).change_context(Error::SerdeYaml)?; + let deserializer: deserializers::Team = crate::yaml::from_project_slice(&bytes, yaml_max_nodes).change_context(Error::SerdeYaml)?; Ok(Self { path: absolute_path.to_owned(), name: deserializer.name, @@ -240,7 +240,7 @@ mod tests { fn team_from_yaml(yaml: &str) -> Result> { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - Team::from_team_file_path(temp_file.path().to_path_buf()) + Team::from_team_file_path(temp_file.path().to_path_buf(), crate::yaml::DEFAULT_MAX_NODES) } #[test] diff --git a/src/project_builder.rs b/src/project_builder.rs index 667a9b1..f7b892e 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -212,6 +212,7 @@ impl<'a> ProjectBuilder<'a> { Vec, ); + let yaml_max_nodes = self.config.yaml_max_nodes; let (project_files, packages, vendored_gems, directory_codeowners, teams): Accumulator = entry_types .into_par_iter() .try_fold( @@ -242,7 +243,7 @@ impl<'a> ProjectBuilder<'a> { } } EntryType::RubyPackage(absolute_path, relative_path) => { - match ruby_package_owner(&absolute_path) + match ruby_package_owner(&absolute_path, yaml_max_nodes) .attach_with(|| format!("Failed to read ruby package: {}", absolute_path.display())) { Ok(Some(owner)) => { @@ -282,7 +283,7 @@ impl<'a> ProjectBuilder<'a> { }); } EntryType::TeamFile(absolute_path, _relative_path) => { - let team = Team::from_team_file_path(absolute_path.clone()) + let team = Team::from_team_file_path(absolute_path.clone(), yaml_max_nodes) .change_context(Error::Io) .attach_with(|| format!("Failed to read team file: {}", absolute_path.display()))?; team_files.push(team); @@ -336,9 +337,10 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { } } -pub(crate) fn ruby_package_owner(path: &Path) -> Result, Report> { +pub(crate) fn ruby_package_owner(path: &Path, yaml_max_nodes: usize) -> Result, Report> { let bytes = std::fs::read(path).change_context(Error::Io)?; - let deserializer: deserializers::RubyPackage = crate::yaml::from_slice(&bytes).change_context(Error::SerdeYaml)?; + let deserializer: deserializers::RubyPackage = + crate::yaml::from_project_slice(&bytes, yaml_max_nodes).change_context(Error::SerdeYaml)?; let top_level_owner = deserializer.owner; let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner); @@ -386,7 +388,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -396,7 +398,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamB".to_string())); } @@ -406,7 +408,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let result = ruby_package_owner(temp_file.path()); + let result = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES); assert!(result.is_err()); } @@ -416,7 +418,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -426,7 +428,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, None); } @@ -436,7 +438,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).expect(yaml); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).expect(yaml); assert_eq!(owner, None, "{yaml:?}"); } } @@ -455,7 +457,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamA".to_string())); } @@ -465,7 +467,7 @@ mod tests { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); - let owner = ruby_package_owner(temp_file.path()).unwrap(); + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamB".to_string())); } } diff --git a/src/runner/api.rs b/src/runner/api.rs index bf4e9c9..8de27bf 100644 --- a/src/runner/api.rs +++ b/src/runner/api.rs @@ -69,6 +69,7 @@ pub fn teams_for_files_from_codeowners( &run_config.project_root, &codeowners_file_path, &config.team_file_glob, + config.yaml_max_nodes, file_paths, ) .map_err(Error::Io)?; diff --git a/src/yaml.rs b/src/yaml.rs index d353700..b526faf 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -1,16 +1,20 @@ //! Options keep serde_yaml 0.9's semantics where serde-saphyr allows it: these files are written for Ruby's Psych, //! and most package.yml keys are ones we never read, so rejecting a file over one would abort the whole run. +use error_stack::Report; use serde::de::DeserializeOwned; use serde::{Deserialize, Deserializer}; -use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options}; +use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options, budget::BudgetBreach}; -fn options() -> Options { +/// Default for the `yaml_max_nodes` config key. Real files have at most a few thousand nodes. +pub(crate) const DEFAULT_MAX_NODES: usize = 50_000; + +fn options(max_nodes: usize) -> Options { serde_saphyr::options! { // LastWins re-buffers nested mappings once per level, so the default budget lets a 500 KB file take gigabytes. - // Real files nest about 5 deep in a few thousand nodes; the depth limit also prevents stack overflow. + // Real files nest about 5 deep; the depth limit also prevents stack overflow. budget: serde_saphyr::budget! { max_depth: 16, - max_nodes: 50_000, + max_nodes: max_nodes, max_total_scalar_bytes: 4 << 20, max_recorded_anchor_events: 100_000, max_recorded_anchor_bytes: 1 << 20, @@ -31,8 +35,30 @@ fn options() -> Options { } /// Takes bytes, not a reader: `from_reader` transcodes after a BOM, replacing invalid UTF-8 and accepting UTF-16, unlike serde_yaml. -pub(crate) fn from_slice(bytes: &[u8]) -> Result { - serde_saphyr::from_slice_with_options(bytes, options()) +pub(crate) fn from_slice(bytes: &[u8], max_nodes: usize) -> Result { + serde_saphyr::from_slice_with_options(bytes, options(max_nodes)) +} + +/// [`from_slice`] for team files and package.yml, whose node limit is the `yaml_max_nodes` config key: a file over it +/// gets a hint naming that key. +pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: usize) -> Result> { + from_slice(bytes, max_nodes).map_err(|error| { + let too_many_nodes = matches!( + error, + serde_saphyr::Error::Budget { + breach: BudgetBreach::Nodes { .. }, + .. + } + ); + let report = Report::new(error); + if too_many_nodes { + report.attach(format!( + "raise `yaml_max_nodes` (now {max_nodes}) in the codeowners config to allow larger files" + )) + } else { + report + } + }) } /// Reads an empty, `~` or `null` value as `""` instead of failing. serde_yaml accepted these, keeping `~` and `null` as text. @@ -56,7 +82,7 @@ mod tests { use crate::project::deserializers::RubyPackage; fn owner(yaml: &[u8]) -> Result, serde_saphyr::Error> { - from_slice::(yaml).map(|package| package.owner) + from_slice::(yaml, DEFAULT_MAX_NODES).map(|package| package.owner) } #[test] @@ -83,7 +109,7 @@ mod tests { } for yaml in ["globs:\n", "globs: ~\n", "globs: null\n", "globs: []\n"] { - let parsed: Globs = from_slice(yaml.as_bytes()).expect(yaml); + let parsed: Globs = from_slice(yaml.as_bytes(), DEFAULT_MAX_NODES).expect(yaml); assert!(parsed.globs.is_empty(), "{yaml:?}"); } } @@ -91,7 +117,7 @@ mod tests { #[test] fn test_does_not_apply_merge_keys() { let yaml = "defaults: &defaults\n owner: TeamA\n<<: *defaults\nmetadata:\n <<: {owner: TeamA}\n"; - let package: RubyPackage = from_slice(yaml.as_bytes()).unwrap(); + let package: RubyPackage = from_slice(yaml.as_bytes(), DEFAULT_MAX_NODES).unwrap(); assert_eq!(package.owner, None); assert_eq!(package.metadata.and_then(|metadata| metadata.owner), None); } @@ -120,9 +146,17 @@ mod tests { #[test] fn test_rejects_too_many_nodes() { - let owner_with_items = |count: usize| owner(format!("owner: TeamA\nignored: [{}]\n", vec!["x"; count].join(", ")).as_bytes()); - - assert_eq!(owner_with_items(10_000).unwrap(), Some("TeamA".to_string())); - assert!(matches!(owner_with_items(100_000), Err(serde_saphyr::Error::Budget { .. }))); + let owner_with_items = |count: usize, max_nodes: usize| { + let yaml = format!("owner: TeamA\nignored: [{}]\n", vec!["x"; count].join(", ")); + from_slice::(yaml.as_bytes(), max_nodes).map(|package| package.owner) + }; + + assert_eq!(owner_with_items(10_000, DEFAULT_MAX_NODES).unwrap(), Some("TeamA".to_string())); + assert!(matches!( + owner_with_items(100_000, DEFAULT_MAX_NODES), + Err(serde_saphyr::Error::Budget { .. }) + )); + assert_eq!(owner_with_items(100_000, 200_000).unwrap(), Some("TeamA".to_string())); + assert!(matches!(owner_with_items(1_000, 500), Err(serde_saphyr::Error::Budget { .. }))); } } diff --git a/tests/yaml_max_nodes_test.rs b/tests/yaml_max_nodes_test.rs new file mode 100644 index 0000000..ed2f222 --- /dev/null +++ b/tests/yaml_max_nodes_test.rs @@ -0,0 +1,82 @@ +use assert_cmd::Command; +use predicates::prelude::*; +use std::{error::Error, fs, path::Path}; +use tempfile::TempDir; + +mod common; + +use common::git_add_all_files; +use common::setup_fixture_repo; + +// 60,000 ignored list items put a package.yml and a team file over the default `yaml_max_nodes` of 50,000. +fn project_with_large_yaml_files(yaml_max_nodes: Option) -> TempDir { + let temp_dir = setup_fixture_repo(Path::new("tests/fixtures/valid_project")); + let root = temp_dir.path(); + let ignored = format!("ignored: [{}]\n", vec!["x"; 60_000].join(", ")); + + fs::write( + root.join("ruby/packages/payroll_flow/package.yml"), + format!("owner: Payroll\n{ignored}"), + ) + .unwrap(); + fs::create_dir_all(root.join("ruby/packages/payroll_flow/app")).unwrap(); + fs::write(root.join("ruby/packages/payroll_flow/app/flow.rb"), "class Flow; end\n").unwrap(); + + let team_path = root.join("config/teams/payroll.yml"); + let team = fs::read_to_string(&team_path).unwrap(); + fs::write(&team_path, format!("{team}{ignored}")).unwrap(); + + if let Some(yaml_max_nodes) = yaml_max_nodes { + let config_path = root.join("config/code_ownership.yml"); + let config = fs::read_to_string(&config_path).unwrap(); + fs::write(&config_path, format!("{config}\nyaml_max_nodes: {yaml_max_nodes}\n")).unwrap(); + } + + git_add_all_files(root); + temp_dir +} + +fn codeowners(project_root: &Path, args: &[&str]) -> Command { + let mut cmd = Command::cargo_bin("codeowners").unwrap(); + cmd.arg("--project-root").arg(project_root).arg("--no-cache").args(args); + cmd +} + +#[test] +fn test_default_yaml_max_nodes_rejects_large_files() -> Result<(), Box> { + let temp_dir = project_with_large_yaml_files(None); + let root = temp_dir.path(); + + codeowners(root, &["generate"]) + .assert() + .failure() + .stderr(predicate::str::contains("budget breached").and(predicate::str::contains("raise `yaml_max_nodes` (now 50000)"))); + codeowners(root, &["for-file", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr(predicate::str::contains("Error reading ruby package").and(predicate::str::contains("budget breached"))); + codeowners(root, &["for-file", "--from-codeowners", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .stderr(predicate::str::contains("Error parsing team file").and(predicate::str::contains("budget breached"))); + + Ok(()) +} + +#[test] +fn test_raised_yaml_max_nodes_accepts_large_files() -> Result<(), Box> { + let temp_dir = project_with_large_yaml_files(Some(100_000)); + let root = temp_dir.path(); + + codeowners(root, &["generate"]).assert().success().stderr(""); + codeowners(root, &["for-file", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .success() + .stdout(predicate::str::contains("Team: Payroll")) + .stderr(""); + codeowners(root, &["for-file", "--from-codeowners", "ruby/packages/payroll_flow/app/flow.rb"]) + .assert() + .success() + .stdout(predicate::str::contains("Team: Payroll")) + .stderr(""); + + Ok(()) +} From e68a0cf60cd7ba7e2ac666481d460f4791f0a043 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 09:32:05 -0700 Subject: [PATCH 4/9] Lower the default yaml_max_nodes to 20,000 50,000 left 23x headroom over the largest real file because the limit could not be raised without a release. Now that yaml_max_nodes can raise it, 20,000 still leaves 9x headroom over the 2,162 nodes of the largest of the 1,015 files read in two large monorepos, and cuts the worst-case peak for a file that fits from about 195 MB to about 40 MB. `generate` output for both monorepos is unchanged. --- README.md | 2 +- src/config.rs | 2 +- src/yaml.rs | 2 +- tests/yaml_max_nodes_test.rs | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 193365f..59f807c 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `yaml_max_nodes` (default: `50000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to `config/code_ownership.yml` itself. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to `config/code_ownership.yml` itself. Example configuration with custom executable name: diff --git a/src/config.rs b/src/config.rs index 1941ffa..3d7002a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -149,7 +149,7 @@ mod tests { ); assert_eq!(config.vendored_gems_path, "vendored/"); assert_eq!(config.executable_name, "codeowners generate"); - assert_eq!(config.yaml_max_nodes, 50_000); + assert_eq!(config.yaml_max_nodes, 20_000); Ok(()) } diff --git a/src/yaml.rs b/src/yaml.rs index b526faf..0b28b0e 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Deserializer}; use serde_saphyr::{DuplicateKeyPolicy, MergeKeyPolicy, NonFiniteFloatPolicy, Options, budget::BudgetBreach}; /// Default for the `yaml_max_nodes` config key. Real files have at most a few thousand nodes. -pub(crate) const DEFAULT_MAX_NODES: usize = 50_000; +pub(crate) const DEFAULT_MAX_NODES: usize = 20_000; fn options(max_nodes: usize) -> Options { serde_saphyr::options! { diff --git a/tests/yaml_max_nodes_test.rs b/tests/yaml_max_nodes_test.rs index ed2f222..77348b9 100644 --- a/tests/yaml_max_nodes_test.rs +++ b/tests/yaml_max_nodes_test.rs @@ -8,11 +8,11 @@ mod common; use common::git_add_all_files; use common::setup_fixture_repo; -// 60,000 ignored list items put a package.yml and a team file over the default `yaml_max_nodes` of 50,000. +// 30,000 ignored list items put a package.yml and a team file over the default `yaml_max_nodes` of 20,000. fn project_with_large_yaml_files(yaml_max_nodes: Option) -> TempDir { let temp_dir = setup_fixture_repo(Path::new("tests/fixtures/valid_project")); let root = temp_dir.path(); - let ignored = format!("ignored: [{}]\n", vec!["x"; 60_000].join(", ")); + let ignored = format!("ignored: [{}]\n", vec!["x"; 30_000].join(", ")); fs::write( root.join("ruby/packages/payroll_flow/package.yml"), @@ -50,7 +50,7 @@ fn test_default_yaml_max_nodes_rejects_large_files() -> Result<(), Box Date: Tue, 20 Oct 2026 09:39:52 -0700 Subject: [PATCH 5/9] Scale the other YAML size caps with yaml_max_nodes Raising yaml_max_nodes did not help a file that then hit the fixed 4 MiB scalar-byte cap or an anchor cap, and those breaches got no hint. The scalar-byte, parser-event and anchor caps now grow with the node limit: 256 bytes, 50 events and 5 anchor copies (64 bytes) per node. At the 20,000 default that is about 5 MB, the 1,000,000 events serde-saphyr defaults to, 100,000 anchor copies and about 1.3 MB. A breach of any of them, including one reached through an alias, gets the yaml_max_nodes hint. Also update the team cache comment, which is keyed on the node limit too. --- README.md | 2 +- src/ownership/file_owner_resolver.rs | 2 +- src/yaml.rs | 59 +++++++++++++++++++++++----- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 59f807c..f97c13c 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. It does not apply to `config/code_ownership.yml` itself. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. The limits on a file's total string bytes, parser events and anchor copies grow with it. It does not apply to `config/code_ownership.yml` itself. Example configuration with custom executable name: diff --git a/src/ownership/file_owner_resolver.rs b/src/ownership/file_owner_resolver.rs index 463d2d4..42da95b 100644 --- a/src/ownership/file_owner_resolver.rs +++ b/src/ownership/file_owner_resolver.rs @@ -107,7 +107,7 @@ struct LoadedTeams { type TeamCacheKey = (PathBuf, Vec, usize); -// Parsing every team file dominates a lookup, so load them once per project root and glob list and share +// Parsing every team file dominates a lookup, so load them once per project root, glob list and node limit and share // the result across threads for the life of the process. static TEAM_CACHE: LazyLock> = LazyLock::new(Default::default); diff --git a/src/yaml.rs b/src/yaml.rs index 0b28b0e..a177628 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -12,12 +12,14 @@ fn options(max_nodes: usize) -> Options { serde_saphyr::options! { // LastWins re-buffers nested mappings once per level, so the default budget lets a 500 KB file take gigabytes. // Real files nest about 5 deep; the depth limit also prevents stack overflow. + // The other caps grow with `yaml_max_nodes`, so raising it is all a larger file needs. budget: serde_saphyr::budget! { max_depth: 16, max_nodes: max_nodes, - max_total_scalar_bytes: 4 << 20, - max_recorded_anchor_events: 100_000, - max_recorded_anchor_bytes: 1 << 20, + max_events: max_nodes.saturating_mul(50), + max_total_scalar_bytes: max_nodes.saturating_mul(256), + max_recorded_anchor_events: max_nodes.saturating_mul(5), + max_recorded_anchor_bytes: max_nodes.saturating_mul(64), }, // The default rejects a repeated key anywhere, including in values we ignore. Last wins, as in Psych. duplicate_keys: DuplicateKeyPolicy::LastWins, @@ -39,19 +41,27 @@ pub(crate) fn from_slice(bytes: &[u8], max_nodes: usize) -> serde_saphyr::from_slice_with_options(bytes, options(max_nodes)) } -/// [`from_slice`] for team files and package.yml, whose node limit is the `yaml_max_nodes` config key: a file over it -/// gets a hint naming that key. +/// [`from_slice`] for team files and package.yml, whose size limits come from the `yaml_max_nodes` config key: a file +/// over them gets a hint naming that key. pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: usize) -> Result> { from_slice(bytes, max_nodes).map_err(|error| { - let too_many_nodes = matches!( - error, + let mut original = &error; + while let serde_saphyr::Error::AliasError { error, .. } = original { + original = error; + } + let too_large = matches!( + original, serde_saphyr::Error::Budget { - breach: BudgetBreach::Nodes { .. }, + breach: BudgetBreach::Nodes { .. } + | BudgetBreach::Events { .. } + | BudgetBreach::ScalarBytes { .. } + | BudgetBreach::RecordedAnchorEvents { .. } + | BudgetBreach::RecordedAnchorBytes { .. }, .. } ); let report = Report::new(error); - if too_many_nodes { + if too_large { report.attach(format!( "raise `yaml_max_nodes` (now {max_nodes}) in the codeowners config to allow larger files" )) @@ -159,4 +169,35 @@ mod tests { assert_eq!(owner_with_items(100_000, 200_000).unwrap(), Some("TeamA".to_string())); assert!(matches!(owner_with_items(1_000, 500), Err(serde_saphyr::Error::Budget { .. }))); } + + #[test] + fn test_scalar_byte_limit_grows_with_max_nodes() { + let yaml = format!("owner: TeamA\nignored: [{}]\n", vec!["x".repeat(400); 15_000].join(", ")); + + let Err(report) = from_project_slice::(yaml.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("15,000 strings of 400 bytes should exceed the default scalar-byte limit"); + }; + assert!(matches!( + report.current_context(), + serde_saphyr::Error::Budget { + breach: BudgetBreach::ScalarBytes { .. }, + .. + } + )); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + assert_eq!( + from_slice::(yaml.as_bytes(), 40_000).unwrap().owner, + Some("TeamA".to_string()) + ); + + let aliased = format!( + "owner: TeamA\nbig: &a \"{}\"\nignored: [{}]\n", + "x".repeat(1_000_000), + ["*a"; 8].join(", ") + ); + let Err(report) = from_project_slice::(aliased.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("eight aliases of a 1 MB string should exceed the default scalar-byte limit"); + }; + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + } } From 4e5d717359221ac0bb77a1d0f1ce12dfa164692d Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 15:17:52 -0700 Subject: [PATCH 6/9] Read an empty package.yml as no owner without relying on serde-saphyr An empty, comment-only or `---`-only package.yml is a null document. serde-saphyr 2.0.0 reads that into a struct whose fields are all optional, so it meant "no owner", but serde-saphyr 2.1.0, merged upstream and not yet released, rejects null for a struct as part of fixing serde-saphyr#233. Our "2.0.0" requirement admits 2.1, and code_ownership resolves serde-saphyr afresh when it next bumps its codeowners tag, so one empty package.yml would then abort generate and validate for the whole repo. Read the document as an Option instead. The empty-file test now also covers a lone BOM, `--- # comment`, `~`, `null` and `...`, and passes on serde-saphyr 2.0.0 and on its master branch, both at 222afe7, which fixed #233, and at 2454b53. Also update the strings_or_nulls comment: #233 is now fixed for 2.1, not just requested. --- src/project_builder.rs | 17 +++++++++++++++-- src/yaml.rs | 4 ++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/project_builder.rs b/src/project_builder.rs index f7b892e..ed7a19f 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -339,8 +339,12 @@ fn matches_globs(path: &Path, globs: &[String]) -> bool { pub(crate) fn ruby_package_owner(path: &Path, yaml_max_nodes: usize) -> Result, Report> { let bytes = std::fs::read(path).change_context(Error::Io)?; - let deserializer: deserializers::RubyPackage = + // An empty, comment-only or `---`-only file is a null document, which serde-saphyr 2.1 no longer reads as a struct. + let package: Option = crate::yaml::from_project_slice(&bytes, yaml_max_nodes).change_context(Error::SerdeYaml)?; + let Some(deserializer) = package else { + return Ok(None); + }; let top_level_owner = deserializer.owner; let metadata_owner = deserializer.metadata.and_then(|metadata| metadata.owner); @@ -434,7 +438,16 @@ mod tests { #[test] fn test_ruby_package_owner_empty_file_has_no_owner() { - for yaml in ["", "# no keys yet\n", "---\n"] { + for yaml in [ + "", + "\u{feff}", + "# no keys yet\n", + "---\n", + "--- # no keys yet\n", + "~\n", + "null\n", + "...\n", + ] { let temp_file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(temp_file.path(), yaml).unwrap(); diff --git a/src/yaml.rs b/src/yaml.rs index a177628..8fb8910 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -76,8 +76,8 @@ pub(crate) fn string_or_null<'de, D: Deserializer<'de>>(deserializer: D) -> Resu Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) } -/// [`string_or_null`] for each item of a list. Reads an empty, `~` or `null` list as empty itself, since serde-saphyr#233 asks -/// serde-saphyr to stop doing that for a `Vec`, and `unowned_globs:` with no value is common. +/// [`string_or_null`] for each item of a list. Reads an empty, `~` or `null` list as empty itself, since serde-saphyr 2.1 +/// stops doing that for a `Vec` (serde-saphyr#233), and `unowned_globs:` with no value is common. pub(crate) fn strings_or_nulls<'de, D: Deserializer<'de>>(deserializer: D) -> Result, D::Error> { Ok(Option::>>::deserialize(deserializer)? .unwrap_or_default() From 178467cf9eb2b110bf4a23c89af23814d2f4f212 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 15:21:54 -0700 Subject: [PATCH 7/9] Print YAML errors from an alias once error_stack's Report::new adds each source() of its context as another frame. serde-saphyr's AliasError already includes the error it wraps in its message, along with where the anchor was defined and used, and it also returns that error from source(), so every error inside an alias, budget breaches included, printed twice. main printed these once. Wrap the error in a YamlError with no source() before building the report. The with_snippet comment no longer cites the double printing, which this removes; snippets stay off because they add several lines to each error. --- src/yaml.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/yaml.rs b/src/yaml.rs index 8fb8910..c21345e 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -31,7 +31,7 @@ fn options(max_nodes: usize) -> Options { ignore_binary_tag_for_string: true, // serde_yaml rejected YAML 1.1 booleans such as `yes` and `off`. strict_booleans: true, - // Snippets span several lines, and error_stack prints them twice through `source()`. + // Snippets add several lines to each error, which serde_yaml kept to one. with_snippet: false, } } @@ -43,7 +43,7 @@ pub(crate) fn from_slice(bytes: &[u8], max_nodes: usize) -> /// [`from_slice`] for team files and package.yml, whose size limits come from the `yaml_max_nodes` config key: a file /// over them gets a hint naming that key. -pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: usize) -> Result> { +pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: usize) -> Result> { from_slice(bytes, max_nodes).map_err(|error| { let mut original = &error; while let serde_saphyr::Error::AliasError { error, .. } = original { @@ -60,7 +60,7 @@ pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: u .. } ); - let report = Report::new(error); + let report = Report::new(YamlError(error)); if too_large { report.attach(format!( "raise `yaml_max_nodes` (now {max_nodes}) in the codeowners config to allow larger files" @@ -71,6 +71,19 @@ pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: u }) } +/// A serde-saphyr error with no `source()`. An alias error's message already includes the error it wraps, and +/// error_stack would print that error again as its source. +#[derive(Debug)] +pub(crate) struct YamlError(pub(crate) serde_saphyr::Error); + +impl std::fmt::Display for YamlError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.0, f) + } +} + +impl core::error::Error for YamlError {} + /// Reads an empty, `~` or `null` value as `""` instead of failing. serde_yaml accepted these, keeping `~` and `null` as text. pub(crate) fn string_or_null<'de, D: Deserializer<'de>>(deserializer: D) -> Result { Ok(Option::::deserialize(deserializer)?.unwrap_or_default()) @@ -178,7 +191,7 @@ mod tests { panic!("15,000 strings of 400 bytes should exceed the default scalar-byte limit"); }; assert!(matches!( - report.current_context(), + &report.current_context().0, serde_saphyr::Error::Budget { breach: BudgetBreach::ScalarBytes { .. }, .. @@ -200,4 +213,13 @@ mod tests { }; assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); } + + #[test] + fn test_reports_an_alias_error_once() { + let Err(report) = from_project_slice::(b"x: &x [a]\nowner: *x\n", DEFAULT_MAX_NODES) else { + panic!("a sequence is not a valid owner"); + }; + let message = format!("{report:?}"); + assert_eq!(message.matches("expected string scalar").count(), 1, "{message}"); + } } From f990a074b9532519c77bff6d1d4b9d3f0cade3bc Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 15:24:58 -0700 Subject: [PATCH 8/9] Correct the yaml_max_nodes docs and hint The hint said "the codeowners config", which can be read as .github/CODEOWNERS. It now names config/code_ownership.yml, as the README's list of config keys does. The README said a file over the limit fails with `budget breached: Nodes`, but a file under it can fail first on the scalar-byte or anchor-copy caps that grow with it. The new text leaves out the event cap: at 50 events per node, it is reached first only when the limit is 0. The README also didn't say that raising the limit raises the memory a crafted file can make codeowners use, or that a fixed depth limit applies to every YAML file, config included. That memory grows by about 6 KB of heap, or about 14 KB of resident memory on macOS, per node. Parsing the worst file found holds at most about 115 MB of heap at the default limit and about 285 MB at 50,000. It makes `generate` peak at about 290 MB on macOS, against 12 MB on main, and at about 710 MB at 50,000, against 18 MB. Earlier commits here give about 40 MB and 195 MB, measured on weaker files. LastWins buffers a mapping again at each level that has an integer key, so the worst file nests 14 mappings that each start with `0: x`, around 9,750 `[]: []` entries. With only string keys the same nesting holds at most about 12 MB of heap. --- README.md | 2 +- src/yaml.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f97c13c..433c198 100644 --- a/README.md +++ b/README.md @@ -214,7 +214,7 @@ codeowners gv --no-cache - `cache_directory` (default: `'tmp/cache/codeowners'`) - `ignore_dirs` (default includes: `.git`, `node_modules`, `tmp`, etc.) - `executable_name` (default: `'codeowners'`): Customize the command name shown in validation error messages. Useful when using `codeowners-rs` via wrappers like the [code_ownership](https://github.com/rubyatscale/code_ownership) Ruby gem. -- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over the limit fails with `budget breached: Nodes`. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. The limits on a file's total string bytes, parser events and anchor copies grow with it. It does not apply to `config/code_ownership.yml` itself. +- `yaml_max_nodes` (default: `20000`): The most YAML nodes (mappings, sequences and scalars) allowed in one team file or `package.yml`. A file over it, or over the limits on total scalar bytes and anchor copies that grow with it, fails with a `budget breached` error and a hint naming this key. Raise it if a large team file hits the limit; real files have at most a few thousand nodes. The memory a crafted file can make `codeowners` use grows with it too: about 290 MB at the default and about 710 MB at 50,000, measured on macOS. It does not apply to `config/code_ownership.yml` itself. Separately, any of these files, or the config, fails if it nests more than 16 levels deep, counting the top level. That limit can't be raised. Example configuration with custom executable name: diff --git a/src/yaml.rs b/src/yaml.rs index c21345e..45b0c68 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -63,7 +63,7 @@ pub(crate) fn from_project_slice(bytes: &[u8], max_nodes: u let report = Report::new(YamlError(error)); if too_large { report.attach(format!( - "raise `yaml_max_nodes` (now {max_nodes}) in the codeowners config to allow larger files" + "raise `yaml_max_nodes` (now {max_nodes}) in config/code_ownership.yml to allow larger files" )) } else { report From 172117159db0b4dde9e99a4c22617caea577e158 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 15:27:22 -0700 Subject: [PATCH 9/9] Close the gaps mutation testing found in the YAML tests Mutation testing found changes that every test still passed: removing the event cap, either anchor-copy cap, or the hint for any of the three; giving a depth breach the hint; moving the depth limit anywhere from 11 to 19 levels; dropping config/code_ownership.yml from the hint; letting strings_or_nulls fail on a null list, which serde-saphyr 2.0 reads as empty even without the helper; and dropping yaml_max_nodes from the memoized team map that for-file --from-codeowners uses. The hint for team files was checked only when generate happened to stop at the large team file rather than the large package.yml, which depends on thread timing. New tests breach each anchor-copy cap at the default limit and parse the same files at 40,000. Two cover the event cap: it grows past serde-saphyr's default of 1,000,000, and a yaml_max_nodes of 0 breaches it with the hint. That is the only limit at which it is reached before the node cap, since a node yields at most about three events. The depth test now checks 16 and 17 levels and that the breach gets no hint. Others check the whole hint on the package.yml path and the hint on the team file path directly, read a null list through serde's unit deserializer, and run the from-codeowners lookup under two limits in one process. Of 94 mutations, 89 now fail at least one test. The 5 that pass change nothing serde-saphyr 2.0.0 lets a test observe: two other event-cap multipliers, non_finite_float_policy AsString, unwrapping one layer of AliasError instead of looping, and reading package.yml as the bare struct, which the empty-file test catches under serde-saphyr 2.1. --- src/ownership/codeowners_file_parser.rs | 26 ++++++++ src/project_builder.rs | 12 ++++ src/yaml.rs | 89 +++++++++++++++++++++++-- tests/yaml_max_nodes_test.rs | 7 +- 4 files changed, 129 insertions(+), 5 deletions(-) diff --git a/src/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index 1dc538a..b14dbaf 100644 --- a/src/ownership/codeowners_file_parser.rs +++ b/src/ownership/codeowners_file_parser.rs @@ -496,6 +496,32 @@ mod tests { Ok(()) } + #[test] + fn test_teams_from_files_paths_is_keyed_on_yaml_max_nodes() -> Result<(), Box> { + let temp_dir = tempfile::tempdir()?; + let root = temp_dir.path(); + fs::create_dir_all(root.join("config/teams"))?; + fs::write( + root.join("config/teams/payroll.yml"), + "name: Payroll\ngithub:\n team: '@PayrollTeam'\n", + )?; + fs::write(root.join("CODEOWNERS"), "# Team-specific owned globs\n/app/**/* @PayrollTeam\n")?; + let owner_of = |yaml_max_nodes: usize| -> Result, Box> { + let parser = Parser { + project_root: root.to_path_buf(), + codeowners_file_path: root.join("CODEOWNERS"), + team_file_globs: vec!["config/teams/**/*.yml".to_string()], + yaml_max_nodes, + }; + let teams = parser.teams_from_files_paths(&[PathBuf::from("app/a.rb")])?; + Ok(teams["app/a.rb"].as_ref().map(|team| team.name.clone())) + }; + + assert_eq!(owner_of(crate::yaml::DEFAULT_MAX_NODES)?, Some("Payroll".to_string())); + assert_eq!(owner_of(3)?, None, "a team file over the limit is skipped"); + Ok(()) + } + #[test] fn test_unignore_regex() -> Result<(), Box> { let un_ignore = Regex::new(r"^# \/")?; diff --git a/src/project_builder.rs b/src/project_builder.rs index ed7a19f..591a65b 100644 --- a/src/project_builder.rs +++ b/src/project_builder.rs @@ -483,4 +483,16 @@ mod tests { let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).unwrap(); assert_eq!(owner, Some("TeamB".to_string())); } + + #[test] + fn test_ruby_package_owner_over_the_node_limit_names_yaml_max_nodes() { + let temp_file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(temp_file.path(), "owner: TeamA\nignored: [a, b, c, d, e]\n").unwrap(); + + let error = ruby_package_owner(temp_file.path(), 5).unwrap_err(); + assert!( + format!("{error:?}").contains("raise `yaml_max_nodes` (now 5) in config/code_ownership.yml to allow larger files"), + "{error:?}" + ); + } } diff --git a/src/yaml.rs b/src/yaml.rs index 45b0c68..0a7d6dd 100644 --- a/src/yaml.rs +++ b/src/yaml.rs @@ -137,6 +137,14 @@ mod tests { } } + #[test] + fn test_strings_or_nulls_reads_null_as_empty_list() { + use serde::de::IntoDeserializer; + // serde-saphyr 2.0 would read null as an empty list even without the helper; serde's unit deserializer does not. + let null: serde::de::value::UnitDeserializer = ().into_deserializer(); + assert_eq!(strings_or_nulls(null).unwrap(), Vec::::new()); + } + #[test] fn test_does_not_apply_merge_keys() { let yaml = "defaults: &defaults\n owner: TeamA\n<<: *defaults\nmetadata:\n <<: {owner: TeamA}\n"; @@ -160,11 +168,23 @@ mod tests { #[test] fn test_rejects_deeply_nested_values() { - let owner_at_depth = - |depth: usize| owner(format!("owner: TeamA\nignored: {}{}\n", "[".repeat(depth), "]".repeat(depth)).as_bytes()); + let nested = |lists: usize| format!("owner: TeamA\nignored: {}{}\n", "[".repeat(lists), "]".repeat(lists)); - assert_eq!(owner_at_depth(10).unwrap(), Some("TeamA".to_string())); - assert!(matches!(owner_at_depth(20), Err(serde_saphyr::Error::Budget { .. }))); + // 16 levels, counting the top-level mapping, are allowed. + assert_eq!(owner(nested(15).as_bytes()).unwrap(), Some("TeamA".to_string())); + assert!(matches!( + owner(nested(16).as_bytes()), + Err(serde_saphyr::Error::Budget { + breach: BudgetBreach::Depth { .. }, + .. + }) + )); + + // Raising `yaml_max_nodes` does not raise the depth limit, so its breach gets no hint. + let Err(report) = from_project_slice::(nested(16).as_bytes(), DEFAULT_MAX_NODES) else { + panic!("17 levels should be rejected"); + }; + assert!(!format!("{report:?}").contains("yaml_max_nodes"), "{report:?}"); } #[test] @@ -183,6 +203,67 @@ mod tests { assert!(matches!(owner_with_items(1_000, 500), Err(serde_saphyr::Error::Budget { .. }))); } + #[test] + fn test_anchor_copy_limits_grow_with_max_nodes() { + // Ten nested anchored lists copy each of their 10,001 items into ten anchor buffers. + let nested_anchors = format!( + "owner: TeamA\nignored: {}{}{}\n", + (0..10).map(|i| format!("&a{i} [")).collect::(), + vec!["x"; 10_001].join(", "), + "]".repeat(10) + ); + // The escape makes the scalar owned, so its 1.3 MB count toward the anchor-copy byte limit. + let owned_anchor = format!("owner: TeamA\nbig: &a \"{}\\t\"\n", "x".repeat(1_300_000)); + + for yaml in [&nested_anchors, &owned_anchor] { + let Err(report) = from_project_slice::(yaml.as_bytes(), DEFAULT_MAX_NODES) else { + panic!("should exceed an anchor-copy limit at the default yaml_max_nodes"); + }; + assert!( + matches!( + &report.current_context().0, + serde_saphyr::Error::Budget { + breach: BudgetBreach::RecordedAnchorEvents { .. } | BudgetBreach::RecordedAnchorBytes { .. }, + .. + } + ), + "{report:?}" + ); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes`"), "{report:?}"); + assert_eq!( + from_slice::(yaml.as_bytes(), 40_000).unwrap().owner, + Some("TeamA".to_string()) + ); + } + } + + #[test] + fn test_event_limit_grows_with_max_nodes() { + // A node yields up to about three parser events, so a fixed event cap such as serde-saphyr's default of 1,000,000 + // would reject files that a raised `yaml_max_nodes` allows. + let budget = options(700_000).budget.unwrap(); + assert!(budget.max_events > serde_saphyr::Budget::default().max_events); + } + + #[test] + fn test_event_limit_breach_names_yaml_max_nodes() { + // The event cap is reached before the node cap only at a limit of 0. + let Err(report) = from_project_slice::(b"owner: TeamA\n", 0) else { + panic!("a yaml_max_nodes of 0 should reject every file"); + }; + assert!( + matches!( + &report.current_context().0, + serde_saphyr::Error::Budget { + breach: BudgetBreach::Events { .. }, + .. + } + ), + "{report:?}" + ); + assert!(format!("{report:?}").contains("raise `yaml_max_nodes` (now 0)"), "{report:?}"); + } + #[test] fn test_scalar_byte_limit_grows_with_max_nodes() { let yaml = format!("owner: TeamA\nignored: [{}]\n", vec!["x".repeat(400); 15_000].join(", ")); diff --git a/tests/yaml_max_nodes_test.rs b/tests/yaml_max_nodes_test.rs index 77348b9..ab681f1 100644 --- a/tests/yaml_max_nodes_test.rs +++ b/tests/yaml_max_nodes_test.rs @@ -54,9 +54,14 @@ fn test_default_yaml_max_nodes_rejects_large_files() -> Result<(), Box