fix: prevent panic on non-string ScanCP args.name/namespace - #415
Conversation
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
…ed fix Signed-off-by: rootp1 <arnav.iitr@gmail.com>
Use comma-ok type assertions in the ScanCP handler and validation path so malformed args.name/args.namespace values return ErrMissingCpInfo instead of panicking. Signed-off-by: rootp1 <arnav.iitr@gmail.com>
📝 WalkthroughWalkthrough
ChangesScan argument validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…e-assertion-panic # Conflicts: # repositories/apiserver_test.go
|
@matthyx can you please have a look whenever you have a chance to |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed at cc7daf0.
The core fix is right. Args is a map[string]interface{} populated straight from the request JSON (controllers/http.go:194, Args: c.Args), so args.name: 123 — or no args at all — reaches newScan.Args[domain.ArgsName].(string) at line 105 before any validation runs. I put your new test in front of main's core/services/scan.go and it reproduces exactly what #414 describes:
--- FAIL: TestScanService_ValidateScanCP/missing_args
panic: interface conversion: interface {} is nil, not string
With the comma-ok assertions the malformed value falls through to ErrMissingCpInfo, which is the behaviour the issue asks for, and go test ./controllers/... ./core/services/... is clean here.
One blocker: this PR re-introduces the VEX backfill loop we dropped in #404 on purpose. repositories/apiserver.go and repositories/apiserver_test.go have nothing to do with #414, and the code they carry is the unreachable loop from the pre-rebase version of that PR. Details inline. Everything else is non-blocking.
Follow-up, not for this PR
The same bug class is still live one layer down, and there it is worse. adapters/v1/backend.go:322-340 does an unchecked val.(string) on seven workload.Args[...] values, fed by the same client-supplied args map (registryScanCommandToScanCommand also does Args: c.Args). Confirmed by calling SubmitCVE with Args: {"registryName": 123}:
panic: interface conversion: interface {} is int, not string
.../adapters/v1/backend.go:322
SubmitCVE runs inside a h.workerPool.Submit(...) task, and gammazero/workerpool has no recover() anywhere in its worker loop — so unlike the panic this PR fixes, which gin.Recovery() (cmd/http/main.go:121-122) downgrades to a 500, that one takes the process down. backend.go:174 in the same file already does it the safe way, so the fix is mechanical. Worth its own issue rather than growing this PR.
|
|
||
| // Backfill statements that were already marked "affected" by a version of kubevuln | ||
| // predating action_statement support, or that were left stale because their CVE fell | ||
| // out of the current relevancy set before the backfill could run on a prior update. | ||
| for i, s := range vexDoc.Statements { | ||
| if s.Status == v1beta1.Status(vex.StatusAffected) && s.ActionStatement == "" { | ||
| vexDoc.Statements[i].ImpactStatement = "" | ||
| vexDoc.Statements[i].ActionStatement = defaultActionStatement | ||
| } | ||
| } |
There was a problem hiding this comment.
Blocker — this is the backfill loop from the pre-rebase version of #404, which we dropped there on purpose, and none of it belongs to #414.
Recap from that thread: I asked for this loop in my first review, then #401 (a5e45e3) landed the reset pass in updateVEX that sends every statement back to not_affected with ActionStatement = "" before markRelevantVulnerabilitiesAsAffectedInVex reapplies the manifest. That made the loop unreachable, so 53b2adf dropped it — merged #404 (33b467b) has no backfill.
It is unreachable on this branch too, on both callers:
createVEX(apiserver.go:856) — statements are constructednot_affected, and the mark loop then setsActionStatement = buildActionStatement(v), which never returns""(it falls back todefaultActionStatement).updateVEX(apiserver.go:952-957) — the reset loop clearsStatusandActionStatementimmediately before the call.
Same probe as last time: replace the loop body with panic("BACKFILL LOOP REACHED") and run the suite.
$ go test ./repositories/...
ok github.com/kubescape/kubevuln/repositories 2.761s
Zero hits — including from the test this PR adds below.
How it got here: the branch was cut before that rework (35619a3 and 2f04c5a predate it) and the cc7daf0 merge of upstream/main auto-merged apiserver.go — main never had the loop, so there was nothing for the merge to remove and your side was kept verbatim. git checkout upstream/main -- repositories/apiserver.go repositories/apiserver_test.go should get this PR back to the three files #414 is about.
| // TestAPIServerStore_updateVEX_backfillsActionStatementOnStaleAffectedStatements guards against a | ||
| // regression where a statement marked "affected" by an older kubevuln (before action_statement | ||
| // support) kept an empty action_statement forever. Since updateVEX now resets every statement to | ||
| // "not_affected" before reapplying the current filtered manifest, a CVE that fell out of the | ||
| // relevancy set is expected to be reset rather than backfilled. | ||
| func TestAPIServerStore_updateVEX_backfillsActionStatementOnStaleAffectedStatements(t *testing.T) { | ||
| cveManifest := tools.FileToCVEManifest("testdata/nginx-cve.json") | ||
| cveManifestFiltered2 := tools.FileToCVEManifest("testdata/nginx-cve-filtered-2.json") | ||
| cveManifestFiltered := tools.FileToCVEManifest("testdata/nginx-cve-filtered.json") | ||
|
|
||
| a := NewFakeAPIServerStorage("kubescape") | ||
|
|
||
| ctx := context.TODO() | ||
| workload := domain.ScanCommand{ | ||
| ImageHash: "sha256:32fdf92b4e986e109e4db0865758020cb0c3b70d6ba80d02fe87bad5cc3dc228", | ||
| InstanceID: "apiVersion-apps/v1/namespace-kubescape/kind-ReplicaSet/name-kubevuln-65bfbfdcdd/containerName-kubevuln", | ||
| Wlid: "wlid://cluster-aaa/namespace-anyNamespaceJob/job-anyJob", | ||
| ImageTag: "registry.k8s.io/coredns/coredns:v1.10.1", | ||
| ContainerName: "anyJobContName", | ||
| } | ||
| ctx = context.WithValue(ctx, domain.WorkloadKey{}, workload) | ||
|
|
||
| // First store: CVE-2005-2541 is relevant and marked affected. | ||
| err := a.StoreVEX(ctx, cveManifest, cveManifestFiltered2, false) | ||
| assert.Equal(t, err, nil) | ||
|
|
||
| vexContainer, err := a.StorageClient.OpenVulnerabilityExchangeContainers(a.Namespace).Get(context.Background(), cveManifest.Name, metav1.GetOptions{}) | ||
| assert.Equal(t, err, nil) | ||
|
|
||
| // Simulate a document written by a pre-fix kubevuln: affected but no action_statement. | ||
| found := false | ||
| for i, stmt := range vexContainer.Spec.Statements { | ||
| if stmt.Vulnerability.Name == "CVE-2005-2541" { | ||
| vexContainer.Spec.Statements[i].ActionStatement = "" | ||
| found = true | ||
| } | ||
| } | ||
| require.True(t, found, "expected CVE-2005-2541 to be present after the first store") | ||
|
|
||
| _, err = a.StorageClient.OpenVulnerabilityExchangeContainers(a.Namespace).Update(ctx, vexContainer, metav1.UpdateOptions{}) | ||
| assert.Equal(t, err, nil) | ||
|
|
||
| // Second store: CVE-2005-2541 is no longer in the filtered manifest. | ||
| err = a.StoreVEX(ctx, cveManifest, cveManifestFiltered, false) | ||
| assert.Equal(t, err, nil) | ||
|
|
||
| vexContainer, err = a.StorageClient.OpenVulnerabilityExchangeContainers(a.Namespace).Get(context.Background(), cveManifest.Name, metav1.GetOptions{}) | ||
| assert.Equal(t, err, nil) | ||
|
|
||
| found = false | ||
| for _, stmt := range vexContainer.Spec.Statements { | ||
| if stmt.Vulnerability.Name == "CVE-2005-2541" { | ||
| found = true | ||
| assert.Equal(t, v1beta1.Status(vex.StatusNotAffected), stmt.Status, "statement no longer relevant should be reset to not_affected") | ||
| assert.Empty(t, stmt.ActionStatement, "not_affected statement must not carry an action_statement") | ||
| } | ||
| } | ||
| require.True(t, found, "expected CVE-2005-2541 statement to still be present after the update") | ||
| } |
There was a problem hiding this comment.
Blocker — goes away with the same revert as apiserver.go, but flagging separately because this test is misleading on its own terms.
The name says backfillsActionStatementOnStaleAffectedStatements; the assertions say the opposite — not_affected and an empty ActionStatement, i.e. that the backfill does not happen. Your own doc comment on lines 453-455 says as much. It passes, but it passes against the reset behaviour from #401, not against the loop it is named for: delete that loop and this test is still green.
The invariant it does check is already covered, and more broadly. TestAPIServerStore_storeVEX_updateRestoresNotAffected (line 360) walks every statement in the document rather than just CVE-2005-2541, and already asserts assert.Empty(t, stmt.ActionStatement, ...) at line 408. So there is no coverage lost by dropping these 60 lines.
| name, _ := newScan.Args[domain.ArgsName].(string) | ||
| namespace, _ := newScan.Args[domain.ArgsNamespace].(string) |
There was a problem hiding this comment.
Non-blocking, but the handler is the half of this fix with no test behind it.
#414's repro hits these two lines before ValidateScanCP is ever reached, yet the only test added is at the service layer — and there is no TestHTTPController_ScanCP at all today (http_test.go has Alive, GenerateSBOM, Ready, ScanCVE, ScanRegistry, registryScanCommandToScanCommand, ContextCancellationIsDetached). Drop either , _ here and the suite stays green.
A small handler test closes it: POST /v1/scanCP with "args": {"name": 123, "namespace": true}, asserting the validation error is returned and no job reaches the pool. One thing to watch — register the route on a bare gin.New() rather than gin.Default(), otherwise gin.Recovery() converts a regression into the same 500 the validation path returns and the test can't tell the two apart. The contextSpyScanService harness at line 289 already gives you the "did the worker run" signal.
| if (err != nil) != tt.wantErr { | ||
| t.Errorf("ValidateScanCP() error = %v, wantErr %v", err, tt.wantErr) | ||
| return | ||
| } |
There was a problem hiding this comment.
Nit: wantErr bool is satisfied by any error, but the contract in #414 and in your description is specifically that malformed values fall through to domain.ErrMissingCpInfo. A wantErr error field with assert.ErrorIs(t, err, tt.wantErr) pins the behaviour you actually claim, and would still trip if someone later swaps the sentinel or short-circuits earlier in the function.
Signed-off-by: rootp1 <arnav.iitr@gmail.com>
|
@matthyx you're right, that was leftover from a merge conflict resolution and doesn't belong in this PR. I removed the backfill loop from apiserver.go and the accompanying test, keeping this PR scoped to the ScanCP args type assertion fix. |
|
@matthyx pushed the fix, could you take another look when you have a chance? |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/services/scan_test.go`:
- Around line 607-621: Expand the invalid-argument cases in the scan validation
tests around the existing “non-string name and namespace” workload so
domain.ArgsName and domain.ArgsNamespace are each tested independently as
missing and non-string. Retain or replace the combined case only if useful, and
ensure each case expects validation failure so the validator must reject either
invalid argument individually.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c173bffe-fb47-49f5-a549-a5e915210003
📒 Files selected for processing (3)
controllers/http.gocore/services/scan.gocore/services/scan_test.go
| { | ||
| name: "missing args", | ||
| workload: domain.ScanCommand{}, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "non-string name and namespace", | ||
| workload: domain.ScanCommand{ | ||
| Args: map[string]interface{}{ | ||
| domain.ArgsName: 123, | ||
| domain.ArgsNamespace: true, | ||
| }, | ||
| }, | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover each invalid argument independently.
The current invalid case makes both domain.ArgsName and domain.ArgsNamespace invalid at the same time. Add cases where only one argument is missing or non-string. The combined case would still pass if the validation condition changed from || to &&.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/services/scan_test.go` around lines 607 - 621, Expand the
invalid-argument cases in the scan validation tests around the existing
“non-string name and namespace” workload so domain.ArgsName and
domain.ArgsNamespace are each tested independently as missing and non-string.
Retain or replace the combined case only if useful, and ensure each case expects
validation failure so the validator must reject either invalid argument
individually.
2 participants
Overview
Both the HTTP controller and the service-layer validation for the
ScanCP(application profile scan) path perform unchecked type assertions onArgs["name"]andArgs["namespace"]. A request with a non-string value for either field causes a panic instead of a validation error.This PR replaces the unchecked type assertions in
controllers/http.go,core/services/scan.go(ValidateScanCP), andcore/services/scan.go(ScanCP) with comma-ok assertions, so malformed values fall through to the existingdomain.ErrMissingCpInfovalidation error path instead of panicking.How to Test
go test ./controllers/... ./core/services/...Added
TestScanService_ValidateScanCP, covering missing args and non-stringname/namespacevalues, which previously would panic.Related issues/PRs:
Resolved #414
Checklist before requesting a review
Summary by CodeRabbit
Bug Fixes
Tests