Skip to content

Commit 517c958

Browse files
committed
fix(sca): use resolved dep.name for registerCve to prevent registry key mismatch for no-pom JARs
For JARs without pom.properties, DependencyResolver.guessFallbackNoPom produces an artifactId-only name (e.g. "junrar" instead of "com.github.junrar:junrar"). The transformer was calling registerCve with entry.artifact() — the full groupId:artifactId from the DB — creating a registry key that mismatched the key DependencyService would later report. Result: the periodic action emitted two separate telemetry entries for the same physical JAR: - "junrar" with metadata:[] (source/hash present, CVE absent — Step 2 lookup missed) - "com.github.junrar:junrar" with CVE data but no source/hash (Step 3, knownDeps miss) Fix: resolveArtifactDep() returns the matched Dependency object (not just the version string), and processClass uses dep.name for registerCve and MethodCallbackSpec. This keeps the registry key consistent with what DependencyService will report regardless of pom.properties presence. Implementation: - Add matchDep() returning Dependency (matchVersion delegates to it) - Change classpathArtifactCache to cache Dependency (preserves resolved name) - Add resolveArtifactDep() returning Dependency; resolveVersionForArtifact delegates to it - Add findArtifactInClasspath() returning Dependency; findArtifactVersionInClasspath delegates to it The smoke test's synthetic pom.properties masked this bug by making both routes agree on the full groupId:artifactId name. No smoke test changes needed — the unit tests cover the fix: - ScaReachabilityTransformerJava9Test: resolveArtifactDep returns artifactId-only name for no-pom - ScaReachabilityPeriodicActionTest: CVE and source/hash merged into single entry for no-pom dep
1 parent f3ab39c commit 517c958

3 files changed

Lines changed: 157 additions & 45 deletions

File tree

dd-java-agent/appsec/src/main/java/com/datadog/appsec/sca/ScaReachabilityTransformer.java

Lines changed: 76 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -66,13 +66,15 @@ public final class ScaReachabilityTransformer implements ClassFileTransformer {
6666
final ConcurrentHashMap<URI, List<Dependency>> jarCache = new ConcurrentHashMap<>();
6767

6868
/**
69-
* Cache: artifact name → classpath-resolved version. Used when the class's own JAR does not
70-
* contain the vulnerable artifact (e.g., Spring Boot starters whose watched classes live in
71-
* transitive dependency JARs). Only non-null results are cached; null means "not yet found" and
72-
* will be retried on the next periodic retransform.
69+
* Cache: artifact name → classpath-resolved {@link Dependency}. Stores the full dependency (name
70+
* + version) so that the resolved {@code dep.name} — which may be an artifactId-only name like
71+
* {@code "junrar"} for JARs without pom.properties — is propagated to {@code registerCve} and
72+
* kept consistent with the name that {@link datadog.telemetry.dependency.DependencyService} will
73+
* report. Only non-null results are cached; null means "not yet found" and will be retried on the
74+
* next periodic retransform.
7375
*/
7476
@VisibleForTesting
75-
final ConcurrentHashMap<String, String> classpathArtifactCache = new ConcurrentHashMap<>();
77+
final ConcurrentHashMap<String, Dependency> classpathArtifactCache = new ConcurrentHashMap<>();
7678

7779
/**
7880
* Classes whose bytecode needs (re)transformation for method-level symbol injection:
@@ -170,15 +172,25 @@ private byte[] processClass(
170172
String dotClassName = null;
171173

172174
for (ScaEntry entry : entries) {
173-
// Resolve version: first check the class's own JAR, then fall back to a full classpath
174-
// scan. The fallback handles cases where the vulnerable artifact is an aggregator/starter
175-
// POM whose watched classes actually live in a transitive dependency JAR (e.g.,
176-
// spring-boot-starter-web watches @Controller, but @Controller is in spring-context.jar).
177-
String version = resolveVersionForArtifact(entry.artifact(), classJarDeps);
178-
if (version == null) {
175+
// Resolve the dependency for this artifact: first check the class's own JAR, then fall back
176+
// to a full classpath scan. The fallback handles aggregator/starter POM artifacts whose
177+
// watched classes actually live in a transitive dependency JAR (e.g., spring-boot-starter-web
178+
// watches @Controller, but @Controller is in spring-context.jar).
179+
//
180+
// We use the resolved dep's name (not entry.artifact()) for registerCve and
181+
// MethodCallbackSpec
182+
// to ensure that the registry key matches the name that DependencyService will later report.
183+
// For JARs without pom.properties, DependencyResolver.guessFallbackNoPom produces an
184+
// artifactId-only name (e.g. "junrar" instead of "com.github.junrar:junrar"). Using
185+
// entry.artifact() as the registry key would cause a mismatch with DependencyService's name,
186+
// causing the CVE telemetry event to lose its source/hash or appear under the wrong name.
187+
Dependency resolvedDep = resolveArtifactDep(entry.artifact(), classJarDeps);
188+
if (resolvedDep == null) {
179189
hasUnresolvedMethodLevelSymbols = true;
180190
continue;
181191
}
192+
String version = resolvedDep.version;
193+
String depName = resolvedDep.name != null ? resolvedDep.name : entry.artifact();
182194

183195
if (!entry.isVersionVulnerable(version)) {
184196
continue;
@@ -191,16 +203,15 @@ private byte[] processClass(
191203
// Register the CVE now (at class load time) with reached=[] so the next heartbeat
192204
// signals the backend that SCA is monitoring this CVE. The callsite will be added
193205
// later when the method is actually called (via ScaReachabilityCallback).
194-
ScaReachabilityDependencyRegistry.INSTANCE.registerCve(
195-
entry.artifact(), version, entry.vulnId());
206+
ScaReachabilityDependencyRegistry.INSTANCE.registerCve(depName, version, entry.vulnId());
196207
if (dotClassName == null) {
197208
dotClassName = Strings.getClassName(className);
198209
}
199210
methodCallbacks
200211
.computeIfAbsent(symbol.method(), k -> new ArrayList<>())
201212
.add(
202213
new MethodCallbackSpec(
203-
entry.vulnId(), entry.artifact(), version, dotClassName, symbol.method()));
214+
entry.vulnId(), depName, version, dotClassName, symbol.method()));
204215
}
205216
}
206217

@@ -314,8 +325,8 @@ public void performPendingRetransforms() {
314325
// callback.
315326
// Two paths in processClass() can trigger fresh JAR I/O under locks:
316327
// 1. resolveDependenciesFromCache() — safe only if jarCache is already populated.
317-
// 2. resolveVersionForArtifact() → findArtifactVersionInClasspath() for aggregator artifacts
318-
// (e.g., spring-boot-starter-web) whose pom.properties is not in the class's own JAR.
328+
// 2. resolveArtifactDep() → findArtifactInClasspath() for aggregator artifacts (e.g.,
329+
// spring-boot-starter-web) whose pom.properties is not in the class's own JAR.
319330
// Without pre-warming classpathArtifactCache, this scans all java.class.path JARs via
320331
// resolveDependencies() under JVM locks, reintroducing the snakeyaml deadlock.
321332
for (Class<?> c : toRetransform) {
@@ -359,50 +370,54 @@ public void performPendingRetransforms() {
359370
// ---------------------------------------------------------------------------
360371

361372
/**
362-
* Resolves the version of {@code artifactName} using a two-step strategy:
373+
* Resolves the {@link Dependency} for {@code artifactName} (groupId:artifactId format), returning
374+
* the matched dependency object — which may have an artifactId-only {@code name} for JARs without
375+
* {@code pom.properties}. Returns {@code null} if the artifact cannot be found.
363376
*
364-
* <ol>
365-
* <li>Check the dependencies resolved from the class's own JAR ({@code classJarDeps}). This
366-
* covers the common case where the class and its artifact live in the same JAR.
367-
* <li>If not found, fall back to a full classpath scan via {@link
368-
* #findArtifactVersionInClasspath}. This handles aggregator/starter POM artifacts (e.g.,
369-
* {@code spring-boot-starter-web}) whose watched classes live in transitive dependency JARs
370-
* rather than in the starter JAR itself. Results of successful scans are cached.
371-
* </ol>
372-
*
373-
* @return the resolved version string, or {@code null} if the artifact cannot be found
377+
* <p>Use this method (not {@link #resolveVersionForArtifact}) whenever the resolved dependency
378+
* name needs to be propagated (e.g., for {@code registerCve} and {@code MethodCallbackSpec}), so
379+
* that registry keys stay consistent with the names that {@link
380+
* datadog.telemetry.dependency.DependencyService} will report.
374381
*/
375382
@VisibleForTesting
376-
String resolveVersionForArtifact(String artifactName, List<Dependency> classJarDeps) {
377-
String version = matchVersion(artifactName, classJarDeps);
378-
if (version != null) {
379-
return version;
383+
Dependency resolveArtifactDep(String artifactName, List<Dependency> classJarDeps) {
384+
Dependency dep = matchDep(artifactName, classJarDeps);
385+
if (dep != null) {
386+
return dep;
380387
}
381388
// Classpath fallback: check cache first, then scan.
382-
String cached = classpathArtifactCache.get(artifactName);
389+
Dependency cached = classpathArtifactCache.get(artifactName);
383390
if (cached != null) {
384391
return cached;
385392
}
386-
version = findArtifactVersionInClasspath(artifactName);
387-
if (version != null) {
388-
classpathArtifactCache.put(artifactName, version); // only cache hits; misses are retried
393+
dep = findArtifactInClasspath(artifactName);
394+
if (dep != null) {
395+
classpathArtifactCache.put(artifactName, dep); // only cache hits; misses are retried
389396
}
390-
return version;
397+
return dep;
398+
}
399+
400+
@VisibleForTesting
401+
String resolveVersionForArtifact(String artifactName, List<Dependency> classJarDeps) {
402+
Dependency dep = resolveArtifactDep(artifactName, classJarDeps);
403+
return dep != null ? dep.version : null;
391404
}
392405

393406
/**
394-
* Matches {@code artifactName} (groupId:artifactId format) against a list of dependencies.
407+
* Matches {@code artifactName} (groupId:artifactId format) against a list of dependencies,
408+
* returning the matched {@link Dependency} object (not just the version).
395409
*
396410
* <p>First tries an exact name match. If that fails, falls back to matching by artifact ID only.
397411
* The fallback handles JARs without {@code pom.properties}: {@code Dependency.guessFallbackNoPom}
398412
* can only extract the artifact ID from the filename (no group ID), producing names like {@code
399-
* "junrar"} for {@code com.github.junrar:junrar}.
413+
* "junrar"} for {@code com.github.junrar:junrar}. The returned dependency's {@code name} reflects
414+
* what {@link datadog.telemetry.dependency.DependencyService} will report for that JAR.
400415
*/
401416
@VisibleForTesting
402-
static String matchVersion(String artifactName, List<Dependency> deps) {
417+
static Dependency matchDep(String artifactName, List<Dependency> deps) {
403418
for (Dependency dep : deps) {
404419
if (artifactName.equals(dep.name)) {
405-
return dep.version;
420+
return dep;
406421
}
407422
}
408423
int colonIdx = artifactName.lastIndexOf(':');
@@ -415,14 +430,24 @@ static String matchVersion(String artifactName, List<Dependency> deps) {
415430
&& !dep.name.contains(":")
416431
&& artifactId.equals(dep.name)
417432
&& dep.version != null) {
418-
return dep.version;
433+
return dep;
419434
}
420435
}
421436
return null;
422437
}
423438

439+
/**
440+
* Matches {@code artifactName} against a list of dependencies, returning the resolved version
441+
* string. Delegates to {@link #matchDep} — use that method when the full dependency object is
442+
* needed.
443+
*/
424444
@VisibleForTesting
425-
String findArtifactVersionInClasspath(String artifactName) {
445+
static String matchVersion(String artifactName, List<Dependency> deps) {
446+
Dependency dep = matchDep(artifactName, deps);
447+
return dep != null ? dep.version : null;
448+
}
449+
450+
private Dependency findArtifactInClasspath(String artifactName) {
426451
// Use URI (not URL) to avoid DNS lookups in equals/hashCode (DMI_COLLECTION_OF_URLS)
427452
Set<URI> scanned = new HashSet<>();
428453

@@ -440,9 +465,9 @@ String findArtifactVersionInClasspath(String artifactName) {
440465
try {
441466
URI uri = new File(entry).toURI();
442467
if (scanned.add(uri)) {
443-
String version = matchVersion(artifactName, resolveDependencies(uri.toURL()));
444-
if (version != null) {
445-
return version;
468+
Dependency dep = matchDep(artifactName, resolveDependencies(uri.toURL()));
469+
if (dep != null) {
470+
return dep;
446471
}
447472
}
448473
} catch (Exception e) {
@@ -452,6 +477,12 @@ String findArtifactVersionInClasspath(String artifactName) {
452477
return null;
453478
}
454479

480+
@VisibleForTesting
481+
String findArtifactVersionInClasspath(String artifactName) {
482+
Dependency dep = findArtifactInClasspath(artifactName);
483+
return dep != null ? dep.version : null;
484+
}
485+
455486
private List<Dependency> resolveDependenciesFromCache(URL url) {
456487
try {
457488
List<Dependency> cached = jarCache.get(url.toURI());

dd-java-agent/appsec/src/test/java/com/datadog/appsec/sca/ScaReachabilityTransformerJava9Test.java

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,40 @@ void matchVersion_nullDepNameDoesNotThrow() {
238238
"com.example:foo", Collections.singletonList(nullName)));
239239
}
240240

241+
/**
242+
* Regression test for the registry key mismatch bug (PR #11614).
243+
*
244+
* <p>For JARs without {@code pom.properties}, {@code DependencyResolver.guessFallbackNoPom}
245+
* produces a dep with {@code name = "junrar"} (artifactId only, no groupId). {@code
246+
* resolveArtifactDep} must return that dep object so that callers ({@code processClass}) can use
247+
* {@code dep.name} — not {@code entry.artifact()} — when calling {@code registerCve} and building
248+
* {@code MethodCallbackSpec}. Using {@code entry.artifact()} would create a registry key ({@code
249+
* "com.github.junrar:junrar@7.5.5"}) that mismatches the key {@code DependencyService} will use
250+
* ({@code "junrar@7.5.5"}), causing the CVE telemetry to lose source/hash or appear under the
251+
* wrong name.
252+
*/
253+
@Test
254+
void resolveArtifactDep_noPomJar_returnsArtifactIdOnlyName() throws Exception {
255+
ScaCveDatabase db = ScaCveDatabase.parse(new StringReader(JACKSON_JSON));
256+
ScaReachabilityTransformer transformer = new ScaReachabilityTransformer(db, null);
257+
258+
// Simulate guessFallbackNoPom: dep.name is artifactId only (no groupId).
259+
Dependency noPomDep =
260+
new Dependency("jackson-databind", "2.9.0", "jackson-databind-2.9.0.jar", null);
261+
262+
Dependency resolved =
263+
transformer.resolveArtifactDep(
264+
"com.fasterxml.jackson.core:jackson-databind", Collections.singletonList(noPomDep));
265+
266+
assertNotNull(resolved, "should resolve via artifactId-only fallback");
267+
assertEquals(
268+
"jackson-databind",
269+
resolved.name,
270+
"resolved dep.name must be the artifactId-only name from the jar, "
271+
+ "not entry.artifact() — so registerCve uses the same key as DependencyService");
272+
assertEquals("2.9.0", resolved.version);
273+
}
274+
241275
/**
242276
* Validates that {@code findArtifactVersionInClasspath} works correctly when the context
243277
* classloader is {@code null} — which is the actual runtime condition on agent threads, because

telemetry/src/test/java/datadog/telemetry/sca/ScaReachabilityPeriodicActionTest.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,4 +569,51 @@ void cveAndDepArriveSameHeartbeat_step2MergeStillWorks() {
569569
assertEquals("lib.jar", emitted.source, "Step 2 merge must preserve source");
570570
assertTrue(emitted.reachabilityMetadata.get(0).contains("GHSA-simultaneous"));
571571
}
572+
573+
/**
574+
* Regression test for the registry key mismatch for JARs without pom.properties (PR #11614).
575+
*
576+
* <p>For JARs without {@code pom.properties}, {@code DependencyResolver.guessFallbackNoPom}
577+
* produces an artifact-ID-only name (e.g. {@code "junrar"} instead of {@code
578+
* "com.github.junrar:junrar"}). The transformer must register the CVE under the same name so that
579+
* the registry key matches what {@code DependencyService} will report.
580+
*
581+
* <p>Without the fix, {@code registerCve} was called with {@code entry.artifact()} = {@code
582+
* "com.github.junrar:junrar"}, while {@code DependencyService} reported {@code dep.name} = {@code
583+
* "junrar"}. The result: two separate telemetry entries for the same physical JAR — one with
584+
* {@code metadata:[]} (from Step 2, carrying source/hash but no CVE) and one with the CVE
585+
* metadata (from Step 3, without source/hash).
586+
*
587+
* <p>With the fix, the transformer uses the resolved {@code dep.name} from {@code matchDep} (i.e.
588+
* {@code "junrar"}) for {@code registerCve}, so the registry and {@code DependencyService} keys
589+
* match. Step 2 merges them into a single emission with both the CVE metadata and source/hash.
590+
*/
591+
@Test
592+
void noPomJar_artifactIdOnlyName_cveAndSourceHashMergedIntoSingleEntry() {
593+
// Simulates what ScaReachabilityTransformer.processClass() now does after the fix:
594+
// registerCve with the artifactId-only name that DependencyService will also report.
595+
ScaReachabilityDependencyRegistry.INSTANCE.registerCve("junrar", "7.5.5", "GHSA-hf5p-test");
596+
597+
// DependencyService returns the dep with the same artifactId-only name (guessFallbackNoPom).
598+
Dependency incoming = new Dependency("junrar", "7.5.5", "junrar-7.5.5.jar", "CAFEBABE");
599+
ScaReachabilityPeriodicAction merged = actionWithDeps(incoming);
600+
601+
merged.doIteration(telService);
602+
603+
// Must produce exactly ONE emission with BOTH the CVE metadata AND source/hash — not two
604+
// separate entries (one with CVE but no source/hash, one with source/hash but no CVE).
605+
ArgumentCaptor<Dependency> captor = ArgumentCaptor.forClass(Dependency.class);
606+
verify(telService, times(1)).addDependency(captor.capture());
607+
Dependency emitted = captor.getValue();
608+
assertEquals(
609+
"junrar", emitted.name, "name must be the artifactId-only name from DependencyService");
610+
assertEquals("7.5.5", emitted.version);
611+
assertEquals(
612+
"junrar-7.5.5.jar", emitted.source, "source/hash from DependencyService must be preserved");
613+
assertEquals("CAFEBABE", emitted.hash, "hash from DependencyService must be preserved");
614+
assertFalse(
615+
emitted.reachabilityMetadata.isEmpty(),
616+
"CVE metadata must NOT be lost — a single merged entry must carry both CVE data and source/hash");
617+
assertTrue(emitted.reachabilityMetadata.get(0).contains("GHSA-hf5p-test"));
618+
}
572619
}

0 commit comments

Comments
 (0)