Skip to content

Add corgea scan --include-image to scan fully built container images - #153

Open
Ibrahimrahhal wants to merge 5 commits into
mainfrom
cursor/cli-include-image-container-scanning-a05d
Open

Add corgea scan --include-image to scan fully built container images#153
Ibrahimrahhal wants to merge 5 commits into
mainfrom
cursor/cli-include-image-container-scanning-a05d

Conversation

@Ibrahimrahhal

@Ibrahimrahhal Ibrahimrahhal commented Aug 12, 2026

Copy link
Copy Markdown
Member

Description

Container scanning today only sees the base images referenced by the source tree (Dockerfile FROM, Compose image:). This adds a way to scan the image a build actually produces.

docker build -t myapp:1.2.3 .
corgea scan --include-image myapp:1.2.3 --include-image ghcr.io/acme/api:latest

For each --include-image value the CLI exports the image with docker save (or podman save) into corgea-image-scanning-<name>-<tag>.tar, stages it in an owner-only temp directory, and adds it to the project zip that gets uploaded. Fusion greps the extracted bundle for corgea-image-scanning-*.tar; when it finds any it scans those archives and skips base-image discovery (companion PR in fusion).

Behavior details

  • Repeatable flag, blast only — same shape as --metadata. Rejected for semgrep/snyk.
  • References are validated and de-duplicated before anything runs: empty values, whitespace, and values starting with - are hard errors, so nothing user-supplied can turn into a flag for the container CLI.
  • Missing images are pulled first. <engine> image inspect decides; on a miss the CLI runs <engine> pull with inherited stdio so the pull progress is visible, and a failure names the image and points at building/pulling and registry login.
  • Engine selection: docker, then podman, else an actionable error. CORGEA_CONTAINER_ENGINE overrides it (also how the tests inject a stub CLI).
  • Archive naming splits repository[:tag][@digest], treats a colon in the registry host as a port (registry:5000/team/app), defaults an untagged reference to latest, sanitizes to [A-Za-z0-9._-], caps the file name at 200 chars, and suffixes any name two references collapse onto — compared case-insensitively, since tags may carry uppercase and a case-insensitive filesystem would otherwise export myapp:V1 over myapp:v1. The name is also what Fusion labels findings with, so it stays readable: myapp:1.2.3 becomes myapp-1.2.3.
  • An included image is sufficient input. When a target matches no source files — a clean tree under --only-uncommitted, say — the scan warns and covers just the image instead of failing after the export.
  • A corgea-image-scanning-*.tar committed to the repository is excluded from the bundle, so it can't put the backend into image-scanning mode on scans that never asked for it.
  • Staging is owner-only and always cleaned up. Every exit after the export removes the directory, so a failed scan doesn't leave gigabytes behind.

create_zip_from_target takes an extra_files: &[(PathBuf, String)] list of (staged path, zip entry name) pairs written to the zip root, with large_file(true) so entries past 4 GiB get ZIP64 headers. Exclude globs deliberately don't apply to them — these files are there because the user named them on the command line.

Review follow-ups

All findings from the automated reviews and @juangaitanv are addressed:

Finding Change
Exports over 4 GiB fail without ZIP64 large_file(true) on the extras' FileOptions; #[ignore]d test writes a sparse 4 GiB file
Insecure staging permissions New create_private_temp_dir: owner-only mode requested explicitly, random name
Cleanup skipped on several exits delete_directory on the zero-target, resolve-error and zip-error exits
Image-only scans incorrectly fail Empty target warns and continues when images are bundled
Checked-in archive hijacks scan mode Prefix added to the default exclude globs
Case-only reference collision Case-insensitive comparison in unique_archive_name
Shell-backed tests break on Windows #[cfg(unix)] on both tests and the stub helper

Two notes on the staging fix. tempfile documents that temporary directories are created with default permissions (only files are private), so Builder::tempdir() alone would still be world-readable — the mode is passed explicitly, and a unit test asserts 0700. And the suggested TempDir RAII guard can't work as-is: blast::run ends the process through std::process::exit in ~30 places, which skips destructors, so cleanup stays explicit and the path is taken with keep(). tempfile moved from 3.12 to 3.20 for TempDir::keep; the lockfile already resolved to 3.23, so no lock change.

Type of Change

  • New feature
  • Tests
  • Documentation update

Testing Method & Results

./harness check is green (685 tests). Coverage:

  • src/images.rs: archive naming (tag, digest, registry port, untagged, length cap), reference validation/de-duplication, export against a stub container CLI, collision suffixing including case-only, and a failing-CLI error.
  • src/utils/generic.rs: extras land at the zip root under the requested entry name; staging directory is 0700; and an #[ignore]d ZIP64 test (cargo test --bin corgea -- --ignored four_gib, ~60s). I confirmed that test fails with "Large file option has not been set" before the fix and passes after.
  • tests/cli_scan_include_image.rs (e2e, real binary + stubbed API + stubbed container CLI): both archives bundled into the uploaded zip and named in the output; nothing bundled without the flag; an unavailable image exits 1 without uploading; a bad reference exits 1; --only-uncommitted on a committed, clean tree still uploads the archive; a checked-in archive is not bundled while source files still are.

Manual checks: ran the built binary against a stub upload server with a stub engine standing in for docker save. The uploaded zip contained main.py, Dockerfile, and both corgea-image-scanning-*.tar entries, and feeding the extracted bundle to Fusion's container step scanned both archives without touching skopeo or the Dockerfile's alpine:3.19. Separately confirmed that a resolve error after an export now leaves no corgea-scan-* directory behind.

Dependency

  • Requires the companion fusion PR to be deployed for the archives to be scanned. Until then the archives are uploaded and ignored.
  • Container scanning must be enabled for the account (container_scan scan config); the CLI can't turn it on.
Open in Web Open in Cursor 

corgea scan --include-image <image:tag> exports each image with docker (or
podman) into corgea-image-scanning-<name>-<tag>.tar, bundles the archives with
the uploaded project zip, and lets the backend scan the fully baked images
instead of rediscovering base images from Dockerfiles and Compose files.

Co-authored-by: ibrahim <ibrahim@corgea.com>
@Ibrahimrahhal
Ibrahimrahhal marked this pull request as ready for review August 12, 2026 06:41
Comment thread src/scanners/blast.rs
Comment thread src/images.rs
Comment thread src/scanners/blast.rs
@corgea-security corgea-security added the dennis-reviewed Dennis completed an automated review label Aug 12, 2026

@corgea-security corgea-security left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review risk: 4/5.

High risk: image staging can expose or leak large archives, and image-only scans can incorrectly fail. Tests also break on Windows.

Critical or high-priority changes must be addressed.

Automatic approval was not submitted: automated review found critical or high-priority findings.

Comment thread src/utils/generic.rs Outdated
Comment thread src/images.rs
Comment thread src/images.rs Outdated
cursoragent and others added 3 commits August 13, 2026 10:26
Archive names were compared case-sensitively, so myapp:V1 and myapp:v1 —
distinct images, since tags allow uppercase — exported over each other on a
case-insensitive filesystem and both scanned as the same image.

Also gates the two tests that execute the POSIX stub engine to Unix. Windows
can't run that script through Command::new, so cargo test failed there; CI
never caught it because the Windows jobs only build wheels.

Co-authored-by: ibrahim <ibrahim@corgea.com>
- Stage in an owner-only directory with an unguessable name. create_dir_all
  under a 0022 umask left the project zip and any exported images readable by
  other local users, and a fixed /tmp/corgea parent can be pre-created, or
  pointed elsewhere by a symlink, by another user first. tempfile creates
  directories with default permissions, so owner-only is requested explicitly.
- Write image archives with ZIP64 headers: without large_file the zip writer
  aborts an entry once it passes 4 GiB, which exported images reach routinely.
- Exclude corgea-image-scanning-*.tar found in the repository, so a committed
  copy cannot put the backend into image-scanning mode on scans that never
  asked for it.
- Treat an exported image as sufficient input. A target that matches nothing —
  a clean tree under --only-uncommitted — now warns and scans the image
  instead of exporting it and then failing on the empty target.
- Delete staging on every exit after the export, not only on upload failure.

Co-authored-by: ibrahim <ibrahim@corgea.com>
Co-authored-by: ibrahim <ibrahim@corgea.com>
Comment thread src/images.rs
Comment thread src/images.rs
Comment thread src/images.rs
Comment thread src/images.rs
Conflict in the blast staging block: main (#150) dropped the get_repo_info call
that sat after it, replacing it with the repo_before/repo_after pair that feeds
reconcile_repo_info_for_upload. Kept this branch's create_private_temp_dir and
main's new repo-state handling, and removed the now-stale repo_info binding.
Comment thread src/scanners/blast.rs
.map(|archive| (archive.path.clone(), archive.archive_name.clone()))
.collect();

let stop_signal = Arc::new(Mutex::new(false));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Quality - The code repeatedly performs spinner stop, join, terminal reset, and cleanup logic across multiple error branches after printing the "Packaging your project..." spinner. This duplicated code risks inconsistency and harder maintenance. View in Corgea ↗

More Details
🎟️Issue Explanation: The code repeatedly performs spinner stop, join, terminal reset, and cleanup logic across multiple error branches after printing the "Packaging your project..." spinner. This duplicated code risks inconsistency and harder maintenance.

- Duplication in spinner lifecycle management across error paths can cause inconsistent terminal states, e.g., missed resets in "stop spinner + join + reset terminal + cleanup" steps.
- Makes code harder to maintain and update since any change requires modifying multiple similar blocks dealing with spinner handling post validation in "packaging_thread".
- Increases cognitive load for the team when reading or debugging the packaging process as spinner logic is scattered rather than encapsulated in a clear RAII pattern or helper function.

We could not generate a fix for this.

@yhoztak yhoztak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dennis-reviewed Dennis completed an automated review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants