Skip to content

Fix: enable SecurityException CRDs in keepLocal mode#349

Merged
matthyx merged 8 commits intomainfrom
fix/security-exception-keeplocal
Apr 17, 2026
Merged

Fix: enable SecurityException CRDs in keepLocal mode#349
matthyx merged 8 commits intomainfrom
fix/security-exception-keeplocal

Conversation

@slashben
Copy link
Copy Markdown
Contributor

@slashben slashben commented Apr 16, 2026

Summary

SecurityException CRDs were not being read when kubevuln runs without cloud credentials (keepLocal mode). MockPlatform.GetCVEExceptions() returned empty, completely bypassing the CRD integration.

Root cause: The SecurityExceptionRepository was only wired into the BackendAdapter (cloud path), not the MockPlatform (keepLocal path).

Fix:

  • Add SecurityExceptionRepository to MockPlatform, read CRDs in GetCVEExceptions()
  • Move seRepo initialization before the keepLocal/cloud branch so both paths use it
  • Export ConvertToVulnerabilityExceptionPolicies for cross-package use
  • Nil-safe: existing tests pass nil repo (no CRD reading in unit tests)

Testing: Found during manual testing in a kind cluster — kubevuln logged "SecurityException CRD integration enabled" but exceptions had no effect because MockPlatform ignored them.

Note: In keepLocal mode, SubmitCVE is still a no-op, so exceptions are read but don't affect the stored VulnerabilityManifest. This is a known limitation — the stored manifest is always the raw Grype output. Exception-aware reporting will be fully effective when the operator watcher (Phase 2) triggers rescans through the full pipeline.

Part of NAUT-1258.

Test plan

  • All existing unit tests pass (adapters, adapters/v1, core/services)
  • kubevuln logs "SecurityException CRD integration enabled" in keepLocal mode
  • E2E: verify exception-marked CVEs in cloud-connected mode

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Security exceptions are now applied to vulnerability scan results, allowing configured exception policies to automatically filter matching CVEs from reports.
    • Vulnerabilities matching exception policies with ignore actions are now moved to ignored matches.
  • Improvements

    • Enhanced exception handling with graceful fallback behavior when fetching exceptions fails.
    • Improved integration of exception management throughout the scanning pipeline.

MockPlatform.GetCVEExceptions() was returning empty, bypassing
SecurityException CRDs when kubevuln runs without cloud credentials.

- Add SecurityExceptionRepository to MockPlatform
- Lift seRepo initialization before the keepLocal/cloud branch
- Export ConvertToVulnerabilityExceptionPolicies for cross-package use
- MockPlatform.GetCVEExceptions now reads CRDs via the repository

Note: In keepLocal mode, exceptions are read and available via
GetCVEExceptions, but SubmitCVE (which applies exceptions to reports)
is a no-op. This will be addressed when the operator watcher triggers
full rescans in Phase 2.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 16, 2026

Warning

Rate limit exceeded

@slashben has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 23 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 23 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9fb1721d-41e4-4e88-a3c9-448a900fb70c

📥 Commits

Reviewing files that changed from the base of the PR and between 01b049f and f6848b9.

📒 Files selected for processing (2)
  • adapters/v1/securityexception_test.go
  • core/services/scan.go
📝 Walkthrough

Walkthrough

This pull request integrates CRD-based security exception handling into the CVE scanning pipeline. The mock platform now accepts a security exception repository and conditionally returns CRD-derived exception policies. Security exceptions are applied to CVE manifests during scanning to move ignored matches to the ignored list. CRD client configuration is enhanced with improved performance parameters.

Changes

Cohort / File(s) Summary
Mock Platform Adapter
adapters/mockplatform.go, adapters/mockplatform_test.go, cmd/http/main_test.go, core/services/scan_test.go
Updated NewMockPlatform constructor to accept securityExceptionRepo parameter; updated GetCVEExceptions to conditionally return CRD-derived exception policies based on repository and workload context; test calls updated to pass nil repository.
Security Exception Conversion & Application
adapters/v1/securityexception.go, adapters/v1/securityexception_test.go, adapters/v1/backend.go
Exported ConvertToVulnerabilityExceptionPolicies (renamed from lowercase); added ApplySecurityExceptions to move ignored CVE matches to IgnoredMatches list with applied rule tracking; added hasIgnoreAction helper; new test coverage for exception application scenarios.
Scan Service CVE Filtering
core/services/scan.go, core/services/scan_test.go
Added applyExceptionsToManifest helper to filter manifests via security exceptions; updated ScanCP and ScanCVE to store filtered CVE manifests and summaries instead of unmodified versions; conditional storage logic now based on increased ignored matches count.
HTTP Server Initialization
cmd/http/main.go
Moved securityExceptionRepo construction to common code path before platform selection; updated local mode mock platform instantiation to accept repository; ensures consistent seRepo availability across all deployment modes.
API Server Storage Configuration
repositories/apiserver.go
Enhanced CRD dynamic client with increased QPS (50) and Burst (100) parameters; added dedicated listing context with 30s timeout for SecurityExceptions and ClusterSecurityExceptions queries.

Sequence Diagram

sequenceDiagram
    participant ScanService
    participant MockPlatform
    participant SecurityExceptionRepo
    participant Adapter as v1.Adapter
    participant CVEManifest as GrypeDocument

    ScanService->>MockPlatform: GetCVEExceptions(ctx)
    MockPlatform->>SecurityExceptionRepo: GetSecurityExceptions(namespace)
    SecurityExceptionRepo-->>MockPlatform: seList, cseList
    MockPlatform->>Adapter: ConvertToVulnerabilityExceptionPolicies()
    Adapter-->>MockPlatform: vulnExceptionList
    MockPlatform-->>ScanService: domain.CVEExceptions

    ScanService->>ScanService: applyExceptionsToManifest(cve)
    ScanService->>Adapter: ApplySecurityExceptions(doc, exceptions)
    Adapter->>CVEManifest: Iterate Matches
    Adapter->>CVEManifest: Move match to IgnoredMatches
    Adapter->>CVEManifest: Set AppliedIgnoreRules
    Adapter-->>ScanService: filtered CVEManifest
    ScanService->>ScanService: Store filtered CVE & summary
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • kubescape/kubevuln#342: Adds SecurityException CRD integration including repository interface, conversion logic, and policy merging into GetCVEExceptions flow.

Suggested reviewers

  • matthyx

Poem

🐰 ✨ Exceptions flow through CRDs with grace,
Filtering CVEs at their resting place,
Matches ignored with applied-rule care,
Security exceptions everywhere!
thump thump 🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately describes the main fix: enabling SecurityException CRDs in keepLocal mode, which is the core bug being addressed across the changeset.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/security-exception-keeplocal

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

Summary:

  • License scan: failure
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: failure

slashben and others added 5 commits April 16, 2026 23:59
Moves excepted CVEs from GrypeDocument.Matches to IgnoredMatches
with AppliedIgnoreRules for audit trail. Respects expiredOnFix.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
Calls platform.GetCVEExceptions and filters GrypeDocument.Matches
before StoreCVE/StoreCVESummary. Excepted CVEs move to IgnoredMatches.
Applies to all three scan paths: full CVE, filtered CVE, and ScanCVE.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
The ScanCVE and ScanCP methods skip the CVE scan when a cached manifest
exists, which also skipped exception filtering. Now exceptions are
applied to cached manifests too, and the filtered result is re-stored.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
The dynamic client for SecurityException CRDs shares the default
rate limiter (QPS=5) with the typed client, causing context canceled
errors during concurrent scans. Give it a dedicated config with
higher QPS/Burst limits.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
The scan context may be canceled by rate limiting before the dynamic
client completes the CRD listing. Use a detached context with a 30s
timeout for SecurityException listing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
@slashben
Copy link
Copy Markdown
Contributor Author

Manual Test Results (kind cluster)

Tested all 10 user stories from the test plan:

# Test Result
1 Basic namespaced exception ✅ Pass
2 Cluster-wide exception ✅ Pass
3 Expired exception ignored ✅ Pass
4 Future expiry respected ✅ Pass
5 Resource-scoped exception ⚠️ Inconclusive (manifests are per-image; resource matching at report level)
6 ExpiredOnFix behavior ✅ Pass
7 Baseline (no exceptions) ✅ Pass
8 Multiple CVEs in one exception ✅ Pass
9 CRD validation (invalid enums) ✅ Pass
10 kubectl UX (short names, columns) ✅ Pass

Evidence: nginx:1.21 scan: 616 matches → 607 matches + 9 in ignoredMatches with appliedIgnoreRules audit trail.

Bugs found and fixed during testing:

  1. MockPlatform didn't read CRDs in keepLocal mode — fixed by adding SecurityExceptionRepository to MockPlatform
  2. Cached CVE manifests skipped exception filtering — fixed by applying exceptions in the cache-hit path too
  3. Dynamic client rate limiter caused context cancellation — fixed with detached context + higher QPS

@github-actions
Copy link
Copy Markdown

Summary:

  • License scan: failure
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: failure

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to ensure SecurityException CRDs are respected when kubevuln runs in keepLocal mode by wiring the SecurityException repository into the local (MockPlatform) path and applying the resulting exception policies during scanning.

Changes:

  • Wire SecurityExceptionRepository into keepLocal mode (MockPlatform.GetCVEExceptions) and initialize it for both keepLocal/cloud in cmd/http/main.go.
  • Apply SecurityException policies to Grype CVE manifests during ScanCP and ScanCVE, and export ConvertToVulnerabilityExceptionPolicies for cross-package use.
  • Adjust APIServer CRD listing behavior (dynamic client config + detached timeout context) and update/add unit tests.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
repositories/apiserver.go Tunes dynamic client config and changes CRD listing to use a detached timeout context.
core/services/scan.go Applies exceptions to manifests before persisting and adds helper applyExceptionsToManifest.
core/services/scan_test.go Updates mock platform construction to match new signature.
cmd/http/main.go Initializes SecurityException repo for both keepLocal and cloud paths; passes repo to platform.
cmd/http/main_test.go Updates mock platform construction to match new signature.
adapters/v1/securityexception.go Exports conversion helper and adds ApplySecurityExceptions that mutates Grype docs.
adapters/v1/securityexception_test.go Updates conversion tests and adds tests for ApplySecurityExceptions.
adapters/v1/backend.go Updates callsite to exported conversion helper.
adapters/mockplatform.go Adds SecurityException repo to keepLocal platform path and reads CRDs in GetCVEExceptions.
adapters/mockplatform_test.go Updates mock platform construction to match new signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread adapters/v1/securityexception.go Outdated
Comment on lines +84 to +88
var remaining []v1beta1.Match
for _, m := range doc.Matches {
isFixed := m.Vulnerability.Fix.State == "fixed"
matched := getCVEExceptionMatchCVENameFromList(exceptions, m.Vulnerability.ID, isFixed)
if len(matched) > 0 {
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

ApplySecurityExceptions decides to ignore a match solely based on CVE ID (getCVEExceptionMatchCVENameFromList) and whether the vuln is fixed. It does not evaluate the exception scope (e.g., Designatores produced from spec.match.resources, namespace-only vs specific workload, etc.). Since CRD exceptions are listed at namespace/cluster scope and then merged, this can incorrectly ignore CVEs for unrelated workloads in the same namespace. Recommend filtering the exception list to the current workload (namespace/kind/name/container/image) before applying, or updating the matching logic to require a designator/resource match in addition to CVE ID.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Acknowledged — manifests are per-image not per-workload, so designator/resource matching can't be applied at this level. This is a known Phase 1 limitation documented in the test plan (US5). Phase 2 adds workload-scoped handling.

Comment thread core/services/scan.go Outdated
Comment on lines +602 to +614
// applyExceptionsToManifest filters the CVE manifest using SecurityException policies.
func (s *ScanService) applyExceptionsToManifest(ctx context.Context, cve *domain.CVEManifest) {
if cve == nil || cve.Content == nil {
return
}
exceptions, err := s.platform.GetCVEExceptions(ctx)
if err != nil {
logger.L().Ctx(ctx).Warning("failed to get CVE exceptions for filtering", helpers.Error(err))
return
}
if len(exceptions) > 0 {
v1.ApplySecurityExceptions(cve.Content, exceptions)
}
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

PR description notes that in keepLocal mode exceptions are read but do not affect the stored VulnerabilityManifest (raw Grype output). With the new applyExceptionsToManifest calls before StoreCVE/StoreCVESummary, exceptions will now alter what gets persisted (when c.Storage is true), changing that behavior. Either update the PR description/docs to match the new behavior, or adjust the implementation to preserve raw manifests and apply exceptions only for reporting.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — the description was from the initial commit before we added manifest filtering. Will update.

Comment thread core/services/scan.go Outdated
Comment on lines +339 to +341
// apply security exceptions before storing
s.applyExceptionsToManifest(ctx, &cvep)

Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

ScanCP calls platform.GetCVEExceptions multiple times per container scan (once for cve, once for cvep, and then again inside BackendAdapter.SubmitCVE). In cloud mode this can add extra backend round-trips per scan. Consider caching exceptions for the scan context (e.g., store them in ctx/value once) or restructuring so exceptions are fetched once and reused for filtering + submission.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid optimization. For keepLocal mode it's just a dynamic client list (fast). For cloud mode, SubmitCVE has its own GetCVEExceptions call which we can't easily deduplicate without a larger refactor. Will consider caching in a follow-up.

Comment thread repositories/apiserver.go Outdated
Comment on lines +109 to +111
// Use a detached context with timeout — the scan context may be canceled
// before the CRD listing completes due to rate limiting.
listCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

GetSecurityExceptions creates listCtx from context.Background(), which drops request-scoped values (trace/span, logger fields, auth info carried via ctx values). Since the goal is only to avoid cancellation, prefer deriving from context.WithoutCancel(ctx) and then applying the timeout, so listing is still correlated with the original request/trace while remaining resilient to cancellation.

Suggested change
// Use a detached context with timeout — the scan context may be canceled
// before the CRD listing completes due to rate limiting.
listCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
// Preserve request-scoped context values while avoiding cancellation from
// the scan context, then bound the listing with a timeout.
listCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 01b049f — using context.WithoutCancel(ctx) now. Good suggestion.

Comment thread core/services/scan.go Outdated
Comment on lines +270 to +272
// apply security exceptions before storing
s.applyExceptionsToManifest(ctx, &cve)

Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

applyExceptionsToManifest mutates cve.Content in-place before both storage and platform.SubmitCVE. In cloud mode, BackendAdapter.SubmitCVE converts only GrypeDocument.Matches (it ignores IgnoredMatches), so moving findings out of Matches will cause ignored CVEs to be omitted from the uploaded report rather than being reported with exception metadata. Consider either (a) applying exceptions on a deep copy used only for local storage, or (b) extending the reporting conversion to also handle IgnoredMatches (or avoid mutating the document before submission).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 01b049fapplyExceptionsToManifest now returns a deep copy. Original is preserved for SubmitCVE, filtered copy used only for storage.

Comment thread core/services/scan.go Outdated
Comment on lines +289 to +301
// apply security exceptions to cached manifest
s.applyExceptionsToManifest(ctx, &cve)

if s.storage {
// store summary CVE if it does not exist
if cveSumm, err := s.cveRepository.GetCVESummary(ctx); err != nil || cveSumm == nil {
err = s.cveRepository.StoreCVESummary(ctx, cve, domain.CVEManifest{}, false)
if err != nil {
logger.L().Ctx(ctx).Warning("storing CVE summary", helpers.Error(err),
helpers.String("imageSlug", slug))
// no continue, storing the CVE summary is not critical
}
// re-store with exceptions applied
err = s.cveRepository.StoreCVE(ctx, cve, false)
if err != nil {
logger.L().Ctx(ctx).Warning("storing CVE with exceptions", helpers.Error(err),
helpers.String("imageSlug", slug))
// no continue, storing the CVE is not critical
}
err = s.cveRepository.StoreCVESummary(ctx, cve, domain.CVEManifest{}, false)
if err != nil {
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

In the cached-manifest branch, the code applies exceptions and then overwrites the stored CVE/summary with the filtered document. This makes exception changes non-reversible without a full rescan (e.g., removing an exception won't bring the CVE back because it was persisted out of Matches). To keep behavior correct when CRDs change, consider keeping the stored manifest raw and applying exceptions only at report/render time, or storing filtered output separately from the raw cache.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 01b049f — cache-hit branches now only re-store when exceptions actually changed the manifest (len(filtered.IgnoredMatches) > len(original.IgnoredMatches)). Also, Phase 2 operator watcher will trigger rescans when CRDs change.

Copy link
Copy Markdown
Contributor

@matthyx matthyx left a comment

Choose a reason for hiding this comment

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

A few concerns around correctness and cache behavior — see inline comments. Great catch on the MockPlatform wiring gap; the core fix is right, but I think the manifest-mutation approach and the cache-hit rewrite need another look before merge.

Comment thread adapters/v1/securityexception.go
Comment thread core/services/scan.go Outdated
if err != nil {
logger.L().Ctx(ctx).Warning("storing CVE summary with exceptions", helpers.Error(err),
helpers.String("imageSlug", slug))
// no continue, storing the CVE summary is not critical
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.

This cache-hit branch used to be:

if cveSumm, err := s.cveRepository.GetCVESummary(ctx); err != nil || cveSumm == nil {
    StoreCVESummary(ctx, cve, domain.CVEManifest{}, false)
}

i.e. write the summary only if missing. The new code unconditionally writes both StoreCVE and StoreCVESummary on every cache hit. Two issues:

  • Doubled API-server writes on every cached scan. For busy clusters this is a meaningful increase in load on the storage backend.
  • Overwrites relevancy data with empty cvep. StoreCVESummary(ctx, cve, domain.CVEManifest{}, false) passes an empty CVE' manifest. If a prior ScanCP run stored a summary that included a real relevant-CVE manifest, this cache-hit path now clobbers it with an empty one. The old if cveSumm == nil guard prevented that.

Consider either (a) keeping the guard and only re-storing when the stored manifest predates exception application, or (b) loading the existing summary's cvep and passing it through instead of domain.CVEManifest{}. Same issue applies in ScanCVE below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 01b049f — restored the guard. Cache-hit branch only re-stores when len(filteredCve.Content.IgnoredMatches) > len(cve.Content.IgnoredMatches), preserving the original skip-if-summary-exists behavior and avoiding clobbering relevancy data.

Comment thread core/services/scan.go
…eck Ignore action

- applyExceptionsToManifest returns a filtered copy, preserving the
  original for SubmitCVE (cloud reports keep ExceptionApplied metadata)
- Cache-hit branches only re-store when exceptions actually changed
  the manifest (avoids unnecessary writes, preserves relevancy data)
- ApplySecurityExceptions now checks for Ignore action before filtering
- Use context.WithoutCancel instead of context.Background for CRD listing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
@slashben
Copy link
Copy Markdown
Contributor Author

A few concerns around correctness and cache behavior — see inline comments. Great catch on the MockPlatform wiring gap; the core fix is right, but I think the manifest-mutation approach and the cache-hit rewrite need another look before merge.

Good points! Addressed all of them

@github-actions
Copy link
Copy Markdown

Summary:

  • License scan: failure
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: failure

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
core/services/scan.go (1)

336-364: ⚠️ Potential issue | 🟠 Major

cvep.Wlid is no longer set — StoreVEX and SubmitCVE receive a Wlid-less manifest.

In the previous version, cvep.Wlid was assigned before storage and filtering. After this refactor, filteredCvep.Wlid = scan.Wlid is set on the filtered copy only; the original cvep (used at line 354 for StoreVEX and line 364 for platform.SubmitCVE) keeps whatever Wlid value it had before. Since applyExceptionsToManifest performs a shallow copy of the CVE manifest struct, writing to filteredCvep.Wlid does not affect cvep.Wlid. This causes cloud reporting and VEX storage to receive manifests without the assigned Wlid.

🛠 Suggested fix — set Wlid on the original before filtering
-			// apply security exceptions for storage (copy — original stays intact for SubmitCVE)
-			filteredCvep := s.applyExceptionsToManifest(ctx, cvep)
-
-			// store filtered CVE'
-			if s.storage {
-				filteredCvep.Wlid = scan.Wlid
-				err = s.cveRepository.StoreCVE(ctx, filteredCvep, true)
+			// set Wlid on the original so it flows to SubmitCVE/StoreVEX as well
+			cvep.Wlid = scan.Wlid
+
+			// apply security exceptions for storage (copy — original stays intact for SubmitCVE)
+			filteredCvep := s.applyExceptionsToManifest(ctx, cvep)
+
+			// store filtered CVE'
+			if s.storage {
+				err = s.cveRepository.StoreCVE(ctx, filteredCvep, true)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@core/services/scan.go` around lines 336 - 364, The problem is that
applyExceptionsToManifest returns a shallow copy so setting filteredCvep.Wlid =
scan.Wlid only updates the copy and leaves cvep.Wlid unset, causing StoreVEX
(cveRepository.StoreVEX) and platform.SubmitCVE to receive manifests without
Wlid; fix by assigning the Wlid to the original manifest before filtering (set
cvep.Wlid = scan.Wlid prior to calling applyExceptionsToManifest or immediately
after obtaining scan.Wlid and before calls to StoreVEX/SubmitCVE) so both cvep
and filteredCvep carry the Wlid when you call cveRepository.StoreVEX and
s.platform.SubmitCVE.
♻️ Duplicate comments (1)
adapters/v1/securityexception.go (1)

89-112: ⚠️ Potential issue | 🟠 Major

ExceptionApplied metadata still lost; cloud-mode Summarize stats may regress.

hasIgnoreAction resolves the "Actions not checked" concern, but the second concern from the prior review is still open: by mutating doc.Matches up-front and recording only AppliedIgnoreRules: [{Vulnerability: ID}], the link to the originating policy (GUID/name/Actions) is dropped before BackendAdapter.SubmitCVE runs Summarize. Summarize in adapters/v1/backend_utils.go relies on ExceptionApplied[0].Actions[0] == armotypes.Ignore to populate ExcludedSeveritiesStats, so cloud-mode excluded-severity counts will likely be understated once scan.applyExceptionsToManifest removes these matches before submission.

Worth confirming excluded-severity stats in cloud mode still match pre-PR behavior, or gating the manifest mutation to the storage/keepLocal path and leaving Summarize-driven filtering intact for cloud submission.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@adapters/v1/securityexception.go` around lines 89 - 112,
ApplySecurityExceptions currently drops the original exception metadata by
appending only {Vulnerability: ID} to AppliedIgnoreRules and removing matches,
which breaks Summarize's reliance on ExceptionApplied.Actions; fix this by
preserving the full exception rule info returned by
getCVEExceptionMatchCVENameFromList (including GUID/name and
ExceptionApplied/Actions) when building v1beta1.IgnoredMatch.AppliedIgnoreRules
instead of emitting a minimal Vulnerability-only rule, and/or avoid mutating
doc.Matches for cloud submissions so BackendAdapter.SubmitCVE -> Summarize still
sees ExceptionApplied; update ApplySecurityExceptions (and its use of
hasIgnoreAction/getCVEExceptionMatchCVENameFromList) to copy the full rule
metadata required by Summarize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@adapters/v1/securityexception_test.go`:
- Around line 215-238: The test TestApplySecurityExceptions_ExpiredOnFix
currently passes for the wrong reason because the exception lacks an Ignore
action; update the test's CVEExceptions item to include Actions:
[]string{"Ignore"} so hasIgnoreAction returns true and the ExpiredOnFix path is
actually exercised in ApplySecurityExceptions (reference CVEExceptions,
hasIgnoreAction, ApplySecurityExceptions); additionally add a negative-control
subcase where ExpiredOnFix is false or nil (same exception with Actions:
["Ignore"]) and assert that when a matching fix exists the match is moved into
doc.IgnoredMatches, while when ExpiredOnFix=true it remains in doc.Matches.

In `@core/services/scan.go`:
- Line 347: The call to s.cveRepository.StoreCVESummary on the CVEP path passes
the unfiltered variable `cve` which causes ignored matches not to be reflected
in the stored summary; change the first argument to `filteredCve` (so call
StoreCVESummary(ctx, filteredCve, filteredCvep, true)) to mirror the new-CVE
path behavior and ensure summaries reflect filtering (update the invocation
where `cve` is currently passed and optionally add a brief comment explaining
the consistency).

---

Outside diff comments:
In `@core/services/scan.go`:
- Around line 336-364: The problem is that applyExceptionsToManifest returns a
shallow copy so setting filteredCvep.Wlid = scan.Wlid only updates the copy and
leaves cvep.Wlid unset, causing StoreVEX (cveRepository.StoreVEX) and
platform.SubmitCVE to receive manifests without Wlid; fix by assigning the Wlid
to the original manifest before filtering (set cvep.Wlid = scan.Wlid prior to
calling applyExceptionsToManifest or immediately after obtaining scan.Wlid and
before calls to StoreVEX/SubmitCVE) so both cvep and filteredCvep carry the Wlid
when you call cveRepository.StoreVEX and s.platform.SubmitCVE.

---

Duplicate comments:
In `@adapters/v1/securityexception.go`:
- Around line 89-112: ApplySecurityExceptions currently drops the original
exception metadata by appending only {Vulnerability: ID} to AppliedIgnoreRules
and removing matches, which breaks Summarize's reliance on
ExceptionApplied.Actions; fix this by preserving the full exception rule info
returned by getCVEExceptionMatchCVENameFromList (including GUID/name and
ExceptionApplied/Actions) when building v1beta1.IgnoredMatch.AppliedIgnoreRules
instead of emitting a minimal Vulnerability-only rule, and/or avoid mutating
doc.Matches for cloud submissions so BackendAdapter.SubmitCVE -> Summarize still
sees ExceptionApplied; update ApplySecurityExceptions (and its use of
hasIgnoreAction/getCVEExceptionMatchCVENameFromList) to copy the full rule
metadata required by Summarize.
🪄 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

Run ID: 9d9d5c6a-1d93-4e59-b612-a1fe6e7d741e

📥 Commits

Reviewing files that changed from the base of the PR and between da24019 and 01b049f.

📒 Files selected for processing (10)
  • adapters/mockplatform.go
  • adapters/mockplatform_test.go
  • adapters/v1/backend.go
  • adapters/v1/securityexception.go
  • adapters/v1/securityexception_test.go
  • cmd/http/main.go
  • cmd/http/main_test.go
  • core/services/scan.go
  • core/services/scan_test.go
  • repositories/apiserver.go

Comment thread adapters/v1/securityexception_test.go
Comment thread core/services/scan.go
…ESummary

- TestApplySecurityExceptions_ExpiredOnFix now sets Actions: [Ignore]
  so it actually tests expiredOnFix behavior, not the action check
- Add comment explaining why StoreCVESummary uses original cve + filteredCvep

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Ben <ben@armosec.io>
@github-actions
Copy link
Copy Markdown

Summary:

  • License scan: failure
  • Credentials scan: failure
  • Vulnerabilities scan: failure
  • Unit test: success
  • Go linting: failure

@matthyx matthyx merged commit d6270db into main Apr 17, 2026
13 of 14 checks passed
@matthyx matthyx deleted the fix/security-exception-keeplocal branch April 17, 2026 15:37
@matthyx matthyx moved this to To Archive in KS PRs tracking Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants