From 805f6c97d4c7841c932cadfdd78261257c3c6a1b Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Mon, 24 Aug 2026 15:18:24 -0500 Subject: [PATCH 1/3] fix: normalize the paths callers supply to validate `validate ` compared caller-supplied paths against project-relative ones without reducing them to the same form first. What that costs depends on the shape of owned_globs, and both outcomes are wrong. Under directory-anchored globs (`{gems,ruby,...}/**/*.rb`), the mishandled path matches nothing, is dropped before any ownership query runs, and the command exits 0 having checked nothing. Silent, and in the unsafe direction: a pre-commit hook or CI job reports success on a file it never looked at. Under `**`-leading globs (`**/*.rb`), it survives the filter instead and is queried in the caller's spelling, which matches no CODEOWNERS entry, so a well-owned file is reported unowned. Three spellings hit this. `./ruby/app/x.rb` and `ruby/a/../x.rb` were never reduced at all. Absolute paths were reduced with strip_prefix against a root that need not agree with them about symlinks: cli.rs canonicalizes --project-root, so on macOS, where TMPDIR lives under /var, a symlink to /private/var, a caller passing the TMPDIR spelling fails to strip -- and a library caller building its own RunConfig (which is how the code_ownership gem calls in) can pass an unresolved root against a resolved path, the mirror image. Fixing only one side leaves the other failing exactly as silently, so the retry resolves both. Absolute paths were also echoed back in the caller's spelling rather than project-relative, since the raw string was what got reported. path_utils::project_relative resolves `.` and `..` lexically and reports failure rather than passing an unstrippable path through, which is how a /var/... path came to be compared against project-relative ones in the first place. Lexically, not by canonicalizing: the project walk records symlink paths rather than their targets, so resolving symlinks would produce paths matching no walked file. The first attempt uses the root as given, so relative paths -- the common case -- cost no syscalls, and the root is resolved once per run rather than per path. Paths that no longer exist are now skipped. A changeset that deletes a file lists it, so a deleted path reaches validate in normal use, and a deleted file cannot have an owner -- reporting it as unowned fails a commit for removing code. `gv ` did exactly that. The gem already filters its list by File.exist? before calling in, so this matches what its callers see and extends it to direct library callers. Only a definite "not there" skips: try_exists().unwrap_or(true) keeps a path whose status is unknown, because a visible error is investigable and a silent pass is not. Three tests asserted on valid_project/ruby/app/unowned.rb, which does not exist -- valid_project has to validate cleanly, so it ships no unowned file. They passed only because a nonexistent path was reported as unowned, meaning they covered typo handling while claiming to cover unowned files, and skipping nonexistent paths removes that accident. Repointed at invalid_project, which has a real one, and narrowed to assert the path and exit status rather than the category wording, so they do not depend on how the report is phrased. The new tests assert the mechanism rather than the outcome: that the report names the *normalized* path and does not echo the caller's spelling. Without that they cannot distinguish "checked correctly" from "mishandled and spuriously reported" -- an earlier draft of this file, written against invalid_project's `**` globs, passed against unfixed code for precisely that reason. Seven of the nine fail without this change; the two that pass are the plain-relative control and the outside-the-project skip, both of which already worked. Co-Authored-By: Claude Fable 5 --- src/cli.rs | 8 +- src/path_utils.rs | 96 ++++++++- src/runner.rs | 71 +++++-- tests/supplied_path_normalization_test.rs | 242 ++++++++++++++++++++++ tests/validate_files_test.rs | 27 ++- 5 files changed, 419 insertions(+), 25 deletions(-) create mode 100644 tests/supplied_path_normalization_test.rs diff --git a/src/cli.rs b/src/cli.rs index 1d0a7f9..202114b 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -39,7 +39,9 @@ enum Command { visible_alias = "v" )] Validate { - #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")] + #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks). Paths are \ + resolved relative to the project root; ones that no longer exist are skipped, so a \ + changeset that deletes files is not reported as unowned.")] files: Vec, }, @@ -47,7 +49,9 @@ enum Command { GenerateAndValidate { #[arg(long, short, default_value = "false", help = "Skip staging the CODEOWNERS file")] skip_stage: bool, - #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks)")] + #[arg(help = "Optional list of files to validate ownership for (fast mode for git hooks). Paths are \ + resolved relative to the project root; ones that no longer exist are skipped, so a \ + changeset that deletes files is not reported as unowned.")] files: Vec, }, diff --git a/src/path_utils.rs b/src/path_utils.rs index 230b1d1..f5962a8 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; /// Return `path` relative to `root` if possible; otherwise return `path` unchanged. pub fn relative_to<'a>(root: &'a Path, path: &'a Path) -> &'a Path { @@ -10,6 +10,58 @@ pub fn relative_to_buf(root: &Path, path: &Path) -> PathBuf { relative_to(root, path).to_path_buf() } +/// Reduce a caller-supplied `path` to the project-relative form that +/// [`crate::project::Project::relative_path`] produces for walked files. +/// +/// Unlike [`relative_to`], which passes an unstrippable path through unchanged, this +/// reports failure. A path that cannot be placed inside the project is not a path the +/// per-file checks can say anything about, and silently treating it as relative is how +/// `/var/...` came to be compared against project-relative paths and matched nothing. +/// +/// Purely lexical — no filesystem access, so it is safe on a path that no longer exists +/// (a deleted file in a changeset). `.` components are dropped and `..` pops the +/// preceding component, so `./a/b.rb` and `a/c/../b.rb` both reduce to `a/b.rb`. +/// +/// Returns `None` when `path` is absolute and does not lie under `root`, when it escapes +/// `root` via `..`, or when it *is* `root`. The absolute case is not necessarily final: +/// `cli.rs` canonicalizes `--project-root`, so on macOS a root of `/private/var/...` will +/// not strip a caller-supplied `/var/...`. A caller that gets `None` for an absolute path +/// should retry with a canonicalized copy. +pub fn project_relative(root: &Path, path: &Path) -> Option { + let relative = if path.is_absolute() { path.strip_prefix(root).ok()? } else { path }; + + let normalized = lexically_normalize(relative); + if normalized.as_os_str().is_empty() || normalized.starts_with("..") { + return None; + } + + Some(normalized) +} + +/// Resolve `.` and `..` without touching the filesystem. +/// +/// Deliberately lexical: canonicalizing would also resolve symlinks, and the project walk +/// records the symlink path rather than its target, so resolving here would produce a path +/// that matches no walked file. +fn lexically_normalize(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + // A `..` that cannot pop is retained, so the caller can detect the escape. + if !normalized.pop() { + normalized.push(Component::ParentDir); + } + } + other => normalized.push(other), + } + } + + normalized +} + #[cfg(test)] mod tests { use super::*; @@ -46,4 +98,46 @@ mod tests { let rel_buf = relative_to_buf(root, path); assert_eq!(rel_ref, rel_buf.as_path()); } + + #[test] + fn project_relative_passes_through_a_plain_relative_path() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_strips_a_leading_dot_slash() { + // `./a.rb` and `a.rb` name the same file, but only one of them used to match a + // walked project file -- the other was silently dropped by the owned_globs filter. + let rel = project_relative(Path::new("/proj"), Path::new("./ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_resolves_interior_parent_dirs() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/services/../app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_strips_the_root_from_an_absolute_path() { + let rel = project_relative(Path::new("/proj"), Path::new("/proj/ruby/app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn project_relative_rejects_an_absolute_path_outside_the_root() { + // The caller retries with a canonicalized copy; see `Runner::project_relative_path`. + assert_eq!(project_relative(Path::new("/private/proj"), Path::new("/proj/a.rb")), None); + } + + #[test] + fn project_relative_rejects_a_path_escaping_the_root() { + assert_eq!(project_relative(Path::new("/proj"), Path::new("../outside/a.rb")), None); + } + + #[test] + fn project_relative_rejects_the_root_itself() { + assert_eq!(project_relative(Path::new("/proj"), Path::new("/proj")), None); + } } diff --git a/src/runner.rs b/src/runner.rs index 5562979..e48ca40 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -146,26 +146,45 @@ impl Runner { let mut unowned_files = Vec::new(); let mut io_errors = Vec::new(); - // Filter files based on owned_globs and unowned_globs configuration - // Only validate files that match owned_globs and don't match unowned_globs - let filtered_paths: Vec = file_paths - .into_iter() - .filter(|file_path| { - // Convert to relative path for glob matching - let path = Path::new(file_path); - let relative_path = if path.is_absolute() { - path.strip_prefix(&self.run_config.project_root).unwrap_or(path) - } else { - path - }; - - // Mirror the filtering applied by ProjectBuilder when walking the project + // Normalize before anything else. A caller-supplied path has to be reduced to the + // project-relative form the rest of the pipeline speaks, or it silently matches + // nothing: `./ruby/app/x.rb`, and an absolute path that disagrees with the root + // about symlinks, were both dropped by the glob filter below, and the run then + // exited 0 having checked nothing -- a false pass in the unsafe direction. + // + // The canonical root is resolved once rather than per path, since only the retry + // inside `project_relative_path` needs it and that retry can fire for every path + // when a caller passes an absolute list. + let canonical_root = self.run_config.project_root.canonicalize().ok(); + + let relative_paths: Vec = file_paths + .iter() + .filter_map(|file_path| { + Self::project_relative_path(&self.run_config.project_root, canonical_root.as_deref(), Path::new(file_path)) + }) + // A path that no longer exists is dropped rather than reported. Changesets + // delete files routinely and `git diff --name-only` lists them, so reporting a + // deleted file as unowned fails a commit for removing code -- and a deleted + // file cannot have an owner. The wrapping `code_ownership` gem already filters + // its list by `File.exist?` before calling in; doing it here too covers callers + // that use the library directly. + // + // `unwrap_or(true)` because only a definite "this is not there" earns a silent + // skip. If the answer is unknown -- a permissions error, a broken symlink -- + // keep the path and let the check report it, because a visible error is + // investigable and a silent pass is not. + .filter(|relative_path| self.run_config.project_root.join(relative_path).try_exists().unwrap_or(true)) + // Mirror the filtering applied by ProjectBuilder when walking the project. + .filter(|relative_path| { matches_globs(relative_path, &self.config.owned_globs) && !matches_globs(relative_path, &self.config.unowned_globs) }) .collect(); debug_span!("per_file_query").in_scope(|| { - for file_path in filtered_paths { + for relative_path in relative_paths { + // Query with the normalized path rather than the caller's spelling, which + // made the query re-derive it using the same broken `strip_prefix`. + let file_path = relative_path.to_string_lossy().to_string(); match team_for_file_from_codeowners(&self.run_config, &file_path) { Ok(Some(_)) => {} Ok(None) => unowned_files.push(file_path), @@ -196,6 +215,28 @@ impl Runner { RunResult::default() } + /// Reduce a caller-supplied path to project-relative form. + /// + /// An absolute path only strips if it and the root agree about symlinks, and there is + /// no guarantee they do — `cli.rs` canonicalizes `--project-root`, but a library caller + /// building its own `RunConfig` (which is how the `code_ownership` gem calls in) does + /// not. So on macOS, where `TMPDIR` lives under `/var`, a symlink to `/private/var`, + /// *either* side can be the unresolved one, and in a symlinked checkout the same is + /// true generally. + /// + /// Hence the retry resolves both sides rather than just the path: fixing only the path + /// leaves the mirror-image case — a canonical path against an unresolved root — failing + /// exactly as silently. The first attempt uses the root as given, so the common case of + /// relative paths costs no syscalls at all. + fn project_relative_path(root: &Path, canonical_root: Option<&Path>, path: &Path) -> Option { + if let Some(relative) = crate::path_utils::project_relative(root, path) { + return Some(relative); + } + + let canonical_path = path.canonicalize().ok()?; + crate::path_utils::project_relative(canonical_root.unwrap_or(root), &canonical_path) + } + pub fn generate(&self, git_stage: bool) -> RunResult { let content = self.ownership.generate_file(); if let Some(parent) = &self.codeowners_file_path.parent() { diff --git a/tests/supplied_path_normalization_test.rs b/tests/supplied_path_normalization_test.rs new file mode 100644 index 0000000..a7b5beb --- /dev/null +++ b/tests/supplied_path_normalization_test.rs @@ -0,0 +1,242 @@ +//! Normalization of the paths a caller supplies to `validate` / `generate-and-validate`. +//! +//! A supplied path has to be reduced to the project-relative form the rest of the pipeline +//! speaks. When it is not, what happens depends on the shape of `owned_globs`, and both +//! outcomes are wrong: +//! +//! - **Directory-anchored globs** (`{gems,ruby,...}/**/*.rb`, as in `valid_project`): the +//! mishandled path matches nothing, is dropped before any ownership query runs, and the +//! command exits 0 having checked nothing. Silent, and in the unsafe direction — a hook +//! or CI job reports success on a file it never looked at. +//! - **`**`-leading globs** (`**/*.rb`, as in `invalid_project`): the mishandled path +//! survives the filter and is queried in the caller's spelling, which matches no +//! CODEOWNERS entry, so a perfectly well-owned file is reported unowned. +//! +//! Most tests here use `valid_project` and assert *failure* on a genuinely unowned file, +//! because under anchored globs a dropped path shows up as a pass. They also assert the +//! report names the **normalized** path rather than echoing the caller's spelling — without +//! that, a test cannot tell "checked correctly" from "mishandled and spuriously reported", +//! which is exactly how an earlier draft of this file passed against unfixed code. +//! +//! One test covers the `**`-glob direction, since the symptom there is the opposite. +//! +//! Path forms covered, against both states the project root can be in (resolved or not, +//! since `cli.rs` canonicalizes it but a library caller need not): +//! +//! - `./a/b.rb`, `a/c/../b.rb` -- leading `.`, interior `..` +//! - absolute, root and path agreeing about symlinks +//! - absolute, root resolved and path not +//! - absolute, path resolved and root not (library callers only) +//! - a deleted path, and a path outside the project -- both skipped, deliberately + +use assert_cmd::prelude::*; +use codeowners::runner::{self, RunConfig}; +use predicates::prelude::*; +use std::{error::Error, process::Command}; +use tempfile::TempDir; + +mod common; + +use common::*; + +/// The project-relative form every spelling below must reduce to. +const NORMALIZED: &str = "ruby/app/unowned.rb"; + +/// `valid_project` with a genuinely unowned file added. +/// +/// The fixture ships without one on purpose — `test_validate_with_no_files` requires it to +/// validate cleanly — so the file is injected here rather than committed. Its `owned_globs` +/// are directory-anchored, which is what makes a mishandled path get dropped, so that +/// asserting failure below is a real signal. +fn fixture_with_an_unowned_file() -> TempDir { + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/valid_project")); + std::fs::write(temp_dir.path().join(NORMALIZED), "# nobody owns this\n").expect("failed to write unowned file"); + git_add_all_files(temp_dir.path()); + temp_dir +} + +/// Assert `validate ` reached the ownership check and reported the file under its +/// normalized name. +fn assert_normalizes(spelling_from: impl Fn(&std::path::Path) -> String) -> Result<(), Box> { + let temp_dir = fixture_with_an_unowned_file(); + let project_root = temp_dir.path(); + let spelling = spelling_from(project_root); + + let output = Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(&spelling) + .output()?; + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!( + !output.status.success(), + "`validate {spelling}` exited 0 -- the path was dropped before any ownership query ran.\nstdout={stdout}" + ); + assert!( + stdout.contains(NORMALIZED), + "report does not name the normalized path `{NORMALIZED}`.\nstdout={stdout}" + ); + if spelling != NORMALIZED { + assert!( + !stdout.contains(&spelling), + "report echoes the caller's spelling `{spelling}` instead of normalizing it, \ + which means the path reached the query unnormalized.\nstdout={stdout}" + ); + } + + Ok(()) +} + +#[test] +fn test_plain_relative_path() -> Result<(), Box> { + // The control. If this fails, none of the others mean what they claim. + assert_normalizes(|_| NORMALIZED.to_string()) +} + +#[test] +fn test_dot_slash_prefixed_path() -> Result<(), Box> { + assert_normalizes(|_| format!("./{NORMALIZED}")) +} + +#[test] +fn test_path_with_an_interior_parent_dir() -> Result<(), Box> { + assert_normalizes(|_| "ruby/app/models/../unowned.rb".to_string()) +} + +#[test] +fn test_absolute_path_agreeing_with_the_root() -> Result<(), Box> { + assert_normalizes(|root| { + root.canonicalize() + .expect("temp dir should canonicalize") + .join(NORMALIZED) + .to_string_lossy() + .to_string() + }) +} + +#[test] +fn test_absolute_path_when_only_the_root_is_resolved() -> Result<(), Box> { + // `cli.rs` canonicalizes `--project-root`, so on macOS the root becomes + // `/private/var/...` while a caller passing the `TMPDIR` spelling supplies `/var/...`. + // `strip_prefix` then fails, the path stays absolute, and the glob filter drops it. + // Deliberately NOT canonicalized here -- that is the case under test. + assert_normalizes(|root| root.join(NORMALIZED).to_string_lossy().to_string()) +} + +#[test] +fn test_absolute_path_when_only_the_path_is_resolved() { + // The mirror image of the test above, and it has to go through the library API: the CLI + // always canonicalizes `--project-root`, so it cannot produce an unresolved root. A + // library caller can, and does -- the `code_ownership` gem builds its own `RunConfig`. + // + // Worth its own test because resolving only the supplied path fixes the case above + // while leaving this one failing exactly as silently. + let temp_dir = fixture_with_an_unowned_file(); + // Deliberately NOT canonicalized -- that is the point. + let project_root = temp_dir.path().to_path_buf(); + + let canonical_file = project_root.join(NORMALIZED).canonicalize().expect("injected file should exist"); + + let run_config = RunConfig { + project_root: project_root.clone(), + codeowners_file_path: Some(project_root.join(".github/CODEOWNERS")), + config_path: project_root.join("config/code_ownership.yml"), + no_cache: true, + executable_name: None, + }; + + let result = runner::validate(&run_config, vec![canonical_file.to_string_lossy().to_string()]); + + assert!( + result.validation_errors.iter().any(|error| error.contains(NORMALIZED)), + "an unowned file was silently skipped: root={} file={} errors={:?}", + project_root.display(), + canonical_file.display(), + result.validation_errors, + ); +} + +#[test] +fn test_owned_file_is_not_spuriously_reported_under_star_star_globs() -> Result<(), Box> { + // The other failure mode. `invalid_project`'s `owned_globs` are `**`-leading, so a + // mishandled path is not dropped by the filter -- it survives, gets queried in the + // caller's spelling, matches no CODEOWNERS entry, and a well-owned file is reported + // unowned. Same cause, opposite symptom, which is why it needs its own fixture. + // + // Uses `generate-and-validate` because that fixture ships an empty CODEOWNERS, so the + // file has to be generated into it before the query can find it. + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/invalid_project")); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + let codeowners_path = project_root.join("tmp/CODEOWNERS"); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--codeowners-file-path") + .arg(&codeowners_path) + .arg("--no-cache") + .arg("generate-and-validate") + .arg("./ruby/app/models/payroll.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_deleted_path_is_skipped() -> Result<(), Box> { + // A changeset that deletes a file lists it, so a deleted path reaches `validate` in + // normal use. A deleted file cannot have an owner, so reporting it as unowned fails a + // commit for removing code. + // + // This is the one case where dropping a supplied path silently is right -- unlike every + // other test here, there is no file left to check. Asserted rather than assumed, + // because "silently skipped" is otherwise exactly the bug this file is about. + // + // Uses `generate-and-validate` so the regenerated CODEOWNERS no longer carries the + // deleted file's stale entry, which would otherwise mask the behavior. + let temp_dir = setup_fixture_repo(std::path::Path::new("tests/fixtures/valid_project")); + let project_root = temp_dir.path(); + git_add_all_files(project_root); + + let codeowners_path = project_root.join("tmp/CODEOWNERS"); + std::fs::remove_file(project_root.join("ruby/app/models/payroll.rb"))?; + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(project_root) + .arg("--codeowners-file-path") + .arg(&codeowners_path) + .arg("--no-cache") + .arg("generate-and-validate") + .arg("ruby/app/models/payroll.rb") + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + +#[test] +fn test_path_outside_the_project_is_skipped_not_panicked_on() -> Result<(), Box> { + // A `..` that escapes the root cannot name a project file, so it is dropped rather than + // treated as relative. Pinned mainly so it stays a skip and not a panic. + let temp_dir = fixture_with_an_unowned_file(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(temp_dir.path()) + .arg("--no-cache") + .arg("validate") + .arg("../outside/the_project.rb") + .assert() + .success(); + + Ok(()) +} diff --git a/tests/validate_files_test.rs b/tests/validate_files_test.rs index 541b5bf..47f21fe 100644 --- a/tests/validate_files_test.rs +++ b/tests/validate_files_test.rs @@ -21,12 +21,22 @@ fn test_validate_with_owned_files() -> Result<(), Box> { #[test] fn test_validate_with_unowned_file() -> Result<(), Box> { + // `invalid_project`, not `valid_project`: this needs a file that genuinely has no + // owner, and `valid_project/ruby/app/unowned.rb` does not exist -- by design, since + // `test_validate_with_no_files` requires that fixture to validate cleanly. Pointed at + // the nonexistent path, this test passed only because a nonexistent path was reported + // as unowned, so it was really covering typo handling while claiming to cover unowned + // files. Now that a path which no longer exists is skipped, that accident is gone. + // `invalid_project/ruby/app/unowned.rb` is a real file with no owner. + // + // Asserts the path and the exit status, not the category wording, so it stays valid + // however the report is phrased. run_codeowners( - "valid_project", + "invalid_project", &["validate", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + predicate::str::contains("ruby/app/unowned.rb"), )?; Ok(()) @@ -34,12 +44,14 @@ fn test_validate_with_unowned_file() -> Result<(), Box> { #[test] fn test_validate_with_mixed_files() -> Result<(), Box> { + // One owned file and one genuinely unowned one; see `test_validate_with_unowned_file` + // for why this uses `invalid_project`. run_codeowners( - "valid_project", + "invalid_project", &["validate", "ruby/app/models/payroll.rb", "ruby/app/unowned.rb"], false, OutputStream::Stdout, - predicate::str::contains("ruby/app/unowned.rb").and(predicate::str::contains("Unowned")), + predicate::str::contains("ruby/app/unowned.rb"), )?; Ok(()) @@ -79,7 +91,9 @@ fn test_generate_and_validate_with_owned_files() -> Result<(), Box> { #[test] fn test_generate_and_validate_with_unowned_file() -> Result<(), Box> { - let fixture_root = std::path::Path::new("tests/fixtures/valid_project"); + // `invalid_project` for the same reason as `test_validate_with_unowned_file`: it holds + // a file that genuinely has no owner. + let fixture_root = std::path::Path::new("tests/fixtures/invalid_project"); let temp_dir = setup_fixture_repo(fixture_root); let project_root = temp_dir.path(); git_add_all_files(project_root); @@ -96,8 +110,7 @@ fn test_generate_and_validate_with_unowned_file() -> Result<(), Box> .arg("ruby/app/unowned.rb") .assert() .failure() - .stdout(predicate::str::contains("ruby/app/unowned.rb")) - .stdout(predicate::str::contains("Unowned")); + .stdout(predicate::str::contains("ruby/app/unowned.rb")); Ok(()) } From 1749a4f402552ca4a99859063944ec46db6657b8 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Mon, 24 Aug 2026 19:34:05 -0500 Subject: [PATCH 2/3] fix: resolve only the parent when retrying an absolute path Two findings from reviewing the previous commit, one of them a false pass it introduced. The retry canonicalized the whole supplied path, which follows a symlinked *file*. The project walk records the symlink path rather than its target -- the reason lexically_normalize is lexical in the first place, stated in its own doc comment two lines above the code that violated it. So an absolute path naming an unowned symlink was silently checked as its owned target and exited 0: validate ruby/app/models/link_unowned.rb -> exit 1 (correct) validate /tmp/proj/ruby/app/models/link_unowned.rb -> exit 0 (false pass) realpath(link_unowned.rb) = /private/tmp/proj/ruby/app/models/payroll.rb A false pass on a different file than the caller named, which is the exact failure class this branch exists to rule out. The retry now resolves the parent and re-attaches the file name, so the ancestor /var -> /private/var discrepancy is still fixed without following the leaf. A symlinked *ancestor* is still resolved, unavoidably -- that is the point in the /var case -- and the walk does not follow symlinked directories anyway, so such a path names no walked file under either spelling. Writing the invariant down was not enough to enforce it. There was no symlink test, so nothing caught the contradiction; there is one now, and it fails if the whole-path canonicalize is reintroduced. The same defect survived in codeowners_query::teams_for_files_from_codeowners, reached from public API as runner::teams_for_files_from_codeowners. It relativized with relative_to_buf, which passes an unstrippable path through unchanged, so a /var/... path against a /private/var/... root was looked up in the CODEOWNERS file as an absolute path, matched no entry, and came back unowned. Fixing validate while leaving the bulk-lookup entry point beside it would have made the branch's claim narrower than it reads. That one falls back to the path as given rather than dropping it, because the returned map is contracted to hold one entry per input and team_for_file_from_codeowners asserts on that. Note the keys were already the relativized form, not the caller's spelling, so they were inconsistent depending on whether strip_prefix happened to succeed; they are now consistently relative. The retry logic moves to path_utils::resolve_project_relative so both callers share it rather than growing a second copy. Adds two positive guards. Every other assertion in the file is that an unowned file gets reported, which would also hold if normalization mangled a path into some other unowned path; these pin that a well-owned file still resolves to itself and passes under `./` and interior `..` spellings. Co-Authored-By: Claude Fable 5 --- src/ownership/codeowners_query.rs | 18 +++-- src/path_utils.rs | 31 ++++++++- src/runner.rs | 26 +------ tests/supplied_path_normalization_test.rs | 85 ++++++++++++++++++++++- 4 files changed, 129 insertions(+), 31 deletions(-) diff --git a/src/ownership/codeowners_query.rs b/src/ownership/codeowners_query.rs index 562bb48..9dff3a7 100644 --- a/src/ownership/codeowners_query.rs +++ b/src/ownership/codeowners_query.rs @@ -10,15 +10,23 @@ pub(crate) fn teams_for_files_from_codeowners( team_file_globs: &[String], file_paths: &[String], ) -> Result>, String> { + // Normalize the same way `Runner::validate_files` does. This is reached from public API + // (`runner::teams_for_files_from_codeowners`) and had the same defect: + // `relative_to_buf` passes an unstrippable path through unchanged, so an absolute path + // that disagreed with `project_root` about symlinks -- a `/var/...` path against a + // `/private/var/...` root -- was looked up in the CODEOWNERS file *as an absolute path*, + // matched no entry, and came back unowned. + // + // Falls back to the path as given rather than dropping it, because the returned map is + // contracted to hold one entry per input and `team_for_file_from_codeowners` asserts on + // that. A path that cannot be placed inside the project has no owner, which is the + // honest answer for a lookup. + let canonical_root = project_root.canonicalize().ok(); let relative_file_paths: Vec = file_paths .iter() .map(Path::new) .map(|path| { - if path.is_absolute() { - crate::path_utils::relative_to_buf(project_root, path) - } else { - path.to_path_buf() - } + crate::path_utils::resolve_project_relative(project_root, canonical_root.as_deref(), path).unwrap_or_else(|| path.to_path_buf()) }) .collect(); diff --git a/src/path_utils.rs b/src/path_utils.rs index f5962a8..9497b31 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -38,6 +38,35 @@ pub fn project_relative(root: &Path, path: &Path) -> Option { Some(normalized) } +/// Like [`project_relative`], but consults the filesystem when the lexical attempt fails. +/// +/// An absolute path only strips if it and `root` agree about symlinks, and there is no +/// guarantee they do. `cli.rs` canonicalizes `--project-root`, but a library caller building +/// its own `RunConfig` (which is how the `code_ownership` gem calls in) does not. So on +/// macOS, where `TMPDIR` lives under `/var`, a symlink to `/private/var`, *either* side can +/// be the unresolved one, and in a symlinked checkout the same is true generally. Resolving +/// only one side leaves the other failing exactly as silently, so the retry resolves both. +/// +/// It resolves the **parent** and re-attaches the file name, rather than canonicalizing the +/// whole path. Canonicalizing the leaf would follow a symlinked *file*, and the project walk +/// records the symlink path rather than its target — so an absolute path naming a symlink +/// would be checked as a different file than the caller asked about, and pass or fail on +/// that file's ownership instead. A symlinked *ancestor* is still resolved, unavoidably: +/// that is the whole point in the `/var` case, and the walk does not follow symlinked +/// directories anyway, so such a path names no walked file under either spelling. +/// +/// `canonical_root` is the resolved `root`, passed in rather than computed so a caller +/// normalizing a whole changeset pays for it once instead of once per path. +pub fn resolve_project_relative(root: &Path, canonical_root: Option<&Path>, path: &Path) -> Option { + if let Some(relative) = project_relative(root, path) { + return Some(relative); + } + + let resolved = path.parent()?.canonicalize().ok()?.join(path.file_name()?); + + project_relative(canonical_root.unwrap_or(root), &resolved) +} + /// Resolve `.` and `..` without touching the filesystem. /// /// Deliberately lexical: canonicalizing would also resolve symlinks, and the project walk @@ -127,7 +156,7 @@ mod tests { #[test] fn project_relative_rejects_an_absolute_path_outside_the_root() { - // The caller retries with a canonicalized copy; see `Runner::project_relative_path`. + // The caller retries with the parent resolved; see `resolve_project_relative`. assert_eq!(project_relative(Path::new("/private/proj"), Path::new("/proj/a.rb")), None); } diff --git a/src/runner.rs b/src/runner.rs index e48ca40..a5c30cf 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -153,14 +153,14 @@ impl Runner { // exited 0 having checked nothing -- a false pass in the unsafe direction. // // The canonical root is resolved once rather than per path, since only the retry - // inside `project_relative_path` needs it and that retry can fire for every path + // inside `resolve_project_relative` needs it and that retry can fire for every path // when a caller passes an absolute list. let canonical_root = self.run_config.project_root.canonicalize().ok(); let relative_paths: Vec = file_paths .iter() .filter_map(|file_path| { - Self::project_relative_path(&self.run_config.project_root, canonical_root.as_deref(), Path::new(file_path)) + crate::path_utils::resolve_project_relative(&self.run_config.project_root, canonical_root.as_deref(), Path::new(file_path)) }) // A path that no longer exists is dropped rather than reported. Changesets // delete files routinely and `git diff --name-only` lists them, so reporting a @@ -215,28 +215,6 @@ impl Runner { RunResult::default() } - /// Reduce a caller-supplied path to project-relative form. - /// - /// An absolute path only strips if it and the root agree about symlinks, and there is - /// no guarantee they do — `cli.rs` canonicalizes `--project-root`, but a library caller - /// building its own `RunConfig` (which is how the `code_ownership` gem calls in) does - /// not. So on macOS, where `TMPDIR` lives under `/var`, a symlink to `/private/var`, - /// *either* side can be the unresolved one, and in a symlinked checkout the same is - /// true generally. - /// - /// Hence the retry resolves both sides rather than just the path: fixing only the path - /// leaves the mirror-image case — a canonical path against an unresolved root — failing - /// exactly as silently. The first attempt uses the root as given, so the common case of - /// relative paths costs no syscalls at all. - fn project_relative_path(root: &Path, canonical_root: Option<&Path>, path: &Path) -> Option { - if let Some(relative) = crate::path_utils::project_relative(root, path) { - return Some(relative); - } - - let canonical_path = path.canonicalize().ok()?; - crate::path_utils::project_relative(canonical_root.unwrap_or(root), &canonical_path) - } - pub fn generate(&self, git_stage: bool) -> RunResult { let content = self.ownership.generate_file(); if let Some(parent) = &self.codeowners_file_path.parent() { diff --git a/tests/supplied_path_normalization_test.rs b/tests/supplied_path_normalization_test.rs index a7b5beb..41e8e8a 100644 --- a/tests/supplied_path_normalization_test.rs +++ b/tests/supplied_path_normalization_test.rs @@ -18,7 +18,10 @@ //! that, a test cannot tell "checked correctly" from "mishandled and spuriously reported", //! which is exactly how an earlier draft of this file passed against unfixed code. //! -//! One test covers the `**`-glob direction, since the symptom there is the opposite. +//! One test covers the `**`-glob direction, since the symptom there is the opposite. Two +//! more assert an *owned* file still passes under each odd spelling — without those, every +//! assertion here would also hold if normalization mangled one path into some other unowned +//! path. //! //! Path forms covered, against both states the project root can be in (resolved or not, //! since `cli.rs` canonicalizes it but a library caller need not): @@ -27,6 +30,7 @@ //! - absolute, root and path agreeing about symlinks //! - absolute, root resolved and path not //! - absolute, path resolved and root not (library callers only) +//! - absolute, naming a symlinked *file* -- must check the symlink, not its target //! - a deleted path, and a path outside the project -- both skipped, deliberately use assert_cmd::prelude::*; @@ -55,6 +59,27 @@ fn fixture_with_an_unowned_file() -> TempDir { temp_dir } +/// Assert `validate ` succeeds, i.e. the path reached the check *and* resolved to +/// a file that really is owned. +/// +/// The counterpart to `assert_normalizes`: those tests would still pass if normalization +/// mangled a path into some *other* unowned path, and these would not. +fn assert_owned_file_passes(spelling: &str) -> Result<(), Box> { + let temp_dir = fixture_with_an_unowned_file(); + + Command::cargo_bin("codeowners")? + .arg("--project-root") + .arg(temp_dir.path()) + .arg("--no-cache") + .arg("validate") + .arg(spelling) + .assert() + .success() + .stdout(predicate::eq("")); + + Ok(()) +} + /// Assert `validate ` reached the ownership check and reported the file under its /// normalized name. fn assert_normalizes(spelling_from: impl Fn(&std::path::Path) -> String) -> Result<(), Box> { @@ -159,6 +184,64 @@ fn test_absolute_path_when_only_the_path_is_resolved() { ); } +#[test] +fn test_owned_file_passes_with_a_dot_slash_prefix() -> Result<(), Box> { + // A positive guard. Every assertion above is that an *unowned* file gets reported, which + // would also hold if normalization mangled the path into some other unowned path. This + // pins that a well-owned file still resolves to itself and passes. + assert_owned_file_passes("./ruby/app/models/payroll.rb") +} + +#[test] +fn test_owned_file_passes_with_an_interior_parent_dir() -> Result<(), Box> { + assert_owned_file_passes("ruby/app/payments/../models/payroll.rb") +} + +#[test] +fn test_absolute_path_to_a_symlink_names_the_symlink_not_its_target() { + // Regression guard. The retry used to canonicalize the whole supplied path, which + // follows a symlinked *file*, so an absolute path naming a symlink was checked as its + // target -- a different file than the caller asked about. The retry now resolves only + // the parent and re-attaches the file name, fixing the ancestor `/var` -> + // `/private/var` discrepancy without following the leaf. + // + // Both the symlink and its target are unowned here, and the assertion is on *which path + // the report names* rather than on pass/fail. An earlier version pointed the symlink at + // an owned file and asserted failure, which was fixture-coupled and wrong: reading + // through a symlink sees the target's contents, so once ownership is resolved through + // the mappers rather than by reading CODEOWNERS back, the symlink genuinely inherits the + // target's `@team` annotation and is owned. Naming the path sidesteps that entirely. + // + // The symlink is created after `setup_fixture_repo` because that helper copies with + // `fs::copy`, which would follow it and write a regular file instead. + let temp_dir = fixture_with_an_unowned_file(); + let project_root = temp_dir.path(); + + let link = project_root.join("ruby/app/link_to_unowned.rb"); + std::os::unix::fs::symlink("unowned.rb", &link).expect("failed to create symlink"); + git_add_all_files(project_root); + + // Deliberately NOT canonicalized, so the retry fires. + let absolute = link.to_string_lossy().to_string(); + + let output = Command::cargo_bin("codeowners") + .expect("binary") + .arg("--project-root") + .arg(project_root) + .arg("--no-cache") + .arg("validate") + .arg(&absolute) + .output() + .expect("run"); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert!( + stdout.contains("link_to_unowned.rb"), + "the report names the symlink's target instead of the symlink the caller asked \ + about, so the retry followed the leaf.\nstdout={stdout}" + ); +} + #[test] fn test_owned_file_is_not_spuriously_reported_under_star_star_globs() -> Result<(), Box> { // The other failure mode. `invalid_project`'s `owned_globs` are `**`-leading, so a From 9e204f175ea897843b1150531e145563619a77c7 Mon Sep 17 00:00:00 2001 From: Perry Hertler Date: Tue, 25 Aug 2026 08:34:22 -0500 Subject: [PATCH 3/3] fix: keep a retained `..` from cancelling its own escape Addresses review of the previous commit, plus a bug found while checking it. lexically_normalize retained a leading `..` so the caller could detect an escape, but pop() does not distinguish that retained `..` from a real component, so a later one popped it. `../../a` therefore cancelled its own escape and came out as `a`: a path plainly outside the project was reported as though it named a file inside it. validate ../ruby/app/unowned.rb -> exit 0 (correctly out of scope) validate ../../ruby/app/unowned.rb -> exit 1 "Unowned files detected: ruby/app/unowned.rb" Same class as the symlink bug the previous commit fixed -- answering about a different file than the caller named -- and the single-`..` case passing made it look handled. A `..` is now only allowed to pop a real component. Restrict the filesystem retry to absolute paths. A relative path is interpreted against the project root by contract, which is what --help promises, and the lexical pass is the whole of that interpretation -- so failure means it escapes the root. Retrying resolved it against the process CWD instead, quietly switching interpretation frames: identical arguments would mean different files depending on where the command ran from. It also spent a syscall per path to reach that wrong answer. Reported by review. Gate the symlink test behind #[cfg(unix)]. std::os::unix::fs::symlink has no portable equivalent and would fail to compile on Windows. This crate ships only macOS and Linux artifacts, so the test is gated rather than made portable, which keeps `cargo test` compiling everywhere. Reported by review. Not changed: review also flagged the `\` line continuations in cli.rs help text as leaving runs of literal spaces. Rust's string-continuation escape strips the newline *and* the following indentation, so it does not. Verified against the rendered output -- the description contains zero interior multi-space runs; the only gap is clap's own column alignment between the argument name and its text. Co-Authored-By: Claude Fable 5 --- src/path_utils.rs | 50 ++++++++++++++++++++++- tests/supplied_path_normalization_test.rs | 8 +++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/path_utils.rs b/src/path_utils.rs index 9497b31..d58f126 100644 --- a/src/path_utils.rs +++ b/src/path_utils.rs @@ -62,6 +62,16 @@ pub fn resolve_project_relative(root: &Path, canonical_root: Option<&Path>, path return Some(relative); } + // Only an absolute path can be rescued. A relative path is interpreted against the + // project root by contract -- that is what `--help` promises -- and the lexical pass is + // the whole of that interpretation, so failure means it escapes the root. Retrying would + // resolve it against the process CWD instead, quietly switching interpretation frames: + // the same arguments would then mean different files depending on where the command was + // run from. It also spends a syscall per path to reach that wrong answer. + if !path.is_absolute() { + return None; + } + let resolved = path.parent()?.canonicalize().ok()?.join(path.file_name()?); project_relative(canonical_root.unwrap_or(root), &resolved) @@ -80,7 +90,13 @@ fn lexically_normalize(path: &Path) -> PathBuf { Component::CurDir => {} Component::ParentDir => { // A `..` that cannot pop is retained, so the caller can detect the escape. - if !normalized.pop() { + // + // A retained `..` must never itself be popped by a later one: `pop()` does + // not distinguish it from a real component, so `../../a` cancelled its own + // escape and came out as `a` -- reporting an out-of-project path as though + // it named a file inside the project. + let escaped = matches!(normalized.components().next_back(), Some(Component::ParentDir)); + if escaped || !normalized.pop() { normalized.push(Component::ParentDir); } } @@ -165,6 +181,38 @@ mod tests { assert_eq!(project_relative(Path::new("/proj"), Path::new("../outside/a.rb")), None); } + #[test] + fn project_relative_rejects_a_path_escaping_via_repeated_parent_dirs() { + // `pop()` does not distinguish a retained `..` from a real component, so this used to + // cancel its own escape and come out as `ruby/app/a.rb` -- an out-of-project path + // silently reported as though it named a file inside the project. + assert_eq!(project_relative(Path::new("/proj"), Path::new("../../ruby/app/a.rb")), None); + assert_eq!(project_relative(Path::new("/proj"), Path::new("../../../a.rb")), None); + } + + #[test] + fn project_relative_rejects_a_path_that_climbs_back_out() { + // Interior `..` still pops normally; the escape only has to survive once it starts. + assert_eq!(project_relative(Path::new("/proj"), Path::new("ruby/../../a.rb")), None); + } + + #[test] + fn project_relative_pops_interior_parent_dirs_without_escaping() { + let rel = project_relative(Path::new("/proj"), Path::new("ruby/app/models/../../app/a.rb")); + assert_eq!(rel, Some(PathBuf::from("ruby/app/a.rb"))); + } + + #[test] + fn resolve_project_relative_does_not_touch_the_filesystem_for_a_relative_path() { + // A relative path is project-root-relative by contract, so the lexical pass is the + // whole interpretation. Retrying would resolve it against the process CWD, making + // the same arguments mean different files depending on where the command was run. + assert_eq!( + resolve_project_relative(Path::new("/proj"), Some(Path::new("/proj")), Path::new("../outside/a.rb")), + None + ); + } + #[test] fn project_relative_rejects_the_root_itself() { assert_eq!(project_relative(Path::new("/proj"), Path::new("/proj")), None); diff --git a/tests/supplied_path_normalization_test.rs b/tests/supplied_path_normalization_test.rs index 41e8e8a..9c215ba 100644 --- a/tests/supplied_path_normalization_test.rs +++ b/tests/supplied_path_normalization_test.rs @@ -197,6 +197,10 @@ fn test_owned_file_passes_with_an_interior_parent_dir() -> Result<(), Box Result<(), Box> { #[test] fn test_path_outside_the_project_is_skipped_not_panicked_on() -> Result<(), Box> { // A `..` that escapes the root cannot name a project file, so it is dropped rather than - // treated as relative. Pinned mainly so it stays a skip and not a panic. + // treated as relative. Pinned mainly so it stays a skip and not a panic -- and, since the + // filesystem retry is now gated to absolute paths, so that it stays a skip regardless of + // the process CWD. let temp_dir = fixture_with_an_unowned_file(); Command::cargo_bin("codeowners")?