From e99cfc6e3fd14b0cc4fa85fa4d31079663e5279b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 15 Sep 2026 11:39:07 +0000 Subject: [PATCH 1/6] Find a report's files when its paths carry a base the CLI's do not A report's paths are relative to whatever directory the customer's scanner ran from, which is not necessarily where `corgea upload` runs. The CLI resolved each path verbatim against the process directory, so when the two disagreed by a leading prefix -- the scanner ran above the directory the CLI is invoked in, or recorded build-agent paths such as C:/jenkins/workspace//src/App.java -- the first file missed and the command exited 1 without sending anything, even though every file was right there. find_report_path_prefix() is a port of Fusion's fusion/util/report_paths.py, which solves the same problem for reports whose source arrives as a zip: sample the report's paths and, when none resolve as written, drop one directory at a time from the prefix they all share until the remainder resolves. Accepting a prefix requires corroboration -- candidates come only from directories every sampled path agrees on, and a lone hit that leaves a bare basename is refused, since common basenames repeat across a tree and uploading one file's contents for a finding about another is worse than uploading nothing. Only the file the CLI reads is rebased. `path=` keeps the path the report wrote, because the engine matches the report against the uploaded file_repo_path and never sees this working tree; rebasing it would trade one kind of miss for another. For the same reason the search also honours a path that resolves as written, absolute ones included, so a report generated on this machine is untouched rather than re-pointed at a same-named file in the working tree. The prefix is settled before corgea.yaml is merged in: policy files are found by walking the project, so one of them counting as a path that already resolves would call off the search for a report that matches nothing. --- src/main.rs | 1 + src/scan.rs | 168 ++++++- src/scanners/report_paths.rs | 428 ++++++++++++++++++ tests/cloud_commands_e2e/main.rs | 1 + .../cloud_commands_e2e/upload_report_paths.rs | 113 +++++ 5 files changed, 693 insertions(+), 18 deletions(-) create mode 100644 src/scanners/report_paths.rs create mode 100644 tests/cloud_commands_e2e/upload_report_paths.rs diff --git a/src/main.rs b/src/main.rs index 9fb413e..335b270 100644 --- a/src/main.rs +++ b/src/main.rs @@ -16,6 +16,7 @@ mod scanners { pub mod blast; pub mod fortify; pub mod parsers; + pub mod report_paths; } mod utils { pub mod api; diff --git a/src/scan.rs b/src/scan.rs index 5d486ab..3823ff8 100644 --- a/src/scan.rs +++ b/src/scan.rs @@ -1,6 +1,7 @@ use crate::cicd::*; use crate::log::debug; use crate::scanners::parsers::ScanParserFactory; +use crate::scanners::report_paths; use crate::{utils, Config}; use reqwest::header; use reqwest::Method; @@ -58,14 +59,46 @@ fn find_corgea_policy_files(root: &Path) -> Vec { found } -fn merge_corgea_policy_files(mut paths: Vec, root: &Path) -> Vec { +/// One file to send to `code-upload`. +struct SourceUpload { + /// Sent as `path=`, exactly as the report wrote it. The engine matches the + /// report's paths against this, so it stays as written even when the file + /// itself was found somewhere else. + report_path: String, + /// Where the file actually is on this machine. + local_path: PathBuf, +} + +/// Pair every path a report names with the file to read for it, and append the +/// repo's `corgea.yaml` policy files. +/// +/// The prefix search runs on the report's paths alone, before the policy files +/// are added: those are found by walking `root`, so they always resolve, and +/// one of them counting as a path that "already resolves" would call off the +/// search for a report that matches nothing. +fn plan_source_uploads(root: &Path, report_paths: &[String]) -> Vec { + let prefix = report_paths::find_report_path_prefix(root, report_paths); + + let mut uploads: Vec = report_paths + .iter() + .map(|path| SourceUpload { + report_path: path.clone(), + local_path: report_paths::local_report_path(root, path, &prefix), + }) + .collect(); + for yaml in find_corgea_policy_files(root) { - if !paths.iter().any(|path| path == &yaml) { - debug(&format!("Including repo policy file: {yaml}")); - paths.push(yaml); + if uploads.iter().any(|upload| upload.report_path == yaml) { + continue; } + debug(&format!("Including repo policy file: {yaml}")); + uploads.push(SourceUpload { + local_path: root.join(&yaml), + report_path: yaml, + }); } - paths + + uploads } pub fn run_command(base_cmd: &String, mut command: Command) -> String { @@ -281,7 +314,7 @@ pub fn upload_scan( project_name: Option, ) -> Option { let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")); - let paths = merge_corgea_policy_files(paths, &cwd); + let uploads = plan_source_uploads(&cwd, &paths); let in_ci = running_in_ci(); let ci_platform = which_ci(); let github_env_vars = get_github_env_vars(); @@ -331,11 +364,23 @@ pub fn upload_scan( let mut upload_error_count = 0; let mut platform_declined = false; - 'files: for path in &paths { - if !Path::new(&path).exists() { + 'files: for upload in &uploads { + let path = &upload.report_path; + let fp = upload.local_path.as_path(); + + if !fp.exists() { + // Name where the file was looked for whenever that is not the path + // in the report, so a prefix that resolved most of the report but + // not this path is visible rather than mystifying. + let looked_in = if fp.as_os_str() == path.as_str() { + String::new() + } else { + format!(" (looked for it at '{}')", fp.display()) + }; log::error!( - "Required file {} not found which is required for the scan, exiting.", - path + "Required file {}{} not found which is required for the scan, exiting.", + path, + looked_in ); std::process::exit(1); } @@ -349,7 +394,6 @@ pub fn upload_scan( base_url, api_base, run_id, path ); debug(&format!("Uploading file: {}", path)); - let fp = Path::new(&path); let mut attempts = 0; let mut success = false; @@ -419,7 +463,7 @@ pub fn upload_scan( // Everything the aborted walk never attempted still counts as unsent, or // the closing summary would report one failure for a whole skipped tree. if platform_declined { - let distinct: HashSet<&String> = paths.iter().collect(); + let distinct: HashSet<&String> = uploads.iter().map(|upload| &upload.report_path).collect(); let unsent = distinct.len() - uploaded_paths.len(); upload_error_count += unsent; log::warn!( @@ -788,21 +832,109 @@ mod tests { ); } + fn planned(root: &Path, report_paths: &[&str]) -> Vec<(String, String)> { + plan_source_uploads( + root, + &report_paths + .iter() + .map(|path| path.to_string()) + .collect::>(), + ) + .into_iter() + .map(|upload| { + ( + upload.report_path, + upload.local_path.to_string_lossy().into_owned(), + ) + }) + .collect() + } + #[test] - fn merge_corgea_policy_files_appends_missing_and_skips_duplicates() { + fn plan_source_uploads_appends_policy_files_and_skips_duplicates() { let root = tempfile::tempdir().unwrap(); write_policy(&root.path().join("corgea.yaml")); + let policy = root + .path() + .join("corgea.yaml") + .to_string_lossy() + .into_owned(); assert_eq!( - merge_corgea_policy_files(vec!["src/source.py".into()], root.path()), - vec!["src/source.py".to_string(), "corgea.yaml".to_string()] + planned(root.path(), &["src/source.py"]), + vec![ + ("src/source.py".to_string(), "src/source.py".to_string()), + ("corgea.yaml".to_string(), policy.clone()), + ] + ); + assert_eq!( + planned(root.path(), &["src/source.py", "corgea.yaml"]), + vec![ + ("src/source.py".to_string(), "src/source.py".to_string()), + ("corgea.yaml".to_string(), "corgea.yaml".to_string()), + ] + ); + } + + /// Only the file read from disk is rebased. `path=` keeps the report's own + /// path, because that is what the engine matches the report against. + #[test] + fn plan_source_uploads_rebases_the_local_file_but_not_the_uploaded_path() { + let root = tempfile::tempdir().unwrap(); + for path in ["src/a.py", "src/b.py"] { + std::fs::create_dir_all(root.path().join(path).parent().unwrap()).unwrap(); + std::fs::write(root.path().join(path), "x = 1\n").unwrap(); + } + + assert_eq!( + planned( + root.path(), + &["/builds/acme/repo/src/a.py", "/builds/acme/repo/src/b.py"] + ), + vec![ + ( + "/builds/acme/repo/src/a.py".to_string(), + root.path().join("src/a.py").to_string_lossy().into_owned(), + ), + ( + "/builds/acme/repo/src/b.py".to_string(), + root.path().join("src/b.py").to_string_lossy().into_owned(), + ), + ] + ); + } + + /// A policy file found by walking the root always exists, so letting one + /// into the prefix search would report the whole report as already + /// matching and call the search off. + #[test] + fn plan_source_uploads_settles_the_prefix_before_adding_policy_files() { + let root = tempfile::tempdir().unwrap(); + write_policy(&root.path().join("corgea.yaml")); + for path in ["src/a.py", "src/b.py"] { + std::fs::create_dir_all(root.path().join(path).parent().unwrap()).unwrap(); + std::fs::write(root.path().join(path), "x = 1\n").unwrap(); + } + + let plan = planned(root.path(), &["ci/src/a.py", "ci/src/b.py"]); + + assert_eq!( + plan[0], + ( + "ci/src/a.py".to_string(), + root.path().join("src/a.py").to_string_lossy().into_owned(), + ) ); assert_eq!( - merge_corgea_policy_files( - vec!["src/source.py".into(), "corgea.yaml".into()], + plan[2], + ( + "corgea.yaml".to_string(), root.path() + .join("corgea.yaml") + .to_string_lossy() + .into_owned(), ), - vec!["src/source.py".to_string(), "corgea.yaml".to_string()] + "the policy file is read from the root, never through the report's prefix" ); } } diff --git a/src/scanners/report_paths.rs b/src/scanners/report_paths.rs new file mode 100644 index 0000000..73aa3f3 --- /dev/null +++ b/src/scanners/report_paths.rs @@ -0,0 +1,428 @@ +//! Match a third-party report's file paths to the files on this machine. +//! +//! A report's paths are relative to whatever directory the customer's scanner +//! ran from, which is not necessarily where `corgea upload` runs. When the two +//! disagree by a leading prefix -- the scanner ran above the directory the CLI +//! is invoked in, or it recorded build-agent paths such as +//! `C:/jenkins/workspace//src/App.java` -- every lookup misses and the +//! upload aborts on the first file even though every file is right there. +//! +//! [`find_report_path_prefix`] settles that prefix once per report by dropping +//! one directory at a time from the prefix the report's paths share, and +//! [`local_report_path`] applies it. This is a port of Fusion's +//! `fusion/util/report_paths.py`, which does the same for reports whose source +//! arrives as a zip. +//! +//! Only the file the CLI *reads* is rebased. The path a file is uploaded under +//! stays exactly as the report wrote it, because the engine matches the report +//! against that path and never sees the local working tree. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +/// Report paths sampled when matching a report to the working tree. The offset +/// between the two is a property of the report as a whole, so a sample settles +/// it; a 50k-finding report should not cost a stat call per finding per +/// candidate prefix to reach the same answer. +const MAX_REPORT_PATHS_SAMPLED: usize = 200; + +/// Max leading directories dropped from report paths. Report paths can be +/// absolute (a Windows Checkmarx path, a Fortify `SourceBasePath`), so the +/// search needs a ceiling. +const MAX_REPORT_PATH_PREFIX_DEPTH: usize = 12; + +/// One report path in both the form the report wrote and the form that can be +/// joined onto the working tree. +struct Sampled { + /// Exactly as the report wrote it, which is how the CLI resolves paths + /// when no prefix is dropped. + raw: String, + /// Slash-normalized and relative, for joining onto the working tree. + relative: String, +} + +/// Report paths may be Windows-style or rooted at the scanner's base path. +fn normalize(path: &str) -> String { + path.replace('\\', "/").trim_start_matches('/').to_string() +} + +/// Distinct report paths to settle the prefix from, capped. +/// +/// Deduplicating before the cap means the cap counts distinct paths: a report +/// that records one finding per line of the same file would otherwise spend the +/// whole sample on a single path. +fn sample_paths(report_paths: &[String]) -> Vec { + let mut sample = Vec::new(); + let mut seen = HashSet::new(); + + for raw in report_paths { + if sample.len() >= MAX_REPORT_PATHS_SAMPLED { + break; + } + let relative = normalize(raw); + // A `..` segment cannot be resolved against the working tree without + // leaving it, and a prefix derived from one would rebase the whole + // report onto somewhere outside the project. + if relative.is_empty() || relative.split('/').any(|segment| segment == "..") { + continue; + } + if seen.insert(relative.clone()) { + sample.push(Sampled { + raw: raw.clone(), + relative, + }); + } + } + + sample +} + +/// The leading directories of `path`, dropping its file name. +fn leading_dirs(path: &str) -> Vec<&str> { + let mut dirs: Vec<&str> = path.split('/').collect(); + dirs.pop(); + dirs +} + +/// Leading directories every path agrees on. +/// +/// Candidate prefixes are built from these, so one path's coincidental suffix +/// can never define the offset for the whole report. +fn shared_dirs<'a>(paths: &[&'a str]) -> Vec<&'a str> { + let mut shared = match paths.first() { + Some(first) => leading_dirs(first), + None => return Vec::new(), + }; + + for path in &paths[1..] { + let dirs = leading_dirs(path); + let agreed = shared + .iter() + .zip(dirs.iter()) + .take_while(|(shared_dir, dir)| shared_dir == dir) + .count(); + shared.truncate(agreed); + } + + shared +} + +/// Return the leading prefix to drop from a report's paths, or `""`. +/// +/// Returns `""` as soon as any sampled path resolves as written, so a report +/// that already matches this working tree is left alone. Otherwise one +/// directory at a time is dropped from the shared prefix, shallowest first, and +/// the first prefix whose remainder resolves under `root` wins. +pub fn find_report_path_prefix(root: &Path, report_paths: &[String]) -> String { + let sample = sample_paths(report_paths); + + if sample.is_empty() { + return String::new(); + } + + // The path as the report wrote it is checked alongside the relativized + // form, not just the latter: a report generated on this machine can carry + // an absolute path that resolves, and dropping a prefix from it would read + // a same-named file out of the working tree instead of the file the finding + // is about. + if sample + .iter() + .any(|path| root.join(&path.relative).is_file() || Path::new(&path.raw).is_file()) + { + return String::new(); + } + + let relatives: Vec<&str> = sample.iter().map(|path| path.relative.as_str()).collect(); + let shared = shared_dirs(&relatives); + + for depth in 1..=shared.len().min(MAX_REPORT_PATH_PREFIX_DEPTH) { + let prefix = format!("{}/", shared[..depth].join("/")); + let hits: Vec<&str> = relatives + .iter() + .filter_map(|path| path.strip_prefix(prefix.as_str())) + .filter(|remainder| root.join(remainder).is_file()) + .collect(); + + // A real offset resolves the report broadly. A lone hit that is also a + // bare basename is more likely a same-named file elsewhere in the tree + // than evidence of the prefix, and accepting it would upload one file's + // contents for a finding about another. + if hits.len() >= 2 || (hits.len() == 1 && hits[0].contains('/')) { + log::warn!( + "The report's paths are not relative to this directory. Dropping '{}' from them resolves {} of {} sampled path(s); uploading those files under the paths the report uses.", + prefix, + hits.len(), + relatives.len() + ); + return prefix; + } + } + + String::new() +} + +/// Rebase one report path onto this working tree. +pub fn strip_report_prefix(path: &str, prefix: &str) -> String { + if prefix.is_empty() || path.is_empty() { + return path.to_string(); + } + + let normalized = normalize(path); + + match normalized.strip_prefix(prefix) { + Some(remainder) => remainder.to_string(), + None => path.to_string(), + } +} + +/// Where to read a report's file from on this machine. +/// +/// Without a prefix this is the path as the report wrote it, which is how the +/// CLI has always resolved it, so a report that already matches is untouched. +/// With one, the remainder is joined onto `root`; a `..` in it falls back to the +/// report's own path, because the prefix search only samples paths and an +/// unsampled one must not be able to walk out of the project. +pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { + let stripped = strip_report_prefix(report_path, prefix); + + if stripped == report_path || stripped.split('/').any(|segment| segment == "..") { + return PathBuf::from(report_path); + } + + root.join(stripped) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Lay out a working tree and return it as a root. + fn tree(root: &Path, paths: &[&str]) -> PathBuf { + for path in paths { + let full_path = root.join(path); + std::fs::create_dir_all(full_path.parent().unwrap()).unwrap(); + std::fs::write(&full_path, "export const a = 1;").unwrap(); + } + root.to_path_buf() + } + + fn report(paths: &[&str]) -> Vec { + paths.iter().map(|path| path.to_string()).collect() + } + + #[test] + fn no_prefix_when_paths_already_resolve() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/index.ts", "src/app.ts"]); + + assert_eq!( + find_report_path_prefix(&root, &report(&["src/index.ts", "src/app.ts"])), + "" + ); + } + + /// The scan ran above the directory `corgea upload` runs in, so every + /// report path is prefixed with that directory's own name. + #[test] + fn drops_the_directory_the_report_repeats() { + let root = tempfile::tempdir().unwrap(); + let root = tree( + root.path(), + &["proj (3)/src/index.ts", "proj (3)/src/app.ts"], + ); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&[ + "Downloads/proj (3)/src/index.ts", + "Downloads/proj (3)/src/app.ts", + ]) + ), + "Downloads/" + ); + } + + /// A Fortify SourceBasePath or an absolute Checkmarx path: none of the + /// prefix's directories exist here, so it can only be dropped. + #[test] + fn drops_a_multi_segment_prefix_absent_from_the_working_tree() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/a.cs", "src/b.cs"]); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&["C:/build/proj/src/a.cs", "C:/build/proj/src/b.cs"]) + ), + "C:/build/proj/" + ); + } + + #[test] + fn normalizes_windows_separators_before_searching() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/a.cs", "src/b.cs"]); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&["C:\\build\\proj\\src\\a.cs", "C:\\build\\proj\\src\\b.cs"]) + ), + "C:/build/proj/" + ); + } + + #[test] + fn stops_at_the_shallowest_prefix_that_resolves() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["proj/src/a.ts", "proj/src/b.ts", "src/a.ts"]); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&["build/proj/src/a.ts", "build/proj/src/b.ts"]) + ), + "build/" + ); + } + + #[test] + fn no_prefix_when_nothing_resolves() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/index.ts"]); + + for paths in [ + report(&["src/Gone.ts", "src/Missing.ts"]), + report(&[]), + report(&["", "/"]), + ] { + assert_eq!(find_report_path_prefix(&root, &paths), ""); + } + } + + #[test] + fn traversal_paths_are_never_sampled() { + let root = tempfile::tempdir().unwrap(); + let outside = root.path().join("outside"); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(outside.join("secret.txt"), "top-secret").unwrap(); + let root = tree(&root.path().join("repo"), &["src/index.ts"]); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&["../outside/secret.txt", "a/../../outside/secret.txt"]) + ), + "" + ); + } + + /// Every sampled path shares its whole directory chain, so the search can + /// strip down to a basename. One hit on a same-named file elsewhere in the + /// tree is not evidence of the prefix, and taking it would upload that + /// file's contents for a finding about another file. + #[test] + fn lone_bare_basename_match_is_rejected() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["index.ts"]); + + assert_eq!( + find_report_path_prefix(&root, &report(&["src/utils/index.ts", "src/utils/x.ts"])), + "" + ); + } + + #[test] + fn bare_basename_accepted_when_corroborated() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["index.ts", "app.ts"]); + + assert_eq!( + find_report_path_prefix(&root, &report(&["proj/src/index.ts", "proj/src/app.ts"])), + "proj/src/" + ); + } + + /// `proj` is shared but `src`/`lib` are not, so `proj/src/` is never tried + /// even though it would resolve. + #[test] + fn prefix_never_exceeds_the_shared_directories() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["index.ts", "app.ts"]); + + assert_eq!( + find_report_path_prefix(&root, &report(&["proj/src/index.ts", "proj/lib/app.ts"])), + "" + ); + } + + /// A report generated on this machine can name a file by an absolute path + /// that resolves. Dropping a prefix from it would read a same-named file + /// out of the working tree instead. + #[test] + fn absolute_paths_that_resolve_are_left_alone() { + let scanned = tempfile::tempdir().unwrap(); + let scanned = tree(scanned.path(), &["src/a.ts", "src/b.ts"]); + let cwd = tempfile::tempdir().unwrap(); + let cwd = tree(cwd.path(), &["src/a.ts", "src/b.ts"]); + + let paths = report(&[ + scanned.join("src/a.ts").to_str().unwrap(), + scanned.join("src/b.ts").to_str().unwrap(), + ]); + + assert_eq!(find_report_path_prefix(&cwd, &paths), ""); + } + + #[test] + fn strip_report_prefix_rebases_only_matching_paths() { + for (path, prefix, expected) in [ + ("Downloads/src/a.ts", "Downloads/", "src/a.ts"), + ("/Downloads/src/a.ts", "Downloads/", "src/a.ts"), + ("Downloads\\src\\a.ts", "Downloads/", "src/a.ts"), + ("src/a.ts", "Downloads/", "src/a.ts"), + ("Downloads/src/a.ts", "", "Downloads/src/a.ts"), + ("", "Downloads/", ""), + ] { + assert_eq!(strip_report_prefix(path, prefix), expected); + } + } + + #[test] + fn local_report_path_joins_the_remainder_onto_the_root() { + let root = Path::new("/work/repo"); + + assert_eq!( + local_report_path(root, "Downloads/src/a.ts", "Downloads/"), + PathBuf::from("/work/repo/src/a.ts") + ); + } + + #[test] + fn local_report_path_leaves_unprefixed_and_unmatched_paths_as_written() { + let root = Path::new("/work/repo"); + + // No prefix: resolved relative to the process directory, as always. + assert_eq!( + local_report_path(root, "src/a.ts", ""), + PathBuf::from("src/a.ts") + ); + // A path from another tree entirely keeps its own resolution rather + // than being forced under the root. + assert_eq!( + local_report_path(root, "/opt/tool/lib/fs.d.ts", "Downloads/"), + PathBuf::from("/opt/tool/lib/fs.d.ts") + ); + } + + #[test] + fn local_report_path_refuses_to_walk_out_of_the_root() { + let root = Path::new("/work/repo"); + + assert_eq!( + local_report_path(root, "Downloads/../../etc/passwd", "Downloads/"), + PathBuf::from("Downloads/../../etc/passwd") + ); + } +} diff --git a/tests/cloud_commands_e2e/main.rs b/tests/cloud_commands_e2e/main.rs index 6d4cc51..13120ae 100644 --- a/tests/cloud_commands_e2e/main.rs +++ b/tests/cloud_commands_e2e/main.rs @@ -8,4 +8,5 @@ mod scan_incremental; mod scan_list; mod scan_skip; mod transient_retry; +mod upload_report_paths; mod upload_wait; diff --git a/tests/cloud_commands_e2e/upload_report_paths.rs b/tests/cloud_commands_e2e/upload_report_paths.rs new file mode 100644 index 0000000..a73bc63 --- /dev/null +++ b/tests/cloud_commands_e2e/upload_report_paths.rs @@ -0,0 +1,113 @@ +//! `corgea upload` when the report's paths are not relative to the directory +//! the command runs in. +//! +//! A scanner that ran on a build agent records that agent's paths, so the files +//! the report names are not where the report says they are. The CLI has to find +//! them anyway, and has to keep uploading them under the paths the report uses, +//! because the engine matches the report against those and never sees this +//! working tree. + +use crate::common::*; +use hyper::Method; +use serde_json::json; +use tempfile::TempDir; + +/// A project whose sources sit at `src/`, reported under a build-agent prefix. +fn build_agent_report_project() -> (TempDir, String) { + let root = TempDir::new().expect("create report project"); + let source_dir = root.path().join("src"); + std::fs::create_dir(&source_dir).expect("create source directory"); + for name in ["main.py", "helper.py"] { + std::fs::write(source_dir.join(name), SOURCE_BODY).expect("write source"); + } + let report = r#"{"version":"semgrep.dev/v1","results":[{"path":"/builds/acme/repo/src/main.py"},{"path":"/builds/acme/repo/src/helper.py"}]}"#; + let report_path = root.path().join("semgrep.json"); + std::fs::write(&report_path, report).expect("write report"); + (root, report.to_string()) +} + +fn expect_source_upload(report_path: &'static str) -> ExpectedRequest { + expected_request( + "upload referenced source", + move |request| { + assert_authenticated_request(request, Method::POST, "/api/v1/code-upload")?; + assert_query(request, "path", report_path)?; + assert_body_contains(request, SOURCE_BODY.as_bytes()) + }, + json_response(json!({"status": "ok"})), + ) +} + +#[test] +fn upload_finds_sources_under_a_build_agent_prefix_and_keeps_the_reported_paths() { + let (project, report) = build_agent_report_project(); + let api = ApiStub::start(vec![ + verify_request(), + expect_source_upload("/builds/acme/repo/src/main.py"), + expect_source_upload("/builds/acme/repo/src/helper.py"), + expected_request( + "upload report", + move |request| { + assert_authenticated_request(request, Method::POST, "/api/v1/scan-upload")?; + assert_body_contains(request, report.as_bytes()) + }, + json_response(json!({ + "status": "ok", + "sast_scan_id": "prefix-scan-123", + "project_id": 7 + })), + ), + ]); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "upload", + project + .path() + .join("semgrep.json") + .to_str() + .expect("UTF-8 report path"), + "--project-name", + "upload-contract", + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + String::from_utf8_lossy(&output.stderr).contains("Dropping 'builds/acme/repo/'"), + "{context}" + ); +} + +/// Nothing resolves however much is dropped, so the report really was generated +/// against a different tree and the command still refuses to guess. +#[test] +fn upload_still_exits_one_when_no_prefix_resolves_the_report() { + let project = TempDir::new().expect("create report project"); + std::fs::write( + project.path().join("semgrep.json"), + r#"{"version":"semgrep.dev/v1","results":[{"path":"/builds/acme/repo/src/main.py"}]}"#, + ) + .expect("write report"); + let api = ApiStub::start(vec![verify_request()]); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "upload", + project + .path() + .join("semgrep.json") + .to_str() + .expect("UTF-8 report path"), + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + assert_eq!(output.status.code(), Some(1), "{context}"); + assert!( + String::from_utf8_lossy(&output.stderr) + .contains("Required file /builds/acme/repo/src/main.py not found"), + "{context}" + ); +} From a32b100bb1b35ad30045925f371337451aae0094 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 15 Sep 2026 11:39:16 +0000 Subject: [PATCH 2/6] Upload only the Fortify location each finding is reported at A Fortify vulnerability can carry several SourceLocations, where the earlier ones are the enclosing scope and only the last is the finding itself. The engine reads that last one and no other, so the earlier ones were uploaded for nothing -- and they can sit in a tree that is not on the machine running the CLI at all: Fortify records its own bundled libraries under AppData/Local/Fortify/sca*/build/.../_fortify_libraries_/, which no source upload can satisfy, so the command exited 1 before sending a single file of a report whose real files were all present. Collecting the resolved location only also keeps the prefix search reading exactly the paths that are looked up. Candidate prefixes are built from the directories every sampled path agrees on, so one path from an unrelated tree disagreeing at the first segment empties the shared prefix and nothing can be dropped. Deduplicating through a set rather than a linear scan drops the quadratic cost on reports that repeat a path per finding. --- src/scanners/fortify.rs | 108 +++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 39 deletions(-) diff --git a/src/scanners/fortify.rs b/src/scanners/fortify.rs index 8c9ff72..99631db 100644 --- a/src/scanners/fortify.rs +++ b/src/scanners/fortify.rs @@ -1,8 +1,9 @@ use crate::scan::{upload_scan, ScanUploadResult}; use crate::Config; -use quick_xml::events::Event; +use quick_xml::events::{BytesStart, Event}; use quick_xml::reader::Reader; use quick_xml::XmlVersion; +use std::collections::HashSet; use std::fs::File; use std::io; use std::io::{BufReader, Read}; @@ -68,6 +69,15 @@ pub fn parse( result } +/// The raw FVDL and the source files the engine will look up for it. +/// +/// A vulnerability can carry several `SourceLocation`s, where the earlier ones +/// are the enclosing scope and only the last is the finding itself, and the +/// engine reads that last one only. The earlier ones can sit in an unrelated +/// tree -- Fortify records its own bundled libraries under +/// `AppData/Local/Fortify/.../_fortify_libraries_/` -- which is not on the +/// machine running the CLI, so collecting them aborted the upload of a report +/// whose real files were all present. fn extract_file_path(scan_file: PathBuf) -> (String, Vec) { let mut paths: Vec = Vec::new(); @@ -84,6 +94,9 @@ fn extract_file_path(scan_file: PathBuf) -> (String, Vec) { let mut buf = Vec::new(); let mut in_vulnerability = false; + let mut seen = HashSet::new(); + // The last SourceLocation seen in the current Vulnerability. + let mut selected: Option = None; loop { match xml_reader.read_event_into(&mut buf) { @@ -94,48 +107,15 @@ fn extract_file_path(scan_file: PathBuf) -> (String, Vec) { if tag_name == b"Vulnerability" { in_vulnerability = true; } else if tag_name == b"SourceLocation" && in_vulnerability { - for attr_result in e.attributes() { - match attr_result { - Ok(attr) => { - let attr_key = attr.key.as_ref(); - if attr_key == b"path" { - if let Ok(value) = - attr.normalized_value(XmlVersion::Implicit1_0) - { - let path_str = value.to_string(); - if !paths.contains(&path_str) { - paths.push(path_str); - } - } - } - } - Err(e) => println!("Error processing attribute: {}", e), - } + if let Some(path) = source_location_path(e) { + selected = Some(path); } } } Ok(Event::Empty(ref e)) => { - let e_name = e.name(); - let tag_name = e_name.as_ref(); - - if tag_name == b"SourceLocation" && in_vulnerability { - for attr_result in e.attributes() { - match attr_result { - Ok(attr) => { - let attr_key = attr.key.as_ref(); - if attr_key == b"path" { - if let Ok(value) = - attr.normalized_value(XmlVersion::Implicit1_0) - { - let path_str = value.to_string(); - if !paths.contains(&path_str) { - paths.push(path_str); - } - } - } - } - Err(e) => println!("Error processing attribute: {}", e), - } + if e.name().as_ref() == b"SourceLocation" && in_vulnerability { + if let Some(path) = source_location_path(e) { + selected = Some(path); } } } @@ -145,6 +125,12 @@ fn extract_file_path(scan_file: PathBuf) -> (String, Vec) { if tag_name == b"Vulnerability" { in_vulnerability = false; + + if let Some(path) = selected.take() { + if seen.insert(path.clone()) { + paths.push(path); + } + } } } Ok(Event::Eof) => break, @@ -157,6 +143,23 @@ fn extract_file_path(scan_file: PathBuf) -> (String, Vec) { (contents, paths) } +/// The `path` attribute of a `SourceLocation`, with XML entities resolved. +fn source_location_path(element: &BytesStart) -> Option { + for attr_result in element.attributes() { + match attr_result { + Ok(attr) if attr.key.as_ref() == b"path" => { + if let Ok(value) = attr.normalized_value(XmlVersion::Implicit1_0) { + return Some(value.to_string()); + } + } + Ok(_) => {} + Err(e) => println!("Error processing attribute: {}", e), + } + } + + None +} + #[cfg(test)] mod tests { use super::*; @@ -176,6 +179,8 @@ mod tests { + + @@ -204,4 +209,29 @@ mod tests { ignores the out-of-scope SourceLocation, and de-duplicates" ); } + + /// The engine reads the last SourceLocation of a vulnerability and no + /// other, so the earlier ones must not be uploaded: they can name Fortify's + /// own bundled libraries, which are not on the machine running the CLI and + /// used to abort the upload before any file was sent. + #[test] + fn extract_file_path_keeps_only_the_location_the_finding_is_reported_at() { + let fvdl = r#" + + + + + + + +"#; + + let mut tmp = tempfile::NamedTempFile::new().expect("create temp fvdl"); + tmp.write_all(fvdl.as_bytes()).expect("write fvdl"); + tmp.flush().expect("flush fvdl"); + + let (_, paths) = extract_file_path(tmp.path().to_path_buf()); + + assert_eq!(paths, vec!["src/App.java".to_string()]); + } } From c78ddd0fab903b51fb1832f0c7218bdeb779a825 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 16 Sep 2026 06:40:37 +0000 Subject: [PATCH 3/6] Read a report's file from the form the prefix search matched it by find_report_path_prefix accepts a path as already matching when either the path as written or its slash-stripped form resolves under the root, but local_report_path only ever opened the path as written when no prefix was dropped. A rooted path whose slash-stripped form is in the tree therefore short-circuited the search and was then opened at its absolute path, which is nothing on this machine: a scanner run with the repo mounted at /src reports /src/main.py, the search confirms src/main.py is right there, and the upload still exits 1 on it. That is the failure this whole search exists to remove, and the error named only /src/main.py because the two paths were equal. The two now resolve a path the same way. The path as written still wins when it names a real file, so a report generated on this machine keeps reading its own absolute paths rather than same-named files in the working tree. Otherwise the prefix is dropped, the remainder normalized, and the result read under the root, which also fixes a Windows-style path whose separators no Unix open will accept. A path that neither exists as written nor carries the prefix is now looked for under the root instead of at its own absolute path. Both are absent and it exits 1 either way; the message names where it looked. --- src/scanners/report_paths.rs | 117 +++++++++++++++--- .../cloud_commands_e2e/upload_report_paths.rs | 59 ++++++++- 2 files changed, 157 insertions(+), 19 deletions(-) diff --git a/src/scanners/report_paths.rs b/src/scanners/report_paths.rs index 73aa3f3..ec33d8e 100644 --- a/src/scanners/report_paths.rs +++ b/src/scanners/report_paths.rs @@ -177,19 +177,39 @@ pub fn strip_report_prefix(path: &str, prefix: &str) -> String { /// Where to read a report's file from on this machine. /// -/// Without a prefix this is the path as the report wrote it, which is how the -/// CLI has always resolved it, so a report that already matches is untouched. -/// With one, the remainder is joined onto `root`; a `..` in it falls back to the -/// report's own path, because the prefix search only samples paths and an -/// unsampled one must not be able to walk out of the project. +/// This has to resolve a path the same way [`find_report_path_prefix`] checked +/// it, or the search proves one file present and the upload opens another. The +/// search reads every path both as written and slash-normalized under `root`, so +/// both are tried here, in that order: +/// +/// * The path as the report wrote it wins when it names a real file. A report +/// generated on this machine can carry absolute paths, and those are the files +/// its findings are about -- not same-named ones in the working tree. +/// * Otherwise the prefix is dropped, the remainder normalized, and the result +/// read under `root`. This covers a rooted path whose slash-stripped form is +/// already in the tree (`/src/a.py`), which needs no prefix but does need +/// rebasing, and a Windows-style path, which needs its separators fixed. +/// +/// A path that is already relative and carries no prefix is returned untouched, +/// so a report that matches resolves against the process directory exactly as it +/// always has. So is one whose remainder holds a `..`: the prefix search only +/// samples paths, and an unsampled one must not walk out of the project. pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { - let stripped = strip_report_prefix(report_path, prefix); + let as_written = PathBuf::from(report_path); + let relative = normalize(&strip_report_prefix(report_path, prefix)); + + if relative == report_path + || relative.is_empty() + || relative.split('/').any(|segment| segment == "..") + { + return as_written; + } - if stripped == report_path || stripped.split('/').any(|segment| segment == "..") { - return PathBuf::from(report_path); + if as_written.is_file() { + return as_written; } - root.join(stripped) + root.join(relative) } #[cfg(test)] @@ -399,20 +419,83 @@ mod tests { ); } + /// A scanner that ran with the repo mounted at `/src` reports `/src/a.py` + /// while the file is at `src/a.py`. `find_report_path_prefix` counts that as + /// already matching -- it checks the slash-stripped form under the root -- + /// so the file that is opened has to be the one it checked, not the + /// absolute path, which is nothing on this machine. #[test] - fn local_report_path_leaves_unprefixed_and_unmatched_paths_as_written() { - let root = Path::new("/work/repo"); + fn a_rooted_path_resolves_under_the_root_that_matched_it() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/a.py", "src/b.py"]); + let paths = report(&["/src/a.py", "/src/b.py"]); - // No prefix: resolved relative to the process directory, as always. + let prefix = find_report_path_prefix(&root, &paths); + + assert_eq!( + prefix, "", + "the slash-stripped paths are already in the tree" + ); assert_eq!( - local_report_path(root, "src/a.ts", ""), + local_report_path(&root, "/src/a.py", &prefix), + root.join("src/a.py") + ); + } + + #[test] + fn windows_separators_resolve_under_the_root_without_a_prefix() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/a.cs", "src/b.cs"]); + let paths = report(&["src\\a.cs", "src\\b.cs"]); + + let prefix = find_report_path_prefix(&root, &paths); + + assert_eq!(prefix, ""); + assert_eq!( + local_report_path(&root, "src\\a.cs", &prefix), + root.join("src/a.cs") + ); + } + + /// An already-relative path carrying no prefix is left completely alone, so + /// a report that matches resolves against the process directory as always. + #[test] + fn local_report_path_leaves_a_matching_relative_path_untouched() { + assert_eq!( + local_report_path(Path::new("/work/repo"), "src/a.ts", ""), PathBuf::from("src/a.ts") ); - // A path from another tree entirely keeps its own resolution rather - // than being forced under the root. + } + + /// A report generated on this machine names files absolutely. Those are the + /// files its findings are about, so an absolute path that exists is read + /// where it is rather than rebased onto a same-named file under the root. + #[test] + fn local_report_path_reads_an_absolute_path_that_exists_where_it_is() { + let elsewhere = tempfile::tempdir().unwrap(); + let elsewhere = tree(elsewhere.path(), &["lib/fs.d.ts"]); + let reported = elsewhere.join("lib/fs.d.ts"); + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["lib/fs.d.ts"]); + + assert_eq!( + local_report_path(&root, reported.to_str().unwrap(), "Downloads/"), + reported + ); + } + + /// Nothing else can be done with a path that neither exists as written nor + /// carries the report's prefix, and the root is the only other place it + /// could be, so the error names where it was looked for. + #[test] + fn local_report_path_falls_back_to_the_root_for_a_path_that_is_not_there() { assert_eq!( - local_report_path(root, "/opt/tool/lib/fs.d.ts", "Downloads/"), - PathBuf::from("/opt/tool/lib/fs.d.ts") + local_report_path( + Path::new("/work/repo"), + "/opt/tool/lib/fs.d.ts", + "Downloads/" + ), + PathBuf::from("/work/repo/opt/tool/lib/fs.d.ts") ); } diff --git a/tests/cloud_commands_e2e/upload_report_paths.rs b/tests/cloud_commands_e2e/upload_report_paths.rs index a73bc63..b536833 100644 --- a/tests/cloud_commands_e2e/upload_report_paths.rs +++ b/tests/cloud_commands_e2e/upload_report_paths.rs @@ -80,6 +80,57 @@ fn upload_finds_sources_under_a_build_agent_prefix_and_keeps_the_reported_paths( ); } +/// A scanner that ran with the repo mounted at `/src` reports `/src/main.py` +/// while the file is at `src/main.py`. No prefix needs dropping -- the paths +/// already match once the leading slash is gone -- but they still have to be +/// read under the working tree rather than at the absolute path, which is +/// nothing on this machine. +#[test] +fn upload_finds_sources_named_by_a_rooted_path() { + let project = TempDir::new().expect("create report project"); + let source_dir = project.path().join("src"); + std::fs::create_dir(&source_dir).expect("create source directory"); + for name in ["main.py", "helper.py"] { + std::fs::write(source_dir.join(name), SOURCE_BODY).expect("write source"); + } + let report = r#"{"version":"semgrep.dev/v1","results":[{"path":"/src/main.py"},{"path":"/src/helper.py"}]}"#; + std::fs::write(project.path().join("semgrep.json"), report).expect("write report"); + + let api = ApiStub::start(vec![ + verify_request(), + expect_source_upload("/src/main.py"), + expect_source_upload("/src/helper.py"), + expected_request( + "upload report", + move |request| { + assert_authenticated_request(request, Method::POST, "/api/v1/scan-upload")?; + assert_body_contains(request, report.as_bytes()) + }, + json_response(json!({ + "status": "ok", + "sast_scan_id": "rooted-scan-123", + "project_id": 9 + })), + ), + ]); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "upload", + project + .path() + .join("semgrep.json") + .to_str() + .expect("UTF-8 report path"), + "--project-name", + "upload-contract", + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + assert_eq!(output.status.code(), Some(0), "{context}"); +} + /// Nothing resolves however much is dropped, so the report really was generated /// against a different tree and the command still refuses to guess. #[test] @@ -105,9 +156,13 @@ fn upload_still_exits_one_when_no_prefix_resolves_the_report() { let transcript = api.assert_finished(); let context = output_context(&output, &transcript); assert_eq!(output.status.code(), Some(1), "{context}"); + let stderr = String::from_utf8_lossy(&output.stderr); assert!( - String::from_utf8_lossy(&output.stderr) - .contains("Required file /builds/acme/repo/src/main.py not found"), + stderr.contains("Required file /builds/acme/repo/src/main.py"), "{context}" ); + assert!( + stderr.contains("builds/acme/repo/src/main.py') not found"), + "names where it looked, which is not the path in the report: {context}" + ); } From 50513647593526d768b3d4653752d9d57c811c9b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 16 Sep 2026 07:56:55 +0000 Subject: [PATCH 4/6] Confine the prefix probe to the working tree it is probing The probe joined the raw remainder onto the root while the upload normalized it first, so the two resolved a path differently again. The shared prefix is built from the report's own segments, so dropping it does not always leave a clean relative path: a SARIF `file:///src/a.py` minus `file:/` leaves `//src/a.py`, and Path::join treats a leading slash as absolute and discards the root. The probe therefore stat'd an absolute path outside the project. A report of `file:////src/a.py` whose files exist at that absolute location but not in the upload had its prefix accepted on the strength of those files, and the upload, which reads under the root, then exited 1 on the first one. Nothing in the project was ever consulted. `confine` is now the single resolver both sides go through: it normalizes the remainder, refuses one that cannot stay under the root, and leaves the caller to join. That also makes the bare-basename guard read the confined remainder, so a leftover `/a.py` counts as the basename it is rather than passing on its leading slash. A `file://` URI whose files are in the upload now resolves through the scheme, since the search walks down the segments it shares like any other prefix. --- src/scanners/report_paths.rs | 99 ++++++++++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 20 deletions(-) diff --git a/src/scanners/report_paths.rs b/src/scanners/report_paths.rs index ec33d8e..e1c4ea2 100644 --- a/src/scanners/report_paths.rs +++ b/src/scanners/report_paths.rs @@ -84,6 +84,28 @@ fn leading_dirs(path: &str) -> Vec<&str> { dirs } +/// A report path with its shared prefix already dropped, as a path relative to +/// the working tree, or `None` when it cannot be confined there. +/// +/// This is the one place that decides how a report path maps onto the tree, and +/// both the prefix search and the upload go through it. A remainder is not +/// necessarily a clean relative path -- the shared prefix is built from the +/// report's own segments, so dropping it can leave leading slashes +/// (`file:///src/a.py` minus `file:/`) or a `..`. `Path::join` treats a leading +/// slash as absolute and discards the root, so a remainder joined on raw +/// escapes the project: the search would accept a prefix on the strength of +/// files that are not in the upload, and the upload would then die on the first +/// of them. +fn confine(remainder: &str) -> Option { + let relative = normalize(remainder); + + if relative.is_empty() || relative.split('/').any(|segment| segment == "..") { + return None; + } + + Some(relative) +} + /// Leading directories every path agrees on. /// /// Candidate prefixes are built from these, so one path's coincidental suffix @@ -107,6 +129,11 @@ fn shared_dirs<'a>(paths: &[&'a str]) -> Vec<&'a str> { shared } +/// Whether a report path remainder names a file inside the working tree. +fn resolves_under(root: &Path, remainder: &str) -> bool { + confine(remainder).is_some_and(|relative| root.join(relative).is_file()) +} + /// Return the leading prefix to drop from a report's paths, or `""`. /// /// Returns `""` as soon as any sampled path resolves as written, so a report @@ -127,7 +154,7 @@ pub fn find_report_path_prefix(root: &Path, report_paths: &[String]) -> String { // is about. if sample .iter() - .any(|path| root.join(&path.relative).is_file() || Path::new(&path.raw).is_file()) + .any(|path| resolves_under(root, &path.relative) || Path::new(&path.raw).is_file()) { return String::new(); } @@ -137,10 +164,11 @@ pub fn find_report_path_prefix(root: &Path, report_paths: &[String]) -> String { for depth in 1..=shared.len().min(MAX_REPORT_PATH_PREFIX_DEPTH) { let prefix = format!("{}/", shared[..depth].join("/")); - let hits: Vec<&str> = relatives + let hits: Vec = relatives .iter() .filter_map(|path| path.strip_prefix(prefix.as_str())) - .filter(|remainder| root.join(remainder).is_file()) + .filter_map(confine) + .filter(|relative| root.join(relative).is_file()) .collect(); // A real offset resolves the report broadly. A lone hit that is also a @@ -177,35 +205,31 @@ pub fn strip_report_prefix(path: &str, prefix: &str) -> String { /// Where to read a report's file from on this machine. /// -/// This has to resolve a path the same way [`find_report_path_prefix`] checked -/// it, or the search proves one file present and the upload opens another. The -/// search reads every path both as written and slash-normalized under `root`, so -/// both are tried here, in that order: +/// This goes through [`confine`], the same resolver [`find_report_path_prefix`] +/// probes with, so the file the search proved present is the file that gets +/// opened. The two resolving a path differently is how a report passes the +/// search and then dies on its first upload. /// /// * The path as the report wrote it wins when it names a real file. A report /// generated on this machine can carry absolute paths, and those are the files /// its findings are about -- not same-named ones in the working tree. -/// * Otherwise the prefix is dropped, the remainder normalized, and the result -/// read under `root`. This covers a rooted path whose slash-stripped form is -/// already in the tree (`/src/a.py`), which needs no prefix but does need -/// rebasing, and a Windows-style path, which needs its separators fixed. +/// * Otherwise the confined remainder is read under `root`. This covers a rooted +/// path whose slash-stripped form is already in the tree (`/src/a.py`), which +/// needs no prefix but does need rebasing, and a Windows-style path, which +/// needs its separators fixed. /// /// A path that is already relative and carries no prefix is returned untouched, /// so a report that matches resolves against the process directory exactly as it -/// always has. So is one whose remainder holds a `..`: the prefix search only -/// samples paths, and an unsampled one must not walk out of the project. +/// always has. So is one that cannot be confined: the prefix search only samples +/// paths, and an unsampled one must not walk out of the project. pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { let as_written = PathBuf::from(report_path); - let relative = normalize(&strip_report_prefix(report_path, prefix)); - if relative == report_path - || relative.is_empty() - || relative.split('/').any(|segment| segment == "..") - { + let Some(relative) = confine(&strip_report_prefix(report_path, prefix)) else { return as_written; - } + }; - if as_written.is_file() { + if relative == report_path || as_written.is_file() { return as_written; } @@ -395,6 +419,41 @@ mod tests { assert_eq!(find_report_path_prefix(&cwd, &paths), ""); } + /// SARIF permits a `file:///` URI. The shared prefix is built from the + /// report's own segments, so dropping it leaves a remainder that still + /// carries leading slashes, and a probe that joins that onto the root + /// silently escapes it -- `Path::join` treats a leading slash as absolute + /// and discards the root. The prefix would then be accepted on the strength + /// of files that are not in the project at all, and the upload, which reads + /// under the root, dies on the first one. + #[test] + fn a_file_uri_pointing_outside_the_project_resolves_nothing() { + let outside = tempfile::tempdir().unwrap(); + let outside = tree(outside.path(), &["src/a.py", "src/b.py"]); + let root = tempfile::tempdir().unwrap(); + let paths = report(&[ + &format!("file://{}/src/a.py", outside.display()), + &format!("file://{}/src/b.py", outside.display()), + ]); + + assert_eq!(find_report_path_prefix(root.path(), &paths), ""); + } + + /// The same URI shape, but the files really are in the project. + #[test] + fn a_file_uri_probes_and_reads_the_same_file() { + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/a.py", "src/b.py"]); + let paths = report(&["file:///src/a.py", "file:///src/b.py"]); + + let prefix = find_report_path_prefix(&root, &paths); + + assert_eq!( + local_report_path(&root, "file:///src/a.py", &prefix), + root.join("src/a.py") + ); + } + #[test] fn strip_report_prefix_rebases_only_matching_paths() { for (path, prefix, expected) in [ From 19d9dc1e93dbd08ddb9fc4cc756f8141c4622794 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 16 Sep 2026 08:30:17 +0000 Subject: [PATCH 5/6] Trust a report prefix only when it resolves the whole sample The upload aborts on the first path it cannot find, so a prefix that resolves part of the report cannot produce a successful upload -- it can only upload whatever sits at the paths that did resolve before dying on the rest. Two hits is Fusion's threshold, where an unresolved finding is skipped and the rest of the report is kept; here it let a coincidental pair of filenames define the offset for a report that was always going to fail. This also settles the shallowest-first case: a shallow prefix that resolves only some of the report no longer beats a deeper one that resolves all of it. Co-authored-by: ibrahim --- src/scanners/report_paths.rs | 80 ++++++++++++++++++++++++++++++++---- 1 file changed, 71 insertions(+), 9 deletions(-) diff --git a/src/scanners/report_paths.rs b/src/scanners/report_paths.rs index e1c4ea2..90c41f3 100644 --- a/src/scanners/report_paths.rs +++ b/src/scanners/report_paths.rs @@ -139,7 +139,9 @@ fn resolves_under(root: &Path, remainder: &str) -> bool { /// Returns `""` as soon as any sampled path resolves as written, so a report /// that already matches this working tree is left alone. Otherwise one /// directory at a time is dropped from the shared prefix, shallowest first, and -/// the first prefix whose remainder resolves under `root` wins. +/// the shallowest prefix that resolves the whole sample wins -- shallowest so +/// the least of the report's own paths is rewritten, whole-sample so a prefix +/// is never trusted on the strength of a few coincidental filenames. pub fn find_report_path_prefix(root: &Path, report_paths: &[String]) -> String { let sample = sample_paths(report_paths); @@ -171,16 +173,25 @@ pub fn find_report_path_prefix(root: &Path, report_paths: &[String]) -> String { .filter(|relative| root.join(relative).is_file()) .collect(); - // A real offset resolves the report broadly. A lone hit that is also a - // bare basename is more likely a same-named file elsewhere in the tree - // than evidence of the prefix, and accepting it would upload one file's - // contents for a finding about another. - if hits.len() >= 2 || (hits.len() == 1 && hits[0].contains('/')) { + // The whole sample has to resolve, not just some of it. The upload + // aborts on the first path it cannot find, so a prefix that resolves + // part of the report cannot produce a successful upload -- it can only + // upload whatever happens to sit at the paths that did resolve before + // dying on the rest. Fusion accepts two hits because it skips the + // findings it cannot resolve and keeps the remainder of the report; if + // this ever stops being all-or-nothing, revisit this with it. + // + // A lone hit that is also a bare basename stays refused on top of that: + // a one-path report always resolves "all" of its sample, and a bare + // basename is more likely a same-named file elsewhere in the tree than + // evidence of the prefix. + let complete = hits.len() == relatives.len(); + + if complete && (hits.len() >= 2 || hits[0].contains('/')) { log::warn!( - "The report's paths are not relative to this directory. Dropping '{}' from them resolves {} of {} sampled path(s); uploading those files under the paths the report uses.", + "The report's paths are not relative to this directory. Dropping '{}' from them resolves all {} sampled path(s); uploading those files under the paths the report uses.", prefix, - hits.len(), - relatives.len() + hits.len() ); return prefix; } @@ -377,6 +388,57 @@ mod tests { ); } + #[test] + fn a_prefix_that_resolves_only_part_of_the_report_is_refused() { + // Another repo's report, sharing two filenames with this working tree + // and nothing else. Both of those resolve once 'build/other/' is + // dropped, which is the whole of the evidence for that prefix. + let root = tempfile::tempdir().unwrap(); + let root = tree(root.path(), &["src/main.py", "src/utils.py"]); + + assert_eq!( + find_report_path_prefix( + &root, + &report(&[ + "build/other/src/main.py", + "build/other/src/utils.py", + "build/other/src/billing.py", + "build/other/src/tenants.py", + ]) + ), + "" + ); + } + + #[test] + fn a_deeper_prefix_that_resolves_everything_beats_a_shallow_partial_one() { + let root = tempfile::tempdir().unwrap(); + let root = tree( + root.path(), + &[ + "proj/src/a.ts", + "proj/src/b.ts", + "src/a.ts", + "src/b.ts", + "src/c.ts", + ], + ); + + // 'build/' resolves a.ts and b.ts through proj/src/ but not c.ts, so + // shallowest-first does not get to stop there. + assert_eq!( + find_report_path_prefix( + &root, + &report(&[ + "build/proj/src/a.ts", + "build/proj/src/b.ts", + "build/proj/src/c.ts", + ]) + ), + "build/proj/" + ); + } + #[test] fn bare_basename_accepted_when_corroborated() { let root = tempfile::tempdir().unwrap(); From f3e0d4140d99bd598cbf2277cb6ea965dbf968bc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 17 Sep 2026 07:56:11 +0000 Subject: [PATCH 6/6] Bump version to 1.14.2 Cargo.toml is the single source of truth for the release version: PyPI takes it through maturin's dynamic version and npm sets its own from the tag name, so this is the only manual edit. Co-authored-by: ibrahim --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca8e76b..0dde899 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -369,7 +369,7 @@ checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" [[package]] name = "corgea" -version = "1.14.1" +version = "1.14.2" dependencies = [ "chrono", "clap", diff --git a/Cargo.toml b/Cargo.toml index 8e8cf23..b56c782 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "corgea" -version = "1.14.1" +version = "1.14.2" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html