Validate docker image references before grype and syft scans - #50847
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Test Quality Sentinel analysis complete: Score 100/100 (Excellent). Both new tests (TestGrypeRunOnImage_RejectsUnsafeImageRef and TestRunSyftOnImage_RejectsUnsafeImageRef) enforce critical security design invariants for docker image reference validation. 3 attack vectors tested per function, no mock violations, 1.05:1 test:prod ratio. Ready for approval. (safeoutputs write disabled in this environment) |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (91 new lines). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Validate docker image references before grype and syft scans
The security improvements are correct and well-structured. The PR properly:
- Validates image refs via
validateDockerImageRefbefore anyexec.Commandcall in bothgrypeRunOnImageandrunSyftOnImage - Resolves
dockerviafileutil.ResolveExecutablePathto avoid PATH-based injection - Fixes stale "grant image reference" → "docker image reference" in error messages
- Uses
shellJoinArgsfor verbose logging (safe quoting) - Adds targeted tests covering flag-injection, newline injection, and semicolon injection
No blocking issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.9 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Verdict: no blocking issues
This is a solid, well-scoped fix. Both grype.go and syft.go now validate imageRef via the shared validateDockerImageRef allow-list before it reaches docker run, and resolve the docker binary via fileutil.ResolveExecutablePath, matching the pattern already used in grant.go, poutine.go, and runner_guard.go. The verbose-mode command string now uses shellJoinArgs instead of fmt.Sprintf, avoiding a printed-command quoting bug. Error message rename (grant image reference → docker image reference) correctly reflects the validator now being shared across three scanners, and no test asserts on the old string. New tests cover option-injection (--entrypoint=...), embedded-newline, and metacharacter (;) image refs for both scanners, and pass locally.
💡 Review notes
- Verified
dockerImageNamePattern/dockerImageTagPatternregex andcontainsControlCharactersare unchanged and still correctly reject--prefixed and control-character refs. - Confirmed no other call site still embeds the stale
grant image referencestring. - Ran
go test ./pkg/cli/... -run 'TestGrypeRunOnImage_RejectsUnsafeImageRef|TestRunSyftOnImage_RejectsUnsafeImageRef|TestValidateDockerImageRef|TestPoutine'— all pass. - No correctness, concurrency, or security regressions found in the changed lines. Nothing rises to a blocking comment.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-targeted; two minor test-quality issues noted.
📋 Key Themes & Highlights
Key Themes
- Test subtest naming: both
grype_test.goandsyft_test.gouse the rawimageRefas the subtest name, which embeds a literal newline for thealpine:latest\n--privilegedcase, garblinggo test -voutput and breaking-runfilters. - Missing positive-path coverage: no test confirms a well-formed image ref passes validation without a false rejection.
Positive Highlights
- ✅ Root cause correctly addressed —
validateDockerImageRefapplied at the call sites, not deeper, keeping the fix minimal and auditable. - ✅
fileutil.ResolveExecutablePath("docker")aligns grype/syft with the existing grant/poutine/runner-guard pattern — consistent architecture. - ✅
shellJoinArgsfor the verbose hint is a nice improvement overfmt.Sprintf. - ✅ Error message generalization in
docker_args_validation.gois clean and backward-compatible. - ✅
#nosec G204comment updated with accurate justification referencing the new validation.
| } | ||
|
|
||
| for _, imageRef := range unsafeRefs { | ||
| t.Run(imageRef, func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Subtest name is the raw imageRef — the embedded-newline case (alpine:latest\n--privileged) embeds a literal newline in the test name, making output garbled and breaking -run filters.
💡 Suggested fix
Use a named struct:
tests := []struct{ name, imageRef string }{
{"option flag", "--entrypoint=/bin/sh"},
{"embedded newline", "alpine:latest\n--privileged"},
{"semicolon", "ghcr.io/org/im;age:latest"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := grypeRunOnImage(tt.imageRef, false)
...
})
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 0ad7a0c47c: grype_test.go now uses stable named subtests instead of raw image refs, so the embedded-newline case no longer affects test names or -run filters.
| "ghcr.io/org/im;age:latest", | ||
| } | ||
|
|
||
| for _, imageRef := range unsafeRefs { |
There was a problem hiding this comment.
[/tdd] Same subtest naming issue as in grype_test.go: using the raw imageRef as a subtest name embeds a literal newline for the second case.
💡 Suggested fix
Apply the same named-struct approach as suggested for grype_test.go to keep test output readable.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0ad7a0c47c: syft_test.go now uses stable named subtests instead of raw image refs, matching the Grype test cleanup.
| t.Fatalf("Expected error for unsafe image reference %q", imageRef) | ||
| } | ||
| if !strings.Contains(err.Error(), "docker image reference") { | ||
| t.Errorf("Expected image reference validation error, got: %v", err) |
There was a problem hiding this comment.
[/tdd] Missing positive-path test: there's no assertion that a well-formed image ref (e.g. ghcr.io/anchore/grype:v0.80.0) is not rejected. Without it, an over-strict regex change could silently break valid scans.
💡 Suggested addition
func TestGrypeRunOnImage_AcceptsValidImageRef(t *testing.T) {
// Should reach docker (not fail validation); we expect a docker-not-found
// or network error, NOT a validation error.
_, err := grypeRunOnImage("ghcr.io/anchore/grype:v0.80.0", false)
if err != nil && strings.Contains(err.Error(), "docker image reference") {
t.Fatalf("Valid image ref was rejected: %v", err)
}
}@copilot please address this.
There was a problem hiding this comment.
Addressed in 0ad7a0c47c: added valid image-ref coverage using a fake docker executable, so the test verifies validation allows well-formed refs without contacting real Docker or the network.
There was a problem hiding this comment.
Pull request overview
Hardens Grype and Syft scans against unsafe manifest-derived image references.
Changes:
- Validates image references and resolves the Docker executable securely.
- Safely quotes verbose command hints and generalizes validation errors.
- Adds rejection tests and updates the workflow skill index.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/grype.go |
Secures Grype invocation. |
pkg/cli/grype_test.go |
Tests unsafe reference rejection. |
pkg/cli/syft.go |
Secures Syft invocation. |
pkg/cli/syft_test.go |
Tests unsafe reference rejection. |
pkg/cli/docker_args_validation.go |
Generalizes validation errors. |
.github/skills/agentic-workflows/SKILL.md |
Adds the designer mappings reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot run pr-finisher still |
|
@copilot Maintainer triage for this PR:
Run: https://github.com/github/gh-aw/actions/runs/31113572782
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
A Sighthound scan flagged five
exec.Commandcall sites as command-injection risks. Auditing them showed all five already canonicalize and validate their inputs — but two sibling scanners that the scan did not rank in the top 5 (grype,syft) do pass manifest-derived image references todocker rununvalidated.Audit of the flagged call sites (no changes needed)
runner_guard.go:115gitRootvalidated absolute; scan path viafilepath.Rel+IsLocal+./prefix (blocks option injection); mount viabuildDockerVolumeMount; docker path resolvedgrant.go:176imageRefallow-listed viavalidateDockerImageRef; policy mount viabuildDockerReadonlyFileMount; container path is a constantpoutine.go:109,:229gitRootvalidated; mount validated; all other args literalupgrade_command.go:507exerun throughfileutil.ValidateExecutablePath; argv rejected on control charactersAll use arg slices — no shell interpolation anywhere.
The actual gap
grypeRunOnImageandrunSyftOnImagetookimageRefstraight from lock-file manifests intodocker run, using the bare binary namedocker. A reference starting with-is parsed as a docker/grype/syft option, not an image:Changes
grype.go,syft.go— validateimageRefwith the existingvalidateDockerImageRefallow-list and resolve the binary viafileutil.ResolveExecutablePath("docker"), aligning with grant/poutine/runner-guard.grype.go,syft.go— verbose "run directly" hints now go throughshellJoinArgsinstead offmt.Sprintf, so the printed command is correctly quoted.docker_args_validation.go— error messages generalized from "grant image reference" to "docker image reference" now that three scanners share the validator.Run: https://github.com/github/gh-aw/actions/runs/31113572782> Generated by 👨🍳 PR Sous Chef · gpt54 · 17.2 AIC · ⊞ 8.3K · ◷