Find a report's files when its paths carry a base the CLI's do not - #173
Conversation
A report's paths are relative to whatever directory the customer's scanner ran from, which is not necessarily where `corgea upload` runs. The CLI resolved each path verbatim against the process directory, so when the two disagreed by a leading prefix -- the scanner ran above the directory the CLI is invoked in, or recorded build-agent paths such as C:/jenkins/workspace/<job>/src/App.java -- the first file missed and the command exited 1 without sending anything, even though every file was right there. find_report_path_prefix() is a port of Fusion's fusion/util/report_paths.py, which solves the same problem for reports whose source arrives as a zip: sample the report's paths and, when none resolve as written, drop one directory at a time from the prefix they all share until the remainder resolves. Accepting a prefix requires corroboration -- candidates come only from directories every sampled path agrees on, and a lone hit that leaves a bare basename is refused, since common basenames repeat across a tree and uploading one file's contents for a finding about another is worse than uploading nothing. Only the file the CLI reads is rebased. `path=` keeps the path the report wrote, because the engine matches the report against the uploaded file_repo_path and never sees this working tree; rebasing it would trade one kind of miss for another. For the same reason the search also honours a path that resolves as written, absolute ones included, so a report generated on this machine is untouched rather than re-pointed at a same-named file in the working tree. The prefix is settled before corgea.yaml is merged in: policy files are found by walking the project, so one of them counting as a path that already resolves would call off the search for a report that matches nothing.
A Fortify vulnerability can carry several SourceLocations, where the earlier ones are the enclosing scope and only the last is the finding itself. The engine reads that last one and no other, so the earlier ones were uploaded for nothing -- and they can sit in a tree that is not on the machine running the CLI at all: Fortify records its own bundled libraries under AppData/Local/Fortify/sca*/build/.../_fortify_libraries_/, which no source upload can satisfy, so the command exited 1 before sending a single file of a report whose real files were all present. Collecting the resolved location only also keeps the prefix search reading exactly the paths that are looked up. Candidate prefixes are built from the directories every sampled path agrees on, so one path from an unrelated tree disagreeing at the first segment empties the shared prefix and nothing can be dropped. Deduplicating through a set rather than a linear scan drops the quadratic cost on reports that repeat a path per finding.
| // a same-named file out of the working tree instead of the file the finding | ||
| // is about. | ||
| if sample | ||
| .iter() |
There was a problem hiding this comment.
high: Absolute paths falsely count as locally resolved
The early check treats root.join(relative) as evidence that no prefix handling is needed. However, an empty prefix makes local_report_path return the original absolute path, not the root-relative path that passed this check. The upload therefore exits as missing even though the checked file exists.
Proof or reproduction:
let raw = "/builds/acme/repo/src/a.py";
write(root.join("builds/acme/repo/src/a.py"), "x");
let paths = vec![raw.to_string()];
assert_eq!(find_report_path_prefix(root, &paths), "");
assert_eq!(local_report_path(root, raw, ""), PathBuf::from(raw));
// Upload reads /builds/... instead of root/builds/... and fails.
There was a problem hiding this comment.
Automated review risk: 4/5.
Prefix selection can reject valid root-relative files or choose a weak match that uploads incorrect source content.
Critical or high-priority changes must be addressed.
Automatic approval was not submitted: automated review found critical or high-priority findings.
There was a problem hiding this comment.
find_report_path_prefix and local_report_path disagree on what “already matches” means for a Unix-absolute report path whose slash-stripped form is already in the tree (/src/a.py → src/a.py). That is Semgrep’s Docker mount; the new tests only cover /builds/acme/repo/..., which never hits the short-circuit. Upload still exits 1 with the files sitting in src/.
Fortify last-SourceLocation, keep-exit-1-on-miss, and the unencoded path= query are as documented — not re-litigating those.
Sent by Cursor Automation: pr-flow
| pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { | ||
| let stripped = strip_report_prefix(report_path, prefix); | ||
|
|
||
| if stripped == report_path || stripped.split('/').any(|segment| segment == "..") { | ||
| return PathBuf::from(report_path); | ||
| } | ||
|
|
||
| root.join(stripped) | ||
| } |
There was a problem hiding this comment.
Empty prefix opens a different path than the “already matches” check just used.
find_report_path_prefix returns "" as soon as any sampled path satisfies root.join(&path.relative).is_file() (report_paths.rs 128–133). normalize has already stripped the leading /, so a Semgrep-in-Docker report (-v "$PWD:/src", paths /src/a.py, /src/b.py) looks like a hit on src/a.py / src/b.py.
This function then takes the empty prefix and returns PathBuf::from("/src/a.py") — the raw absolute path, which is not that file. upload_scan uses exists() on that raw path and exit(1)s. Fusion does not have this split: it reads via extracted_dir / relative. The CLI port kept Fusion’s “relative form under root means already matches” check but not Fusion’s read.
Reproduced against this algorithm (tree = src/a.py, src/b.py):
| report paths | prefix chosen | file opened | exists |
|---|---|---|---|
/builds/acme/repo/src/{a,b}.py |
builds/acme/repo/ |
<root>/src/{a,b}.py |
yes (tested) |
/app/src/{a,b}.py |
app/ |
<root>/src/{a,b}.py |
yes |
/src/{a,b}.py |
"" (early return) |
/src/{a,b}.py |
no |
Coverity already strips the leading slash in its parser, so it never reaches this. Semgrep (the e2e format) and Fortify do not. plan_source_uploads_rebases_the_local_file_but_not_the_uploaded_path and the e2e both use /builds/acme/repo/..., whose normalized form is not in the tree, so they never exercise this short-circuit.
Impact: corgea upload of the standard Semgrep Docker report still dies on the first file — the failure this PR is meant to fix — and the error names /src/a.py with no (looked for it at …) because local_path equals the report path.
Fix: when the prefix is empty and the raw path is not a file, open root.join(normalize(path)) if that is a file and has no ... Keep using the raw path when it does exist (absolute_paths_that_resolve_are_left_alone). Add a plan_source_uploads / e2e case for /src/a.py + /src/b.py next to the /builds one.
| pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { | |
| let stripped = strip_report_prefix(report_path, prefix); | |
| if stripped == report_path || stripped.split('/').any(|segment| segment == "..") { | |
| return PathBuf::from(report_path); | |
| } | |
| root.join(stripped) | |
| } | |
| let stripped = strip_report_prefix(report_path, prefix); | |
| if stripped == report_path || stripped.split('/').any(|segment| segment == "..") { | |
| let raw = PathBuf::from(report_path); | |
| if !raw.is_file() { | |
| let relative = normalize(report_path); | |
| if !relative.is_empty() && !relative.split('/').any(|segment| segment == "..") { | |
| let under_root = root.join(&relative); | |
| if under_root.is_file() { | |
| return under_root; | |
| } | |
| } | |
| } | |
| return raw; | |
| } | |
| root.join(stripped) |
find_report_path_prefix accepts a path as already matching when either the path as written or its slash-stripped form resolves under the root, but local_report_path only ever opened the path as written when no prefix was dropped. A rooted path whose slash-stripped form is in the tree therefore short-circuited the search and was then opened at its absolute path, which is nothing on this machine: a scanner run with the repo mounted at /src reports /src/main.py, the search confirms src/main.py is right there, and the upload still exits 1 on it. That is the failure this whole search exists to remove, and the error named only /src/main.py because the two paths were equal. The two now resolve a path the same way. The path as written still wins when it names a real file, so a report generated on this machine keeps reading its own absolute paths rather than same-named files in the working tree. Otherwise the prefix is dropped, the remainder normalized, and the result read under the root, which also fixes a Windows-style path whose separators no Unix open will accept. A path that neither exists as written nor carries the prefix is now looked for under the root instead of at its own absolute path. Both are absent and it exits 1 either way; the message names where it looked.
The probe joined the raw remainder onto the root while the upload normalized it first, so the two resolved a path differently again. The shared prefix is built from the report's own segments, so dropping it does not always leave a clean relative path: a SARIF `file:///src/a.py` minus `file:/` leaves `//src/a.py`, and Path::join treats a leading slash as absolute and discards the root. The probe therefore stat'd an absolute path outside the project. A report of `file:///<abs>/src/a.py` whose files exist at that absolute location but not in the upload had its prefix accepted on the strength of those files, and the upload, which reads under the root, then exited 1 on the first one. Nothing in the project was ever consulted. `confine` is now the single resolver both sides go through: it normalizes the remainder, refuses one that cannot stay under the root, and leaves the caller to join. That also makes the bare-basename guard read the confined remainder, so a leftover `/a.py` counts as the basename it is rather than passing on its leading slash. A `file://` URI whose files are in the upload now resolves through the scheme, since the search walks down the segments it shares like any other prefix.
| /// so a report that matches resolves against the process directory exactly as it | ||
| /// always has. So is one that cannot be confined: the prefix search only samples | ||
| /// paths, and an unsampled one must not walk out of the project. | ||
| pub fn local_report_path(root: &Path, report_path: &str, prefix: &str) -> PathBuf { |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 High) - The untrusted "report_path" is converted directly into "as_written", and when "confine" rejects traversal components, "local_report_path" returns that unvalidated path unchanged. A caller that opens the returned "PathBuf" can therefore access paths such as "../../etc/passwd" outside "root". The function should reject the path or return an error instead of returning "as_written" for an unconfined input. View in Corgea ↗
We could not generate a fix for this.
There was a problem hiding this comment.
Real, but pre-existing, and this branch narrows it rather than adding it. Leaving it.
main opened the report path with no containment at all:
if !Path::new(&path).exists() { /* exit 1 */ }
let fp = Path::new(&path);A report naming ../../etc/passwd was read and uploaded there too. The as_written fallback this comment points at is that behaviour preserved for the inputs confine refuses; everywhere else the read is now root.join(relative) with .. rejected, so the set of files reachable from a report is a strict subset of what it was.
The fix as described — refuse the path outright — is a behaviour change, not a hardening: it would also refuse absolute paths that legitimately resolve on the machine that generated the report, which is the case absolute_paths_that_resolve_are_left_alone exists to protect and which an earlier review round turned on. Whether corgea upload may read outside the working directory is a product decision about the trust placed in a report file, worth taking deliberately and with its own tests, not folded into a path-matching PR.
| return as_written; | ||
| }; | ||
|
|
||
| if relative == report_path || as_written.is_file() { |
There was a problem hiding this comment.
Path Traversal (🔒 Security, 🔴 High) - The user-controlled "report_path" is returned as an absolute or otherwise externally located path whenever "as_written.is_file()" succeeds. This permits a report to select and cause subsequent processing to read any existing file accessible to the process, rather than restricting reads to "root". The path must be canonicalized and verified to remain beneath the intended root before it is returned or opened. View in Corgea ↗
We could not generate a fix for this.
There was a problem hiding this comment.
Same root as the finding above, and same answer: pre-existing, and narrowed rather than introduced here.
main read Path::new(report_path) unconditionally, so an absolute path in a report was opened whether or not it resolved anywhere near the project. The as_written.is_file() branch is a guard on that, not the cause of it — after it, a path that doesn't exist where the report put it is resolved under root instead. The set of externally-located files a report can select is strictly smaller on this branch than on main, never larger.
Canonicalizing and requiring the result to sit beneath the root would break absolute_paths_that_resolve_are_left_alone: a report generated on this machine carrying an absolute path is the case that test covers, and dropping a prefix from it instead would upload a same-named file from the working tree for a finding about a different file. That tradeoff is the one being flagged, and it belongs in its own change with the product call about whether a report may name files outside the upload.
The upload aborts on the first path it cannot find, so a prefix that resolves part of the report cannot produce a successful upload -- it can only upload whatever sits at the paths that did resolve before dying on the rest. Two hits is Fusion's threshold, where an unresolved finding is skipped and the rest of the report is kept; here it let a coincidental pair of filenames define the offset for a report that was always going to fail. This also settles the shallowest-first case: a shallow prefix that resolves only some of the report no longer beats a deeper one that resolves all of it. Co-authored-by: ibrahim <ibrahim@corgea.com>
Cargo.toml is the single source of truth for the release version: PyPI takes it through maturin's dynamic version and npm sets its own from the tag name, so this is the only manual edit. Co-authored-by: ibrahim <ibrahim@corgea.com>


What
Ports Fusion's
fusion/util/report_paths.pysmart path matching into the CLI's third-party report upload, and stops Fortify from uploading locations the engine never reads.Why
A report's paths are relative to whatever directory the customer's scanner ran from, which is not necessarily where
corgea uploadruns. The CLI resolved each path verbatim against the process directory:So when the two disagreed by a leading prefix — the scanner ran one directory above where the CLI is invoked, or recorded build-agent paths such as
C:/jenkins/workspace/<job>/src/App.java— the first file missed and the command exited 1 without sending anything, even though every file was right there on disk. Fusion hit the same class of failure on the zip side (a customer's Fortify report whose paths all began withDownloads/, whichextract_ziphad already made the project root) and fixed it oncursor/smart-report-path-base-a29a.How
src/scanners/report_paths.rsis a port of the Fusion module, same algorithm and same constants:\to/and dropping..paths.""as soon as one resolves, so a report that already matches is untouched.confineis the single resolver that decides how a report path maps onto the working tree, and both the prefix search and the upload go through it. Keeping them in step is load-bearing: the search accepts a path as already matching when either the path as written or its slash-stripped form resolves, and the prefix is built from the report's own segments, so dropping it does not always leave a clean relative path.Path::jointreats a leading slash as absolute and discards the root, so a remainder joined on raw escapes the project entirely.Only the file read from disk is rebased
path=oncode-uploadkeeps the path the report wrote. Doghouse stores it verbatim asSourceFile.file_repo_path, and Fusion matches the report against it with an exact string compare (get_source_file_by_path) — with per-file uploadsextracted_diris"", so Fusion's own prefix search is a no-op and there is no second chance. Rebasing the uploaded path would trade one kind of miss for another.The prefix is settled before
corgea.yamlis merged in. Policy files are found by walking the project, so they always resolve, and one of them counting as a path that "already resolves" would call off the search for a report that matches nothing.Fortify
A Fortify vulnerability can carry several
SourceLocations, where the earlier ones are the enclosing scope and only the last is the finding itself. Fusion reads that last one and no other, but the CLI collected all of them, so the earlier ones were uploaded for nothing — and they can sit in a tree that is not on the CLI's machine at all: Fortify records its own bundled libraries underAppData/Local/Fortify/sca*/build/.../_fortify_libraries_/, which no source upload can satisfy. That alone exited 1 before a single file was sent. It also broke the prefix search, since one path disagreeing at the first segment empties the shared prefix.Deliberate behaviour decisions
A path that resolves under no prefix is still a fatal
exit 1, with the message now naming where the file was looked for when that differs from the report's path. Fusion skips such a finding and reports a customer-visible warning instead; making the CLI partial-upload is a product decision, not part of this port.The acceptance threshold diverges from Fusion, deliberately. Fusion accepts a prefix on two hits; the CLI requires the whole sample. The reason is the line above: the upload aborts on the first path it cannot find, so a prefix resolving part of the report cannot produce a successful upload — it can only upload whatever sits at the paths that did resolve before dying on the rest. Fusion skips the findings it cannot resolve and keeps the remainder, so a partial offset is still worth having there. The branch carries that note so whoever relaxes the
exit 1policy revisits the threshold with it. This costs no upload that succeeds today, since a sampled path that misses under the prefix would have exited 1 either way.Shallowest-first still holds among complete matches, so the least aggressive rewrite is still what wins — a deeper strip discards structure the report gave us and is likelier to collide with same-named files, which is also why the bare-basename guard exists. What no longer happens is a shallow partial match stopping the search ahead of a deeper complete one (
a_deeper_prefix_that_resolves_everything_beats_a_shallow_partial_one).A report run against the wrong checkout still uploads the wrong files, and no threshold closes that: a report naming only paths that happen to exist in this tree resolves completely by definition.
corgea uploadhas always resolved report paths against the cwd, so this is long-standing — what this PR widens is the reach, from reports whose paths already match to reports whose paths match after a prefix drop. Closing it needs a repo identity check (remote URL or commit in the report against the checkout) and depends on the report formats carrying one; worth its own issue.Containment is narrowed but not closed.
mainopenedPath::new(report_path)with no containment at all, so a report naming an absolute path or../../etc/passwdwas read and uploaded. On this branch every path that carries the prefix is read asroot.join(relative)with..refused, and the as-written form is only used wheremainwould have used it anyway — a strict subset. Refusing out-of-root paths outright would also refuse absolute paths that legitimately resolve on the machine that generated the report, which is a product call about how far a report file is trusted and wants its own change.Pre-existing bug found nearby, not fixed here
path=is interpolated into thecode-uploadquery string unencoded, so a report path containing&or#is truncated on the wire and the file is stored under a path Fusion can never match:pathdoghouse receivesC:/Users/John Doe/src/App.javaC:/Users/John Doe/src/App.java(space is encoded byUrl::parse, round-trips)src/a&b.javasrc/asrc/a#b.javasrc/a&in a filename is not hypothetical —extract_file_path_pulls_scoped_source_locationscovers exactly that, since FVDL writes it as&. Passing the value throughurlencoding::encodeis the obvious fix and Django'sQueryDictwould decode it back, but it would send/as%2Fon every upload, so it wants a round-trip test against a real doghouse before shipping rather than being folded into this PR.Release
Cargo.tomlis bumped to 1.14.2 (from the releasedv1.14.1, which nothing else has landed on top of, so this branch is the whole of the next release). That is the only manual version edit: PyPI takes it through maturin's dynamic version and npm sets its own from the tag name, perRELEASING.md. Tagv1.14.2after merge.Testing
src/scanners/report_paths.rs, mirroringtests/test_report_paths.pyplus CLI-specific cases: rooted paths,file://URIs pointing inside and outside the project, Windows separators, absolute paths that resolve, containment, and partial vs. complete prefix matches.src/scan.rscoveringplan_source_uploads: the uploaded path stays as reported while the local file is rebased, and the prefix is settled before policy files are added.tests/cloud_commands_e2e/upload_report_paths.rsdriving the real binary against the HTTP stub: a build-agent-prefixed report and a rooted (/src/...) report each upload both files under their reported paths and exit 0; a report that resolves under no prefix still exits 1 and names where it looked../harness cigreen: strict clippy, format check, dep audit, 864 tests + coverage gate,corgea --versionreporting1.14.2.