From 8c39b4b2b2dc38fdfdb7b4f3f85d86e960439d3f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 31 Aug 2026 11:55:00 +0000 Subject: [PATCH 1/3] Add force-include rules to the scan packager and a --include flag A file the CLI leaves out of the archive cannot be scanned whatever the engine later decides about it, so DEFAULT_EXCLUDE_GLOBS and .gitignore silently overruled any platform-side attempt to force-scan misclassified proprietary code. Read the project's include rules from GET /api/v1/scan-settings before packaging, union them with the new repeatable --include flag, and add every matched file to the zip regardless of the default excludes, --exclude, or .gitignore. Force-included files also join the incremental changed-file list. The server carries findings forward for whatever the diff omits, and an include rule exists precisely because the file was never scanned, so there is nothing to carry. Only the --include values travel with the upload; the project's own rules are already stored server-side. Co-authored-by: ibrahim --- skills/corgea/SKILL.md | 5 + src/include_rules.rs | 228 ++++++++++++++++++ src/incremental.rs | 80 +++++++ src/main.rs | 20 ++ src/scanners/blast.rs | 49 ++++ src/utils/api.rs | 74 ++++++ src/utils/generic.rs | 64 +++++- tests/cli_scan_include.rs | 229 +++++++++++++++++++ tests/cloud_commands_e2e/common/mod.rs | 20 +- tests/cloud_commands_e2e/scan_incremental.rs | 10 +- tests/cloud_commands_e2e/scan_skip.rs | 6 +- 11 files changed, 777 insertions(+), 8 deletions(-) create mode 100644 src/include_rules.rs create mode 100644 tests/cli_scan_include.rs diff --git a/skills/corgea/SKILL.md b/skills/corgea/SKILL.md index d675396..c57502d 100644 --- a/skills/corgea/SKILL.md +++ b/skills/corgea/SKILL.md @@ -48,6 +48,9 @@ corgea scan --target "src/**/*.py" # Glob patterns corgea scan --target git:diff=origin/main...HEAD # Git diff range corgea scan --target git:staged,git:modified # Git selectors corgea scan --target - # File list from stdin +corgea scan --exclude "tests/**,*.md" # Exclude glob patterns (comma-separated) +corgea scan --include src/myProj/MyClass.java # Force a file in that Corgea would skip as vendored +corgea scan --include 'vendor/our-fork/**' --include generated/ # Repeatable; path, directory or glob corgea scan --scan-type secrets # Single scan type corgea scan --scan-type blast,policy,secrets,pii # Multiple scan types corgea scan --scan-type policy --policy 1 # Specific policy ID @@ -80,6 +83,8 @@ Scan types: `blast` (base AI), `policy` (PolicyIQ), `malicious`, `secrets`, `pii An included image is enough on its own: when it is combined with `--only-uncommitted` or `--target` and no source files match (a clean working tree, for example), the scan warns and covers just the image rather than failing. An archive named `corgea-image-scanning-*.tar` that is committed to the repository is ignored — only images passed on the command line are scanned. +`--include` forces files into the scan that Corgea would otherwise skip because it classified them as vendored, third-party, generated or test code. It overrides the CLI's own packaging filters, `.gitignore`, `--exclude`, and the engine's classification, and force-included files are analyzed on every run — including incremental ones, where an unchanged file would normally have its previous findings carried forward. Use it when proprietary code lives somewhere Corgea assumes dependencies live, e.g. a fork checked into `vendor/`. Project-level include rules configured in the web app (scan settings → File Include Rules) are fetched before packaging and applied too; `--include` adds to them for one run. + `--only-uncommitted` and `--target` are mutually exclusive. `--fail-on`, `--fail`, and `--block-on` are mutually exclusive. `--out-format`/`--out-file` and `--sbom` are honored regardless of the gate: the report and the SBOM are written before `--fail`/`--block-on` are evaluated, so a scan that exits 1 on a blocking rule still leaves the report file behind for the pipeline to ingest. diff --git a/src/include_rules.rs b/src/include_rules.rs new file mode 100644 index 0000000..ee14355 --- /dev/null +++ b/src/include_rules.rs @@ -0,0 +1,228 @@ +//! Force-include rules: files Corgea must scan even though it would skip them. +//! +//! Corgea leaves out vendored, third-party, test and generated code in two +//! places: this CLI's packaging filters (`DEFAULT_EXCLUDE_GLOBS`, `.gitignore`) +//! and the engine's own classification of what it extracted. When either gets +//! that call wrong for proprietary code, an include rule overrides it. +//! +//! Rules come from two places and are unioned: the project's rules on the +//! platform, fetched here before packaging, and `--include` on this command +//! line. Both matter locally — a file the packager leaves out of the zip cannot +//! be scanned whatever the engine later decides — and only the flag values +//! travel with the upload, since the platform already knows its own rules. + +use crate::config::Config; +use crate::utils::api; +use globset::{Glob, GlobSet, GlobSetBuilder}; +use ignore::WalkBuilder; +use std::path::{Path, PathBuf}; + +/// Ceiling on files one run may force into the archive. A rule like `**/*.js` +/// would otherwise pull an entire `node_modules` tree into the upload. +const MAX_FORCE_INCLUDED_FILES: usize = 5_000; + +/// The force-include rules in effect for one scan. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct IncludeRules { + /// Every pattern in effect: the project's rules plus `--include`. + pub patterns: Vec, + /// Just the `--include` values, the ones the server does not know yet. + pub cli_patterns: Vec, +} + +impl IncludeRules { + pub fn is_empty(&self) -> bool { + self.patterns.is_empty() + } + + /// Paths under `root` that the rules match, relative to `root`. + /// + /// Walks with the standard ignore filters off, since the point is to reach + /// files `.gitignore` and the default excludes hide. `.git` is still + /// skipped: it holds no source and its object store is large. + pub fn matching_files(&self, root: &Path) -> Vec { + let Some(glob_set) = build_glob_set(&self.patterns) else { + return Vec::new(); + }; + let mut matches = Vec::new(); + let walker = WalkBuilder::new(root) + .standard_filters(false) + .filter_entry(|entry| entry.file_name() != ".git") + .build(); + for entry in walker.flatten() { + if !entry.file_type().is_some_and(|kind| kind.is_file()) { + continue; + } + let Ok(relative) = entry.path().strip_prefix(root) else { + continue; + }; + if glob_set.is_match(relative) { + matches.push(relative.to_path_buf()); + } + if matches.len() >= MAX_FORCE_INCLUDED_FILES { + log::warn!( + "Include rules matched more than {} files; only the first {} are forced into this scan.", + MAX_FORCE_INCLUDED_FILES, + MAX_FORCE_INCLUDED_FILES + ); + break; + } + } + matches.sort(); + matches + } +} + +/// Build a matcher, dropping patterns globset cannot compile. +/// +/// One unparseable pattern must not discard the rest: the others are still a +/// clear instruction, and silently scanning less than asked is the failure this +/// whole feature exists to fix. +fn build_glob_set(patterns: &[String]) -> Option { + let mut builder = GlobSetBuilder::new(); + let mut usable = 0; + for pattern in patterns { + match Glob::new(pattern) { + Ok(glob) => { + builder.add(glob); + usable += 1; + } + Err(e) => log::warn!("Ignoring include rule '{pattern}': {e}"), + } + // A bare path or directory prefix should match what is under it, which + // is how the same pattern reads in the platform's ignore rules. + if !pattern.contains('*') { + let descendants = format!("{}/**", pattern.trim_end_matches('/')); + if let Ok(glob) = Glob::new(&descendants) { + builder.add(glob); + usable += 1; + } + } + } + if usable == 0 { + return None; + } + builder.build().ok() +} + +/// Collect the rules for this run: the project's, plus `--include`. +/// +/// A failed lookup is a warning, not a failure. It leaves the project's rules +/// unapplied for this run, which is the behavior every release before this one +/// had; refusing to scan would be worse. +pub fn resolve( + config: &Config, + project_name: &str, + repo_url: Option<&str>, + cli_include: &[String], +) -> IncludeRules { + let cli_patterns = normalize_patterns(cli_include); + let mut patterns = Vec::new(); + + match api::query_scan_settings(&config.get_url(), project_name, repo_url) { + Ok(Some(settings)) => { + for pattern in normalize_patterns(&settings.include_paths) { + push_unique(&mut patterns, pattern); + } + if !patterns.is_empty() { + println!( + "Applying {} project include rule(s) from Corgea: {}.", + patterns.len(), + patterns.join(", ") + ); + } + } + // A backend without the endpoint has no include rules to apply either. + Ok(None) => {} + Err(e) => log::warn!( + "Could not read the project's include rules, so only --include applies to this run: {e}" + ), + } + + for pattern in &cli_patterns { + push_unique(&mut patterns, pattern.clone()); + } + IncludeRules { + patterns, + cli_patterns, + } +} + +fn normalize_patterns(patterns: &[String]) -> Vec { + let mut normalized = Vec::new(); + for pattern in patterns { + let trimmed = pattern.trim(); + if !trimmed.is_empty() { + push_unique(&mut normalized, trimmed.to_string()); + } + } + normalized +} + +fn push_unique(patterns: &mut Vec, pattern: String) { + if !patterns.contains(&pattern) { + patterns.push(pattern); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::TempDir; + + fn rules(patterns: &[&str]) -> IncludeRules { + IncludeRules { + patterns: patterns.iter().map(|p| p.to_string()).collect(), + cli_patterns: Vec::new(), + } + } + + fn write(root: &TempDir, relative: &str) { + let path = root.path().join(relative); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + fs::write(path, "class A {}\n").unwrap(); + } + + #[test] + fn normalize_trims_blanks_and_dedupes() { + let input = ["a/**".to_string(), " a/** ".to_string(), " ".to_string()]; + assert_eq!(normalize_patterns(&input), vec!["a/**".to_string()]); + } + + #[test] + fn no_patterns_matches_nothing() { + let root = TempDir::new().unwrap(); + write(&root, "src/App.java"); + assert!(IncludeRules::default() + .matching_files(root.path()) + .is_empty()); + assert!(build_glob_set(&[]).is_none()); + } + + #[test] + fn unparseable_pattern_does_not_discard_the_others() { + assert!(build_glob_set(&["src/**".to_string(), "[".to_string()]).is_some()); + } + + #[test] + fn matching_files_reaches_gitignored_and_vendored_paths() { + let root = TempDir::new().unwrap(); + write(&root, "vendor/mylib/Payments.java"); + write(&root, "vendor/other/Other.java"); + write(&root, "node_modules/pkg/index.js"); + write(&root, ".git/objects/blob"); + fs::write(root.path().join(".gitignore"), "vendor/\nnode_modules/\n").unwrap(); + + let matched = + rules(&["vendor/mylib", "node_modules/pkg/index.js"]).matching_files(root.path()); + + assert_eq!( + matched, + vec![ + PathBuf::from("node_modules/pkg/index.js"), + PathBuf::from("vendor/mylib/Payments.java"), + ] + ); + } +} diff --git a/src/incremental.rs b/src/incremental.rs index 8a8523b..56fbe51 100644 --- a/src/incremental.rs +++ b/src/incremental.rs @@ -60,6 +60,41 @@ pub struct IncrementalPlan { pub covers_worktree: bool, } +impl IncrementalPlan { + /// Add force-included files to what the server will analyze. + /// + /// The server carries findings forward for every file the diff omits, so a + /// force-included file that has not changed would never be looked at — and + /// the reason to add an include rule is precisely that the file was never + /// scanned before, so there is nothing to carry forward. Returns `None` + /// when the combined list outgrows an incremental scan, which falls back to + /// scanning everything. + pub fn including(mut self, forced: &[String]) -> Option { + let mut listed: BTreeSet = self.changed_files.iter().cloned().collect(); + let additions: Vec = forced + .iter() + .filter(|path| listed.insert((*path).clone())) + .cloned() + .collect(); + if additions.is_empty() { + return Some(self); + } + if self.changed_files.len() + additions.len() > MAX_CHANGED_FILES { + explain_full_scan( + "the include rules cover more files than an incremental scan is worth", + ); + return None; + } + match additions.len() { + 1 => println!("Incremental scan: also analyzing 1 force-included file."), + count => println!("Incremental scan: also analyzing {count} force-included files."), + } + self.changed_files.extend(additions); + self.changed_files.sort(); + Some(self) + } +} + /// What an incremental scan of this commit would cover, or `None` to scan /// everything. pub fn resolve_incremental_plan( @@ -442,6 +477,51 @@ mod tests { assert_eq!(short_sha("ααααααααα"), "ααααααα"); } + fn plan(changed: &[&str]) -> IncrementalPlan { + IncrementalPlan { + base_sha: "abc123".to_string(), + changed_files: changed.iter().map(|f| f.to_string()).collect(), + covers_worktree: false, + } + } + + #[test] + fn including_adds_force_included_files_the_diff_left_out() { + let forced = vec![ + "src/app.py".to_string(), + "vendor/mylib/Payments.java".to_string(), + ]; + + let widened = plan(&["src/app.py"]) + .including(&forced) + .expect("still worth it"); + + assert_eq!( + widened.changed_files, + vec![ + "src/app.py".to_string(), + "vendor/mylib/Payments.java".to_string() + ] + ); + } + + #[test] + fn including_nothing_new_leaves_the_plan_alone() { + let original = plan(&["src/app.py"]); + assert_eq!( + original.clone().including(&["src/app.py".to_string()]), + Some(original) + ); + } + + #[test] + fn including_too_many_files_falls_back_to_a_full_scan() { + let forced: Vec = (0..=MAX_CHANGED_FILES) + .map(|i| format!("v/{i}.js")) + .collect(); + assert_eq!(plan(&["src/app.py"]).including(&forced), None); + } + #[test] fn a_completed_clean_blast_scan_is_a_baseline() { assert!(is_usable_baseline(&scan("main", "abc"))); diff --git a/src/main.rs b/src/main.rs index 9fb413e..fd46f60 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,7 @@ mod authorize; mod cicd; mod config; mod images; +mod include_rules; mod incremental; mod inspect; mod list; @@ -40,6 +41,11 @@ struct Cli { args: Vec, } +// `Scan` carries by far the largest flag set of any subcommand, and exactly one +// `Commands` value exists per process — parsed once at startup and destructured +// immediately — so the wasted stack in the other variants costs nothing that +// boxing would recover. +#[allow(clippy::large_enum_variant)] #[derive(Subcommand, Debug)] enum Commands { /// Authenticate to Corgea @@ -157,6 +163,13 @@ enum Commands { )] exclude: Option, + #[arg( + long = "include", + value_name = "PATH", + help = "Force files into the scan that Corgea would otherwise skip as vendored, third-party, generated or test code (repeatable), e.g. --include src/myProj/MyClass.java --include 'vendor/our-fork/**'. Accepts a path, a directory, or a glob pattern, and overrides this command's packaging filters, .gitignore, --exclude and the engine's own classification. Force-included files are analyzed on every run, including incremental ones. Your project's include rules in Corgea apply too; this flag adds to them for one run." + )] + include: Vec, + #[arg( long, help = "The name of the Corgea project. Defaults to git repository name if found, otherwise to the current directory name." @@ -720,6 +733,7 @@ fn main() { out_file, target, exclude, + include, project_name, sbom, include_image, @@ -849,6 +863,11 @@ fn main() { std::process::exit(1); } + if !include.is_empty() && *scanner != Scanner::Blast { + ::log::error!("--include is only supported with the blast scanner."); + std::process::exit(1); + } + if sbom.is_some() && *scanner != Scanner::Blast { ::log::error!("sbom is only supported with blast scanner."); std::process::exit(1); @@ -904,6 +923,7 @@ fn main() { out_file.clone(), target.clone(), exclude.clone(), + include.clone(), project_name.clone(), sbom.clone(), include_images, diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index ca813fe..92c1da0 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -62,6 +62,7 @@ pub fn run( out_file: Option, target: Option, exclude: Option, + include: Vec, project_name: Option, sbom: Option, include_images: Vec, @@ -118,6 +119,7 @@ pub fn run( policy, target, exclude, + include, include_images, ), }; @@ -277,6 +279,19 @@ pub fn run( } } +/// Repo-relative paths as the `/`-separated strings the server's file lists use. +fn repo_relative_strings(paths: &[PathBuf]) -> Vec { + paths + .iter() + .map(|path| { + path.components() + .map(|component| component.as_os_str().to_string_lossy()) + .collect::>() + .join("/") + }) + .collect() +} + /// Package the project, upload it, and wait for the scan to finish. /// /// Returns the new scan's id and, when the server reported one, its project id. @@ -292,6 +307,7 @@ fn start_new_scan( policy: Option, target: Option, exclude: Option, + include: Vec, include_images: Vec, ) -> (String, Option) { println!("\nScanning with BLAST 🚀🚀🚀"); @@ -350,6 +366,33 @@ fn start_new_scan( // Before packaging: mid-pack HEAD move must not look like a clean new SHA. let repo_before = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); + // Resolved before packaging: the rules decide what goes into the archive, + // and a file left out of it cannot be scanned however the engine classifies + // what it did receive. + let include_rules = crate::include_rules::resolve( + config, + project_name, + repo_before + .as_ref() + .and_then(|info| info.repo_url.as_deref()), + &include, + ); + let force_included = include_rules.matching_files(Path::new(".")); + if !include_rules.is_empty() && force_included.is_empty() { + log::warn!( + "\n{}", + utils::terminal::set_text_color( + "⚠️ No files matched your include rules, so nothing was forced into this scan.", + utils::terminal::TerminalColor::Yellow + ) + ); + } else if !force_included.is_empty() { + println!( + "Force-including {} file(s) Corgea would otherwise skip.", + force_included.len() + ); + } + if target_str.is_none() && exclude.is_some() { println!("Excluding files matching: {}", exclude.as_deref().unwrap()); } @@ -449,6 +492,7 @@ fn start_new_scan( &zip_path, None, exclude.as_deref(), + &force_included, &extra_zip_files, ) { Ok(added_files) => { @@ -541,6 +585,10 @@ fn start_new_scan( repo_info.as_ref().is_some_and(|info| info.dirty), *ignore_dirty_worktree, ) + // Force-included files are usually unchanged, and the server carries + // findings forward for whatever the diff omits — so without this an + // include rule would never get the file looked at on an incremental run. + .and_then(|plan| plan.including(&repo_relative_strings(&force_included))) }; println!("\n\nSubmitting scan to Corgea:"); let upload_result = match utils::api::upload_zip( @@ -553,6 +601,7 @@ fn start_new_scan( policy, metadata, incremental: incremental_plan, + include_paths: include_rules.cli_patterns, }, ) { Ok(result) => result, diff --git a/src/utils/api.rs b/src/utils/api.rs index f02db74..9b1aa1b 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -243,6 +243,9 @@ pub struct UploadOptions { /// Set when this run resolved a diff for the server to analyze instead of /// the whole project. pub incremental: Option, + /// `--include` patterns for this run. The project's own include rules are + /// already stored server-side, so only the flag values are sent. + pub include_paths: Vec, } pub fn upload_zip( @@ -257,7 +260,18 @@ pub fn upload_zip( policy, metadata, incremental, + include_paths, } = options; + let include_paths_field = match include_paths.is_empty() { + true => None, + false => match serde_json::to_string(&include_paths) { + Ok(json) => Some(json), + Err(e) => { + debug(&format!("Could not serialize the --include patterns: {e}")); + None + } + }, + }; let client = http_client(); let file_size = std::fs::metadata(file_path)?.len(); let file_name = Path::new(file_path).file_name().unwrap().to_str().unwrap(); @@ -386,6 +400,9 @@ pub fn upload_zip( if let Some(meta) = &metadata { form = form.part("metadata", multipart::Part::text(meta.clone())); } + if let Some(patterns) = &include_paths_field { + form = form.part("include_paths", multipart::Part::text(patterns.clone())); + } // Both fields or neither: the list is only safe next to the commit it // was measured from, and a server seeing one without the other would // guess a baseline. A list that will not serialize drops both, leaving @@ -949,6 +966,63 @@ fn request_scan_list( } } +/// Project-level path rules a client needs before it packages a scan. +#[derive(Deserialize, Debug, Default, PartialEq, Eq)] +pub struct ScanSettings { + /// Patterns that force files into the scan even when Corgea would classify + /// them as vendored, third-party, generated or test code. + #[serde(default)] + pub include_paths: Vec, + #[serde(default)] + pub ignore_paths: Vec, +} + +#[derive(Deserialize, Debug, Default)] +struct ScanSettingsResponse { + #[serde(default)] + settings: ScanSettings, +} + +/// GET /api/v1/scan-settings — the project's ignore and include rules. +/// +/// `Ok(None)` only for a 404, which is a backend predating the endpoint: it has +/// no rules to apply, so the caller proceeds with just its own flags. Anything +/// else is an `Err`, because reading zero rules from a broken lookup and +/// reading zero rules from a project that has none are not the same thing. +pub fn query_scan_settings( + url: &str, + project_name: &str, + repo_url: Option<&str>, +) -> Result, Box> { + let request_url = format!("{}{}/scan-settings", url, API_BASE); + let client = http_client(); + let mut query = vec![("project_name", project_name.to_string())]; + if let Some(repo_url) = repo_url { + query.push(("repo_url", repo_url.to_string())); + } + debug(&format!( + "Reading project scan settings from {} ({:?})", + request_url, query + )); + let response = client.get(&request_url).query(&query).send()?; + check_for_warnings(response.headers(), response.status()); + let status = response.status(); + if status == StatusCode::NOT_FOUND { + return Ok(None); + } + if !status.is_success() { + return Err(format!("/scan-settings request failed: HTTP {}", status).into()); + } + let text = response.text()?; + match serde_json::from_str::(&text) { + Ok(parsed) => Ok(Some(parsed.settings)), + Err(e) => { + debug(&format!("/scan-settings response body: {}", text)); + Err(format!("Failed to parse the /scan-settings response: {}", e).into()) + } + } +} + #[derive(Deserialize, Debug)] pub struct ProjectSummary { pub name: String, diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 274ccd6..4fb8150 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -2,6 +2,7 @@ use crate::utils::terminal::{set_text_color, TerminalColor}; use git2::{Repository, StatusOptions}; use globset::{Glob, GlobSetBuilder}; use ignore::WalkBuilder; +use std::collections::HashSet; use std::env; use std::fs::{self, File}; use std::io; @@ -67,6 +68,10 @@ const DEFAULT_EXCLUDE_GLOBS: &[&str] = &[ /// - If `target` is `Some(target_str)`, resolves the target using the targets module and creates zip from those files. /// The target string can be a comma-separated list of files, directories, globs, or git selectors. /// - `user_exclude` is an optional comma-separated list of glob patterns from `--exclude`. +/// - `force_include` are repo-relative paths the project's include rules or +/// `--include` matched. They override every filter here — the default +/// excludes, `--exclude`, and `.gitignore` — because a file left out of the +/// archive cannot be scanned whatever the engine later decides about it. /// - `extra_files` are staged files added to the root of the zip as /// `(source path, zip entry name)`. They come from explicit flags such as /// `--include-image`, so exclude rules don't apply to them. @@ -75,6 +80,7 @@ pub fn create_zip_from_target>( output_zip: P, exclude_globs: Option<&[&str]>, user_exclude: Option<&str>, + force_include: &[PathBuf], extra_files: &[(PathBuf, String)], ) -> Result, Box> { let exclude_globs = exclude_globs.unwrap_or(DEFAULT_EXCLUDE_GLOBS); @@ -88,7 +94,7 @@ pub fn create_zip_from_target>( let user_exclude_glob_set = crate::targets::build_user_exclude_glob_set(user_exclude) .map_err(|e| format!("Failed to build exclude patterns: {}", e))?; - let files_to_zip: Vec<(PathBuf, PathBuf)> = if let Some(target_str) = target { + let mut files_to_zip: Vec<(PathBuf, PathBuf)> = if let Some(target_str) = target { let current_dir = env::current_dir()?; let result = crate::targets::resolve_targets_with_exclude(target_str, user_exclude) .map_err(|e| format!("Failed to resolve targets: {}", e))?; @@ -132,6 +138,20 @@ pub fn create_zip_from_target>( files }; + let forced: HashSet<&Path> = force_include.iter().map(PathBuf::as_path).collect(); + let already_present: HashSet = files_to_zip + .iter() + .map(|(_, relative)| relative.clone()) + .collect(); + for relative in force_include { + if already_present.contains(relative) { + continue; + } + if relative.is_file() { + files_to_zip.push((relative.clone(), relative.clone())); + } + } + let zip_file = File::create(output_zip.as_ref())?; let mut zip = ZipWriter::new(zip_file); @@ -144,7 +164,8 @@ pub fn create_zip_from_target>( for (path, relative_path) in files_to_zip { // Match repo-relative paths so abs `/tmp/...` targets don't hit `**/tmp/**`. - let is_excluded = glob_set.is_match(&relative_path); + let is_excluded = + glob_set.is_match(&relative_path) && !forced.contains(relative_path.as_path()); if (path.is_file() || path.is_dir()) && !is_excluded { if path.is_file() { @@ -918,8 +939,9 @@ mod tests { // which would exclude *everything*. The filter + warn path under test // is identical either way. let excludes: &[&str] = &["**/node_modules/**"]; - let added = create_zip_from_target(Some(&target), &output_zip, Some(excludes), None, &[]) - .expect("zip creation should succeed"); + let added = + create_zip_from_target(Some(&target), &output_zip, Some(excludes), None, &[], &[]) + .expect("zip creation should succeed"); assert!( added.iter().any(|p| p.ends_with("src/main.py")), @@ -933,6 +955,38 @@ mod tests { ); } + /// A force-include rule is the customer overruling Corgea's own judgement + /// about a file, so it has to beat the default excludes. + #[test] + fn create_zip_from_target_keeps_force_included_files() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + + let node_modules = root.join("node_modules"); + fs::create_dir_all(&node_modules).unwrap(); + let forced = node_modules.join("internal-sdk.js"); + fs::write(&forced, "console.log(1)").unwrap(); + let excluded = node_modules.join("third-party.js"); + fs::write(&excluded, "console.log(2)").unwrap(); + + let output_zip = root.join("out.zip"); + let target = format!("{},{}", forced.display(), excluded.display()); + // Explicit file targets outside the cwd keep their absolute paths as + // zip entry names, so that is the shape the exemption check compares. + let added = create_zip_from_target( + Some(&target), + &output_zip, + Some(&["**/node_modules/**"]), + None, + std::slice::from_ref(&forced), + &[], + ) + .expect("zip creation should succeed"); + + assert!(added.contains(&forced), "force-included: {:?}", added); + assert!(!added.contains(&excluded), "still excluded: {:?}", added); + } + /// The staging directory holds the project zip and exported images, so other /// local users must not be able to read it. #[cfg(unix)] @@ -967,6 +1021,7 @@ mod tests { &output_zip, Some(&[]), None, + &[], &extra_files, ) .expect("zip creation should succeed"); @@ -1009,6 +1064,7 @@ mod tests { &output_zip, Some(&[]), None, + &[], &extra_files, ) .expect("a >4 GiB entry needs ZIP64, not an error"); diff --git a/tests/cli_scan_include.rs b/tests/cli_scan_include.rs new file mode 100644 index 0000000..97d1a6a --- /dev/null +++ b/tests/cli_scan_include.rs @@ -0,0 +1,229 @@ +//! End-to-end coverage for force-include rules: drives the real binary through +//! the blast scan flow against a stubbed HTTP server and asserts that files the +//! packager would normally leave out — `node_modules`, `.gitignore`d paths, +//! `--exclude`d paths — are bundled when `--include` or the project's own +//! include rules name them, and that the rules travel with the upload. + +mod common; + +use common::corgea_isolated; +use std::fs; +use std::io::Write; +use std::net::TcpListener; +use std::sync::{Arc, Mutex}; +use tempfile::TempDir; + +/// Raw bodies of the chunk uploads the CLI sent. +type Uploads = Arc>>>; + +/// The blast scan route table, answering `/scan-settings` with `include_paths` +/// so a test can exercise the platform-configured rules as well as the flag. +fn spawn_scan_stub( + scan_id: &'static str, + project_include_paths: &'static str, +) -> (String, Uploads) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind stub"); + let base_url = format!("http://127.0.0.1:{}", listener.local_addr().unwrap().port()); + let uploads: Uploads = Default::default(); + let recorder = Arc::clone(&uploads); + + std::thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { continue }; + let request = corgea::vuln_api_stub::read_http_request(&mut stream); + let request_line = String::from_utf8_lossy(&request[..request.len().min(1024)]) + .lines() + .next() + .unwrap_or("") + .to_string(); + let target = request_line.split_whitespace().nth(1).unwrap_or(""); + let path = target.split('?').next().unwrap_or(target); + + let (status, body) = if path == "/api/v1/verify" { + ("200 OK", r#"{"status":"ok"}"#.to_string()) + } else if path == "/api/v1/scan-settings" { + ( + "200 OK", + format!( + r#"{{"status":"ok","project":null,"settings":{{"include_paths":{},"ignore_paths":[]}}}}"#, + project_include_paths + ), + ) + } else if path == "/api/v1/start-scan" { + ("200 OK", r#"{"transfer_id":"transfer-1"}"#.to_string()) + } else if path == "/api/v1/start-scan/transfer-1/" { + recorder.lock().unwrap().push(request.clone()); + ( + "200 OK", + format!(r#"{{"scan_id":"{}","project_id":"1"}}"#, scan_id), + ) + } else if path == format!("/api/v1/scan/{}", scan_id) { + ( + "200 OK", + format!( + r#"{{"id":"{}","project":"proj","repo":null,"branch":null,"status":"complete","engine":"blast","created_at":"2026-01-01T00:00:00Z"}}"#, + scan_id + ), + ) + } else if path == format!("/api/v1/scan/{}/issues", scan_id) { + ( + "200 OK", + r#"{"status":"ok","issues":[],"page":1,"total_pages":1,"total_issues":0}"# + .to_string(), + ) + } else { + ("404 Not Found", r#"{"message":"not found"}"#.to_string()) + }; + + let response = corgea::vuln_api_stub::http_response(status, "", &body); + let _ = stream.write_all(response.as_bytes()); + } + }); + + (base_url, uploads) +} + +/// Everything the CLI uploaded, as lossy text. Zip entry names are stored +/// verbatim in each local file header, so searching for a path here proves it +/// was bundled. +fn uploaded_text(uploads: &Uploads) -> String { + let uploads = uploads.lock().expect("upload log"); + assert!(!uploads.is_empty(), "no chunk upload was recorded"); + uploads + .iter() + .map(|chunk| String::from_utf8_lossy(chunk).into_owned()) + .collect() +} + +/// A project whose proprietary code sits where Corgea assumes dependencies live. +fn stub_project() -> TempDir { + let project = TempDir::new().expect("project dir"); + let root = project.path(); + fs::write(root.join("main.py"), "print(1)\n").expect("write source file"); + fs::create_dir_all(root.join("node_modules/internal-sdk")).expect("create vendor dir"); + fs::write( + root.join("node_modules/internal-sdk/index.js"), + "module.exports = 1;\n", + ) + .expect("write force-include candidate"); + fs::create_dir_all(root.join("node_modules/third-party")).expect("create dependency dir"); + fs::write( + root.join("node_modules/third-party/index.js"), + "module.exports = 2;\n", + ) + .expect("write third-party file"); + project +} + +fn scan(base_url: &str, project: &TempDir, args: &[&str]) -> std::process::Output { + let (mut cmd, _home) = corgea_isolated(); + cmd.current_dir(project.path()) + .env("CORGEA_URL", base_url) + .env("CORGEA_TOKEN", "test-token") + .arg("scan") + .args(args); + let output = cmd.output().expect("run corgea scan"); + assert!( + output.status.success(), + "stdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + output +} + +#[test] +fn include_flag_bundles_a_file_the_default_excludes_would_drop() { + let (base_url, uploads) = spawn_scan_stub("scan-include-flag", "[]"); + let project = stub_project(); + + let output = scan( + &base_url, + &project, + &["--include", "node_modules/internal-sdk/index.js"], + ); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Force-including 1 file(s)"), + "should report what it forced in, got:\n{stdout}" + ); + + let uploaded = uploaded_text(&uploads); + assert!( + uploaded.contains("node_modules/internal-sdk/index.js"), + "the force-included file should be bundled" + ); + assert!( + !uploaded.contains("node_modules/third-party/index.js"), + "other node_modules files stay excluded" + ); + assert!(uploaded.contains("main.py"), "source files still upload"); + // The server does not know this run's flag values, so they travel with it. + assert!( + uploaded.contains(r#"["node_modules/internal-sdk/index.js"]"#), + "the --include patterns should be sent with the upload" + ); +} + +#[test] +fn project_include_rules_from_the_platform_are_applied() { + let (base_url, uploads) = + spawn_scan_stub("scan-include-project", r#"["node_modules/internal-sdk"]"#); + let project = stub_project(); + + let output = scan(&base_url, &project, &[]); + + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("Applying 1 project include rule(s) from Corgea"), + "should say the rules came from the platform, got:\n{stdout}" + ); + + let uploaded = uploaded_text(&uploads); + assert!(uploaded.contains("node_modules/internal-sdk/index.js")); + assert!(!uploaded.contains("node_modules/third-party/index.js")); + // Already stored server-side, so nothing to send back. + assert!(!uploaded.contains(r#"name="include_paths""#)); +} + +#[test] +fn an_include_rule_overrides_exclude_patterns() { + let (base_url, uploads) = spawn_scan_stub("scan-include-exclude", "[]"); + let project = stub_project(); + fs::write(project.path().join(".gitignore"), "generated/\n").expect("write gitignore"); + fs::create_dir_all(project.path().join("generated")).expect("create generated dir"); + fs::write( + project.path().join("generated/Payments.java"), + "class Payments {}\n", + ) + .expect("write generated file"); + + scan( + &base_url, + &project, + &[ + "--exclude", + "generated/**", + "--include", + "generated/Payments.java", + ], + ); + + assert!(uploaded_text(&uploads).contains("generated/Payments.java")); +} + +#[test] +fn an_include_rule_that_matches_nothing_warns_and_still_scans() { + let (base_url, uploads) = spawn_scan_stub("scan-include-nomatch", "[]"); + let project = stub_project(); + + let output = scan(&base_url, &project, &["--include", "no/such/path.java"]); + + assert!( + String::from_utf8_lossy(&output.stderr).contains("No files matched your include rules"), + "stderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(uploaded_text(&uploads).contains("main.py")); +} diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index b9548d3..8325441 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -596,6 +596,24 @@ pub(crate) fn verify_request() -> ExpectedRequest { ) } +/// Every new BLAST scan reads the project's include rules before packaging, so +/// files Corgea would classify away can still be forced into the archive. +pub(crate) fn scan_settings_request(project: &str) -> ExpectedRequest { + let project = project.to_string(); + expected_request( + "read project include rules", + move |request| { + assert_authenticated_request(request, Method::GET, "/api/v1/scan-settings")?; + assert_query(request, "project_name", &project) + }, + json_response(json!({ + "status": "ok", + "project": null, + "settings": {"include_paths": [], "ignore_paths": []} + })), + ) +} + pub(crate) fn scan_response(scan_id: &str, project: &str, status: &str) -> Value { json!({ "id": scan_id, @@ -814,7 +832,7 @@ pub(crate) fn blast_upload_plan(sha: &str, dirty: bool, include_sca: bool) -> Ve let patch_path = "/api/v1/start-scan/transfer-123/".to_string(); let detail_path = "/api/v1/scan/blast-scan-123".to_string(); let issue_path = "/api/v1/scan/blast-scan-123/issues".to_string(); - let mut plan = vec![verify_request()]; + let mut plan = vec![verify_request(), scan_settings_request("cloud-e2e")]; // Scans are incremental by default, so every clean-tree run looks for a // baseline before uploading -- once per trunk branch, since the fixture // records no origin/HEAD. Answering with no scans keeps this the full-scan diff --git a/tests/cloud_commands_e2e/scan_incremental.rs b/tests/cloud_commands_e2e/scan_incremental.rs index c9385ba..21d9972 100644 --- a/tests/cloud_commands_e2e/scan_incremental.rs +++ b/tests/cloud_commands_e2e/scan_incremental.rs @@ -130,6 +130,7 @@ fn the_upload_carries_the_baseline_commit_and_the_files_that_changed_since_it() let expected_base = base_sha.clone(); let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), baseline_lookup("main", vec![baseline_scan(&base_sha)]), start_upload(), expected_request( @@ -180,7 +181,7 @@ fn a_project_with_no_baseline_scan_uploads_without_a_diff() { let head_sha = second_commit(&project); let patch_sha = head_sha.clone(); - let mut plan = vec![verify_request()]; + let mut plan = vec![verify_request(), scan_settings_request(PROJECT)]; plan.extend(baseline_lookups_finding_nothing()); plan.extend([ start_upload(), @@ -234,6 +235,7 @@ fn a_baseline_on_a_later_page_is_still_found() { let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), baseline_lookup_page("main", 1, 2, vec![unusable]), baseline_lookup_page("main", 2, 2, vec![baseline_scan(&base_sha)]), start_upload(), @@ -273,6 +275,7 @@ fn a_failed_lookup_is_not_reported_as_a_missing_baseline() { let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), expected_request( "fail the baseline lookup", |request| assert_baseline_lookup_request(request, PROJECT, "main"), @@ -323,6 +326,7 @@ fn disable_incremental_does_not_even_look_for_a_baseline() { let patch_sha = head_sha.clone(); let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), start_upload(), expected_request( "upload BLAST archive with no diff", @@ -372,6 +376,7 @@ fn a_narrowed_archive_skips_incremental_without_claiming_a_full_scan() { let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), start_upload(), expected_request( "upload narrowed BLAST archive", @@ -420,6 +425,7 @@ fn a_directory_that_is_not_a_git_repository_scans_everything() { let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), start_upload(), expected_request( "upload BLAST archive with no repo metadata", @@ -466,6 +472,7 @@ fn ignore_dirty_worktree_diffs_the_working_tree_instead_of_refusing() { let expected_base = base_sha.clone(); let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), baseline_lookup("main", vec![baseline_scan(&base_sha)]), start_upload(), expected_request( @@ -524,6 +531,7 @@ fn a_dirty_worktree_skips_the_baseline_lookup_and_scans_everything() { let patch_sha = head_sha.clone(); let mut plan = vec![ verify_request(), + scan_settings_request(PROJECT), start_upload(), expected_request( "upload BLAST archive with no diff", diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index 6b073b1..6a292cd 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -446,9 +446,11 @@ fn ignore_dirty_worktree_still_uploads_dirty_when_nothing_is_reused() { let project = git_project(); std::fs::write(project.path().join("main.py"), "print('dirty')\n") .expect("modify tracked file"); + // blast_upload_plan already holds verify then the include-rule lookup; the + // baselines follow it and the reuse lookup precedes it. let mut plan = blast_upload_plan(&project.sha, true, false); - plan.insert(1, baseline_lookup_for_branch("master", vec![])); - plan.insert(1, baseline_lookup_for_branch("main", vec![])); + plan.insert(2, baseline_lookup_for_branch("master", vec![])); + plan.insert(2, baseline_lookup_for_branch("main", vec![])); plan.insert(1, commit_lookup(&project.sha, vec![])); let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); From b85a4c5170b521e0477961c019f05b2478762af3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 31 Aug 2026 12:03:49 +0000 Subject: [PATCH 2/3] Refuse scan reuse alongside an include rule A reused scan predates the include rule, so skipping would leave the very file the run mandated unscanned. Under-reporting, unlike --exclude's over-reporting, so clap refuses the combination rather than warning. Co-authored-by: ibrahim --- skills/corgea/SKILL.md | 2 +- src/main.rs | 4 ++-- tests/cloud_commands_e2e/scan_skip.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/skills/corgea/SKILL.md b/skills/corgea/SKILL.md index c57502d..bfbedd0 100644 --- a/skills/corgea/SKILL.md +++ b/skills/corgea/SKILL.md @@ -91,7 +91,7 @@ An included image is enough on its own: when it is combined with `--only-uncommi `--skip-if-commit-scanned-recently` reuses the project's most recent reusable scan of the current commit instead of starting a duplicate, when one ran inside the `--scanned-within` window (default `24h`; accepts `90s`, `30m`, `4h`, `7d`, and a bare number as hours). The reused scan takes the new scan's place for the rest of the command — results table, `--block-on` gate and its exit code, `--out-file` report — so the pipeline behaves the same either way. It prints `CORGEA_SCAN_SKIPPED=true` plus `CORGEA_SCAN_ID=` on a reuse and `CORGEA_SCAN_SKIPPED=false` when a scan runs, so a later step can branch on it. -Reuse requires a candidate that answers the same question: a completed `corgea-blast` scan of that commit, on a branch rather than a pull request, from an explicitly clean worktree, reporting no scanner problems. Anything else runs a real scan (nothing in the window, a failed or still-running scan, a worktree `git status` reports changes in, or a failed lookup). `--ignore-dirty-worktree` (requires `--skip-if-commit-scanned-recently`) overrides the dirty-worktree half of that test: reuse proceeds even if this worktree is dirty or the prior scan recorded `worktree_dirty=true`. A prior scan that never reported the flag is still not reused. A new scan still reports the real dirty status. An unresolvable commit is a hard error (exit 1). Because the API exposes neither a scan's configured scan types and target policies nor whether it bundled a container image, a run that changes what gets scanned cannot be matched against a candidate, so the flag cannot be combined with `--scan-type`, `--policy`, `--include-image`, `--only-uncommitted`, or `--target`. `--exclude` is allowed but warns on a skip: what gets reused is a scan of the whole commit, so the results and the gate can cover files the run would have skipped (over-reporting, never under-reporting). +Reuse requires a candidate that answers the same question: a completed `corgea-blast` scan of that commit, on a branch rather than a pull request, from an explicitly clean worktree, reporting no scanner problems. Anything else runs a real scan (nothing in the window, a failed or still-running scan, a worktree `git status` reports changes in, or a failed lookup). `--ignore-dirty-worktree` (requires `--skip-if-commit-scanned-recently`) overrides the dirty-worktree half of that test: reuse proceeds even if this worktree is dirty or the prior scan recorded `worktree_dirty=true`. A prior scan that never reported the flag is still not reused. A new scan still reports the real dirty status. An unresolvable commit is a hard error (exit 1). Because the API exposes neither a scan's configured scan types and target policies nor whether it bundled a container image, a run that changes what gets scanned cannot be matched against a candidate, so the flag cannot be combined with `--scan-type`, `--policy`, `--include-image`, `--include`, `--only-uncommitted`, or `--target`. `--exclude` is allowed but warns on a skip: what gets reused is a scan of the whole commit, so the results and the gate can cover files the run would have skipped (over-reporting, never under-reporting). ### Upload — `corgea upload [report]` diff --git a/src/main.rs b/src/main.rs index fd46f60..ef97de9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -194,8 +194,8 @@ enum Commands { #[arg( long = "skip-if-commit-scanned-recently", - conflicts_with_all = ["only_uncommitted", "target", "scan_type", "policy", "include_image", "disable_incremental"], - help = "Do not start a new scan when this commit already has a recent completed scan in the project. That scan then drives the rest of the command — results table, --block-on gate, --out-file report — so the pipeline behaves the same either way. Prints CORGEA_SCAN_SKIPPED=true/false so a pipeline can tell the two apart, and fails if no git commit can be resolved. What can be reused is a scan of the whole commit, and no API tells this run how a past scan was scoped or configured, so the flag is refused with --only-uncommitted, --target, --scan-type, --policy, --include-image and --disable-incremental; with --exclude it warns instead, since a reused scan covers files this run would have skipped." + conflicts_with_all = ["only_uncommitted", "target", "scan_type", "policy", "include_image", "include", "disable_incremental"], + help = "Do not start a new scan when this commit already has a recent completed scan in the project. That scan then drives the rest of the command — results table, --block-on gate, --out-file report — so the pipeline behaves the same either way. Prints CORGEA_SCAN_SKIPPED=true/false so a pipeline can tell the two apart, and fails if no git commit can be resolved. What can be reused is a scan of the whole commit, and no API tells this run how a past scan was scoped or configured, so the flag is refused with --only-uncommitted, --target, --scan-type, --policy, --include-image, --include and --disable-incremental; with --exclude it warns instead, since a reused scan covers files this run would have skipped." )] skip_if_commit_scanned_recently: bool, diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index 6a292cd..7975052 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -676,6 +676,33 @@ fn the_window_cannot_be_set_without_the_skip_flag() { ); } +/// A force-include rule widens what gets scanned, and the reused candidate was +/// scanned without it — so reuse would silently skip the very file the run +/// mandated. That is under-reporting, which the flag refuses rather than warns. +#[test] +fn reuse_is_refused_alongside_an_include_rule() { + let api = ApiStub::start(Vec::new()); + let project = git_project(); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "scan", + "blast", + "--skip-if-commit-scanned-recently", + "--include", + "vendor/our-fork/**", + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + + assert_eq!(output.status.code(), Some(2), "{context}"); + assert!( + String::from_utf8_lossy(&output.stderr).contains("--include"), + "{context}" + ); +} + /// `--ignore-dirty-worktree` stands alone now: it governs the incremental diff /// as well as reuse, so a run may pass it without the reuse flag. Covered end /// to end in `scan_incremental`; this only asserts clap accepts it. From 18a84a8ae4997a00c2a677050eb283cabbdf9bfa Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 9 Sep 2026 13:10:47 +0000 Subject: [PATCH 3/3] Address force-include review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse: resolve include rules in run(), before resolve_reusable_scan. Only the --include flag conflicted with --skip-if-commit-scanned-recently, and the rules were read inside start_new_scan — after reuse had already been chosen. So the primary path, a rule configured in the web app with no flag, could reuse a scan that never packaged the files the rule forces in. Project rules and a failed lookup both refuse reuse now, and the refusal still prints CORGEA_SCAN_SKIPPED=false, which the flag promises on every run. Empty target: a force-included file is a complete payload, like an exported image, so --target matching nothing no longer aborts a run whose include rule did match. Credentials: strip userinfo from the git remote before it reaches the /scan-settings query string and the debug log. Only scheme URLs are touched; scp-style git@host:path carries no secret and stripping it would stop the server normalizing it to the stored URL. Also: reject invalid and repo-wide --include patterns up front instead of dropping them locally while still uploading them; sort matches before applying the 5k cap so equivalent runs force in the same files; report traversal errors rather than letting an unreadable directory turn an include into a silent no-match; cap the settings lookup at 15s, since the run continues without it; and print the forced paths, which override .gitignore and **/*.env. Co-authored-by: ibrahim --- skills/corgea/SKILL.md | 6 +- src/include_rules.rs | 171 +++++++++++++++++++++++-- src/main.rs | 8 ++ src/scanners/blast.rs | 73 ++++++++--- src/skip_scan.rs | 9 ++ src/utils/api.rs | 21 ++- src/utils/generic.rs | 51 ++++++++ tests/cli_scan_include.rs | 52 ++++++++ tests/cloud_commands_e2e/common/mod.rs | 21 ++- tests/cloud_commands_e2e/scan_skip.rs | 66 ++++++++-- 10 files changed, 425 insertions(+), 53 deletions(-) diff --git a/skills/corgea/SKILL.md b/skills/corgea/SKILL.md index bfbedd0..6539306 100644 --- a/skills/corgea/SKILL.md +++ b/skills/corgea/SKILL.md @@ -83,7 +83,11 @@ Scan types: `blast` (base AI), `policy` (PolicyIQ), `malicious`, `secrets`, `pii An included image is enough on its own: when it is combined with `--only-uncommitted` or `--target` and no source files match (a clean working tree, for example), the scan warns and covers just the image rather than failing. An archive named `corgea-image-scanning-*.tar` that is committed to the repository is ignored — only images passed on the command line are scanned. -`--include` forces files into the scan that Corgea would otherwise skip because it classified them as vendored, third-party, generated or test code. It overrides the CLI's own packaging filters, `.gitignore`, `--exclude`, and the engine's classification, and force-included files are analyzed on every run — including incremental ones, where an unchanged file would normally have its previous findings carried forward. Use it when proprietary code lives somewhere Corgea assumes dependencies live, e.g. a fork checked into `vendor/`. Project-level include rules configured in the web app (scan settings → File Include Rules) are fetched before packaging and applied too; `--include` adds to them for one run. +`--include` forces files into the scan that Corgea would otherwise skip because it classified them as vendored, third-party, generated or test code. It overrides the CLI's own packaging filters, `.gitignore`, `--exclude`, and the engine's classification, and force-included files are analyzed on every run — including incremental ones, where an unchanged file would normally have its previous findings carried forward. Use it when proprietary code lives somewhere Corgea assumes dependencies live, e.g. a fork checked into `vendor/`. Project-level include rules configured in the web app (scan settings → File Include Rules) are fetched at the start of every run and applied too; `--include` adds to them for one run. The paths actually forced in are printed, because they override `.gitignore` and the default excludes (which cover `**/*.env` among others). + +A pattern that matches the whole repository (`**`, `*`, `/**`) is refused, by both the flag and the web app: include beats exclude, so it would disable every configured exclusion for the scan, including ones set to keep sensitive paths out. Name a directory or file instead. + +Include rules also take scan reuse off the table. A reusable scan ran before the rules existed, so its results cannot cover the files they force in — a run with any include rule (from the flag or the project) starts a real scan and prints `CORGEA_SCAN_SKIPPED=false`, as does a run whose rule lookup failed. `--only-uncommitted` and `--target` are mutually exclusive. `--fail-on`, `--fail`, and `--block-on` are mutually exclusive. diff --git a/src/include_rules.rs b/src/include_rules.rs index ee14355..a0e1554 100644 --- a/src/include_rules.rs +++ b/src/include_rules.rs @@ -28,6 +28,9 @@ pub struct IncludeRules { pub patterns: Vec, /// Just the `--include` values, the ones the server does not know yet. pub cli_patterns: Vec, + /// Whether the project's rules could not be read. Distinct from "no rules": + /// this run does not know what the project asked for. + pub lookup_failed: bool, } impl IncludeRules { @@ -35,21 +38,64 @@ impl IncludeRules { self.patterns.is_empty() } + /// Why a previous scan must not be reused for this run, if it must not be. + /// + /// A reusable candidate was scanned before these rules existed, so its + /// results omit the files they force in — under-reporting, which is the + /// exact failure this feature exists to fix, and the reason `--include` + /// already refuses `--skip-if-commit-scanned-recently` at the flag level. + /// Project rules have the same semantics but clap cannot see them, so they + /// are checked here. A failed lookup refuses too: not knowing whether the + /// project has rules is not the same as knowing it has none, and a real + /// scan is only slower. + pub fn reuse_refusal(&self) -> Option { + if self.lookup_failed { + return Some( + "this project's include rules could not be read, so a reused scan \ + cannot be shown to cover them" + .to_string(), + ); + } + if self.patterns.is_empty() { + return None; + } + Some(format!( + "this project has {} include rule(s), and a scan from before they \ + applied would not cover the files they force in", + self.patterns.len() + )) + } + /// Paths under `root` that the rules match, relative to `root`. /// /// Walks with the standard ignore filters off, since the point is to reach /// files `.gitignore` and the default excludes hide. `.git` is still /// skipped: it holds no source and its object store is large. + /// + /// Sorted before the cap is applied, so two runs over the same tree force + /// in the same files — filesystem traversal order is not a stable contract. + /// A traversal error is reported rather than swallowed: an unreadable + /// directory would otherwise turn an explicit include into a silent + /// no-match. pub fn matching_files(&self, root: &Path) -> Vec { let Some(glob_set) = build_glob_set(&self.patterns) else { return Vec::new(); }; let mut matches = Vec::new(); + let mut walk_errors = 0; let walker = WalkBuilder::new(root) .standard_filters(false) .filter_entry(|entry| entry.file_name() != ".git") .build(); - for entry in walker.flatten() { + for result in walker { + let entry = match result { + Ok(entry) => entry, + Err(e) => { + walk_errors += 1; + log::debug!("Include rules: could not read a path: {e}"); + continue; + } + }; if !entry.file_type().is_some_and(|kind| kind.is_file()) { continue; } @@ -59,20 +105,63 @@ impl IncludeRules { if glob_set.is_match(relative) { matches.push(relative.to_path_buf()); } - if matches.len() >= MAX_FORCE_INCLUDED_FILES { - log::warn!( - "Include rules matched more than {} files; only the first {} are forced into this scan.", - MAX_FORCE_INCLUDED_FILES, - MAX_FORCE_INCLUDED_FILES - ); - break; - } + } + if walk_errors > 0 { + log::warn!( + "Include rules: {walk_errors} path(s) could not be read, so files they \ + hold were not force-included. Run with --verbose for details." + ); } matches.sort(); + if matches.len() > MAX_FORCE_INCLUDED_FILES { + log::warn!( + "Include rules matched {} files; only the first {} are forced into this scan.", + matches.len(), + MAX_FORCE_INCLUDED_FILES + ); + matches.truncate(MAX_FORCE_INCLUDED_FILES); + } matches } } +/// A pattern that names the repository rather than anything inside it. +/// +/// Include beats exclude, so `**` would pull every `.gitignore`d and +/// default-excluded file into the archive — `.env` and `*.pem` among them. Kept +/// in step with doghouse's `pattern_matches_whole_repo`. +fn matches_whole_repo(pattern: &str) -> bool { + let stripped = pattern.trim().trim_matches('/'); + stripped.is_empty() || stripped.split('/').all(|s| s == "*" || s == "**") +} + +/// Check `--include` values before the scan starts. +/// +/// Rejected rather than dropped: a pattern globset cannot compile packages +/// nothing locally, yet would still be uploaded, so the command would appear to +/// succeed having ignored what the user explicitly asked for. Server-supplied +/// patterns stay tolerant (see `build_glob_set`) — one bad rule in the web app +/// must not stop a scan. +pub fn validate_cli_patterns(patterns: &[String]) -> Result<(), String> { + for pattern in patterns { + let trimmed = pattern.trim(); + if trimmed.is_empty() { + return Err("--include needs a path, directory or glob pattern.".to_string()); + } + if matches_whole_repo(trimmed) { + return Err(format!( + "--include '{trimmed}' matches the whole repository, which would \ + override every exclude rule including .gitignore. Name a directory \ + or file instead, e.g. --include vendor/our-fork/**." + )); + } + if let Err(e) = Glob::new(trimmed) { + return Err(format!("--include '{trimmed}' is not a valid pattern: {e}")); + } + } + Ok(()) +} + /// Build a matcher, dropping patterns globset cannot compile. /// /// One unparseable pattern must not discard the rest: the others are still a @@ -118,6 +207,7 @@ pub fn resolve( ) -> IncludeRules { let cli_patterns = normalize_patterns(cli_include); let mut patterns = Vec::new(); + let mut lookup_failed = false; match api::query_scan_settings(&config.get_url(), project_name, repo_url) { Ok(Some(settings)) => { @@ -132,11 +222,15 @@ pub fn resolve( ); } } - // A backend without the endpoint has no include rules to apply either. + // A backend without the endpoint has no include rules to apply either, + // so this is a clean "none", not a failure. Ok(None) => {} - Err(e) => log::warn!( - "Could not read the project's include rules, so only --include applies to this run: {e}" - ), + Err(e) => { + lookup_failed = true; + log::warn!( + "Could not read the project's include rules, so only --include applies to this run: {e}" + ); + } } for pattern in &cli_patterns { @@ -145,6 +239,7 @@ pub fn resolve( IncludeRules { patterns, cli_patterns, + lookup_failed, } } @@ -174,7 +269,7 @@ mod tests { fn rules(patterns: &[&str]) -> IncludeRules { IncludeRules { patterns: patterns.iter().map(|p| p.to_string()).collect(), - cli_patterns: Vec::new(), + ..Default::default() } } @@ -205,6 +300,54 @@ mod tests { assert!(build_glob_set(&["src/**".to_string(), "[".to_string()]).is_some()); } + /// A reused scan predates the rules, so it cannot cover the files they + /// force in. Not knowing whether the project has rules refuses too — a real + /// scan is only slower, while under-reporting is the bug being fixed. + #[test] + fn reuse_is_refused_when_rules_exist_or_could_not_be_read() { + assert!(IncludeRules::default().reuse_refusal().is_none()); + assert!(rules(&["vendor/our-fork/**"]).reuse_refusal().is_some()); + assert!(IncludeRules { + lookup_failed: true, + ..Default::default() + } + .reuse_refusal() + .is_some()); + } + + #[test] + fn repo_wide_cli_patterns_are_rejected_before_the_scan() { + for pattern in ["**", "*", "/**", "**/*", " ", "/"] { + assert!( + validate_cli_patterns(&[pattern.to_string()]).is_err(), + "{pattern:?} should be refused" + ); + } + assert!(validate_cli_patterns(&["[".to_string()]).is_err()); + assert!(validate_cli_patterns(&[ + "vendor/our-fork/**".to_string(), + "src/A.java".to_string(), + ]) + .is_ok()); + } + + /// The cap has to be applied to a sorted list: traversal order is not a + /// stable contract, so truncating first lets two runs over the same tree + /// force in different files. + #[test] + fn matching_files_is_sorted_before_the_cap_applies() { + let root = TempDir::new().unwrap(); + for i in 0..(MAX_FORCE_INCLUDED_FILES + 10) { + write(&root, &format!("vendor/f{i:05}.java")); + } + + let matched = rules(&["vendor/**"]).matching_files(root.path()); + + assert_eq!(matched.len(), MAX_FORCE_INCLUDED_FILES); + assert!(matched.windows(2).all(|w| w[0] <= w[1])); + assert_eq!(matched[0], PathBuf::from("vendor/f00000.java")); + } + #[test] fn matching_files_reaches_gitignored_and_vendored_paths() { let root = TempDir::new().unwrap(); diff --git a/src/main.rs b/src/main.rs index ef97de9..3951277 100644 --- a/src/main.rs +++ b/src/main.rs @@ -868,6 +868,14 @@ fn main() { std::process::exit(1); } + // Checked up front rather than dropped later: a pattern that cannot + // compile packages nothing, so the scan would appear to succeed + // having silently ignored what was asked for. + if let Err(msg) = include_rules::validate_cli_patterns(include) { + ::log::error!("{}", msg); + std::process::exit(1); + } + if sbom.is_some() && *scanner != Scanner::Blast { ::log::error!("sbom is only supported with blast scanner."); std::process::exit(1); diff --git a/src/scanners/blast.rs b/src/scanners/blast.rs index 92c1da0..3b7de16 100644 --- a/src/scanners/blast.rs +++ b/src/scanners/blast.rs @@ -13,6 +13,10 @@ use std::thread; use std::time::{Duration, Instant}; /// Overrides how long `wait_for_scan` polls before giving up. +/// How many force-included paths to name before collapsing the rest to a count. +/// Same shape as the `--target` file preview. +const FORCE_INCLUDE_PREVIEW: usize = 20; + const SCAN_TIMEOUT_ENV: &str = "CORGEA_SCAN_TIMEOUT_SECONDS"; const DEFAULT_SCAN_TIMEOUT: Duration = Duration::from_secs(10 * 60 * 60); @@ -93,10 +97,32 @@ pub fn run( let project_name = utils::generic::determine_project_name(project_name.as_deref()); + // Resolved before the reuse decision, not inside start_new_scan: a + // reusable scan predates any include rule, so reusing it would leave the + // files the rule forces in unscanned. `--include` already refuses the reuse + // flag in clap; the project's own rules are invisible to clap, so they are + // checked here. + let include_rules = crate::include_rules::resolve( + config, + &project_name, + utils::generic::get_repo_info_for_scan("./") + .unwrap_or_default() + .and_then(|info| info.repo_url) + .as_deref(), + &include, + ); + // A reused scan stands in for the new one: everything below this point — // the results table, the blocking-rule gate, the report file — runs against // whichever scan id this resolves to. let reused_scan = skip_recent.as_ref().and_then(|skip| { + if let Some(reason) = include_rules.reuse_refusal() { + println!("Scanning instead of reusing a previous scan: {reason}."); + // The flag promises this marker on every run, and reuse was + // declined before resolve_reusable_scan could print it. + crate::skip_scan::report_scan_not_skipped(); + return None; + } crate::skip_scan::resolve_reusable_scan( config, &project_name, @@ -119,7 +145,7 @@ pub fn run( policy, target, exclude, - include, + include_rules, include_images, ), }; @@ -307,7 +333,7 @@ fn start_new_scan( policy: Option, target: Option, exclude: Option, - include: Vec, + include_rules: crate::include_rules::IncludeRules, include_images: Vec, ) -> (String, Option) { println!("\nScanning with BLAST 🚀🚀🚀"); @@ -366,17 +392,9 @@ fn start_new_scan( // Before packaging: mid-pack HEAD move must not look like a clean new SHA. let repo_before = utils::generic::get_repo_info_for_scan("./").unwrap_or_default(); - // Resolved before packaging: the rules decide what goes into the archive, - // and a file left out of it cannot be scanned however the engine classifies - // what it did receive. - let include_rules = crate::include_rules::resolve( - config, - project_name, - repo_before - .as_ref() - .and_then(|info| info.repo_url.as_deref()), - &include, - ); + // The rules were resolved in run(), before the reuse decision; they decide + // what goes into the archive, and a file left out of it cannot be scanned + // however the engine classifies what it did receive. let force_included = include_rules.matching_files(Path::new(".")); if !include_rules.is_empty() && force_included.is_empty() { log::warn!( @@ -387,10 +405,19 @@ fn start_new_scan( ) ); } else if !force_included.is_empty() { + // Named, not just counted: these paths override .gitignore and the + // default excludes (which cover `**/*.env` among others), so whoever + // reads the log needs to see what actually went into the archive. println!( - "Force-including {} file(s) Corgea would otherwise skip.", + "Force-including {} file(s) Corgea would otherwise skip:", force_included.len() ); + for path in force_included.iter().take(FORCE_INCLUDE_PREVIEW) { + println!(" {}", path.display()); + } + if force_included.len() > FORCE_INCLUDE_PREVIEW { + println!(" (+{} more)", force_included.len() - FORCE_INCLUDE_PREVIEW); + } } if target_str.is_none() && exclude.is_some() { @@ -401,9 +428,10 @@ fn start_new_scan( match targets::resolve_targets_with_exclude(target_value, exclude.as_deref()) { Ok(result) => { if result.files.is_empty() { - // An exported image is a complete payload on its own, so a - // target that matches nothing is only fatal without one. - if image_archives.is_empty() { + // An exported image, or a file an include rule forces in, + // is a complete payload on its own — so a target that + // matches nothing is only fatal without either. + if image_archives.is_empty() && force_included.is_empty() { *stop_signal.lock().unwrap() = true; let _ = packaging_thread.join(); print!( @@ -432,10 +460,19 @@ fn start_new_scan( std::process::exit(1); } + let covers = match (force_included.is_empty(), image_archives.is_empty()) { + (true, _) => "the included container image(s)", + (false, true) => "the force-included file(s)", + (false, false) => { + "the force-included file(s) and the included container image(s)" + } + }; log::warn!( "\n{}", utils::terminal::set_text_color( - "⚠️ No scannable files matched your target, so this scan covers only the included container image(s).", + &format!( + "⚠️ No scannable files matched your target, so this scan covers only {covers}." + ), utils::terminal::TerminalColor::Yellow ) ); diff --git a/src/skip_scan.rs b/src/skip_scan.rs index aa75b98..64a7bfb 100644 --- a/src/skip_scan.rs +++ b/src/skip_scan.rs @@ -291,6 +291,15 @@ fn confirm_reusable_scan(config: &Config, scan_id: &str) -> Result<(), String> { Ok(()) } +/// Report that this run is scanning, for a refusal decided before +/// `resolve_reusable_scan` is reached. +/// +/// The flag promises the marker on every run so a pipeline can branch on it, so +/// a caller that declines reuse on its own still has to emit it. +pub fn report_scan_not_skipped() { + print_skipped_marker(None); +} + /// `CORGEA_SCAN_SKIPPED=true|false`, plus the reused scan id when there is one. /// Shell-assignment shaped so a pipeline can `eval` or `grep` it. fn print_skipped_marker(reused_scan_id: Option<&str>) { diff --git a/src/utils/api.rs b/src/utils/api.rs index 9b1aa1b..32bbed3 100644 --- a/src/utils/api.rs +++ b/src/utils/api.rs @@ -25,6 +25,11 @@ const DIRTY_FALSE: &str = "false"; /// How long any one request may take before the client gives up. const REQUEST_TIMEOUT: Duration = Duration::from_secs(150); +/// Budget for the pre-scan project-settings lookup. Short on purpose: the run +/// continues without the project's include rules if it fails, so waiting out +/// the default would delay every scan for no gain. +const SETTINGS_REQUEST_TIMEOUT: Duration = Duration::from_secs(15); + fn auth_headers(token: &str) -> HeaderMap { let mut headers = HeaderMap::new(); let (name, value) = auth_header(token); @@ -998,13 +1003,25 @@ pub fn query_scan_settings( let client = http_client(); let mut query = vec![("project_name", project_name.to_string())]; if let Some(repo_url) = repo_url { - query.push(("repo_url", repo_url.to_string())); + // A git origin can embed a token (`https://oauth2:glpat-x@host/...`). + // Strip it before it reaches a query string, a proxy log, or --verbose. + query.push(( + "repo_url", + utils::generic::strip_remote_credentials(repo_url), + )); } debug(&format!( "Reading project scan settings from {} ({:?})", request_url, query )); - let response = client.get(&request_url).query(&query).send()?; + // Pre-flight lookup whose failure path is "run without the project's + // rules", so it must not sit behind the 150s default: a hung endpoint would + // otherwise add minutes to every scan before falling back. + let response = client + .get(&request_url) + .query(&query) + .timeout(SETTINGS_REQUEST_TIMEOUT) + .send()?; check_for_warnings(response.headers(), response.status()); let status = response.status(); if status == StatusCode::NOT_FOUND { diff --git a/src/utils/generic.rs b/src/utils/generic.rs index 4fb8150..829ca0b 100644 --- a/src/utils/generic.rs +++ b/src/utils/generic.rs @@ -510,6 +510,28 @@ pub fn extract_repo_host(url: &str) -> Option { Some(split_remote(url)?[0].to_lowercase()) } +/// A git remote with any embedded credential removed. +/// +/// `https://oauth2:glpat-xxx@gitlab.com/org/repo` becomes +/// `https://gitlab.com/org/repo`. Only URLs with a `://` scheme are touched: +/// scp-style `git@github.com:org/repo` carries no secret, and stripping its +/// `git@` would stop the server recognising it as scp-style, so it would no +/// longer normalize to the same stored URL. +/// +/// This is the same userinfo strip the server applies before storing a +/// `repo_url`, so a redacted value still resolves to the same project — while +/// keeping the token out of query strings, proxy logs and `--verbose` output. +pub fn strip_remote_credentials(url: &str) -> String { + let Some((scheme, rest)) = url.split_once("://") else { + return url.to_string(); + }; + let host_end = rest.find('/').unwrap_or(rest.len()); + match rest[..host_end].rfind('@') { + Some(at) => format!("{scheme}://{}", &rest[at + 1..]), + None => url.to_string(), + } +} + /// Split a git remote into `[host, path segments…]`, dropping scheme, userinfo /// and port. None when fewer than two path segments follow the host, or when /// nothing marks the value as a network remote. @@ -955,6 +977,35 @@ mod tests { ); } + /// A git origin can embed a token, and the settings lookup puts the remote + /// in a query string and the debug log. + #[test] + fn strip_remote_credentials_removes_userinfo_from_scheme_urls() { + assert_eq!( + strip_remote_credentials("https://oauth2:glpat-secret@gitlab.com/org/repo"), + "https://gitlab.com/org/repo" + ); + assert_eq!( + strip_remote_credentials("https://token@github.com/org/repo.git"), + "https://github.com/org/repo.git" + ); + assert_eq!( + strip_remote_credentials("https://github.com/org/repo"), + "https://github.com/org/repo" + ); + // scp-style carries no secret, and stripping `git@` would stop the + // server recognising the shape and normalizing it to the stored URL. + assert_eq!( + strip_remote_credentials("git@github.com:org/repo.git"), + "git@github.com:org/repo.git" + ); + // An `@` in the path is not userinfo. + assert_eq!( + strip_remote_credentials("https://github.com/org/re@po"), + "https://github.com/org/re@po" + ); + } + /// A force-include rule is the customer overruling Corgea's own judgement /// about a file, so it has to beat the default excludes. #[test] diff --git a/tests/cli_scan_include.rs b/tests/cli_scan_include.rs index 97d1a6a..d5e2494 100644 --- a/tests/cli_scan_include.rs +++ b/tests/cli_scan_include.rs @@ -213,6 +213,58 @@ fn an_include_rule_overrides_exclude_patterns() { assert!(uploaded_text(&uploads).contains("generated/Payments.java")); } +/// A force-included file is a complete payload on its own, like an exported +/// image: a target that matches nothing must not abort the run when an include +/// rule did match something. +#[test] +fn a_target_matching_nothing_still_scans_the_force_included_files() { + let (base_url, uploads) = spawn_scan_stub("scan-include-empty-target", "[]"); + let project = stub_project(); + + let output = scan( + &base_url, + &project, + &[ + "--target", + "no/such/dir/**", + "--include", + "node_modules/internal-sdk/index.js", + ], + ); + + assert!( + String::from_utf8_lossy(&output.stderr).contains("the force-included file(s)"), + "stderr:\n{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(uploaded_text(&uploads).contains("node_modules/internal-sdk/index.js")); +} + +#[test] +fn a_repo_wide_include_pattern_is_refused_before_the_scan() { + let (base_url, uploads) = spawn_scan_stub("scan-include-broad", "[]"); + let project = stub_project(); + + for pattern in ["**", "["] { + let (mut cmd, _home) = corgea_isolated(); + cmd.current_dir(project.path()) + .env("CORGEA_URL", &base_url) + .env("CORGEA_TOKEN", "test-token") + .args(["scan", "--include", pattern]); + let output = cmd.output().expect("run corgea scan"); + + assert_eq!(output.status.code(), Some(1), "{pattern} should be refused"); + assert!( + String::from_utf8_lossy(&output.stderr).contains("--include"), + "stderr should name the flag for {pattern}" + ); + } + assert!( + uploads.lock().unwrap().is_empty(), + "nothing should be packaged for a pattern that was refused" + ); +} + #[test] fn an_include_rule_that_matches_nothing_warns_and_still_scans() { let (base_url, uploads) = spawn_scan_stub("scan-include-nomatch", "[]"); diff --git a/tests/cloud_commands_e2e/common/mod.rs b/tests/cloud_commands_e2e/common/mod.rs index 8325441..3b7bb91 100644 --- a/tests/cloud_commands_e2e/common/mod.rs +++ b/tests/cloud_commands_e2e/common/mod.rs @@ -596,21 +596,28 @@ pub(crate) fn verify_request() -> ExpectedRequest { ) } -/// Every new BLAST scan reads the project's include rules before packaging, so -/// files Corgea would classify away can still be forced into the archive. +/// Every BLAST run reads the project's include rules before deciding whether a +/// previous scan can be reused and before packaging, so files Corgea would +/// classify away can still be forced into the archive. pub(crate) fn scan_settings_request(project: &str) -> ExpectedRequest { + scan_settings_request_with(project, &[]) +} + +/// `scan_settings_request` answering with the given project include rules. +pub(crate) fn scan_settings_request_with(project: &str, include_paths: &[&str]) -> ExpectedRequest { let project = project.to_string(); + let body = json!({ + "status": "ok", + "project": null, + "settings": {"include_paths": include_paths, "ignore_paths": []} + }); expected_request( "read project include rules", move |request| { assert_authenticated_request(request, Method::GET, "/api/v1/scan-settings")?; assert_query(request, "project_name", &project) }, - json_response(json!({ - "status": "ok", - "project": null, - "settings": {"include_paths": [], "ignore_paths": []} - })), + json_response(body), ) } diff --git a/tests/cloud_commands_e2e/scan_skip.rs b/tests/cloud_commands_e2e/scan_skip.rs index 7975052..a11289e 100644 --- a/tests/cloud_commands_e2e/scan_skip.rs +++ b/tests/cloud_commands_e2e/scan_skip.rs @@ -126,6 +126,7 @@ fn skipped_scan_still_fails_the_build_on_the_prior_scans_blocking_rules() { let out_file = out_dir.path().join("results.sarif"); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), clean_detail(&project.sha), reused_scan_issues(), @@ -177,6 +178,7 @@ fn skipped_scan_reports_the_prior_findings() { let project = git_project(); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), clean_detail(&project.sha), reused_scan_issues(), @@ -213,7 +215,7 @@ fn a_scan_older_than_the_window_still_triggers_a_new_scan() { let project = git_project(); let mut plan = blast_upload_plan(&project.sha, false, false); plan.insert( - 1, + 2, commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(30))]), ); let api = ApiStub::start(plan); @@ -244,7 +246,7 @@ fn a_shorter_window_rejects_a_scan_the_default_would_reuse() { let project = git_project(); let mut plan = blast_upload_plan(&project.sha, false, false); plan.insert( - 1, + 2, commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), ); let api = ApiStub::start(plan); @@ -316,6 +318,7 @@ fn a_file_hidden_from_git_status_reuses_the_commits_scan() { .expect("modify assume-unchanged file"); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), clean_detail(&project.sha), reused_scan_issues(), @@ -359,6 +362,7 @@ fn ignore_dirty_worktree_reuses_a_scan_a_dirty_tree_would_otherwise_run() { .expect("modify tracked file"); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), clean_detail(&project.sha), reused_scan_issues(), @@ -407,6 +411,7 @@ fn ignore_dirty_worktree_reuses_a_prior_dirty_scan() { let path = format!("/api/v1/scan/{PRIOR_SCAN}"); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior]), expected_request( "confirm the dirty scan being reused", @@ -446,12 +451,12 @@ fn ignore_dirty_worktree_still_uploads_dirty_when_nothing_is_reused() { let project = git_project(); std::fs::write(project.path().join("main.py"), "print('dirty')\n") .expect("modify tracked file"); - // blast_upload_plan already holds verify then the include-rule lookup; the - // baselines follow it and the reuse lookup precedes it. + // blast_upload_plan already holds verify then the include-rule lookup. The + // reuse lookup follows those, then the baselines. let mut plan = blast_upload_plan(&project.sha, true, false); - plan.insert(2, baseline_lookup_for_branch("master", vec![])); - plan.insert(2, baseline_lookup_for_branch("main", vec![])); - plan.insert(1, commit_lookup(&project.sha, vec![])); + plan.insert(2, commit_lookup(&project.sha, vec![])); + plan.insert(3, baseline_lookup_for_branch("main", vec![])); + plan.insert(4, baseline_lookup_for_branch("master", vec![])); let api = ApiStub::start(plan); let (mut command, _home) = cloud_command(&api, project.path()); command.args([ @@ -486,11 +491,11 @@ fn a_degraded_prior_scan_is_not_reused() { let project = git_project(); let mut plan = blast_upload_plan(&project.sha, false, false); plan.insert( - 1, + 2, commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), ); plan.insert( - 2, + 3, reused_scan_detail( &project.sha, json!([{ @@ -529,7 +534,9 @@ fn a_degraded_prior_scan_is_not_reused() { fn no_resolvable_commit_fails_before_anything_is_uploaded() { let project = TempDir::new().expect("create non-git project"); std::fs::write(project.path().join("main.py"), "print('hi')\n").expect("write source"); - let api = ApiStub::start(vec![verify_request()]); + // Include rules are read before the reuse decision, so the lookup happens + // even on a run that then refuses for want of a commit. + let api = ApiStub::start(vec![verify_request(), scan_settings_request(PROJECT)]); let (mut command, _home) = cloud_command(&api, project.path()); command.args([ "scan", @@ -626,6 +633,7 @@ fn excluding_files_warns_but_still_reuses_the_commits_scan() { let project = git_project(); let api = ApiStub::start(vec![ verify_request(), + scan_settings_request(PROJECT), commit_lookup(&project.sha, vec![prior_scan(&project.sha, &ago(3))]), clean_detail(&project.sha), reused_scan_issues(), @@ -676,6 +684,42 @@ fn the_window_cannot_be_set_without_the_skip_flag() { ); } +/// The primary SAST-01 path is a rule configured in the web app, with no flag +/// on the command line — so clap cannot refuse it and the check has to happen +/// after the rules are read. Retrying a commit after adding a rule must scan, +/// not reuse a scan that never packaged the files the rule forces in. +#[test] +fn a_project_include_rule_refuses_reuse_and_starts_a_new_scan() { + let project = git_project(); + // The full new-scan contract, with the settings lookup answering with a + // rule. No commit lookup in it: the rules take reuse off the table before + // resolve_reusable_scan is ever asked. + let mut plan = blast_upload_plan(&project.sha, false, false); + plan[1] = scan_settings_request_with(PROJECT, &["vendor/our-fork/**"]); + let api = ApiStub::start(plan); + let (mut command, _home) = cloud_command(&api, project.path()); + command.args([ + "scan", + "blast", + "--skip-if-commit-scanned-recently", + "--project-name", + PROJECT, + ]); + + let output = run_with_timeout(command, &api); + let transcript = api.assert_finished(); + let context = output_context(&output, &transcript); + let stdout = String::from_utf8_lossy(&output.stdout); + + assert_eq!(output.status.code(), Some(0), "{context}"); + assert!( + stdout.contains("Scanning instead of reusing a previous scan"), + "{context}" + ); + assert!(stdout.contains("CORGEA_SCAN_SKIPPED=false"), "{context}"); + assert!(stdout.contains("Scanning with BLAST"), "{context}"); +} + /// A force-include rule widens what gets scanned, and the reused candidate was /// scanned without it — so reuse would silently skip the very file the run /// mandated. That is under-reporting, which the flag refuses rather than warns. @@ -735,7 +779,7 @@ fn a_scan_of_another_commit_is_never_reused() { let other_commit = "ffffffffffffffffffffffffffffffffffffffff"; let mut plan = blast_upload_plan(&project.sha, false, false); plan.insert( - 1, + 2, commit_lookup(&project.sha, vec![prior_scan(other_commit, &ago(1))]), ); let api = ApiStub::start(plan);