From 25be0f19e1e7e20a183933cec4fd550eabf3fa88 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Tue, 20 Oct 2026 22:53:45 -0700 Subject: [PATCH] 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 crate the advisory names for serde users. All YAML parsing now goes through src/yaml.rs. Callers read the whole file and parse the bytes with options that keep serde_yaml's behavior where serde-saphyr allows it: `<<` stays an ordinary key, `!!binary` is neither decoded nor validated, `.inf`, `.nan` and `1e999` still parse in values we ignore, YAML 1.1 booleans such as `yes` are still rejected, and errors stay on one line. A repeated key takes its last value, as in Psych. serde-saphyr's default rejects one anywhere in a file, and serde_yaml rejected one only in a field we read. Error text also changes, and a few rare inputs are now rejected, accepted or read differently, as the pull request describes. Two deserialize_with helpers keep accepting an empty, `~` or `null` string field or list item, but read all three as "", where serde_yaml kept `~` and `null` as text. The list helper also reads an empty, `~` or `null` list as empty, where serde_yaml rejected the last two. package.yml is read as an Option, so an empty file still means no owner, and now so does a file that is only `~`, `null` or `...`. serde-saphyr 2.0 does both of these itself, but its unreleased 2.1, which our "2.0.0" requirement admits, stops (serde-saphyr#233). Errors from team files and package.yml are wrapped in a type with no source(), since error_stack would otherwise print an error inside an alias twice. With duplicate keys set to last-wins, serde-saphyr buffers each mapping it reads into one of our structs before serde sees it, and buffers a nested mapping again at each level where it follows an integer key, so under its default budget a crafted 480 KB package.yml made `generate` peak at about 2.7 GB, against 35 MB on main. Nesting is now capped at 16 levels and nodes at 20,000, and the caps on scalar bytes, parser events, recorded anchor events and recorded anchor bytes grow with the node limit, at 256, 50, 5 and 64 per node. The worst file found that fits, 68 KB, makes `generate` peak at about 290 MB on macOS, against 12 MB on main. Ordinary files are unaffected: the 1,015 team and package.yml files in two large private monorepos nest at most 5 deep with fewer than 2,200 nodes, and `generate` gives the same output for both as on main, with the same peak memory within about 2%. The node limit is the new `yaml_max_nodes` key in config/code_ownership.yml, so a repo whose team files or package.yml files outgrow it can raise it, and the caps that grow with it, without waiting for a release. Every command that reads team files or package.yml honors it, and the team cache and the memoized team map are keyed on it. A file over it, or over a cap that grows with it, fails like a malformed file, with a hint naming the key. The config file itself is always read with the default. Release archives now also hold a licenses/ directory next to the codeowners binary, which stays at the archive root, where dotslash-config.json looks for it. dev/license-notices copies into it the license files of two crates serde-saphyr brings in, whose licenses require binary redistributions to carry their notices: granit-parser, its parser, which asks for all inherited and new notices, and encoding_rs, whose WHATWG-derived data is under BSD-3-Clause. Today's macOS binaries don't link encoding_rs, since we parse only byte slices, so its notice is a precaution. The script takes versions from Cargo.lock and fails if a listed crate leaves the dependency graph or ships no license files. The jobs that build release archives run only on main after a version bump, so the Check job also runs the script on pull requests. Dependabot's cargo group now excludes serde-saphyr, so its bumps get their own pull request. New tests pin each option, budget and hint, both helpers and every deserialize_with field, empty package.yml files, and the node limit through the CLI. For the event cap they check only that it grows with the node limit. The config tests go through Config::load_from_path, as production does. Fixes #141 --- .github/dependabot.yml | 4 + .github/workflows/ci.yml | 23 +- Cargo.lock | 141 +++++++---- Cargo.toml | 4 +- README.md | 3 + dev/license-notices | 66 +++++ src/common_test.rs | 9 +- src/config.rs | 138 +++++++++-- src/lib.rs | 1 + src/ownership/codeowners_file_parser.rs | 33 ++- src/ownership/codeowners_query.rs | 2 + src/ownership/file_owner_resolver.rs | 57 ++++- src/project.rs | 89 ++++++- src/project_builder.rs | 88 ++++++- src/runner/api.rs | 1 + src/yaml.rs | 306 ++++++++++++++++++++++++ tests/yaml_max_nodes_test.rs | 87 +++++++ 17 files changed, 936 insertions(+), 116 deletions(-) create mode 100755 dev/license-notices create mode 100644 src/yaml.rs create mode 100644 tests/yaml_max_nodes_test.rs 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/.github/workflows/ci.yml b/.github/workflows/ci.yml index b31e0a4..b6bf4ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,11 @@ jobs: - name: Run cargo check run: cargo check + + - name: Collect the release archives' license notices + # The release jobs run only on main, so this catches a dependency change that breaks the + # script on its pull request rather than at release time. + run: dev/license-notices "$RUNNER_TEMP/licenses" test: name: Test Suite runs-on: ubuntu-latest @@ -129,10 +134,14 @@ jobs: # This combines the intel and m1 binaries into a single binary lipo -create -output target/codeowners target/aarch64-apple-darwin/release/codeowners target/x86_64-apple-darwin/release/codeowners - # Creates artifact for homebrew. -C means run from `target` directory - tar -czf target/codeowners-mac.tar.gz -C target codeowners + # Notices that some dependencies' licenses require redistributions of the binary to carry + dev/license-notices target/licenses + + # Creates artifact for homebrew. -C means run from `target` directory, which keeps the + # binary at the archive root, where dotslash-config.json looks for it + tar -czf target/codeowners-mac.tar.gz -C target codeowners licenses - # This tarball is a binary that is executable + # This tarball holds the executable binary and the license notices gh release upload $NEW_VERSION target/codeowners-mac.tar.gz env: @@ -158,8 +167,12 @@ jobs: cross build --release --target aarch64-unknown-linux-gnu - name: Upload linux binaries run: | - tar -czf target/x86_64-unknown-linux-gnu.tar.gz -C target/x86_64-unknown-linux-gnu/release codeowners - tar -czf target/aarch64-unknown-linux-gnu.tar.gz -C target/aarch64-unknown-linux-gnu/release codeowners + # Notices that some dependencies' licenses require redistributions of the binary to carry. + # Each -C keeps the binary at its archive's root, where dotslash-config.json looks for it. + dev/license-notices target/x86_64-unknown-linux-gnu/release/licenses + dev/license-notices target/aarch64-unknown-linux-gnu/release/licenses + tar -czf target/x86_64-unknown-linux-gnu.tar.gz -C target/x86_64-unknown-linux-gnu/release codeowners licenses + tar -czf target/aarch64-unknown-linux-gnu.tar.gz -C target/aarch64-unknown-linux-gnu/release codeowners licenses gh release upload $NEW_VERSION target/x86_64-unknown-linux-gnu.tar.gz gh release upload $NEW_VERSION target/aarch64-unknown-linux-gnu.tar.gz env: 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/README.md b/README.md index ebbafcf..5e9b9b7 100644 --- a/README.md +++ b/README.md @@ -47,6 +47,8 @@ You can run `codeowners` without installing a platform-specific binary by using 2. Download the latest DotSlash text file from a release, for example [https://github.com/rubyatscale/codeowners-rs/releases](https://github.com/rubyatscale/codeowners-rs/releases). 3. Execute the downloaded file with DotSlash; it will fetch and run the correct binary. +Release archives after v0.5.0 also hold a `licenses` directory next to the `codeowners` binary. It has the license notices that the dependencies listed in `dev/license-notices` require redistributions of the binary to include. + ### Option B: From source with Cargo Requires Rust toolchain. @@ -214,6 +216,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 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. It does not apply to `config/code_ownership.yml` itself. Raising it also raises how much memory a single file can take: a file crafted to waste memory can make `codeowners` use about 280 MB more at the default and about 700 MB more at 50,000, measured on macOS, while ordinary files add almost nothing. 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/dev/license-notices b/dev/license-notices new file mode 100755 index 0000000..94987f8 --- /dev/null +++ b/dev/license-notices @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Copies, verbatim, the license files of the crates listed below into /-/ for +# the release archives, since their licenses require redistributions of the binary to carry those +# notices. Versions come from Cargo.lock. +# +# Usage: dev/license-notices +# +# must not exist yet, so an archive never carries notices left over from other versions. Fails +# if a listed crate is no longer a dependency or ships no license files, so whoever drops or +# replaces one updates the list. +# +# Keep this bash 3.2 compatible: the mac release job runs it with macOS's /bin/bash, while the +# pull request check runs it on ubuntu. + +set -euo pipefail +shopt -s nullglob nocaseglob + +crates=( + # serde-saphyr's parser. Its LICENSE requires redistributions to include its notice and the ones + # it inherited, which it keeps in .licenses/. + granit-parser + # serde-saphyr decodes reader input with it, through encoding_rs_io. Its WHATWG-derived data is + # under BSD-3-Clause, which requires binary redistributions to reproduce that notice. + encoding_rs +) + +if [[ $# -ne 1 ]]; then + echo "usage: $0 " >&2 + exit 2 +fi +out=$1 +if [[ -e $out ]]; then + echo "$0: $out already exists" >&2 + exit 1 +fi + +manifest="$(dirname "$0")/../Cargo.toml" +metadata=$(cargo metadata --format-version 1 --locked --manifest-path "$manifest") +mkdir -p "$out" + +for crate in "${crates[@]}"; do + # One "\t" line per version of the crate in Cargo.lock. + packages=$(jq -r --arg name "$crate" \ + '.packages[] | select(.name == $name) | "\(.version)\t\(.manifest_path)"' <<<"$metadata") + if [[ -z $packages ]]; then + echo "$0: $crate is no longer a dependency. Update the crate list in $0." >&2 + exit 1 + fi + + while IFS=$'\t' read -r version manifest_path; do + src=$(dirname "$manifest_path") + # The license files at the crate root, plus the .licenses directory that granit-parser's LICENSE + # links to. Copying keeps their relative paths, so those links still resolve. + notices=("$src"/{LICENSE,LICENCE,COPYRIGHT,NOTICE,COPYING}*) + if [[ -d $src/.licenses ]]; then + notices+=("$src/.licenses") + fi + if [[ ${#notices[@]} -eq 0 ]]; then + echo "$0: $src has no license files. Check $crate's license and update $0." >&2 + exit 1 + fi + + mkdir "$out/$crate-$version" + cp -RL "${notices[@]}" "$out/$crate-$version" + done <<<"$packages" +done 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..3d7002a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,36 +1,48 @@ 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, + + #[serde(default = "default_yaml_max_nodes")] + pub yaml_max_nodes: usize, } #[allow(dead_code)] @@ -91,19 +103,22 @@ 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 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))?; + // 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)) } } #[cfg(test)] mod tests { - use std::{ - error::Error, - fs::{self, File}, - }; + use std::{error::Error, fs}; use indoc::indoc; use tempfile::tempdir; @@ -120,8 +135,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}"] @@ -135,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, 20_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(()) } @@ -149,8 +180,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 +195,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 +210,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 +226,67 @@ 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\" + - + 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", ""]); + 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(()) + } + + #[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/ownership/codeowners_file_parser.rs b/src/ownership/codeowners_file_parser.rs index fd25691..b14dbaf 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()); @@ -495,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/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..42da95b 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,9 +105,9 @@ 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 +// 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); @@ -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 12b09a6..3deb58e 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}, }; @@ -44,9 +43,9 @@ 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)?; + 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_project_slice(&bytes, yaml_max_nodes).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,72 @@ 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(), crate::yaml::DEFAULT_MAX_NODES) + } + + #[test] + fn test_team_from_team_file_path_accepts_empty_values() { + let yaml = indoc! {" + name: + 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, ""); + assert_eq!(team.github_team, ""); + assert!(team.avoid_ownership); + assert_eq!(team.owned_globs, vec!["app/payroll/**".to_string(), "".to_string()]); + assert_eq!(team.subtracted_globs, vec!["".to_string()]); + assert_eq!(team.owned_gems, vec!["".to_string()]); + } + + #[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..591a65b 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,14 @@ 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)?; +pub(crate) fn ruby_package_owner(path: &Path, yaml_max_nodes: usize) -> Result, Report> { + let bytes = std::fs::read(path).change_context(Error::Io)?; + // 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); @@ -364,6 +370,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}"; @@ -384,7 +392,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())); } @@ -394,7 +402,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())); } @@ -404,7 +412,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()); } @@ -414,7 +422,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())); } @@ -424,7 +432,67 @@ 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); } + + #[test] + fn test_ruby_package_owner_empty_file_has_no_owner() { + 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(); + + let owner = ruby_package_owner(temp_file.path(), crate::yaml::DEFAULT_MAX_NODES).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(), crate::yaml::DEFAULT_MAX_NODES).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(), 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/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 new file mode 100644 index 0000000..0a7d6dd --- /dev/null +++ b/src/yaml.rs @@ -0,0 +1,306 @@ +//! 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, 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 = 20_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; 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_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, + // 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 add several lines to each error, which serde_yaml kept to one. + 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], max_nodes: usize) -> Result { + serde_saphyr::from_slice_with_options(bytes, options(max_nodes)) +} + +/// [`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 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 { .. } + | BudgetBreach::Events { .. } + | BudgetBreach::ScalarBytes { .. } + | BudgetBreach::RecordedAnchorEvents { .. } + | BudgetBreach::RecordedAnchorBytes { .. }, + .. + } + ); + let report = Report::new(YamlError(error)); + if too_large { + report.attach(format!( + "raise `yaml_max_nodes` (now {max_nodes}) in config/code_ownership.yml to allow larger files" + )) + } else { + report + } + }) +} + +/// 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()) +} + +/// [`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() + .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, DEFAULT_MAX_NODES).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_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(), DEFAULT_MAX_NODES).expect(yaml); + assert!(parsed.globs.is_empty(), "{yaml:?}"); + } + } + + #[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"; + 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); + } + + #[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 nested = |lists: usize| format!("owner: TeamA\nignored: {}{}\n", "[".repeat(lists), "]".repeat(lists)); + + // 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] + fn test_rejects_too_many_nodes() { + 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 { .. }))); + } + + #[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(", ")); + + 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().0, + 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:?}"); + } + + #[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}"); + } +} diff --git a/tests/yaml_max_nodes_test.rs b/tests/yaml_max_nodes_test.rs new file mode 100644 index 0000000..ab681f1 --- /dev/null +++ b/tests/yaml_max_nodes_test.rs @@ -0,0 +1,87 @@ +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; + +// 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"; 30_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 20000)"))); + 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"))); + // generate stops at whichever large file it reads first, but --from-codeowners reads only team files. + 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")) + .and(predicate::str::contains("raise `yaml_max_nodes` (now 20000)")), + ); + + 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(()) +}