Merge pull request #503 from DependencyTrack/vuln-policy-fixes-2

Fix various minor bugs in vulnerability policy evaluation
DependencyTrack · Dec 21, 2023 · 327b4a5 · 327b4a5
2 parents ea90698 + d718026
commit 327b4a5
Show file tree

Hide file tree

Showing 8 changed files with 168 additions and 70 deletions.
diff --git a/...main/java/org/dependencytrack/event/kafka/processor/VulnerabilityScanResultProcessor.java b/...main/java/org/dependencytrack/event/kafka/processor/VulnerabilityScanResultProcessor.java
@@ -201,12 +201,26 @@ private Set<Vulnerability> syncVulnerabilities(final QueryManager qm, final Scan
             }
 
             try {
-                syncedVulns.add(syncVulnerability(qm, vuln, scannerResult.getScanner()));
+                final Vulnerability syncedVuln = syncVulnerability(qm, vuln, scannerResult.getScanner());
+
+                // Detach vulnerabilities from JDO persistence context.
+                // We do not want to trigger any DB interactions by accessing their fields later.
+                // Note that even PersistenceManager#detachCopy will load / unload fields based
+                // on the current FetchPlan. But we just want to keep the data we already have,
+                // and #makeTransientAll does exactly that.
+                qm.getPersistenceManager().makeTransient(syncedVuln);
+
                 if (vuln.getAliases() != null && !vuln.getAliases().isEmpty()) {
+                    final var syncedAliases = new ArrayList<VulnerabilityAlias>();
                     for (VulnerabilityAlias alias : vuln.getAliases()) {
-                        qm.synchronizeVulnerabilityAlias(alias);
+                        final VulnerabilityAlias syncedAlias = qm.synchronizeVulnerabilityAlias(alias);
+                        qm.getPersistenceManager().makeTransient(syncedAlias);
+                        syncedAliases.add(syncedAlias);
                     }
+                    syncedVuln.setAliases(syncedAliases);
                 }
+
+                syncedVulns.add(syncedVuln);
             } catch (RuntimeException e) {
                 // Use a broad catch here, so we can still try to process other
                 // vulnerabilities, even though processing one of them failed.
@@ -216,13 +230,6 @@ private Set<Vulnerability> syncVulnerabilities(final QueryManager qm, final Scan
             }
         }
 
-        // Detach vulnerabilities from JDO persistence context.
-        // We do not want to trigger any DB interactions by accessing their fields.
-        // Note that even PersistenceManager#detachCopy will load / unload fields based
-        // on the current FetchPlan. But we just want to keep the data we already have,
-        // and #makeTransientAll does exactly that.
-        qm.getPersistenceManager().makeTransientAll(syncedVulns);
-
         return syncedVulns;
     }
 

diff --git a/src/main/java/org/dependencytrack/policy/cel/CelPolicyScriptHost.java b/src/main/java/org/dependencytrack/policy/cel/CelPolicyScriptHost.java
@@ -13,7 +13,6 @@
 import org.projectnessie.cel.CEL;
 import org.projectnessie.cel.Env;
 import org.projectnessie.cel.Env.AstIssuesTuple;
-import org.projectnessie.cel.EnvOption;
 import org.projectnessie.cel.Program;
 import org.projectnessie.cel.common.CELError;
 import org.projectnessie.cel.common.Errors;
@@ -52,17 +51,17 @@ public enum CacheMode {
     private final AbstractCacheManager cacheManager;
     private final Env environment;
 
-    CelPolicyScriptHost(final AbstractCacheManager cacheManager, final List<EnvOption> envOptions) {
+    public CelPolicyScriptHost(final AbstractCacheManager cacheManager, final CelPolicyType policyType) {
         this.locks = Striped.lock(128);
         this.cacheManager = cacheManager;
         this.environment = Env.newCustomEnv(
                 ProtoTypeRegistry.newRegistry(),
-                envOptions
+                policyType.envOptions()
         );
     }
 
     public static synchronized CelPolicyScriptHost getInstance(final CelPolicyType policyType) {
-        return INSTANCES.computeIfAbsent(policyType, ignored -> new CelPolicyScriptHost(CacheManager.getInstance(), policyType.envOptions()));
+        return INSTANCES.computeIfAbsent(policyType, ignored -> new CelPolicyScriptHost(CacheManager.getInstance(), policyType));
     }
 
     /**

diff --git a/src/main/java/org/dependencytrack/policy/cel/CelVulnerabilityPolicyEvaluator.java b/src/main/java/org/dependencytrack/policy/cel/CelVulnerabilityPolicyEvaluator.java
@@ -55,13 +55,14 @@ public class CelVulnerabilityPolicyEvaluator implements VulnerabilityPolicyEvalu
     private final CelPolicyScriptHost scriptHost;
     private final AbstractCacheManager cacheManager;
 
+    @SuppressWarnings("unused") // Called by ServiceLoader
     public CelVulnerabilityPolicyEvaluator() {
         this(ServiceLoader.load(VulnerabilityPolicyProvider.class).findFirst().orElseThrow(),
                 CelPolicyScriptHost.getInstance(CelPolicyType.VULNERABILITY), CacheManager.getInstance());
     }
 
-    CelVulnerabilityPolicyEvaluator(final VulnerabilityPolicyProvider policyProvider,
-                                    final CelPolicyScriptHost scriptHost, final AbstractCacheManager cacheManager) {
+    public CelVulnerabilityPolicyEvaluator(final VulnerabilityPolicyProvider policyProvider,
+                                           final CelPolicyScriptHost scriptHost, final AbstractCacheManager cacheManager) {
         this.policyProvider = policyProvider;
         this.scriptHost = scriptHost;
         this.cacheManager = cacheManager;

diff --git a/src/main/java/org/dependencytrack/policy/cel/persistence/CelPolicyDao.java b/src/main/java/org/dependencytrack/policy/cel/persistence/CelPolicyDao.java
@@ -256,7 +256,7 @@ private static Set<String> determineFieldsToLoad(final Descriptor typeDescriptor
             if (fieldDescriptor.isRepeated() && typeInstance.getRepeatedFieldCount(fieldDescriptor) == 0) {
                 // There's no way differentiate between repeated fields being not set or just empty.
                 fieldsToLoad.add(fieldName);
-            } else if (!typeInstance.hasField(fieldDescriptor)) {
+            } else if (!fieldDescriptor.isRepeated() && !typeInstance.hasField(fieldDescriptor)) {
                 fieldsToLoad.add(fieldName);
             }
         }

diff --git a/src/test/java/org/dependencytrack/TestCacheManager.java b/src/test/java/org/dependencytrack/TestCacheManager.java
@@ -0,0 +1,13 @@
+package org.dependencytrack;
+
+import alpine.server.cache.AbstractCacheManager;
+
+import java.util.concurrent.TimeUnit;
+
+public class TestCacheManager extends AbstractCacheManager {
+
+    public TestCacheManager(final long expiresAfter, final TimeUnit timeUnit, final long maxSize) {
+        super(expiresAfter, timeUnit, maxSize);
+    }
+
+}