fix(scanner): handle digest-only MainAssetName without panic - #42
fix(scanner): handle digest-only MainAssetName without panic#42henrikfrech wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a panic in BaseAdapter.ToHarborVulnerabilityReport when Sysdig returns digest-only (tagless) mainAssetName values by introducing a safer parser and using it during report conversion.
Changes:
- Replaced brittle string slicing with a new
parseMainAssetNamehelper to support bothrepo:tag@digestandrepo@digestformats. - Added Ginkgo tests covering common
mainAssetNamevariants (with/without tags, with/without registry prefixes). - Updated scanner test fixture expectations to include populated
Artifactdata.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/scanner/base_adapter.go | Uses parseMainAssetName to avoid panics and correctly populate Harbor artifact fields. |
| pkg/scanner/base_adapter_test.go | Adds unit tests for parseMainAssetName across tagged/digest-only/registry cases. |
| pkg/scanner/backend_adapter_test.go | Updates expected vulnerability report to include Artifact details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Treat a tag separator with an empty tag (e.g. "repo:@sha256:...") as a malformed reference so the caller logs and skips instead of returning an empty tag. Add tests for registry-host-with-port, single-name official images, and the empty-tag malformed case. Addresses Copilot review comments on PR sysdiglabs#42.
|
Hello @airadier, |
Add a doc comment describing the accepted MainAssetName reference forms and the registry-host stripping rule, and rename the hash return value/local to digest to match its value (algo:hex) and the harbor.Artifact.Digest field. Addresses code-review nits from PR sysdiglabs#42 round 2.
airadier
left a comment
airadier
left a comment
There was a problem hiding this comment.
Approving.
This fixes a real, reproducible panic (slice bounds out of range [:-1]) in ToHarborVulnerabilityReport when Sysdig returns a digest-only MainAssetName, which is hit on the active InlineAdapter path (inline_adapter.go:248). As a bonus it also fixes a latent bug where un-prefixed repo names (e.g. sysdig/agent) had their first segment stripped as if it were a registry, so Artifact was silently never populated — hence the corrected fixture in backend_adapter_test.go.
Reviewed via 3 independent review passes (Go idioms, general quality, adversarial edge cases): unanimous, 0 critical/high. The parser was exercised against 30+ reference forms with no panics, and the added tests lock in the tricky cases (digest-only, registry+port disambiguation, empty tag).
The two Copilot comments (empty-tag rejection + its test) are addressed, and I pushed two small maintainability commits (doc comment on parseMainAssetName, hash→digest rename). Remaining low-severity items are pre-existing and out of scope — tracked as a follow-up issue.
LGTM. Thanks @henrikfrech for the fix.
|
Follow-up items (all pre-existing, out of scope for this PR) tracked in #43. |
|
Thanks so much for this contribution, @henrikfrech! 🙌 This is a genuinely valuable fix — you caught a real panic on the active scanning path, diagnosed the root cause precisely (the We ran it through a thorough review and it came out great. I've pushed a couple of tiny maintainability touch-ups (a doc comment and a Really appreciate you taking the time to not just report the issue but send a well-tested fix. Contributions like this are exactly what makes the project better. Thank you! 🎉 |
|
Hi @henrikfrech — quick heads-up on how we're landing this. 🙏 Your fix is spot on, but we hit a purely mechanical hurdle: this PR comes from a fork, and for security GitHub doesn't share our repository secrets with workflows triggered by fork PRs. Our So to get everything green and merge cleanly, we've re-applied your exact commits (with your authorship preserved) onto an internal branch and opened #44, where the pipeline has access to the secrets and runs the full suite. We also folded in a Nothing needed from you — we'll merge via #44 and this PR will be closed as part of that. Huge thanks again for the well-tested contribution; it's shipping exactly as you wrote it. 🎉 |
…des #42) (#44) * fix: handle digest-only image references in report parsing * fix(scanner): reject empty tag in parseMainAssetName Treat a tag separator with an empty tag (e.g. "repo:@sha256:...") as a malformed reference so the caller logs and skips instead of returning an empty tag. Add tests for registry-host-with-port, single-name official images, and the empty-tag malformed case. Addresses Copilot review comments on PR #42. * docs(scanner): document parseMainAssetName and rename hash to digest Add a doc comment describing the accepted MainAssetName reference forms and the registry-host stripping rule, and rename the hash return value/local to digest to match its value (algo:hex) and the harbor.Artifact.Digest field. Addresses code-review nits from PR #42 round 2. * chore(deps): bump golang.org/x/net to v0.57.0 to fix CVEs Trivy flagged 8 vulnerabilities (4 HIGH) in golang.org/x/net v0.51.0 (CVE-2026-25681, CVE-2026-33814, CVE-2026-39821, and others). Bump the golang.org/x/* modules and refresh the nix vendorHash accordingly. --------- Co-authored-by: Henrik Frech <henrik.frech@sva.de>
What
Fix panic in scanner report conversion when Sysdig returns digest-only references in
mainAssetName(e.g.repo@sha256:...).Why
ToHarborVulnerabilityReportassumedrepo:tag@digestand sliced withlastColon=-1for tagless images, causing:panic: runtime error: slice bounds out of range [:-1].Changes
parseMainAssetNamehelper inpkg/scanner/base_adapter.gorepo:tag@sha256:...repo@sha256:...pkg/scanner/base_adapter_test.gopkg/scanner/backend_adapter_test.goValidation
go test -count=1 ./pkg/scanner✅go test ./pkg/scanner/...✅Impact