Force-include rules and a --include flag for the scan packager (SAST-01) - #166
Force-include rules and a --include flag for the scan packager (SAST-01)#166Ibrahimrahhal wants to merge 2 commits into
Conversation
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 <ibrahim@corgea.com>
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 <ibrahim@corgea.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Project include rules never suppress --skip-if-commit-scanned-recently. The clap conflict only covers the --include flag, so the primary SAST-01 path (rules from the web app, no flag) can reuse a scan that never packaged those files — the same under-reporting this PR already refuses for the flag.
One inline comment with the fix. I did not find a second merge-blocking issue in the packager exemption, the incremental union, or the --include upload field.
Sent by Cursor Automation: pr-flow
| 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"], |
There was a problem hiding this comment.
--skip-if-commit-scanned-recently still reuses a scan when the project has File Include Rules.
This conflict list is the right policy for --include: a reused scan predates the rule, so skipping would leave the mandated file unscanned. Project-level rules from GET /api/v1/scan-settings have the same semantics and are the main SAST-01 path, but clap cannot see them.
Reuse is decided in blast::run before start_new_scan, which is the only place include_rules::resolve runs:
let reused_scan = skip_recent.as_ref().and_then(|skip| {
crate::skip_scan::resolve_reusable_scan(
config,
&project_name,
skip,
exclude.as_deref(),
*ignore_dirty_worktree,
)
});
Successful-skip e2e plans (scan_skip.rs) go verify → commit lookup and never hit /scan-settings, so this is untested.
Impact: Adding include rules in the web app and re-running CI on the same SHA (retry, promotion, or a verification re-run) reuses the old scan. The files this feature exists to force in stay out. New commits still scan; retries and first-verification runs do not.
Fix: Resolve include rules once in run() before resolve_reusable_scan. If !include_rules.is_empty(), do not reuse (same fail-closed stance as --scan-type / --include). On a /scan-settings error, also skip reuse — skip_scan already treats an incomplete lookup as “run a new scan.” Pass the resolved IncludeRules into start_new_scan so the endpoint is not fetched twice.
Cover it with an e2e: --skip-if-commit-scanned-recently plus a stub that returns include_paths: ["vendor/our-fork/**"] must start a new scan (upload present), not reuse.
| 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; | ||
| } |
There was a problem hiding this comment.
the 5,000-file cap uses walk order; could we sort matches before applying it so equivalent runs select the same paths?
| } else if !force_included.is_empty() { | ||
| println!( | ||
| "Force-including {} file(s) Corgea would otherwise skip.", | ||
| force_included.len() | ||
| ); | ||
| } |
There was a problem hiding this comment.
force-included paths override ignore and exclude rules, including **/*.env; could we print a bounded path list with the count for archive review?
| .and_then(|info| info.repo_url.as_deref()), | ||
| &include, | ||
| ); | ||
| let force_included = include_rules.matching_files(Path::new(".")); |
There was a problem hiding this comment.
a zero-file target can still have force-included matches; could we treat a nonempty force_included list as a valid payload before the guard?
| 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 |
There was a problem hiding this comment.
the origin can contain credentials and is sent in the query and debug logs; could we derive a credential-free repository identity before the lookup?
| if !entry.file_type().is_some_and(|kind| kind.is_file()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
discarded traversal errors can turn an explicit include into a no-match; could we propagate them or distinguish them from no matches?
| IncludeRules { | ||
| patterns, | ||
| cli_patterns, | ||
| } |
There was a problem hiding this comment.
invalid CLI globs are skipped locally but uploaded in cli_patterns; could we validate them consistently or reject them during argument handling?
| "Reading project scan settings from {} ({:?})", | ||
| request_url, query | ||
| )); | ||
| let response = client.get(&request_url).query(&query).send()?; |
There was a problem hiding this comment.
the settings lookup inherits the 150-second timeout and can delay CLI fallback; could we set a shorter per-request timeout?
3 participants
Description
A file the CLI leaves out of the archive cannot be scanned whatever the engine later decides about it, so
DEFAULT_EXCLUDE_GLOBSand.gitignoresilently overruled any platform-side attempt to force-scan misclassified proprietary code. This makes the packager honor force-include rules.New
src/include_rules.rs. Before packaging, reads the project's rules fromGET /api/v1/scan-settingsand unions them with the new repeatable--includeflag. Patterns accept a path, a directory, or a glob; a bare path also matches its descendants, so the same pattern means the same thing here as in the platform's ignore rules. Expansion walks with the standard ignore filters off — the point is to reach files.gitignoreand the default excludes hide — skipping.git, and capped at 5 000 files so a rule like**/*.jscannot drag a wholenode_modulestree into the upload.Packaging.
create_zip_from_targettakes the matched repo-relative paths and exempts them from the default excludes,--exclude, and the walk's gitignore pruning, appending any the walk never reached. Same shape as the existingextra_filespath used by--include-image.Incremental.
IncrementalPlan::includingadds force-included files to the changed-file list. The server carries findings forward for every file the diff omits, and an include rule exists precisely because the file was never scanned — so there is nothing to carry forward, and without this the rule would never get the file looked at. Blowing pastMAX_CHANGED_FILESfalls back to a full scan, which is only slower.Upload. Only the
--includevalues travel with the archive (include_paths, JSON array); the project's own rules are already stored server-side.Failure handling. A failed
/scan-settingslookup warns and proceeds on the flag alone — that is exactly today's behavior, and refusing to scan would be worse. A404is a backend without the endpoint, which has no rules to apply either. An unparseable pattern is dropped without discarding the rest: silently scanning less than asked is the failure this feature exists to fix.Flag conflict.
--includejoins--targetand--include-imagein refusing--skip-if-commit-scanned-recently. 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, which still only warns.Related PRs
All three are needed for the feature to work end to end:
/scan-settings, acceptsinclude_pathson upload.include_pathswhen classifying extracted files.Requirement ID: SAST-01 (Post-Migration).
Testing
New
tests/cli_scan_include.rsdrives the real binary against a stubbed API:--includebundles anode_modulesfile while its siblings stay excluded, platform-configured rules apply with nothing sent back, an include rule beats--exclude, and a rule matching nothing warns without failing the scan. Unit tests ininclude_rules.rscover normalization, unparseable patterns, and reaching gitignored paths;incremental.rscovers the changed-file union and its cap;generic.rscovers the zip exemption. Thecloud_commands_e2econtract now asserts the/scan-settingsrequest in the BLAST plan, andscan_skip.rsasserts the new flag conflict.One suppression added:
#[allow(clippy::large_enum_variant)]onCommands.Scancarries by far the largest flag set, and exactly oneCommandsvalue exists per process — parsed at startup and destructured immediately — so boxing would not recover anything real.