feat(providers): add SHA-256 hash computation for Maven dependencies - #612
feat(providers): add SHA-256 hash computation for Maven dependencies#612a-oren wants to merge 2 commits into
Conversation
Compute SHA-256 hashes from artifact files in the local Maven repository cache (~/.m2/repository/) and include them in Maven SBOM components. - Add _buildMavenHashMap() that parses dependency tree lines to extract groupId, artifactId, packaging, version, and optional classifier, then constructs the correct .m2 file path and computes the hash - Handle packaging-to-extension mapping (bundle/eclipse-plugin → .jar) - Skip POM-only artifacts (no hash for metadata-only dependencies) - Support classified dependencies with correct file path construction - Support custom Maven repo path via TRUSTIFY_DA_MVN_REPO env var - Gracefully omit hashes when artifact files are not in the local cache - Pass hash map through parseDependencyTree() to sbom.addDependency() Implements TC-5549 Assisted-by: Claude Code
Reviewer's GuideAdds SHA-256 hash computation for Maven dependencies by reading artifacts from the local Maven repository, threads hashes through SBOM generation, and extends tests to cover hash behavior, packaging/classifier handling, and duplicate skipping. Sequence diagram for Maven dependency hash computation and SBOM integrationsequenceDiagram
participant Java_maven
participant MavenRepo as Maven_repo_fs
participant Base_Java
participant Sbom
Java_maven->>Java_maven: _buildMavenHashMap(depTreeText, opts)
loop for each dependency line
Java_maven->>MavenRepo: fs.readFileSync(artifactPath)
MavenRepo-->>Java_maven: fileContent
Java_maven->>Java_maven: crypto.createHash('sha256').update(fileContent).digest('hex')
Java_maven->>Java_maven: toPurl(groupId, artifactId, purlVersion).toString()
Java_maven->>Java_maven: hashMap.set(purl, [{alg: SHA-256, content: digest}])
end
Java_maven->>Java_maven: createSbomFileFromTextFormat(depTreeText, ignoredDeps, opts, manifestPath, hashMap)
Java_maven->>Sbom: addRoot(rootPurl, license)
Java_maven->>Base_Java: parseDependencyTree(root, 0, lines, sbom, hashMap)
loop for each parsed dependency
Base_Java->>Sbom: addDependency(from, to, undefined, hashes)
end
Sbom-->>Java_maven: getAsJsonString(opts)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_buildMavenHashMap, thecatch {}block silently swallows all filesystem errors; consider handlingENOENTseparately for missing artifacts while logging or surfacing other error types to avoid hiding unexpected issues. - The synchronous
fs.readFileSyncinside the dependency-tree loop may become a bottleneck for large Maven projects; consider switching to an async implementation or batching reads to avoid blocking the event loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_buildMavenHashMap`, the `catch {}` block silently swallows all filesystem errors; consider handling `ENOENT` separately for missing artifacts while logging or surfacing other error types to avoid hiding unexpected issues.
- The synchronous `fs.readFileSync` inside the dependency-tree loop may become a bottleneck for large Maven projects; consider switching to an async implementation or batching reads to avoid blocking the event loop.
## Individual Comments
### Comment 1
<location path="src/providers/java_maven.js" line_range="179-184" />
<code_context>
+ const trimmed = rawLine.trim()
+ if (!trimmed || trimmed.startsWith('(')) { continue }
+
+ const parts = trimmed.split(':').map(p => p ? p.match(this.DEP_REGEX)?.[0] ?? '' : '')
+ if (parts.length < 4) { continue }
+
+ const groupId = parts[0]
+ const artifactId = parts[1]
+ const packaging = parts[2]
+
+ if (packaging === 'pom') { continue }
</code_context>
<issue_to_address>
**issue:** Guard against incomplete parts when DEP_REGEX does not match segments.
When `DEP_REGEX` doesn’t match a segment, the corresponding `parts` entry is set to `''`, but later logic still treats `groupId`, `artifactId`, and `packaging` as valid. This can lead to malformed paths and unnecessary filesystem access. Add a guard that bails out when any of these fields is empty before computing or using the artifact path.
</issue_to_address>
### Comment 2
<location path="src/providers/java_maven.js" line_range="201-219" />
<code_context>
+ : `${artifactId}-${version}.${ext}`
+ const artifactPath = path.join(m2Repo, groupPath, artifactId, version, fileName)
+
+ try {
+ const fileContent = fs.readFileSync(artifactPath)
+ const digest = crypto.createHash('sha256').update(fileContent).digest('hex')
+ // Key by the PURL that parseDep() will produce for this line
+ const purlVersion = classifier ? `${version}-${classifier}` : version
+ const purl = this.toPurl(groupId, artifactId, purlVersion).toString()
+ hashMap.set(purl, [{ alg: 'SHA-256', content: digest }])
+ } catch {
+ if (process.env['TRUSTIFY_DA_DEBUG'] === 'true') {
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid recomputing hashes for duplicate artifacts appearing multiple times in the dependency tree.
When the same artifact appears multiple times in the tree, you currently re-read and hash the file for each occurrence, repeatedly overwriting the same entry in `hashMap`. Check `hashMap.has(purl)` and skip recomputing when it’s already present to avoid redundant I/O and hashing, especially for large Maven repositories.
```suggestion
const ext = Java_maven.PACKAGING_TO_JAR[packaging] || packaging
const groupPath = groupId.replaceAll('.', path.sep)
const fileName = classifier
? `${artifactId}-${version}-${classifier}.${ext}`
: `${artifactId}-${version}.${ext}`
const artifactPath = path.join(m2Repo, groupPath, artifactId, version, fileName)
// Key by the PURL that parseDep() will produce for this line
const purlVersion = classifier ? `${version}-${classifier}` : version
const purl = this.toPurl(groupId, artifactId, purlVersion).toString()
// Avoid recomputing hashes for duplicate artifacts
if (hashMap.has(purl)) {
continue
}
try {
const fileContent = fs.readFileSync(artifactPath)
const digest = crypto.createHash('sha256').update(fileContent).digest('hex')
hashMap.set(purl, [{ alg: 'SHA-256', content: digest }])
} catch {
if (process.env['TRUSTIFY_DA_DEBUG'] === 'true') {
console.error(`Maven hash: artifact not found at ${artifactPath}, omitting hash`)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #612 +/- ##
==========================================
+ Coverage 91.22% 91.32% +0.10%
==========================================
Files 42 43 +1
Lines 9175 9467 +292
Branches 1624 1693 +69
==========================================
+ Hits 8370 8646 +276
- Misses 805 821 +16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review —
|
Verification Report for TC-5549 (commit 31e1fc9)
Overall: WARNOne code change request from sourcery-ai review (guard against empty DEP_REGEX parts) resulted in sub-task TC-5581. All other checks pass. Implementation correctly adds SHA-256 hash computation for Maven dependencies with comprehensive test coverage. This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Skip dependency tree lines where groupId, artifactId, or packaging is empty after DEP_REGEX matching to prevent malformed .m2 paths. Implements TC-5581 Assisted-by: Claude Code
ruromero
left a comment
ruromero
left a comment
There was a problem hiding this comment.
Code Review — SHA-256 Hash Computation for Maven Dependencies
Overall the feature direction is solid — attaching artifact hashes to SBOM components is valuable. However, the current implementation has correctness bugs that will cause silent hash loss for classified dependencies and several design issues worth addressing before merge.
Summary of findings
Bugs (must fix):
- PURL key mismatch on conflict overrides with classifiers —
parseDepand_buildMavenHashMapconstruct different PURLs for the same classified dependency when a conflict override is present, sohashMap.get()silently returnsundefined. - Dead
startsWith('(')guard — Maven tree lines always have tree-drawing prefixes before parentheses, so this guard never fires. The test passes by accident (jar absent from fixture, not because the line is skipped). - Scope list divergence —
MAVEN_SCOPEShas 6 scopes butparseDeponly checks 3 for classifier detection, causing divergent PURL construction.
Efficiency:
4. No deduplication — same artifact can be read and hashed multiple times.
5. readFileSync loads entire files into memory — large jars can cause memory spikes.
Design:
6. Duplicated coordinate parsing will drift silently.
7. Maven-specific hashMap parameter leaks into the shared Base_java abstraction.
8. provideComponent() omits hashes while provideStack() includes them — asymmetric behavior.
See inline comments for details.
🤖 Generated with Claude Code
|
|
||
| // Handle conflict overrides the same way parseDep does | ||
| const override = rawLine.match(this.CONFLICT_REGEX) | ||
| if (override) { version = override[1] } |
There was a problem hiding this comment.
Bug: PURL key mismatch for classified dependencies with conflict overrides
When a classified dependency has a conflict override (e.g., io.netty:netty-transport:jar:linux-x86_64:4.1.0:compile - omitted for conflict with 4.2.0):
_buildMavenHashMaptracksclassifier=linux-x86_64, overridesversionto4.2.0, then constructspurlVersion=4.2.0-linux-x86_64→ PURL@4.2.0-linux-x86_64parseDep(inbase_java.js) first setsversion=4.1.0-linux-x86_64, thenCONFLICT_REGEXreplaces the entire version string with4.2.0→ PURL@4.2.0
Since @4.2.0 != @4.2.0-linux-x86_64, hashMap.get(to.toString()) returns undefined and the hash is silently lost.
Fix: Either unify the parsing into a single function (preferred — see duplication comment), or ensure the conflict-override branch in _buildMavenHashMap drops the classifier the same way parseDep does.
|
|
||
| for (const rawLine of lines) { | ||
| const trimmed = rawLine.trim() | ||
| if (!trimmed || trimmed.startsWith('(')) { continue } |
There was a problem hiding this comment.
Dead code: trimmed.startsWith('(') never matches
Maven dependency tree lines for duplicates/conflicts look like:
\- (org.slf4j:slf4j-api:jar:1.7.36:compile - omitted for duplicate)
After trim(), the line starts with \, not (. This guard never fires.
The test skips parenthesized duplicate entries passes for the wrong reason — org.slf4j:slf4j-api is not in the mock .m2 fixture, so readFileSync throws and the catch block silently skips it. If you added that jar to the fixture, the test would fail (the entry would appear in the hash map), revealing that the guard is dead.
Fix: Strip tree-drawing characters before checking for (, e.g.:
const cleaned = trimmed.replace(/^[|+\\\- ]+/, '')
if (!cleaned || cleaned.startsWith('(')) { continue }| if (packaging === 'pom') { continue } | ||
|
|
||
| let version, classifier | ||
| if (parts.length >= 6 && Java_maven.MAVEN_SCOPES.includes(parts[5])) { |
There was a problem hiding this comment.
Bug: Scope list divergence — 6 scopes here vs 3 in parseDep
MAVEN_SCOPES includes ['compile', 'provided', 'runtime', 'test', 'system', 'import'] (6 scopes), but parseDep in base_java.js:104 only checks ['compile', 'provided', 'runtime'] for classifier detection.
For a classified dependency with scope system (e.g., com.sun:tools:jar:jdk8:1.8.0:system):
_buildMavenHashMapdetects the classifier → PURL@1.8.0-jdk8parseDepdoes NOT detect it → PURL@jdk8
The PURL keys diverge and the hash lookup fails silently.
While -Dscope=compile currently filters system-scoped deps, createSbomFileFromTextFormat is public and the filter could change. Both parsers should use the same scope list.
| * @param {{}} [opts={}] Options bag (may contain TRUSTIFY_DA_MVN_REPO) | ||
| * @returns {Map<string, Array<{alg: string, content: string}>>} | ||
| */ | ||
| _buildMavenHashMap(depTreeText, opts = {}) { |
There was a problem hiding this comment.
Design: Duplicated coordinate parsing will silently drift
This method re-implements the split-by-colon, DEP_REGEX application, classifier detection, and conflict-override handling that parseDep in base_java.js:98-117 already does. The two implementations already diverge (scope lists, conflict-override classifier handling).
Any future change to parseDep that isn't mirrored here will silently break hash lookups with no test failure, because the test mocks don't cover the cross-function invariant.
Suggestion: Extract a shared parseCoordinate(rawLine) helper that both parseDep and _buildMavenHashMap call, returning {groupId, artifactId, version, classifier, packaging, scope}. This eliminates the entire class of drift bugs.
| : `${artifactId}-${version}.${ext}` | ||
| const artifactPath = path.join(m2Repo, groupPath, artifactId, version, fileName) | ||
|
|
||
| try { |
There was a problem hiding this comment.
Efficiency: No deduplication guard before file I/O
The same artifact can appear in multiple branches of the dependency tree. Combined with the dead startsWith('(') guard, parenthesized duplicate lines also get processed. Each occurrence triggers a separate fs.readFileSync + SHA-256 computation.
In a multi-module project with 5 modules sharing 50 deps, that's up to 250 redundant file reads.
Fix: Add a check before the try block:
const purl = this.toPurl(groupId, artifactId, purlVersion).toString()
if (hashMap.has(purl)) { continue }| const artifactPath = path.join(m2Repo, groupPath, artifactId, version, fileName) | ||
|
|
||
| try { | ||
| const fileContent = fs.readFileSync(artifactPath) |
There was a problem hiding this comment.
Efficiency: readFileSync loads entire file into memory
Large artifacts (e.g., aws-java-sdk-bundle ~300MB) are fully loaded into a Node.js buffer. Combined with the lack of deduplication, peak memory can spike significantly. In memory-constrained CI containers this could trigger OOM kills.
Fix: Use streaming hash:
const stream = fs.createReadStream(artifactPath)
const hash = crypto.createHash('sha256')
for await (const chunk of stream) { hash.update(chunk) }
const digest = hash.digest('hex')Note: this would make _buildMavenHashMap async.
| const purlVersion = classifier ? `${version}-${classifier}` : version | ||
| const purl = this.toPurl(groupId, artifactId, purlVersion).toString() | ||
| hashMap.set(purl, [{ alg: 'SHA-256', content: digest }]) | ||
| } catch { |
There was a problem hiding this comment.
Observability: Silent degradation with incomplete .m2 cache
The catch block swallows all errors unless TRUSTIFY_DA_DEBUG is set. In CI environments with partial caches (ephemeral containers, resolve-only phases), every readFileSync can fail silently, producing an SBOM with zero hashes and no visible signal.
Consider logging a summary warning at the end (e.g., "N of M artifacts could not be hashed") even in non-debug mode, so users have visibility into hash coverage.
| * @param {Map<string, Array<{alg: string, content: string}>>} [hashMap] - Optional PURL→hashes map | ||
| */ | ||
| parseDependencyTree(src, srcDepth, lines, sbom) { | ||
| parseDependencyTree(src, srcDepth, lines, sbom, hashMap) { |
There was a problem hiding this comment.
Design: Maven-specific parameter leaks into shared base class
The hashMap parameter is only meaningful for Maven. Gradle extends Base_java but uses its own tree parser and never passes hashMap. This leaks a Maven-specific concern into the shared abstraction.
Consider keeping hash attachment in the Maven subclass (e.g., a post-processing step that walks the SBOM components and attaches hashes) rather than threading it through the base class.
3 participants
Summary
.m2/repository/cacheTRUSTIFY_DA_MVN_REPOenv var_buildMavenHashMapto prevent malformed.m2paths (review feedback fix)Implements TC-5549
Implements TC-5581
Test plan
🤖 Generated with Claude Code