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 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/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()]); + } } diff --git a/src/scanners/report_paths.rs b/src/scanners/report_paths.rs new file mode 100644 index 0000000..90c41f3 --- /dev/null +++ b/src/scanners/report_paths.rs @@ -0,0 +1,632 @@ +//! 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 +} + +/// 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 +/// 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 +} + +/// 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 +/// 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 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); + + 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| resolves_under(root, &path.relative) || 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 = relatives + .iter() + .filter_map(|path| path.strip_prefix(prefix.as_str())) + .filter_map(confine) + .filter(|relative| root.join(relative).is_file()) + .collect(); + + // 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 all {} sampled path(s); uploading those files under the paths the report uses.", + prefix, + hits.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. +/// +/// 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 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 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 Some(relative) = confine(&strip_report_prefix(report_path, prefix)) else { + return as_written; + }; + + if relative == report_path || as_written.is_file() { + return as_written; + } + + root.join(relative) +} + +#[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 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(); + 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), ""); + } + + /// 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 [ + ("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") + ); + } + + /// 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 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"]); + + 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.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 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( + Path::new("/work/repo"), + "/opt/tool/lib/fs.d.ts", + "Downloads/" + ), + PathBuf::from("/work/repo/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..b536833 --- /dev/null +++ b/tests/cloud_commands_e2e/upload_report_paths.rs @@ -0,0 +1,168 @@ +//! `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}" + ); +} + +/// 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] +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}"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + 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}" + ); +}