Diff the baseline's stored file checksums when git cannot reach it - #174
Ibrahimrahhal wants to merge 6 commits into
Conversation
An incremental scan needs to know what changed, and the only answer the CLI has today is a git diff in the clone it is run from. A shallow checkout cannot reach the commit the last scan covered, a detached HEAD names no branch, and a pipeline unpacking a tarball has no .git at all, so each of those analyzes every file on every run. Hash each file on its way into the archive instead. The digests describe what was uploaded rather than what the repository holds, which is also the more accurate question: ignored paths, uncommitted edits and untracked files all make the two disagree, and the archive is what the scanner reads. Only a whole-project archive gets one. A --target, --exclude or --only-uncommitted run packs a subset, and a manifest of a subset would read to the server as every other file having been deleted. Co-authored-by: ibrahim <ibrahim@corgea.com>
The manifest travels with the chunk that completes the archive, since that is the request the server registers the scan from; sending it with every chunk would re-upload it once per 50 MB for a server that discards all but the last copy. Its root travels beside the bytes rather than inside them, so a truncated manifest costs a full scan instead of reading as a tree that shrank. The scope verdict now comes back on the upload response, and it supersedes what this run worked out beforehand. A manifest is diffed against a baseline the client does not hold, so the local decision can be both confident and wrong; it is held back and printed only when the response says nothing, which covers a deployment predating the field and an upload that deduplicated onto a scan already running. --disable-incremental withholds the manifest, since the server would otherwise scope the scan from it. Co-authored-by: ibrahim <ibrahim@corgea.com>
![corgea-staging[bot]](https://avatars.githubusercontent.com/u/69728621?s=60&v=4){kind=small}
| pub fn encode(&self) -> Option<EncodedManifest> { | ||
| if self.entries.is_empty() || self.entries.len() > MAX_ENTRIES { | ||
| return None; | ||
| } | ||
| let canonical = self.canonical(); | ||
| let root = format!("{:x}", Sha256::digest(&canonical)); | ||
| let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); | ||
| encoder.write_all(&canonical).ok()?; | ||
| let body = encoder.finish().ok()?; | ||
| Some(EncodedManifest { body, root }) |
There was a problem hiding this comment.
🧹 Quality - The encode method suppresses I/O/compression errors by converting them into None via .ok()?. This makes it impossible for callers to distinguish “manifest intentionally not produced” (empty or too many entries) from “manifest failed to encode” (e.g., gzip writer failure), which can cause incorrect fallback behavior and makes diagnosing failures harder. Consider returning a Result<Option<EncodedManifest>, io::Error> (or a custom error enum) so error cases are observable while still allowing “no manifest” as a valid outcome. View in Corgea ↗
More Details
🪄Fix Explanation: Manifest encoding now propagates I/O failures instead of silently converting them into absence. This preserves the distinction between an invalid/empty manifest and an encoding failure, improving error handling and diagnosability.
<bullet_point>"encode" returns "Result<Option<EncodedManifest>, io::Error>", exposing compression and write failures to callers. <bullet_point>"Ok(None)" still represents empty or oversized manifests, while "Err" represents an actual I/O failure. <bullet_point>Replacing ".ok()? " with "?" prevents "write_all" and "finish" errors from being silently discarded. <bullet_point>Explicit error propagation improves observability and lets higher-level code choose appropriate recovery, logging, or user-facing behavior.</bullet_point>
💡Important Instructions: Update every
encode caller to handle the new Result, propagating or reporting io::Error while preserving the existing None handling.
| pub fn encode(&self) -> Option<EncodedManifest> { | |
| if self.entries.is_empty() || self.entries.len() > MAX_ENTRIES { | |
| return None; | |
| } | |
| let canonical = self.canonical(); | |
| let root = format!("{:x}", Sha256::digest(&canonical)); | |
| let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); | |
| encoder.write_all(&canonical).ok()?; | |
| let body = encoder.finish().ok()?; | |
| Some(EncodedManifest { body, root }) | |
| pub fn encode(&self) -> Result<Option<EncodedManifest>, io::Error> { | |
| if self.entries.is_empty() || self.entries.len() > MAX_ENTRIES { | |
| return Ok(None); | |
| } | |
| let canonical = self.canonical(); | |
| let root = format!("{:x}", Sha256::digest(&canonical)); | |
| let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); | |
| encoder.write_all(&canonical)?; | |
| let body = encoder.finish()?; | |
| Ok(Some(EncodedManifest { body, root })) |
![corgea[bot]](https://avatars.githubusercontent.com/in/673913?s=60&v=4){kind=small}
| Self::default() | ||
| } | ||
|
|
||
| /// Record one archived file. `path` must be the zip entry name, byte for |
There was a problem hiding this comment.
🧹 Quality - Manifest canonicalization writes path directly into a line-based format without escaping. If a path contains a newline, carriage return, or other control characters, the canonical form becomes ambiguous (it can look like multiple entries) and the computed root can represent something different than intended. This is a correctness and maintainability problem because callers are told “byte for byte” matching matters, yet the encoding doesn’t enforce or validate that the bytes are safe for this format. Consider validating/rejecting paths containing \n/\r (and possibly NUL), or switching to a length-prefixed/binary encoding. View in Corgea ↗
More Details
🪄Fix Explanation: Added validation to reject paths containing control characters in both insertion and finalization steps, preventing ambiguous or incorrect manifest states and ensuring data integrity.
"insert()" now checks for control chars "\n, \r, \0" in paths before insertion, avoiding invalid data entry.
A "debug_assert!" warns developers about invalid paths to catch issues early during development.
The "finalize()" method rejects manifests containing any such invalid paths, preserving canonical form correctness.
These checks avoid subtle bugs from malformed paths and ensure the algorithm processes only well-formed data, improving robustness.
💡Important Instructions: Update any external path generation or input sanitization to avoid control characters, ensuring compliance with new manifest constraints.
diff --git a/src/manifest.rs b/src/manifest.rs
index da3b280..8004d94 100644
--- a/src/manifest.rs
+++ b/src/manifest.rs
@@ -141,6 +141,11 @@ impl Manifest {
/// and against stored findings, and a path spelled differently in either
/// place is a file whose old findings are carried forward untouched.
pub fn insert(&mut self, path: String, digest: String) {
+ let invalid = path.chars().any(|c| c == '\n' || c == '\r' || c == '\0');
+ debug_assert!(!invalid, "manifest path contains control characters (\\n, \\r, or NUL): {}", path);
+ if invalid {
+ return;
+ }
self.entries.insert(path, digest);
}
@@ -169,6 +174,10 @@ impl Manifest {
if self.entries.is_empty() || self.entries.len() > MAX_ENTRIES {
return None;
}
+ // Reject ambiguous paths to keep canonical form unambiguous
+ if self.entries.keys().any(|p| p.chars().any(|c| c == '\n' || c == '\r' || c == '\0')) {
+ return None;
+ }
let canonical = self.canonical();
let root = format!("{:x}", Sha256::digest(&canonical));
let mut encoder = GzEncoder::new(Vec::new(), Compression::default());
To apply the fix, Download .patch.
There was a problem hiding this comment.
findings: good
fix: bad
The ambiguity is real because these control characters are legal in Unix paths and the protocol is line-based. Silently dropping the path from insert is unsafe because the file remains in the ZIP while its absence from the manifest can be interpreted as deletion; encoding should escape paths or fail the archive operation.
high: Line-based manifest accepts ambiguous paths
Unix filenames may contain newlines or carriage returns. Writing such paths directly into <digest> <path>\n can create apparent additional entries and make server-side manifest parsing differ from the actual ZIP entries.
Proof or reproduction:
manifest.insert("real.py\naaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa injected.py".into(), digest) serializes as two apparent entry lines despite representing one archive path.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two merge blockers, both the same class of bug this PR already names: a path that is missing or spelled differently in the manifest is treated as a deletion (or as a file whose findings never update).
archives_whole_project lets a git-subdirectory walk through, and zip/manifest keys keep OS separators. Either one can drop findings on the next scan without those files having been analyzed.
Sent by Cursor Automation: pr-flow
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | ||
| target.is_none() && user_exclude.is_none() | ||
| } |
There was a problem hiding this comment.
This is not a whole-project archive when CWD is below the git worktree root. create_zip_from_target(None, …) walks . and keys the manifest by those CWD-relative names — the same subset-looks-like-deletion case this helper exists to refuse.
get_repo_info already returns None for a nested CWD so the zip is not labeled with the parent HEAD (is_at_repo_root). The manifest gate does not use that check. A scan from backend/ (or Actions working-directory) with --project still uploads a subtree manifest. The server’s baseline is the newest scan that carries one, so the next root / shallow-clone scan subtracts {app.py, …} from {backend/app.py, other/…} and drops every finding for a file this run never looked at.
--only-uncommitted is already covered because it is a target string. Non-git trees (tarball / no .git) must keep a manifest — that is this PR. Fail closed only when git discovery succeeds and CWD is not the worktree root.
The existing test only asserts the predicate, so this path is untested.
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | |
| target.is_none() && user_exclude.is_none() | |
| } | |
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | |
| if target.is_some() || user_exclude.is_some() { | |
| return false; | |
| } | |
| // Packaging walks `.`. Below the worktree root that is a subtree, and a | |
| // subtree manifest reads as every other file having been deleted. | |
| match Repository::discover(".") { | |
| Ok(_) => is_at_repo_root("."), | |
| Err(_) => true, | |
| } | |
| } |
There was a problem hiding this comment.
Two merge blockers, both the same class of bug this PR already names: a path that is missing or spelled differently in the manifest is treated as a deletion (or as a file whose findings never update).
archives_whole_project lets a git-subdirectory walk through, and zip/manifest keys keep OS separators. Either one can drop findings on the next scan without those files having been analyzed.
Sent by Cursor Automation: pr-flow
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | ||
| target.is_none() && user_exclude.is_none() | ||
| } |
There was a problem hiding this comment.
This is not a whole-project archive when CWD is below the git worktree root. create_zip_from_target(None, …) walks . and keys the manifest by those CWD-relative names — the same subset-looks-like-deletion case this helper exists to refuse.
get_repo_info already returns None for a nested CWD so the zip is not labeled with the parent HEAD (is_at_repo_root). The manifest gate does not use that check. A scan from backend/ (or Actions working-directory) with --project still uploads a subtree manifest. The server’s baseline is the newest scan that carries one, so the next root / shallow-clone scan subtracts {app.py, …} from {backend/app.py, other/…} and drops every finding for a file this run never looked at.
--only-uncommitted is already covered because it is a target string. Non-git trees (tarball / no .git) must keep a manifest — that is this PR. Fail closed only when git discovery succeeds and CWD is not the worktree root.
The existing test only asserts the predicate, so this path is untested.
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | |
| target.is_none() && user_exclude.is_none() | |
| } | |
| fn archives_whole_project(target: Option<&str>, user_exclude: Option<&str>) -> bool { | |
| if target.is_some() || user_exclude.is_some() { | |
| return false; | |
| } | |
| // Packaging walks `.`. Below the worktree root that is a subtree, and a | |
| // subtree manifest reads as every other file having been deleted. | |
| match Repository::discover(".") { | |
| Ok(_) => is_at_repo_root("."), | |
| Err(_) => true, | |
| } | |
| } |
| let entry_name = relative_path.to_string_lossy().into_owned(); | ||
| zip.start_file(entry_name.as_str(), options)?; | ||
| let mut file = File::open(&path)?; | ||
| io::copy(&mut file, &mut zip)?; | ||
| match manifest.as_mut() { | ||
| Some(manifest) => { | ||
| let mut tee = TeeWriter::new(&mut zip); | ||
| io::copy(&mut file, &mut tee)?; | ||
| manifest.insert(entry_name, tee.finish()); |
There was a problem hiding this comment.
to_string_lossy() is OS-native, and ZipWriter::start_file stores that string verbatim (only start_file_from_path rewrites separators). On Windows the zip entry and the manifest key are src\\app.py; on Linux they are src/app.py.
The server subtracts manifests by those keys and matches them to stored findings. A Windows laptop scan followed by Linux CI (or the reverse) looks like every file was deleted and every file was added — the exact finding-drop this PR is trying to prevent. changed_files_since already documents the other direction: git stores / on every platform, and a backslash there is a filename byte, so a blind \\ → / replace is wrong. Join path components so a Unix file named foo\\bar stays one component.
The zip entry and the manifest key have to be the same string. Normalize both.
| let entry_name = relative_path.to_string_lossy().into_owned(); | |
| zip.start_file(entry_name.as_str(), options)?; | |
| let mut file = File::open(&path)?; | |
| io::copy(&mut file, &mut zip)?; | |
| match manifest.as_mut() { | |
| Some(manifest) => { | |
| let mut tee = TeeWriter::new(&mut zip); | |
| io::copy(&mut file, &mut tee)?; | |
| manifest.insert(entry_name, tee.finish()); | |
| let entry_name = relative_path | |
| .iter() | |
| .map(|s| s.to_string_lossy()) | |
| .collect::<Vec<_>>() | |
| .join("/"); | |
| zip.start_file(entry_name.as_str(), options)?; | |
| let mut file = File::open(&path)?; | |
| match manifest.as_mut() { | |
| Some(manifest) => { | |
| let mut tee = TeeWriter::new(&mut zip); | |
| io::copy(&mut file, &mut tee)?; | |
| manifest.insert(entry_name, tee.finish()); |
| @@ -165,6 +201,9 @@ pub fn create_zip_from_target<P: AsRef<Path>>( | |||
| let large_file_options: FileOptions<()> = options.large_file(true); | |||
|
|
|||
There was a problem hiding this comment.
high: Changed image archives are invisible to manifest diffing
The ZIP includes every extra file, but the manifest deliberately excludes them. Two uploads with identical source files but different image archives therefore have identical manifest roots, allowing the server to report no changed files and skip analysis even though scanner input changed.
Proof or reproduction:
Build archive A with source app.py plus image.tar contents A, then archive B with the same app.py plus image.tar contents B; because the extra_files loop never calls manifest.insert, archive_a.manifest.encode().root == archive_b.manifest.encode().root while the ZIP entries differ.
|
|
||
| let mut added_files = Vec::new(); | ||
| let mut excluded_files = Vec::new(); | ||
| // Hashing rides along on the copy that compresses each file, so the archive |
There was a problem hiding this comment.
nitpick: Disabled incremental scans still hash every archived file
Manifest construction is decided solely from target and exclusion options, so a whole-project run with --disable-incremental still computes SHA-256 for every file even though start_new_scan always discards the resulting manifest.
Proof or reproduction:
For target=None and user_exclude=None, archives_whole_project returns true and TeeWriter hashes every copied byte; later file_manifest is unconditionally set to None when disable_incremental is true.
There was a problem hiding this comment.
Automated review risk: 4/5.
High risk: manifest-based scoping can miss changed image archives and incorrectly reuse stale findings.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
Incremental scans pick a baseline the same way as before -- the newest completed clean scan of trunk -- and now work out what changed since it from the file checksums that scan stored, falling back to git diff when it stored none. The checksums need no git history, so a shallow clone, a detached HEAD and a directory with no .git scan incrementally instead of analyzing every file on every run. They also describe the archive rather than the repository, so a dirty worktree no longer needs --ignore-dirty-worktree. The upload names the baseline by scan id for a checksum diff, since the scan that wrote them may have had no commit of its own. Co-authored-by: ibrahim <ibrahim@corgea.com>
cursor
Bot
changed the title
A project scanned without git records no branch, no commit and no dirty flag, so the trunk-and-known-clean baseline lookup could never match one of its own earlier scans -- the case checksums exist for. A clone with no git now makes one branchless lookup, and a run carrying its own checksums accepts a baseline with no commit and any dirtiness: the manifest records what that scan analyzed, so neither is needed. The git diff keeps both requirements, since it measures from the commit. Co-authored-by: ibrahim <ibrahim@corgea.com>
Doghouse rebuilds the manifest of scans uploaded before this CLI computed one, from the archive they uploaded. Two implementations of one format only stay one format if something compares them, and a disagreement here is silent: the next scan diffs against a manifest spelling paths differently and decides the wrong files changed. Three fixtures close the loop. A shared vector both suites assert one root for. An archive a real CLI build wrote, so doghouse's assumptions about zip layout are observed rather than guessed. And the manifest doghouse rebuilt from it, decoded back here to the tree it started as. Co-authored-by: ibrahim <ibrahim@corgea.com>
| #[test] | ||
| fn a_body_that_is_not_a_manifest_is_refused_rather_than_read_as_an_empty_tree() { | ||
| let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); | ||
| encoder.write_all(b"corgea-file-manifest/9 blake3\n").ok(); |
There was a problem hiding this comment.
🧹 Quality - The test intentionally discards the Result from write_all by calling .ok() and not asserting anything about success or failure. If gzip writing fails for any reason, the test will continue and may fail later in a less obvious way (or even pass while not actually exercising the intended behavior). Using expect(...)/unwrap() (like nearby tests do) keeps failures explicit and improves test correctness and debuggability. View in Corgea ↗
More Details
🪄Fix Explanation: The test now explicitly handles gzip write failures instead of silently discarding them. Using "expect" makes setup failures visible and prevents the test from continuing with incomplete data.
• Replaces ".ok()" with ".expect("gzip write")", ensuring a failed write causes an immediate, diagnosable test failure.
• Avoids silently ignoring the "Result" returned by "write_all", improving reliability and making the test’s behavior consistent with its intent.
• The failure message identifies the failing operation, reducing debugging time when compression setup breaks.
• Prevents "encoder.finish()" and manifest validation from running against potentially incomplete gzip data.
| encoder.write_all(b"corgea-file-manifest/9 blake3\n").ok(); | |
| let mut encoder = GzEncoder::new(Vec::new(), Compression::default()); | |
| encoder.write_all(b"corgea-file-manifest/9 blake3\n").expect("gzip write"); |
| let url = format!("{}{}/scan/{}/file-manifest", url, API_BASE, scan_id); | ||
| debug(&format!("Sending request to URL: {}", url)); | ||
| let response = http_client() | ||
| .get(url) | ||
| .send() | ||
| .map_err(|e| format!("API request failed: {}", e))?; | ||
| check_for_warnings(response.headers(), response.status()); | ||
| if !response.status().is_success() { | ||
| return Err(format!("API request failed with status: {}", response.status()).into()); | ||
| } | ||
| Ok(response.bytes()?.to_vec()) |
There was a problem hiding this comment.
SSRF (🔒 Security, 🔴 High) - The function accepts "url" as a caller-provided API base URL and constructs a request destination from it without enforcing an HTTPS scheme, host allowlist, or private-network restriction. The resulting URL is passed directly to "client.get", so a user who can supply the base URL can direct the client to localhost, cloud metadata services, or other internal hosts. Requests may include the shared authentication headers, making this an authenticated SSRF primitive. View in Corgea ↗
More Details
🪄Fix Explanation: The fix restricts requests to HTTPS on the approved API host, rejects unsafe DNS destinations, pins the connection to a validated address, disables redirects, and limits response size to prevent SSRF and resource exhaustion.
- Parses and validates the base URL, requiring HTTPS on port 443 with no credentials and an exact match against "ALLOWED_API_HOSTS".
- Resolves the host before connecting and rejects private, loopback, link-local, multicast, unspecified, and other prohibited network ranges.
- Pins the client to the validated address using ".resolve(host, pinned_address)", preventing DNS rebinding during the request.
- Disables redirects with "Policy::none()" and verifies the constructed request remains HTTPS and targets the approved host.
- Reads at most "MAX_RESPONSE_SIZE + 1" bytes, rejecting responses larger than 10 MiB.
💡Important Instructions: Add or verify the required imports for
Duration, Read, and ToSocketAddrs, then add tests covering invalid URLs, prohibited DNS results, redirects, and oversized responses.
| let url = format!("{}{}/scan/{}/file-manifest", url, API_BASE, scan_id); | |
| debug(&format!("Sending request to URL: {}", url)); | |
| let response = http_client() | |
| .get(url) | |
| .send() | |
| .map_err(|e| format!("API request failed: {}", e))?; | |
| check_for_warnings(response.headers(), response.status()); | |
| if !response.status().is_success() { | |
| return Err(format!("API request failed with status: {}", response.status()).into()); | |
| } | |
| Ok(response.bytes()?.to_vec()) | |
| const ALLOWED_API_HOSTS: &[&str] = &["api.corgea.com"]; | |
| const MAX_RESPONSE_SIZE: u64 = 10 * 1024 * 1024; | |
| let base_url = url::Url::parse(url) | |
| .map_err(|e| format!("Invalid API base URL: {}", e))?; | |
| let host = base_url.host_str() | |
| .ok_or_else(|| "API base URL must include a host".to_string())?; | |
| if base_url.scheme() != "https" | |
| || base_url.port_or_known_default() != Some(443) | |
| || !base_url.username().is_empty() | |
| || base_url.password().is_some() | |
| || !ALLOWED_API_HOSTS.contains(&host) | |
| { | |
| return Err("API base URL is not permitted".into()); | |
| } | |
| let port = base_url.port_or_known_default() | |
| .ok_or_else(|| "API base URL must include a valid port".to_string())?; | |
| let addresses: Vec<_> = std::net::ToSocketAddrs::to_socket_addrs(&(host, port)) | |
| .map_err(|e| format!("Failed to resolve API host: {}", e))? | |
| .collect(); | |
| if addresses.is_empty() | |
| || addresses.iter().any(|address| match address.ip() { | |
| std::net::IpAddr::V4(ip) => { | |
| ip.is_private() | |
| || ip.is_loopback() | |
| || ip.is_link_local() | |
| || ip.is_multicast() | |
| || ip.is_unspecified() | |
| || ip.is_broadcast() | |
| || ip.octets()[0] == 0 | |
| || (ip.octets()[0] == 100 && (64..=127).contains(&ip.octets()[1])) | |
| } | |
| std::net::IpAddr::V6(ip) => { | |
| ip.is_loopback() | |
| || ip.is_unicast_link_local() | |
| || ip.is_unique_local() | |
| || ip.is_multicast() | |
| || ip.is_unspecified() | |
| } | |
| }) | |
| { | |
| return Err("API host resolved to a prohibited network address".into()); | |
| } | |
| let pinned_address = addresses[0]; | |
| let url = url::Url::parse(&format!("{}{}/scan/{}/file-manifest", url, API_BASE, scan_id)) | |
| .map_err(|e| format!("Invalid API request URL: {}", e))?; | |
| if url.scheme() != "https" || url.host_str() != Some(host) { | |
| return Err("API request URL is not permitted".into()); | |
| } | |
| debug(&format!("Sending request to URL: {}", url)); | |
| let request = http_client() | |
| .get(url) | |
| .build() | |
| .map_err(|e| format!("Failed to build API request: {}", e))?; | |
| let client = reqwest::blocking::Client::builder() | |
| .connect_timeout(Duration::from_secs(10)) | |
| .timeout(Duration::from_secs(30)) | |
| .redirect(reqwest::redirect::Policy::none()) | |
| .resolve(host, pinned_address) | |
| .build() | |
| .map_err(|e| format!("Failed to build secure API client: {}", e))?; | |
| let mut response = client | |
| .execute(request) | |
| .map_err(|e| format!("API request failed: {}", e))?; | |
| check_for_warnings(response.headers(), response.status()); | |
| if !response.status().is_success() { | |
| return Err(format!("API request failed with status: {}", response.status()).into()); | |
| } | |
| let mut body = Vec::new(); | |
| (&mut response) | |
| .take(MAX_RESPONSE_SIZE + 1) | |
| .read_to_end(&mut body)?; | |
| if body.len() as u64 > MAX_RESPONSE_SIZE { | |
| return Err("API response exceeded the maximum permitted size".into()); | |
| } | |
| Ok(body) |
The first baseline walk cannot ask the server to drop scans that are not known-clean: that is how a git-less scan reports itself, and those are the ones carrying checksums. The cost is that dirty scans now reach the client, and enough of them bury a clean baseline past the page budget -- which is every project in the window before any of its scans has stored a manifest. So when the unfiltered walk finds nothing and a git diff is still possible, ask again for clean scans only. A checksum baseline is ruled out by then, so nothing is left for the filter to wrongly exclude, and the extra request only happens on the path that was going to be a full scan anyway. Co-authored-by: ibrahim <ibrahim@corgea.com>
| baseline.manifest_version.as_deref().unwrap_or("unknown") | ||
| )); | ||
| } | ||
| let body = api::download_scan_file_manifest(&config.get_url(), &baseline.id) |
There was a problem hiding this comment.
SSRF (🔒 Security, 🔴 High) - The API base URL returned by "config.get_url()" is passed into "api::download_scan_file_manifest" without validating its scheme, host, or destination address. That function then performs an authenticated HTTP request using the shared client, which can send the configured authentication token to an attacker-controlled internal or external endpoint when "CORGEA_URL" or the saved configuration URL is manipulated. This enables server-side request forgery and potential credential disclosure through the manifest download path. View in Corgea ↗
We could not generate a fix for this.
3 participants
Adds a second way to work out what changed since the last scan: the CLI hashes every file it packages, uploads the manifest alongside the archive, and the next run downloads the baseline scan's manifest and diffs the two locally. Where git is unavailable or shallow, this is the only comparison that works.
The git diff is unchanged and still the fallback — checksums are used only when the baseline stored a readable manifest.
Picking a baseline
Which scans can be diffed against depends on how the diff will be measured:
A project scanned without git records no branch, no commit and no dirty flag, so a trunk-and-known-clean lookup could never match one of its own earlier scans. A clone with no git therefore makes one branchless lookup and takes the newest usable scan.
The lookup runs twice when it has to. The first walk accepts a baseline of either kind, so it cannot let the server drop scans that are not known-clean — that is how a git-less scan reports itself. If nothing turns up and a git diff is still possible, it asks again for clean scans only, so a clean baseline behind a page budget's worth of dirty scans is still reachable. That is not a corner case during rollout: no project has a stored manifest yet, so every one of them takes the second walk.
Older backends
Everything degrades to the current behaviour rather than breaking.
file_manifest_rootandfile_manifest_versionare#[serde(default)], so a scan list that omits them parses as a scan with no checksums instead of failing the lookup outright.incremental_base_scan_idis only ever sent for a baseline that advertised a manifest root, which only a backend that has this feature does.Safety
The manifest is gzipped and identified by a root hash over its entries. A download whose contents do not hash to the advertised root, or that exceeds the decoded size cap, is discarded and the run falls back to the git diff rather than trusting a partial list of changed files.
Format compatibility with doghouse
Doghouse rebuilds manifests for scans uploaded before this existed (doghouse#2108), so two implementations now write one format. Three fixtures keep them honest: a shared vector both test suites assert the same root for, an archive a real CLI build wrote so doghouse's assumptions about zip layout are observed rather than guessed, and a manifest doghouse rebuilt from that archive, decoded back here to the tree it started as.