fix: validate OpenVulnerabilityExchangeContainer spec (#355)

[Shreya2005-2005]

Overview

OpenVulnerabilityExchangeContainerStrategy.Validate() discarded the
object it was given and always returned an empty error list, silently
accepting a completely empty or malformed VEX spec.

This adds real validation, following the same pattern already used by
NetworkNeighborhoodStrategy.Validate(): require a non-empty Author.
The same check now also runs on ValidateUpdate, since kubevuln's
steady-state path is update, not create.

Before

errors found: 0

(on a completely empty spec)

After

errors found: 1
  - spec.author: Required value: must not be empty

(zero-statement specs with an author set — e.g. a clean-image scan
with nothing to report — are correctly accepted, not rejected)

Testing

strategy_test.go covers: missing author, a valid spec with zero
statements (the clean-image scan case), a valid spec with statements,
and both create and update paths.

Update (per review)

The original version of this PR also required at least one statement,
which incorrectly rejected valid zero-finding scans from kubevuln's
createVEX. That check has been removed. Validation is now shared
between Validate and ValidateUpdate via a single helper, and the
type assertion uses the safer comma-ok form.

Related issues/PRs:

• Resolved OpenVulnerabilityExchangeContainer Validate() accepts empty/malformed specs - no actual validation #355

Checklist before requesting a review

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• New and existing unit tests pass locally with my changes

[coderabbitai]

📝 Walkthrough

Walkthrough

The VEX strategy now validates object types and requires spec.author during create and update operations. Tests cover missing authors, empty statements, and populated statements.

Changes

VEX validation

Layer / File(s)	Summary
Required fields and validation tests pkg/registry/softwarecomposition/openvulnerabilityexchange/strategy.go, pkg/registry/softwarecomposition/openvulnerabilityexchange/strategy_test.go	The strategy applies shared validation to create and update operations. It returns internal errors for invalid object types and requires spec.author. Tests cover missing authors and authored objects with empty or populated statements.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation validates spec.author but allows empty spec.statements, so it does not meet the required validation in [#355].	Require at least one spec.statements entry and return the corresponding field.Required error when the list is empty.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The implementation and tests remain within the linked issue scope, despite missing one required validation condition.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: validating the OpenVulnerabilityExchangeContainer specification.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[matthyx]

Thanks for picking this up — the diagnosis in #355 is correct: Validate() really was a no-op, and the object parameter really was discarded. The test file is well structured and matches the table-driven style used elsewhere in the repo. I ran it locally against main and it passes (go test ./pkg/registry/softwarecomposition/openvulnerabilityexchange/..., go vet clean).

Two things block merge as written.

1. The statements check rejects writes that are legitimate today (cross-repo regression)

See the inline comment on strategy.go. Short version: kubevuln builds the statement list by looping over the scan matches, so an image with zero findings produces a VEX document with a valid author and zero statements. After this PR the apiserver rejects that create with a 422, kubevuln logs storing VEX as a warning on every scan of every clean image, and the VEX object is never created for those images. A VEX document asserting "scanner ran, nothing to report" is meaningful, and OpenVEX itself does not require a non-empty statement list.

2. ValidateUpdate is still a no-op, so the invariant isn't actually enforced

ValidateUpdate at strategy.go:90 still returns an empty field.ErrorList{}. Since kubevuln's steady-state path is updateVEX → Update() (only the very first write goes through Create), any spec that fails the new create-time check can still be written via update. Both siblings this PR follows — applicationprofile and networkneighborhood — run the same validation from both Validate and ValidateUpdate. Whatever rule we settle on in point 1 should be applied in both places (factored into a shared helper), otherwise the guarantee is cosmetic.

Suggested shape

Keep the spec.author check (kubevuln always sets it, and an unattributed VEX document is genuinely malformed), drop the spec.statements check, and wire the same helper into ValidateUpdate. That fixes the real bug from #355 without changing what the cluster is allowed to store. If you do want to keep a statements rule, it needs a heads-up on the kubevuln side first, since it is a cross-repo behavior change.

Non-blocking notes

• The unit-test workflow (pr-created.yaml) hasn't reported on this PR yet — only DCO / GitGuardian / CodeRabbit did. A maintainer needs to approve workflow runs for a first-time contributor; worth getting green before merge.
• The PR description ends with a stray ``` fence.

[matthyx]

Blocker: this rejects a create that is valid and routine today.

kubevuln's createVEX builds the statement list by ranging over the scan matches:

vexDoc := v1beta1.VEX{Metadata: v1beta1.Metadata{Author: "kubescape.io", ...}}
for _, v := range cve.Content.Matches {
    vexDoc.Statements = append(vexDoc.Statements, ...)
}
// ...
_, err = a.StorageClient.OpenVulnerabilityExchangeContainers(a.Namespace).Create(ctx, &vexContainer, ...)

(kubescape/kubevuln, repositories/apiserver.go)

For an image with zero vulnerabilities — distroless/scratch images, or anything grype finds nothing in — Matches is empty, so Statements is nil while Author is set. StoreVEX has no guard for this; it only short-circuits on an empty name. So after this PR:

1. the apiserver returns 422 Invalid: spec.statements: Required value,
2. StoreVEX propagates the error and core/services/scan.go logs storing VEX as a warning on every scan of every clean image,
3. no VEX object is ever created for those images, where one is created today.

That's a cross-repo behavior change, and it fires on the happy path. It's also arguably wrong on the merits: "the scanner ran and found nothing to declare" is a meaningful VEX document, and OpenVEX doesn't require a non-empty statement list either — go-vex validates individual statements (pkg/vex/statement.go), not a document-level minimum.

I'd drop this check. The spec.author check above is the one that catches the real defect from #355 without changing what the cluster accepts.

[matthyx]

Nit (consistent with the siblings, so not blocking): the unchecked type assertion panics if this is ever called with an unexpected type, and a panic inside a strategy takes down the apiserver request path. applicationprofile / networkneighborhood do the same thing, so I won't hold the PR on it, but the comma-ok form returning field.ErrorList{field.InternalError(field.NewPath(""), err)} is cheap insurance if you're touching the line anyway.

[matthyx]

Worth noting how shallow this makes the "valid" case: Statement{} is entirely empty — no vulnerability, no status, no products — yet it counts as a valid spec. go-vex's own Statement.Validate() (pkg/vex/statement.go) requires at least a vulnerability name and a status, so this passes admission while being a document go-vex would reject.

Not a blocker on its own, but it does show the value the statements-length check is actually buying is close to zero: it only catches len == 0, not malformed content. If we want real statement validation that's a separate, larger change worth its own issue.

[Shreya2005-2005]

@matthyx thanks for the thorough review , all three points addressed:

• dropped the spec.statements check entirely (kept only spec.author,
  since zero-statement documents from clean-image scans are valid)
• ValidateUpdate now shares the same check via validateVEXSpec,
  instead of being a no-op
• switched to the comma-ok type assertion in both Validate and
  ValidateUpdate

Also updated the tests to cover the zero-statement valid case and both
create/update paths, and cleaned up the stray fence in the description.

Let me know if anything else needs adjusting

[matthyx]

All three points addressed correctly — thanks for the quick turnaround.

Blocker 1 (statements) — resolved. The spec.statements check is gone, and the comment on validateVEXSpec records why it's deliberately absent, which is exactly the right thing to leave behind so nobody re-adds it in six months. The new author set, zero statements is valid (clean-image scan) case pins the behavior in a test.

Blocker 2 (asymmetry) — resolved. Validate and ValidateUpdate now share validateVEXSpec, matching how applicationprofile and networkneighborhood do it, and TestValidateUpdate_RequiresAuthor covers the update path directly.

Nit (type assertion) — resolved, in both methods.

I re-checked the surviving spec.author requirement against every writer, since it now applies to updates too. kubevuln is the only component that writes these objects: createVEX hardcodes Author: "kubescape.io", and updateVEX copies the existing spec (vexDoc := vexContainer.Spec), mutates only LastUpdated and the statement list, then writes it back — so the author always survives a round-trip. No write path loses it.

Verified locally on 9c03759:

go build ./...                                                    ok
go vet ./pkg/registry/softwarecomposition/...                     ok
go test -count=1 ./pkg/registry/softwarecomposition/openvulnerabilityexchange/...  ok (4/4 pass)
gofmt -l                                                          clean

One non-blocking note for whoever merges: spec.author is now enforced on update, so any VEX object already stored with an empty author would become un-updatable. In practice that set should be empty (kubevuln has always set the author, and the file registry writes below the REST layer without going through validation), so I don't think it warrants a guard — just worth knowing it's the one behavior that reaches back to existing objects.

LGTM. Note the repo's unit-test workflow still hasn't reported here — a maintainer needs to approve the run for a first-time contributor before merge.